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.
