From: Stefan Sperling Subject: Re: make qwx(4) flush Rx rings when the interface goes down To: Mark Kettenis , tech@openbsd.org Date: Fri, 25 Apr 2025 18:04:31 +0200 On Fri, Apr 25, 2025 at 05:22:10PM +0200, Stefan Sperling wrote: > On Fri, Apr 25, 2025 at 12:23:25AM +0200, Mark Kettenis wrote: > > Sadly this doesn't fix the panic I'm seeing on the x13s. > > > > What I'm doing to provoke the panic is: > > > > 1. Log in to the machine over ssh > > > > 2. Run 'ifconfig qwx0 down' > > > > 3. Pick up the laptop > > > > 4. Run 'ifconfig qwx0 up' > > > > There seems to be something really fishy going on with bringing the > > interface down when there are packets in flight... > > Thanks Mark! I could reproduce the crash on Z13 by following your recipe. > > This diff, applied on top of the previous diff, seems to fix it for me. > Can you confirm? Here is a slightly improved version, which does not leave us hanging in a non-INIT state in case the task code runs into some error case. If a failure occurs, all we can do is what we did previously, forcefully moving to INIT state in the kernel and hoping for the best. And we must cancel the init_task which is scheduled in case of failure. I've not seen any such errors occur during testing, but they are not an impossible scenario. M sys/dev/ic/qwx.c | 23+ 3- 1 file changed, 23 insertions(+), 3 deletions(-) commit - 733c622d5b43cf595e1a2ddfe07326238879463a commit + d60a72e819acb4ac27fccdfde4bb06814eb39e49 blob - fcb17633d81d7f0734e5f38ac655d446d0659a33 blob + 782c41f10b6a1c6b22d074dc873f12bb770345f6 --- sys/dev/ic/qwx.c +++ sys/dev/ic/qwx.c @@ -341,8 +341,24 @@ qwx_stop(struct ifnet *ifp) ifp->if_flags &= ~IFF_RUNNING; ifq_clr_oactive(&ifp->if_snd); - sc->sc_newstate(ic, IEEE80211_S_INIT, -1); - sc->ns_nstate = IEEE80211_S_INIT; + /* + * 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) { + 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); + qwx_newstate_task(sc); + if (ic->ic_state != IEEE80211_S_INIT) { /* task code failed */ + task_del(systq, &sc->init_task); + sc->sc_newstate(ic, IEEE80211_S_INIT, -1); + } + refcnt_finalize(&sc->task_refs, "qwxstop"); + } + sc->scan.state = ATH11K_SCAN_IDLE; sc->vdev_id_11d_scan = QWX_11D_INVALID_VDEV_ID; sc->pdevs_active = 0; @@ -899,6 +915,9 @@ qwx_newstate_task(void *arg) } /* FALLTHROUGH */ case IEEE80211_S_SCAN: + if (nstate < IEEE80211_S_SCAN) + qwx_scan_abort(sc); + break; case IEEE80211_S_INIT: break; } @@ -954,7 +973,8 @@ out: task_add(systq, &sc->init_task); else sc->sc_newstate(ic, nstate, sc->ns_arg); - } + } else if (err == 0) + sc->sc_newstate(ic, nstate, sc->ns_arg); refcnt_rele_wake(&sc->task_refs); splx(s); }