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

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

On 3/12/2018 10:45 AM, Hans de Goede wrote:
> Hi,
>
> On 12-03-18 09:55, Arend van Spriel wrote:
>> On 3/11/2018 10:47 PM, Hans de Goede wrote:
>>> Various models Asus laptops with a SDIO attached brcmfmac wifi chip,
>>> store
>>> the nvram contents in a special EFI variable. This commit adds
>>> support for
>>> getting nvram directly from this EFI variable, without the user
>>> needing to
>>> manually copy it.
>>>
>>> This makes Wifi / Bluetooth work out of the box on these devices
>>> instead of
>>> requiring manual setup.
>>>
>>> Note that at least on the Asus T200TA the nvram from the EFI variable
>>> wrongly contains "ccode=ALL" which the firmware does not understand, the
>>> code to fetch the nvram from the EFI variable will fix this up to:
>>> "ccode=XV" which is the correct way to specify the worldwide broadcast
>>> regime.
>>>
>>> This has been tested on the following models: Asus T100CHI, Asus T100HA,
>>> Asus T100TA and Asus T200TA
>>
>> Hi Hans,
>>
>> I like to have this as well, but .... (see below)
>>
>>> Tested-by: Hans de Goede <hdegoede@redhat.com>
>>
>> Duh. I would expect anyone to have tested their patches although you
>> can not cover every system out there obviously :-p
>
> Heh, I added it in this case as I went to the trouble of finding 4 devices
> which use this and test it on all 4 devices.

Not really a problem, but it looked funny :-)

>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>> ---
>>>   .../broadcom/brcm80211/brcmfmac/firmware.c         | 68
>>> +++++++++++++++++++++-
>>>   1 file changed, 67 insertions(+), 1 deletion(-)
>>>
>>> diff --git
>>> a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
>>> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
>>> index 091b52979e03..cbac407ae132 100644
>>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
>>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
>>> @@ -14,6 +14,8 @@
>>>    * CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
>>>    */
>>>
>>> +#include <linux/dmi.h>
>>> +#include <linux/efi.h>
>>>   #include <linux/kernel.h>
>>>   #include <linux/slab.h>
>>>   #include <linux/device.h>
>>> @@ -446,6 +448,67 @@ struct brcmf_fw {
>>>                void *nvram_image, u32 nvram_len);
>>>   };
>>>
>>> +#ifdef CONFIG_EFI
>>> +static u8 *brcmf_fw_nvram_from_efi(size_t *data_len_ret)
>>> +{
>>> +    const u16 name[] = { 'n', 'v', 'r', 'a', 'm', 0 };
>>
>> Isn't there some helper function to make this conversion to u16 array?
>> Hmm, maybe it is my itch to scratch later :-)
>
> Nope, I copied this style from existing code under drivers/firmware/efi

Maybe I add a helper function at some point.

>>> +    struct efivar_entry *nvram_efivar;
>>> +    unsigned long data_len = 0;
>>> +    u8 *data = NULL;
>>> +    char *ccode;
>>> +    int err;
>>> +
>>> +    /* So far only Asus devices store the nvram in an EFI var */
>>> +    if (!dmi_name_in_vendors("ASUSTeK COMPUTER INC."))
>>> +        return NULL;
>>
>> 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.

>>> +    nvram_efivar = kzalloc(sizeof(*nvram_efivar), GFP_KERNEL);
>>> +    if (!nvram_efivar)
>>> +        return NULL;
>>> +
>>> +    memcpy(&nvram_efivar->var.VariableName, name, sizeof(name));
>>> +    nvram_efivar->var.VendorGuid = EFI_GUID(0x74b00bd9, 0x805a, 0x4d61,
>>> +                        0xb5, 0x1f, 0x43, 0x26,
>>> +                        0x81, 0x23, 0xd1, 0x13);
>>> +
>>> +    err = efivar_entry_size(nvram_efivar, &data_len);
>>> +    if (err)
>>> +        goto fail;
>>> +
>>> +    data = kmalloc(data_len, GFP_KERNEL);
>>> +    if (!data)
>>> +        goto fail;
>>> +
>>> +    err = efivar_entry_get(nvram_efivar, NULL, &data_len, data);
>>> +    if (err)
>>> +        goto fail;
>>> +
>>> +    /* In some cases the EFI-var stored nvram contains "ccode=ALL" but
>>> +     * the firmware does not understand "ALL" change this to "XV" which
>>> +     * is the correct way to specify the "worldwide" compatible
>>> settings.
>>> +     */
>>
>> This is actually quite bad. The ALL ccode should never end up on the
>> market as it is intended for internal use only during bringup project
>> phase so these seems to be programmed wrong.
>
> I see lots of cheap Windows 10 and ARM devices with brcmfmac sdio wifi
> which ship with on disk nvram files using the "ALL" ccode. I actually have
> pinned my home wifi at channel 13 to catch this, because channel 13 does
> not work with the ALL ccode. If I understand correctly the worldwide
> domain starts with channel 13/14 in passive/listen only mode and allows
> using them once it has seen valid wifi traffic on them, so they do allow
> communicating with an AP on channel 13 here in the Netherlands where that
> is allowed?

*sigh* (regarding ALL being shipped)

>> 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.

> 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. 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.

>>>       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.

Regards,
Arend