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

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

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

> 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 :-)

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

> +	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. Also simply selecting XV 
instead is not correct. There is not just one worldwide domain in the 
regulatory database of the firmware image.

> +	ccode = strnstr((char *)data, "ccode=ALL", data_len);
> +	if (ccode) {
> +		ccode[6] = 'X';
> +		ccode[7] = 'V';
> +		ccode[8] = '\n';
> +	}
> +
> +	brcmf_info("Using nvram EFI variable\n");
> +
> +	kfree(nvram_efivar);
> +	*data_len_ret = data_len;
> +	return data;
> +
> +fail:
> +	kfree(data);
> +	kfree(nvram_efivar);
> +	return NULL;
> +}
> +#else
> +static u8 *brcmf_fw_nvram_from_efi(size_t *data_len) { return NULL; }
> +#endif
> +
>   static void brcmf_fw_request_nvram_done(const struct firmware *fw, void *ctx)
>   {
>   	struct brcmf_fw *fwctx = ctx;
> @@ -462,6 +525,8 @@ static void brcmf_fw_request_nvram_done(const struct firmware *fw, void *ctx)
>   		raw_nvram = false;
>   	} else {
>   		data = bcm47xx_nvram_get_contents(&data_len);
> +		if (!data)
> +			data = brcmf_fw_nvram_from_efi(&data_len);
>   		if (!data && !(fwctx->flags & BRCMF_FW_REQ_NV_OPTIONAL))
>   			goto fail;
>   		raw_nvram = true;
> @@ -472,7 +537,8 @@ static void brcmf_fw_request_nvram_done(const struct firmware *fw, void *ctx)
>   					     fwctx->domain_nr, fwctx->bus_nr);
>
>   	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.

Regards,
Arend