no feedback yet. anyone care to comment on this? On Thu, Dec 16, 2010 at 12:28:56AM +0000, Jacob Meuser wrote: > the following diff tries to make sure that no other processes/threads > are or will be using the drivers software context when the driver is > detached. > > this diff covers rum(4), run(4), ural(4) and urtw(4). without the > diff, I can get the kernel to crash by starting a scan with > the deice, then ejecting it while the scan is running. with this > diff, I cannot get a crash. > > normally, usb device detach happens in the usb_task thread. the sole > exception is when the usb bus itself is being detached. this happens > with cardbus devices, and in that case, the usb device detach happens > in the generic workq thread. > > this adds a simple reference counting/waiting mechanism. this is > definitely needed for ioctl(2), which can sleep then start using the > driver again. it may not be needed for timeout(9)s now, but maybe > someday it will. another possible process using the device is the > usb_task thread. in the normal case, this is the same process that > is detaching, so there is no concurrency issue. there is already > usb_rem_wait_task() to deal with detaches from other processes, > because the bus driver needs it. > > this also adds more uses of usbd_is_dying() to check if the device > has been detached: > * before adding timeouts > * when entering timeouts, usb_tasks, and ioctl > > also of note is the order things are done in the detach routines: > 1) remove pending timeouts > 2) remove (and possibly wait for) pending usb_tasks > 3) wait for other processes > 4) detach from the network stack > 5) cleanup/free usb resources > > thoughts? ok? > > -- > jake...@sdf.lonestar.org > SDF Public Access UNIX System - http://sdf.lonestar.org > > > Index: if_ral.c > =================================================================== > RCS file: /cvs/src/sys/dev/usb/if_ral.c,v > retrieving revision 1.117 > diff -u -p -r1.117 if_ral.c > --- if_ral.c 6 Dec 2010 04:41:39 -0000 1.117 > +++ if_ral.c 15 Dec 2010 02:58:35 -0000 > @@ -363,17 +363,20 @@ ural_detach(struct device *self, int fla > > s = splusb(); > > - if (ifp->if_softc != NULL) { > - ieee80211_ifdetach(ifp); /* free all nodes */ > - if_detach(ifp); > - } > - > - usb_rem_task(sc->sc_udev, &sc->sc_task); > if (timeout_initialized(&sc->scan_to)) > timeout_del(&sc->scan_to); > if (timeout_initialized(&sc->amrr_to)) > timeout_del(&sc->amrr_to); > > + usb_rem_wait_task(sc->sc_udev, &sc->sc_task); > + > + usbd_ref_wait(sc->sc_udev); > + > + if (ifp->if_softc != NULL) { > + ieee80211_ifdetach(ifp); /* free all nodes */ > + if_detach(ifp); > + } > + > if (sc->amrr_xfer != NULL) { > usbd_free_xfer(sc->amrr_xfer); > sc->amrr_xfer = NULL; > @@ -547,8 +550,15 @@ ural_next_scan(void *arg) > struct ieee80211com *ic = &sc->sc_ic; > struct ifnet *ifp = &ic->ic_if; > > + if (usbd_is_dying(sc->sc_udev)) > + return; > + > + usbd_ref_incr(sc->sc_udev); > + > if (ic->ic_state == IEEE80211_S_SCAN) > ieee80211_next_scan(ifp); > + > + usbd_ref_decr(sc->sc_udev); > } > > void > @@ -559,6 +569,9 @@ ural_task(void *arg) > enum ieee80211_state ostate; > struct ieee80211_node *ni; > > + if (usbd_is_dying(sc->sc_udev)) > + return; > + > ostate = ic->ic_state; > > switch (sc->sc_state) { > @@ -574,7 +587,8 @@ ural_task(void *arg) > > case IEEE80211_S_SCAN: > ural_set_chan(sc, ic->ic_bss->ni_chan); > - timeout_add_msec(&sc->scan_to, 200); > + if (!usbd_is_dying(sc->sc_udev)) > + timeout_add_msec(&sc->scan_to, 200); > break; > > case IEEE80211_S_AUTH: > @@ -1324,6 +1338,11 @@ ural_ioctl(struct ifnet *ifp, u_long cmd > struct ifreq *ifr; > int s, error = 0; > > + if (usbd_is_dying(sc->sc_udev)) > + return ENXIO; > + > + usbd_ref_incr(sc->sc_udev); > + > s = splnet(); > > switch (cmd) { > @@ -1387,6 +1406,8 @@ ural_ioctl(struct ifnet *ifp, u_long cmd > > splx(s); > > + usbd_ref_decr(sc->sc_udev); > + > return error; > } > > @@ -2147,7 +2168,8 @@ ural_amrr_start(struct ural_softc *sc, s > i--); > ni->ni_txrate = i; > > - timeout_add_sec(&sc->amrr_to, 1); > + if (!usbd_is_dying(sc->sc_udev)) > + timeout_add_sec(&sc->amrr_to, 1); > } > > void > @@ -2157,6 +2179,11 @@ ural_amrr_timeout(void *arg) > usb_device_request_t req; > int s; > > + if (usbd_is_dying(sc->sc_udev)) > + return; > + > + usbd_ref_incr(sc->sc_udev); > + > s = splusb(); > > /* > @@ -2174,6 +2201,8 @@ ural_amrr_timeout(void *arg) > (void)usbd_transfer(sc->amrr_xfer); > > splx(s); > + > + usbd_ref_decr(sc->sc_udev); > } > > void > @@ -2203,7 +2232,8 @@ ural_amrr_update(usbd_xfer_handle xfer, > > ieee80211_amrr_choose(&sc->amrr, sc->sc_ic.ic_bss, &sc->amn); > > - timeout_add_sec(&sc->amrr_to, 1); > + if (!usbd_is_dying(sc->sc_udev)) > + timeout_add_sec(&sc->amrr_to, 1); > } > > int > Index: if_rum.c > =================================================================== > RCS file: /cvs/src/sys/dev/usb/if_rum.c,v > retrieving revision 1.94 > diff -u -p -r1.94 if_rum.c > --- if_rum.c 6 Dec 2010 04:41:39 -0000 1.94 > +++ if_rum.c 15 Dec 2010 02:58:35 -0000 > @@ -460,17 +460,20 @@ rum_detach(struct device *self, int flag > > s = splusb(); > > - if (ifp->if_softc != NULL) { > - ieee80211_ifdetach(ifp); /* free all nodes */ > - if_detach(ifp); > - } > - > - usb_rem_task(sc->sc_udev, &sc->sc_task); > if (timeout_initialized(&sc->scan_to)) > timeout_del(&sc->scan_to); > if (timeout_initialized(&sc->amrr_to)) > timeout_del(&sc->amrr_to); > > + usb_rem_wait_task(sc->sc_udev, &sc->sc_task); > + > + usbd_ref_wait(sc->sc_udev); > + > + if (ifp->if_softc != NULL) { > + ieee80211_ifdetach(ifp); /* free all nodes */ > + if_detach(ifp); > + } > + > if (sc->amrr_xfer != NULL) { > usbd_free_xfer(sc->amrr_xfer); > sc->amrr_xfer = NULL; > @@ -647,8 +650,12 @@ rum_next_scan(void *arg) > if (usbd_is_dying(sc->sc_udev)) > return; > > + usbd_ref_incr(sc->sc_udev); > + > if (ic->ic_state == IEEE80211_S_SCAN) > ieee80211_next_scan(ifp); > + > + usbd_ref_decr(sc->sc_udev); > } > > void > @@ -676,7 +683,8 @@ rum_task(void *arg) > > case IEEE80211_S_SCAN: > rum_set_chan(sc, ic->ic_bss->ni_chan); > - timeout_add_msec(&sc->scan_to, 200); > + if (!usbd_is_dying(sc->sc_udev)) > + timeout_add_msec(&sc->scan_to, 200); > break; > > case IEEE80211_S_AUTH: > @@ -1350,6 +1358,11 @@ rum_ioctl(struct ifnet *ifp, u_long cmd, > struct ifreq *ifr; > int s, error = 0; > > + if (usbd_is_dying(sc->sc_udev)) > + return ENXIO; > + > + usbd_ref_incr(sc->sc_udev); > + > s = splnet(); > > switch (cmd) { > @@ -1413,6 +1426,8 @@ rum_ioctl(struct ifnet *ifp, u_long cmd, > > splx(s); > > + usbd_ref_decr(sc->sc_udev); > + > return error; > } > > @@ -2236,7 +2251,8 @@ rum_amrr_start(struct rum_softc *sc, str > i--); > ni->ni_txrate = i; > > - timeout_add_sec(&sc->amrr_to, 1); > + if (!usbd_is_dying(sc->sc_udev)) > + timeout_add_sec(&sc->amrr_to, 1); > } > > void > @@ -2290,7 +2306,8 @@ rum_amrr_update(usbd_xfer_handle xfer, u > > ieee80211_amrr_choose(&sc->amrr, sc->sc_ic.ic_bss, &sc->amn); > > - timeout_add_sec(&sc->amrr_to, 1); > + if (!usbd_is_dying(sc->sc_udev)) > + timeout_add_sec(&sc->amrr_to, 1); > } > > int > Index: if_run.c > =================================================================== > RCS file: /cvs/src/sys/dev/usb/if_run.c,v > retrieving revision 1.81 > diff -u -p -r1.81 if_run.c > --- if_run.c 6 Nov 2010 00:09:30 -0000 1.81 > +++ if_run.c 15 Dec 2010 02:58:36 -0000 > @@ -283,6 +283,7 @@ static const struct usb_devno run_devs[] > int run_match(struct device *, void *, void *); > void run_attach(struct device *, struct device *, void *); > int run_detach(struct device *, int); > +int run_activate(struct device *, int); > int run_alloc_rx_ring(struct run_softc *); > void run_free_rx_ring(struct run_softc *); > int run_alloc_tx_ring(struct run_softc *, int); > @@ -368,7 +369,8 @@ struct cfdriver run_cd = { > }; > > const struct cfattach run_ca = { > - sizeof (struct run_softc), run_match, run_attach, run_detach > + sizeof (struct run_softc), run_match, run_attach, run_detach, > + run_activate > }; > > static const struct { > @@ -590,17 +592,32 @@ run_detach(struct device *self, int flag > struct ifnet *ifp = &sc->sc_ic.ic_if; > int qid, s; > > - s = splnet(); > - > - /* wait for all queued asynchronous commands to complete */ > - while (sc->cmdq.queued > 0) > - tsleep(&sc->cmdq, 0, "cmdq", 0); > + s = splusb(); > > if (timeout_initialized(&sc->scan_to)) > timeout_del(&sc->scan_to); > if (timeout_initialized(&sc->calib_to)) > timeout_del(&sc->calib_to); > > + /* wait for all queued asynchronous commands to complete */ > +#if 0 > + while (sc->cmdq.queued > 0) > + tsleep(&sc->cmdq, 0, "cmdq", 0); > +#endif > + /* the async commands are run in a task */ > + usb_rem_wait_task(sc->sc_udev, &sc->sc_task); > + > + /* but the task might not have run if it did not start before > + * usbd_deactivate() was called, so wakeup now. we're > + * detaching, no need to try to run more commands. > + */ > + if (sc->cmdq.queued > 0) { > + sc->cmdq.queued = 0; > + wakeup(&sc->cmdq); > + } > + > + usbd_ref_wait(sc->sc_udev); > + > if (ifp->if_softc != NULL) { > ifp->if_flags &= ~(IFF_RUNNING | IFF_OACTIVE); > ieee80211_ifdetach(ifp); > @@ -1437,8 +1454,15 @@ run_next_scan(void *arg) > { > struct run_softc *sc = arg; > > + if (usbd_is_dying(sc->sc_udev)) > + return; > + > + usbd_ref_incr(sc->sc_udev); > + > if (sc->sc_ic.ic_state == IEEE80211_S_SCAN) > ieee80211_next_scan(&sc->sc_ic.ic_if); > + > + usbd_ref_decr(sc->sc_udev); > } > > void > @@ -1449,6 +1473,9 @@ run_task(void *arg) > struct run_host_cmd *cmd; > int s; > > + if (usbd_is_dying(sc->sc_udev)) > + return; > + > /* process host commands */ > s = splusb(); > while (ring->next != ring->cur) { > @@ -1472,6 +1499,9 @@ run_do_async(struct run_softc *sc, void > struct run_host_cmd *cmd; > int s; > > + if (usbd_is_dying(sc->sc_udev)) > + return; > + > s = splusb(); > cmd = &ring->cmd[ring->cur]; > cmd->cb = cb; > @@ -1530,7 +1560,8 @@ run_newstate_cb(struct run_softc *sc, vo > > case IEEE80211_S_SCAN: > run_set_chan(sc, ic->ic_bss->ni_chan); > - timeout_add_msec(&sc->scan_to, 200); > + if (!usbd_is_dying(sc->sc_udev)) > + timeout_add_msec(&sc->scan_to, 200); > break; > > case IEEE80211_S_AUTH: > @@ -1566,7 +1597,8 @@ run_newstate_cb(struct run_softc *sc, vo > run_read_region_1(sc, RT2860_TX_STA_CNT0, > (uint8_t *)sta, sizeof sta); > /* start calibration timer */ > - timeout_add_sec(&sc->calib_to, 1); > + if (!usbd_is_dying(sc->sc_udev)) > + timeout_add_sec(&sc->calib_to, 1); > } > > /* turn link LED on */ > @@ -1812,7 +1844,9 @@ run_calibrate_cb(struct run_softc *sc, v > ieee80211_amrr_choose(&sc->amrr, sc->sc_ic.ic_bss, &sc->amn); > splx(s); > > -skip: timeout_add_sec(&sc->calib_to, 1); > +skip: > + if (!usbd_is_dying(sc->sc_udev)) > + timeout_add_sec(&sc->calib_to, 1); > } > > void > @@ -2290,6 +2324,11 @@ run_ioctl(struct ifnet *ifp, u_long cmd, > struct ifreq *ifr; > int s, error = 0; > > + if (usbd_is_dying(sc->sc_udev)) > + return ENXIO; > + > + usbd_ref_incr(sc->sc_udev); > + > s = splnet(); > > switch (cmd) { > @@ -2352,6 +2391,8 @@ run_ioctl(struct ifnet *ifp, u_long cmd, > > splx(s); > > + usbd_ref_decr(sc->sc_udev); > + > return error; > } > > @@ -3263,6 +3304,9 @@ run_init(struct ifnet *ifp) > uint8_t bbp1, bbp3; > int i, error, qid, ridx, ntries; > > + if (usbd_is_dying(sc->sc_udev)) > + return ENXIO; > + > for (ntries = 0; ntries < 100; ntries++) { > if ((error = run_read(sc, RT2860_ASIC_VER_ID, &tmp)) != 0) > goto fail; > @@ -3520,4 +3564,21 @@ run_stop(struct ifnet *ifp, int disable) > for (qid = 0; qid < 4; qid++) > run_free_tx_ring(sc, qid); > run_free_rx_ring(sc); > +} > + > +int > +run_activate(struct device *self, int act) > +{ > + struct run_softc *sc = (struct run_softc *)self; > + > + switch (act) { > + case DVACT_ACTIVATE: > + break; > + > + case DVACT_DEACTIVATE: > + usbd_deactivate(sc->sc_udev); > + break; > + } > + > + return 0; > } > Index: if_urtw.c > =================================================================== > RCS file: /cvs/src/sys/dev/usb/if_urtw.c,v > retrieving revision 1.35 > diff -u -p -r1.35 if_urtw.c > --- if_urtw.c 6 Dec 2010 04:41:39 -0000 1.35 > +++ if_urtw.c 15 Dec 2010 02:58:37 -0000 > @@ -775,18 +775,21 @@ urtw_detach(struct device *self, int fla > > s = splusb(); > > - if (ifp->if_softc != NULL) { > - ieee80211_ifdetach(ifp); /* free all nodes */ > - if_detach(ifp); > - } > - > - usb_rem_task(sc->sc_udev, &sc->sc_task); > - usb_rem_task(sc->sc_udev, &sc->sc_ledtask); > if (timeout_initialized(&sc->scan_to)) > timeout_del(&sc->scan_to); > if (timeout_initialized(&sc->sc_led_ch)) > timeout_del(&sc->sc_led_ch); > > + usb_rem_wait_task(sc->sc_udev, &sc->sc_task); > + usb_rem_wait_task(sc->sc_udev, &sc->sc_ledtask); > + > + usbd_ref_wait(sc->sc_udev); > + > + if (ifp->if_softc != NULL) { > + ieee80211_ifdetach(ifp); /* free all nodes */ > + if_detach(ifp); > + } > + > /* abort and free xfers */ > urtw_free_tx_data_list(sc); > urtw_free_rx_data_list(sc); > @@ -1913,7 +1916,8 @@ urtw_led_mode0(struct urtw_softc *sc, in > URTW_LED_OFF : URTW_LED_ON; > t.tv_sec = 0; > t.tv_usec = 100 * 1000L; > - timeout_add(&sc->sc_led_ch, tvtohz(&t)); > + if (!usbd_is_dying(sc->sc_udev)) > + timeout_add(&sc->sc_led_ch, tvtohz(&t)); > break; > case URTW_LED_POWER_ON_BLINK: > urtw_led_on(sc, URTW_LED_GPIO); > @@ -2034,7 +2038,8 @@ urtw_led_blink(struct urtw_softc *sc) > case URTW_LED_BLINK_NORMAL: > t.tv_sec = 0; > t.tv_usec = 100 * 1000L; > - timeout_add(&sc->sc_led_ch, tvtohz(&t)); > + if (!usbd_is_dying(sc->sc_udev)) > + timeout_add(&sc->sc_led_ch, tvtohz(&t)); > break; > default: > panic("unknown LED status 0x%x", sc->sc_gpio_ledstate); > @@ -2397,6 +2402,11 @@ urtw_ioctl(struct ifnet *ifp, u_long cmd > struct ifreq *ifr; > int s, error = 0; > > + if (usbd_is_dying(sc->sc_udev)) > + return (ENXIO); > + > + usbd_ref_incr(sc->sc_udev); > + > s = splnet(); > > switch (cmd) { > @@ -2470,6 +2480,8 @@ urtw_ioctl(struct ifnet *ifp, u_long cmd > > splx(s); > > + usbd_ref_decr(sc->sc_udev); > + > return (error); > } > > @@ -3513,8 +3525,15 @@ urtw_next_scan(void *arg) > struct ieee80211com *ic = &sc->sc_ic; > struct ifnet *ifp = &ic->ic_if; > > + if (usbd_is_dying(sc->sc_udev)) > + return; > + > + usbd_ref_incr(sc->sc_udev); > + > if (ic->ic_state == IEEE80211_S_SCAN) > ieee80211_next_scan(ifp); > + > + usbd_ref_decr(sc->sc_udev); > } > > void > @@ -3526,6 +3545,9 @@ urtw_task(void *arg) > enum ieee80211_state ostate; > usbd_status error = 0; > > + if (usbd_is_dying(sc->sc_udev)) > + return; > + > ostate = ic->ic_state; > > switch (sc->sc_state) { > @@ -3538,7 +3560,8 @@ urtw_task(void *arg) > > case IEEE80211_S_SCAN: > urtw_set_chan(sc, ic->ic_bss->ni_chan); > - timeout_add_msec(&sc->scan_to, 200); > + if (!usbd_is_dying(sc->sc_udev)) > + timeout_add_msec(&sc->scan_to, 200); > break; > > case IEEE80211_S_AUTH: > Index: usbdi.c > =================================================================== > RCS file: /cvs/src/sys/dev/usb/usbdi.c,v > retrieving revision 1.41 > diff -u -p -r1.41 usbdi.c > --- usbdi.c 6 Dec 2010 04:25:27 -0000 1.41 > +++ usbdi.c 15 Dec 2010 02:58:37 -0000 > @@ -97,6 +97,26 @@ usbd_deactivate(usbd_device_handle dev) > dev->dying = 1; > } > > +void > +usbd_ref_incr(usbd_device_handle dev) > +{ > + dev->ref_cnt++; > +} > + > +void > +usbd_ref_decr(usbd_device_handle dev) > +{ > + if (--dev->ref_cnt == 0 && dev->dying) > + wakeup(&dev->ref_cnt); > +} > + > +void > +usbd_ref_wait(usbd_device_handle dev) > +{ > + while (dev->ref_cnt > 0) > + tsleep(&dev->ref_cnt, PWAIT, "usbref", hz * 60); > +} > + > static __inline int > usbd_xfer_isread(usbd_xfer_handle xfer) > { > Index: usbdi.h > =================================================================== > RCS file: /cvs/src/sys/dev/usb/usbdi.h,v > retrieving revision 1.36 > diff -u -p -r1.36 usbdi.h > --- usbdi.h 6 Dec 2010 04:25:27 -0000 1.36 > +++ usbdi.h 15 Dec 2010 02:58:37 -0000 > @@ -168,6 +168,10 @@ int usbd_ratecheck(struct timeval *last) > int usbd_is_dying(usbd_device_handle); > void usbd_deactivate(usbd_device_handle); > > +void usbd_ref_incr(usbd_device_handle); > +void usbd_ref_decr(usbd_device_handle); > +void usbd_ref_wait(usbd_device_handle); > + > /* An iterator for descriptors. */ > typedef struct { > const uByte *cur; > Index: usbdivar.h > =================================================================== > RCS file: /cvs/src/sys/dev/usb/usbdivar.h,v > retrieving revision 1.40 > diff -u -p -r1.40 usbdivar.h > --- usbdivar.h 6 Dec 2010 04:25:27 -0000 1.40 > +++ usbdivar.h 15 Dec 2010 02:58:37 -0000 > @@ -125,7 +125,8 @@ struct usbd_bus { > struct usbd_device { > struct usbd_bus *bus; /* our controller */ > struct usbd_pipe *default_pipe; /* pipe 0 */ > - u_int8_t dying; /* removed */ > + u_int8_t dying; /* hardware removed */ > + u_int8_t ref_cnt; /* # of procs using device */ > u_int8_t address; /* device address */ > u_int8_t config; /* current configuration # */ > u_int8_t depth; /* distance from root hub */
-- jake...@sdf.lonestar.org SDF Public Access UNIX System - http://sdf.lonestar.org
