* [PATCH wireless-next v3] wifi: brcmfmac: fix EXTSAE WPA3 connection failure due to AUTH TX failure
@ 2025-07-09 12:04 Gokul Sivakumar
2025-07-23 9:02 ` Arend van Spriel
0 siblings, 1 reply; 3+ messages in thread
From: Gokul Sivakumar @ 2025-07-09 12:04 UTC (permalink / raw)
To: Arend van Spriel, Johannes Berg; +Cc: linux-wireless, Gokul Sivakumar
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
@@ -5544,8 +5544,8 @@ brcmf_cfg80211_mgmt_tx(struct wiphy *wiphy, struct wireless_dev *wdev,
struct brcmf_fil_action_frame_le *action_frame;
struct brcmf_fil_af_params_le *af_params;
bool ack;
- s32 chan_nr;
- u32 freq;
+ s32 ch;
+ __le32 hw_ch;
brcmf_dbg(TRACE, "Enter\n");
@@ -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);
+ } 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 {
+ af_params->channel = hw_ch;
+ }
+ }
+
af_params->dwell_time = cpu_to_le32(params->wait);
memcpy(action_frame->data, &buf[DOT11_MGMT_HDR_LEN],
le16_to_cpu(action_frame->len));
- brcmf_dbg(TRACE, "Action frame, cookie=%lld, len=%d, freq=%d\n",
- *cookie, le16_to_cpu(action_frame->len), freq);
+ brcmf_dbg(TRACE, "Action frame, cookie=%lld, len=%d, channel=%d\n",
+ *cookie, le16_to_cpu(action_frame->len),
+ le32_to_cpu(af_params->channel));
ack = brcmf_p2p_send_action_frame(cfg, cfg_to_ndev(cfg),
af_params);
cfg80211_mgmt_tx_status(wdev, *cookie, buf, len, ack,
GFP_KERNEL);
+free:
kfree(af_params);
} else {
brcmf_dbg(TRACE, "Unhandled, fc=%04x!!\n", mgmt->frame_control);
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
@@ -112,8 +112,8 @@ int brcmf_cyw_mgmt_tx(struct wiphy *wiphy, struct wireless_dev *wdev,
struct brcmf_cfg80211_vif *vif;
s32 err = 0;
bool ack = false;
- s32 chan_nr;
- u32 freq;
+ s32 ch;
+ __le16 hw_ch;
struct brcmf_mf_params_le *mf_params;
u32 mf_params_len;
s32 ready;
@@ -143,13 +143,20 @@ int brcmf_cyw_mgmt_tx(struct wiphy *wiphy, struct wireless_dev *wdev,
mf_params->len = cpu_to_le16(len - DOT11_MGMT_HDR_LEN);
mf_params->frame_control = mgmt->frame_control;
- 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);
- mf_params->channel = cpu_to_le16(chan_nr);
+ 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;
+ }
+ }
+
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);
@@ -159,7 +166,8 @@ int brcmf_cyw_mgmt_tx(struct wiphy *wiphy, struct wireless_dev *wdev,
brcmf_dbg(TRACE, "Auth frame, cookie=%d, fc=%04x, len=%d, channel=%d\n",
le32_to_cpu(mf_params->packet_id),
le16_to_cpu(mf_params->frame_control),
- le16_to_cpu(mf_params->len), chan_nr);
+ le16_to_cpu(mf_params->len),
+ le16_to_cpu(mf_params->channel));
vif->mgmt_tx_id = le32_to_cpu(mf_params->packet_id);
set_bit(BRCMF_MGMT_TX_SEND_FRAME, &vif->mgmt_tx_status);
@@ -185,6 +193,7 @@ int brcmf_cyw_mgmt_tx(struct wiphy *wiphy, struct wireless_dev *wdev,
tx_status:
cfg80211_mgmt_tx_status(wdev, *cookie, buf, len, ack,
GFP_KERNEL);
+free:
kfree(mf_params);
return err;
}
base-commit: 6b04716cdcac37bdbacde34def08bc6fdb5fc4e2
--
2.43.0
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH wireless-next v3] wifi: brcmfmac: fix EXTSAE WPA3 connection failure due to AUTH TX failure
2025-07-09 12:04 [PATCH wireless-next v3] wifi: brcmfmac: fix EXTSAE WPA3 connection failure due to AUTH TX failure Gokul Sivakumar
@ 2025-07-23 9:02 ` Arend van Spriel
2025-07-23 9:57 ` Gokul Sivakumar
0 siblings, 1 reply; 3+ messages in thread
From: Arend van Spriel @ 2025-07-23 9:02 UTC (permalink / raw)
To: Gokul Sivakumar, Johannes Berg; +Cc: linux-wireless
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));
> + if (err) {
> + bphy_err(drvr,
> + "unable to get current hw channel\n");
> + goto free;
> + }
> + }
af_params->channel = hw_ch;
> 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
> @@ -112,8 +112,8 @@ int brcmf_cyw_mgmt_tx(struct wiphy *wiphy, struct wireless_dev *wdev,
> struct brcmf_cfg80211_vif *vif;
> s32 err = 0;
> bool ack = false;
> - s32 chan_nr;
> - u32 freq;
> + s32 ch;
> + __le16 hw_ch;
> struct brcmf_mf_params_le *mf_params;
> u32 mf_params_len;
> s32 ready;
> @@ -143,13 +143,20 @@ int brcmf_cyw_mgmt_tx(struct wiphy *wiphy, struct wireless_dev *wdev,
> mf_params->len = cpu_to_le16(len - DOT11_MGMT_HDR_LEN);
> mf_params->frame_control = mgmt->frame_control;
>
> - 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);
> - mf_params->channel = cpu_to_le16(chan_nr);
> + 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.
Regards,
Arend
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH wireless-next v3] wifi: brcmfmac: fix EXTSAE WPA3 connection failure due to AUTH TX failure
2025-07-23 9:02 ` Arend van Spriel
@ 2025-07-23 9:57 ` Gokul Sivakumar
0 siblings, 0 replies; 3+ messages in thread
From: Gokul Sivakumar @ 2025-07-23 9:57 UTC (permalink / raw)
To: Arend van Spriel, Johannes Berg
Cc: linux-wireless, brcm80211, wlan-kernel-dev-list, Gokul Sivakumar
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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2025-07-23 9:57 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-07-09 12:04 [PATCH wireless-next v3] wifi: brcmfmac: fix EXTSAE WPA3 connection failure due to AUTH TX failure Gokul Sivakumar
2025-07-23 9:02 ` Arend van Spriel
2025-07-23 9:57 ` Gokul Sivakumar
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox