firmware: add SmPL grammar to avoid issues

Thou shalt not make firmware calls early on init or probe.

systemd already ripped support out for the usermode helper a while ago, there are still users that require the usermode helper, however systemd's use of the usermode helper exacerbated a long lasting issue of the fact that we have many drivers that load firmware on init or probe. Independent of the usermode helper loading firmware on init or probe is a bad idea for a few reasons.

When the firmware is read directly from the filesystem by the kernel, if the driver is built-in to the kernel the firmware may not yet be available, for such uses one could use initramfs and stuff the firmware inside, or one also use CONFIG_EXTRA_FIRMWARE; however not all distributions use this, as such generally one cannot count on this. There is another corner cases to consider, since we are accessing the firmware directly folks cannot expect new found firmware on a filesystem after we switch off from an initramfs with pivot_root().

Supporting such situations is possible today but fixing it for good is really complex due to the large amount of variablity in the boot up process.

Instead just document the expectations properly and add a grammar rule to enable folks to check / validate / police if drivers are using the request firmware API early on init or probe.

The SmPL rule used to check for the probe routine is loose and is currently defined through a regexp, that can easily be extended to any other known bus probe routine names. It also uses the new Python iteration support which allows us to backtrack from a request_firmware*() call back to a possible probe or init, iteratively. Without iteration we would only be able to get reports for callers who directly use the request_firmware*() API on the initial probe or init routine.

There are 4 offenders at this time:

mcgrof at ergon ~/linux-next (git::20160609)$ export COCCI=scripts/coccinelle/api/request_firmware.cocci mcgrof at ergon ~/linux-next (git::20160609)$ make coccicheck MODE=report

drivers/fmc/fmc-fakedev.c: ERROR: driver call request firmware call on its init routine on line 321. drivers/fmc/fmc-write-eeprom.c: ERROR: driver call request firmware call on its probe routine on line 136. drivers/tty/serial/rp2.c: ERROR: driver call request firmware call on its probe routine on line 796. drivers/tty/serial/ucc_uart.c: ERROR: driver call request firmware call on its probe routine on line 1246.

I checked and verified all these are valid reports. This list also matches the same list as in 20150805, so we have fortunately not gotten worse. Let's keep it that way and also fix these drivers.

v2: changes from v1 [0]:

o This now supports iteration, this extends our coverage on the report

o Update documentation and commit log to accept the fate of not being able to remove the usermode helper.

[0] https://lkml.kernel.org/r/1440811107-861-1-git-send-email-mcgrof at do-not-panic.com

Cc Alessandro Rubini <[email protected]>

Cc Kevin Cernekee <[email protected]>

Cc Daniel Vetter <[email protected]>

Cc Mimi Zohar <[email protected]>

Cc David Woodhouse <[email protected]>

Cc Kees Cook <[email protected]>

Cc Dmitry Torokhov <[email protected]>

Cc Ming Lei <[email protected]>

Cc Jonathan Corbet <[email protected]>

Cc Julia Lawall <[email protected]>

Cc Gilles Muller <[email protected]>

Cc Nicolas Palix <[email protected]>

Cc Thierry Martinez <[email protected]>

Cc Michal Marek <[email protected]>

Cc cocci at systeme.lip6.fr

Cc Alessandro Rubini <[email protected]>

Cc Kevin Cernekee <[email protected]>

Cc Greg Kroah-Hartman <[email protected]>

Cc Jiri Slaby <[email protected]>

Cc linux-serial at vger.kernel.org

Cc linux-doc at vger.kernel.org

Cc linux-serial at vger.kernel.org

Cc linuxppc-dev at lists.ozlabs.org

Signed-off-by Luis R. Rodriguez <[email protected]>

---
 Documentation/firmware_class/README                |  20 ++++
 drivers/base/Kconfig                               |   2 +-
 .../request_firmware-avoid-init-probe-init.cocci   | 130 +++++++++++++++++++++
 3 files changed, 151 insertions(+), 1 deletion(-)
 create mode 100644 scripts/coccinelle/api/request_firmware-avoid-init-probe-init.cocci

Documentation/firmware_class/README +20 -0

index cafdca8b3b15..056d1cb9d365 100644
@@ -93,6 +93,26 @@
    user contexts to request firmware asynchronously, but can't be called
    in atomic contexts.
 
+Requirements:
+=============
+
+You should avoid at all costs requesting firmware on both init and probe paths
+of your device driver. Reason for this is the complexity needed to ensure a
+firmware will be available for a driver early in boot through different
+build configurations. Consider built-in drivers needing firmware early, or
+consider a driver assuming it will only get firmware after pivot_root().
+
+Drivers that really need firmware early should use stuff the firmware in
+initramfs or consider using CONFIG_EXTRA_FIRMWARE. Using initramfs is much
+more portable to more distributions as not all distributions wish to enable
+CONFIG_EXTRA_FIRMWARE. Should a driver require the firmware being built-in
+it should depend on CONFIG_EXTRA_FIRMWARE. There is no current annotation for
+requiring a firmware on initramfs.
+
+If you're a maintainer you can help police this with:
+
+$ export COCCI=scripts/coccinelle/api/request_firmware-avoid-init-probe-init.cocci
+$ make coccicheck MODE=report
 
  about in-kernel persistence:
  ---------------------------

drivers/base/Kconfig +1 -1

index 98504ec99c7d..12b4f5551501 100644
@@ -152,7 +152,7 @@ config FW_LOADER_USER_HELPER
        bool
 
 config FW_LOADER_USER_HELPER_FALLBACK
-       bool "Fallback user-helper invocation for firmware loading"
+       bool "Fallback user-helper invocation for firmware loading (avoid)"
        depends on FW_LOADER
        select FW_LOADER_USER_HELPER
        help

scripts/coccinelle/api/request_firmware-avoid-init-probe-init.cocci new file +130 -1

new file mode 100644
index 000000000000..cf180c59e042
@@ -0,0 +1,130 @@
+///
+/// Thou shalt not request firmware on init or probe
+///
+// Confidence: High
+// Copyright: (C) 2016 Luis R. Rodriguez <[email protected]>
+// Copyright: (C) 2015 Julia Lawall, Inria/LIP6.
+//
+// GPLv2.
+// URL: http://coccinelle.lip6.fr/
+// Options: --no-includes --include-headers --no-show-diff
+// Requires: 1.0.5
+//
+// Coccinelle 1.0.5 is required given that this uses the shiny new
+// python iteration feature.
+
+virtual report
+virtual after_start
+
+// -------------------------------------------------------------------------
+
+ at initialize:python@
+@@
+
+seen = set()
+
+def add_if_not_present(f, file, starter_var):
+    if (f, file, starter_var) not in seen:
+        seen.add((f, file, starter_var))
+        it = Iteration()
+        if file != None:
+            it.set_files([file])
+    it.add_virtual_rule(after_start)
+    it.add_virtual_rule(report)
+    it.add_virtual_identifier(fn, f)
+    it.add_virtual_identifier(starter, starter_var)
+    it.register()
+
+reported = set()
+
+def print_err(str, file, line):
+    if (file, line) not in reported:
+        reported.add((file, line))
+       print "%s: ERROR: driver call request firmware call on its %s routine on line %d." % (file, str, line)
+
+@ defines_module_init@
+declarer name module_init;
+identifier init;
+@@
+
+module_init(init);
+
+@ has_probe depends on defines_module_init@
+identifier drv_calls, drv_probe;
+type bus_driver;
+identifier probe_op =~ "(probe)";
+@@
+
+bus_driver drv_calls = {
+       .probe_op = drv_probe,
+};
+
+ at hascall depends on !after_start && defines_module_init@
+position p;
+@@
+
+(
+request_firmware at p(...)
+|
+request_firmware_nowait at p(...)
+|
+request_firmware_direct at p(...)
+)
+
+ at script:python@
+init << defines_module_init.init;
+p << hascall.p;
+@@
+
+if p[0].current_element == init:
+    print_err("init", p[0].file, int(p[0].line))
+    cocci.include_match(False)
+
+ at script:python@
+drv_probe << has_probe.drv_probe;
+p << hascall.p;
+@@
+
+if p[0].current_element == drv_probe:
+    print_err("probe", p[0].file, int(p[0].line))
+    cocci.include_match(False)
+
+ at script:python@
+p << hascall.p;
+@@
+
+add_if_not_present(p[0].current_element, p[0].file, p[0].line)
+
+ at hasrecall depends on after_start@
+position p;
+identifier virtual.fn;
+@@
+
+fn at p(...)
+
+ at script:python@
+init << defines_module_init.init;
+p << hasrecall.p;
+starter << virtual.starter;
+@@
+
+if p[0].current_element == init:
+    print_err("init", p[0].file, int(starter))
+    cocci.include_match(False)
+
+ at script:python@
+drv_probe << has_probe.drv_probe;
+p << hasrecall.p;
+starter << virtual.starter;
+@@
+
+if p[0].current_element == drv_probe:
+    print_err("probe", p[0].file, int(starter))
+    cocci.include_match(False)
+
+ at script:python@
+p << hasrecall.p;
+starter << virtual.starter;
+@@
+
+add_if_not_present(p[0].current_element, p[0].file, starter)
--

2.8.2

Quoted 1 line · Luis R. Rodriguez

Thou shalt not make firmware calls early on init or probe.

Quoted 5 lines

There are 4 offenders at this time:

mcgrof at ergon ~/linux-next (git::20160609)$ export COCCI=scripts/coccinelle/api/request_firmware.cocci mcgrof at ergon ~/linux-next (git::20160609)$ make coccicheck MODE=report

Quoted 4 lines

drivers/fmc/fmc-fakedev.c: ERROR: driver call request firmware call on its init routine on line 321. drivers/fmc/fmc-write-eeprom.c: ERROR: driver call request firmware call on its probe routine on line 136. drivers/tty/serial/rp2.c: ERROR: driver call request firmware call on its probe routine on line 796. drivers/tty/serial/ucc_uart.c: ERROR: driver call request firmware call on its probe routine on line 1246.

Plus all gpu drivers which need firmware. And yes we must load them at probe

because people are generally pissed when they boot their machine and the screen goes black. On top of that a lot of people want their gpu drivers to be built-in, but can't ship the firmware blobs in the kernel image because gpl. Yep, there's a bit a contradiction there ...

I think what would work is loading the different subsystems of the driver in parallel (we already do that largely)

, and then if one
firmware blob isn't there yet simply stall that async worker until it shows up.

But the answers I've gotten thus far from request_firmware() folks (well at least Greg) is don't do that ...

Is that problem still somewhere on the radar?

Atm there's various
wait_for_rootfs hacks out there floating in vendor/product trees.

"Avoid at all costs" sounds like upstream prefers to not care about android/cros in those case (yes I know most arm socs don't need firmware, which would make it a problem fo just be a subset of all devices).

Quoted 2 lines · Daniel Vetter

Quoted 1 line · Luis R. Rodriguez

Thou shalt not make firmware calls early on init or probe.

<-- snip -->

Quoted 10 lines

Quoted 9 lines

There are 4 offenders at this time:

mcgrof at ergon ~/linux-next (git::20160609)$ export COCCI=scripts/coccinelle/api/request_firmware.cocci mcgrof at ergon ~/linux-next (git::20160609)$ make coccicheck MODE=report

drivers/fmc/fmc-fakedev.c: ERROR: driver call request firmware call on its init routine on line 321. drivers/fmc/fmc-write-eeprom.c: ERROR: driver call request firmware call on its probe routine on line 136. drivers/tty/serial/rp2.c: ERROR: driver call request firmware call on its probe routine on line 796. drivers/tty/serial/ucc_uart.c: ERROR: driver call request firmware call on its probe routine on line 1246.

Quoted 2 lines

Plus all gpu drivers which need firmware. And yes we must load them at probe

Do you have an upstream driver in mind that does this ? Is it on device drier module probe or a DRM subsystem specific probe call of some sort ?

Quoted 4 lines

because people are generally pissed when they boot their machine and the screen goes black. On top of that a lot of people want their gpu drivers to be built-in, but can't ship the firmware blobs in the kernel image because gpl. Yep, there's a bit a contradiction there ...

Can they use initramfs for this ?

Also just curious -- as with other subsystems, is it possible to load a generic driver first, say vesa, and then a more enhanced one later ? I am not saying this is ideal or am I suggesting this, I'd just like to know the feasibility of this.

Quoted 2 lines

I think what would work is loading the different subsystems of the driver in parallel (we already do that largely)

Init level stuff is actually pretty synchronous, and in fact both init and probe are called serially. There are a few subsystems which have been doing things a bit differently, but these are exceptions.

When you say we already do this largely, can you describe a bit more precisely what you mean ?

Quoted 3 lines

, and then if one
firmware blob isn't there yet simply stall that async worker until it shows up.

Is this an existing framework or do you mean if we add something generic to do this async loading of subsystems ?

Quoted 2 lines

But the answers I've gotten thus far from request_firmware() folks (well at least Greg) is don't do that ...

Well in this patch set I'm adding myself as a MAINTAINER and I've been extending the firmware API recently to help with a few new features I want, I've been wanting to hear more feedback from other's needs and I had actually not gotten much -- except only recently with the usermode helper and reasons why some folks thought they could not use direct firmware loading from the fs. I'm keen to hear or more use cases and needs specially if they have to do with improving boot time and asynchronous boot.

Quoted 1 line

Is that problem still somewhere on the radar?

Not mine.

Quoted 2 lines

Atm there's various
wait_for_rootfs hacks out there floating in vendor/product trees.

This one I've heard about recently, and I suggested two possible solutions, with a preference to a simply notification of when rootfs is available from userspace.

Quoted 4 lines

"Avoid at all costs" sounds like upstream prefers to not care about android/cros in those case (yes I know most arm socs don't need firmware, which would make it a problem fo just be a subset of all devices).

In my days of dealing with Android I learned most folks did not frankly care too much about upstream-first model. That means things were reactive. That's a business mind set and that's fine. However for upstream we want what is best and to discuss. I haven't seen proposals so, so long as we just hear of "hacks" that some folks do in vendor/product trees, what can we do ?

So .. I agree, let's avoid the hacks. Patches welcomed.

Quoted 16 lines · Luis R. Rodriguez

Quoted 2 lines · Daniel Vetter

Quoted 1 line · Luis R. Rodriguez

Thou shalt not make firmware calls early on init or probe.

<-- snip -->

Quoted 10 lines

Quoted 9 lines

There are 4 offenders at this time:

mcgrof at ergon ~/linux-next (git::20160609)$ export COCCI=scripts/coccinelle/api/request_firmware.cocci mcgrof at ergon ~/linux-next (git::20160609)$ make coccicheck MODE=report

drivers/fmc/fmc-fakedev.c: ERROR: driver call request firmware call on its init routine on line 321. drivers/fmc/fmc-write-eeprom.c: ERROR: driver call request firmware call on its probe routine on line 136. drivers/tty/serial/rp2.c: ERROR: driver call request firmware call on its probe routine on line 796. drivers/tty/serial/ucc_uart.c: ERROR: driver call request firmware call on its probe routine on line 1246.

Quoted 5 lines

Quoted 2 lines

Plus all gpu drivers which need firmware. And yes we must load them at probe

Do you have an upstream driver in mind that does this ? Is it on device drier module probe or a DRM subsystem specific probe call of some sort ?

i915 is the one I care about for obvious reasons ;-) It's all from the pci device probe function, but nested really deeply.

Quoted 6 lines

Quoted 4 lines

because people are generally pissed when they boot their machine and the screen goes black. On top of that a lot of people want their gpu drivers to be built-in, but can't ship the firmware blobs in the kernel image because gpl. Yep, there's a bit a contradiction there ...

Can they use initramfs for this ?

Apparently that's also uncool with the embedded folks.

Tbh I don't
know exactly why. Also I thought initramfs is available only after built-in drivers have finished loading, but maybe that changed.

Quoted 4 lines

Also just curious -- as with other subsystems, is it possible to load a generic driver first, say vesa, and then a more enhanced one later ? I am not saying this is ideal or am I suggesting this, I'd just like to know the feasibility of this.

Some users want a fully running gfx stack 2s after power-on. There's not even enough time to load an uefi or vga driver first. i915 directly initializes the gpu from power-on state on those.

Quoted 7 lines

Quoted 2 lines

I think what would work is loading the different subsystems of the driver in parallel (we already do that largely)

Init level stuff is actually pretty synchronous, and in fact both init and probe are called serially. There are a few subsystems which have been doing things a bit differently, but these are exceptions.

Quoted 2 lines

When you say we already do this largely, can you describe a bit more precisely what you mean ?

Oh, this isn't subsystems as in linux device/driver model, but different parts within the driver. We fire up a bunch of struct work to get various bits done asynchronously.

Quoted 6 lines

Quoted 3 lines

, and then if one
firmware blob isn't there yet simply stall that async worker until it shows up.

Is this an existing framework or do you mean if we add something generic to do this async loading of subsystems ?

normal workers, and flush_work and friends to sync up. We have some really fancy ideas for essentially async init tasks that can declare their depencies systemd-unit file style, but that's only in the prototype stage.

We need this (eventually) since handling the ordering correctly is getting unwieldy. But again just struct work launched from the main pci probe function.

Quoted 28 lines

Quoted 2 lines

But the answers I've gotten thus far from request_firmware() folks (well at least Greg) is don't do that ...

Well in this patch set I'm adding myself as a MAINTAINER and I've been extending the firmware API recently to help with a few new features I want, I've been wanting to hear more feedback from other's needs and I had actually not gotten much -- except only recently with the usermode helper and reasons why some folks thought they could not use direct firmware loading from the fs. I'm keen to hear or more use cases and needs specially if they have to do with improving boot time and asynchronous boot.

Quoted 1 line

Is that problem still somewhere on the radar?

Not mine.

Quoted 2 lines

Atm there's various
wait_for_rootfs hacks out there floating in vendor/product trees.

This one I've heard about recently, and I suggested two possible solutions, with a preference to a simply notification of when rootfs is available from userspace.

Quoted 4 lines

"Avoid at all costs" sounds like upstream prefers to not care about android/cros in those case (yes I know most arm socs don't need firmware, which would make it a problem fo just be a subset of all devices).

Quoted 6 lines

In my days of dealing with Android I learned most folks did not frankly care too much about upstream-first model. That means things were reactive. That's a business mind set and that's fine. However for upstream we want what is best and to discuss. I haven't seen proposals so, so long as we just hear of "hacks" that some folks do in vendor/product trees, what can we do ?

One of the proposals which would have worked for us died a while back:

https://lkml.org/lkml/2014/6/15/47

That's essentially what I'd like to have.

[cut discussion about async probe]

Quoted 1 line

So .. I agree, let's avoid the hacks. Patches welcomed.

Hm, this is a definite change of tack - back when I discussed this with Greg about 2 ks ago it sounded like "don't do this". The only thing we need is some way to wait for rootfs before we do the request_firmware. Everything else we handle already in the kernel.

Quoted 6 lines

Quoted 1 line

So .. I agree, let's avoid the hacks. Patches welcomed.

Hm, this is a definite change of tack - back when I discussed this with Greg about 2 ks ago it sounded like "don't do this". The only thing we need is some way to wait for rootfs before we do the request_firmware. Everything else we handle already in the kernel.

OK so lets just get this userspace event done, and we're set.

Cutting down since a lot of this is probably better discussed at ks/lpc. Aside, if you want to check out Chris Wilson's work on our new depency handling, it's called kfence.

https://lkml.org/lkml/2016/7/17/37

Quoted 7 lines

Quoted 6 lines

Quoted 1 line

So .. I agree, let's avoid the hacks. Patches welcomed.

Hm, this is a definite change of tack - back when I discussed this with Greg about 2 ks ago it sounded like "don't do this". The only thing we need is some way to wait for rootfs before we do the request_firmware. Everything else we handle already in the kernel.

Quoted 1 line

OK so lets just get this userspace event done, and we're set.

Ah great. As long as that special wait-for-rootfs-pls firmware request is still allowed, i915&gfx folks will be happy. We will call it from probe though ;-) but all from our own workers.

Quoted 5 lines · Daniel Vetter

Quoted 1 line · Luis R. Rodriguez

Can they use initramfs for this ?

Apparently that's also uncool with the embedded folks.

What's uncool with embedded folks? To use initramfs for firmware ? If so can you explain why ?

Quoted 4 lines · Daniel Vetter

On Wed, Aug 24, 2016 at 10:39 PM, Luis R. Rodriguez <[email protected]> wrote:
Some users want a fully running gfx stack 2s after power-on. There's not even enough time to load an uefi or vga driver first. i915 directly initializes the gpu from power-on state on those.

I see.. thanks.

Quoted 7 lines

Quoted 7 lines

Quoted 2 lines

I think what would work is loading the different subsystems of the driver in parallel (we already do that largely)

Init level stuff is actually pretty synchronous, and in fact both init and probe are called serially. There are a few subsystems which have been doing things a bit differently, but these are exceptions.

Quoted 6 lines

Quoted 2 lines

When you say we already do this largely, can you describe a bit more precisely what you mean ?

Oh, this isn't subsystems as in linux device/driver model, but different parts within the driver. We fire up a bunch of struct work to get various bits done asynchronously.

Thanks for the clarification.

Quoted 3 lines

One of the proposals which would have worked for us died a while back:

https://lkml.org/lkml/2014/6/15/47

Interesting, the problem stated here, which comes from the fact (as clarified above) the rootfs comes up only *after* we run do_initcalls() as such there are possible theoretical races even with request_firmware_nowait() -- as such that justifies even more the SmPL grammar rule to include even request_firmware_nowait() on probe / init as this patch has.

Anyway yeah that patch has good intentions but its wrong for a slew of reasons. The problem is the same as discussed with Bjorn recently. The solution discussed is that since only userspace knows when the *real* rootfs is ready (because of slew of reasons) the best we can do is have an event issued by userspace to inform us of that.

Thoughts ?

Quoted 2 lines · Luis R. Rodriguez

Thoughts ?

I really think this is a horrible hack.

It's basically the kernel giving up, and relying on user space to give a single flag, and it's broken nasty crap. Worse, it's broken nasty crap with a user interface, so we'll be stuck with it forever. Please no.

What are the drivers that need this, and why can't those drivers just be fixed to not do braindead things?

Like what? Some devices do need to have firmware loaded so we know their capabilities, so we really can't push the firmware loading into "open".

Quoted 4 lines · Dmitry Torokhov

Like what? Some devices do need to have firmware loaded so we know their capabilities, so we really can't push the firmware loading into "open".

So you
(a) document that

(b) make the driver only build as a module

(c) make sure the module and the firmware go together

End of problem.

Why make up random interfaces for crazy stuff?

Unfortunately module loading and availability of firmware is very loosely coupled.

Quoted 3 lines · Dmitry Torokhov

Unfortunately module loading and availability of firmware is very loosely coupled.

The whole "let's add a new magical proc entry to say that all filesystems are mounted" is all about the user space coupling them.

I'm just saying that if user space must know about when firmware is ready to be loaded anyway, just do it rigth. Not with some hacky "you can now do random things" flag. But by having user space actually say "put this module together with this firmware".

If you just put the two pieces together, then the module "will just work".

And quite frankly, that sounds like a much better maintenance practice anyway. If some module doesn't work without the firmware, then dammit, the two *should* go together. Putting them in two different places would be *INSANE*.

Quoted 7 lines · Linus Torvalds

Quoted 3 lines · Dmitry Torokhov

Unfortunately module loading and availability of firmware is very loosely coupled.

The whole "let's add a new magical proc entry

Quoted 2 lines

to say that all
filesystems are mounted" is all about the user space coupling them.

I was thinking if we kernel could post "conditions" (maybe simple strings) that it waits for, and userspace could unlock these "conditions". One of them might be "firmware available".

Quoted 3 lines

I'm just saying that if user space must know about when firmware is ready to be loaded anyway, just do it right. Not with some hacky "you can now do random things" flag.

Quoted 2 lines

But by having user space actually say
"put this module together with this firmware".

Quoted 6 lines

If you just put the two pieces together, then the module "will just work".

And quite frankly, that sounds like a much better maintenance practice anyway. If some module doesn't work without the firmware, then dammit, the two *should* go together. Putting them in two different places would be *INSANE*.

Quite often it does until it does not. Most of the touch controllers work just fine until some event (abrupt cutting of power for example) where nvram gets corrupted and they come up in bootloader mode. It is just an example.

We already have MODULE_FIRMWARE(), we could have MODULE_FIRMWARE_REQ() or something like it to help annotate the the driver was only functional with the firmware, punt things to kmod to deal with the requirements.

Quoted 3 lines · Luis R. Rodriguez

We already have MODULE_FIRMWARE(), we could have MODULE_FIRMWARE_REQ() or something like it to help annotate the the driver was only functional with the firmware, punt things to kmod to deal with the requirements.

That implies that a single driver will only use a single version of the firmware.

There are cases where we want a single driver to load firmware depending on e.g. hardware revisions,

or previous firmware version and
there are cases where we want to load firmware based on requested use-cases.

I was thinking if we kernel could post
"conditions" (maybe simple stings) that it waits for, and userspace could unlock these "conditions". One of them might be "firmware available".

I was thinking if we kernel could post
"conditions" (maybe simple stings) that it waits for, and userspace could unlock these "conditions". One of them might be "firmware available".

I was thinking if we kernel could post
"conditions" (maybe simple stings) that it waits for, and userspace could unlock these "conditions". One of them might be "firmware available".

Quoted 4 lines

I was thinking if we kernel could post
"conditions" (maybe simple stings) that it waits for, and userspace could unlock these "conditions". One of them might be "firmware available".

On idea offered by Josh Triplett that seems to overlap with this one is to have something similar to the (deprecated) userhelper with *per-blob* requests and notifications except for one major difference:
userspace would not anymore be in charge of *providing* the blob but would instead only *signal* when a given blob becomes available and is either found or found missing. Then the kernel loads the blob _by itself_; unlike the userhelper. No new =E2=80=9Ccritical filesystem=E2=80=

and a *per-blob basis*, allowing any variation of blob locations across any number of initramfs and filesystems.

Quoted 6 lines · Marc

Quoted 4 lines · Dmitry Torokhov

I was thinking if we kernel could post
"conditions" (maybe simple stings) that it waits for, and userspace could unlock these "conditions". One of them might be "firmware available".

Quoted 10 lines

On idea offered by Josh Triplett that seems to overlap with this one is to have something similar to the (deprecated) userhelper with *per-blob* requests and notifications except for one major difference:
userspace would not anymore be in charge of *providing* the blob but would instead only *signal* when a given blob becomes available and is either found or found missing. Then the kernel loads the blob _by itself_; unlike the userhelper. No new “critical filesystem” concept and a *per-blob basis*, allowing any variation of blob locations across any number of initramfs and filesystems.

Really, I do not quite understand why people have issues with usermode helper/uevents.

It used to work reasonably well (if you were using request_firmware_nowait()), as the kernel would post the request and then, when userspace was ready[^Hier], uevents would be processed and firmware would be loaded. We had a timeout of 60(?) seconds by default, but that would be adjusted as systems needed.

Unfortunately it all broke when udev started insisting [1] on servicing some uevents in strict sequence, which resulted in boot stalls.

Maybe the ultimate answer is to write a firmware loading daemon that would also listen to netlink events and do properly what udev refused to be doing?

The distribution would know when it is ready
to service firmware requests (and thus when to start this daemon), and we would have the freedom of having drivers both built-in and as modules and bulding firmware into kernel, intiramfs or keep on a "real" fs available at later time.

I am not sure how/why a firmware loading daemon would be a better idea now. What Marc describes that Josh proposed with signals for userspcae seems more aligned with what we likely need

Quoted 4 lines · Luis R. Rodriguez

I am not sure how/why a firmware loading daemon would be a better idea now. What Marc describes that Josh proposed with signals for userspcae seems more aligned with what we likely need

Quite frankly, I doubt you want a signal.

You will want to have some way to specify where the firmware files are. Right now we have "fw_path[]" which is hardcoded except for the first entry that can be set as a module parameter. But you'd probably want to expand on that, which implies some /sys or /proc interface.

And once you do that, wouldn't it make more sense to just make the "update the firmware path /proc/sys/kernel/fw_path file" make things re-search for firmware?

In other words, the interface has to be something *sensible*. Not some idiotic ad-hoc "send a signal" (of which that stupid original patch was just a very odd example).

Note that the races are beyond firmware, so all kernel_read_file_from_path() users, as such re-using such old /sys/ interafeces for firmware will not suffice to cover all ground now for the same race for other possible users.

Quoted 5 lines · Luis R. Rodriguez

Note that the races are beyond firmware, so all kernel_read_file_from_path() users, as such re-using such old /sys/ interafeces for firmware will not suffice to cover all ground now for the same race for other possible users.

Blah blah blah.

The reason I've hated this whole discussion is that it's full of "let's re-architect everything", and then it has these horribly warty interfaces.

It's classic second-system syndrome.

Just do *one* thing, and do it well. Don't change anything else. Don't force existign drivers to use new interfaces. Don't over-architect, and don't do stupid interfaces.

If user-space mounts a new filesystem (or just unpacks files from a tar-file that has firmware images in it, for chissake), that is not some magical "critical mount event". The whole concept is just stupid. Is it a "mount event" when the user downloads a new firmware image from the internet?

HELL NO.

But what is equally stupid is to then dismiss simple models because some totally unrelated "beyond firmware" issue.

Anything that is "beyond firmware" shouldn't even be discussed, for chrissake! It has nothing what-so-ever to do with firmware loading. If there ends up being some common helper functions, and shared code, that *still* doesn't make it so.

Basic rules of thumb:

(a) don't over-design

(b) don't have stupid illogical interfaces

(c) don't conflate different issues just because you think they may
have shared code.

(4) be consistent. Don't make up new interfaces, and most certainly
do *NOT* dismiss something just because it's what we have done before.

That's it.

Quoted 3 lines · Luis R. Rodriguez

I am not sure how/why a firmware loading daemon would be a better idea now. What Marc describes that Josh proposed with signals for userspcae seems more aligned with what we likely need

Quite frankly, I doubt you want a signal.

You will want to have some way to specify where the firmware files are. Right now we have "fw_path[]" which is hardcoded except for the first entry that can be set as a module parameter. But you'd probably want to expand on that, which implies some /sys or /proc interface.

And once you do that, wouldn't it make more sense to just make the "update the firmware path /proc/sys/kernel/fw_path file" make things re-search for firmware?

I definitely don't think it
should be a system-wide "mount event"; it should be a per-device "go direct-load your firmware" poke from userspace.

Quoted 2 lines

Thoughts ?

What are the drivers that need this, and why can't those drivers just be fixed to not do braindead things?

It's basically the kernel giving up, and relying on user space to give a single flag, and it's broken nasty crap. Worse, it's broken nasty crap with a user interface, so we'll be stuck with it forever. Please no.

I really think this is a horrible hack.

Linus, I reversed the order of your questions/answers to fit my answer better.

Quoted 3 lines · Bjorn Andersson

Linus, I reversed the order of your questions/answers to fit my answer better.

Nobody has actually answered the "why don't we just tie the firmware and module together" question.

Really. If the driver doesn't work without the firmware, then why the hell is it separated from it in the first place?

The hack is a hack, and it just sounds *stupid*.

Quoted 3 lines · Linus Torvalds

On Tue, Sep 6, 2016 at 10:46 AM, Bjorn Andersson
Nobody has actually answered the "why don't we just tie the firmware and module together" question.

The answer to this depends on the details of the suggestion; but generally there's a much stronger bond between the kernel and the driver than between the driver and the firmware in my cases.

Quoted 6 lines · Bjorn Andersson

Quoted 3 lines · Linus Torvalds

On Tue, Sep 6, 2016 at 10:46 AM, Bjorn Andersson
Nobody has actually answered the "why don't we just tie the firmware and module together" question.

Quoted 3 lines

The answer to this depends on the details of the suggestion; but generally there's a much stronger bond between the kernel and the driver than between the driver and the firmware in my cases.

I call BS.

Let me be very clear. I'm not applying that shit-for-brains stupid patch, and will not be pulling it unless somebody tricks me into it.

Because all these arguments make no sense at all.

If the driver doesn't work without the firmware, then anybody who distributes the driver binary without a firmware is just *fundamentally* doing something insane.

You may do it for *development* purposes, but doing so for actual *use* would be entirely pointless.

See my point? If a distribution is distributing the driver without the firmware, then what the hell is the point of such a thing?

But even if you decide to do that for some odd reason, the patch is still just stupid. Instead of adding some crazy infrastructure for "now I've mounted everything", you could equally well just

(a) make the driver fail the module load if it cannot find a firmware binary

(b) after user space has mounted everything, just do "insmod -a"
again (or insmod just that driver).

See? The point is, this "generic" hacky interface is just stupid. It's not adding any value. If you add user space "I'm ready now" points anyway, you might as well make those points do the right thing and just load the module that is now loadable.

We could mark such "late loading" modules explicitly if people want to, so that you can automate the whole thing about delaying the loading in user space.

At no point does it make sense to say "I have now mounted all the important filesystems". Maybe the firmware is extracted later by user space downloading it from the internet, and the module will then work only after that point"./

This whole "I have mounted important filesystems" is just pure and utter garbage. Stop pushing this shit.

They claim that without it there is the race between /lib/firmware being ready and driver asking for the firmware.

I was told there were quite a bit of out-of-tree hacks to address this without using the usermode helper,

the goal of this patch was to create the discussion needed to a proper resolution to this.

Quoted 9 lines

Quoted 9 lines · Linus Torvalds

Quoted 4 lines

On Tue, Sep 6, 2016 at 10:46 AM, Bjorn Andersson Nobody has actually answered the "why don't we just tie the firmware and module together" question.

The answer to this depends on the details of the suggestion; but generally there's a much stronger bond between the kernel and the driver than between the driver and the firmware in my cases.

Plus all gpu drivers which need firmware. And yes we must load them at probe because people are generally pissed when they boot their machine and the screen goes black. On top of that a lot of people want their gpu drivers to be built-in, but can't ship the firmware blobs in the kernel image because gpl. Yep, there's a bit a contradiction there ...

From this perspective the kernel plays a role similar to a boot server;

Quoted 2 lines · Luis R. Rodriguez

They claim that without it there is the race between /lib/firmware being ready and driver asking for the firmware.

Hope it's understood by now.

Quoted 2 lines

I was told there were quite a bit of out-of-tree hacks to address this without using the usermode helper,

There are:
https://chromium-review.googlesource.com/#/c/354089/
wait until SYSTEM_RUNNING before loading DMC firmware.

Quoted 2 lines

the goal of this patch was to create the discussion needed to a proper resolution to this.

Sincere thanks.

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines · Linus Torvalds

Quoted 4 lines

On Tue, Sep 6, 2016 at 10:46 AM, Bjorn Andersson Nobody has actually answered the "why don't we just tie the firmware and module together" question.

The answer to this depends on the details of the suggestion; but generally there's a much stronger bond between the kernel and the driver than between the driver and the firmware in my cases.

Indeed.

The i915 DMC firmware is an interesting example. First of all it’s _optional_! It’s critical for battery-powered systems but the i915 driver works without it.

Quoted 3 lines · Dan

Plus all gpu drivers which need firmware. And yes we must load them at probe because people are generally pissed when they boot their machine and the screen goes black.

Quoted 4 lines

On top of that a lot of people
want their gpu drivers to be built-in, but can't ship the firmware blobs in the kernel image because gpl. Yep, there's a bit a contradiction there ...

Eppur si muove:
1) As Dan just wrote, users expect the screen to light up as soon as they press the power button so the i915 driver is built-in
2) ... yet they’ll never notice the nanojoules of battery loss caused by the DMC firmware being on a filesystem and loaded a tiny bit later.

SoCs and platforms have become some new kind of distributed systems where other processors run their own, specific software/OS/firmware. From this perspective the kernel plays a role similar to a boot server; and choke point. Granted: booting various and heterogeneous distributed systems doesn’t look like a simple problem to solve generically. Yet at the moment the kernel doesn’t help by not even supporting something as basic as being told when the files it’s (unfortunately) in charge to deploy to other nodes become available and ready to deploy.

It can’t be assumed that the driver and the firmware are two parts of the same software piece whereas they actually run on two different processors, are most likely developed and validated by completely different teams and released on different lifecycles. Especially in the Linux case.

I hope this distributed systems analogy captures the essence of the examples and rationales detailed elsewhere in this thread.

Quoted 3 lines · Bjorn Andersson

Linus, I reversed the order of your questions/answers to fit my answer better.

Nobody has actually answered the "why don't we just tie the firmware

and module together" question.

Really. If the driver doesn't work without the firmware, then why the hell is it separated from it in the first place?

kernel_read_file_from_path() can try to read a file from the system's filesystem. This is typically done for firmware for instance, which lives in /lib/firmware. One issue with this is that the kernel cannot know for sure when the real final /lib/firmare/ is ready, and even if you use initramfs drivers are currently initialized *first* prior to the initramfs kicking off.

During init we run through all init calls first (do_initcalls()) and finally the initramfs is processed via prepare_namespace():

Quoted 7 lines · Luis R. Rodriguez

kernel_read_file_from_path() can try to read a file from the system's filesystem. This is typically done for firmware for instance, which lives in /lib/firmware. One issue with this is that the kernel cannot know for sure when the real final /lib/firmare/ is ready, and even if you use initramfs drivers are currently initialized *first* prior to the initramfs kicking off.

Why?

Quoted 3 lines

During init we run through all init calls first (do_initcalls()) and finally the initramfs is processed via prepare_namespace():

What's the downside of moving initramfs cpio extraction earlier in the boot?

I did some shuffling around of those code to make initmpfs work, does anybody know why initramfs extraction _before_ we initialize drivers would be a bad thing?

Quoted 3 lines · Rob Landley

I did some shuffling around of those code to make initmpfs work, does anybody know why initramfs extraction _before_ we initialize drivers would be a bad thing?

No, but it seems sensible to me, if its done before do_initcalls() that should resolve the race for initramfs users

Quoted 8 lines · Luis R. Rodriguez

Quoted 3 lines · Rob Landley

I did some shuffling around of those code to make initmpfs work, does anybody know why initramfs extraction _before_ we initialize drivers would be a bad thing?

No, but it seems sensible to me, if its done before do_initcalls() that should resolve the race for initramfs users

initramfs should already be set up before drivers are.

Exactly what is it that has trouble right now?

The gating issue for initramfs is that technically the filesystem setup needs to be done, which means that it currently ends up being populated _fairly_ late in the initcall series, but certainly before drivers. But since initramfs really only needs very limited filesystem functionality, I assume Rob had few problems with just moving it earlier.

Still, what kind of ordering issues did people have? What is it that needs to load files even before driver init? Some crazy subsystem?

Quoted 11 lines · Linus Torvalds

Quoted 8 lines · Luis R. Rodriguez

Quoted 3 lines · Rob Landley

I did some shuffling around of those code to make initmpfs work, does anybody know why initramfs extraction _before_ we initialize drivers would be a bad thing?

No, but it seems sensible to me, if its done before do_initcalls() that should resolve the race for initramfs users

initramfs should already be set up before drivers are.

Actually you are right, the issue would only be for old initrd, for initramfs we populate that via rootfs_initcall(populate_rootfs), so as long as drivers in question use an init level beyond rootfs's we're good there.

Quoted 1 line

Exactly what is it that has trouble right now?

It would seem then that the only current stated race possible should then be non-initramfs users.

One example if very large firmware for
remote-proc, whereby an initramfs is just not practical or desirable.

Quoted 9 lines

The gating issue for initramfs is that technically the filesystem setup needs to be done, which means that it currently ends up being populated _fairly_ late in the initcall series, but certainly before drivers. But since initramfs really only needs very limited filesystem functionality, I assume Rob had few problems with just moving it earlier.

Still, what kind of ordering issues did people have? What is it that needs to load files even before driver init? Some crazy subsystem?

No, I think this is just about non-initramfs users now,

if we disregard
old initrd users. Bjorn, Marc, correct me if I'm wrong, as I think its so far you both who have seemed to run into race issues and have then ended up trying to look for hacks to address this race or considered using the usermode helper (which we're trying to minimize users for). Daniel seems to note a lot of video drivers use firmware on probe as well so there's a potential issue for those users if they don't use initramfs.

Quoted 13 lines · Luis R. Rodriguez

Quoted 11 lines · Linus Torvalds

Quoted 8 lines · Luis R. Rodriguez

Quoted 3 lines · Rob Landley

I did some shuffling around of those code to make initmpfs work, does anybody know why initramfs extraction _before_ we initialize drivers would be a bad thing?

No, but it seems sensible to me, if its done before do_initcalls() that should resolve the race for initramfs users

initramfs should already be set up before drivers are.

Quoted 8 lines

Actually you are right, the issue would only be for old initrd, for initramfs we populate that via rootfs_initcall(populate_rootfs), so as long as drivers in question use an init level beyond rootfs's we're good there.

Quoted 1 line

Exactly what is it that has trouble right now?

It would seem then that the only current stated race possible should then be non-initramfs users.

Or:

a) initramfs users that include a driver but do not include the firmware into initramfs

b) driver is built-in but firmware is not in initrafms (Johannes reports this causes driver failure on intel wireless for instance, and I guess you need to reload)

Quoted 2 lines

One example if very large firmware for
remote-proc, whereby an initramfs is just not practical or desirable.

This issue still stands. At Plumbers Johannes Berg did indicate to me he had a simple elegant solution in mind. He suggested that since the usermode helper was available, he had added support to be able to differentiate async firmware request calls form sync requests, and that userspace should not return an error *iff* the request made was async and it can determine we're initramfs. The semantics issue is the same though, is there a generic way to determine we're initramfs ? What if we move multiple levels? Anyway -- provided we could figure this out, userspace would simply yield and wait until the real rootfs is met. Upon pivot_root() the assumption is all previous udev events pending would be re-triggered and finally udev could finally confirm/deny if the firmware was present. This would *also* allow you to stuff your firmware whever, however big it was. This however relied on the userspace firmware loading support, it turns out that (I think because of an incorrect negative backlash back in the day over blaming this over booting issues due to the timeout whereas the real issue was the kmod timeout was affecting our long standing serial init()/probe()) the systemd userspace firmware laoding support was removed from systemd udev in 2014 by Kay via commit be2ea723b1d023b3d ("udev: remove userspace firmware loading support").

Systemd might *still* be able to provide a solution here, however I will note Johannes was asking for *all* async firmware requests to always rely on the kernel syfs UMH fallback -- this suggestion is against the direction we've been taking to eventually compartamentalize the kernel UMH code, so whatever we decide to do, lets please take a breather and seriously address this properly *with* systemd folks.

A side race discussed at Plumbers worth mentioning given its related to the UMH was inspired by Jiri's talk on the abuse of the freezer for kthreads -- and his suggestion to use freeze_super(). Currently the UMH lock is used for the UMH but as I have noted in Daniel Wagner's recent patches to give some love to this code and further compartamentalize the UMH -- the UMH lock was originally added to help avoid drivers use the firmware API on resume, given the races. The firmware cache solution implemented by Ming Lei years ago helped address this, whereby devm helpers are used based on the requested firmware and prior to suspend we cache all required firmware for drivers so that upon resume calls would work without the effective race present. This mitigated the actual races / issues with drivers, but they must not use the firmware API on suspend/resume. Since this solution *kills* all pending UMH caller on suspend obviously this means on suspend using request_firmware*() API and expecting it to work is brutally dumb as we will eventually kill any pending requests. This is a long winded way to say that if you rely on the UMH for firmware you must figure out your own proactive firmware cache solution and must definitely not request firmware on suspend. Two things then:

1) I've been brainstorming with Daniel how to use freeze_super() to replace the now invalid UMH lock -- its purpose only helps races on boot, for the fallback case to the UMH. But note most distributions disable forcing it always on, so these days we *only* rely on the UMH as a fallback if the driver explicitly requested it

2) Drivers relying on the UMH very likely have a broken cache solution if they are doing this on suspend

Whatever the outcome of this discussion is -- Johannes seemed to *want* to further use the UMH by default on *all* async alls... even if the driver did not explicitly requested it -- I'm concerned about this given all the above and the existing flip/flop on systemd for it. Whatever we try to dream up here, please consider all the above as well.

Quoted 7 lines

Quoted 7 lines

The gating issue for initramfs is that technically the filesystem setup needs to be done, which means that it currently ends up being populated _fairly_ late in the initcall series, but certainly before drivers. But since initramfs really only needs very limited filesystem functionality, I assume Rob had few problems with just moving it earlier.

Quoted 4 lines

Quoted 2 lines

Still, what kind of ordering issues did people have? What is it that needs to load files even before driver init? Some crazy subsystem?

No, I think this is just about non-initramfs users now,

And as Johannes pointed two above to cases.

Quoted 7 lines

if we disregard
old initrd users. Bjorn, Marc, correct me if I'm wrong, as I think its so far you both who have seemed to run into race issues and have then ended up trying to look for hacks to address this race or considered using the usermode helper (which we're trying to minimize users for). Daniel seems to note a lot of video drivers use firmware on probe as well so there's a potential issue for those users if they don't use initramfs.

[CC: added Harald]

As Harald pointed out over a beer yesterday evening, there is at least one more reason why UMH isn't obsolete. The ordering of the firmware loading might be of important. Say you want to greet the user with a splash screen really early on, the graphic card firmware should be loaded first. Also the automotive world has this fancy requirement that rear camera must be on the screen within 2 seconds. So controlling the firmware loading order is of importance (e.g. also do not overcommit the I/O bandwith not so important firmwares). A user space helper is able to prioritize the request accordingly the use case.

Quoted 11 lines · Daniel Wagner

[CC: added Harald]

As Harald pointed out over a beer yesterday evening, there is at least one more reason why UMH isn't obsolete. The ordering of the firmware loading might be of important. Say you want to greet the user with a splash screen really early on, the graphic card firmware should be loaded first. Also the automotive world has this fancy requirement that rear camera must be on the screen within 2 seconds. So controlling the firmware loading order is of importance (e.g. also do not overcommit the I/O bandwith not so important firmwares). A user space helper is able to prioritize the request accordingly the use case.

That seems like a valid problem, but I don't think that UMH adequately solves it. Sure, loading firmware in the right order avoids a >2sec delay due to firmware loading, but what happens when you have a slow USB device that *doesn't* need firmware plugged in to your car's shiny USB port when you start the car?

It seems to me that this use case requires explicit control over device probing and, if that gets added, you get your firmware ordering for free (just probe the important devices first).

Quoted 11 lines · Daniel Wagner

[CC: added Harald]

As Harald pointed out over a beer yesterday evening, there is at least one more reason why UMH isn't obsolete. The ordering of the firmware loading might be of important. Say you want to greet the user with a splash screen really early on, the graphic card firmware should be loaded first. Also the automotive world has this fancy requirement that rear camera must be on the screen within 2 seconds. So controlling the firmware loading order is of importance (e.g. also do not overcommit the I/O bandwith not so important firmwares). A user space helper is able to prioritize the request accordingly the use case.

That seems like a valid problem, but I don't think that UMH adequately solves it. Sure, loading firmware in the right order avoids a >2sec delay due to firmware loading, but what happens when you have a slow USB device that *doesn't* need firmware plugged in to your car's shiny USB port when you start the car?

It seems to me that this use case requires explicit control over device probing and, if that gets added, you get your firmware ordering for free (just probe the important devices first).

Whatever the outcome of this discussion is -- Johannes seemed to *want* to further use the UMH by default on *all* async alls... even if the driver did not explicitly requested it -- I'm concerned about this given all the above and the existing flip/flop on systemd for it. Whatever we try to dream up here, please consider all the above as well.

This issue still stands. At Plumbers Johannes Berg did indicate to me he had a simple elegant solution in mind. He suggested that since the usermode helper was available, he had added support to be able to differentiate async firmware request calls form sync requests,

it can determine we're initramfs. The semantics issue is the same though, is there a generic way to determine we're initramfs ? What if we move multiple levels? Anyway -- provided we could figure this out, userspace would simply yield and wait until the real rootfs is met.

Upon pivot_root() the assumption is all previous udev events pending would be re-triggered and finally udev could finally confirm/deny if the firmware was present.

note Johannes was asking for *all* async firmware requests to always rely on the kernel syfs UMH fallback -- this suggestion is against the direction we've been taking to eventually compartamentalize the kernel UMH code, so whatever we decide to do, lets please take a breather and seriously address this properly *with* systemd folks.

My argument basically goes like this:

First, given good drivers (i.e. using request_firmware_nowait()) putting firmware even for a built-in driver into initramfs or not should be a system integrator decision. If they don't need the device that early, it should be possible for them to delay it. Or, perhaps, if the firmware is too big, etc. I'm sure we can all come up with more examples of why you'd want to do it one way or another.

Quoted 8 lines · Johannes Berg

My argument basically goes like this:

First, given good drivers (i.e. using request_firmware_nowait()) putting firmware even for a built-in driver into initramfs or not should be a system integrator decision. If they don't need the device that early, it should be possible for them to delay it. Or, perhaps, if the firmware is too big, etc. I'm sure we can all come up with more examples of why you'd want to do it one way or another.

This is how I understood the the situation, but I never quite bought it. What is wrong with the kernel saying "you must put your module and your firmware together"? Sure, people may want to do things differently, but what is the real blocker?

Fundamentally, it seems to me that if a module needs firmware, it makes no sense to make the module available before the firmware. I'm probably missing something though :)

Quoted 16 lines · Tom Gundersen

Quoted 12 lines · Johannes Berg

My argument basically goes like this:

First, given good drivers (i.e. using request_firmware_nowait()) putting firmware even for a built-in driver into initramfs or not should be a system integrator decision. If they don't need the device
that early, it should be possible for them to delay it. Or, perhaps, if
the firmware is too big, etc. I'm sure we can all come up with more
examples of why you'd want to do it one way or another.

Quoted 6 lines

This is how I understood the the situation, but I never quite bought
it. What is wrong with the kernel saying "you must put your module and
your firmware together"? Sure, people may want to do things differently, but what is the real blocker?

0) Firmware upgrades are possible
1) Some firmware is optional
2) Firmware licenses may often not be GPLv2 compatible
3) Some firmwares may be stupid large (remote-proc) as such neither built-in firmware nor using the firmware in initramfs is reasonable.

The firmware API has had some issues a while ago, some of this is not well documented, and its still hard to grasp. This documents some of these issues, adds SmPL grammar rules to enable us to hunt for issues, and annotations to help us with our effort to finally compartamentalize that pesky usermode helper.

Previously this was just one patch, the grammar rule to help find request firmware API users on init or probe, this series extends that effort with usermode helper grammar rules, and some annotations and documentation on the firmware_class driver to avoid further issues. Documenting the usermode helper and making it clear why we cannot remove it is important for analysis for the next series which adds the new flexible sysdata firmware API.

This series depends on the coccicheck series which enables annotations on coccinelle patches to require a specific version of coccinelle [0], as such coordination with Michal is in order.

This series is also further extended next with the new sydata API, the full set of changes is available on my linux-next tree [1].

Perhaps now a good time to discuss -- if 0-day should enable the rule scripts/coccinelle/api/request_firmware-usermode.cocci to be called on every 0-day iteration, it runs rather fast and it should help police against avoiding futher explicit users of the usermode helper.

[0] https://lkml.kernel.org/r/[email protected]
[1] https://git.kernel.org/cgit/linux/kernel/git/mcgrof/linux-next.git/log/?h=20160616-sysdata-v2

Luis R. Rodriguez (5):
MAINTAINERS: extend firmware_class maintainer list
firmware: annotate thou shalt not request fw on init or probe
firmware: update usermode helper docs and add SmPL report
firmware: add usermode helper DECLARE_FW_LOADER_USER() annotation
firmware: fix fw cache to avoid usermode helper on suspend

 Documentation/firmware_class/README                |  59 +++++++++-
 MAINTAINERS                                        |   1 +
 drivers/base/Kconfig                               |   2 +-
 drivers/base/firmware_class.c                      |   2 +-
 drivers/firmware/dell_rbu.c                        |   1 +
 drivers/leds/leds-lp55xx-common.c                  |   1 +
 include/linux/firmware.h                           |   7 ++
 .../request_firmware-avoid-init-probe-init.cocci   | 130 +++++++++++++++++++++
 .../coccinelle/api/request_firmware-usermode.cocci |  44 +++++++
 9 files changed, 240 insertions(+), 7 deletions(-)
 create mode 100644 scripts/coccinelle/api/request_firmware-avoid-init-probe-init.cocci
 create mode 100644 scripts/coccinelle/api/request_firmware-usermode.cocci

Signature · 1 line

2.8.2

Quoted 14 lines · Luis R. Rodriguez

The firmware API has had some issues a while ago, some of this is not well documented, and its still hard to grasp. This documents some of these issues, adds SmPL grammar rules to enable us to hunt for issues, and annotations to help us with our effort to finally compartamentalize that pesky usermode helper.

Previously this was just one patch, the grammar rule to help find request firmware API users on init or probe, this series extends that effort with usermode helper grammar rules, and some annotations and documentation on the firmware_class driver to avoid further issues. Documenting the usermode helper and making it clear why we cannot remove it is important for analysis for the next series which adds the new flexible sysdata firmware API.

Quoted 4 lines

This series depends on the coccicheck series which enables annotations on coccinelle patches to require a specific version of coccinelle [0], as such coordination with Michal is in order.

Michal is out until July 11, and upon further thought such coordination is not need, the annotation is in place as comments and as such merging this now won't have any negative effects other than the version check. Also the patches in question for the coccicheck change are all acked now and I expect them to be merged anyway.

Which tree should firmware changes go through ?

Quoted 3 lines

This series is also further extended next with the new sydata API, the full set of changes is available on my linux-next tree [1].

Quoted 4 lines

Perhaps now a good time to discuss -- if 0-day should enable the rule scripts/coccinelle/api/request_firmware-usermode.cocci to be called on every 0-day iteration, it runs rather fast and it should help police against avoiding futher explicit users of the usermode helper.

And if we are going to merge this anyone oppose enabling hunting for further explicit users of the usermode helper using grammar through 0-day ?

Quoted 36 lines · Luis R. Rodriguez

Quoted 14 lines · Luis R. Rodriguez

The firmware API has had some issues a while ago, some of this is not well documented, and its still hard to grasp. This documents some of these issues, adds SmPL grammar rules to enable us to hunt for issues, and annotations to help us with our effort to finally compartamentalize that pesky usermode helper.

Previously this was just one patch, the grammar rule to help find request firmware API users on init or probe, this series extends that effort with usermode helper grammar rules, and some annotations and documentation on the firmware_class driver to avoid further issues. Documenting the usermode helper and making it clear why we cannot remove it is important for analysis for the next series which adds the new flexible sysdata firmware API.

Quoted 4 lines

This series depends on the coccicheck series which enables annotations on coccinelle patches to require a specific version of coccinelle [0], as such coordination with Michal is in order.

Michal is out until July 11, and upon further thought such coordination is not need, the annotation is in place as comments and as such merging this now won't have any negative effects other than the version check. Also the patches in question for the coccicheck change are all acked now and I expect them to be merged anyway.

Which tree should firmware changes go through ?

Quoted 7 lines

This series is also further extended next with the new sydata API, the full set of changes is available on my linux-next tree [1].

Perhaps now a good time to discuss -- if 0-day should enable the rule scripts/coccinelle/api/request_firmware-usermode.cocci to be called on every 0-day iteration, it runs rather fast and it should help police against avoiding futher explicit users of the usermode helper.

Quoted 3 lines

And if we are going to merge this anyone oppose enabling hunting for further explicit users of the usermode helper using grammar through 0-day ?

*Poke*

Quoted 37 lines · Luis R. Rodriguez

Quoted 36 lines · Luis R. Rodriguez

Quoted 14 lines · Luis R. Rodriguez

The firmware API has had some issues a while ago, some of this is not well documented, and its still hard to grasp. This documents some of these issues, adds SmPL grammar rules to enable us to hunt for issues, and annotations to help us with our effort to finally compartamentalize that pesky usermode helper.

Previously this was just one patch, the grammar rule to help find request firmware API users on init or probe, this series extends that effort with usermode helper grammar rules, and some annotations and documentation on the firmware_class driver to avoid further issues. Documenting the usermode helper and making it clear why we cannot remove it is important for analysis for the next series which adds the new flexible sysdata firmware API.

Quoted 4 lines

This series depends on the coccicheck series which enables annotations on coccinelle patches to require a specific version of coccinelle [0], as such coordination with Michal is in order.

Michal is out until July 11, and upon further thought such coordination is not need, the annotation is in place as comments and as such merging this now won't have any negative effects other than the version check. Also the patches in question for the coccicheck change are all acked now and I expect them to be merged anyway.

Which tree should firmware changes go through ?

Quoted 7 lines

This series is also further extended next with the new sydata API, the full set of changes is available on my linux-next tree [1].

Perhaps now a good time to discuss -- if 0-day should enable the rule scripts/coccinelle/api/request_firmware-usermode.cocci to be called on every 0-day iteration, it runs rather fast and it should help police against avoiding futher explicit users of the usermode helper.

Quoted 5 lines

Quoted 3 lines

And if we are going to merge this anyone oppose enabling hunting for further explicit users of the usermode helper using grammar through 0-day ?

*Poke*

*Re-poke*

Quoted 44 lines · Luis R. Rodriguez

Quoted 42 lines · Luis R. Rodriguez

Quoted 31 lines · Luis R. Rodriguez

Quoted 14 lines · Luis R. Rodriguez

The firmware API has had some issues a while ago, some of this is not well documented, and its still hard to grasp. This documents some of these issues, adds SmPL grammar rules to enable us to hunt for issues, and annotations to help us with our effort to finally compartamentalize that pesky usermode helper.

Previously this was just one patch, the grammar rule to help find request firmware API users on init or probe, this series extends that effort with usermode helper grammar rules, and some annotations and documentation on the firmware_class driver to avoid further issues. Documenting the usermode helper and making it clear why we cannot remove it is important for analysis for the next series which adds the new flexible sysdata firmware API.

Quoted 4 lines

This series depends on the coccicheck series which enables annotations on coccinelle patches to require a specific version of coccinelle [0], as such coordination with Michal is in order.

Michal is out until July 11, and upon further thought such coordination is not need, the annotation is in place as comments and as such merging this now won't have any negative effects other than the version check. Also the patches in question for the coccicheck change are all acked now and I expect them to be merged anyway.

Which tree should firmware changes go through ?

Quoted 3 lines

This series is also further extended next with the new sydata API, the full set of changes is available on my linux-next tree [1].

Quoted 8 lines

Quoted 4 lines

Perhaps now a good time to discuss -- if 0-day should enable the rule scripts/coccinelle/api/request_firmware-usermode.cocci to be called on every 0-day iteration, it runs rather fast and it should help police against avoiding futher explicit users of the usermode helper.

And if we are going to merge this anyone oppose enabling hunting for further explicit users of the usermode helper using grammar through 0-day ?

*Poke*

Quoted 1 line

*Re-poke*

Re-re-poke.

The scripts/coccicheck changes are now merged on Linus' tree, so these patches have no other pending changes upstream.

Who's tree can this go through or is this too late now?

Quoted 49 lines · Luis R. Rodriguez

Quoted 38 lines · Luis R. Rodriguez

Quoted 37 lines · Luis R. Rodriguez

Quoted 36 lines · Luis R. Rodriguez

Quoted 14 lines · Luis R. Rodriguez

The firmware API has had some issues a while ago, some of this is not well documented, and its still hard to grasp. This documents some of these issues, adds SmPL grammar rules to enable us to hunt for issues, and annotations to help us with our effort to finally compartamentalize that pesky usermode helper.

Previously this was just one patch, the grammar rule to help find request firmware API users on init or probe, this series extends that effort with usermode helper grammar rules, and some annotations and documentation on the firmware_class driver to avoid further issues. Documenting the usermode helper and making it clear why we cannot remove it is important for analysis for the next series which adds the new flexible sysdata firmware API.

Quoted 4 lines

This series depends on the coccicheck series which enables annotations on coccinelle patches to require a specific version of coccinelle [0], as such coordination with Michal is in order.

Michal is out until July 11, and upon further thought such coordination is not need, the annotation is in place as comments and as such merging this now won't have any negative effects other than the version check. Also the patches in question for the coccicheck change are all acked now and I expect them to be merged anyway.

Which tree should firmware changes go through ?

Quoted 7 lines

This series is also further extended next with the new sydata API, the full set of changes is available on my linux-next tree [1].

Perhaps now a good time to discuss -- if 0-day should enable the rule scripts/coccinelle/api/request_firmware-usermode.cocci to be called on every 0-day iteration, it runs rather fast and it should help police against avoiding futher explicit users of the usermode helper.

Quoted 7 lines

Quoted 5 lines

Quoted 3 lines

And if we are going to merge this anyone oppose enabling hunting for further explicit users of the usermode helper using grammar through 0-day ?

*Poke*

*Re-poke*

Re-re-poke.

Quoted 4 lines

The scripts/coccicheck changes are now merged on Linus' tree, so these patches have no other pending changes upstream.

Who's tree can this go through or is this too late now?

It's way too late for 4.8-rc1, it will have to go into a maintainer tree after 4.8-rc1 is out.

Quoted 53 lines · Greg KH

Quoted 52 lines · Luis R. Rodriguez

Quoted 38 lines · Luis R. Rodriguez

Quoted 37 lines · Luis R. Rodriguez

Quoted 36 lines · Luis R. Rodriguez

Quoted 14 lines · Luis R. Rodriguez

The firmware API has had some issues a while ago, some of this is not well documented, and its still hard to grasp. This documents some of these issues, adds SmPL grammar rules to enable us to hunt for issues, and annotations to help us with our effort to finally compartamentalize that pesky usermode helper.

Previously this was just one patch, the grammar rule to help find request firmware API users on init or probe, this series extends that effort with usermode helper grammar rules, and some annotations and documentation on the firmware_class driver to avoid further issues. Documenting the usermode helper and making it clear why we cannot remove it is important for analysis for the next series which adds the new flexible sysdata firmware API.

Quoted 4 lines

This series depends on the coccicheck series which enables annotations on coccinelle patches to require a specific version of coccinelle [0], as such coordination with Michal is in order.

Michal is out until July 11, and upon further thought such coordination is not need, the annotation is in place as comments and as such merging this now won't have any negative effects other than the version check. Also the patches in question for the coccicheck change are all acked now and I expect them to be merged anyway.

Which tree should firmware changes go through ?

Quoted 7 lines

This series is also further extended next with the new sydata API, the full set of changes is available on my linux-next tree [1].

Perhaps now a good time to discuss -- if 0-day should enable the rule scripts/coccinelle/api/request_firmware-usermode.cocci to be called on every 0-day iteration, it runs rather fast and it should help police against avoiding futher explicit users of the usermode helper.

Quoted 7 lines

Quoted 5 lines

Quoted 3 lines

And if we are going to merge this anyone oppose enabling hunting for further explicit users of the usermode helper using grammar through 0-day ?

*Poke*

*Re-poke*

Re-re-poke.

The scripts/coccicheck changes are now merged on Linus' tree, so these patches have no other pending changes upstream.

Quoted 4 lines

Quoted 1 line

Who's tree can this go through or is this too late now?

It's way too late for 4.8-rc1, it will have to go into a maintainer tree after 4.8-rc1 is out.

OK thanks, what maintainer tree should this go through ? firmware_class changes seem to sporadically go through different maintainers, and often some changes go in without much review. Not sure if this is helping.

Quoted 59 lines · Luis R. Rodriguez

Quoted 57 lines · Greg KH

Quoted 45 lines · Luis R. Rodriguez

Quoted 44 lines · Luis R. Rodriguez

Quoted 42 lines · Luis R. Rodriguez

Quoted 31 lines · Luis R. Rodriguez

Quoted 14 lines · Luis R. Rodriguez

The firmware API has had some issues a while ago, some of this is not well documented, and its still hard to grasp. This documents some of these issues, adds SmPL grammar rules to enable us to hunt for issues, and annotations to help us with our effort to finally compartamentalize that pesky usermode helper.

Previously this was just one patch, the grammar rule to help find request firmware API users on init or probe, this series extends that effort with usermode helper grammar rules, and some annotations and documentation on the firmware_class driver to avoid further issues. Documenting the usermode helper and making it clear why we cannot remove it is important for analysis for the next series which adds the new flexible sysdata firmware API.

Quoted 4 lines

This series depends on the coccicheck series which enables annotations on coccinelle patches to require a specific version of coccinelle [0], as such coordination with Michal is in order.

Michal is out until July 11, and upon further thought such coordination is not need, the annotation is in place as comments and as such merging this now won't have any negative effects other than the version check. Also the patches in question for the coccicheck change are all acked now and I expect them to be merged anyway.

Which tree should firmware changes go through ?

Quoted 3 lines

This series is also further extended next with the new sydata API, the full set of changes is available on my linux-next tree [1].

Quoted 8 lines

Quoted 4 lines

Perhaps now a good time to discuss -- if 0-day should enable the rule scripts/coccinelle/api/request_firmware-usermode.cocci to be called on every 0-day iteration, it runs rather fast and it should help police against avoiding futher explicit users of the usermode helper.

And if we are going to merge this anyone oppose enabling hunting for further explicit users of the usermode helper using grammar through 0-day ?

*Poke*

Quoted 8 lines

Quoted 1 line

*Re-poke*

Re-re-poke.

The scripts/coccicheck changes are now merged on Linus' tree, so these patches have no other pending changes upstream.

Who's tree can this go through or is this too late now?

It's way too late for 4.8-rc1, it will have to go into a maintainer tree after 4.8-rc1 is out.

Quoted 3 lines

OK thanks, what maintainer tree should this go through ? firmware_class changes seem to sporadically go through different maintainers, and often some changes go in without much review. Not sure if this is helping.

I usually take them, after they are acked by the firmware maintainer. I haven't looked to see if this series is, if it is, I can easily take them.

But again, I can't do anything until after 4.8-rc1 is out, and then I get to start working through my huge backlog due to a vacation and conference travel I did last month.

Quoted 14 lines · Luis R. Rodriguez

The firmware API has had some issues a while ago, some of this is not well documented, and its still hard to grasp. This documents some of these issues, adds SmPL grammar rules to enable us to hunt for issues, and annotations to help us with our effort to finally compartamentalize that pesky usermode helper.

Previously this was just one patch, the grammar rule to help find request firmware API users on init or probe, this series extends that effort with usermode helper grammar rules, and some annotations and documentation on the firmware_class driver to avoid further issues. Documenting the usermode helper and making it clear why we cannot remove it is important for analysis for the next series which adds the new flexible sysdata firmware API.

Quoted 4 lines

This series depends on the coccicheck series which enables annotations on coccinelle patches to require a specific version of coccinelle [0], as such coordination with Michal is in order.

Michal is out until July 11, and upon further thought such coordination is not need, the annotation is in place as comments and as such merging this now won't have any negative effects other than the version check. Also the patches in question for the coccicheck change are all acked now and I expect them to be merged anyway.

Which tree should firmware changes go through ?

Quoted 3 lines

This series is also further extended next with the new sydata API, the full set of changes is available on my linux-next tree [1].

Quoted 4 lines

Perhaps now a good time to discuss -- if 0-day should enable the rule scripts/coccinelle/api/request_firmware-usermode.cocci to be called on every 0-day iteration, it runs rather fast and it should help police against avoiding futher explicit users of the usermode helper.

And if we are going to merge this anyone oppose enabling hunting for further explicit users of the usermode helper using grammar through 0-day ?

Quoted 28 lines

[0] https://lkml.kernel.org/r/[email protected]
[1] https://git.kernel.org/cgit/linux/kernel/git/mcgrof/linux-next.git/log/?h=20160616-sysdata-v2

Luis R. Rodriguez (5):
MAINTAINERS: extend firmware_class maintainer list
firmware: annotate thou shalt not request fw on init or probe
firmware: update usermode helper docs and add SmPL report
firmware: add usermode helper DECLARE_FW_LOADER_USER() annotation
firmware: fix fw cache to avoid usermode helper on suspend

 Documentation/firmware_class/README                |  59 +++++++++-
 MAINTAINERS                                        |   1 +
 drivers/base/Kconfig                               |   2 +-
 drivers/base/firmware_class.c                      |   2 +-
 drivers/firmware/dell_rbu.c                        |   1 +
 drivers/leds/leds-lp55xx-common.c                  |   1 +
 include/linux/firmware.h                           |   7 ++
 .../request_firmware-avoid-init-probe-init.cocci   | 130 +++++++++++++++++++++
 .../coccinelle/api/request_firmware-usermode.cocci |  44 +++++++
 9 files changed, 240 insertions(+), 7 deletions(-)
 create mode 100644 scripts/coccinelle/api/request_firmware-avoid-init-probe-init.cocci
 create mode 100644 scripts/coccinelle/api/request_firmware-usermode.cocci

--
2.8.2

Signature · 2 lines

Luis Rodriguez, SUSE LINUX GmbH
Maxfeldstrasse 5; D-90409 Nuernberg

Hi Luis,

Quoted 36 lines · Luis R. Rodriguez

Quoted 14 lines · Luis R. Rodriguez

The firmware API has had some issues a while ago, some of this is not well documented, and its still hard to grasp. This documents some of these issues, adds SmPL grammar rules to enable us to hunt for issues, and annotations to help us with our effort to finally compartamentalize that pesky usermode helper.

Previously this was just one patch, the grammar rule to help find request firmware API users on init or probe, this series extends that effort with usermode helper grammar rules, and some annotations and documentation on the firmware_class driver to avoid further issues. Documenting the usermode helper and making it clear why we cannot remove it is important for analysis for the next series which adds the new flexible sysdata firmware API.

Quoted 4 lines

This series depends on the coccicheck series which enables annotations on coccinelle patches to require a specific version of coccinelle [0], as such coordination with Michal is in order.

Michal is out until July 11, and upon further thought such coordination is not need, the annotation is in place as comments and as such merging this now won't have any negative effects other than the version check. Also the patches in question for the coccicheck change are all acked now and I expect them to be merged anyway.

Which tree should firmware changes go through ?

Quoted 7 lines

This series is also further extended next with the new sydata API, the full set of changes is available on my linux-next tree [1].

Perhaps now a good time to discuss -- if 0-day should enable the rule scripts/coccinelle/api/request_firmware-usermode.cocci to be called on every 0-day iteration, it runs rather fast and it should help police against avoiding futher explicit users of the usermode helper.

Quoted 3 lines

And if we are going to merge this anyone oppose enabling hunting for further explicit users of the usermode helper using grammar through 0-day ?

When *.cocci scripts lands upstream they'll be auto picked up by the 0-day bot to guard new patches/commits.

Are there further steps 0-day should do for request_firmware-upstream.cocci?

Quoted 43 lines · Fengguang Wu

Hi Luis,

Quoted 39 lines · Luis R. Rodriguez

Quoted 14 lines · Luis R. Rodriguez

The firmware API has had some issues a while ago, some of this is not well documented, and its still hard to grasp. This documents some of these issues, adds SmPL grammar rules to enable us to hunt for issues, and annotations to help us with our effort to finally compartamentalize that pesky usermode helper.

Previously this was just one patch, the grammar rule to help find request firmware API users on init or probe, this series extends that effort with usermode helper grammar rules, and some annotations and documentation on the firmware_class driver to avoid further issues. Documenting the usermode helper and making it clear why we cannot remove it is important for analysis for the next series which adds the new flexible sysdata firmware API.

Quoted 4 lines

This series depends on the coccicheck series which enables annotations on coccinelle patches to require a specific version of coccinelle [0], as such coordination with Michal is in order.

Michal is out until July 11, and upon further thought such coordination is not need, the annotation is in place as comments and as such merging this now won't have any negative effects other than the version check. Also the patches in question for the coccicheck change are all acked now and I expect them to be merged anyway.

Which tree should firmware changes go through ?

Quoted 7 lines

This series is also further extended next with the new sydata API, the full set of changes is available on my linux-next tree [1].

Perhaps now a good time to discuss -- if 0-day should enable the rule scripts/coccinelle/api/request_firmware-usermode.cocci to be called on every 0-day iteration, it runs rather fast and it should help police against avoiding futher explicit users of the usermode helper.

And if we are going to merge this anyone oppose enabling hunting for further explicit users of the usermode helper using grammar through 0-day ?

Quoted 2 lines

When *.cocci scripts lands upstream they'll be auto picked up by the 0-day bot to guard new patches/commits.

Great thanks!

Quoted 1 line

Are there further steps 0-day should do for request_firmware-upstream.cocci?

It just requires coccinelle >= 1.0.5.

Quoted 47 lines · Luis R. Rodriguez

Quoted 45 lines · Fengguang Wu

Hi Luis,

Quoted 31 lines · Luis R. Rodriguez

Quoted 14 lines · Luis R. Rodriguez

The firmware API has had some issues a while ago, some of this is not well documented, and its still hard to grasp. This documents some of these issues, adds SmPL grammar rules to enable us to hunt for issues, and annotations to help us with our effort to finally compartamentalize that pesky usermode helper.

Previously this was just one patch, the grammar rule to help find request firmware API users on init or probe, this series extends that effort with usermode helper grammar rules, and some annotations and documentation on the firmware_class driver to avoid further issues. Documenting the usermode helper and making it clear why we cannot remove it is important for analysis for the next series which adds the new flexible sysdata firmware API.

Quoted 4 lines

This series depends on the coccicheck series which enables annotations on coccinelle patches to require a specific version of coccinelle [0], as such coordination with Michal is in order.

Michal is out until July 11, and upon further thought such coordination is not need, the annotation is in place as comments and as such merging this now won't have any negative effects other than the version check. Also the patches in question for the coccicheck change are all acked now and I expect them to be merged anyway.

Which tree should firmware changes go through ?

Quoted 3 lines

This series is also further extended next with the new sydata API, the full set of changes is available on my linux-next tree [1].

Quoted 8 lines

Quoted 4 lines

Perhaps now a good time to discuss -- if 0-day should enable the rule scripts/coccinelle/api/request_firmware-usermode.cocci to be called on every 0-day iteration, it runs rather fast and it should help police against avoiding futher explicit users of the usermode helper.

And if we are going to merge this anyone oppose enabling hunting for further explicit users of the usermode helper using grammar through 0-day ?

When *.cocci scripts lands upstream they'll be auto picked up by the 0-day bot to guard new patches/commits.

Quoted 5 lines

Great thanks!

Quoted 1 line

Are there further steps 0-day should do for request_firmware-upstream.cocci?

It just requires coccinelle >= 1.0.5.

That looks easy.

When do you estimate the script will land upstream?

So we can make sure upgrade coccinelle before that time.

Quoted 54 lines · Fengguang Wu

Quoted 52 lines · Luis R. Rodriguez

Quoted 39 lines · Fengguang Wu

Hi Luis,

Quoted 36 lines · Luis R. Rodriguez

Quoted 14 lines · Luis R. Rodriguez

The firmware API has had some issues a while ago, some of this is not well documented, and its still hard to grasp. This documents some of these issues, adds SmPL grammar rules to enable us to hunt for issues, and annotations to help us with our effort to finally compartamentalize that pesky usermode helper.

Previously this was just one patch, the grammar rule to help find request firmware API users on init or probe, this series extends that effort with usermode helper grammar rules, and some annotations and documentation on the firmware_class driver to avoid further issues. Documenting the usermode helper and making it clear why we cannot remove it is important for analysis for the next series which adds the new flexible sysdata firmware API.

Quoted 4 lines

This series depends on the coccicheck series which enables annotations on coccinelle patches to require a specific version of coccinelle [0], as such coordination with Michal is in order.

Michal is out until July 11, and upon further thought such coordination is not need, the annotation is in place as comments and as such merging this now won't have any negative effects other than the version check. Also the patches in question for the coccicheck change are all acked now and I expect them to be merged anyway.

Which tree should firmware changes go through ?

Quoted 7 lines

This series is also further extended next with the new sydata API, the full set of changes is available on my linux-next tree [1].

Perhaps now a good time to discuss -- if 0-day should enable the rule scripts/coccinelle/api/request_firmware-usermode.cocci to be called on every 0-day iteration, it runs rather fast and it should help police against avoiding futher explicit users of the usermode helper.

Quoted 6 lines

Quoted 3 lines

And if we are going to merge this anyone oppose enabling hunting for further explicit users of the usermode helper using grammar through 0-day ?

When *.cocci scripts lands upstream they'll be auto picked up by the 0-day bot to guard new patches/commits.

Great thanks!

Quoted 1 line

Are there further steps 0-day should do for request_firmware-upstream.cocci?

It just requires coccinelle >= 1.0.5.

Quoted 1 line

That looks easy.

Nice!

Quoted 1 line

When do you estimate the script will land upstream?

Well, this series has gone by a while now without any complaints, so I was poking to see if they can be merged now.

Quoted 1 line

So we can make sure upgrade coccinelle before that time.

There is another series which modernizes coccicheck [0] for which I just poked at as a well [1], one change which may be of importance to you is groks the
Requires: tag on top of an SmPL patch, with that we simply just skip an SmPL patch if the version of coccinelle is older than the one specified, with that in place you can just upgrade when you want -- you'd just gain support for more SmPL patches when you do. Without that coccinelle would not work (fail) on the SmPL patch when tried. For this reason I originally had suggested perhaps this series should be carried by Michal.

[0] https://lkml.kernel.org/r/[email protected]
[1] https://lkml.kernel.org/r/[email protected]

---
 Documentation/firmware_class/README                |  20 ++++
 drivers/base/Kconfig                               |   2 +-
 .../request_firmware-avoid-init-probe-init.cocci   | 130 +++++++++++++++++++++
 3 files changed, 151 insertions(+), 1 deletion(-)
 create mode 100644 scripts/coccinelle/api/request_firmware-avoid-init-probe-init.cocci

Documentation/firmware_class/README +0 -0

index cafdca8b3b15..056d1cb9d365 100644
@@ -93,6 +93,26 @@
    user contexts to request firmware asynchronously, but can't be called
    in atomic contexts.
 

Documentation/firmware_class/README +10 -0

+initramfs or consider using CONFIG_EXTRA_FIRMWARE. Using initramfs is much
+more portable to more distributions as not all distributions wish to enable
+CONFIG_EXTRA_FIRMWARE. Should a driver require the firmware being built-in
+it should depend on CONFIG_EXTRA_FIRMWARE. There is no current annotation for
+requiring a firmware on initramfs.
+
+If you're a maintainer you can help police this with:
+
+$ export COCCI=scripts/coccinelle/api/request_firmware-avoid-init-probe-init.cocci
+$ make coccicheck MODE=report
 
  about in-kernel persistence:
  ---------------------------

On Tue, Aug 23, 2016 at 05:45:04PM -0700, [email protected] wrote:

[snip]

Quoted 15 lines

---
 Documentation/firmware_class/README                |  20 ++++
 drivers/base/Kconfig                               |   2 +-
 .../request_firmware-avoid-init-probe-init.cocci   | 130 +++++++++++++++++++++
 3 files changed, 151 insertions(+), 1 deletion(-)
 create mode 100644 scripts/coccinelle/api/request_firmware-avoid-init-probe-init.cocci

Documentation/firmware_class/README +0 -0

index cafdca8b3b15..056d1cb9d365 100644
@@ -93,6 +93,26 @@
    user contexts to request firmware asynchronously, but can't be called
    in atomic contexts.
 

Documentation/firmware_class/README line 105

Quoted 10 lines

Documentation/firmware_class/README +10 -0

+Requirements:
+=============
+
+You should avoid at all costs requesting firmware on both init and probe paths
+of your device driver. Reason for this is the complexity needed to ensure a
+firmware will be available for a driver early in boot through different
+build configurations. Consider built-in drivers needing firmware early, or
+consider a driver assuming it will only get firmware after pivot_root().
+
+Drivers that really need firmware early should use stuff the firmware in

Minor grammatical nit: s/use//

drivers/base/firmware_class.c +0 -0

index 22d1760a4278..dca4f9cbf4db 100644

Hi Luis,

Quoted 6 lines · On 08/24/2016 02:45 AM, [email protected] wrote:

drivers/base/firmware_class.c +0 -0

index 22d1760a4278..dca4f9cbf4db 100644

drivers/base/firmware_class.c static int cache_firmware(const char *fw_name) · line 1418

Quoted 8 lines

drivers/base/firmware_class.c +1 -1

@@ -1412,7 +1412,7 @@ static int cache_firmware(const char *fw_name)
 
 	pr_debug("%s: %s\n", __func__, fw_name);
 
-	ret = request_firmware(&fw, fw_name, NULL);
+	ret = request_firmware_direct(&fw, fw_name, NULL);
 	if (!ret)
 		kfree(fw);
 

While you are at it you should kill this here as well:

(quoted diff) +0 -12

@@ -1622,7 +1637,6 @@ static void __device_uncache_fw_images(void)
 static void device_cache_fw_images(void)
 {
 	struct firmware_cache *fwc = &fw_cache;
-	int old_timeout;
 	DEFINE_WAIT(wait);
 
 	pr_debug("%s\n", __func__);

@@ -1630,17 +1644,6 @@ static void device_cache_fw_images(void)
 	/* cancel uncache work */
 	cancel_delayed_work_sync(&fwc->work);
 
-	/*
-	 * use small loading timeout for caching devices' firmware
-	 * because all these firmware images have been loaded
-	 * successfully at lease once, also system is ready for
-	 * completing firmware loading now. The maximum size of
-	 * firmware in current distributions is about 2M bytes,
-	 * so 10 secs should be enough.
-	 */
-	old_timeout = loading_timeout;
-	loading_timeout = 10;
-
 	mutex_lock(&fw_lock);
 	fwc->state = FW_LOADER_START_CACHE;
 	dpm_for_each_dev(NULL, dev_cache_fw_image);

I've been reviewing changes proactively, and plan on doing more of this work. I'm doing this early as I should be getting e-mailed about proposed changes.

Signed-off-by Luis R. Rodriguez <[email protected]>

---
 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

MAINTAINERS +0 -0

index fcb387135242..af64eb999ef4 100644

Quoted 15 lines · Luis R. Rodriguez

I've been reviewing changes proactively, and plan on doing more of this work. I'm doing this early as I should be getting e-mailed about proposed changes.

Signed-off-by Luis R. Rodriguez <[email protected]>

---
 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

MAINTAINERS +0 -0

index fcb387135242..af64eb999ef4 100644
@@ -4891,6 +4891,7 @@ F:	tools/firewire/
 

MAINTAINERS F: tools/firewire/ · line 4894

Quoted 3 lines

MAINTAINERS +1 -0

 FIRMWARE LOADER (request_firmware)
 M:	Ming Lei <[email protected]>
+M:	Luis R. Rodriguez <[email protected]>

What does Ming think of this? :)

Quoted 15 lines · Luis R. Rodriguez

I've been reviewing changes proactively, and plan on doing more of this work. I'm doing this early as I should be getting e-mailed about proposed changes.

Signed-off-by Luis R. Rodriguez <[email protected]>

---
 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

MAINTAINERS +0 -0

index fcb387135242..af64eb999ef4 100644
@@ -4891,6 +4891,7 @@ F:      tools/firewire/
 

MAINTAINERS F: tools/firewire/ · line 4894

Quoted 3 lines

MAINTAINERS +1 -0

 FIRMWARE LOADER (request_firmware)
 M:   Ming Lei <[email protected]>
+M:   Luis R. Rodriguez <[email protected]>

What does Ming think of this? :)