From 59d95190240311f6e1fc36019f7812f736decfb9 Mon Sep 17 00:00:00 2001 From: bde Date: Wed, 20 Dec 2006 11:14:45 +0000 Subject: 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 --- sys/dev/bge/if_bge.c | 28 ++++++++++++++++++++++------ 1 file changed, 22 insertions(+), 6 deletions(-) (limited to 'sys/dev') 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); -- cgit v1.1