| Message ID | 20230315132904.31779-1-quic_adisi@quicinc.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | wifi: cfg80211/mac80211: extend 6 GHz support for all power modes | expand |
On 3/15/2023 18:58, 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. > > [..] > Hi Johannes, Could you review this series and provide your comments please? Aditya
Hi Johannes, Could you give comments for these v3 patches? On 3/15/2023 9:28 PM, Aditya Kumar Singh wrote: ...
Hi Johannes, Kindly Reminder. Could you give comments for these v3 patches? On 6/9/2023 1:41 PM, Wen Gong wrote: > Hi Johannes, > > Could you give comments for these v3 patches? > > On 3/15/2023 9:28 PM, Aditya Kumar Singh wrote: > ...
Wen Gong <quic_wgong@quicinc.com> writes: > Hi Johannes, > > Kindly Reminder. Could you give comments for these v3 patches? > > On 6/9/2023 1:41 PM, Wen Gong wrote: >> Hi Johannes, >> >> Could you give comments for these v3 patches? Please stop spamming the lists like this, it's simply rude. And check our summer schedule: https://lore.kernel.org/all/87y1kncuh4.fsf@kernel.org/
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? 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. 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. 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? For a chandef, _maybe_? But also see the discussion around puncturing, I'm not sure that's actually the right solution either. johannes
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.
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
On Wed, 2023-08-30 at 10:43 +0530, Aditya Kumar Singh wrote: > > > > Why do you even need to set *from userspace* the power mode for a > > client? That ... doesn't make that much sense? Oh so you addressed that here, sorry. > 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 just don't understand how userspace would possibly know what to do. You can't really expect the _user_ to select this. So how does wpa_supplicant know what to do? How does it know better than the driver? Where does it get the information from? johannes
On 8/30/23 13:14, Johannes Berg wrote: > On Wed, 2023-08-30 at 10:43 +0530, Aditya Kumar Singh wrote: >>> >>> Why do you even need to set *from userspace* the power mode for a >>> client? That ... doesn't make that much sense? > > Oh so you addressed that here, sorry. > >> 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 just don't understand how userspace would possibly know what to do. > You can't really expect the _user_ to select this. So how does > wpa_supplicant know what to do? How does it know better than the driver? > Where does it get the information from? > The power mode selection for client purely depends on how much tx power the user wants for the client. In short, subordinate mode type client can operate on more tx-power when compared to Default mode type client. For example, let's assume they are going to connect to LPI AP. PSD for Default client should be -1 dBm/MHz or less and for sub-ordinate client should be 5 dBM/MHz or less​ (US regulatory). Technically, the power mode of client affects the PSD which has indirect relation with the Tx-power. So if user wants more PSD, in supplicant, it can be configured with Subordinate or else default type. Reg your other question: > Anyway you're tied to what the AP is doing, no? Yeah since for AP the command was introduced, leveraged the same for client to. But now since we have option to leverage from update beacon infra, something similar for client need to explore. > johannes
On 8/30/2023 1:50 AM, Johannes Berg wrote:
[...]
> johannes
This patch "[PATCH v3 2/9] wifi: cfg80211: save Power Spectral Density
(PSD) of the regulatory rule" does not have relation with concurrency.
Because it only add a field "s8 psd" into struct ieee80211_reg_rule
and ieee80211_channel. The psd value is same with other field such as
max_reg_power.
max_reg_power is also different value for AP and station mode in db.txt
here:
for example:
"country TW: DFS-FCC" and "country US: DFS-FCC"
# 5.15 ~ 5.25 GHz: 30 dBm for master mode, 23 dBm for clients
https://git.kernel.org/pub/scm/linux/kernel/git/sforshee/wireless-regdb.git/tree/db.txt#n1785
So could you merge the "[PATCH v3 2/9] wifi: cfg80211: save Power Spectral
Density (PSD) of the regulatory rule" firstly?
On Wed, 2023-09-13 at 18:35 +0800, Wen Gong wrote: > > This patch "[PATCH v3 2/9] wifi: cfg80211: save Power Spectral Density > (PSD) of the regulatory rule" does not have relation with concurrency. > Because it only add a field "s8 psd" into struct ieee80211_reg_rule > and ieee80211_channel. The psd value is same with other field such as > max_reg_power. > > max_reg_power is also different value for AP and station mode in db.txt > here: > for example: > "country TW: DFS-FCC" and "country US: DFS-FCC" > # 5.15 ~ 5.25 GHz: 30 dBm for master mode, 23 dBm for clients > https://git.kernel.org/pub/scm/linux/kernel/git/sforshee/wireless-regdb.git/tree/db.txt#n1785 > > So could you merge the "[PATCH v3 2/9] wifi: cfg80211: save Power Spectral > Density (PSD) of the regulatory rule" firstly? Well, arguing with the regdb is kind of a bad thing because it reminds me that nobody ever cares about it anymore ... maybe we should just give up on it? I mean, clearly, if you actually cared your patch here would come with additions to the regdb format (which aren't even that hard now) which then the wireless-regdb could actually use rather than the comment, right? So really all you've achieved right now is again reminding me how little you really care about upstream? johannes
On 9/13/2023 7:55 AM, Johannes Berg wrote: > So really all you've achieved right now is again reminding me how little > you really care about upstream? I have to step in here. As someone who worked many years on the out-of-tree Android drivers I understand where you're coming from. But attitudes change, and my transition to the upstream team is a reflection of that. I believe everyone from Qualcomm currently contributing to nl/cfg/mac80211 are doing so in support of upstream ath11k/12k drivers. There are a multitude of patches that you'll get to see once the precursor work is out of the way. I'd prefer to stick to technical discussions :) Wen, Can you update your patch to better describe WHY your change is needed? /jeff
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?: -------------------------------------------------------------------------------- Let (A) be the implementation of having separate channel pools based on power mode: [...] struct ieee80211_6ghz_channel { struct ieee80211_channel *channels; int num_channels; }; struct ieee80211_supported_band { /* Other members */ struct ieee80211_6ghz_channel[MAX_POWER_MODE]; /* Other members */ } [...] After updating the regulatory rules, in case of (A), all the channels stored in the struct ieee80211_6ghz_channel will have its desired value based on the power mode. Let the alternate implementation be (B) which maintains array of such variables which can vary based on power mode: [...] struct ieee80211_channel { /* Other members*/ u32 flags[MAX_POWER_MODE]; int max_reg_power[MAX_POWER_MODE]; s8 psd[MAX_POWER_MODE]; /* Other members*/ } [...] After updating the regulatory rules in case of (B), for 6 GHz sband channels, all the values based on the power mode are stored in array for each channel. During 6 GHz interface bring up, power mode will be known and in method (A), accordingly the corresponding ieee80211_channel will be selected from the pool. With this, all its members are having the desired value stored in them just like any other ieee80211_channel. For method B, ieee80211_channel will be selected, but its members are having array of possible values. Even though configured power mode is known, still, the members dont have the exact required value. Now, as per the code flow, a pointer for the configured channel is stored in chandef and thats parsed to extract the required information like flags, power, frequency information, etc. If current implementation (A) is followed then the subsequent functions using the set ieee80211_channel will need not be modified since the exact required values based on the power mode is applied to its members already and everything will fall in its place. But if method (B) is used, then in all subsequent functions using the configured ieee80211_channel, condition check on power mode and then accordingly using the appropriate values from the array needs to be implemented at each single place. Hence, method (B) would require code changes at a lot of places leading to more possiblity of errors to come in. Also, currently only flags, power and PSD value is changing based on power mode. If later on any new member or exisiting member is varyed then with method (A), less code changes will be required since altering the ieee80211_channel struct would do. But if method (B) is used then apart from altering ieee80211_channel struct, all functions using ieee80211_channels needs to be modified accordingly to fecth this new varying member from correct power mode index. Aditya Kumar Singh (8): wifi: mac80211: rework on 6 GHz power type definition wifi: mac80211: add combined power type definition for 6 GHz wifi: cfg80211: add NL command to set 6 GHz power mode wifi: mac80211: add support for 6 GHz channels and regulatory wifi: cfg80211: rework nl80211_parse_chandef for 6 GHz wifi: cfg80211: save 6 GHz power mode of the regulatory rules wifi: cfg80211: fix chandef identical logic for 6 GHz chandefs wifi: mac80211: use proper API to fetch 6 GHz channel Wen Gong (1): wifi: cfg80211: save Power Spectral Density (PSD) of the regulatory rule --- v3: - resolved sta mode association issue and AP-STA concurrency bring up * added patch 8 to handle concurrent AP-STA bring up on 6 GHz * added patch 9 to handle bss assoc in 6 GHz - no other changes in patches 1-7 from v2. v2: addressed v1 review comments. * moved variables to link specific w.r.t MLO * rebased on latest ToT * added "wifi:" tag in commit title. --- include/linux/ieee80211.h | 33 +++--- include/net/cfg80211.h | 111 ++++++++++++++++++-- include/net/regulatory.h | 2 + include/uapi/linux/nl80211.h | 61 +++++++++++ net/mac80211/cfg.c | 41 ++++++++ net/mac80211/ieee80211_i.h | 3 + net/mac80211/mlme.c | 14 ++- net/mac80211/scan.c | 15 ++- net/mac80211/util.c | 40 ++++++- net/wireless/ap.c | 2 + net/wireless/nl80211.c | 196 ++++++++++++++++++++++++++++++----- net/wireless/nl80211.h | 3 +- net/wireless/pmsr.c | 8 +- net/wireless/rdev-ops.h | 21 ++++ net/wireless/reg.c | 61 +++++++++-- net/wireless/sme.c | 2 + net/wireless/trace.h | 34 ++++++ net/wireless/util.c | 87 ++++++++++++++++ 18 files changed, 671 insertions(+), 63 deletions(-) base-commit: 4eca8cbf7ba83c3291b5841905ce64584036b1ff