Re: [PATCH] wifi: brcmfmac: cyw: clean up PMKID and cookie code

[Gokul Sivakumar]

On 07/14, Bogdan Nicolae wrote:
> Hi Arend,
> 
> The code already sets *cookie=0 in the beginning, which according to
> my understanding tells the driver it does not require async tracking
> upstream. This is correct, because the function calls
> wait_for_completion_timeout, so mgmt tx completes by the time we exit
> the function and doesn't need to be tracked upstream. However, while
> we are waiting for mgmt tx to finish, the firmware may generate
> non-matching completion events captured by notify_mgmt_tx_status.
> That's why setting a non-zero packed_id (sent to and returned by
> firmware as part of a completion event) and checking against
> vif->mgmt_tx_id is important.

Avoid top-posting. Need to use interleaved style.
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

> Cheers,
> Bogdan
> 
> On Sun, Jul 12, 2026 at 5:30 PM Arend van Spriel
> <arend.vanspriel@xxxxxxxxxxxx> wrote:
> >
> > On 09/07/2026 14:23, Bogdan Nicolae wrote:
> > > Avoid setting packet_id to cookie, which is always 0. Instead, use an
> > > increasing atomic counter. Avoids mismatches of completion events later
> > > in brcmf_notify_mgmt_tx_status, where packet_id != vif->mgmt_tx_id is
> > > checked.
> > >
> > > Also, zero out auth_status on initialization. Otherwise, garbage will
> > > leak from the stack to the firmware (when bssid is less than 32 bytes

BSSID will always be less than 32, you must have meant to say "ssid" here
instead of "bssid". Also the auth_req_status struct does not have any "bssid"
member, it only has an "ssid" struct member. Need to fix in commit message.

> > > and/or when params->pmkid is set). Then, pass the params->pmkid to the
> > > firmware (without it, the firmware caches a garbage PMKID on successful
> > > authentication and denies a subsequent association request that includes
> > > the PMKID).
> > >
> > > Signed-off-by: Bogdan Nicolae <bogdan.nicolae@xxxxxxx>
> >
> > I always get a bit confused when people use different email addresses to
> > send and sign-off patches.
> >    > ---
> > >   .../net/wireless/broadcom/brcm80211/brcmfmac/cyw/core.c   | 8 ++++++--
> > >   1 file changed, 6 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cyw/core.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cyw/core.c
> > > index ce09d44fa..cca53ff19 100644
> > > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cyw/core.c
> > > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cyw/core.c
> > > @@ -23,6 +23,8 @@
> > >   #define MGMT_AUTH_FRAME_DWELL_TIME  4000
> > >   #define MGMT_AUTH_FRAME_WAIT_TIME   (MGMT_AUTH_FRAME_DWELL_TIME + 100)
> > >
> > > +static atomic_t brcmf_cyw_mgmt_tx_id = ATOMIC_INIT(0);
> > > +
> > >   static int brcmf_cyw_set_sae_pwd(struct brcmf_if *ifp,
> > >                                struct cfg80211_crypto_settings *crypto)
> > >   {
> > > @@ -155,7 +157,7 @@ int brcmf_cyw_mgmt_tx(struct wiphy *wiphy, struct wireless_dev *wdev,
> > >
> > >       memcpy(&mf_params->da[0], &mgmt->da[0], ETH_ALEN);
> > >       memcpy(&mf_params->bssid[0], &mgmt->bssid[0], ETH_ALEN);
> > > -     mf_params->packet_id = cpu_to_le32(*cookie);
> > > +     mf_params->packet_id = cpu_to_le32(atomic_inc_return(&brcmf_cyw_mgmt_tx_id));
> >
> > As I understand things the cookie value here is an output parameter. The
> > driver should assign it:
> >
> > +       *cookie = atomic_inc_return(&brcmf_cyw_mgmt_tx_id);
> >         mf_params->packet_id = cpu_to_le32(*cookie);

Agree with Arend's suggestion here. It is not correct to leave *cookie = 0 as it
is and only assign a different packet_id while sending requst to firmware, because
in that case the driver will not return the real cookie for this TX to cfg80211.
Driver should assign the generated ID to *cookie and then copy that into packet_id.


Gokul

> > >       memcpy(mf_params->data, &buf[DOT11_MGMT_HDR_LEN],
> > >              le16_to_cpu(mf_params->len));
> > >
> > Regards,
> > Arend
>