wl1251: NVS firmware data

wl1251: NVS firmware data

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 1673 bytes --]

Hello,

wifi driver wl1251 needs NVS calibration data for working. These 
data are loaded by driver via request_firmware from userspace 
file: ti-connectivity/wl1251-nvs.bin. In linux-fimrware git tree 
there is generic wl1251-nvs.bin file which is used by default.

Driver wl1251 is used on Nokia N900 cellphone for its wifi chip. 
This cellphone has one special MTD partition (called CAL) where 
are stored some configuration data in special binary (key-value) 
format. And there is also stored correct calibration data for 
specific device (each device has different data). It is preferred 
to use those data instead generic one (provided by linux-firmware 
git tree).

Now my question is: How to correctly load calibration data from 
special Nokia N900 CAL partition into wl1251 kernel driver?

By default kernel reads ti-connectivity/wl1251-nvs.bin file from 
VFS if exists without any userspace support. If it fails then it 
fallback to loading via udev.

Reading correct data from CAL partition is not easy (structure is 
difficult), but there is open source program which can parse CAL 
partition and write NVS data to stdout. So adding this CAL parser 
into kernel is not good idea (program is GPLv3+ code -- 
incompatible with kernel).

So how to solve this problem? How to load correct NVS data from 
CAL partition into wl1251 driver?

It is possible to tell kernel to use some helper userspace 
program for loading data and if it fails then fallback to direct 
loading? E.g first try to use model specific data and if it fails 
for some reasons then fallback to reading genetic data.

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: wl1251: NVS firmware data

[Ming Lei]

On Thu, Nov 27, 2014 at 10:06 PM, Pali Rohár <pali.rohar@gmail.com> wrote:
> Hello,
>
> wifi driver wl1251 needs NVS calibration data for working. These
> data are loaded by driver via request_firmware from userspace
> file: ti-connectivity/wl1251-nvs.bin. In linux-fimrware git tree
> there is generic wl1251-nvs.bin file which is used by default.
>
> Driver wl1251 is used on Nokia N900 cellphone for its wifi chip.
> This cellphone has one special MTD partition (called CAL) where
> are stored some configuration data in special binary (key-value)
> format. And there is also stored correct calibration data for
> specific device (each device has different data). It is preferred
> to use those data instead generic one (provided by linux-firmware
> git tree).
>
> Now my question is: How to correctly load calibration data from
> special Nokia N900 CAL partition into wl1251 kernel driver?

It is better to let user space script handle the request.

>
> By default kernel reads ti-connectivity/wl1251-nvs.bin file from
> VFS if exists without any userspace support. If it fails then it
> fallback to loading via udev.

You can remove or rename this file so that loading from user space
can be triggered.

>
> Reading correct data from CAL partition is not easy (structure is
> difficult), but there is open source program which can parse CAL
> partition and write NVS data to stdout. So adding this CAL parser
> into kernel is not good idea (program is GPLv3+ code --
> incompatible with kernel).
>
> So how to solve this problem? How to load correct NVS data from
> CAL partition into wl1251 driver?
>
> It is possible to tell kernel to use some helper userspace
> program for loading data and if it fails then fallback to direct
> loading? E.g first try to use model specific data and if it fails
> for some reasons then fallback to reading genetic data.

One solution is to introduce request_firmware_user() and let
this API handle your case, but CONFIG_FW_LOADER_USER_HELPER
has to be enabled.

If request_firmware_user() fails, request_firmware_direct() can be
tried further.

Thanks,
Ming Lei

Re: wl1251: NVS firmware data

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 2970 bytes --]

On Thursday 27 November 2014 15:21:44 Ming Lei wrote:
> On Thu, Nov 27, 2014 at 10:06 PM, Pali Rohár 
<pali.rohar@gmail.com> wrote:
> > Hello,
> > 
> > wifi driver wl1251 needs NVS calibration data for working.
> > These data are loaded by driver via request_firmware from
> > userspace file: ti-connectivity/wl1251-nvs.bin. In
> > linux-fimrware git tree there is generic wl1251-nvs.bin
> > file which is used by default.
> > 
> > Driver wl1251 is used on Nokia N900 cellphone for its wifi
> > chip. This cellphone has one special MTD partition (called
> > CAL) where are stored some configuration data in special
> > binary (key-value) format. And there is also stored correct
> > calibration data for specific device (each device has
> > different data). It is preferred to use those data instead
> > generic one (provided by linux-firmware git tree).
> > 
> > Now my question is: How to correctly load calibration data
> > from special Nokia N900 CAL partition into wl1251 kernel
> > driver?
> 
> It is better to let user space script handle the request.
> 

Yes, this makes sense. Implementing CAL parser in kernel wl1251 
driver would be hard...

> > By default kernel reads ti-connectivity/wl1251-nvs.bin file
> > from VFS if exists without any userspace support. If it
> > fails then it fallback to loading via udev.
> 
> You can remove or rename this file so that loading from user
> space can be triggered.
> 

It is no so easy... In case when CAL does not contains NVS data 
then we want to use this generic NVS file. And telling everybody 
to rename this is file is not good solution...

> > Reading correct data from CAL partition is not easy
> > (structure is difficult), but there is open source program
> > which can parse CAL partition and write NVS data to stdout.
> > So adding this CAL parser into kernel is not good idea
> > (program is GPLv3+ code -- incompatible with kernel).
> > 
> > So how to solve this problem? How to load correct NVS data
> > from CAL partition into wl1251 driver?
> > 
> > It is possible to tell kernel to use some helper userspace
> > program for loading data and if it fails then fallback to
> > direct loading? E.g first try to use model specific data
> > and if it fails for some reasons then fallback to reading
> > genetic data.
> 
> One solution is to introduce request_firmware_user() and let
> this API handle your case, but CONFIG_FW_LOADER_USER_HELPER
> has to be enabled.
> 
> If request_firmware_user() fails, request_firmware_direct()
> can be tried further.
> 
> Thanks,
> Ming Lei

Ok, new kernel function which will change order of loading 
firmware should work.

Which userspace helper programs for (automatic) firmware loading 
are used? Can be udev configured to use own program for loading 
firmware instead that udev integrated which looking for firmware 
only in /lib/firmware files?

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: wl1251: NVS firmware data

[Ming Lei]

On Thu, Nov 27, 2014 at 10:43 PM, Pali Rohár <pali.rohar@gmail.com> wrote:
> On Thursday 27 November 2014 15:21:44 Ming Lei wrote:
>> On Thu, Nov 27, 2014 at 10:06 PM, Pali Rohár
> <pali.rohar@gmail.com> wrote:
>> > Hello,
>> >
>> > wifi driver wl1251 needs NVS calibration data for working.
>> > These data are loaded by driver via request_firmware from
>> > userspace file: ti-connectivity/wl1251-nvs.bin. In
>> > linux-fimrware git tree there is generic wl1251-nvs.bin
>> > file which is used by default.
>> >
>> > Driver wl1251 is used on Nokia N900 cellphone for its wifi
>> > chip. This cellphone has one special MTD partition (called
>> > CAL) where are stored some configuration data in special
>> > binary (key-value) format. And there is also stored correct
>> > calibration data for specific device (each device has
>> > different data). It is preferred to use those data instead
>> > generic one (provided by linux-firmware git tree).
>> >
>> > Now my question is: How to correctly load calibration data
>> > from special Nokia N900 CAL partition into wl1251 kernel
>> > driver?
>>
>> It is better to let user space script handle the request.
>>
>
> Yes, this makes sense. Implementing CAL parser in kernel wl1251
> driver would be hard...
>
>> > By default kernel reads ti-connectivity/wl1251-nvs.bin file
>> > from VFS if exists without any userspace support. If it
>> > fails then it fallback to loading via udev.
>>
>> You can remove or rename this file so that loading from user
>> space can be triggered.
>>
>
> It is no so easy... In case when CAL does not contains NVS data
> then we want to use this generic NVS file. And telling everybody
> to rename this is file is not good solution...
>
>> > Reading correct data from CAL partition is not easy
>> > (structure is difficult), but there is open source program
>> > which can parse CAL partition and write NVS data to stdout.
>> > So adding this CAL parser into kernel is not good idea
>> > (program is GPLv3+ code -- incompatible with kernel).
>> >
>> > So how to solve this problem? How to load correct NVS data
>> > from CAL partition into wl1251 driver?
>> >
>> > It is possible to tell kernel to use some helper userspace
>> > program for loading data and if it fails then fallback to
>> > direct loading? E.g first try to use model specific data
>> > and if it fails for some reasons then fallback to reading
>> > genetic data.
>>
>> One solution is to introduce request_firmware_user() and let
>> this API handle your case, but CONFIG_FW_LOADER_USER_HELPER
>> has to be enabled.
>>
>> If request_firmware_user() fails, request_firmware_direct()
>> can be tried further.
>>
>> Thanks,
>> Ming Lei
>
> Ok, new kernel function which will change order of loading
> firmware should work.

I mean you can do that in your driver.

>
> Which userspace helper programs for (automatic) firmware loading
> are used? Can be udev configured to use own program for loading

At default, udev had its builtin firmware helper, but some new
udev stops to handle firmware request.

If the udev you are using still supports to handle firmware request,
you can write your load helper and add your rule for handling
the special case. Otherwise, you need to write code to monitor
the netlink uevents from the kernel and handle your firmware
loading request.

Thanks,
Ming Lei

Re: wl1251: NVS firmware data

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 1089 bytes --]

On Thursday 27 November 2014 16:13:51 Ming Lei wrote:
> > Which userspace helper programs for (automatic) firmware
> > loading are used? Can be udev configured to use own program
> > for loading
> 
> At default, udev had its builtin firmware helper, but some new
> udev stops to handle firmware request.
> 
> If the udev you are using still supports to handle firmware
> request, you can write your load helper and add your rule for
> handling the special case. Otherwise, you need to write code
> to monitor the netlink uevents from the kernel and handle
> your firmware loading request.
> 
> Thanks,
> Ming Lei

For Nokia N900 (and Maemo system) we are using older udev version 
(0.125), so here should be support for firmware helper.

And if some new udev version will not work, we can just revert 
back to older working... No need to use super-uber-fantastic 
systemd-udevd software which removing old useful code.

So looks like adding new rule file to udev for handling firmware 
requests from kernel could work...

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: wl1251: NVS firmware data

[Greg Kroah-Hartman]

On Thu, Nov 27, 2014 at 03:43:23PM +0100, Pali Rohár wrote:
> Which userspace helper programs for (automatic) firmware loading 
> are used? Can be udev configured to use own program for loading 
> firmware instead that udev integrated which looking for firmware 
> only in /lib/firmware files?

The code to load firmware from userspace has been removed from udev, so
that's not going to work at all, sorry.

greg k-h

Re: wl1251: NVS firmware data

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 683 bytes --]

On Thursday 27 November 2014 16:14:48 Greg Kroah-Hartman wrote:
> On Thu, Nov 27, 2014 at 03:43:23PM +0100, Pali Rohár wrote:
> > Which userspace helper programs for (automatic) firmware
> > loading are used? Can be udev configured to use own program
> > for loading firmware instead that udev integrated which
> > looking for firmware only in /lib/firmware files?
> 
> The code to load firmware from userspace has been removed from
> udev, so that's not going to work at all, sorry.
> 
> greg k-h

Ok and how to load (dynamic) firmware files into kernel? What is 
preferred way if not udev (because it removed that support)?

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: wl1251: NVS firmware data

[Ming Lei]

On Thu, Nov 27, 2014 at 11:24 PM, Pali Rohár <pali.rohar@gmail.com> wrote:
> On Thursday 27 November 2014 16:14:48 Greg Kroah-Hartman wrote:
>> On Thu, Nov 27, 2014 at 03:43:23PM +0100, Pali Rohár wrote:
>> > Which userspace helper programs for (automatic) firmware
>> > loading are used? Can be udev configured to use own program
>> > for loading firmware instead that udev integrated which
>> > looking for firmware only in /lib/firmware files?
>>
>> The code to load firmware from userspace has been removed from
>> udev, so that's not going to work at all, sorry.
>>
>> greg k-h
>
> Ok and how to load (dynamic) firmware files into kernel? What is
> preferred way if not udev (because it removed that support)?

As I said, it isn't difficult to write a helper with uevent monitor for
your case, and you can refer to mdev or android's implementation.

Thanks,
Ming Lei

Re: wl1251: NVS firmware data

[Greg Kroah-Hartman]

On Thu, Nov 27, 2014 at 03:43:23PM +0100, Pali Rohár wrote:
> On Thursday 27 November 2014 15:21:44 Ming Lei wrote:
> > On Thu, Nov 27, 2014 at 10:06 PM, Pali Rohár 
> <pali.rohar@gmail.com> wrote:
> > > Hello,
> > > 
> > > wifi driver wl1251 needs NVS calibration data for working.
> > > These data are loaded by driver via request_firmware from
> > > userspace file: ti-connectivity/wl1251-nvs.bin. In
> > > linux-fimrware git tree there is generic wl1251-nvs.bin
> > > file which is used by default.
> > > 
> > > Driver wl1251 is used on Nokia N900 cellphone for its wifi
> > > chip. This cellphone has one special MTD partition (called
> > > CAL) where are stored some configuration data in special
> > > binary (key-value) format. And there is also stored correct
> > > calibration data for specific device (each device has
> > > different data). It is preferred to use those data instead
> > > generic one (provided by linux-firmware git tree).
> > > 
> > > Now my question is: How to correctly load calibration data
> > > from special Nokia N900 CAL partition into wl1251 kernel
> > > driver?
> > 
> > It is better to let user space script handle the request.
> > 
> 
> Yes, this makes sense. Implementing CAL parser in kernel wl1251 
> driver would be hard...
> 
> > > By default kernel reads ti-connectivity/wl1251-nvs.bin file
> > > from VFS if exists without any userspace support. If it
> > > fails then it fallback to loading via udev.
> > 
> > You can remove or rename this file so that loading from user
> > space can be triggered.
> > 
> 
> It is no so easy... In case when CAL does not contains NVS data 
> then we want to use this generic NVS file. And telling everybody 
> to rename this is file is not good solution...

But that's up to your system configuration, not the kernel.  Make a
userspace package for the firmware that creates it in the format you
need it to be in, for the hardware you have, and then there would not be
any need for a kernel change, right?

greg k-h

Re: wl1251: NVS firmware data

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 2389 bytes --]

On Thursday 27 November 2014 16:16:55 Greg Kroah-Hartman wrote:
> On Thu, Nov 27, 2014 at 03:43:23PM +0100, Pali Rohár wrote:
> > On Thursday 27 November 2014 15:21:44 Ming Lei wrote:
> > > On Thu, Nov 27, 2014 at 10:06 PM, Pali Rohár
> > 
> > <pali.rohar@gmail.com> wrote:
> > > > Hello,
> > > > 
> > > > wifi driver wl1251 needs NVS calibration data for
> > > > working. These data are loaded by driver via
> > > > request_firmware from userspace file:
> > > > ti-connectivity/wl1251-nvs.bin. In linux-fimrware git
> > > > tree there is generic wl1251-nvs.bin file which is used
> > > > by default.
> > > > 
> > > > Driver wl1251 is used on Nokia N900 cellphone for its
> > > > wifi chip. This cellphone has one special MTD partition
> > > > (called CAL) where are stored some configuration data
> > > > in special binary (key-value) format. And there is also
> > > > stored correct calibration data for specific device
> > > > (each device has different data). It is preferred to
> > > > use those data instead generic one (provided by
> > > > linux-firmware git tree).
> > > > 
> > > > Now my question is: How to correctly load calibration
> > > > data from special Nokia N900 CAL partition into wl1251
> > > > kernel driver?
> > > 
> > > It is better to let user space script handle the request.
> > 
> > Yes, this makes sense. Implementing CAL parser in kernel
> > wl1251 driver would be hard...
> > 
> > > > By default kernel reads ti-connectivity/wl1251-nvs.bin
> > > > file from VFS if exists without any userspace support.
> > > > If it fails then it fallback to loading via udev.
> > > 
> > > You can remove or rename this file so that loading from
> > > user space can be triggered.
> > 
> > It is no so easy... In case when CAL does not contains NVS
> > data then we want to use this generic NVS file. And telling
> > everybody to rename this is file is not good solution...
> 
> But that's up to your system configuration, not the kernel. 
> Make a userspace package for the firmware that creates it in
> the format you need it to be in, for the hardware you have,
> and then there would not be any need for a kernel change,
> right?
> 
> greg k-h

Not so simple as you think. Some parts of NVS data are configured 
based on location and cellular station. Data are not static.

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: wl1251: NVS firmware data

[Greg Kroah-Hartman]

On Thu, Nov 27, 2014 at 04:22:58PM +0100, Pali Rohár wrote:
> On Thursday 27 November 2014 16:16:55 Greg Kroah-Hartman wrote:
> > On Thu, Nov 27, 2014 at 03:43:23PM +0100, Pali Rohár wrote:
> > > On Thursday 27 November 2014 15:21:44 Ming Lei wrote:
> > > > On Thu, Nov 27, 2014 at 10:06 PM, Pali Rohár
> > > 
> > > <pali.rohar@gmail.com> wrote:
> > > > > Hello,
> > > > > 
> > > > > wifi driver wl1251 needs NVS calibration data for
> > > > > working. These data are loaded by driver via
> > > > > request_firmware from userspace file:
> > > > > ti-connectivity/wl1251-nvs.bin. In linux-fimrware git
> > > > > tree there is generic wl1251-nvs.bin file which is used
> > > > > by default.
> > > > > 
> > > > > Driver wl1251 is used on Nokia N900 cellphone for its
> > > > > wifi chip. This cellphone has one special MTD partition
> > > > > (called CAL) where are stored some configuration data
> > > > > in special binary (key-value) format. And there is also
> > > > > stored correct calibration data for specific device
> > > > > (each device has different data). It is preferred to
> > > > > use those data instead generic one (provided by
> > > > > linux-firmware git tree).
> > > > > 
> > > > > Now my question is: How to correctly load calibration
> > > > > data from special Nokia N900 CAL partition into wl1251
> > > > > kernel driver?
> > > > 
> > > > It is better to let user space script handle the request.
> > > 
> > > Yes, this makes sense. Implementing CAL parser in kernel
> > > wl1251 driver would be hard...
> > > 
> > > > > By default kernel reads ti-connectivity/wl1251-nvs.bin
> > > > > file from VFS if exists without any userspace support.
> > > > > If it fails then it fallback to loading via udev.
> > > > 
> > > > You can remove or rename this file so that loading from
> > > > user space can be triggered.
> > > 
> > > It is no so easy... In case when CAL does not contains NVS
> > > data then we want to use this generic NVS file. And telling
> > > everybody to rename this is file is not good solution...
> > 
> > But that's up to your system configuration, not the kernel. 
> > Make a userspace package for the firmware that creates it in
> > the format you need it to be in, for the hardware you have,
> > and then there would not be any need for a kernel change,
> > right?
> > 
> > greg k-h
> 
> Not so simple as you think. Some parts of NVS data are configured 
> based on location and cellular station. Data are not static.

Then you need a dynamic program that you control, in userspace, to dump
the needed data into the kernel.  Don't try to do it with "static"
firmware files.  Use the binary sysfs file interface for this if you
want.

good luck,

greg k-h

Re: wl1251: NVS firmware data

[Pavel Machek]

On Thu 2014-11-27 07:58:40, Greg Kroah-Hartman wrote:
> On Thu, Nov 27, 2014 at 04:22:58PM +0100, Pali Rohár wrote:
> > On Thursday 27 November 2014 16:16:55 Greg Kroah-Hartman wrote:
> > > On Thu, Nov 27, 2014 at 03:43:23PM +0100, Pali Rohár wrote:
> > > > On Thursday 27 November 2014 15:21:44 Ming Lei wrote:
> > > > > On Thu, Nov 27, 2014 at 10:06 PM, Pali Rohár
> > > > 
> > > > <pali.rohar@gmail.com> wrote:
> > > > > > Hello,
> > > > > > 
> > > > > > wifi driver wl1251 needs NVS calibration data for
> > > > > > working. These data are loaded by driver via
> > > > > > request_firmware from userspace file:
> > > > > > ti-connectivity/wl1251-nvs.bin. In linux-fimrware git
> > > > > > tree there is generic wl1251-nvs.bin file which is used
> > > > > > by default.
> > > > > > 
> > > > > > Driver wl1251 is used on Nokia N900 cellphone for its
> > > > > > wifi chip. This cellphone has one special MTD partition
> > > > > > (called CAL) where are stored some configuration data
> > > > > > in special binary (key-value) format. And there is also
> > > > > > stored correct calibration data for specific device
> > > > > > (each device has different data). It is preferred to
> > > > > > use those data instead generic one (provided by
> > > > > > linux-firmware git tree).
> > > > > > 
> > > > > > Now my question is: How to correctly load calibration
> > > > > > data from special Nokia N900 CAL partition into wl1251
> > > > > > kernel driver?
> > > > > 
> > > > > It is better to let user space script handle the request.
> > > > 
> > > > Yes, this makes sense. Implementing CAL parser in kernel
> > > > wl1251 driver would be hard...
> > > > 
> > > > > > By default kernel reads ti-connectivity/wl1251-nvs.bin
> > > > > > file from VFS if exists without any userspace support.
> > > > > > If it fails then it fallback to loading via udev.
> > > > > 
> > > > > You can remove or rename this file so that loading from
> > > > > user space can be triggered.
> > > > 
> > > > It is no so easy... In case when CAL does not contains NVS
> > > > data then we want to use this generic NVS file. And telling
> > > > everybody to rename this is file is not good solution...
> > > 
> > > But that's up to your system configuration, not the kernel. 
> > > Make a userspace package for the firmware that creates it in
> > > the format you need it to be in, for the hardware you have,
> > > and then there would not be any need for a kernel change,
> > > right?
> > > 
> > > greg k-h
> > 
> > Not so simple as you think. Some parts of NVS data are configured 
> > based on location and cellular station. Data are not static.
> 
> Then you need a dynamic program that you control, in userspace, to dump
> the needed data into the kernel.  Don't try to do it with "static"
> firmware files.  Use the binary sysfs file interface for this if you
> want.

Actually, this seems to be similar situation to fpga programming.

There, it is static firmware for 90% users, but some special use cases
want it more dynamic.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: wl1251: NVS firmware data

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 6283 bytes --]

On Saturday 06 December 2014 13:49:54 Pavel Machek wrote:
> On Thu 2014-11-27 07:58:40, Greg Kroah-Hartman wrote:
> > On Thu, Nov 27, 2014 at 04:22:58PM +0100, Pali Rohár wrote:
> > > On Thursday 27 November 2014 16:16:55 Greg Kroah-Hartman wrote:
> > > > On Thu, Nov 27, 2014 at 03:43:23PM +0100, Pali Rohár wrote:
> > > > > On Thursday 27 November 2014 15:21:44 Ming Lei wrote:
> > > > > > On Thu, Nov 27, 2014 at 10:06 PM, Pali Rohár
> > > > > 
> > > > > <pali.rohar@gmail.com> wrote:
> > > > > > > Hello,
> > > > > > > 
> > > > > > > wifi driver wl1251 needs NVS calibration data for
> > > > > > > working. These data are loaded by driver via
> > > > > > > request_firmware from userspace file:
> > > > > > > ti-connectivity/wl1251-nvs.bin. In linux-fimrware
> > > > > > > git tree there is generic wl1251-nvs.bin file
> > > > > > > which is used by default.
> > > > > > > 
> > > > > > > Driver wl1251 is used on Nokia N900 cellphone for
> > > > > > > its wifi chip. This cellphone has one special MTD
> > > > > > > partition (called CAL) where are stored some
> > > > > > > configuration data in special binary (key-value)
> > > > > > > format. And there is also stored correct
> > > > > > > calibration data for specific device (each device
> > > > > > > has different data). It is preferred to use those
> > > > > > > data instead generic one (provided by
> > > > > > > linux-firmware git tree).
> > > > > > > 
> > > > > > > Now my question is: How to correctly load
> > > > > > > calibration data from special Nokia N900 CAL
> > > > > > > partition into wl1251 kernel driver?
> > > > > > 
> > > > > > It is better to let user space script handle the
> > > > > > request.
> > > > > 
> > > > > Yes, this makes sense. Implementing CAL parser in
> > > > > kernel wl1251 driver would be hard...
> > > > > 
> > > > > > > By default kernel reads
> > > > > > > ti-connectivity/wl1251-nvs.bin file from VFS if
> > > > > > > exists without any userspace support. If it fails
> > > > > > > then it fallback to loading via udev.
> > > > > > 
> > > > > > You can remove or rename this file so that loading
> > > > > > from user space can be triggered.
> > > > > 
> > > > > It is no so easy... In case when CAL does not contains
> > > > > NVS data then we want to use this generic NVS file.
> > > > > And telling everybody to rename this is file is not
> > > > > good solution...
> > > > 
> > > > But that's up to your system configuration, not the
> > > > kernel. Make a userspace package for the firmware that
> > > > creates it in the format you need it to be in, for the
> > > > hardware you have, and then there would not be any need
> > > > for a kernel change, right?
> > > > 
> > > > greg k-h
> > > 
> > > Not so simple as you think. Some parts of NVS data are
> > > configured based on location and cellular station. Data
> > > are not static.
> > 
> > Then you need a dynamic program that you control, in
> > userspace, to dump the needed data into the kernel.  Don't
> > try to do it with "static" firmware files.  Use the binary
> > sysfs file interface for this if you want.
> 
> Actually, this seems to be similar situation to fpga
> programming.
> 
> There, it is static firmware for 90% users, but some special
> use cases want it more dynamic.
> 									Pavel

Greg, Ming, what do you think about this approach?

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 3d785eb..810c4b9 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -111,6 +111,11 @@ static inline long firmware_loading_timeout(void)
 #define FW_OPT_FALLBACK		0
 #endif
 #define FW_OPT_NO_WARN	(1U << 3)
+#ifdef CONFIG_FW_LOADER_USER_HELPER
+#define FW_OPT_PREFER_USER	(1U << 4)
+#else
+#define FW_OPT_PREFER_USER	0
+#endif
 
 struct firmware_cache {
 	/* firmware_buf instance will be added into the below list */
@@ -1131,7 +1136,20 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
 		}
 	}
 
-	ret = fw_get_filesystem_firmware(device, fw->priv);
+	if (opt_flags & FW_OPT_PREFER_USER) {
+		ret = fw_load_from_user_helper(fw, name, device, opt_flags, timeout);
+		if (ret) {
+			dev_warn(device,
+				 "User helper firmware load for %s failed with error %d\n",
+				 name, ret);
+			dev_warn(device, "Falling back to direct firmware load\n");
+		}
+	} else {
+		ret = -EINVAL;
+	}
+
+	if (ret)
+		ret = fw_get_filesystem_firmware(device, fw->priv);
 	if (ret) {
 		if (!(opt_flags & FW_OPT_NO_WARN))
 			dev_warn(device,
@@ -1218,6 +1236,28 @@ int request_firmware_direct(const struct firmware **firmware_p,
 EXPORT_SYMBOL_GPL(request_firmware_direct);
 
 /**
+ * request_firmware_prefer_user: - prefer usermode helper for loading firmware
+ * @firmware_p: pointer to firmware image
+ * @name: name of firmware file
+ * @device: device for which firmware is being loaded
+ *
+ * This function works pretty much like request_firmware(), but it prefer
+ * usermode helper. If usermode helper fails then it fallback to direct access.
+ * Usefull for dynamic or model specific firmware data.
+ **/
+int request_firmware_prefer_user(const struct firmware **firmware_p,
+			    const char *name, struct device *device)
+{
+	int ret;
+	__module_get(THIS_MODULE);
+	ret = _request_firmware(firmware_p, name, device,
+				FW_OPT_UEVENT | FW_OPT_PREFER_USER);
+	module_put(THIS_MODULE);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(request_firmware_prefer_user);
+
+/**
  * release_firmware: - release the resource associated with a firmware image
  * @fw: firmware resource to release
  **/
diff --git a/include/linux/firmware.h b/include/linux/firmware.h
index 5c41c5e..d35c511 100644
--- a/include/linux/firmware.h
+++ b/include/linux/firmware.h
@@ -47,6 +47,8 @@ int request_firmware_nowait(
 	void (*cont)(const struct firmware *fw, void *context));
 int request_firmware_direct(const struct firmware **fw, const char *name,
 			    struct device *device);
+int request_firmware_prefer_user(const struct firmware **fw, const char *name,
+				 struct device *device);
 
 void release_firmware(const struct firmware *fw);
 #else


-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: wl1251: NVS firmware data

[Ming Lei]

On Sat, Dec 6, 2014 at 9:02 PM, Pali Rohár <pali.rohar@gmail.com> wrote:
> On Saturday 06 December 2014 13:49:54 Pavel Machek wrote:

>
>  /**
> + * request_firmware_prefer_user: - prefer usermode helper for loading firmware
> + * @firmware_p: pointer to firmware image
> + * @name: name of firmware file
> + * @device: device for which firmware is being loaded
> + *
> + * This function works pretty much like request_firmware(), but it prefer
> + * usermode helper. If usermode helper fails then it fallback to direct access.
> + * Usefull for dynamic or model specific firmware data.
> + **/
> +int request_firmware_prefer_user(const struct firmware **firmware_p,
> +                           const char *name, struct device *device)
> +{
> +       int ret;
> +       __module_get(THIS_MODULE);
> +       ret = _request_firmware(firmware_p, name, device,
> +                               FW_OPT_UEVENT | FW_OPT_PREFER_USER);
> +       module_put(THIS_MODULE);
> +       return ret;
> +}
> +EXPORT_SYMBOL_GPL(request_firmware_prefer_user);

I'd like to introduce request_firmware_user() which only requests
firmware from user space, and this way is simpler and more flexible
since we have request_firmware_direct() already.

Thanks,
Ming Lei

Re: wl1251: NVS firmware data

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 1626 bytes --]

On Monday 08 December 2014 16:18:18 Ming Lei wrote:
> On Sat, Dec 6, 2014 at 9:02 PM, Pali Rohár 
<pali.rohar@gmail.com> wrote:
> > On Saturday 06 December 2014 13:49:54 Pavel Machek wrote:
> >  /**
> > 
> > + * request_firmware_prefer_user: - prefer usermode helper
> > for loading firmware + * @firmware_p: pointer to firmware
> > image
> > + * @name: name of firmware file
> > + * @device: device for which firmware is being loaded
> > + *
> > + * This function works pretty much like request_firmware(),
> > but it prefer + * usermode helper. If usermode helper fails
> > then it fallback to direct access. + * Usefull for dynamic
> > or model specific firmware data. + **/
> > +int request_firmware_prefer_user(const struct firmware
> > **firmware_p, +                           const char *name,
> > struct device *device) +{
> > +       int ret;
> > +       __module_get(THIS_MODULE);
> > +       ret = _request_firmware(firmware_p, name, device,
> > +                               FW_OPT_UEVENT |
> > FW_OPT_PREFER_USER); +       module_put(THIS_MODULE);
> > +       return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(request_firmware_prefer_user);
> 
> I'd like to introduce request_firmware_user() which only
> requests firmware from user space, and this way is simpler
> and more flexible since we have request_firmware_direct()
> already.
> 
> Thanks,
> Ming Lei

Ming, for wl1251 NVS data we need to load use usermode helper and 
fallback to direct load. So I think it is better to handle this 
request in firmware code and not in driver.

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: wl1251: NVS firmware data

[Ming Lei]

On Mon, Dec 8, 2014 at 11:22 PM, Pali Rohár <pali.rohar@gmail.com> wrote:
> On Monday 08 December 2014 16:18:18 Ming Lei wrote:
>> On Sat, Dec 6, 2014 at 9:02 PM, Pali Rohár
> <pali.rohar@gmail.com> wrote:
>> > On Saturday 06 December 2014 13:49:54 Pavel Machek wrote:
>> >  /**
>> >
>> > + * request_firmware_prefer_user: - prefer usermode helper
>> > for loading firmware + * @firmware_p: pointer to firmware
>> > image
>> > + * @name: name of firmware file
>> > + * @device: device for which firmware is being loaded
>> > + *
>> > + * This function works pretty much like request_firmware(),
>> > but it prefer + * usermode helper. If usermode helper fails
>> > then it fallback to direct access. + * Usefull for dynamic
>> > or model specific firmware data. + **/
>> > +int request_firmware_prefer_user(const struct firmware
>> > **firmware_p, +                           const char *name,
>> > struct device *device) +{
>> > +       int ret;
>> > +       __module_get(THIS_MODULE);
>> > +       ret = _request_firmware(firmware_p, name, device,
>> > +                               FW_OPT_UEVENT |
>> > FW_OPT_PREFER_USER); +       module_put(THIS_MODULE);
>> > +       return ret;
>> > +}
>> > +EXPORT_SYMBOL_GPL(request_firmware_prefer_user);
>>
>> I'd like to introduce request_firmware_user() which only
>> requests firmware from user space, and this way is simpler
>> and more flexible since we have request_firmware_direct()
>> already.
>>
>> Thanks,
>> Ming Lei
>
> Ming, for wl1251 NVS data we need to load use usermode helper and
> fallback to direct load. So I think it is better to handle this
> request in firmware code and not in driver.

Please do that in driver and don't mess firmware loader.

With introducing request_firmware_user(), it is even possible to
clean up firmware loader further.

Thanks,
Ming Lei

Re: wl1251: NVS firmware data

[Greg Kroah-Hartman]

On Mon, Dec 08, 2014 at 11:18:18PM +0800, Ming Lei wrote:
> On Sat, Dec 6, 2014 at 9:02 PM, Pali Rohár <pali.rohar@gmail.com> wrote:
> > On Saturday 06 December 2014 13:49:54 Pavel Machek wrote:
> 
> >
> >  /**
> > + * request_firmware_prefer_user: - prefer usermode helper for loading firmware
> > + * @firmware_p: pointer to firmware image
> > + * @name: name of firmware file
> > + * @device: device for which firmware is being loaded
> > + *
> > + * This function works pretty much like request_firmware(), but it prefer
> > + * usermode helper. If usermode helper fails then it fallback to direct access.
> > + * Usefull for dynamic or model specific firmware data.
> > + **/
> > +int request_firmware_prefer_user(const struct firmware **firmware_p,
> > +                           const char *name, struct device *device)
> > +{
> > +       int ret;
> > +       __module_get(THIS_MODULE);
> > +       ret = _request_firmware(firmware_p, name, device,
> > +                               FW_OPT_UEVENT | FW_OPT_PREFER_USER);
> > +       module_put(THIS_MODULE);
> > +       return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(request_firmware_prefer_user);
> 
> I'd like to introduce request_firmware_user() which only requests
> firmware from user space, and this way is simpler and more flexible
> since we have request_firmware_direct() already.

Why would a driver care about what program provides the firmware?  It
shouldn't at all, and we want to get rid of the userspace firmware
loader, not encourage drivers to use it "exclusively" at all.

So no, I don't want to see this, and I don't want drivers to worry about
this either.

greg k-h

Re: wl1251: NVS firmware data

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 1991 bytes --]

On Monday 08 December 2014 17:37:14 Greg Kroah-Hartman wrote:
> On Mon, Dec 08, 2014 at 11:18:18PM +0800, Ming Lei wrote:
> > On Sat, Dec 6, 2014 at 9:02 PM, Pali Rohár 
<pali.rohar@gmail.com> wrote:
> > > On Saturday 06 December 2014 13:49:54 Pavel Machek wrote:
> > >  /**
> > > 
> > > + * request_firmware_prefer_user: - prefer usermode helper
> > > for loading firmware + * @firmware_p: pointer to firmware
> > > image
> > > + * @name: name of firmware file
> > > + * @device: device for which firmware is being loaded
> > > + *
> > > + * This function works pretty much like
> > > request_firmware(), but it prefer + * usermode helper. If
> > > usermode helper fails then it fallback to direct access.
> > > + * Usefull for dynamic or model specific firmware data.
> > > + **/
> > > +int request_firmware_prefer_user(const struct firmware
> > > **firmware_p, +                           const char
> > > *name, struct device *device) +{
> > > +       int ret;
> > > +       __module_get(THIS_MODULE);
> > > +       ret = _request_firmware(firmware_p, name, device,
> > > +                               FW_OPT_UEVENT |
> > > FW_OPT_PREFER_USER); +       module_put(THIS_MODULE);
> > > +       return ret;
> > > +}
> > > +EXPORT_SYMBOL_GPL(request_firmware_prefer_user);
> > 
> > I'd like to introduce request_firmware_user() which only
> > requests firmware from user space, and this way is simpler
> > and more flexible since we have request_firmware_direct()
> > already.
> 
> Why would a driver care about what program provides the
> firmware?  It shouldn't at all, and we want to get rid of the
> userspace firmware loader, not encourage drivers to use it
> "exclusively" at all.
> 

Do not remove it! Without userspace firmware loader it is 
impossible to load dynamic firmware files.

> So no, I don't want to see this, and I don't want drivers to
> worry about this either.
> 
> greg k-h

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: wl1251: NVS firmware data

[Marcel Holtmann]

Hi Pali,

>>>> On Saturday 06 December 2014 13:49:54 Pavel Machek wrote:
>>>> /**
>>>> 
>>>> + * request_firmware_prefer_user: - prefer usermode helper
>>>> for loading firmware + * @firmware_p: pointer to firmware
>>>> image
>>>> + * @name: name of firmware file
>>>> + * @device: device for which firmware is being loaded
>>>> + *
>>>> + * This function works pretty much like
>>>> request_firmware(), but it prefer + * usermode helper. If
>>>> usermode helper fails then it fallback to direct access.
>>>> + * Usefull for dynamic or model specific firmware data.
>>>> + **/
>>>> +int request_firmware_prefer_user(const struct firmware
>>>> **firmware_p, +                           const char
>>>> *name, struct device *device) +{
>>>> +       int ret;
>>>> +       __module_get(THIS_MODULE);
>>>> +       ret = _request_firmware(firmware_p, name, device,
>>>> +                               FW_OPT_UEVENT |
>>>> FW_OPT_PREFER_USER); +       module_put(THIS_MODULE);
>>>> +       return ret;
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(request_firmware_prefer_user);
>>> 
>>> I'd like to introduce request_firmware_user() which only
>>> requests firmware from user space, and this way is simpler
>>> and more flexible since we have request_firmware_direct()
>>> already.
>> 
>> Why would a driver care about what program provides the
>> firmware?  It shouldn't at all, and we want to get rid of the
>> userspace firmware loader, not encourage drivers to use it
>> "exclusively" at all.
>> 
> 
> Do not remove it! Without userspace firmware loader it is 
> impossible to load dynamic firmware files.

why is this dynamic in the first place. It does not sound like dynamic data to me at all. This is like the WiFi MAC address(es) or Bluetooth BD_ADDR. They are all static information. The only difference is that they are on the host accessibly filesystem or storage and not on the device itself.

To be honest, for Bluetooth we solved this now. If the device is missing key information like the calibration data or BD_ADDR, then it comes up unconfigured. A userspace process can then go and load the right data into it and then the device becomes available as Bluetooth device.

Trying to use request_firmware to load some random data and insist on going through userspace helper for that sounds crazy to me. Especially since we are trying hard to get away from the userspace loader. Forcing to keep it for new stuff sounds backwards to me.

With the special Nokia partition in mind, why hasn't this been turned into a mountable filesystem or into a driver/subsystem that can access the data direct from the kernel. I advocated for this some time ago. Maybe there should be a special subsystem for access to these factory persistent information that drivers then just can access. I seem to remember that some systems provide these via ACPI. Why does the ARM platform has to be special here?

And the problem of getting Ethernet and WiFi MAC address and Bluetooth BD_ADDR comes up many many times. Why not have something generic here. And don't tell me request_firmware is that generic solution ;)

Regards

Marcel

Re: wl1251: NVS firmware data

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 3623 bytes --]

On Monday 08 December 2014 18:05:37 Marcel Holtmann wrote:
> Hi Pali,
> 
> >>>> On Saturday 06 December 2014 13:49:54 Pavel Machek wrote:
> >>>> /**
> >>>> 
> >>>> + * request_firmware_prefer_user: - prefer usermode
> >>>> helper for loading firmware + * @firmware_p: pointer to
> >>>> firmware image
> >>>> + * @name: name of firmware file
> >>>> + * @device: device for which firmware is being loaded
> >>>> + *
> >>>> + * This function works pretty much like
> >>>> request_firmware(), but it prefer + * usermode helper. If
> >>>> usermode helper fails then it fallback to direct access.
> >>>> + * Usefull for dynamic or model specific firmware data.
> >>>> + **/
> >>>> +int request_firmware_prefer_user(const struct firmware
> >>>> **firmware_p, +                           const char
> >>>> *name, struct device *device) +{
> >>>> +       int ret;
> >>>> +       __module_get(THIS_MODULE);
> >>>> +       ret = _request_firmware(firmware_p, name, device,
> >>>> +                               FW_OPT_UEVENT |
> >>>> FW_OPT_PREFER_USER); +       module_put(THIS_MODULE);
> >>>> +       return ret;
> >>>> +}
> >>>> +EXPORT_SYMBOL_GPL(request_firmware_prefer_user);
> >>> 
> >>> I'd like to introduce request_firmware_user() which only
> >>> requests firmware from user space, and this way is simpler
> >>> and more flexible since we have request_firmware_direct()
> >>> already.
> >> 
> >> Why would a driver care about what program provides the
> >> firmware?  It shouldn't at all, and we want to get rid of
> >> the userspace firmware loader, not encourage drivers to
> >> use it "exclusively" at all.
> > 
> > Do not remove it! Without userspace firmware loader it is
> > impossible to load dynamic firmware files.
> 
> why is this dynamic in the first place. It does not sound like
> dynamic data to me at all. This is like the WiFi MAC
> address(es) or Bluetooth BD_ADDR. They are all static
> information. The only difference is that they are on the host
> accessibly filesystem or storage and not on the device
> itself.
> 
> To be honest, for Bluetooth we solved this now. If the device
> is missing key information like the calibration data or
> BD_ADDR, then it comes up unconfigured. A userspace process
> can then go and load the right data into it and then the
> device becomes available as Bluetooth device.
> 
> Trying to use request_firmware to load some random data and
> insist on going through userspace helper for that sounds
> crazy to me. Especially since we are trying hard to get away
> from the userspace loader. Forcing to keep it for new stuff
> sounds backwards to me.
> 
> With the special Nokia partition in mind, why hasn't this been
> turned into a mountable filesystem or into a driver/subsystem
> that can access the data direct from the kernel. I advocated
> for this some time ago. Maybe there should be a special
> subsystem for access to these factory persistent information
> that drivers then just can access. I seem to remember that
> some systems provide these via ACPI. Why does the ARM
> platform has to be special here?
> 
> And the problem of getting Ethernet and WiFi MAC address and
> Bluetooth BD_ADDR comes up many many times. Why not have
> something generic here. And don't tell me request_firmware is
> that generic solution ;)
> 
> Regards
> 
> Marcel

Hi Marcel. I think you did not understand this problem. This 
discussion is not about mac address. Please read email thread 
again and if there are some unclear pars, then ask. Thanks!

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: wl1251: NVS firmware data

[Marcel Holtmann]

Hi Pali,

>>>>>> On Saturday 06 December 2014 13:49:54 Pavel Machek wrote:
>>>>>> /**
>>>>>> 
>>>>>> + * request_firmware_prefer_user: - prefer usermode
>>>>>> helper for loading firmware + * @firmware_p: pointer to
>>>>>> firmware image
>>>>>> + * @name: name of firmware file
>>>>>> + * @device: device for which firmware is being loaded
>>>>>> + *
>>>>>> + * This function works pretty much like
>>>>>> request_firmware(), but it prefer + * usermode helper. If
>>>>>> usermode helper fails then it fallback to direct access.
>>>>>> + * Usefull for dynamic or model specific firmware data.
>>>>>> + **/
>>>>>> +int request_firmware_prefer_user(const struct firmware
>>>>>> **firmware_p, +                           const char
>>>>>> *name, struct device *device) +{
>>>>>> +       int ret;
>>>>>> +       __module_get(THIS_MODULE);
>>>>>> +       ret = _request_firmware(firmware_p, name, device,
>>>>>> +                               FW_OPT_UEVENT |
>>>>>> FW_OPT_PREFER_USER); +       module_put(THIS_MODULE);
>>>>>> +       return ret;
>>>>>> +}
>>>>>> +EXPORT_SYMBOL_GPL(request_firmware_prefer_user);
>>>>> 
>>>>> I'd like to introduce request_firmware_user() which only
>>>>> requests firmware from user space, and this way is simpler
>>>>> and more flexible since we have request_firmware_direct()
>>>>> already.
>>>> 
>>>> Why would a driver care about what program provides the
>>>> firmware?  It shouldn't at all, and we want to get rid of
>>>> the userspace firmware loader, not encourage drivers to
>>>> use it "exclusively" at all.
>>> 
>>> Do not remove it! Without userspace firmware loader it is
>>> impossible to load dynamic firmware files.
>> 
>> why is this dynamic in the first place. It does not sound like
>> dynamic data to me at all. This is like the WiFi MAC
>> address(es) or Bluetooth BD_ADDR. They are all static
>> information. The only difference is that they are on the host
>> accessibly filesystem or storage and not on the device
>> itself.
>> 
>> To be honest, for Bluetooth we solved this now. If the device
>> is missing key information like the calibration data or
>> BD_ADDR, then it comes up unconfigured. A userspace process
>> can then go and load the right data into it and then the
>> device becomes available as Bluetooth device.
>> 
>> Trying to use request_firmware to load some random data and
>> insist on going through userspace helper for that sounds
>> crazy to me. Especially since we are trying hard to get away
>> from the userspace loader. Forcing to keep it for new stuff
>> sounds backwards to me.
>> 
>> With the special Nokia partition in mind, why hasn't this been
>> turned into a mountable filesystem or into a driver/subsystem
>> that can access the data direct from the kernel. I advocated
>> for this some time ago. Maybe there should be a special
>> subsystem for access to these factory persistent information
>> that drivers then just can access. I seem to remember that
>> some systems provide these via ACPI. Why does the ARM
>> platform has to be special here?
>> 
>> And the problem of getting Ethernet and WiFi MAC address and
>> Bluetooth BD_ADDR comes up many many times. Why not have
>> something generic here. And don't tell me request_firmware is
>> that generic solution ;)
>> 
>> Regards
>> 
>> Marcel
> 
> Hi Marcel. I think you did not understand this problem. This 
> discussion is not about mac address. Please read email thread 
> again and if there are some unclear pars, then ask. Thanks!

I think that I pretty clearly understand the problem. Calibration data, MAC address, what is the difference? For me this is all the same. It is data that is specific to a device or type of devices and it is stored somewhere else. In most cases in some immutable memory/flash area.

What you want is access to this data since the kernel driver needs it. Do I get this so far ;)

So my take is that request_firmware is not the right way to get this data. Or more precisely make sure that this data is available to kernel drivers. And what I am seeing here is that instead of actually solving the bigger problem, we just hack around it with request_firmware. Now surprisingly the request_firmware loads files directly from the kernel and all the hacks do not work anymore.

Regards

Marcel

Re: wl1251: NVS firmware data

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 6711 bytes --]

On Monday 08 December 2014 19:50:17 Marcel Holtmann wrote:
> Hi Pali,
> 
> >>>>>> On Saturday 06 December 2014 13:49:54 Pavel Machek
> >>>>>> wrote: /**
> >>>>>> 
> >>>>>> + * request_firmware_prefer_user: - prefer usermode
> >>>>>> helper for loading firmware + * @firmware_p: pointer to
> >>>>>> firmware image
> >>>>>> + * @name: name of firmware file
> >>>>>> + * @device: device for which firmware is being loaded
> >>>>>> + *
> >>>>>> + * This function works pretty much like
> >>>>>> request_firmware(), but it prefer + * usermode helper.
> >>>>>> If usermode helper fails then it fallback to direct
> >>>>>> access. + * Usefull for dynamic or model specific
> >>>>>> firmware data. + **/
> >>>>>> +int request_firmware_prefer_user(const struct firmware
> >>>>>> **firmware_p, +                           const char
> >>>>>> *name, struct device *device) +{
> >>>>>> +       int ret;
> >>>>>> +       __module_get(THIS_MODULE);
> >>>>>> +       ret = _request_firmware(firmware_p, name,
> >>>>>> device, +                               FW_OPT_UEVENT
> >>>>>> | FW_OPT_PREFER_USER); +      
> >>>>>> module_put(THIS_MODULE); +       return ret;
> >>>>>> +}
> >>>>>> +EXPORT_SYMBOL_GPL(request_firmware_prefer_user);
> >>>>> 
> >>>>> I'd like to introduce request_firmware_user() which only
> >>>>> requests firmware from user space, and this way is
> >>>>> simpler and more flexible since we have
> >>>>> request_firmware_direct() already.
> >>>> 
> >>>> Why would a driver care about what program provides the
> >>>> firmware?  It shouldn't at all, and we want to get rid of
> >>>> the userspace firmware loader, not encourage drivers to
> >>>> use it "exclusively" at all.
> >>> 
> >>> Do not remove it! Without userspace firmware loader it is
> >>> impossible to load dynamic firmware files.
> >> 
> >> why is this dynamic in the first place. It does not sound
> >> like dynamic data to me at all. This is like the WiFi MAC
> >> address(es) or Bluetooth BD_ADDR. They are all static
> >> information. The only difference is that they are on the
> >> host accessibly filesystem or storage and not on the
> >> device itself.
> >> 
> >> To be honest, for Bluetooth we solved this now. If the
> >> device is missing key information like the calibration
> >> data or BD_ADDR, then it comes up unconfigured. A
> >> userspace process can then go and load the right data into
> >> it and then the device becomes available as Bluetooth
> >> device.
> >> 
> >> Trying to use request_firmware to load some random data and
> >> insist on going through userspace helper for that sounds
> >> crazy to me. Especially since we are trying hard to get
> >> away from the userspace loader. Forcing to keep it for new
> >> stuff sounds backwards to me.
> >> 
> >> With the special Nokia partition in mind, why hasn't this
> >> been turned into a mountable filesystem or into a
> >> driver/subsystem that can access the data direct from the
> >> kernel. I advocated for this some time ago. Maybe there
> >> should be a special subsystem for access to these factory
> >> persistent information that drivers then just can access.
> >> I seem to remember that some systems provide these via
> >> ACPI. Why does the ARM platform has to be special here?
> >> 
> >> And the problem of getting Ethernet and WiFi MAC address
> >> and Bluetooth BD_ADDR comes up many many times. Why not
> >> have something generic here. And don't tell me
> >> request_firmware is that generic solution ;)
> >> 
> >> Regards
> >> 
> >> Marcel
> > 
> > Hi Marcel. I think you did not understand this problem. This
> > discussion is not about mac address. Please read email
> > thread again and if there are some unclear pars, then ask.
> > Thanks!
> 
> I think that I pretty clearly understand the problem.
> Calibration data, MAC address, what is the difference? For me
> this is all the same. It is data that is specific to a device
> or type of devices and it is stored somewhere else. In most
> cases in some immutable memory/flash area.
> 

Those calibration data (in form of binary NVS firmware file) 
needs to be sent to wl1251 chip. Mac address is not needed at 
this step (and kernel generate some random if is not provided).

(Just to note wl1271 driver loads both MAC address and NVS data 
via one firmware file which is prepared by userspace, but this 
discussion is about wl1251...)

> What you want is access to this data since the kernel driver
> needs it. Do I get this so far ;)
> 

Yes, we need to provide NVS data to kernel when kernel ask for 
them.

> So my take is that request_firmware is not the right way to
> get this data. Or more precisely make sure that this data is
> available to kernel drivers. And what I am seeing here is
> that instead of actually solving the bigger problem, we just
> hack around it with request_firmware. Now surprisingly the
> request_firmware loads files directly from the kernel and all
> the hacks do not work anymore.
> 
> Regards
> 
> Marcel

Just read emails again...

Our problem is:

linux-firmware.git tree provides two binary firmware files:

ti-connectivity/wl1251-fw.bin
ti-connectivity/wl1251-nvs.bin

First is firmware file, second NVS file with generic calibration 
data. Kernel driver wl1251 now loads both firmware files via 
request_firmware. Generic calibration data are enough for wl1251 
chip (it should work). But devices have own calibration data 
stored somewhere else.

On Nokia N900 NVS data are generated on-the-fly from some bytes 
from CAL (/dev/mtd1), from state of cellular network and from 
some other regulation settings.

So I think that files stored in linux-firmware.git tree (which 
are also installed into /lib/firmware/) should be loaded with 
request_firmware function. Or not? Do you think something else? 
What other developers think?

I'm against kernel driver for CAL (/dev/mtd1) for more reasons:

1) we have userspace open source code, but licensed under GPLv3. 
And until kernel change license, we cannot include it.

2) NVS data are (probably) not in one place, plus they depends on 
something other.

3) If manufacture XYZ create new device with its own storage 
format of calibration data this means that correct solution for 
XYZ is also to implement new kernel fs driver for its own format. 
Do you really want to have in kernel all those drivers for all 
different (proprietary) storage formats?

4) It does not help us with existence of generic file 
/lib/firmware/ti-connectivity/wl1251-nvs.bin which comes from 
linux-firmware.git tree.

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: wl1251: NVS firmware data

[Dan Williams]

On Mon, 2014-12-08 at 20:15 +0100, Pali Rohár wrote:
> On Monday 08 December 2014 19:50:17 Marcel Holtmann wrote:
> > Hi Pali,
> > 
> > >>>>>> On Saturday 06 December 2014 13:49:54 Pavel Machek
> > >>>>>> wrote: /**
> > >>>>>> 
> > >>>>>> + * request_firmware_prefer_user: - prefer usermode
> > >>>>>> helper for loading firmware + * @firmware_p: pointer to
> > >>>>>> firmware image
> > >>>>>> + * @name: name of firmware file
> > >>>>>> + * @device: device for which firmware is being loaded
> > >>>>>> + *
> > >>>>>> + * This function works pretty much like
> > >>>>>> request_firmware(), but it prefer + * usermode helper.
> > >>>>>> If usermode helper fails then it fallback to direct
> > >>>>>> access. + * Usefull for dynamic or model specific
> > >>>>>> firmware data. + **/
> > >>>>>> +int request_firmware_prefer_user(const struct firmware
> > >>>>>> **firmware_p, +                           const char
> > >>>>>> *name, struct device *device) +{
> > >>>>>> +       int ret;
> > >>>>>> +       __module_get(THIS_MODULE);
> > >>>>>> +       ret = _request_firmware(firmware_p, name,
> > >>>>>> device, +                               FW_OPT_UEVENT
> > >>>>>> | FW_OPT_PREFER_USER); +      
> > >>>>>> module_put(THIS_MODULE); +       return ret;
> > >>>>>> +}
> > >>>>>> +EXPORT_SYMBOL_GPL(request_firmware_prefer_user);
> > >>>>> 
> > >>>>> I'd like to introduce request_firmware_user() which only
> > >>>>> requests firmware from user space, and this way is
> > >>>>> simpler and more flexible since we have
> > >>>>> request_firmware_direct() already.
> > >>>> 
> > >>>> Why would a driver care about what program provides the
> > >>>> firmware?  It shouldn't at all, and we want to get rid of
> > >>>> the userspace firmware loader, not encourage drivers to
> > >>>> use it "exclusively" at all.
> > >>> 
> > >>> Do not remove it! Without userspace firmware loader it is
> > >>> impossible to load dynamic firmware files.
> > >> 
> > >> why is this dynamic in the first place. It does not sound
> > >> like dynamic data to me at all. This is like the WiFi MAC
> > >> address(es) or Bluetooth BD_ADDR. They are all static
> > >> information. The only difference is that they are on the
> > >> host accessibly filesystem or storage and not on the
> > >> device itself.
> > >> 
> > >> To be honest, for Bluetooth we solved this now. If the
> > >> device is missing key information like the calibration
> > >> data or BD_ADDR, then it comes up unconfigured. A
> > >> userspace process can then go and load the right data into
> > >> it and then the device becomes available as Bluetooth
> > >> device.
> > >> 
> > >> Trying to use request_firmware to load some random data and
> > >> insist on going through userspace helper for that sounds
> > >> crazy to me. Especially since we are trying hard to get
> > >> away from the userspace loader. Forcing to keep it for new
> > >> stuff sounds backwards to me.
> > >> 
> > >> With the special Nokia partition in mind, why hasn't this
> > >> been turned into a mountable filesystem or into a
> > >> driver/subsystem that can access the data direct from the
> > >> kernel. I advocated for this some time ago. Maybe there
> > >> should be a special subsystem for access to these factory
> > >> persistent information that drivers then just can access.
> > >> I seem to remember that some systems provide these via
> > >> ACPI. Why does the ARM platform has to be special here?
> > >> 
> > >> And the problem of getting Ethernet and WiFi MAC address
> > >> and Bluetooth BD_ADDR comes up many many times. Why not
> > >> have something generic here. And don't tell me
> > >> request_firmware is that generic solution ;)
> > >> 
> > >> Regards
> > >> 
> > >> Marcel
> > > 
> > > Hi Marcel. I think you did not understand this problem. This
> > > discussion is not about mac address. Please read email
> > > thread again and if there are some unclear pars, then ask.
> > > Thanks!
> > 
> > I think that I pretty clearly understand the problem.
> > Calibration data, MAC address, what is the difference? For me
> > this is all the same. It is data that is specific to a device
> > or type of devices and it is stored somewhere else. In most
> > cases in some immutable memory/flash area.
> > 
> 
> Those calibration data (in form of binary NVS firmware file) 
> needs to be sent to wl1251 chip. Mac address is not needed at 
> this step (and kernel generate some random if is not provided).
> 
> (Just to note wl1271 driver loads both MAC address and NVS data 
> via one firmware file which is prepared by userspace, but this 
> discussion is about wl1251...)
> 
> > What you want is access to this data since the kernel driver
> > needs it. Do I get this so far ;)
> > 
> 
> Yes, we need to provide NVS data to kernel when kernel ask for 
> them.
> 
> > So my take is that request_firmware is not the right way to
> > get this data. Or more precisely make sure that this data is
> > available to kernel drivers. And what I am seeing here is
> > that instead of actually solving the bigger problem, we just
> > hack around it with request_firmware. Now surprisingly the
> > request_firmware loads files directly from the kernel and all
> > the hacks do not work anymore.
> > 
> > Regards
> > 
> > Marcel
> 
> Just read emails again...
> 
> Our problem is:
> 
> linux-firmware.git tree provides two binary firmware files:
> 
> ti-connectivity/wl1251-fw.bin
> ti-connectivity/wl1251-nvs.bin
> 
> First is firmware file, second NVS file with generic calibration 
> data. Kernel driver wl1251 now loads both firmware files via 
> request_firmware. Generic calibration data are enough for wl1251 
> chip (it should work). But devices have own calibration data 
> stored somewhere else.
> 
> On Nokia N900 NVS data are generated on-the-fly from some bytes 
> from CAL (/dev/mtd1), from state of cellular network and from 
> some other regulation settings.
> 
> So I think that files stored in linux-firmware.git tree (which 
> are also installed into /lib/firmware/) should be loaded with 
> request_firmware function. Or not? Do you think something else? 
> What other developers think?
> 
> I'm against kernel driver for CAL (/dev/mtd1) for more reasons:
> 
> 1) we have userspace open source code, but licensed under GPLv3. 
> And until kernel change license, we cannot include it.
> 
> 2) NVS data are (probably) not in one place, plus they depends on 
> something other.
> 
> 3) If manufacture XYZ create new device with its own storage 
> format of calibration data this means that correct solution for 
> XYZ is also to implement new kernel fs driver for its own format. 
> Do you really want to have in kernel all those drivers for all 
> different (proprietary) storage formats?
> 
> 4) It does not help us with existence of generic file 
> /lib/firmware/ti-connectivity/wl1251-nvs.bin which comes from 
> linux-firmware.git tree.

a) change driver to prefer a new "wl1251-nvs-n900.bin" file, but fall
back to "wl1251-nvs.bin" if the first one isn't present
b) have a "wl1251-cal-nvs-update" service that, if wl1521-nvs-n900.bin
is *not* present mounts the CAL MTD, reads the data, writes it out into
wl1521-nvs-n900.bin, and the rmmod/modprobes the driver

and done?  Stuff that's not N900 just wouldn't ship the update service
and would proceed like none of this happened.

Dan

Re: wl1251: NVS firmware data

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 8876 bytes --]

On Monday 08 December 2014 20:26:53 Dan Williams wrote:
> On Mon, 2014-12-08 at 20:15 +0100, Pali Rohár wrote:
> > On Monday 08 December 2014 19:50:17 Marcel Holtmann wrote:
> > > Hi Pali,
> > > 
> > > >>>>>> On Saturday 06 December 2014 13:49:54 Pavel Machek
> > > >>>>>> wrote: /**
> > > >>>>>> 
> > > >>>>>> + * request_firmware_prefer_user: - prefer usermode
> > > >>>>>> helper for loading firmware + * @firmware_p:
> > > >>>>>> pointer to firmware image
> > > >>>>>> + * @name: name of firmware file
> > > >>>>>> + * @device: device for which firmware is being
> > > >>>>>> loaded + *
> > > >>>>>> + * This function works pretty much like
> > > >>>>>> request_firmware(), but it prefer + * usermode
> > > >>>>>> helper. If usermode helper fails then it fallback
> > > >>>>>> to direct access. + * Usefull for dynamic or model
> > > >>>>>> specific firmware data. + **/
> > > >>>>>> +int request_firmware_prefer_user(const struct
> > > >>>>>> firmware **firmware_p, +                          
> > > >>>>>> const char *name, struct device *device) +{
> > > >>>>>> +       int ret;
> > > >>>>>> +       __module_get(THIS_MODULE);
> > > >>>>>> +       ret = _request_firmware(firmware_p, name,
> > > >>>>>> device, +                              
> > > >>>>>> FW_OPT_UEVENT
> > > >>>>>> 
> > > >>>>>> | FW_OPT_PREFER_USER); +
> > > >>>>>> 
> > > >>>>>> module_put(THIS_MODULE); +       return ret;
> > > >>>>>> +}
> > > >>>>>> +EXPORT_SYMBOL_GPL(request_firmware_prefer_user);
> > > >>>>> 
> > > >>>>> I'd like to introduce request_firmware_user() which
> > > >>>>> only requests firmware from user space, and this
> > > >>>>> way is simpler and more flexible since we have
> > > >>>>> request_firmware_direct() already.
> > > >>>> 
> > > >>>> Why would a driver care about what program provides
> > > >>>> the firmware?  It shouldn't at all, and we want to
> > > >>>> get rid of the userspace firmware loader, not
> > > >>>> encourage drivers to use it "exclusively" at all.
> > > >>> 
> > > >>> Do not remove it! Without userspace firmware loader it
> > > >>> is impossible to load dynamic firmware files.
> > > >> 
> > > >> why is this dynamic in the first place. It does not
> > > >> sound like dynamic data to me at all. This is like the
> > > >> WiFi MAC address(es) or Bluetooth BD_ADDR. They are
> > > >> all static information. The only difference is that
> > > >> they are on the host accessibly filesystem or storage
> > > >> and not on the device itself.
> > > >> 
> > > >> To be honest, for Bluetooth we solved this now. If the
> > > >> device is missing key information like the calibration
> > > >> data or BD_ADDR, then it comes up unconfigured. A
> > > >> userspace process can then go and load the right data
> > > >> into it and then the device becomes available as
> > > >> Bluetooth device.
> > > >> 
> > > >> Trying to use request_firmware to load some random data
> > > >> and insist on going through userspace helper for that
> > > >> sounds crazy to me. Especially since we are trying
> > > >> hard to get away from the userspace loader. Forcing to
> > > >> keep it for new stuff sounds backwards to me.
> > > >> 
> > > >> With the special Nokia partition in mind, why hasn't
> > > >> this been turned into a mountable filesystem or into a
> > > >> driver/subsystem that can access the data direct from
> > > >> the kernel. I advocated for this some time ago. Maybe
> > > >> there should be a special subsystem for access to
> > > >> these factory persistent information that drivers then
> > > >> just can access. I seem to remember that some systems
> > > >> provide these via ACPI. Why does the ARM platform has
> > > >> to be special here?
> > > >> 
> > > >> And the problem of getting Ethernet and WiFi MAC
> > > >> address and Bluetooth BD_ADDR comes up many many
> > > >> times. Why not have something generic here. And don't
> > > >> tell me request_firmware is that generic solution ;)
> > > >> 
> > > >> Regards
> > > >> 
> > > >> Marcel
> > > > 
> > > > Hi Marcel. I think you did not understand this problem.
> > > > This discussion is not about mac address. Please read
> > > > email thread again and if there are some unclear pars,
> > > > then ask. Thanks!
> > > 
> > > I think that I pretty clearly understand the problem.
> > > Calibration data, MAC address, what is the difference? For
> > > me this is all the same. It is data that is specific to a
> > > device or type of devices and it is stored somewhere
> > > else. In most cases in some immutable memory/flash area.
> > 
> > Those calibration data (in form of binary NVS firmware file)
> > needs to be sent to wl1251 chip. Mac address is not needed
> > at this step (and kernel generate some random if is not
> > provided).
> > 
> > (Just to note wl1271 driver loads both MAC address and NVS
> > data via one firmware file which is prepared by userspace,
> > but this discussion is about wl1251...)
> > 
> > > What you want is access to this data since the kernel
> > > driver needs it. Do I get this so far ;)
> > 
> > Yes, we need to provide NVS data to kernel when kernel ask
> > for them.
> > 
> > > So my take is that request_firmware is not the right way
> > > to get this data. Or more precisely make sure that this
> > > data is available to kernel drivers. And what I am seeing
> > > here is that instead of actually solving the bigger
> > > problem, we just hack around it with request_firmware.
> > > Now surprisingly the request_firmware loads files
> > > directly from the kernel and all the hacks do not work
> > > anymore.
> > > 
> > > Regards
> > > 
> > > Marcel
> > 
> > Just read emails again...
> > 
> > Our problem is:
> > 
> > linux-firmware.git tree provides two binary firmware files:
> > 
> > ti-connectivity/wl1251-fw.bin
> > ti-connectivity/wl1251-nvs.bin
> > 
> > First is firmware file, second NVS file with generic
> > calibration data. Kernel driver wl1251 now loads both
> > firmware files via request_firmware. Generic calibration
> > data are enough for wl1251 chip (it should work). But
> > devices have own calibration data stored somewhere else.
> > 
> > On Nokia N900 NVS data are generated on-the-fly from some
> > bytes from CAL (/dev/mtd1), from state of cellular network
> > and from some other regulation settings.
> > 
> > So I think that files stored in linux-firmware.git tree
> > (which are also installed into /lib/firmware/) should be
> > loaded with request_firmware function. Or not? Do you think
> > something else? What other developers think?
> > 
> > I'm against kernel driver for CAL (/dev/mtd1) for more
> > reasons:
> > 
> > 1) we have userspace open source code, but licensed under
> > GPLv3. And until kernel change license, we cannot include
> > it.
> > 
> > 2) NVS data are (probably) not in one place, plus they
> > depends on something other.
> > 
> > 3) If manufacture XYZ create new device with its own storage
> > format of calibration data this means that correct solution
> > for XYZ is also to implement new kernel fs driver for its
> > own format. Do you really want to have in kernel all those
> > drivers for all different (proprietary) storage formats?
> > 
> > 4) It does not help us with existence of generic file
> > /lib/firmware/ti-connectivity/wl1251-nvs.bin which comes
> > from linux-firmware.git tree.
> 
> a) change driver to prefer a new "wl1251-nvs-n900.bin" file,

Why to "*-n900.bin" ? wl1251 driver is used on other devices too.

> but fall back to "wl1251-nvs.bin" if the first one isn't
> present

> b) have a "wl1251-cal-nvs-update" service that, if
> wl1521-nvs-n900.bin is *not* present mounts the CAL MTD,
> reads the data, writes it out into wl1521-nvs-n900.bin, and
> the rmmod/modprobes the driver
> 

Quote:
> On Nokia N900 NVS data are generated on-the-fly from some bytes 
> from CAL (/dev/mtd1), from state of cellular network and from 
> some other regulation settings.

This basically means to rewrite it every boot or everytime when 
country was changed (for regulation settings). And Ii really do 
not want to do that.

And rmmod is not working on statically linked drivers into 
zImage. So this is not solution.

> and done?  Stuff that's not N900 just wouldn't ship the update
> service and would proceed like none of this happened.
> 
> Dan

Again, what is wrong with userspace firmware helper? I think that 
it fix this problem in a clean way without any hacks (like CAL in 
kernel or creating new FS specially for parsing NVS and so on) in 
kernel. And in userspace we can implement program which generate 
NVS firmware data on-the-fly and send them to kernel in 
compatible format of ti-connectivity/wl1251-nvs.bin

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: wl1251: NVS firmware data

[Marcel Holtmann]

Hi Pali,

>>>>>>>>>> On Saturday 06 December 2014 13:49:54 Pavel Machek
>>>>>>>>>> wrote: /**
>>>>>>>>>> 
>>>>>>>>>> + * request_firmware_prefer_user: - prefer usermode
>>>>>>>>>> helper for loading firmware + * @firmware_p:
>>>>>>>>>> pointer to firmware image
>>>>>>>>>> + * @name: name of firmware file
>>>>>>>>>> + * @device: device for which firmware is being
>>>>>>>>>> loaded + *
>>>>>>>>>> + * This function works pretty much like
>>>>>>>>>> request_firmware(), but it prefer + * usermode
>>>>>>>>>> helper. If usermode helper fails then it fallback
>>>>>>>>>> to direct access. + * Usefull for dynamic or model
>>>>>>>>>> specific firmware data. + **/
>>>>>>>>>> +int request_firmware_prefer_user(const struct
>>>>>>>>>> firmware **firmware_p, +                          
>>>>>>>>>> const char *name, struct device *device) +{
>>>>>>>>>> +       int ret;
>>>>>>>>>> +       __module_get(THIS_MODULE);
>>>>>>>>>> +       ret = _request_firmware(firmware_p, name,
>>>>>>>>>> device, +                              
>>>>>>>>>> FW_OPT_UEVENT
>>>>>>>>>> 
>>>>>>>>>> | FW_OPT_PREFER_USER); +
>>>>>>>>>> 
>>>>>>>>>> module_put(THIS_MODULE); +       return ret;
>>>>>>>>>> +}
>>>>>>>>>> +EXPORT_SYMBOL_GPL(request_firmware_prefer_user);
>>>>>>>>> 
>>>>>>>>> I'd like to introduce request_firmware_user() which
>>>>>>>>> only requests firmware from user space, and this
>>>>>>>>> way is simpler and more flexible since we have
>>>>>>>>> request_firmware_direct() already.
>>>>>>>> 
>>>>>>>> Why would a driver care about what program provides
>>>>>>>> the firmware?  It shouldn't at all, and we want to
>>>>>>>> get rid of the userspace firmware loader, not
>>>>>>>> encourage drivers to use it "exclusively" at all.
>>>>>>> 
>>>>>>> Do not remove it! Without userspace firmware loader it
>>>>>>> is impossible to load dynamic firmware files.
>>>>>> 
>>>>>> why is this dynamic in the first place. It does not
>>>>>> sound like dynamic data to me at all. This is like the
>>>>>> WiFi MAC address(es) or Bluetooth BD_ADDR. They are
>>>>>> all static information. The only difference is that
>>>>>> they are on the host accessibly filesystem or storage
>>>>>> and not on the device itself.
>>>>>> 
>>>>>> To be honest, for Bluetooth we solved this now. If the
>>>>>> device is missing key information like the calibration
>>>>>> data or BD_ADDR, then it comes up unconfigured. A
>>>>>> userspace process can then go and load the right data
>>>>>> into it and then the device becomes available as
>>>>>> Bluetooth device.
>>>>>> 
>>>>>> Trying to use request_firmware to load some random data
>>>>>> and insist on going through userspace helper for that
>>>>>> sounds crazy to me. Especially since we are trying
>>>>>> hard to get away from the userspace loader. Forcing to
>>>>>> keep it for new stuff sounds backwards to me.
>>>>>> 
>>>>>> With the special Nokia partition in mind, why hasn't
>>>>>> this been turned into a mountable filesystem or into a
>>>>>> driver/subsystem that can access the data direct from
>>>>>> the kernel. I advocated for this some time ago. Maybe
>>>>>> there should be a special subsystem for access to
>>>>>> these factory persistent information that drivers then
>>>>>> just can access. I seem to remember that some systems
>>>>>> provide these via ACPI. Why does the ARM platform has
>>>>>> to be special here?
>>>>>> 
>>>>>> And the problem of getting Ethernet and WiFi MAC
>>>>>> address and Bluetooth BD_ADDR comes up many many
>>>>>> times. Why not have something generic here. And don't
>>>>>> tell me request_firmware is that generic solution ;)
>>>>>> 
>>>>>> Regards
>>>>>> 
>>>>>> Marcel
>>>>> 
>>>>> Hi Marcel. I think you did not understand this problem.
>>>>> This discussion is not about mac address. Please read
>>>>> email thread again and if there are some unclear pars,
>>>>> then ask. Thanks!
>>>> 
>>>> I think that I pretty clearly understand the problem.
>>>> Calibration data, MAC address, what is the difference? For
>>>> me this is all the same. It is data that is specific to a
>>>> device or type of devices and it is stored somewhere
>>>> else. In most cases in some immutable memory/flash area.
>>> 
>>> Those calibration data (in form of binary NVS firmware file)
>>> needs to be sent to wl1251 chip. Mac address is not needed
>>> at this step (and kernel generate some random if is not
>>> provided).
>>> 
>>> (Just to note wl1271 driver loads both MAC address and NVS
>>> data via one firmware file which is prepared by userspace,
>>> but this discussion is about wl1251...)
>>> 
>>>> What you want is access to this data since the kernel
>>>> driver needs it. Do I get this so far ;)
>>> 
>>> Yes, we need to provide NVS data to kernel when kernel ask
>>> for them.
>>> 
>>>> So my take is that request_firmware is not the right way
>>>> to get this data. Or more precisely make sure that this
>>>> data is available to kernel drivers. And what I am seeing
>>>> here is that instead of actually solving the bigger
>>>> problem, we just hack around it with request_firmware.
>>>> Now surprisingly the request_firmware loads files
>>>> directly from the kernel and all the hacks do not work
>>>> anymore.
>>>> 
>>>> Regards
>>>> 
>>>> Marcel
>>> 
>>> Just read emails again...
>>> 
>>> Our problem is:
>>> 
>>> linux-firmware.git tree provides two binary firmware files:
>>> 
>>> ti-connectivity/wl1251-fw.bin
>>> ti-connectivity/wl1251-nvs.bin
>>> 
>>> First is firmware file, second NVS file with generic
>>> calibration data. Kernel driver wl1251 now loads both
>>> firmware files via request_firmware. Generic calibration
>>> data are enough for wl1251 chip (it should work). But
>>> devices have own calibration data stored somewhere else.
>>> 
>>> On Nokia N900 NVS data are generated on-the-fly from some
>>> bytes from CAL (/dev/mtd1), from state of cellular network
>>> and from some other regulation settings.
>>> 
>>> So I think that files stored in linux-firmware.git tree
>>> (which are also installed into /lib/firmware/) should be
>>> loaded with request_firmware function. Or not? Do you think
>>> something else? What other developers think?
>>> 
>>> I'm against kernel driver for CAL (/dev/mtd1) for more
>>> reasons:
>>> 
>>> 1) we have userspace open source code, but licensed under
>>> GPLv3. And until kernel change license, we cannot include
>>> it.
>>> 
>>> 2) NVS data are (probably) not in one place, plus they
>>> depends on something other.
>>> 
>>> 3) If manufacture XYZ create new device with its own storage
>>> format of calibration data this means that correct solution
>>> for XYZ is also to implement new kernel fs driver for its
>>> own format. Do you really want to have in kernel all those
>>> drivers for all different (proprietary) storage formats?
>>> 
>>> 4) It does not help us with existence of generic file
>>> /lib/firmware/ti-connectivity/wl1251-nvs.bin which comes
>>> from linux-firmware.git tree.
>> 
>> a) change driver to prefer a new "wl1251-nvs-n900.bin" file,
> 
> Why to "*-n900.bin" ? wl1251 driver is used on other devices too.
> 
>> but fall back to "wl1251-nvs.bin" if the first one isn't
>> present
> 
>> b) have a "wl1251-cal-nvs-update" service that, if
>> wl1521-nvs-n900.bin is *not* present mounts the CAL MTD,
>> reads the data, writes it out into wl1521-nvs-n900.bin, and
>> the rmmod/modprobes the driver
>> 
> 
> Quote:
>> On Nokia N900 NVS data are generated on-the-fly from some bytes 
>> from CAL (/dev/mtd1), from state of cellular network and from 
>> some other regulation settings.
> 
> This basically means to rewrite it every boot or everytime when 
> country was changed (for regulation settings). And Ii really do 
> not want to do that.
> 
> And rmmod is not working on statically linked drivers into 
> zImage. So this is not solution.

actually module removal is still considered a race condition. If re-loading a module is part of your solution, then you are already heading into the wrong direction.

WiFi subsystem have a solution for handling regulatory enforcement. I don't know why would you try to invent that same via NVS files.

> 
>> and done?  Stuff that's not N900 just wouldn't ship the update
>> service and would proceed like none of this happened.
>> 
>> Dan
> 
> Again, what is wrong with userspace firmware helper? I think that 
> it fix this problem in a clean way without any hacks (like CAL in 
> kernel or creating new FS specially for parsing NVS and so on) in 
> kernel. And in userspace we can implement program which generate 
> NVS firmware data on-the-fly and send them to kernel in 
> compatible format of ti-connectivity/wl1251-nvs.bin

How do you run a userspace firmware helper if the root filesystem has not yet been mounted? How do you run userspace helper during resume?

Honestly for drivers linked statically into the kernel, the best approach is that they stay unconfigured until userspace is available and can run a tool to provide the correct data.

Regards

Marcel

Re: wl1251: NVS firmware data

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 10243 bytes --]

On Monday 08 December 2014 20:46:07 Marcel Holtmann wrote:
> Hi Pali,
> 
> >>>>>>>>>> On Saturday 06 December 2014 13:49:54 Pavel Machek
> >>>>>>>>>> wrote: /**
> >>>>>>>>>> 
> >>>>>>>>>> + * request_firmware_prefer_user: - prefer usermode
> >>>>>>>>>> helper for loading firmware + * @firmware_p:
> >>>>>>>>>> pointer to firmware image
> >>>>>>>>>> + * @name: name of firmware file
> >>>>>>>>>> + * @device: device for which firmware is being
> >>>>>>>>>> loaded + *
> >>>>>>>>>> + * This function works pretty much like
> >>>>>>>>>> request_firmware(), but it prefer + * usermode
> >>>>>>>>>> helper. If usermode helper fails then it fallback
> >>>>>>>>>> to direct access. + * Usefull for dynamic or model
> >>>>>>>>>> specific firmware data. + **/
> >>>>>>>>>> +int request_firmware_prefer_user(const struct
> >>>>>>>>>> firmware **firmware_p, +
> >>>>>>>>>> const char *name, struct device *device) +{
> >>>>>>>>>> +       int ret;
> >>>>>>>>>> +       __module_get(THIS_MODULE);
> >>>>>>>>>> +       ret = _request_firmware(firmware_p, name,
> >>>>>>>>>> device, +
> >>>>>>>>>> FW_OPT_UEVENT
> >>>>>>>>>> 
> >>>>>>>>>> | FW_OPT_PREFER_USER); +
> >>>>>>>>>> 
> >>>>>>>>>> module_put(THIS_MODULE); +       return ret;
> >>>>>>>>>> +}
> >>>>>>>>>> +EXPORT_SYMBOL_GPL(request_firmware_prefer_user);
> >>>>>>>>> 
> >>>>>>>>> I'd like to introduce request_firmware_user() which
> >>>>>>>>> only requests firmware from user space, and this
> >>>>>>>>> way is simpler and more flexible since we have
> >>>>>>>>> request_firmware_direct() already.
> >>>>>>>> 
> >>>>>>>> Why would a driver care about what program provides
> >>>>>>>> the firmware?  It shouldn't at all, and we want to
> >>>>>>>> get rid of the userspace firmware loader, not
> >>>>>>>> encourage drivers to use it "exclusively" at all.
> >>>>>>> 
> >>>>>>> Do not remove it! Without userspace firmware loader it
> >>>>>>> is impossible to load dynamic firmware files.
> >>>>>> 
> >>>>>> why is this dynamic in the first place. It does not
> >>>>>> sound like dynamic data to me at all. This is like the
> >>>>>> WiFi MAC address(es) or Bluetooth BD_ADDR. They are
> >>>>>> all static information. The only difference is that
> >>>>>> they are on the host accessibly filesystem or storage
> >>>>>> and not on the device itself.
> >>>>>> 
> >>>>>> To be honest, for Bluetooth we solved this now. If the
> >>>>>> device is missing key information like the calibration
> >>>>>> data or BD_ADDR, then it comes up unconfigured. A
> >>>>>> userspace process can then go and load the right data
> >>>>>> into it and then the device becomes available as
> >>>>>> Bluetooth device.
> >>>>>> 
> >>>>>> Trying to use request_firmware to load some random data
> >>>>>> and insist on going through userspace helper for that
> >>>>>> sounds crazy to me. Especially since we are trying
> >>>>>> hard to get away from the userspace loader. Forcing to
> >>>>>> keep it for new stuff sounds backwards to me.
> >>>>>> 
> >>>>>> With the special Nokia partition in mind, why hasn't
> >>>>>> this been turned into a mountable filesystem or into a
> >>>>>> driver/subsystem that can access the data direct from
> >>>>>> the kernel. I advocated for this some time ago. Maybe
> >>>>>> there should be a special subsystem for access to
> >>>>>> these factory persistent information that drivers then
> >>>>>> just can access. I seem to remember that some systems
> >>>>>> provide these via ACPI. Why does the ARM platform has
> >>>>>> to be special here?
> >>>>>> 
> >>>>>> And the problem of getting Ethernet and WiFi MAC
> >>>>>> address and Bluetooth BD_ADDR comes up many many
> >>>>>> times. Why not have something generic here. And don't
> >>>>>> tell me request_firmware is that generic solution ;)
> >>>>>> 
> >>>>>> Regards
> >>>>>> 
> >>>>>> Marcel
> >>>>> 
> >>>>> Hi Marcel. I think you did not understand this problem.
> >>>>> This discussion is not about mac address. Please read
> >>>>> email thread again and if there are some unclear pars,
> >>>>> then ask. Thanks!
> >>>> 
> >>>> I think that I pretty clearly understand the problem.
> >>>> Calibration data, MAC address, what is the difference?
> >>>> For me this is all the same. It is data that is specific
> >>>> to a device or type of devices and it is stored
> >>>> somewhere else. In most cases in some immutable
> >>>> memory/flash area.
> >>> 
> >>> Those calibration data (in form of binary NVS firmware
> >>> file) needs to be sent to wl1251 chip. Mac address is not
> >>> needed at this step (and kernel generate some random if
> >>> is not provided).
> >>> 
> >>> (Just to note wl1271 driver loads both MAC address and NVS
> >>> data via one firmware file which is prepared by userspace,
> >>> but this discussion is about wl1251...)
> >>> 
> >>>> What you want is access to this data since the kernel
> >>>> driver needs it. Do I get this so far ;)
> >>> 
> >>> Yes, we need to provide NVS data to kernel when kernel ask
> >>> for them.
> >>> 
> >>>> So my take is that request_firmware is not the right way
> >>>> to get this data. Or more precisely make sure that this
> >>>> data is available to kernel drivers. And what I am seeing
> >>>> here is that instead of actually solving the bigger
> >>>> problem, we just hack around it with request_firmware.
> >>>> Now surprisingly the request_firmware loads files
> >>>> directly from the kernel and all the hacks do not work
> >>>> anymore.
> >>>> 
> >>>> Regards
> >>>> 
> >>>> Marcel
> >>> 
> >>> Just read emails again...
> >>> 
> >>> Our problem is:
> >>> 
> >>> linux-firmware.git tree provides two binary firmware
> >>> files:
> >>> 
> >>> ti-connectivity/wl1251-fw.bin
> >>> ti-connectivity/wl1251-nvs.bin
> >>> 
> >>> First is firmware file, second NVS file with generic
> >>> calibration data. Kernel driver wl1251 now loads both
> >>> firmware files via request_firmware. Generic calibration
> >>> data are enough for wl1251 chip (it should work). But
> >>> devices have own calibration data stored somewhere else.
> >>> 
> >>> On Nokia N900 NVS data are generated on-the-fly from some
> >>> bytes from CAL (/dev/mtd1), from state of cellular network
> >>> and from some other regulation settings.
> >>> 
> >>> So I think that files stored in linux-firmware.git tree
> >>> (which are also installed into /lib/firmware/) should be
> >>> loaded with request_firmware function. Or not? Do you
> >>> think something else? What other developers think?
> >>> 
> >>> I'm against kernel driver for CAL (/dev/mtd1) for more
> >>> reasons:
> >>> 
> >>> 1) we have userspace open source code, but licensed under
> >>> GPLv3. And until kernel change license, we cannot include
> >>> it.
> >>> 
> >>> 2) NVS data are (probably) not in one place, plus they
> >>> depends on something other.
> >>> 
> >>> 3) If manufacture XYZ create new device with its own
> >>> storage format of calibration data this means that
> >>> correct solution for XYZ is also to implement new kernel
> >>> fs driver for its own format. Do you really want to have
> >>> in kernel all those drivers for all different
> >>> (proprietary) storage formats?
> >>> 
> >>> 4) It does not help us with existence of generic file
> >>> /lib/firmware/ti-connectivity/wl1251-nvs.bin which comes
> >>> from linux-firmware.git tree.
> >> 
> >> a) change driver to prefer a new "wl1251-nvs-n900.bin"
> >> file,
> > 
> > Why to "*-n900.bin" ? wl1251 driver is used on other devices
> > too.
> > 
> >> but fall back to "wl1251-nvs.bin" if the first one isn't
> >> present
> >> 
> >> b) have a "wl1251-cal-nvs-update" service that, if
> >> wl1521-nvs-n900.bin is *not* present mounts the CAL MTD,
> >> reads the data, writes it out into wl1521-nvs-n900.bin, and
> >> the rmmod/modprobes the driver
> > 
> > Quote:
> >> On Nokia N900 NVS data are generated on-the-fly from some
> >> bytes from CAL (/dev/mtd1), from state of cellular network
> >> and from some other regulation settings.
> > 
> > This basically means to rewrite it every boot or everytime
> > when country was changed (for regulation settings). And Ii
> > really do not want to do that.
> > 
> > And rmmod is not working on statically linked drivers into
> > zImage. So this is not solution.
> 
> actually module removal is still considered a race condition.
> If re-loading a module is part of your solution, then you are
> already heading into the wrong direction.
> 

No, I'm not doing it and I want to have driver still loaded.

> WiFi subsystem have a solution for handling regulatory
> enforcement. I don't know why would you try to invent that
> same via NVS files.
> 

Both firmware and NVS files are sent to chip as blob data. And 
they are sent every time when userspace ask to bring interface 
up.

So looks like modprobing driver works without data. Kernel ask 
for them once userspace want to use wifi network.

> >> and done?  Stuff that's not N900 just wouldn't ship the
> >> update service and would proceed like none of this
> >> happened.
> >> 
> >> Dan
> > 
> > Again, what is wrong with userspace firmware helper? I think
> > that it fix this problem in a clean way without any hacks
> > (like CAL in kernel or creating new FS specially for
> > parsing NVS and so on) in kernel. And in userspace we can
> > implement program which generate NVS firmware data
> > on-the-fly and send them to kernel in compatible format of
> > ti-connectivity/wl1251-nvs.bin
> 
> How do you run a userspace firmware helper if the root
> filesystem has not yet been mounted? How do you run userspace
> helper during resume?
> 

I cannot do that.

> Honestly for drivers linked statically into the kernel, the
> best approach is that they stay unconfigured until userspace
> is available and can run a tool to provide the correct data.
> 
> Regards
> 
> Marcel

So ifconfig wlan0 should fail? In our case for wl1251 I think we 
should fallback to generic NVS data if are available (and maybe 
later when request will be there again, userspace can provide 
data).

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: wl1251: NVS firmware data

[Dan Williams]

On Mon, 2014-12-08 at 20:36 +0100, Pali Rohár wrote:
> On Monday 08 December 2014 20:26:53 Dan Williams wrote:
> > On Mon, 2014-12-08 at 20:15 +0100, Pali Rohár wrote:
> > > On Monday 08 December 2014 19:50:17 Marcel Holtmann wrote:
> > > > Hi Pali,
> > > > 
> > > > >>>>>> On Saturday 06 December 2014 13:49:54 Pavel Machek
> > > > >>>>>> wrote: /**
> > > > >>>>>> 
> > > > >>>>>> + * request_firmware_prefer_user: - prefer usermode
> > > > >>>>>> helper for loading firmware + * @firmware_p:
> > > > >>>>>> pointer to firmware image
> > > > >>>>>> + * @name: name of firmware file
> > > > >>>>>> + * @device: device for which firmware is being
> > > > >>>>>> loaded + *
> > > > >>>>>> + * This function works pretty much like
> > > > >>>>>> request_firmware(), but it prefer + * usermode
> > > > >>>>>> helper. If usermode helper fails then it fallback
> > > > >>>>>> to direct access. + * Usefull for dynamic or model
> > > > >>>>>> specific firmware data. + **/
> > > > >>>>>> +int request_firmware_prefer_user(const struct
> > > > >>>>>> firmware **firmware_p, +                          
> > > > >>>>>> const char *name, struct device *device) +{
> > > > >>>>>> +       int ret;
> > > > >>>>>> +       __module_get(THIS_MODULE);
> > > > >>>>>> +       ret = _request_firmware(firmware_p, name,
> > > > >>>>>> device, +                              
> > > > >>>>>> FW_OPT_UEVENT
> > > > >>>>>> 
> > > > >>>>>> | FW_OPT_PREFER_USER); +
> > > > >>>>>> 
> > > > >>>>>> module_put(THIS_MODULE); +       return ret;
> > > > >>>>>> +}
> > > > >>>>>> +EXPORT_SYMBOL_GPL(request_firmware_prefer_user);
> > > > >>>>> 
> > > > >>>>> I'd like to introduce request_firmware_user() which
> > > > >>>>> only requests firmware from user space, and this
> > > > >>>>> way is simpler and more flexible since we have
> > > > >>>>> request_firmware_direct() already.
> > > > >>>> 
> > > > >>>> Why would a driver care about what program provides
> > > > >>>> the firmware?  It shouldn't at all, and we want to
> > > > >>>> get rid of the userspace firmware loader, not
> > > > >>>> encourage drivers to use it "exclusively" at all.
> > > > >>> 
> > > > >>> Do not remove it! Without userspace firmware loader it
> > > > >>> is impossible to load dynamic firmware files.
> > > > >> 
> > > > >> why is this dynamic in the first place. It does not
> > > > >> sound like dynamic data to me at all. This is like the
> > > > >> WiFi MAC address(es) or Bluetooth BD_ADDR. They are
> > > > >> all static information. The only difference is that
> > > > >> they are on the host accessibly filesystem or storage
> > > > >> and not on the device itself.
> > > > >> 
> > > > >> To be honest, for Bluetooth we solved this now. If the
> > > > >> device is missing key information like the calibration
> > > > >> data or BD_ADDR, then it comes up unconfigured. A
> > > > >> userspace process can then go and load the right data
> > > > >> into it and then the device becomes available as
> > > > >> Bluetooth device.
> > > > >> 
> > > > >> Trying to use request_firmware to load some random data
> > > > >> and insist on going through userspace helper for that
> > > > >> sounds crazy to me. Especially since we are trying
> > > > >> hard to get away from the userspace loader. Forcing to
> > > > >> keep it for new stuff sounds backwards to me.
> > > > >> 
> > > > >> With the special Nokia partition in mind, why hasn't
> > > > >> this been turned into a mountable filesystem or into a
> > > > >> driver/subsystem that can access the data direct from
> > > > >> the kernel. I advocated for this some time ago. Maybe
> > > > >> there should be a special subsystem for access to
> > > > >> these factory persistent information that drivers then
> > > > >> just can access. I seem to remember that some systems
> > > > >> provide these via ACPI. Why does the ARM platform has
> > > > >> to be special here?
> > > > >> 
> > > > >> And the problem of getting Ethernet and WiFi MAC
> > > > >> address and Bluetooth BD_ADDR comes up many many
> > > > >> times. Why not have something generic here. And don't
> > > > >> tell me request_firmware is that generic solution ;)
> > > > >> 
> > > > >> Regards
> > > > >> 
> > > > >> Marcel
> > > > > 
> > > > > Hi Marcel. I think you did not understand this problem.
> > > > > This discussion is not about mac address. Please read
> > > > > email thread again and if there are some unclear pars,
> > > > > then ask. Thanks!
> > > > 
> > > > I think that I pretty clearly understand the problem.
> > > > Calibration data, MAC address, what is the difference? For
> > > > me this is all the same. It is data that is specific to a
> > > > device or type of devices and it is stored somewhere
> > > > else. In most cases in some immutable memory/flash area.
> > > 
> > > Those calibration data (in form of binary NVS firmware file)
> > > needs to be sent to wl1251 chip. Mac address is not needed
> > > at this step (and kernel generate some random if is not
> > > provided).
> > > 
> > > (Just to note wl1271 driver loads both MAC address and NVS
> > > data via one firmware file which is prepared by userspace,
> > > but this discussion is about wl1251...)
> > > 
> > > > What you want is access to this data since the kernel
> > > > driver needs it. Do I get this so far ;)
> > > 
> > > Yes, we need to provide NVS data to kernel when kernel ask
> > > for them.
> > > 
> > > > So my take is that request_firmware is not the right way
> > > > to get this data. Or more precisely make sure that this
> > > > data is available to kernel drivers. And what I am seeing
> > > > here is that instead of actually solving the bigger
> > > > problem, we just hack around it with request_firmware.
> > > > Now surprisingly the request_firmware loads files
> > > > directly from the kernel and all the hacks do not work
> > > > anymore.
> > > > 
> > > > Regards
> > > > 
> > > > Marcel
> > > 
> > > Just read emails again...
> > > 
> > > Our problem is:
> > > 
> > > linux-firmware.git tree provides two binary firmware files:
> > > 
> > > ti-connectivity/wl1251-fw.bin
> > > ti-connectivity/wl1251-nvs.bin
> > > 
> > > First is firmware file, second NVS file with generic
> > > calibration data. Kernel driver wl1251 now loads both
> > > firmware files via request_firmware. Generic calibration
> > > data are enough for wl1251 chip (it should work). But
> > > devices have own calibration data stored somewhere else.
> > > 
> > > On Nokia N900 NVS data are generated on-the-fly from some
> > > bytes from CAL (/dev/mtd1), from state of cellular network
> > > and from some other regulation settings.
> > > 
> > > So I think that files stored in linux-firmware.git tree
> > > (which are also installed into /lib/firmware/) should be
> > > loaded with request_firmware function. Or not? Do you think
> > > something else? What other developers think?
> > > 
> > > I'm against kernel driver for CAL (/dev/mtd1) for more
> > > reasons:
> > > 
> > > 1) we have userspace open source code, but licensed under
> > > GPLv3. And until kernel change license, we cannot include
> > > it.
> > > 
> > > 2) NVS data are (probably) not in one place, plus they
> > > depends on something other.
> > > 
> > > 3) If manufacture XYZ create new device with its own storage
> > > format of calibration data this means that correct solution
> > > for XYZ is also to implement new kernel fs driver for its
> > > own format. Do you really want to have in kernel all those
> > > drivers for all different (proprietary) storage formats?
> > > 
> > > 4) It does not help us with existence of generic file
> > > /lib/firmware/ti-connectivity/wl1251-nvs.bin which comes
> > > from linux-firmware.git tree.
> > 
> > a) change driver to prefer a new "wl1251-nvs-n900.bin" file,
> 
> Why to "*-n900.bin" ? wl1251 driver is used on other devices too.

Is the CAL data format generic to all wl1251 devices?  Or is the stuff
in the CAL partition Nokia-specific?

> > but fall back to "wl1251-nvs.bin" if the first one isn't
> > present
> 
> > b) have a "wl1251-cal-nvs-update" service that, if
> > wl1521-nvs-n900.bin is *not* present mounts the CAL MTD,
> > reads the data, writes it out into wl1521-nvs-n900.bin, and
> > the rmmod/modprobes the driver
> > 
> 
> Quote:
> > On Nokia N900 NVS data are generated on-the-fly from some bytes 
> > from CAL (/dev/mtd1), from state of cellular network and from 
> > some other regulation settings.
> 
> This basically means to rewrite it every boot or everytime when 
> country was changed (for regulation settings). And Ii really do 
> not want to do that.

I'm not sure why it would be set every boot, if it already existed?
Isn't the CAL data just the default regulatory domain?  Whatever
*changes* the CAL data would clearly need to invalidate the
existing .bin file too.  But...

You have to re-send regulatory information to the chip anyway, whenever
cfg80211 changes the regulatory domain of the device.  (iw reg set
Poland)

You have to re-send the regulatory information to the chip anyway,
whenever the user registers with an operator.  (eg, MCC/MNC is now a
Polish operator).

In either case, you need to adjust the regulatory domain of the device
on-the-fly.

You also need to set the default regulatory domain at bootup, from the
CAL data, just in case the phone is in airplane mode and no MCC/MNC is
available.

The mechanism for each should be the same, through the normal
mac80211/cfg80211 hooks to set the regulatory domain.

> And rmmod is not working on statically linked drivers into 
> zImage. So this is not solution.
> 
> > and done?  Stuff that's not N900 just wouldn't ship the update
> > service and would proceed like none of this happened.
> > 
> > Dan
> 
> Again, what is wrong with userspace firmware helper? I think that 
> it fix this problem in a clean way without any hacks (like CAL in 
> kernel or creating new FS specially for parsing NVS and so on) in 
> kernel. And in userspace we can implement program which generate 
> NVS firmware data on-the-fly and send them to kernel in 
> compatible format of ti-connectivity/wl1251-nvs.bin

Because that has other issues as Greg describes.  What's wrong with a
*udev* helper that pushes the information down to the chip after the MTD
partition is mounted?  Why does it have to a firmware helper?

Also, changing the regulatory information based on MCC/MNC implies that
you have to get the different regulatory information from somewhere.
Where is that information stored?  Also in the CAL partition?  Or
somewhere else?  How big is all that information?

Dan

Re: wl1251: NVS firmware data

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 248 bytes --]

On Monday 08 December 2014 23:51:07 Dan Williams wrote:
> Is the CAL data format generic to all wl1251 devices?  Or is
> the stuff in the CAL partition Nokia-specific?
> 

Specific for Nokia devices.

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: wl1251: NVS firmware data

[Dan Williams]

On Tue, 2014-12-09 at 00:23 +0100, Pali Rohár wrote:
> On Monday 08 December 2014 23:51:07 Dan Williams wrote:
> > Is the CAL data format generic to all wl1251 devices?  Or is
> > the stuff in the CAL partition Nokia-specific?
> > 
> 
> Specific for Nokia devices.

Ok, but then something must transform that data into a format that the
wl1251 chip can consume, correct?  What do other wl1251 platforms do to
retrieve this information and set regulatory region?

Dan

Re: wl1251: NVS firmware data

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 973 bytes --]

On Tuesday 09 December 2014 00:42:17 Dan Williams wrote:
> On Tue, 2014-12-09 at 00:23 +0100, Pali Rohár wrote:
> > On Monday 08 December 2014 23:51:07 Dan Williams wrote:
> > > Is the CAL data format generic to all wl1251 devices?  Or
> > > is the stuff in the CAL partition Nokia-specific?
> > 
> > Specific for Nokia devices.
> 
> Ok, but then something must transform that data into a format
> that the wl1251 chip can consume, correct?  What do other
> wl1251 platforms do to retrieve this information and set
> regulatory region?
> 
> Dan

It looks like wl1251 driver supports standard way to set 
regulatory settings. There is application which reads CAL data, 
cellular network info, ... and sent it to kernel (via Nokia 
specific netlink interface patch). And binary data which are sent 
are in same format as file wl1251-nvs.bin. And some bits which 
deterministically send to kernl depends on fcc.

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: wl1251: NVS firmware data

[Ivaylo Dimitrov]

On  8.12.2014 21:26, Dan Williams wrote:
>
> a) change driver to prefer a new "wl1251-nvs-n900.bin" file, but fall
> back to "wl1251-nvs.bin" if the first one isn't present
> b) have a "wl1251-cal-nvs-update" service that, if wl1521-nvs-n900.bin
> is *not* present mounts the CAL MTD, reads the data, writes it out into
> wl1521-nvs-n900.bin, and the rmmod/modprobes the driver
>
> and done?  Stuff that's not N900 just wouldn't ship the update service
> and would proceed like none of this happened.
>
> Dan
>
>

That would mean that the driver should not be built-in, as afaik we 
cannot rmmod built-in drivers. Sure, it will work after a reboot, but 
this is a bit hacky, agree?

Also, new NVS file needs to be loaded when fcc regulation changes(flying 
abroad), so that would mean that the device would be outside of those 
until reboot (in case of built-in driver)

Ivo

Re: wl1251: NVS firmware data

[Dan Williams]

On Mon, 2014-12-08 at 21:42 +0200, Ivaylo Dimitrov wrote:
> 
> On  8.12.2014 21:26, Dan Williams wrote:
> >
> > a) change driver to prefer a new "wl1251-nvs-n900.bin" file, but fall
> > back to "wl1251-nvs.bin" if the first one isn't present
> > b) have a "wl1251-cal-nvs-update" service that, if wl1521-nvs-n900.bin
> > is *not* present mounts the CAL MTD, reads the data, writes it out into
> > wl1521-nvs-n900.bin, and the rmmod/modprobes the driver
> >
> > and done?  Stuff that's not N900 just wouldn't ship the update service
> > and would proceed like none of this happened.
> >
> > Dan
> >
> >
> 
> That would mean that the driver should not be built-in, as afaik we 
> cannot rmmod built-in drivers. Sure, it will work after a reboot, but 
> this is a bit hacky, agree?
> 
> Also, new NVS file needs to be loaded when fcc regulation changes(flying 
> abroad), so that would mean that the device would be outside of those 
> until reboot (in case of built-in driver)

Regulatory stuff needs to be hooked into CRDA or the existing regulatory
codepaths, not some other path.  So when cfg80211 sets the regulatory
domain on the driver the driver needs to get the necessary NVS data.
Either the NVS for every domain (which cannot be a lot of them) gets
hardcoded into the driver, and then selected based on what cfg80211
says, or the driver needs to ask userspace for the NVS data based on
what cfg80211 says.  In all cases, cfg80211 drives the regulatory domain
[1].

a) How many regulatory domains does the driver support, how much data is
there for each domain, and can that be put into the driver instead of
getting it from the CAL partition?

b) what do *other* (non-N900) wl1251 devices do for regulatory data?

Dan

[1] unless there's some *restriction* hardcoded into the EEPROM of the
device, which in the case of the N900 there isn't, since the regulatory
data changes based on the MCC/MNC of the cellular side.

Re: wl1251: NVS firmware data

[Marcel Holtmann]

Hi Dan,

>>> a) change driver to prefer a new "wl1251-nvs-n900.bin" file, but fall
>>> back to "wl1251-nvs.bin" if the first one isn't present
>>> b) have a "wl1251-cal-nvs-update" service that, if wl1521-nvs-n900.bin
>>> is *not* present mounts the CAL MTD, reads the data, writes it out into
>>> wl1521-nvs-n900.bin, and the rmmod/modprobes the driver
>>> 
>>> and done?  Stuff that's not N900 just wouldn't ship the update service
>>> and would proceed like none of this happened.
>>> 
>>> Dan
>>> 
>>> 
>> 
>> That would mean that the driver should not be built-in, as afaik we 
>> cannot rmmod built-in drivers. Sure, it will work after a reboot, but 
>> this is a bit hacky, agree?
>> 
>> Also, new NVS file needs to be loaded when fcc regulation changes(flying 
>> abroad), so that would mean that the device would be outside of those 
>> until reboot (in case of built-in driver)
> 
> Regulatory stuff needs to be hooked into CRDA or the existing regulatory
> codepaths, not some other path.  So when cfg80211 sets the regulatory
> domain on the driver the driver needs to get the necessary NVS data.
> Either the NVS for every domain (which cannot be a lot of them) gets
> hardcoded into the driver, and then selected based on what cfg80211
> says, or the driver needs to ask userspace for the NVS data based on
> what cfg80211 says.  In all cases, cfg80211 drives the regulatory domain
> [1].
> 
> a) How many regulatory domains does the driver support, how much data is
> there for each domain, and can that be put into the driver instead of
> getting it from the CAL partition?
> 
> b) what do *other* (non-N900) wl1251 devices do for regulatory data?
> 
> Dan
> 
> [1] unless there's some *restriction* hardcoded into the EEPROM of the
> device, which in the case of the N900 there isn't, since the regulatory
> data changes based on the MCC/MNC of the cellular side.

and even these ones would all work just fine. The regulatory handling of cfg80211 is already multi-layer anyway. I will intersect from the driver provided information and userspace provided information.

I mean if userspace can just make up some NVS data to change the regulatory enforcement or let cfg80211 do it for you, there is no difference. So the real question is why bother with this at all in the first place. We already have a solution that does exactly what you want. Use CRDA and be done with it.

And calibration data should be rather static for each device. It might differ from device a to device b, but on device a it would stay the same. These calibration data are normally programmed at the factory line and then never changed again.

For me this sounds all a bit like that everybody is buying into this NVS file solution for everything and now trying to hack that into something workable. But nobody actually looks at the existing solutions out there and really tries to fix the mess that Nokia and TI left behind for this chipset.

Regards

Marcel

Re: wl1251: NVS firmware data

[Marcel Holtmann]

Hi Pali,

>>>>>>>> On Saturday 06 December 2014 13:49:54 Pavel Machek
>>>>>>>> wrote: /**
>>>>>>>> 
>>>>>>>> + * request_firmware_prefer_user: - prefer usermode
>>>>>>>> helper for loading firmware + * @firmware_p: pointer to
>>>>>>>> firmware image
>>>>>>>> + * @name: name of firmware file
>>>>>>>> + * @device: device for which firmware is being loaded
>>>>>>>> + *
>>>>>>>> + * This function works pretty much like
>>>>>>>> request_firmware(), but it prefer + * usermode helper.
>>>>>>>> If usermode helper fails then it fallback to direct
>>>>>>>> access. + * Usefull for dynamic or model specific
>>>>>>>> firmware data. + **/
>>>>>>>> +int request_firmware_prefer_user(const struct firmware
>>>>>>>> **firmware_p, +                           const char
>>>>>>>> *name, struct device *device) +{
>>>>>>>> +       int ret;
>>>>>>>> +       __module_get(THIS_MODULE);
>>>>>>>> +       ret = _request_firmware(firmware_p, name,
>>>>>>>> device, +                               FW_OPT_UEVENT
>>>>>>>> | FW_OPT_PREFER_USER); +      
>>>>>>>> module_put(THIS_MODULE); +       return ret;
>>>>>>>> +}
>>>>>>>> +EXPORT_SYMBOL_GPL(request_firmware_prefer_user);
>>>>>>> 
>>>>>>> I'd like to introduce request_firmware_user() which only
>>>>>>> requests firmware from user space, and this way is
>>>>>>> simpler and more flexible since we have
>>>>>>> request_firmware_direct() already.
>>>>>> 
>>>>>> Why would a driver care about what program provides the
>>>>>> firmware?  It shouldn't at all, and we want to get rid of
>>>>>> the userspace firmware loader, not encourage drivers to
>>>>>> use it "exclusively" at all.
>>>>> 
>>>>> Do not remove it! Without userspace firmware loader it is
>>>>> impossible to load dynamic firmware files.
>>>> 
>>>> why is this dynamic in the first place. It does not sound
>>>> like dynamic data to me at all. This is like the WiFi MAC
>>>> address(es) or Bluetooth BD_ADDR. They are all static
>>>> information. The only difference is that they are on the
>>>> host accessibly filesystem or storage and not on the
>>>> device itself.
>>>> 
>>>> To be honest, for Bluetooth we solved this now. If the
>>>> device is missing key information like the calibration
>>>> data or BD_ADDR, then it comes up unconfigured. A
>>>> userspace process can then go and load the right data into
>>>> it and then the device becomes available as Bluetooth
>>>> device.
>>>> 
>>>> Trying to use request_firmware to load some random data and
>>>> insist on going through userspace helper for that sounds
>>>> crazy to me. Especially since we are trying hard to get
>>>> away from the userspace loader. Forcing to keep it for new
>>>> stuff sounds backwards to me.
>>>> 
>>>> With the special Nokia partition in mind, why hasn't this
>>>> been turned into a mountable filesystem or into a
>>>> driver/subsystem that can access the data direct from the
>>>> kernel. I advocated for this some time ago. Maybe there
>>>> should be a special subsystem for access to these factory
>>>> persistent information that drivers then just can access.
>>>> I seem to remember that some systems provide these via
>>>> ACPI. Why does the ARM platform has to be special here?
>>>> 
>>>> And the problem of getting Ethernet and WiFi MAC address
>>>> and Bluetooth BD_ADDR comes up many many times. Why not
>>>> have something generic here. And don't tell me
>>>> request_firmware is that generic solution ;)
>>>> 
>>>> Regards
>>>> 
>>>> Marcel
>>> 
>>> Hi Marcel. I think you did not understand this problem. This
>>> discussion is not about mac address. Please read email
>>> thread again and if there are some unclear pars, then ask.
>>> Thanks!
>> 
>> I think that I pretty clearly understand the problem.
>> Calibration data, MAC address, what is the difference? For me
>> this is all the same. It is data that is specific to a device
>> or type of devices and it is stored somewhere else. In most
>> cases in some immutable memory/flash area.
>> 
> 
> Those calibration data (in form of binary NVS firmware file) 
> needs to be sent to wl1251 chip. Mac address is not needed at 
> this step (and kernel generate some random if is not provided).

the MAC address is just an example or similar data. And to be clear the kernel generating some random address is not a good idea either. If you get a new random address on every boot that is total disaster. Because it sort of works does not mean it is the right way to do it. That is why I am including MAC address in the list here. It is same kind of data that is needed before a device can be declared fully functional.

> (Just to note wl1271 driver loads both MAC address and NVS data 
> via one firmware file which is prepared by userspace, but this 
> discussion is about wl1251...)

There is no difference between any drivers here. I do not know why are you trying to tie this to a specific driver. Why does it matter what kind of information these are. The point is they are not static, they are device specific and come from different sources. And the kernel driver needs them.

>> What you want is access to this data since the kernel driver
>> needs it. Do I get this so far ;)
>> 
> 
> Yes, we need to provide NVS data to kernel when kernel ask for 
> them.
> 
>> So my take is that request_firmware is not the right way to
>> get this data. Or more precisely make sure that this data is
>> available to kernel drivers. And what I am seeing here is
>> that instead of actually solving the bigger problem, we just
>> hack around it with request_firmware. Now surprisingly the
>> request_firmware loads files directly from the kernel and all
>> the hacks do not work anymore.
>> 
>> Regards
>> 
>> Marcel
> 
> Just read emails again...
> 
> Our problem is:
> 
> linux-firmware.git tree provides two binary firmware files:
> 
> ti-connectivity/wl1251-fw.bin
> ti-connectivity/wl1251-nvs.bin
> 
> First is firmware file, second NVS file with generic calibration 
> data. Kernel driver wl1251 now loads both firmware files via 
> request_firmware. Generic calibration data are enough for wl1251 
> chip (it should work). But devices have own calibration data 
> stored somewhere else.

Loading generic data that is static and stored on the filesystem via request_firmware is totally fine. If you have the NVS data in that file, then great. If you have specific data, then overwrite the file, link it to the real file or do something with it. As long as it is a file on the filesystem, you will be just fine.

If you however want to hook into some magic userspace helper to build the content of the file and somehow load it, then that sounds like the wrong approach to me.

> On Nokia N900 NVS data are generated on-the-fly from some bytes 
> from CAL (/dev/mtd1), from state of cellular network and from 
> some other regulation settings.
> 
> So I think that files stored in linux-firmware.git tree (which 
> are also installed into /lib/firmware/) should be loaded with 
> request_firmware function. Or not? Do you think something else? 
> What other developers think?
> 
> I'm against kernel driver for CAL (/dev/mtd1) for more reasons:
> 
> 1) we have userspace open source code, but licensed under GPLv3. 
> And until kernel change license, we cannot include it.
> 
> 2) NVS data are (probably) not in one place, plus they depends on 
> something other.
> 
> 3) If manufacture XYZ create new device with its own storage 
> format of calibration data this means that correct solution for 
> XYZ is also to implement new kernel fs driver for its own format. 
> Do you really want to have in kernel all those drivers for all 
> different (proprietary) storage formats?
> 
> 4) It does not help us with existence of generic file 
> /lib/firmware/ti-connectivity/wl1251-nvs.bin which comes from 
> linux-firmware.git tree.

As I said before, I think that a driver should not register with its subsystem until it has all data that it needs. Or if it can tell the subsystem that it is missing data and the subsystem knows how to provide hooks for getting this data.

We all know that many embedded devices need extra data to operation properly. This data is normally programmed onto the device in the factory. So if someone would now build a subsystem that can retrieve magic blobs of data from magic places like ACPI, devicetree, userspace or whatever that would help. I can see that request_firmware looks a lot like this. However the reality is that you have a race condition here. request_firmware relies on the fact that it is file in userspace. That is what it was designed for in the first place. Firmware files that are place on the hosts filesystem. It does not have the option to start a notifier when blob xyz becomes available. And that is what you essentially need for the drivers. The driver finds the hardware and goes, now I need blob xyz to function 
 and then it sits and waits until it gets told that blob is now available. Then it initializes the hardware and registers it to the subsystem.

I fully realize that request_firmware is pretty close in this regard, but the semantics and timing that many of these NVS data like addresses, our calibration information are different. It is more than just this specific drivers problem. There are many devices out there that have certain settings stored somewhere and it needs these based on how the device is build or how it is provisioning in the factory.

What I would actually prefer to see that the driver just requests this blob of information and then a separate subsystem deals with getting it. As I said, in some cases the information might be in ACPI or devicetree or accessible by a special driver. In that case no userspace interaction would be needed at all. However the driver has to deal with the fact that the data blob might not be available for a certain period of time. If you mention cellular modem, then that one has to boot up first and get its data. More reason to actually design this cleanly so that there are no race conditions.

Regards

Marcel

Re: wl1251: NVS firmware data

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 11342 bytes --]

On Monday 08 December 2014 20:41:13 Marcel Holtmann wrote:
> Hi Pali,
> 
> >>>>>>>> On Saturday 06 December 2014 13:49:54 Pavel Machek
> >>>>>>>> wrote: /**
> >>>>>>>> 
> >>>>>>>> + * request_firmware_prefer_user: - prefer usermode
> >>>>>>>> helper for loading firmware + * @firmware_p: pointer
> >>>>>>>> to firmware image
> >>>>>>>> + * @name: name of firmware file
> >>>>>>>> + * @device: device for which firmware is being
> >>>>>>>> loaded + *
> >>>>>>>> + * This function works pretty much like
> >>>>>>>> request_firmware(), but it prefer + * usermode
> >>>>>>>> helper. If usermode helper fails then it fallback to
> >>>>>>>> direct access. + * Usefull for dynamic or model
> >>>>>>>> specific firmware data. + **/
> >>>>>>>> +int request_firmware_prefer_user(const struct
> >>>>>>>> firmware **firmware_p, +                          
> >>>>>>>> const char *name, struct device *device) +{
> >>>>>>>> +       int ret;
> >>>>>>>> +       __module_get(THIS_MODULE);
> >>>>>>>> +       ret = _request_firmware(firmware_p, name,
> >>>>>>>> device, +                               FW_OPT_UEVENT
> >>>>>>>> 
> >>>>>>>> | FW_OPT_PREFER_USER); +
> >>>>>>>> 
> >>>>>>>> module_put(THIS_MODULE); +       return ret;
> >>>>>>>> +}
> >>>>>>>> +EXPORT_SYMBOL_GPL(request_firmware_prefer_user);
> >>>>>>> 
> >>>>>>> I'd like to introduce request_firmware_user() which
> >>>>>>> only requests firmware from user space, and this way
> >>>>>>> is simpler and more flexible since we have
> >>>>>>> request_firmware_direct() already.
> >>>>>> 
> >>>>>> Why would a driver care about what program provides the
> >>>>>> firmware?  It shouldn't at all, and we want to get rid
> >>>>>> of the userspace firmware loader, not encourage
> >>>>>> drivers to use it "exclusively" at all.
> >>>>> 
> >>>>> Do not remove it! Without userspace firmware loader it
> >>>>> is impossible to load dynamic firmware files.
> >>>> 
> >>>> why is this dynamic in the first place. It does not sound
> >>>> like dynamic data to me at all. This is like the WiFi MAC
> >>>> address(es) or Bluetooth BD_ADDR. They are all static
> >>>> information. The only difference is that they are on the
> >>>> host accessibly filesystem or storage and not on the
> >>>> device itself.
> >>>> 
> >>>> To be honest, for Bluetooth we solved this now. If the
> >>>> device is missing key information like the calibration
> >>>> data or BD_ADDR, then it comes up unconfigured. A
> >>>> userspace process can then go and load the right data
> >>>> into it and then the device becomes available as
> >>>> Bluetooth device.
> >>>> 
> >>>> Trying to use request_firmware to load some random data
> >>>> and insist on going through userspace helper for that
> >>>> sounds crazy to me. Especially since we are trying hard
> >>>> to get away from the userspace loader. Forcing to keep
> >>>> it for new stuff sounds backwards to me.
> >>>> 
> >>>> With the special Nokia partition in mind, why hasn't this
> >>>> been turned into a mountable filesystem or into a
> >>>> driver/subsystem that can access the data direct from the
> >>>> kernel. I advocated for this some time ago. Maybe there
> >>>> should be a special subsystem for access to these factory
> >>>> persistent information that drivers then just can access.
> >>>> I seem to remember that some systems provide these via
> >>>> ACPI. Why does the ARM platform has to be special here?
> >>>> 
> >>>> And the problem of getting Ethernet and WiFi MAC address
> >>>> and Bluetooth BD_ADDR comes up many many times. Why not
> >>>> have something generic here. And don't tell me
> >>>> request_firmware is that generic solution ;)
> >>>> 
> >>>> Regards
> >>>> 
> >>>> Marcel
> >>> 
> >>> Hi Marcel. I think you did not understand this problem.
> >>> This discussion is not about mac address. Please read
> >>> email thread again and if there are some unclear pars,
> >>> then ask. Thanks!
> >> 
> >> I think that I pretty clearly understand the problem.
> >> Calibration data, MAC address, what is the difference? For
> >> me this is all the same. It is data that is specific to a
> >> device or type of devices and it is stored somewhere else.
> >> In most cases in some immutable memory/flash area.
> > 
> > Those calibration data (in form of binary NVS firmware file)
> > needs to be sent to wl1251 chip. Mac address is not needed
> > at this step (and kernel generate some random if is not
> > provided).
> 
> the MAC address is just an example or similar data. And to be
> clear the kernel generating some random address is not a good
> idea either. If you get a new random address on every boot
> that is total disaster. Because it sort of works does not
> mean it is the right way to do it. That is why I am including
> MAC address in the list here. It is same kind of data that is
> needed before a device can be declared fully functional.
> 
> > (Just to note wl1271 driver loads both MAC address and NVS
> > data via one firmware file which is prepared by userspace,
> > but this discussion is about wl1251...)
> 
> There is no difference between any drivers here. I do not know
> why are you trying to tie this to a specific driver. Why does
> it matter what kind of information these are. The point is
> they are not static, they are device specific and come from
> different sources. And the kernel driver needs them.
> 

Right.

> >> What you want is access to this data since the kernel
> >> driver needs it. Do I get this so far ;)
> > 
> > Yes, we need to provide NVS data to kernel when kernel ask
> > for them.
> > 
> >> So my take is that request_firmware is not the right way to
> >> get this data. Or more precisely make sure that this data
> >> is available to kernel drivers. And what I am seeing here
> >> is that instead of actually solving the bigger problem, we
> >> just hack around it with request_firmware. Now
> >> surprisingly the request_firmware loads files directly
> >> from the kernel and all the hacks do not work anymore.
> >> 
> >> Regards
> >> 
> >> Marcel
> > 
> > Just read emails again...
> > 
> > Our problem is:
> > 
> > linux-firmware.git tree provides two binary firmware files:
> > 
> > ti-connectivity/wl1251-fw.bin
> > ti-connectivity/wl1251-nvs.bin
> > 
> > First is firmware file, second NVS file with generic
> > calibration data. Kernel driver wl1251 now loads both
> > firmware files via request_firmware. Generic calibration
> > data are enough for wl1251 chip (it should work). But
> > devices have own calibration data stored somewhere else.
> 
> Loading generic data that is static and stored on the
> filesystem via request_firmware is totally fine. If you have
> the NVS data in that file, then great. If you have specific
> data, then overwrite the file, link it to the real file or do
> something with it. As long as it is a file on the filesystem,
> you will be just fine.
> 

I think that I should be able to rsync disk image of system and 
then copy it into another device (of same type). So I do not want 
to store device specific files into /lib/firmware/...

> If you however want to hook into some magic userspace helper
> to build the content of the file and somehow load it, then
> that sounds like the wrong approach to me.
> 

If data depends on something from userspace (location of device 
or type of sim card, etc, ...) then there is needed some 
userspace program which prepare data. It cannot be automatically 
done from kernel.

> > On Nokia N900 NVS data are generated on-the-fly from some
> > bytes from CAL (/dev/mtd1), from state of cellular network
> > and from some other regulation settings.
> > 
> > So I think that files stored in linux-firmware.git tree
> > (which are also installed into /lib/firmware/) should be
> > loaded with request_firmware function. Or not? Do you think
> > something else? What other developers think?
> > 
> > I'm against kernel driver for CAL (/dev/mtd1) for more
> > reasons:
> > 
> > 1) we have userspace open source code, but licensed under
> > GPLv3. And until kernel change license, we cannot include
> > it.
> > 
> > 2) NVS data are (probably) not in one place, plus they
> > depends on something other.
> > 
> > 3) If manufacture XYZ create new device with its own storage
> > format of calibration data this means that correct solution
> > for XYZ is also to implement new kernel fs driver for its
> > own format. Do you really want to have in kernel all those
> > drivers for all different (proprietary) storage formats?
> > 
> > 4) It does not help us with existence of generic file
> > /lib/firmware/ti-connectivity/wl1251-nvs.bin which comes
> > from linux-firmware.git tree.
> 
> As I said before, I think that a driver should not register
> with its subsystem until it has all data that it needs. Or if
> it can tell the subsystem that it is missing data and the
> subsystem knows how to provide hooks for getting this data.
> 
> We all know that many embedded devices need extra data to
> operation properly. This data is normally programmed onto the
> device in the factory. So if someone would now build a
> subsystem that can retrieve magic blobs of data from magic
> places like ACPI, devicetree, userspace or whatever that
> would help. I can see that request_firmware looks a lot like
> this. However the reality is that you have a race condition
> here. request_firmware relies on the fact that it is file in
> userspace. That is what it was designed for in the first
> place. Firmware files that are place on the hosts filesystem.
> It does not have the option to start a notifier when blob xyz
> becomes available. And that is what you essentially need for
> the drivers. The driver finds the hardware and goes, now I
> need blob xyz to function and then it sits and waits until it
> gets told that blob is now available. Then it initializes the
> hardware and registers it to the subsystem.
> 
> I fully realize that request_firmware is pretty close in this
> regard, but the semantics and timing that many of these NVS
> data like addresses, our calibration information are
> different. It is more than just this specific drivers
> problem. There are many devices out there that have certain
> settings stored somewhere and it needs these based on how the
> device is build or how it is provisioning in the factory.
> 
> What I would actually prefer to see that the driver just
> requests this blob of information and then a separate
> subsystem deals with getting it. As I said, in some cases the
> information might be in ACPI or devicetree or accessible by a
> special driver. In that case no userspace interaction would
> be needed at all. However the driver has to deal with the
> fact that the data blob might not be available for a certain
> period of time. If you mention cellular modem, then that one
> has to boot up first and get its data. More reason to
> actually design this cleanly so that there are no race
> conditions.
> 
> Regards
> 
> Marcel

Yes, this sounds good. Some subsystem which reads needed data (or 
generate them or wait until something other provide them) sounds 
good.

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: wl1251: NVS firmware data

[Greg Kroah-Hartman]

On Mon, Dec 08, 2014 at 08:15:18PM +0100, Pali Rohár wrote:
> On Nokia N900 NVS data are generated on-the-fly from some bytes 
> from CAL (/dev/mtd1), from state of cellular network and from 
> some other regulation settings.

When is this "generated"?  At boot time?  Or by the firmware loader
program you have hooked into being called by the kernel at "load the
firmware now please" call time?

> So I think that files stored in linux-firmware.git tree (which 
> are also installed into /lib/firmware/) should be loaded with 
> request_firmware function. Or not? Do you think something else? 
> What other developers think?
> 
> I'm against kernel driver for CAL (/dev/mtd1) for more reasons:
> 
> 1) we have userspace open source code, but licensed under GPLv3. 
> And until kernel change license, we cannot include it.

You can change the license of your code if you want to, don't make this
type of nonsense argument.

> 2) NVS data are (probably) not in one place, plus they depends on 
> something other.

What is "something other"?  Where are they located?  Why would the
firmware interface know or care anything about this?

> 3) If manufacture XYZ create new device with its own storage 
> format of calibration data this means that correct solution for 
> XYZ is also to implement new kernel fs driver for its own format. 

Yes, as it is doing it's own custom thing, why overload an existing
interface to do something it was never designed to do?

> Do you really want to have in kernel all those drivers for all 
> different (proprietary) storage formats?

Yes, we are not afraid of lots of different drivers.  That is not even a
valid argument, you know better than this :)

> 4) It does not help us with existence of generic file 
> /lib/firmware/ti-connectivity/wl1251-nvs.bin which comes from 
> linux-firmware.git tree.

Again, not an issue.  If you don't want that file in the repo, ask for
it to be removed, and it will be, just send a patch to do it.

greg k-h

Re: wl1251: NVS firmware data

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 2445 bytes --]

On Monday 08 December 2014 22:00:18 Greg Kroah-Hartman wrote:
> On Mon, Dec 08, 2014 at 08:15:18PM +0100, Pali Rohár wrote:
> > On Nokia N900 NVS data are generated on-the-fly from some
> > bytes from CAL (/dev/mtd1), from state of cellular network
> > and from some other regulation settings.
> 
> When is this "generated"?  At boot time?  Or by the firmware
> loader program you have hooked into being called by the
> kernel at "load the firmware now please" call time?
> 

When userspace system network daemon is started.

> > So I think that files stored in linux-firmware.git tree
> > (which are also installed into /lib/firmware/) should be
> > loaded with request_firmware function. Or not? Do you think
> > something else? What other developers think?
> > 
> > I'm against kernel driver for CAL (/dev/mtd1) for more
> > reasons:
> > 
> > 1) we have userspace open source code, but licensed under
> > GPLv3. And until kernel change license, we cannot include
> > it.
> 
> You can change the license of your code if you want to, don't
> make this type of nonsense argument.
> 

Code is not mine, so I cannot change license.

> > 2) NVS data are (probably) not in one place, plus they
> > depends on something other.
> 
> What is "something other"?  Where are they located?  Why would
> the firmware interface know or care anything about this?
> 

fcc bit and some other data retrieved from daemon which 
communicating with cellular modem.

> > 3) If manufacture XYZ create new device with its own storage
> > format of calibration data this means that correct solution
> > for XYZ is also to implement new kernel fs driver for its
> > own format.
> 
> Yes, as it is doing it's own custom thing, why overload an
> existing interface to do something it was never designed to
> do?
> 
> > Do you really want to have in kernel all those drivers for
> > all different (proprietary) storage formats?
> 
> Yes, we are not afraid of lots of different drivers.  That is
> not even a valid argument, you know better than this :)
> 
> > 4) It does not help us with existence of generic file
> > /lib/firmware/ti-connectivity/wl1251-nvs.bin which comes
> > from linux-firmware.git tree.
> 
> Again, not an issue.  If you don't want that file in the repo,
> ask for it to be removed, and it will be, just send a patch
> to do it.
> 
> greg k-h

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: wl1251: NVS firmware data

[Greg Kroah-Hartman]

On Mon, Dec 08, 2014 at 05:47:30PM +0100, Pali Rohár wrote:
> On Monday 08 December 2014 17:37:14 Greg Kroah-Hartman wrote:
> > On Mon, Dec 08, 2014 at 11:18:18PM +0800, Ming Lei wrote:
> > > On Sat, Dec 6, 2014 at 9:02 PM, Pali Rohár 
> <pali.rohar@gmail.com> wrote:
> > > > On Saturday 06 December 2014 13:49:54 Pavel Machek wrote:
> > > >  /**
> > > > 
> > > > + * request_firmware_prefer_user: - prefer usermode helper
> > > > for loading firmware + * @firmware_p: pointer to firmware
> > > > image
> > > > + * @name: name of firmware file
> > > > + * @device: device for which firmware is being loaded
> > > > + *
> > > > + * This function works pretty much like
> > > > request_firmware(), but it prefer + * usermode helper. If
> > > > usermode helper fails then it fallback to direct access.
> > > > + * Usefull for dynamic or model specific firmware data.
> > > > + **/
> > > > +int request_firmware_prefer_user(const struct firmware
> > > > **firmware_p, +                           const char
> > > > *name, struct device *device) +{
> > > > +       int ret;
> > > > +       __module_get(THIS_MODULE);
> > > > +       ret = _request_firmware(firmware_p, name, device,
> > > > +                               FW_OPT_UEVENT |
> > > > FW_OPT_PREFER_USER); +       module_put(THIS_MODULE);
> > > > +       return ret;
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(request_firmware_prefer_user);
> > > 
> > > I'd like to introduce request_firmware_user() which only
> > > requests firmware from user space, and this way is simpler
> > > and more flexible since we have request_firmware_direct()
> > > already.
> > 
> > Why would a driver care about what program provides the
> > firmware?  It shouldn't at all, and we want to get rid of the
> > userspace firmware loader, not encourage drivers to use it
> > "exclusively" at all.
> > 
> 
> Do not remove it! Without userspace firmware loader it is 
> impossible to load dynamic firmware files.

You should not be loading "dynamic" firmware files with the firmware
interface, as that's not a "firmware" file anymore, it's a "special
binary file that my driver needs to be created and sent into the
kernel."

Use your own custom usermode helper for stuff like this, not the
firmware interface.  But use a binary sysfs file if you want, that seems
to make sense for it...

greg k-h

Re: wl1251: NVS firmware data

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 2777 bytes --]

On Monday 08 December 2014 21:57:21 Greg Kroah-Hartman wrote:
> On Mon, Dec 08, 2014 at 05:47:30PM +0100, Pali Rohár wrote:
> > On Monday 08 December 2014 17:37:14 Greg Kroah-Hartman wrote:
> > > On Mon, Dec 08, 2014 at 11:18:18PM +0800, Ming Lei wrote:
> > > > On Sat, Dec 6, 2014 at 9:02 PM, Pali Rohár
> > 
> > <pali.rohar@gmail.com> wrote:
> > > > > On Saturday 06 December 2014 13:49:54 Pavel Machek 
wrote:
> > > > >  /**
> > > > > 
> > > > > + * request_firmware_prefer_user: - prefer usermode
> > > > > helper for loading firmware + * @firmware_p: pointer
> > > > > to firmware image
> > > > > + * @name: name of firmware file
> > > > > + * @device: device for which firmware is being loaded
> > > > > + *
> > > > > + * This function works pretty much like
> > > > > request_firmware(), but it prefer + * usermode helper.
> > > > > If usermode helper fails then it fallback to direct
> > > > > access. + * Usefull for dynamic or model specific
> > > > > firmware data. + **/
> > > > > +int request_firmware_prefer_user(const struct
> > > > > firmware **firmware_p, +                          
> > > > > const char *name, struct device *device) +{
> > > > > +       int ret;
> > > > > +       __module_get(THIS_MODULE);
> > > > > +       ret = _request_firmware(firmware_p, name,
> > > > > device, +                               FW_OPT_UEVENT
> > > > > | FW_OPT_PREFER_USER); +      
> > > > > module_put(THIS_MODULE); +       return ret;
> > > > > +}
> > > > > +EXPORT_SYMBOL_GPL(request_firmware_prefer_user);
> > > > 
> > > > I'd like to introduce request_firmware_user() which only
> > > > requests firmware from user space, and this way is
> > > > simpler and more flexible since we have
> > > > request_firmware_direct() already.
> > > 
> > > Why would a driver care about what program provides the
> > > firmware?  It shouldn't at all, and we want to get rid of
> > > the userspace firmware loader, not encourage drivers to
> > > use it "exclusively" at all.
> > 
> > Do not remove it! Without userspace firmware loader it is
> > impossible to load dynamic firmware files.
> 
> You should not be loading "dynamic" firmware files with the
> firmware interface, as that's not a "firmware" file anymore,
> it's a "special binary file that my driver needs to be
> created and sent into the kernel."
> 
> Use your own custom usermode helper for stuff like this, not
> the firmware interface.  But use a binary sysfs file if you
> want, that seems to make sense for it...
> 
> greg k-h

Nokia for this problem invented its own netlink interface into 
wl1251 driver. But because it was specific for N900 device it was 
rejected for inclusion into mainline kernel.

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: wl1251: NVS firmware data

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 544 bytes --]

On Monday 08 December 2014 21:57:21 Greg Kroah-Hartman wrote:
> Use your own custom usermode helper for stuff like this, not
> the firmware interface.  But use a binary sysfs file if you
> want, that seems to make sense for it...
> 
> greg k-h

Patch for telling permanent mac address from userspace via sysfs 
file was rejected for inclusion into mainline kernel.

So I do not think that now maintainers of network subsystem allow 
it for nvs data...

https://lkml.org/lkml/2013/12/8/35

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: wl1251: NVS firmware data

[Marcel Holtmann]

Hi Pali,

>> Use your own custom usermode helper for stuff like this, not
>> the firmware interface.  But use a binary sysfs file if you
>> want, that seems to make sense for it...
>> 
>> greg k-h
> 
> Patch for telling permanent mac address from userspace via sysfs 
> file was rejected for inclusion into mainline kernel.
> 
> So I do not think that now maintainers of network subsystem allow 
> it for nvs data...
> 
> https://lkml.org/lkml/2013/12/8/35

this magic sysfs that has to be written to at the right time of the boot process to make this all work is something that will not work. I does not work for WiFi and it does not work for Bluetooth. Seems the discussion come full circle now and we are back to square one.

The problem is that either the driver does not register the device with mac80211 until it gets the MAC address provided by userspace or mac80211 should get an unconfigured state. The unconfigured state is something telling userspace that the device is present, but needs a few extra configuration information before it can become useful.

For Bluetooth, I went the later route. Mainly because the missing address is such a generic problem that solving it in the core makes a lot more sense. The driver just tells the core that it has no address for this device and provides a custom callback to configure the address when provided. After that everything resumes as normal.

However userspace gets informed when an unconfigured device gets added and at boot it can just query the list of unconfigured devices, or at runtime wait to be told there is a new unconfigured device that needs its attention. And then just provide the information. Once all information are present, the devices disappears as unconfigured and shows up as configured device ready for normal operation.

https://git.kernel.org/cgit/bluetooth/bluez.git/tree/doc/mgmt-api.txt#n2006
http://permalink.gmane.org/gmane.linux.bluez.kernel/50716

Can something similar be added to cfg80211 + nl80211. It bet it can, but that is up to Johannes to decide and someone to actually do the work and implement it.

Regards

Marcel

Re: wl1251: NVS firmware data

[Ming Lei]

On Tue, Dec 9, 2014 at 4:57 AM, Greg Kroah-Hartman
<gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org> wrote:
> On Mon, Dec 08, 2014 at 05:47:30PM +0100, Pali Rohár wrote:
>> On Monday 08 December 2014 17:37:14 Greg Kroah-Hartman wrote:
>> > On Mon, Dec 08, 2014 at 11:18:18PM +0800, Ming Lei wrote:
>> > > On Sat, Dec 6, 2014 at 9:02 PM, Pali Rohár
>> <pali.rohar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> > > > On Saturday 06 December 2014 13:49:54 Pavel Machek wrote:
>> > > >  /**
>> > > >
>> > > > + * request_firmware_prefer_user: - prefer usermode helper
>> > > > for loading firmware + * @firmware_p: pointer to firmware
>> > > > image
>> > > > + * @name: name of firmware file
>> > > > + * @device: device for which firmware is being loaded
>> > > > + *
>> > > > + * This function works pretty much like
>> > > > request_firmware(), but it prefer + * usermode helper. If
>> > > > usermode helper fails then it fallback to direct access.
>> > > > + * Usefull for dynamic or model specific firmware data.
>> > > > + **/
>> > > > +int request_firmware_prefer_user(const struct firmware
>> > > > **firmware_p, +                           const char
>> > > > *name, struct device *device) +{
>> > > > +       int ret;
>> > > > +       __module_get(THIS_MODULE);
>> > > > +       ret = _request_firmware(firmware_p, name, device,
>> > > > +                               FW_OPT_UEVENT |
>> > > > FW_OPT_PREFER_USER); +       module_put(THIS_MODULE);
>> > > > +       return ret;
>> > > > +}
>> > > > +EXPORT_SYMBOL_GPL(request_firmware_prefer_user);
>> > >
>> > > I'd like to introduce request_firmware_user() which only
>> > > requests firmware from user space, and this way is simpler
>> > > and more flexible since we have request_firmware_direct()
>> > > already.
>> >
>> > Why would a driver care about what program provides the
>> > firmware?  It shouldn't at all, and we want to get rid of the
>> > userspace firmware loader, not encourage drivers to use it
>> > "exclusively" at all.
>> >
>>
>> Do not remove it! Without userspace firmware loader it is
>> impossible to load dynamic firmware files.
>
> You should not be loading "dynamic" firmware files with the firmware
> interface, as that's not a "firmware" file anymore, it's a "special
> binary file that my driver needs to be created and sent into the
> kernel."

It is reasonable to put firmware somewhere instead of default
search path, maybe in network.

>
> Use your own custom usermode helper for stuff like this, not the
> firmware interface.  But use a binary sysfs file if you want, that seems
> to make sense for it...

It sounds like implementing an variant of request_firmware_user(), :-)

Thanks,
Ming Lei
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: wl1251: NVS firmware data

[Greg Kroah-Hartman]

On Tue, Dec 09, 2014 at 08:48:28AM +0800, Ming Lei wrote:
> On Tue, Dec 9, 2014 at 4:57 AM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > On Mon, Dec 08, 2014 at 05:47:30PM +0100, Pali Rohár wrote:
> >> On Monday 08 December 2014 17:37:14 Greg Kroah-Hartman wrote:
> >> > On Mon, Dec 08, 2014 at 11:18:18PM +0800, Ming Lei wrote:
> >> > > On Sat, Dec 6, 2014 at 9:02 PM, Pali Rohár
> >> <pali.rohar@gmail.com> wrote:
> >> > > > On Saturday 06 December 2014 13:49:54 Pavel Machek wrote:
> >> > > >  /**
> >> > > >
> >> > > > + * request_firmware_prefer_user: - prefer usermode helper
> >> > > > for loading firmware + * @firmware_p: pointer to firmware
> >> > > > image
> >> > > > + * @name: name of firmware file
> >> > > > + * @device: device for which firmware is being loaded
> >> > > > + *
> >> > > > + * This function works pretty much like
> >> > > > request_firmware(), but it prefer + * usermode helper. If
> >> > > > usermode helper fails then it fallback to direct access.
> >> > > > + * Usefull for dynamic or model specific firmware data.
> >> > > > + **/
> >> > > > +int request_firmware_prefer_user(const struct firmware
> >> > > > **firmware_p, +                           const char
> >> > > > *name, struct device *device) +{
> >> > > > +       int ret;
> >> > > > +       __module_get(THIS_MODULE);
> >> > > > +       ret = _request_firmware(firmware_p, name, device,
> >> > > > +                               FW_OPT_UEVENT |
> >> > > > FW_OPT_PREFER_USER); +       module_put(THIS_MODULE);
> >> > > > +       return ret;
> >> > > > +}
> >> > > > +EXPORT_SYMBOL_GPL(request_firmware_prefer_user);
> >> > >
> >> > > I'd like to introduce request_firmware_user() which only
> >> > > requests firmware from user space, and this way is simpler
> >> > > and more flexible since we have request_firmware_direct()
> >> > > already.
> >> >
> >> > Why would a driver care about what program provides the
> >> > firmware?  It shouldn't at all, and we want to get rid of the
> >> > userspace firmware loader, not encourage drivers to use it
> >> > "exclusively" at all.
> >> >
> >>
> >> Do not remove it! Without userspace firmware loader it is
> >> impossible to load dynamic firmware files.
> >
> > You should not be loading "dynamic" firmware files with the firmware
> > interface, as that's not a "firmware" file anymore, it's a "special
> > binary file that my driver needs to be created and sent into the
> > kernel."
> 
> It is reasonable to put firmware somewhere instead of default
> search path, maybe in network.

That's why we allow you to change the firmware search path at build
time, and kernel boot time (or firmware class module load time.)  To do
this on a per-firmware-file basis is crazy.

thanks,

greg k-h