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 The diff below is an attempt at the first approach simply because it can be written with less code (no busy waits). And if we ignore an error response and the firmware freaks out, well, we'll eventually "handle" the problem by resetting the hardware. There's nothing much else we could do if a command fails to complete after busy-wait anyway. I would very much like other people to test this change to ensure I'm not breaking some other use case I can't cover. And perhaps there's a condition where a busy-wait is necessary to ensure proper operation? I've tested suspend/resume, and switching associations between a couple of different APs, and interface down/up on the AP side. I'm not seeing any regressions apart from the fact that the firmware will crash if a scan is run while the interface is up -- bringing the interface back up is possible so this is not entirely horrible. Note that we must get rid of iwm_release() or we break suspend/resume and de-auth from AP, because iwm_release() calls hw_init() which will tsleep() for loading firmware. This is probably related to why scans don't work while we're up. However, the way the driver currently handles RUN->SCAN is pretty broken as well since it merely provides an illusion of it working and doesn't actually know how to handle this transition in firmware/hardware. 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 18 Jun 2015 13:29:46 -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); @@ -338,9 +330,9 @@ int iwm_send_cmd(struct iwm_softc *, str int iwm_mvm_send_cmd_pdu(struct iwm_softc *, uint8_t, uint32_t, uint16_t, const void *); int iwm_mvm_send_cmd_status(struct iwm_softc *, struct iwm_host_cmd *, - uint32_t *); + uint32_t *, int); int iwm_mvm_send_cmd_pdu_status(struct iwm_softc *, uint8_t, - uint16_t, const void *, uint32_t *); + uint16_t, const void *, uint32_t *, int); void iwm_free_resp(struct iwm_softc *, struct iwm_host_cmd *); void iwm_cmd_done(struct iwm_softc *, struct iwm_rx_packet *); void iwm_update_sched(struct iwm_softc *, int, int, uint8_t, uint16_t); @@ -401,12 +393,10 @@ int iwm_mvm_mac_ctxt_changed(struct iwm_ int iwm_mvm_update_quotas(struct iwm_softc *, struct iwm_node *); int iwm_auth(struct iwm_softc *); int iwm_assoc(struct iwm_softc *); -int iwm_release(struct iwm_softc *, struct iwm_node *); struct ieee80211_node *iwm_node_alloc(struct ieee80211com *); void iwm_calib_timeout(void *); void iwm_setrates(struct iwm_node *); int iwm_media_change(struct ifnet *); -void iwm_newstate_cb(void *); int iwm_newstate(struct ieee80211com *, enum ieee80211_state, int); void iwm_endscan_cb(void *); int iwm_init_hw(struct iwm_softc *); @@ -2188,10 +2178,10 @@ iwm_mvm_send_time_event_cmd(struct iwm_s if (sc->sc_capaflags & IWM_UCODE_TLV_FLAGS_TIME_EVENT_API_V2) return iwm_mvm_send_cmd_pdu(sc, IWM_TIME_EVENT_CMD, - IWM_CMD_SYNC, sizeof(*cmd), cmd); + IWM_CMD_ASYNC, sizeof(*cmd), cmd); iwm_mvm_te_v2_to_v1(cmd, &cmd_v1); - return iwm_mvm_send_cmd_pdu(sc, IWM_TIME_EVENT_CMD, IWM_CMD_SYNC, + return iwm_mvm_send_cmd_pdu(sc, IWM_TIME_EVENT_CMD, IWM_CMD_ASYNC, sizeof(cmd_v1), &cmd_v1); } @@ -3189,7 +3179,6 @@ iwm_mvm_binding_cmd(struct iwm_softc *sc struct iwm_binding_cmd cmd; struct iwm_mvm_phy_ctxt *phyctxt = in->in_phyctxt; int i, ret; - uint32_t status; memset(&cmd, 0, sizeof(cmd)); @@ -3202,20 +3191,11 @@ iwm_mvm_binding_cmd(struct iwm_softc *sc for (i = 1; i < IWM_MAX_MACS_IN_BINDING; i++) cmd.macs[i] = htole32(IWM_FW_CTXT_INVALID); - status = 0; ret = iwm_mvm_send_cmd_pdu_status(sc, IWM_BINDING_CONTEXT_CMD, - sizeof(cmd), &cmd, &status); - if (ret) { + sizeof(cmd), &cmd, NULL, 1); + if (ret) DPRINTF(("%s: Failed to send binding (action:%d): %d\n", DEVNAME(sc), action, ret)); - return ret; - } - - if (status) { - DPRINTF(("%s: Binding command failed: %u\n", DEVNAME(sc), - status)); - ret = EIO; - } return ret; } @@ -3300,7 +3280,7 @@ iwm_mvm_phy_ctxt_apply(struct iwm_softc iwm_mvm_phy_ctxt_cmd_data(sc, &cmd, ctxt->channel, chains_static, chains_dynamic); - ret = iwm_mvm_send_cmd_pdu(sc, IWM_PHY_CONTEXT_CMD, IWM_CMD_SYNC, + ret = iwm_mvm_send_cmd_pdu(sc, IWM_PHY_CONTEXT_CMD, IWM_CMD_ASYNC, sizeof(struct iwm_phy_context_cmd), &cmd); if (ret) { DPRINTF(("PHY ctxt cmd error. ret=%d\n", ret)); @@ -3516,19 +3496,23 @@ iwm_mvm_send_cmd_pdu(struct iwm_softc *s int iwm_mvm_send_cmd_status(struct iwm_softc *sc, - struct iwm_host_cmd *cmd, uint32_t *status) + struct iwm_host_cmd *cmd, uint32_t *status, int async) { struct iwm_rx_packet *pkt; struct iwm_cmd_response *resp; int error, resp_len; - //lockdep_assert_held(&mvm->mutex); - KASSERT((cmd->flags & IWM_CMD_WANT_SKB) == 0); - cmd->flags |= IWM_CMD_SYNC | IWM_CMD_WANT_SKB; + if (async) + cmd->flags |= IWM_CMD_ASYNC; + else + cmd->flags |= (IWM_CMD_SYNC | IWM_CMD_WANT_SKB); if ((error = iwm_send_cmd(sc, cmd)) != 0) return error; + if (async) + return (0); + pkt = cmd->resp_pkt; /* Can happen if RFKILL is asserted */ @@ -3557,7 +3541,7 @@ iwm_mvm_send_cmd_status(struct iwm_softc int iwm_mvm_send_cmd_pdu_status(struct iwm_softc *sc, uint8_t id, - uint16_t len, const void *data, uint32_t *status) + uint16_t len, const void *data, uint32_t *status, int async) { struct iwm_host_cmd cmd = { .id = id, @@ -3565,7 +3549,7 @@ iwm_mvm_send_cmd_pdu_status(struct iwm_s .data = { data, }, }; - return iwm_mvm_send_cmd_status(sc, &cmd, status); + return iwm_mvm_send_cmd_status(sc, &cmd, status, async); } void @@ -3982,7 +3966,7 @@ iwm_mvm_beacon_filter_send_cmd(struct iw int ret; ret = iwm_mvm_send_cmd_pdu(sc, IWM_REPLY_BEACON_FILTERING_CMD, - IWM_CMD_SYNC, sizeof(struct iwm_beacon_filter_cmd), cmd); + IWM_CMD_ASYNC, sizeof(struct iwm_beacon_filter_cmd), cmd); if (!ret) { DPRINTF(("ba_enable_beacon_abort is: %d\n", @@ -4113,7 +4097,7 @@ iwm_mvm_power_mac_update_mode(struct iwm iwm_mvm_power_log(sc, &cmd); if ((ret = iwm_mvm_send_cmd_pdu(sc, IWM_MAC_PM_POWER_TABLE, - IWM_CMD_SYNC, sizeof(cmd), &cmd)) != 0) + IWM_CMD_ASYNC, sizeof(cmd), &cmd)) != 0) return ret; ba_enable = !!(cmd.flags & @@ -4216,13 +4200,13 @@ iwm_mvm_send_add_sta_cmd_status(struct i if (sc->sc_capaflags & IWM_UCODE_TLV_FLAGS_STA_KEY_CMD) { return iwm_mvm_send_cmd_pdu_status(sc, IWM_ADD_STA, - sizeof(*cmd), cmd, status); + sizeof(*cmd), cmd, status, 1); } iwm_mvm_add_sta_cmd_v6_to_v5(cmd, &cmd_v5); return iwm_mvm_send_cmd_pdu_status(sc, IWM_ADD_STA, sizeof(cmd_v5), - &cmd_v5, status); + &cmd_v5, status, 1); } /* send station add/update command to firmware */ @@ -4231,7 +4215,6 @@ iwm_mvm_sta_send_to_fw(struct iwm_softc { struct iwm_mvm_add_sta_cmd_v6 add_sta_cmd; int ret; - uint32_t status; memset(&add_sta_cmd, 0, sizeof(add_sta_cmd)); @@ -4246,20 +4229,7 @@ iwm_mvm_sta_send_to_fw(struct iwm_softc add_sta_cmd.station_flags_msk |= htole32(IWM_STA_FLG_FAT_EN_MSK | IWM_STA_FLG_MIMO_EN_MSK); - status = IWM_ADD_STA_SUCCESS; - ret = iwm_mvm_send_add_sta_cmd_status(sc, &add_sta_cmd, &status); - if (ret) - return ret; - - switch (status) { - case IWM_ADD_STA_SUCCESS: - break; - default: - ret = EIO; - DPRINTF(("IWM_ADD_STA failed\n")); - break; - } - + ret = iwm_mvm_send_add_sta_cmd_status(sc, &add_sta_cmd, NULL); return ret; } @@ -4287,7 +4257,6 @@ iwm_mvm_add_int_sta_common(struct iwm_so { struct iwm_mvm_add_sta_cmd_v6 cmd; int ret; - uint32_t status; memset(&cmd, 0, sizeof(cmd)); cmd.sta_id = sta->sta_id; @@ -4298,20 +4267,7 @@ iwm_mvm_add_int_sta_common(struct iwm_so if (addr) memcpy(cmd.addr, addr, ETHER_ADDR_LEN); - ret = iwm_mvm_send_add_sta_cmd_status(sc, &cmd, &status); - if (ret) - return ret; - - switch (status) { - case IWM_ADD_STA_SUCCESS: - DPRINTF(("Internal station added.\n")); - return 0; - default: - DPRINTF(("%s: Add internal station failed, status=0x%x\n", - DEVNAME(sc), status)); - ret = EIO; - break; - } + ret = iwm_mvm_send_add_sta_cmd_status(sc, &cmd, NULL); return ret; } @@ -4536,13 +4492,12 @@ iwm_mvm_scan_request(struct iwm_softc *s .id = IWM_SCAN_REQUEST_CMD, .len = { 0, }, .data = { sc->sc_scan_cmd, }, - .flags = IWM_CMD_SYNC, + .flags = IWM_CMD_ASYNC, .dataflags = { IWM_HCMD_DFL_NOCOPY, }, }; struct iwm_scan_cmd *cmd = sc->sc_scan_cmd; int is_assoc = 0; int ret; - uint32_t status; int basic_ssid = !(sc->sc_capaflags & IWM_UCODE_TLV_FLAGS_NO_BASIC_SSID); //lockdep_assert_held(&mvm->mutex); @@ -4601,19 +4556,7 @@ iwm_mvm_scan_request(struct iwm_softc *s (cmd->channel_count * sizeof(struct iwm_scan_channel))); hcmd.len[0] = le16toh(cmd->len); - status = IWM_SCAN_RESPONSE_OK; - ret = iwm_mvm_send_cmd_status(sc, &hcmd, &status); - if (!ret && status == IWM_SCAN_RESPONSE_OK) { - DPRINTF(("Scan request was sent successfully\n")); - } else { - /* - * If the scan failed, it usually means that the FW was unable - * to allocate the time events. Warn on it, but maybe we - * should try to send the command again with different params. - */ - sc->sc_scanband = 0; - ret = EIO; - } + ret = iwm_mvm_send_cmd_status(sc, &hcmd, NULL, 1); return ret; } @@ -4750,7 +4693,7 @@ iwm_mvm_mac_ctxt_cmd_common(struct iwm_s int iwm_mvm_mac_ctxt_send_cmd(struct iwm_softc *sc, struct iwm_mac_ctx_cmd *cmd) { - int ret = iwm_mvm_send_cmd_pdu(sc, IWM_MAC_CONTEXT_CMD, IWM_CMD_SYNC, + int ret = iwm_mvm_send_cmd_pdu(sc, IWM_MAC_CONTEXT_CMD, IWM_CMD_ASYNC, sizeof(*cmd), cmd); if (ret) DPRINTF(("%s: Failed to send MAC context (action:%d): %d\n", @@ -4962,7 +4905,7 @@ iwm_mvm_update_quotas(struct iwm_softc * /* Give the remainder of the session to the first binding */ cmd.quotas[0].quota = htole32(le32toh(cmd.quotas[0].quota) + quota_rem); - ret = iwm_mvm_send_cmd_pdu(sc, IWM_TIME_QUOTA_CMD, IWM_CMD_SYNC, + ret = iwm_mvm_send_cmd_pdu(sc, IWM_TIME_QUOTA_CMD, IWM_CMD_ASYNC, sizeof(cmd), &cmd); if (ret) DPRINTF(("%s: Failed to send quota: %d\n", DEVNAME(sc), ret)); @@ -4984,7 +4927,7 @@ iwm_auth(struct iwm_softc *sc) struct iwm_node *in = (void *)ic->ic_bss; uint32_t duration; uint32_t min_duration; - int error; + int error, i; in->in_assoc = 0; @@ -5025,11 +4968,9 @@ iwm_auth(struct iwm_softc *sc) 100 + in->in_ni.ni_intval); iwm_mvm_protect_session(sc, in, duration, min_duration, 500); - while (sc->sc_auth_prot != 2) { - /* - * well, meh, but if the kernel is sleeping for half a - * second, we have bigger problems - */ + for (i = 0; i < 100; i++) { + if (sc->sc_auth_prot == 2) + break; if (sc->sc_auth_prot == 0) { DPRINTF(("%s: missed auth window!\n", DEVNAME(sc))); return ETIMEDOUT; @@ -5038,8 +4979,10 @@ iwm_auth(struct iwm_softc *sc) sc->sc_auth_prot = 0; return EAUTH; } - tsleep(&sc->sc_auth_prot, 0, "iwmau2", 0); + DELAY(10); } + if (sc->sc_auth_prot != 2) + return (ETIMEDOUT); return 0; } @@ -5065,64 +5008,6 @@ iwm_assoc(struct iwm_softc *sc) return 0; } -int -iwm_release(struct iwm_softc *sc, struct iwm_node *in) -{ - /* - * Ok, so *technically* the proper set of calls for going - * from RUN back to SCAN is: - * - * iwm_mvm_power_mac_disable(sc, in); - * iwm_mvm_mac_ctxt_changed(sc, in); - * iwm_mvm_rm_sta(sc, in); - * iwm_mvm_update_quotas(sc, NULL); - * iwm_mvm_mac_ctxt_changed(sc, in); - * iwm_mvm_binding_remove_vif(sc, in); - * iwm_mvm_mac_ctxt_remove(sc, in); - * - * However, that freezes the device not matter which permutations - * and modifications are attempted. Obviously, this driver is missing - * something since it works in the Linux driver, but figuring out what - * is missing is a little more complicated. Now, since we're going - * back to nothing anyway, we'll just do a complete device reset. - * Up your's, device! - */ - //iwm_mvm_flush_tx_path(sc, 0xf, 1); - iwm_stop_device(sc); - iwm_init_hw(sc); - if (in) - in->in_assoc = 0; - return 0; - -#if 0 - int error; - - iwm_mvm_power_mac_disable(sc, in); - - if ((error = iwm_mvm_mac_ctxt_changed(sc, in)) != 0) { - printf("%s: mac ctxt change fail 1 %d\n", DEVNAME(sc), error); - return error; - } - - if ((error = iwm_mvm_rm_sta(sc, in)) != 0) { - printf("%s: sta remove fail %d\n", DEVNAME(sc), error); - return error; - } - error = iwm_mvm_rm_sta(sc, in); - in->in_assoc = 0; - iwm_mvm_update_quotas(sc, NULL); - if ((error = iwm_mvm_mac_ctxt_changed(sc, in)) != 0) { - printf("%s: mac ctxt change fail 2 %d\n", DEVNAME(sc), error); - return error; - } - iwm_mvm_binding_remove_vif(sc, in); - - iwm_mvm_mac_ctxt_remove(sc, in); - - return error; -#endif -} - struct ieee80211_node * iwm_node_alloc(struct ieee80211com *ic) { @@ -5258,32 +5143,17 @@ iwm_media_change(struct ifnet *ifp) return error; } -void -iwm_newstate_cb(void *wk) +int +iwm_newstate(struct ieee80211com *ic, enum ieee80211_state nstate, int 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_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(("switching state %s->%s\n", + 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); @@ -5294,27 +5164,6 @@ iwm_newstate_cb(void *wk) if (((in = (void *)ic->ic_bss) != NULL)) in->in_assoc = 0; - iwm_release(sc, NULL); - - /* - * It's impossible to directly go RUN->SCAN. If we iwm_release() - * above then the card will be completely reinitialized, - * so the driver must do everything necessary to bring the card - * from INIT to SCAN. - * - * Additionally, upon receiving deauth frame from AP, - * OpenBSD 802.11 stack puts the driver in IEEE80211_S_AUTH - * state. This will also fail with this driver, so bring the FSM - * from IEEE80211_S_RUN to IEEE80211_S_SCAN in this case as well. - */ - 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(("Going INIT->SCAN\n")); - nstate = IEEE80211_S_SCAN; - } } switch (nstate) { @@ -5330,17 +5179,17 @@ iwm_newstate_cb(void *wk) ic->ic_des_esslen != 0, ic->ic_des_essid, ic->ic_des_esslen)) != 0) { printf("%s: could not initiate scan\n", DEVNAME(sc)); - return; + return (error); } ic->ic_state = nstate; iwm_led_blink_start(sc); - return; + return (0); case IEEE80211_S_AUTH: if ((error = iwm_auth(sc)) != 0) { DPRINTF(("%s: could not move to auth state: %d\n", DEVNAME(sc), error)); - return; + return (error); } break; @@ -5349,7 +5198,7 @@ iwm_newstate_cb(void *wk) if ((error = iwm_assoc(sc)) != 0) { DPRINTF(("%s: failed to associate: %d\n", DEVNAME(sc), error)); - return; + return (error); } break; @@ -5357,7 +5206,7 @@ iwm_newstate_cb(void *wk) struct iwm_host_cmd cmd = { .id = IWM_LQ_CMD, .len = { sizeof(in->in_lq), }, - .flags = IWM_CMD_SYNC, + .flags = IWM_CMD_ASYNC, }; in = (struct iwm_node *)ic->ic_bss; @@ -5379,33 +5228,7 @@ iwm_newstate_cb(void *wk) break; } - sc->sc_newstate(ic, nstate, arg); -} - -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; - - task_set(&iwmns->ns_wk, iwm_newstate_cb, iwmns); - task_add(sc->sc_nswq, &iwmns->ns_wk); - - return 0; + return sc->sc_newstate(ic, nstate, arg); } void @@ -5536,7 +5359,7 @@ iwm_allow_mcast(struct iwm_softc *sc) IEEE80211_ADDR_COPY(cmd->bssid, ni->ni_bssid); error = iwm_mvm_send_cmd_pdu(sc, IWM_MCAST_FILTER_CMD, - IWM_CMD_SYNC, size, cmd); + IWM_CMD_ASYNC, size, cmd); free(cmd, M_DEVBUF, size); return error; }
