| Message ID | 8665E2433BC68541A24DFFCA87B70F5B363E1A3D@DFRE01.ent.ti.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
"Reizer, Eyal" <eyalr@ti.com> writes: > When working with wl18xx the nvs file is used for defining an alternate > mac address and override the default mac address that is stored inside > the wl18xx chip. > update the structure field with the same default nvs file name that has > been used in the past, otherwise userspace backward compatibility is > broken when upgrading from older kernels, as the alternate mac address > would not be read from the nvs that is already present in the file system > (lib/firmware/ti-connectivity/wl1271-nvs.bin) causing mac address change > of the wlan interface. > > Signed-off-by: Eyal Reizer <eyalr@ti.com> Should we also cc stable? And a Fixes line would be nice.
* Kalle Valo <kvalo@codeaurora.org> [170703 04:30]: > "Reizer, Eyal" <eyalr@ti.com> writes: > > > When working with wl18xx the nvs file is used for defining an alternate > > mac address and override the default mac address that is stored inside > > the wl18xx chip. > > update the structure field with the same default nvs file name that has > > been used in the past, otherwise userspace backward compatibility is > > broken when upgrading from older kernels, as the alternate mac address > > would not be read from the nvs that is already present in the file system > > (lib/firmware/ti-connectivity/wl1271-nvs.bin) causing mac address change > > of the wlan interface. > > > > Signed-off-by: Eyal Reizer <eyalr@ti.com> > > Should we also cc stable? And a Fixes line would be nice. Argh this mess again. I think there are few things to consider here. What about booting the same rootfs on multiple devices for example with NFSroot? Not sure if this really is a regression as we've always had a bogus wl1271-nvs.bin in linux-firmware.git. Sure would be nice to fix it, but going back to using a generic wl1271-nvs.bin sure does not seem like a good long term fix to me :) Isn't the nvs file supposed to be device specific? If so, we should not provide it in linux-firmware.git at all because of the issues it causes.. And since it's provided, how are people supposed to know to remove it from their file system and replace it with something board specific? Maybe add a check to first try to find wl18xx-nvs.bin if it exists (and not add it to linux-firmware.git), and if not found, then fall back to wl1271-nvs.bin, but only if it's not the bogus generic file.. Produce a warning if the bogux linux-firmware.git wl1271-nvs.bin is being used.. Regards, Tony
Hi Tony, > > * Kalle Valo <kvalo@codeaurora.org> [170703 04:30]: > > "Reizer, Eyal" <eyalr@ti.com> writes: > > > > > When working with wl18xx the nvs file is used for defining an alternate > > > mac address and override the default mac address that is stored inside > > > the wl18xx chip. > > > update the structure field with the same default nvs file name that has > > > been used in the past, otherwise userspace backward compatibility is > > > broken when upgrading from older kernels, as the alternate mac address > > > would not be read from the nvs that is already present in the file system > > > (lib/firmware/ti-connectivity/wl1271-nvs.bin) causing mac address change > > > of the wlan interface. > > > > > > Signed-off-by: Eyal Reizer <eyalr@ti.com> > > > > Should we also cc stable? And a Fixes line would be nice. > > Argh this mess again. I think there are few things to consider here. What > about booting the same rootfs on multiple devices for example with NFSroot? > > Not sure if this really is a regression as we've always had a bogus > wl1271-nvs.bin in linux-firmware.git. Sure would be nice to fix it, > but going back to using a generic wl1271-nvs.bin sure does not seem > like a good long term fix to me :) A lot of legacy here... Wl1271-nvs has been used mainly with wilink6/7 and indeed was device specific Holding calibration info etc. When they started with wilink8 the device specific configuration that was Part of it has switched to wl18xx-conf.bin and using the wlaconf tool for setting it. Also there is no calibration specific info per device with wilink8 so the wl18xx-conf.bin The only thing left in wl1271-nvs.bin for wilink8 was the mac address override. There are wilink8 customer using this feature which is the only reason for keeping This file for wilink8. So for wilink8 there is really not issue with having the same wl1271-nvs.bin On NFSroot. > > Isn't the nvs file supposed to be device specific? If so, we should not > provide it in linux-firmware.git at all because of the issues it causes.. > > And since it's provided, how are people supposed to know to remove it > from their file system and replace it with something board specific? I think the wl1271-nvs.bin should be removed from linux-firmware.git anyway. For wilink6/7 it is really device specific and for wilink8 if a customer Want an alternate mac address he can create this file on his file system. > > Maybe add a check to first try to find wl18xx-nvs.bin if it exists (and > not add it to linux-firmware.git), and if not found, then fall back to > wl1271-nvs.bin, but only if it's not the bogus generic file.. Produce > a warning if the bogux linux-firmware.git wl1271-nvs.bin is being used.. > Removing it from linux-firmware.git may be better instead of adding an additional check? People are already familiar with the wl1271-nvs.bin file as a mean for Mac address override for wilink8: http://processors.wiki.ti.com/index.php/WL18xx_Writing_MAC_address so I am not sure that a name change Here is really necessary and may only create confusion especially for Customers that already have products in the field and only updating Their kernel. Best Regards, Eyal
* Reizer, Eyal <eyalr@ti.com> [170704 01:47]: > Hi Tony, > > > > > * Kalle Valo <kvalo@codeaurora.org> [170703 04:30]: > > > "Reizer, Eyal" <eyalr@ti.com> writes: > > > > > > > When working with wl18xx the nvs file is used for defining an alternate > > > > mac address and override the default mac address that is stored inside > > > > the wl18xx chip. > > > > update the structure field with the same default nvs file name that has > > > > been used in the past, otherwise userspace backward compatibility is > > > > broken when upgrading from older kernels, as the alternate mac address > > > > would not be read from the nvs that is already present in the file system > > > > (lib/firmware/ti-connectivity/wl1271-nvs.bin) causing mac address change > > > > of the wlan interface. > > > > > > > > Signed-off-by: Eyal Reizer <eyalr@ti.com> > > > > > > Should we also cc stable? And a Fixes line would be nice. > > > > Argh this mess again. I think there are few things to consider here. What > > about booting the same rootfs on multiple devices for example with NFSroot? > > > > Not sure if this really is a regression as we've always had a bogus > > wl1271-nvs.bin in linux-firmware.git. Sure would be nice to fix it, > > but going back to using a generic wl1271-nvs.bin sure does not seem > > like a good long term fix to me :) > A lot of legacy here... > Wl1271-nvs has been used mainly with wilink6/7 and indeed was device specific > Holding calibration info etc. > When they started with wilink8 the device specific configuration that was > Part of it has switched to wl18xx-conf.bin and using the wlaconf tool for setting it. > Also there is no calibration specific info per device with wilink8 so the wl18xx-conf.bin > The only thing left in wl1271-nvs.bin for wilink8 was the mac address override. And the default wl1271-nvs.bin sets the mac address to a bogus deadbeef address, so it's wrong to use and totally broken for distros :( > There are wilink8 customer using this feature which is the only reason for keeping > This file for wilink8. > So for wilink8 there is really not issue with having the same wl1271-nvs.bin > On NFSroot. > > > > > Isn't the nvs file supposed to be device specific? If so, we should not > > provide it in linux-firmware.git at all because of the issues it causes.. > > > > And since it's provided, how are people supposed to know to remove it > > from their file system and replace it with something board specific? > > I think the wl1271-nvs.bin should be removed from linux-firmware.git > anyway. I agree. But distros carry it already, so we need to also add checks to the device driver to warn about the bogus wl1271-nvs.bin. And the driver should only attempt to use wl1271-nvs.bin if not the bogus one. > For wilink6/7 it is really device specific and for wilink8 if a customer > Want an alternate mac address he can create this file on his file system. Yes makes sense, but only with fixing the driver to ignore the bogus wl1271-nvs.bin. > > Maybe add a check to first try to find wl18xx-nvs.bin if it exists (and > > not add it to linux-firmware.git), and if not found, then fall back to > > wl1271-nvs.bin, but only if it's not the bogus generic file.. Produce > > a warning if the bogux linux-firmware.git wl1271-nvs.bin is being used.. > > > Removing it from linux-firmware.git may be better instead of adding an > additional check? I think we need both to avoid totally broken defaults. > People are already familiar with the wl1271-nvs.bin file as a mean for > Mac address override for wilink8: > http://processors.wiki.ti.com/index.php/WL18xx_Writing_MAC_address Yes so checking, ignoring and warning about the bogus one should solve your customer issues. And it will also fix the defaults for distros. > so I am not sure that a name change Well that can be done later. The problem of booting the same rootfs on multiple devices remains where having wl1271-nvs.bin breaks things. > Here is really necessary and may only create confusion especially for > Customers that already have products in the field and only updating > Their kernel. Yeah using wl1271-nvs.bin should be the last resort but only if properly configured. Regards, Tony
Hi, On Wed, Jul 05, 2017 at 01:06:54AM -0700, Tony Lindgren wrote: > > > Not sure if this really is a regression as we've always had a bogus > > > wl1271-nvs.bin in linux-firmware.git. Sure would be nice to fix it, > > > but going back to using a generic wl1271-nvs.bin sure does not seem > > > like a good long term fix to me :) > > A lot of legacy here... > > Wl1271-nvs has been used mainly with wilink6/7 and indeed was device specific > > Holding calibration info etc. > > When they started with wilink8 the device specific configuration that was > > Part of it has switched to wl18xx-conf.bin and using the wlaconf tool for setting it. > > Also there is no calibration specific info per device with wilink8 so the wl18xx-conf.bin > > The only thing left in wl1271-nvs.bin for wilink8 was the mac address override. > > And the default wl1271-nvs.bin sets the mac address to a bogus deadbeef address, > so it's wrong to use and totally broken for distros :( So use something like the following pseudo-code? if (fw->mac_address == deadbeef) { fw->mac_address = get_random_mac(); dev_warn(dev, "Detected unconfigured wl1271-nvs.bin.\n" "Your device will run with limited performance.\n" "Please use ti-utils to configure your device.\n"); } -- Sebastian
* Sebastian Reichel <sebastian.reichel@collabora.co.uk> [170705 01:29]: > Hi, > > On Wed, Jul 05, 2017 at 01:06:54AM -0700, Tony Lindgren wrote: > > > > Not sure if this really is a regression as we've always had a bogus > > > > wl1271-nvs.bin in linux-firmware.git. Sure would be nice to fix it, > > > > but going back to using a generic wl1271-nvs.bin sure does not seem > > > > like a good long term fix to me :) > > > A lot of legacy here... > > > Wl1271-nvs has been used mainly with wilink6/7 and indeed was device specific > > > Holding calibration info etc. > > > When they started with wilink8 the device specific configuration that was > > > Part of it has switched to wl18xx-conf.bin and using the wlaconf tool for setting it. > > > Also there is no calibration specific info per device with wilink8 so the wl18xx-conf.bin > > > The only thing left in wl1271-nvs.bin for wilink8 was the mac address override. > > > > And the default wl1271-nvs.bin sets the mac address to a bogus deadbeef address, > > so it's wrong to use and totally broken for distros :( > > So use something like the following pseudo-code? > > if (fw->mac_address == deadbeef) { > fw->mac_address = get_random_mac(); > dev_warn(dev, "Detected unconfigured wl1271-nvs.bin.\n" > "Your device will run with limited performance.\n" > "Please use ti-utils to configure your device.\n"); > } Yeah something like that should do the trick :) Regards, Tony
> > > > On Wed, Jul 05, 2017 at 01:06:54AM -0700, Tony Lindgren wrote: > > > > > Not sure if this really is a regression as we've always had a bogus > > > > > wl1271-nvs.bin in linux-firmware.git. Sure would be nice to fix it, > > > > > but going back to using a generic wl1271-nvs.bin sure does not seem > > > > > like a good long term fix to me :) > > > > A lot of legacy here... > > > > Wl1271-nvs has been used mainly with wilink6/7 and indeed was device > specific > > > > Holding calibration info etc. > > > > When they started with wilink8 the device specific configuration that was > > > > Part of it has switched to wl18xx-conf.bin and using the wlaconf tool for > setting it. > > > > Also there is no calibration specific info per device with wilink8 so the > wl18xx-conf.bin > > > > The only thing left in wl1271-nvs.bin for wilink8 was the mac address > override. > > > > > > And the default wl1271-nvs.bin sets the mac address to a bogus deadbeef > address, > > > so it's wrong to use and totally broken for distros :( > > > > So use something like the following pseudo-code? > > > > if (fw->mac_address == deadbeef) { > > fw->mac_address = get_random_mac(); Deadbeef0000 is a valid mac address, so I suggest detecting it and warning the user of the bogus (default) nvs but don't attempt to get a random mac address for him as it has to many rules (registered oui addresses etc.) > > dev_warn(dev, "Detected unconfigured wl1271-nvs.bin.\n" > > "Your device will run with limited performance.\n" > > "Please use ti-utils to configure your device.\n"); > > } > > Yeah something like that should do the trick :) > Seems logical, will submit a v3 using wl1271-nvs.bin for wl18xx while adding this Check/warn. Best Regards, Eyal
diff --git a/drivers/net/wireless/ti/wlcore/sdio.c b/drivers/net/wireless/ti/wlcore/sdio.c index 2fb3871..f8a1fea 100644 --- a/drivers/net/wireless/ti/wlcore/sdio.c +++ b/drivers/net/wireless/ti/wlcore/sdio.c @@ -230,6 +230,7 @@ static const struct wilink_family_data wl128x_data = { static const struct wilink_family_data wl18xx_data = { .name = "wl18xx", .cfg_name = "ti-connectivity/wl18xx-conf.bin", + .nvs_name = "ti-connectivity/wl1271-nvs.bin", }; static const struct of_device_id wlcore_sdio_of_match_table[] = { diff --git a/drivers/net/wireless/ti/wlcore/spi.c b/drivers/net/wireless/ti/wlcore/spi.c index fdabb92..62ce54a 100644 --- a/drivers/net/wireless/ti/wlcore/spi.c +++ b/drivers/net/wireless/ti/wlcore/spi.c @@ -92,6 +92,7 @@ static const struct wilink_family_data wl128x_data = { static const struct wilink_family_data wl18xx_data = { .name = "wl18xx", .cfg_name = "ti-connectivity/wl18xx-conf.bin", + .nvs_name = "ti-connectivity/wl1271-nvs.bin", }; struct wl12xx_spi_glue {
When working with wl18xx the nvs file is used for defining an alternate mac address and override the default mac address that is stored inside the wl18xx chip. update the structure field with the same default nvs file name that has been used in the past, otherwise userspace backward compatibility is broken when upgrading from older kernels, as the alternate mac address would not be read from the nvs that is already present in the file system (lib/firmware/ti-connectivity/wl1271-nvs.bin) causing mac address change of the wlan interface. Signed-off-by: Eyal Reizer <eyalr@ti.com> --- drivers/net/wireless/ti/wlcore/sdio.c | 1 + drivers/net/wireless/ti/wlcore/spi.c | 1 + 2 files changed, 2 insertions(+)