Re: small set of qwx bug fixes

On Mon, Jun 22, 2026 at 01:43:57PM -0700, Greg Steuck wrote:
> resume in -current to ~reliable. I got bored after 16 wakeups and the
> running ping recovering with no manual intervention. Ship it!

There is a regression with one of the changes. I've seen mcl2k pool
corruption checks trigger again after about a day of running the previous
diff. I worked hard on preventing pool curruption some months ago, and
seeing it return is annoying.

I might have found the reason: The previous diff skipped calling
qwx_flush_rx_rings() in some cases. This function is our crude way of
preventing firmware from doing more Rx, by making all Rx rings empty.
If we don't call this function then firmware might still be doing DMA
on a ring we have freed, since the ring pointers it has cached imply
non-empty rings.

Please test this version, which always keeps calling qwx_flush_rx_rings()
unless loading firmware failed. These panics don't trigger very easily.
Having several users of the 2k mbuf cluster pool helps to trigger them,
e.g. when using ethernet and wifi in parallel.

If no crashes occur within a few days of use, this can be considered solid.

M  sys/dev/ic/qwx.c          |  67+  52-
M  sys/dev/pci/if_qwx_pci.c  |   2+   1-

2 files changed, 69 insertions(+), 53 deletions(-)

commit - 06943c8222484abb6aad11c1b1d5f752922ba0d1
commit + bb95bd9ab9390805516cf7407ab355934bcfa93d
blob - 4c744b205a09dec658faed98adb3e4e7a7f557be
blob + 77c7504ab632b53f0cfc75d09e8d2a4038c990a5
--- sys/dev/ic/qwx.c
+++ sys/dev/ic/qwx.c
@@ -271,13 +271,16 @@ qwx_init(struct ifnet *ifp)
 	sc->scan.state = ATH11K_SCAN_IDLE;
 	sc->vdev_id_11d_scan = QWX_11D_INVALID_VDEV_ID;
 
-	error = qwx_core_init(sc);
-	if (error)
-		return error;
-
 	memset(&sc->qrtr_server, 0, sizeof(sc->qrtr_server));
 	sc->qrtr_server.node = QRTR_NODE_BCAST;
 
+	/* This flag will be cleared if firmware starts up successfully. */
+	set_bit(ATH11K_FLAG_QMI_FAIL, sc->sc_flags);
+
+	error = qwx_core_init(sc);
+	if (error)
+		return error;
+
 	/* wait for QRTR init to be done */
 	while (sc->qrtr_server.node == QRTR_NODE_BCAST) {
 		error = tsleep_nsec(&sc->qrtr_server, 0, "qwxqrtr",
@@ -309,19 +312,18 @@ qwx_init(struct ifnet *ifp)
 			    sc->sc_dev.dv_xname, ether_sprintf(ic->ic_myaddr),
 			    error);
 
-		ieee80211_media_init(ifp, qwx_media_change,
+		ieee80211_media_init(ifp, ieee80211_media_change,
 		    ieee80211_media_status);
 	}
 
 	if (ifp->if_flags & IFF_UP) {
-		refcnt_init(&sc->task_refs);
-
-		ifq_clr_oactive(&ifp->if_snd);
-
 		error = qwx_mac_start(sc);
 		if (error)
 			return error;
 
+		refcnt_init(&sc->task_refs);
+		ifq_clr_oactive(&ifp->if_snd);
+
 		ifp->if_flags |= IFF_RUNNING;
 		sc->ops.irq_enable(sc);
 		ieee80211_begin_scan(ifp);
@@ -414,10 +416,12 @@ qwx_stop(struct ifnet *ifp)
 	struct qwx_softc *sc = ifp->if_softc;
 	struct ieee80211com *ic = &sc->sc_ic;
 	int s = splnet();
+	int was_running;
 
 	rw_assert_wrlock(&sc->ioctl_rwl);
 
-	if (ic->ic_opmode == IEEE80211_M_STA &&
+	was_running = (ifp->if_flags & IFF_RUNNING) != 0;
+	if (was_running && ic->ic_opmode == IEEE80211_M_STA &&
 	    ic->ic_state == IEEE80211_S_RUN &&
 	    (ic->ic_bss->ni_flags & IEEE80211_NODE_MFP) &&
 	    ic->ic_bss->ni_port_valid)
@@ -433,7 +437,8 @@ qwx_stop(struct ifnet *ifp)
 	/* Cancel scheduled tasks and let any stale tasks finish up. */
 	task_del(systq, &sc->init_task);
 	qwx_del_task_all(sc);
-	refcnt_finalize(&sc->task_refs, "qwxstop");
+	if (was_running)
+		refcnt_finalize(&sc->task_refs, "qwxstop");
 
 	ifp->if_timer = sc->sc_tx_timer = 0;
 
@@ -444,20 +449,21 @@ qwx_stop(struct ifnet *ifp)
 	sc->bgscan_unref_arg = NULL;
 	sc->bgscan_unref_arg_size = 0;
 
-	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
 	 * from our current AP, and/or stop the VIF, etc.
 	 */
-	if (ic->ic_state != IEEE80211_S_INIT) {
+	if (was_running && ic->ic_state != IEEE80211_S_INIT) {
 		sc->ns_nstate = IEEE80211_S_INIT;
 		sc->ns_arg = -1; /* do not send management frames */
 		refcnt_init(&sc->task_refs);
 		refcnt_take(&sc->task_refs);
+		clear_bit(ATH11K_FLAG_CRASH_FLUSH, sc->sc_flags);
 		qwx_newstate_task(sc);
-		if (ic->ic_state != IEEE80211_S_INIT) { /* task code failed */
+		set_bit(ATH11K_FLAG_CRASH_FLUSH, sc->sc_flags);
+		if (ic->ic_state != IEEE80211_S_INIT) {
+			/* task code failed */
 			task_del(systq, &sc->init_task);
 			sc->sc_newstate(ic, IEEE80211_S_INIT, -1);
 		}
@@ -472,7 +478,14 @@ qwx_stop(struct ifnet *ifp)
 	sc->vdev_id_11d_scan = QWX_11D_INVALID_VDEV_ID;
 	sc->pdevs_active = 0;
 
-	/* power off hardware */
+	/*
+	 * If we were running then allow commands to be sent to
+	 * firmware during core_deinit().
+	 */
+	if (was_running)
+		clear_bit(ATH11K_FLAG_CRASH_FLUSH, sc->sc_flags);
+
+	/* free some DMA allocations and power off hardware */
 	qwx_core_deinit(sc);
 
 	qwx_vif_free_all(sc);
@@ -517,6 +530,10 @@ qwx_ioctl(struct ifnet *ifp, u_long cmd, caddr_t data)
 				/* Force reload of firmware image from disk. */
 				qwx_free_firmware(sc);
 				err = qwx_init(ifp);
+				if (err) {
+					KASSERT(!(ifp->if_flags & IFF_RUNNING));
+					qwx_stop(ifp);
+				}
 			}
 		} else {
 			if (ifp->if_flags & IFF_RUNNING)
@@ -681,24 +698,6 @@ qwx_watchdog(struct ifnet *ifp)
 }
 
 int
-qwx_media_change(struct ifnet *ifp)
-{
-	int err;
-
-	err = ieee80211_media_change(ifp);
-	if (err != ENETRESET)
-		return err;
-
-	if ((ifp->if_flags & (IFF_UP | IFF_RUNNING)) ==
-	    (IFF_UP | IFF_RUNNING)) {
-		qwx_stop(ifp);
-		err = qwx_init(ifp);
-	}
-
-	return err;
-}
-
-int
 qwx_queue_setkey_cmd(struct ieee80211com *ic, struct ieee80211_node *ni,
     struct ieee80211_key *k, int cmd)
 {
@@ -15353,6 +15352,9 @@ qwx_dp_rxdma_ring_buf_setup(struct qwx_softc *sc,
 	if (rx_ring->rx_data == NULL)
 		return ENOMEM;
 
+	rx_ring->bufs_max = num_entries;
+	memset(rx_ring->freemap, 0xff, sizeof(rx_ring->freemap));
+
 	for (i = 0; i < num_entries; i++) {
 		struct qwx_rx_data *rx_data = &rx_ring->rx_data[i];
 
@@ -15361,9 +15363,6 @@ qwx_dp_rxdma_ring_buf_setup(struct qwx_softc *sc,
 			return ENOMEM;
 	}
 
-	rx_ring->bufs_max = num_entries;
-	memset(rx_ring->freemap, 0xff, sizeof(rx_ring->freemap));
-
 	return qwx_dp_rxbufs_replenish(sc, dp->mac_id, rx_ring, num_entries,
 	    sc->hw_params.hal_params->rx_buf_rbm);
 }
@@ -20786,11 +20785,13 @@ qwx_flush_rx_rings(struct qwx_softc *sc)
 void
 qwx_core_stop(struct qwx_softc *sc)
 {
-	if (!test_bit(ATH11K_FLAG_CRASH_FLUSH, sc->sc_flags))
+	if (!test_bit(ATH11K_FLAG_CRASH_FLUSH, sc->sc_flags) &&
+	    !test_bit(ATH11K_FLAG_QMI_FAIL, sc->sc_flags))
 		qwx_qmi_firmware_stop(sc);
-	
-	qwx_flush_rx_rings(sc);
 
+	if (!test_bit(ATH11K_FLAG_QMI_FAIL, sc->sc_flags))
+		qwx_flush_rx_rings(sc);
+
 	sc->ops.stop(sc);
 	qwx_wmi_detach(sc);
 	qwx_dp_pdev_reo_cleanup(sc);
@@ -20921,7 +20922,8 @@ qwx_core_qmi_firmware_ready(struct qwx_softc *sc)
 	default:
 		printf("%s: invalid crypto_mode: %d\n",
 		    sc->sc_dev.dv_xname, sc->crypto_mode);
-		return EINVAL;
+		ret = EINVAL;
+		goto err_dp_free;
 	}
 
 	if (sc->frame_mode == ATH11K_HW_TXRX_RAW)
@@ -20970,7 +20972,7 @@ err_firmware_stop:
 	return ret;
 }
 
-void
+int
 qwx_qmi_fw_init_done(struct qwx_softc *sc)
 {
 	int ret = 0;
@@ -20984,10 +20986,15 @@ qwx_qmi_fw_init_done(struct qwx_softc *sc)
 		clear_bit(ATH11K_FLAG_RECOVERY, sc->sc_flags);
 		ret = qwx_core_qmi_firmware_ready(sc);
 		if (ret) {
+			/*
+			 * This flags tells qwx_stop() that core is stopped
+			 * and ATH11K_FIRMWARE_MODE_OFF was already sent.
+			 */
 			set_bit(ATH11K_FLAG_QMI_FAIL, sc->sc_flags);
-			return;
 		}
 	}
+
+	return ret;
 }
 
 int
@@ -21047,8 +21054,7 @@ qwx_qmi_event_server_arrive(struct qwx_softc *sc)
 		}
 	}
 
-	qwx_qmi_fw_init_done(sc);
-	return 0;
+	return qwx_qmi_fw_init_done(sc);
 }
 
 int
@@ -21577,11 +21583,8 @@ qwx_hal_free_cont_wrp(struct qwx_softc *sc)
 int
 qwx_hal_srng_init(struct qwx_softc *sc)
 {
-	struct ath11k_hal *hal = &sc->hal;
 	int ret;
 
-	memset(hal, 0, sizeof(*hal));
-
 	ret = qwx_hal_srng_create_config(sc);
 	if (ret)
 		goto err_hal;
@@ -22661,7 +22664,8 @@ qwx_ce_cleanup_pipes(struct qwx_softc *sc)
 		qwx_ce_rx_pipe_cleanup(pipe);
 
 		/* Cleanup any src CE's which have interrupts disabled */
-		qwx_ce_poll_send_completed(sc, pipe_num);
+		if (!test_bit(ATH11K_FLAG_QMI_FAIL, sc->sc_flags))
+			qwx_ce_poll_send_completed(sc, pipe_num);
 	}
 }
 
@@ -24323,8 +24327,17 @@ qwx_init_task(void *arg)
 	struct qwx_softc *sc = arg;
 	struct ifnet *ifp = &sc->sc_ic.ic_if;
 	int s = splnet();
-	rw_enter_write(&sc->ioctl_rwl);
 
+	/*
+	 * Do not sleep for this lock. The init task is a one-shot
+	 * recovery mechanism. If the ioctl handler is busy then
+	 * we are being reconfigured or reset already.
+	 */
+	if (rw_enter(&sc->ioctl_rwl, RW_WRITE | RW_NOSLEEP) != 0) {
+		splx(s);
+		return;
+	}
+
 	if (ifp->if_flags & IFF_RUNNING)
 		qwx_stop(ifp);
 
@@ -27444,11 +27457,11 @@ qwx_activate(struct device *self, int act)
 
 	switch (act) {
 	case DVACT_QUIESCE:
+		rw_enter_write(&sc->ioctl_rwl);
 		if (ifp->if_flags & IFF_RUNNING) {
-			rw_enter_write(&sc->ioctl_rwl);
 			qwx_stop(ifp);
-			rw_exit(&sc->ioctl_rwl);
 		}
+		rw_exit(&sc->ioctl_rwl);
 		break;
 	case DVACT_RESUME:
 		err = qwx_hal_srng_init(sc);
@@ -27457,12 +27470,14 @@ qwx_activate(struct device *self, int act)
 			    sc->sc_dev.dv_xname);
 		break;
 	case DVACT_WAKEUP:
+		rw_enter_write(&sc->ioctl_rwl);
 		if ((ifp->if_flags & (IFF_UP | IFF_RUNNING)) == IFF_UP) {
 			err = qwx_init(ifp);
 			if (err)
 				printf("%s: could not initialize hardware\n",
 				    sc->sc_dev.dv_xname);
 		}
+		rw_exit(&sc->ioctl_rwl);
 		break;
 	}
 
blob - a0e416a3cafea6c49646852c11c9fa9045b71ce2
blob + 00da7a38928ecf5dee26f3b73a8eeb10ca0fd04c
--- sys/dev/pci/if_qwx_pci.c
+++ sys/dev/pci/if_qwx_pci.c
@@ -1113,7 +1113,8 @@ unsupported_wcn6855_soc:
 	memcpy(ifp->if_xname, sc->sc_dev.dv_xname, IFNAMSIZ);
 	if_attach(ifp);
 	ieee80211_ifattach(ifp);
-	ieee80211_media_init(ifp, qwx_media_change, ieee80211_media_status);
+	ieee80211_media_init(ifp, ieee80211_media_change,
+	    ieee80211_media_status);
 
 	ic->ic_node_alloc = qwx_node_alloc;