On Thu, Jun 18, 2015 at 04:23:43PM +0200, Stefan Sperling wrote:
> The net80211 stack assumes drivers will switch IEEE80211_S_* states in
> interrupt context. iwm(4) does not follow this rule. Since it insists on
> responses from firmware commands to look for success or failure and it
> uses tsleep() to wait for responses it cannot switch state in interrupt
> context. So currently, the entire state machine is deferred to process
> context (big hammer solution) :-/
>
> Complications arise in the suspend/resume path because of this, such as
> http://marc.info/?l=openbsd-tech&m=143438073018743&w=2 apart from several
> other such issues where a failure on part of the firmware to respond will
> deadlock the driver in an endless tsleep.
>
> I would very much like iwm_newstate() to be interrupt safe and get rid of
> the pesky newstate_cb task which wraps it. It makes debugging and following
> the control flow difficult. And I hope the driver will be more stable overall.
>
> There are two ways to approach this:
>
> - Simply don't care about answers from firmware when in interrupt
> (note that this is what iwn(4) does)
>
> - Busy-wait for replies from the firmware when in interrupt
Here's a diff implementing a third approach, discussed with mpi@.
- Keep the newstate transitions in a task thread, but only ever
schedule one 80211 state transition at a time.
Requires a tweak for suspend/resume, which wants to run two state
transitions at resume time if the interface was up during suspend
(back to INIT, then INIT -> SCAN).
Please test this if you use iwm(4). It should make the driver more
reliable, e.g. when bringing the interface up which sometimes fails
because of... reasons.
Index: if_iwm.c
===================================================================
RCS file: /cvs/src/sys/dev/pci/if_iwm.c,v
retrieving revision 1.45
diff -u -p -r1.45 if_iwm.c
--- if_iwm.c 15 Jun 2015 08:06:11 -0000 1.45
+++ if_iwm.c 19 Jul 2015 02:13:13 -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,13 +398,13 @@ 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 *);
int iwm_init(struct ifnet *);
void iwm_start(struct ifnet *);
-void iwm_stop(struct ifnet *, int);
+void iwm_stop(struct ifnet *);
void iwm_watchdog(struct ifnet *);
int iwm_ioctl(struct ifnet *, u_long, iwm_caddr_t);
const char *iwm_desc_lookup(uint32_t);
@@ -427,7 +419,8 @@ void iwm_attach_hook(iwm_hookarg_t);
void iwm_attach(struct device *, struct device *, void *);
void iwm_init_task(void *);
int iwm_activate(struct device *, int);
-void iwm_wakeup(struct iwm_softc *);
+void iwm_suspend(struct iwm_softc *);
+void iwm_resume(struct iwm_softc *);
#if NBPFILTER > 0
void iwm_radiotap_attach(struct iwm_softc *);
@@ -5252,38 +5245,25 @@ iwm_media_change(struct ifnet *ifp)
if ((ifp->if_flags & (IFF_UP | IFF_RUNNING)) ==
(IFF_UP | IFF_RUNNING)) {
- iwm_stop(ifp, 0);
+ iwm_stop(ifp);
error = iwm_init(ifp);
}
return error;
}
void
-iwm_newstate_cb(void *wk)
+iwm_newstate_task(void *arg)
{
- 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 = arg;
+ struct ieee80211com *ic = &sc->sc_ic;
+ struct iwm_newstate_task_arg *task_arg = &sc->sc_newstate_task_arg;
+ enum ieee80211_state nstate = task_arg->state;
struct iwm_node *in;
- int arg = iwmns->ns_arg;
- struct ifnet *ifp = IC2IFP(ic);
- struct iwm_softc *sc = ifp->if_softc;
int error;
- free(iwmns, M_DEVBUF, sizeof(*iwmns));
-
- DPRINTF(("Prepare to switch state %d->%d\n", ic->ic_state, 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 %d->%d\n", ic->ic_state, nstate));
+ DPRINTF(("%s: %s->%s\n", __func__,
+ ieee80211_state_name[ic->ic_state],
+ ieee80211_state_name[nstate]));
if (ic->ic_state == IEEE80211_S_SCAN && nstate != ic->ic_state)
iwm_led_blink_stop(sc);
@@ -5292,6 +5272,8 @@ iwm_newstate_cb(void *wk)
if (ic->ic_state == IEEE80211_S_RUN && nstate != ic->ic_state) {
iwm_mvm_disable_beacon_filter(sc, (void *)ic->ic_bss);
+ timeout_del(&sc->sc_calib_to);
+
if (((in = (void *)ic->ic_bss) != NULL))
in->in_assoc = 0;
iwm_release(sc, NULL);
@@ -5310,8 +5292,9 @@ iwm_newstate_cb(void *wk)
if (nstate == IEEE80211_S_SCAN ||
nstate == IEEE80211_S_AUTH ||
nstate == IEEE80211_S_ASSOC) {
- DPRINTF(("Force transition to INIT; MGT=%d\n", arg));
- sc->sc_newstate(ic, IEEE80211_S_INIT, arg);
+ DPRINTF(("Force transition to INIT; MGT=%d\n",
+ task_arg->arg));
+ sc->sc_newstate(ic, IEEE80211_S_INIT, task_arg->arg);
DPRINTF(("Going INIT->SCAN\n"));
nstate = IEEE80211_S_SCAN;
}
@@ -5320,6 +5303,10 @@ iwm_newstate_cb(void *wk)
switch (nstate) {
case IEEE80211_S_INIT:
sc->sc_scanband = 0;
+ sc->sc_auth_prot = 0;
+ ic->ic_scan_lock = IEEE80211_SCAN_UNLOCKED;
+ sc->sc_flags |= IWM_FLAG_STOPPED;
+ iwm_stop_device(sc);
break;
case IEEE80211_S_SCAN:
@@ -5379,31 +5366,27 @@ iwm_newstate_cb(void *wk)
break;
}
- sc->sc_newstate(ic, nstate, arg);
+ sc->sc_newstate(ic, nstate, task_arg->arg);
+
+ if (nstate == IEEE80211_S_INIT && (sc->sc_flags & IWM_FLAG_RESET)) {
+ sc->sc_flags &= ~IWM_FLAG_RESET;
+ iwm_init(IC2IFP(&sc->sc_ic));
+ }
}
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;
- }
+ struct iwm_softc *sc = IC2IFP(ic)->if_softc;
+ struct iwm_newstate_task_arg *task_arg = &sc->sc_newstate_task_arg;
- iwmns->ns_ic = ic;
- iwmns->ns_nstate = nstate;
- iwmns->ns_arg = arg;
- iwmns->ns_generation = sc->sc_generation;
+ /* Remove any already scheduled transition. */
+ task_del(systq, &sc->sc_newstate_task);
- task_set(&iwmns->ns_wk, iwm_newstate_cb, iwmns);
- task_add(sc->sc_nswq, &iwmns->ns_wk);
+ /* Schedule state transition in a process context. */
+ task_arg->state = nstate;
+ task_arg->arg = arg;
+ task_add(systq, &sc->sc_newstate_task);
return 0;
}
@@ -5551,14 +5534,11 @@ iwm_init(struct ifnet *ifp)
struct iwm_softc *sc = ifp->if_softc;
int error;
- if (sc->sc_flags & IWM_FLAG_HW_INITED) {
- return 0;
- }
sc->sc_generation++;
sc->sc_flags &= ~IWM_FLAG_STOPPED;
if ((error = iwm_init_hw(sc)) != 0) {
- iwm_stop(ifp, 1);
+ iwm_stop(ifp);
return error;
}
@@ -5570,7 +5550,6 @@ iwm_init(struct ifnet *ifp)
ifp->if_flags |= IFF_RUNNING;
ieee80211_begin_scan(ifp);
- sc->sc_flags |= IWM_FLAG_HW_INITED;
return 0;
}
@@ -5648,26 +5627,17 @@ iwm_start(struct ifnet *ifp)
}
void
-iwm_stop(struct ifnet *ifp, int disable)
+iwm_stop(struct ifnet *ifp)
{
struct iwm_softc *sc = ifp->if_softc;
struct ieee80211com *ic = &sc->sc_ic;
- sc->sc_flags &= ~IWM_FLAG_HW_INITED;
- sc->sc_flags |= IWM_FLAG_STOPPED;
- sc->sc_generation++;
- sc->sc_scanband = 0;
- sc->sc_auth_prot = 0;
- ic->ic_scan_lock = IEEE80211_SCAN_UNLOCKED;
ifp->if_flags &= ~(IFF_RUNNING | IFF_OACTIVE);
+ ifp->if_timer = 0;
+ sc->sc_tx_timer = 0; /* cancel watchdog */
if (ic->ic_state != IEEE80211_S_INIT)
ieee80211_new_state(ic, IEEE80211_S_INIT, -1);
-
- timeout_del(&sc->sc_calib_to);
- iwm_led_blink_stop(sc);
- ifp->if_timer = sc->sc_tx_timer = 0;
- iwm_stop_device(sc);
}
void
@@ -5730,7 +5700,7 @@ iwm_ioctl(struct ifnet *ifp, u_long cmd,
}
} else {
if (ifp->if_flags & IFF_RUNNING)
- iwm_stop(ifp, 1);
+ iwm_stop(ifp);
}
break;
@@ -5752,7 +5722,7 @@ iwm_ioctl(struct ifnet *ifp, u_long cmd,
error = 0;
if ((ifp->if_flags & (IFF_UP | IFF_RUNNING)) ==
(IFF_UP | IFF_RUNNING)) {
- iwm_stop(ifp, 0);
+ iwm_stop(ifp);
error = iwm_init(ifp);
}
}
@@ -6227,7 +6197,7 @@ iwm_intr(void *arg)
printf("%s: fatal firmware error\n", DEVNAME(sc));
ifp->if_flags &= ~IFF_UP;
- iwm_stop(ifp, 1);
+ iwm_stop(ifp);
rv = 1;
goto out;
@@ -6237,7 +6207,7 @@ iwm_intr(void *arg)
handled |= IWM_CSR_INT_BIT_HW_ERR;
printf("%s: hardware error, stopping device \n", DEVNAME(sc));
ifp->if_flags &= ~IFF_UP;
- iwm_stop(ifp, 1);
+ iwm_stop(ifp);
rv = 1;
goto out;
}
@@ -6257,7 +6227,7 @@ iwm_intr(void *arg)
DPRINTF(("%s: rfkill switch, disabling interface\n",
DEVNAME(sc)));
ifp->if_flags &= ~IFF_UP;
- iwm_stop(ifp, 1);
+ iwm_stop(ifp);
}
}
@@ -6533,9 +6503,6 @@ iwm_attach(struct device *parent, struct
sc->sc_eswq = taskq_create("iwmes", 1, IPL_NET, 0);
if (sc->sc_eswq == NULL)
goto fail4;
- sc->sc_nswq = taskq_create("iwmns", 1, IPL_NET, 0);
- if (sc->sc_nswq == NULL)
- goto fail4;
/* Clear pending interrupts. */
IWM_WRITE(sc, IWM_CSR_INT, 0xffffffff);
@@ -6587,6 +6554,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->sc_newstate_task, iwm_newstate_task, sc);
/*
* We cannot read the MAC address without loading the
@@ -6634,34 +6602,62 @@ void
iwm_init_task(void *arg1)
{
struct iwm_softc *sc = arg1;
+ struct ieee80211com *ic = &sc->sc_ic;
struct ifnet *ifp = &sc->sc_ic.ic_if;
- int s;
- s = splnet();
- while (sc->sc_flags & IWM_FLAG_BUSY)
- tsleep(&sc->sc_flags, 0, "iwmpwr", 0);
- sc->sc_flags |= IWM_FLAG_BUSY;
+ if (ic->ic_state == IEEE80211_S_INIT)
+ return;
- iwm_stop(ifp, 0);
- if ((ifp->if_flags & (IFF_UP | IFF_RUNNING)) == IFF_UP)
- iwm_init(ifp);
+ if ((ifp->if_flags & (IFF_UP | IFF_RUNNING)) != IFF_UP)
+ return;
- sc->sc_flags &= ~IWM_FLAG_BUSY;
- wakeup(&sc->sc_flags);
- splx(s);
+ /* Schedule state transition to INIT followed by INIT -> SCAN. */
+ sc->sc_flags |= IWM_FLAG_RESET;
+ ieee80211_new_state(ic, IEEE80211_S_INIT, -1);
+}
+
+void
+iwm_suspend(struct iwm_softc *sc)
+{
+ struct ifnet *ifp = &sc->sc_ic.ic_if;
+ struct ieee80211com *ic = &sc->sc_ic;
+
+ ifp->if_flags &= ~(IFF_RUNNING | IFF_OACTIVE);
+ ifp->if_timer = 0;
+ sc->sc_tx_timer = 0; /* cancel watchdog */
+
+ task_del(systq, &sc->init_task);
+
+ /* Remove any already scheduled transition. */
+ task_del(systq, &sc->sc_newstate_task);
+
+ /* Cancel pending timeouts. */
+ if (ic->ic_state == IEEE80211_S_SCAN)
+ iwm_led_blink_stop(sc);
+ if (ic->ic_state == IEEE80211_S_RUN)
+ timeout_del(&sc->sc_calib_to);
+
+ /* Stop hardware. */
+ iwm_stop_device(sc);
}
void
-iwm_wakeup(struct iwm_softc *sc)
+iwm_resume(struct iwm_softc *sc)
{
pcireg_t reg;
+ struct ieee80211com *ic = &sc->sc_ic;
+ struct ifnet *ifp = &sc->sc_ic.ic_if;
/* Clear device-specific "PCI retry timeout" register (41h). */
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);
-
+ if (ic->ic_state != IEEE80211_S_INIT &&
+ (ifp->if_flags & (IFF_UP | IFF_RUNNING)) == IFF_UP) {
+ /* Schedule transition to INIT followed by INIT -> SCAN. */
+ sc->sc_flags |= IWM_FLAG_RESET;
+ ieee80211_new_state(ic, IEEE80211_S_INIT, -1);
+ }
}
int
@@ -6673,10 +6669,10 @@ iwm_activate(struct device *self, int ac
switch (act) {
case DVACT_SUSPEND:
if (ifp->if_flags & IFF_RUNNING)
- iwm_stop(ifp, 0);
+ iwm_suspend(sc);
break;
- case DVACT_WAKEUP:
- iwm_wakeup(sc);
+ case DVACT_RESUME:
+ iwm_resume(sc);
break;
}
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 16 Jul 2015 00:56:37 -0000
@@ -288,7 +288,7 @@ struct iwm_rx_ring {
};
#define IWM_FLAG_USE_ICT 0x01
-#define IWM_FLAG_HW_INITED 0x02
+#define IWM_FLAG_RESET 0x02
#define IWM_FLAG_STOPPED 0x04
#define IWM_FLAG_RFKILL 0x08
#define IWM_FLAG_BUSY 0x10
@@ -353,11 +353,17 @@ struct iwm_bf_data {
int last_cqm_event;
};
+struct iwm_newstate_task_arg {
+ enum ieee80211_state state;
+ int arg;
+};
+
struct iwm_softc {
struct device sc_dev;
struct ieee80211com sc_ic;
int (*sc_newstate)(struct ieee80211com *, enum ieee80211_state, int);
- int sc_newstate_pending;
+ struct task sc_newstate_task;
+ struct iwm_newstate_task_arg sc_newstate_task_arg;
struct ieee80211_amrr sc_amrr;
struct timeout sc_calib_to;
@@ -448,7 +454,7 @@ struct iwm_softc {
uint8_t sc_cmd_resp[IWM_CMD_RESP_MAX];
int sc_wantresp;
- struct taskq *sc_nswq, *sc_eswq;
+ struct taskq *sc_eswq;
struct task sc_eswk;
struct iwm_rx_phy_info sc_last_phy_info;
