diff mbox series

[v6,1/2] rtw88: add regulatory process strategy for different chipset

Message ID 20200324075216.22553-2-yhchuang@realtek.com (mailing list archive)
State Changes Requested
Delegated to: Kalle Valo
Headers show
Series rtw88: update regulatory settings | expand

Commit Message

Tony Chuang March 24, 2020, 7:52 a.m. UTC 
From: Tzu-En Huang <tehuang@realtek.com>

If the country code in efuse is not set to a specific regulatory,
then the efuse setting is WW (world-wide).

For the country codes that are set to a specific country, the driver
is not going to apply any new setting notified from the stack.

For the country codes that are not set, which is treated as WW, the
driver hints stack with "00" country code. But when there's any other
regulatory found in IE, notified by NL80211_REGDOM_SET_BY_COUNTRY_IE,
the driver will apply the setting found in the IE for 802.11d.

We want all of the regulatory rules being set strictly, not to loose
the restrictions of the country setting, so apply REGULATORY_STRICT_REG
to all of the regulatory settings.

For user notification (NL80211_REGDOM_SET_BY_USER), we'd like to
default disable it because as FCC publication 594280 states:
"In particular, users must not be relied on to set a country code or
location code to ensure compliance". If any distributions or OSes want
to set regulatory through user space tool (ex, iw reg set), then they
should honor the compile flag option RTW88_REGD_USER_REG_HINTS.

Signed-off-by: Tzu-En Huang <tehuang@realtek.com>
Signed-off-by: Yan-Hsuan Chuang <yhchuang@realtek.com>
---

v3 -> v4
  * squashed with "rtw88: support dynamic user regulatory setting"
  * modify the commit log

v4 -> v5
  * no change

v5 -> v6
  * remove custom world-wide, use stack world-wide
  * surface error codes

 drivers/net/wireless/realtek/rtw88/Kconfig | 10 ++++
 drivers/net/wireless/realtek/rtw88/main.c  |  5 +-
 drivers/net/wireless/realtek/rtw88/regd.c  | 54 ++++++++++++++++++----
 3 files changed, 57 insertions(+), 12 deletions(-)

Comments

Brian Norris March 24, 2020, 4:51 p.m. UTC | #1 
Hi,

On Tue, Mar 24, 2020 at 03:52:15PM +0800, yhchuang@realtek.com wrote:
> --- a/drivers/net/wireless/realtek/rtw88/Kconfig
> +++ b/drivers/net/wireless/realtek/rtw88/Kconfig
> @@ -52,4 +52,14 @@ config RTW88_DEBUGFS
>  
>  	  If unsure, say Y to simplify debug problems
>  
> +config RTW88_REGD_USER_REG_HINTS
> +	bool "Realtek rtw88 user regulatory hints"
> +	depends on RTW88_CORE
> +	default n
> +	help
> +	  Enable regulatoy user hints
> +
> +	  If unsure, say N. This should only be allowed on distributions
> +	  that need this to correct the regulatory.
> +

I'm still not sure why rtw88 needs this, and nobody else does. I read
your commit message, but that doesn't sound like something that belongs
in a single driver still.

>  endif
> diff --git a/drivers/net/wireless/realtek/rtw88/main.c b/drivers/net/wireless/realtek/rtw88/main.c
> index 7640e97706f5..5d43bef91a3c 100644
> --- a/drivers/net/wireless/realtek/rtw88/main.c
> +++ b/drivers/net/wireless/realtek/rtw88/main.c
> @@ -1501,8 +1501,9 @@ int rtw_register_hw(struct rtw_dev *rtwdev, struct ieee80211_hw *hw)
>  		return ret;
>  	}
>  
> -	if (regulatory_hint(hw->wiphy, rtwdev->regd.alpha2))
> -		rtw_err(rtwdev, "regulatory_hint fail\n");
> +	ret = regulatory_hint(hw->wiphy, rtwdev->efuse.country_code);
> +	if (ret)
> +		rtw_warn(rtwdev, "failed to hint regulatory: %d\n", ret);

I don't think this is what you want; you had it right in previous
revisions:

	if (!rtwdev->efuse.country_worldwide) {
		if (regulatory_hint(hw->wiphy, rtwdev->efuse.country_code))
			rtw_err( ... );
	}

Without the 'country_worlwide' check, you start "hinting" (even on
worldwide chips) that you really wanted "country" 00 only, and so we
*never* adapt to more strict country settings. That's not how world-wide
settings are supposed to work.

>  
>  	rtw_debugfs_init(rtwdev);
>  
> diff --git a/drivers/net/wireless/realtek/rtw88/regd.c b/drivers/net/wireless/realtek/rtw88/regd.c
> index 69744dd65968..4cc1234bfb9a 100644
> --- a/drivers/net/wireless/realtek/rtw88/regd.c
> +++ b/drivers/net/wireless/realtek/rtw88/regd.c
>  static int rtw_regd_notifier_apply(struct rtw_dev *rtwdev,
>  				   struct wiphy *wiphy,
>  				   struct regulatory_request *request)
>  {
> -	if (request->initiator == NL80211_REGDOM_SET_BY_USER)
> +	if (request->initiator == NL80211_REGDOM_SET_BY_DRIVER)

Why are you ignoring SET_BY_DRIVER? That's what happens when (a few
lines up) you call regulatory_hint(). At a minimum, that doesn't deserve
a loud error print when we "fail" this function -- you should handle it
properly.

Brian

> +		return -ENOTSUPP;
> +
> +	if (request->initiator == NL80211_REGDOM_SET_BY_USER &&
> +	    !IS_ENABLED(CONFIG_RTW88_REGD_USER_REG_HINTS))
> +		return -EPERM;
> +
> +	if (request->initiator == NL80211_REGDOM_SET_BY_CORE) {
> +		char *country_code;
> +
> +		/* return to the efuse setting */
> +		country_code = rtwdev->efuse.country_code;
> +		rtwdev->regd = rtw_regd_find_reg_by_name(country_code);
>  		return 0;
> +	}
> +
>  	rtwdev->regd = rtw_regd_find_reg_by_name(request->alpha2);
>  	rtw_regd_apply_world_flags(wiphy, request->initiator);
>
Andy Huang March 25, 2020, 3:11 a.m. UTC | #2 
> 
> Hi,
> 
> On Tue, Mar 24, 2020 at 03:52:15PM +0800, yhchuang@realtek.com wrote:
> > --- a/drivers/net/wireless/realtek/rtw88/Kconfig
> > +++ b/drivers/net/wireless/realtek/rtw88/Kconfig
> > @@ -52,4 +52,14 @@ config RTW88_DEBUGFS
> >
> >  	  If unsure, say Y to simplify debug problems
> >
> > +config RTW88_REGD_USER_REG_HINTS
> > +	bool "Realtek rtw88 user regulatory hints"
> > +	depends on RTW88_CORE
> > +	default n
> > +	help
> > +	  Enable regulatoy user hints
> > +
> > +	  If unsure, say N. This should only be allowed on distributions
> > +	  that need this to correct the regulatory.
> > +
> 
> I'm still not sure why rtw88 needs this, and nobody else does. I read 

I think in Atheros driver, ATH_REG_DYNAMIC_USER_REG_HINTS config serves
the same purpose.

> your commit message, but that doesn't sound like something that belongs
> in a single driver still.
> 

As our previous commit message claims, it is due to FCC publication 594280
statement, "In particular, users must not be relied on to set a country code or
location code to ensure compliance".

> >  endif
> > diff --git a/drivers/net/wireless/realtek/rtw88/main.c
> b/drivers/net/wireless/realtek/rtw88/main.c
> > index 7640e97706f5..5d43bef91a3c 100644
> > --- a/drivers/net/wireless/realtek/rtw88/main.c
> > +++ b/drivers/net/wireless/realtek/rtw88/main.c
> > @@ -1501,8 +1501,9 @@ int rtw_register_hw(struct rtw_dev *rtwdev,
> struct ieee80211_hw *hw)
> >  		return ret;
> >  	}
> >
> > -	if (regulatory_hint(hw->wiphy, rtwdev->regd.alpha2))
> > -		rtw_err(rtwdev, "regulatory_hint fail\n");
> > +	ret = regulatory_hint(hw->wiphy, rtwdev->efuse.country_code);
> > +	if (ret)
> > +		rtw_warn(rtwdev, "failed to hint regulatory: %d\n", ret);
> 
> I don't think this is what you want; you had it right in previous
> revisions:
> 
> 	if (!rtwdev->efuse.country_worldwide) {
> 		if (regulatory_hint(hw->wiphy, rtwdev->efuse.country_code))
> 			rtw_err( ... );
> 	}
> 
> Without the 'country_worlwide' check, you start "hinting" (even on
> worldwide chips) that you really wanted "country" 00 only, and so we
> *never* adapt to more strict country settings. That's not how world-wide
> settings are supposed to work.
> 

It doesn't mean that we want country 00 only, we will get country notifies
from stack, and we will apply it if we accept it. We don't want stack to change
the channel plan for us.
And this is also related to RTW88_REGD_USER_REG_HINTS config, since we
do not want stack to change the channel plan set from user space without
this config.

> >
> >  	rtw_debugfs_init(rtwdev);
> >
> > diff --git a/drivers/net/wireless/realtek/rtw88/regd.c
> b/drivers/net/wireless/realtek/rtw88/regd.c
> > index 69744dd65968..4cc1234bfb9a 100644
> > --- a/drivers/net/wireless/realtek/rtw88/regd.c
> > +++ b/drivers/net/wireless/realtek/rtw88/regd.c
> >  static int rtw_regd_notifier_apply(struct rtw_dev *rtwdev,
> >  				   struct wiphy *wiphy,
> >  				   struct regulatory_request *request)
> >  {
> > -	if (request->initiator == NL80211_REGDOM_SET_BY_USER)
> > +	if (request->initiator == NL80211_REGDOM_SET_BY_DRIVER)
> 
> Why are you ignoring SET_BY_DRIVER? That's what happens when (a few
> lines up) you call regulatory_hint(). At a minimum, that doesn't deserve
> a loud error print when we "fail" this function -- you should handle it
> properly.
> 
> Brian

Since the notification with NL80211_REGDOM_SET_BY_DRIVER flag might
comes from an another chipset's regulatory_hint().

Tzu-En

> 
> > +		return -ENOTSUPP;
> > +
> > +	if (request->initiator == NL80211_REGDOM_SET_BY_USER &&
> > +	    !IS_ENABLED(CONFIG_RTW88_REGD_USER_REG_HINTS))
> > +		return -EPERM;
> > +
> > +	if (request->initiator == NL80211_REGDOM_SET_BY_CORE) {
> > +		char *country_code;
> > +
> > +		/* return to the efuse setting */
> > +		country_code = rtwdev->efuse.country_code;
> > +		rtwdev->regd = rtw_regd_find_reg_by_name(country_code);
> >  		return 0;
> > +	}
> > +
> >  	rtwdev->regd = rtw_regd_find_reg_by_name(request->alpha2);
> >  	rtw_regd_apply_world_flags(wiphy, request->initiator);
> >
> 
> ------Please consider the environment before printing this e-mail.
Brian Norris March 26, 2020, 1:45 a.m. UTC | #3 
[ I'll preface this by saying that the more I look at the regulatory
core, the more I realize I'm confused or wrong at times. So forgive me
if I've made errors along the way, and please do correct me. ]

On Tue, Mar 24, 2020 at 8:11 PM Andy Huang <tehuang@realtek.com> wrote:
> > On Tue, Mar 24, 2020 at 03:52:15PM +0800, yhchuang@realtek.com wrote:
> > > --- a/drivers/net/wireless/realtek/rtw88/Kconfig
> > > +++ b/drivers/net/wireless/realtek/rtw88/Kconfig

> > I'm still not sure why rtw88 needs this, and nobody else does. I read
>
> I think in Atheros driver, ATH_REG_DYNAMIC_USER_REG_HINTS config serves
> the same purpose.

Ah, I forgot about that one, sorry.

> > your commit message, but that doesn't sound like something that belongs
> > in a single driver still.
> >
>
> As our previous commit message claims, it is due to FCC [...]

Yes, I saw that: my point was that effectively all drivers are subject
to this FCC rule, and so this could be a common CONFIG_*. But if we
already have the ATH_* one (I missed that, above), I guess we can have
an rtw88 one too. It might be less confusing (and more
straightforwardly-implemented) if we moved this stuff to the core
someday, though.

> > > +   ret = regulatory_hint(hw->wiphy, rtwdev->efuse.country_code);
> > > +   if (ret)
> > > +           rtw_warn(rtwdev, "failed to hint regulatory: %d\n", ret);
> >
> > I don't think this is what you want; you had it right in previous
> > revisions:
> >
> >       if (!rtwdev->efuse.country_worldwide) {
> >               if (regulatory_hint(hw->wiphy, rtwdev->efuse.country_code))
> >                       rtw_err( ... );
> >       }
> >
> > Without the 'country_worlwide' check, you start "hinting" (even on
> > worldwide chips) that you really wanted "country" 00 only, and so we
> > *never* adapt to more strict country settings. That's not how world-wide
> > settings are supposed to work.
>
> It doesn't mean that we want country 00 only, we will get country notifies
> from stack, and we will apply it if we accept it. We don't want stack to change
> the channel plan for us.

I noted this to you privately, but I don't believe it's expected to
call regulatory_hint() with "00". See the kerneldoc:

 * @alpha2: the ISO/IEC 3166 alpha2 the driver claims its regulatory domain
 *      should be in. If @rd is set this should be NULL. Note that if you
 *      set this to NULL you should still set rd->alpha2 to some accepted
 *      alpha2.

Note that "00" is *not* actually an ISO 3166 alpha2 code.

The key problem I'm seeing: once you do this, you establish a
wiphy-specific regd, and this regd never updates its country code or
DFS region according to IE updates. So attributes like
NL80211_ATTR_DFS_REGION and NL80211_ATTR_REG_ALPHA2 remain unset.

Your previous revision -- which for WW settings used
wiphy_apply_custom_regulatory() and *not* regulatory_hint() -- did not
have that problem.

> > Why are you ignoring SET_BY_DRIVER?
>
> Since the notification with NL80211_REGDOM_SET_BY_DRIVER flag might
> comes from an another chipset's regulatory_hint().

Ack.

Brian
Andy Huang March 26, 2020, 7:11 a.m. UTC | #4 
> [ I'll preface this by saying that the more I look at the regulatory
> core, the more I realize I'm confused or wrong at times. So forgive me
> if I've made errors along the way, and please do correct me. ]
> 
> On Tue, Mar 24, 2020 at 8:11 PM Andy Huang <tehuang@realtek.com> wrote:
> > > On Tue, Mar 24, 2020 at 03:52:15PM +0800, yhchuang@realtek.com
> wrote:
> > > > --- a/drivers/net/wireless/realtek/rtw88/Kconfig
> > > > +++ b/drivers/net/wireless/realtek/rtw88/Kconfig
> 
> > > I'm still not sure why rtw88 needs this, and nobody else does. I read
> >
> > I think in Atheros driver, ATH_REG_DYNAMIC_USER_REG_HINTS config
> serves
> > the same purpose.
> 
> Ah, I forgot about that one, sorry.
> 
> > > your commit message, but that doesn't sound like something that belongs
> > > in a single driver still.
> > >
> >
> > As our previous commit message claims, it is due to FCC [...]
> 
> Yes, I saw that: my point was that effectively all drivers are subject
> to this FCC rule, and so this could be a common CONFIG_*. But if we
> already have the ATH_* one (I missed that, above), I guess we can have
> an rtw88 one too. It might be less confusing (and more
> straightforwardly-implemented) if we moved this stuff to the core
> someday, though.
> 
> > > > +   ret = regulatory_hint(hw->wiphy, rtwdev->efuse.country_code);
> > > > +   if (ret)
> > > > +           rtw_warn(rtwdev, "failed to hint regulatory: %d\n", ret);
> > >
> > > I don't think this is what you want; you had it right in previous
> > > revisions:
> > >
> > >       if (!rtwdev->efuse.country_worldwide) {
> > >               if (regulatory_hint(hw->wiphy,
> rtwdev->efuse.country_code))
> > >                       rtw_err( ... );
> > >       }
> > >
> > > Without the 'country_worlwide' check, you start "hinting" (even on
> > > worldwide chips) that you really wanted "country" 00 only, and so we
> > > *never* adapt to more strict country settings. That's not how world-wide
> > > settings are supposed to work.
> >
> > It doesn't mean that we want country 00 only, we will get country notifies
> > from stack, and we will apply it if we accept it. We don't want stack to
> change
> > the channel plan for us.
> 
> I noted this to you privately, but I don't believe it's expected to
> call regulatory_hint() with "00". See the kerneldoc:
> 
>  * @alpha2: the ISO/IEC 3166 alpha2 the driver claims its regulatory domain
>  *      should be in. If @rd is set this should be NULL. Note that if you
>  *      set this to NULL you should still set rd->alpha2 to some accepted
>  *      alpha2.
> 
> Note that "00" is *not* actually an ISO 3166 alpha2 code.
> 
Yes, I think you make sense, we will provide v7, which will be similar to v5 with
more detailed explain in commit log.

Tzu-En

> The key problem I'm seeing: once you do this, you establish a
> wiphy-specific regd, and this regd never updates its country code or
> DFS region according to IE updates. So attributes like
> NL80211_ATTR_DFS_REGION and NL80211_ATTR_REG_ALPHA2 remain unset.
> 
> Your previous revision -- which for WW settings used
> wiphy_apply_custom_regulatory() and *not* regulatory_hint() -- did not
> have that problem.
> 
> > > Why are you ignoring SET_BY_DRIVER?
> >
> > Since the notification with NL80211_REGDOM_SET_BY_DRIVER flag might
> > comes from an another chipset's regulatory_hint().
> 
> Ack.
> 
> Brian
> 
> ------Please consider the environment before printing this e-mail.
diff mbox series

Patch

diff --git a/drivers/net/wireless/realtek/rtw88/Kconfig b/drivers/net/wireless/realtek/rtw88/Kconfig
index 33bd7ed797ff..04b84ec1dfc1 100644
--- a/drivers/net/wireless/realtek/rtw88/Kconfig
+++ b/drivers/net/wireless/realtek/rtw88/Kconfig
@@ -52,4 +52,14 @@  config RTW88_DEBUGFS
 
 	  If unsure, say Y to simplify debug problems
 
+config RTW88_REGD_USER_REG_HINTS
+	bool "Realtek rtw88 user regulatory hints"
+	depends on RTW88_CORE
+	default n
+	help
+	  Enable regulatoy user hints
+
+	  If unsure, say N. This should only be allowed on distributions
+	  that need this to correct the regulatory.
+
 endif
diff --git a/drivers/net/wireless/realtek/rtw88/main.c b/drivers/net/wireless/realtek/rtw88/main.c
index 7640e97706f5..5d43bef91a3c 100644
--- a/drivers/net/wireless/realtek/rtw88/main.c
+++ b/drivers/net/wireless/realtek/rtw88/main.c
@@ -1501,8 +1501,9 @@  int rtw_register_hw(struct rtw_dev *rtwdev, struct ieee80211_hw *hw)
 		return ret;
 	}
 
-	if (regulatory_hint(hw->wiphy, rtwdev->regd.alpha2))
-		rtw_err(rtwdev, "regulatory_hint fail\n");
+	ret = regulatory_hint(hw->wiphy, rtwdev->efuse.country_code);
+	if (ret)
+		rtw_warn(rtwdev, "failed to hint regulatory: %d\n", ret);
 
 	rtw_debugfs_init(rtwdev);
 
diff --git a/drivers/net/wireless/realtek/rtw88/regd.c b/drivers/net/wireless/realtek/rtw88/regd.c
index 69744dd65968..4cc1234bfb9a 100644
--- a/drivers/net/wireless/realtek/rtw88/regd.c
+++ b/drivers/net/wireless/realtek/rtw88/regd.c
@@ -339,12 +339,34 @@  static struct rtw_regulatory rtw_regd_find_reg_by_name(char *alpha2)
 	return rtw_defined_chplan;
 }
 
+static bool rtw_regd_is_ww(struct rtw_regulatory *reg)
+{
+	if (reg->txpwr_regd == RTW_REGD_WW)
+		return true;
+
+	return false;
+}
+
 static int rtw_regd_notifier_apply(struct rtw_dev *rtwdev,
 				   struct wiphy *wiphy,
 				   struct regulatory_request *request)
 {
-	if (request->initiator == NL80211_REGDOM_SET_BY_USER)
+	if (request->initiator == NL80211_REGDOM_SET_BY_DRIVER)
+		return -ENOTSUPP;
+
+	if (request->initiator == NL80211_REGDOM_SET_BY_USER &&
+	    !IS_ENABLED(CONFIG_RTW88_REGD_USER_REG_HINTS))
+		return -EPERM;
+
+	if (request->initiator == NL80211_REGDOM_SET_BY_CORE) {
+		char *country_code;
+
+		/* return to the efuse setting */
+		country_code = rtwdev->efuse.country_code;
+		rtwdev->regd = rtw_regd_find_reg_by_name(country_code);
 		return 0;
+	}
+
 	rtwdev->regd = rtw_regd_find_reg_by_name(request->alpha2);
 	rtw_regd_apply_world_flags(wiphy, request->initiator);
 
@@ -352,16 +374,20 @@  static int rtw_regd_notifier_apply(struct rtw_dev *rtwdev,
 }
 
 static int
-rtw_regd_init_wiphy(struct rtw_regulatory *reg, struct wiphy *wiphy,
+rtw_regd_init_wiphy(struct rtw_dev *rtwdev, struct wiphy *wiphy,
 		    void (*reg_notifier)(struct wiphy *wiphy,
 					 struct regulatory_request *request))
 {
-	wiphy->reg_notifier = reg_notifier;
+	struct rtw_regulatory *reg = &rtwdev->regd;
 
-	wiphy->regulatory_flags &= ~REGULATORY_CUSTOM_REG;
-	wiphy->regulatory_flags &= ~REGULATORY_STRICT_REG;
-	wiphy->regulatory_flags &= ~REGULATORY_DISABLE_BEACON_HINTS;
+	if (rtw_regd_is_ww(reg)) {
+		/* "00" implies worldwide in stack */
+		rtwdev->efuse.country_code[0] = '0';
+		rtwdev->efuse.country_code[1] = '0';
+	}
 
+	wiphy->reg_notifier = reg_notifier;
+	wiphy->regulatory_flags |= REGULATORY_STRICT_REG;
 	rtw_regd_apply_hw_cap_flags(wiphy);
 
 	return 0;
@@ -377,7 +403,7 @@  int rtw_regd_init(struct rtw_dev *rtwdev,
 		return -EINVAL;
 
 	rtwdev->regd = rtw_regd_find_reg_by_name(rtwdev->efuse.country_code);
-	rtw_regd_init_wiphy(&rtwdev->regd, wiphy, reg_notifier);
+	rtw_regd_init_wiphy(rtwdev, wiphy, reg_notifier);
 
 	return 0;
 }
@@ -387,12 +413,20 @@  void rtw_regd_notifier(struct wiphy *wiphy, struct regulatory_request *request)
 	struct ieee80211_hw *hw = wiphy_to_ieee80211_hw(wiphy);
 	struct rtw_dev *rtwdev = hw->priv;
 	struct rtw_hal *hal = &rtwdev->hal;
+	int ret;
+
+	ret = rtw_regd_notifier_apply(rtwdev, wiphy, request);
+	if (ret) {
+		rtw_warn(rtwdev, "failed to apply regulatory from initiator %d: %d\n",
+			 request->initiator, ret);
+		return;
+	}
 
-	rtw_regd_notifier_apply(rtwdev, wiphy, request);
 	rtw_dbg(rtwdev, RTW_DBG_REGD,
 		"get alpha2 %c%c from initiator %d, mapping to chplan 0x%x, txregd %d\n",
-		request->alpha2[0], request->alpha2[1], request->initiator,
-		rtwdev->regd.chplan, rtwdev->regd.txpwr_regd);
+		request->alpha2[0], request->alpha2[1],
+		request->initiator, rtwdev->regd.chplan,
+		rtwdev->regd.txpwr_regd);
 
 	rtw_phy_set_tx_power_level(rtwdev, hal->current_channel);
 }