Date: Wed, 21 Sep 2005 18:41:53 +0300 From: Ruslan Ermilov <ru@FreeBSD.org> To: Robert Watson <rwatson@FreeBSD.org> Cc: cvs-all@FreeBSD.org, cvs-src@FreeBSD.org, src-committers@FreeBSD.org, "M. Warner Losh" <imp@bsdimp.com>, John Baldwin <jhb@FreeBSD.org> 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 ... Message-ID: <20050921154153.GB22964@ip.net.ua> In-Reply-To: <20050920223315.V34322@fledge.watson.org> 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>
next in thread | previous in thread | raw e-mail | index | archive | help
--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--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20050921154153.GB22964>