Received-SPF: pass (google.com: best guess record for domain of linux-wireless-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Tue, 24 Mar 2020 09:51:05 -0700
From:   Brian Norris <briannorris@chromium.org>
To:     yhchuang@realtek.com
Cc:     kvalo@codeaurora.org, linux-wireless@vger.kernel.org,
        tehuang@realtek.com
Subject: Re: [PATCH v6 1/2] rtw88: add regulatory process strategy for
 different chipset
Message-ID: <20200324165105.GA99185@google.com>
References: <20200324075216.22553-1-yhchuang@realtek.com>
 <20200324075216.22553-2-yhchuang@realtek.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200324075216.22553-2-yhchuang@realtek.com>
User-Agent: Mutt/1.12.2 (2019-09-21)
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

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);
>