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
