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?
Because there are two possibilities? Default client and also connect to
Low Power Indoor AP as well as sub-ordinate client. So to let the kernel
know which mode originally the client is in, the command was introduced.
I do understand the concern here about possible misuse for the command
from the user space, I would re-visit this area and try to cover the
loop holes if any. But don't you think it should be the case? Basically
same like how we tell via user space the SSID, keys/suite info. freq
list and all for a client, in a similar way tell 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.
Sure. I had the same thought but unfortunate in few patches, named the
title wrongly. My bad, sorry for that. I will correct 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?
Sure, let me work on this 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 will check that out too.
