From: Mark Kettenis <mark.kettenis@xs4all.nl>
Subject: Re: make qwx(4) flush Rx rings when the interface goes down
To: Stefan Sperling <stsp@stsp.name>
Cc: tech@openbsd.org
Date: Fri, 25 Apr 2025 19:43:56 +0200

> Date: Fri, 25 Apr 2025 18:04:31 +0200
> From: Stefan Sperling <stsp@stsp.name>
> 
> 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.

Still works, but doesn't get rid of the error messages I reported
earlier.

> 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);
>  }
>