I will test this patch with my Linksys rum(4) AP tonight and report back to you.

-luis

On 12/22/10 18:36, Jacob Meuser wrote:
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 */

Reply via email to