From owner-cvs-src@FreeBSD.ORG Wed Sep 21 15:41:42 2005 Return-Path: X-Original-To: cvs-src@FreeBSD.org Delivered-To: cvs-src@FreeBSD.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id DE20C16A41F; Wed, 21 Sep 2005 15:41:42 +0000 (GMT) (envelope-from ru@ip.net.ua) Received: from tigra.ip.net.ua (tigra.ip.net.ua [82.193.96.10]) by mx1.FreeBSD.org (Postfix) with ESMTP id 1758E43D45; Wed, 21 Sep 2005 15:41:41 +0000 (GMT) (envelope-from ru@ip.net.ua) Received: from localhost (rocky.ip.net.ua [82.193.96.2]) by tigra.ip.net.ua (8.12.11/8.12.11) with ESMTP id j8LFfbTC018197; Wed, 21 Sep 2005 18:41:37 +0300 (EEST) (envelope-from ru@ip.net.ua) Received: from tigra.ip.net.ua ([82.193.96.10]) by localhost (rocky.ipnet [82.193.96.2]) (amavisd-new, port 10024) with LMTP id 83782-05; Wed, 21 Sep 2005 18:41:37 +0300 (EEST) Received: from heffalump.ip.net.ua (heffalump.ip.net.ua [82.193.96.213]) by tigra.ip.net.ua (8.12.11/8.12.11) with ESMTP id j8LFfaPV018194 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Wed, 21 Sep 2005 18:41:37 +0300 (EEST) (envelope-from ru@ip.net.ua) Received: (from ru@localhost) by heffalump.ip.net.ua (8.13.3/8.13.3) id j8LFfr7M023946; Wed, 21 Sep 2005 18:41:53 +0300 (EEST) (envelope-from ru) Date: Wed, 21 Sep 2005 18:41:53 +0300 From: Ruslan Ermilov To: Robert Watson Message-ID: <20050921154153.GB22964@ip.net.ua> References: <200509190310.j8J3ALgt095979@repoman.freebsd.org> <20050919055028.GC65954@ip.net.ua> <20050919.083146.105425670.imp@bsdimp.com> <200509201551.11396.jhb@FreeBSD.org> <20050920222930.N34322@fledge.watson.org> <20050920223315.V34322@fledge.watson.org> Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="uXxzq0nDebZQVNAZ" Content-Disposition: inline In-Reply-To: <20050920223315.V34322@fledge.watson.org> User-Agent: Mutt/1.5.9i X-Virus-Scanned: by amavisd-new at ip.net.ua Cc: cvs-all@FreeBSD.org, cvs-src@FreeBSD.org, src-committers@FreeBSD.org, "M. Warner Losh" , John Baldwin Subject: Re: cvs commit: src/sys/dev/an if_an.c src/sys/dev/arl if_arl_isa.c src/sys/dev/awi if_awi_pccard.c src/sys/dev/cm if_cm_isa.c src/sys/dev/cnw if_cnw.c src/sys/dev/cp if_cp.c src/sys/dev/cs if_cs.c src/sys/dev/ed if_ed.c src/sys/dev/em if_em.c ... X-BeenThere: cvs-src@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: CVS commit messages for the src tree List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 21 Sep 2005 15:41:43 -0000 --uXxzq0nDebZQVNAZ Content-Type: multipart/mixed; boundary="24zk1gE8NUlDmwG9" Content-Disposition: inline --24zk1gE8NUlDmwG9 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Sep 20, 2005 at 10:33:43PM +0100, Robert Watson wrote: >=20 > On Tue, 20 Sep 2005, Robert Watson wrote: >=20 > > > >On Tue, 20 Sep 2005, John Baldwin wrote: > > > >>Regarding other comments I saw today on some e-mail or another, I do=20 > >>think that to make the locking sane, we might should push the checks fo= r=20 > >>IFF_DRV_RUNNING down into the foo_start() routines rather than doing it= =20 > >>in the network layer where the driver lock isn't held. > > > >This was a change I was thinking of trying to get into 6.0 a few weeks= =20 > >ago, but was worried that wholesale frobbing of the network interface=20 > >drivers would introduce too many bugs. Also, it will increase the cost= =20 > >of injecting packets into the send queue under load as you'll always=20 > >have to acquire and drop the driver mutex to test the flag. I.e., it's= =20 > >not clear we're actually racing, but we might pay a hefty cost for=20 > >fixing it. If you want to take a cut at it, I'm happy to help=20 > >characterize the cost and decide if it's the right thing to do. It=20 > >would be nice to get it into 6.0 if possible as it will become part of= =20 > >the device driver API if so. >=20 > ... getting late ... >=20 > I mean the IFF_DRV_OACTIVE flag test used in the handoff. >=20 Here's what I'd like to do, if nobody objects: 1. Finish the if_free() move after bus_teardown_intr() that Warner has started. This is needed to fix foo_intr() accessing already freed struct ifnet. 2. Fix all drivers to check IFF_DRV_RUNNING in foo_intr() and exit if it's unset. This should fix some of the shutdown panics when shared IRQs are in use. 3. Fix all drivers to check IFF_DRV_RUNNING in foo_start() and exit if it's unset. This should fix another half of shutdown panics, e.g. the one demonstrated by glebius@ in recent if_em.c commit. 4. Remove IFF_DRV_RUNNING check from ether_output(). 5. Fix all drivers to set some flag in foo_detach() and foo_shutdown() and refuse to work in foo_ioctl() if it's set. This should fix panics when BPF listener is attached while interface goes away or module is unloaded. Relevant PRs: kern/85005, kern/62889. Attached is a demo patch for rl(4) that does all of the above except #4. Cheers, --=20 Ruslan Ermilov ru@FreeBSD.org FreeBSD committer --24zk1gE8NUlDmwG9 Content-Type: text/plain; charset=us-ascii Content-Disposition: attachment; filename=p Content-Transfer-Encoding: quoted-printable Index: if_rl.c =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D RCS file: /home/ncvs/src/sys/pci/if_rl.c,v retrieving revision 1.156 diff -u -p -r1.156 if_rl.c --- if_rl.c 16 Sep 2005 11:11:51 -0000 1.156 +++ if_rl.c 21 Sep 2005 15:29:39 -0000 @@ -1015,6 +1015,7 @@ rl_detach(device_t dev) /* These should only be active if attach succeeded */ if (device_is_attached(dev)) { RL_LOCK(sc); + sc->dying =3D 1; rl_stop(sc); RL_UNLOCK(sc); ether_ifdetach(ifp); @@ -1022,8 +1023,6 @@ rl_detach(device_t dev) #if 0 sc->suspended =3D 1; #endif - if (ifp) - if_free(ifp); if (sc->rl_miibus) device_delete_child(dev, sc->rl_miibus); bus_generic_detach(dev); @@ -1035,6 +1034,9 @@ rl_detach(device_t dev) if (sc->rl_res) bus_release_resource(dev, RL_RES, RL_RID, sc->rl_res); =20 + if (ifp) + if_free(ifp); + if (sc->rl_tag) { bus_dmamap_unload(sc->rl_tag, sc->rl_cdata.rl_rx_dmamap); bus_dmamem_free(sc->rl_tag, sc->rl_cdata.rl_rx_buf, @@ -1351,6 +1353,9 @@ rl_intr(void *arg) =20 RL_LOCK(sc); =20 + if ((ifp->if_drv_flags & IFF_DRV_RUNNING) =3D=3D 0) + goto done_locked; + if (sc->suspended) goto done_locked; =20 @@ -1448,7 +1453,8 @@ rl_start(struct ifnet *ifp) struct rl_softc *sc =3D ifp->if_softc; =20 RL_LOCK(sc); - rl_start_locked(ifp); + if (ifp->if_drv_flags & IFF_DRV_RUNNING) + rl_start_locked(ifp); RL_UNLOCK(sc); } =20 @@ -1650,9 +1656,14 @@ rl_ioctl(struct ifnet *ifp, u_long comma struct rl_softc *sc =3D ifp->if_softc; int error =3D 0; =20 + RL_LOCK(sc); + if (sc->dying) { + RL_UNLOCK(sc); + return (0); /* silently discard request */ + } + switch (command) { case SIOCSIFFLAGS: - RL_LOCK(sc); if (ifp->if_flags & IFF_UP) { rl_init_locked(sc); } else { @@ -1664,21 +1675,23 @@ rl_ioctl(struct ifnet *ifp, u_long comma break; case SIOCADDMULTI: case SIOCDELMULTI: - RL_LOCK(sc); rl_setmulti(sc); RL_UNLOCK(sc); error =3D 0; break; case SIOCGIFMEDIA: case SIOCSIFMEDIA: + RL_UNLOCK(sc); mii =3D device_get_softc(sc->rl_miibus); error =3D ifmedia_ioctl(ifp, ifr, &mii->mii_media, command); break; case SIOCSIFCAP: + RL_UNLOCK(sc); ifp->if_capenable &=3D ~IFCAP_POLLING; ifp->if_capenable |=3D ifr->ifr_reqcap & IFCAP_POLLING; break; default: + RL_UNLOCK(sc); error =3D ether_ioctl(ifp, command, data); break; } @@ -1802,6 +1815,7 @@ rl_shutdown(device_t dev) sc =3D device_get_softc(dev); =20 RL_LOCK(sc); + sc->dying =3D 1; rl_stop(sc); RL_UNLOCK(sc); } Index: if_rlreg.h =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D RCS file: /home/ncvs/src/sys/pci/if_rlreg.h,v retrieving revision 1.52 diff -u -p -r1.52 if_rlreg.h --- if_rlreg.h 5 Aug 2005 08:19:12 -0000 1.52 +++ if_rlreg.h 21 Sep 2005 15:18:11 -0000 @@ -702,6 +702,7 @@ struct rl_softc { uint32_t rl_rxlenmask; int rl_testmode; int suspended; /* 0 =3D normal 1 =3D suspended */ + int dying; #ifdef DEVICE_POLLING int rxcycles; #endif --24zk1gE8NUlDmwG9-- --uXxzq0nDebZQVNAZ Content-Type: application/pgp-signature Content-Disposition: inline -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.1 (FreeBSD) iD8DBQFDMX9BqRfpzJluFF4RAlMIAKCUXSK6Gj8iIyakpdt3MR/rPh7aOQCgi3b+ PrLg7uIMdJAXyNHA4HfEoL8= =+AsU -----END PGP SIGNATURE----- --uXxzq0nDebZQVNAZ--