[RFC v3 0/8] cfg80211/mac80211: support defining multiple radios per wiphy

[RFC v3 0/8] cfg80211/mac80211: support defining multiple radios per wiphy

[Felix Fietkau]

The prerequisite for MLO support in cfg80211/mac80211 is that all the links
participating in MLO must be from the same wiphy/ieee80211_hw. To meet this
expectation, some drivers may need to group multiple discrete hardware each
acting as a link in MLO under single wiphy.

With this series, the bands and supported frequencies of each individual
radio are reported to user space. This allows user space to figure out the
limitations of what combination of channels can be used concurrently.

Each mac80211 channel context is assigned to a radio based on radio specific
frequency ranges and interface combinations.

This is loosely based on Karthikeyan Periyasamy's series
"[PATCH 00/13] wifi: Add multi physical hardware iface combination support"
with some differences:

 - a struct wiphy_radio is defined, which holds the frequency ranges and
   a full struct ieee80211_iface_combination array
 - a channel context is explicitly assigned to a radio when created
 - both global and per-radio interface combination limits are checked
   and enforced on channel context assignment

Changes since RFC v2:
 - fix uninitialized variables
 - fix multiple radios with DFS
 - add support for per-radio beacon interval checking

Changes since RFC:
 - replace static per-radio number of channels limit with full ifcomb
   checks
 - remove band bitmask in favor of only using freq ranges

Felix Fietkau (8):
  wifi: nl80211: split helper function from nl80211_put_iface_combinations
  wifi: cfg80211: add support for advertising multiple radios belonging to a wiphy
  wifi: cfg80211: extend interface combination check for multi-radio
  wifi: mac80211: add support for DFS with multiple radios
  wifi: mac80211: add radio index to ieee80211_chanctx_conf
  wifi: mac80211: extend ifcomb check functions for multi-radio
  wifi: mac80211: move code in ieee80211_link_reserve_chanctx to a helper
  wifi: mac80211: add wiphy radio assignment and validation

 include/net/cfg80211.h       |  43 +++++++-
 include/net/mac80211.h       |   2 +-
 include/uapi/linux/nl80211.h |  48 ++++++++-
 net/mac80211/cfg.c           |   7 +-
 net/mac80211/chan.c          | 228 +++++++++++++++++++++++-------------
 net/mac80211/ibss.c          |   2 +-
 net/mac80211/ieee80211_i.h   |   6 +-
 net/mac80211/iface.c         |   2 +-
 net/mac80211/main.c          |  32 +++--
 net/mac80211/util.c          | 131 +++++++++++++++------
 net/wireless/nl80211.c       | 190 +++++++++++++++++++++---------
 net/wireless/rdev-ops.h      |  17 +++-
 net/wireless/trace.h         |   5 +-
 net/wireless/util.c          |  36 ++++--
 14 files changed, 560 insertions(+), 189 deletions(-)

base-commit: 5bcd9a0a59953b5ff55ac226f903397319bf7f40
-- 
git-series 0.9.1

[RFC v3 1/8] wifi: nl80211: split helper function from nl80211_put_iface_combinations

[Felix Fietkau]

Create a helper function that puts the data from struct
ieee80211_iface_combination to a nl80211 message.
This will be used for adding per-radio interface combination data.

Signed-off-by: Felix Fietkau <nbd@nbd.name>
---
 net/wireless/nl80211.c | 111 ++++++++++++++++++++++--------------------
 1 file changed, 59 insertions(+), 52 deletions(-)

diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 8ff5f79d446a..7b0ba0fff082 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -1622,71 +1622,78 @@ static int nl80211_put_iftypes(struct sk_buff *msg, u32 attr, u16 ifmodes)
 	return -ENOBUFS;
 }
 
-static int nl80211_put_iface_combinations(struct wiphy *wiphy,
-					  struct sk_buff *msg,
-					  bool large)
+static int nl80211_put_ifcomb_data(struct sk_buff *msg, bool large, int idx,
+				   const struct ieee80211_iface_combination *c)
 {
-	struct nlattr *nl_combis;
-	int i, j;
+	struct nlattr *nl_combi, *nl_limits;
+	int i;
 
-	nl_combis = nla_nest_start_noflag(msg,
-					  NL80211_ATTR_INTERFACE_COMBINATIONS);
-	if (!nl_combis)
+	nl_combi = nla_nest_start_noflag(msg, idx);
+	if (!nl_combi)
 		goto nla_put_failure;
 
-	for (i = 0; i < wiphy->n_iface_combinations; i++) {
-		const struct ieee80211_iface_combination *c;
-		struct nlattr *nl_combi, *nl_limits;
+	nl_limits = nla_nest_start_noflag(msg, NL80211_IFACE_COMB_LIMITS);
+	if (!nl_limits)
+		goto nla_put_failure;
 
-		c = &wiphy->iface_combinations[i];
+	for (i = 0; i < c->n_limits; i++) {
+		struct nlattr *nl_limit;
 
-		nl_combi = nla_nest_start_noflag(msg, i + 1);
-		if (!nl_combi)
+		nl_limit = nla_nest_start_noflag(msg, i + 1);
+		if (!nl_limit)
 			goto nla_put_failure;
-
-		nl_limits = nla_nest_start_noflag(msg,
-						  NL80211_IFACE_COMB_LIMITS);
-		if (!nl_limits)
+		if (nla_put_u32(msg, NL80211_IFACE_LIMIT_MAX, c->limits[i].max))
 			goto nla_put_failure;
+		if (nl80211_put_iftypes(msg, NL80211_IFACE_LIMIT_TYPES,
+					c->limits[i].types))
+			goto nla_put_failure;
+		nla_nest_end(msg, nl_limit);
+	}
 
-		for (j = 0; j < c->n_limits; j++) {
-			struct nlattr *nl_limit;
+	nla_nest_end(msg, nl_limits);
 
-			nl_limit = nla_nest_start_noflag(msg, j + 1);
-			if (!nl_limit)
-				goto nla_put_failure;
-			if (nla_put_u32(msg, NL80211_IFACE_LIMIT_MAX,
-					c->limits[j].max))
-				goto nla_put_failure;
-			if (nl80211_put_iftypes(msg, NL80211_IFACE_LIMIT_TYPES,
-						c->limits[j].types))
-				goto nla_put_failure;
-			nla_nest_end(msg, nl_limit);
-		}
+	if (c->beacon_int_infra_match &&
+	    nla_put_flag(msg, NL80211_IFACE_COMB_STA_AP_BI_MATCH))
+		goto nla_put_failure;
+	if (nla_put_u32(msg, NL80211_IFACE_COMB_NUM_CHANNELS,
+			c->num_different_channels) ||
+	    nla_put_u32(msg, NL80211_IFACE_COMB_MAXNUM,
+			c->max_interfaces))
+		goto nla_put_failure;
+	if (large &&
+	    (nla_put_u32(msg, NL80211_IFACE_COMB_RADAR_DETECT_WIDTHS,
+			c->radar_detect_widths) ||
+	     nla_put_u32(msg, NL80211_IFACE_COMB_RADAR_DETECT_REGIONS,
+			c->radar_detect_regions)))
+		goto nla_put_failure;
+	if (c->beacon_int_min_gcd &&
+	    nla_put_u32(msg, NL80211_IFACE_COMB_BI_MIN_GCD,
+			c->beacon_int_min_gcd))
+		goto nla_put_failure;
 
-		nla_nest_end(msg, nl_limits);
+	nla_nest_end(msg, nl_combi);
 
-		if (c->beacon_int_infra_match &&
-		    nla_put_flag(msg, NL80211_IFACE_COMB_STA_AP_BI_MATCH))
-			goto nla_put_failure;
-		if (nla_put_u32(msg, NL80211_IFACE_COMB_NUM_CHANNELS,
-				c->num_different_channels) ||
-		    nla_put_u32(msg, NL80211_IFACE_COMB_MAXNUM,
-				c->max_interfaces))
-			goto nla_put_failure;
-		if (large &&
-		    (nla_put_u32(msg, NL80211_IFACE_COMB_RADAR_DETECT_WIDTHS,
-				c->radar_detect_widths) ||
-		     nla_put_u32(msg, NL80211_IFACE_COMB_RADAR_DETECT_REGIONS,
-				c->radar_detect_regions)))
-			goto nla_put_failure;
-		if (c->beacon_int_min_gcd &&
-		    nla_put_u32(msg, NL80211_IFACE_COMB_BI_MIN_GCD,
-				c->beacon_int_min_gcd))
-			goto nla_put_failure;
+	return 0;
+nla_put_failure:
+	return -ENOBUFS;
+}
 
-		nla_nest_end(msg, nl_combi);
-	}
+static int nl80211_put_iface_combinations(struct wiphy *wiphy,
+					  struct sk_buff *msg,
+					  bool large)
+{
+	struct nlattr *nl_combis;
+	int i;
+
+	nl_combis = nla_nest_start_noflag(msg,
+					  NL80211_ATTR_INTERFACE_COMBINATIONS);
+	if (!nl_combis)
+		goto nla_put_failure;
+
+	for (i = 0; i < wiphy->n_iface_combinations; i++)
+		if (nl80211_put_ifcomb_data(msg, large, i + 1,
+					    &wiphy->iface_combinations[i]))
+			goto nla_put_failure;
 
 	nla_nest_end(msg, nl_combis);
 
-- 
git-series 0.9.1

[RFC v3 2/8] wifi: cfg80211: add support for advertising multiple radios belonging to a wiphy

[Felix Fietkau]

The prerequisite for MLO support in cfg80211/mac80211 is that all the links
participating in MLO must be from the same wiphy/ieee80211_hw. To meet this
expectation, some drivers may need to group multiple discrete hardware each
acting as a link in MLO under single wiphy.
With this change, supported frequencies and interface combinations of each
individual radio are reported to user space.
This allows user space to figure out the limitations of what combination of
channels can be used concurrently.

Signed-off-by: Felix Fietkau <nbd@nbd.name>
---
 include/net/cfg80211.h       | 38 ++++++++++++++++++-
 include/uapi/linux/nl80211.h | 48 ++++++++++++++++++++++-
 net/wireless/nl80211.c       | 79 +++++++++++++++++++++++++++++++++++++-
 3 files changed, 165 insertions(+)

diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index 5da9bb0ac6a4..27355e08ae52 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -5403,6 +5403,38 @@ struct wiphy_iftype_akm_suites {
 	int n_akm_suites;
 };
 
+/**
+ * struct wiphy_radio_freq_range - wiphy frequency range
+ * @start_freq:  start range edge frequency (kHz)
+ * @end_freq:    end range edge frequency (kHz)
+ */
+struct wiphy_radio_freq_range {
+	u32 start_freq;
+	u32 end_freq;
+};
+
+
+/**
+ * struct wiphy_radio - This structure describes a physical radio belonging
+ * to a wiphy. It is used to describe concurrent-channel capabilities of the
+ * phy. Only one channel can be active on the radio described by struct
+ * wiphy_radio.
+ *
+ * @freq_range: frequency range that the radio can operate on.
+ * @n_freq_range: number of elements in @freq_range
+ *
+ * @iface_combinations: Valid interface combinations array, should not
+ *	list single interface types.
+ * @n_iface_combinations: number of entries in @iface_combinations array.
+ */
+struct wiphy_radio {
+	const struct wiphy_radio_freq_range *freq_range;
+	int n_freq_range;
+
+	const struct ieee80211_iface_combination *iface_combinations;
+	int n_iface_combinations;
+};
+
 #define CFG80211_HW_TIMESTAMP_ALL_PEERS	0xffff
 
 /**
@@ -5621,6 +5653,9 @@ struct wiphy_iftype_akm_suites {
  *	A value of %CFG80211_HW_TIMESTAMP_ALL_PEERS indicates the driver
  *	supports enabling HW timestamping for all peers (i.e. no need to
  *	specify a mac address).
+ *
+ * @radio: radios belonging to this wiphy
+ * @n_radio: number of radios
  */
 struct wiphy {
 	struct mutex mtx;
@@ -5771,6 +5806,9 @@ struct wiphy {
 
 	u16 hw_timestamp_max_peers;
 
+	const struct wiphy_radio *radio;
+	int n_radio;
+
 	char priv[] __aligned(NETDEV_ALIGN);
 };
 
diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
index f917bc6c9b6f..784bf7501d97 100644
--- a/include/uapi/linux/nl80211.h
+++ b/include/uapi/linux/nl80211.h
@@ -3401,6 +3401,8 @@ enum nl80211_attrs {
 
 	NL80211_ATTR_ASSOC_SPP_AMSDU,
 
+	NL80211_ATTR_RADIOS,
+
 	/* add attributes here, update the policy in nl80211.c */
 
 	__NL80211_ATTR_AFTER_LAST,
@@ -7999,4 +8001,50 @@ enum nl80211_ap_settings_flags {
 	NL80211_AP_SETTINGS_SA_QUERY_OFFLOAD_SUPPORT	= 1 << 1,
 };
 
+/**
+ * enum nl80211_wiphy_radio_attrs - wiphy radio attributes
+ *
+ * @__NL80211_WIPHY_RADIO_ATTR_INVALID: Invalid
+ *
+ * @NL80211_WIPHY_RADIO_ATTR_FREQ_RANGES: Nested array of frequency ranges
+ *	supported by this radio.
+ * @NL80211_WIPHY_RADIO_ATTR_INTERFACE_COMBINATIONS: Nested attribute listing
+ *	the supported interface combinations for this radio. In each nested item,
+ *	it contains attributes defined in &enum nl80211_if_combination_attrs.
+ *
+ * @__NL80211_WIPHY_RADIO_ATTR_LAST: Internal
+ * @NL80211_WIPHY_RADIO_ATTR_MAX: Highest attribute
+ */
+enum nl80211_wiphy_radio_attrs {
+	__NL80211_WIPHY_RADIO_ATTR_INVALID,
+
+	NL80211_WIPHY_RADIO_ATTR_FREQ_RANGES,
+	NL80211_WIPHY_RADIO_ATTR_INTERFACE_COMBINATIONS,
+
+	/* keep last */
+	__NL80211_WIPHY_RADIO_ATTR_LAST,
+	NL80211_WIPHY_RADIO_ATTR_MAX = __NL80211_WIPHY_RADIO_ATTR_LAST - 1,
+};
+
+/**
+ * enum nl80211_wiphy_radio_freq_range - wiphy radio frequency range
+ *
+ * @__NL80211_WIPHY_RADIO_FREQ_ATTR_INVALID: Invalid
+ *
+ * @NL80211_WIPHY_RADIO_FREQ_ATTR_START: Frequency range start
+ * @NL80211_WIPHY_RADIO_FREQ_ATTR_END: Frequency range end
+ *
+ * @__NL80211_WIPHY_RADIO_FREQ_ATTR_LAST: Internal
+ * @NL80211_WIPHY_RADIO_FREQ_ATTR_MAX: Highest attribute
+ */
+enum nl80211_wiphy_radio_freq_range {
+	__NL80211_WIPHY_RADIO_FREQ_ATTR_INVALID,
+
+	NL80211_WIPHY_RADIO_FREQ_ATTR_START,
+	NL80211_WIPHY_RADIO_FREQ_ATTR_END,
+
+	__NL80211_WIPHY_RADIO_FREQ_ATTR_LAST,
+	NL80211_WIPHY_RADIO_FREQ_ATTR_MAX = __NL80211_WIPHY_RADIO_FREQ_ATTR_LAST - 1,
+};
+
 #endif /* __LINUX_NL80211_H */
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 7b0ba0fff082..a8e3a08e908d 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -2399,6 +2399,79 @@ static int nl80211_put_mbssid_support(struct wiphy *wiphy, struct sk_buff *msg)
 	return -ENOBUFS;
 }
 
+static int nl80211_put_radio(struct wiphy *wiphy, struct sk_buff *msg, int idx)
+{
+	const struct wiphy_radio *r = &wiphy->radio[idx];
+	struct nlattr *radio, *freqs, *freq, *nl_combis;
+	int i;
+
+	radio = nla_nest_start(msg, idx);
+	if (!radio)
+		return -ENOBUFS;
+
+	freqs = nla_nest_start_noflag(msg, NL80211_WIPHY_RADIO_ATTR_FREQ_RANGES);
+	if (!freqs)
+		goto nla_put_failure;
+
+	for (i = 0; i < r->n_freq_range; i++) {
+		const struct wiphy_radio_freq_range *range = &r->freq_range[i];
+		int ret;
+
+		freq = nla_nest_start(msg, i);
+		ret = nla_put_u32(msg, NL80211_WIPHY_RADIO_FREQ_ATTR_START,
+				  range->start_freq) ||
+		      nla_put_u32(msg, NL80211_WIPHY_RADIO_FREQ_ATTR_END,
+				  range->end_freq);
+		nla_nest_end(msg, freq);
+
+		if (ret)
+			goto nla_put_failure;
+	}
+
+	nla_nest_end(msg, freqs);
+
+	nl_combis = nla_nest_start_noflag(msg,
+					  NL80211_WIPHY_RADIO_ATTR_INTERFACE_COMBINATIONS);
+	if (!nl_combis)
+		goto nla_put_failure;
+
+	for (i = 0; i < r->n_iface_combinations; i++)
+		if (nl80211_put_ifcomb_data(msg, true, i + 1,
+					    &r->iface_combinations[i]))
+			goto nla_put_failure;
+
+	nla_nest_end(msg, nl_combis);
+	nla_nest_end(msg, radio);
+	return 0;
+
+nla_put_failure:
+	return -ENOBUFS;
+}
+
+static int nl80211_put_radios(struct wiphy *wiphy, struct sk_buff *msg)
+{
+	struct nlattr *radios;
+	int i;
+
+	if (!wiphy->n_radio)
+		return 0;
+
+	radios = nla_nest_start(msg, NL80211_ATTR_RADIOS);
+	if (!radios)
+		return -ENOBUFS;
+
+	for (i = 0; i < wiphy->n_radio; i++)
+		if (nl80211_put_radio(wiphy, msg, i))
+			goto fail;
+
+	nla_nest_end(msg, radios);
+	return 0;
+
+fail:
+	nla_nest_cancel(msg, radios);
+	return -ENOBUFS;
+}
+
 struct nl80211_dump_wiphy_state {
 	s64 filter_wiphy;
 	long start;
@@ -3008,6 +3081,12 @@ static int nl80211_send_wiphy(struct cfg80211_registered_device *rdev,
 				rdev->wiphy.hw_timestamp_max_peers))
 			goto nla_put_failure;
 
+		state->split_start++;
+		break;
+	case 17:
+		if (nl80211_put_radios(&rdev->wiphy, msg))
+			goto nla_put_failure;
+
 		/* done */
 		state->split_start = 0;
 		break;
-- 
git-series 0.9.1

Re: [RFC v3 2/8] wifi: cfg80211: add support for advertising multiple radios belonging to a wiphy

[Johannes Berg]

On Thu, 2024-06-06 at 20:07 +0200, Felix Fietkau wrote:

> The prerequisite for MLO support in cfg80211/mac80211 is that all the links
> participating in MLO must be from the same wiphy/ieee80211_hw. To meet this
> expectation, some drivers may need to group multiple discrete hardware each
> acting as a link in MLO under single wiphy.

This is of course the motivation now, but I do wonder if this wouldn't
potentially also apply to a single device that's full dual-band capable
in some way? But doesn't really matter now.

But the thing is that it would let userspace differentiate between what
we mostly have today in a single device (multiple channels can be used,
but you have to go to powersave etc.), vs. a fully concurrent device.

IOW, it feels like this could be used to advertise fully concurrent
capabilities?

> + * struct wiphy_radio - This structure describes a physical radio belonging
> + * to a wiphy. It is used to describe concurrent-channel capabilities of the
> + * phy. Only one channel can be active on the radio described by struct
> + * wiphy_radio.

that's a bit long for the 'short description' :P

maybe just say "struct wiphy_radio - physical radio of a wiphy" and move
the full description down.

> + *
> + * @radio: radios belonging to this wiphy
> + * @n_radio: number of radios

Somewhere - either here or above - we should probably say that it's
assumed you only have a single radio (with the properties covered by the
interface combinations in the wiphy itself) if this isn't given at all.

(Which is what we assume today, more or less.)

> +++ b/include/uapi/linux/nl80211.h
> @@ -3401,6 +3401,8 @@ enum nl80211_attrs {
>  
>  	NL80211_ATTR_ASSOC_SPP_AMSDU,
>  
> +	NL80211_ATTR_RADIOS,

missing docs

> +/**
> + * enum nl80211_wiphy_radio_attrs - wiphy radio attributes
> + *
> + * @__NL80211_WIPHY_RADIO_ATTR_INVALID: Invalid

maybe if this is WIPHY_RADIO also call it NL80211_ATTR_WIPHY_RADIOS
above?

> + * @NL80211_WIPHY_RADIO_ATTR_FREQ_RANGES: Nested array of frequency ranges
> + *	supported by this radio.

Do we really want this complexity? We only have a single range now, do
we expect that to change? Non-contiguous ranges, where a hole in the
middle is supported by another radio?

Not sure I see the value vs. just having min/max freq directly here?

> +	freqs = nla_nest_start_noflag(msg, NL80211_WIPHY_RADIO_ATTR_FREQ_RANGES);

Please don't add new _noflag code.

> +	nl_combis = nla_nest_start_noflag(msg,
> +					  NL80211_WIPHY_RADIO_ATTR_INTERFACE_COMBINATIONS);

same here

(and yes maybe userspace wants to unify the parsing of this with the
existing interface combinations attribute and pass the attribute ID or
something, but then it can fix the nested flag too.)

johannes

Re: [RFC v3 2/8] wifi: cfg80211: add support for advertising multiple radios belonging to a wiphy

[Felix Fietkau]

On 07.06.24 11:24, Johannes Berg wrote:
> On Thu, 2024-06-06 at 20:07 +0200, Felix Fietkau wrote:
> 
>> The prerequisite for MLO support in cfg80211/mac80211 is that all the links
>> participating in MLO must be from the same wiphy/ieee80211_hw. To meet this
>> expectation, some drivers may need to group multiple discrete hardware each
>> acting as a link in MLO under single wiphy.
> 
> This is of course the motivation now, but I do wonder if this wouldn't
> potentially also apply to a single device that's full dual-band capable
> in some way? But doesn't really matter now.
> 
> But the thing is that it would let userspace differentiate between what
> we mostly have today in a single device (multiple channels can be used,
> but you have to go to powersave etc.), vs. a fully concurrent device.
> 
> IOW, it feels like this could be used to advertise fully concurrent
> capabilities?

Yes, that's my intention with the patches as well. I will update the 
description.

>> + * struct wiphy_radio - This structure describes a physical radio belonging
>> + * to a wiphy. It is used to describe concurrent-channel capabilities of the
>> + * phy. Only one channel can be active on the radio described by struct
>> + * wiphy_radio.
> 
> that's a bit long for the 'short description' :P
> 
> maybe just say "struct wiphy_radio - physical radio of a wiphy" and move
> the full description down.

Sure

>> + *
>> + * @radio: radios belonging to this wiphy
>> + * @n_radio: number of radios
> 
> Somewhere - either here or above - we should probably say that it's
> assumed you only have a single radio (with the properties covered by the
> interface combinations in the wiphy itself) if this isn't given at all.
> 
> (Which is what we assume today, more or less.)
> 
>> +++ b/include/uapi/linux/nl80211.h
>> @@ -3401,6 +3401,8 @@ enum nl80211_attrs {
>>  
>>  	NL80211_ATTR_ASSOC_SPP_AMSDU,
>>  
>> +	NL80211_ATTR_RADIOS,
> 
> missing docs
> 
>> +/**
>> + * enum nl80211_wiphy_radio_attrs - wiphy radio attributes
>> + *
>> + * @__NL80211_WIPHY_RADIO_ATTR_INVALID: Invalid
> 
> maybe if this is WIPHY_RADIO also call it NL80211_ATTR_WIPHY_RADIOS
> above?

Will do

>> + * @NL80211_WIPHY_RADIO_ATTR_FREQ_RANGES: Nested array of frequency ranges
>> + *	supported by this radio.
> 
> Do we really want this complexity? We only have a single range now, do
> we expect that to change? Non-contiguous ranges, where a hole in the
> middle is supported by another radio?
> 
> Not sure I see the value vs. just having min/max freq directly here?

I think there's hardware where one of the radios can be dual-band 
(non-concurrent).

>> +	freqs = nla_nest_start_noflag(msg, NL80211_WIPHY_RADIO_ATTR_FREQ_RANGES);
> 
> Please don't add new _noflag code.
> 
>> +	nl_combis = nla_nest_start_noflag(msg,
>> +					  NL80211_WIPHY_RADIO_ATTR_INTERFACE_COMBINATIONS);
> 
> same here
> 
> (and yes maybe userspace wants to unify the parsing of this with the
> existing interface combinations attribute and pass the attribute ID or
> something, but then it can fix the nested flag too.)

Will remove _noflag.

- Felix

[RFC v3 3/8] wifi: cfg80211: extend interface combination check for multi-radio

[Felix Fietkau]

Add a field in struct iface_combination_params to check per-radio
interface combinations instead of per-wiphy ones.

Signed-off-by: Felix Fietkau <nbd@nbd.name>
---
 include/net/cfg80211.h  |  5 +++++
 net/wireless/rdev-ops.h | 17 +++++++++++++++++
 net/wireless/trace.h    |  5 +++++
 net/wireless/util.c     | 36 +++++++++++++++++++++++++++++-------
 4 files changed, 56 insertions(+), 7 deletions(-)

diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index 27355e08ae52..35d4d18e0500 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -1595,6 +1595,7 @@ struct cfg80211_color_change_settings {
  *
  * Used to pass interface combination parameters
  *
+ * @radio: when set, check radio specific interface combinations.
  * @num_different_channels: the number of different channels we want
  *	to use for verification
  * @radar_detect: a bitmap where each bit corresponds to a channel
@@ -1608,6 +1609,7 @@ struct cfg80211_color_change_settings {
  *	the verification
  */
 struct iface_combination_params {
+	const struct wiphy_radio *radio;
 	int num_different_channels;
 	u8 radar_detect;
 	int iftype_num[NUM_NL80211_IFTYPES];
@@ -4577,6 +4579,7 @@ struct mgmt_frame_regs {
  *
  * @set_hw_timestamp: Enable/disable HW timestamping of TM/FTM frames.
  * @set_ttlm: set the TID to link mapping.
+ * @get_radio_mask: get bitmask of radios in use
  */
 struct cfg80211_ops {
 	int	(*suspend)(struct wiphy *wiphy, struct cfg80211_wowlan *wow);
@@ -4938,6 +4941,8 @@ struct cfg80211_ops {
 				    struct cfg80211_set_hw_timestamp *hwts);
 	int	(*set_ttlm)(struct wiphy *wiphy, struct net_device *dev,
 			    struct cfg80211_ttlm_params *params);
+	int	(*get_radio_mask)(struct wiphy *wiphy, struct net_device *dev,
+				  u32 *mask);
 };
 
 /*
diff --git a/net/wireless/rdev-ops.h b/net/wireless/rdev-ops.h
index 43897a5269b6..43d72d09ef34 100644
--- a/net/wireless/rdev-ops.h
+++ b/net/wireless/rdev-ops.h
@@ -1542,4 +1542,21 @@ rdev_set_ttlm(struct cfg80211_registered_device *rdev,
 
 	return ret;
 }
+
+static inline int
+rdev_get_radio_mask(struct cfg80211_registered_device *rdev,
+		    struct net_device *dev, u32 *mask)
+{
+	struct wiphy *wiphy = &rdev->wiphy;
+	int ret;
+
+	if (!rdev->ops->get_radio_mask)
+		return -EOPNOTSUPP;
+
+	trace_rdev_get_radio_mask(wiphy, dev);
+	ret = rdev->ops->get_radio_mask(wiphy, dev, mask);
+	trace_rdev_return_int(wiphy, ret);
+
+	return ret;
+}
 #endif /* __CFG80211_RDEV_OPS */
diff --git a/net/wireless/trace.h b/net/wireless/trace.h
index 14cfa0aba93a..12081e81f9c5 100644
--- a/net/wireless/trace.h
+++ b/net/wireless/trace.h
@@ -4081,6 +4081,11 @@ TRACE_EVENT(cfg80211_links_removed,
 		  __entry->link_mask)
 );
 
+DEFINE_EVENT(wiphy_netdev_evt, rdev_get_radio_mask,
+	TP_PROTO(struct wiphy *wiphy, struct net_device *netdev),
+	TP_ARGS(wiphy, netdev)
+);
+
 #endif /* !__RDEV_OPS_TRACE || TRACE_HEADER_MULTI_READ */
 
 #undef TRACE_INCLUDE_PATH
diff --git a/net/wireless/util.c b/net/wireless/util.c
index 2bde8a354631..cab40e4c3491 100644
--- a/net/wireless/util.c
+++ b/net/wireless/util.c
@@ -2305,20 +2305,35 @@ static int cfg80211_wdev_bi(struct wireless_dev *wdev)
 
 static void cfg80211_calculate_bi_data(struct wiphy *wiphy, u32 new_beacon_int,
 				       u32 *beacon_int_gcd,
-				       bool *beacon_int_different)
+				       bool *beacon_int_different,
+				       const struct wiphy_radio *radio)
 {
+	struct cfg80211_registered_device *rdev;
 	struct wireless_dev *wdev;
+	int radio_idx = -1;
 
 	*beacon_int_gcd = 0;
 	*beacon_int_different = false;
+	if (radio)
+		radio_idx = radio - wiphy->radio;
 
+	rdev = wiphy_to_rdev(wiphy);
 	list_for_each_entry(wdev, &wiphy->wdev_list, list) {
 		int wdev_bi;
+		u32 mask;
 
 		/* this feature isn't supported with MLO */
 		if (wdev->valid_links)
 			continue;
 
+		if (radio_idx >= 0) {
+			if (rdev_get_radio_mask(rdev, wdev->netdev, &mask))
+				continue;
+
+			if (!(mask & BIT(radio_idx)))
+				continue;
+		}
+
 		wdev_bi = cfg80211_wdev_bi(wdev);
 
 		if (!wdev_bi)
@@ -2366,9 +2381,11 @@ int cfg80211_iter_combinations(struct wiphy *wiphy,
 					    void *data),
 			       void *data)
 {
+	const struct wiphy_radio *radio = params->radio;
+	const struct ieee80211_iface_combination *c;
 	const struct ieee80211_regdomain *regdom;
 	enum nl80211_dfs_regions region = 0;
-	int i, j, iftype;
+	int i, j, n, iftype;
 	int num_interfaces = 0;
 	u32 used_iftypes = 0;
 	u32 beacon_int_gcd;
@@ -2385,7 +2402,8 @@ int cfg80211_iter_combinations(struct wiphy *wiphy,
 	 * interfaces (while being brought up) and channel/radar data.
 	 */
 	cfg80211_calculate_bi_data(wiphy, params->new_beacon_int,
-				   &beacon_int_gcd, &beacon_int_different);
+				   &beacon_int_gcd, &beacon_int_different,
+				   radio);
 
 	if (params->radar_detect) {
 		rcu_read_lock();
@@ -2402,13 +2420,17 @@ int cfg80211_iter_combinations(struct wiphy *wiphy,
 			used_iftypes |= BIT(iftype);
 	}
 
-	for (i = 0; i < wiphy->n_iface_combinations; i++) {
-		const struct ieee80211_iface_combination *c;
+	if (radio) {
+		c = radio->iface_combinations;
+		n = radio->n_iface_combinations;
+	} else {
+		c = wiphy->iface_combinations;
+		n = wiphy->n_iface_combinations;
+	}
+	for (i = 0; i < n; i++, c++) {
 		struct ieee80211_iface_limit *limits;
 		u32 all_iftypes = 0;
 
-		c = &wiphy->iface_combinations[i];
-
 		if (num_interfaces > c->max_interfaces)
 			continue;
 		if (params->num_different_channels > c->num_different_channels)
-- 
git-series 0.9.1

Re: [RFC v3 3/8] wifi: cfg80211: extend interface combination check for multi-radio

[Johannes Berg]

On Thu, 2024-06-06 at 20:07 +0200, Felix Fietkau wrote:
> 
> @@ -4577,6 +4579,7 @@ struct mgmt_frame_regs {
>   *
>   * @set_hw_timestamp: Enable/disable HW timestamping of TM/FTM frames.
>   * @set_ttlm: set the TID to link mapping.
> + * @get_radio_mask: get bitmask of radios in use
>   */
>  struct cfg80211_ops {
>  	int	(*suspend)(struct wiphy *wiphy, struct cfg80211_wowlan *wow);
> @@ -4938,6 +4941,8 @@ struct cfg80211_ops {
>  				    struct cfg80211_set_hw_timestamp *hwts);
>  	int	(*set_ttlm)(struct wiphy *wiphy, struct net_device *dev,
>  			    struct cfg80211_ttlm_params *params);
> +	int	(*get_radio_mask)(struct wiphy *wiphy, struct net_device *dev,
> +				  u32 *mask);


not sure I see the point of this being a callback rather than being
passed in?

(Also, if really needed, do you actually expect a device with 32 radios?
if not you can use a return value instead of u32 *mask out pointer :) )


> +DEFINE_EVENT(wiphy_netdev_evt, rdev_get_radio_mask,
> +	TP_PROTO(struct wiphy *wiphy, struct net_device *netdev),
> +	TP_ARGS(wiphy, netdev)
> +);

and if we do need it that really should trace not just the fact that it
happened but also the return value and mask

>  static void cfg80211_calculate_bi_data(struct wiphy *wiphy, u32 new_beacon_int,
>  				       u32 *beacon_int_gcd,
> -				       bool *beacon_int_different)
> +				       bool *beacon_int_different,
> +				       const struct wiphy_radio *radio)
>  {
> +	struct cfg80211_registered_device *rdev;
>  	struct wireless_dev *wdev;
> +	int radio_idx = -1;
>  
>  	*beacon_int_gcd = 0;
>  	*beacon_int_different = false;
> +	if (radio)
> +		radio_idx = radio - wiphy->radio;

This can go oh so wrong ... and technically even be UB. I'd rather pass
the index from the driver, I guess, and validate it against n_radios.
 
> +	rdev = wiphy_to_rdev(wiphy);
>  	list_for_each_entry(wdev, &wiphy->wdev_list, list) {
>  		int wdev_bi;
> +		u32 mask;
>  
>  		/* this feature isn't supported with MLO */
>  		if (wdev->valid_links)
>  			continue;

Are we expecting this to change? because the premise of this patchset is
MLO support, and yet with real MLO we won't get here?

Or is that because non-MLO interfaces could be created on this wiphy?

>  
> +		if (radio_idx >= 0) {
> +			if (rdev_get_radio_mask(rdev, wdev->netdev, &mask))
> +				continue;


here: given that 'radio'/'radio_idx' is passed in, not sure I see why
the mask couldn't also be passed in?

> +			if (!(mask & BIT(radio_idx)))
> +				continue;

that could use a comment

> -	for (i = 0; i < wiphy->n_iface_combinations; i++) {
> -		const struct ieee80211_iface_combination *c;
> +	if (radio) {
> +		c = radio->iface_combinations;
> +		n = radio->n_iface_combinations;
> +	} else {
> +		c = wiphy->iface_combinations;
> +		n = wiphy->n_iface_combinations;
> +	}
> +	for (i = 0; i < n; i++, c++) {

that c++ is a bit too hidden for my taste there, but YMMV and I guess if
I wasn't reading the diff it'd be more obvious :)

johannes

Re: [RFC v3 3/8] wifi: cfg80211: extend interface combination check for multi-radio

[Felix Fietkau]

On 07.06.24 11:32, Johannes Berg wrote:
> On Thu, 2024-06-06 at 20:07 +0200, Felix Fietkau wrote:
>> 
>> @@ -4577,6 +4579,7 @@ struct mgmt_frame_regs {
>>   *
>>   * @set_hw_timestamp: Enable/disable HW timestamping of TM/FTM frames.
>>   * @set_ttlm: set the TID to link mapping.
>> + * @get_radio_mask: get bitmask of radios in use
>>   */
>>  struct cfg80211_ops {
>>  	int	(*suspend)(struct wiphy *wiphy, struct cfg80211_wowlan *wow);
>> @@ -4938,6 +4941,8 @@ struct cfg80211_ops {
>>  				    struct cfg80211_set_hw_timestamp *hwts);
>>  	int	(*set_ttlm)(struct wiphy *wiphy, struct net_device *dev,
>>  			    struct cfg80211_ttlm_params *params);
>> +	int	(*get_radio_mask)(struct wiphy *wiphy, struct net_device *dev,
>> +				  u32 *mask);
> 
> 
> not sure I see the point of this being a callback rather than being
> passed in?
> 
> (Also, if really needed, do you actually expect a device with 32 radios?
> if not you can use a return value instead of u32 *mask out pointer :) )

I'll update the callback to return u32

>> +DEFINE_EVENT(wiphy_netdev_evt, rdev_get_radio_mask,
>> +	TP_PROTO(struct wiphy *wiphy, struct net_device *netdev),
>> +	TP_ARGS(wiphy, netdev)
>> +);
> 
> and if we do need it that really should trace not just the fact that it
> happened but also the return value and mask
> 
>>  static void cfg80211_calculate_bi_data(struct wiphy *wiphy, u32 new_beacon_int,
>>  				       u32 *beacon_int_gcd,
>> -				       bool *beacon_int_different)
>> +				       bool *beacon_int_different,
>> +				       const struct wiphy_radio *radio)
>>  {
>> +	struct cfg80211_registered_device *rdev;
>>  	struct wireless_dev *wdev;
>> +	int radio_idx = -1;
>>  
>>  	*beacon_int_gcd = 0;
>>  	*beacon_int_different = false;
>> +	if (radio)
>> +		radio_idx = radio - wiphy->radio;
> 
> This can go oh so wrong ... and technically even be UB. I'd rather pass
> the index from the driver, I guess, and validate it against n_radios.

Will pass the index in directly.

>> +	rdev = wiphy_to_rdev(wiphy);
>>  	list_for_each_entry(wdev, &wiphy->wdev_list, list) {
>>  		int wdev_bi;
>> +		u32 mask;
>>  
>>  		/* this feature isn't supported with MLO */
>>  		if (wdev->valid_links)
>>  			continue;
> 
> Are we expecting this to change? because the premise of this patchset is
> MLO support, and yet with real MLO we won't get here?
> 
> Or is that because non-MLO interfaces could be created on this wiphy?

Not sure about this. I guess we can revisit it later since it's out of 
scope for this series.

>>  
>> +		if (radio_idx >= 0) {
>> +			if (rdev_get_radio_mask(rdev, wdev->netdev, &mask))
>> +				continue;
> 
> 
> here: given that 'radio'/'radio_idx' is passed in, not sure I see why
> the mask couldn't also be passed in?

mask is supposed to be per wdev, which is iterated in a loop here.

> 
>> +			if (!(mask & BIT(radio_idx)))
>> +				continue;
> 
> that could use a comment
> 
>> -	for (i = 0; i < wiphy->n_iface_combinations; i++) {
>> -		const struct ieee80211_iface_combination *c;
>> +	if (radio) {
>> +		c = radio->iface_combinations;
>> +		n = radio->n_iface_combinations;
>> +	} else {
>> +		c = wiphy->iface_combinations;
>> +		n = wiphy->n_iface_combinations;
>> +	}
>> +	for (i = 0; i < n; i++, c++) {
> 
> that c++ is a bit too hidden for my taste there, but YMMV and I guess if
> I wasn't reading the diff it'd be more obvious :)

Will move it into the loop body to make it more obvious.

Thanks,

- Felix

[RFC v3 4/8] wifi: mac80211: add support for DFS with multiple radios

[Felix Fietkau]

DFS can be supported with multi-channel combinations, as long as each DFS
capable radio only supports one channel.

Signed-off-by: Felix Fietkau <nbd@nbd.name>
---
 net/mac80211/main.c | 32 ++++++++++++++++++++++++--------
 1 file changed, 24 insertions(+), 8 deletions(-)

diff --git a/net/mac80211/main.c b/net/mac80211/main.c
index 40fbf397ce74..e9c4cf611e94 100644
--- a/net/mac80211/main.c
+++ b/net/mac80211/main.c
@@ -1084,6 +1084,21 @@ static int ieee80211_init_cipher_suites(struct ieee80211_local *local)
 	return 0;
 }
 
+static bool
+ieee80211_ifcomb_check_radar(const struct ieee80211_iface_combination *comb,
+			     int n_comb)
+{
+	int i;
+
+	/* DFS is not supported with multi-channel combinations yet */
+	for (i = 0; i < n_comb; i++, comb++)
+		if (comb->radar_detect_widths &&
+		    comb->num_different_channels > 1)
+			return false;
+
+	return true;
+}
+
 int ieee80211_register_hw(struct ieee80211_hw *hw)
 {
 	struct ieee80211_local *local = hw_to_local(hw);
@@ -1173,17 +1188,18 @@ int ieee80211_register_hw(struct ieee80211_hw *hw)
 			if (comb->num_different_channels > 1)
 				return -EINVAL;
 		}
-	} else {
-		/* DFS is not supported with multi-channel combinations yet */
-		for (i = 0; i < local->hw.wiphy->n_iface_combinations; i++) {
-			const struct ieee80211_iface_combination *comb;
-
-			comb = &local->hw.wiphy->iface_combinations[i];
+	} else if (hw->wiphy->n_radio) {
+		for (i = 0; i < hw->wiphy->n_radio; i++) {
+			const struct wiphy_radio *radio = &hw->wiphy->radio[i];
 
-			if (comb->radar_detect_widths &&
-			    comb->num_different_channels > 1)
+			if (!ieee80211_ifcomb_check_radar(radio->iface_combinations,
+							  radio->n_iface_combinations))
 				return -EINVAL;
 		}
+	} else {
+		if (!ieee80211_ifcomb_check_radar(hw->wiphy->iface_combinations,
+						  hw->wiphy->n_iface_combinations))
+			return -EINVAL;
 	}
 
 	/* Only HW csum features are currently compatible with mac80211 */
-- 
git-series 0.9.1

Re: [RFC v3 4/8] wifi: mac80211: add support for DFS with multiple radios

[Karthikeyan Periyasamy]

On 6/6/2024 11:37 PM, Felix Fietkau wrote:
> DFS can be supported with multi-channel combinations, as long as each DFS
> capable radio only supports one channel.
> 
> Signed-off-by: Felix Fietkau <nbd@nbd.name>
> ---
>   net/mac80211/main.c | 32 ++++++++++++++++++++++++--------
>   1 file changed, 24 insertions(+), 8 deletions(-)
> 
> diff --git a/net/mac80211/main.c b/net/mac80211/main.c
> index 40fbf397ce74..e9c4cf611e94 100644
> --- a/net/mac80211/main.c
> +++ b/net/mac80211/main.c

...

>   int ieee80211_register_hw(struct ieee80211_hw *hw)
>   {
>   	struct ieee80211_local *local = hw_to_local(hw);
> @@ -1173,17 +1188,18 @@ int ieee80211_register_hw(struct ieee80211_hw *hw)
>   			if (comb->num_different_channels > 1)
>   				return -EINVAL;
>   		}
> -	} else {
> -		/* DFS is not supported with multi-channel combinations yet */
> -		for (i = 0; i < local->hw.wiphy->n_iface_combinations; i++) {
> -			const struct ieee80211_iface_combination *comb;
> -
> -			comb = &local->hw.wiphy->iface_combinations[i];
> +	} else if (hw->wiphy->n_radio) {
> +		for (i = 0; i < hw->wiphy->n_radio; i++) {
> +			const struct wiphy_radio *radio = &hw->wiphy->radio[i];
>   
> -			if (comb->radar_detect_widths &&
> -			    comb->num_different_channels > 1)
> +			if (!ieee80211_ifcomb_check_radar(radio->iface_combinations,
> +							  radio->n_iface_combinations))
>   				return -EINVAL;
>   		}
> +	} else {
> +		if (!ieee80211_ifcomb_check_radar(hw->wiphy->iface_combinations,
> +						  hw->wiphy->n_iface_combinations))
> +			return -EINVAL;
>   	}
>   
>   	/* Only HW csum features are currently compatible with mac80211 */

Are we omitting the "wiphy->iface_combinations" if the radio specific 
iface combination advertised ?

If so, it looks like unused "wiphy->iface_combinations" for radio 
specific combination advertised.

-- 
Karthikeyan Periyasamy
--
கார்த்திகேயன் பெரியசாமி

Re: [RFC v3 4/8] wifi: mac80211: add support for DFS with multiple radios

[Felix Fietkau]

On 07.06.24 06:25, Karthikeyan Periyasamy wrote:
> 
> 
> On 6/6/2024 11:37 PM, Felix Fietkau wrote:
>> DFS can be supported with multi-channel combinations, as long as each DFS
>> capable radio only supports one channel.
>> 
>> Signed-off-by: Felix Fietkau <nbd@nbd.name>
>> ---
>>   net/mac80211/main.c | 32 ++++++++++++++++++++++++--------
>>   1 file changed, 24 insertions(+), 8 deletions(-)
>> 
>> diff --git a/net/mac80211/main.c b/net/mac80211/main.c
>> index 40fbf397ce74..e9c4cf611e94 100644
>> --- a/net/mac80211/main.c
>> +++ b/net/mac80211/main.c
> 
> ...
> 
>>   int ieee80211_register_hw(struct ieee80211_hw *hw)
>>   {
>>   	struct ieee80211_local *local = hw_to_local(hw);
>> @@ -1173,17 +1188,18 @@ int ieee80211_register_hw(struct ieee80211_hw *hw)
>>   			if (comb->num_different_channels > 1)
>>   				return -EINVAL;
>>   		}
>> -	} else {
>> -		/* DFS is not supported with multi-channel combinations yet */
>> -		for (i = 0; i < local->hw.wiphy->n_iface_combinations; i++) {
>> -			const struct ieee80211_iface_combination *comb;
>> -
>> -			comb = &local->hw.wiphy->iface_combinations[i];
>> +	} else if (hw->wiphy->n_radio) {
>> +		for (i = 0; i < hw->wiphy->n_radio; i++) {
>> +			const struct wiphy_radio *radio = &hw->wiphy->radio[i];
>>   
>> -			if (comb->radar_detect_widths &&
>> -			    comb->num_different_channels > 1)
>> +			if (!ieee80211_ifcomb_check_radar(radio->iface_combinations,
>> +							  radio->n_iface_combinations))
>>   				return -EINVAL;
>>   		}
>> +	} else {
>> +		if (!ieee80211_ifcomb_check_radar(hw->wiphy->iface_combinations,
>> +						  hw->wiphy->n_iface_combinations))
>> +			return -EINVAL;
>>   	}
>>   
>>   	/* Only HW csum features are currently compatible with mac80211 */
> 
> Are we omitting the "wiphy->iface_combinations" if the radio specific
> iface combination advertised ?
> 
> If so, it looks like unused "wiphy->iface_combinations" for radio
> specific combination advertised.

This patch series assumes that you have both wiphy->iface_combinations 
and radio->iface_combinations set. wiphy->iface_combinations applies to 
the full wiphy, whereas radio->iface_combinations only applies to vifs 
assigned to the radio.

- Felix

Re: [RFC v3 4/8] wifi: mac80211: add support for DFS with multiple radios

[Karthikeyan Periyasamy]

On 6/7/2024 10:05 AM, Felix Fietkau wrote:
> On 07.06.24 06:25, Karthikeyan Periyasamy wrote:
>>
>>
>> On 6/6/2024 11:37 PM, Felix Fietkau wrote:
>>> DFS can be supported with multi-channel combinations, as long as each 
>>> DFS
>>> capable radio only supports one channel.
>>>
>>> Signed-off-by: Felix Fietkau <nbd@nbd.name>
>>> ---
>>>   net/mac80211/main.c | 32 ++++++++++++++++++++++++--------
>>>   1 file changed, 24 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/net/mac80211/main.c b/net/mac80211/main.c
>>> index 40fbf397ce74..e9c4cf611e94 100644
>>> --- a/net/mac80211/main.c
>>> +++ b/net/mac80211/main.c
>>
>> ...
>>
>>>   int ieee80211_register_hw(struct ieee80211_hw *hw)
>>>   {
>>>       struct ieee80211_local *local = hw_to_local(hw);
>>> @@ -1173,17 +1188,18 @@ int ieee80211_register_hw(struct ieee80211_hw 
>>> *hw)
>>>               if (comb->num_different_channels > 1)
>>>                   return -EINVAL;
>>>           }
>>> -    } else {
>>> -        /* DFS is not supported with multi-channel combinations yet */
>>> -        for (i = 0; i < local->hw.wiphy->n_iface_combinations; i++) {
>>> -            const struct ieee80211_iface_combination *comb;
>>> -
>>> -            comb = &local->hw.wiphy->iface_combinations[i];
>>> +    } else if (hw->wiphy->n_radio) {
>>> +        for (i = 0; i < hw->wiphy->n_radio; i++) {
>>> +            const struct wiphy_radio *radio = &hw->wiphy->radio[i];
>>> -            if (comb->radar_detect_widths &&
>>> -                comb->num_different_channels > 1)
>>> +            if 
>>> (!ieee80211_ifcomb_check_radar(radio->iface_combinations,
>>> +                              radio->n_iface_combinations))
>>>                   return -EINVAL;
>>>           }
>>> +    } else {
>>> +        if 
>>> (!ieee80211_ifcomb_check_radar(hw->wiphy->iface_combinations,
>>> +                          hw->wiphy->n_iface_combinations))
>>> +            return -EINVAL;
>>>       }
>>>       /* Only HW csum features are currently compatible with mac80211 */
>>
>> Are we omitting the "wiphy->iface_combinations" if the radio specific
>> iface combination advertised ?
>>
>> If so, it looks like unused "wiphy->iface_combinations" for radio
>> specific combination advertised.
> 
> This patch series assumes that you have both wiphy->iface_combinations 
> and radio->iface_combinations set. wiphy->iface_combinations applies to 
> the full wiphy, whereas radio->iface_combinations only applies to vifs 
> assigned to the radio.


If radio->iface_combinations is set then always vifs assigned to the 
radio. so wiphy->iface_combinations get avoid for all the use cases.

Ultimately either of one combination only get used by this proposal.

or I am missing something here ?


> 
> - Felix

-- 
Karthikeyan Periyasamy
--
கார்த்திகேயன் பெரியசாமி

Re: [RFC v3 4/8] wifi: mac80211: add support for DFS with multiple radios

[Felix Fietkau]

On 07.06.24 06:54, Karthikeyan Periyasamy wrote:
> 
> 
> On 6/7/2024 10:05 AM, Felix Fietkau wrote:
>> On 07.06.24 06:25, Karthikeyan Periyasamy wrote:
>>>
>>>
>>> On 6/6/2024 11:37 PM, Felix Fietkau wrote:
>>>> DFS can be supported with multi-channel combinations, as long as each 
>>>> DFS
>>>> capable radio only supports one channel.
>>>>
>>>> Signed-off-by: Felix Fietkau <nbd@nbd.name>
>>>> ---
>>>>   net/mac80211/main.c | 32 ++++++++++++++++++++++++--------
>>>>   1 file changed, 24 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/net/mac80211/main.c b/net/mac80211/main.c
>>>> index 40fbf397ce74..e9c4cf611e94 100644
>>>> --- a/net/mac80211/main.c
>>>> +++ b/net/mac80211/main.c
>>>
>>> ...
>>>
>>>>   int ieee80211_register_hw(struct ieee80211_hw *hw)
>>>>   {
>>>>       struct ieee80211_local *local = hw_to_local(hw);
>>>> @@ -1173,17 +1188,18 @@ int ieee80211_register_hw(struct ieee80211_hw 
>>>> *hw)
>>>>               if (comb->num_different_channels > 1)
>>>>                   return -EINVAL;
>>>>           }
>>>> -    } else {
>>>> -        /* DFS is not supported with multi-channel combinations yet */
>>>> -        for (i = 0; i < local->hw.wiphy->n_iface_combinations; i++) {
>>>> -            const struct ieee80211_iface_combination *comb;
>>>> -
>>>> -            comb = &local->hw.wiphy->iface_combinations[i];
>>>> +    } else if (hw->wiphy->n_radio) {
>>>> +        for (i = 0; i < hw->wiphy->n_radio; i++) {
>>>> +            const struct wiphy_radio *radio = &hw->wiphy->radio[i];
>>>> -            if (comb->radar_detect_widths &&
>>>> -                comb->num_different_channels > 1)
>>>> +            if 
>>>> (!ieee80211_ifcomb_check_radar(radio->iface_combinations,
>>>> +                              radio->n_iface_combinations))
>>>>                   return -EINVAL;
>>>>           }
>>>> +    } else {
>>>> +        if 
>>>> (!ieee80211_ifcomb_check_radar(hw->wiphy->iface_combinations,
>>>> +                          hw->wiphy->n_iface_combinations))
>>>> +            return -EINVAL;
>>>>       }
>>>>       /* Only HW csum features are currently compatible with mac80211 */
>>>
>>> Are we omitting the "wiphy->iface_combinations" if the radio specific
>>> iface combination advertised ?
>>>
>>> If so, it looks like unused "wiphy->iface_combinations" for radio
>>> specific combination advertised.
>> 
>> This patch series assumes that you have both wiphy->iface_combinations 
>> and radio->iface_combinations set. wiphy->iface_combinations applies to 
>> the full wiphy, whereas radio->iface_combinations only applies to vifs 
>> assigned to the radio.
> 
> 
> If radio->iface_combinations is set then always vifs assigned to the
> radio. so wiphy->iface_combinations get avoid for all the use cases.
> 
> Ultimately either of one combination only get used by this proposal.
> 
> or I am missing something here ?

The functions that perform interface combination checks are called both 
with -1 as radio_idx (meaning per-wiphy), as well as with the assigned 
radio id. That way, both kinds of combinations/limits are checked and 
enforced.

- Felix

Re: [RFC v3 4/8] wifi: mac80211: add support for DFS with multiple radios

[Karthikeyan Periyasamy]

On 6/7/2024 10:33 AM, Felix Fietkau wrote:
> On 07.06.24 06:54, Karthikeyan Periyasamy wrote:
>>
>>
>> On 6/7/2024 10:05 AM, Felix Fietkau wrote:
>>> On 07.06.24 06:25, Karthikeyan Periyasamy wrote:
>>>>
>>>>
>>>> On 6/6/2024 11:37 PM, Felix Fietkau wrote:
>>>>> DFS can be supported with multi-channel combinations, as long as 
>>>>> each DFS
>>>>> capable radio only supports one channel.
>>>>>
>>>>> Signed-off-by: Felix Fietkau <nbd@nbd.name>
>>>>> ---
>>>>>   net/mac80211/main.c | 32 ++++++++++++++++++++++++--------
>>>>>   1 file changed, 24 insertions(+), 8 deletions(-)
>>>>>
>>>>> diff --git a/net/mac80211/main.c b/net/mac80211/main.c
>>>>> index 40fbf397ce74..e9c4cf611e94 100644
>>>>> --- a/net/mac80211/main.c
>>>>> +++ b/net/mac80211/main.c
>>>>
>>>> ...
>>>>
>>>>>   int ieee80211_register_hw(struct ieee80211_hw *hw)
>>>>>   {
>>>>>       struct ieee80211_local *local = hw_to_local(hw);
>>>>> @@ -1173,17 +1188,18 @@ int ieee80211_register_hw(struct 
>>>>> ieee80211_hw *hw)
>>>>>               if (comb->num_different_channels > 1)
>>>>>                   return -EINVAL;
>>>>>           }
>>>>> -    } else {
>>>>> -        /* DFS is not supported with multi-channel combinations 
>>>>> yet */
>>>>> -        for (i = 0; i < local->hw.wiphy->n_iface_combinations; i++) {
>>>>> -            const struct ieee80211_iface_combination *comb;
>>>>> -
>>>>> -            comb = &local->hw.wiphy->iface_combinations[i];
>>>>> +    } else if (hw->wiphy->n_radio) {
>>>>> +        for (i = 0; i < hw->wiphy->n_radio; i++) {
>>>>> +            const struct wiphy_radio *radio = &hw->wiphy->radio[i];
>>>>> -            if (comb->radar_detect_widths &&
>>>>> -                comb->num_different_channels > 1)
>>>>> +            if 
>>>>> (!ieee80211_ifcomb_check_radar(radio->iface_combinations,
>>>>> +                              radio->n_iface_combinations))
>>>>>                   return -EINVAL;
>>>>>           }
>>>>> +    } else {
>>>>> +        if 
>>>>> (!ieee80211_ifcomb_check_radar(hw->wiphy->iface_combinations,
>>>>> +                          hw->wiphy->n_iface_combinations))
>>>>> +            return -EINVAL;
>>>>>       }
>>>>>       /* Only HW csum features are currently compatible with 
>>>>> mac80211 */
>>>>
>>>> Are we omitting the "wiphy->iface_combinations" if the radio specific
>>>> iface combination advertised ?
>>>>
>>>> If so, it looks like unused "wiphy->iface_combinations" for radio
>>>> specific combination advertised.
>>>
>>> This patch series assumes that you have both 
>>> wiphy->iface_combinations and radio->iface_combinations set. 
>>> wiphy->iface_combinations applies to the full wiphy, whereas 
>>> radio->iface_combinations only applies to vifs assigned to the radio.
>>
>>
>> If radio->iface_combinations is set then always vifs assigned to the
>> radio. so wiphy->iface_combinations get avoid for all the use cases.
>>
>> Ultimately either of one combination only get used by this proposal.
>>
>> or I am missing something here ?
> 
> The functions that perform interface combination checks are called both 
> with -1 as radio_idx (meaning per-wiphy), as well as with the assigned 
> radio id. That way, both kinds of combinations/limits are checked and 
> enforced.
> 
In the radio specific iface advertisement, global iface combination 
represent the union or intersection of all radio iface combination ?

-- 
Karthikeyan Periyasamy
--
கார்த்திகேயன் பெரியசாமி

Re: [RFC v3 4/8] wifi: mac80211: add support for DFS with multiple radios

[Felix Fietkau]

On 07.06.24 08:45, Karthikeyan Periyasamy wrote:
> 
> 
> On 6/7/2024 10:33 AM, Felix Fietkau wrote:
>> On 07.06.24 06:54, Karthikeyan Periyasamy wrote:
>>>
>>>
>>> On 6/7/2024 10:05 AM, Felix Fietkau wrote:
>>>> On 07.06.24 06:25, Karthikeyan Periyasamy wrote:
>>>>>
>>>>>
>>>>> On 6/6/2024 11:37 PM, Felix Fietkau wrote:
>>>>>> DFS can be supported with multi-channel combinations, as long as 
>>>>>> each DFS
>>>>>> capable radio only supports one channel.
>>>>>>
>>>>>> Signed-off-by: Felix Fietkau <nbd@nbd.name>
>>>>>> ---
>>>>>>   net/mac80211/main.c | 32 ++++++++++++++++++++++++--------
>>>>>>   1 file changed, 24 insertions(+), 8 deletions(-)
>>>>>>
>>>>>> diff --git a/net/mac80211/main.c b/net/mac80211/main.c
>>>>>> index 40fbf397ce74..e9c4cf611e94 100644
>>>>>> --- a/net/mac80211/main.c
>>>>>> +++ b/net/mac80211/main.c
>>>>>
>>>>> ...
>>>>>
>>>>>>   int ieee80211_register_hw(struct ieee80211_hw *hw)
>>>>>>   {
>>>>>>       struct ieee80211_local *local = hw_to_local(hw);
>>>>>> @@ -1173,17 +1188,18 @@ int ieee80211_register_hw(struct 
>>>>>> ieee80211_hw *hw)
>>>>>>               if (comb->num_different_channels > 1)
>>>>>>                   return -EINVAL;
>>>>>>           }
>>>>>> -    } else {
>>>>>> -        /* DFS is not supported with multi-channel combinations 
>>>>>> yet */
>>>>>> -        for (i = 0; i < local->hw.wiphy->n_iface_combinations; i++) {
>>>>>> -            const struct ieee80211_iface_combination *comb;
>>>>>> -
>>>>>> -            comb = &local->hw.wiphy->iface_combinations[i];
>>>>>> +    } else if (hw->wiphy->n_radio) {
>>>>>> +        for (i = 0; i < hw->wiphy->n_radio; i++) {
>>>>>> +            const struct wiphy_radio *radio = &hw->wiphy->radio[i];
>>>>>> -            if (comb->radar_detect_widths &&
>>>>>> -                comb->num_different_channels > 1)
>>>>>> +            if 
>>>>>> (!ieee80211_ifcomb_check_radar(radio->iface_combinations,
>>>>>> +                              radio->n_iface_combinations))
>>>>>>                   return -EINVAL;
>>>>>>           }
>>>>>> +    } else {
>>>>>> +        if 
>>>>>> (!ieee80211_ifcomb_check_radar(hw->wiphy->iface_combinations,
>>>>>> +                          hw->wiphy->n_iface_combinations))
>>>>>> +            return -EINVAL;
>>>>>>       }
>>>>>>       /* Only HW csum features are currently compatible with 
>>>>>> mac80211 */
>>>>>
>>>>> Are we omitting the "wiphy->iface_combinations" if the radio specific
>>>>> iface combination advertised ?
>>>>>
>>>>> If so, it looks like unused "wiphy->iface_combinations" for radio
>>>>> specific combination advertised.
>>>>
>>>> This patch series assumes that you have both 
>>>> wiphy->iface_combinations and radio->iface_combinations set. 
>>>> wiphy->iface_combinations applies to the full wiphy, whereas 
>>>> radio->iface_combinations only applies to vifs assigned to the radio.
>>>
>>>
>>> If radio->iface_combinations is set then always vifs assigned to the
>>> radio. so wiphy->iface_combinations get avoid for all the use cases.
>>>
>>> Ultimately either of one combination only get used by this proposal.
>>>
>>> or I am missing something here ?
>> 
>> The functions that perform interface combination checks are called both 
>> with -1 as radio_idx (meaning per-wiphy), as well as with the assigned 
>> radio id. That way, both kinds of combinations/limits are checked and 
>> enforced.
>> 
> In the radio specific iface advertisement, global iface combination
> represent the union or intersection of all radio iface combination ?

The global interface combination should be a union of all radio 
interface combinations.
You can also use them to apply extra limits, e.g. if you have a limit on 
the per-wiphy number of interfaces (regardless of the assigned radio), 
you use the global interface combination to apply it.

- Felix

Re: [RFC v3 4/8] wifi: mac80211: add support for DFS with multiple radios

[Karthikeyan Periyasamy]

On 6/7/2024 1:46 PM, Felix Fietkau wrote:
> On 07.06.24 08:45, Karthikeyan Periyasamy wrote:
>>
>>
>> On 6/7/2024 10:33 AM, Felix Fietkau wrote:
>>> On 07.06.24 06:54, Karthikeyan Periyasamy wrote:
>>>>
>>>>
>>>> On 6/7/2024 10:05 AM, Felix Fietkau wrote:
>>>>> On 07.06.24 06:25, Karthikeyan Periyasamy wrote:
>>>>>>
>>>>>>
>>>>>> On 6/6/2024 11:37 PM, Felix Fietkau wrote:
>>>>>>> DFS can be supported with multi-channel combinations, as long as 
>>>>>>> each DFS
>>>>>>> capable radio only supports one channel.
>>>>>>>
>>>>>>> Signed-off-by: Felix Fietkau <nbd@nbd.name>
>>>>>>> ---
>>>>>>>   net/mac80211/main.c | 32 ++++++++++++++++++++++++--------
>>>>>>>   1 file changed, 24 insertions(+), 8 deletions(-)
>>>>>>>
>>>>>>> diff --git a/net/mac80211/main.c b/net/mac80211/main.c
>>>>>>> index 40fbf397ce74..e9c4cf611e94 100644
>>>>>>> --- a/net/mac80211/main.c
>>>>>>> +++ b/net/mac80211/main.c
>>>>>>
>>>>>> ...
>>>>>>
>>>>>>>   int ieee80211_register_hw(struct ieee80211_hw *hw)
>>>>>>>   {
>>>>>>>       struct ieee80211_local *local = hw_to_local(hw);
>>>>>>> @@ -1173,17 +1188,18 @@ int ieee80211_register_hw(struct 
>>>>>>> ieee80211_hw *hw)
>>>>>>>               if (comb->num_different_channels > 1)
>>>>>>>                   return -EINVAL;
>>>>>>>           }
>>>>>>> -    } else {
>>>>>>> -        /* DFS is not supported with multi-channel combinations 
>>>>>>> yet */
>>>>>>> -        for (i = 0; i < local->hw.wiphy->n_iface_combinations; 
>>>>>>> i++) {
>>>>>>> -            const struct ieee80211_iface_combination *comb;
>>>>>>> -
>>>>>>> -            comb = &local->hw.wiphy->iface_combinations[i];
>>>>>>> +    } else if (hw->wiphy->n_radio) {
>>>>>>> +        for (i = 0; i < hw->wiphy->n_radio; i++) {
>>>>>>> +            const struct wiphy_radio *radio = &hw->wiphy->radio[i];
>>>>>>> -            if (comb->radar_detect_widths &&
>>>>>>> -                comb->num_different_channels > 1)
>>>>>>> +            if 
>>>>>>> (!ieee80211_ifcomb_check_radar(radio->iface_combinations,
>>>>>>> +                              radio->n_iface_combinations))
>>>>>>>                   return -EINVAL;
>>>>>>>           }
>>>>>>> +    } else {
>>>>>>> +        if 
>>>>>>> (!ieee80211_ifcomb_check_radar(hw->wiphy->iface_combinations,
>>>>>>> +                          hw->wiphy->n_iface_combinations))
>>>>>>> +            return -EINVAL;
>>>>>>>       }
>>>>>>>       /* Only HW csum features are currently compatible with 
>>>>>>> mac80211 */
>>>>>>
>>>>>> Are we omitting the "wiphy->iface_combinations" if the radio specific
>>>>>> iface combination advertised ?
>>>>>>
>>>>>> If so, it looks like unused "wiphy->iface_combinations" for radio
>>>>>> specific combination advertised.
>>>>>
>>>>> This patch series assumes that you have both 
>>>>> wiphy->iface_combinations and radio->iface_combinations set. 
>>>>> wiphy->iface_combinations applies to the full wiphy, whereas 
>>>>> radio->iface_combinations only applies to vifs assigned to the radio.
>>>>
>>>>
>>>> If radio->iface_combinations is set then always vifs assigned to the
>>>> radio. so wiphy->iface_combinations get avoid for all the use cases.
>>>>
>>>> Ultimately either of one combination only get used by this proposal.
>>>>
>>>> or I am missing something here ?
>>>
>>> The functions that perform interface combination checks are called 
>>> both with -1 as radio_idx (meaning per-wiphy), as well as with the 
>>> assigned radio id. That way, both kinds of combinations/limits are 
>>> checked and enforced.
>>>
>> In the radio specific iface advertisement, global iface combination
>> represent the union or intersection of all radio iface combination ?
> 
> The global interface combination should be a union of all radio 
> interface combinations.
> You can also use them to apply extra limits, e.g. if you have a limit on 
> the per-wiphy number of interfaces (regardless of the assigned radio), 
> you use the global interface combination to apply it.
> 
If the global combination follow union representation, the non-ML client 
takes wrong/invalid perception against the global advertisement.

Ex:

Global iface = 14 ( Radio iface: 2G = 4, 5G = 4, 6G = 6 ).

2G non-client read the global configuration and understand it can able 
to create 14 interfaces. But in reality it allowed to create max 4 
interface only.

we have to follow intersection or minimal set, no ?


-- 
Karthikeyan Periyasamy
--
கார்த்திகேயன் பெரியசாமி

Re: [RFC v3 4/8] wifi: mac80211: add support for DFS with multiple radios

[Felix Fietkau]

On 07.06.24 10:54, Karthikeyan Periyasamy wrote:
> 
> 
> On 6/7/2024 1:46 PM, Felix Fietkau wrote:
>> On 07.06.24 08:45, Karthikeyan Periyasamy wrote:
>>>
>>>
>>> On 6/7/2024 10:33 AM, Felix Fietkau wrote:
>>>> On 07.06.24 06:54, Karthikeyan Periyasamy wrote:
>>>>>
>>>>>
>>>>> On 6/7/2024 10:05 AM, Felix Fietkau wrote:
>>>>>> On 07.06.24 06:25, Karthikeyan Periyasamy wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 6/6/2024 11:37 PM, Felix Fietkau wrote:
>>>>>>>> DFS can be supported with multi-channel combinations, as long as 
>>>>>>>> each DFS
>>>>>>>> capable radio only supports one channel.
>>>>>>>>
>>>>>>>> Signed-off-by: Felix Fietkau <nbd@nbd.name>
>>>>>>>> ---
>>>>>>>>   net/mac80211/main.c | 32 ++++++++++++++++++++++++--------
>>>>>>>>   1 file changed, 24 insertions(+), 8 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/net/mac80211/main.c b/net/mac80211/main.c
>>>>>>>> index 40fbf397ce74..e9c4cf611e94 100644
>>>>>>>> --- a/net/mac80211/main.c
>>>>>>>> +++ b/net/mac80211/main.c
>>>>>>>
>>>>>>> ...
>>>>>>>
>>>>>>>>   int ieee80211_register_hw(struct ieee80211_hw *hw)
>>>>>>>>   {
>>>>>>>>       struct ieee80211_local *local = hw_to_local(hw);
>>>>>>>> @@ -1173,17 +1188,18 @@ int ieee80211_register_hw(struct 
>>>>>>>> ieee80211_hw *hw)
>>>>>>>>               if (comb->num_different_channels > 1)
>>>>>>>>                   return -EINVAL;
>>>>>>>>           }
>>>>>>>> -    } else {
>>>>>>>> -        /* DFS is not supported with multi-channel combinations 
>>>>>>>> yet */
>>>>>>>> -        for (i = 0; i < local->hw.wiphy->n_iface_combinations; 
>>>>>>>> i++) {
>>>>>>>> -            const struct ieee80211_iface_combination *comb;
>>>>>>>> -
>>>>>>>> -            comb = &local->hw.wiphy->iface_combinations[i];
>>>>>>>> +    } else if (hw->wiphy->n_radio) {
>>>>>>>> +        for (i = 0; i < hw->wiphy->n_radio; i++) {
>>>>>>>> +            const struct wiphy_radio *radio = &hw->wiphy->radio[i];
>>>>>>>> -            if (comb->radar_detect_widths &&
>>>>>>>> -                comb->num_different_channels > 1)
>>>>>>>> +            if 
>>>>>>>> (!ieee80211_ifcomb_check_radar(radio->iface_combinations,
>>>>>>>> +                              radio->n_iface_combinations))
>>>>>>>>                   return -EINVAL;
>>>>>>>>           }
>>>>>>>> +    } else {
>>>>>>>> +        if 
>>>>>>>> (!ieee80211_ifcomb_check_radar(hw->wiphy->iface_combinations,
>>>>>>>> +                          hw->wiphy->n_iface_combinations))
>>>>>>>> +            return -EINVAL;
>>>>>>>>       }
>>>>>>>>       /* Only HW csum features are currently compatible with 
>>>>>>>> mac80211 */
>>>>>>>
>>>>>>> Are we omitting the "wiphy->iface_combinations" if the radio specific
>>>>>>> iface combination advertised ?
>>>>>>>
>>>>>>> If so, it looks like unused "wiphy->iface_combinations" for radio
>>>>>>> specific combination advertised.
>>>>>>
>>>>>> This patch series assumes that you have both 
>>>>>> wiphy->iface_combinations and radio->iface_combinations set. 
>>>>>> wiphy->iface_combinations applies to the full wiphy, whereas 
>>>>>> radio->iface_combinations only applies to vifs assigned to the radio.
>>>>>
>>>>>
>>>>> If radio->iface_combinations is set then always vifs assigned to the
>>>>> radio. so wiphy->iface_combinations get avoid for all the use cases.
>>>>>
>>>>> Ultimately either of one combination only get used by this proposal.
>>>>>
>>>>> or I am missing something here ?
>>>>
>>>> The functions that perform interface combination checks are called 
>>>> both with -1 as radio_idx (meaning per-wiphy), as well as with the 
>>>> assigned radio id. That way, both kinds of combinations/limits are 
>>>> checked and enforced.
>>>>
>>> In the radio specific iface advertisement, global iface combination
>>> represent the union or intersection of all radio iface combination ?
>> 
>> The global interface combination should be a union of all radio 
>> interface combinations.
>> You can also use them to apply extra limits, e.g. if you have a limit on 
>> the per-wiphy number of interfaces (regardless of the assigned radio), 
>> you use the global interface combination to apply it.
>> 
> If the global combination follow union representation, the non-ML client
> takes wrong/invalid perception against the global advertisement.
> 
> Ex:
> 
> Global iface = 14 ( Radio iface: 2G = 4, 5G = 4, 6G = 6 ).
> 
> 2G non-client read the global configuration and understand it can able
> to create 14 interfaces. But in reality it allowed to create max 4
> interface only.
> 
> we have to follow intersection or minimal set, no ?

Maybe I misunderstood your question earlier. I meant that the driver can 
set the global limit as a sort of union of all per-radio limits.
For each individual radio, the intersection of the global limit (taking 
into account other radios as well) and the per-radio limit (considering 
only per-radio vifs) applies.

- Felix

Re: [RFC v3 4/8] wifi: mac80211: add support for DFS with multiple radios

[Karthikeyan Periyasamy]

On 6/6/2024 11:37 PM, Felix Fietkau wrote:
> DFS can be supported with multi-channel combinations, as long as each DFS
> capable radio only supports one channel.
> 
> Signed-off-by: Felix Fietkau <nbd@nbd.name>
> ---
>   net/mac80211/main.c | 32 ++++++++++++++++++++++++--------
>   1 file changed, 24 insertions(+), 8 deletions(-)
> 
> diff --git a/net/mac80211/main.c b/net/mac80211/main.c
> index 40fbf397ce74..e9c4cf611e94 100644
> --- a/net/mac80211/main.c
> +++ b/net/mac80211/main.c
> @@ -1084,6 +1084,21 @@ static int ieee80211_init_cipher_suites(struct ieee80211_local *local)
>   	return 0;
>   }
>   
> +static bool
> +ieee80211_ifcomb_check_radar(const struct ieee80211_iface_combination *comb,
> +			     int n_comb)
> +{
> +	int i;
> +
> +	/* DFS is not supported with multi-channel combinations yet */
> +	for (i = 0; i < n_comb; i++, comb++)
> +		if (comb->radar_detect_widths &&
> +		    comb->num_different_channels > 1)
> +			return false;
> +
> +	return true;
> +}
> +
>   int ieee80211_register_hw(struct ieee80211_hw *hw)
>   {
>   	struct ieee80211_local *local = hw_to_local(hw);
> @@ -1173,17 +1188,18 @@ int ieee80211_register_hw(struct ieee80211_hw *hw)
>   			if (comb->num_different_channels > 1)
>   				return -EINVAL;
>   		}
> -	} else {
> -		/* DFS is not supported with multi-channel combinations yet */
> -		for (i = 0; i < local->hw.wiphy->n_iface_combinations; i++) {
> -			const struct ieee80211_iface_combination *comb;
> -
> -			comb = &local->hw.wiphy->iface_combinations[i];
> +	} else if (hw->wiphy->n_radio) {
> +		for (i = 0; i < hw->wiphy->n_radio; i++) {
> +			const struct wiphy_radio *radio = &hw->wiphy->radio[i];
>   
> -			if (comb->radar_detect_widths &&
> -			    comb->num_different_channels > 1)
> +			if (!ieee80211_ifcomb_check_radar(radio->iface_combinations,
> +							  radio->n_iface_combinations))
>   				return -EINVAL;
>   		}

When driver advertise per radio iface combination, you are not checking 
the global iface combination. using the uncheck global iface combination 
lead to unknown issues.


> +	} else {
> +		if (!ieee80211_ifcomb_check_radar(hw->wiphy->iface_combinations,
> +						  hw->wiphy->n_iface_combinations))
> +			return -EINVAL;
>   	}
>   
>   	/* Only HW csum features are currently compatible with mac80211 */

-- 
Karthikeyan Periyasamy
--
கார்த்திகேயன் பெரியசாமி

Re: [RFC v3 4/8] wifi: mac80211: add support for DFS with multiple radios

[Felix Fietkau]

On 12.06.24 16:23, Karthikeyan Periyasamy wrote:
> 
> 
> On 6/6/2024 11:37 PM, Felix Fietkau wrote:
>> DFS can be supported with multi-channel combinations, as long as each DFS
>> capable radio only supports one channel.
>> 
>> Signed-off-by: Felix Fietkau <nbd@nbd.name>
>> ---
>>   net/mac80211/main.c | 32 ++++++++++++++++++++++++--------
>>   1 file changed, 24 insertions(+), 8 deletions(-)
>> 
>> diff --git a/net/mac80211/main.c b/net/mac80211/main.c
>> index 40fbf397ce74..e9c4cf611e94 100644
>> --- a/net/mac80211/main.c
>> +++ b/net/mac80211/main.c
>> @@ -1084,6 +1084,21 @@ static int ieee80211_init_cipher_suites(struct ieee80211_local *local)
>>   	return 0;
>>   }
>>   
>> +static bool
>> +ieee80211_ifcomb_check_radar(const struct ieee80211_iface_combination *comb,
>> +			     int n_comb)
>> +{
>> +	int i;
>> +
>> +	/* DFS is not supported with multi-channel combinations yet */
>> +	for (i = 0; i < n_comb; i++, comb++)
>> +		if (comb->radar_detect_widths &&
>> +		    comb->num_different_channels > 1)
>> +			return false;
>> +
>> +	return true;
>> +}
>> +
>>   int ieee80211_register_hw(struct ieee80211_hw *hw)
>>   {
>>   	struct ieee80211_local *local = hw_to_local(hw);
>> @@ -1173,17 +1188,18 @@ int ieee80211_register_hw(struct ieee80211_hw *hw)
>>   			if (comb->num_different_channels > 1)
>>   				return -EINVAL;
>>   		}
>> -	} else {
>> -		/* DFS is not supported with multi-channel combinations yet */
>> -		for (i = 0; i < local->hw.wiphy->n_iface_combinations; i++) {
>> -			const struct ieee80211_iface_combination *comb;
>> -
>> -			comb = &local->hw.wiphy->iface_combinations[i];
>> +	} else if (hw->wiphy->n_radio) {
>> +		for (i = 0; i < hw->wiphy->n_radio; i++) {
>> +			const struct wiphy_radio *radio = &hw->wiphy->radio[i];
>>   
>> -			if (comb->radar_detect_widths &&
>> -			    comb->num_different_channels > 1)
>> +			if (!ieee80211_ifcomb_check_radar(radio->iface_combinations,
>> +							  radio->n_iface_combinations))
>>   				return -EINVAL;
>>   		}
> 
> When driver advertise per radio iface combination, you are not checking
> the global iface combination. using the uncheck global iface combination
> lead to unknown issues.

The only constraint being checked here is num_different_channel > 1 XOR 
comb->radar_detect_widths != 0.
The global wiphy needs both being set, since it applies the sum of all 
radios. For each individual radio, the constraint is enforced, so there 
should not be any unknown issues.

- Felix

[RFC v3 5/8] wifi: mac80211: add radio index to ieee80211_chanctx_conf

[Felix Fietkau]

Will be used to explicitly assign a channel context to a wiphy radio.

Signed-off-by: Felix Fietkau <nbd@nbd.name>
---
 include/net/mac80211.h | 2 ++
 net/mac80211/chan.c    | 8 +++++---
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index ecfa65ade226..98394970c991 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -250,6 +250,7 @@ struct ieee80211_chan_req {
  * @min_def: the minimum channel definition currently required.
  * @ap: the channel definition the AP actually is operating as,
  *	for use with (wider bandwidth) OFDMA
+ * @radio_idx: index of the wiphy radio used used for this channel
  * @rx_chains_static: The number of RX chains that must always be
  *	active on the channel to receive MIMO transmissions
  * @rx_chains_dynamic: The number of RX chains that must be enabled
@@ -264,6 +265,7 @@ struct ieee80211_chanctx_conf {
 	struct cfg80211_chan_def min_def;
 	struct cfg80211_chan_def ap;
 
+	int radio_idx;
 	u8 rx_chains_static, rx_chains_dynamic;
 
 	bool radar_enabled;
diff --git a/net/mac80211/chan.c b/net/mac80211/chan.c
index ec16d7676088..574ae961a7cf 100644
--- a/net/mac80211/chan.c
+++ b/net/mac80211/chan.c
@@ -638,7 +638,8 @@ ieee80211_chanctx_radar_required(struct ieee80211_local *local,
 static struct ieee80211_chanctx *
 ieee80211_alloc_chanctx(struct ieee80211_local *local,
 			const struct ieee80211_chan_req *chanreq,
-			enum ieee80211_chanctx_mode mode)
+			enum ieee80211_chanctx_mode mode,
+			int radio_idx)
 {
 	struct ieee80211_chanctx *ctx;
 
@@ -656,6 +657,7 @@ ieee80211_alloc_chanctx(struct ieee80211_local *local,
 	ctx->conf.rx_chains_dynamic = 1;
 	ctx->mode = mode;
 	ctx->conf.radar_enabled = false;
+	ctx->conf.radio_idx = radio_idx;
 	_ieee80211_recalc_chanctx_min_def(local, ctx, NULL);
 
 	return ctx;
@@ -696,7 +698,7 @@ ieee80211_new_chanctx(struct ieee80211_local *local,
 
 	lockdep_assert_wiphy(local->hw.wiphy);
 
-	ctx = ieee80211_alloc_chanctx(local, chanreq, mode);
+	ctx = ieee80211_alloc_chanctx(local, chanreq, mode, -1);
 	if (!ctx)
 		return ERR_PTR(-ENOMEM);
 
@@ -1126,7 +1128,7 @@ int ieee80211_link_reserve_chanctx(struct ieee80211_link_data *link,
 			    !list_empty(&curr_ctx->reserved_links))
 				return -EBUSY;
 
-			new_ctx = ieee80211_alloc_chanctx(local, chanreq, mode);
+			new_ctx = ieee80211_alloc_chanctx(local, chanreq, mode, -1);
 			if (!new_ctx)
 				return -ENOMEM;
 
-- 
git-series 0.9.1

[RFC v3 6/8] wifi: mac80211: extend ifcomb check functions for multi-radio

[Felix Fietkau]

Add support for counting global and per-radio max/current number of
channels, as well as checking radio-specific interface combinations.

Signed-off-by: Felix Fietkau <nbd@nbd.name>
---
 net/mac80211/cfg.c         |   7 +-
 net/mac80211/chan.c        |  17 +++--
 net/mac80211/ibss.c        |   2 +-
 net/mac80211/ieee80211_i.h |   6 +-
 net/mac80211/iface.c       |   2 +-
 net/mac80211/util.c        | 131 +++++++++++++++++++++++++++-----------
 6 files changed, 116 insertions(+), 49 deletions(-)

diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index 62119e957cd8..950b7b72f0b8 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -263,7 +263,7 @@ static int ieee80211_start_p2p_device(struct wiphy *wiphy,
 
 	lockdep_assert_wiphy(sdata->local->hw.wiphy);
 
-	ret = ieee80211_check_combinations(sdata, NULL, 0, 0);
+	ret = ieee80211_check_combinations(sdata, NULL, 0, 0, -1);
 	if (ret < 0)
 		return ret;
 
@@ -285,7 +285,7 @@ static int ieee80211_start_nan(struct wiphy *wiphy,
 
 	lockdep_assert_wiphy(sdata->local->hw.wiphy);
 
-	ret = ieee80211_check_combinations(sdata, NULL, 0, 0);
+	ret = ieee80211_check_combinations(sdata, NULL, 0, 0, -1);
 	if (ret < 0)
 		return ret;
 
@@ -4001,7 +4001,7 @@ __ieee80211_channel_switch(struct wiphy *wiphy, struct net_device *dev,
 		goto out;
 
 	/* if reservation is invalid then this will fail */
-	err = ieee80211_check_combinations(sdata, NULL, chanctx->mode, 0);
+	err = ieee80211_check_combinations(sdata, NULL, chanctx->mode, 0, -1);
 	if (err) {
 		ieee80211_link_unreserve_chanctx(link_data);
 		goto out;
@@ -5199,4 +5199,5 @@ const struct cfg80211_ops mac80211_config_ops = {
 	.del_link_station = ieee80211_del_link_station,
 	.set_hw_timestamp = ieee80211_set_hw_timestamp,
 	.set_ttlm = ieee80211_set_ttlm,
+	.get_radio_mask = ieee80211_get_radio_mask,
 };
diff --git a/net/mac80211/chan.c b/net/mac80211/chan.c
index 574ae961a7cf..0e899c07bc2b 100644
--- a/net/mac80211/chan.c
+++ b/net/mac80211/chan.c
@@ -47,24 +47,29 @@ int ieee80211_chanctx_refcount(struct ieee80211_local *local,
 	       ieee80211_chanctx_num_reserved(local, ctx);
 }
 
-static int ieee80211_num_chanctx(struct ieee80211_local *local)
+static int ieee80211_num_chanctx(struct ieee80211_local *local, int radio_idx)
 {
 	struct ieee80211_chanctx *ctx;
 	int num = 0;
 
 	lockdep_assert_wiphy(local->hw.wiphy);
 
-	list_for_each_entry(ctx, &local->chanctx_list, list)
+	list_for_each_entry(ctx, &local->chanctx_list, list) {
+		if (radio_idx >= 0 && ctx->conf.radio_idx != radio_idx)
+			continue;
 		num++;
+	}
 
 	return num;
 }
 
-static bool ieee80211_can_create_new_chanctx(struct ieee80211_local *local)
+static bool ieee80211_can_create_new_chanctx(struct ieee80211_local *local,
+					     int radio_idx)
 {
 	lockdep_assert_wiphy(local->hw.wiphy);
 
-	return ieee80211_num_chanctx(local) < ieee80211_max_num_channels(local);
+	return ieee80211_num_chanctx(local, radio_idx) <
+	       ieee80211_max_num_channels(local, radio_idx);
 }
 
 static struct ieee80211_chanctx *
@@ -1072,7 +1077,7 @@ int ieee80211_link_reserve_chanctx(struct ieee80211_link_data *link,
 
 	new_ctx = ieee80211_find_reservation_chanctx(local, chanreq, mode);
 	if (!new_ctx) {
-		if (ieee80211_can_create_new_chanctx(local)) {
+		if (ieee80211_can_create_new_chanctx(local, -1)) {
 			new_ctx = ieee80211_new_chanctx(local, chanreq, mode,
 							false);
 			if (IS_ERR(new_ctx))
@@ -1767,7 +1772,7 @@ int _ieee80211_link_use_channel(struct ieee80211_link_data *link,
 	link->radar_required = ret;
 
 	ret = ieee80211_check_combinations(sdata, &chanreq->oper, mode,
-					   radar_detect_width);
+					   radar_detect_width, -1);
 	if (ret < 0)
 		goto out;
 
diff --git a/net/mac80211/ibss.c b/net/mac80211/ibss.c
index bf338f3d4dd3..522e964bb186 100644
--- a/net/mac80211/ibss.c
+++ b/net/mac80211/ibss.c
@@ -1745,7 +1745,7 @@ int ieee80211_ibss_join(struct ieee80211_sub_if_data *sdata,
 		IEEE80211_CHANCTX_SHARED : IEEE80211_CHANCTX_EXCLUSIVE;
 
 	ret = ieee80211_check_combinations(sdata, &params->chandef, chanmode,
-					   radar_detect_width);
+					   radar_detect_width, -1);
 	if (ret < 0)
 		return ret;
 
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index 3fd7b1adbfab..33c783432383 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -2038,6 +2038,8 @@ static inline bool ieee80211_sdata_running(struct ieee80211_sub_if_data *sdata)
 {
 	return test_bit(SDATA_STATE_RUNNING, &sdata->state);
 }
+int ieee80211_get_radio_mask(struct wiphy *wiphy, struct net_device *dev,
+			     u32 *mask);
 
 /* link handling */
 void ieee80211_link_setup(struct ieee80211_link_data *link);
@@ -2620,8 +2622,8 @@ void ieee80211_recalc_dtim(struct ieee80211_local *local,
 int ieee80211_check_combinations(struct ieee80211_sub_if_data *sdata,
 				 const struct cfg80211_chan_def *chandef,
 				 enum ieee80211_chanctx_mode chanmode,
-				 u8 radar_detect);
-int ieee80211_max_num_channels(struct ieee80211_local *local);
+				 u8 radar_detect, int radio_idx);
+int ieee80211_max_num_channels(struct ieee80211_local *local, int radio_idx);
 void ieee80211_recalc_chanctx_chantype(struct ieee80211_local *local,
 				       struct ieee80211_chanctx *ctx);
 
diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c
index d1a49ee4a194..5dc85f9409cd 100644
--- a/net/mac80211/iface.c
+++ b/net/mac80211/iface.c
@@ -397,7 +397,7 @@ static int ieee80211_check_concurrent_iface(struct ieee80211_sub_if_data *sdata,
 		}
 	}
 
-	return ieee80211_check_combinations(sdata, NULL, 0, 0);
+	return ieee80211_check_combinations(sdata, NULL, 0, 0, -1);
 }
 
 static int ieee80211_check_queues(struct ieee80211_sub_if_data *sdata,
diff --git a/net/mac80211/util.c b/net/mac80211/util.c
index 927f752a0209..9a7b70b1553c 100644
--- a/net/mac80211/util.c
+++ b/net/mac80211/util.c
@@ -3928,13 +3928,96 @@ static u8 ieee80211_chanctx_radar_detect(struct ieee80211_local *local,
 	return radar_detect;
 }
 
+static u32
+__ieee80211_get_radio_mask(struct ieee80211_sub_if_data *sdata)
+{
+	struct ieee80211_local *local = sdata->local;
+	struct ieee80211_chanctx_conf *conf;
+	struct ieee80211_link_data *link;
+	u32 mask = 0;
+
+	for_each_sdata_link(local, link) {
+		conf = rcu_dereference(link->conf->chanctx_conf);
+		if (!conf || conf->radio_idx < 0)
+			continue;
+
+		mask |= BIT(conf->radio_idx);
+	}
+
+	return mask;
+}
+
+int ieee80211_get_radio_mask(struct wiphy *wiphy, struct net_device *dev,
+			     u32 *mask)
+{
+	struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev);
+
+	*mask = __ieee80211_get_radio_mask(sdata);
+
+	return 0;
+}
+
+static bool
+ieee80211_sdata_uses_radio(struct ieee80211_sub_if_data *sdata, int radio_idx)
+{
+	if (radio_idx < 0)
+		return true;
+
+	return __ieee80211_get_radio_mask(sdata) & BIT(radio_idx);
+}
+
+static void
+ieee80211_fill_ifcomb_params(struct ieee80211_local *local,
+			     struct iface_combination_params *params,
+			     const struct cfg80211_chan_def *chandef,
+			     struct ieee80211_sub_if_data *sdata,
+			     int radio_idx)
+{
+	struct ieee80211_sub_if_data *sdata_iter;
+	struct ieee80211_chanctx *ctx;
+
+	list_for_each_entry(ctx, &local->chanctx_list, list) {
+		if (ctx->replace_state == IEEE80211_CHANCTX_WILL_BE_REPLACED)
+			continue;
+
+		if (radio_idx >= 0 && ctx->conf.radio_idx != radio_idx)
+			continue;
+
+		if (chandef &&
+		    cfg80211_chandef_compatible(chandef, &ctx->conf.def))
+			continue;
+
+		params->num_different_channels++;
+
+		params->radar_detect |=
+			ieee80211_chanctx_radar_detect(local, ctx);
+	}
+
+	list_for_each_entry_rcu(sdata_iter, &local->interfaces, list) {
+		struct wireless_dev *wdev_iter;
+
+		wdev_iter = &sdata_iter->wdev;
+
+		if (sdata_iter == sdata ||
+		    !ieee80211_sdata_running(sdata_iter) ||
+		    cfg80211_iftype_allowed(local->hw.wiphy,
+					    wdev_iter->iftype, 0, 1))
+			continue;
+
+		if (!ieee80211_sdata_uses_radio(sdata_iter, radio_idx))
+			continue;
+
+		params->iftype_num[wdev_iter->iftype]++;
+	}
+}
+
 int ieee80211_check_combinations(struct ieee80211_sub_if_data *sdata,
 				 const struct cfg80211_chan_def *chandef,
 				 enum ieee80211_chanctx_mode chanmode,
-				 u8 radar_detect)
+				 u8 radar_detect, int radio_idx)
 {
+	bool shared = chanmode == IEEE80211_CHANCTX_SHARED;
 	struct ieee80211_local *local = sdata->local;
-	struct ieee80211_sub_if_data *sdata_iter;
 	enum nl80211_iftype iftype = sdata->wdev.iftype;
 	struct ieee80211_chanctx *ctx;
 	int total = 1;
@@ -3977,6 +4060,8 @@ int ieee80211_check_combinations(struct ieee80211_sub_if_data *sdata,
 	if (iftype != NL80211_IFTYPE_UNSPECIFIED)
 		params.iftype_num[iftype] = 1;
 
+	ieee80211_fill_ifcomb_params(local, &params, shared ? chandef : NULL,
+				     sdata, radio_idx);
 	list_for_each_entry(ctx, &local->chanctx_list, list) {
 		if (ctx->replace_state == IEEE80211_CHANCTX_WILL_BE_REPLACED)
 			continue;
@@ -3986,28 +4071,9 @@ int ieee80211_check_combinations(struct ieee80211_sub_if_data *sdata,
 			params.num_different_channels++;
 			continue;
 		}
-		if (chandef && chanmode == IEEE80211_CHANCTX_SHARED &&
-		    cfg80211_chandef_compatible(chandef,
-						&ctx->conf.def))
-			continue;
 		params.num_different_channels++;
 	}
 
-	list_for_each_entry_rcu(sdata_iter, &local->interfaces, list) {
-		struct wireless_dev *wdev_iter;
-
-		wdev_iter = &sdata_iter->wdev;
-
-		if (sdata_iter == sdata ||
-		    !ieee80211_sdata_running(sdata_iter) ||
-		    cfg80211_iftype_allowed(local->hw.wiphy,
-					    wdev_iter->iftype, 0, 1))
-			continue;
-
-		params.iftype_num[wdev_iter->iftype]++;
-		total++;
-	}
-
 	if (total == 1 && !params.radar_detect)
 		return 0;
 
@@ -4024,28 +4090,21 @@ ieee80211_iter_max_chans(const struct ieee80211_iface_combination *c,
 					  c->num_different_channels);
 }
 
-int ieee80211_max_num_channels(struct ieee80211_local *local)
+int ieee80211_max_num_channels(struct ieee80211_local *local, int radio_idx)
 {
-	struct ieee80211_sub_if_data *sdata;
-	struct ieee80211_chanctx *ctx;
+	struct wiphy *wiphy = local->hw.wiphy;
 	u32 max_num_different_channels = 1;
 	int err;
 	struct iface_combination_params params = {0};
 
-	lockdep_assert_wiphy(local->hw.wiphy);
-
-	list_for_each_entry(ctx, &local->chanctx_list, list) {
-		if (ctx->replace_state == IEEE80211_CHANCTX_WILL_BE_REPLACED)
-			continue;
-
-		params.num_different_channels++;
+	if (radio_idx >= wiphy->n_radio)
+		return -EINVAL;
+	else if (radio_idx >= 0)
+		params.radio = &wiphy->radio[radio_idx];
 
-		params.radar_detect |=
-			ieee80211_chanctx_radar_detect(local, ctx);
-	}
+	lockdep_assert_wiphy(local->hw.wiphy);
 
-	list_for_each_entry_rcu(sdata, &local->interfaces, list)
-		params.iftype_num[sdata->wdev.iftype]++;
+	ieee80211_fill_ifcomb_params(local, &params, NULL, NULL, radio_idx);
 
 	err = cfg80211_iter_combinations(local->hw.wiphy, &params,
 					 ieee80211_iter_max_chans,
-- 
git-series 0.9.1

Re: [RFC v3 6/8] wifi: mac80211: extend ifcomb check functions for multi-radio

[Karthikeyan Periyasamy]

On 6/6/2024 11:37 PM, Felix Fietkau wrote:
> Add support for counting global and per-radio max/current number of
> channels, as well as checking radio-specific interface combinations.
> 
> Signed-off-by: Felix Fietkau <nbd@nbd.name>
> ---
>   net/mac80211/cfg.c         |   7 +-
>   net/mac80211/chan.c        |  17 +++--
>   net/mac80211/ibss.c        |   2 +-
>   net/mac80211/ieee80211_i.h |   6 +-
>   net/mac80211/iface.c       |   2 +-
>   net/mac80211/util.c        | 131 +++++++++++++++++++++++++++-----------
>   6 files changed, 116 insertions(+), 49 deletions(-)
> 

...

>   
> +static u32
> +__ieee80211_get_radio_mask(struct ieee80211_sub_if_data *sdata)
> +{
> +	struct ieee80211_local *local = sdata->local;
> +	struct ieee80211_chanctx_conf *conf;
> +	struct ieee80211_link_data *link;
> +	u32 mask = 0;
> +
> +	for_each_sdata_link(local, link) {
> +		conf = rcu_dereference(link->conf->chanctx_conf);
> +		if (!conf || conf->radio_idx < 0)
> +			continue;
> +
> +		mask |= BIT(conf->radio_idx);
> +	}
> +
> +	return mask;
> +}
> +

I believe __ieee80211_get_radio_mask(sdata) should return the radio mask
used by this sdata right ?

if so, then you should not use "for_each_sdata_link(local, link)" 
because it iterate for all the sdata in the given local and give the 
radio mask. So always return all the radio (bitmap mask) used by the 
wiphy currently.

You can use either of below one

for_each_vif_active_link()

  or

for (link_id = 0; link_id < ARRAY_SIZE(sdata->link); link_id++)


> +int ieee80211_get_radio_mask(struct wiphy *wiphy, struct net_device *dev,
> +			     u32 *mask)
> +{
> +	struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev);
> +
> +	*mask = __ieee80211_get_radio_mask(sdata);
> +
> +	return 0;
> +}
> +
> +static bool
> +ieee80211_sdata_uses_radio(struct ieee80211_sub_if_data *sdata, int radio_idx)
> +{
> +	if (radio_idx < 0)
> +		return true;
> +
> +	return __ieee80211_get_radio_mask(sdata) & BIT(radio_idx);

same here __ieee80211_get_radio_mask(sdata) usage

> +}
> +
-- 
Karthikeyan Periyasamy
--
கார்த்திகேயன் பெரியசாமி

Re: [RFC v3 6/8] wifi: mac80211: extend ifcomb check functions for multi-radio

[Felix Fietkau]

On 07.06.24 06:45, Karthikeyan Periyasamy wrote:
> 
> 
> On 6/6/2024 11:37 PM, Felix Fietkau wrote:
>> Add support for counting global and per-radio max/current number of
>> channels, as well as checking radio-specific interface combinations.
>> 
>> Signed-off-by: Felix Fietkau <nbd@nbd.name>
>> ---
>>   net/mac80211/cfg.c         |   7 +-
>>   net/mac80211/chan.c        |  17 +++--
>>   net/mac80211/ibss.c        |   2 +-
>>   net/mac80211/ieee80211_i.h |   6 +-
>>   net/mac80211/iface.c       |   2 +-
>>   net/mac80211/util.c        | 131 +++++++++++++++++++++++++++-----------
>>   6 files changed, 116 insertions(+), 49 deletions(-)
>> 
> 
> ...
> 
>>   
>> +static u32
>> +__ieee80211_get_radio_mask(struct ieee80211_sub_if_data *sdata)
>> +{
>> +	struct ieee80211_local *local = sdata->local;
>> +	struct ieee80211_chanctx_conf *conf;
>> +	struct ieee80211_link_data *link;
>> +	u32 mask = 0;
>> +
>> +	for_each_sdata_link(local, link) {
>> +		conf = rcu_dereference(link->conf->chanctx_conf);
>> +		if (!conf || conf->radio_idx < 0)
>> +			continue;
>> +
>> +		mask |= BIT(conf->radio_idx);
>> +	}
>> +
>> +	return mask;
>> +}
>> +
> 
> I believe __ieee80211_get_radio_mask(sdata) should return the radio mask
> used by this sdata right ?
> 
> if so, then you should not use "for_each_sdata_link(local, link)"
> because it iterate for all the sdata in the given local and give the
> radio mask. So always return all the radio (bitmap mask) used by the
> wiphy currently.
> 
> You can use either of below one
> 
> for_each_vif_active_link()
> 
>    or
> 
> for (link_id = 0; link_id < ARRAY_SIZE(sdata->link); link_id++)

Right, I copied the wrong code :)
Will fix, thanks.

- Felix

Re: [RFC v3 6/8] wifi: mac80211: extend ifcomb check functions for multi-radio

[Karthikeyan Periyasamy]

On 6/6/2024 11:37 PM, Felix Fietkau wrote:

...

>   
> +static u32
> +__ieee80211_get_radio_mask(struct ieee80211_sub_if_data *sdata)
> +{
> +	struct ieee80211_local *local = sdata->local;
> +	struct ieee80211_chanctx_conf *conf;
> +	struct ieee80211_link_data *link;
> +	u32 mask = 0;
> +
> +	for_each_sdata_link(local, link) {
> +		conf = rcu_dereference(link->conf->chanctx_conf);
> +		if (!conf || conf->radio_idx < 0)
> +			continue;
> +
> +		mask |= BIT(conf->radio_idx);
> +	}
> +
> +	return mask;
> +}
> +
> +int ieee80211_get_radio_mask(struct wiphy *wiphy, struct net_device *dev,
> +			     u32 *mask)
> +{
> +	struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev);
> +
> +	*mask = __ieee80211_get_radio_mask(sdata);
> +
> +	return 0;
> +}
> +
> +static bool
> +ieee80211_sdata_uses_radio(struct ieee80211_sub_if_data *sdata, int radio_idx)
> +{
> +	if (radio_idx < 0)
> +		return true;
> +
> +	return __ieee80211_get_radio_mask(sdata) & BIT(radio_idx);
> +}
> +
> +static void
> +ieee80211_fill_ifcomb_params(struct ieee80211_local *local,
> +			     struct iface_combination_params *params,
> +			     const struct cfg80211_chan_def *chandef,
> +			     struct ieee80211_sub_if_data *sdata,
> +			     int radio_idx)
> +{
> +	struct ieee80211_sub_if_data *sdata_iter;
> +	struct ieee80211_chanctx *ctx;
> +
> +	list_for_each_entry(ctx, &local->chanctx_list, list) {
> +		if (ctx->replace_state == IEEE80211_CHANCTX_WILL_BE_REPLACED)
> +			continue;
> +
> +		if (radio_idx >= 0 && ctx->conf.radio_idx != radio_idx)
> +			continue;
> +
> +		if (chandef &&
> +		    cfg80211_chandef_compatible(chandef, &ctx->conf.def))
> +			continue;
> +
> +		params->num_different_channels++;
> +
> +		params->radar_detect |=
> +			ieee80211_chanctx_radar_detect(local, ctx);
> +	}
> +
> +	list_for_each_entry_rcu(sdata_iter, &local->interfaces, list) {
> +		struct wireless_dev *wdev_iter;
> +
> +		wdev_iter = &sdata_iter->wdev;
> +
> +		if (sdata_iter == sdata ||
> +		    !ieee80211_sdata_running(sdata_iter) ||
> +		    cfg80211_iftype_allowed(local->hw.wiphy,
> +					    wdev_iter->iftype, 0, 1))
> +			continue;
> +
> +		if (!ieee80211_sdata_uses_radio(sdata_iter, radio_idx))
> +			continue;
> +
> +		params->iftype_num[wdev_iter->iftype]++;
> +	}
> +}
> +
>   int ieee80211_check_combinations(struct ieee80211_sub_if_data *sdata,
>   				 const struct cfg80211_chan_def *chandef,
>   				 enum ieee80211_chanctx_mode chanmode,
> -				 u8 radar_detect)
> +				 u8 radar_detect, int radio_idx)
>   {
> +	bool shared = chanmode == IEEE80211_CHANCTX_SHARED;
>   	struct ieee80211_local *local = sdata->local;
> -	struct ieee80211_sub_if_data *sdata_iter;
>   	enum nl80211_iftype iftype = sdata->wdev.iftype;
>   	struct ieee80211_chanctx *ctx;
>   	int total = 1;
> @@ -3977,6 +4060,8 @@ int ieee80211_check_combinations(struct ieee80211_sub_if_data *sdata,
>   	if (iftype != NL80211_IFTYPE_UNSPECIFIED)
>   		params.iftype_num[iftype] = 1;
>   
> +	ieee80211_fill_ifcomb_params(local, &params, shared ? chandef : NULL,
> +				     sdata, radio_idx);
>   	list_for_each_entry(ctx, &local->chanctx_list, list) {
>   		if (ctx->replace_state == IEEE80211_CHANCTX_WILL_BE_REPLACED)
>   			continue;
> @@ -3986,28 +4071,9 @@ int ieee80211_check_combinations(struct ieee80211_sub_if_data *sdata,
>   			params.num_different_channels++;
>   			continue;
>   		}
> -		if (chandef && chanmode == IEEE80211_CHANCTX_SHARED &&
> -		    cfg80211_chandef_compatible(chandef,
> -						&ctx->conf.def))
> -			continue;
>   		params.num_different_channels++;
>   	}
>   
> -	list_for_each_entry_rcu(sdata_iter, &local->interfaces, list) {
> -		struct wireless_dev *wdev_iter;
> -
> -		wdev_iter = &sdata_iter->wdev;
> -
> -		if (sdata_iter == sdata ||
> -		    !ieee80211_sdata_running(sdata_iter) ||
> -		    cfg80211_iftype_allowed(local->hw.wiphy,
> -					    wdev_iter->iftype, 0, 1))
> -			continue;
> -
> -		params.iftype_num[wdev_iter->iftype]++;
> -		total++;
> -	}
> -
>   	if (total == 1 && !params.radar_detect)
>   		return 0;

ieee80211_check_combinations() missed to update "params.radio", no ?

-- 
Karthikeyan Periyasamy
--
கார்த்திகேயன் பெரியசாமி

Re: [RFC v3 6/8] wifi: mac80211: extend ifcomb check functions for multi-radio

[Karthikeyan Periyasamy]

On 6/6/2024 11:37 PM, Felix Fietkau wrote:
> Add support for counting global and per-radio max/current number of
> channels, as well as checking radio-specific interface combinations.
> 
> Signed-off-by: Felix Fietkau <nbd@nbd.name>
> ---
>   net/mac80211/cfg.c         |   7 +-
>   net/mac80211/chan.c        |  17 +++--
>   net/mac80211/ibss.c        |   2 +-
>   net/mac80211/ieee80211_i.h |   6 +-
>   net/mac80211/iface.c       |   2 +-
>   net/mac80211/util.c        | 131 +++++++++++++++++++++++++++-----------
>   6 files changed, 116 insertions(+), 49 deletions(-)
> 
> diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
> index 62119e957cd8..950b7b72f0b8 100644
> --- a/net/mac80211/cfg.c
> +++ b/net/mac80211/cfg.c
> @@ -263,7 +263,7 @@ static int ieee80211_start_p2p_device(struct wiphy *wiphy,
>   
>   	lockdep_assert_wiphy(sdata->local->hw.wiphy);
>   
> -	ret = ieee80211_check_combinations(sdata, NULL, 0, 0);
> +	ret = ieee80211_check_combinations(sdata, NULL, 0, 0, -1);
>   	if (ret < 0)
>   		return ret;
>   
> @@ -285,7 +285,7 @@ static int ieee80211_start_nan(struct wiphy *wiphy,
>   
>   	lockdep_assert_wiphy(sdata->local->hw.wiphy);
>   
> -	ret = ieee80211_check_combinations(sdata, NULL, 0, 0);
> +	ret = ieee80211_check_combinations(sdata, NULL, 0, 0, -1);
>   	if (ret < 0)
>   		return ret;
>   
> @@ -4001,7 +4001,7 @@ __ieee80211_channel_switch(struct wiphy *wiphy, struct net_device *dev,
>   		goto out;
>   
>   	/* if reservation is invalid then this will fail */
> -	err = ieee80211_check_combinations(sdata, NULL, chanctx->mode, 0);
> +	err = ieee80211_check_combinations(sdata, NULL, chanctx->mode, 0, -1);

Once we reach the global limit, all the -1 passing caller get fail 
becuase the iface check param (existing and new) is validated against 
the global limit. since global limt as a sort of union of all per-radio 
limits.

Ex:
Global iface = 6 (Radio iface 2GHz:4, 5GHz:4, 6GHz:6)


So far 6 iface created (Radio iface 2GHz:2, 5GHz:3, 6GHz:1)

In this case, new iface creation get fail because caller uses 
ieee80211_check_combinations() with -1 as radio idx, so it checked 
against global limit


-- 
Karthikeyan Periyasamy
--
கார்த்திகேயன் பெரியசாமி

Re: [RFC v3 6/8] wifi: mac80211: extend ifcomb check functions for multi-radio

[Felix Fietkau]

On 07.06.24 12:07, Karthikeyan Periyasamy wrote:
> 
> 
> On 6/6/2024 11:37 PM, Felix Fietkau wrote:
>> Add support for counting global and per-radio max/current number of
>> channels, as well as checking radio-specific interface combinations.
>> 
>> Signed-off-by: Felix Fietkau <nbd@nbd.name>
>> ---
>>   net/mac80211/cfg.c         |   7 +-
>>   net/mac80211/chan.c        |  17 +++--
>>   net/mac80211/ibss.c        |   2 +-
>>   net/mac80211/ieee80211_i.h |   6 +-
>>   net/mac80211/iface.c       |   2 +-
>>   net/mac80211/util.c        | 131 +++++++++++++++++++++++++++-----------
>>   6 files changed, 116 insertions(+), 49 deletions(-)
>> 
>> diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
>> index 62119e957cd8..950b7b72f0b8 100644
>> --- a/net/mac80211/cfg.c
>> +++ b/net/mac80211/cfg.c
>> @@ -263,7 +263,7 @@ static int ieee80211_start_p2p_device(struct wiphy *wiphy,
>>   
>>   	lockdep_assert_wiphy(sdata->local->hw.wiphy);
>>   
>> -	ret = ieee80211_check_combinations(sdata, NULL, 0, 0);
>> +	ret = ieee80211_check_combinations(sdata, NULL, 0, 0, -1);
>>   	if (ret < 0)
>>   		return ret;
>>   
>> @@ -285,7 +285,7 @@ static int ieee80211_start_nan(struct wiphy *wiphy,
>>   
>>   	lockdep_assert_wiphy(sdata->local->hw.wiphy);
>>   
>> -	ret = ieee80211_check_combinations(sdata, NULL, 0, 0);
>> +	ret = ieee80211_check_combinations(sdata, NULL, 0, 0, -1);
>>   	if (ret < 0)
>>   		return ret;
>>   
>> @@ -4001,7 +4001,7 @@ __ieee80211_channel_switch(struct wiphy *wiphy, struct net_device *dev,
>>   		goto out;
>>   
>>   	/* if reservation is invalid then this will fail */
>> -	err = ieee80211_check_combinations(sdata, NULL, chanctx->mode, 0);
>> +	err = ieee80211_check_combinations(sdata, NULL, chanctx->mode, 0, -1);
> 
> Once we reach the global limit, all the -1 passing caller get fail
> becuase the iface check param (existing and new) is validated against
> the global limit. since global limt as a sort of union of all per-radio
> limits.
> 
> Ex:
> Global iface = 6 (Radio iface 2GHz:4, 5GHz:4, 6GHz:6)
> 
> 
> So far 6 iface created (Radio iface 2GHz:2, 5GHz:3, 6GHz:1)
> 
> In this case, new iface creation get fail because caller uses
> ieee80211_check_combinations() with -1 as radio idx, so it checked
> against global limit

Use the sum of the number of interfaces from each radio instead of the 
maximum.

- Felix

Re: [RFC v3 6/8] wifi: mac80211: extend ifcomb check functions for multi-radio

[Karthikeyan Periyasamy]

On 6/7/2024 3:52 PM, Felix Fietkau wrote:
> On 07.06.24 12:07, Karthikeyan Periyasamy wrote:
>>
>>
>> On 6/6/2024 11:37 PM, Felix Fietkau wrote:
>>> Add support for counting global and per-radio max/current number of
>>> channels, as well as checking radio-specific interface combinations.
>>>
>>> Signed-off-by: Felix Fietkau <nbd@nbd.name>
>>> ---
>>>   net/mac80211/cfg.c         |   7 +-
>>>   net/mac80211/chan.c        |  17 +++--
>>>   net/mac80211/ibss.c        |   2 +-
>>>   net/mac80211/ieee80211_i.h |   6 +-
>>>   net/mac80211/iface.c       |   2 +-
>>>   net/mac80211/util.c        | 131 
>>> +++++++++++++++++++++++++++-----------
>>>   6 files changed, 116 insertions(+), 49 deletions(-)
>>>
>>> diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
>>> index 62119e957cd8..950b7b72f0b8 100644
>>> --- a/net/mac80211/cfg.c
>>> +++ b/net/mac80211/cfg.c
>>> @@ -263,7 +263,7 @@ static int ieee80211_start_p2p_device(struct 
>>> wiphy *wiphy,
>>>       lockdep_assert_wiphy(sdata->local->hw.wiphy);
>>> -    ret = ieee80211_check_combinations(sdata, NULL, 0, 0);
>>> +    ret = ieee80211_check_combinations(sdata, NULL, 0, 0, -1);
>>>       if (ret < 0)
>>>           return ret;
>>> @@ -285,7 +285,7 @@ static int ieee80211_start_nan(struct wiphy *wiphy,
>>>       lockdep_assert_wiphy(sdata->local->hw.wiphy);
>>> -    ret = ieee80211_check_combinations(sdata, NULL, 0, 0);
>>> +    ret = ieee80211_check_combinations(sdata, NULL, 0, 0, -1);
>>>       if (ret < 0)
>>>           return ret;
>>> @@ -4001,7 +4001,7 @@ __ieee80211_channel_switch(struct wiphy *wiphy, 
>>> struct net_device *dev,
>>>           goto out;
>>>       /* if reservation is invalid then this will fail */
>>> -    err = ieee80211_check_combinations(sdata, NULL, chanctx->mode, 0);
>>> +    err = ieee80211_check_combinations(sdata, NULL, chanctx->mode, 
>>> 0, -1);
>>
>> Once we reach the global limit, all the -1 passing caller get fail
>> becuase the iface check param (existing and new) is validated against
>> the global limit. since global limt as a sort of union of all per-radio
>> limits.
>>
>> Ex:
>> Global iface = 6 (Radio iface 2GHz:4, 5GHz:4, 6GHz:6)
>>
>>
>> So far 6 iface created (Radio iface 2GHz:2, 5GHz:3, 6GHz:1)
>>
>> In this case, new iface creation get fail because caller uses
>> ieee80211_check_combinations() with -1 as radio idx, so it checked
>> against global limit
> 
> Use the sum of the number of interfaces from each radio instead of the 
> maximum.

Oh, then legacy user have misconception of the global interfaces 
advertised and try to fail for the allowed limits.

-- 
Karthikeyan Periyasamy
--
கார்த்திகேயன் பெரியசாமி

Re: [RFC v3 6/8] wifi: mac80211: extend ifcomb check functions for multi-radio

[Felix Fietkau]

On 07.06.24 12:53, Karthikeyan Periyasamy wrote:
> 
> 
> On 6/7/2024 3:52 PM, Felix Fietkau wrote:
>> On 07.06.24 12:07, Karthikeyan Periyasamy wrote:
>>>
>>>
>>> On 6/6/2024 11:37 PM, Felix Fietkau wrote:
>>>> Add support for counting global and per-radio max/current number of
>>>> channels, as well as checking radio-specific interface combinations.
>>>>
>>>> Signed-off-by: Felix Fietkau <nbd@nbd.name>
>>>> ---
>>>>   net/mac80211/cfg.c         |   7 +-
>>>>   net/mac80211/chan.c        |  17 +++--
>>>>   net/mac80211/ibss.c        |   2 +-
>>>>   net/mac80211/ieee80211_i.h |   6 +-
>>>>   net/mac80211/iface.c       |   2 +-
>>>>   net/mac80211/util.c        | 131 
>>>> +++++++++++++++++++++++++++-----------
>>>>   6 files changed, 116 insertions(+), 49 deletions(-)
>>>>
>>>> diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
>>>> index 62119e957cd8..950b7b72f0b8 100644
>>>> --- a/net/mac80211/cfg.c
>>>> +++ b/net/mac80211/cfg.c
>>>> @@ -263,7 +263,7 @@ static int ieee80211_start_p2p_device(struct 
>>>> wiphy *wiphy,
>>>>       lockdep_assert_wiphy(sdata->local->hw.wiphy);
>>>> -    ret = ieee80211_check_combinations(sdata, NULL, 0, 0);
>>>> +    ret = ieee80211_check_combinations(sdata, NULL, 0, 0, -1);
>>>>       if (ret < 0)
>>>>           return ret;
>>>> @@ -285,7 +285,7 @@ static int ieee80211_start_nan(struct wiphy *wiphy,
>>>>       lockdep_assert_wiphy(sdata->local->hw.wiphy);
>>>> -    ret = ieee80211_check_combinations(sdata, NULL, 0, 0);
>>>> +    ret = ieee80211_check_combinations(sdata, NULL, 0, 0, -1);
>>>>       if (ret < 0)
>>>>           return ret;
>>>> @@ -4001,7 +4001,7 @@ __ieee80211_channel_switch(struct wiphy *wiphy, 
>>>> struct net_device *dev,
>>>>           goto out;
>>>>       /* if reservation is invalid then this will fail */
>>>> -    err = ieee80211_check_combinations(sdata, NULL, chanctx->mode, 0);
>>>> +    err = ieee80211_check_combinations(sdata, NULL, chanctx->mode, 
>>>> 0, -1);
>>>
>>> Once we reach the global limit, all the -1 passing caller get fail
>>> becuase the iface check param (existing and new) is validated against
>>> the global limit. since global limt as a sort of union of all per-radio
>>> limits.
>>>
>>> Ex:
>>> Global iface = 6 (Radio iface 2GHz:4, 5GHz:4, 6GHz:6)
>>>
>>>
>>> So far 6 iface created (Radio iface 2GHz:2, 5GHz:3, 6GHz:1)
>>>
>>> In this case, new iface creation get fail because caller uses
>>> ieee80211_check_combinations() with -1 as radio idx, so it checked
>>> against global limit
>> 
>> Use the sum of the number of interfaces from each radio instead of the 
>> maximum.
> 
> Oh, then legacy user have misconception of the global interfaces
> advertised and try to fail for the allowed limits.

Sure, but that might be an issue either way until user space is updated 
and users start looking at the per-radio ifcomb data.
The global data is simply not enough to describe the details of the 
radio split.

- Felix

Re: [RFC v3 6/8] wifi: mac80211: extend ifcomb check functions for multi-radio

[Johannes Berg]

On Fri, 2024-06-07 at 13:04 +0200, Felix Fietkau wrote:
> > > 
> > > Use the sum of the number of interfaces from each radio instead of the 
> > > maximum.
> > 
> > Oh, then legacy user have misconception of the global interfaces
> > advertised and try to fail for the allowed limits.
> 
> Sure, but that might be an issue either way until user space is updated 
> and users start looking at the per-radio ifcomb data.

I'm kind of with Karthikeyan here - this could be understood as a
regression, since you're now telling userspace something you can't
actually do.

> The global data is simply not enough to describe the details of the 
> radio split.

Obviously, but that doesn't mean the global data as advertised in the
existing attributes must be *wrong*. It could be a subset, and the
superset data is only available to new implementations.

johannes

Re: [RFC v3 6/8] wifi: mac80211: extend ifcomb check functions for multi-radio

[Felix Fietkau]

On 12.06.24 14:05, Johannes Berg wrote:
> On Fri, 2024-06-07 at 13:04 +0200, Felix Fietkau wrote:
>> > > 
>> > > Use the sum of the number of interfaces from each radio instead of the 
>> > > maximum.
>> > 
>> > Oh, then legacy user have misconception of the global interfaces
>> > advertised and try to fail for the allowed limits.
>> 
>> Sure, but that might be an issue either way until user space is updated 
>> and users start looking at the per-radio ifcomb data.
> 
> I'm kind of with Karthikeyan here - this could be understood as a
> regression, since you're now telling userspace something you can't
> actually do.

Well, we can actually do it, just with some extra restrictions - i.e. 
the interfaces we create need to be spread across radios to match the 
per-radio limits.

>> The global data is simply not enough to describe the details of the 
>> radio split.
> 
> Obviously, but that doesn't mean the global data as advertised in the
> existing attributes must be *wrong*. It could be a subset, and the
> superset data is only available to new implementations.
So you'd prefer something like picking one radio and advertising its 
limits instead?

- Felix

[RFC v3 7/8] wifi: mac80211: move code in ieee80211_link_reserve_chanctx to a helper

[Felix Fietkau]

Reduces indentation in preparation for further changes

Signed-off-by: Felix Fietkau <nbd@nbd.name>
---
 net/mac80211/chan.c | 141 ++++++++++++++++++++++-----------------------
 1 file changed, 72 insertions(+), 69 deletions(-)

diff --git a/net/mac80211/chan.c b/net/mac80211/chan.c
index 0e899c07bc2b..ac49c2c71d2b 100644
--- a/net/mac80211/chan.c
+++ b/net/mac80211/chan.c
@@ -1060,6 +1060,71 @@ int ieee80211_link_unreserve_chanctx(struct ieee80211_link_data *link)
 	return 0;
 }
 
+static struct ieee80211_chanctx *
+ieee80211_replace_chanctx(struct ieee80211_local *local,
+			  const struct ieee80211_chan_req *chanreq,
+			  enum ieee80211_chanctx_mode mode,
+			  struct ieee80211_chanctx *curr_ctx)
+{
+	struct ieee80211_chanctx *new_ctx, *ctx;
+
+	if (!curr_ctx || (curr_ctx->replace_state ==
+			  IEEE80211_CHANCTX_WILL_BE_REPLACED) ||
+	    !list_empty(&curr_ctx->reserved_links)) {
+		/*
+		 * Another link already requested this context for a
+		 * reservation. Find another one hoping all links assigned
+		 * to it will also switch soon enough.
+		 *
+		 * TODO: This needs a little more work as some cases
+		 * (more than 2 chanctx capable devices) may fail which could
+		 * otherwise succeed provided some channel context juggling was
+		 * performed.
+		 *
+		 * Consider ctx1..3, link1..6, each ctx has 2 links. link1 and
+		 * link2 from ctx1 request new different chandefs starting 2
+		 * in-place reserations with ctx4 and ctx5 replacing ctx1 and
+		 * ctx2 respectively. Next link5 and link6 from ctx3 reserve
+		 * ctx4. If link3 and link4 remain on ctx2 as they are then this
+		 * fails unless `replace_ctx` from ctx5 is replaced with ctx3.
+		 */
+		list_for_each_entry(ctx, &local->chanctx_list, list) {
+			if (ctx->replace_state !=
+			    IEEE80211_CHANCTX_REPLACE_NONE)
+				continue;
+
+			if (!list_empty(&ctx->reserved_links))
+				continue;
+
+			curr_ctx = ctx;
+			break;
+		}
+	}
+
+	/*
+	 * If that's true then all available contexts already have reservations
+	 * and cannot be used.
+	 */
+	if (!curr_ctx || (curr_ctx->replace_state ==
+			  IEEE80211_CHANCTX_WILL_BE_REPLACED) ||
+	    !list_empty(&curr_ctx->reserved_links))
+		return ERR_PTR(-EBUSY);
+
+	new_ctx = ieee80211_alloc_chanctx(local, chanreq, mode, -1);
+	if (!new_ctx)
+		return ERR_PTR(-ENOMEM);
+
+	new_ctx->replace_ctx = curr_ctx;
+	new_ctx->replace_state = IEEE80211_CHANCTX_REPLACES_OTHER;
+
+	curr_ctx->replace_ctx = new_ctx;
+	curr_ctx->replace_state = IEEE80211_CHANCTX_WILL_BE_REPLACED;
+
+	list_add_rcu(&new_ctx->list, &local->chanctx_list);
+
+	return new_ctx;
+}
+
 int ieee80211_link_reserve_chanctx(struct ieee80211_link_data *link,
 				   const struct ieee80211_chan_req *chanreq,
 				   enum ieee80211_chanctx_mode mode,
@@ -1067,7 +1132,7 @@ int ieee80211_link_reserve_chanctx(struct ieee80211_link_data *link,
 {
 	struct ieee80211_sub_if_data *sdata = link->sdata;
 	struct ieee80211_local *local = sdata->local;
-	struct ieee80211_chanctx *new_ctx, *curr_ctx, *ctx;
+	struct ieee80211_chanctx *new_ctx, *curr_ctx;
 
 	lockdep_assert_wiphy(local->hw.wiphy);
 
@@ -1077,76 +1142,14 @@ int ieee80211_link_reserve_chanctx(struct ieee80211_link_data *link,
 
 	new_ctx = ieee80211_find_reservation_chanctx(local, chanreq, mode);
 	if (!new_ctx) {
-		if (ieee80211_can_create_new_chanctx(local, -1)) {
+		if (ieee80211_can_create_new_chanctx(local, -1))
 			new_ctx = ieee80211_new_chanctx(local, chanreq, mode,
 							false);
-			if (IS_ERR(new_ctx))
-				return PTR_ERR(new_ctx);
-		} else {
-			if (!curr_ctx ||
-			    (curr_ctx->replace_state ==
-			     IEEE80211_CHANCTX_WILL_BE_REPLACED) ||
-			    !list_empty(&curr_ctx->reserved_links)) {
-				/*
-				 * Another link already requested this context
-				 * for a reservation. Find another one hoping
-				 * all links assigned to it will also switch
-				 * soon enough.
-				 *
-				 * TODO: This needs a little more work as some
-				 * cases (more than 2 chanctx capable devices)
-				 * may fail which could otherwise succeed
-				 * provided some channel context juggling was
-				 * performed.
-				 *
-				 * Consider ctx1..3, link1..6, each ctx has 2
-				 * links. link1 and link2 from ctx1 request new
-				 * different chandefs starting 2 in-place
-				 * reserations with ctx4 and ctx5 replacing
-				 * ctx1 and ctx2 respectively. Next link5 and
-				 * link6 from ctx3 reserve ctx4. If link3 and
-				 * link4 remain on ctx2 as they are then this
-				 * fails unless `replace_ctx` from ctx5 is
-				 * replaced with ctx3.
-				 */
-				list_for_each_entry(ctx, &local->chanctx_list,
-						    list) {
-					if (ctx->replace_state !=
-					    IEEE80211_CHANCTX_REPLACE_NONE)
-						continue;
-
-					if (!list_empty(&ctx->reserved_links))
-						continue;
-
-					curr_ctx = ctx;
-					break;
-				}
-			}
-
-			/*
-			 * If that's true then all available contexts already
-			 * have reservations and cannot be used.
-			 */
-			if (!curr_ctx ||
-			    (curr_ctx->replace_state ==
-			     IEEE80211_CHANCTX_WILL_BE_REPLACED) ||
-			    !list_empty(&curr_ctx->reserved_links))
-				return -EBUSY;
-
-			new_ctx = ieee80211_alloc_chanctx(local, chanreq, mode, -1);
-			if (!new_ctx)
-				return -ENOMEM;
-
-			new_ctx->replace_ctx = curr_ctx;
-			new_ctx->replace_state =
-					IEEE80211_CHANCTX_REPLACES_OTHER;
-
-			curr_ctx->replace_ctx = new_ctx;
-			curr_ctx->replace_state =
-					IEEE80211_CHANCTX_WILL_BE_REPLACED;
-
-			list_add_rcu(&new_ctx->list, &local->chanctx_list);
-		}
+		else
+			new_ctx = ieee80211_replace_chanctx(local, chanreq,
+							    mode, curr_ctx);
+		if (IS_ERR(new_ctx))
+			return PTR_ERR(new_ctx);
 	}
 
 	list_add(&link->reserved_chanctx_list, &new_ctx->reserved_links);
-- 
git-series 0.9.1

[RFC v3 8/8] wifi: mac80211: add wiphy radio assignment and validation

[Felix Fietkau]

Validate number of channels and interface combinations per radio.
Assign each channel context to a radio.

Signed-off-by: Felix Fietkau <nbd@nbd.name>
---
 net/mac80211/chan.c | 70 ++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 65 insertions(+), 5 deletions(-)

diff --git a/net/mac80211/chan.c b/net/mac80211/chan.c
index ac49c2c71d2b..257ee3b1100b 100644
--- a/net/mac80211/chan.c
+++ b/net/mac80211/chan.c
@@ -696,14 +696,15 @@ static struct ieee80211_chanctx *
 ieee80211_new_chanctx(struct ieee80211_local *local,
 		      const struct ieee80211_chan_req *chanreq,
 		      enum ieee80211_chanctx_mode mode,
-		      bool assign_on_failure)
+		      bool assign_on_failure,
+		      int radio_idx)
 {
 	struct ieee80211_chanctx *ctx;
 	int err;
 
 	lockdep_assert_wiphy(local->hw.wiphy);
 
-	ctx = ieee80211_alloc_chanctx(local, chanreq, mode, -1);
+	ctx = ieee80211_alloc_chanctx(local, chanreq, mode, radio_idx);
 	if (!ctx)
 		return ERR_PTR(-ENOMEM);
 
@@ -1060,6 +1061,24 @@ int ieee80211_link_unreserve_chanctx(struct ieee80211_link_data *link)
 	return 0;
 }
 
+static bool
+ieee80211_radio_freq_match(const struct wiphy_radio *radio,
+			   const struct ieee80211_chan_req *chanreq)
+{
+	const struct wiphy_radio_freq_range *r;
+	u32 freq;
+	int i;
+
+	freq = ieee80211_channel_to_khz(chanreq->oper.chan);
+	for (i = 0; i < radio->n_freq_range; i++) {
+		r = &radio->freq_range[i];
+		if (freq >= r->start_freq && freq <= r->end_freq)
+			return true;
+	}
+
+	return false;
+}
+
 static struct ieee80211_chanctx *
 ieee80211_replace_chanctx(struct ieee80211_local *local,
 			  const struct ieee80211_chan_req *chanreq,
@@ -1067,6 +1086,8 @@ ieee80211_replace_chanctx(struct ieee80211_local *local,
 			  struct ieee80211_chanctx *curr_ctx)
 {
 	struct ieee80211_chanctx *new_ctx, *ctx;
+	struct wiphy *wiphy = local->hw.wiphy;
+	const struct wiphy_radio *radio;
 
 	if (!curr_ctx || (curr_ctx->replace_state ==
 			  IEEE80211_CHANCTX_WILL_BE_REPLACED) ||
@@ -1096,6 +1117,12 @@ ieee80211_replace_chanctx(struct ieee80211_local *local,
 			if (!list_empty(&ctx->reserved_links))
 				continue;
 
+			if (ctx->conf.radio_idx >= 0) {
+					radio = &wiphy->radio[ctx->conf.radio_idx];
+					if (!ieee80211_radio_freq_match(radio, chanreq))
+							continue;
+			}
+
 			curr_ctx = ctx;
 			break;
 		}
@@ -1125,6 +1152,34 @@ ieee80211_replace_chanctx(struct ieee80211_local *local,
 	return new_ctx;
 }
 
+static bool
+ieee80211_find_available_radio(struct ieee80211_local *local,
+			       const struct ieee80211_chan_req *chanreq,
+			       int *radio_idx)
+{
+	struct wiphy *wiphy = local->hw.wiphy;
+	const struct wiphy_radio *radio;
+	int i;
+
+	*radio_idx = -1;
+	if (!wiphy->n_radio)
+		return true;
+
+	for (i = 0; i < wiphy->n_radio; i++) {
+		radio = &wiphy->radio[i];
+		if (!ieee80211_radio_freq_match(radio, chanreq))
+			continue;
+
+		if (!ieee80211_can_create_new_chanctx(local, i))
+			continue;
+
+		*radio_idx = i;
+		return true;
+	}
+
+	return false;
+}
+
 int ieee80211_link_reserve_chanctx(struct ieee80211_link_data *link,
 				   const struct ieee80211_chan_req *chanreq,
 				   enum ieee80211_chanctx_mode mode,
@@ -1133,6 +1188,7 @@ int ieee80211_link_reserve_chanctx(struct ieee80211_link_data *link,
 	struct ieee80211_sub_if_data *sdata = link->sdata;
 	struct ieee80211_local *local = sdata->local;
 	struct ieee80211_chanctx *new_ctx, *curr_ctx;
+	int radio_idx;
 
 	lockdep_assert_wiphy(local->hw.wiphy);
 
@@ -1142,9 +1198,10 @@ int ieee80211_link_reserve_chanctx(struct ieee80211_link_data *link,
 
 	new_ctx = ieee80211_find_reservation_chanctx(local, chanreq, mode);
 	if (!new_ctx) {
-		if (ieee80211_can_create_new_chanctx(local, -1))
+		if (ieee80211_can_create_new_chanctx(local, -1) &&
+		    ieee80211_find_available_radio(local, chanreq, &radio_idx))
 			new_ctx = ieee80211_new_chanctx(local, chanreq, mode,
-							false);
+							false, radio_idx);
 		else
 			new_ctx = ieee80211_replace_chanctx(local, chanreq,
 							    mode, curr_ctx);
@@ -1755,6 +1812,7 @@ int _ieee80211_link_use_channel(struct ieee80211_link_data *link,
 	struct ieee80211_chanctx *ctx;
 	u8 radar_detect_width = 0;
 	bool reserved = false;
+	int radio_idx;
 	int ret;
 
 	lockdep_assert_wiphy(local->hw.wiphy);
@@ -1785,9 +1843,11 @@ int _ieee80211_link_use_channel(struct ieee80211_link_data *link,
 	/* Note: context is now reserved */
 	if (ctx)
 		reserved = true;
+	else if (!ieee80211_find_available_radio(local, chanreq, &radio_idx))
+		ctx = ERR_PTR(-EBUSY);
 	else
 		ctx = ieee80211_new_chanctx(local, chanreq, mode,
-					    assign_on_failure);
+					    assign_on_failure, radio_idx);
 	if (IS_ERR(ctx)) {
 		ret = PTR_ERR(ctx);
 		goto out;
-- 
git-series 0.9.1

Re: [RFC v3 8/8] wifi: mac80211: add wiphy radio assignment and validation

[Johannes Berg]

On Thu, 2024-06-06 at 20:07 +0200, Felix Fietkau wrote:
> 
> +static bool
> +ieee80211_radio_freq_match(const struct wiphy_radio *radio,
> +			   const struct ieee80211_chan_req *chanreq)
> +{
> +	const struct wiphy_radio_freq_range *r;
> +	u32 freq;
> +	int i;
> +
> +	freq = ieee80211_channel_to_khz(chanreq->oper.chan);
> +	for (i = 0; i < radio->n_freq_range; i++) {
> +		r = &radio->freq_range[i];
> +		if (freq >= r->start_freq && freq <= r->end_freq)
> +			return true;

IMHO should be inclusive only on one side of the range. Can always make
it work since channels are at least a few MHz apart, but the purist in
me says it's easier to grok if the end is not inclusive :)

Maybe this should be a cfg80211 helper like

struct wiphy_radio *wiphy_get_radio(struct wiphy *wiphy, ... *chandef);

which could also check that the _whole_ chandef fits (though that should
probably also be handled elsewhere, like chandef_valid), and you can use
it to get the radio pointer and then check for == below.

The point would be to have a single place with this kind of range logic.
This is only going to get done by mac80211 now, but it'd really be
awkward if some other driver had some other logic later.

>  	if (!curr_ctx || (curr_ctx->replace_state ==
>  			  IEEE80211_CHANCTX_WILL_BE_REPLACED) ||
> @@ -1096,6 +1117,12 @@ ieee80211_replace_chanctx(struct ieee80211_local *local,
>  			if (!list_empty(&ctx->reserved_links))
>  				continue;
>  
> +			if (ctx->conf.radio_idx >= 0) {
> +					radio = &wiphy->radio[ctx->conf.radio_idx];
> +					if (!ieee80211_radio_freq_match(radio, chanreq))
> +							continue;
> +			}

something happened to indentation here :)

> +static bool
> +ieee80211_find_available_radio(struct ieee80211_local *local,
> +			       const struct ieee80211_chan_req *chanreq,
> +			       int *radio_idx)
> +{
> +	struct wiphy *wiphy = local->hw.wiphy;
> +	const struct wiphy_radio *radio;
> +	int i;
> +
> +	*radio_idx = -1;
> +	if (!wiphy->n_radio)
> +		return true;
> +
> +	for (i = 0; i < wiphy->n_radio; i++) {
> +		radio = &wiphy->radio[i];
> +		if (!ieee80211_radio_freq_match(radio, chanreq))
> +			continue;
> +
> +		if (!ieee80211_can_create_new_chanctx(local, i))
> +			continue;
> +
> +		*radio_idx = i;
> +		return true;
> +	}
> +
> +	return false;
> +}

which would also get used here to find the radio first, though would
have to differentiate n_radio still, of course.

johannes

Re: [RFC v3 8/8] wifi: mac80211: add wiphy radio assignment and validation

[Felix Fietkau]

On 07.06.24 11:44, Johannes Berg wrote:
> On Thu, 2024-06-06 at 20:07 +0200, Felix Fietkau wrote:
>> 
>> +static bool
>> +ieee80211_radio_freq_match(const struct wiphy_radio *radio,
>> +			   const struct ieee80211_chan_req *chanreq)
>> +{
>> +	const struct wiphy_radio_freq_range *r;
>> +	u32 freq;
>> +	int i;
>> +
>> +	freq = ieee80211_channel_to_khz(chanreq->oper.chan);
>> +	for (i = 0; i < radio->n_freq_range; i++) {
>> +		r = &radio->freq_range[i];
>> +		if (freq >= r->start_freq && freq <= r->end_freq)
>> +			return true;
> 
> IMHO should be inclusive only on one side of the range. Can always make
> it work since channels are at least a few MHz apart, but the purist in
> me says it's easier to grok if the end is not inclusive :)

Sure, no problem.

> Maybe this should be a cfg80211 helper like
> 
> struct wiphy_radio *wiphy_get_radio(struct wiphy *wiphy, ... *chandef);

I didn't add such a helper, in case we get hardware where multiple 
radios support the same band. That's why ieee80211_find_available_radio 
loops over all radios until it finds one that matches both the freq 
range and the ifcomb constraints.

> which could also check that the _whole_ chandef fits (though that should
> probably also be handled elsewhere, like chandef_valid), and you can use
> it to get the radio pointer and then check for == below.
> 
> The point would be to have a single place with this kind of range logic.
> This is only going to get done by mac80211 now, but it'd really be
> awkward if some other driver had some other logic later.

I will move a variation of the freq range match helper to cfg80211.

>>  	if (!curr_ctx || (curr_ctx->replace_state ==
>>  			  IEEE80211_CHANCTX_WILL_BE_REPLACED) ||
>> @@ -1096,6 +1117,12 @@ ieee80211_replace_chanctx(struct ieee80211_local *local,
>>  			if (!list_empty(&ctx->reserved_links))
>>  				continue;
>>  
>> +			if (ctx->conf.radio_idx >= 0) {
>> +					radio = &wiphy->radio[ctx->conf.radio_idx];
>> +					if (!ieee80211_radio_freq_match(radio, chanreq))
>> +							continue;
>> +			}
> 
> something happened to indentation here :)

Will fix :)

>> +static bool
>> +ieee80211_find_available_radio(struct ieee80211_local *local,
>> +			       const struct ieee80211_chan_req *chanreq,
>> +			       int *radio_idx)
>> +{
>> +	struct wiphy *wiphy = local->hw.wiphy;
>> +	const struct wiphy_radio *radio;
>> +	int i;
>> +
>> +	*radio_idx = -1;
>> +	if (!wiphy->n_radio)
>> +		return true;
>> +
>> +	for (i = 0; i < wiphy->n_radio; i++) {
>> +		radio = &wiphy->radio[i];
>> +		if (!ieee80211_radio_freq_match(radio, chanreq))
>> +			continue;
>> +
>> +		if (!ieee80211_can_create_new_chanctx(local, i))
>> +			continue;
>> +
>> +		*radio_idx = i;
>> +		return true;
>> +	}
>> +
>> +	return false;
>> +}
> 
> which would also get used here to find the radio first, though would
> have to differentiate n_radio still, of course.

See above

- Felix

Re: [RFC v3 8/8] wifi: mac80211: add wiphy radio assignment and validation

[Johannes Berg]

On Fri, 2024-06-07 at 11:53 +0200, Felix Fietkau wrote:

> > struct wiphy_radio *wiphy_get_radio(struct wiphy *wiphy, ... *chandef);
> 
> I didn't add such a helper, in case we get hardware where multiple 
> radios support the same band. That's why ieee80211_find_available_radio 
> loops over all radios until it finds one that matches both the freq 
> range and the ifcomb constraints.

Ah, fair.

Thinking more about the "whole chandef" thing, I think I want to have a
check in cfg80211 somewhere that ensures you don't split up ranges that
could be used for a wider channel?

Say (for a stupid example) you have a device that (only) supports
channels 36-40:

 * 5180
 * 5200

but now you say it has two radios:

 * radio 1 ranges: 5170-5190
 * radio 2 ranges: 5190-5210

Now you can't use 40 MHz... but nothing will actually really prevent it.

Obviously this is a totally useless case, so I'd argue we should just
check during wiphy registration that you don't split the channel list in
this way with multiple radios?

Even on the potential Qualcomm 5 GHz low/mid/high split radios you'd
have gaps between the channels (e.g. no channel 80, no channel 148), so
it feels like you should always be able to split it in a way that the
radio range boundaries don't land between two adjacent channels in the
channel array.

Not sure how to implement such a check best, probably easiest to find
all non-adjacent channels first:

 
 - go over bands
   - ensure channels are sorted by increasing frequency
     (not sure we do that today? but every driver probably does)
   - find adjacent channels:
     - while more channels:
       - start_freq = current channel's freq - 10
       - end_freq = current channel's freq + 10
       - while current channel's freq == end_freq - 10:
         - go to next channel
       - check all radio's ranges cover this full or not at all
         (neither start nor end of a range falls into the calculated
          [start_freq, end_freq) interval)

or something like that?

(Also some docs on this I guess!)

johannes