From: Peter Hessler Subject: Re: small set of qwx bug fixes To: Greg Steuck , tech@openbsd.org Date: Fri, 26 Jun 2026 22:29:26 +0200 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"