From: Moritz Buhl Subject: Re: bwfm fixes To: tech@openbsd.org Cc: stsp@stsp.name Date: Thu, 21 Aug 2025 13:55:54 +0200 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);