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

[Arend van Spriel <arend.vanspriel@broadcom.com>]

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