Re: better run(4) fix (was: run(4): fix crash in run_task())

[Stefan Sperling]

On Tue, Jul 14, 2015 at 09:56:53PM -0600, Stefan Sperling wrote:
> This implements your suggestion to properly abort async tasks when
> bringing the interface down.
> 
> Fixes the problem for me just as well.

And yet another diff after more discussion.

This diff makes the driver schedule a single 80211 state transition at a time.
The command queue is now used for commands to the device firmware only.

A task scheduled by the scan timeout will now be overwritten when run_stop()
schedules a transition to INIT. So there cannot be a SCAN->SCAN transition
that restarts the timeout while the interface goes down.

If this pattern works out we could move storage and management of this task
up into the net80211 layer eventyally, and make use of it in more drivers.

Index: if_run.c
===================================================================
RCS file: /cvs/src/sys/dev/usb/if_run.c,v
retrieving revision 1.109
diff -u -p -r1.109 if_run.c
--- if_run.c    12 Jun 2015 15:47:31 -0000      1.109
+++ if_run.c    15 Jul 2015 19:54:40 -0000
@@ -34,6 +34,7 @@
 #include <sys/conf.h>
 #include <sys/device.h>
 #include <sys/endian.h>
+#include <sys/task.h>
 
 #include <machine/intr.h>
 
@@ -361,7 +362,7 @@ void                run_task(void *);
 void           run_do_async(struct run_softc *, void (*)(struct run_softc *,
                    void *), void *, int);
 int            run_newstate(struct ieee80211com *, enum ieee80211_state, int);
-void           run_newstate_cb(struct run_softc *, void *);
+void           run_newstate_task(void *);
 void           run_updateedca(struct ieee80211com *);
 void           run_updateedca_cb(struct run_softc *, void *);
 int            run_set_key(struct ieee80211com *, struct ieee80211_node *,
@@ -568,6 +569,7 @@ run_attach(struct device *parent, struct
        }
 
        usb_init_task(&sc->sc_task, run_task, sc, USB_TASK_TYPE_GENERIC);
+       task_set(&sc->sc_newstate_task, run_newstate_task, sc);
        timeout_set(&sc->scan_to, run_next_scan, sc);
        timeout_set(&sc->calib_to, run_calibrate_to, sc);
 
@@ -685,6 +687,8 @@ run_detach(struct device *self, int flag
 
        s = splusb();
 
+       task_del(systq, &sc->sc_newstate_task);
+
        if (timeout_initialized(&sc->scan_to))
                timeout_del(&sc->scan_to);
        if (timeout_initialized(&sc->calib_to))
@@ -1767,25 +1771,30 @@ int
 run_newstate(struct ieee80211com *ic, enum ieee80211_state nstate, int arg)
 {
        struct run_softc *sc = ic->ic_softc;
-       struct run_cmd_newstate cmd;
+       struct run_newstate_task_arg *task_arg = &sc->sc_newstate_task_arg;
+
+       /* remove any already scheduled transition */
+       task_del(systq, &sc->sc_newstate_task);
+
+       /* schedule transition in a process context */
+       task_arg->state = nstate;
+       task_arg->arg = arg;
+       task_add(systq, &sc->sc_newstate_task);
 
-       /* do it in a process context */
-       cmd.state = nstate;
-       cmd.arg = arg;
-       run_do_async(sc, run_newstate_cb, &cmd, sizeof cmd);
        return 0;
 }
 
 void
-run_newstate_cb(struct run_softc *sc, void *arg)
+run_newstate_task(void *arg)
 {
-       struct run_cmd_newstate *cmd = arg;
+       struct run_softc *sc = arg;
+       struct run_newstate_task_arg *task_arg = &sc->sc_newstate_task_arg;
        struct ieee80211com *ic = &sc->sc_ic;
        enum ieee80211_state ostate;
        struct ieee80211_node *ni;
        uint32_t tmp, sta[3];
        uint8_t wcid;
-       int s;
+       int s, nstate = task_arg->state;
 
        s = splnet();
        ostate = ic->ic_state;
@@ -1795,7 +1804,7 @@ run_newstate_cb(struct run_softc *sc, vo
                run_set_leds(sc, RT2860_LED_RADIO);
        }
 
-       switch (cmd->state) {
+       switch (nstate) {
        case IEEE80211_S_INIT:
                if (ostate == IEEE80211_S_RUN) {
                        /* abort TSF synchronization */
@@ -1855,7 +1864,7 @@ run_newstate_cb(struct run_softc *sc, vo
                     RT2860_LED_LINK_2GHZ : RT2860_LED_LINK_5GHZ));
                break;
        }
-       (void)sc->sc_newstate(ic, cmd->state, cmd->arg);
+       (void)sc->sc_newstate(ic, task_arg->state, task_arg->arg);
        splx(s);
 }