Re: [PATCH 2/2] cfg80211: Modify wiphy registration semantics for self managed hints

[asinghal]

hi Johannes,
replies inline:

On 2018-04-19 08:07, Johannes Berg wrote:

On Fri, 2018-04-13 at 13:32 -0700, asing...@codeaurora.org wrote:

hi Johannes,
 please fine some replies inline:

On 2018-03-21 03:15, Johannes Berg wrote:
> So I really think this should just be one patch - it's not about
> "registration semantics" but about which types of requests get passed
> to reg_notifier(), and if you do it in one place you'd better also do
> it in the other.

Sure, I have combined the two patches in one patch now:


So now you should probably resend it properly, with a new subject that
explains it better? Just "modify" doesn't really seem all that
appropriate - what's the modification?

Patchwork also lost half the patch for some reason, probably you
copy/pasted it and lost some whitespace at an empty line.


We have posted a new patch-set with better commit title and message.


Call the regulatory notifier for self managed hints only if
initiator is NL80211_REGDOM_SET_BY_USER and hint type is
NL80211_USER_REG_HINT_CELL_BASE. Also call regulatory
notifier when wiphy is registered under similar conditions.


I guess this should say why.


list_for_each_entry(rdev, _rdev_list, list) {
wiphy = >wiphy;
-   if (wiphy->regulatory_flags & REGULATORY_WIPHY_SELF_MANAGED)
+   if (wiphy->regulatory_flags & REGULATORY_WIPHY_SELF_MANAGED) {
self_managed_found = true;
-   else
-   return false;
+   if (request->initiator == NL80211_REGDOM_SET_BY_USER &&
+   request->user_reg_hint_type ==
+   NL80211_USER_REG_HINT_CELL_BASE)
+   reg_call_notifier(wiphy, request);
+   } else {
+   self_managed_found = false;
+   }
}


This is awkward now - how about

self_managed_found = regulatory_flags & SELF_MANAGED;
if (self_managed_found &&
request->initiator == ... &&
...)
reg_call_notifier(...)


This has been addressed in the new patch with a separate notify 
function.




@@ -3700,15 +3705,21 @@ void wiphy_regulatory_register(struct wiphy
*wiphy)
  {
struct regulatory_request *lr;

-   /* self-managed devices ignore external hints */
-   if (wiphy->regulatory_flags & REGULATORY_WIPHY_SELF_MANAGED)
+   lr = get_last_request();
+
+   /* self-managed devices ignore beacon hints and 11d IE */
+   if (wiphy->regulatory_flags & REGULATORY_WIPHY_SELF_MANAGED) {
wiphy->regulatory_flags |= REGULATORY_DISABLE_BEACON_HINTS |
-  REGULATORY_COUNTRY_IE_IGNORE;
+   REGULATORY_COUNTRY_IE_IGNORE;


no need to change the indentation here


has been addressed in new patch.
thanks,
amar


johannes

Re: [PATCH 2/2] cfg80211: Modify wiphy registration semantics for self managed hints

[Johannes Berg]

On Fri, 2018-04-13 at 13:32 -0700, asing...@codeaurora.org wrote:
> hi Johannes,
>  please fine some replies inline:
> 
> On 2018-03-21 03:15, Johannes Berg wrote:
> > So I really think this should just be one patch - it's not about
> > "registration semantics" but about which types of requests get passed
> > to reg_notifier(), and if you do it in one place you'd better also do
> > it in the other.
> 
> Sure, I have combined the two patches in one patch now:

So now you should probably resend it properly, with a new subject that
explains it better? Just "modify" doesn't really seem all that
appropriate - what's the modification?

Patchwork also lost half the patch for some reason, probably you
copy/pasted it and lost some whitespace at an empty line.

> Call the regulatory notifier for self managed hints only if
> initiator is NL80211_REGDOM_SET_BY_USER and hint type is
> NL80211_USER_REG_HINT_CELL_BASE. Also call regulatory
> notifier when wiphy is registered under similar conditions.

I guess this should say why.

>   list_for_each_entry(rdev, _rdev_list, list) {
>   wiphy = >wiphy;
> - if (wiphy->regulatory_flags & REGULATORY_WIPHY_SELF_MANAGED)
> + if (wiphy->regulatory_flags & REGULATORY_WIPHY_SELF_MANAGED) {
>   self_managed_found = true;
> - else
> - return false;
> + if (request->initiator == NL80211_REGDOM_SET_BY_USER &&
> + request->user_reg_hint_type ==
> + NL80211_USER_REG_HINT_CELL_BASE)
> + reg_call_notifier(wiphy, request);
> + } else {
> + self_managed_found = false;
> + }
>   }

This is awkward now - how about

self_managed_found = regulatory_flags & SELF_MANAGED;
if (self_managed_found &&
request->initiator == ... &&
...)
reg_call_notifier(...)


> @@ -3700,15 +3705,21 @@ void wiphy_regulatory_register(struct wiphy 
> *wiphy)
>   {
>   struct regulatory_request *lr;
> 
> - /* self-managed devices ignore external hints */
> - if (wiphy->regulatory_flags & REGULATORY_WIPHY_SELF_MANAGED)
> + lr = get_last_request();
> +
> + /* self-managed devices ignore beacon hints and 11d IE */
> + if (wiphy->regulatory_flags & REGULATORY_WIPHY_SELF_MANAGED) {
>   wiphy->regulatory_flags |= REGULATORY_DISABLE_BEACON_HINTS |
> -REGULATORY_COUNTRY_IE_IGNORE;
> + REGULATORY_COUNTRY_IE_IGNORE;

no need to change the indentation here

johannes

Re: [PATCH 2/2] cfg80211: Modify wiphy registration semantics for self managed hints

[asinghal]

hi Johannes,
please fine some replies inline:

On 2018-03-21 03:15, Johannes Berg wrote:

So I really think this should just be one patch - it's not about
"registration semantics" but about which types of requests get passed
to reg_notifier(), and if you do it in one place you'd better also do
it in the other.


Sure, I have combined the two patches in one patch now:

From: Amar Singhal 

Call the regulatory notifier for self managed hints only if
initiator is NL80211_REGDOM_SET_BY_USER and hint type is
NL80211_USER_REG_HINT_CELL_BASE. Also call regulatory
notifier when wiphy is registered under similar conditions.

Signed-off-by: Amar Singhal 
Signed-off-by: Kiran Kumar Lokere 
Signed-off-by: Jouni Malinen 
---
 net/wireless/reg.c | 31 +--
 1 file changed, 21 insertions(+), 10 deletions(-)

diff --git a/net/wireless/reg.c b/net/wireless/reg.c
index 16c7e4e..d74de76 100644
--- a/net/wireless/reg.c
+++ b/net/wireless/reg.c
@@ -2768,20 +2768,25 @@ static void reg_process_hint(struct 
regulatory_request *reg_request)

reg_free_request(reg_request);
 }

-static bool reg_only_self_managed_wiphys(void)
+static bool reg_only_self_managed_wiphys(struct regulatory_request 
*request)

 {
struct cfg80211_registered_device *rdev;
struct wiphy *wiphy;
-   bool self_managed_found = false;
+   bool self_managed_found = true;

ASSERT_RTNL();

list_for_each_entry(rdev, _rdev_list, list) {
wiphy = >wiphy;
-   if (wiphy->regulatory_flags & REGULATORY_WIPHY_SELF_MANAGED)
+   if (wiphy->regulatory_flags & REGULATORY_WIPHY_SELF_MANAGED) {
self_managed_found = true;
-   else
-   return false;
+   if (request->initiator == NL80211_REGDOM_SET_BY_USER &&
+   request->user_reg_hint_type ==
+   NL80211_USER_REG_HINT_CELL_BASE)
+   reg_call_notifier(wiphy, request);
+   } else {
+   self_managed_found = false;
+   }
}

/* make sure at least one self-managed wiphy exists */
@@ -2819,7 +2824,7 @@ static void reg_process_pending_hints(void)

spin_unlock(_requests_lock);

-   if (reg_only_self_managed_wiphys()) {
+   if (reg_only_self_managed_wiphys(reg_request)) {
reg_free_request(reg_request);
return;
}
@@ -3700,15 +3705,21 @@ void wiphy_regulatory_register(struct wiphy 
*wiphy)

 {
struct regulatory_request *lr;

-   /* self-managed devices ignore external hints */
-   if (wiphy->regulatory_flags & REGULATORY_WIPHY_SELF_MANAGED)
+   lr = get_last_request();
+
+   /* self-managed devices ignore beacon hints and 11d IE */
+   if (wiphy->regulatory_flags & REGULATORY_WIPHY_SELF_MANAGED) {
wiphy->regulatory_flags |= REGULATORY_DISABLE_BEACON_HINTS |
-  REGULATORY_COUNTRY_IE_IGNORE;
+   REGULATORY_COUNTRY_IE_IGNORE;
+
+   if (lr->initiator == NL80211_REGDOM_SET_BY_USER &&
+   lr->user_reg_hint_type == NL80211_USER_REG_HINT_CELL_BASE)
+   reg_call_notifier(wiphy, lr);
+   }

if (!reg_dev_ignore_cell_hint(wiphy))
reg_num_devs_support_basehint++;

-   lr = get_last_request();
wiphy_update_regulatory(wiphy, lr->initiator);
wiphy_all_share_dfs_chan_state(wiphy);
 }
--
1.9.1


Secondly, this changes behaviour - not only for ath which presumably is
what you want, but also for
 * brcmfmac
 * brcmsmac
 * libertas
 * mwifiex
 * mt76
 * qtnfmac
 * rtlwifi (& its staging cousin)
 * rsi
 * wlcore
 * rtl8723bs (staging)

So I'm not really convinced we should do this unconditionally.


I am calling the callback conditionally.
Only couple of drivers are registering regulatory flag 
REGULATORY_WIPHY_SELF_MANAGED:
drivers/net/wireless/intel/iwlwifi/mvm/mac80211.c:  
>hw->wiphy->regulatory_flags |= REGULATORY_WIPHY_SELF_MANAGED;

above driver does not register reg_notifier callback
drivers/net/wireless/quantenna/qtnfmac/cfg80211.c:  
>wiphy->regulatory_flags |= REGULATORY_WIPHY_SELF_MANAGED;

drivers/net/wireless/quantenna/qtnfmac/cfg80211.c:
(wiphy->regulatory_flags & REGULATORY_WIPHY_SELF_MANAGED)
Above driver registers reg_notifier callback only when flag 
REGULATORY_WIPHY_SELF_MANAGED is not defined.


Also, in this change, when REGULATORY_WIPHY_SELF_MANAGED is defined, the 
callback is conditional on request initiator being 
NL80211_REGDOM_SET_BY_USER and user_reg_hint_type being 
NL80211_USER_REG_HINT_CELL_BASE.




Johannes


regards,
Amar

Re: [PATCH 2/2] cfg80211: Modify wiphy registration semantics for self managed hints

[Johannes Berg]

So I really think this should just be one patch - it's not about
"registration semantics" but about which types of requests get passed
to reg_notifier(), and if you do it in one place you'd better also do
it in the other.

Secondly, this changes behaviour - not only for ath which presumably is
what you want, but also for
 * brcmfmac
 * brcmsmac
 * libertas
 * mwifiex
 * mt76
 * qtnfmac
 * rtlwifi (& its staging cousin)
 * rsi
 * wlcore
 * rtl8723bs (staging)

So I'm not really convinced we should do this unconditionally.

johannes

[PATCH 2/2] cfg80211: Modify wiphy registration semantics for self managed hints

[Jouni Malinen]

From: Amar Singhal 

Call regulatory notifier conditionally for self managed hints during
wiphy registration. Call when regulatory hint source is
NL80211_REGDOM_SET_BY_USER and sub-type is
NL80211_USER_REG_HINT_CELL_BASE. Call the notifier with last
reg-domain request. This is needed to allow trusted regulatory domain
changes to be processed by the driver.

Signed-off-by: Amar Singhal 
Signed-off-by: Kiran Kumar Lokere 
Signed-off-by: Jouni Malinen 
---
 net/wireless/reg.c | 12 +---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/net/wireless/reg.c b/net/wireless/reg.c
index 4f25a11b..48eaa8d 100644
--- a/net/wireless/reg.c
+++ b/net/wireless/reg.c
@@ -3518,15 +3518,21 @@ void wiphy_regulatory_register(struct wiphy *wiphy)
 {
struct regulatory_request *lr;
 
-   /* self-managed devices ignore external hints */
-   if (wiphy->regulatory_flags & REGULATORY_WIPHY_SELF_MANAGED)
+   lr = get_last_request();
+
+   /* self-managed devices ignore beacon hints and 11d IE */
+   if (wiphy->regulatory_flags & REGULATORY_WIPHY_SELF_MANAGED) {
wiphy->regulatory_flags |= REGULATORY_DISABLE_BEACON_HINTS |
   REGULATORY_COUNTRY_IE_IGNORE;
 
+   if (lr->initiator == NL80211_REGDOM_SET_BY_USER &&
+   lr->user_reg_hint_type == NL80211_USER_REG_HINT_CELL_BASE)
+   reg_call_notifier(wiphy, lr);
+   }
+
if (!reg_dev_ignore_cell_hint(wiphy))
reg_num_devs_support_basehint++;
 
-   lr = get_last_request();
wiphy_update_regulatory(wiphy, lr->initiator);
wiphy_all_share_dfs_chan_state(wiphy);
 }
-- 
2.7.4