Re: [PATCH 4/5] wifi: brcmfmac: Support bss_info up to v112

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

[-- Attachment #1: Type: text/plain, Size: 3027 bytes --]

On 10/19/2023 3:42 AM, Daniel Berlin wrote:
> From: Hector Martin <marcan@marcan.st>
> 
> The structures are compatible and just add fields, so we can just treat
> it as always v112. If we start using new fields, that will have to be
> gated on the version.

Seems EHT is creeping in here.

Having doubts about compatibility statement (see below)...

> Signed-off-by: Hector Martin <marcan@marcan.st>
> ---
>   .../broadcom/brcm80211/brcmfmac/cfg80211.c    |  5 ++-
>   .../broadcom/brcm80211/brcmfmac/fwil_types.h  | 37 +++++++++++++++++--
>   2 files changed, 36 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> index 4cf728368892..bc8355d7f9b5 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> @@ -3496,8 +3496,9 @@ static s32 brcmf_inform_bss(struct brcmf_cfg80211_info *cfg)
>   
>   	bss_list = (struct brcmf_scan_results *)cfg->escan_info.escan_buf;
>   	if (bss_list->count != 0 &&
> -	    bss_list->version != BRCMF_BSS_INFO_VERSION) {
> -		bphy_err(drvr, "Version %d != WL_BSS_INFO_VERSION\n",
> +	    (bss_list->version < BRCMF_BSS_INFO_MIN_VERSION ||
> +	    bss_list->version > BRCMF_BSS_INFO_MAX_VERSION)) {
> +		bphy_err(drvr, "BSS info version %d unsupported\n",
>   			 bss_list->version);
>   		return -EOPNOTSUPP;
>   	}
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil_types.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil_types.h
> index 1077e6f1d61a..81f2d77cb004 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil_types.h
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil_types.h
> @@ -18,7 +18,8 @@
>   #define BRCMF_ARP_OL_HOST_AUTO_REPLY	0x00000004
>   #define BRCMF_ARP_OL_PEER_AUTO_REPLY	0x00000008
>   
> -#define	BRCMF_BSS_INFO_VERSION	109 /* curr ver of brcmf_bss_info_le struct */
> +#define	BRCMF_BSS_INFO_MIN_VERSION	109 /* min ver of brcmf_bss_info_le struct */
> +#define	BRCMF_BSS_INFO_MAX_VERSION	112 /* max ver of brcmf_bss_info_le struct */
>   #define BRCMF_BSS_RSSI_ON_CHANNEL	0x0004
>   
>   #define BRCMF_STA_BRCM			0x00000001	/* Running a Broadcom driver */
> @@ -323,28 +324,56 @@ struct brcmf_bss_info_le {
>   	__le16 capability;	/* Capability information */
>   	u8 SSID_len;
>   	u8 SSID[32];
> +	u8 bcnflags;		/* additional flags w.r.t. beacon */

Ehm. Coming back to your statement "structures are compatible and just 
add fields". How are they compatible? You now treat v109 struct as v112 
so fields below are shifted because of bcnflags. So you read invalid 
information. This does not fly or I am missing something here.

>   	struct {
>   		__le32 count;   /* # rates in this set */
>   		u8 rates[16]; /* rates in 500kbps units w/hi bit set if basic */
>   	} rateset;		/* supported rates */
>   	__le16 chanspec;	/* chanspec for bss */
>   	__le16 atim_window;	/* units are Kusec */

[...]

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4219 bytes --]