Download raw body.
qwx fixes (was: Re: qwz: enable WPA2 association on WCN7850)
qwx fixes (was: Re: qwz: enable WPA2 association on WCN7850)
> Date: Sun, 26 Apr 2026 13:15:54 +0200
> From: Stefan Sperling <stsp@stsp.name>
>
> On Sun, Apr 26, 2026 at 07:56:10AM +0200, Stefan Sperling wrote:
> > I will try out your change to see how qwx behaves with it.
>
> This patch ports the most obvious (to me) fixes from your qwz diff
> to qwx:
>
> - Set WMI_PEER_AUTHORIZE only after the WPA handshake completes.
> However, we should be testing this on unencrypted networks as well.
> I suspect your patch broke that case. My diff is still sending
> WMI_PEER_AUTHORIZE to the firmware as before if WPA/RSN is disabled,
> cause otherwise we would never send it on non-WPA networks.
>
> - Init the REO queue for the non-QoS TID. I have also adjusted the
> constant used for sizing the peer's rx_tid array in the header file.
>
> - Interrupt vector fix in qwx_pcic_ext_irq_config.
>
> - Assoc ID fix.
>
> Two additional bugs I'm investigating here are:
>
> I've seen my laptop crash in the qwx newstate task upon resuming from
> hibernation, where the task was trying to move to AUTH state.
> qwx_stop() is called when suspending and it removes the newstate task
> from its task queue if already scheduled. However, there is a window
> where FLAG_CRASH_FLUSH is unset while IFF_RUNNING is still set.
> If we enter qwx_newstate() in this window for some reason (e.g. due to
> frames received in interrupt context), the newstate task might get
> scheduled again. I'm not sure yet if this change fixes the crash I
> saw, but closing the window where the flags are set badly cannot hurt.
>
> The second issue is that we never reset the MHI ring's "queued" counter
> even across ifconfig down/up. I suspect this can eventually cause kernel
> or firmware panics when the interface goes down and back up over time.
> Reset the counter in qwx_mhi_init_cmd_ring since the command ring is
> cleared at this point and should be considered empty from there on.
>
> Ok for all or some of this?
Works for me on the AMD x13g4. As I said, the vector code is slightly
dodgy, although looking at the qwx(4) version makes me understand
things a little bit better, and I think the fix is going in the right
direction.
So basically that code is trying to handle mapping M interrupt
handlers on N available MSI vectors, where M might be larger than N.
One example is the case where N=1 that we struggled with early on and
decided we wouldn't support, at least for now. There are hardware
specific configs that divy up the available vectors across consumers,
including configs for hardware that only has 16 MSI vectors. I don't
think we actually support that particular hardware. But there are
some clues in the code that even with 32 MSI vectors the driver might
want to install more than 32 interrupt vectors. In qwxvar.h we have:
#define ATH11K_IRQ_NUM_MAX 52
and if we look at the individual consumers we have in qwxreg.h:
#define CE_COUNT_MAX 12
but if you look at the configs in if_qwx_pci.c we see:
{ .name = "CE", .num_vectors = 10, .base_vector = 3 },
that said, the CE configs for the PCIe hardware variants in qwx.c all
seem to use only 9 CE engines. So this probably is fine.
It is a bit less clear for the DP interrupts. But with 18 vectors, I
suspect we won't run out either with the currently supported hardware.
I suspect the code it just needlessly complicated to cater for
non-PCIe hardware.
So this is fine.
P.S.
If you see any
splassert: uvm_mapent_free: want 0 have 4
messages, those are fallout from the bounce buffer diff that I
committed recently. Those are related to bus_dmamap_destroy() calls
from interrupt context. The man page doesn't say if that's allowed or
not. But it seems like a bad idea to me. So I'll have to look into
how widespread the use of bus_dmamap_destroy() from interrupt context
actually is.
> M sys/dev/ic/qwx.c | 29+ 15-
> M sys/dev/ic/qwxvar.h | 1+ 1-
> M sys/dev/pci/if_qwx_pci.c | 3+ 1-
>
> 3 files changed, 33 insertions(+), 17 deletions(-)
>
> commit - 9ce5d767a9ce105eb003f5e22b4e7bcb1da0c6ea
> commit + bf705e227e1fb1fca5555eb22c68dcd9268e4193
> blob - 333da499f818b16c992fbff9251463a35b7b7097
> blob + b5bd277bd1bae458de12f4cb5feac0e2e64c1204
> --- sys/dev/ic/qwx.c
> +++ sys/dev/ic/qwx.c
> @@ -417,8 +417,6 @@ qwx_stop(struct ifnet *ifp)
> qwx_del_task(sc, systq, &sc->bgscan_task);
> refcnt_finalize(&sc->task_refs, "qwxstop");
>
> - clear_bit(ATH11K_FLAG_CRASH_FLUSH, sc->sc_flags);
> -
> if (ic->ic_opmode == IEEE80211_M_STA &&
> ic->ic_state == IEEE80211_S_RUN &&
> (ic->ic_bss->ni_flags & IEEE80211_NODE_MFP))
> @@ -431,6 +429,8 @@ qwx_stop(struct ifnet *ifp)
> ifp->if_flags &= ~IFF_RUNNING;
> ifq_clr_oactive(&ifp->if_snd);
>
> + clear_bit(ATH11K_FLAG_CRASH_FLUSH, sc->sc_flags);
> +
> /*
> * Manually run the newstate task's code for switching to INIT state.
> * This reconfigures firmware state to stop scanning, or disassociate
> @@ -910,8 +910,20 @@ qwx_add_sta_key(struct qwx_softc *sc, struct ieee80211
> nq->flags |= QWX_NODE_FLAG_HAVE_PAIRWISE_KEY;
>
> if ((nq->flags & want_keymask) == want_keymask) {
> + uint8_t pdev_id = 0; /* TODO: derive pdev ID somehow? */
> +
> DPRINTF("marking port %s valid\n",
> ether_sprintf(ni->ni_macaddr));
> +
> + /* 4-way handshake done; authorize the BSS peer now. */
> + ret = qwx_wmi_set_peer_param(sc, ni->ni_macaddr, arvif->vdev_id,
> + pdev_id, WMI_PEER_AUTHORIZE, 1);
> + if (ret) {
> + printf("%s: unable to authorize BSS peer: %d\n",
> + sc->sc_dev.dv_xname, ret);
> + return ret;
> + }
> +
> ni->ni_port_valid = 1;
> ieee80211_set_link_state(ic, LINK_STATE_UP);
> }
> @@ -1033,7 +1045,7 @@ qwx_clear_pn_replay_config(struct qwx_softc *sc, struc
> HAL_REO_CMD_UPD0_PN_CHECK |
> HAL_REO_CMD_UPD0_SVLD;
>
> - for (tid = 0; tid < IEEE80211_NUM_TID; tid++) {
> + for (tid = 0; tid <= HAL_DESC_REO_NON_QOS_TID; tid++) {
> rx_tid = &peer->rx_tid[tid];
> if (!rx_tid->active)
> continue;
> @@ -24702,7 +24714,7 @@ qwx_peer_frags_flush(struct qwx_softc *sc, struct ath1
> #ifdef notyet
> lockdep_assert_held(&ar->ab->base_lock);
> #endif
> - for (i = 0; i < IEEE80211_NUM_TID; i++) {
> + for (i = 0; i <= HAL_DESC_REO_NON_QOS_TID; i++) {
> rx_tid = &peer->rx_tid[i];
>
> qwx_dp_rx_frags_cleanup(sc, rx_tid, 1);
> @@ -24722,7 +24734,7 @@ qwx_peer_rx_tid_cleanup(struct qwx_softc *sc, struct a
> #ifdef notyet
> lockdep_assert_held(&ar->ab->base_lock);
> #endif
> - for (i = 0; i < IEEE80211_NUM_TID; i++) {
> + for (i = 0; i <= HAL_DESC_REO_NON_QOS_TID; i++) {
> rx_tid = &peer->rx_tid[i];
>
> qwx_peer_rx_tid_delete(sc, peer, i);
> @@ -24949,7 +24961,7 @@ qwx_dp_peer_setup(struct qwx_softc *sc, int vdev_id, i
> return ret;
> }
>
> - for (tid = 0; tid < IEEE80211_NUM_TID; tid++) {
> + for (tid = 0; tid <= HAL_DESC_REO_NON_QOS_TID; tid++) {
> ret = qwx_peer_rx_tid_setup(sc, ni, vdev_id, pdev_id,
> tid, 1, 0, HAL_PN_TYPE_NONE);
> if (ret) {
> @@ -25032,7 +25044,7 @@ qwx_dp_peer_rx_pn_replay_config(struct qwx_softc *sc,
> if (peer == NULL)
> return EINVAL;
>
> - for (tid = 0; tid < IEEE80211_NUM_TID; tid++) {
> + for (tid = 0; tid <= HAL_DESC_REO_NON_QOS_TID; tid++) {
> rx_tid = &peer->rx_tid[tid];
> if (!rx_tid->active)
> continue;
> @@ -26189,7 +26201,7 @@ qwx_peer_assoc_h_basic(struct qwx_softc *sc, struct qw
>
> IEEE80211_ADDR_COPY(arg->peer_mac, ni->ni_macaddr);
> arg->vdev_id = arvif->vdev_id;
> - arg->peer_associd = ni->ni_associd;
> + arg->peer_associd = IEEE80211_AID(ni->ni_associd);
> arg->auth_flag = 1;
> arg->peer_listen_intval = ni->ni_intval;
> arg->peer_nss = 1;
> @@ -26627,7 +26639,7 @@ qwx_assoc(struct qwx_softc *sc)
> WARN_ON(arvif->is_up);
> #endif
>
> - arvif->aid = ni->ni_associd;
> + arvif->aid = IEEE80211_AID(ni->ni_associd);
> IEEE80211_ADDR_COPY(arvif->bssid, ni->ni_bssid);
> sc->bss_peer_id = nq->peer_id;
>
> @@ -26666,12 +26678,14 @@ qwx_run(struct qwx_softc *sc)
> DNPRINTF(QWX_D_MAC, "%s: vdev %d up (associated) bssid %s aid %d\n",
> __func__, arvif->vdev_id, ether_sprintf(ni->ni_bssid), arvif->aid);
>
> - ret = qwx_wmi_set_peer_param(sc, ni->ni_macaddr, arvif->vdev_id,
> - pdev_id, WMI_PEER_AUTHORIZE, 1);
> - if (ret) {
> - printf("%s: unable to authorize BSS peer: %d\n",
> - sc->sc_dev.dv_xname, ret);
> - return ret;
> + if ((ic->ic_flags & IEEE80211_F_RSNON) == 0 || ni->ni_port_valid) {
> + ret = qwx_wmi_set_peer_param(sc, ni->ni_macaddr, arvif->vdev_id,
> + pdev_id, WMI_PEER_AUTHORIZE, 1);
> + if (ret) {
> + printf("%s: unable to authorize BSS peer: %d\n",
> + sc->sc_dev.dv_xname, ret);
> + return ret;
> + }
> }
>
> return 0;
> blob - 1d0699c9aec0fe9f6ed45a564cadda2a93de6937
> blob + d0fff9701bfccdfbb380757b05f32ce24988acff
> --- sys/dev/ic/qwxvar.h
> +++ sys/dev/ic/qwxvar.h
> @@ -1790,7 +1790,7 @@ struct ath11k_peer {
> /* protected by ab->data_lock */
> struct ieee80211_key_conf *keys[WMI_MAX_KEY_INDEX + 1];
> #endif
> - struct dp_rx_tid rx_tid[IEEE80211_NUM_TID + 1];
> + struct dp_rx_tid rx_tid[HAL_DESC_REO_NON_QOS_TID + 1];
> #if 0
> /* peer id based rhashtable list pointer */
> struct rhash_head rhash_id;
> blob - e8c86c7809bebb4b78b1ecb1ed39e0d4bd08b8bc
> blob + 7037646d93e1cf829a33510700f0e8f3654adf2f
> --- sys/dev/pci/if_qwx_pci.c
> +++ sys/dev/pci/if_qwx_pci.c
> @@ -1650,9 +1650,10 @@ qwx_pcic_ext_irq_config(struct qwx_softc *sc, struct p
>
> if (num_irq) {
> int irq_idx = irq_grp->irqs[0];
> + int vector = (i % num_vectors) + base_vector;
> pci_intr_handle_t ih;
>
> - if (pci_intr_map_msivec(pa, irq_idx, &ih) != 0 &&
> + if (pci_intr_map_msivec(pa, vector, &ih) != 0 &&
> pci_intr_map(pa, &ih) != 0) {
> printf("%s: can't map interrupt\n",
> sc->sc_dev.dv_xname);
> @@ -2539,6 +2540,7 @@ qwx_mhi_init_cmd_ring(struct qwx_pci_softc *psc)
> len = ring->size;
>
> ring->rp = ring->wp = paddr;
> + ring->queued = 0;
>
> c = (struct qwx_mhi_cmd_ctxt *)QWX_DMA_KVA(psc->cmd_ctxt);
> c->rbase = htole64(paddr);
>
qwx fixes (was: Re: qwz: enable WPA2 association on WCN7850)
qwx fixes (was: Re: qwz: enable WPA2 association on WCN7850)