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
