| Message ID | 20180311214751.12769-1-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
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
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. >> 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 >> + 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. Anyways the overhead is small and this only runs once, so I will drop the check for v2. >> + 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? > 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? 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 ? >> + 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. 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 ? Regards, Hans
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
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). 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? > 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. 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. >>>> 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. Regards, Hans
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
Hi, On 16-03-18 21:41, Arend van Spriel wrote: > 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. Ok. >>> 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. So as asked I've retried using "iw list" instead of "iwlist". here is the output on a brcmfmac43340 device when not associated to any AP. I've tried with ccode=ALL / XV / X2 the ccode seems to not matter: * 2412 MHz [1] (20.0 dBm) * 2417 MHz [2] (20.0 dBm) * 2422 MHz [3] (20.0 dBm) * 2427 MHz [4] (20.0 dBm) * 2432 MHz [5] (20.0 dBm) * 2437 MHz [6] (20.0 dBm) * 2442 MHz [7] (20.0 dBm) * 2447 MHz [8] (20.0 dBm) * 2452 MHz [9] (20.0 dBm) * 2457 MHz [10] (20.0 dBm) * 2462 MHz [11] (20.0 dBm) * 2467 MHz [12] (20.0 dBm) * 2472 MHz [13] (20.0 dBm) * 2484 MHz [14] (disabled) * 5170 MHz [34] (disabled) * 5180 MHz [36] (20.0 dBm) * 5190 MHz [38] (20.0 dBm) * 5200 MHz [40] (20.0 dBm) * 5210 MHz [42] (20.0 dBm) * 5220 MHz [44] (20.0 dBm) * 5230 MHz [46] (20.0 dBm) * 5240 MHz [48] (20.0 dBm) * 5260 MHz [52] (20.0 dBm) (radar detection) * 5280 MHz [56] (20.0 dBm) (radar detection) * 5300 MHz [60] (20.0 dBm) (radar detection) * 5320 MHz [64] (20.0 dBm) (radar detection) * 5500 MHz [100] (20.0 dBm) (radar detection) * 5520 MHz [104] (20.0 dBm) (radar detection) * 5540 MHz [108] (20.0 dBm) (radar detection) * 5560 MHz [112] (20.0 dBm) (radar detection) * 5580 MHz [116] (20.0 dBm) (radar detection) * 5600 MHz [120] (20.0 dBm) (radar detection) * 5620 MHz [124] (20.0 dBm) (radar detection) * 5640 MHz [128] (20.0 dBm) (radar detection) * 5660 MHz [132] (20.0 dBm) (radar detection) * 5680 MHz [136] (20.0 dBm) (radar detection) * 5700 MHz [140] (20.0 dBm) (radar detection) * 5720 MHz [144] (disabled) * 5745 MHz [149] (disabled) * 5765 MHz [153] (disabled) * 5785 MHz [157] (disabled) * 5805 MHz [161] (disabled) * 5825 MHz [165] (disabled) After connecting to an AP at channel 13 (which requires ccode=XV or ccode=X2 to be visible) this changes to: * 2412 MHz [1] (20.0 dBm) * 2417 MHz [2] (20.0 dBm) * 2422 MHz [3] (20.0 dBm) * 2427 MHz [4] (20.0 dBm) * 2432 MHz [5] (20.0 dBm) * 2437 MHz [6] (20.0 dBm) * 2442 MHz [7] (20.0 dBm) * 2447 MHz [8] (20.0 dBm) * 2452 MHz [9] (20.0 dBm) * 2457 MHz [10] (20.0 dBm) * 2462 MHz [11] (20.0 dBm) * 2467 MHz [12] (20.0 dBm) * 2472 MHz [13] (20.0 dBm) * 2484 MHz [14] (20.0 dBm) * 5170 MHz [34] (20.0 dBm) * 5180 MHz [36] (20.0 dBm) * 5190 MHz [38] (20.0 dBm) * 5200 MHz [40] (20.0 dBm) * 5210 MHz [42] (20.0 dBm) * 5220 MHz [44] (20.0 dBm) * 5230 MHz [46] (20.0 dBm) * 5240 MHz [48] (20.0 dBm) * 5260 MHz [52] (20.0 dBm) * 5280 MHz [56] (20.0 dBm) * 5300 MHz [60] (20.0 dBm) * 5320 MHz [64] (20.0 dBm) * 5500 MHz [100] (20.0 dBm) * 5520 MHz [104] (20.0 dBm) * 5540 MHz [108] (20.0 dBm) * 5560 MHz [112] (20.0 dBm) * 5580 MHz [116] (20.0 dBm) * 5600 MHz [120] (20.0 dBm) * 5620 MHz [124] (20.0 dBm) * 5640 MHz [128] (20.0 dBm) * 5660 MHz [132] (20.0 dBm) * 5680 MHz [136] (20.0 dBm) * 5700 MHz [140] (20.0 dBm) * 5720 MHz [144] (20.0 dBm) * 5745 MHz [149] (20.0 dBm) * 5765 MHz [153] (20.0 dBm) * 5785 MHz [157] (20.0 dBm) * 5805 MHz [161] (20.0 dBm) * 5825 MHz [165] (20.0 dBm) So it seems that once connected for some reason all channels report the same flags, maybe a driver bug? >> 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. Or not (not a different topic), while running the above tests with ccode=XV vs ccode=X2 I noticed something very interesting. Although the reported channels in iwlist do not change, the APs seem while scanning do change. With ccode=X2 I can see both my home AP at channel 13 and the 5GHz channel of my home ap (chan 100). So it seems that with the brcmfmac43340 + current linux-firmware we need ccode=X2. This might be related to: https://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware.git/commit/brcm?id=ffdec3f6a5f29eb8a848b6a2417e0a1b45d32fcc I guess? So to fix this I have gone ahead for now (in my personal tree) with adding a firmware-filename -> ccode fixup table. I will post a v2 of the patch with this (I know this is likely not what you want, but it helps illustrate the problem). Note that with the brcmfmac43241b4-sdio.bin firmware ccode=XV works fine and does see 5GHz APs. > Could you try iw (which uses nl80211) instead of iwlist (which is old WEXT cruft). See above. Regards, Hans
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 }; + 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; + + 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. + */ + 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 */ + release_firmware(fw); if (!nvram && !(fwctx->flags & BRCMF_FW_REQ_NV_OPTIONAL)) goto fail;