Date: Mon, 11 Oct 2010 17:39:25 +0200 From: Hans Petter Selasky <hselasky@c2i.net> To: Weongyo Jeong <weongyo@freebsd.org> Cc: "src-committers@freebsd.org" <src-committers@freebsd.org>, "svn-src-user@freebsd.org" <svn-src-user@freebsd.org> Subject: Re: svn commit: r213540 - user/weongyo/usb/sys/dev/usb/net Message-ID: <201010111739.25082.hselasky@c2i.net> In-Reply-To: <201010080152.o981q1gJ074407@svn.freebsd.org> References: <201010080152.o981q1gJ074407@svn.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Friday 08 October 2010 03:52:01 Weongyo Jeong wrote: > Author: weongyo > Date: Fri Oct 8 01:52:01 2010 > New Revision: 213540 > URL: http://svn.freebsd.org/changeset/base/213540 > > Log: > o fixes a regression that setting the promiscuous mode should be > happened at the taskqueue. It's to avoid a `sleepable after > non-sleepable' because ioctl handler could be called with holding bpf > mtx which is a default mutex. > o defines SLEEPOUT_DRAIN_TASK helper. > > Modified: > user/weongyo/usb/sys/dev/usb/net/if_aue.c > user/weongyo/usb/sys/dev/usb/net/if_auereg.h > user/weongyo/usb/sys/dev/usb/net/if_axe.c > user/weongyo/usb/sys/dev/usb/net/if_axereg.h > user/weongyo/usb/sys/dev/usb/net/if_cue.c > user/weongyo/usb/sys/dev/usb/net/if_cuereg.h > user/weongyo/usb/sys/dev/usb/net/if_kue.c > user/weongyo/usb/sys/dev/usb/net/if_rue.c > user/weongyo/usb/sys/dev/usb/net/if_ruereg.h > user/weongyo/usb/sys/dev/usb/net/if_udav.c > user/weongyo/usb/sys/dev/usb/net/if_udavreg.h > > Modified: user/weongyo/usb/sys/dev/usb/net/if_aue.c > =========================================================================== > === --- user/weongyo/usb/sys/dev/usb/net/if_aue.c Fri Oct 8 01:47:14 > 2010 (r213539) +++ user/weongyo/usb/sys/dev/usb/net/if_aue.c Fri Oct 8 > 01:52:01 2010 (r213540) @@ -230,7 +230,8 @@ static > void aue_setmulti_locked(struct a > static void aue_rxflush(struct aue_softc *); > static int aue_rxbuf(struct aue_softc *, struct usb_page_cache *, > unsigned int, unsigned int); > -static void aue_setpromisc(struct aue_softc *); > +static void aue_setpromisc(void *, int); > +static void aue_setpromisc_locked(struct aue_softc *); > static void aue_init_locked(struct aue_softc *); > static void aue_watchdog(void *); > > @@ -709,6 +710,7 @@ aue_attach(device_t dev) > sleepout_create(&sc->sc_sleepout, "aue sleepout"); > sleepout_init_mtx(&sc->sc_sleepout, &sc->sc_watchdog, &sc->sc_mtx, 0); > TASK_INIT(&sc->sc_setmulti, 0, aue_setmulti, sc); > + TASK_INIT(&sc->sc_setpromisc, 0, aue_setpromisc, sc); > > iface_index = AUE_IFACE_IDX; > error = usbd_transfer_setup(uaa->device, &iface_index, > @@ -764,7 +766,8 @@ aue_detach(device_t dev) > struct ifnet *ifp = sc->sc_ifp; > > sleepout_drain(&sc->sc_watchdog); > - taskqueue_drain(sc->sc_sleepout.s_taskqueue, &sc->sc_setmulti); > + SLEEPOUT_DRAIN_TASK(&sc->sc_sleepout, &sc->sc_setpromisc); > + SLEEPOUT_DRAIN_TASK(&sc->sc_sleepout, &sc->sc_setmulti); > usbd_transfer_unsetup(sc->sc_xfer, AUE_N_TRANSFER); > > if (sc->sc_miibus != NULL) > @@ -1032,7 +1035,7 @@ aue_init_locked(struct aue_softc *sc) > aue_csr_write_1(sc, AUE_PAR0 + i, IF_LLADDR(ifp)[i]); > > /* update promiscuous setting */ > - aue_setpromisc(sc); > + aue_setpromisc_locked(sc); > > /* Load the multicast filter. */ > aue_setmulti_locked(sc); > @@ -1050,7 +1053,17 @@ aue_init_locked(struct aue_softc *sc) > } > > static void > -aue_setpromisc(struct aue_softc *sc) > +aue_setpromisc(void *arg, int npending) > +{ > + struct aue_softc *sc = arg; > + > + AUE_LOCK(sc); > + aue_setpromisc_locked(sc); > + AUE_UNLOCK(sc); > +} > + > +static void > +aue_setpromisc_locked(struct aue_softc *sc) > { > struct ifnet *ifp = sc->sc_ifp; > > @@ -1140,7 +1153,8 @@ aue_ioctl(struct ifnet *ifp, u_long comm > AUE_LOCK(sc); > if (ifp->if_flags & IFF_UP) { > if (ifp->if_drv_flags & IFF_DRV_RUNNING) > - aue_setpromisc(sc); > + SLEEPOUT_RUN_TASK(&sc->sc_sleepout, > + &sc->sc_setpromisc); > else > aue_init_locked(sc); > } else > > Modified: user/weongyo/usb/sys/dev/usb/net/if_auereg.h > =========================================================================== > === --- user/weongyo/usb/sys/dev/usb/net/if_auereg.h Fri Oct 8 01:47:14 > 2010 (r213539) +++ user/weongyo/usb/sys/dev/usb/net/if_auereg.h Fri Oct 8 > 01:52:01 2010 (r213540) @@ -222,6 +222,7 @@ struct aue_softc { > struct sleepout sc_sleepout; > struct sleepout_task sc_watchdog; > struct task sc_setmulti; > + struct task sc_setpromisc; > }; > > #define AUE_LOCK(_sc) mtx_lock(&(_sc)->sc_mtx) > > Modified: user/weongyo/usb/sys/dev/usb/net/if_axe.c > =========================================================================== > === --- user/weongyo/usb/sys/dev/usb/net/if_axe.c Fri Oct 8 01:47:14 > 2010 (r213539) +++ user/weongyo/usb/sys/dev/usb/net/if_axe.c Fri Oct 8 > 01:52:01 2010 (r213540) @@ -206,6 +206,8 @@ static > void axe_start_locked(struct ifne > static void axe_tick(struct axe_softc *); > static void axe_stop(struct axe_softc *); > static void axe_setmulti_locked(struct axe_softc *); > +static void axe_setpromisc(void *, int); > +static void axe_setpromisc_locked(struct axe_softc *); > > static const struct usb_config axe_config[AXE_N_TRANSFER] = { > [AXE_BULK_DT_WR] = { > @@ -787,6 +789,7 @@ axe_attach(device_t dev) > sleepout_create(&sc->sc_sleepout, "axe sleepout"); > sleepout_init_mtx(&sc->sc_sleepout, &sc->sc_watchdog, &sc->sc_mtx, 0); > TASK_INIT(&sc->sc_setmulti, 0, axe_setmulti, sc); > + TASK_INIT(&sc->sc_setpromisc, 0, axe_setpromisc, sc); > > iface_index = AXE_IFACE_IDX; > error = usbd_transfer_setup(uaa->device, &iface_index, sc->sc_xfer, > @@ -846,7 +849,8 @@ axe_detach(device_t dev) > struct ifnet *ifp = sc->sc_ifp; > > sleepout_drain(&sc->sc_watchdog); > - taskqueue_drain(sc->sc_sleepout.s_taskqueue, &sc->sc_setmulti); > + SLEEPOUT_DRAIN_TASK(&sc->sc_sleepout, &sc->sc_setpromisc); > + SLEEPOUT_DRAIN_TASK(&sc->sc_sleepout, &sc->sc_setmulti); > usbd_transfer_unsetup(sc->sc_xfer, AXE_N_TRANSFER); > > if (sc->sc_miibus != NULL) > @@ -1232,7 +1236,17 @@ axe_init_locked(struct axe_softc *sc) > } > > static void > -axe_setpromisc(struct axe_softc *sc) > +axe_setpromisc(void *arg, int npending) > +{ > + struct axe_softc *sc = arg; > + > + AXE_LOCK(sc); > + axe_setpromisc_locked(sc); > + AXE_UNLOCK(sc); > +} > + > +static void > +axe_setpromisc_locked(struct axe_softc *sc) > { > struct ifnet *ifp = sc->sc_ifp; > uint16_t rxmode; > @@ -1286,7 +1300,8 @@ axe_ioctl(struct ifnet *ifp, u_long comm > AXE_LOCK(sc); > if (ifp->if_flags & IFF_UP) { > if (ifp->if_drv_flags & IFF_DRV_RUNNING) > - axe_setpromisc(sc); > + SLEEPOUT_RUN_TASK(&sc->sc_sleepout, > + &sc->sc_setpromisc); > else > axe_init_locked(sc); > } else > > Modified: user/weongyo/usb/sys/dev/usb/net/if_axereg.h > =========================================================================== > === --- user/weongyo/usb/sys/dev/usb/net/if_axereg.h Fri Oct 8 01:47:14 > 2010 (r213539) +++ user/weongyo/usb/sys/dev/usb/net/if_axereg.h Fri Oct 8 > 01:52:01 2010 (r213540) @@ -210,6 +210,7 @@ struct axe_softc { > struct sleepout sc_sleepout; > struct sleepout_task sc_watchdog; > struct task sc_setmulti; > + struct task sc_setpromisc; > > int sc_phyno; > int sc_flags; > > Modified: user/weongyo/usb/sys/dev/usb/net/if_cue.c > =========================================================================== > === --- user/weongyo/usb/sys/dev/usb/net/if_cue.c Fri Oct 8 01:47:14 > 2010 (r213539) +++ user/weongyo/usb/sys/dev/usb/net/if_cue.c Fri Oct 8 > 01:52:01 2010 (r213540) @@ -131,6 +131,8 @@ static int cue_rxbuf(struct > cue_softc *, > static void cue_rxflush(struct cue_softc *); > static void cue_stop(struct cue_softc *); > static void cue_watchdog(void *); > +static void cue_setpromisc(void *, int); > +static void cue_setpromisc_locked(struct cue_softc *); > > #ifdef USB_DEBUG > static int cue_debug = 0; > @@ -286,7 +288,17 @@ cue_mchash(const uint8_t *addr) > } > > static void > -cue_setpromisc(struct cue_softc *sc) > +cue_setpromisc(void *arg, int npending) > +{ > + struct cue_softc *sc = arg; > + > + CUE_LOCK(sc); > + cue_setpromisc_locked(sc); > + CUE_UNLOCK(sc); > +} > + > +static void > +cue_setpromisc_locked(struct cue_softc *sc) > { > struct ifnet *ifp = sc->sc_ifp; > > @@ -406,6 +418,7 @@ cue_attach(device_t dev) > mtx_init(&sc->sc_mtx, device_get_nameunit(dev), NULL, MTX_DEF); > sleepout_create(&sc->sc_sleepout, "cue sleepout"); > sleepout_init_mtx(&sc->sc_sleepout, &sc->sc_watchdog, &sc->sc_mtx, 0); > + TASK_INIT(&sc->sc_setpromisc, 0, cue_setpromisc, sc); > TASK_INIT(&sc->sc_setmulti, 0, cue_setmulti, sc); > > iface_index = CUE_IFACE_IDX; > @@ -454,7 +467,8 @@ cue_detach(device_t dev) > struct ifnet *ifp = sc->sc_ifp; > > sleepout_drain(&sc->sc_watchdog); > - taskqueue_drain(sc->sc_sleepout.s_taskqueue, &sc->sc_setmulti); > + SLEEPOUT_DRAIN_TASK(&sc->sc_sleepout, &sc->sc_setpromisc); > + SLEEPOUT_DRAIN_TASK(&sc->sc_sleepout, &sc->sc_setmulti); > usbd_transfer_unsetup(sc->sc_xfer, CUE_N_TRANSFER); > if (ifp != NULL) { > CUE_LOCK(sc); > @@ -643,7 +657,7 @@ cue_init_locked(struct cue_softc *sc) > cue_csr_write_1(sc, CUE_ETHCTL, CUE_ETHCTL_RX_ON | CUE_ETHCTL_MCAST_ON); > > /* Load the multicast filter */ > - cue_setpromisc(sc); > + cue_setpromisc_locked(sc); > > /* > * Set the number of RX and TX buffers that we want > @@ -766,7 +780,8 @@ cue_ioctl(struct ifnet *ifp, u_long comm > CUE_LOCK(sc); > if (ifp->if_flags & IFF_UP) { > if (ifp->if_drv_flags & IFF_DRV_RUNNING) > - cue_setpromisc(sc); > + SLEEPOUT_RUN_TASK(&sc->sc_sleepout, > + &sc->sc_setpromisc); > else > cue_init_locked(sc); > } else > > Modified: user/weongyo/usb/sys/dev/usb/net/if_cuereg.h > =========================================================================== > === --- user/weongyo/usb/sys/dev/usb/net/if_cuereg.h Fri Oct 8 01:47:14 > 2010 (r213539) +++ user/weongyo/usb/sys/dev/usb/net/if_cuereg.h Fri Oct 8 > 01:52:01 2010 (r213540) @@ -126,6 +126,7 @@ struct cue_softc { > struct usb_xfer *sc_xfer[CUE_N_TRANSFER]; > struct sleepout sc_sleepout; > struct sleepout_task sc_watchdog; > + struct task sc_setpromisc; > struct task sc_setmulti; > struct ifqueue sc_rxq; > /* ethernet address from eeprom */ > > Modified: user/weongyo/usb/sys/dev/usb/net/if_kue.c > =========================================================================== > === --- user/weongyo/usb/sys/dev/usb/net/if_kue.c Fri Oct 8 01:47:14 > 2010 (r213539) +++ user/weongyo/usb/sys/dev/usb/net/if_kue.c Fri Oct 8 > 01:52:01 2010 (r213540) @@ -527,7 +527,7 @@ kue_detach(device_t dev) > struct kue_softc *sc = device_get_softc(dev); > struct ifnet *ifp = sc->sc_ifp; > > - taskqueue_drain(sc->sc_sleepout.s_taskqueue, &sc->sc_setmulti); > + SLEEPOUT_DRAIN_TASK(&sc->sc_sleepout, &sc->sc_setmulti); > usbd_transfer_unsetup(sc->sc_xfer, KUE_N_TRANSFER); > if (ifp != NULL) { > KUE_LOCK(sc); > > Modified: user/weongyo/usb/sys/dev/usb/net/if_rue.c > =========================================================================== > === --- user/weongyo/usb/sys/dev/usb/net/if_rue.c Fri Oct 8 01:47:14 > 2010 (r213539) +++ user/weongyo/usb/sys/dev/usb/net/if_rue.c Fri Oct 8 > 01:52:01 2010 (r213540) @@ -160,6 +160,8 @@ static void rue_start(struct > ifnet *); > static void rue_start_locked(struct ifnet *); > static void rue_stop(struct rue_softc *); > static void rue_watchdog(void *); > +static void rue_setpromisc(void *, int); > +static void rue_setpromisc_locked(struct rue_softc *); > > static const struct usb_config rue_config[RUE_N_TRANSFER] = { > [RUE_BULK_DT_WR] = { > @@ -451,7 +453,17 @@ rue_miibus_statchg(device_t dev) > } > > static void > -rue_setpromisc(struct rue_softc *sc) > +rue_setpromisc(void *arg, int npending) > +{ > + struct rue_softc *sc = arg; > + > + RUE_LOCK(sc); > + rue_setpromisc_locked(sc); > + RUE_UNLOCK(sc); > +} > + > +static void > +rue_setpromisc_locked(struct rue_softc *sc) > { > struct ifnet *ifp = sc->sc_ifp; > > @@ -588,6 +600,7 @@ rue_attach(device_t dev) > mtx_init(&sc->sc_mtx, device_get_nameunit(dev), NULL, MTX_DEF); > sleepout_create(&sc->sc_sleepout, "rue sleepout"); > sleepout_init_mtx(&sc->sc_sleepout, &sc->sc_watchdog, &sc->sc_mtx, 0); > + TASK_INIT(&sc->sc_setpromisc, 0, rue_setpromisc, sc); > TASK_INIT(&sc->sc_setmulti, 0, rue_setmulti, sc); > > iface_index = RUE_IFACE_IDX; > @@ -644,7 +657,8 @@ rue_detach(device_t dev) > struct ifnet *ifp = sc->sc_ifp; > > sleepout_drain(&sc->sc_watchdog); > - taskqueue_drain(sc->sc_sleepout.s_taskqueue, &sc->sc_setmulti); > + SLEEPOUT_DRAIN_TASK(&sc->sc_sleepout, &sc->sc_setpromisc); > + SLEEPOUT_DRAIN_TASK(&sc->sc_sleepout, &sc->sc_setmulti); > usbd_transfer_unsetup(sc->sc_xfer, RUE_N_TRANSFER); > if (sc->sc_miibus != NULL) > device_delete_child(sc->sc_dev, sc->sc_miibus); > @@ -957,7 +971,7 @@ rue_init_locked(struct rue_softc *sc) > rue_csr_write_2(sc, RUE_RCR, RUE_RCR_CONFIG|RUE_RCR_AB); > > /* Load the multicast filter */ > - rue_setpromisc(sc); > + rue_setpromisc_locked(sc); > /* Load the multicast filter. */ > rue_setmulti(sc, 0); > > @@ -1044,7 +1058,8 @@ rue_ioctl(struct ifnet *ifp, u_long comm > RUE_LOCK(sc); > if (ifp->if_flags & IFF_UP) { > if (ifp->if_drv_flags & IFF_DRV_RUNNING) > - rue_setpromisc(sc); > + SLEEPOUT_RUN_TASK(&sc->sc_sleepout, > + &sc->sc_setpromisc); > else > rue_init_locked(sc); > } else > > Modified: user/weongyo/usb/sys/dev/usb/net/if_ruereg.h > =========================================================================== > === --- user/weongyo/usb/sys/dev/usb/net/if_ruereg.h Fri Oct 8 01:47:14 > 2010 (r213539) +++ user/weongyo/usb/sys/dev/usb/net/if_ruereg.h Fri Oct 8 > 01:52:01 2010 (r213540) @@ -178,6 +178,7 @@ struct rue_softc { > struct usb_xfer *sc_xfer[RUE_N_TRANSFER]; > struct sleepout sc_sleepout; > struct sleepout_task sc_watchdog; > + struct task sc_setpromisc; > struct task sc_setmulti; > struct ifqueue sc_rxq; > /* ethernet address from eeprom */ > > Modified: user/weongyo/usb/sys/dev/usb/net/if_udav.c > =========================================================================== > === --- user/weongyo/usb/sys/dev/usb/net/if_udav.c Fri Oct 8 01:47:14 > 2010 (r213539) +++ user/weongyo/usb/sys/dev/usb/net/if_udav.c Fri Oct 8 > 01:52:01 2010 (r213540) @@ -116,7 +116,8 @@ static void udav_start(struct > ifnet *); > static void udav_start_locked(struct ifnet *); > static void udav_setmulti(void *, int); > static void udav_stop(struct udav_softc *); > -static void udav_setpromisc(struct udav_softc *); > +static void udav_setpromisc(void *, int); > +static void udav_setpromisc_locked(struct udav_softc *); > static void udav_watchdog(void *); > > static miibus_readreg_t udav_miibus_readreg; > @@ -250,6 +251,7 @@ udav_attach(device_t dev) > sleepout_create(&sc->sc_sleepout, "axe sleepout"); > sleepout_init_mtx(&sc->sc_sleepout, &sc->sc_watchdog, &sc->sc_mtx, 0); > TASK_INIT(&sc->sc_setmulti, 0, udav_setmulti, sc); > + TASK_INIT(&sc->sc_setpromisc, 0, udav_setpromisc, sc); > > iface_index = UDAV_IFACE_INDEX; > error = usbd_transfer_setup(uaa->device, &iface_index, > @@ -304,7 +306,8 @@ udav_detach(device_t dev) > struct ifnet *ifp = sc->sc_ifp; > > sleepout_drain(&sc->sc_watchdog); > - taskqueue_drain(sc->sc_sleepout.s_taskqueue, &sc->sc_setmulti); > + SLEEPOUT_DRAIN_TASK(&sc->sc_sleepout, &sc->sc_setpromisc); > + SLEEPOUT_DRAIN_TASK(&sc->sc_sleepout, &sc->sc_setmulti); > usbd_transfer_unsetup(sc->sc_xfer, UDAV_N_TRANSFER); > > if (sc->sc_miibus != NULL) > @@ -469,7 +472,7 @@ udav_init_locked(struct udav_softc *sc) > UDAV_SETBIT(sc, UDAV_RCR, UDAV_RCR_DIS_LONG | UDAV_RCR_DIS_CRC); > > /* load multicast filter and update promiscious mode bit */ > - udav_setpromisc(sc); > + udav_setpromisc_locked(sc); > > /* enable RX */ > UDAV_SETBIT(sc, UDAV_RCR, UDAV_RCR_RXEN); > @@ -557,7 +560,17 @@ udav_setmulti(void *arg, int npending) > } > > static void > -udav_setpromisc(struct udav_softc *sc) > +udav_setpromisc(void *arg, int npending) > +{ > + struct udav_softc *sc = arg; > + > + UDAV_LOCK(sc); > + udav_setpromisc_locked(sc); > + UDAV_UNLOCK(sc); > +} > + > +static void > +udav_setpromisc_locked(struct udav_softc *sc) > { > struct ifnet *ifp = sc->sc_ifp; > uint8_t rxmode; > @@ -982,7 +995,8 @@ udav_ioctl(struct ifnet *ifp, u_long com > UDAV_LOCK(sc); > if (ifp->if_flags & IFF_UP) { > if (ifp->if_drv_flags & IFF_DRV_RUNNING) > - udav_setpromisc(sc); > + SLEEPOUT_RUN_TASK(&sc->sc_sleepout, > + &sc->sc_setpromisc); > else > udav_init_locked(sc); > } else > > Modified: user/weongyo/usb/sys/dev/usb/net/if_udavreg.h > =========================================================================== > === --- user/weongyo/usb/sys/dev/usb/net/if_udavreg.h Fri Oct 8 01:47:14 > 2010 (r213539) +++ user/weongyo/usb/sys/dev/usb/net/if_udavreg.h Fri Oct > 8 01:52:01 2010 (r213540) @@ -161,6 +161,7 @@ struct udav_softc { > struct sleepout sc_sleepout; > struct sleepout_task sc_watchdog; > struct task sc_setmulti; > + struct task sc_setpromisc; > struct ifqueue sc_rxq; > /* ethernet address from eeprom */ > uint8_t sc_eaddr[ETHER_ADDR_LEN]; These taskqueues belong in the network stack and not the USB drivers! And please understand that you cannot use taskqueues for these commands, because the events can be executed out of order!!! --HPS
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201010111739.25082.hselasky>