From: Stefan Sperling Subject: Re: small set of qwx bug fixes To: tech@openbsd.org Date: Mon, 22 Jun 2026 00:57:06 +0200 On Sun, Jun 21, 2026 at 03:00:52PM +0200, Stefan Sperling wrote: > I found 3 distinct bugs in qwx while looking for causes of qwx instability > across suspend/resume cycles. > Not sure yet if these fixes really make suspend/resume more stable. Across > about a dozen syspend/resume cycles I couldn't trigger problem anymore > with these changes applied. But it might just be down to luck. Some more progress on this. Changes below are as follows and should be applied on top of the previous diff I sent in this thread: i. Dealloc resources via qwx_stop() if we fail to get RUNNING in qwx_init(). Cleanup via qwx_stop() was only performed in case of complete success, i.e. once IFF_RUNNING is set. This explains why I've seen KASSERT failures during down/up once the interface had failed to come up once, during suspend/resume or otherwise. I have verified that qwx_stop() can now be called twice in a row without any bad side effects, and that errors raised during qwx_init() no longer cause problems during subsequent down/up cycles. Such errors might leave the interface in down state after resume even if the interface was marked up before suspend. This is not a new issue and I am not entirely sure why it happens. The driver never clears IFF_UP. Apparently some other layer clears it and doesn't set it again if the driver reports an error from the qwx_ioctl handler if we fail to go up? 2. Remove pointless down/up dance in qwx_media_change(). The main ioctl handler already does the ENETRESET dance. The dance in qwx_media_change was redundant and didn't handle qwx_init() errors correctly either. 3. Ensure that qrtr_server state does not race with device during power-on. 4. Use the ioctl rwlock correctly in qwx_activate(). ok? (on top of previous diff) M sys/dev/ic/qwx.c | 37+ 39- M sys/dev/pci/if_qwx_pci.c | 2+ 1- 2 files changed, 39 insertions(+), 40 deletions(-) commit - 8c088bd41d300a603d5464ec5cf71694983b8b78 commit + 21cf34a2af9cf57c41c269d7de5379c819722831 blob - c2526028507bb5d92bd5d4f6924ac90d01bd51dd blob + 2764b01db995bcb8b2da6a5fd67c8d87ec8dc600 --- sys/dev/ic/qwx.c +++ sys/dev/ic/qwx.c @@ -271,13 +271,13 @@ 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; + 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 +309,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 +413,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 +434,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 +446,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 +475,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 +527,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 +695,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) { @@ -20786,10 +20782,10 @@ 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)) { qwx_qmi_firmware_stop(sc); - - qwx_flush_rx_rings(sc); + qwx_flush_rx_rings(sc); + } sc->ops.stop(sc); qwx_wmi_detach(sc); @@ -27450,11 +27446,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); @@ -27463,12 +27459,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;