Re: small set of qwx bug fixes

On 2026 Jun 23 (Tue) at 11:16:31 +0200 (+0200), Stefan Sperling wrote:
: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.
:

been running with this on my X13s for a couple days, all solid.

OK


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

-- 
Bizoos, n.:
	The millions of tiny individual bumps that make up a
basketball.
		-- Rich Hall, "Sniglets"