From: "Pali Rohár" <pali.rohar@gmail.com>
To: "Luis R. Rodriguez" <mcgrof@kernel.org>
Cc: "Arend Van Spriel" <arend.vanspriel@broadcom.com>,
"Tom Gundersen" <teg@jklm.no>, "Daniel Wagner" <wagi@monom.org>,
"Johannes Berg" <johannes@sipsolutions.net>,
"Ming Lei" <ming.lei@canonical.com>,
"Mimi Zohar" <zohar@linux.vnet.ibm.com>,
"Bjorn Andersson" <bjorn.andersson@linaro.org>,
"Rafał Miłecki" <zajec5@gmail.com>,
"Kalle Valo" <kvalo@codeaurora.org>,
"Sebastian Reichel" <sre@kernel.org>,
"Pavel Machek" <pavel@ucw.cz>,
"Michal Kazior" <michal.kazior@tieto.com>,
"Ivaylo Dimitrov" <ivo.g.dimitrov.75@gmail.com>,
"Aaro Koskinen" <aaro.koskinen@iki.fi>,
"Tony Lindgren" <tony@atomide.com>,
linux-wireless <linux-wireless@vger.kernel.org>,
"Network Development" <netdev@vger.kernel.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"David Woodhouse" <dwmw2@infradead.org>,
"Takashi Iwai" <tiwai@suse.de>,
"Josh Boyer" <jwboyer@fedoraproject.org>,
"Dmitry Torokhov" <dmitry.torokhov@gmail.com>
Subject: Re: wl1251 & mac address & calibration data
Date: Fri, 16 Dec 2016 11:35:42 +0100 [thread overview]
Message-ID: <201612161135.43044@pali> (raw)
In-Reply-To: <CAB=NE6VHF+U7DzapENvLMKtN4Dgi9J0qMoeUJ8o4vSwc2-rRFQ@mail.gmail.com>
[-- 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 --]
next prev parent reply other threads:[~2016-12-16 10:35 UTC|newest]
Thread overview: 41+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-11-11 17:20 wl1251 & mac address & calibration data Pali Rohár
2016-11-21 15:51 ` Pali Rohár
2016-11-22 15:22 ` Michal Kazior
2016-11-22 15:31 ` Pali Rohár
2016-11-22 16:14 ` Michal Kazior
2016-11-22 17:05 ` Pali Rohár
2016-11-23 8:24 ` Arend Van Spriel
2016-11-23 22:23 ` Pavel Machek
2016-11-23 22:39 ` Pali Rohár
2016-11-24 7:51 ` Pavel Machek
2016-11-24 8:33 ` Pali Rohár
2016-11-24 15:13 ` Sebastian Reichel
2016-11-24 15:20 ` Pali Rohár
2016-11-24 15:31 ` Ivaylo Dimitrov
2016-11-24 16:08 ` Sebastian Reichel
2016-11-24 16:49 ` Pali Rohár
2016-11-24 18:11 ` Sebastian Reichel
2016-11-24 18:35 ` Pali Rohár
2016-12-15 8:18 ` Kalle Valo
2016-12-15 15:33 ` Pali Rohár
2016-12-15 20:12 ` Arend Van Spriel
2016-12-16 2:03 ` Luis R. Rodriguez
2016-12-16 7:25 ` Daniel Wagner
2016-12-16 10:40 ` Pali Rohár
2016-12-18 10:49 ` Arend Van Spriel
2016-12-18 11:04 ` Pali Rohár
2016-12-18 11:54 ` Arend Van Spriel
2016-12-18 12:09 ` Pali Rohár
2016-12-18 20:08 ` Arend Van Spriel
2016-12-20 11:47 ` Kalle Valo
2016-12-20 16:56 ` Tony Lindgren
2016-12-20 17:06 ` Pali Rohár
2016-12-20 17:11 ` Kalle Valo
2016-12-20 17:21 ` Tony Lindgren
2017-01-12 8:50 ` Pavel Machek
2016-12-16 10:35 ` Pali Rohár [this message]
2016-12-16 10:26 ` Pali Rohár
2016-11-24 18:46 ` Aaro Koskinen
2016-11-26 17:17 ` Pavel Machek
2016-11-26 17:20 ` Pali Rohár
2016-12-05 23:51 ` Tony Lindgren
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=201612161135.43044@pali \
--to=pali.rohar@gmail.com \
--cc=aaro.koskinen@iki.fi \
--cc=arend.vanspriel@broadcom.com \
--cc=bjorn.andersson@linaro.org \
--cc=dmitry.torokhov@gmail.com \
--cc=dwmw2@infradead.org \
--cc=ivo.g.dimitrov.75@gmail.com \
--cc=johannes@sipsolutions.net \
--cc=jwboyer@fedoraproject.org \
--cc=kvalo@codeaurora.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-wireless@vger.kernel.org \
--cc=mcgrof@kernel.org \
--cc=michal.kazior@tieto.com \
--cc=ming.lei@canonical.com \
--cc=netdev@vger.kernel.org \
--cc=pavel@ucw.cz \
--cc=sre@kernel.org \
--cc=teg@jklm.no \
--cc=tiwai@suse.de \
--cc=tony@atomide.com \
--cc=wagi@monom.org \
--cc=zajec5@gmail.com \
--cc=zohar@linux.vnet.ibm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).