Re: [PATCH] brcmfmac: Add support for getting nvram contents from EFI variables

[Arend van Spriel <arend.vanspriel@broadcom.com>]

On 3/14/2018 9:43 AM, Hans de Goede wrote:
> Hi,
>
> On 13-03-18 21:19, Arend van Spriel wrote:
>> On 3/12/2018 10:45 AM, Hans de Goede wrote:
>
> <snip>
>
>>>> Actually had a Sony device with nvram in EFI. Why not just drop this
>>>> optimization.
>>>
>>> Ok, do you know if that variable had the same name and guid though ?
>>> Because
>>> if it doesn't then this code is not going to work for the Sony case.
>>
>> If I am not mistaken the name and guid are defined by Broadcom/Microsoft.
>>
>>> Anyways the overhead is small and this only runs once, so I will drop
>>> the
>>> check for v2.
>>
>> Due to XV issue we may want to keep the check for now.
>
> If we're going to do ccode=ALL replacement based on a dmi-model
> table then there is no need to keep the check just for the XV stuff.
>
> <snip>
>
>>>> Also simply selecting XV instead is not correct. There is not just one
>>>> worldwide domain in the regulatory database of the firmware image.
>>>
>>> Right, I've read elsewhere that "X2" is the right magic value to use and
>>> I've tested that on some other devices and that does seem to work.
>>>
>>> I've also seen "XY" but the other Asus devices all use "XV" and that
>>> works (makes channel 13 work) so it seemed like a good value.
>>>
>>> Can you help me understand this problem a bit better? Is the problem
>>> with
>>> "XV" that it may not work with all firmware versions, so on some
>>> firmware
>>> versions it will be as bad as using ALL which the firmware also does not
>>> understand? Or do all firmwares understand XV / XY / X2 but are there
>>> subtle differences?
>>
>> The firmware has a per-device recipe of what should be in the
>> regulatory database and per release branch it can differ. So for the
>> same device customer A could get XV and XY in the firmware regdb and
>> customer B could get XY and X2.
>
> Hmm, so whether XV, XY and/or X2 works depends on the firmware used,
> not on the model of the laptop? That means that:
>
>>> So how do you suggest we deal with this?
>>>
>>> One solution I see is:
>>>
>>> 1) check for ccode=ALL
>>> 2) if found use DMI strings to match the specific model and set a
>>> different
>>> ccode based on the model (so for now use XV for the T200TA only)
>>> 3) if found and the model is not known, warn about this and do nothing
>>>
>>> Would that work for you ?
>>
>> I think so.
>
> This is no good, because the model of the laptop and which firmware build
> gets used are not really coupled. I think instead it would make more sense
> to assume the firmware builds from linux-firmware and have a table of
> which ccode=ALL override to use based on wifi-chip model, so in the
> case of the T200TA, map brcmfmac43340-sdio to a ccode=XV override
> (if ccode=ALL is present).

For our customers, ie. OEM/ODM, we provide a particular device and with 
it comes a firmware build with a regulatory database aka CLM. So in that 
project flow the laptop/phone/whatever model and firmware are really 
coupled. However, for linux-firmware the story is more as you depict it, 
because we simply do not know in what kind of device the chip will be used.

> So basically what I'm suggesting is:
>
> static const char * const ccode_all_map[][2] = {
>         { "brcm/brcmfmac43241b4-sdio.txt", "XV\n" }, /* Tested on Asus
> T100TA, T100CHI */
>         { "brcm/brcmfmac43340-sdio.txt", "XV\n" },   /* Tested on Asus
> T200TA */
> };
>
> ...
>
>      ccode = strnstr((char *)data, "ccode=ALL", data_len);
>      if (ccode) {
>          /* lookup fwctx->nvram_name in ccode_all_map */
>          /* if found patch in override string */
>          /* else brcmf_info("EFI nvram contains ccode=ALL and %s is
> missing from ccode-map, please report\n", fwctx->nvram_name) */
>      }
>
> So we actually decide what to replace all with based on the
> firmware name, rather then on the laptop model, does that make
> sense?

Somewhat. However, I am leaning to a different approach. The ALL country 
code should not be supported in the firmware so it would fallback to 
something else and I would like to know what. The country code can be 
retrieved and set using firmware command. So I would like to retrieve it 
in brcmf_cfg80211_attach() just before doing the 
brcmf_setup_wiphy_bands() call. I want to know if it returns ALL or some 
other fallback country code. At this stage I am not sure what the 
criteria has to be to set the country code to XV.

>> I hope to get some more clarification from our regulatory team about
>> the use of ALL and XV. Could you tell me what happens with T200TA when
>> you leave ccode=ALL in place. What output do you get from "iw list"?
>> Only channels 1 to 11 and no 5G? Or does it only have 2G.
>
> With ccode=ALL in place, I do see channel 13, but not 14 and channel 13
> does
> not work (the machine does not associate with my AP which is configured
> at chan 13)
> if I change it to "XV" then channel 13 does work, and shortly after
> associating
> channel 14 also shows up in the "iwlist wlan0 freq" output.

So XV seems to be the worldwide country code used for PC-OEMs. So that 
seems ok-ish. I would like to verify whether the firmwares released to 
linux-firmware all have XV in the firmware regulatory database.

Channel 14 is only applicable for Japan as far as I know. So that is 
weird unless your AP has JP in its beacon.

> As for 5GHz on the T200TA that is really a different topic, I can access
> 5GHz wifi
> under Windows but not under Linux, the channels are there in "iwlist
> wlan0 freq"
> but "wlist wlan0 scan" only shows 2.4 GHz APs. I've tried replacing the
> nvram
> with the file from the Windows partition referenced by the .inf file there,
> but that does not help. I'm not sure yet if this is a firmware / nvram /
> driver
> problem, so as said this really is a different topic.

Could you try iw (which uses nl80211) instead of iwlist (which is old 
WEXT cruft).

>>>>>       if (raw_nvram)
>>>>> -        bcm47xx_nvram_release_contents(data);
>>>>> +        kvfree(data); /* vfree for bcm47xx case / kfree for efi
>>>>> case */
>>>>
>>>> While this clearly works I can not say I like this. If you want to do
>>>> it this way, please submit a patch to remove
>>>> bcm47xx_nvram_release_contents() as it is no longer needed since we
>>>> now have kvfree(). From maintainance perspective also drop the postfix
>>>> comment. We just might end up doing vmalloc in efi case at some point
>>>> and this would likely be forgotten.
>>>
>>> It might be better if I replace the raw_nvram variable which is poorly
>>> named with
>>> a "bool free_bcm47xx_nvram = false;" and add a "bool kfree_nvram =
>>> false;"
>>>
>>> Would that work for you ?
>>
>> Sure.
>
> Ok, I will do that for v2 then as soon as we've figured out how to deal
> with
> the ccode=ALL issue.

Let me prep a patch for obtaining and possibly setting the country code 
in brcmf_cfg80211_attach().

Regards,
Arend