Re: [PATCH wireless-next v3] wifi: brcmfmac: fix EXTSAE WPA3 connection failure due to AUTH TX failure

[Gokul Sivakumar <gokulkumar.sivakumar@infineon.com>]

On 07/23, Arend van Spriel wrote:
> On 7/9/2025 2:04 PM, Gokul Sivakumar wrote:
> > From: Ting-Ying Li <tingying.li@cypress.com>
> > 
> > For WPA3-SAE Connection in EXTSAE mode, the userspace daemon is allowed to
> > generate the SAE Auth frames. The driver uses the "mgmt_frame" FW IOVAR to
> > transmit this MGMT frame.
> > 
> > Before sending the IOVAR, the Driver is incorrectly treating the channel
> > number read from the FW as a frequency value and again attempts to convert
> > this into a channel number using ieee80211_frequency_to_channel().
> > 
> > This added an invalid channel number as part of the IOVAR request to the FW
> > And some FW which strictly expects a valid channel would return BAD_CHAN
> > error, while failing to transmit the driver requested SAE Auth MGMT frame.
> > 
> > Fix this in the CYW vendor specific MGMT TX cfg80211 ops handler, by not
> > treating the channel number read from the FW as frequency value and skip
> > the attempt to convert it again into a channel number.
> > 
> > Also fix this in the generic MGMT TX cfg80211 ops handler.
> > 
> > Fixes: c2ff8cad6423 ("brcm80211: make mgmt_tx in brcmfmac accept a NULL channel")
> > Fixes: 66f909308a7c ("wifi: brcmfmac: cyw: support external SAE authentication in station mode")
> > Signed-off-by: Ting-Ying Li <tingying.li@cypress.com>
> > Signed-off-by: Gokul Sivakumar <gokulkumar.sivakumar@infineon.com>
> > ---
> > 
> > v3:
> >    * Fixed the "warning: incorrect type in assignment (different base types)"
> >      properly now, after kernel test robot reported it again.
> > 
> >    * Used brcmf_fil_cmd_data_get() instead of brcmf_fil_cmd_int_get() util
> >      for reading the channel number from the firmware as __le32 / __le16
> >      type instead of s32 type.
> > 
> > v2:
> >    * Fixed wifibot "warning: incorrect type in assignment (different base types)"
> >      in cyw/core.c file.
> > 
> >    * Fixed >80 line length checkpatch warning by reducing variable name len
> >      in cfg80211.c file.
> > 
> >    * Handled the return value of the BRCMF_C_GET_CHANNEL IOCTL Read operation
> >      in cfg80211.c & cyw/core.c files.
> > 
> >   .../broadcom/brcm80211/brcmfmac/cfg80211.c    | 33 ++++++++++++-------
> >   .../broadcom/brcm80211/brcmfmac/cyw/core.c    | 29 ++++++++++------
> >   2 files changed, 41 insertions(+), 21 deletions(-)
> > 
> > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> > index 40a9a8177de6..54b1f0c8117e 100644
> > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> 
> [...]
> 
> > @@ -5606,25 +5606,36 @@ brcmf_cfg80211_mgmt_tx(struct wiphy *wiphy, struct wireless_dev *wdev,
> >               /* Add the channel. Use the one specified as parameter if any or
> >                * the current one (got from the firmware) otherwise
> >                */
> > -             if (chan)
> > -                     freq = chan->center_freq;
> > -             else
> > -                     brcmf_fil_cmd_int_get(vif->ifp, BRCMF_C_GET_CHANNEL,
> > -                                           &freq);
> > -             chan_nr = ieee80211_frequency_to_channel(freq);
> > -             af_params->channel = cpu_to_le32(chan_nr);
> > +             if (chan) {
> > +                     ch = ieee80211_frequency_to_channel(chan->center_freq);
> > +                     af_params->channel = cpu_to_le32(ch);
> 
> When we have the chan instance we can simply do following instead:
> 
>                        af_params->channel = cpu_to_le32(chan->hw_value);
> 
> > +             } else {
> > +                     err = brcmf_fil_cmd_data_get(vif->ifp,
> > +                                                  BRCMF_C_GET_CHANNEL,
> > +                                                  &hw_ch, sizeof(hw_ch));
> 
> I understand the motivation to use brcmf_fil_cmd_data_get() here, but it
> may confuse people reading the code. So how about this incorporating the
> previous comment:
> 
>                if (chan) {
>                        hw_ch = cpu_to_le32(chan->hw_value);
>                } else {
>                        err = brcmf_fil_cmd_data_get(vif->ifp,
>                                                     BRCMF_C_GET_CHANNEL,
>                                                     &hw_ch, sizeof(hw_ch));

yeah, this suggestion looks clean. Also the local variable "hw_ch" can replaced
directly with "af_params->channel".

> > +                     if (err) {
> > +                             bphy_err(drvr,
> > +                                      "unable to get current hw channel\n");
> > +                             goto free;
> > +                     }
> > +             }
>                af_params->channel = hw_ch;

So that the above line can be removed. Will update and send a v4 patch.

> >               af_params->dwell_time = cpu_to_le32(params->wait);
> >               memcpy(action_frame->data, &buf[DOT11_MGMT_HDR_LEN],
> >                      le16_to_cpu(action_frame->len));
> 
> [...]
> 
> > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cyw/core.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cyw/core.c
> > index c9537fb597ce..2cbb4a809ca7 100644
> > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cyw/core.c
> > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cyw/core.c

[...]

> > +     if (chan) {
> > +             ch = ieee80211_frequency_to_channel(chan->center_freq);
> > +             mf_params->channel = cpu_to_le16(ch);
> > +     } else {
> > +             err = brcmf_fil_cmd_data_get(vif->ifp, BRCMF_C_GET_CHANNEL,
> > +                                          &hw_ch, sizeof(hw_ch));
> > +             if (err) {
> > +                     bphy_err(drvr, "unable to get current hw channel\n");
> > +                     goto free;
> > +             } else {
> > +                     mf_params->channel = hw_ch;
> > +             }
> > +     }
> > +
> 
> proposing similar construct here.

Will update here as well in v4 patch.

Gokul