wl1251 & mac address & calibration data

wl1251 & mac address & calibration data

[Pali Rohár]

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

Hi! I will open discussion about mac address and calibration data for 
wl1251 wireless chip again...

Problem: Mac address & calibration data for wl1251 chip on Nokia N900 
are stored on second nand partition (mtd1) in special proprietary format 
which is used only for Nokia N900 (probably on N8x0 and N9 too). 
Wireless driver wl1251.ko cannot work without mac address and 
calibration data.

Absence of mac address cause that driver generates random mac address at 
every kernel boot which has couple of problems (unstable identifier of 
wireless device due to udev permanent storage rules; unpredictable 
behaviour for dhcp mac address assignment, mac address filtering, ...).

Currently there is no way to set (permanent) mac address for network 
interface from userspace. And it does not make sense to implement in 
linux kernel large parser for proprietary format of second nand 
partition where is mac address stored only for one device -- Nokia N900.

Driver wl1251.ko loads calibration data via request_firmware() for file 
wl1251-nvs.bin. There are some "example" calibration file in linux-
firmware repository, but it is not suitable for normal usage as real 
calibration data are per-device specific.

So questions are:

1) How to set mac address from userspace for that wl1251 interface? In 
userspace I can write parser for that proprietary format of nand 
partition and extract mac address from it

2) How to send calibration data to wl1251 driver? Those are again stored 
in proprietary format and I can write userspace parser for it.

-- 
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 & mac address & calibration data

[Pali Rohár]

On Friday 11 November 2016 18:20:50 Pali Rohár wrote:
> Hi! I will open discussion about mac address and calibration data for 
> wl1251 wireless chip again...
> 
> Problem: Mac address & calibration data for wl1251 chip on Nokia N900 
> are stored on second nand partition (mtd1) in special proprietary format 
> which is used only for Nokia N900 (probably on N8x0 and N9 too). 
> Wireless driver wl1251.ko cannot work without mac address and 
> calibration data.
> 
> Absence of mac address cause that driver generates random mac address at 
> every kernel boot which has couple of problems (unstable identifier of 
> wireless device due to udev permanent storage rules; unpredictable 
> behaviour for dhcp mac address assignment, mac address filtering, ...).
> 
> Currently there is no way to set (permanent) mac address for network 
> interface from userspace. And it does not make sense to implement in 
> linux kernel large parser for proprietary format of second nand 
> partition where is mac address stored only for one device -- Nokia N900.
> 
> Driver wl1251.ko loads calibration data via request_firmware() for file 
> wl1251-nvs.bin. There are some "example" calibration file in linux-
> firmware repository, but it is not suitable for normal usage as real 
> calibration data are per-device specific.
> 
> So questions are:
> 
> 1) How to set mac address from userspace for that wl1251 interface? In 
> userspace I can write parser for that proprietary format of nand 
> partition and extract mac address from it

Proposed solutions for 1)

* Introduce new IOCL for setting that permanent mac address from
  userspace. Currently we have IOCL for get request

* Use request_firmware() (with flag from 2)) to ask for mac address from
  userspace. This is already used by wl12xx driver (as mac address is
  part of calibration data firmware file)

* Allow to set mac address via sysfs file, e.g.
  /sys/class/ieee80211/phy0/macaddress

> 2) How to send calibration data to wl1251 driver? Those are again stored 
> in proprietary format and I can write userspace parser for it.

Proposed solution for 2)

Introduce new flag for request_firmware(), so it first try to use
userspace helper for loading firmware file with possibility to fallback
to direct VFS access.


So... what do you think about it?

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

Re: wl1251 & mac address & calibration data

[Michal Kazior]

On 21 November 2016 at 16:51, Pali Rohár <pali.rohar@gmail.com> wrote:
> On Friday 11 November 2016 18:20:50 Pali Rohár wrote:
>> Hi! I will open discussion about mac address and calibration data for
>> wl1251 wireless chip again...
>>
>> Problem: Mac address & calibration data for wl1251 chip on Nokia N900
>> are stored on second nand partition (mtd1) in special proprietary format
>> which is used only for Nokia N900 (probably on N8x0 and N9 too).
>> Wireless driver wl1251.ko cannot work without mac address and
>> calibration data.

Same problem applies to some ath9k/ath10k supported routers. Some even
carry mac address as implicit offset from ethernet mac address. As far
as I understand OpenWRT cooks cal blobs on first boot prior to loading
modules.


>> Absence of mac address cause that driver generates random mac address at
>> every kernel boot which has couple of problems (unstable identifier of
>> wireless device due to udev permanent storage rules; unpredictable
>> behaviour for dhcp mac address assignment, mac address filtering, ...).
>>
>> Currently there is no way to set (permanent) mac address for network
>> interface from userspace. And it does not make sense to implement in
>> linux kernel large parser for proprietary format of second nand
>> partition where is mac address stored only for one device -- Nokia N900.
>>
>> Driver wl1251.ko loads calibration data via request_firmware() for file
>> wl1251-nvs.bin. There are some "example" calibration file in linux-
>> firmware repository, but it is not suitable for normal usage as real
>> calibration data are per-device specific.

You could hook up a script that cooks up the cal/mac file via
modprobe's install hook, no?


Michał

Re: wl1251 & mac address & calibration data

[Pali Rohár]

On Tuesday 22 November 2016 16:22:57 Michal Kazior wrote:
> On 21 November 2016 at 16:51, Pali Rohár <pali.rohar@gmail.com> wrote:
> > On Friday 11 November 2016 18:20:50 Pali Rohár wrote:
> >> Hi! I will open discussion about mac address and calibration data for
> >> wl1251 wireless chip again...
> >>
> >> Problem: Mac address & calibration data for wl1251 chip on Nokia N900
> >> are stored on second nand partition (mtd1) in special proprietary format
> >> which is used only for Nokia N900 (probably on N8x0 and N9 too).
> >> Wireless driver wl1251.ko cannot work without mac address and
> >> calibration data.
> 
> Same problem applies to some ath9k/ath10k supported routers. Some even
> carry mac address as implicit offset from ethernet mac address. As far
> as I understand OpenWRT cooks cal blobs on first boot prior to loading
> modules.

So... wl1251 on Nokia N900 is not alone and this problem is there for
more drivers and devices. Which means we should come up with some
generic solution.

> >> Absence of mac address cause that driver generates random mac address at
> >> every kernel boot which has couple of problems (unstable identifier of
> >> wireless device due to udev permanent storage rules; unpredictable
> >> behaviour for dhcp mac address assignment, mac address filtering, ...).
> >>
> >> Currently there is no way to set (permanent) mac address for network
> >> interface from userspace. And it does not make sense to implement in
> >> linux kernel large parser for proprietary format of second nand
> >> partition where is mac address stored only for one device -- Nokia N900.
> >>
> >> Driver wl1251.ko loads calibration data via request_firmware() for file
> >> wl1251-nvs.bin. There are some "example" calibration file in linux-
> >> firmware repository, but it is not suitable for normal usage as real
> >> calibration data are per-device specific.
> 
> You could hook up a script that cooks up the cal/mac file via
> modprobe's install hook, no?

Via modprobe hook I can either pass custom module parameter or call any
other system (shell) commands.

As wl1251.ko does not accept mac_address as module parameter, such
modprobe hook does not help -- as there is absolutely no way from
userspace to set or change (permanent) mac address.

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

Re: wl1251 & mac address & calibration data

[Michal Kazior]

On 22 November 2016 at 16:31, Pali Rohár <pali.rohar@gmail.com> wrote:
> On Tuesday 22 November 2016 16:22:57 Michal Kazior wrote:
>> On 21 November 2016 at 16:51, Pali Rohár <pali.rohar@gmail.com> wrote:
>> > On Friday 11 November 2016 18:20:50 Pali Rohár wrote:
>> >> Hi! I will open discussion about mac address and calibration data for
>> >> wl1251 wireless chip again...
>> >>
>> >> Problem: Mac address & calibration data for wl1251 chip on Nokia N900
>> >> are stored on second nand partition (mtd1) in special proprietary format
>> >> which is used only for Nokia N900 (probably on N8x0 and N9 too).
>> >> Wireless driver wl1251.ko cannot work without mac address and
>> >> calibration data.
>>
>> Same problem applies to some ath9k/ath10k supported routers. Some even
>> carry mac address as implicit offset from ethernet mac address. As far
>> as I understand OpenWRT cooks cal blobs on first boot prior to loading
>> modules.
>
> So... wl1251 on Nokia N900 is not alone and this problem is there for
> more drivers and devices. Which means we should come up with some
> generic solution.

This isn't particularly a problem for ath9k/ath10k.

Let me give you more background on ath10k.

ath10k devices can come with caldata and macaddr stored in their
OTP/EEPROM. In that case a generic "template" board file is used.
Userspace doesn't need to do anything special.

Some vendors however decide to use flash partition to store caldata.
In that case ath10k expects userspace to prepare cal-$bus-$devname.bin
files, each for a different radio (you can have multiple radios on a
system).

Now translating this for wl1251 I would expect it should also use
something like wl1251-nvs-sdio-0x0001.bin for devices like N900 that
have caldata on flash partition (instead of the generic
wl1251-nvs.bin). I'm not sure if wl1251-nvs.bin is something
comparable to (the generic) board.bin ath10k has though. Maybe the
entire idea behind wl1251-nvs.bin is flawed as it's supposed to be
device specific and is oblivious to possibility of having multiple
wl1251 radios on one system (probably sane assumption from practical
standpoint but still).


>> >> Absence of mac address cause that driver generates random mac address at
>> >> every kernel boot which has couple of problems (unstable identifier of
>> >> wireless device due to udev permanent storage rules; unpredictable
>> >> behaviour for dhcp mac address assignment, mac address filtering, ...).
>> >>
>> >> Currently there is no way to set (permanent) mac address for network
>> >> interface from userspace. And it does not make sense to implement in
>> >> linux kernel large parser for proprietary format of second nand
>> >> partition where is mac address stored only for one device -- Nokia N900.
>> >>
>> >> Driver wl1251.ko loads calibration data via request_firmware() for file
>> >> wl1251-nvs.bin. There are some "example" calibration file in linux-
>> >> firmware repository, but it is not suitable for normal usage as real
>> >> calibration data are per-device specific.
>>
>> You could hook up a script that cooks up the cal/mac file via
>> modprobe's install hook, no?
>
> Via modprobe hook I can either pass custom module parameter or call any
> other system (shell) commands.
>
> As wl1251.ko does not accept mac_address as module parameter, such
> modprobe hook does not help -- as there is absolutely no way from
> userspace to set or change (permanent) mac address.

Quoting modprobe.d manual:

>       install modulename command...
>           This command instructs modprobe to run your
>           command instead of inserting the module in the
>           kernel as normal. The command can be any shell
>           command: this allows you to do any kind of
>           complex processing you might wish. [...]

You can hook up a script that cooks up wl1251-nvs.bin (caldata,
macaddr) and then insmod the actual wl1251.ko module. Or you can just
cook up the nvs on first device boot and store it in /lib/firmware
(possibly overwriting the "generic" wl1251 from linux-firmware).


Michal

Re: wl1251 & mac address & calibration data

[Pali Rohár]

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

On Tuesday 22 November 2016 17:14:28 Michal Kazior wrote:
> On 22 November 2016 at 16:31, Pali Rohár <pali.rohar@gmail.com> wrote:
> > On Tuesday 22 November 2016 16:22:57 Michal Kazior wrote:
> >> On 21 November 2016 at 16:51, Pali Rohár <pali.rohar@gmail.com>
> >> wrote:
> >> > On Friday 11 November 2016 18:20:50 Pali Rohár wrote:
> >> >> Hi! I will open discussion about mac address and calibration
> >> >> data for wl1251 wireless chip again...
> >> >> 
> >> >> Problem: Mac address & calibration data for wl1251 chip on
> >> >> Nokia N900 are stored on second nand partition (mtd1) in
> >> >> special proprietary format which is used only for Nokia N900
> >> >> (probably on N8x0 and N9 too). Wireless driver wl1251.ko
> >> >> cannot work without mac address and calibration data.
> >> 
> >> Same problem applies to some ath9k/ath10k supported routers. Some
> >> even carry mac address as implicit offset from ethernet mac
> >> address. As far as I understand OpenWRT cooks cal blobs on first
> >> boot prior to loading modules.
> > 
> > So... wl1251 on Nokia N900 is not alone and this problem is there
> > for more drivers and devices. Which means we should come up with
> > some generic solution.
> 
> This isn't particularly a problem for ath9k/ath10k.
> 
> Let me give you more background on ath10k.
> 
> ath10k devices can come with caldata and macaddr stored in their
> OTP/EEPROM. In that case a generic "template" board file is used.
> Userspace doesn't need to do anything special.
> 
> Some vendors however decide to use flash partition to store caldata.
> In that case ath10k expects userspace to prepare
> cal-$bus-$devname.bin files, each for a different radio (you can
> have multiple radios on a system).
> 
> Now translating this for wl1251 I would expect it should also use
> something like wl1251-nvs-sdio-0x0001.bin for devices like N900 that
> have caldata on flash partition (instead of the generic
> wl1251-nvs.bin). I'm not sure if wl1251-nvs.bin is something
> comparable to (the generic) board.bin ath10k has though. Maybe the
> entire idea behind wl1251-nvs.bin is flawed as it's supposed to be
> device specific and is oblivious to possibility of having multiple
> wl1251 radios on one system (probably sane assumption from practical
> standpoint but still).

Basically nvs data are device specific, in ideal case they should be 
generated in factory by some calibration process (or so).

> >> >> Absence of mac address cause that driver generates random mac
> >> >> address at every kernel boot which has couple of problems
> >> >> (unstable identifier of wireless device due to udev permanent
> >> >> storage rules; unpredictable behaviour for dhcp mac address
> >> >> assignment, mac address filtering, ...).
> >> >> 
> >> >> Currently there is no way to set (permanent) mac address for
> >> >> network interface from userspace. And it does not make sense
> >> >> to implement in linux kernel large parser for proprietary
> >> >> format of second nand partition where is mac address stored
> >> >> only for one device -- Nokia N900.
> >> >> 
> >> >> Driver wl1251.ko loads calibration data via request_firmware()
> >> >> for file wl1251-nvs.bin. There are some "example" calibration
> >> >> file in linux- firmware repository, but it is not suitable for
> >> >> normal usage as real calibration data are per-device specific.
> >> 
> >> You could hook up a script that cooks up the cal/mac file via
> >> modprobe's install hook, no?
> > 
> > Via modprobe hook I can either pass custom module parameter or call
> > any other system (shell) commands.
> > 
> > As wl1251.ko does not accept mac_address as module parameter, such
> > modprobe hook does not help -- as there is absolutely no way from
> > userspace to set or change (permanent) mac address.
> 
> Quoting modprobe.d manual:
> >       install modulename command...
> >       
> >           This command instructs modprobe to run your
> >           command instead of inserting the module in the
> >           kernel as normal. The command can be any shell
> >           command: this allows you to do any kind of
> >           complex processing you might wish. [...]

I know. But this do not allow me to send mac address to kernel -- as 
kernel does not support such command yet (reason for my first question).

> You can hook up a script that cooks up wl1251-nvs.bin (caldata,
> macaddr) and then insmod the actual wl1251.ko module. Or you can just
> cook up the nvs on first device boot and store it in /lib/firmware
> (possibly overwriting the "generic" wl1251 from linux-firmware).

This is what I would like to prevent -- overwriting (possible readonly) 
system files with some device specific. It is really bad idea!

-- 
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 & mac address & calibration data

[Arend Van Spriel]

On 22-11-2016 18:05, Pali Rohár wrote:
> On Tuesday 22 November 2016 17:14:28 Michal Kazior wrote:
>> On 22 November 2016 at 16:31, Pali Rohár <pali.rohar@gmail.com> wrote:
>>> On Tuesday 22 November 2016 16:22:57 Michal Kazior wrote:
>>>> On 21 November 2016 at 16:51, Pali Rohár <pali.rohar@gmail.com>
>>>> wrote:
>>>>> On Friday 11 November 2016 18:20:50 Pali Rohár wrote:
>>>>>> Hi! I will open discussion about mac address and calibration
>>>>>> data for wl1251 wireless chip again...
>>>>>>
>>>>>> Problem: Mac address & calibration data for wl1251 chip on
>>>>>> Nokia N900 are stored on second nand partition (mtd1) in
>>>>>> special proprietary format which is used only for Nokia N900
>>>>>> (probably on N8x0 and N9 too). Wireless driver wl1251.ko
>>>>>> cannot work without mac address and calibration data.
>>>>
>>>> Same problem applies to some ath9k/ath10k supported routers. Some
>>>> even carry mac address as implicit offset from ethernet mac
>>>> address. As far as I understand OpenWRT cooks cal blobs on first
>>>> boot prior to loading modules.
>>>
>>> So... wl1251 on Nokia N900 is not alone and this problem is there
>>> for more drivers and devices. Which means we should come up with
>>> some generic solution.
>>
>> This isn't particularly a problem for ath9k/ath10k.
>>
>> Let me give you more background on ath10k.
>>
>> ath10k devices can come with caldata and macaddr stored in their
>> OTP/EEPROM. In that case a generic "template" board file is used.
>> Userspace doesn't need to do anything special.
>>
>> Some vendors however decide to use flash partition to store caldata.
>> In that case ath10k expects userspace to prepare
>> cal-$bus-$devname.bin files, each for a different radio (you can
>> have multiple radios on a system).
>>
>> Now translating this for wl1251 I would expect it should also use
>> something like wl1251-nvs-sdio-0x0001.bin for devices like N900 that
>> have caldata on flash partition (instead of the generic
>> wl1251-nvs.bin). I'm not sure if wl1251-nvs.bin is something
>> comparable to (the generic) board.bin ath10k has though. Maybe the
>> entire idea behind wl1251-nvs.bin is flawed as it's supposed to be
>> device specific and is oblivious to possibility of having multiple
>> wl1251 radios on one system (probably sane assumption from practical
>> standpoint but still).
> 
> Basically nvs data are device specific, in ideal case they should be 
> generated in factory by some calibration process (or so).

For brcmfmac we have what we call nvram data, which is determined during
manufacturing. We use the firmware_class API to obtain that file, but on
router it may be stored in flash. So an API was created for that router
architecture and brcmfmac calls that API [1]. Not a generic solution but
it gets the job done. Personally, I would have liked this to be handled
behind the firmware_class API to hide the storage details from the driver.

Regards,
Arend

[1]
http://lxr.free-electrons.com/source/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c#L449

Re: wl1251 & mac address & calibration data

[Pavel Machek]

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

Hi!

> > > As wl1251.ko does not accept mac_address as module parameter, such
> > > modprobe hook does not help -- as there is absolutely no way from
> > > userspace to set or change (permanent) mac address.
> > 
> > Quoting modprobe.d manual:
> > >       install modulename command...
> > >       
> > >           This command instructs modprobe to run your
> > >           command instead of inserting the module in the
> > >           kernel as normal. The command can be any shell
> > >           command: this allows you to do any kind of
> > >           complex processing you might wish. [...]
> 
> I know. But this do not allow me to send mac address to kernel -- as 
> kernel does not support such command yet (reason for my first
> question).

Plus, this does not really work for cases where wl1251 is not a
module.

> > You can hook up a script that cooks up wl1251-nvs.bin (caldata,
> > macaddr) and then insmod the actual wl1251.ko module. Or you can just
> > cook up the nvs on first device boot and store it in /lib/firmware
> > (possibly overwriting the "generic" wl1251 from linux-firmware).
> 
> This is what I would like to prevent -- overwriting (possible readonly) 
> system files with some device specific. It is really bad idea!

Agreed.

"ifconfig hw ether XX" normally sets the address. I guess that's
ioctl? And I guess we should use similar mechanism for permanent
address.

Best regards,

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: wl1251 & mac address & calibration data

[Pali Rohár]

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

On Wednesday 23 November 2016 23:23:35 Pavel Machek wrote:
> Hi!
> 
> > > > As wl1251.ko does not accept mac_address as module parameter,
> > > > such modprobe hook does not help -- as there is absolutely no
> > > > way from userspace to set or change (permanent) mac address.
> > > 
> > > Quoting modprobe.d manual:
> > > >       install modulename command...
> > > >       
> > > >           This command instructs modprobe to run your
> > > >           command instead of inserting the module in the
> > > >           kernel as normal. The command can be any shell
> > > >           command: this allows you to do any kind of
> > > >           complex processing you might wish. [...]
> > 
> > I know. But this do not allow me to send mac address to kernel --
> > as kernel does not support such command yet (reason for my first
> > question).
> 
> Plus, this does not really work for cases where wl1251 is not a
> module.

Yes, this is another problem.

> > > You can hook up a script that cooks up wl1251-nvs.bin (caldata,
> > > macaddr) and then insmod the actual wl1251.ko module. Or you can
> > > just cook up the nvs on first device boot and store it in
> > > /lib/firmware (possibly overwriting the "generic" wl1251 from
> > > linux-firmware).
> > 
> > This is what I would like to prevent -- overwriting (possible
> > readonly) system files with some device specific. It is really bad
> > idea!
> 
> Agreed.
> 
> "ifconfig hw ether XX" normally sets the address. I guess that's
> ioctl?

This sets temporary address and it is ioctl. IIRC same as what ethtool 
uses. (ifconfig is already deprecated).

> And I guess we should use similar mechanism for permanent
> address.

I'm not sure here... Above ioctl ↑↑↑ is for changing temporary mac 
address. But here we do not want to change permanent mac address. We 
want to tell kernel driver current permanent mac address which is stored 
in permanent mac address storage (in N900 case in mtd). Just like 
userspace helper as kernel driver do not have code which can read 
permanent mac address.

-- 
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 & mac address & calibration data

[Pavel Machek]

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

Hi!

> > "ifconfig hw ether XX" normally sets the address. I guess that's
> > ioctl?
> 
> This sets temporary address and it is ioctl. IIRC same as what ethtool 
> uses. (ifconfig is already deprecated).
> 
> > And I guess we should use similar mechanism for permanent
> > address.
> 
> I'm not sure here... Above ioctl ↑↑↑ is for changing temporary mac 
> address. But here we do not want to change permanent mac address. We 
> want to tell kernel driver current permanent mac address which is
> stored

Well... I'd still use similar mechanism :-).
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: wl1251 & mac address & calibration data

[Pali Rohár]

On Thursday 24 November 2016 08:51:04 Pavel Machek wrote:
> Hi!
> 
> > > "ifconfig hw ether XX" normally sets the address. I guess that's
> > > ioctl?
> > 
> > This sets temporary address and it is ioctl. IIRC same as what ethtool 
> > uses. (ifconfig is already deprecated).
> > 
> > > And I guess we should use similar mechanism for permanent
> > > address.
> > 
> > I'm not sure here... Above ioctl ↑↑↑ is for changing temporary mac 
> > address. But here we do not want to change permanent mac address. We 
> > want to tell kernel driver current permanent mac address which is
> > stored
> 
> Well... I'd still use similar mechanism :-).

Thats problematic, because in time when wlan0 interface is registered
into system and visible in ifconfig output it already needs to have
permanent mac address assigned.

We should assign permanent mac address before wlan0 of wl1251 is
registered into system.

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

Re: wl1251 & mac address & calibration data

[Sebastian Reichel]

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

Hi,

On Thu, Nov 24, 2016 at 09:33:29AM +0100, Pali Rohár wrote:
> On Thursday 24 November 2016 08:51:04 Pavel Machek wrote:
> > Hi!
> > 
> > > > "ifconfig hw ether XX" normally sets the address. I guess that's
> > > > ioctl?
> > > 
> > > This sets temporary address and it is ioctl. IIRC same as what ethtool 
> > > uses. (ifconfig is already deprecated).
> > > 
> > > > And I guess we should use similar mechanism for permanent
> > > > address.
> > > 
> > > I'm not sure here... Above ioctl ↑↑↑ is for changing temporary mac 
> > > address. But here we do not want to change permanent mac address. We 
> > > want to tell kernel driver current permanent mac address which is
> > > stored
> > 
> > Well... I'd still use similar mechanism :-).
> 
> Thats problematic, because in time when wlan0 interface is registered
> into system and visible in ifconfig output it already needs to have
> permanent mac address assigned.
> 
> We should assign permanent mac address before wlan0 of wl1251 is
> registered into system.

You can just add the MAC address to the NVS data, which is also
required for the device initialization.

I wonder if those information could be put into DT. Iirc some
network devices get their MAC address from DT. Maybe we can add
all NVS info to DT? How much data is it?

Userspace application can add all those information to the DT
using a DT overlay. Also the u-boot could parse and add it at
some point in the future.

-- Sebastian

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

Re: wl1251 & mac address & calibration data

[Pali Rohár]

On Thursday 24 November 2016 16:13:17 Sebastian Reichel wrote:
> Hi,
> 
> On Thu, Nov 24, 2016 at 09:33:29AM +0100, Pali Rohár wrote:
> > On Thursday 24 November 2016 08:51:04 Pavel Machek wrote:
> > > Hi!
> > > 
> > > > > "ifconfig hw ether XX" normally sets the address. I guess that's
> > > > > ioctl?
> > > > 
> > > > This sets temporary address and it is ioctl. IIRC same as what ethtool 
> > > > uses. (ifconfig is already deprecated).
> > > > 
> > > > > And I guess we should use similar mechanism for permanent
> > > > > address.
> > > > 
> > > > I'm not sure here... Above ioctl ↑↑↑ is for changing temporary mac 
> > > > address. But here we do not want to change permanent mac address. We 
> > > > want to tell kernel driver current permanent mac address which is
> > > > stored
> > > 
> > > Well... I'd still use similar mechanism :-).
> > 
> > Thats problematic, because in time when wlan0 interface is registered
> > into system and visible in ifconfig output it already needs to have
> > permanent mac address assigned.
> > 
> > We should assign permanent mac address before wlan0 of wl1251 is
> > registered into system.
> 
> You can just add the MAC address to the NVS data, which is also
> required for the device initialization.

NVS data file has fixed size, there is IIRC no place for it.

But one of my suggestion was to use another request_firmware for MAC
address. So this is similar to what you are proposing, as NVS data are
loaded by request_firmware too...

> I wonder if those information could be put into DT. Iirc some
> network devices get their MAC address from DT. Maybe we can add
> all NVS info to DT? How much data is it?

Proprietary, signed and closed bootloader NOLO does not support DT. So
for booting you need to append DTS file to kernel image.

U-Boot is optional and can be used as intermediate bootloader between
NOLO and kernel. But still it has problems with reading from nand, so
cannot read NVS data nor MAC address.

> Userspace application can add all those information to the DT
> using a DT overlay. Also the u-boot could parse and add it at
> some point in the future.

In case when wl1251 is statically linked into kernel image, it is loaded
and initialized before even userspace applications starts.

So no... adding NVS data or MAC address into DT or DT overlay is not a
solution.

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

Re: wl1251 & mac address & calibration data

[Ivaylo Dimitrov]

Hi,

On 24.11.2016 17:20, Pali Rohár wrote:
>
> In case when wl1251 is statically linked into kernel image, it is loaded
> and initialized before even userspace applications starts.
>

Which leaves no option, but integrating libcal into kernel :).

Ivo

Re: wl1251 & mac address & calibration data

[Sebastian Reichel]

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

Hi,

On Thu, Nov 24, 2016 at 04:20:45PM +0100, Pali Rohár wrote:
> On Thursday 24 November 2016 16:13:17 Sebastian Reichel wrote:
> > On Thu, Nov 24, 2016 at 09:33:29AM +0100, Pali Rohár wrote:
> > > On Thursday 24 November 2016 08:51:04 Pavel Machek wrote:
> > > > > > "ifconfig hw ether XX" normally sets the address. I guess that's
> > > > > > ioctl?
> > > > > 
> > > > > This sets temporary address and it is ioctl. IIRC same as what ethtool 
> > > > > uses. (ifconfig is already deprecated).
> > > > > 
> > > > > > And I guess we should use similar mechanism for permanent
> > > > > > address.
> > > > > 
> > > > > I'm not sure here... Above ioctl ↑↑↑ is for changing temporary mac 
> > > > > address. But here we do not want to change permanent mac address. We 
> > > > > want to tell kernel driver current permanent mac address which is
> > > > > stored
> > > > 
> > > > Well... I'd still use similar mechanism :-).
> > > 
> > > Thats problematic, because in time when wlan0 interface is registered
> > > into system and visible in ifconfig output it already needs to have
> > > permanent mac address assigned.
> > > 
> > > We should assign permanent mac address before wlan0 of wl1251 is
> > > registered into system.
> > 
> > You can just add the MAC address to the NVS data, which is also
> > required for the device initialization.
> 
> NVS data file has fixed size, there is IIRC no place for it.
> 
> But one of my suggestion was to use another request_firmware for MAC
> address. So this is similar to what you are proposing, as NVS data are
> loaded by request_firmware too...

Just append it to NVS data and modify the size check accordingly?

> > I wonder if those information could be put into DT. Iirc some
> > network devices get their MAC address from DT. Maybe we can add
> > all NVS info to DT? How much data is it?
> 
> Proprietary, signed and closed bootloader NOLO does not support DT. So
> for booting you need to append DTS file to kernel image.

Yeah, so NOLO without U-Boot will depend on userspace to fixup DT.

> U-Boot is optional and can be used as intermediate bootloader between
> NOLO and kernel. But still it has problems with reading from nand, so
> cannot read NVS data nor MAC address.

It may in the future?

> > Userspace application can add all those information to the DT
> > using a DT overlay. Also the u-boot could parse and add it at
> > some point in the future.
> 
> In case when wl1251 is statically linked into kernel image, it is loaded
> and initialized before even userspace applications starts.
>
> So no... adding NVS data or MAC address into DT or DT overlay is not a
> solution.

Actually with data loaded from DT you *can* load data quite early in
the boot process, while your suggestions always require userspace.
So you argument against yourself?

-- Sebastian

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

Re: wl1251 & mac address & calibration data

[Pali Rohár]

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

On Thursday 24 November 2016 17:08:30 Sebastian Reichel wrote:
> Hi,
> 
> On Thu, Nov 24, 2016 at 04:20:45PM +0100, Pali Rohár wrote:
> > On Thursday 24 November 2016 16:13:17 Sebastian Reichel wrote:
> > > On Thu, Nov 24, 2016 at 09:33:29AM +0100, Pali Rohár wrote:
> > > > On Thursday 24 November 2016 08:51:04 Pavel Machek wrote:
> > > > > > > "ifconfig hw ether XX" normally sets the address. I guess
> > > > > > > that's ioctl?
> > > > > > 
> > > > > > This sets temporary address and it is ioctl. IIRC same as
> > > > > > what ethtool uses. (ifconfig is already deprecated).
> > > > > > 
> > > > > > > And I guess we should use similar mechanism for permanent
> > > > > > > address.
> > > > > > 
> > > > > > I'm not sure here... Above ioctl ↑↑↑ is for changing
> > > > > > temporary mac address. But here we do not want to change
> > > > > > permanent mac address. We want to tell kernel driver
> > > > > > current permanent mac address which is stored
> > > > > 
> > > > > Well... I'd still use similar mechanism :-).
> > > > 
> > > > Thats problematic, because in time when wlan0 interface is
> > > > registered into system and visible in ifconfig output it
> > > > already needs to have permanent mac address assigned.
> > > > 
> > > > We should assign permanent mac address before wlan0 of wl1251
> > > > is registered into system.
> > > 
> > > You can just add the MAC address to the NVS data, which is also
> > > required for the device initialization.
> > 
> > NVS data file has fixed size, there is IIRC no place for it.
> > 
> > But one of my suggestion was to use another request_firmware for
> > MAC address. So this is similar to what you are proposing, as NVS
> > data are loaded by request_firmware too...
> 
> Just append it to NVS data and modify the size check accordingly?

First that would mean to have request_firmware() function which will not 
use direct VFS access, but instead use userspace helper.

NVS data file with some default values (not suitable for usage) is 
already present in linux-firmware and available in /lib/firmware/...

Also I'm not sure if such thing is allowed by license:
https://git.kernel.org/cgit/linux/kernel/git/firmware/linux-firmware.git/tree/LICENCE.ti-connectivity

> > > I wonder if those information could be put into DT. Iirc some
> > > network devices get their MAC address from DT. Maybe we can add
> > > all NVS info to DT? How much data is it?
> > 
> > Proprietary, signed and closed bootloader NOLO does not support DT.
> > So for booting you need to append DTS file to kernel image.
> 
> Yeah, so NOLO without U-Boot will depend on userspace to fixup DT.
> 
> > U-Boot is optional and can be used as intermediate bootloader
> > between NOLO and kernel. But still it has problems with reading
> > from nand, so cannot read NVS data nor MAC address.
> 
> It may in the future?

I already tried that, but I failed. I was not able to access N900's nand 
from u-boot. No idea where was problem...

And if somebody fix onenand in u-boot, then needs to reimplement Nokia's 
proprietary parser of that partition where is stored NVS and mac address 
&& make this support in upstream u-boot.

So... I doubt it will be in any future.

+ no men power

> > > Userspace application can add all those information to the DT
> > > using a DT overlay. Also the u-boot could parse and add it at
> > > some point in the future.
> > 
> > In case when wl1251 is statically linked into kernel image, it is
> > loaded and initialized before even userspace applications starts.
> > 
> > So no... adding NVS data or MAC address into DT or DT overlay is
> > not a solution.
> 
> Actually with data loaded from DT you *can* load data quite early in
> the boot process, while your suggestions always require userspace.
> So you argument against yourself?

You cannot add DTS in uboot (no support). And if you modify DTS on 
running kernel from userspace, then it is too late when wl1251 is 
statically linked into kernel image.

wl1251 will not wait until some userspace modify DTS and add data...

But request_firmware() in fallback mode *can* wait for userspace so 
wl1251 can postpone its operation until mdev/udev/whatever starts 
listening for events and push needed firmware data to kernel.

So no, there is no argument against... request_firmware() in fallback 
mode with userspace helper is by design blocking and waiting for 
userspace. But waiting for some change in DST in kernel is just 
nonsense.

-- 
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 & mac address & calibration data

[Sebastian Reichel]

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

Hi,

On Thu, Nov 24, 2016 at 05:49:33PM +0100, Pali Rohár wrote:
> On Thursday 24 November 2016 17:08:30 Sebastian Reichel wrote:
> > On Thu, Nov 24, 2016 at 04:20:45PM +0100, Pali Rohár wrote:
> > > On Thursday 24 November 2016 16:13:17 Sebastian Reichel wrote:
> > > > On Thu, Nov 24, 2016 at 09:33:29AM +0100, Pali Rohár wrote:
> > > > > On Thursday 24 November 2016 08:51:04 Pavel Machek wrote:
> > > > > > > > "ifconfig hw ether XX" normally sets the address. I guess
> > > > > > > > that's ioctl?
> > > > > > > 
> > > > > > > This sets temporary address and it is ioctl. IIRC same as
> > > > > > > what ethtool uses. (ifconfig is already deprecated).
> > > > > > > 
> > > > > > > > And I guess we should use similar mechanism for permanent
> > > > > > > > address.
> > > > > > > 
> > > > > > > I'm not sure here... Above ioctl ↑↑↑ is for changing
> > > > > > > temporary mac address. But here we do not want to change
> > > > > > > permanent mac address. We want to tell kernel driver
> > > > > > > current permanent mac address which is stored
> > > > > > 
> > > > > > Well... I'd still use similar mechanism :-).
> > > > > 
> > > > > Thats problematic, because in time when wlan0 interface is
> > > > > registered into system and visible in ifconfig output it
> > > > > already needs to have permanent mac address assigned.
> > > > > 
> > > > > We should assign permanent mac address before wlan0 of wl1251
> > > > > is registered into system.
> > > > 
> > > > You can just add the MAC address to the NVS data, which is also
> > > > required for the device initialization.
> > > 
> > > NVS data file has fixed size, there is IIRC no place for it.
> > > 
> > > But one of my suggestion was to use another request_firmware for
> > > MAC address. So this is similar to what you are proposing, as NVS
> > > data are loaded by request_firmware too...
> > 
> > Just append it to NVS data and modify the size check accordingly?
> 
> First that would mean to have request_firmware() function which will not 
> use direct VFS access, but instead use userspace helper.

Permanent MAC is device specific init data, NVS is device specific
init data => load together.

The userspace helper stuff is only needed to get the data from
the NAND calibration area on the fly.

> NVS data file with some default values (not suitable for usage) is 
> already present in linux-firmware and available in /lib/firmware/...

You mentioned NVS data is fixed size, so this should be enough?

switch(loaded_size)
    case IMAGE_SIZE + MAC_SIZE:
        /* extract mac */
        /* fallthrough */
    case IMAGE_SIZE:
        /* load NVS */
        break;
    default:
        /* fail */
}

> Also I'm not sure if such thing is allowed by license:
> https://git.kernel.org/cgit/linux/kernel/git/firmware/linux-firmware.git/tree/LICENCE.ti-connectivity

concating data is not a modification, otherwise you can't
put the file in a filesystem.

> > > > I wonder if those information could be put into DT. Iirc some
> > > > network devices get their MAC address from DT. Maybe we can add
> > > > all NVS info to DT? How much data is it?
> > > 
> > > Proprietary, signed and closed bootloader NOLO does not support DT.
> > > So for booting you need to append DTS file to kernel image.
> > 
> > Yeah, so NOLO without U-Boot will depend on userspace to fixup DT.
> > 
> > > U-Boot is optional and can be used as intermediate bootloader
> > > between NOLO and kernel. But still it has problems with reading
> > > from nand, so cannot read NVS data nor MAC address.
> > 
> > It may in the future?
> 
> I already tried that, but I failed. I was not able to access N900's nand 
> from u-boot. No idea where was problem...
>
> And if somebody fix onenand in u-boot, then needs to reimplement Nokia's 
> proprietary parser of that partition where is stored NVS and mac address 
> && make this support in upstream u-boot.

Are we talking about this?

https://github.com/community-ssu/libcal/blob/master/cal.c

That's ~1k loc for read-only. Getting NAND working looks harder than
porting this to me.

> So... I doubt it will be in any future.
> 
> + no men power

yeah :(

But with that reasoning you should just extract the NVS data
from NAND and put it in your rootfs.

> > > > Userspace application can add all those information to the DT
> > > > using a DT overlay. Also the u-boot could parse and add it at
> > > > some point in the future.
> > > 
> > > In case when wl1251 is statically linked into kernel image, it is
> > > loaded and initialized before even userspace applications starts.
> > > 
> > > So no... adding NVS data or MAC address into DT or DT overlay is
> > > not a solution.
> > 
> > Actually with data loaded from DT you *can* load data quite early in
> > the boot process, while your suggestions always require userspace.
> > So you argument against yourself?
> 
> You cannot add DTS in uboot (no support).

AFAIK that's supported:

http://www.denx.de/wiki/DULG/LinuxFDTBlob

"Note that U-Boot makes some automatic modifications to the blob
before passing it to the kernel - mainly adding and modifying
information that is learnt at run-time."

> And if you modify DTS on running kernel from userspace, then it is
> too late when wl1251 is statically linked into kernel image.
> 
> wl1251 will not wait until some userspace modify DTS and add data...

if (nvs info missing)
    return -EPROBE_DEFER;

> But request_firmware() in fallback mode *can* wait for userspace so 
> wl1251 can postpone its operation until mdev/udev/whatever starts 
> listening for events and push needed firmware data to kernel.

As you said one workaround is waiting. That's not a solution, that
only works with request_firmware().

> So no, there is no argument against... request_firmware() in fallback 
> mode with userspace helper is by design blocking and waiting for 
> userspace. But waiting for some change in DTS in kernel is just 
> nonsense.

I would just mark the wlan device with status = "disabled" and
enable it in the overlay together with adding the NVS & MAC info.

-- Sebastian

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

Re: wl1251 & mac address & calibration data

[Pali Rohár]

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

On Thursday 24 November 2016 19:11:39 Sebastian Reichel wrote:
> Hi,
> 
> On Thu, Nov 24, 2016 at 05:49:33PM +0100, Pali Rohár wrote:
> > On Thursday 24 November 2016 17:08:30 Sebastian Reichel wrote:
> > > On Thu, Nov 24, 2016 at 04:20:45PM +0100, Pali Rohár wrote:
> > > > On Thursday 24 November 2016 16:13:17 Sebastian Reichel wrote:
> > > > > On Thu, Nov 24, 2016 at 09:33:29AM +0100, Pali Rohár wrote:
> > > > > > On Thursday 24 November 2016 08:51:04 Pavel Machek wrote:
> > > > > > > > > "ifconfig hw ether XX" normally sets the address. I
> > > > > > > > > guess that's ioctl?
> > > > > > > > 
> > > > > > > > This sets temporary address and it is ioctl. IIRC same
> > > > > > > > as what ethtool uses. (ifconfig is already
> > > > > > > > deprecated).
> > > > > > > > 
> > > > > > > > > And I guess we should use similar mechanism for
> > > > > > > > > permanent address.
> > > > > > > > 
> > > > > > > > I'm not sure here... Above ioctl ↑↑↑ is for changing
> > > > > > > > temporary mac address. But here we do not want to
> > > > > > > > change permanent mac address. We want to tell kernel
> > > > > > > > driver current permanent mac address which is stored
> > > > > > > 
> > > > > > > Well... I'd still use similar mechanism :-).
> > > > > > 
> > > > > > Thats problematic, because in time when wlan0 interface is
> > > > > > registered into system and visible in ifconfig output it
> > > > > > already needs to have permanent mac address assigned.
> > > > > > 
> > > > > > We should assign permanent mac address before wlan0 of
> > > > > > wl1251 is registered into system.
> > > > > 
> > > > > You can just add the MAC address to the NVS data, which is
> > > > > also required for the device initialization.
> > > > 
> > > > NVS data file has fixed size, there is IIRC no place for it.
> > > > 
> > > > But one of my suggestion was to use another request_firmware
> > > > for MAC address. So this is similar to what you are proposing,
> > > > as NVS data are loaded by request_firmware too...
> > > 
> > > Just append it to NVS data and modify the size check accordingly?
> > 
> > First that would mean to have request_firmware() function which
> > will not use direct VFS access, but instead use userspace helper.
> 
> Permanent MAC is device specific init data, NVS is device specific
> init data => load together.
> 
> The userspace helper stuff is only needed to get the data from
> the NAND calibration area on the fly.

But it is needed... and currently request_firmware() prefer direct VFS 
access...

> > NVS data file with some default values (not suitable for usage) is
> > already present in linux-firmware and available in
> > /lib/firmware/...
> 
> You mentioned NVS data is fixed size, so this should be enough?
> 
> switch(loaded_size)
>     case IMAGE_SIZE + MAC_SIZE:
>         /* extract mac */
>         /* fallthrough */
>     case IMAGE_SIZE:
>         /* load NVS */
>         break;
>     default:
>         /* fail */
> }
> 
> > Also I'm not sure if such thing is allowed by license:
> > https://git.kernel.org/cgit/linux/kernel/git/firmware/linux-firmwar
> > e.git/tree/LICENCE.ti-connectivity
> 
> concating data is not a modification, otherwise you can't
> put the file in a filesystem.

Ok, if net maintainers agree.

> > > > > I wonder if those information could be put into DT. Iirc some
> > > > > network devices get their MAC address from DT. Maybe we can
> > > > > add all NVS info to DT? How much data is it?
> > > > 
> > > > Proprietary, signed and closed bootloader NOLO does not support
> > > > DT. So for booting you need to append DTS file to kernel
> > > > image.
> > > 
> > > Yeah, so NOLO without U-Boot will depend on userspace to fixup
> > > DT.
> > > 
> > > > U-Boot is optional and can be used as intermediate bootloader
> > > > between NOLO and kernel. But still it has problems with reading
> > > > from nand, so cannot read NVS data nor MAC address.
> > > 
> > > It may in the future?
> > 
> > I already tried that, but I failed. I was not able to access N900's
> > nand from u-boot. No idea where was problem...
> > 
> > And if somebody fix onenand in u-boot, then needs to reimplement
> > Nokia's proprietary parser of that partition where is stored NVS
> > and mac address && make this support in upstream u-boot.
> 
> Are we talking about this?
> 
> https://github.com/community-ssu/libcal/blob/master/cal.c
> 
> That's ~1k loc for read-only.

Yes. There is also read-only alternative library:
https://github.com/pali/0xFFFF/blob/master/src/cal.c

But still, this is proprietary format useful only for one device (or 
two) and I do not know if such thing could be accepted by u-boot 
developers...

> Getting NAND working looks harder than porting this to me.

Right.

> > So... I doubt it will be in any future.
> > 
> > + no men power
> 
> yeah :(
> 
> But with that reasoning you should just extract the NVS data
> from NAND and put it in your rootfs.

Not a clean solution. Some rootfs parts can used as readonly. And 
normally rootfs is flashed into nand, so I still will say that roofs 
(image) should not contain any device specific data.

> > > > > Userspace application can add all those information to the DT
> > > > > using a DT overlay. Also the u-boot could parse and add it at
> > > > > some point in the future.
> > > > 
> > > > In case when wl1251 is statically linked into kernel image, it
> > > > is loaded and initialized before even userspace applications
> > > > starts.
> > > > 
> > > > So no... adding NVS data or MAC address into DT or DT overlay
> > > > is not a solution.
> > > 
> > > Actually with data loaded from DT you *can* load data quite early
> > > in the boot process, while your suggestions always require
> > > userspace. So you argument against yourself?
> > 
> > You cannot add DTS in uboot (no support).
> 
> AFAIK that's supported:
> 
> http://www.denx.de/wiki/DULG/LinuxFDTBlob
> 
> "Note that U-Boot makes some automatic modifications to the blob
> before passing it to the kernel - mainly adding and modifying
> information that is learnt at run-time."

I mean we do not have support for due to n900 nand.

> > And if you modify DTS on running kernel from userspace, then it is
> > too late when wl1251 is statically linked into kernel image.
> > 
> > wl1251 will not wait until some userspace modify DTS and add
> > data...
> 
> if (nvs info missing)
>     return -EPROBE_DEFER;

Forever? Only N times? How long? Only N second?

Here we do not know if userspace is going to send data or not.

My guess is that such code will not be accepted into net/wireless 
subsystem or drivers by maintainers.

> > But request_firmware() in fallback mode *can* wait for userspace so
> > wl1251 can postpone its operation until mdev/udev/whatever starts
> > listening for events and push needed firmware data to kernel.
> 
> As you said one workaround is waiting. That's not a solution, that
> only works with request_firmware().

Yes, but request_firmware() uses interaction with userspace. Your 
proposed solution does not do any interaction with userspace that some 
process needs to fill missing data into DT.

And request_firmware() is already used for loading NVS data.

> > So no, there is no argument against... request_firmware() in
> > fallback mode with userspace helper is by design blocking and
> > waiting for userspace. But waiting for some change in DTS in
> > kernel is just nonsense.
> 
> I would just mark the wlan device with status = "disabled" and
> enable it in the overlay together with adding the NVS & MAC info.

So if you think that this solution make sense, we can wait what net 
wireless maintainers say about it...

For me it looks like that solution can be:

extending request_firmware() to use only userspace helper

and load mac address also via request_firmware() either by appending it 
into NVS data or via separate call

-- 
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 & mac address & calibration data

[Kalle Valo]

(Adding Luis because he has been working on request_firmware() lately)

Pali Rohár <pali.rohar@gmail.com> writes:

>> > So no, there is no argument against... request_firmware() in
>> > fallback mode with userspace helper is by design blocking and
>> > waiting for userspace. But waiting for some change in DTS in
>> > kernel is just nonsense.
>> 
>> I would just mark the wlan device with status = "disabled" and
>> enable it in the overlay together with adding the NVS & MAC info.
>
> So if you think that this solution make sense, we can wait what net 
> wireless maintainers say about it...
>
> For me it looks like that solution can be:
>
> extending request_firmware() to use only userspace helper

I haven't followed the discussion very closely but this is my preference
what drivers should do:

1) First the driver should do try to get the calibration data and mac
   address from the device tree.

2) If they are not in DT the driver should retrieve the calibration data
   with request_firmware(). BUT with an option for user space to
   implement that with a helper script so that the data can be created
   dynamically, which I believe openwrt does with ath10k calibration
   data right now.

> and load mac address also via request_firmware() either by appending it 
> into NVS data or via separate call

I'm not really fan of the idea providing permanent mac address through
request_firmware(). For example, how to handle multiple devices on the
same host, would there be a need for some kind of bus ids encoded to the
filename? And what about devices with multiple mac addresses?

I wish there would be a better way than request_firmware() to provide
the permanent mac addresses from user space (if device tree is not
available), I just don't know what that could be :) But if we would
start to use request_firmware() for this at least there should be a
wider concensus about that and it should be properly documented, just
like the device tree bindings.

-- 
Kalle Valo

Re: wl1251 & mac address & calibration data

[Pali Rohár]

On Thu Dec 15 09:18:44 2016 Kalle Valo <kvalo@codeaurora.org> wrote:
> (Adding Luis because he has been working on request_firmware() lately)
> 
> Pali Rohár <pali.rohar@gmail.com> writes:
> 
> > > > So no, there is no argument against... request_firmware() in
> > > > fallback mode with userspace helper is by design blocking and
> > > > waiting for userspace. But waiting for some change in DTS in
> > > > kernel is just nonsense.
> > > 
> > > I would just mark the wlan device with status = "disabled" and
> > > enable it in the overlay together with adding the NVS & MAC info.
> > 
> > So if you think that this solution make sense, we can wait what net 
> > wireless maintainers say about it...
> > 
> > For me it looks like that solution can be:
> > 
> > extending request_firmware() to use only userspace helper
> 
> I haven't followed the discussion very closely but this is my preference
> what drivers should do:
> 
> 1) First the driver should do try to get the calibration data and mac
>       address from the device tree.
> 

Ok, but there is no (dynamic, device specific) data in DTS for N900. So 1) is noop.

> 2) If they are not in DT the driver should retrieve the calibration data
>       with request_firmware(). BUT with an option for user space to
>       implement that with a helper script so that the data can be created
>       dynamically, which I believe openwrt does with ath10k calibration
>       data right now.

Currently there is flag for request_firmware() that it should fallback to user helper if direct VFS access not find needed firmware.

But this flag is not suitable as /lib/firmware already provides default (not device specific) calibration data.

So I would suggest to add another flag/function which will primary use user helper.

> > and load mac address also via request_firmware() either by appending
> > it   into NVS data or via separate call
> 
> I'm not really fan of the idea providing permanent mac address through
> request_firmware(). For example, how to handle multiple devices on the
> same host, would there be a need for some kind of bus ids encoded to the
> filename? And what about devices with multiple mac addresses?

For N900 there is only one wl1251 device. And... wl12xx is already using appended MAC address in calibration data read by request firmware. So reason why I prefer similar usage also for wl1251.

> I wish there would be a better way than request_firmware() to provide
> the permanent mac addresses from user space (if device tree is not
> available), I just don't know what that could be :) But if we would
> start to use request_firmware() for this at least there should be a
> wider concensus about that and it should be properly documented, just
> like the device tree bindings.
> 
> -- 
> Kalle Valo

I do not know about any other, so reason why I'm asking :-) and there are my proposed solutions. If you (or any other) came up with better we can discuss about it :-)

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

Re: wl1251 & mac address & calibration data

[Arend Van Spriel]

On 15-12-2016 16:33, Pali Rohár wrote:
> On Thu Dec 15 09:18:44 2016 Kalle Valo <kvalo@codeaurora.org> wrote:
>> (Adding Luis because he has been working on request_firmware() lately)
>>
>> Pali Rohár <pali.rohar@gmail.com> writes:
>>
>>>>> So no, there is no argument against... request_firmware() in
>>>>> fallback mode with userspace helper is by design blocking and
>>>>> waiting for userspace. But waiting for some change in DTS in
>>>>> kernel is just nonsense.
>>>>
>>>> I would just mark the wlan device with status = "disabled" and
>>>> enable it in the overlay together with adding the NVS & MAC info.
>>>
>>> So if you think that this solution make sense, we can wait what net 
>>> wireless maintainers say about it...
>>>
>>> For me it looks like that solution can be:
>>>
>>> extending request_firmware() to use only userspace helper
>>
>> I haven't followed the discussion very closely but this is my preference
>> what drivers should do:
>>
>> 1) First the driver should do try to get the calibration data and mac
>>        address from the device tree.
>>
> 
> Ok, but there is no (dynamic, device specific) data in DTS for N900. So 1) is noop.

Uhm. What do you mean? You can propose a patch to the DT bindings [1] to
get it in there and create your N900 DTB or am I missing something here.
Are there hardware restrictions that do not allow you to boot with your
own DTB.

>> 2) If they are not in DT the driver should retrieve the calibration data
>>        with request_firmware(). BUT with an option for user space to
>>        implement that with a helper script so that the data can be created
>>        dynamically, which I believe openwrt does with ath10k calibration
>>        data right now.
> 
> Currently there is flag for request_firmware() that it should fallback to user helper if direct VFS access not find needed firmware.
> 
> But this flag is not suitable as /lib/firmware already provides default (not device specific) calibration data.
> 
> So I would suggest to add another flag/function which will primary use user helper.

I recall Luis saying that user-mode helper (fallback) should be
discouraged, because there is no assurance that there is a user-mode
helper so you might just be pissing in the wind. The idea was to have a
dedicated API call that explicitly does the request towards user-space.

By the way, are we talking here about wl1251 device or driver as you
also mentioned wl12xx? I did not read the entire thread.

Regards,
Arend

Re: wl1251 & mac address & calibration data

[Luis R. Rodriguez]

On Thu, Dec 15, 2016 at 2:12 PM, Arend Van Spriel
<arend.vanspriel@broadcom.com> wrote:
> On 15-12-2016 16:33, Pali Rohár wrote:
>> On Thu Dec 15 09:18:44 2016 Kalle Valo <kvalo@codeaurora.org> wrote:
>>> (Adding Luis because he has been working on request_firmware() lately)
>>>
>>> Pali Rohár <pali.rohar@gmail.com> writes:
>>>
>>>>>> So no, there is no argument against... request_firmware() in
>>>>>> fallback mode with userspace helper is by design blocking and
>>>>>> waiting for userspace. But waiting for some change in DTS in
>>>>>> kernel is just nonsense.
>>>>>
>>>>> I would just mark the wlan device with status = "disabled" and
>>>>> enable it in the overlay together with adding the NVS & MAC info.
>>>>
>>>> So if you think that this solution make sense, we can wait what net
>>>> wireless maintainers say about it...
>>>>
>>>> For me it looks like that solution can be:
>>>>
>>>> extending request_firmware() to use only userspace helper
>>>
>>> I haven't followed the discussion very closely but this is my preference
>>> what drivers should do:
>>>
>>> 1) First the driver should do try to get the calibration data and mac
>>>        address from the device tree.
>>>
>>
>> Ok, but there is no (dynamic, device specific) data in DTS for N900. So 1) is noop.
>
> Uhm. What do you mean? You can propose a patch to the DT bindings [1] to
> get it in there and create your N900 DTB or am I missing something here.
> Are there hardware restrictions that do not allow you to boot with your
> own DTB.
>
>>> 2) If they are not in DT the driver should retrieve the calibration data
>>>        with request_firmware(). BUT with an option for user space to
>>>        implement that with a helper script so that the data can be created
>>>        dynamically, which I believe openwrt does with ath10k calibration
>>>        data right now.
>>
>> Currently there is flag for request_firmware() that it should fallback to user helper if direct VFS access not find needed firmware.
>>
>> But this flag is not suitable as /lib/firmware already provides default (not device specific) calibration data.
>>
>> So I would suggest to add another flag/function which will primary use user helper.
> I recall Luis saying that user-mode helper (fallback) should be
> discouraged, because there is no assurance that there is a user-mode
> helper so you might just be pissing in the wind.

There's tons of issues with the current status quo of the so called
"firmware usermode helper". To start off with its a complete misnomer,
the kernel's usermode helper infrastructure is implemented in
lib/kmod.c and it provides an API to help call out to userspace some
helper for you. That's the real kernel usermode helper. In so far as
the firmware_class.c driver is concerned -- it only makes use of the
kernel user mode helper as an option if you have
CONFIG_UEVENT_HELPER_PATH set to help address kobject uevents. Most
distributions do not use this, and back in the day systemd udev, and
prior to that hotplug used to process firmware kobject uevents.
systemd udev is long gone. Gone. This kobject uevents really are a
"fallback mechanism" for firmware only -- if cached firmware, built-in
firmware, or direct filesystem lookup fails. These each are their own
beast. Finally kobject uevents are only one of the fallback
mechanisms, "custom fallback mechanisms" are the other option and its
what you and others seem to describe to want for these sorts of custom
things.

There are issues with the existing request_firmware() API in that
everyone and their mother keeps extending it with stupid small API
extensions to do yet-another-tweak, and then having to go and change
tons of drivers. Or a new API call added for just one custom knob.
Naturally this is all plain dumb, so yet-another-API call or new
argument is not going to help us. We don't have "flags" persi in this
API, that was one issue with the design of the API, but there are much
more. The entire change in mechanism of "firmware fallback mechanism"
depending on which kernel config options you have is another. Its a
big web of gum put together. All this needs to end.

I've recently proposed a new patch to first help with understanding
each of the individual items I've listed above with as much new
information as is possible. I myself need to re-read it to just keep
tabs on what the hell is going on at times. After this I have a new
API proposal which I've been working on for a long time now which adds
an extensible API with only 2 calls: sync, async, and lets us modify
attributes through a parameters struct. This is what we should use in
the future for further extensions.

For the new API a solution for "fallback mechanisms" should be clean
though and I am looking to stay as far as possible from the existing
mess. A solution to help both the old API and new API is possible for
the "fallback mechanism" though -- but for that I can only refer you
at this point to some of Daniel Wagner and Tom Gunderson's firmwared
deamon prospect. It should help pave the way for a clean solution and
help address other stupid issues.

I'll note -- the "custom fallback mechanism", part of the old API is
just a terrible idea for many reasons. I've documented issues in my
documentation revamp, I suggest to read that, refer to this thread:

https://lkml.kernel.org/r/20161213030828.17820-4-mcgrof@kernel.org

> The idea was to have a
> dedicated API call that explicitly does the request towards user-space.

In so far as this driver example that was mentioned in this thread my
recommendation is to use the default existing MAC address /
calibration data on /lib/firmware/ and then use another request to
help override for the custom thing. The only issue of course is that
the timeout for the custom thing is 60 seconds, but if your platforms
are controlled -- just reduce this to 1 second or so given that udev
is long gone and it would seem you'd only use a custom fw request to
depend on. You could also wrap this thing into a kconfig option for
now, and have the filename specified, if empty then no custom request
is sent. If set then you have it request it.

Please note the other patches in my series for the custom fallback
though. We have only 2 users upstream -- and from recent discussions
it would seem it might be possible to address what you need with
firmwared in a clean way without this custom old fallback crap thing.
Johannes at least has a similar requirement and is convinced a
"custom" thing can be done without even adding an extra custom kobject
uevent attribute as from what I recall he hinted, drivers have no
business to be involved in this. If you do end up using the custom
fallback mechanism be sure to add super crystal clear documentation
for it (see my other patches for the declarer for this documentation).
Since we have only 2 drivers using this custom thing its hard to get
folks to commit to writing the docs, write it now and be sure you
think of the future as well.

Oh also the "custom firmware fallback" was also broken on recent
kernels for many kernel revisions, it just did not work until recently
a fix which went in, so your users wills need this fix cherry picked.
Hey I tell you, the custom fw thing was a terrible incarnation. Only 2
drivers use it. There are good reasons to not like it (be sure to read
the suspend issue I documented).

> By the way, are we talking here about wl1251 device or driver as you
> also mentioned wl12xx? I did not read the entire thread.

  Luis

Re: wl1251 & mac address & calibration data

[Daniel Wagner]

On 12/16/2016 03:03 AM, Luis R. Rodriguez wrote:
> For the new API a solution for "fallback mechanisms" should be clean
> though and I am looking to stay as far as possible from the existing
> mess. A solution to help both the old API and new API is possible for
> the "fallback mechanism" though -- but for that I can only refer you
> at this point to some of Daniel Wagner and Tom Gunderson's firmwared
> deamon prospect. It should help pave the way for a clean solution and
> help address other stupid issues.

The firmwared project is hosted here

https://github.com/teg/firmwared

As Luis pointed out, firmwared relies on FW_LOADER_USER_HELPER_FALLBACK, 
which is not enabled by default. I don't see any reason why firmwared 
should not also support loading calibration data. If we find a sound way 
to do this.

As you can see from the commit history it is a pretty young project and 
more ore less reanimation of the old udev firmware loader feature.  We 
are getting int into shape, adding integration tests etc.

The main motivation for this project is the get movement back in stuck 
discussion on the firmware loader API. Luis was very busy writing up all 
the details on the current situation and purely from the amount of 
documentation need to describe the API you can tell something is awry.

Thanks,
Daniel

Re: wl1251 & mac address & calibration data

[Pali Rohár]

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

On Friday 16 December 2016 08:25:44 Daniel Wagner wrote:
> On 12/16/2016 03:03 AM, Luis R. Rodriguez wrote:
> > For the new API a solution for "fallback mechanisms" should be
> > clean though and I am looking to stay as far as possible from the
> > existing mess. A solution to help both the old API and new API is
> > possible for the "fallback mechanism" though -- but for that I can
> > only refer you at this point to some of Daniel Wagner and Tom
> > Gunderson's firmwared deamon prospect. It should help pave the way
> > for a clean solution and help address other stupid issues.
> 
> The firmwared project is hosted here
> 
> https://github.com/teg/firmwared
> 
> As Luis pointed out, firmwared relies on
> FW_LOADER_USER_HELPER_FALLBACK, which is not enabled by default.

I know. But it does not mean that I cannot enable this option at kernel 
compile time.

Bigger problem is that currently request_firmware() first try to load 
firmware directly from VFS and after that (if fails) fallback to user 
helper.

So I would need to extend kernel firmware code with new function (or 
flag) to not use VFS and try only user mode helper.

> I
> don't see any reason why firmwared should not also support loading
> calibration data. If we find a sound way to do this.

It can, but why should I use another daemon for firmware loading as 
non-systemd version of udev (and eudev fork) support firmware loading? 
I think I stay with udev/eudev.

> As you can see from the commit history it is a pretty young project
> and more ore less reanimation of the old udev firmware loader
> feature.  We are getting int into shape, adding integration tests
> etc.
> 
> The main motivation for this project is the get movement back in
> stuck discussion on the firmware loader API. Luis was very busy
> writing up all the details on the current situation and purely from
> the amount of documentation need to describe the API you can tell
> something is awry.
> 
> Thanks,
> Daniel

-- 
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 & mac address & calibration data

[Arend Van Spriel]

On 16-12-2016 11:40, Pali Rohár wrote:
> On Friday 16 December 2016 08:25:44 Daniel Wagner wrote:
>> On 12/16/2016 03:03 AM, Luis R. Rodriguez wrote:
>>> For the new API a solution for "fallback mechanisms" should be
>>> clean though and I am looking to stay as far as possible from the
>>> existing mess. A solution to help both the old API and new API is
>>> possible for the "fallback mechanism" though -- but for that I can
>>> only refer you at this point to some of Daniel Wagner and Tom
>>> Gunderson's firmwared deamon prospect. It should help pave the way
>>> for a clean solution and help address other stupid issues.
>>
>> The firmwared project is hosted here
>>
>> https://github.com/teg/firmwared
>>
>> As Luis pointed out, firmwared relies on
>> FW_LOADER_USER_HELPER_FALLBACK, which is not enabled by default.
> 
> I know. But it does not mean that I cannot enable this option at kernel 
> compile time.
> 
> Bigger problem is that currently request_firmware() first try to load 
> firmware directly from VFS and after that (if fails) fallback to user 
> helper.
> 
> So I would need to extend kernel firmware code with new function (or 
> flag) to not use VFS and try only user mode helper.

Why do you need the user-mode helper anyway. This is all static data,
right? So why not cook up a firmware file in user-space once and put it
in /lib/firmware for the driver to request directly. Seems a bit
overkill to have a {e,}udev or whatever daemon running if the result is
always the same. Just my 2 cents.

Regards,
Arend

Re: wl1251 & mac address & calibration data

[Pali Rohár]

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

On Sunday 18 December 2016 11:49:53 Arend Van Spriel wrote:
> On 16-12-2016 11:40, Pali Rohár wrote:
> > On Friday 16 December 2016 08:25:44 Daniel Wagner wrote:
> >> On 12/16/2016 03:03 AM, Luis R. Rodriguez wrote:
> >>> For the new API a solution for "fallback mechanisms" should be
> >>> clean though and I am looking to stay as far as possible from the
> >>> existing mess. A solution to help both the old API and new API is
> >>> possible for the "fallback mechanism" though -- but for that I
> >>> can only refer you at this point to some of Daniel Wagner and
> >>> Tom Gunderson's firmwared deamon prospect. It should help pave
> >>> the way for a clean solution and help address other stupid
> >>> issues.
> >> 
> >> The firmwared project is hosted here
> >> 
> >> https://github.com/teg/firmwared
> >> 
> >> As Luis pointed out, firmwared relies on
> >> FW_LOADER_USER_HELPER_FALLBACK, which is not enabled by default.
> > 
> > I know. But it does not mean that I cannot enable this option at
> > kernel compile time.
> > 
> > Bigger problem is that currently request_firmware() first try to
> > load firmware directly from VFS and after that (if fails) fallback
> > to user helper.
> > 
> > So I would need to extend kernel firmware code with new function
> > (or flag) to not use VFS and try only user mode helper.
> 
> Why do you need the user-mode helper anyway. This is all static data,
> right?

Those are static data, but device specific!

> So why not cook up a firmware file in user-space once and put
> it in /lib/firmware for the driver to request directly.

1. Violates FHS

2. Does not work for readonly /, readonly /lib, readonly /lib/firmware

3. Backup & restore of rootfs between same devices does not work (as 
rootfs now contains device specific data).

4. Sharing one rootfs (either via nfs or other technology) does not work 
for more devices (even in state when rootfs is used only by one device 
at one time).

And it is common that N900 developers have rootfs in laptop and via usb 
(cdc_ether) exports it over nfs to N900 device and boot system. It 
basically break booting from one nfs-exported rootfs, as that export 
become model specific...

> Seems a bit
> overkill to have a {e,}udev or whatever daemon running if the result
> is always the same. Just my 2 cents.

No it is not. It will break couple of other things in Linux and device 
and model specific calibration data should not be in /lib/firmware! That 
directory is used for firmware files, not calibration.

-- 
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 & mac address & calibration data

[Arend Van Spriel]

On 18-12-2016 12:04, Pali Rohár wrote:
> On Sunday 18 December 2016 11:49:53 Arend Van Spriel wrote:
>> On 16-12-2016 11:40, Pali Rohár wrote:
>>> On Friday 16 December 2016 08:25:44 Daniel Wagner wrote:
>>>> On 12/16/2016 03:03 AM, Luis R. Rodriguez wrote:
>>>>> For the new API a solution for "fallback mechanisms" should be
>>>>> clean though and I am looking to stay as far as possible from the
>>>>> existing mess. A solution to help both the old API and new API is
>>>>> possible for the "fallback mechanism" though -- but for that I
>>>>> can only refer you at this point to some of Daniel Wagner and
>>>>> Tom Gunderson's firmwared deamon prospect. It should help pave
>>>>> the way for a clean solution and help address other stupid
>>>>> issues.
>>>>
>>>> The firmwared project is hosted here
>>>>
>>>> https://github.com/teg/firmwared
>>>>
>>>> As Luis pointed out, firmwared relies on
>>>> FW_LOADER_USER_HELPER_FALLBACK, which is not enabled by default.
>>>
>>> I know. But it does not mean that I cannot enable this option at
>>> kernel compile time.
>>>
>>> Bigger problem is that currently request_firmware() first try to
>>> load firmware directly from VFS and after that (if fails) fallback
>>> to user helper.
>>>
>>> So I would need to extend kernel firmware code with new function
>>> (or flag) to not use VFS and try only user mode helper.
>>
>> Why do you need the user-mode helper anyway. This is all static data,
>> right?
> 
> Those are static data, but device specific!

So what?

>> So why not cook up a firmware file in user-space once and put
>> it in /lib/firmware for the driver to request directly.
> 
> 1. Violates FHS

How?

> 2. Does not work for readonly /, readonly /lib, readonly /lib/firmware

Que?

> 3. Backup & restore of rootfs between same devices does not work (as 
> rootfs now contains device specific data).

True.

> 4. Sharing one rootfs (either via nfs or other technology) does not work 
> for more devices (even in state when rootfs is used only by one device 
> at one time).

Indeed.

> And it is common that N900 developers have rootfs in laptop and via usb 
> (cdc_ether) exports it over nfs to N900 device and boot system. It 
> basically break booting from one nfs-exported rootfs, as that export 
> become model specific...

These are all you choices and more a logistic issue. If your take is
that udev is the way to solve those, fine by me.

>> Seems a bit
>> overkill to have a {e,}udev or whatever daemon running if the result
>> is always the same. Just my 2 cents.
> 
> No it is not. It will break couple of other things in Linux and device 

Now I am curious. What "couple of other things" will be broken.

> and model specific calibration data should not be in /lib/firmware! That 
> directory is used for firmware files, not calibration.

What is "firmware"? Really. These are binary blobs required to make the
device work. And guess what, your device needs calibration data. Why
make the distinction.

Regards,
Arend

Re: wl1251 & mac address & calibration data

[Pali Rohár]

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

On Sunday 18 December 2016 12:54:00 Arend Van Spriel wrote:
> On 18-12-2016 12:04, Pali Rohár wrote:
> > On Sunday 18 December 2016 11:49:53 Arend Van Spriel wrote:
> >> On 16-12-2016 11:40, Pali Rohár wrote:
> >>> On Friday 16 December 2016 08:25:44 Daniel Wagner wrote:
> >>>> On 12/16/2016 03:03 AM, Luis R. Rodriguez wrote:
> >>>>> For the new API a solution for "fallback mechanisms" should be
> >>>>> clean though and I am looking to stay as far as possible from
> >>>>> the existing mess. A solution to help both the old API and new
> >>>>> API is possible for the "fallback mechanism" though -- but for
> >>>>> that I can only refer you at this point to some of Daniel
> >>>>> Wagner and Tom Gunderson's firmwared deamon prospect. It
> >>>>> should help pave the way for a clean solution and help address
> >>>>> other stupid issues.
> >>>> 
> >>>> The firmwared project is hosted here
> >>>> 
> >>>> https://github.com/teg/firmwared
> >>>> 
> >>>> As Luis pointed out, firmwared relies on
> >>>> FW_LOADER_USER_HELPER_FALLBACK, which is not enabled by default.
> >>> 
> >>> I know. But it does not mean that I cannot enable this option at
> >>> kernel compile time.
> >>> 
> >>> Bigger problem is that currently request_firmware() first try to
> >>> load firmware directly from VFS and after that (if fails)
> >>> fallback to user helper.
> >>> 
> >>> So I would need to extend kernel firmware code with new function
> >>> (or flag) to not use VFS and try only user mode helper.
> >> 
> >> Why do you need the user-mode helper anyway. This is all static
> >> data, right?
> > 
> > Those are static data, but device specific!
> 
> So what?
> 
> >> So why not cook up a firmware file in user-space once and put
> >> it in /lib/firmware for the driver to request directly.
> > 
> > 1. Violates FHS
> 
> How?
> 
> > 2. Does not work for readonly /, readonly /lib, readonly
> > /lib/firmware
> 
> Que?
> 
> > 3. Backup & restore of rootfs between same devices does not work
> > (as rootfs now contains device specific data).
> 
> True.
> 
> > 4. Sharing one rootfs (either via nfs or other technology) does not
> > work for more devices (even in state when rootfs is used only by
> > one device at one time).
> 
> Indeed.
> 
> > And it is common that N900 developers have rootfs in laptop and via
> > usb (cdc_ether) exports it over nfs to N900 device and boot
> > system. It basically break booting from one nfs-exported rootfs,
> > as that export become model specific...
> 
> These are all you choices and more a logistic issue. If your take is
> that udev is the way to solve those, fine by me.
> 
> >> Seems a bit
> >> overkill to have a {e,}udev or whatever daemon running if the
> >> result is always the same. Just my 2 cents.
> > 
> > No it is not. It will break couple of other things in Linux and
> > device
> 
> Now I am curious. What "couple of other things" will be broken.
> 
> > and model specific calibration data should not be in /lib/firmware!
> > That directory is used for firmware files, not calibration.
> 
> What is "firmware"? Really. These are binary blobs required to make
> the device work. And guess what, your device needs calibration data.
> Why make the distinction.
> 
> Regards,
> Arend

File wl1251-nvs.bin is provided by linux-firmware package and contains 
default data which should be overriden by model specific calibrated 
data.

But overwriting that one file is not possible as it next update of 
linux-firmware package will overwrite it back. It break any normal usage 
of package management.

Also it is ridiculously broken by design if some "boot" files needs to 
be overwritten to initialize hardware properly. To not break booting you 
need to overwrite that file before first boot. But without booting 
device you cannot read calibration data. So some hack with autoreboot 
after boot is needed. And how to detect that we have real overwritten 
calibration data and not default one from linux-firmware? Any heuristic 
or checks will be broken here. And no, nothing like you need to reboot 
your device now (and similar concept) from windows world is not 
accepted.

"firmware" is one for chip. Any N900 device with wl1251 chip needs 
exactly same firmware "wl1251-fw.bin". But every N900 needs different 
calibration data which is not firmware.

-- 
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 & mac address & calibration data

[Arend Van Spriel]

On 18-12-2016 13:09, Pali Rohár wrote:
> On Sunday 18 December 2016 12:54:00 Arend Van Spriel wrote:
>> On 18-12-2016 12:04, Pali Rohár wrote:
>>> On Sunday 18 December 2016 11:49:53 Arend Van Spriel wrote:
>>>> On 16-12-2016 11:40, Pali Rohár wrote:
>>>>> On Friday 16 December 2016 08:25:44 Daniel Wagner wrote:
>>>>>> On 12/16/2016 03:03 AM, Luis R. Rodriguez wrote:
>>>>>>> For the new API a solution for "fallback mechanisms" should be
>>>>>>> clean though and I am looking to stay as far as possible from
>>>>>>> the existing mess. A solution to help both the old API and new
>>>>>>> API is possible for the "fallback mechanism" though -- but for
>>>>>>> that I can only refer you at this point to some of Daniel
>>>>>>> Wagner and Tom Gunderson's firmwared deamon prospect. It
>>>>>>> should help pave the way for a clean solution and help address
>>>>>>> other stupid issues.
>>>>>>
>>>>>> The firmwared project is hosted here
>>>>>>
>>>>>> https://github.com/teg/firmwared
>>>>>>
>>>>>> As Luis pointed out, firmwared relies on
>>>>>> FW_LOADER_USER_HELPER_FALLBACK, which is not enabled by default.
>>>>>
>>>>> I know. But it does not mean that I cannot enable this option at
>>>>> kernel compile time.
>>>>>
>>>>> Bigger problem is that currently request_firmware() first try to
>>>>> load firmware directly from VFS and after that (if fails)
>>>>> fallback to user helper.
>>>>>
>>>>> So I would need to extend kernel firmware code with new function
>>>>> (or flag) to not use VFS and try only user mode helper.
>>>>
>>>> Why do you need the user-mode helper anyway. This is all static
>>>> data, right?
>>>
>>> Those are static data, but device specific!
>>
>> So what?
>>
>>>> So why not cook up a firmware file in user-space once and put
>>>> it in /lib/firmware for the driver to request directly.
>>>
>>> 1. Violates FHS
>>
>> How?
>>
>>> 2. Does not work for readonly /, readonly /lib, readonly
>>> /lib/firmware
>>
>> Que?
>>
>>> 3. Backup & restore of rootfs between same devices does not work
>>> (as rootfs now contains device specific data).
>>
>> True.
>>
>>> 4. Sharing one rootfs (either via nfs or other technology) does not
>>> work for more devices (even in state when rootfs is used only by
>>> one device at one time).
>>
>> Indeed.
>>
>>> And it is common that N900 developers have rootfs in laptop and via
>>> usb (cdc_ether) exports it over nfs to N900 device and boot
>>> system. It basically break booting from one nfs-exported rootfs,
>>> as that export become model specific...
>>
>> These are all you choices and more a logistic issue. If your take is
>> that udev is the way to solve those, fine by me.
>>
>>>> Seems a bit
>>>> overkill to have a {e,}udev or whatever daemon running if the
>>>> result is always the same. Just my 2 cents.
>>>
>>> No it is not. It will break couple of other things in Linux and
>>> device
>>
>> Now I am curious. What "couple of other things" will be broken.
>>
>>> and model specific calibration data should not be in /lib/firmware!
>>> That directory is used for firmware files, not calibration.
>>
>> What is "firmware"? Really. These are binary blobs required to make
>> the device work. And guess what, your device needs calibration data.
>> Why make the distinction.
>>
>> Regards,
>> Arend
> 
> File wl1251-nvs.bin is provided by linux-firmware package and contains 
> default data which should be overriden by model specific calibrated 
> data.

Ah. Someone thought it was a good idea to provide the "one ring to rule
them all". Nice.

> But overwriting that one file is not possible as it next update of 
> linux-firmware package will overwrite it back. It break any normal usage 
> of package management.
> 
> Also it is ridiculously broken by design if some "boot" files needs to 
> be overwritten to initialize hardware properly. To not break booting you 
> need to overwrite that file before first boot. But without booting 
> device you cannot read calibration data. So some hack with autoreboot 
> after boot is needed. And how to detect that we have real overwritten 
> calibration data and not default one from linux-firmware? Any heuristic 
> or checks will be broken here. And no, nothing like you need to reboot 
> your device now (and similar concept) from windows world is not 
> accepted.

Well. After reading and creating calibration data you could just rebind
the driver to the device to have it probed again. But yeah, the default
one from linux-firmware should never have been there in the first place.

> "firmware" is one for chip. Any N900 device with wl1251 chip needs 
> exactly same firmware "wl1251-fw.bin". But every N900 needs different 
> calibration data which is not firmware.

Ok. This is exactly why Luis is giving the new API different name just
calling it "data".

Regards,
Arend

Re: wl1251 & mac address & calibration data

[Kalle Valo]

Arend Van Spriel <arend.vanspriel@broadcom.com> writes:

> On 18-12-2016 13:09, Pali Rohár wrote:
>
>> File wl1251-nvs.bin is provided by linux-firmware package and contains 
>> default data which should be overriden by model specific calibrated 
>> data.
>
> Ah. Someone thought it was a good idea to provide the "one ring to rule
> them all". Nice.

Yes, that was a bad idea. wl1251-nvs.bin in linux-firmware.git should be
renamed to wl1251-nvs.bin.example, or something like that, as it should
be only installed to a real system only if there's no real calibration
data available (only for developers to use, not real users).

>> But overwriting that one file is not possible as it next update of 
>> linux-firmware package will overwrite it back. It break any normal usage 
>> of package management.
>> 
>> Also it is ridiculously broken by design if some "boot" files needs to 
>> be overwritten to initialize hardware properly. To not break booting you 
>> need to overwrite that file before first boot. But without booting 
>> device you cannot read calibration data. So some hack with autoreboot 
>> after boot is needed.

Providing the calibration data via Device Tree is the proper way to
solve this. Yes yes, I know N900 doesn't support it but that's a
deficiency in N900, not Linux.

>> And how to detect that we have real overwritten calibration data and
>> not default one from linux-firmware? Any heuristic or checks will be
>> broken here. And no, nothing like you need to reboot your device now
>> (and similar concept) from windows world is not accepted.
>
> Well. After reading and creating calibration data you could just rebind
> the driver to the device to have it probed again.

Or load wl1251 as a module and make sure calibration data is installed
before the module is loaded. LEDE does that with ath10k:

https://git.lede-project.org/?p=source.git;a=blob;f=target/linux/ar71xx/base-files/etc/hotplug.d/firmware/11-ath10k-caldata;h=97875bd79a579a0010da3f60324b6ec966fe9c6a;hb=HEAD

> But yeah, the default one from linux-firmware should never have been
> there in the first place.

Agreed.

-- 
Kalle Valo

Re: wl1251 & mac address & calibration data

[Tony Lindgren]

* Kalle Valo <kvalo@codeaurora.org> [161220 03:47]:
> Arend Van Spriel <arend.vanspriel@broadcom.com> writes:
> 
> > On 18-12-2016 13:09, Pali Rohár wrote:
> >
> >> File wl1251-nvs.bin is provided by linux-firmware package and contains 
> >> default data which should be overriden by model specific calibrated 
> >> data.
> >
> > Ah. Someone thought it was a good idea to provide the "one ring to rule
> > them all". Nice.
> 
> Yes, that was a bad idea. wl1251-nvs.bin in linux-firmware.git should be
> renamed to wl1251-nvs.bin.example, or something like that, as it should
> be only installed to a real system only if there's no real calibration
> data available (only for developers to use, not real users).

Makes sense to me. Note that with the recent changes to wlcore, we can
now easily provide board specific calibration firmware simply by adding a
new compatible value. So for n900, we could have something like
compatible = "ti,wl1251-n900" and have it point to n900 specific calibration
file wl1251-nvs-n900.bin. Of course this won't help with the mac address,
or any of the device specific data..

That is assuming the calibration values are the same for each similar
device and don't have to be generated for each device. And naturally wl1251
needs simlar changes done to make use of devices specific calibration files.

Regards,

Tony

Re: wl1251 & mac address & calibration data

[Pali Rohár]

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

On Tuesday 20 December 2016 17:56:58 Tony Lindgren wrote:
> * Kalle Valo <kvalo@codeaurora.org> [161220 03:47]:
> > Arend Van Spriel <arend.vanspriel@broadcom.com> writes:
> > > On 18-12-2016 13:09, Pali Rohár wrote:
> > >> File wl1251-nvs.bin is provided by linux-firmware package and
> > >> contains default data which should be overriden by model
> > >> specific calibrated data.
> > > 
> > > Ah. Someone thought it was a good idea to provide the "one ring
> > > to rule them all". Nice.
> > 
> > Yes, that was a bad idea. wl1251-nvs.bin in linux-firmware.git
> > should be renamed to wl1251-nvs.bin.example, or something like
> > that, as it should be only installed to a real system only if
> > there's no real calibration data available (only for developers to
> > use, not real users).
> 
> Makes sense to me. Note that with the recent changes to wlcore, we
> can now easily provide board specific calibration firmware simply by
> adding a new compatible value. So for n900, we could have something
> like compatible = "ti,wl1251-n900" and have it point to n900
> specific calibration file wl1251-nvs-n900.bin. Of course this won't
> help with the mac address, or any of the device specific data..
> 
> That is assuming the calibration values are the same for each similar
> device and don't have to be generated for each device. And naturally
> wl1251 needs simlar changes done to make use of devices specific
> calibration files.
> 
> Regards,
> 
> Tony

As wrote in another thread "wl1251 NVS calibration data format" 
calibration data for wl1251 (wl1251-nvs.bin) contains also MAC address, 
which kernel sends to wl1251 chip. Kernel just do not use it.

So... my idea now is:

1) extend request_firmware function family with ability to use userspace 
helper first and fallback to VFS

2) teach wl1251.ko to parse MAC address from wl1251-nvs.bin and use it 
(in case it is not empty or 00:00:20:07:03:09 which is in that example 
linux-firmware package)

3) write Nokia n900 specific userspace helper for providing data when 
kernel requests wl1251-nvs.bin. So userspace helper reads MAC address 
and calibration data from CAL, place MAC address into calibration data 
and send put it into kernel.

Are you OK with this idea?

-- 
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 & mac address & calibration data

[Kalle Valo]

Tony Lindgren <tony@atomide.com> writes:

> * Kalle Valo <kvalo@codeaurora.org> [161220 03:47]:
>> Arend Van Spriel <arend.vanspriel@broadcom.com> writes:
>> 
>> > On 18-12-2016 13:09, Pali Rohár wrote:
>> >
>> >> File wl1251-nvs.bin is provided by linux-firmware package and contains 
>> >> default data which should be overriden by model specific calibrated 
>> >> data.
>> >
>> > Ah. Someone thought it was a good idea to provide the "one ring to rule
>> > them all". Nice.
>> 
>> Yes, that was a bad idea. wl1251-nvs.bin in linux-firmware.git should be
>> renamed to wl1251-nvs.bin.example, or something like that, as it should
>> be only installed to a real system only if there's no real calibration
>> data available (only for developers to use, not real users).
>
> Makes sense to me. Note that with the recent changes to wlcore, we can
> now easily provide board specific calibration firmware simply by adding a
> new compatible value. So for n900, we could have something like
> compatible = "ti,wl1251-n900" and have it point to n900 specific calibration
> file wl1251-nvs-n900.bin. Of course this won't help with the mac address,
> or any of the device specific data..
>
> That is assuming the calibration values are the same for each similar
> device and don't have to be generated for each device. And naturally wl1251
> needs simlar changes done to make use of devices specific calibration files.

No, these are unique per each sold device. Every N900 was calibrated at
the factory and they all have different calibration data which is stored
to the flash. So when N900 boots (and in _every_ boot) it has to load
the calibration data from the flash and provide it to the wl1251 driver
somehow.

-- 
Kalle Valo

Re: wl1251 & mac address & calibration data

[Tony Lindgren]

* Kalle Valo <kvalo@codeaurora.org> [161220 09:12]:
> Tony Lindgren <tony@atomide.com> writes:
> 
> > * Kalle Valo <kvalo@codeaurora.org> [161220 03:47]:
> >> Arend Van Spriel <arend.vanspriel@broadcom.com> writes:
> >> 
> >> > On 18-12-2016 13:09, Pali Rohár wrote:
> >> >
> >> >> File wl1251-nvs.bin is provided by linux-firmware package and contains 
> >> >> default data which should be overriden by model specific calibrated 
> >> >> data.
> >> >
> >> > Ah. Someone thought it was a good idea to provide the "one ring to rule
> >> > them all". Nice.
> >> 
> >> Yes, that was a bad idea. wl1251-nvs.bin in linux-firmware.git should be
> >> renamed to wl1251-nvs.bin.example, or something like that, as it should
> >> be only installed to a real system only if there's no real calibration
> >> data available (only for developers to use, not real users).
> >
> > Makes sense to me. Note that with the recent changes to wlcore, we can
> > now easily provide board specific calibration firmware simply by adding a
> > new compatible value. So for n900, we could have something like
> > compatible = "ti,wl1251-n900" and have it point to n900 specific calibration
> > file wl1251-nvs-n900.bin. Of course this won't help with the mac address,
> > or any of the device specific data..
> >
> > That is assuming the calibration values are the same for each similar
> > device and don't have to be generated for each device. And naturally wl1251
> > needs simlar changes done to make use of devices specific calibration files.
> 
> No, these are unique per each sold device. Every N900 was calibrated at
> the factory and they all have different calibration data which is stored
> to the flash. So when N900 boots (and in _every_ boot) it has to load
> the calibration data from the flash and provide it to the wl1251 driver
> somehow.

Urgh, OK. So much for that idea then.

Thanks,

Tony

Re: wl1251 & mac address & calibration data

[Pavel Machek]

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

Hi!

> >> But overwriting that one file is not possible as it next update of 
> >> linux-firmware package will overwrite it back. It break any normal usage 
> >> of package management.
> >> 
> >> Also it is ridiculously broken by design if some "boot" files needs to 
> >> be overwritten to initialize hardware properly. To not break booting you 
> >> need to overwrite that file before first boot. But without booting 
> >> device you cannot read calibration data. So some hack with autoreboot 
> >> after boot is needed.
> 
> Providing the calibration data via Device Tree is the proper way to
> solve this. Yes yes, I know N900 doesn't support it but that's a
> deficiency in N900, not Linux.

Linux has to work with whatever hardware provides. You may not like
N900 design, but we have to support it, anyway.

								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: wl1251 & mac address & calibration data

[Pali Rohár]

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

On Friday 16 December 2016 03:03:19 Luis R. Rodriguez wrote:
> On Thu, Dec 15, 2016 at 2:12 PM, Arend Van Spriel
> 
> <arend.vanspriel@broadcom.com> wrote:
> > On 15-12-2016 16:33, Pali Rohár wrote:
> >> On Thu Dec 15 09:18:44 2016 Kalle Valo <kvalo@codeaurora.org>
> >> wrote:
> >>> (Adding Luis because he has been working on request_firmware()
> >>> lately)
> >>> 
> >>> Pali Rohár <pali.rohar@gmail.com> writes:
> >>>>>> So no, there is no argument against... request_firmware() in
> >>>>>> fallback mode with userspace helper is by design blocking and
> >>>>>> waiting for userspace. But waiting for some change in DTS in
> >>>>>> kernel is just nonsense.
> >>>>> 
> >>>>> I would just mark the wlan device with status = "disabled" and
> >>>>> enable it in the overlay together with adding the NVS & MAC
> >>>>> info.
> >>>> 
> >>>> So if you think that this solution make sense, we can wait what
> >>>> net wireless maintainers say about it...
> >>>> 
> >>>> For me it looks like that solution can be:
> >>>> 
> >>>> extending request_firmware() to use only userspace helper
> >>> 
> >>> I haven't followed the discussion very closely but this is my
> >>> preference what drivers should do:
> >>> 
> >>> 1) First the driver should do try to get the calibration data and
> >>> mac
> >>> 
> >>>        address from the device tree.
> >> 
> >> Ok, but there is no (dynamic, device specific) data in DTS for
> >> N900. So 1) is noop.
> > 
> > Uhm. What do you mean? You can propose a patch to the DT bindings
> > [1] to get it in there and create your N900 DTB or am I missing
> > something here. Are there hardware restrictions that do not allow
> > you to boot with your own DTB.
> > 
> >>> 2) If they are not in DT the driver should retrieve the
> >>> calibration data
> >>> 
> >>>        with request_firmware(). BUT with an option for user space
> >>>        to implement that with a helper script so that the data
> >>>        can be created dynamically, which I believe openwrt does
> >>>        with ath10k calibration data right now.
> >> 
> >> Currently there is flag for request_firmware() that it should
> >> fallback to user helper if direct VFS access not find needed
> >> firmware.
> >> 
> >> But this flag is not suitable as /lib/firmware already provides
> >> default (not device specific) calibration data.
> >> 
> >> So I would suggest to add another flag/function which will primary
> >> use user helper.
> > 
> > I recall Luis saying that user-mode helper (fallback) should be
> > discouraged, because there is no assurance that there is a
> > user-mode helper so you might just be pissing in the wind.
> 
> There's tons of issues with the current status quo of the so called
> "firmware usermode helper". To start off with its a complete
> misnomer, the kernel's usermode helper infrastructure is implemented
> in lib/kmod.c and it provides an API to help call out to userspace
> some helper for you. That's the real kernel usermode helper. In so
> far as the firmware_class.c driver is concerned -- it only makes use
> of the kernel user mode helper as an option if you have
> CONFIG_UEVENT_HELPER_PATH set to help address kobject uevents. Most
> distributions do not use this, and back in the day systemd udev, and
> prior to that hotplug used to process firmware kobject uevents.
> systemd udev is long gone. Gone. This kobject uevents really are a
> "fallback mechanism" for firmware only -- if cached firmware,
> built-in firmware, or direct filesystem lookup fails. These each are
> their own beast. Finally kobject uevents are only one of the
> fallback
> mechanisms, "custom fallback mechanisms" are the other option and its
> what you and others seem to describe to want for these sorts of
> custom things.
> 
> There are issues with the existing request_firmware() API in that
> everyone and their mother keeps extending it with stupid small API
> extensions to do yet-another-tweak, and then having to go and change
> tons of drivers. Or a new API call added for just one custom knob.
> Naturally this is all plain dumb, so yet-another-API call or new
> argument is not going to help us. We don't have "flags" persi in this
> API, that was one issue with the design of the API, but there are
> much more. The entire change in mechanism of "firmware fallback
> mechanism" depending on which kernel config options you have is
> another. Its a big web of gum put together. All this needs to end.
> 
> I've recently proposed a new patch to first help with understanding
> each of the individual items I've listed above with as much new
> information as is possible. I myself need to re-read it to just keep
> tabs on what the hell is going on at times. After this I have a new
> API proposal which I've been working on for a long time now which
> adds an extensible API with only 2 calls: sync, async, and lets us
> modify attributes through a parameters struct. This is what we
> should use in the future for further extensions.
> 
> For the new API a solution for "fallback mechanisms" should be clean
> though and I am looking to stay as far as possible from the existing
> mess. A solution to help both the old API and new API is possible for
> the "fallback mechanism" though -- but for that I can only refer you
> at this point to some of Daniel Wagner and Tom Gunderson's firmwared
> deamon prospect. It should help pave the way for a clean solution and
> help address other stupid issues.
> 
> I'll note -- the "custom fallback mechanism", part of the old API is
> just a terrible idea for many reasons. I've documented issues in my
> documentation revamp, I suggest to read that, refer to this thread:
> 
> https://lkml.kernel.org/r/20161213030828.17820-4-mcgrof@kernel.org
> 
> > The idea was to have a
> > dedicated API call that explicitly does the request towards
> > user-space.
> 
> In so far as this driver example that was mentioned in this thread my
> recommendation is to use the default existing MAC address /
> calibration data on /lib/firmware/ and then use another request to
> help override for the custom thing. The only issue of course is that
> the timeout for the custom thing is 60 seconds, but if your platforms
> are controlled -- just reduce this to 1 second or so given that udev
> is long gone and it would seem you'd only use a custom fw request to
> depend on. You could also wrap this thing into a kconfig option for
> now, and have the filename specified, if empty then no custom request
> is sent. If set then you have it request it.
> 
> Please note the other patches in my series for the custom fallback
> though. We have only 2 users upstream -- and from recent discussions
> it would seem it might be possible to address what you need with
> firmwared in a clean way without this custom old fallback crap thing.
> Johannes at least has a similar requirement and is convinced a
> "custom" thing can be done without even adding an extra custom
> kobject uevent attribute as from what I recall he hinted, drivers
> have no business to be involved in this. If you do end up using the
> custom fallback mechanism be sure to add super crystal clear
> documentation for it (see my other patches for the declarer for this
> documentation). Since we have only 2 drivers using this custom thing
> its hard to get folks to commit to writing the docs, write it now
> and be sure you think of the future as well.
> 
> Oh also the "custom firmware fallback" was also broken on recent
> kernels for many kernel revisions, it just did not work until
> recently a fix which went in, so your users wills need this fix
> cherry picked. Hey I tell you, the custom fw thing was a terrible
> incarnation. Only 2 drivers use it. There are good reasons to not
> like it (be sure to read the suspend issue I documented).
> 
> > By the way, are we talking here about wl1251 device or driver as
> > you also mentioned wl12xx? I did not read the entire thread.
> 
>   Luis

Hi Luis! wl1251 already uses request_firmware for loading calibration 
data from VFS. And because /lib/firmware/ contains preinstalled default 
calibration data it uses it -- which is wrong as wireless has bad 
quality.

In my setup I'm going to use udev with firmware loading support 
(probably fork eudev), so it should be systemd-independent.

-- 
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 & mac address & calibration data

[Pali Rohár]

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

On Thursday 15 December 2016 21:12:47 Arend Van Spriel wrote:
> On 15-12-2016 16:33, Pali Rohár wrote:
> > On Thu Dec 15 09:18:44 2016 Kalle Valo <kvalo@codeaurora.org> wrote:
> >> (Adding Luis because he has been working on request_firmware()
> >> lately)
> >> 
> >> Pali Rohár <pali.rohar@gmail.com> writes:
> >>>>> So no, there is no argument against... request_firmware() in
> >>>>> fallback mode with userspace helper is by design blocking and
> >>>>> waiting for userspace. But waiting for some change in DTS in
> >>>>> kernel is just nonsense.
> >>>> 
> >>>> I would just mark the wlan device with status = "disabled" and
> >>>> enable it in the overlay together with adding the NVS & MAC
> >>>> info.
> >>> 
> >>> So if you think that this solution make sense, we can wait what
> >>> net wireless maintainers say about it...
> >>> 
> >>> For me it looks like that solution can be:
> >>> 
> >>> extending request_firmware() to use only userspace helper
> >> 
> >> I haven't followed the discussion very closely but this is my
> >> preference what drivers should do:
> >> 
> >> 1) First the driver should do try to get the calibration data and
> >> mac
> >> 
> >>        address from the device tree.
> > 
> > Ok, but there is no (dynamic, device specific) data in DTS for
> > N900. So 1) is noop.
> 
> Uhm. What do you mean? You can propose a patch to the DT bindings [1]
> to get it in there and create your N900 DTB or am I missing
> something here. Are there hardware restrictions that do not allow
> you to boot with your own DTB.

What is [1]?

N900's bootloader does not support DTB and it does not pass any DTB. It 
boots kernel in ATAGs mode. What we are doing is appending DTB compiled 
from kernel sources to end of zImage.

But that appended DTB cannot contains device specific nodes (e.g. 
calibration data for device) as zImage is there for any N900 device, not 
just only one.

> >> 2) If they are not in DT the driver should retrieve the
> >> calibration data
> >> 
> >>        with request_firmware(). BUT with an option for user space
> >>        to implement that with a helper script so that the data
> >>        can be created dynamically, which I believe openwrt does
> >>        with ath10k calibration data right now.
> > 
> > Currently there is flag for request_firmware() that it should
> > fallback to user helper if direct VFS access not find needed
> > firmware.
> > 
> > But this flag is not suitable as /lib/firmware already provides
> > default (not device specific) calibration data.
> > 
> > So I would suggest to add another flag/function which will primary
> > use user helper.
> 
> I recall Luis saying that user-mode helper (fallback) should be
> discouraged, because there is no assurance that there is a user-mode
> helper so you might just be pissing in the wind. The idea was to have
> a dedicated API call that explicitly does the request towards
> user-space.
> 
> By the way, are we talking here about wl1251 device or driver as you
> also mentioned wl12xx? I did not read the entire thread.

Yes, we are talking about wl1251 device, which is in Nokia N900 and 
wl1251.ko and wl1251_spi.ko drivers.

I mentioned wl12xx as it already uses similar approach with mac address 
via request_firmware(). And as those drivers are very similar plus from 
one manufactor and in same kernel folder I mentioned similar API for 
consistency...

-- 
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 & mac address & calibration data

[Aaro Koskinen]

Hi,

On Thu, Nov 24, 2016 at 04:20:45PM +0100, Pali Rohár wrote:
> Proprietary, signed and closed bootloader NOLO does not support DT. So
> for booting you need to append DTS file to kernel image.
> 
> U-Boot is optional and can be used as intermediate bootloader between
> NOLO and kernel. But still it has problems with reading from nand, so
> cannot read NVS data nor MAC address.

You could use kexec to pass the fixed DT.

A.

Re: wl1251 & mac address & calibration data

[Pavel Machek]

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

On Thu 2016-11-24 20:46:01, Aaro Koskinen wrote:
> Hi,
> 
> On Thu, Nov 24, 2016 at 04:20:45PM +0100, Pali Rohár wrote:
> > Proprietary, signed and closed bootloader NOLO does not support DT. So
> > for booting you need to append DTS file to kernel image.
> > 
> > U-Boot is optional and can be used as intermediate bootloader between
> > NOLO and kernel. But still it has problems with reading from nand, so
> > cannot read NVS data nor MAC address.
> 
> You could use kexec to pass the fixed DT.

Yeah. You could also strap desktop PC to a USB GPRS card, and call it
phone. You could also make a pig fly.

But because you could does not mean you should. No, sorry, kexec is
not acceptable. Too hard to set up, slows boot too much.

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: wl1251 & mac address & calibration data

[Pali Rohár]

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

On Thursday 24 November 2016 19:46:01 Aaro Koskinen wrote:
> Hi,
> 
> On Thu, Nov 24, 2016 at 04:20:45PM +0100, Pali Rohár wrote:
> > Proprietary, signed and closed bootloader NOLO does not support DT.
> > So for booting you need to append DTS file to kernel image.
> > 
> > U-Boot is optional and can be used as intermediate bootloader
> > between NOLO and kernel. But still it has problems with reading
> > from nand, so cannot read NVS data nor MAC address.
> 
> You could use kexec to pass the fixed DT.
> 
> A.

IIRC it was broken for N900/omap3, no idea if somebody fixed it.

-- 
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 & mac address & calibration data

[Tony Lindgren]

* Pali Rohár <pali.rohar@gmail.com> [161126 09:21]:
> On Thursday 24 November 2016 19:46:01 Aaro Koskinen wrote:
> > Hi,
> > 
> > On Thu, Nov 24, 2016 at 04:20:45PM +0100, Pali Rohár wrote:
> > > Proprietary, signed and closed bootloader NOLO does not support DT.
> > > So for booting you need to append DTS file to kernel image.
> > > 
> > > U-Boot is optional and can be used as intermediate bootloader
> > > between NOLO and kernel. But still it has problems with reading
> > > from nand, so cannot read NVS data nor MAC address.
> > 
> > You could use kexec to pass the fixed DT.
> > 
> > A.
> 
> IIRC it was broken for N900/omap3, no idea if somebody fixed it.

FYI, at least in next-20161205 kexec works on omap3 for me.

Regards,

Tony