| Message ID | 20210820122041.12157-2-wgong@codeaurora.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | cfg80211/mac80211: Add support for 6GHZ STA for various modes : LPI, SP and VLP | expand |
> struct cfg80211_chan_def { > struct ieee80211_channel *chan; > @@ -684,6 +685,7 @@ struct cfg80211_chan_def { > u32 center_freq2; > struct ieee80211_edmg edmg; > u16 freq1_offset; > + enum nl80211_ap_reg_power power_type; I'm not sure why this should be in the chandef, there's no way that anything in cfg80211 is ever using it there, at least in your patches. > +/** > + * enum nl80211_ap_reg_power - regulatory power for a Access Point > + * > + * @NL80211_REG_UNSET_AP: Access Point has no regulatory power mode > + * @NL80211_REG_LPI: Indoor Access Point > + * @NL80211_REG_SP: Standard power Access Point > + * @NL80211_REG_VLP: Very low power Access Point > + */ > +enum nl80211_ap_reg_power { > + NL80211_REG_UNSET_AP, > + NL80211_REG_LPI_AP, > + NL80211_REG_SP_AP, > + NL80211_REG_VLP_AP, > + NL80211_REG_AP_POWER_AFTER_LAST, > + NL80211_REG_AP_POWER_MAX = > + NL80211_REG_AP_POWER_AFTER_LAST - 1, > +}; Similarly here (and the other enum), why is this in nl80211 if it's never used in nl80211? johannes
On Thu, 2021-08-26 at 10:20 +0200, Johannes Berg wrote: > > struct cfg80211_chan_def { > > struct ieee80211_channel *chan; > > @@ -684,6 +685,7 @@ struct cfg80211_chan_def { > > u32 center_freq2; > > struct ieee80211_edmg edmg; > > u16 freq1_offset; > > + enum nl80211_ap_reg_power power_type; > > I'm not sure why this should be in the chandef, there's no way that > anything in cfg80211 is ever using it there, at least in your patches. Does it even *apply* to a channel? What if I'm connecting to two APs on the same channel (two client interfaces)? johannes
On 2021-08-26 16:20, Johannes Berg wrote: >> struct cfg80211_chan_def { >> struct ieee80211_channel *chan; >> @@ -684,6 +685,7 @@ struct cfg80211_chan_def { >> u32 center_freq2; >> struct ieee80211_edmg edmg; >> u16 freq1_offset; >> + enum nl80211_ap_reg_power power_type; > > I'm not sure why this should be in the chandef, there's no way that > anything in cfg80211 is ever using it there, at least in your patches. > It is used in mac80211 of [PATCH v2 3/8] mac80211: add parse regulatory info in 6 GHz operation information. should i move it to mac80211's .h file? >> +/** >> + * enum nl80211_ap_reg_power - regulatory power for a Access Point >> + * >> + * @NL80211_REG_UNSET_AP: Access Point has no regulatory power mode >> + * @NL80211_REG_LPI: Indoor Access Point >> + * @NL80211_REG_SP: Standard power Access Point >> + * @NL80211_REG_VLP: Very low power Access Point >> + */ >> +enum nl80211_ap_reg_power { >> + NL80211_REG_UNSET_AP, >> + NL80211_REG_LPI_AP, >> + NL80211_REG_SP_AP, >> + NL80211_REG_VLP_AP, >> + NL80211_REG_AP_POWER_AFTER_LAST, >> + NL80211_REG_AP_POWER_MAX = >> + NL80211_REG_AP_POWER_AFTER_LAST - 1, >> +}; > > Similarly here (and the other enum), why is this in nl80211 if it's > never used in nl80211? > It is used in mac80211 of [PATCH v2 3/8] mac80211: add parse regulatory info in 6 GHz operation information. should i move it to mac80211's .h file? > johannes
On Thu, 2021-08-26 at 18:57 +0800, Wen Gong wrote: > On 2021-08-26 16:20, Johannes Berg wrote: > > > struct cfg80211_chan_def { > > > struct ieee80211_channel *chan; > > > @@ -684,6 +685,7 @@ struct cfg80211_chan_def { > > > u32 center_freq2; > > > struct ieee80211_edmg edmg; > > > u16 freq1_offset; > > > + enum nl80211_ap_reg_power power_type; > > > > I'm not sure why this should be in the chandef, there's no way that > > anything in cfg80211 is ever using it there, at least in your patches. > > > It is used in mac80211 of [PATCH v2 3/8] mac80211: add parse regulatory > info in 6 GHz operation information. > should i move it to mac80211's .h file? > > > +/** > > > + * enum nl80211_ap_reg_power - regulatory power for a Access Point [...] > > > It is used in mac80211 of [PATCH v2 3/8] mac80211: add parse regulatory > info in 6 GHz operation information. > should i move it to mac80211's .h file? Yeah I saw both of them are used, but why are they defined as nl80211 API? Do you have any intention to set them through nl80211? And like I said, I'm not really convinced this belongs into struct cfg80211_chan_def either. Maybe it should be in bss_conf too? johannes >
On 2021-08-26 18:59, Johannes Berg wrote: > On Thu, 2021-08-26 at 18:57 +0800, Wen Gong wrote: >> On 2021-08-26 16:20, Johannes Berg wrote: >> > > struct cfg80211_chan_def { >> > > struct ieee80211_channel *chan; >> > > @@ -684,6 +685,7 @@ struct cfg80211_chan_def { >> > > u32 center_freq2; >> > > struct ieee80211_edmg edmg; >> > > u16 freq1_offset; >> > > + enum nl80211_ap_reg_power power_type; >> > >> > I'm not sure why this should be in the chandef, there's no way that >> > anything in cfg80211 is ever using it there, at least in your patches. >> > >> It is used in mac80211 of [PATCH v2 3/8] mac80211: add parse >> regulatory >> info in 6 GHz operation information. >> should i move it to mac80211's .h file? >> > > +/** >> > > + * enum nl80211_ap_reg_power - regulatory power for a Access Point > [...] >> > >> It is used in mac80211 of [PATCH v2 3/8] mac80211: add parse >> regulatory >> info in 6 GHz operation information. >> should i move it to mac80211's .h file? > > Yeah I saw both of them are used, but why are they defined as nl80211 > API? Do you have any intention to set them through nl80211? > > And like I said, I'm not really convinced this belongs into struct > cfg80211_chan_def either. Maybe it should be in bss_conf too? yes, I also want to move it to struct ieee80211_bss_conf. > > johannes >>
On 2021-08-26 16:22, Johannes Berg wrote: > On Thu, 2021-08-26 at 10:20 +0200, Johannes Berg wrote: >> > struct cfg80211_chan_def { >> > struct ieee80211_channel *chan; >> > @@ -684,6 +685,7 @@ struct cfg80211_chan_def { >> > u32 center_freq2; >> > struct ieee80211_edmg edmg; >> > u16 freq1_offset; >> > + enum nl80211_ap_reg_power power_type; >> >> I'm not sure why this should be in the chandef, there's no way that >> anything in cfg80211 is ever using it there, at least in your patches. > > Does it even *apply* to a channel? What if I'm connecting to two APs on > the same channel (two client interfaces)? > this is one copy for each connection, each client has its own cfg80211_chan_def. also it can be moved to struct ieee80211_bss_conf. > johannes
On Thu, 2021-08-26 at 19:02 +0800, Wen Gong wrote: > On 2021-08-26 16:22, Johannes Berg wrote: > > On Thu, 2021-08-26 at 10:20 +0200, Johannes Berg wrote: > > > > struct cfg80211_chan_def { > > > > struct ieee80211_channel *chan; > > > > @@ -684,6 +685,7 @@ struct cfg80211_chan_def { > > > > u32 center_freq2; > > > > struct ieee80211_edmg edmg; > > > > u16 freq1_offset; > > > > + enum nl80211_ap_reg_power power_type; > > > > > > I'm not sure why this should be in the chandef, there's no way that > > > anything in cfg80211 is ever using it there, at least in your patches. > > > > Does it even *apply* to a channel? What if I'm connecting to two APs on > > the same channel (two client interfaces)? > > > this is one copy for each connection, each client has its own > cfg80211_chan_def. That depends on where you check it - but you're basically saying "use this only from vif->bss_conf.chandef (or something, didn't check now), but chandef shows up in many other places and you don't maintain it anywhere else. johannes
diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h index 58c2cd417e89..f17a4d1202fc 100644 --- a/include/net/cfg80211.h +++ b/include/net/cfg80211.h @@ -676,6 +676,7 @@ struct key_params { * chan will define the primary channel and all other * parameters are ignored. * @freq1_offset: offset from @center_freq1, in KHz + * @power_type: power type of BSS for 6 GHz */ struct cfg80211_chan_def { struct ieee80211_channel *chan; @@ -684,6 +685,7 @@ struct cfg80211_chan_def { u32 center_freq2; struct ieee80211_edmg edmg; u16 freq1_offset; + enum nl80211_ap_reg_power power_type; }; /* diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h index f962c06e9818..ab1d4aa85272 100644 --- a/include/uapi/linux/nl80211.h +++ b/include/uapi/linux/nl80211.h @@ -4088,6 +4088,40 @@ enum nl80211_dfs_regions { NL80211_DFS_JP = 3, }; +/** + * enum nl80211_ap_reg_power - regulatory power for a Access Point + * + * @NL80211_REG_UNSET_AP: Access Point has no regulatory power mode + * @NL80211_REG_LPI: Indoor Access Point + * @NL80211_REG_SP: Standard power Access Point + * @NL80211_REG_VLP: Very low power Access Point + */ +enum nl80211_ap_reg_power { + NL80211_REG_UNSET_AP, + NL80211_REG_LPI_AP, + NL80211_REG_SP_AP, + NL80211_REG_VLP_AP, + NL80211_REG_AP_POWER_AFTER_LAST, + NL80211_REG_AP_POWER_MAX = + NL80211_REG_AP_POWER_AFTER_LAST - 1, +}; + +/** + * enum nl80211_client_reg_power - regulatory power for a client + * + * @NL80211_REG_UNSET_CLIENT: Client has no regulatory power mode + * @NL80211_REG_DEFAULT_CLIENT: Default Client + * @NL80211_REG_SUBORDINATE_CLIENT: Subordinate Client + */ +enum nl80211_client_reg_power { + NL80211_REG_UNSET_CLIENT, + NL80211_REG_DEFAULT_CLIENT, + NL80211_REG_SUBORDINATE_CLIENT, + NL80211_REG_CLIENT_POWER_AFTER_LAST, + NL80211_REG_CLIENT_POWER_MAX = + NL80211_REG_CLIENT_POWER_AFTER_LAST - 1, +}; + /** * enum nl80211_user_reg_hint_type - type of user regulatory hint *
6 GHz regulatory domains introduces different modes for 6 GHz AP operations Low Power Indoor(LPI), Standard Power(SP) and Very Low Power(VLP). 6 GHz STAs could be operated as either Regular or Subordinate clients. This patch is define the flags for power type of AP and STATION mode. Signed-off-by: Wen Gong <wgong@codeaurora.org> --- include/net/cfg80211.h | 2 ++ include/uapi/linux/nl80211.h | 34 ++++++++++++++++++++++++++++++++++ 2 files changed, 36 insertions(+)