On 07/23, Arend van Spriel wrote:
> On 7/9/2025 2:04 PM, Gokul Sivakumar wrote:
> > From: Ting-Ying Li <tingying.li@xxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxx>
> > Signed-off-by: Gokul Sivakumar <gokulkumar.sivakumar@xxxxxxxxxxxx>
> > ---
> >
> > 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
