[BUG] Re: [PATCH] brcmfmac: use ISO3166 country code and 0 rev as fallback

[Shawn Guo]

On Thu, Sep 09, 2021 at 10:39:58AM +0200, Soeren Moch wrote:
> Hi Shawn,
> 
> On 09.09.21 04:20, Shawn Guo wrote:
> > On Wed, Sep 08, 2021 at 07:08:06AM +0200, Soeren Moch wrote:
> >> Hi Shawn,
> >>
> >> On 08.09.21 03:00, Shawn Guo wrote:
> >>> Hi Soeren,
> >>>
> >>> On Tue, Sep 07, 2021 at 09:22:52PM +0200, Soeren Moch wrote:
> >>>> On 25.04.21 13:02, Shawn Guo wrote:
> >>>>> Instead of aborting country code setup in firmware, use ISO3166 country
> >>>>> code and 0 rev as fallback, when country_codes mapping table is not
> >>>>> configured.  This fallback saves the country_codes table setup for recent
> >>>>> brcmfmac chipsets/firmwares, which just use ISO3166 code and require no
> >>>>> revision number.
> >>>> This patch breaks wireless support on RockPro64. At least the access
> >>>> point is not usable, station mode not tested.
> >>>>
> >>>> brcmfmac: brcmf_c_preinit_dcmds: Firmware: BCM4359/9 wl0: Mar  6 2017
> >>>> 10:16:06 version 9.87.51.7 (r686312) FWID 01-4dcc75d9
> >>>>
> >>>> Reverting this patch makes the access point show up again with linux-5.14 .
> >>> Sorry for breaking your device!
> >>>
> >>> So it sounds like you do not have country_codes configured for your
> >>> BCM4359/9 device, while it needs particular `rev` setup for the ccode
> >>> you are testing with.  It was "working" likely because you have a static
> >>> `ccode` and `regrev` setting in nvram file.
> >> It always has been a mystery to me how country codes are configured for
> >> this device. Before I read your patch I did not even know that a
> >> translation table is required. Is there some documentation how this is
> >> supposed to work? Not sure if this makes a difference, BCM4359/9 is a
> >> Cypress device I think, I added mainline support for it some time ago.
> > One way to add the translation table is using DT.  You can find more
> > info and example in following commits:
> >
> > b41936227078 ("dt-bindings: bcm4329-fmac: add optional brcm,ccode-map")
> > 1a3ac5c651a0 ("brcmfmac: support parse country code map from DT")
> OK, thanks.
> When one way is to use DT, what is the 'traditional way' to add such table?

To be honest, I don't know what the 'traditional way' is, because I
haven't seen how `country_codes` is used before I add DT support of it.

> And maybe the more interesting question, where can these settings be
> obtained from? The tweaked device specific settings probably from the
> device vendor, good luck!
> But the general country specific settings, as you are obviously
> interested in with your trivial mapping, shouldn't they go into driver
> directly? Only to be overruled when device specific settings are
> available via DT? And of course only for device/firmware combinations
> that support this general mapping, so that other devices with 'unknown
> mapping' are not broken by this enhancement?

The patch was accepted based on Arend's assumption[1] that every
chipset/firmware include a rev 0 for each country code , but
it's never been officially confirmed.  And from what you report here,
it doesn't seem to stand unfortunately.

[1] https://lore.kernel.org/netdev/17998013ac0.279b.9b12b7fc0a3841636cfb5e919b41b954@broadcom.com/

> >> I have installed different firmware files, brcmfmac4359-sdio.clm_blob,
> >> brcmfmac4359-sdio.bin, brcmfmac4359-sdio.txt, the latter also linked as
> >> brcmfmac4359-sdio.pine64,rockpro64-2.1.txt. This probably is the nvram
> >> file. ccode and regrev are set to zero, which probably means
> >> 'international save settings".
> > I'm not sure how this 'international save settings' works for brcmfmac
> > devices.  Do you have more info or any pointers?
> The correct term in this context probably is 'world regulatory domain',
> the most restrictive wifi settings that can be used all over the world.
> This usually is taken as default by cfg80211, apparently also for
> (some?) brcmfmac devices/firmwares.
> 
> These 'world' settings can be replaced by more permissive country
> specific regulatory domain settings, but for brcmfmac devices this seems
> to be firmware specific and requires this country mapping.
> 
> I have seen a country code "00" for the world regulatory domain in the
> past, not sure if this is standard or a device/driver/software specific
> hack and if this can be used for brcmfmac (mapping from string "00" to
> country_code=0 ?). For sure here are more experienced wifi developers
> who know better.

Yeah, it would be nice if someone can help clarify what both `ccode` and
`regrev` in nvram file being zero means, like what should be working and
what's not.

> >>> But roaming to a different
> >>> region will mostly get you a broken WiFi support.  Is it possible to set
> >>> up the country_codes for your device to get it work properly?
> >> In linux-5.13 it worked, probably with save settings (not all channels
> >> selectable, limited tx power), with linux-5.14 it stopped working, so it
> >> is a regression.
> >> I personally would like to learn how all this is configured properly.
> >> For general use I think save settings are better than no wifi at all
> >> with this patch. This fallback to ISO CC seams to work with newer
> >> (Synaptics?) devices only.
> > I do not mind you send a reverting if you have problem to add a proper
> > translation table for your device.  But that would mean I have to add
> > a pretty "meaningless" translation table for my devices :(
> >
> Is this not the usual DT policy, that missing optional properties should
> not prevent a device to work, that old dtbs should still work when new
> properties are added?
> 
> I'm not sure what's the best way forward. A plain revert of this patch
> would at least bring back wifi support for RockPro64 devices with
> existing dtbs. Maybe someone else has a better proposal how to proceed.

Go ahead to revert if we do not hear a better solution, I would say.

Shawn