Avoid a race and a pessimization in bge_intr():

- moved the synchronizing bus read to after the bus write for the first
  interrupt ack so that it actually synchronizes everything necessary.

  We were acking not only the status update that triggered the interrupt
  together with any status updates that occurred before we got around
  to the bus write for the ack, but also any status updates that occur
  after we do the bus write but before the write reaches the device.
  The corresponding race for the second interrupt ack resulted in
  sometimes returning from the interrupt handler with acked but
  unserviced interrupt events.  Such events then remain unserviced
  until further events cause another interrupt or the watchdog times
  out.

  The race was often lost on my 5705, apparently since my 5705 has broken
  event coalescing which causes a status update for almost every packet,
  so another status update is quite likely to occur while the interrupt
  handler is running.  Watchdog timeouts weren't very noticeable,
  apparently because bge_txeof() has one of the usual bugs resetting the
  watchdog.

- don't disable device interrupts while bge_intr() is running.  Doing this
  just had the side effects of:
  - entering a device mode in which different coalescing parameters apply.
    Different coalescing parameters can be used to either inhibit or
    enhance the chance of getting another status update while in the
    interrupt handler.  This feature is useless with the current
    organization of the interrupt handler but might be useful with a
    taskqueue handler.
  - giving a race for ack+reenable/return.  This cannot be handled
    by simply rearranging the order of bus accesses like the race for
    ack+keepenable/entry.  It is necessary to sync the ack and then
    check for new events.
  - taking longer, especially with the extra code to avoid the race on
    ack+reenable/return.

Reviewed by:	ru, gleb, scottl

1 files changed, 22 insertions, 6 deletions

diff --git a/sys/dev/bge/if_bge.c b/sys/dev/bge/if_bge.c
index 5bd13cb..3363584 100644
--- a/sys/dev/bge/if_bge.c
+++ b/sys/dev/bge/if_bge.c
@@ -2978,13 +2978,32 @@ bge_intr(void *xsc)
 #endif
 
 	/*
+	 * Ack the interrupt by writing something to BGE_MBX_IRQ0_LO.  Don't
+	 * disable interrupts by writing nonzero like we used to, since with
+	 * our current organization this just gives complications and
+	 * pessimizations for re-enabling interrupts.  We used to have races
+	 * instead of the necessary complications.  Disabling interrupts
+	 * would just reduce the chance of a status update while we are
+	 * running (by switching to the interrupt-mode coalescence
+	 * parameters), but this chance is already very low so it is more
+	 * efficient to get another interrupt than prevent it.
+	 *
+	 * We do the ack first to ensure another interrupt if there is a
+	 * status update after the ack.  We don't check for the status
+	 * changing later because it is more efficient to get another
+	 * interrupt than prevent it, not quite as above (not checking is
+	 * a smaller optimization than not toggling the interrupt enable,
+	 * since checking doesn't involve PCI accesses and toggling require
+	 * the status check).  So toggling would probably be a pessimization
+	 * even with MSI.  It would only be needed for using a task queue.
+	 */
+	CSR_WRITE_4(sc, BGE_MBX_IRQ0_LO, 0);
+
+	/*
 	 * Do the mandatory PCI flush as well as get the link status.
 	 */
 	statusword = CSR_READ_4(sc, BGE_MAC_STS) & BGE_MACSTAT_LINK_CHANGED;
 
-	/* Ack interrupt and stop others from occuring. */
-	CSR_WRITE_4(sc, BGE_MBX_IRQ0_LO, 1);
-
 	/* Make sure the descriptor ring indexes are coherent. */
 	bus_dmamap_sync(sc->bge_cdata.bge_status_tag,
 	    sc->bge_cdata.bge_status_map, BUS_DMASYNC_POSTREAD);
@@ -3004,9 +3023,6 @@ bge_intr(void *xsc)
 		bge_txeof(sc);
 	}
 
-	/* Re-enable interrupts. */
-	CSR_WRITE_4(sc, BGE_MBX_IRQ0_LO, 0);
-
 	if (ifp->if_drv_flags & IFF_DRV_RUNNING &&
 	    !IFQ_DRV_IS_EMPTY(&ifp->if_snd))
 		bge_start_locked(ifp);