On 8/29/23 23:20, Johannes Berg wrote:
Hi,
So ... yeah. I shied away from even reviewing this because it's such a
messy topic and we've had so many threads about this. Sorry about that.
On Wed, 2023-03-15 at 18:58 +0530, Aditya Kumar Singh wrote:
6 GHz introduces various power modes of operation. Currently, based
on the power mode, channel's Power Spectral Density (PSD) value,
Regulatory power value, as well as channel disabled flag can change.
For single interface, current implementation works just fine. But for
multi-interfaces, for example AP-STA concurrency, two different channel
context needs to be maintained. This is because, STA power mode also
depends on the AP's power mode it is going to associate with. Hence,
PSD value, regulatory power value and channel disabled flag might vary.
In this case, same channel context cannot be used for both AP and STA.
Therefore, to support multiple channel space for each power mode, the
6 GHz channels needs a separate storage space in data structure
ieee80211_supported_band. Because of this, the existing APIs to get the
channel/frequency from frequency/channel will not work for 6 GHz band.
Add support to store all possible 6 GHz channel pools according to the
power mode as well as add API support for getting channel/frequency info
from the new struct ieee80211_6ghz_channel.
Why different channel pools and not array of varying member in the same channel?:
I really don't understand.
Why do you even need to set *from userspace* the power mode for a
client? That ... doesn't make that much sense?
Cause client also has two power modes? Default client can also connect
and sub-ordinate client too connect to all the AP types. So user space
would tell what is the power mode.
I mean, you're explaining here the mechanics, which is all fine, but
what's the "theory of operation"? Yes, I get that 6 GHz spectrum use
introduced power modes, but why do we even need to handle it this way?
None of this or the patches actually introduce any rationale?
Also, I'll note that the patch subjects with "cfg80211" or "mac80211"
are _really_ unclear. Sometimes you have "cfg80211" patches that mostly
change mac80211, etc. It'd make reviewing easier if you actually did
limit cfg80211 patches to touching cfg80211 only (with the exception of
necessary API updates, of course), and mac80211 patches to implementing
the new cfg80211 APIs. The first patch is probably neither, you can mark
it as ieee80211.
Yeah, I too have the same thought but unfortunately by mistake named it
wrong. Sorry! Will rectify it in next version.
And then trivial things like - don't introduce a bug only to fix it in
the next patch! I mean, really?
I _still_ don't like "(A)" operation with different channel pointers -
you talk about introducing bugs with (B) but at least those would be
bugs in the new parts; (A) is almost certainly introducing bugs in
existing code that compares pointers and you never even know about it.
Got it.
Also, related to that, is kind of a fundamental question - should we
really think that two *channels* are different because you use different
(TX) power control/scheme on them? That seems a bit strange, and you've
not gotten into that at all, from a semantic POV, only from a code POV.
I actually currently don't think that makes sense, but convince me
otherwise?
Hmm... Sure, let me think again and get back.
For a chandef, _maybe_? But also see the discussion around puncturing,
I'm not sure that's actually the right solution either.
Okay.
johannes
