Index | Thread | Search

From:
Stefan Sperling <stsp@stsp.name>
Subject:
qwx fixes (was: Re: qwz: enable WPA2 association on WCN7850)
To:
Marcus Glocker <marcus@nazgul.ch>, tech@openbsd.org, Mark Kettenis <mark.kettenis@xs4all.nl>, "Kirill A. Korinsky" <kirill@korins.ky>, Patrick Wildt <mail@patrick-wildt.de>
Date:
Sun, 26 Apr 2026 13:15:54 +0200

Download raw body.

Thread
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?

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);