[PATCH] wlcore: add missing nvs file name info for wilink8

[PATCH] wlcore: add missing nvs file name info for wilink8

[Reizer, Eyal]

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(+)

diff --git a/drivers/net/wireless/ti/wlcore/sdio.c b/drivers/net/wireless/t=
i/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 =3D =
{
 static const struct wilink_family_data wl18xx_data =3D {
 	.name =3D "wl18xx",
 	.cfg_name =3D "ti-connectivity/wl18xx-conf.bin",
+	.nvs_name =3D "ti-connectivity/wl1271-nvs.bin",
 };
=20
 static const struct of_device_id wlcore_sdio_of_match_table[] =3D {
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 =3D {
 static const struct wilink_family_data wl18xx_data =3D {
 	.name =3D "wl18xx",
 	.cfg_name =3D "ti-connectivity/wl18xx-conf.bin",
+	.nvs_name =3D "ti-connectivity/wl1271-nvs.bin",
 };
=20
 struct wl12xx_spi_glue {
--=20
2.7.4

Re: [PATCH] wlcore: add missing nvs file name info for wilink8

[Kalle Valo]

"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

Re: [PATCH] wlcore: add missing nvs file name info for wilink8

[Tony Lindgren]

* 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

RE: [PATCH] wlcore: add missing nvs file name info for wilink8

[Reizer, Eyal]

Hi Tony,

>=20
> * 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 alterna=
te
> > > mac address and override the default mac address that is stored insid=
e
> > > the wl18xx chip.
> > > update the structure field with the same default nvs file name that h=
as
> > > been used in the past, otherwise userspace backward compatibility is
> > > broken when upgrading from older kernels, as the alternate mac addres=
s
> > > would not be read from the nvs that is already present in the file sy=
stem
> > > (lib/firmware/ti-connectivity/wl1271-nvs.bin) causing mac address cha=
nge
> > > of the wlan interface.
> > >
> > > Signed-off-by: Eyal Reizer <eyalr@ti.com>
> >
> > Should we also cc stable? And a Fixes line would be nice.
>=20
> 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 NFSroo=
t?
>=20
> 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 specif=
ic
Holding calibration info etc.
When they started with wilink8 the device specific configuration that was=20
Part of it has switched to wl18xx-conf.bin and using the wlaconf tool for s=
etting it.
Also there is no calibration specific info per device with wilink8 so the w=
l18xx-conf.bin
The only thing left in wl1271-nvs.bin for wilink8 was the mac address overr=
ide.
There are wilink8 customer using this feature which is the only reason for =
keeping=20
This file for wilink8.
So for wilink8 there is really not issue with having the same wl1271-nvs.bi=
n=20
On NFSroot.=20

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

>=20
> 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..
>=20
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=20
Customers that already have products in the field and only updating
Their kernel.

Best Regards,
Eyal

Re: [PATCH] wlcore: add missing nvs file name info for wilink8

[Tony Lindgren]

* 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

Re: [PATCH] wlcore: add missing nvs file name info for wilink8

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 1314 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] wlcore: add missing nvs file name info for wilink8

[Tony Lindgren]

* 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

RE: [PATCH] wlcore: add missing nvs file name info for wilink8

[Reizer, Eyal]

> >
> > 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 bog=
us
> > > > > wl1271-nvs.bin in linux-firmware.git. Sure would be nice to fix i=
t,
> > > > > but going back to using a generic wl1271-nvs.bin sure does not se=
em
> > > > > 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 devic=
e
> specific
> > > > Holding calibration info etc.
> > > > When they started with wilink8 the device specific configuration th=
at was
> > > > Part of it has switched to wl18xx-conf.bin and using the wlaconf to=
ol 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 addre=
ss
> override.
> > >
> > > And the default wl1271-nvs.bin sets the mac address to a bogus deadbe=
ef
> address,
> > > so it's wrong to use and totally broken for distros :(
> >
> > So use something like the following pseudo-code?
> >
> > if (fw->mac_address =3D=3D deadbeef) {
> >     fw->mac_address =3D 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");
> > }
>=20
> Yeah something like that should do the trick  :)
>=20

Seems logical, will submit a v3 using wl1271-nvs.bin for wl18xx while addin=
g this
Check/warn.

Best Regards,
Eyal