Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1680756 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] wlcore: add missing nvs file name info for wilink8 Tony Lindgren <tony@atomide.com> - 2017-07-04 10:20 +0200
    RE: [PATCH] wlcore: add missing nvs file name info for wilink8 "Reizer, Eyal" <eyalr@ti.com> - 2017-07-04 10:50 +0200
      Re: [PATCH] wlcore: add missing nvs file name info for wilink8 Tony Lindgren <tony@atomide.com> - 2017-07-05 10:10 +0200
        Re: [PATCH] wlcore: add missing nvs file name info for wilink8 Sebastian Reichel <sebastian.reichel@collabora.co.uk> - 2017-07-05 10:30 +0200
          Re: [PATCH] wlcore: add missing nvs file name info for wilink8 Tony Lindgren <tony@atomide.com> - 2017-07-06 08:30 +0200

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

* 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

[toc] | [next] | [standalone]

#1680770

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

[toc] | [prev] | [next] | [standalone]

#1681347

* 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

[toc] | [prev] | [next] | [standalone]

#1681361

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [next] | [standalone]

#1682066

* 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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web