Devicetree

* Re: [PATCH V6 4/3] brcmfmac: use wiphy_read_of_freq_limits to respect extra limits
From: Arend Van Spriel @ 2017-01-07 12:52 UTC (permalink / raw)
  To: Rafał Miłecki
  Cc: Johannes Berg,
	linux-wireless-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	Martin Blumenstingl, Felix Fietkau, Arend van Spriel,
	Arnd Bergmann, devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	Rob Herring, Rafał Miłecki
In-Reply-To: <CACna6rw22benfuw_7BSFw1wedavmMJWTo_hfPLCVa1t0kV+Aqg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>

On 5-1-2017 11:02, Rafał Miłecki wrote:
> On 5 January 2017 at 10:31, Arend Van Spriel
> <arend.vanspriel-dY08KVG/lbpWk0Htik3J/w@public.gmane.org> wrote:
>> On 4-1-2017 22:19, Rafał Miłecki wrote:
>>> On 4 January 2017 at 21:07, Arend Van Spriel
>>> <arend.vanspriel-dY08KVG/lbpWk0Htik3J/w@public.gmane.org> wrote:
>>>> On 4-1-2017 18:58, Rafał Miłecki wrote:
>>>>> From: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>
>>>>>
>>>>> There are some devices (e.g. Netgear R8000 home router) with one chipset
>>>>> model used for different radios, some of them limited to subbands. NVRAM
>>>>> entries don't contain any extra info on such limitations and firmware
>>>>> reports full list of channels to us. We need to store extra limitation
>>>>> info in DT to support such devices properly.
>>>>>
>>>>> Now there is a cfg80211 helper for reading such info use it in brcmfmac.
>>>>> This patch adds check for channel being disabled with orig_flags which
>>>>> is how this wiphy helper and wiphy_register work.
>>>>>
>>>>> Signed-off-by: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>
>>>>> ---
>>>>> This patch should probably go through wireless-driver-next which is why
>>>>> it got weird number 4/3. I'm sending it just as a proof of concept.
>>>>> It was succesfully tested on SmartRG SR400ac with BCM43602.
>>>>>
>>>>> V4: Respect IEEE80211_CHAN_DISABLED in orig_flags
>>>>> V5: Update commit message
>>>>> V6: Call wiphy_read_of_freq_limits after brcmf_setup_wiphybands to make it work
>>>>>     with helper setting "flags" instead of "orig_flags".
>>>>> ---
>>>>>  drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c | 9 ++++++++-
>>>>>  1 file changed, 8 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
>>>>> index ccae3bb..a008ba5 100644
>>>>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
>>>>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
>>>>> @@ -5886,6 +5886,9 @@ static int brcmf_construct_chaninfo(struct brcmf_cfg80211_info *cfg,
>>>>>                                                      band->band);
>>>>>               channel[index].hw_value = ch.control_ch_num;
>>>>>
>>>>> +             if (channel->orig_flags & IEEE80211_CHAN_DISABLED)
>>>>> +                     continue;
>>>>> +
>>>>
>>>> So to be clear this is still needed for subsequent calls to
>>>> brcmf_setup_wiphybands(). The subsequent calls are done from the
>>>> regulatory notifier. So I think we have an issue with this approach. Say
>>>> the device comes up with US. That would set DISABLED flags for channels
>>>> 12 to 14. With a country update to PL we would want to enable channels
>>>> 12 and 13, right? The orig_flags are copied from the initial flags
>>>> during wiphy_register() so it seems we will skip enabling 12 and 13. I
>>>> think we should remove the check above and call
>>>> wiphy_read_of_freq_limits() as a last step within
>>>> brcmf_setup_wiphybands(). It means it is called every time, but it
>>>> safeguards that the limits in DT are always applied.
>>>
>>> I'm not exactly happy with channels management in brcmfmac. Before
>>> calling wiphy_register it already disables channels unavailable for
>>> current country. This results in setting IEEE80211_CHAN_DISABLED in
>>
>> What do you mean by current country. There is none that we are aware off
>> in the driver. So we obtain the channels for the current
>> country/revision in the firmware and enable those before
>> wiphy_register(). This all is within the probe/init sequence so I do not
>> really see an issue. As the wiphy object is not yet registered there is
>> no user-space awareness
> 
> It seems I'm terrible as describing my patches/problems/ideas :( Here
> I used 1 inaccurate word and you couldn't understand my point.

Well. Because of our track record of miscommunication and other
annoyances I want to be sure this time :-p

> By "current country" I meant current region (and so a set of
> regulatory rules) used by the firmware. I believe firmware describes
> it using "ccode" and "regrev".
> 
> Now, about the issue I see:
> 
> AFAIU if you set IEEE80211_CHAN_DISABLED in "orig_flags" it's meant to
> be there for good. Some reference code that makes me believe so

Indeed. Admittedly, it is the first time I became aware of it when
Johannes brought it up.

> (reg.c):
> pr_debug("Disabling freq %d MHz for good\n", chan->center_freq);
> chan->orig_flags |= IEEE80211_CHAN_DISABLED;
> 
> This is what happens with brcmfmac right now. If firmware doesn't
> report some channels, you set "flags" to IEEE80211_CHAN_DISABLED for
> them. Then you call wiphy_register which copies that "flags" to the
> "orig_flags". I read it as: we are never going to use these channels.
> 
> Now, consider you support regdom change (I do with my local patches).
> You translate alpha2 to a proper firmware request (board specific!),
> you execute it and then firmware may allow you to use channels that
> you marked as disabled for good. You would need to mess with
> orig_flags to recover from this issue.
> 
> Does my explanation make more sense of this issue now?

Sure. It seems we should leave all channels enabled and disable them
after wiphy_register() based on firmware information. Or did you have
something else in mind. DFS channels may need to pass a feature check in
firmware and always be disabled otherwise.

>>> orig_flags of channels that may become available later, after country
>>> change. Please note it happens even right now, without this patch.
>>
>> Nope. As stated earlier the country setting in firmware is not updated
>> unless you provide a *proper* mapping of user-space country code to
>> firmware country code/revision. That is the reason, ie. firmware simply
>> returns the same list of channels as nothing changed from its
>> perspective. We may actually drop 11d support.
> 
> I implemented mapping support locally, this is the feature I'm talking about.

Ok. So this is not an upstream feature. Or are you getting the mapping
from DT?

>>> Maybe you can workaround this by ignoring orig_flags in custom
>>> regulatory code, but I'd just prefer to have it nicely handled in the
>>> first place.
>>
>> Please care to explain your ideas before putting any effort in this
>> "feature". As the author of the code that makes you unhappy and as
>> driver maintainer I would like to get a clearer picture of your point of
>> view. What exactly is the issue that makes you unhappy.
> 
> I meant that problem with "orig_flags" I described in the first
> paragraph. I wasn't trying to hide whatever issue I'm seeing, I swear
> ;)
> 
> 
>>> This is the next feature I'm going to work on. AFAIU this patch won't
>>> be applied for now (it's for wireless-drivers-next and we first need
>>> to get wiphy_read_of_freq_limits in that tree). By the time that
>>> happens I may have another patchset cleaning brcmfmac ready. And FWIW
>>> this patch wouldn't make things worse *at this point* as we don't
>>> really support country switching for any device yet.
>>
>> Now who is *we*? We as Broadcom can, because we know how to map the ISO
>> 3166-1 country code to firmware country code/revision for a specific
>> firmware release. Firmware uses its own regulatory rules which may
>> differ from what regdb has. Now I know Intel submitted a mechanism to
>> export firmware rules to regdb so maybe we should consider switching to
>> that api if that has been upstreamed. Need to check.
> 
> We as a driver developers. Please read
> "we don't really support country switching for any device yet"
> as
> "brcmfmac doesn't really support country switching for any device yet"
> 
> Does it help to get the context?

I indeed prefer to talk about the driver instead of we. Indeed it is
true due to the orig_flags behavior although that only seems to involve
regulatory code. Could it be that brcmfmac undo that through the notifier?

Regards,
Arend
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply