[PATCH v3] wifi: cfg80211: check radio iface combination for multi radio per wiphy

[PATCH v3] wifi: cfg80211: check radio iface combination for multi radio per wiphy

[Karthikeyan Periyasamy]

Currently, wiphy_verify_combinations() fails for the multi-radio per wiphy
due to the condition check on global interface combination that DFS only
works on one channel. In a multi-radio scenario, global interface
combination encompasses the capabilities of all radio combinations, so it
supports more than one channel with DFS. For multi-radio per wiphy,
interface combination verification needs to be performed for radio specific
interface combinations. This is necessary as the global interface
combination combines the capabilities of all radio combinations.

Fixes: a01b1e9f9955 ("wifi: mac80211: add support for DFS with multiple radios")
Signed-off-by: Karthikeyan Periyasamy <quic_periyasa@quicinc.com>
---
 v3:
  - Add validation check for global combination where all radios combined
 v2:
  - Rebased to ToT

 net/wireless/core.c | 49 ++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 42 insertions(+), 7 deletions(-)

diff --git a/net/wireless/core.c b/net/wireless/core.c
index 4d5d351bd0b5..45b00b0322dc 100644
--- a/net/wireless/core.c
+++ b/net/wireless/core.c
@@ -603,16 +603,20 @@ struct wiphy *wiphy_new_nm(const struct cfg80211_ops *ops, int sizeof_priv,
 }
 EXPORT_SYMBOL(wiphy_new_nm);
 
-static int wiphy_verify_combinations(struct wiphy *wiphy)
+static
+int wiphy_verify_iface_combinations(struct wiphy *wiphy,
+				    const struct ieee80211_iface_combination *iface_comb,
+				    int n_iface_comb,
+				    bool combined_radio)
 {
 	const struct ieee80211_iface_combination *c;
 	int i, j;
 
-	for (i = 0; i < wiphy->n_iface_combinations; i++) {
+	for (i = 0; i < n_iface_comb; i++) {
 		u32 cnt = 0;
 		u16 all_iftypes = 0;
 
-		c = &wiphy->iface_combinations[i];
+		c = &iface_comb[i];
 
 		/*
 		 * Combinations with just one interface aren't real,
@@ -626,8 +630,9 @@ static int wiphy_verify_combinations(struct wiphy *wiphy)
 			return -EINVAL;
 
 		/* DFS only works on one channel. */
-		if (WARN_ON(c->radar_detect_widths &&
-			    (c->num_different_channels > 1)))
+		if (!combined_radio &&
+		    WARN_ON(c->radar_detect_widths &&
+			    c->num_different_channels > 1))
 			return -EINVAL;
 
 		if (WARN_ON(!c->n_limits))
@@ -649,12 +654,14 @@ static int wiphy_verify_combinations(struct wiphy *wiphy)
 				return -EINVAL;
 
 			/* Only a single P2P_DEVICE can be allowed */
-			if (WARN_ON(types & BIT(NL80211_IFTYPE_P2P_DEVICE) &&
+			if (!combined_radio &&
+			    WARN_ON(types & BIT(NL80211_IFTYPE_P2P_DEVICE) &&
 				    c->limits[j].max > 1))
 				return -EINVAL;
 
 			/* Only a single NAN can be allowed */
-			if (WARN_ON(types & BIT(NL80211_IFTYPE_NAN) &&
+			if (!combined_radio &&
+			    WARN_ON(types & BIT(NL80211_IFTYPE_NAN) &&
 				    c->limits[j].max > 1))
 				return -EINVAL;
 
@@ -693,6 +700,34 @@ static int wiphy_verify_combinations(struct wiphy *wiphy)
 	return 0;
 }
 
+static int wiphy_verify_combinations(struct wiphy *wiphy)
+{
+	int i, ret;
+	bool combined_radio = false;
+
+	if (wiphy->n_radio) {
+		for (i = 0; i < wiphy->n_radio; i++) {
+			const struct wiphy_radio *radio = &wiphy->radio[i];
+
+			ret = wiphy_verify_iface_combinations(wiphy,
+							      radio->iface_combinations,
+							      radio->n_iface_combinations,
+							      false);
+			if (ret)
+				return ret;
+		}
+
+		combined_radio = true;
+	}
+
+	ret = wiphy_verify_iface_combinations(wiphy,
+					      wiphy->iface_combinations,
+					      wiphy->n_iface_combinations,
+					      combined_radio);
+
+	return ret;
+}
+
 int wiphy_register(struct wiphy *wiphy)
 {
 	struct cfg80211_registered_device *rdev = wiphy_to_rdev(wiphy);
-- 
2.34.1

Re: [PATCH v3] wifi: cfg80211: check radio iface combination for multi radio per wiphy

[Johannes Berg]

On Wed, 2024-09-04 at 16:59 +0530, Karthikeyan Periyasamy wrote:
> Currently, wiphy_verify_combinations() fails for the multi-radio per wiphy
> due to the condition check on global interface combination that DFS only
> works on one channel.

As it should, really.

> In a multi-radio scenario, global interface
> combination encompasses the capabilities of all radio combinations, so it
> supports more than one channel with DFS.

No, that's not correct, it doesn't.

> For multi-radio per wiphy,
> interface combination verification needs to be performed for radio specific
> interface combinations.

Agree, but it shouldn't make an exception for DFS.


> This is necessary as the global interface
> combination combines the capabilities of all radio combinations.

No, it doesn't.

johannes

Re: [PATCH v3] wifi: cfg80211: check radio iface combination for multi radio per wiphy

[Karthikeyan Periyasamy]

On 9/11/2024 3:12 PM, Johannes Berg wrote:
> On Wed, 2024-09-04 at 16:59 +0530, Karthikeyan Periyasamy wrote:
>> Currently, wiphy_verify_combinations() fails for the multi-radio per wiphy
>> due to the condition check on global interface combination that DFS only
>> works on one channel.
> 
> As it should, really.
> 
>> In a multi-radio scenario, global interface
>> combination encompasses the capabilities of all radio combinations, so it
>> supports more than one channel with DFS.
> 
> No, that's not correct, it doesn't.

But the attribute comment section clearly says the Global iface 
combination encompass all radio combination as below

* @NL80211_ATTR_WIPHY_INTERFACE_COMBINATIONS: Nested attribute listing * 
the
*      supported interface combinations for all radios combined. In each
*      nested item, it contains attributes defined in
*      &enum nl80211_if_combination_attrs.

Is my understanding incorrect ?

> 
>> For multi-radio per wiphy,
>> interface combination verification needs to be performed for radio specific
>> interface combinations.
> 
> Agree, but it shouldn't make an exception for DFS.
> 
> 
>> This is necessary as the global interface
>> combination combines the capabilities of all radio combinations.
> 
> No, it doesn't.


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

Re: [PATCH v3] wifi: cfg80211: check radio iface combination for multi radio per wiphy

[Johannes Berg]

On Wed, 2024-09-11 at 18:21 +0530, Karthikeyan Periyasamy wrote:
> 
> On 9/11/2024 3:12 PM, Johannes Berg wrote:
> > On Wed, 2024-09-04 at 16:59 +0530, Karthikeyan Periyasamy wrote:
> > > Currently, wiphy_verify_combinations() fails for the multi-radio per wiphy
> > > due to the condition check on global interface combination that DFS only
> > > works on one channel.
> > 
> > As it should, really.
> > 
> > > In a multi-radio scenario, global interface
> > > combination encompasses the capabilities of all radio combinations, so it
> > > supports more than one channel with DFS.
> > 
> > No, that's not correct, it doesn't.
> 
> But the attribute comment section clearly says the Global iface 
> combination encompass all radio combination as below
> 
> * @NL80211_ATTR_WIPHY_INTERFACE_COMBINATIONS: Nested attribute listing * 
> the
> *      supported interface combinations for all radios combined. In each
> *      nested item, it contains attributes defined in
> *      &enum nl80211_if_combination_attrs.
> 
> Is my understanding incorrect ?

I guess it depends on how you interpret "combined". It must be something
that can actually be done *regardless* of radio assignment, to be
compatible with older userspace.

So if you think "combined" == "superset of all radios" then your
understanding is incorrect. You need to think "combined" == "what the
device can do without caring about radio assignment".

johannes

Re: [PATCH v3] wifi: cfg80211: check radio iface combination for multi radio per wiphy

[Karthikeyan Periyasamy]

On 9/11/2024 6:22 PM, Johannes Berg wrote:
> On Wed, 2024-09-11 at 18:21 +0530, Karthikeyan Periyasamy wrote:
>>
>> On 9/11/2024 3:12 PM, Johannes Berg wrote:
>>> On Wed, 2024-09-04 at 16:59 +0530, Karthikeyan Periyasamy wrote:
>>>> Currently, wiphy_verify_combinations() fails for the multi-radio per wiphy
>>>> due to the condition check on global interface combination that DFS only
>>>> works on one channel.
>>>
>>> As it should, really.
>>>
>>>> In a multi-radio scenario, global interface
>>>> combination encompasses the capabilities of all radio combinations, so it
>>>> supports more than one channel with DFS.
>>>
>>> No, that's not correct, it doesn't.
>>
>> But the attribute comment section clearly says the Global iface
>> combination encompass all radio combination as below
>>
>> * @NL80211_ATTR_WIPHY_INTERFACE_COMBINATIONS: Nested attribute listing *
>> the
>> *      supported interface combinations for all radios combined. In each
>> *      nested item, it contains attributes defined in
>> *      &enum nl80211_if_combination_attrs.
>>
>> Is my understanding incorrect ?
> 
> I guess it depends on how you interpret "combined". It must be something
> that can actually be done *regardless* of radio assignment, to be
> compatible with older userspace.
> 
> So if you think "combined" == "superset of all radios" then your
> understanding is incorrect. You need to think "combined" == "what the
> device can do without caring about radio assignment".
> 
The current implementation of radio specific advertisement global iface 
combination (NL80211_ATTR_WIPHY_INTERFACE_COMBINATIONS) expects the 
superset of all radios, wherever the radio idx is -1 from caller of 
cfg80211_iter_combinations().

Is it incorrect ?

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

Re: [PATCH v3] wifi: cfg80211: check radio iface combination for multi radio per wiphy

[Johannes Berg]

On Wed, 2024-09-11 at 18:45 +0530, Karthikeyan Periyasamy wrote:
> > I guess it depends on how you interpret "combined". It must be something
> > that can actually be done *regardless* of radio assignment, to be
> > compatible with older userspace.
> > 
> > So if you think "combined" == "superset of all radios" then your
> > understanding is incorrect. You need to think "combined" == "what the
> > device can do without caring about radio assignment".
> > 
> The current implementation of radio specific advertisement global iface 
> combination (NL80211_ATTR_WIPHY_INTERFACE_COMBINATIONS) expects the 
> superset of all radios, wherever the radio idx is -1 from caller of 
> cfg80211_iter_combinations().

How so?

johannes

Re: [PATCH v3] wifi: cfg80211: check radio iface combination for multi radio per wiphy

[Karthikeyan Periyasamy]

On 9/11/2024 7:10 PM, Johannes Berg wrote:
> On Wed, 2024-09-11 at 18:45 +0530, Karthikeyan Periyasamy wrote:
>>> I guess it depends on how you interpret "combined". It must be something
>>> that can actually be done *regardless* of radio assignment, to be
>>> compatible with older userspace.
>>>
>>> So if you think "combined" == "superset of all radios" then your
>>> understanding is incorrect. You need to think "combined" == "what the
>>> device can do without caring about radio assignment".
>>>
>> The current implementation of radio specific advertisement global iface
>> combination (NL80211_ATTR_WIPHY_INTERFACE_COMBINATIONS) expects the
>> superset of all radios, wherever the radio idx is -1 from caller of
>> cfg80211_iter_combinations().
> 
> How so?
> 

ieee80211_link_reserve_chanctx() calls 
ieee80211_can_create_new_chanctx() with radio_idx (-1) to calculate the 
max channel (ieee80211_max_num_channels) after the iface combination 
check (cfg80211_iter_combinations) passed for the global iface 
combination. Here the expectation is number of channel context is less 
than the number of different channel. So in multi-radio advertisement, 
each radio support atleast one channel, so totally multiple different 
channels advertised in the global iface combination to pass this 
ieee80211_max_num_channels().

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

Re: [PATCH v3] wifi: cfg80211: check radio iface combination for multi radio per wiphy

[Johannes Berg]

On Wed, 2024-09-11 at 20:51 +0530, Karthikeyan Periyasamy wrote:
> 
> ieee80211_link_reserve_chanctx() calls 
> ieee80211_can_create_new_chanctx() with radio_idx (-1) to calculate the 
> max channel (ieee80211_max_num_channels) after the iface combination 
> check (cfg80211_iter_combinations) passed for the global iface 
> combination. Here the expectation is number of channel context is less 
> than the number of different channel. So in multi-radio advertisement, 
> each radio support atleast one channel, so totally multiple different 
> channels advertised in the global iface combination to pass this 
> ieee80211_max_num_channels().

So maybe that's broken then, I dunno. You should figure it out with
Felix I guess.

The intent was, and clearly it has to be, that the global combinations
are something that can be handled regardless of radio information, to be
backward compatible with existing uses. Therefore, it cannot be
something where you say two channels and radar detection on both because
that would imply being able to use channels 36 and 40 with 20 MHz at the
same time with radar detection, which isn't actually possible.
In this case, the "two channels" is only possible with also two radios,
which has to rely on the per-radio advertisement, and the global one has
to be just one channel for radar detection.

We still need the part of the patch that calls the validation on each
radio, but it shouldn't be different from the global one. If you could
make that patch I'd appreciate it.

johannes

Re: [PATCH v3] wifi: cfg80211: check radio iface combination for multi radio per wiphy

[Felix Fietkau]

On 11.09.24 17:25, Johannes Berg wrote:
> On Wed, 2024-09-11 at 20:51 +0530, Karthikeyan Periyasamy wrote:
>> 
>> ieee80211_link_reserve_chanctx() calls 
>> ieee80211_can_create_new_chanctx() with radio_idx (-1) to calculate the 
>> max channel (ieee80211_max_num_channels) after the iface combination 
>> check (cfg80211_iter_combinations) passed for the global iface 
>> combination. Here the expectation is number of channel context is less 
>> than the number of different channel. So in multi-radio advertisement, 
>> each radio support atleast one channel, so totally multiple different 
>> channels advertised in the global iface combination to pass this 
>> ieee80211_max_num_channels().
> 
> So maybe that's broken then, I dunno. You should figure it out with
> Felix I guess.
> 
> The intent was, and clearly it has to be, that the global combinations
> are something that can be handled regardless of radio information, to be
> backward compatible with existing uses. Therefore, it cannot be
> something where you say two channels and radar detection on both because
> that would imply being able to use channels 36 and 40 with 20 MHz at the
> same time with radar detection, which isn't actually possible.
> In this case, the "two channels" is only possible with also two radios,
> which has to rely on the per-radio advertisement, and the global one has
> to be just one channel for radar detection.
> 
> We still need the part of the patch that calls the validation on each
> radio, but it shouldn't be different from the global one. If you could
> make that patch I'd appreciate it.

It's implemented the way we discussed it back when this topic came up. 
Legacy user space gets the ifcomb data from the first radio for 
compatibility. The actual global ifcomb data is reported in a new attribute.

- Felix

Re: [PATCH v3] wifi: cfg80211: check radio iface combination for multi radio per wiphy

[Johannes Berg]

On Wed, 2024-09-11 at 17:58 +0200, Felix Fietkau wrote:
> > 
> > We still need the part of the patch that calls the validation on each
> > radio, but it shouldn't be different from the global one. If you could
> > make that patch I'd appreciate it.
> 
> It's implemented the way we discussed it back when this topic came up. 
> Legacy user space gets the ifcomb data from the first radio for 
> compatibility. The actual global ifcomb data is reported in a new attribute.
> 

Hah, I forgot about that. Then that means we _do_ need to relax the
validation in this case, but I guess needs to come with a big comment
saying how the driver-global != nl80211 global ...

Sorry!

johannes

Re: [PATCH v3] wifi: cfg80211: check radio iface combination for multi radio per wiphy

[Karthikeyan Periyasamy]

On 9/11/2024 8:55 PM, Johannes Berg wrote:
> On Wed, 2024-09-11 at 20:51 +0530, Karthikeyan Periyasamy wrote:
>>
>> ieee80211_link_reserve_chanctx() calls
>> ieee80211_can_create_new_chanctx() with radio_idx (-1) to calculate the
>> max channel (ieee80211_max_num_channels) after the iface combination
>> check (cfg80211_iter_combinations) passed for the global iface
>> combination. Here the expectation is number of channel context is less
>> than the number of different channel. So in multi-radio advertisement,
>> each radio support atleast one channel, so totally multiple different
>> channels advertised in the global iface combination to pass this
>> ieee80211_max_num_channels().
> 
> So maybe that's broken then, I dunno. You should figure it out with
> Felix I guess.
> 
> The intent was, and clearly it has to be, that the global combinations
> are something that can be handled regardless of radio information, to be
> backward compatible with existing uses. Therefore, it cannot be
> something where you say two channels and radar detection on both because
> that would imply being able to use channels 36 and 40 with 20 MHz at the
> same time with radar detection, which isn't actually possible.
> In this case, the "two channels" is only possible with also two radios,
> which has to rely on the per-radio advertisement, and the global one has
> to be just one channel for radar detection.
> 
> We still need the part of the patch that calls the validation on each
> radio, but it shouldn't be different from the global one. If you could
> make that patch I'd appreciate it.
> 

@Felix any thought on the current implementation ?

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