Re: [PATCH V6 4/3] brcmfmac: use wiphy_read_of_freq_limits to respect extra limits

On 5 January 2017 at 11:02, Rafa=C5=82 Mi=C5=82ecki [off-list ref] wrot=
e:

On 5 January 2017 at 10:31, Arend Van Spriel
[off-list ref] wrote:

quoted

On 4-1-2017 22:19, Rafa=C5=82 Mi=C5=82ecki wrote:

quoted

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.

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
(reg.c):
pr_debug("Disabling freq %d MHz for good\n", chan->center_freq);
chan->orig_flags |=3D 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.

I was wrong about this. Current notifier implementation in brcmfmac
doesn't care about "orig_flags" and it just sets "flags" as it wants.
This way it can enable even these channels that were believed to be
disabled for good (DISABLED in "orig_flags").

For my test I booted SR400ac with ccode & regrev set to values that
didn't allow me to use channels 149 - 165 (5735 - 5835). It matches my
current country:
country PL: DFS-ETSI
        (2402 - 2482 @ 40), (20)
        (5170 - 5250 @ 80), (20), AUTO-BW
        (5250 - 5330 @ 80), (20), DFS, AUTO-BW
        (5490 - 5710 @ 160), (27), DFS
        # 60 GHz band channels 1-4, ref: Etsi En 302 567
        (57000 - 66000 @ 2160), (40)

Then I used "iw reg set ??" to set country that allows these channels.
My locally modified brcmfmac driver sent proper "country" iovar to the
firmware and queried it for the updated "chanspecs". See my debugging
messages below:
brcmfmac: [brcmf_construct_chaninfo] hw_value:34 orig_flags:0x101 flags:0x0=
01
brcmfmac: [brcmf_construct_chaninfo] hw_value:36 orig_flags:0x1a0 flags:0x0=
a0
brcmfmac: [brcmf_construct_chaninfo] hw_value:38 orig_flags:0x101 flags:0x0=
01
brcmfmac: [brcmf_construct_chaninfo] hw_value:40 orig_flags:0x190 flags:0x0=
90
brcmfmac: [brcmf_construct_chaninfo] hw_value:42 orig_flags:0x101 flags:0x0=
01
brcmfmac: [brcmf_construct_chaninfo] hw_value:44 orig_flags:0x1a0 flags:0x0=
a0
brcmfmac: [brcmf_construct_chaninfo] hw_value:46 orig_flags:0x101 flags:0x0=
01
brcmfmac: [brcmf_construct_chaninfo] hw_value:48 orig_flags:0x190 flags:0x0=
90
brcmfmac: [brcmf_construct_chaninfo] hw_value:52 orig_flags:0x1aa flags:0x0=
aa
brcmfmac: [brcmf_construct_chaninfo] hw_value:56 orig_flags:0x19a flags:0x0=
9a
brcmfmac: [brcmf_construct_chaninfo] hw_value:60 orig_flags:0x1aa flags:0x0=
aa
brcmfmac: [brcmf_construct_chaninfo] hw_value:64 orig_flags:0x19a flags:0x0=
9a
brcmfmac: [brcmf_construct_chaninfo] hw_value:100 orig_flags:0x1aa flags:0x=
001
brcmfmac: [brcmf_construct_chaninfo] hw_value:104 orig_flags:0x19a flags:0x=
001
brcmfmac: [brcmf_construct_chaninfo] hw_value:108 orig_flags:0x1aa flags:0x=
001
brcmfmac: [brcmf_construct_chaninfo] hw_value:112 orig_flags:0x19a flags:0x=
001
brcmfmac: [brcmf_construct_chaninfo] hw_value:116 orig_flags:0x1aa flags:0x=
001
brcmfmac: [brcmf_construct_chaninfo] hw_value:120 orig_flags:0x19a flags:0x=
001
brcmfmac: [brcmf_construct_chaninfo] hw_value:124 orig_flags:0x1aa flags:0x=
001
brcmfmac: [brcmf_construct_chaninfo] hw_value:128 orig_flags:0x19a flags:0x=
001
brcmfmac: [brcmf_construct_chaninfo] hw_value:132 orig_flags:0x1aa flags:0x=
001
brcmfmac: [brcmf_construct_chaninfo] hw_value:136 orig_flags:0x19a flags:0x=
001
brcmfmac: [brcmf_construct_chaninfo] hw_value:140 orig_flags:0x1ba flags:0x=
001
brcmfmac: [brcmf_construct_chaninfo] hw_value:144 orig_flags:0x101 flags:0x=
001
brcmfmac: [brcmf_construct_chaninfo] hw_value:149 orig_flags:0x101 flags:0x=
0a0
brcmfmac: [brcmf_construct_chaninfo] hw_value:153 orig_flags:0x101 flags:0x=
090
brcmfmac: [brcmf_construct_chaninfo] hw_value:157 orig_flags:0x101 flags:0x=
0a0
brcmfmac: [brcmf_construct_chaninfo] hw_value:161 orig_flags:0x101 flags:0x=
090
brcmfmac: [brcmf_construct_chaninfo] hw_value:165 orig_flags:0x101 flags:0x=
0b0

As you can see brcmfmac dropped IEEE80211_CHAN_DISABLED (hint: 0x1)
for channels 149 - 165 even though they were disabled according to the
"orig_flags".

So this patch with its
if (channel->orig_flags & IEEE80211_CHAN_DISABLED)
        continue;
could be considered some kind of regression. My change wouldn't allow
enabling such channels anymore.

Of course noone would notice this as noone uses country_codes table
yet I guess (except for me locally). Anyway, this patch should be
reworked to make sure it still allows implementing support for
country_codes tables in the future.