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

Groups > linux.kernel > #1542547 > unrolled thread

Re: wl1251 & mac address & calibration data

Back to article view | Back to linux.kernel

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

Contents

  Re: wl1251 & mac address & calibration data Kalle Valo <kvalo@codeaurora.org> - 2016-12-15 09:20 +0100
    Re: wl1251 & mac address & calibration data Pali Rohár <pali.rohar@gmail.com> - 2016-12-15 16:40 +0100
      Re: wl1251 & mac address & calibration data Arend Van Spriel <arend.vanspriel@broadcom.com> - 2016-12-15 21:20 +0100
        Re: wl1251 & mac address & calibration data "Luis R. Rodriguez" <mcgrof@kernel.org> - 2016-12-16 03:10 +0100
          Re: wl1251 & mac address & calibration data Daniel Wagner <wagi@monom.org> - 2016-12-16 08:30 +0100
            Re: wl1251 & mac address & calibration data Pali Rohár <pali.rohar@gmail.com> - 2016-12-16 11:50 +0100
              Re: wl1251 & mac address & calibration data Arend Van Spriel <arend.vanspriel@broadcom.com> - 2016-12-18 12:00 +0100
                Re: wl1251 & mac address & calibration data Pali Rohár <pali.rohar@gmail.com> - 2016-12-18 12:10 +0100
                  Re: wl1251 & mac address & calibration data Arend Van Spriel <arend.vanspriel@broadcom.com> - 2016-12-18 13:00 +0100
                    Re: wl1251 & mac address & calibration data Pali Rohár <pali.rohar@gmail.com> - 2016-12-18 13:10 +0100
                      Re: wl1251 & mac address & calibration data Arend Van Spriel <arend.vanspriel@broadcom.com> - 2016-12-18 21:10 +0100
                        Re: wl1251 & mac address & calibration data Kalle Valo <kvalo@codeaurora.org> - 2016-12-20 12:50 +0100
                          Re: wl1251 & mac address & calibration data Tony Lindgren <tony@atomide.com> - 2016-12-20 18:00 +0100
                            Re: wl1251 & mac address & calibration data Pali Rohár <pali.rohar@gmail.com> - 2016-12-20 18:10 +0100
                            Re: wl1251 & mac address & calibration data Kalle Valo <kvalo@codeaurora.org> - 2016-12-20 18:20 +0100
                              Re: wl1251 & mac address & calibration data Tony Lindgren <tony@atomide.com> - 2016-12-20 18:30 +0100
          Re: wl1251 & mac address & calibration data Pali Rohár <pali.rohar@gmail.com> - 2016-12-16 11:40 +0100
        Re: wl1251 & mac address & calibration data Pali Rohár <pali.rohar@gmail.com> - 2016-12-16 11:30 +0100

#1542547 — Re: wl1251 & mac address & calibration data

(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

[toc] | [next] | [standalone]

#1542836

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

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

#1543030

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

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

#1543180

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

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

#1543227

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

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

#1543350

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

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

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

#1544119

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

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

#1544121

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

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

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

#1544131

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

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

#1544132

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

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

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

#1544197

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

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

#1545067

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

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

#1545261

* 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

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

#1545293

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

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

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

#1545300

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

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

#1545310

* 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

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

#1543342

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

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

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

#1543341

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

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web