Date: Mon, 05 Sep 2005 13:07:43 +0100 From: Gavin Atkinson <gavin.atkinson@ury.york.ac.uk> To: John Baldwin <jhb@freebsd.org> Cc: current@freebsd.org Subject: Re: [PATCH] Locking fixes for tl(4) Message-ID: <1125922064.75892.20.camel@buffy.york.ac.uk> In-Reply-To: <20050904235509.J31032@ury.york.ac.uk> References: <200508311636.49278.jhb@FreeBSD.org> <20050904234626.H31032@ury.york.ac.uk> <20050904235509.J31032@ury.york.ac.uk>
next in thread | previous in thread | raw e-mail | index | archive | help
[-- Attachment #1 --] On Mon, 2005-09-05 at 00:01 +0100, Gavin Atkinson wrote: > On Sun, 4 Sep 2005, Gavin Atkinson wrote: > > On Wed, 31 Aug 2005, John Baldwin wrote: > > > >> Patch fixes locking for tl(4) and marks it MPSAFE. Please test, thanks! > >> > >> http://www.FreeBSD.org/~jhb/patches/tl_locking.patch > > > > Doesn't work, I'm afraid. Panic on attach: > > ... because tl_hardreset() is called before sc->tl_ifp is allocated. I'm > recompiling now, having moved the if_alloc and related code to before the > first hardreset() call. The attached patch has survived pan average amount of network activity overnight. It's basically your original patch, but with a slight rearrangement in tl_attach() to move the if_alloc() call earlier. Gavin [-- Attachment #2 --] Index: src/sys/pci/if_tl.c =================================================================== RCS file: /usr/cvs/src/sys/pci/if_tl.c,v retrieving revision 1.101 diff -u -r1.101 if_tl.c --- src/sys/pci/if_tl.c 9 Aug 2005 10:20:02 -0000 1.101 +++ src/sys/pci/if_tl.c 5 Sep 2005 07:52:03 -0000 @@ -276,8 +276,10 @@ static void tl_intr(void *); static void tl_start(struct ifnet *); +static void tl_start_locked(struct ifnet *); static int tl_ioctl(struct ifnet *, u_long, caddr_t); static void tl_init(void *); +static void tl_init_locked(struct tl_softc *); static void tl_stop(struct tl_softc *); static void tl_watchdog(struct ifnet *); static void tl_shutdown(device_t); @@ -650,7 +652,8 @@ int i, ack; int minten = 0; - TL_LOCK(sc); + if (sc->tl_ifp->if_input != NULL) + TL_LOCK_ASSERT(sc); tl_mii_sync(sc); @@ -730,8 +733,6 @@ tl_dio_setbit(sc, TL_NETSIO, TL_SIO_MINTEN); } - TL_UNLOCK(sc); - if (ack) return(1); return(0); @@ -745,7 +746,8 @@ { int minten; - TL_LOCK(sc); + if (sc->tl_ifp->if_input != NULL) + TL_LOCK_ASSERT(sc); tl_mii_sync(sc); @@ -789,8 +791,6 @@ if (minten) tl_dio_setbit(sc, TL_NETSIO, TL_SIO_MINTEN); - TL_UNLOCK(sc); - return(0); } @@ -840,7 +840,8 @@ struct mii_data *mii; sc = device_get_softc(dev); - TL_LOCK(sc); + if (sc->tl_ifp->if_input != NULL) + TL_LOCK_ASSERT(sc); mii = device_get_softc(sc->tl_miibus); if ((mii->mii_media_active & IFM_GMASK) == IFM_FDX) { @@ -848,7 +849,6 @@ } else { tl_dio_clrbit(sc, TL_NETCMD, TL_CMD_DUPLEX); } - TL_UNLOCK(sc); return; } @@ -1138,7 +1138,7 @@ } mtx_init(&sc->tl_mtx, device_get_nameunit(dev), MTX_NETWORK_LOCK, - MTX_DEF | MTX_RECURSE); + MTX_DEF); /* * Map control/status registers. @@ -1223,6 +1223,13 @@ if (t->tl_vid == OLICOM_VENDORID) sc->tl_eeaddr = TL_EEPROM_EADDR_OC; + ifp = sc->tl_ifp = if_alloc(IFT_ETHER); + if (ifp == NULL) { + device_printf(dev, "can not if_alloc()\n"); + error = ENOSPC; + goto fail; + } + /* Reset the adapter. */ tl_softreset(sc, 1); tl_hardreset(dev); @@ -1234,6 +1241,7 @@ if (tl_read_eeprom(sc, eaddr, sc->tl_eeaddr, ETHER_ADDR_LEN)) { device_printf(dev, "failed to read station address\n"); error = ENXIO; + if_free(ifp); goto fail; } @@ -1259,23 +1267,16 @@ } } - ifp = sc->tl_ifp = if_alloc(IFT_ETHER); - if (ifp == NULL) { - device_printf(dev, "can not if_alloc()\n"); - error = ENOSPC; - goto fail; - } ifp->if_softc = sc; if_initname(ifp, device_get_name(dev), device_get_unit(dev)); - ifp->if_flags = IFF_BROADCAST | IFF_SIMPLEX | IFF_MULTICAST | - IFF_NEEDSGIANT; + ifp->if_flags = IFF_BROADCAST | IFF_SIMPLEX | IFF_MULTICAST; ifp->if_ioctl = tl_ioctl; ifp->if_start = tl_start; ifp->if_watchdog = tl_watchdog; ifp->if_init = tl_init; ifp->if_mtu = ETHERMTU; ifp->if_snd.ifq_maxlen = TL_TX_LIST_CNT - 1; - callout_handle_init(&sc->tl_stat_ch); + callout_init_mtx(&sc->tl_stat_callout, &sc->tl_mtx, 0); /* Reset the adapter again. */ tl_softreset(sc, 1); @@ -1310,7 +1311,7 @@ ether_ifattach(ifp, eaddr); /* Hook interrupt last to avoid having to lock softc */ - error = bus_setup_intr(dev, sc->tl_irq, INTR_TYPE_NET, + error = bus_setup_intr(dev, sc->tl_irq, INTR_TYPE_NET | INTR_MPSAFE, tl_intr, sc, &sc->tl_intrhand); if (error) { @@ -1343,12 +1344,14 @@ sc = device_get_softc(dev); KASSERT(mtx_initialized(&sc->tl_mtx), ("tl mutex not initialized")); - TL_LOCK(sc); ifp = sc->tl_ifp; /* These should only be active if attach succeeded */ if (device_is_attached(dev)) { + TL_LOCK(sc); tl_stop(sc); + TL_UNLOCK(sc); + callout_drain(&sc->tl_stat_callout); ether_ifdetach(ifp); if_free(ifp); } @@ -1368,7 +1371,6 @@ if (sc->tl_res) bus_release_resource(dev, TL_RES, TL_RID, sc->tl_res); - TL_UNLOCK(sc); mtx_destroy(&sc->tl_mtx); return(0); @@ -1444,16 +1446,10 @@ { struct mbuf *m_new = NULL; - MGETHDR(m_new, M_DONTWAIT, MT_DATA); + m_new = m_getcl(M_DONTWAIT, MT_DATA, M_PKTHDR); if (m_new == NULL) return(ENOBUFS); - MCLGET(m_new, M_DONTWAIT); - if (!(m_new->m_flags & M_EXT)) { - m_freem(m_new); - return(ENOBUFS); - } - #ifdef __alpha__ m_new->m_data += 2; #endif @@ -1691,7 +1687,7 @@ tl_softreset(sc, 1); tl_stop(sc); - tl_init(sc); + tl_init_locked(sc); CMD_SET(sc, TL_CMD_INTSON); return(0); @@ -1784,7 +1780,7 @@ } if (ifp->if_snd.ifq_head != NULL) - tl_start(ifp); + tl_start_locked(ifp); TL_UNLOCK(sc); @@ -1804,7 +1800,7 @@ bzero((char *)&tl_stats, sizeof(struct tl_stats)); sc = xsc; - TL_LOCK(sc); + TL_LOCK_ASSERT(sc); ifp = sc->tl_ifp; p = (u_int32_t *)&tl_stats; @@ -1838,15 +1834,13 @@ } } - sc->tl_stat_ch = timeout(tl_stats_update, sc, hz); + callout_reset(&sc->tl_stat_callout, hz, tl_stats_update, sc); if (!sc->tl_bitrate) { mii = device_get_softc(sc->tl_miibus); mii_tick(mii); } - TL_UNLOCK(sc); - return; } @@ -1957,12 +1951,24 @@ struct ifnet *ifp; { struct tl_softc *sc; + + sc = ifp->if_softc; + TL_LOCK(sc); + tl_start_locked(ifp); + TL_UNLOCK(sc); +} + +static void +tl_start_locked(ifp) + struct ifnet *ifp; +{ + struct tl_softc *sc; struct mbuf *m_head = NULL; u_int32_t cmd; struct tl_chain *prev = NULL, *cur_tx = NULL, *start_tx; sc = ifp->if_softc; - TL_LOCK(sc); + TL_LOCK_ASSERT(sc); /* * Check for an available queue slot. If there are none, @@ -1970,7 +1976,6 @@ */ if (sc->tl_cdata.tl_tx_free == NULL) { ifp->if_drv_flags |= IFF_DRV_OACTIVE; - TL_UNLOCK(sc); return; } @@ -2007,10 +2012,8 @@ /* * If there are no packets queued, bail. */ - if (cur_tx == NULL) { - TL_UNLOCK(sc); + if (cur_tx == NULL) return; - } /* * That's all we can stands, we can't stands no more. @@ -2040,7 +2043,6 @@ * Set a timeout in case the chip goes out to lunch. */ ifp->if_timer = 5; - TL_UNLOCK(sc); return; } @@ -2050,10 +2052,20 @@ void *xsc; { struct tl_softc *sc = xsc; + + TL_LOCK(sc); + tl_init_locked(sc); + TL_UNLOCK(sc); +} + +static void +tl_init_locked(sc) + struct tl_softc *sc; +{ struct ifnet *ifp = sc->tl_ifp; struct mii_data *mii; - TL_LOCK(sc); + TL_LOCK_ASSERT(sc); ifp = sc->tl_ifp; @@ -2098,7 +2110,6 @@ if_printf(ifp, "initialization failed: no memory for rx buffers\n"); tl_stop(sc); - TL_UNLOCK(sc); return; } @@ -2128,8 +2139,7 @@ ifp->if_drv_flags &= ~IFF_DRV_OACTIVE; /* Start the stats update counter */ - sc->tl_stat_ch = timeout(tl_stats_update, sc, hz); - TL_UNLOCK(sc); + callout_reset(&sc->tl_stat_callout, hz, tl_stats_update, sc); return; } @@ -2146,12 +2156,14 @@ sc = ifp->if_softc; + TL_LOCK(sc); if (sc->tl_bitrate) tl_setmode(sc, sc->ifmedia.ifm_media); else { mii = device_get_softc(sc->tl_miibus); mii_mediachg(mii); } + TL_UNLOCK(sc); return(0); } @@ -2169,6 +2181,7 @@ sc = ifp->if_softc; + TL_LOCK(sc); ifmr->ifm_active = IFM_ETHER; if (sc->tl_bitrate) { @@ -2187,6 +2200,7 @@ ifmr->ifm_active = mii->mii_media_active; ifmr->ifm_status = mii->mii_media_status; } + TL_UNLOCK(sc); return; } @@ -2199,12 +2213,11 @@ { struct tl_softc *sc = ifp->if_softc; struct ifreq *ifr = (struct ifreq *) data; - int s, error = 0; - - s = splimp(); + int error = 0; switch(command) { case SIOCSIFFLAGS: + TL_LOCK(sc); if (ifp->if_flags & IFF_UP) { if (ifp->if_drv_flags & IFF_DRV_RUNNING && ifp->if_flags & IFF_PROMISC && @@ -2217,18 +2230,21 @@ tl_dio_clrbit(sc, TL_NETCMD, TL_CMD_CAF); tl_setmulti(sc); } else - tl_init(sc); + tl_init_locked(sc); } else { if (ifp->if_drv_flags & IFF_DRV_RUNNING) { tl_stop(sc); } } sc->tl_if_flags = ifp->if_flags; + TL_UNLOCK(sc); error = 0; break; case SIOCADDMULTI: case SIOCDELMULTI: + TL_LOCK(sc); tl_setmulti(sc); + TL_UNLOCK(sc); error = 0; break; case SIOCSIFMEDIA: @@ -2247,8 +2263,6 @@ break; } - (void)splx(s); - return(error); } @@ -2262,10 +2276,12 @@ if_printf(ifp, "device timeout\n"); + TL_LOCK(sc); ifp->if_oerrors++; tl_softreset(sc, 1); - tl_init(sc); + tl_init_locked(sc); + TL_UNLOCK(sc); return; } @@ -2281,12 +2297,12 @@ register int i; struct ifnet *ifp; - TL_LOCK(sc); + TL_LOCK_ASSERT(sc); ifp = sc->tl_ifp; /* Stop the stats updater. */ - untimeout(tl_stats_update, sc, sc->tl_stat_ch); + callout_stop(&sc->tl_stat_callout); /* Stop the transmitter */ CMD_CLR(sc, TL_CMD_RT); @@ -2333,7 +2349,6 @@ sizeof(sc->tl_ldata->tl_tx_list)); ifp->if_drv_flags &= ~(IFF_DRV_RUNNING | IFF_DRV_OACTIVE); - TL_UNLOCK(sc); return; } @@ -2350,7 +2365,9 @@ sc = device_get_softc(dev); + TL_LOCK(sc); tl_stop(sc); + TL_UNLOCK(sc); return; } Index: src/sys/pci/if_tlreg.h =================================================================== RCS file: /usr/cvs/src/sys/pci/if_tlreg.h,v retrieving revision 1.21 diff -u -r1.21 if_tlreg.h --- src/sys/pci/if_tlreg.h 10 Jun 2005 16:49:23 -0000 1.21 +++ src/sys/pci/if_tlreg.h 5 Sep 2005 07:52:03 -0000 @@ -124,7 +124,7 @@ u_int8_t tl_txeoc; u_int8_t tl_bitrate; int tl_if_flags; - struct callout_handle tl_stat_ch; + struct callout tl_stat_callout; struct mtx tl_mtx; };
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?1125922064.75892.20.camel>