From cb2c8bdf97c24d447cc1fbf95b2241a3307e99e2 Mon Sep 17 00:00:00 2001 From: bms Date: Mon, 5 Jul 2004 02:51:32 +0000 Subject: Locking cleanup for rl(4). - Eliminate the use of a recursive mutex. - Mark the driver INTR_MPSAFE. This work is incomplete and will be refined in a future commit. - Most notably, _locked() variants of entry points need to be introduced. - The mii upcall/downcall may still be racy. - Add a stubbed-out guard against racing rl_detach() for the time being. Tested on: UP, debug.mpsafenet && !debug.mpsafenet Reviewed by: silence on -net --- sys/pci/if_rl.c | 112 ++++++++++++++++++++++++++++++-------------------------- 1 file changed, 60 insertions(+), 52 deletions(-) (limited to 'sys/pci') diff --git a/sys/pci/if_rl.c b/sys/pci/if_rl.c index 658f6db..fbcfdf1 100644 --- a/sys/pci/if_rl.c +++ b/sys/pci/if_rl.c @@ -542,12 +542,10 @@ rl_miibus_readreg(device_t dev, int phy, int reg) uint16_t rl8139_reg = 0; sc = device_get_softc(dev); - RL_LOCK(sc); if (sc->rl_type == RL_8139) { /* Pretend the internal PHY is only at address 0 */ if (phy) { - RL_UNLOCK(sc); return (0); } switch (reg) { @@ -568,7 +566,6 @@ rl_miibus_readreg(device_t dev, int phy, int reg) break; case MII_PHYIDR1: case MII_PHYIDR2: - RL_UNLOCK(sc); return (0); /* * Allow the rlphy driver to read the media status @@ -578,15 +575,12 @@ rl_miibus_readreg(device_t dev, int phy, int reg) */ case RL_MEDIASTAT: rval = CSR_READ_1(sc, RL_MEDIASTAT); - RL_UNLOCK(sc); return (rval); default: if_printf(&sc->arpcom.ac_if, "bad phy register\n"); - RL_UNLOCK(sc); return (0); } rval = CSR_READ_2(sc, rl8139_reg); - RL_UNLOCK(sc); return (rval); } @@ -595,8 +589,6 @@ rl_miibus_readreg(device_t dev, int phy, int reg) frame.mii_regaddr = reg; rl_mii_readreg(sc, &frame); - RL_UNLOCK(sc); - return (frame.mii_data); } @@ -608,12 +600,10 @@ rl_miibus_writereg(device_t dev, int phy, int reg, int data) uint16_t rl8139_reg = 0; sc = device_get_softc(dev); - RL_LOCK(sc); if (sc->rl_type == RL_8139) { /* Pretend the internal PHY is only at address 0 */ if (phy) { - RL_UNLOCK(sc); return (0); } switch (reg) { @@ -634,16 +624,13 @@ rl_miibus_writereg(device_t dev, int phy, int reg, int data) break; case MII_PHYIDR1: case MII_PHYIDR2: - RL_UNLOCK(sc); return (0); break; default: if_printf(&sc->arpcom.ac_if, "bad phy register\n"); - RL_UNLOCK(sc); return (0); } CSR_WRITE_2(sc, rl8139_reg, data); - RL_UNLOCK(sc); return (0); } @@ -653,8 +640,6 @@ rl_miibus_writereg(device_t dev, int phy, int reg, int data) frame.mii_data = data; rl_mii_writereg(sc, &frame); - RL_UNLOCK(sc); - return (0); } @@ -676,6 +661,8 @@ rl_setmulti(struct rl_softc *sc) uint32_t rxfilt; int mcnt = 0; + RL_LOCK_ASSERT(sc); + rxfilt = CSR_READ_4(sc, RL_RXCFG); if (ifp->if_flags & IFF_ALLMULTI || ifp->if_flags & IFF_PROMISC) { @@ -718,6 +705,8 @@ rl_reset(struct rl_softc *sc) { register int i; + RL_LOCK_ASSERT(sc); + CSR_WRITE_1(sc, RL_COMMAND, RL_CMD_RESET); for (i = 0; i < RL_TIMEOUT; i++) { @@ -761,17 +750,9 @@ rl_probe(device_t dev) sc->rl_btag = rman_get_bustag(sc->rl_res); sc->rl_bhandle = rman_get_bushandle(sc->rl_res); - mtx_init(&sc->rl_mtx, - device_get_nameunit(dev), - MTX_NETWORK_LOCK, MTX_DEF); - RL_LOCK(sc); - hwrev = CSR_READ_4(sc, RL_TXCFG) & RL_TXCFG_HWREV; bus_release_resource(dev, RL_RES, RL_RID, sc->rl_res); - RL_UNLOCK(sc); - mtx_destroy(&sc->rl_mtx); - /* Don't attach to 8139C+ or 8169/8110 chips. */ if (hwrev == RL_HWREV_8139CPLUS || (hwrev == RL_HWREV_8169 && @@ -811,7 +792,7 @@ rl_attach(device_t dev) unit = device_get_unit(dev); mtx_init(&sc->rl_mtx, device_get_nameunit(dev), MTX_NETWORK_LOCK, - MTX_DEF | MTX_RECURSE); + MTX_DEF); pci_enable_busmaster(dev); @@ -851,8 +832,14 @@ rl_attach(device_t dev) goto fail; } - /* Reset the adapter. */ + /* + * Reset the adapter. Only take the lock here as it's needed in + * order to call rl_reset(). + */ + RL_LOCK(sc); rl_reset(sc); + RL_UNLOCK(sc); + sc->rl_eecmd_read = RL_EECMD_READ_6BIT; rl_read_eeprom(sc, (uint8_t *)&rl_did, 0, 1, 0); if (rl_did != 0x8129) @@ -980,13 +967,11 @@ rl_attach(device_t dev) ether_ifattach(ifp, eaddr); /* Hook interrupt last to avoid having to lock softc */ - error = bus_setup_intr(dev, sc->rl_irq, INTR_TYPE_NET, + error = bus_setup_intr(dev, sc->rl_irq, INTR_TYPE_NET | INTR_MPSAFE, rl_intr, sc, &sc->rl_intrhand); - if (error) { if_printf(ifp, "couldn't set up irq\n"); ether_ifdetach(ifp); - goto fail; } fail: @@ -1014,6 +999,9 @@ rl_detach(device_t dev) KASSERT(mtx_initialized(&sc->rl_mtx), ("rl mutex not initialized")); RL_LOCK(sc); +#if 0 + sc->suspended = 1; +#endif /* These should only be active if attach succeeded */ if (device_is_attached(dev)) { @@ -1055,6 +1043,8 @@ rl_list_tx_init(struct rl_softc *sc) struct rl_chain_data *cd; int i; + RL_LOCK_ASSERT(sc); + cd = &sc->rl_cdata; for (i = 0; i < RL_TX_LIST_CNT; i++) { cd->rl_tx_chain[i] = NULL; @@ -1213,6 +1203,8 @@ rl_txeof(struct rl_softc *sc) struct ifnet *ifp = &sc->arpcom.ac_if; uint32_t txstat; + RL_LOCK_ASSERT(sc); + /* * Go through our tx list and free mbufs for those * frames that have been uploaded. @@ -1298,8 +1290,11 @@ rl_poll (struct ifnet *ifp, enum poll_cmd cmd, int count) rl_rxeof(sc); rl_txeof(sc); - if (!IFQ_DRV_IS_EMPTY(&ifp->if_snd)) + if (!IFQ_DRV_IS_EMPTY(&ifp->if_snd)) { + RL_UNLOCK(sc); rl_start(ifp); + RL_LOCK(sc); + } if (cmd == POLL_AND_CHECK_STATUS) { uint16_t status; @@ -1315,7 +1310,9 @@ rl_poll (struct ifnet *ifp, enum poll_cmd cmd, int count) if (status & RL_ISR_SYSTEM_ERR) { rl_reset(sc); + RL_UNLOCK(sc); rl_init(sc); + RL_LOCK(sc); } } done: @@ -1327,24 +1324,28 @@ static void rl_intr(void *arg) { struct rl_softc *sc = arg; - struct ifnet *ifp; + struct ifnet *ifp = &sc->arpcom.ac_if; uint16_t status; - if (sc->suspended) - return; - RL_LOCK(sc); - ifp = &sc->arpcom.ac_if; + + if (sc->suspended) { + RL_UNLOCK(sc); + return; + } #ifdef DEVICE_POLLING - if (ifp->if_flags & IFF_POLLING) - goto done; + if (ifp->if_flags & IFF_POLLING) { + RL_UNLOCK(sc); + return; + } if ((ifp->if_capenable & IFCAP_POLLING) && ether_poll_register(rl_poll, ifp)) { /* Disable interrupts. */ CSR_WRITE_2(sc, RL_IMR, 0x0000); + RL_UNLOCK(sc); rl_poll(ifp, 0, 1); - goto done; + return; } #endif /* DEVICE_POLLING */ @@ -1365,17 +1366,16 @@ rl_intr(void *arg) rl_txeof(sc); if (status & RL_ISR_SYSTEM_ERR) { rl_reset(sc); + RL_UNLOCK(sc); rl_init(sc); + RL_LOCK(sc); } } + RL_UNLOCK(sc); + if (!IFQ_DRV_IS_EMPTY(&ifp->if_snd)) rl_start(ifp); - -#ifdef DEVICE_POLLING -done: -#endif - RL_UNLOCK(sc); } /* @@ -1387,6 +1387,8 @@ rl_encap(struct rl_softc *sc, struct mbuf *m_head) { struct mbuf *m_new = NULL; + RL_LOCK_ASSERT(sc); + /* * The RealTek is brain damaged and wants longword-aligned * TX buffers, plus we can only have one fragment buffer @@ -1615,21 +1617,23 @@ rl_ioctl(struct ifnet *ifp, u_long command, caddr_t data) struct rl_softc *sc = ifp->if_softc; int error = 0; - RL_LOCK(sc); - switch (command) { case SIOCSIFFLAGS: if (ifp->if_flags & IFF_UP) { rl_init(sc); } else { if (ifp->if_flags & IFF_RUNNING) + RL_LOCK(sc); rl_stop(sc); + RL_UNLOCK(sc); } error = 0; break; case SIOCADDMULTI: case SIOCDELMULTI: + RL_LOCK(sc); rl_setmulti(sc); + RL_UNLOCK(sc); error = 0; break; case SIOCGIFMEDIA: @@ -1646,8 +1650,6 @@ rl_ioctl(struct ifnet *ifp, u_long command, caddr_t data) break; } - RL_UNLOCK(sc); - return (error); } @@ -1663,9 +1665,10 @@ rl_watchdog(struct ifnet *ifp) rl_txeof(sc); rl_rxeof(sc); - rl_init(sc); RL_UNLOCK(sc); + + rl_init(sc); } /* @@ -1678,10 +1681,9 @@ rl_stop(struct rl_softc *sc) register int i; struct ifnet *ifp = &sc->arpcom.ac_if; - ifp->if_timer = 0; - - RL_LOCK(sc); + RL_LOCK_ASSERT(sc); + ifp->if_timer = 0; untimeout(rl_tick, sc, sc->rl_stat_ch); ifp->if_flags &= ~(IFF_RUNNING | IFF_OACTIVE); #ifdef DEVICE_POLLING @@ -1707,8 +1709,6 @@ rl_stop(struct rl_softc *sc) 0x0000000); } } - - RL_UNLOCK(sc); } /* @@ -1722,8 +1722,11 @@ rl_suspend(device_t dev) struct rl_softc *sc; sc = device_get_softc(dev); + + RL_LOCK(sc); rl_stop(sc); sc->suspended = 1; + RL_UNLOCK(sc); return (0); } @@ -1746,7 +1749,9 @@ rl_resume(device_t dev) if (ifp->if_flags & IFF_UP) rl_init(sc); + RL_LOCK(sc); sc->suspended = 0; + RL_UNLOCK(sc); return (0); } @@ -1761,5 +1766,8 @@ rl_shutdown(device_t dev) struct rl_softc *sc; sc = device_get_softc(dev); + + RL_LOCK(sc); rl_stop(sc); + RL_UNLOCK(sc); } -- cgit v1.1