On Sun, Sep 27, 2015 at 08:00:19PM +0200, Stefan Sperling wrote:
> This is yet another attempt at improving the iwm(4) newstate task.
This diff has been working nicely for me, with many suspend/resume cycles.
Never had a problem connecting to several wifis.
Any objections? Any Oks?
> The goal is to simplify things by only queuing one state transition
> at a time. The newstate task now always transitions to the most
> recently requested state, rather than hopping along with every request.
>
> This allows us get rid of the silly newstate generation counter, and
> allows us to simply cancel any outstanding transition when the interface
> goes down.
>
> The old code was queuing *additional* work from iwm_stop(). Which meant,
> for example, that every time upon resume, a task ran only to discover that
> it is no longer relevant.
>
> This probably needs some testing to shake out bugs.
> Test reports are very much appreciated!
>
> This change might also fix semi-frequent firmware errors during association.
> But not all -- I've found that running with IWM_DEBUG cranked up produces
> sufficient printfs to make firmware commands time out more often.
> It all seems very sensitive to timing which is hard to get completely
> right with tasks involved.
>
> Index: if_iwm.c
> ===================================================================
> RCS file: /cvs/src/sys/dev/pci/if_iwm.c,v
> retrieving revision 1.51
> diff -u -p -r1.51 if_iwm.c
> --- if_iwm.c 27 Sep 2015 16:53:38 -0000 1.51
> +++ if_iwm.c 27 Sep 2015 17:33:58 -0000
> @@ -195,14 +195,6 @@ const struct iwm_rate {
> #define IWM_RIDX_IS_CCK(_i_) ((_i_) < IWM_RIDX_OFDM)
> #define IWM_RIDX_IS_OFDM(_i_) ((_i_) >= IWM_RIDX_OFDM)
>
> -struct iwm_newstate_state {
> - struct task ns_wk;
> - struct ieee80211com *ns_ic;
> - enum ieee80211_state ns_nstate;
> - int ns_arg;
> - int ns_generation;
> -};
> -
> int iwm_store_cscheme(struct iwm_softc *, uint8_t *, size_t);
> int iwm_firmware_store_section(struct iwm_softc *, enum iwm_ucode_type,
> uint8_t *, size_t);
> @@ -406,7 +398,7 @@ struct ieee80211_node *iwm_node_alloc(st
> void iwm_calib_timeout(void *);
> void iwm_setrates(struct iwm_node *);
> int iwm_media_change(struct ifnet *);
> -void iwm_newstate_cb(void *);
> +void iwm_newstate_task(void *);
> int iwm_newstate(struct ieee80211com *, enum ieee80211_state, int);
> void iwm_endscan_cb(void *);
> int iwm_init_hw(struct iwm_softc *);
> @@ -5263,43 +5255,29 @@ iwm_media_change(struct ifnet *ifp)
> }
>
> void
> -iwm_newstate_cb(void *wk)
> +iwm_newstate_task(void *psc)
> {
> - struct iwm_newstate_state *iwmns = (void *)wk;
> - struct ieee80211com *ic = iwmns->ns_ic;
> - enum ieee80211_state nstate = iwmns->ns_nstate;
> - int generation = iwmns->ns_generation;
> + struct iwm_softc *sc = (struct iwm_softc *)psc;
> + struct ieee80211com *ic = &sc->sc_ic;
> + enum ieee80211_state nstate = sc->ns_nstate;
> + enum ieee80211_state ostate = ic->ic_state;
> struct iwm_node *in;
> - int arg = iwmns->ns_arg;
> - struct ifnet *ifp = IC2IFP(ic);
> - struct iwm_softc *sc = ifp->if_softc;
> + int arg = sc->ns_arg;
> int error;
>
> - free(iwmns, M_DEVBUF, sizeof(*iwmns));
> -
> - DPRINTF(("Prepare to switch state %s->%s\n",
> - ieee80211_state_name[ic->ic_state],
> - ieee80211_state_name[nstate]));
> - if (sc->sc_generation != generation) {
> - DPRINTF(("newstate_cb: someone pulled the plug meanwhile\n"));
> - if (nstate == IEEE80211_S_INIT) {
> - DPRINTF(("newstate_cb: nstate == IEEE80211_S_INIT:
> calling sc_newstate()\n"));
> - sc->sc_newstate(ic, nstate, arg);
> - }
> - return;
> - }
> -
> DPRINTF(("switching state %s->%s\n",
> - ieee80211_state_name[ic->ic_state],
> + ieee80211_state_name[ostate],
> ieee80211_state_name[nstate]));
>
> - if (ic->ic_state == IEEE80211_S_SCAN && nstate != ic->ic_state)
> + if (ostate == IEEE80211_S_SCAN && nstate != ostate)
> iwm_led_blink_stop(sc);
>
> /* disable beacon filtering if we're hopping out of RUN */
> - if (ic->ic_state == IEEE80211_S_RUN && nstate != ic->ic_state) {
> + if (ostate == IEEE80211_S_RUN && nstate != ostate)
> iwm_mvm_disable_beacon_filter(sc, (void *)ic->ic_bss);
>
> + /* Reset the device if moving out of AUTH, ASSOC, or RUN. */
> + if (ostate > IEEE80211_S_SCAN && nstate < ostate) {
> if (((in = (void *)ic->ic_bss) != NULL))
> in->in_assoc = 0;
> iwm_release(sc, NULL);
> @@ -5393,25 +5371,15 @@ iwm_newstate_cb(void *wk)
> int
> iwm_newstate(struct ieee80211com *ic, enum ieee80211_state nstate, int arg)
> {
> - struct iwm_newstate_state *iwmns;
> struct ifnet *ifp = IC2IFP(ic);
> struct iwm_softc *sc = ifp->if_softc;
>
> timeout_del(&sc->sc_calib_to);
>
> - iwmns = malloc(sizeof(*iwmns), M_DEVBUF, M_NOWAIT);
> - if (!iwmns) {
> - DPRINTF(("%s: allocating state cb mem failed\n", DEVNAME(sc)));
> - return ENOMEM;
> - }
> -
> - iwmns->ns_ic = ic;
> - iwmns->ns_nstate = nstate;
> - iwmns->ns_arg = arg;
> - iwmns->ns_generation = sc->sc_generation;
> + sc->ns_nstate = nstate;
> + sc->ns_arg = arg;
>
> - task_set(&iwmns->ns_wk, iwm_newstate_cb, iwmns);
> - task_add(sc->sc_nswq, &iwmns->ns_wk);
> + task_add(sc->sc_nswq, &sc->newstate_task);
>
> return 0;
> }
> @@ -5676,8 +5644,10 @@ iwm_stop(struct ifnet *ifp, int disable)
> ic->ic_scan_lock = IEEE80211_SCAN_UNLOCKED;
> ifp->if_flags &= ~(IFF_RUNNING | IFF_OACTIVE);
>
> - if (ic->ic_state != IEEE80211_S_INIT)
> - ieee80211_new_state(ic, IEEE80211_S_INIT, -1);
> + task_del(systq, &sc->init_task);
> + task_del(sc->sc_nswq, &sc->newstate_task);
> + task_del(sc->sc_eswq, &sc->sc_eswk);
> + sc->sc_newstate(ic, IEEE80211_S_INIT, -1);
>
> timeout_del(&sc->sc_calib_to);
> iwm_led_blink_stop(sc);
> @@ -6614,6 +6584,7 @@ iwm_attach(struct device *parent, struct
> timeout_set(&sc->sc_calib_to, iwm_calib_timeout, sc);
> timeout_set(&sc->sc_led_blink_to, iwm_led_blink_timeout, sc);
> task_set(&sc->init_task, iwm_init_task, sc);
> + task_set(&sc->newstate_task, iwm_newstate_task, sc);
>
> /*
> * We cannot read the MAC address without loading the
> @@ -6687,8 +6658,7 @@ iwm_wakeup(struct iwm_softc *sc)
> reg = pci_conf_read(sc->sc_pct, sc->sc_pcitag, 0x40);
> pci_conf_write(sc->sc_pct, sc->sc_pcitag, 0x40, reg & ~0xff00);
>
> - iwm_init_task(sc);
> -
> + task_add(systq, &sc->init_task);
> }
>
> int
> Index: if_iwmvar.h
> ===================================================================
> RCS file: /cvs/src/sys/dev/pci/if_iwmvar.h,v
> retrieving revision 1.9
> diff -u -p -r1.9 if_iwmvar.h
> --- if_iwmvar.h 15 Jun 2015 08:06:12 -0000 1.9
> +++ if_iwmvar.h 27 Sep 2015 17:09:03 -0000
> @@ -364,6 +364,9 @@ struct iwm_softc {
> struct timeout sc_led_blink_to;
>
> struct task init_task;
> + struct task newstate_task;
> + enum ieee80211_state ns_nstate;
> + int ns_arg;
>
> bus_space_tag_t sc_st;
> bus_space_handle_t sc_sh;
