Re: wl1251 & mac address & calibration data

[Pali RohÃr]

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@xxxxxxxxxxxx> wrote:
> > On 15-12-2016 16:33, Pali RohÃr wrote:
> >> On Thu Dec 15 09:18:44 2016 Kalle Valo <kvalo@xxxxxxxxxxxxxx>
> >> wrote:
> >>> (Adding Luis because he has been working on request_firmware()
> >>> lately)
> >>> 
> >>> Pali RohÃr <pali.rohar@xxxxxxxxx> 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@xxxxxxxxxx
> 
> > 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@xxxxxxxxx

Attachment: signature.asc
Description: This is a digitally signed message part.