Add support for encoding/decoding 6g chanspecs

Quoted 5 lines · Daniel Berlin

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 <[email protected]>> Signed-off-by: Daniel Berlin <[email protected]>

Quoted 10 lines

---
  .../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(-)

drivers/net/wireless/broadcom/brcm80211/brcmutil/d11.c +0 -0

index 1e2b1e487eb7..faf7eeeeb2d5 100644

[...]

drivers/net/wireless/broadcom/brcm80211/brcmutil/d11.c static void brcmu_d11n_decchspec(struct brcmu_chan *ch) · line 132

Quoted 8 lines

drivers/net/wireless/broadcom/brcm80211/brcmutil/d11.c +3 -1

@@ -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.

drivers/net/wireless/broadcom/brcm80211/brcmutil/d11.c static void brcmu_d11n_decchspec(struct brcmu_chan *ch) · line 145

Quoted 10 lines

drivers/net/wireless/broadcom/brcm80211/brcmutil/d11.c +2 -1

  		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

drivers/net/wireless/broadcom/brcm80211/brcmutil/d11.c static void brcmu_d11ac_decchspec(struct brcmu_chan *ch) · line 174

Quoted 11 lines

drivers/net/wireless/broadcom/brcm80211/brcmutil/d11.c +3 -1

  		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

drivers/net/wireless/broadcom/brcm80211/brcmutil/d11.c static void brcmu_d11ac_decchspec(struct brcmu_chan *ch) · line 197

Quoted 11 lines

drivers/net/wireless/broadcom/brcm80211/brcmutil/d11.c +3 -1

  		}
  		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

drivers/net/wireless/broadcom/brcm80211/brcmutil/d11.c static void brcmu_d11ac_decchspec(struct brcmu_chan *ch) · line 233

Quoted 11 lines

drivers/net/wireless/broadcom/brcm80211/brcmutil/d11.c +3 -1

  			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

Quoted 19 lines

drivers/net/wireless/broadcom/brcm80211/brcmutil/d11.c +6 -1

  			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;

[...]

Quoted 17 lines

drivers/net/wireless/broadcom/brcm80211/include/brcmu_d11.h +9 -0

index f6344023855c..bb48b7442062 100644
@@ -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

drivers/net/wireless/broadcom/brcm80211/include/brcmu_d11.h line 90

Quoted 10 lines

drivers/net/wireless/broadcom/brcm80211/include/brcmu_d11.h +10 -0

+#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.

Quoted 11 lines

drivers/net/wireless/broadcom/brcm80211/include/brcmu_d11.h +0 -9

  #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

drivers/net/wireless/broadcom/brcm80211/include/brcmu_d11.h line 98

Quoted 10 lines

drivers/net/wireless/broadcom/brcm80211/include/brcmu_d11.h +6 -4

-#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.

Quoted 14 lines

drivers/net/wireless/broadcom/brcm80211/include/brcmu_d11.h +8 -0

+#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,

Arend van Spriel

author Reviewed-by

2023-10-20 09:58 UTC #7 lore raw

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

Quoted 9 lines · Daniel Berlin

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

Quoted 10 lines

Signed-off-by Daniel Berlin <[email protected]>
---
  .../broadcom/brcm80211/brcmfmac/cfg80211.c    | 365 +++++++++++++++---
  1 file changed, 313 insertions(+), 52 deletions(-)
drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c +0 -0
index 667462369a32..7143ffe659f6 100644

drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c static struct ieee80211_rate __wl_rates[] = { · line 183

Quoted 6 lines

drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c +3 -0

@@ -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)),

drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c static struct ieee80211_rate __wl_rates[] = { · line 187

Quoted 4 lines

drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c +4 -0
+	.hw_value		= (_channel),			\
+	.max_antenna_gain	= 0,				\
+	.max_power		= 30,				\
+}

so we can drop this one below...

Quoted 172 lines

drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c +71 ~~-20~~

+#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);
+

drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c static s32 brcmf_inform_single_bss(struct brcmf_cfg80211_info *cfg, · line 3437

Quoted 10 lines

drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c +9 -0

+		/* 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.

Quoted 111 lines

drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c +39 -6

+	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:

drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c static int brcmf_construct_chaninfo(struct brcmf_cfg80211_info *cfg, · line 7069

Quoted 4 lines

drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c +1 -0

@@ -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?

Quoted 89 lines

drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c +33 ~~-13~~

+		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,
  	}
  }

drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c static void brcmf_update_vht_cap(struct ieee80211_supported_band *band, · line 7395

Quoted 10 lines

drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c +10 -0

+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.

drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c static void brcmf_update_vht_cap(struct ieee80211_supported_band *band, · line 7400

Quoted 5 lines

drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c +5 -0

+		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.

drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c static void brcmf_update_vht_cap(struct ieee80211_supported_band *band, · line 7401

Quoted 1 line

drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c +1 -0
+	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

Quoted 97 lines

drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c +93 -0

+	/* 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;

drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c static int brcmf_setup_wiphybands(struct brcmf_cfg80211_info *cfg) · line 7504

Quoted 6 lines

drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c +2 -1

@@ -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.

Quoted 15 lines

drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c +2 -0

  	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 {

drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c static int brcmf_setup_wiphybands(struct brcmf_cfg80211_info *cfg) · line 7527

Quoted 10 lines

drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c +5 -3

-		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? ...

drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c static int brcmf_setup_wiphybands(struct brcmf_cfg80211_info *cfg) · line 7569

Quoted 9 lines

drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c +2 -0

  	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?

drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c static int brcmf_setup_wiphy(struct wiphy *wiphy, struct brcmf_if *ifp) · line 7940

Quoted 9 lines

drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c +1 -1

  	}
 
  	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.

Quoted 39 lines

drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c +21 -2

+			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;

drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c struct brcmf_cfg80211_info *brcmf_cfg80211_attach(struct brcmf_pub *drvr, · line 8594

Quoted 10 lines

drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c +3 -6

 
-	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.

Quoted 15 lines

drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c +6 -0

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

Arend van Spriel

author Reviewed-by

2023-10-20 09:58 UTC #14 lore raw

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

Quoted 5 lines · Daniel Berlin

From: Hector Martin <[email protected]>
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 <[email protected]>

Quoted 101 lines

---
  .../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(-)

drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c +14 -6

index 7143ffe659f6..4cf728368892 100644
@@ -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;

drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.c +15 -1

index 6d10c9efbe93..c545d3b250e1 100644
@@ -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",

drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.h +0 -0

index 7f4f0b3e4a7b..3317e80c398a 100644

drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.h line 61

Quoted 6 lines

drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.h +1 -0

@@ -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.

Quoted 47 lines

drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil_types.h +14 -1

index 9d248ba1c0b2..1077e6f1d61a 100644
@@ -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.
   *

Arend van Spriel

author

2023-10-20 09:59 UTC #16 lore raw

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

Quoted 5 lines · Daniel Berlin

From: Hector Martin <[email protected]>
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)...

Quoted 37 lines

---
  .../broadcom/brcm80211/brcmfmac/cfg80211.c    |  5 ++-
  .../broadcom/brcm80211/brcmfmac/fwil_types.h  | 37 +++++++++++++++++--
  2 files changed, 36 insertions(+), 6 deletions(-)

drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c +3 -2

index 4cf728368892..bc8355d7f9b5 100644
@@ -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;
  	}

drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil_types.h +2 -1

index 1077e6f1d61a..81f2d77cb004 100644
@@ -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 */

drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil_types.h struct brcmf_bss_info_le { · line 327

Quoted 4 lines

drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil_types.h +1 -0

@@ -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.

Quoted 6 lines

drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil_types.h +0 -0

  	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 */

[...]

Arend van Spriel

author Reviewed-by

2023-10-20 10:00 UTC #25 lore raw

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

Quoted 8 lines · Daniel Berlin

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

Quoted 15 lines

Signed-off-by Daniel Berlin <[email protected]>

---
  .../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(-)

drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c +0 -0

index bc8355d7f9b5..3656790ec4c9 100644

drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c line 35

Quoted 4 lines

drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c +1 -0
@@ -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.

Quoted 3 lines

drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c +0 -0
  #define BRCMF_SCAN_IE_LEN_MAX		2048
 

[...]

drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil_types.h struct brcmf_pno_param_le { · line 1055

Quoted 10 lines

drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil_types.h +2 -0

index 81f2d77cb004..b35c27a64db1 100644
@@ -1051,6 +1051,46 @@ struct brcmf_pno_param_le {
  	__le32 slow_freq;
  };
 
+/**
+ * struct brcmf_pno_param_le - PNO scan configuration parameters

v3?

Quoted 21 lines

drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil_types.h +21 -0

+ *
+ * @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 {

[...]

Daniel Berlin

2023-10-20 16:35 UTC #8 lore raw

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

Quoted 12 lines

maybe handle channel 2 here as well, ie.:

        .center_freq = ((_channel) == 2) ? 5935 : 5950 + (5 * (_channel)),

Review

Quoted 4 lines

(quoted diff) +4 -0

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

so we can drop this one below...

Will do.

Quoted 13 lines

Review

Quoted 12 lines

(quoted diff) +9 -0

+             /* 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);

Quoted 3 lines

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.

Quoted 10 lines

Review static int brcmf_construct_chaninfo(struct brcmf_cfg80211_info *cfg, · line 7069
Quoted 7 lines
(quoted diff) +1 -0
@@ -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)

Quoted 10 lines

Review static int brcmf_construct_chaninfo(struct brcmf_cfg80211_info *cfg, · line 7074

Quoted 8 lines

(quoted diff) +4 -2

-     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.

Quoted 4 lines

(quoted diff) +0 -0

@@ -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;

Quoted 11 lines

Review struct brcmf_cfg80211_info *brcmf_cfg80211_attach(struct brcmf_pub *drvr, · line 8594

Quoted 9 lines

(quoted diff) +3 -6

-     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.

Quoted 4 lines

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.

Quoted 17 lines

Quoted 16 lines

(quoted diff) +6 -0

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

Daniel Berlin

2023-10-20 16:36 UTC #3 lore raw

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

Quoted 182 lines · Arend van Spriel

Quoted 5 lines · Daniel Berlin

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

Signed-off-by Daniel Berlin <[email protected]>

Quoted 10 lines

---
  .../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(-)

drivers/net/wireless/broadcom/brcm80211/brcmutil/d11.c +0 -0

index 1e2b1e487eb7..faf7eeeeb2d5 100644

[...]

drivers/net/wireless/broadcom/brcm80211/brcmutil/d11.c static void brcmu_d11n_decchspec(struct brcmu_chan *ch) · line 132

Quoted 8 lines

drivers/net/wireless/broadcom/brcm80211/brcmutil/d11.c +3 -1

@@ -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.

drivers/net/wireless/broadcom/brcm80211/brcmutil/d11.c static void brcmu_d11n_decchspec(struct brcmu_chan *ch) · line 145

Quoted 10 lines

drivers/net/wireless/broadcom/brcm80211/brcmutil/d11.c +2 -1

              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

drivers/net/wireless/broadcom/brcm80211/brcmutil/d11.c static void brcmu_d11ac_decchspec(struct brcmu_chan *ch) · line 174

Quoted 11 lines

drivers/net/wireless/broadcom/brcm80211/brcmutil/d11.c +3 -1

              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

drivers/net/wireless/broadcom/brcm80211/brcmutil/d11.c static void brcmu_d11ac_decchspec(struct brcmu_chan *ch) · line 197

Quoted 11 lines

drivers/net/wireless/broadcom/brcm80211/brcmutil/d11.c +3 -1

              }
              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

drivers/net/wireless/broadcom/brcm80211/brcmutil/d11.c static void brcmu_d11ac_decchspec(struct brcmu_chan *ch) · line 233

Quoted 11 lines

drivers/net/wireless/broadcom/brcm80211/brcmutil/d11.c +3 -1

                      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

Quoted 19 lines

drivers/net/wireless/broadcom/brcm80211/brcmutil/d11.c +6 -1

                      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;

[...]

Quoted 17 lines

drivers/net/wireless/broadcom/brcm80211/include/brcmu_d11.h +9 -0

index f6344023855c..bb48b7442062 100644
@@ -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

drivers/net/wireless/broadcom/brcm80211/include/brcmu_d11.h line 90

Quoted 10 lines

drivers/net/wireless/broadcom/brcm80211/include/brcmu_d11.h +10 -0

+#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.

Quoted 11 lines

drivers/net/wireless/broadcom/brcm80211/include/brcmu_d11.h +0 -9

  #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

drivers/net/wireless/broadcom/brcm80211/include/brcmu_d11.h line 98

Quoted 10 lines

drivers/net/wireless/broadcom/brcm80211/include/brcmu_d11.h +6 -4

-#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.

Quoted 14 lines

drivers/net/wireless/broadcom/brcm80211/include/brcmu_d11.h +8 -0

+#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,

Daniel Berlin

2023-10-20 17:32 UTC #17 lore raw

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

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.

Quoted 67 lines · Arend van Spriel

Quoted 5 lines · Daniel Berlin

From: Hector Martin <[email protected]>
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)...

Quoted 37 lines

In reply to Daniel Berlin

---
  .../broadcom/brcm80211/brcmfmac/cfg80211.c    |  5 ++-
  .../broadcom/brcm80211/brcmfmac/fwil_types.h  | 37 +++++++++++++++++--
  2 files changed, 36 insertions(+), 6 deletions(-)

drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c +3 -2

index 4cf728368892..bc8355d7f9b5 100644
@@ -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;
      }

drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil_types.h +2 -1

index 1077e6f1d61a..81f2d77cb004 100644
@@ -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 */

drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil_types.h struct brcmf_bss_info_le { · line 327

Quoted 4 lines

drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil_types.h +1 -0

@@ -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 */

Quoted 6 lines

drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil_types.h +0 -0

      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 */

[...]

Arend van Spriel

author

2023-10-20 18:37 UTC #9 lore raw

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

Quoted 11 lines · Daniel Berlin

Quoted 7 lines
Review static int brcmf_construct_chaninfo(struct brcmf_cfg80211_info *cfg, · line 7069
Quoted 5 lines
(quoted diff) +1 -0
@@ -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

Jeff Johnson

2023-10-20 21:46 UTC #4 lore raw

In reply to Daniel Berlin

Quoted 3 lines · Arend van Spriel

Quoted 2 lines · Daniel Berlin
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/>

Arend van Spriel

author

2023-10-21 18:29 UTC #5 lore raw

Quoted 6 lines · Jeff Johnson

Quoted 3 lines · Arend van Spriel
Quoted 2 lines · Daniel Berlin
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.

Quoted 2 lines

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

Daniel Berlin

2023-10-23 11:42 UTC #10 lore raw

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

Quoted 24 lines · Daniel Berlin

Quoted 11 lines
Review
Quoted 9 lines
(quoted diff) +4 -2
-     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.

Quoted 1 line

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).

Arend Van Spriel

author

2023-10-23 18:06 UTC #11 lore raw

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

Quoted 30 lines · Daniel Berlin

Quoted 24 lines · Daniel Berlin
Quoted 11 lines
Review
Quoted 9 lines
(quoted diff) +4 -2
-     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.
Quoted 1 line
I'll double check other ones.
So, I reached a conclusion on this piece.

Quoted 8 lines

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.

Quoted 9 lines

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

Arend Van Spriel

author

2023-10-23 18:09 UTC #12 lore raw

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

Quoted 48 lines · Arend Van Spriel

Quoted 30 lines · Daniel Berlin
Quoted 24 lines · Daniel Berlin
Quoted 11 lines
Review
Quoted 9 lines
(quoted diff) +4 -2
-     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.
Quoted 1 line
I'll double check other ones.
So, I reached a conclusion on this piece.
Quoted 8 lines
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.
Quoted 4 lines
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)

Quoted 8 lines

Quoted 5 lines
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

Quoted 1 line

Gr. AvS

Daniel Berlin

2023-10-31 14:04 UTC #18 lore raw

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

In reply to Daniel Berlin

Quoted 8 lines · 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.

Quoted 5 lines

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

Arend Van Spriel

author

2023-10-31 16:27 UTC #19 lore raw

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

Quoted 23 lines · Daniel Berlin

Quoted 8 lines · 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.
Quoted 5 lines
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 ;)

Quoted 8 lines

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

Hector Martin

2023-11-07 11:11 UTC #20 lore raw

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

Quoted 46 lines · Arend van Spriel

Quoted 5 lines · Daniel Berlin

From: Hector Martin <[email protected]>
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)...

Quoted 35 lines