Date: Wed, 20 May 2009 08:26:02 -0400 From: John Baldwin <jhb@freebsd.org> To: Weongyo Jeong <weongyo@freebsd.org> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r192419 - head/sys/dev/usb/wlan Message-ID: <200905200826.03063.jhb@freebsd.org> In-Reply-To: <200905200349.n4K3nHSe018257@svn.freebsd.org> References: <200905200349.n4K3nHSe018257@svn.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Tuesday 19 May 2009 11:49:17 pm Weongyo Jeong wrote: > Author: weongyo > Date: Wed May 20 03:49:16 2009 > New Revision: 192419 > URL: http://svn.freebsd.org/changeset/base/192419 > > Log: > try to unsetup USB xfers before calling ieee80211_ifdetach() to fix a > bug referencing a destroyed lock within TX callbacks during device > detach. > > Submitted by: hps (original version) > Tested by: Lucius Windschuh <lwindschuh at googlemail.com> > > Modified: > head/sys/dev/usb/wlan/if_uath.c > head/sys/dev/usb/wlan/if_upgt.c > > Modified: head/sys/dev/usb/wlan/if_uath.c > ============================================================================== > --- head/sys/dev/usb/wlan/if_uath.c Wed May 20 03:33:27 2009 (r192418) > +++ head/sys/dev/usb/wlan/if_uath.c Wed May 20 03:49:16 2009 (r192419) > @@ -517,12 +517,12 @@ uath_detach(device_t dev) > > sc->sc_flags |= UATH_FLAG_INVALID; > uath_stop(ifp); > - ieee80211_ifdetach(ic); > > callout_drain(&sc->stat_ch); > callout_drain(&sc->watchdog_ch); > > usb2_transfer_unsetup(sc->sc_xfer, UATH_N_XFERS); > + ieee80211_ifdetach(ic); > > /* free buffers */ > UATH_LOCK(sc); This sequence looks very wrong. You should not be stopping the interface or draining anything until after you have detached the interface. Otherwise you have a race condition where a user 'ifconfig up' request may be processed after usb2_transfer_unsetup(). The proper order of operations should be something like: bpfdetach() ieee80211_ifdetach() UATH_LOCK(); uath_stop(); // calls callout_stop, clears IFF_DRV_RUNNING UATH_UNLOCK(); callout_drain(); usb2_transfer_unsetup(); Note that one thing this requires you to do is explicitly check in various callbacks to see if IFF_DRV_RUNNING is not set at the start of the callback (or immediately after acquiring your driver's lock if you drop it (e.g. to call if_input()). This is not needed for callout routines if you use callout_init_mtx() as in that case the callout code can effectively do that check for you as a result of the callout_stop(). However, any other callbacks from a framework that do not use the driver's lock directly will need to explicitly check IFF_DRV_RUNNING, and it sounds like that may be the correct fix here for your USB TX callbacks (either that or you need to somehow not destroy the lock until after usb_transfer_unsetup() has returned). -- John Baldwin
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200905200826.03063.jhb>