From 997452064e282fb87d60f8af2639bb470958a20d Mon Sep 17 00:00:00 2001 From: John Baldwin Date: Thu, 18 Aug 2005 19:24:30 +0000 Subject: [PATCH] Various fixups to locking: - Remove a lot of superfluous locking during attach. There is no need to lock access to the driver until some other thread has a way of getting to it. For ethernet drivers the other ways include registering an interrupt handler via bus_setup_intr(), calling ether_ifattach() to hook into the network stack, and kicking off a callout-driven timer via callout_reset(). - Use callout_* rather than timeout/untimeout. - Break out of xl_rxeof() if IFF_DRV_RUNNING is clear after ifp->if_input returns to handle the case where the interface was stopped while we were passing a packet up the stack. Don't call xl_rxeof() in xl_rxeof_task() unless IFF_DRV_RUNNING is set. With these fixes in place, any outstanding task will gracefully terminate as soon as it gets a chance to run after the interface has been stopped via xl_stop(). As a result, taskqueue_drain() is no longer required in xl_stop(). The task is still drained in detach() however to make sure that detach() can safely destroy the driver mutex at the end of the function. - Lock the driver lock in the ifmedia callouts and don't lock across ifmedia_ioctl() in xl_ioctl(). Note: glebius came up with most of (3) as well independently. I took a rather roundabout way of arriving at the same conclusion. MFC after: 3 days --- sys/pci/if_xl.c | 80 +++++++++++++++++++--------------------------- sys/pci/if_xlreg.h | 2 +- 2 files changed, 33 insertions(+), 49 deletions(-) diff --git a/sys/pci/if_xl.c b/sys/pci/if_xl.c index 78d100de5d37..b4f1f86e87bf 100644 --- a/sys/pci/if_xl.c +++ b/sys/pci/if_xl.c @@ -458,8 +458,6 @@ xl_mii_readreg(struct xl_softc *sc, struct xl_mii_frame *frame) { int i, ack; - /*XL_LOCK_ASSERT(sc);*/ - /* Set up frame for RX. */ frame->mii_stdelim = XL_MII_STARTDELIM; frame->mii_opcode = XL_MII_READOP; @@ -528,8 +526,6 @@ static int xl_mii_writereg(struct xl_softc *sc, struct xl_mii_frame *frame) { - /*XL_LOCK_ASSERT(sc);*/ - /* Set up frame for TX. */ frame->mii_stdelim = XL_MII_STARTDELIM; frame->mii_opcode = XL_MII_WRITEOP; @@ -617,8 +613,6 @@ xl_miibus_statchg(device_t dev) sc = device_get_softc(dev); mii = device_get_softc(sc->xl_miibus); - /*XL_LOCK_ASSERT(sc);*/ - xl_setcfg(sc); /* Set ASIC's duplex mode to match the PHY. */ @@ -651,8 +645,6 @@ xl_miibus_mediainit(device_t dev) mii = device_get_softc(sc->xl_miibus); ifm = &mii->mii_media; - /*XL_LOCK_ASSERT(sc);*/ - if (sc->xl_media & (XL_MEDIAOPT_AUI | XL_MEDIAOPT_10FL)) { /* * Check for a 10baseFL board in disguise. @@ -716,8 +708,6 @@ xl_read_eeprom(struct xl_softc *sc, caddr_t dest, int off, int cnt, int swap) int err = 0, i; u_int16_t word = 0, *ptr; - XL_LOCK_ASSERT(sc); - #define EEPROM_5BIT_OFFSET(A) ((((A) << 2) & 0x7F00) | ((A) & 0x003F)) #define EEPROM_8BIT_OFFSET(A) ((A) & 0x003F) /* @@ -904,7 +894,7 @@ xl_setmode(struct xl_softc *sc, int media) u_int16_t mediastat; char *pmsg = "", *dmsg = ""; - /*XL_LOCK_ASSERT(sc);*/ + XL_LOCK_ASSERT(sc); XL_SEL_WIN(4); mediastat = CSR_READ_2(sc, XL_W4_MEDIA_STATUS); @@ -1091,8 +1081,6 @@ static void xl_mediacheck(struct xl_softc *sc) { - XL_LOCK_ASSERT(sc); - /* * If some of the media options bits are set, assume they are * correct. If not, try to figure it out down below. @@ -1364,10 +1352,10 @@ xl_attach(device_t dev) ifp->if_softc = sc; if_initname(ifp, device_get_name(dev), device_get_unit(dev)); - XL_LOCK(sc); - /* Reset the adapter. */ + XL_LOCK(sc); xl_reset(sc); + XL_UNLOCK(sc); /* * Get station address from the EEPROM. @@ -1375,14 +1363,11 @@ xl_attach(device_t dev) if (xl_read_eeprom(sc, (caddr_t)&eaddr, XL_EE_OEM_ADR0, 3, 1)) { device_printf(dev, "failed to read station address\n"); error = ENXIO; - XL_UNLOCK(sc); goto fail; } - XL_UNLOCK(sc); - sc->xl_unit = unit; - callout_handle_init(&sc->xl_stat_ch); + callout_init_mtx(&sc->xl_stat_callout, &sc->xl_mtx, 0); TASK_INIT(&sc->xl_task, 0, xl_rxeof_task, sc); /* @@ -1473,8 +1458,6 @@ xl_attach(device_t dev) if (error) goto fail; - XL_LOCK(sc); - /* * Figure out the card type. 3c905B adapters have the * 'supportsNoTxLength' bit set in the capabilities @@ -1533,9 +1516,6 @@ xl_attach(device_t dev) xl_mediacheck(sc); - /* XXX Downcalls to ifmedia, miibus about to happen. */ - XL_UNLOCK(sc); - if (sc->xl_media & XL_MEDIAOPT_MII || sc->xl_media & XL_MEDIAOPT_BTX || sc->xl_media & XL_MEDIAOPT_BT4) { @@ -1556,12 +1536,8 @@ xl_attach(device_t dev) * a 10/100 card of some kind, we need to force the transceiver * type to something sane. */ - if (sc->xl_xcvr == XL_XCVR_AUTO) { - /* XXX Direct hardware access needs lock coverage. */ - XL_LOCK(sc); + if (sc->xl_xcvr == XL_XCVR_AUTO) xl_choose_xcvr(sc, bootverbose); - XL_UNLOCK(sc); - } /* * Do ifmedia setup. @@ -1610,7 +1586,6 @@ xl_attach(device_t dev) ifmedia_add(&sc->ifmedia, IFM_ETHER|IFM_100_FX, 0, NULL); } - /* XXX: Unlocked, leaf will take lock. */ media = IFM_ETHER|IFM_100_TX|IFM_FDX; xl_choose_media(sc, &media); @@ -1618,7 +1593,6 @@ xl_attach(device_t dev) ifmedia_set(&sc->ifmedia, media); done: - /* XXX: Unlocked hardware access, narrow race. */ if (sc->xl_flags & XL_FLAG_NO_XCVR_PWR) { XL_SEL_WIN(0); CSR_WRITE_2(sc, XL_W0_MFG_ID, XL_NO_XCVR_PWR_MAGICBITS); @@ -1648,7 +1622,8 @@ xl_attach(device_t dev) /* * Choose a default media. * XXX This is a leaf function only called by xl_attach() and - * acquires/releases the non-recursible driver mutex. + * acquires/releases the non-recursible driver mutex to + * satisfy lock assertions. */ static void xl_choose_media(struct xl_softc *sc, int *media) @@ -1715,7 +1690,6 @@ xl_detach(device_t dev) ifp = sc->xl_ifp; KASSERT(mtx_initialized(&sc->xl_mtx), ("xl mutex not initialized")); - XL_LOCK(sc); if (sc->xl_flags & XL_FLAG_USE_MMIO) { rid = XL_PCI_LOMEM; @@ -1727,8 +1701,12 @@ xl_detach(device_t dev) /* These should only be active if attach succeeded */ if (device_is_attached(dev)) { + XL_LOCK(sc); xl_reset(sc); xl_stop(sc); + XL_UNLOCK(sc); + taskqueue_drain(taskqueue_swi, &sc->xl_task); + callout_drain(&sc->xl_stat_callout); ether_ifdetach(ifp); if_free(ifp); } @@ -1766,7 +1744,6 @@ xl_detach(device_t dev) bus_dma_tag_destroy(sc->xl_ldata.xl_tx_tag); } - XL_UNLOCK(sc); mtx_destroy(&sc->xl_mtx); return (0); @@ -2076,6 +2053,14 @@ xl_rxeof(struct xl_softc *sc) XL_UNLOCK(sc); (*ifp->if_input)(ifp, m); XL_LOCK(sc); + + /* + * If we are running from the taskqueue, the interface + * might have been stopped while we were passing the last + * packet up the network stack. + */ + if (!(ifp->if_drv_flags & IFF_DRV_RUNNING)) + return; } /* @@ -2111,7 +2096,8 @@ xl_rxeof_task(void *arg, int pending) NET_LOCK_GIANT(); XL_LOCK(sc); - xl_rxeof(sc); + if (sc->xl_ifp->if_drv_flags & IFF_DRV_RUNNING) + xl_rxeof(sc); XL_UNLOCK(sc); NET_UNLOCK_GIANT(); } @@ -2441,9 +2427,8 @@ xl_stats_update(void *xsc) { struct xl_softc *sc = xsc; - XL_LOCK(sc); + XL_LOCK_ASSERT(sc); xl_stats_update_locked(sc); - XL_UNLOCK(sc); } static void @@ -2490,7 +2475,7 @@ xl_stats_update_locked(struct xl_softc *sc) XL_SEL_WIN(7); if (!sc->xl_stats_no_timeout) - sc->xl_stat_ch = timeout(xl_stats_update, sc, hz); + callout_reset(&sc->xl_stat_callout, hz, xl_stats_update, sc); } /* @@ -3032,7 +3017,7 @@ xl_init_locked(struct xl_softc *sc) ifp->if_drv_flags |= IFF_DRV_RUNNING; ifp->if_drv_flags &= ~IFF_DRV_OACTIVE; - sc->xl_stat_ch = timeout(xl_stats_update, sc, hz); + callout_reset(&sc->xl_stat_callout, hz, xl_stats_update, sc); } /* @@ -3045,7 +3030,7 @@ xl_ifmedia_upd(struct ifnet *ifp) struct ifmedia *ifm = NULL; struct mii_data *mii = NULL; - /*XL_LOCK_ASSERT(sc);*/ + XL_LOCK(sc); if (sc->xl_miibus != NULL) mii = device_get_softc(sc->xl_miibus); @@ -3069,11 +3054,13 @@ xl_ifmedia_upd(struct ifnet *ifp) if (sc->xl_media & XL_MEDIAOPT_MII || sc->xl_media & XL_MEDIAOPT_BTX || sc->xl_media & XL_MEDIAOPT_BT4) { - xl_init(sc); /* XXX */ + xl_init_locked(sc); } else { xl_setmode(sc, ifm->ifm_media); } + XL_UNLOCK(sc); + return (0); } @@ -3088,7 +3075,7 @@ xl_ifmedia_sts(struct ifnet *ifp, struct ifmediareq *ifmr) u_int16_t status = 0; struct mii_data *mii = NULL; - /*XL_LOCK_ASSERT(sc);*/ + XL_LOCK(sc); if (sc->xl_miibus != NULL) mii = device_get_softc(sc->xl_miibus); @@ -3148,6 +3135,8 @@ xl_ifmedia_sts(struct ifnet *ifp, struct ifmediareq *ifmr) if_printf(ifp, "unknown XCVR type: %d\n", icfg); break; } + + XL_UNLOCK(sc); } static int @@ -3205,8 +3194,6 @@ xl_ioctl(struct ifnet *ifp, u_long command, caddr_t data) break; case SIOCGIFMEDIA: case SIOCSIFMEDIA: - /* XXX Downcall from ifmedia possibly with locks held. */ - /*XL_LOCK(sc);*/ if (sc->xl_miibus != NULL) mii = device_get_softc(sc->xl_miibus); if (mii == NULL) @@ -3215,7 +3202,6 @@ xl_ioctl(struct ifnet *ifp, u_long command, caddr_t data) else error = ifmedia_ioctl(ifp, ifr, &mii->mii_media, command); - /*XL_UNLOCK(sc);*/ break; case SIOCSIFCAP: XL_LOCK(sc); @@ -3286,8 +3272,6 @@ xl_stop(struct xl_softc *sc) ether_poll_deregister(ifp); #endif /* DEVICE_POLLING */ - taskqueue_drain(taskqueue_swi, &sc->xl_task); - CSR_WRITE_2(sc, XL_COMMAND, XL_CMD_RX_DISABLE); CSR_WRITE_2(sc, XL_COMMAND, XL_CMD_STATS_DISABLE); CSR_WRITE_2(sc, XL_COMMAND, XL_CMD_INTR_ENB); @@ -3311,7 +3295,7 @@ xl_stop(struct xl_softc *sc) bus_space_write_4(sc->xl_ftag, sc->xl_fhandle, 4, 0x8000); /* Stop the stats updater. */ - untimeout(xl_stats_update, sc, sc->xl_stat_ch); + callout_stop(&sc->xl_stat_callout); /* * Free data in the RX lists. diff --git a/sys/pci/if_xlreg.h b/sys/pci/if_xlreg.h index d1d2e8ffd187..f999b71fa74c 100644 --- a/sys/pci/if_xlreg.h +++ b/sys/pci/if_xlreg.h @@ -601,7 +601,7 @@ struct xl_softc { int xl_if_flags; struct xl_list_data xl_ldata; struct xl_chain_data xl_cdata; - struct callout_handle xl_stat_ch; + struct callout xl_stat_callout; int xl_flags; struct resource *xl_fres; bus_space_handle_t xl_fhandle;