From: "requiem." <rqm@0xde501a7a.net>
Subject: Re: bwfm fixes
To: tech@openbsd.org
Date: Wed, 6 Aug 2025 18:05:48 +0100

If there's a patch of the patch coming I'll gladly do some testing on
my A1502 intel Mac with a BCM943602CS in it.

On Tue, 5 Aug 2025 12:00:19 +0200
Stefan Sperling <stsp@stsp.name> wrote:

> On Mon, Aug 04, 2025 at 08:19:08PM +0200, Moritz Buhl wrote:
> > Dear tech,
> > 
> > I currently use a MacBook m2 and I always have trouble with the
> > bwfm in it.
> > 
> > Here are three fixes:
> > 1. receive EAPOL event messages.
> > After debugging this for a few days, I noticed that EAPOL frames
> > do not cause an interrupt and with my macbook the messag either
> > comes with the previous WL_EVENTS that change the link state and
> > finish the ASSOC state, or the firmware crashes about 100ms later.  
> 
> Nice find! I admire your persistence in tracking this down.
> 
> > I can imagine this may also help with crashes in AP mode on other
> > devices.
> > 
> > 2. do ieee80211 state changes inside of the interrupt handler.
> > To my understanding this is in line with outher drivers and it is
> > necessary for the wpa handshake to process it within 100 ms which
> > we could miss because of the additional queueing.
> > 
> > 3. process control messages before rx messages.
> > Otherwise a EAPOL frame could be processed before changing from
> > ASSOC to RUN.
> > 
> > I would appreciate additional testing on other devices.  
> 
> ok stsp@ with some suggestions below:
>  
> > Index: ic/bwfm.c
> > ===================================================================
> > RCS file: /cvs/src/sys/dev/ic/bwfm.c,v
> > diff -u -p -r1.111 bwfm.c
> > --- ic/bwfm.c	19 Feb 2024 21:23:02 -0000	1.111
> > +++ ic/bwfm.c	4 Aug 2025 15:43:12 -0000
> > @@ -494,6 +494,7 @@ bwfm_init(struct ifnet *ifp)
> >  		BWFM_EVENT(BWFM_E_LINK);
> >  		BWFM_EVENT(BWFM_E_AUTH);
> >  		BWFM_EVENT(BWFM_E_ASSOC);
> > +		BWFM_EVENT(BWFM_E_EAPOL_MSG);
> >  		BWFM_EVENT(BWFM_E_DEAUTH);
> >  		BWFM_EVENT(BWFM_E_DISASSOC);
> >  		BWFM_EVENT(BWFM_E_ESCAN_RESULT);
> > @@ -503,6 +504,7 @@ bwfm_init(struct ifnet *ifp)
> >  		BWFM_EVENT(BWFM_E_AUTH_IND);
> >  		BWFM_EVENT(BWFM_E_ASSOC_IND);
> >  		BWFM_EVENT(BWFM_E_REASSOC_IND);
> > +		BWFM_EVENT(BWFM_E_EAPOL_MSG);
> >  		BWFM_EVENT(BWFM_E_DEAUTH_IND);
> >  		BWFM_EVENT(BWFM_E_DISASSOC_IND);
> >  		BWFM_EVENT(BWFM_E_ESCAN_RESULT);
> > @@ -2551,12 +2551,44 @@ void
> >  bwfm_rx_event(struct bwfm_softc *sc, struct mbuf *m)
> >  {
> >  	int s;
> > +	struct ieee80211com *ic = &sc->sc_ic;
> > +	struct bwfm_event *e = mtod(m, void *);
> >  
> >  	s = splnet();
> > +	switch (ntohl(e->msg.event_type)) {
> > +	case BWFM_E_AUTH:
> > +		if (ntohl(e->msg.status) == BWFM_E_STATUS_SUCCESS
> > &&
> > +		    ic->ic_state == IEEE80211_S_AUTH) {
> > +			ieee80211_new_state(ic, IEEE80211_S_ASSOC,
> > -1);
> > +			goto done;
> > +		}
> > +		goto enqueue;
> > +	case BWFM_E_ASSOC:
> > +		if (ntohl(e->msg.status) == BWFM_E_STATUS_SUCCESS
> > &&
> > +		    ic->ic_state == IEEE80211_S_ASSOC) {
> > +			ieee80211_new_state(ic, IEEE80211_S_RUN,
> > -1);
> > +		} else if (ntohl(e->msg.status) !=
> > BWFM_E_STATUS_UNSOLICITED) {
> > +			goto enqueue;
> > +		}
> > +		goto done;
> > +	case BWFM_E_LINK:
> > +		if (ntohl(e->msg.status) == BWFM_E_STATUS_SUCCESS
> > &&
> > +		    ntohl(e->msg.reason) == 0)
> > +			goto done;
> > +		goto enqueue;
> > +	default:
> > +		goto enqueue;
> > +	}
> > +done:
> > +	splx(s);
> > +	m_freem(m);
> > +	return;
> > +enqueue:
> >  	ml_enqueue(&sc->sc_evml, m);
> >  	splx(s);
> >  
> >  	task_add(sc->sc_taskq, &sc->sc_task);
> > +	return;  
> 
> Nitpick: The above 'return' is not strictly needed.
> 
> >  }
> >  
> >  void
> > @@ -2631,18 +2663,10 @@ bwfm_rx_event_cb(struct bwfm_softc *sc, 
> >  		break;
> >  		}
> >  	case BWFM_E_AUTH:
> > -		if (ntohl(e->msg.status) == BWFM_E_STATUS_SUCCESS
> > &&
> > -		    ic->ic_state == IEEE80211_S_AUTH)
> > -			ieee80211_new_state(ic, IEEE80211_S_ASSOC,
> > -1);
> > -		else
> > -			ieee80211_begin_scan(ifp);
> > +		ieee80211_begin_scan(ifp);  
> 
> This code handles the case where authentication to the AP has failed
> and we need to try again (and potentially try a different AP).
> This would be much more obvious if the begin_scan() call was guarded
> by an appropriate condition. For example:
> 
> 		if (ntohl(e->msg.status) != BWFM_E_STATUS_SUCCESS) &&
> 		    ic->ic_state == IEEE80211_S_AUTH)
> 			ieee80211_begin_scan(ifp);
> 
> >  		break;
> >  	case BWFM_E_ASSOC:
> > -		if (ntohl(e->msg.status) == BWFM_E_STATUS_SUCCESS
> > &&
> > -		    ic->ic_state == IEEE80211_S_ASSOC)
> > -			ieee80211_new_state(ic, IEEE80211_S_RUN,
> > -1);
> > -		else if (ntohl(e->msg.status) !=
> > BWFM_E_STATUS_UNSOLICITED)
> > -			ieee80211_begin_scan(ifp);
> > +		ieee80211_begin_scan(ifp);  
> 
> Same here:
> 		if (ntohl(e->msg.status) != BWFM_E_STATUS_SUCCESS) &&
> 		    ic->ic_state == IEEE80211_S_ASSOC)
> 			ieee80211_begin_scan(ifp);
> 
> >  		break;
> >  	case BWFM_E_DEAUTH:
> >  	case BWFM_E_DISASSOC:
> > @@ -2650,9 +2674,6 @@ bwfm_rx_event_cb(struct bwfm_softc *sc, 
> >  			ieee80211_begin_scan(ifp);
> >  		break;
> >  	case BWFM_E_LINK:
> > -		if (ntohl(e->msg.status) == BWFM_E_STATUS_SUCCESS
> > &&
> > -		    ntohl(e->msg.reason) == 0)
> > -			break;
> >  		/* Link status has changed */
> >  		if (ic->ic_state > IEEE80211_S_SCAN)
> >  			ieee80211_begin_scan(ifp);  
>