Re: [PATCH 1/5] [brcmfmac] Add support for encoding/decoding 6g chanspecs

Re: [PATCH 1/5] [brcmfmac] Add support for encoding/decoding 6g chanspecs

[Arend van Spriel]

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

On 10/19/2023 3:42 AM, Daniel Berlin wrote:
> This patch adds support for 6G chanspecs, as part of adding 6G and
> 802.11ax support.
> 
> I added the correct values for the upcoming 320mhz as well so that the
> info is complete.

Reviewed-by: Arend van Spriel <arend.vanspriel@broadcom.com>> 
Signed-off-by: Daniel Berlin <dberlin@dberlin.org>
> ---
>   .../broadcom/brcm80211/brcmutil/d11.c         | 46 +++++++++++++++----
>   .../broadcom/brcm80211/include/brcmu_d11.h    | 46 +++++++++++++------
>   .../broadcom/brcm80211/include/brcmu_wifi.h   | 27 ++++++++---
>   3 files changed, 89 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmutil/d11.c b/drivers/net/wireless/broadcom/brcm80211/brcmutil/d11.c
> index 1e2b1e487eb7..faf7eeeeb2d5 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmutil/d11.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmutil/d11.c

[...]

> @@ -117,7 +127,9 @@ static void brcmu_d11n_decchspec(struct brcmu_chan *ch)
>   		}
>   		break;
>   	default:
> -		WARN_ONCE(1, "Invalid chanspec 0x%04x\n", ch->chspec);
> +		WARN_ONCE(1,
> +			  "Invalid chanspec - unknown 11n bandwidth 0x%04x\n",
> +			  ch->chspec);

I don't think this provides more info. The WARN_ONCE() already prints 
code location and stack trace.

>   		break;
>   	}
>   
> @@ -129,7 +141,8 @@ static void brcmu_d11n_decchspec(struct brcmu_chan *ch)
>   		ch->band = BRCMU_CHAN_BAND_2G;
>   		break;
>   	default:
> -		WARN_ONCE(1, "Invalid chanspec 0x%04x\n", ch->chspec);
> +		WARN_ONCE(1, "Invalid chanspec - unknown 11n band 0x%04x\n",
> +			  ch->chspec);

ditto

>   		break;
>   	}
>   }
> @@ -156,7 +169,9 @@ static void brcmu_d11ac_decchspec(struct brcmu_chan *ch)
>   			ch->sb = BRCMU_CHAN_SB_U;
>   			ch->control_ch_num += CH_10MHZ_APART;
>   		} else {
> -			WARN_ONCE(1, "Invalid chanspec 0x%04x\n", ch->chspec);
> +			WARN_ONCE(1,
> +				  "Invalid chanspec - unknown 11ac channel distance 0x%04x\n",
> +				  ch->chspec);

ditto

>   		}
>   		break;
>   	case BRCMU_CHSPEC_D11AC_BW_80:
> @@ -177,7 +192,9 @@ static void brcmu_d11ac_decchspec(struct brcmu_chan *ch)
>   			ch->control_ch_num += CH_30MHZ_APART;
>   			break;
>   		default:
> -			WARN_ONCE(1, "Invalid chanspec 0x%04x\n", ch->chspec);
> +			WARN_ONCE(1,
> +				  "Invalid chanspec - unknown 11ac channel distance 0x%04x\n",
> +				  ch->chspec);

ditto

>   			break;
>   		}
>   		break;
> @@ -211,17 +228,24 @@ static void brcmu_d11ac_decchspec(struct brcmu_chan *ch)
>   			ch->control_ch_num += CH_70MHZ_APART;
>   			break;
>   		default:
> -			WARN_ONCE(1, "Invalid chanspec 0x%04x\n", ch->chspec);
> +			WARN_ONCE(1,
> +				  "Invalid chanspec - unknown 11ac channel distance 0x%04x\n",
> +				  ch->chspec);

ditto

>   			break;
>   		}
>   		break;
>   	case BRCMU_CHSPEC_D11AC_BW_8080:
>   	default:
> -		WARN_ONCE(1, "Invalid chanspec 0x%04x\n", ch->chspec);
> +		WARN_ONCE(1,
> +			  "Invalid chanspec - unknown 11ac channel bandwidth 0x%04x\n",
> +			  ch->chspec);
>   		break;
>   	}
>   
>   	switch (ch->chspec & BRCMU_CHSPEC_D11AC_BND_MASK) {
> +	case BRCMU_CHSPEC_D11AC_BND_6G:
> +		ch->band = BRCMU_CHAN_BAND_6G;
> +		break;
>   	case BRCMU_CHSPEC_D11AC_BND_5G:
>   		ch->band = BRCMU_CHAN_BAND_5G;
>   		break;

[...]

> diff --git a/drivers/net/wireless/broadcom/brcm80211/include/brcmu_d11.h b/drivers/net/wireless/broadcom/brcm80211/include/brcmu_d11.h
> index f6344023855c..bb48b7442062 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/include/brcmu_d11.h
> +++ b/drivers/net/wireless/broadcom/brcm80211/include/brcmu_d11.h
> @@ -69,24 +69,44 @@
>   #define  BRCMU_CHSPEC_D11AC_SB_UU	BRCMU_CHSPEC_D11AC_SB_LUU
>   #define  BRCMU_CHSPEC_D11AC_SB_L	BRCMU_CHSPEC_D11AC_SB_LLL
>   #define  BRCMU_CHSPEC_D11AC_SB_U	BRCMU_CHSPEC_D11AC_SB_LLU
> +/* channel sideband indication for frequency >= 240MHz */
> +#define BRCMU_CHSPEC_D11AC_320_SB_MASK	0x0780
> +#define BRCMU_CHSPEC_D11AC_320_SB_SHIFT	7
> +#define BRCMU_CHSPEC_D11AC_SB_LLLL	0x0000
> +#define BRCMU_CHSPEC_D11AC_SB_LLLU	0x0080
> +#define BRCMU_CHSPEC_D11AC_SB_LLUL	0x0100
> +#define BRCMU_CHSPEC_D11AC_SB_LLUU	0x0180
> +#define BRCMU_CHSPEC_D11AC_SB_LULL	0x0200
> +#define BRCMU_CHSPEC_D11AC_SB_LULU	0x0280
> +#define BRCMU_CHSPEC_D11AC_SB_LUUL	0x0300
> +#define BRCMU_CHSPEC_D11AC_SB_LUUU	0x0380
> +#define BRCMU_CHSPEC_D11AC_SB_ULLL	0x0400
> +#define BRCMU_CHSPEC_D11AC_SB_ULLU	0x0480
> +#define BRCMU_CHSPEC_D11AC_SB_ULUL	0x0500
> +#define BRCMU_CHSPEC_D11AC_SB_ULUU	0x0580
> +#define BRCMU_CHSPEC_D11AC_SB_UULL	0x0600
> +#define BRCMU_CHSPEC_D11AC_SB_UULU	0x0680
> +#define BRCMU_CHSPEC_D11AC_SB_UUUL	0x0700
> +#define BRCMU_CHSPEC_D11AC_SB_UUUU	0x0780

These are WCC specific, but I think it is okay to have these definitions 
here.

>   #define BRCMU_CHSPEC_D11AC_BW_MASK	0x3800
>   #define BRCMU_CHSPEC_D11AC_BW_SHIFT	11
> -#define  BRCMU_CHSPEC_D11AC_BW_5	0x0000
> -#define  BRCMU_CHSPEC_D11AC_BW_10	0x0800
> -#define  BRCMU_CHSPEC_D11AC_BW_20	0x1000
> -#define  BRCMU_CHSPEC_D11AC_BW_40	0x1800
> -#define  BRCMU_CHSPEC_D11AC_BW_80	0x2000
> -#define  BRCMU_CHSPEC_D11AC_BW_160	0x2800
> -#define  BRCMU_CHSPEC_D11AC_BW_8080	0x3000
> -#define BRCMU_CHSPEC_D11AC_BND_MASK	0xc000
> -#define BRCMU_CHSPEC_D11AC_BND_SHIFT	14
> -#define  BRCMU_CHSPEC_D11AC_BND_2G	0x0000
> -#define  BRCMU_CHSPEC_D11AC_BND_3G	0x4000
> -#define  BRCMU_CHSPEC_D11AC_BND_4G	0x8000
> -#define  BRCMU_CHSPEC_D11AC_BND_5G	0xc000
> +#define BRCMU_CHSPEC_D11AC_BW_10    0x0800
> +#define BRCMU_CHSPEC_D11AC_BW_20    0x1000
> +#define BRCMU_CHSPEC_D11AC_BW_40    0x1800
> +#define BRCMU_CHSPEC_D11AC_BW_80    0x2000
> +#define BRCMU_CHSPEC_D11AC_BW_160   0x2800
> +#define BRCMU_CHSPEC_D11AC_BW_320   0x0000

320MHz is 802.11be. No need to add this already, but not a biggy.

> +#define BRCMU_CHSPEC_D11AC_BW_8080  0x3000
> +#define BRCMU_CHSPEC_D11AC_BND_MASK 0xc000
> +#define BRCMU_CHSPEC_D11AC_BND_SHIFT 14
> +#define BRCMU_CHSPEC_D11AC_BND_2G   0x0000
> +#define BRCMU_CHSPEC_D11AC_BND_4G   0x8000
> +#define BRCMU_CHSPEC_D11AC_BND_5G   0xc000
> +#define BRCMU_CHSPEC_D11AC_BND_6G   0x4000
>   
>   #define BRCMU_CHAN_BAND_2G		0
>   #define BRCMU_CHAN_BAND_5G		1
> +#define BRCMU_CHAN_BAND_6G		2
>   
>   enum brcmu_chan_bw {
>   	BRCMU_CHAN_BW_20,


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

Re: [PATCH 1/5] [brcmfmac] Add support for encoding/decoding 6g chanspecs

[Daniel Berlin]

I'll remove the extra prints, and the 320MHZ.

On Fri, Oct 20, 2023 at 5:57 AM Arend van Spriel
<arend.vanspriel@broadcom.com> wrote:
>
> On 10/19/2023 3:42 AM, Daniel Berlin wrote:
> > This patch adds support for 6G chanspecs, as part of adding 6G and
> > 802.11ax support.
> >
> > I added the correct values for the upcoming 320mhz as well so that the
> > info is complete.
>
> Reviewed-by: Arend van Spriel <arend.vanspriel@broadcom.com>>
> Signed-off-by: Daniel Berlin <dberlin@dberlin.org>
> > ---
> >   .../broadcom/brcm80211/brcmutil/d11.c         | 46 +++++++++++++++----
> >   .../broadcom/brcm80211/include/brcmu_d11.h    | 46 +++++++++++++------
> >   .../broadcom/brcm80211/include/brcmu_wifi.h   | 27 ++++++++---
> >   3 files changed, 89 insertions(+), 30 deletions(-)
> >
> > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmutil/d11.c b/drivers/net/wireless/broadcom/brcm80211/brcmutil/d11.c
> > index 1e2b1e487eb7..faf7eeeeb2d5 100644
> > --- a/drivers/net/wireless/broadcom/brcm80211/brcmutil/d11.c
> > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmutil/d11.c
>
> [...]
>
> > @@ -117,7 +127,9 @@ static void brcmu_d11n_decchspec(struct brcmu_chan *ch)
> >               }
> >               break;
> >       default:
> > -             WARN_ONCE(1, "Invalid chanspec 0x%04x\n", ch->chspec);
> > +             WARN_ONCE(1,
> > +                       "Invalid chanspec - unknown 11n bandwidth 0x%04x\n",
> > +                       ch->chspec);
>
> I don't think this provides more info. The WARN_ONCE() already prints
> code location and stack trace.
>
> >               break;
> >       }
> >
> > @@ -129,7 +141,8 @@ static void brcmu_d11n_decchspec(struct brcmu_chan *ch)
> >               ch->band = BRCMU_CHAN_BAND_2G;
> >               break;
> >       default:
> > -             WARN_ONCE(1, "Invalid chanspec 0x%04x\n", ch->chspec);
> > +             WARN_ONCE(1, "Invalid chanspec - unknown 11n band 0x%04x\n",
> > +                       ch->chspec);
>
> ditto
>
> >               break;
> >       }
> >   }
> > @@ -156,7 +169,9 @@ static void brcmu_d11ac_decchspec(struct brcmu_chan *ch)
> >                       ch->sb = BRCMU_CHAN_SB_U;
> >                       ch->control_ch_num += CH_10MHZ_APART;
> >               } else {
> > -                     WARN_ONCE(1, "Invalid chanspec 0x%04x\n", ch->chspec);
> > +                     WARN_ONCE(1,
> > +                               "Invalid chanspec - unknown 11ac channel distance 0x%04x\n",
> > +                               ch->chspec);
>
> ditto
>
> >               }
> >               break;
> >       case BRCMU_CHSPEC_D11AC_BW_80:
> > @@ -177,7 +192,9 @@ static void brcmu_d11ac_decchspec(struct brcmu_chan *ch)
> >                       ch->control_ch_num += CH_30MHZ_APART;
> >                       break;
> >               default:
> > -                     WARN_ONCE(1, "Invalid chanspec 0x%04x\n", ch->chspec);
> > +                     WARN_ONCE(1,
> > +                               "Invalid chanspec - unknown 11ac channel distance 0x%04x\n",
> > +                               ch->chspec);
>
> ditto
>
> >                       break;
> >               }
> >               break;
> > @@ -211,17 +228,24 @@ static void brcmu_d11ac_decchspec(struct brcmu_chan *ch)
> >                       ch->control_ch_num += CH_70MHZ_APART;
> >                       break;
> >               default:
> > -                     WARN_ONCE(1, "Invalid chanspec 0x%04x\n", ch->chspec);
> > +                     WARN_ONCE(1,
> > +                               "Invalid chanspec - unknown 11ac channel distance 0x%04x\n",
> > +                               ch->chspec);
>
> ditto
>
> >                       break;
> >               }
> >               break;
> >       case BRCMU_CHSPEC_D11AC_BW_8080:
> >       default:
> > -             WARN_ONCE(1, "Invalid chanspec 0x%04x\n", ch->chspec);
> > +             WARN_ONCE(1,
> > +                       "Invalid chanspec - unknown 11ac channel bandwidth 0x%04x\n",
> > +                       ch->chspec);
> >               break;
> >       }
> >
> >       switch (ch->chspec & BRCMU_CHSPEC_D11AC_BND_MASK) {
> > +     case BRCMU_CHSPEC_D11AC_BND_6G:
> > +             ch->band = BRCMU_CHAN_BAND_6G;
> > +             break;
> >       case BRCMU_CHSPEC_D11AC_BND_5G:
> >               ch->band = BRCMU_CHAN_BAND_5G;
> >               break;
>
> [...]
>
> > diff --git a/drivers/net/wireless/broadcom/brcm80211/include/brcmu_d11.h b/drivers/net/wireless/broadcom/brcm80211/include/brcmu_d11.h
> > index f6344023855c..bb48b7442062 100644
> > --- a/drivers/net/wireless/broadcom/brcm80211/include/brcmu_d11.h
> > +++ b/drivers/net/wireless/broadcom/brcm80211/include/brcmu_d11.h
> > @@ -69,24 +69,44 @@
> >   #define  BRCMU_CHSPEC_D11AC_SB_UU   BRCMU_CHSPEC_D11AC_SB_LUU
> >   #define  BRCMU_CHSPEC_D11AC_SB_L    BRCMU_CHSPEC_D11AC_SB_LLL
> >   #define  BRCMU_CHSPEC_D11AC_SB_U    BRCMU_CHSPEC_D11AC_SB_LLU
> > +/* channel sideband indication for frequency >= 240MHz */
> > +#define BRCMU_CHSPEC_D11AC_320_SB_MASK       0x0780
> > +#define BRCMU_CHSPEC_D11AC_320_SB_SHIFT      7
> > +#define BRCMU_CHSPEC_D11AC_SB_LLLL   0x0000
> > +#define BRCMU_CHSPEC_D11AC_SB_LLLU   0x0080
> > +#define BRCMU_CHSPEC_D11AC_SB_LLUL   0x0100
> > +#define BRCMU_CHSPEC_D11AC_SB_LLUU   0x0180
> > +#define BRCMU_CHSPEC_D11AC_SB_LULL   0x0200
> > +#define BRCMU_CHSPEC_D11AC_SB_LULU   0x0280
> > +#define BRCMU_CHSPEC_D11AC_SB_LUUL   0x0300
> > +#define BRCMU_CHSPEC_D11AC_SB_LUUU   0x0380
> > +#define BRCMU_CHSPEC_D11AC_SB_ULLL   0x0400
> > +#define BRCMU_CHSPEC_D11AC_SB_ULLU   0x0480
> > +#define BRCMU_CHSPEC_D11AC_SB_ULUL   0x0500
> > +#define BRCMU_CHSPEC_D11AC_SB_ULUU   0x0580
> > +#define BRCMU_CHSPEC_D11AC_SB_UULL   0x0600
> > +#define BRCMU_CHSPEC_D11AC_SB_UULU   0x0680
> > +#define BRCMU_CHSPEC_D11AC_SB_UUUL   0x0700
> > +#define BRCMU_CHSPEC_D11AC_SB_UUUU   0x0780
>
> These are WCC specific, but I think it is okay to have these definitions
> here.
>
> >   #define BRCMU_CHSPEC_D11AC_BW_MASK  0x3800
> >   #define BRCMU_CHSPEC_D11AC_BW_SHIFT 11
> > -#define  BRCMU_CHSPEC_D11AC_BW_5     0x0000
> > -#define  BRCMU_CHSPEC_D11AC_BW_10    0x0800
> > -#define  BRCMU_CHSPEC_D11AC_BW_20    0x1000
> > -#define  BRCMU_CHSPEC_D11AC_BW_40    0x1800
> > -#define  BRCMU_CHSPEC_D11AC_BW_80    0x2000
> > -#define  BRCMU_CHSPEC_D11AC_BW_160   0x2800
> > -#define  BRCMU_CHSPEC_D11AC_BW_8080  0x3000
> > -#define BRCMU_CHSPEC_D11AC_BND_MASK  0xc000
> > -#define BRCMU_CHSPEC_D11AC_BND_SHIFT 14
> > -#define  BRCMU_CHSPEC_D11AC_BND_2G   0x0000
> > -#define  BRCMU_CHSPEC_D11AC_BND_3G   0x4000
> > -#define  BRCMU_CHSPEC_D11AC_BND_4G   0x8000
> > -#define  BRCMU_CHSPEC_D11AC_BND_5G   0xc000
> > +#define BRCMU_CHSPEC_D11AC_BW_10    0x0800
> > +#define BRCMU_CHSPEC_D11AC_BW_20    0x1000
> > +#define BRCMU_CHSPEC_D11AC_BW_40    0x1800
> > +#define BRCMU_CHSPEC_D11AC_BW_80    0x2000
> > +#define BRCMU_CHSPEC_D11AC_BW_160   0x2800
> > +#define BRCMU_CHSPEC_D11AC_BW_320   0x0000
>
> 320MHz is 802.11be. No need to add this already, but not a biggy.
>
> > +#define BRCMU_CHSPEC_D11AC_BW_8080  0x3000
> > +#define BRCMU_CHSPEC_D11AC_BND_MASK 0xc000
> > +#define BRCMU_CHSPEC_D11AC_BND_SHIFT 14
> > +#define BRCMU_CHSPEC_D11AC_BND_2G   0x0000
> > +#define BRCMU_CHSPEC_D11AC_BND_4G   0x8000
> > +#define BRCMU_CHSPEC_D11AC_BND_5G   0xc000
> > +#define BRCMU_CHSPEC_D11AC_BND_6G   0x4000
> >
> >   #define BRCMU_CHAN_BAND_2G          0
> >   #define BRCMU_CHAN_BAND_5G          1
> > +#define BRCMU_CHAN_BAND_6G           2
> >
> >   enum brcmu_chan_bw {
> >       BRCMU_CHAN_BW_20,
>

Re: [PATCH 1/5] [brcmfmac] Add support for encoding/decoding 6g chanspecs

[Jeff Johnson]

On 10/20/2023 2:57 AM, Arend van Spriel wrote:
> On 10/19/2023 3:42 AM, Daniel Berlin wrote:
>> This patch adds support for 6G chanspecs, as part of adding 6G and
>> 802.11ax support.

In ath drivers we are actively discouraging the use of "G" since 1) that 
is not the SI unit for gigahertz, and 2) it conflicts with the use of 
that term in cellular communication where it refers to the generation of 
technology. So I'd recommend using GHz in descriptions/comments and 
GHZ/ghz in identifiers.

Side note: in USA, Comcast have started promoting their network as 
"10G". In their case they're referring to 10 Gbps speeds. They are being 
called out for misleading consumers with that terminology.
<https://arstechnica.com/tech-policy/2023/10/comcast-should-stop-advertising-slower-speeds-as-10g-industry-group-says/>

Re: [PATCH 1/5] [brcmfmac] Add support for encoding/decoding 6g chanspecs

[Arend van Spriel]

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

On 10/20/2023 11:46 PM, Jeff Johnson wrote:
> On 10/20/2023 2:57 AM, Arend van Spriel wrote:
>> On 10/19/2023 3:42 AM, Daniel Berlin wrote:
>>> This patch adds support for 6G chanspecs, as part of adding 6G and
>>> 802.11ax support.
> 
> In ath drivers we are actively discouraging the use of "G" since 1) that 
> is not the SI unit for gigahertz, and 2) it conflicts with the use of 
> that term in cellular communication where it refers to the generation of 
> technology. So I'd recommend using GHz in descriptions/comments and 
> GHZ/ghz in identifiers.

I understand, but I am not going to push back on it. The identifiers 
using this are defined and used in the firmware API so at least those I 
prefer to keep similar/same. For comments in the driver code we should 
consider using GHz instead.

> Side note: in USA, Comcast have started promoting their network as 
> "10G". In their case they're referring to 10 Gbps speeds. They are being 
> called out for misleading consumers with that terminology.
> <https://arstechnica.com/tech-policy/2023/10/comcast-should-stop-advertising-slower-speeds-as-10g-industry-group-says/>

If consumers are that stupid they can mislead by anything ;-)

Thanks,
Arend

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

Re: [PATCH 2/5] [brcmfmac] Add support for 6G bands

[Arend van Spriel]

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

On 10/19/2023 3:42 AM, Daniel Berlin wrote:
> This patch adds support for 6G bands, along with HE capabilities,
> as they are required to register 6G bands with wiphy.
> This in turn, enables 802.11ax support for the other bands.
> 
> Scanning is not updated in this patch, so the bands are unused
> except to be able to process what the firmware tells us.
> 
> Existing code is updated to handle all the bands rather than just 2g and
> 5g channels.

Reviewed-by: Arend van Spriel <arend.vanspriel@broadcom.com>
> Signed-off-by: Daniel Berlin <dberlin@dberlin.org>
> ---
>   .../broadcom/brcm80211/brcmfmac/cfg80211.c    | 365 +++++++++++++++---
>   1 file changed, 313 insertions(+), 52 deletions(-)
> 
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> index 667462369a32..7143ffe659f6 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> @@ -178,6 +178,22 @@ static struct ieee80211_rate __wl_rates[] = {
>   	.max_power		= 30,				\
>   }
>   
> +#define CHAN6G(_channel) {					\
> +	.band			= NL80211_BAND_6GHZ,		\
> +	.center_freq		= 5950 + (5 * (_channel)),	\

maybe handle channel 2 here as well, ie.:
	.center_freq = ((_channel) == 2) ? 5935 : 5950 + (5 * (_channel)),

> +	.hw_value		= (_channel),			\
> +	.max_antenna_gain	= 0,				\
> +	.max_power		= 30,				\
> +}

so we can drop this one below...

> +#define CHAN6G_CHAN2() {					\
> +	.band			= NL80211_BAND_6GHZ,		\
> +	.center_freq		= 5935,				\
> +	.hw_value		= 2,				\
> +	.max_antenna_gain	= 0,				\
> +	.max_power		= 30,				\
> +}
> +
>   static struct ieee80211_channel __wl_2ghz_channels[] = {
>   	CHAN2G(1, 2412), CHAN2G(2, 2417), CHAN2G(3, 2422), CHAN2G(4, 2427),
>   	CHAN2G(5, 2432), CHAN2G(6, 2437), CHAN2G(7, 2442), CHAN2G(8, 2447),
> @@ -194,6 +210,23 @@ static struct ieee80211_channel __wl_5ghz_channels[] = {
>   	CHAN5G(153), CHAN5G(157), CHAN5G(161), CHAN5G(165)
>   };
> 
> +static struct ieee80211_channel __wl_6ghz_channels[] = {
> +	CHAN6G_CHAN2(), CHAN6G(1),   CHAN6G(5),	  CHAN6G(9),   CHAN6G(13),
> +	CHAN6G(17),	CHAN6G(21),  CHAN6G(25),  CHAN6G(29),  CHAN6G(33),
> +	CHAN6G(37),	CHAN6G(41),  CHAN6G(45),  CHAN6G(49),  CHAN6G(53),
> +	CHAN6G(57),	CHAN6G(61),  CHAN6G(65),  CHAN6G(69),  CHAN6G(73),
> +	CHAN6G(77),	CHAN6G(81),  CHAN6G(85),  CHAN6G(89),  CHAN6G(93),
> +	CHAN6G(97),	CHAN6G(101), CHAN6G(105), CHAN6G(109), CHAN6G(113),
> +	CHAN6G(117),	CHAN6G(121), CHAN6G(125), CHAN6G(129), CHAN6G(133),
> +	CHAN6G(137),	CHAN6G(141), CHAN6G(145), CHAN6G(149), CHAN6G(153),
> +	CHAN6G(157),	CHAN6G(161), CHAN6G(165), CHAN6G(169), CHAN6G(173),
> +	CHAN6G(177),	CHAN6G(181), CHAN6G(185), CHAN6G(189), CHAN6G(193),
> +	CHAN6G(197),	CHAN6G(201), CHAN6G(205), CHAN6G(209), CHAN6G(213),
> +	CHAN6G(217),	CHAN6G(221), CHAN6G(225), CHAN6G(229), CHAN6G(233),
> +};
> +
> +struct ieee80211_sband_iftype_data sdata[NUM_NL80211_BANDS];
> +
>   /* Band templates duplicated per wiphy. The channel info
>    * above is added to the band during setup.
>    */
> @@ -209,6 +242,12 @@ static const struct ieee80211_supported_band __wl_band_5ghz = {
>   	.n_bitrates = wl_a_rates_size,
>   };
>   
> +static const struct ieee80211_supported_band __wl_band_6ghz = {
> +	.band = NL80211_BAND_6GHZ,
> +	.bitrates = wl_a_rates,
> +	.n_bitrates = wl_a_rates_size,
> +};
> +
>   /* This is to override regulatory domains defined in cfg80211 module (reg.c)
>    * By default world regulatory domain defined in reg.c puts the flags
>    * NL80211_RRF_NO_IR for 5GHz channels (for * 36..48 and 149..165).
> @@ -217,20 +256,22 @@ static const struct ieee80211_supported_band __wl_band_5ghz = {
>    * domain are to be done here.
>    */
>   static const struct ieee80211_regdomain brcmf_regdom = {
> -	.n_reg_rules = 4,
> +	.n_reg_rules = 5,
>   	.alpha2 =  "99",
>   	.reg_rules = {
>   		/* IEEE 802.11b/g, channels 1..11 */
> -		REG_RULE(2412-10, 2472+10, 40, 6, 20, 0),
> +		REG_RULE(2412 - 10, 2472 + 10, 40, 6, 20, 0),
>   		/* If any */
>   		/* IEEE 802.11 channel 14 - Only JP enables
>   		 * this and for 802.11b only
>   		 */
> -		REG_RULE(2484-10, 2484+10, 20, 6, 20, 0),
> +		REG_RULE(2484 - 10, 2484 + 10, 20, 6, 20, 0),
>   		/* IEEE 802.11a, channel 36..64 */
> -		REG_RULE(5150-10, 5350+10, 160, 6, 20, 0),
> +		REG_RULE(5150 - 10, 5350 + 10, 160, 6, 20, 0),
>   		/* IEEE 802.11a, channel 100..165 */
> -		REG_RULE(5470-10, 5850+10, 160, 6, 20, 0), }
> +		REG_RULE(5470 - 10, 5850 + 10, 160, 6, 20, 0),
> +		/* IEEE 802.11ax, 6E */
> +		REG_RULE(5935 - 10, 7115 + 10, 160, 6, 20, 0), }
>   };
>   
>   /* Note: brcmf_cipher_suites is an array of int defining which cipher suites
> @@ -316,6 +357,8 @@ static u8 nl80211_band_to_fwil(enum nl80211_band band)
>   		return WLC_BAND_2G;
>   	case NL80211_BAND_5GHZ:
>   		return WLC_BAND_5G;
> +	case NL80211_BAND_6GHZ:
> +		return WLC_BAND_6G;
>   	default:
>   		WARN_ON(1);
>   		break;
> @@ -323,6 +366,23 @@ static u8 nl80211_band_to_fwil(enum nl80211_band band)
>   	return 0;
>   }
>   
> +static __le32 nl80211_band_to_chanspec_band(enum nl80211_band band)
> +{
> +	switch (band) {
> +	case NL80211_BAND_2GHZ:
> +		return BRCMU_CHAN_BAND_2G;
> +	case NL80211_BAND_5GHZ:
> +		return BRCMU_CHAN_BAND_5G;
> +	case NL80211_BAND_6GHZ:
> +		return BRCMU_CHAN_BAND_6G;
> +	case NL80211_BAND_60GHZ:
> +	default:
> +		WARN_ON_ONCE(1);
> +		// Choose a safe default
> +		return BRCMU_CHAN_BAND_2G;
> +	}
> +}
> +
>   static u16 chandef_to_chanspec(struct brcmu_d11inf *d11inf,
>   			       struct cfg80211_chan_def *ch)
>   {
> @@ -382,17 +442,7 @@ static u16 chandef_to_chanspec(struct brcmu_d11inf *d11inf,
>   	default:
>   		WARN_ON_ONCE(1);
>   	}
> -	switch (ch->chan->band) {
> -	case NL80211_BAND_2GHZ:
> -		ch_inf.band = BRCMU_CHAN_BAND_2G;
> -		break;
> -	case NL80211_BAND_5GHZ:
> -		ch_inf.band = BRCMU_CHAN_BAND_5G;
> -		break;
> -	case NL80211_BAND_60GHZ:
> -	default:
> -		WARN_ON_ONCE(1);
> -	}
> +	ch_inf.band = nl80211_band_to_chanspec_band(ch->chan->band);
>   	d11inf->encchspec(&ch_inf);
>   
>   	brcmf_dbg(TRACE, "chanspec: 0x%x\n", ch_inf.chspec);
> @@ -404,6 +454,7 @@ u16 channel_to_chanspec(struct brcmu_d11inf *d11inf,
>   {
>   	struct brcmu_chan ch_inf;
>   
> +	ch_inf.band = nl80211_band_to_chanspec_band(ch->band);
>   	ch_inf.chnum = ieee80211_frequency_to_channel(ch->center_freq);
>   	ch_inf.bw = BRCMU_CHAN_BW_20;
>   	d11inf->encchspec(&ch_inf);
> @@ -3340,6 +3391,7 @@ static s32 brcmf_inform_single_bss(struct brcmf_cfg80211_info *cfg,
>   	struct cfg80211_bss *bss;
>   	enum nl80211_band band;
>   	struct brcmu_chan ch;
> +	u16 chanspec;
>   	u16 channel;
>   	u32 freq;
>   	u16 notify_capability;
> @@ -3353,20 +3405,41 @@ static s32 brcmf_inform_single_bss(struct brcmf_cfg80211_info *cfg,
>   		return -EINVAL;
>   	}
>   
> +	chanspec = le16_to_cpu(bi->chanspec);
>   	if (!bi->ctl_ch) {
> -		ch.chspec = le16_to_cpu(bi->chanspec);
> +		ch.chspec = chanspec;
>   		cfg->d11inf.decchspec(&ch);
>   		bi->ctl_ch = ch.control_ch_num;
>   	}
>   	channel = bi->ctl_ch;
>   
> -	if (channel <= CH_MAX_2G_CHANNEL)
> -		band = NL80211_BAND_2GHZ;
> -	else
> +	if (CHSPEC_IS6G(chanspec))
> +		band = NL80211_BAND_6GHZ;
> +	else if (CHSPEC_IS5G(chanspec))
>   		band = NL80211_BAND_5GHZ;
> +	else
> +		band = NL80211_BAND_2GHZ;
>   
>   	freq = ieee80211_channel_to_frequency(channel, band);
> +	if (!freq) {
> +		brcmf_err("Invalid frequency %d returned for channel %d, band %d. chanspec was %04x\n",
> +			  freq, channel, band, bi->chanspec);
> +
> +		/* We ignore this BSS ID rather than try to continue on.
> +		 * Otherwise we will cause an OOPs because our frequency is 0.
> +		 * The main case this occurs is some new frequency band
> +		 * we have not seen before, and if we return an error,
> +		 * we will cause the scan to fail.  It seems better to
> +		 * report the error, skip this BSS, and move on.
> +		 */
> +		return 0;
> +	}
>   	bss_data.chan = ieee80211_get_channel(wiphy, freq);

How could this fail? Our wiphy registers all possible channels so if 
ieee80211_channel_to_frequency() succeeds ieee80211_get_channel() can 
not fail.

> +	if (!bss_data.chan) {
> +		brcmf_err("Could not convert frequency into channel for channel %d, band %d, chanspec was %04x\n",
> +			  channel, band, bi->chanspec);
> +		return 0;
> +	}
>   	bss_data.boottime_ns = ktime_to_ns(ktime_get_boottime());
>   
>   	notify_capability = le16_to_cpu(bi->capability);
> @@ -3454,7 +3527,7 @@ static s32 brcmf_inform_ibss(struct brcmf_cfg80211_info *cfg,
>   	buf = kzalloc(WL_BSS_INFO_MAX, GFP_KERNEL);
>   	if (buf == NULL) {
>   		err = -ENOMEM;
> -		goto CleanUp;
> +		goto cleanup;
>   	}
>   
>   	*(__le32 *)buf = cpu_to_le32(WL_BSS_INFO_MAX);
> @@ -3463,7 +3536,7 @@ static s32 brcmf_inform_ibss(struct brcmf_cfg80211_info *cfg,
>   				     buf, WL_BSS_INFO_MAX);
>   	if (err) {
>   		bphy_err(drvr, "WLC_GET_BSS_INFO failed: %d\n", err);
> -		goto CleanUp;
> +		goto cleanup;
>   	}
>   
>   	bi = (struct brcmf_bss_info_le *)(buf + 4);
> @@ -3473,10 +3546,18 @@ static s32 brcmf_inform_ibss(struct brcmf_cfg80211_info *cfg,
>   
>   	if (ch.band == BRCMU_CHAN_BAND_2G)
>   		band = wiphy->bands[NL80211_BAND_2GHZ];
> -	else
> +	else if (ch.band == BRCMU_CHAN_BAND_5G)
>   		band = wiphy->bands[NL80211_BAND_5GHZ];
> +	else
> +		band = wiphy->bands[NL80211_BAND_6GHZ];
>   
>   	freq = ieee80211_channel_to_frequency(ch.control_ch_num, band->band);
> +	if (freq == 0) {
> +		brcmf_err("Invalid frequency %d returned for channel %d, band %d. chanspec was %04x\n",
> +			  freq, ch.control_ch_num, ch.band, bi->chanspec);
> +		goto cleanup;
> +	}
> +
>   	cfg->channel = freq;
>   	notify_channel = ieee80211_get_channel(wiphy, freq);
>   
> @@ -3499,12 +3580,12 @@ static s32 brcmf_inform_ibss(struct brcmf_cfg80211_info *cfg,
>   
>   	if (!bss) {
>   		err = -ENOMEM;
> -		goto CleanUp;
> +		goto cleanup;
>   	}
>   
>   	cfg80211_put_bss(wiphy, bss);
>   
> -CleanUp:
> +cleanup:
>   
>   	kfree(buf);
>   
> @@ -5707,6 +5788,9 @@ static int brcmf_cfg80211_get_channel(struct wiphy *wiphy,
>   	case BRCMU_CHAN_BAND_5G:
>   		band = NL80211_BAND_5GHZ;
>   		break;
> +	case BRCMU_CHAN_BAND_6G:
> +		band = NL80211_BAND_6GHZ;
> +		break;
>   	}
>   
>   	switch (ch.bw) {
> @@ -5728,9 +5812,19 @@ static int brcmf_cfg80211_get_channel(struct wiphy *wiphy,
>   	}
>   
>   	freq = ieee80211_channel_to_frequency(ch.control_ch_num, band);
> +	if (freq == 0) {
> +		brcmf_err("Invalid frequency %d returned for channel %d, band %d. chanspec was %04x\n",
> +			  freq, ch.control_ch_num, ch.band, chanspec);
> +		return -EINVAL;
> +	}
>   	chandef->chan = ieee80211_get_channel(wiphy, freq);
>   	chandef->width = width;
>   	chandef->center_freq1 = ieee80211_channel_to_frequency(ch.chnum, band);
> +	if (chandef->center_freq1 == 0) {
> +		brcmf_err("Invalid frequency %d returned for channel %d, band %d. chanspec was %04x\n",
> +			  freq, ch.chnum, ch.band, chanspec);
> +		return -EINVAL;
> +	}
>   	chandef->center_freq2 = 0;
>   
>   	return 0;
> @@ -6386,10 +6480,17 @@ brcmf_bss_roaming_done(struct brcmf_cfg80211_info *cfg,
>   
>   	if (ch.band == BRCMU_CHAN_BAND_2G)
>   		band = wiphy->bands[NL80211_BAND_2GHZ];
> -	else
> +	else if (ch.band == BRCMU_CHAN_BAND_5G)
>   		band = wiphy->bands[NL80211_BAND_5GHZ];
> +	else
> +		band = wiphy->bands[NL80211_BAND_6GHZ];
>   
>   	freq = ieee80211_channel_to_frequency(ch.control_ch_num, band->band);
> +	if (freq == 0) {
> +		brcmf_err("Invalid frequency %d returned for channel %d, band %d. chanspec was %04x\n",
> +			  freq, ch.control_ch_num, ch.band, bi->chanspec);
> +		goto done;
> +	}
>   	notify_channel = ieee80211_get_channel(wiphy, freq);
>   
>   done:
> @@ -6965,6 +7066,10 @@ static int brcmf_construct_chaninfo(struct brcmf_cfg80211_info *cfg,
>   		for (i = 0; i < band->n_channels; i++)
>   			band->channels[i].flags = IEEE80211_CHAN_DISABLED;
>   	band = wiphy->bands[NL80211_BAND_5GHZ];
> +	if (band)

Eh. Why is this conditional? We are creating all bands in the wiphy 
instance so why the null check here?

> +		for (i = 0; i < band->n_channels; i++)
> +			band->channels[i].flags = IEEE80211_CHAN_DISABLED;
> +	band = wiphy->bands[NL80211_BAND_6GHZ];
>   	if (band)
>   		for (i = 0; i < band->n_channels; i++)
>   			band->channels[i].flags = IEEE80211_CHAN_DISABLED;
> @@ -6985,6 +7090,8 @@ static int brcmf_construct_chaninfo(struct brcmf_cfg80211_info *cfg,
>   			band = wiphy->bands[NL80211_BAND_2GHZ];
>   		} else if (ch.band == BRCMU_CHAN_BAND_5G) {
>   			band = wiphy->bands[NL80211_BAND_5GHZ];
> +		} else if (ch.band == BRCMU_CHAN_BAND_6G) {
> +			band = wiphy->bands[NL80211_BAND_6GHZ];
>   		} else {
>   			bphy_err(drvr, "Invalid channel Spec. 0x%x.\n",
>   				 ch.chspec);
> @@ -7150,7 +7257,7 @@ static int brcmf_enable_bw40_2g(struct brcmf_cfg80211_info *cfg)
>   	return err;
>   }
>   
> -static void brcmf_get_bwcap(struct brcmf_if *ifp, u32 bw_cap[])
> +static void brcmf_get_bwcap(struct brcmf_if *ifp, u32 bw_cap[], bool has_6g)
>   {
>   	struct brcmf_pub *drvr = ifp->drvr;
>   	u32 band, mimo_bwcap;
> @@ -7158,17 +7265,29 @@ static void brcmf_get_bwcap(struct brcmf_if *ifp, u32 bw_cap[])
>   
>   	band = WLC_BAND_2G;
>   	err = brcmf_fil_iovar_int_get(ifp, "bw_cap", &band);
> -	if (!err) {
> -		bw_cap[NL80211_BAND_2GHZ] = band;
> -		band = WLC_BAND_5G;
> -		err = brcmf_fil_iovar_int_get(ifp, "bw_cap", &band);
> -		if (!err) {
> -			bw_cap[NL80211_BAND_5GHZ] = band;
> -			return;
> -		}
> -		WARN_ON(1);
> +	if (err)
> +		goto fallback;
> +	bw_cap[NL80211_BAND_2GHZ] = band;
> +	band = WLC_BAND_5G;
> +	err |= brcmf_fil_iovar_int_get(ifp, "bw_cap", &band);
> +	if (err)
> +		goto fallback;
> +	bw_cap[NL80211_BAND_5GHZ] = band;
> +	if (!has_6g)
>   		return;
> -	}
> +	band = WLC_BAND_6G;
> +	err |= brcmf_fil_iovar_int_get(ifp, "bw_cap", &band);
> +	/* Prior to the introduction of 6g, this function only
> +	 * did fallback in the case of 2g and 5g -failing.
> +	 * As mimo_bwcap does not have 6g bwcap info anyway,
> +	 * we keep that behavior.
> +	 */
> +	if (err)
> +		return;
> +	bw_cap[NL80211_BAND_6GHZ] = band;
> +	return;
> +fallback:
> +
>   	brcmf_dbg(INFO, "fallback to mimo_bw_cap info\n");
>   	mimo_bwcap = 0;
>   	err = brcmf_fil_iovar_int_get(ifp, "mimo_bw_cap", &mimo_bwcap);
> @@ -7195,6 +7314,9 @@ static void brcmf_get_bwcap(struct brcmf_if *ifp, u32 bw_cap[])
>   static void brcmf_update_ht_cap(struct ieee80211_supported_band *band,
>   				u32 bw_cap[2], u32 nchain)
>   {
> +	/* Not supported in 6G band */
> +	if (band->band == NL80211_BAND_6GHZ)
> +		return;
>   	band->ht_cap.ht_supported = true;
>   	if (bw_cap[band->band] & WLC_BW_40MHZ_BIT) {
>   		band->ht_cap.cap |= IEEE80211_HT_CAP_SGI_40;
> @@ -7225,8 +7347,8 @@ static void brcmf_update_vht_cap(struct ieee80211_supported_band *band,
>   {
>   	__le16 mcs_map;
>   
> -	/* not allowed in 2.4G band */
> -	if (band->band == NL80211_BAND_2GHZ)
> +	/* not allowed in 2.4G or 6G band */
> +	if (band->band == NL80211_BAND_2GHZ || band->band == NL80211_BAND_6GHZ)
>   		return;
>   
>   	band->vht_cap.vht_supported = true;
> @@ -7261,6 +7383,116 @@ static void brcmf_update_vht_cap(struct ieee80211_supported_band *band,
>   	}
>   }
>   
> +static void brcmf_update_he_cap(struct ieee80211_supported_band *band,
> +				struct ieee80211_sband_iftype_data *data)
> +{
> +	int idx = 1;
> +	struct ieee80211_sta_he_cap *he_cap = &data->he_cap;
> +	struct ieee80211_he_cap_elem *he_cap_elem = &he_cap->he_cap_elem;
> +	struct ieee80211_he_mcs_nss_supp *he_mcs = &he_cap->he_mcs_nss_supp;
> +	struct ieee80211_he_6ghz_capa *he_6ghz_capa = &data->he_6ghz_capa;
> +
> +	if (!data) {

There is no allocation in sight here *and* data is already dereferenced 
above to obtain the he_cap. data points to static global array so it is 
never NULL.

> +		brcmf_err("failed to allocate sdata\n");
> +		return;
> +	}
> +
> +	data->types_mask = BIT(NL80211_IFTYPE_STATION) | BIT(NL80211_IFTYPE_AP);

It is unlikely STA and AP ever have the same capabilities.

> +	he_cap->has_he = true;

has_he should be set according feature flag. The firmware capability 
should have '11ax' in it.

Below stuff is all hardcoded. That is fine to enable its use for now, 
but may not be fully true. At least for BCA I know that there is a 
firmware command for it. Expect WCC chips do not have that. If it can 
differ per device we can not use static global array. Maybe better to 
take that into account off the bat:

1) define hard-coded global sdata array
2) only pass band as parameter
3) duplicate sdata entry and setup iftype data
4) add iftype data reference to band

> +	/* HE MAC Capabilities Information */
> +	he_cap_elem->mac_cap_info[0] = IEEE80211_HE_MAC_CAP0_HTC_HE |
> +				       IEEE80211_HE_MAC_CAP0_TWT_REQ |
> +				       IEEE80211_HE_MAC_CAP0_TWT_RES;
> +
> +	he_cap_elem->mac_cap_info[1] =
> +		IEEE80211_HE_MAC_CAP1_TF_MAC_PAD_DUR_8US |
> +		IEEE80211_HE_MAC_CAP1_TF_MAC_PAD_DUR_16US;
> +
> +	he_cap_elem->mac_cap_info[2] = IEEE80211_HE_MAC_CAP2_BSR |
> +				       IEEE80211_HE_MAC_CAP2_BCAST_TWT;
> +
> +	he_cap_elem->mac_cap_info[3] =
> +		IEEE80211_HE_MAC_CAP3_OMI_CONTROL |
> +		IEEE80211_HE_MAC_CAP3_MAX_AMPDU_LEN_EXP_EXT_1 |
> +		IEEE80211_HE_MAC_CAP3_FLEX_TWT_SCHED;
> +
> +	he_cap_elem->mac_cap_info[4] = IEEE80211_HE_MAC_CAP4_AMSDU_IN_AMPDU;
> +
> +	/* HE PHY Capabilities Information */
> +	he_cap_elem->phy_cap_info[0] =
> +		IEEE80211_HE_PHY_CAP0_CHANNEL_WIDTH_SET_40MHZ_IN_2G |
> +		IEEE80211_HE_PHY_CAP0_CHANNEL_WIDTH_SET_40MHZ_80MHZ_IN_5G |
> +		IEEE80211_HE_PHY_CAP0_CHANNEL_WIDTH_SET_160MHZ_IN_5G;
> +	;
> +
> +	he_cap_elem->phy_cap_info[1] =
> +		IEEE80211_HE_PHY_CAP1_LDPC_CODING_IN_PAYLOAD;
> +
> +	he_cap_elem->phy_cap_info[2] =
> +		IEEE80211_HE_PHY_CAP2_NDP_4x_LTF_AND_3_2US |
> +		IEEE80211_HE_PHY_CAP2_UL_MU_FULL_MU_MIMO |
> +		IEEE80211_HE_PHY_CAP2_UL_MU_PARTIAL_MU_MIMO;
> +
> +	he_cap_elem->phy_cap_info[3] =
> +		IEEE80211_HE_PHY_CAP3_DCM_MAX_CONST_TX_QPSK |
> +		IEEE80211_HE_PHY_CAP3_DCM_MAX_TX_NSS_2 |
> +		IEEE80211_HE_PHY_CAP3_DCM_MAX_CONST_RX_16_QAM |
> +		IEEE80211_HE_PHY_CAP3_SU_BEAMFORMER;
> +
> +	he_cap_elem->phy_cap_info[4] =
> +		IEEE80211_HE_PHY_CAP4_SU_BEAMFORMEE |
> +		IEEE80211_HE_PHY_CAP4_BEAMFORMEE_MAX_STS_UNDER_80MHZ_8;
> +
> +	he_cap_elem->phy_cap_info[5] =
> +		IEEE80211_HE_PHY_CAP5_NG16_SU_FEEDBACK |
> +		IEEE80211_HE_PHY_CAP5_NG16_MU_FEEDBACK |
> +		IEEE80211_HE_PHY_CAP5_BEAMFORMEE_NUM_SND_DIM_UNDER_80MHZ_2;
> +
> +	he_cap_elem->phy_cap_info[6] =
> +		IEEE80211_HE_PHY_CAP6_CODEBOOK_SIZE_42_SU |
> +		IEEE80211_HE_PHY_CAP6_CODEBOOK_SIZE_75_MU |
> +		IEEE80211_HE_PHY_CAP6_TRIG_SU_BEAMFORMING_FB |
> +		IEEE80211_HE_PHY_CAP6_TRIG_MU_BEAMFORMING_PARTIAL_BW_FB |
> +		IEEE80211_HE_PHY_CAP6_TRIG_CQI_FB |
> +		IEEE80211_HE_PHY_CAP6_PARTIAL_BW_EXT_RANGE |
> +		IEEE80211_HE_PHY_CAP6_PPE_THRESHOLD_PRESENT;
> +
> +	he_cap_elem->phy_cap_info[7] =
> +		IEEE80211_HE_PHY_CAP7_HE_SU_MU_PPDU_4XLTF_AND_08_US_GI |
> +		IEEE80211_HE_PHY_CAP7_MAX_NC_1;
> +
> +	he_cap_elem->phy_cap_info[8] =
> +		IEEE80211_HE_PHY_CAP8_HE_ER_SU_PPDU_4XLTF_AND_08_US_GI |
> +		IEEE80211_HE_PHY_CAP8_20MHZ_IN_40MHZ_HE_PPDU_IN_2G |
> +		IEEE80211_HE_PHY_CAP8_20MHZ_IN_160MHZ_HE_PPDU |
> +		IEEE80211_HE_PHY_CAP8_80MHZ_IN_160MHZ_HE_PPDU;
> +
> +	he_cap_elem->phy_cap_info[9] =
> +		IEEE80211_HE_PHY_CAP9_TX_1024_QAM_LESS_THAN_242_TONE_RU |
> +		IEEE80211_HE_PHY_CAP9_RX_1024_QAM_LESS_THAN_242_TONE_RU |
> +		IEEE80211_HE_PHY_CAP9_RX_FULL_BW_SU_USING_MU_WITH_COMP_SIGB |
> +		IEEE80211_HE_PHY_CAP9_RX_FULL_BW_SU_USING_MU_WITH_NON_COMP_SIGB;
> +
> +	/* HE Supported MCS and NSS Set */
> +	he_mcs->rx_mcs_80 = cpu_to_le16(0xfffa);
> +	he_mcs->tx_mcs_80 = cpu_to_le16(0xfffa);
> +	/* HE 6 GHz band capabilities */
> +	if (band->band == NL80211_BAND_6GHZ) {
> +		u16 capa = 0;
> +
> +		capa = FIELD_PREP(IEEE80211_HE_6GHZ_CAP_MIN_MPDU_START,
> +				  IEEE80211_HT_MPDU_DENSITY_8) |
> +		       FIELD_PREP(IEEE80211_HE_6GHZ_CAP_MAX_AMPDU_LEN_EXP,
> +				  IEEE80211_VHT_MAX_AMPDU_1024K) |
> +		       FIELD_PREP(IEEE80211_HE_6GHZ_CAP_MAX_MPDU_LEN,
> +				  IEEE80211_VHT_CAP_MAX_MPDU_LENGTH_11454);
> +		he_6ghz_capa->capa = cpu_to_le16(capa);
> +	}
> +	band->n_iftype_data = idx;
> +	band->iftype_data = data;
> +}
> +
>   static int brcmf_setup_wiphybands(struct brcmf_cfg80211_info *cfg)
>   {
>   	struct brcmf_pub *drvr = cfg->pub;
> @@ -7268,7 +7500,8 @@ static int brcmf_setup_wiphybands(struct brcmf_cfg80211_info *cfg)
>   	struct wiphy *wiphy = cfg_to_wiphy(cfg);
>   	u32 nmode = 0;
>   	u32 vhtmode = 0;
> -	u32 bw_cap[2] = { WLC_BW_20MHZ_BIT, WLC_BW_20MHZ_BIT };
> +	/* 2GHZ, 5GHZ, 60GHZ, 6GHZ */
> +	u32 bw_cap[4] = { WLC_BW_20MHZ_BIT, WLC_BW_20MHZ_BIT, 0, 0 };

Too bad we reserve space for 60G which we probably never will support.

>   	u32 rxchain;
>   	u32 nchain;
>   	int err;
> @@ -7277,17 +7510,21 @@ static int brcmf_setup_wiphybands(struct brcmf_cfg80211_info *cfg)
>   	u32 txstreams = 0;
>   	u32 txbf_bfe_cap = 0;
>   	u32 txbf_bfr_cap = 0;
> +	u32 he_cap[2] = {0, 0};
>   
>   	(void)brcmf_fil_iovar_int_get(ifp, "vhtmode", &vhtmode);
> +	(void)brcmf_fil_iovar_data_get(ifp, "he", &he_cap, sizeof(he_cap));
>   	err = brcmf_fil_iovar_int_get(ifp, "nmode", &nmode);
>   	if (err) {
>   		bphy_err(drvr, "nmode error (%d)\n", err);
>   	} else {
> -		brcmf_get_bwcap(ifp, bw_cap);
> +		brcmf_get_bwcap(ifp, bw_cap, he_cap[0] != 0);
>   	}
> -	brcmf_dbg(INFO, "nmode=%d, vhtmode=%d, bw_cap=(%d, %d)\n",
> +	brcmf_dbg(INFO,
> +		  "nmode=%d, vhtmode=%d, bw_cap=(%d, %d, %d), he_cap=(%d, %d)\n",
>   		  nmode, vhtmode, bw_cap[NL80211_BAND_2GHZ],
> -		  bw_cap[NL80211_BAND_5GHZ]);
> +		  bw_cap[NL80211_BAND_5GHZ], bw_cap[NL80211_BAND_6GHZ],
> +		  he_cap[0], he_cap[1]);

So are these he mac and phy capabilities? ...

>   
>   	err = brcmf_fil_iovar_int_get(ifp, "rxchain", &rxchain);
>   	if (err) {
> @@ -7328,6 +7565,8 @@ static int brcmf_setup_wiphybands(struct brcmf_cfg80211_info *cfg)
>   		if (vhtmode)
>   			brcmf_update_vht_cap(band, bw_cap, nchain, txstreams,
>   					     txbf_bfe_cap, txbf_bfr_cap);
> +		if (he_cap[0])
> +			brcmf_update_he_cap(band, &sdata[band->band]);

... if so should they be passed here?

>   	}
>   
>   	return 0;
> @@ -7698,12 +7937,27 @@ static int brcmf_setup_wiphy(struct wiphy *wiphy, struct brcmf_if *ifp)
>   			band->n_channels = ARRAY_SIZE(__wl_5ghz_channels);
>   			wiphy->bands[NL80211_BAND_5GHZ] = band;
>   		}
> -	}
> +		if (bandlist[i] == cpu_to_le32(WLC_BAND_6G)) {

Ok. So wiphy band can be NULL.

> +			band = kmemdup(&__wl_band_6ghz, sizeof(__wl_band_6ghz),
> +				       GFP_KERNEL);
> +			if (!band)
> +				return -ENOMEM;
> +
> +			band->channels = kmemdup(&__wl_6ghz_channels,
> +						 sizeof(__wl_6ghz_channels),
> +						 GFP_KERNEL);
> +			if (!band->channels) {
> +				kfree(band);
> +				return -ENOMEM;
> +			}
>   
> +			band->n_channels = ARRAY_SIZE(__wl_6ghz_channels);
> +			wiphy->bands[NL80211_BAND_6GHZ] = band;
> +		}
> +	}
>   	if (wiphy->bands[NL80211_BAND_5GHZ] &&
>   	    brcmf_feat_is_enabled(ifp, BRCMF_FEAT_DOT11H))
> -		wiphy_ext_feature_set(wiphy,
> -				      NL80211_EXT_FEATURE_DFS_OFFLOAD);
> +		wiphy_ext_feature_set(wiphy, NL80211_EXT_FEATURE_DFS_OFFLOAD);
>   
>   	wiphy_ext_feature_set(wiphy, NL80211_EXT_FEATURE_CQM_RSSI_LIST);
>   
> @@ -8240,6 +8494,10 @@ static void brcmf_free_wiphy(struct wiphy *wiphy)
>   		kfree(wiphy->bands[NL80211_BAND_5GHZ]->channels);
>   		kfree(wiphy->bands[NL80211_BAND_5GHZ]);
>   	}
> +	if (wiphy->bands[NL80211_BAND_6GHZ]) {
> +		kfree(wiphy->bands[NL80211_BAND_6GHZ]->channels);
> +		kfree(wiphy->bands[NL80211_BAND_6GHZ]);
> +	}
>   #if IS_ENABLED(CONFIG_PM)
>   	if (wiphy->wowlan != &brcmf_wowlan_support)
>   		kfree(wiphy->wowlan);
> @@ -8331,18 +8589,21 @@ struct brcmf_cfg80211_info *brcmf_cfg80211_attach(struct brcmf_pub *drvr,
>   	if (brcmf_feat_is_enabled(ifp, BRCMF_FEAT_DUMP_OBSS))
>   		ops->dump_survey = brcmf_cfg80211_dump_survey;
>   
> -	err = wiphy_register(wiphy);
> -	if (err < 0) {
> -		bphy_err(drvr, "Could not register wiphy device (%d)\n", err);
> -		goto priv_out;
> -	}
> -
> +	/* We have to configure the bands before we register the wiphy device
> +	 * because it requires that band capabilities be correct.
> +	 */

Is it? The order was deliberate. brcmf_setup_wiphybands() calls 
brcmf_construct_chaninfo() which disables all channels. When you do that 
before wiphy_register() the orig_flags of the channel will be DISABLED 
and can never be used.

>   	err = brcmf_setup_wiphybands(cfg);
>   	if (err) {
>   		bphy_err(drvr, "Setting wiphy bands failed (%d)\n", err);
>   		goto wiphy_unreg_out;
>   	}
>   
> +	err = wiphy_register(wiphy);
> +	if (err < 0) {
> +		bphy_err(drvr, "Could not register wiphy device (%d)\n", err);
> +		goto priv_out;
> +	}
> +
>   	/* If cfg80211 didn't disable 40MHz HT CAP in wiphy_register(),
>   	 * setup 40MHz in 2GHz band and enable OBSS scanning.
>   	 */


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

Re: [PATCH 2/5] [brcmfmac] Add support for 6G bands

[Daniel Berlin]

>
>
> maybe handle channel 2 here as well, ie.:
>         .center_freq = ((_channel) == 2) ? 5935 : 5950 + (5 * (_channel)),
>
> > +     .hw_value               = (_channel),                   \
> > +     .max_antenna_gain       = 0,                            \
> > +     .max_power              = 30,                           \
> > +}
>
> so we can drop this one below...
>
Will do.

>
>
> > +             /* We ignore this BSS ID rather than try to continue on.
> > +              * Otherwise we will cause an OOPs because our frequency is 0.
> > +              * The main case this occurs is some new frequency band
> > +              * we have not seen before, and if we return an error,
> > +              * we will cause the scan to fail.  It seems better to
> > +              * report the error, skip this BSS, and move on.
> > +              */
> > +             return 0;
> > +     }
> >       bss_data.chan = ieee80211_get_channel(wiphy, freq);
>
> How could this fail? Our wiphy registers all possible channels so if
> ieee80211_channel_to_frequency() succeeds ieee80211_get_channel() can
> not fail.

I agree.
>
>
> > @@ -6965,6 +7066,10 @@ static int brcmf_construct_chaninfo(struct brcmf_cfg80211_info *cfg,
> >               for (i = 0; i < band->n_channels; i++)
> >                       band->channels[i].flags = IEEE80211_CHAN_DISABLED;
> >       band = wiphy->bands[NL80211_BAND_5GHZ];
> > +     if (band)
>
> Eh. Why is this conditional? We are creating all bands in the wiphy
> instance so why the null check here?


I just matched what was there, I can remove all of them if we want.

(I'll take care of the rest of the comments between here)

>
> > -     brcmf_dbg(INFO, "nmode=%d, vhtmode=%d, bw_cap=(%d, %d)\n",
> > +     brcmf_dbg(INFO,
> > +               "nmode=%d, vhtmode=%d, bw_cap=(%d, %d, %d), he_cap=(%d, %d)\n",
> >                 nmode, vhtmode, bw_cap[NL80211_BAND_2GHZ],
> > -               bw_cap[NL80211_BAND_5GHZ]);
> > +               bw_cap[NL80211_BAND_5GHZ], bw_cap[NL80211_BAND_6GHZ],
> > +               he_cap[0], he_cap[1]);
>
> So are these he mac and phy capabilities? ...

No, unfortunately, it's either 1 or 0 on these chips, and all chips i tested.
This is the hardware capability iovar.

In the debug firmware i have access to (not apple's), i do see a
command that looks like it may give the he cap, but i can't find how
it would ever be triggered.
(The iovar code for the iovar above is either always just return 0 or return 1)
There are no obvious iovars that relate, and  the absolute latest
bcmdhd hardcodes the he caps, as do infineon's latest ifx code.
:(
I'l hack around see if i can get the caps out of it.

I'll double check other ones.

> > @@ -8331,18 +8589,21 @@ struct brcmf_cfg80211_info *brcmf_cfg80211_attach(struct brcmf_pub *drvr,
> >       if (brcmf_feat_is_enabled(ifp, BRCMF_FEAT_DUMP_OBSS))
> >               ops->dump_survey = brcmf_cfg80211_dump_survey;
> >
> > -     err = wiphy_register(wiphy);
> > -     if (err < 0) {
> > -             bphy_err(drvr, "Could not register wiphy device (%d)\n", err);
> > -             goto priv_out;
> > -     }
> > -
> > +     /* We have to configure the bands before we register the wiphy device
> > +      * because it requires that band capabilities be correct.
> > +      */
>
> Is it?

If you register the 6g band without he_cap set, 80211 is unhappy.
It sanity checks the bands in wiphy_register, and we get caught in the
HE supported check for 6g.

See here:
https://elixir.bootlin.com/linux/latest/source/net/wireless/core.c#L823

In general, you can see it sanity checks the bands/etc as part of
registration.  It happens that 6g triggers the he one, but it seems in
general the bands are supposed to be sane before registration.



> The order was deliberate. brcmf_setup_wiphybands() calls
> brcmf_construct_chaninfo() which disables all channels. When you do that
> before wiphy_register() the orig_flags of the channel will be DISABLED
> and can never be used.

I'll take care of this by copying the flags around.

>
> >       err = brcmf_setup_wiphybands(cfg);
> >       if (err) {
> >               bphy_err(drvr, "Setting wiphy bands failed (%d)\n", err);
> >               goto wiphy_unreg_out;
> >       }
> >
> > +     err = wiphy_register(wiphy);
> > +     if (err < 0) {
> > +             bphy_err(drvr, "Could not register wiphy device (%d)\n", err);
> > +             goto priv_out;
> > +     }
> > +
> >       /* If cfg80211 didn't disable 40MHz HT CAP in wiphy_register(),
> >        * setup 40MHz in 2GHz band and enable OBSS scanning.
> >        */
>

Re: [PATCH 2/5] [brcmfmac] Add support for 6G bands

[Arend van Spriel]

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

On 10/20/2023 6:35 PM, Daniel Berlin wrote:
>>> @@ -6965,6 +7066,10 @@ static int brcmf_construct_chaninfo(struct brcmf_cfg80211_info *cfg,
>>>                for (i = 0; i < band->n_channels; i++)
>>>                        band->channels[i].flags = IEEE80211_CHAN_DISABLED;
>>>        band = wiphy->bands[NL80211_BAND_5GHZ];
>>> +     if (band)
>> Eh. Why is this conditional? We are creating all bands in the wiphy
>> instance so why the null check here?
> 
> I just matched what was there, I can remove all of them if we want.
> 
> (I'll take care of the rest of the comments between here)

I stumbled upon the bandlist usage later in this patch (I think) so we 
do add bands only when the device supports them. Hence you can ignore my 
comment here.

Regards,
Arend

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

Re: [PATCH 2/5] [brcmfmac] Add support for 6G bands

[Daniel Berlin]

On Fri, Oct 20, 2023 at 12:35 PM Daniel Berlin <dberlin@dberlin.org> wrote:
>
> >
> > > -     brcmf_dbg(INFO, "nmode=%d, vhtmode=%d, bw_cap=(%d, %d)\n",
> > > +     brcmf_dbg(INFO,
> > > +               "nmode=%d, vhtmode=%d, bw_cap=(%d, %d, %d), he_cap=(%d, %d)\n",
> > >                 nmode, vhtmode, bw_cap[NL80211_BAND_2GHZ],
> > > -               bw_cap[NL80211_BAND_5GHZ]);
> > > +               bw_cap[NL80211_BAND_5GHZ], bw_cap[NL80211_BAND_6GHZ],
> > > +               he_cap[0], he_cap[1]);
> >
> > So are these he mac and phy capabilities? ...
>
> No, unfortunately, it's either 1 or 0 on these chips, and all chips i tested.
> This is the hardware capability iovar.
>
> In the debug firmware i have access to (not apple's), i do see a
> command that looks like it may give the he cap, but i can't find how
> it would ever be triggered.
> (The iovar code for the iovar above is either always just return 0 or return 1)
> There are no obvious iovars that relate, and  the absolute latest
> bcmdhd hardcodes the he caps, as do infineon's latest ifx code.
> :(
> I'l hack around see if i can get the caps out of it.
>
> I'll double check other ones.


So, I reached a  conclusion on this piece.

This is really an xtlv with subcommands that everyone (including me)
is wrongly treating as a non-xtlv.
The above is really showing you the enable value.
There is also hardware cap value (which is 0/1 as well).

In the 4398/4390 firmware, a "defcap" subcommand was added to the
firmware which can retrieve the default HE capabilities bytes for the
mac and phy and be used to fill them in.
However, it is unsupported in the firmware for earlier chips,
including these chips (or at least, any firmware i've found for it,
apple's or not)

As such, at least for these, STA/AP caps will have to be hardcoded.

I have updated the code to include the subcommands that exist here,
and properly use an xtlv command to retrieve this (it's really a uint8
value).

Re: [PATCH 2/5] [brcmfmac] Add support for 6G bands

[Arend Van Spriel]

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


On October 23, 2023 1:41:34 PM Daniel Berlin <dberlin@dberlin.org> wrote:

> On Fri, Oct 20, 2023 at 12:35 PM Daniel Berlin <dberlin@dberlin.org> wrote:
>>
>>>
>>>> -     brcmf_dbg(INFO, "nmode=%d, vhtmode=%d, bw_cap=(%d, %d)\n",
>>>> +     brcmf_dbg(INFO,
>>>> +               "nmode=%d, vhtmode=%d, bw_cap=(%d, %d, %d), he_cap=(%d, %d)\n",
>>>>            nmode, vhtmode, bw_cap[NL80211_BAND_2GHZ],
>>>> -               bw_cap[NL80211_BAND_5GHZ]);
>>>> +               bw_cap[NL80211_BAND_5GHZ], bw_cap[NL80211_BAND_6GHZ],
>>>> +               he_cap[0], he_cap[1]);
>>>
>>> So are these he mac and phy capabilities? ...
>>
>> No, unfortunately, it's either 1 or 0 on these chips, and all chips i tested.
>> This is the hardware capability iovar.
>>
>> In the debug firmware i have access to (not apple's), i do see a
>> command that looks like it may give the he cap, but i can't find how
>> it would ever be triggered.
>> (The iovar code for the iovar above is either always just return 0 or return 1)
>> There are no obvious iovars that relate, and  the absolute latest
>> bcmdhd hardcodes the he caps, as do infineon's latest ifx code.
>> :(
>> I'l hack around see if i can get the caps out of it.
>>
>> I'll double check other ones.
>
>
> So, I reached a  conclusion on this piece.
>
> This is really an xtlv with subcommands that everyone (including me)
> is wrongly treating as a non-xtlv.
> The above is really showing you the enable value.
> There is also hardware cap value (which is 0/1 as well).
>
> In the 4398/4390 firmware, a "defcap" subcommand was added to the
> firmware which can retrieve the default HE capabilities bytes for the
> mac and phy and be used to fill them in.

So they adopted the same subcommand I added in our firmware. Hopefully it 
is a true copy.

> However, it is unsupported in the firmware for earlier chips,
> including these chips (or at least, any firmware i've found for it,
> apple's or not)
>
> As such, at least for these, STA/AP caps will have to be hardcoded.
>
> I have updated the code to include the subcommands that exist here,
> and properly use an xtlv command to retrieve this (it's really a uint8
> value).

I do have patch lying around here that add xtlv support to fwil.c. Or did I 
already submit that.

Gr. AvS




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

Re: [PATCH 2/5] [brcmfmac] Add support for 6G bands

[Arend Van Spriel]

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

On October 23, 2023 8:06:52 PM Arend Van Spriel 
<arend.vanspriel@broadcom.com> wrote:

> On October 23, 2023 1:41:34 PM Daniel Berlin <dberlin@dberlin.org> wrote:
>
>> On Fri, Oct 20, 2023 at 12:35 PM Daniel Berlin <dberlin@dberlin.org> wrote:
>>>
>>>>
>>>>> -     brcmf_dbg(INFO, "nmode=%d, vhtmode=%d, bw_cap=(%d, %d)\n",
>>>>> +     brcmf_dbg(INFO,
>>>>> +               "nmode=%d, vhtmode=%d, bw_cap=(%d, %d, %d), he_cap=(%d, %d)\n",
>>>>>      nmode, vhtmode, bw_cap[NL80211_BAND_2GHZ],
>>>>> -               bw_cap[NL80211_BAND_5GHZ]);
>>>>> +               bw_cap[NL80211_BAND_5GHZ], bw_cap[NL80211_BAND_6GHZ],
>>>>> +               he_cap[0], he_cap[1]);
>>>>
>>>> So are these he mac and phy capabilities? ...
>>>
>>> No, unfortunately, it's either 1 or 0 on these chips, and all chips i tested.
>>> This is the hardware capability iovar.
>>>
>>> In the debug firmware i have access to (not apple's), i do see a
>>> command that looks like it may give the he cap, but i can't find how
>>> it would ever be triggered.
>>> (The iovar code for the iovar above is either always just return 0 or return 1)
>>> There are no obvious iovars that relate, and  the absolute latest
>>> bcmdhd hardcodes the he caps, as do infineon's latest ifx code.
>>> :(
>>> I'l hack around see if i can get the caps out of it.
>>>
>>> I'll double check other ones.
>>
>>
>> So, I reached a  conclusion on this piece.
>>
>> This is really an xtlv with subcommands that everyone (including me)
>> is wrongly treating as a non-xtlv.
>> The above is really showing you the enable value.
>> There is also hardware cap value (which is 0/1 as well).
>>
>> In the 4398/4390 firmware, a "defcap" subcommand was added to the
>> firmware which can retrieve the default HE capabilities bytes for the
>> mac and phy and be used to fill them in.
>
> So they adopted the same subcommand I added in our firmware. Hopefully it
> is a true copy.
>
>> However, it is unsupported in the firmware for earlier chips,
>> including these chips (or at least, any firmware i've found for it,
>> apple's or not)
>>
>> As such, at least for these, STA/AP caps will have to be hardcoded.
>>
>> I have updated the code to include the subcommands that exist here,
>> and properly use an xtlv command to retrieve this (it's really a uint8
>> value).
>
> I do have patch lying around here that add xtlv support to fwil.c. Or did I
> already submit that.

It's already there indeed :-D
> Gr. AvS




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

Re: [PATCH 3/5] wifi: brcmfmac: Add support for SCAN_V3

[Arend van Spriel]

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

On 10/19/2023 3:42 AM, Daniel Berlin wrote:
> From: Hector Martin <marcan@marcan.st>
> 
> This is essentially identical to SCAN_V2 with an extra field where we
> had a padding byte, so don't bother duplicating the entire structure.
> Just add the field and the logic to set the version properly.

Reviewed-by: Arend van Spriel <arend.vanspriel@broadcom.com>
> Signed-off-by: Hector Martin <marcan@marcan.st>
> ---
>   .../broadcom/brcm80211/brcmfmac/cfg80211.c    | 20 +++++++++++++------
>   .../broadcom/brcm80211/brcmfmac/feature.c     | 16 ++++++++++++++-
>   .../broadcom/brcm80211/brcmfmac/feature.h     |  1 +
>   .../broadcom/brcm80211/brcmfmac/fwil_types.h  | 15 +++++++++++++-
>   4 files changed, 44 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> index 7143ffe659f6..4cf728368892 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> @@ -1125,6 +1125,7 @@ static void brcmf_scan_params_v2_to_v1(struct brcmf_scan_params_v2_le *params_v2
>   }
>   
>   static void brcmf_escan_prep(struct brcmf_cfg80211_info *cfg,
> +			     struct brcmf_if *ifp,
>   			     struct brcmf_scan_params_v2_le *params_le,
>   			     struct cfg80211_scan_request *request)
>   {
> @@ -1141,8 +1142,13 @@ static void brcmf_escan_prep(struct brcmf_cfg80211_info *cfg,
>   
>   	length = BRCMF_SCAN_PARAMS_V2_FIXED_SIZE;
>   
> -	params_le->version = cpu_to_le16(BRCMF_SCAN_PARAMS_VERSION_V2);
> +	if (brcmf_feat_is_enabled(ifp, BRCMF_FEAT_SCAN_V3))
> +		params_le->version = cpu_to_le16(BRCMF_SCAN_PARAMS_VERSION_V3);
> +	else
> +		params_le->version = cpu_to_le16(BRCMF_SCAN_PARAMS_VERSION_V2);
> +
>   	params_le->bss_type = DOT11_BSSTYPE_ANY;
> +	params_le->ssid_type = 0;
>   	params_le->scan_type = cpu_to_le32(BRCMF_SCANTYPE_ACTIVE);
>   	params_le->channel_num = 0;
>   	params_le->nprobes = cpu_to_le32(-1);
> @@ -1237,7 +1243,7 @@ s32 brcmf_notify_escan_complete(struct brcmf_cfg80211_info *cfg,
>   		/* Do a scan abort to stop the driver's scan engine */
>   		brcmf_dbg(SCAN, "ABORT scan in firmware\n");
>   
> -		brcmf_escan_prep(cfg, &params_v2_le, NULL);
> +		brcmf_escan_prep(cfg, ifp, &params_v2_le, NULL);
>   
>   		/* E-Scan (or anyother type) can be aborted by SCAN */
>   		if (brcmf_feat_is_enabled(ifp, BRCMF_FEAT_SCAN_V2)) {
> @@ -1497,11 +1503,13 @@ brcmf_run_escan(struct brcmf_cfg80211_info *cfg, struct brcmf_if *ifp,
>   		goto exit;
>   	}
>   	BUG_ON(params_size + sizeof("escan") >= BRCMF_DCMD_MEDLEN);
> -	brcmf_escan_prep(cfg, &params->params_v2_le, request);
> +	brcmf_escan_prep(cfg, ifp, &params->params_v2_le, request);
>   
> -	params->version = cpu_to_le32(BRCMF_ESCAN_REQ_VERSION_V2);
> -
> -	if (!brcmf_feat_is_enabled(ifp, BRCMF_FEAT_SCAN_V2)) {
> +	if (brcmf_feat_is_enabled(ifp, BRCMF_FEAT_SCAN_V3)) {
> +		params->version = cpu_to_le32(BRCMF_ESCAN_REQ_VERSION_V3);
> +	} else if (brcmf_feat_is_enabled(ifp, BRCMF_FEAT_SCAN_V2)) {
> +		params->version = cpu_to_le32(BRCMF_ESCAN_REQ_VERSION_V2);
> +	} else {
>   		struct brcmf_escan_params_le *params_v1;
>   
>   		params_size -= BRCMF_SCAN_PARAMS_V2_FIXED_SIZE;
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.c
> index 6d10c9efbe93..c545d3b250e1 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.c
> @@ -287,6 +287,7 @@ static int brcmf_feat_fwcap_debugfs_read(struct seq_file *seq, void *data)
>   void brcmf_feat_attach(struct brcmf_pub *drvr)
>   {
>   	struct brcmf_if *ifp = brcmf_get_ifp(drvr, 0);
> +	struct brcmf_wl_scan_version_le scan_ver;
>   	struct brcmf_pno_macaddr_le pfn_mac;
>   	struct brcmf_gscan_config gscan_cfg;
>   	u32 wowl_cap;
> @@ -337,7 +338,20 @@ void brcmf_feat_attach(struct brcmf_pub *drvr)
>   		ifp->drvr->feat_flags |= BIT(BRCMF_FEAT_SCAN_RANDOM_MAC);
>   
>   	brcmf_feat_iovar_int_get(ifp, BRCMF_FEAT_FWSUP, "sup_wpa");
> -	brcmf_feat_iovar_int_get(ifp, BRCMF_FEAT_SCAN_V2, "scan_ver");
> +
> +	err = brcmf_fil_iovar_data_get(ifp, "scan_ver", &scan_ver, sizeof(scan_ver));
> +	if (!err) {
> +		int ver = le16_to_cpu(scan_ver.scan_ver_major);
> +
> +		if (ver == 2) {
> +			ifp->drvr->feat_flags |= BIT(BRCMF_FEAT_SCAN_V2);
> +		} else if (ver == 3) {
> +			/* We consider SCAN_V3 a subtype of SCAN_V2 since the
> +			 * structure is essentially the same.
> +			 */
> +			ifp->drvr->feat_flags |= BIT(BRCMF_FEAT_SCAN_V2) | BIT(BRCMF_FEAT_SCAN_V3);
> +		}
> +	}
>   
>   	if (drvr->settings->feature_disable) {
>   		brcmf_dbg(INFO, "Features: 0x%02x, disable: 0x%02x\n",
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.h
> index 7f4f0b3e4a7b..3317e80c398a 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.h
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.h
> @@ -56,6 +56,7 @@
>   	BRCMF_FEAT_DEF(FWAUTH) \
>   	BRCMF_FEAT_DEF(DUMP_OBSS) \
>   	BRCMF_FEAT_DEF(SCAN_V2) \
> +	BRCMF_FEAT_DEF(SCAN_V3) \
>   	BRCMF_FEAT_DEF(PMKID_V2) \
>   	BRCMF_FEAT_DEF(PMKID_V3)

Having API versioning marked as feature does not feel right. Will 
probably come up with different mechanism for that.

> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil_types.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil_types.h
> index 9d248ba1c0b2..1077e6f1d61a 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil_types.h
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil_types.h
> @@ -52,6 +52,7 @@
>   
>   /* version of brcmf_scan_params structure */
>   #define BRCMF_SCAN_PARAMS_VERSION_V2	2
> +#define BRCMF_SCAN_PARAMS_VERSION_V3	3
>   
>   /* masks for channel and ssid count */
>   #define BRCMF_SCAN_PARAMS_COUNT_MASK	0x0000ffff
> @@ -72,6 +73,7 @@
>   #define DOT11_BSSTYPE_ANY		2
>   #define BRCMF_ESCAN_REQ_VERSION		1
>   #define BRCMF_ESCAN_REQ_VERSION_V2	2
> +#define BRCMF_ESCAN_REQ_VERSION_V3	3
>   
>   #define BRCMF_MAXRATES_IN_SET		16	/* max # of rates in rateset */
>   
> @@ -414,7 +416,7 @@ struct brcmf_scan_params_v2_le {
>   	s8 bss_type;		/* default: any,
>   				 * DOT11_BSSTYPE_ANY/INFRASTRUCTURE/INDEPENDENT
>   				 */
> -	u8 pad;
> +	u8 ssid_type;		/* v3 only */
>   	__le32 scan_type;	/* flags, 0 use default */
>   	__le32 nprobes;		/* -1 use default, number of probes per channel */
>   	__le32 active_time;	/* -1 use default, dwell time per channel for
> @@ -833,6 +835,17 @@ struct brcmf_wlc_version_le {
>   	__le16 wlc_ver_minor;
>   };
>   
> +/**
> + * struct brcmf_wl_scan_version_le - scan interface version
> + */
> +struct brcmf_wl_scan_version_le {
> +	__le16 version;
> +	__le16 length;
> +	__le16 scan_ver_major;
> +};
> +
> +#define BRCMF_WL_SCAN_VERSION_VERSION 1
> +
>   /**
>    * struct brcmf_assoclist_le - request assoc list.
>    *


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

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

[Arend van Spriel]

[-- 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 --]

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

[Daniel Berlin]

So, at least in the example code I have, all variants of the 109 and
112 structures both have bcnflags in that place - it was always
missing here.
For example, see
https://android.googlesource.com/kernel/google-modules/wlan/bcmdhd/bcm4398/+/refs/heads/android-gs-shusky-5.15-u-qpr1-beta2/include/wlioctl.h
and  compare v109 and v112 of bssinfo.

As such, the 109 and 112 structures are compatible given these definitions.

I don't know if what is there right now is wrong, or it is "yet
another variant" of the 109 structure that we need to handle.
Any idea what the ground truth is?

If we need to separate the structures and support 3 variants (2 109
variants, 1 112 variant), we probably need to start figuring out the
more general structure versioning issue.

We just aren't implementing the bigger ones (for example, there are 5
versions of pfn scanresults, and 6 versions of roam profiles. Most are
still in use, AFAIK)
Any thoughts?

For the bss info here, I suspect I could hide most of this, and use a
structure of function pointers + versioned data that returns the
nn-versioned info that is being asked about (IE the same way we do
with buscore, etc).

IE struct bss_info_data {
void *versioned_data;
nl80211_band (*get_band_for_idx)(struct bss_info_data *, u8 idx);
}

Most of the time we are converting it from this structure to 80211 info, etc.

I haven't looked too hard to see if we are really digging around in
this structure, but I would suspect it could be encapsulated.
This would in turn let us avoid using feature flags to control
structure versions (we would just set them after querying the
firmware), and would make it easier to clean up/refactor code.

I'm open to other ideas as well.


On Fri, Oct 20, 2023 at 5:59 AM Arend van Spriel
<arend.vanspriel@broadcom.com> wrote:
>
> 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 */
>
> [...]

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

[Daniel Berlin]

On Fri, Oct 20, 2023 at 1:31 PM Daniel Berlin <dberlin@dberlin.org> wrote:
>
> So, at least in the example code I have, all variants of the 109 and
> 112 structures both have bcnflags in that place - it was always
> missing here.
> For example, see
> https://android.googlesource.com/kernel/google-modules/wlan/bcmdhd/bcm4398/+/refs/heads/android-gs-shusky-5.15-u-qpr1-beta2/include/wlioctl.h
> and  compare v109 and v112 of bssinfo.
>
> As such, the 109 and 112 structures are compatible given these definitions.
>
> I don't know if what is there right now is wrong, or it is "yet
> another variant" of the 109 structure that we need to handle.
> Any idea what the ground truth is?

Circling back to this - i have checked other sources as well -
infineon also has u8 (though it's marked as padding) there, etc.

As far as i can tell, the structure should have that u8 there in all
versions i can find.

Nevertheless, I think I can make it not matter ;)

I'm going to post an RFC of some patches that handle this and other
structure versioning
things through function pointer structures that we set based on
interface versions available. So instead of setting feature flags, we
query the various iovars/firmware
info for the right interface versions, and set up the
structures/function pointers to handle the versions
we will get from the firmware.

This also makes the common code cleaner as they no longer have to deal
with the structure differences - for example,  brcmf_cfg80211_connect
now just calls
something like drvr->join_params_handler.get_struct_from_connect to
get an extended join params struct of the right version
and doesn't look inside the result anymore. It's an RFC so we can
argue about the approach and APIs, i just did what seemed
easy/obvious.

I have converted bss info, join params, scan params, and netinfo
versions to use this approach, and all now support all versions of the
structures available.

If we go that route, it wont matter if we overlay bss info structures or not.

As an aside, while doing the function pointer work, i discovered some
other structure bugs (missing fields) that seem to be causing
failures/fallbacks on every chip i tested (no matter who the vendor
was).

So for example, some of the extended join parameter substructs are
missing fields, and every chip i tried (5 variants, representing 2
vendors and 3 join_param versions) all gave
BUFTOSHORT when trying to use the join iovar as a result, and all fell
back to using the SET_SSID method.
With the v0/v1 structure fixed, they all use the join iovar successfully and
never fall back.

--Dan

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

[Arend Van Spriel]

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



On October 31, 2023 3:04:12 PM Daniel Berlin <dberlin@dberlin.org> wrote:

> On Fri, Oct 20, 2023 at 1:31 PM Daniel Berlin <dberlin@dberlin.org> wrote:
>>
>> So, at least in the example code I have, all variants of the 109 and
>> 112 structures both have bcnflags in that place - it was always
>> missing here.
>> For example, see
>> https://android.googlesource.com/kernel/google-modules/wlan/bcmdhd/bcm4398/+/refs/heads/android-gs-shusky-5.15-u-qpr1-beta2/include/wlioctl.h
>> and  compare v109 and v112 of bssinfo.
>>
>> As such, the 109 and 112 structures are compatible given these definitions.
>>
>> I don't know if what is there right now is wrong, or it is "yet
>> another variant" of the 109 structure that we need to handle.
>> Any idea what the ground truth is?
>
> Circling back to this - i have checked other sources as well -
> infineon also has u8 (though it's marked as padding) there, etc.
>
> As far as i can tell, the structure should have that u8 there in all
> versions i can find.
>
> Nevertheless, I think I can make it not matter ;)
>
> I'm going to post an RFC of some patches that handle this and other
> structure versioning
> things through function pointer structures that we set based on
> interface versions available. So instead of setting feature flags, we
> query the various iovars/firmware
> info for the right interface versions, and set up the
> structures/function pointers to handle the versions
> we will get from the firmware

The feature flag approach was a concern and the above is more or less what 
I had in mind so curious to see how this takes shape.

Regards,
Arend



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

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

[Hector Martin]

On 20/10/2023 18.59, Arend van Spriel wrote:
> 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.

bcmflags was previously an implied padding byte. If you actually check
the offsets of the subsequent fields, you'll see they haven't changed.
In fact this was added at some point in the past and just missing here,
and is a general case of "padding bytes were not explicitly specified"
which is arguably an anti-pattern and should never have been the case.

Had all the padding been specified correctly from the get go, it would
have been clear that this field was taking over an existing padding
byte, not adding anything nor shifting the offsets of subsequent fields.

> 
>>   	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 */
> 
> [...]

- Hector

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

[Arend van Spriel]

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

On 11/7/2023 12:11 PM, Hector Martin wrote:
> On 20/10/2023 18.59, Arend van Spriel wrote:
>> 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(-)
>>>

[...]

>>> @@ -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.
> 
> bcmflags was previously an implied padding byte. If you actually check
> the offsets of the subsequent fields, you'll see they haven't changed.
> In fact this was added at some point in the past and just missing here,
> and is a general case of "padding bytes were not explicitly specified"
> which is arguably an anti-pattern and should never have been the case.

Yeah. Let's not argue ;-) I did miss something here and leave it with 
that. What about the EHT stuff? I would prefer to keep it out unless 
full EHT support is added.

Regards,
Arend

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

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

[Daniel Berlin]

On Tue, Nov 7, 2023 at 6:51 AM Arend van Spriel
<arend.vanspriel@broadcom.com> wrote:
>
> On 11/7/2023 12:11 PM, Hector Martin wrote:
> > On 20/10/2023 18.59, Arend van Spriel wrote:
> >> 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(-)
> >>>
>
> [...]
>
> >>> @@ -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.
> >
> > bcmflags was previously an implied padding byte. If you actually check
> > the offsets of the subsequent fields, you'll see they haven't changed.
> > In fact this was added at some point in the past and just missing here,
> > and is a general case of "padding bytes were not explicitly specified"
> > which is arguably an anti-pattern and should never have been the case.
>
> Yeah. Let's not argue ;-) I did miss something here and leave it with
> that. What about the EHT stuff? I would prefer to keep it out unless
> full EHT support is added.

The EHT stuff you flagged (defines, etc) in the other patches can be
left out - this one can't since it is definitionally part of the v112
struct.

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

[Arend van Spriel]

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

On 11/7/2023 1:00 PM, Daniel Berlin wrote:
> On Tue, Nov 7, 2023 at 6:51 AM Arend van Spriel
> <arend.vanspriel@broadcom.com> wrote:
>>
>> On 11/7/2023 12:11 PM, Hector Martin wrote:
>>> On 20/10/2023 18.59, Arend van Spriel wrote:
>>>> 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)...
>>>>

Doubts resolved.

Acked-by: Arend van Spriel <arend.vanspriel@broadcom.com>
>>>>> 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(-)

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

Re: [PATCH 5/5] [brcmfmac] Add remaining support for 6G by supporting new scan structures.

[Arend van Spriel]

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

On 10/19/2023 3:42 AM, Daniel Berlin wrote:
> Add support for netinfo v3 and other PNO v3 structures, which contains
> a chanspec rather than just a channel.
> Gate support for netinfo_v3 and other structures on the proper feature
> caps in the firmware.
> 
> Unfortunately, the v3 structures are different enough that we have to
> use different handling for them in places (even the same named fields
> are in different places).

Reviewed-by: Arend van Spriel <arend.vanspriel@broadcom.com>
> Signed-off-by: Daniel Berlin <dberlin@dberlin.org>
> ---
>   .../broadcom/brcm80211/brcmfmac/cfg80211.c    | 239 ++++++++++++++----
>   .../broadcom/brcm80211/brcmfmac/feature.c     |  10 +
>   .../broadcom/brcm80211/brcmfmac/feature.h     |   6 +-
>   .../broadcom/brcm80211/brcmfmac/fwil_types.h  |  62 +++++
>   .../broadcom/brcm80211/brcmfmac/pno.c         | 105 +++++++-
>   .../broadcom/brcm80211/brcmfmac/pno.h         |   9 +
>   6 files changed, 371 insertions(+), 60 deletions(-)
> 
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> index bc8355d7f9b5..3656790ec4c9 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> @@ -32,6 +32,7 @@
>   #include "vendor.h"
>   #include "bus.h"
>   #include "common.h"
> +#include "feature.h"

Suspect this was already include, but no harm being explicit about it.

>   
>   #define BRCMF_SCAN_IE_LEN_MAX		2048
>   

[...]

> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil_types.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil_types.h
> index 81f2d77cb004..b35c27a64db1 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil_types.h
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil_types.h
> @@ -1051,6 +1051,46 @@ struct brcmf_pno_param_le {
>   	__le32 slow_freq;
>   };
>   
> +/**
> + * struct brcmf_pno_param_le - PNO scan configuration parameters

v3?

> + *
> + * @version: PNO parameters version.
> + * @length: Length of PNO structure
> + * @scan_freq: scan frequency.
> + * @lost_network_timeout: #sec. to declare discovered network as lost.
> + * @flags: Bit field to control features of PFN such as sort criteria auto
> + *	enable switch and background scan.
> + * @rssi_margin: Margin to avoid jitter for choosing a PFN based on RSSI sort
> + *	criteria.
> + * @bestn: number of best networks in each scan.
> + * @mscan: number of scans recorded.
> + * @repeat: minimum number of scan intervals before scan frequency changes
> + *	in adaptive scan.
> + * @exp: exponent of 2 for maximum scan interval.
> + * @slow_freq: slow scan period.
> + * @min_bound: min bound for scan time randomization
> + * @max_bound: max bound for scan time randomization
> + * @pfn_lp_scan_disable: unused
> + * @pfn_lp_scan_cnt: allow interleaving lp scan with hp scan
> + */
> +struct brcmf_pno_param_v3_le {

[...]

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