Re: [PATCH v3 2/5] firmware: annotate thou shalt not request fw on init or probe

[Luis R. Rodriguez]

On Wed, Aug 24, 2016 at 10:17:38AM +0200, Gabriel Paubert wrote:
> On Tue, Aug 23, 2016 at 05:45:04PM -0700, mcg...@kernel.org wrote:
> 
> [snip]
> > ---
> >  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
> > 
> > diff --git a/Documentation/firmware_class/README 
> > b/Documentation/firmware_class/README
> > index cafdca8b3b15..056d1cb9d365 100644
> > --- a/Documentation/firmware_class/README
> > +++ b/Documentation/firmware_class/README
> > @@ -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
> 
> Minor grammatical nit: s/use//

Fixed, thanks.

  Luis
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 2/5] firmware: annotate thou shalt not request fw on init or probe

[mcgrof]

From: "Luis R. Rodriguez" 

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@ergon ~/linux-next (git::20160609)$ export 
COCCI=scripts/coccinelle/api/request_firmware.cocci
mcgrof@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-mcg...@do-not-panic.com

Cc: Alessandro Rubini 
Cc: Kevin Cernekee 
Cc: Daniel Vetter 
Cc: Mimi Zohar 
Cc: David Woodhouse 
Cc: Kees Cook 
Cc: Dmitry Torokhov 
Cc: Ming Lei 
Cc: Jonathan Corbet 
Cc: Julia Lawall 
Cc: Gilles Muller 
Cc: Nicolas Palix 
Cc: Thierry Martinez 
Cc: Michal Marek 
Cc: co...@systeme.lip6.fr
Cc: Alessandro Rubini 
Cc: Kevin Cernekee 
Cc: Greg Kroah-Hartman 
Cc: Jiri Slaby 
Cc: linux-ser...@vger.kernel.org
Cc: linux-doc@vger.kernel.org
Cc: linux-ser...@vger.kernel.org
Cc: linuxppc-...@lists.ozlabs.org
Signed-off-by: Luis R. Rodriguez 
---
 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

diff --git a/Documentation/firmware_class/README 
b/Documentation/firmware_class/README
index cafdca8b3b15..056d1cb9d365 100644
--- a/Documentation/firmware_class/README
+++ b/Documentation/firmware_class/README
@@ -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