Download raw body.
bwfm fixes
Below is an updated diff that addresses your suggestions.
I further stop enqueueing the BWFM_E_EAPOL_MSG messages
and switched the done goto to a simple break.
On Tue, Aug 05, 2025 at 12:00:19PM +0200, Stefan Sperling 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);
still ok?
I would like to get this in as soon as possible to learn about any
regressions.
mbuhl
Index: dev/ic/bwfm.c
===================================================================
RCS file: /cvs/src/sys/dev/ic/bwfm.c,v
diff -u -p -r1.112 bwfm.c
--- dev/ic/bwfm.c 3 Aug 2025 14:25:32 -0000 1.112
+++ dev/ic/bwfm.c 21 Aug 2025 11:40:18 -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,8 +2553,41 @@ 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);
+ break;
+ }
+ 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;
+ }
+ break;
+ case BWFM_E_LINK:
+ if (ntohl(e->msg.status) == BWFM_E_STATUS_SUCCESS &&
+ ntohl(e->msg.reason) == 0)
+ break;
+ goto enqueue;
+ case BWFM_E_EAPOL_MSG:
+ break;
+ default:
+ goto enqueue;
+ }
+
+ m_freem(m);
+ splx(s);
+ return;
+enqueue:
ml_enqueue(&sc->sc_evml, m);
splx(s);
@@ -2631,17 +2666,13 @@ bwfm_rx_event_cb(struct bwfm_softc *sc,
break;
}
case BWFM_E_AUTH:
- if (ntohl(e->msg.status) == BWFM_E_STATUS_SUCCESS &&
+ 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);
break;
case BWFM_E_ASSOC:
- if (ntohl(e->msg.status) == BWFM_E_STATUS_SUCCESS &&
+ 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);
break;
case BWFM_E_DEAUTH:
@@ -2650,9 +2681,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);
Index: dev/pci/if_bwfm_pci.c
===================================================================
RCS file: /cvs/src/sys/dev/pci/if_bwfm_pci.c,v
diff -u -p -r1.77 if_bwfm_pci.c
--- dev/pci/if_bwfm_pci.c 12 Jul 2024 08:33:25 -0000 1.77
+++ dev/pci/if_bwfm_pci.c 21 Aug 2025 09:58:37 -0000
@@ -2369,9 +2369,9 @@ bwfm_pci_intr(void *v)
mask = BWFM_PCI_64_PCIE2REG_MAILBOXMASK_INT_D2H_DB;
if (status & mask) {
+ bwfm_pci_ring_rx(sc, &sc->sc_ctrl_complete, &ml);
bwfm_pci_ring_rx(sc, &sc->sc_rx_complete, &ml);
bwfm_pci_ring_rx(sc, &sc->sc_tx_complete, &ml);
- bwfm_pci_ring_rx(sc, &sc->sc_ctrl_complete, &ml);
if (ifiq_input(&ifp->if_rcv, &ml))
if_rxr_livelocked(&sc->sc_rxbuf_ring);
bwfm fixes