diff mbox series

wifi: cfg80211: skip regulatory checks when the channel is punctured

Message ID 20240826123341.5405-1-quic_kkavita@quicinc.com (mailing list archive)
State Changes Requested
Delegated to: Johannes Berg
Headers show
Series wifi: cfg80211: skip regulatory checks when the channel is punctured | expand

Commit Message

Kavita Kavita Aug. 26, 2024, 12:33 p.m. UTC 
The kernel performs several regulatory checks for AP mode in
nl80211/cfg80211. These checks include radar detection,
verification of whether the sub-channel is disabled, and
an examination to determine if the channel is a DFS channel
(both DFS usable and DFS available). These checks are
performed across a frequency range, examining each sub-channel.

However, these checks are also performed on frequencies that
have been punctured, which should not be examined as they are
not in use.

This leads to the issue where the AP stops because one of
the 20 MHz sub-channels is disabled or radar detected on
the channel, even when the sub-channel is punctured.

To address this issue, add a condition check wherever
regulatory checks exist for AP mode in nl80211/cfg80211.
This check identifies punctured channels and, upon finding
them, skips the regulatory checks for those channels.

Co-developed-by: Manaswini Paluri <quic_mpaluri@quicinc.com>
Signed-off-by: Manaswini Paluri <quic_mpaluri@quicinc.com>
Signed-off-by: Kavita Kavita <quic_kkavita@quicinc.com>
---
 net/wireless/chan.c | 59 ++++++++++++++++++++++++++++++++-------------
 1 file changed, 42 insertions(+), 17 deletions(-)

Comments

Kalle Valo Aug. 26, 2024, 7:05 p.m. UTC | #1 
Kavita Kavita <quic_kkavita@quicinc.com> writes:

> The kernel performs several regulatory checks for AP mode in
> nl80211/cfg80211. These checks include radar detection,
> verification of whether the sub-channel is disabled, and
> an examination to determine if the channel is a DFS channel
> (both DFS usable and DFS available). These checks are
> performed across a frequency range, examining each sub-channel.
>
> However, these checks are also performed on frequencies that
> have been punctured, which should not be examined as they are
> not in use.
>
> This leads to the issue where the AP stops because one of
> the 20 MHz sub-channels is disabled or radar detected on
> the channel, even when the sub-channel is punctured.
>
> To address this issue, add a condition check wherever
> regulatory checks exist for AP mode in nl80211/cfg80211.
> This check identifies punctured channels and, upon finding
> them, skips the regulatory checks for those channels.
>
> Co-developed-by: Manaswini Paluri <quic_mpaluri@quicinc.com>
> Signed-off-by: Manaswini Paluri <quic_mpaluri@quicinc.com>
> Signed-off-by: Kavita Kavita <quic_kkavita@quicinc.com>

Kavita, is your first and last name really the same? Just trying to
verify that s-o-b is correct.
Kavita Kavita Aug. 27, 2024, 9:28 a.m. UTC | #2 
On 8/27/2024 12:35 AM, Kalle Valo wrote:
> Kavita Kavita <quic_kkavita@quicinc.com> writes:
> 
>>
>> Co-developed-by: Manaswini Paluri <quic_mpaluri@quicinc.com>
>> Signed-off-by: Manaswini Paluri <quic_mpaluri@quicinc.com>
>> Signed-off-by: Kavita Kavita <quic_kkavita@quicinc.com>
> 
> Kavita, is your first and last name really the same? Just trying to
> verify that s-o-b is correct.
> 

Yes, My first name is same as last name.
Kalle Valo Aug. 27, 2024, 11:45 a.m. UTC | #3 
Kavita Kavita <quic_kkavita@quicinc.com> writes:

> On 8/27/2024 12:35 AM, Kalle Valo wrote:
>> Kavita Kavita <quic_kkavita@quicinc.com> writes:
>> 
>>>
>>> Co-developed-by: Manaswini Paluri <quic_mpaluri@quicinc.com>
>>> Signed-off-by: Manaswini Paluri <quic_mpaluri@quicinc.com>
>>> Signed-off-by: Kavita Kavita <quic_kkavita@quicinc.com>
>>
>> Kavita, is your first and last name really the same? Just trying to
>> verify that s-o-b is correct.
>
> Yes, My first name is same as last name.

Thanks for confirming, you have a cool name :)
Johannes Berg Aug. 28, 2024, 10:21 a.m. UTC | #4 
On Mon, 2024-08-26 at 18:03 +0530, Kavita Kavita wrote:
> The kernel performs several regulatory checks for AP mode in
> nl80211/cfg80211. These checks include radar detection,
> verification of whether the sub-channel is disabled, and
> an examination to determine if the channel is a DFS channel
> (both DFS usable and DFS available). These checks are
> performed across a frequency range, examining each sub-channel.
> 
> However, these checks are also performed on frequencies that
> have been punctured, which should not be examined as they are
> not in use.

Makes sense.

> This leads to the issue where the AP stops because one of
> the 20 MHz sub-channels is disabled or radar detected on
> the channel, even when the sub-channel is punctured.

I'm curious, how did that even happen? How did it detect radar on a
punctured channel in the first place?

Or are you saying it was detected before, but you say "the AP stops"
rather than "the AP fails to start"?

However, this possibly also points to something that's missing in this
patch and/or Aditya's patchset: if we do radar detection with a chandef
that's already punctured, we don't know that all the subchannels were
actually radar-free, and shouldn't mark them accordingly.

I think it'd make sense to incorporate that here as well, could you do
that?

> @@ -781,7 +784,7 @@ int cfg80211_chandef_dfs_required(struct wiphy *wiphy,
>  
>  		ret = cfg80211_get_chans_dfs_required(wiphy,
>  					MHZ_TO_KHZ(chandef->center_freq2),
> -					width, iftype);
> +					width, chandef->punctured, iftype);

This isn't really right: center_freq2 is for 80+80 which cannot use
puncturing, certainly cannot use puncturing in the secondary 80. It's
probably not strictly wrong either since 80+80 cannot be legal with
puncturing in the first place, but this really should just pass 0 I'd
think.

> @@ -868,7 +877,7 @@ bool cfg80211_chandef_dfs_usable(struct wiphy *wiphy,
>  		WARN_ON(!chandef->center_freq2);
>  		r2 = cfg80211_get_chans_dfs_usable(wiphy,
>  					MHZ_TO_KHZ(chandef->center_freq2),
> -					width);
> +					width, chandef->punctured);

same here

> @@ -1113,7 +1128,7 @@ static bool cfg80211_chandef_dfs_available(struct wiphy *wiphy,
>  		WARN_ON(!chandef->center_freq2);
>  		r = cfg80211_get_chans_dfs_available(wiphy,
>  					MHZ_TO_KHZ(chandef->center_freq2),
> -					width);
> +					width, chandef->punctured);

and here, obviously

> @@ -1139,6 +1155,12 @@ static unsigned int cfg80211_get_chans_dfs_cac_time(struct wiphy *wiphy,
>  		if (!c)
>  			return 0;
>  
> +		if (punctured & 1) {
> +			punctured >>= 1;
> +			continue;
> +		}
> +		punctured >>= 1;
> +
>  		if (c->flags & IEEE80211_CHAN_DISABLED)
>  			return 0;

We have this pattern a lot! I think perhaps we should add a kind of
for_each_subchannel() macro?

Perhaps even iterate subchannels of a chandef including center_freq2,
though I'm not sure how we'd arrange that...

Something like cfg80211_wdev_on_sub_chan() also seems to need to take
puncturing into account and could be rewritten with such a helper.

#define for_each_subchannel(chandef, subchan)
  for (subchan = ieee80211_next_subchan(chandef, NULL);
       subchan;
       subchan = ieee80211_next_subchan(chandef, subchan))

or so, with ieee80211_next_subchan() containing some necessary iteration
logic?

johannes
Kavita Kavita Nov. 25, 2024, 5:21 a.m. UTC | #5 
On 8/28/2024 3:51 PM, Johannes Berg wrote:
> On Mon, 2024-08-26 at 18:03 +0530, Kavita Kavita wrote:
>> The kernel performs several regulatory checks for AP mode in
>> nl80211/cfg80211. These checks include radar detection,
>> verification of whether the sub-channel is disabled, and
>> an examination to determine if the channel is a DFS channel
>> (both DFS usable and DFS available). These checks are
>> performed across a frequency range, examining each sub-channel.
>>
>> However, these checks are also performed on frequencies that
>> have been punctured, which should not be examined as they are
>> not in use.
> 
> Makes sense.
> 
>> This leads to the issue where the AP stops because one of
>> the 20 MHz sub-channels is disabled or radar detected on
>> the channel, even when the sub-channel is punctured.
> 
> I'm curious, how did that even happen? How did it detect radar on a
> punctured channel in the first place?
> 
> Or are you saying it was detected before, but you say "the AP stops"
> rather than "the AP fails to start"?
> 

One of the use cases we are targeting here is after AP start, if radar 
is detected, SME offload drivers can indicate a channel switch with the 
radar-detected channel as punctured instead of switching to a new 
channel. But when the driver does this, currently the kernel stops the 
AP even after the driver punctures that channel.

Also, for the AP start case mentioned, the AP shouldn't stop or fail to 
start if the radar-detected channel is punctured in the START_AP command.

> However, this possibly also points to something that's missing in this
> patch and/or Aditya's patchset: if we do radar detection with a chandef
> that's already punctured, we don't know that all the subchannels were
> actually radar-free, and shouldn't mark them accordingly.
> 
> I think it'd make sense to incorporate that here as well, could you do
> that?
> 


Thanks, I added some additional changes in the v2 patch to handle this.

>> @@ -781,7 +784,7 @@ int cfg80211_chandef_dfs_required(struct wiphy *wiphy,
>>   
>>   		ret = cfg80211_get_chans_dfs_required(wiphy,
>>   					MHZ_TO_KHZ(chandef->center_freq2),
>> -					width, iftype);
>> +					width, chandef->punctured, iftype);
> 
> This isn't really right: center_freq2 is for 80+80 which cannot use
> puncturing, certainly cannot use puncturing in the secondary 80. It's
> probably not strictly wrong either since 80+80 cannot be legal with
> puncturing in the first place, but this really should just pass 0 I'd
> think.
> 

Thanks, I fixed this in the v2 patch.

>> @@ -868,7 +877,7 @@ bool cfg80211_chandef_dfs_usable(struct wiphy *wiphy,
>>   		WARN_ON(!chandef->center_freq2);
>>   		r2 = cfg80211_get_chans_dfs_usable(wiphy,
>>   					MHZ_TO_KHZ(chandef->center_freq2),
>> -					width);
>> +					width, chandef->punctured);
> 
> same here
> 
>> @@ -1113,7 +1128,7 @@ static bool cfg80211_chandef_dfs_available(struct wiphy *wiphy,
>>   		WARN_ON(!chandef->center_freq2);
>>   		r = cfg80211_get_chans_dfs_available(wiphy,
>>   					MHZ_TO_KHZ(chandef->center_freq2),
>> -					width);
>> +					width, chandef->punctured);
> 
> and here, obviously
> 
>> @@ -1139,6 +1155,12 @@ static unsigned int cfg80211_get_chans_dfs_cac_time(struct wiphy *wiphy,
>>   		if (!c)
>>   			return 0;
>>   
>> +		if (punctured & 1) {
>> +			punctured >>= 1;
>> +			continue;
>> +		}
>> +		punctured >>= 1;
>> +
>>   		if (c->flags & IEEE80211_CHAN_DISABLED)
>>   			return 0;
> 
> We have this pattern a lot! I think perhaps we should add a kind of
> for_each_subchannel() macro?
> 
> Perhaps even iterate subchannels of a chandef including center_freq2,
> though I'm not sure how we'd arrange that...
> 
> Something like cfg80211_wdev_on_sub_chan() also seems to need to take
> puncturing into account and could be rewritten with such a helper.
> 
> #define for_each_subchannel(chandef, subchan)
>    for (subchan = ieee80211_next_subchan(chandef, NULL);
>         subchan;
>         subchan = ieee80211_next_subchan(chandef, subchan))
> 
> or so, with ieee80211_next_subchan() containing some necessary iteration
> logic?
> 

Added macro for_each_subchan() in v2 patch.

I tried to handle traversing both center_freq1 and center_freq2 with 
for_each_subchannel(chandef, subchan) as suggested by you, but it's 
complicated to handle both center_freq1 and center_freq2 cases together 
due to their different handling requirements.

> johannes
diff mbox series

Patch

diff --git a/net/wireless/chan.c b/net/wireless/chan.c
index e579d7e1425f..b363649acd03 100644
--- a/net/wireless/chan.c
+++ b/net/wireless/chan.c
@@ -727,6 +727,7 @@  static bool cfg80211_dfs_permissive_chan(struct wiphy *wiphy,
 static int cfg80211_get_chans_dfs_required(struct wiphy *wiphy,
 					    u32 center_freq,
 					    u32 bandwidth,
+					    u16 punctured,
 					    enum nl80211_iftype iftype)
 {
 	struct ieee80211_channel *c;
@@ -740,9 +741,11 @@  static int cfg80211_get_chans_dfs_required(struct wiphy *wiphy,
 		if (!c)
 			return -EINVAL;
 
-		if (c->flags & IEEE80211_CHAN_RADAR &&
+		if (!(punctured & 1) && (c->flags & IEEE80211_CHAN_RADAR) &&
 		    !cfg80211_dfs_permissive_chan(wiphy, iftype, c))
 			return 1;
+
+		punctured >>= 1;
 	}
 
 	return 0;
@@ -770,7 +773,7 @@  int cfg80211_chandef_dfs_required(struct wiphy *wiphy,
 
 		ret = cfg80211_get_chans_dfs_required(wiphy,
 					ieee80211_chandef_to_khz(chandef),
-					width, iftype);
+					width, chandef->punctured, iftype);
 		if (ret < 0)
 			return ret;
 		else if (ret > 0)
@@ -781,7 +784,7 @@  int cfg80211_chandef_dfs_required(struct wiphy *wiphy,
 
 		ret = cfg80211_get_chans_dfs_required(wiphy,
 					MHZ_TO_KHZ(chandef->center_freq2),
-					width, iftype);
+					width, chandef->punctured, iftype);
 		if (ret < 0)
 			return ret;
 		else if (ret > 0)
@@ -808,7 +811,7 @@  EXPORT_SYMBOL(cfg80211_chandef_dfs_required);
 
 static int cfg80211_get_chans_dfs_usable(struct wiphy *wiphy,
 					 u32 center_freq,
-					 u32 bandwidth)
+					 u32 bandwidth, u16 punctured)
 {
 	struct ieee80211_channel *c;
 	u32 freq, start_freq, end_freq;
@@ -828,6 +831,12 @@  static int cfg80211_get_chans_dfs_usable(struct wiphy *wiphy,
 		if (!c)
 			return -EINVAL;
 
+		if (punctured & 1) {
+			punctured >>= 1;
+			continue;
+		}
+		punctured >>= 1;
+
 		if (c->flags & IEEE80211_CHAN_DISABLED)
 			return -EINVAL;
 
@@ -858,7 +867,7 @@  bool cfg80211_chandef_dfs_usable(struct wiphy *wiphy,
 
 	r1 = cfg80211_get_chans_dfs_usable(wiphy,
 					   MHZ_TO_KHZ(chandef->center_freq1),
-					   width);
+					   width, chandef->punctured);
 
 	if (r1 < 0)
 		return false;
@@ -868,7 +877,7 @@  bool cfg80211_chandef_dfs_usable(struct wiphy *wiphy,
 		WARN_ON(!chandef->center_freq2);
 		r2 = cfg80211_get_chans_dfs_usable(wiphy,
 					MHZ_TO_KHZ(chandef->center_freq2),
-					width);
+					width, chandef->punctured);
 		if (r2 < 0)
 			return false;
 		break;
@@ -1053,7 +1062,7 @@  bool cfg80211_any_wiphy_oper_chan(struct wiphy *wiphy,
 
 static bool cfg80211_get_chans_dfs_available(struct wiphy *wiphy,
 					     u32 center_freq,
-					     u32 bandwidth)
+					     u32 bandwidth, u16 punctured)
 {
 	struct ieee80211_channel *c;
 	u32 freq, start_freq, end_freq;
@@ -1075,6 +1084,12 @@  static bool cfg80211_get_chans_dfs_available(struct wiphy *wiphy,
 		if (!c)
 			return false;
 
+		if (punctured & 1) {
+			punctured >>= 1;
+			continue;
+		}
+		punctured >>= 1;
+
 		if (c->flags & IEEE80211_CHAN_DISABLED)
 			return false;
 
@@ -1102,7 +1117,7 @@  static bool cfg80211_chandef_dfs_available(struct wiphy *wiphy,
 
 	r = cfg80211_get_chans_dfs_available(wiphy,
 					     MHZ_TO_KHZ(chandef->center_freq1),
-					     width);
+					     width, chandef->punctured);
 
 	/* If any of channels unavailable for cf1 just return */
 	if (!r)
@@ -1113,7 +1128,7 @@  static bool cfg80211_chandef_dfs_available(struct wiphy *wiphy,
 		WARN_ON(!chandef->center_freq2);
 		r = cfg80211_get_chans_dfs_available(wiphy,
 					MHZ_TO_KHZ(chandef->center_freq2),
-					width);
+					width, chandef->punctured);
 		break;
 	default:
 		WARN_ON(chandef->center_freq2);
@@ -1125,7 +1140,8 @@  static bool cfg80211_chandef_dfs_available(struct wiphy *wiphy,
 
 static unsigned int cfg80211_get_chans_dfs_cac_time(struct wiphy *wiphy,
 						    u32 center_freq,
-						    u32 bandwidth)
+						    u32 bandwidth,
+						    u16 punctured)
 {
 	struct ieee80211_channel *c;
 	u32 start_freq, end_freq, freq;
@@ -1139,6 +1155,12 @@  static unsigned int cfg80211_get_chans_dfs_cac_time(struct wiphy *wiphy,
 		if (!c)
 			return 0;
 
+		if (punctured & 1) {
+			punctured >>= 1;
+			continue;
+		}
+		punctured >>= 1;
+
 		if (c->flags & IEEE80211_CHAN_DISABLED)
 			return 0;
 
@@ -1168,14 +1190,14 @@  cfg80211_chandef_dfs_cac_time(struct wiphy *wiphy,
 
 	t1 = cfg80211_get_chans_dfs_cac_time(wiphy,
 					     MHZ_TO_KHZ(chandef->center_freq1),
-					     width);
+					     width, chandef->punctured);
 
 	if (!chandef->center_freq2)
 		return t1;
 
 	t2 = cfg80211_get_chans_dfs_cac_time(wiphy,
 					     MHZ_TO_KHZ(chandef->center_freq2),
-					     width);
+					     width, chandef->punctured);
 
 	return max(t1, t2);
 }
@@ -1183,6 +1205,7 @@  EXPORT_SYMBOL(cfg80211_chandef_dfs_cac_time);
 
 static bool cfg80211_secondary_chans_ok(struct wiphy *wiphy,
 					u32 center_freq, u32 bandwidth,
+					u16 punctured,
 					u32 prohibited_flags,
 					u32 permitting_flags)
 {
@@ -1198,8 +1221,10 @@  static bool cfg80211_secondary_chans_ok(struct wiphy *wiphy,
 			return false;
 		if (c->flags & permitting_flags)
 			continue;
-		if (c->flags & prohibited_flags)
+		if ((c->flags & prohibited_flags) && !(punctured & 1))
 			return false;
+
+		punctured >>= 1;
 	}
 
 	return true;
@@ -1423,16 +1448,16 @@  bool _cfg80211_chandef_usable(struct wiphy *wiphy,
 
 	if (!cfg80211_secondary_chans_ok(wiphy,
 					 ieee80211_chandef_to_khz(chandef),
-					 width, prohibited_flags,
-					 permitting_flags))
+					 width, chandef->punctured,
+					 prohibited_flags, permitting_flags))
 		return false;
 
 	if (!chandef->center_freq2)
 		return true;
 	return cfg80211_secondary_chans_ok(wiphy,
 					   MHZ_TO_KHZ(chandef->center_freq2),
-					   width, prohibited_flags,
-					   permitting_flags);
+					   width, chandef->punctured,
+					   prohibited_flags, permitting_flags);
 }
 
 bool cfg80211_chandef_usable(struct wiphy *wiphy,