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

On Tue, Jul 14, 2026 at 8:03 AM Gokul Sivakumar
<[email protected]> wrote:
>
> 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
> > <[email protected]> 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.

Fixed

> > > > 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 <[email protected]>
> > >
> > > 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.

Fixed

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

Regards,
Bogdan