Minor filter tweaks:

o If we don't have a filter, also check to make sure the card is there before
  calling the scheduled ISR.  This is necessary to help old drivers whose
  ISRs can't cope with being called with the hardware missing, which sadly
  still exist in the tree.  This is the main reason why we have an extra
  layer of indirection for cardbus interrupts.
o If the card is no longer present, mark the interrupt as 'handled' rather
  than 'stray' because this accounts for why the interrupt happened.  Stray
  isn't all bad, since there are other filters that would claim it...
o Fix some comments
  + Add comment about why we check for CARD_OK and touch the hardware in both
    the filter and ISR.
  + add a note about why we don't care about Giant
  + also note that giant can't be taken out in a filter...
  + Some minor formatting nits on very long comments.

1 files changed, 47 insertions, 26 deletions

diff --git a/sys/dev/pccbb/pccbb.c b/sys/dev/pccbb/pccbb.c
index 78f8704..778614e 100644
--- a/sys/dev/pccbb/pccbb.c
+++ b/sys/dev/pccbb/pccbb.c
@@ -369,12 +369,6 @@ cbb_setup_intr(device_t dev, device_t child, struct resource *irq,
 
 	if (filt == NULL && intr == NULL)
 		return (EINVAL);
-	/*
-	 * Well, this is no longer strictly true.  You can have multiple
-	 * FAST ISRs, but can't mix fast and slow, so we have to assume
-	 * least common denominator until the base system supports mixing
-	 * and matching better.
-	 */
 	ih = malloc(sizeof(struct cbb_intrhand), M_DEVBUF, M_NOWAIT);
 	if (ih == NULL)
 		return (ENOMEM);
@@ -595,25 +589,24 @@ cbb_removal(struct cbb_softc *sc)
 /************************************************************************/
 
 /*
- * Since we touch hardware in the worst case, we don't need to use atomic
- * ops on the CARD_OK tests.  They would save us a trip to the hardware
- * if CARD_OK was recently cleared and the caches haven't updated yet.
- * However, an atomic op costs between 100-200 CPU cycles.  On a 3GHz
- * machine, this is about 33-66ns, whereas a trip the the hardware
- * is about that.  On slower machines, the cost is even higher, so the
- * trip to the hardware is cheaper and achieves the same ends that
- * a fully locked operation would give us.
+ * Since we touch hardware in the worst case, we don't need to use atomic ops
+ * on the CARD_OK tests.  They would save us a trip to the hardware if CARD_OK
+ * was recently cleared and the caches haven't updated yet.  However, an
+ * atomic op costs between 100-200 CPU cycles.  On a 3GHz machine, this is
+ * about 33-66ns, whereas a trip the the hardware is about that.  On slower
+ * machines, the cost is even higher, so the trip to the hardware is cheaper
+ * and achieves the same ends that a fully locked operation would give us.
  *
- * This is a separate routine because we'd have to use locking and/or
- * other synchronization in cbb_intr to do this there.  That would be
- * even more expensive.
+ * This is a separate routine because we'd have to use locking and/or other
+ * synchronization in cbb_intr to do this there.  That would be even more
+ * expensive.
  *
- * I need to investigate what this means for a SMP machine with multiple
- * CPUs servicing the ISR when an eject happens.  In the case of a dirty
- * eject, CD glitches and we might read 'card present' from the hardware
- * due to this jitter.  If we assumed that cbb_intr() ran before
- * cbb_func_intr(), we could just check the SOCKET_MASK register and if
- * CD changes were clear there, then we'd know the card was gone.
+ * I need to investigate what this means for a SMP machine with multiple CPUs
+ * servicing the ISR when an eject happens.  In the case of a dirty eject, CD
+ * glitches and we might read 'card present' from the hardware due to this
+ * jitter.  If we assumed that cbb_intr() ran before cbb_func_intr(), we could
+ * just check the SOCKET_MASK register and if CD changes were clear there,
+ * then we'd know the card was gone.
  */
 static int
 cbb_func_filt(void *arg)
@@ -628,12 +621,12 @@ cbb_func_filt(void *arg)
 		return (FILTER_STRAY);
 	if (!CBB_CARD_PRESENT(cbb_get(sc, CBB_SOCKET_STATE))) {
 		sc->flags &= ~CBB_CARD_OK;
-		return (FILTER_STRAY);
+		return (FILTER_HANDLED);
 	}
 
 	/*
-	 * nb: don't have to check for giant or not, since that's done
-	 * in the ISR dispatch
+	 * nb: don't have to check for giant or not, since that's done in the
+	 * ISR dispatch and one can't hold Giant in a filter anyway...
 	 */
 	if (ih->filt != NULL)
 		return ((*ih->filt)(ih->arg));	
@@ -644,7 +637,35 @@ static void
 cbb_func_intr(void *arg)
 {
 	struct cbb_intrhand *ih = (struct cbb_intrhand *)arg;
+	struct cbb_softc *sc = ih->sc;
 
+	/*
+	 * While this check may seem redundant, it helps close a race
+	 * condition.  If the card is ejected after the filter runs, but
+	 * before this ISR can be scheduled, then we need to do the same
+	 * filtering to prevent the card's ISR from being called.  One could
+	 * argue that the card's ISR should be able to cope, but experience
+	 * has shown they can't always.  This mitigates the problem by making
+	 * the race quite a bit smaller.  Properly written client ISRs should
+	 * cope with the card going away in the middle of the ISR.  We assume
+	 * that drivers that are sophisticated enough to use filters don't
+	 * need our protection.  This also allows us to ensure they *ARE*
+	 * called if their filter said they needed to be called.
+	 */
+	if (ih->filt == NULL) {
+		if ((sc->flags & CBB_CARD_OK) == 0)
+			return;
+		if (!CBB_CARD_PRESENT(cbb_get(sc, CBB_SOCKET_STATE))) {
+			sc->flags &= ~CBB_CARD_OK;
+			return;
+		}
+	}
+
+	/*
+	 * Call the registered ithread interrupt handler.  This entire routine
+	 * will be called with Giant if this isn't an MP safe driver, or not
+	 * if it is.  Either way, we don't have to worry.
+	 */
 	ih->intr(ih->arg);
 }