[PATCH v2 4/5] firmware: add SmPL report for custom fallback mechanism

[PATCH v2 4/5] firmware: add SmPL report for custom fallback mechanism

[Luis R. Rodriguez]

Even though most distributions today disable the fallback mechanism
by default we've determined that we cannot remove them from the kernel.
This is not well understood so document the reason and logic behind that.

Recent discussions suggest some future userspace development prospects which
may enable fallback mechanisms to become more useful while avoiding some
historical issues. These discussions have made it clear though that there
is less value to the custom fallback mechanism and an alternative can be
provided in the future. Its also clear that some old users of the custom
fallback mechanism were using it as a copy and paste error. Because of
all this add a Coccinelle SmPL patch to help maintainers police for new
incorrect users of the custom fallback mechanism.

Best we can do for now then is police for new users of the custom
fallback mechanism and and fix incorrect users when they are spotted.
Drivers can only be transitioned out of the custom fallback mechanism
once we know old userspace cannot be not be broken by a kernel change.

The current SmPL patch reports:

$ export COCCI=scripts/coccinelle/api/request_firmware-custom-fallback.cocci
$ make coccicheck MODE=report

drivers/leds/leds-lp55xx-common.c:227:8-31: WARNING: please check if driver really needs a custom fallback mechanism
drivers/firmware/dell_rbu.c:622:17-40: WARNING: please check if driver really needs a custom fallback mechanism

Cc: Richard Purdie <rpurdie@rpsys.net>
Cc: Jacek Anaszewski <j.anaszewski@samsung.com>
Cc: linux-leds@vger.kernel.org
Cc: Abhay Salunke <Abhay_Salunke@dell.com>
Acked-by: Julia.Lawall@lip6.fr
Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
 .../driver-api/firmware/fallback-mechanisms.rst    | 17 ++++++++++
 .../api/request_firmware-custom-fallback.cocci     | 37 ++++++++++++++++++++++
 2 files changed, 54 insertions(+)
 create mode 100644 scripts/coccinelle/api/request_firmware-custom-fallback.cocci

diff --git a/Documentation/driver-api/firmware/fallback-mechanisms.rst b/Documentation/driver-api/firmware/fallback-mechanisms.rst
index d19354794e67..b87a292153c6 100644
--- a/Documentation/driver-api/firmware/fallback-mechanisms.rst
+++ b/Documentation/driver-api/firmware/fallback-mechanisms.rst
@@ -28,6 +28,12 @@ CONFIG_FW_LOADER_USER_HELPER_FALLBACK=n
 the kobject uevent fallback mechanism will never take effect even
 for request_firmware_nowait() when uevent is set to true.
 
+Although the fallback mechanisms are not used widely today they cannot be
+removed from the kernel since some old userspace may exist which could
+entirely depend on the fallback mechanism enabled with the kernel config option
+CONFIG_FW_LOADER_USER_HELPER_FALLBACK. In the future though drivers may opt
+to embrace a different API which provides alternative fallback mechanisms.
+
 Justifying the firmware fallback mechanism
 ==========================================
 
@@ -176,6 +182,17 @@ but you want to suppress kobject uevents, as you have a custom solution which
 will monitor for your device addition into the device hierarchy somehow and
 load firmware for you through a custom path.
 
+The custom fallback mechanism can often be enabled by mistake. We currently
+have only 2 users of it, and little justification to enable it for other users.
+Since it is a common driver developer mistake to enable it, help police for
+new users of the custom fallback mechanism with::
+
+        $ export COCCI=scripts/coccinelle/api/request_firmware-avoid-init-probe-init.cocci
+        $ make coccicheck MODE=report
+
+Drivers can only be transitioned out of the custom fallback mechanism
+once we know old userspace cannot be not be broken by a kernel change.
+
 Firmware fallback timeout
 =========================
 
diff --git a/scripts/coccinelle/api/request_firmware-custom-fallback.cocci b/scripts/coccinelle/api/request_firmware-custom-fallback.cocci
new file mode 100644
index 000000000000..c7598cfc4780
--- /dev/null
+++ b/scripts/coccinelle/api/request_firmware-custom-fallback.cocci
@@ -0,0 +1,37 @@
+// Avoid the firmware custom fallback mechanism at all costs
+//
+// request_firmware_nowait() API enables explicit request for use of the custom
+// fallback mechanism if firmware is not found. Chances are high its use is
+// just a copy and paste bug. Before you fix the driver be sure to *verify* no
+// custom firmware loading tool exists that would otherwise break if we replace
+// the driver to use the uevent fallback mechanism.
+//
+// Confidence: High
+//
+// Reason for low confidence:
+//
+// Copyright: (C) 2016 Luis R. Rodriguez <mcgrof@kernel.org> GPLv2.
+//
+// Options: --include-headers
+
+virtual report
+virtual context
+
+@ r1 depends on report || context @
+expression mod, name, dev, gfp, drv, cb;
+position p;
+@@
+
+(
+*request_firmware_nowait@p(mod, false, name, dev, gfp, drv, cb)
+|
+*request_firmware_nowait@p(mod, 0, name, dev, gfp, drv, cb)
+|
+*request_firmware_nowait@p(mod, FW_ACTION_NOHOTPLUG, name, dev, gfp, drv, cb)
+)
+
+@script:python depends on report@
+p << r1.p;
+@@
+
+coccilib.report.print_report(p[0], "WARNING: please check if driver really needs a custom fallback mechanism")
-- 
2.10.1

[PATCH v2 5/5] firmware: add DECLARE_FW_CUSTOM_FALLBACK() annotation

[Luis R. Rodriguez]

We need to ensure that when driver developers use the custom firmware
fallback mechanism it was not a copy and paste bug. These use cases on
upstream drivers are rare, we only have 2 upstream users and its for
really old drivers. Since valid uses are rare but possible enable a
white-list for its use, and use this same white-list annotation to refer
to the documentation covering the custom use case.

New faulty users can be reported via 0-day now.

Cc: Fengguang Wu <fengguang.wu@intel.com>
Cc: Richard Purdie <rpurdie@rpsys.net>
Cc: Jacek Anaszewski <j.anaszewski@samsung.com>
Cc: linux-leds@vger.kernel.org
Cc: Abhay Salunke <Abhay_Salunke@dell.com>
Acked-by: Jacek Anaszewski <j.anaszewski@samsung.com>
Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
 Documentation/driver-api/firmware/fallback-mechanisms.rst     | 7 +++++--
 drivers/firmware/dell_rbu.c                                   | 1 +
 drivers/leds/leds-lp55xx-common.c                             | 1 +
 include/linux/firmware.h                                      | 7 +++++++
 scripts/coccinelle/api/request_firmware-custom-fallback.cocci | 9 ++++++++-
 5 files changed, 22 insertions(+), 3 deletions(-)

diff --git a/Documentation/driver-api/firmware/fallback-mechanisms.rst b/Documentation/driver-api/firmware/fallback-mechanisms.rst
index b87a292153c6..73f509a8d2de 100644
--- a/Documentation/driver-api/firmware/fallback-mechanisms.rst
+++ b/Documentation/driver-api/firmware/fallback-mechanisms.rst
@@ -184,8 +184,11 @@ load firmware for you through a custom path.
 
 The custom fallback mechanism can often be enabled by mistake. We currently
 have only 2 users of it, and little justification to enable it for other users.
-Since it is a common driver developer mistake to enable it, help police for
-new users of the custom fallback mechanism with::
+Since it is a common driver developer mistake to enable it, driver developers
+should use DECLARE_FW_CUSTOM_FALLBACK() to both white-list and validate their
+use and also refer to the documentation for the custom loading solution.
+
+Invalid users of the custom fallback mechanism can be policed using::
 
         $ export COCCI=scripts/coccinelle/api/request_firmware-avoid-init-probe-init.cocci
         $ make coccicheck MODE=report
diff --git a/drivers/firmware/dell_rbu.c b/drivers/firmware/dell_rbu.c
index 2f452f1f7c8a..3f2aa35bc54d 100644
--- a/drivers/firmware/dell_rbu.c
+++ b/drivers/firmware/dell_rbu.c
@@ -586,6 +586,7 @@ static ssize_t read_rbu_image_type(struct file *filp, struct kobject *kobj,
 	return size;
 }
 
+DECLARE_FW_CUSTOM_FALLBACK("Documentation/dell_rbu.txt");
 static ssize_t write_rbu_image_type(struct file *filp, struct kobject *kobj,
 				    struct bin_attribute *bin_attr,
 				    char *buffer, loff_t pos, size_t count)
diff --git a/drivers/leds/leds-lp55xx-common.c b/drivers/leds/leds-lp55xx-common.c
index 5377f22ff994..04161428ee3b 100644
--- a/drivers/leds/leds-lp55xx-common.c
+++ b/drivers/leds/leds-lp55xx-common.c
@@ -219,6 +219,7 @@ static void lp55xx_firmware_loaded(const struct firmware *fw, void *context)
 	release_firmware(chip->fw);
 }
 
+DECLARE_FW_CUSTOM_FALLBACK("Documentation/leds/leds-lp55xx.txt");
 static int lp55xx_request_firmware(struct lp55xx_chip *chip)
 {
 	const char *name = chip->cl->name;
diff --git a/include/linux/firmware.h b/include/linux/firmware.h
index b1f9f0ccb8ac..e6ca19c03dcc 100644
--- a/include/linux/firmware.h
+++ b/include/linux/firmware.h
@@ -8,6 +8,13 @@
 #define FW_ACTION_NOHOTPLUG 0
 #define FW_ACTION_HOTPLUG 1
 
+/*
+ * Helper for scripts/coccinelle/api/request_firmware-custom-fallback.cocci
+ * and so users can also easily search for the documentation for the
+ * respectively needed custom fallback mechanism.
+ */
+#define DECLARE_FW_CUSTOM_FALLBACK(__usermode_helper)
+
 struct firmware {
 	size_t size;
 	const u8 *data;
diff --git a/scripts/coccinelle/api/request_firmware-custom-fallback.cocci b/scripts/coccinelle/api/request_firmware-custom-fallback.cocci
index c7598cfc4780..68cacab35b76 100644
--- a/scripts/coccinelle/api/request_firmware-custom-fallback.cocci
+++ b/scripts/coccinelle/api/request_firmware-custom-fallback.cocci
@@ -17,6 +17,13 @@
 virtual report
 virtual context
 
+@ r0 depends on report || context @
+declarer name DECLARE_FW_CUSTOM_FALLBACK;
+expression E;
+@@
+
+DECLARE_FW_CUSTOM_FALLBACK(E);
+
 @ r1 depends on report || context @
 expression mod, name, dev, gfp, drv, cb;
 position p;
@@ -30,7 +37,7 @@ position p;
 *request_firmware_nowait@p(mod, FW_ACTION_NOHOTPLUG, name, dev, gfp, drv, cb)
 )
 
-@script:python depends on report@
+@script:python depends on report && !r0 @
 p << r1.p;
 @@
 
-- 
2.10.1

Re: [PATCH v2 5/5] firmware: add DECLARE_FW_CUSTOM_FALLBACK() annotation

[Julia Lawall]

On Fri, 16 Dec 2016, Luis R. Rodriguez wrote:

> We need to ensure that when driver developers use the custom firmware
> fallback mechanism it was not a copy and paste bug. These use cases on
> upstream drivers are rare, we only have 2 upstream users and its for
> really old drivers. Since valid uses are rare but possible enable a
> white-list for its use, and use this same white-list annotation to refer
> to the documentation covering the custom use case.
>
> New faulty users can be reported via 0-day now.
>
> Cc: Fengguang Wu <fengguang.wu@intel.com>
> Cc: Richard Purdie <rpurdie@rpsys.net>
> Cc: Jacek Anaszewski <j.anaszewski@samsung.com>
> Cc: linux-leds@vger.kernel.org
> Cc: Abhay Salunke <Abhay_Salunke@dell.com>
> Acked-by: Jacek Anaszewski <j.anaszewski@samsung.com>
> Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
> ---
>  Documentation/driver-api/firmware/fallback-mechanisms.rst     | 7 +++++--
>  drivers/firmware/dell_rbu.c                                   | 1 +
>  drivers/leds/leds-lp55xx-common.c                             | 1 +
>  include/linux/firmware.h                                      | 7 +++++++
>  scripts/coccinelle/api/request_firmware-custom-fallback.cocci | 9 ++++++++-
>  5 files changed, 22 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/driver-api/firmware/fallback-mechanisms.rst b/Documentation/driver-api/firmware/fallback-mechanisms.rst
> index b87a292153c6..73f509a8d2de 100644
> --- a/Documentation/driver-api/firmware/fallback-mechanisms.rst
> +++ b/Documentation/driver-api/firmware/fallback-mechanisms.rst
> @@ -184,8 +184,11 @@ load firmware for you through a custom path.
>
>  The custom fallback mechanism can often be enabled by mistake. We currently
>  have only 2 users of it, and little justification to enable it for other users.
> -Since it is a common driver developer mistake to enable it, help police for
> -new users of the custom fallback mechanism with::
> +Since it is a common driver developer mistake to enable it, driver developers
> +should use DECLARE_FW_CUSTOM_FALLBACK() to both white-list and validate their
> +use and also refer to the documentation for the custom loading solution.
> +
> +Invalid users of the custom fallback mechanism can be policed using::
>
>          $ export COCCI=scripts/coccinelle/api/request_firmware-avoid-init-probe-init.cocci
>          $ make coccicheck MODE=report
> diff --git a/drivers/firmware/dell_rbu.c b/drivers/firmware/dell_rbu.c
> index 2f452f1f7c8a..3f2aa35bc54d 100644
> --- a/drivers/firmware/dell_rbu.c
> +++ b/drivers/firmware/dell_rbu.c
> @@ -586,6 +586,7 @@ static ssize_t read_rbu_image_type(struct file *filp, struct kobject *kobj,
>  	return size;
>  }
>
> +DECLARE_FW_CUSTOM_FALLBACK("Documentation/dell_rbu.txt");
>  static ssize_t write_rbu_image_type(struct file *filp, struct kobject *kobj,
>  				    struct bin_attribute *bin_attr,
>  				    char *buffer, loff_t pos, size_t count)
> diff --git a/drivers/leds/leds-lp55xx-common.c b/drivers/leds/leds-lp55xx-common.c
> index 5377f22ff994..04161428ee3b 100644
> --- a/drivers/leds/leds-lp55xx-common.c
> +++ b/drivers/leds/leds-lp55xx-common.c
> @@ -219,6 +219,7 @@ static void lp55xx_firmware_loaded(const struct firmware *fw, void *context)
>  	release_firmware(chip->fw);
>  }
>
> +DECLARE_FW_CUSTOM_FALLBACK("Documentation/leds/leds-lp55xx.txt");
>  static int lp55xx_request_firmware(struct lp55xx_chip *chip)
>  {
>  	const char *name = chip->cl->name;
> diff --git a/include/linux/firmware.h b/include/linux/firmware.h
> index b1f9f0ccb8ac..e6ca19c03dcc 100644
> --- a/include/linux/firmware.h
> +++ b/include/linux/firmware.h
> @@ -8,6 +8,13 @@
>  #define FW_ACTION_NOHOTPLUG 0
>  #define FW_ACTION_HOTPLUG 1
>
> +/*
> + * Helper for scripts/coccinelle/api/request_firmware-custom-fallback.cocci
> + * and so users can also easily search for the documentation for the
> + * respectively needed custom fallback mechanism.
> + */
> +#define DECLARE_FW_CUSTOM_FALLBACK(__usermode_helper)
> +
>  struct firmware {
>  	size_t size;
>  	const u8 *data;
> diff --git a/scripts/coccinelle/api/request_firmware-custom-fallback.cocci b/scripts/coccinelle/api/request_firmware-custom-fallback.cocci
> index c7598cfc4780..68cacab35b76 100644
> --- a/scripts/coccinelle/api/request_firmware-custom-fallback.cocci
> +++ b/scripts/coccinelle/api/request_firmware-custom-fallback.cocci
> @@ -17,6 +17,13 @@
>  virtual report
>  virtual context
>
> +@ r0 depends on report || context @
> +declarer name DECLARE_FW_CUSTOM_FALLBACK;
> +expression E;
> +@@
> +
> +DECLARE_FW_CUSTOM_FALLBACK(E);
> +
>  @ r1 depends on report || context @
>  expression mod, name, dev, gfp, drv, cb;
>  position p;
> @@ -30,7 +37,7 @@ position p;
>  *request_firmware_nowait@p(mod, FW_ACTION_NOHOTPLUG, name, dev, gfp, drv, cb)

It looks suspicious that your added a new rule r0, that the python rule
below depends on r0 failing, and that the rule with the * (context mode)
does not depend on r0 in any way.

julia

>  )
>
> -@script:python depends on report@
> +@script:python depends on report && !r0 @
>  p << r1.p;
>  @@
>
> --
> 2.10.1
>
>

Re: [PATCH v2 5/5] firmware: add DECLARE_FW_CUSTOM_FALLBACK() annotation

[Luis R. Rodriguez]

On Mon, Dec 19, 2016 at 11:23:17AM +0100, Julia Lawall wrote:
> 
> 
> On Fri, 16 Dec 2016, Luis R. Rodriguez wrote:
> 
> > We need to ensure that when driver developers use the custom firmware
> > fallback mechanism it was not a copy and paste bug. These use cases on
> > upstream drivers are rare, we only have 2 upstream users and its for
> > really old drivers. Since valid uses are rare but possible enable a
> > white-list for its use, and use this same white-list annotation to refer
> > to the documentation covering the custom use case.
> >
> > New faulty users can be reported via 0-day now.
> >
> > Cc: Fengguang Wu <fengguang.wu@intel.com>
> > Cc: Richard Purdie <rpurdie@rpsys.net>
> > Cc: Jacek Anaszewski <j.anaszewski@samsung.com>
> > Cc: linux-leds@vger.kernel.org
> > Cc: Abhay Salunke <Abhay_Salunke@dell.com>
> > Acked-by: Jacek Anaszewski <j.anaszewski@samsung.com>
> > Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
> > ---
> >  Documentation/driver-api/firmware/fallback-mechanisms.rst     | 7 +++++--
> >  drivers/firmware/dell_rbu.c                                   | 1 +
> >  drivers/leds/leds-lp55xx-common.c                             | 1 +
> >  include/linux/firmware.h                                      | 7 +++++++
> >  scripts/coccinelle/api/request_firmware-custom-fallback.cocci | 9 ++++++++-
> >  5 files changed, 22 insertions(+), 3 deletions(-)
> >
> > diff --git a/Documentation/driver-api/firmware/fallback-mechanisms.rst b/Documentation/driver-api/firmware/fallback-mechanisms.rst
> > index b87a292153c6..73f509a8d2de 100644
> > --- a/Documentation/driver-api/firmware/fallback-mechanisms.rst
> > +++ b/Documentation/driver-api/firmware/fallback-mechanisms.rst
> > @@ -184,8 +184,11 @@ load firmware for you through a custom path.
> >
> >  The custom fallback mechanism can often be enabled by mistake. We currently
> >  have only 2 users of it, and little justification to enable it for other users.
> > -Since it is a common driver developer mistake to enable it, help police for
> > -new users of the custom fallback mechanism with::
> > +Since it is a common driver developer mistake to enable it, driver developers
> > +should use DECLARE_FW_CUSTOM_FALLBACK() to both white-list and validate their
> > +use and also refer to the documentation for the custom loading solution.
> > +
> > +Invalid users of the custom fallback mechanism can be policed using::
> >
> >          $ export COCCI=scripts/coccinelle/api/request_firmware-avoid-init-probe-init.cocci
> >          $ make coccicheck MODE=report
> > diff --git a/drivers/firmware/dell_rbu.c b/drivers/firmware/dell_rbu.c
> > index 2f452f1f7c8a..3f2aa35bc54d 100644
> > --- a/drivers/firmware/dell_rbu.c
> > +++ b/drivers/firmware/dell_rbu.c
> > @@ -586,6 +586,7 @@ static ssize_t read_rbu_image_type(struct file *filp, struct kobject *kobj,
> >  	return size;
> >  }
> >
> > +DECLARE_FW_CUSTOM_FALLBACK("Documentation/dell_rbu.txt");
> >  static ssize_t write_rbu_image_type(struct file *filp, struct kobject *kobj,
> >  				    struct bin_attribute *bin_attr,
> >  				    char *buffer, loff_t pos, size_t count)
> > diff --git a/drivers/leds/leds-lp55xx-common.c b/drivers/leds/leds-lp55xx-common.c
> > index 5377f22ff994..04161428ee3b 100644
> > --- a/drivers/leds/leds-lp55xx-common.c
> > +++ b/drivers/leds/leds-lp55xx-common.c
> > @@ -219,6 +219,7 @@ static void lp55xx_firmware_loaded(const struct firmware *fw, void *context)
> >  	release_firmware(chip->fw);
> >  }
> >
> > +DECLARE_FW_CUSTOM_FALLBACK("Documentation/leds/leds-lp55xx.txt");
> >  static int lp55xx_request_firmware(struct lp55xx_chip *chip)
> >  {
> >  	const char *name = chip->cl->name;
> > diff --git a/include/linux/firmware.h b/include/linux/firmware.h
> > index b1f9f0ccb8ac..e6ca19c03dcc 100644
> > --- a/include/linux/firmware.h
> > +++ b/include/linux/firmware.h
> > @@ -8,6 +8,13 @@
> >  #define FW_ACTION_NOHOTPLUG 0
> >  #define FW_ACTION_HOTPLUG 1
> >
> > +/*
> > + * Helper for scripts/coccinelle/api/request_firmware-custom-fallback.cocci
> > + * and so users can also easily search for the documentation for the
> > + * respectively needed custom fallback mechanism.
> > + */
> > +#define DECLARE_FW_CUSTOM_FALLBACK(__usermode_helper)
> > +
> >  struct firmware {
> >  	size_t size;
> >  	const u8 *data;
> > diff --git a/scripts/coccinelle/api/request_firmware-custom-fallback.cocci b/scripts/coccinelle/api/request_firmware-custom-fallback.cocci
> > index c7598cfc4780..68cacab35b76 100644
> > --- a/scripts/coccinelle/api/request_firmware-custom-fallback.cocci
> > +++ b/scripts/coccinelle/api/request_firmware-custom-fallback.cocci
> > @@ -17,6 +17,13 @@
> >  virtual report
> >  virtual context
> >
> > +@ r0 depends on report || context @
> > +declarer name DECLARE_FW_CUSTOM_FALLBACK;
> > +expression E;
> > +@@
> > +
> > +DECLARE_FW_CUSTOM_FALLBACK(E);
> > +
> >  @ r1 depends on report || context @
> >  expression mod, name, dev, gfp, drv, cb;
> >  position p;
> > @@ -30,7 +37,7 @@ position p;
> >  *request_firmware_nowait@p(mod, FW_ACTION_NOHOTPLUG, name, dev, gfp, drv, cb)
> 
> It looks suspicious that your added a new rule r0, that the python rule
> below depends on r0 failing, and that the rule with the * (context mode)
> does not depend on r0 in any way.

You're right, the context mode would report all cases, I've changed it as follows:

diff --git a/scripts/coccinelle/api/request_firmware-custom-fallback.cocci b/scripts/coccinelle/api/request_firmware-custom-fallback.cocci
index 68cacab35b76..9548e5be9c0e 100644
--- a/scripts/coccinelle/api/request_firmware-custom-fallback.cocci
+++ b/scripts/coccinelle/api/request_firmware-custom-fallback.cocci
@@ -24,7 +24,7 @@ expression E;
 
 DECLARE_FW_CUSTOM_FALLBACK(E);
 
-@ r1 depends on report || context @
+@ r1 depends on !r0 && report || context @
 expression mod, name, dev, gfp, drv, cb;
 position p;
 @@
@@ -37,7 +37,7 @@ position p;
 *request_firmware_nowait@p(mod, FW_ACTION_NOHOTPLUG, name, dev, gfp, drv, cb)
 )
 
-@script:python depends on report && !r0 @
+@script:python depends on report && r1 @
 p << r1.p;
 @@
 

Re: [PATCH v2 5/5] firmware: add DECLARE_FW_CUSTOM_FALLBACK() annotation

[Luis R. Rodriguez]

On Mon, Jan 9, 2017 at 2:46 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> On Mon, Dec 19, 2016 at 11:23:17AM +0100, Julia Lawall wrote:
>> It looks suspicious that your added a new rule r0, that the python rule
>> below depends on r0 failing, and that the rule with the * (context mode)
>> does not depend on r0 in any way.
>
> You're right, the context mode would report all cases, I've changed it as follows:

That still didn't cut it.. will dig further.

 Luis

Re: [PATCH v2 5/5] firmware: add DECLARE_FW_CUSTOM_FALLBACK() annotation

[Julia Lawall]

On Mon, 9 Jan 2017, Luis R. Rodriguez wrote:

> On Mon, Dec 19, 2016 at 11:23:17AM +0100, Julia Lawall wrote:
> >
> >
> > On Fri, 16 Dec 2016, Luis R. Rodriguez wrote:
> >
> > > We need to ensure that when driver developers use the custom firmware
> > > fallback mechanism it was not a copy and paste bug. These use cases on
> > > upstream drivers are rare, we only have 2 upstream users and its for
> > > really old drivers. Since valid uses are rare but possible enable a
> > > white-list for its use, and use this same white-list annotation to refer
> > > to the documentation covering the custom use case.
> > >
> > > New faulty users can be reported via 0-day now.
> > >
> > > Cc: Fengguang Wu <fengguang.wu@intel.com>
> > > Cc: Richard Purdie <rpurdie@rpsys.net>
> > > Cc: Jacek Anaszewski <j.anaszewski@samsung.com>
> > > Cc: linux-leds@vger.kernel.org
> > > Cc: Abhay Salunke <Abhay_Salunke@dell.com>
> > > Acked-by: Jacek Anaszewski <j.anaszewski@samsung.com>
> > > Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
> > > ---
> > >  Documentation/driver-api/firmware/fallback-mechanisms.rst     | 7 +++++--
> > >  drivers/firmware/dell_rbu.c                                   | 1 +
> > >  drivers/leds/leds-lp55xx-common.c                             | 1 +
> > >  include/linux/firmware.h                                      | 7 +++++++
> > >  scripts/coccinelle/api/request_firmware-custom-fallback.cocci | 9 ++++++++-
> > >  5 files changed, 22 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/Documentation/driver-api/firmware/fallback-mechanisms.rst b/Documentation/driver-api/firmware/fallback-mechanisms.rst
> > > index b87a292153c6..73f509a8d2de 100644
> > > --- a/Documentation/driver-api/firmware/fallback-mechanisms.rst
> > > +++ b/Documentation/driver-api/firmware/fallback-mechanisms.rst
> > > @@ -184,8 +184,11 @@ load firmware for you through a custom path.
> > >
> > >  The custom fallback mechanism can often be enabled by mistake. We currently
> > >  have only 2 users of it, and little justification to enable it for other users.
> > > -Since it is a common driver developer mistake to enable it, help police for
> > > -new users of the custom fallback mechanism with::
> > > +Since it is a common driver developer mistake to enable it, driver developers
> > > +should use DECLARE_FW_CUSTOM_FALLBACK() to both white-list and validate their
> > > +use and also refer to the documentation for the custom loading solution.
> > > +
> > > +Invalid users of the custom fallback mechanism can be policed using::
> > >
> > >          $ export COCCI=scripts/coccinelle/api/request_firmware-avoid-init-probe-init.cocci
> > >          $ make coccicheck MODE=report
> > > diff --git a/drivers/firmware/dell_rbu.c b/drivers/firmware/dell_rbu.c
> > > index 2f452f1f7c8a..3f2aa35bc54d 100644
> > > --- a/drivers/firmware/dell_rbu.c
> > > +++ b/drivers/firmware/dell_rbu.c
> > > @@ -586,6 +586,7 @@ static ssize_t read_rbu_image_type(struct file *filp, struct kobject *kobj,
> > >  	return size;
> > >  }
> > >
> > > +DECLARE_FW_CUSTOM_FALLBACK("Documentation/dell_rbu.txt");
> > >  static ssize_t write_rbu_image_type(struct file *filp, struct kobject *kobj,
> > >  				    struct bin_attribute *bin_attr,
> > >  				    char *buffer, loff_t pos, size_t count)
> > > diff --git a/drivers/leds/leds-lp55xx-common.c b/drivers/leds/leds-lp55xx-common.c
> > > index 5377f22ff994..04161428ee3b 100644
> > > --- a/drivers/leds/leds-lp55xx-common.c
> > > +++ b/drivers/leds/leds-lp55xx-common.c
> > > @@ -219,6 +219,7 @@ static void lp55xx_firmware_loaded(const struct firmware *fw, void *context)
> > >  	release_firmware(chip->fw);
> > >  }
> > >
> > > +DECLARE_FW_CUSTOM_FALLBACK("Documentation/leds/leds-lp55xx.txt");
> > >  static int lp55xx_request_firmware(struct lp55xx_chip *chip)
> > >  {
> > >  	const char *name = chip->cl->name;
> > > diff --git a/include/linux/firmware.h b/include/linux/firmware.h
> > > index b1f9f0ccb8ac..e6ca19c03dcc 100644
> > > --- a/include/linux/firmware.h
> > > +++ b/include/linux/firmware.h
> > > @@ -8,6 +8,13 @@
> > >  #define FW_ACTION_NOHOTPLUG 0
> > >  #define FW_ACTION_HOTPLUG 1
> > >
> > > +/*
> > > + * Helper for scripts/coccinelle/api/request_firmware-custom-fallback.cocci
> > > + * and so users can also easily search for the documentation for the
> > > + * respectively needed custom fallback mechanism.
> > > + */
> > > +#define DECLARE_FW_CUSTOM_FALLBACK(__usermode_helper)
> > > +
> > >  struct firmware {
> > >  	size_t size;
> > >  	const u8 *data;
> > > diff --git a/scripts/coccinelle/api/request_firmware-custom-fallback.cocci b/scripts/coccinelle/api/request_firmware-custom-fallback.cocci
> > > index c7598cfc4780..68cacab35b76 100644
> > > --- a/scripts/coccinelle/api/request_firmware-custom-fallback.cocci
> > > +++ b/scripts/coccinelle/api/request_firmware-custom-fallback.cocci
> > > @@ -17,6 +17,13 @@
> > >  virtual report
> > >  virtual context
> > >
> > > +@ r0 depends on report || context @
> > > +declarer name DECLARE_FW_CUSTOM_FALLBACK;
> > > +expression E;
> > > +@@
> > > +
> > > +DECLARE_FW_CUSTOM_FALLBACK(E);
> > > +
> > >  @ r1 depends on report || context @
> > >  expression mod, name, dev, gfp, drv, cb;
> > >  position p;
> > > @@ -30,7 +37,7 @@ position p;
> > >  *request_firmware_nowait@p(mod, FW_ACTION_NOHOTPLUG, name, dev, gfp, drv, cb)
> >
> > It looks suspicious that your added a new rule r0, that the python rule
> > below depends on r0 failing, and that the rule with the * (context mode)
> > does not depend on r0 in any way.
>
> You're right, the context mode would report all cases, I've changed it as follows:
>
> diff --git a/scripts/coccinelle/api/request_firmware-custom-fallback.cocci b/scripts/coccinelle/api/request_firmware-custom-fallback.cocci
> index 68cacab35b76..9548e5be9c0e 100644
> --- a/scripts/coccinelle/api/request_firmware-custom-fallback.cocci
> +++ b/scripts/coccinelle/api/request_firmware-custom-fallback.cocci
> @@ -24,7 +24,7 @@ expression E;
>
>  DECLARE_FW_CUSTOM_FALLBACK(E);
>
> -@ r1 depends on report || context @
> +@ r1 depends on !r0 && report || context @
>  expression mod, name, dev, gfp, drv, cb;
>  position p;
>  @@
> @@ -37,7 +37,7 @@ position p;
>  *request_firmware_nowait@p(mod, FW_ACTION_NOHOTPLUG, name, dev, gfp, drv, cb)
>  )
>
> -@script:python depends on report && !r0 @
> +@script:python depends on report && r1 @
>  p << r1.p;
>  @@

It is never useful in a python rule to mention an inherited rule in the
depends line that is also mentioned in the metavariable list.  A python
rule is only applied if all the metavariables are bound.  Thus, the use of
r1.p means that r1 has to be satisfied.

If you need further suggestions, just send the whole thing again, and I
will take a look.

julia

Re: [PATCH v2 5/5] firmware: add DECLARE_FW_CUSTOM_FALLBACK() annotation

[Luis R. Rodriguez]

On Mon, Jan 09, 2017 at 10:09:51PM +0100, Julia Lawall wrote:
> 
> 
> On Mon, 9 Jan 2017, Luis R. Rodriguez wrote:
> 
> > On Mon, Dec 19, 2016 at 11:23:17AM +0100, Julia Lawall wrote:
> > >
> > >
> > > On Fri, 16 Dec 2016, Luis R. Rodriguez wrote:
> > >
> > > > We need to ensure that when driver developers use the custom firmware
> > > > fallback mechanism it was not a copy and paste bug. These use cases on
> > > > upstream drivers are rare, we only have 2 upstream users and its for
> > > > really old drivers. Since valid uses are rare but possible enable a
> > > > white-list for its use, and use this same white-list annotation to refer
> > > > to the documentation covering the custom use case.
> > > >
> > > > New faulty users can be reported via 0-day now.
> > > >
> > > > Cc: Fengguang Wu <fengguang.wu@intel.com>
> > > > Cc: Richard Purdie <rpurdie@rpsys.net>
> > > > Cc: Jacek Anaszewski <j.anaszewski@samsung.com>
> > > > Cc: linux-leds@vger.kernel.org
> > > > Cc: Abhay Salunke <Abhay_Salunke@dell.com>
> > > > Acked-by: Jacek Anaszewski <j.anaszewski@samsung.com>
> > > > Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
> > > > ---
> > > >  Documentation/driver-api/firmware/fallback-mechanisms.rst     | 7 +++++--
> > > >  drivers/firmware/dell_rbu.c                                   | 1 +
> > > >  drivers/leds/leds-lp55xx-common.c                             | 1 +
> > > >  include/linux/firmware.h                                      | 7 +++++++
> > > >  scripts/coccinelle/api/request_firmware-custom-fallback.cocci | 9 ++++++++-
> > > >  5 files changed, 22 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/Documentation/driver-api/firmware/fallback-mechanisms.rst b/Documentation/driver-api/firmware/fallback-mechanisms.rst
> > > > index b87a292153c6..73f509a8d2de 100644
> > > > --- a/Documentation/driver-api/firmware/fallback-mechanisms.rst
> > > > +++ b/Documentation/driver-api/firmware/fallback-mechanisms.rst
> > > > @@ -184,8 +184,11 @@ load firmware for you through a custom path.
> > > >
> > > >  The custom fallback mechanism can often be enabled by mistake. We currently
> > > >  have only 2 users of it, and little justification to enable it for other users.
> > > > -Since it is a common driver developer mistake to enable it, help police for
> > > > -new users of the custom fallback mechanism with::
> > > > +Since it is a common driver developer mistake to enable it, driver developers
> > > > +should use DECLARE_FW_CUSTOM_FALLBACK() to both white-list and validate their
> > > > +use and also refer to the documentation for the custom loading solution.
> > > > +
> > > > +Invalid users of the custom fallback mechanism can be policed using::
> > > >
> > > >          $ export COCCI=scripts/coccinelle/api/request_firmware-avoid-init-probe-init.cocci
> > > >          $ make coccicheck MODE=report
> > > > diff --git a/drivers/firmware/dell_rbu.c b/drivers/firmware/dell_rbu.c
> > > > index 2f452f1f7c8a..3f2aa35bc54d 100644
> > > > --- a/drivers/firmware/dell_rbu.c
> > > > +++ b/drivers/firmware/dell_rbu.c
> > > > @@ -586,6 +586,7 @@ static ssize_t read_rbu_image_type(struct file *filp, struct kobject *kobj,
> > > >  	return size;
> > > >  }
> > > >
> > > > +DECLARE_FW_CUSTOM_FALLBACK("Documentation/dell_rbu.txt");
> > > >  static ssize_t write_rbu_image_type(struct file *filp, struct kobject *kobj,
> > > >  				    struct bin_attribute *bin_attr,
> > > >  				    char *buffer, loff_t pos, size_t count)
> > > > diff --git a/drivers/leds/leds-lp55xx-common.c b/drivers/leds/leds-lp55xx-common.c
> > > > index 5377f22ff994..04161428ee3b 100644
> > > > --- a/drivers/leds/leds-lp55xx-common.c
> > > > +++ b/drivers/leds/leds-lp55xx-common.c
> > > > @@ -219,6 +219,7 @@ static void lp55xx_firmware_loaded(const struct firmware *fw, void *context)
> > > >  	release_firmware(chip->fw);
> > > >  }
> > > >
> > > > +DECLARE_FW_CUSTOM_FALLBACK("Documentation/leds/leds-lp55xx.txt");
> > > >  static int lp55xx_request_firmware(struct lp55xx_chip *chip)
> > > >  {
> > > >  	const char *name = chip->cl->name;
> > > > diff --git a/include/linux/firmware.h b/include/linux/firmware.h
> > > > index b1f9f0ccb8ac..e6ca19c03dcc 100644
> > > > --- a/include/linux/firmware.h
> > > > +++ b/include/linux/firmware.h
> > > > @@ -8,6 +8,13 @@
> > > >  #define FW_ACTION_NOHOTPLUG 0
> > > >  #define FW_ACTION_HOTPLUG 1
> > > >
> > > > +/*
> > > > + * Helper for scripts/coccinelle/api/request_firmware-custom-fallback.cocci
> > > > + * and so users can also easily search for the documentation for the
> > > > + * respectively needed custom fallback mechanism.
> > > > + */
> > > > +#define DECLARE_FW_CUSTOM_FALLBACK(__usermode_helper)
> > > > +
> > > >  struct firmware {
> > > >  	size_t size;
> > > >  	const u8 *data;
> > > > diff --git a/scripts/coccinelle/api/request_firmware-custom-fallback.cocci b/scripts/coccinelle/api/request_firmware-custom-fallback.cocci
> > > > index c7598cfc4780..68cacab35b76 100644
> > > > --- a/scripts/coccinelle/api/request_firmware-custom-fallback.cocci
> > > > +++ b/scripts/coccinelle/api/request_firmware-custom-fallback.cocci
> > > > @@ -17,6 +17,13 @@
> > > >  virtual report
> > > >  virtual context
> > > >
> > > > +@ r0 depends on report || context @
> > > > +declarer name DECLARE_FW_CUSTOM_FALLBACK;
> > > > +expression E;
> > > > +@@
> > > > +
> > > > +DECLARE_FW_CUSTOM_FALLBACK(E);
> > > > +
> > > >  @ r1 depends on report || context @
> > > >  expression mod, name, dev, gfp, drv, cb;
> > > >  position p;
> > > > @@ -30,7 +37,7 @@ position p;
> > > >  *request_firmware_nowait@p(mod, FW_ACTION_NOHOTPLUG, name, dev, gfp, drv, cb)
> > >
> > > It looks suspicious that your added a new rule r0, that the python rule
> > > below depends on r0 failing, and that the rule with the * (context mode)
> > > does not depend on r0 in any way.
> >
> > You're right, the context mode would report all cases, I've changed it as follows:
> >
> > diff --git a/scripts/coccinelle/api/request_firmware-custom-fallback.cocci b/scripts/coccinelle/api/request_firmware-custom-fallback.cocci
> > index 68cacab35b76..9548e5be9c0e 100644
> > --- a/scripts/coccinelle/api/request_firmware-custom-fallback.cocci
> > +++ b/scripts/coccinelle/api/request_firmware-custom-fallback.cocci
> > @@ -24,7 +24,7 @@ expression E;
> >
> >  DECLARE_FW_CUSTOM_FALLBACK(E);
> >
> > -@ r1 depends on report || context @
> > +@ r1 depends on !r0 && report || context @
> >  expression mod, name, dev, gfp, drv, cb;
> >  position p;
> >  @@
> > @@ -37,7 +37,7 @@ position p;
> >  *request_firmware_nowait@p(mod, FW_ACTION_NOHOTPLUG, name, dev, gfp, drv, cb)
> >  )
> >
> > -@script:python depends on report && !r0 @
> > +@script:python depends on report && r1 @
> >  p << r1.p;
> >  @@
> 
> It is never useful in a python rule to mention an inherited rule in the
> depends line that is also mentioned in the metavariable list.  A python
> rule is only applied if all the metavariables are bound.  Thus, the use of
> r1.p means that r1 has to be satisfied.
> 
> If you need further suggestions, just send the whole thing again, and I
> will take a look.

Sure, I can just also drop context support for now as what we really are after
are the reports, and that works well.

  Luis

Re: [PATCH v2 4/5] firmware: add SmPL report for custom fallback mechanism

[Greg KH]

On Fri, Dec 16, 2016 at 03:10:37AM -0800, Luis R. Rodriguez wrote:
> Even though most distributions today disable the fallback mechanism
> by default we've determined that we cannot remove them from the kernel.
> This is not well understood so document the reason and logic behind that.

Well, the biggest reason is that some distros still rely on this.  I've
seen new products being made that rely on it, so let's please stop
treating it like it is somehow a "deprecated" interface.  We can't get
rid of it, so we just have to live with it, for forever, sorry.

greg k-h

Re: [PATCH v2 4/5] firmware: add SmPL report for custom fallback mechanism

[Luis R. Rodriguez]

On Wed, Jan 11, 2017 at 09:32:26AM +0100, Greg KH wrote:
> On Fri, Dec 16, 2016 at 03:10:37AM -0800, Luis R. Rodriguez wrote:
> > Even though most distributions today disable the fallback mechanism
> > by default we've determined that we cannot remove them from the kernel.
> > This is not well understood so document the reason and logic behind that.
> 
> Well, the biggest reason is that some distros still rely on this.  I've
> seen new products being made that rely on it,

Let's be a bit more precise: upstream there are only two driver relying on this
and I've learned about the non-upstream uses which folks have been calling for
ensuring this functionality is kept for: a) non-upstream mobile 802.11 drivers or
upstream 802.11 drivers with slight out-of-tree customizations with a requirements to
get calibration data using custom mechanisms b) remote-proc users with huge firmware
requirements for which initramfs is not well suited for.

I've taken these requirements seriously in this series. Perhaps this is not well
reflected in the series enough so let me elaborate.

> so let's please stop
> treating it like it is somehow a "deprecated" interface.

In this series I no longer treats it as a deprecated interface.  This series
however does acknowledge a bit of the drawbacks and cautions folks should take
when using these interfaces though. These issues are real.

> We can't get rid of it,

I am way past that point. I've had to personally deal with both pushes now: the
misplaced crusade against the both the mislabeled "firmware usermode helper" which
was originally actually caused by the "firmware timeout issue" and now properly
diagnosed, and later those out of a) and b) users. I've listened to both carefully
and given this much thought and discussion at Plumbers through hallway tracks
and private exchanges including with systemd folks and have tried to itemize
the current drawbacks and also finally address them. One thing is clear: the
out of tree custom fallback users simply need a custom way to load firmware,
the use of the kobject driven uevent mechanism should suffice, its just we
never had a clear documented way to enable custom solutions. In discussions
with Tom Gundersen, and Johannes Berg it seems we can enable these users easily
with the kobject uevent fallback mechanism, we just need to address the existing
drawback issues. This means if taking into consideration upstream and
non-upstream drivers -- the custom fallback mechanism becomes more of a legacy thing.
So sure -- we cannot remove it, but we should avoid more propagation of it by
mistake upstream, and hence this patch.

To this end my new goal is to first properly label and document the interfaces first, 
then to itemize the drawbacks of the "custom firmware fallback mechanism", avoid further
copy and paste bugs which implicated the "custom firmware fallback mechanism" and were
frequent before (its the purpose of this patch). Then work with kernel/systemd folks
to provide a proper resolution to the drawbacks as best as we can for the general
uevent kobject fallback mechanism. This solution will work for the existing
request_firmware_nowait() API, so it will benefit from it, but only once that is
properly hashed out will I plan to add equivalent a fallback mechanism to the
drvdata API.

> so we just have to live with it, for forever, sorry.

This documentation revamp acknowledges this, but paves the way for what we
need to do for the old users of the custom fallback mechanism and of the
users which I've heard from which need a type of fallback mechanism. The
next patch white-lists though the few old upstream uses of the custom
fallback mechanism.

The plan is to never remove the old custom fallback mechanism then. The
drvdata API will start without any fallback mechanism to start with but
the plan is to incorporate support for one once we iron out all the kinks
for a clean solution for a general fallback mechanism we are happy with.

The old custom fallback mechanism will be kept forever.

  Luis

Re: [PATCH v2 4/5] firmware: add SmPL report for custom fallback mechanism

[Greg KH]

On Wed, Jan 11, 2017 at 03:02:22PM +0100, Luis R. Rodriguez wrote:
> On Wed, Jan 11, 2017 at 09:32:26AM +0100, Greg KH wrote:
> > On Fri, Dec 16, 2016 at 03:10:37AM -0800, Luis R. Rodriguez wrote:
> > > Even though most distributions today disable the fallback mechanism
> > > by default we've determined that we cannot remove them from the kernel.
> > > This is not well understood so document the reason and logic behind that.
> > 
> > Well, the biggest reason is that some distros still rely on this.  I've
> > seen new products being made that rely on it,
> 
> Let's be a bit more precise: upstream there are only two driver relying on this
> and I've learned about the non-upstream uses which folks have been calling for
> ensuring this functionality is kept for: a) non-upstream mobile 802.11 drivers or
> upstream 802.11 drivers with slight out-of-tree customizations with a requirements to
> get calibration data using custom mechanisms b) remote-proc users with huge firmware
> requirements for which initramfs is not well suited for.

That b) is a lot of devices, I know of a few million phones in the wild
right now that rely on it.  And millions is a pretty big number :)

Anyway, thanks for addressing my concerns, I'm guessing you will respin
these remaining patches and resend them as I think there were still some
comments on them?  I took the first 3 here.

Is the "drvdata" code ready in your opinion to be merged / reviewed yet?

thanks,

greg k-h

Re: [PATCH v2 4/5] firmware: add SmPL report for custom fallback mechanism

[Luis R. Rodriguez]

On Wed, Jan 11, 2017 at 9:49 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Wed, Jan 11, 2017 at 03:02:22PM +0100, Luis R. Rodriguez wrote:
>> On Wed, Jan 11, 2017 at 09:32:26AM +0100, Greg KH wrote:
>> > On Fri, Dec 16, 2016 at 03:10:37AM -0800, Luis R. Rodriguez wrote:
>> > > Even though most distributions today disable the fallback mechanism
>> > > by default we've determined that we cannot remove them from the kernel.
>> > > This is not well understood so document the reason and logic behind that.
>> >
>> > Well, the biggest reason is that some distros still rely on this.  I've
>> > seen new products being made that rely on it,
>>
>> Let's be a bit more precise: upstream there are only two driver relying on this
>> and I've learned about the non-upstream uses which folks have been calling for
>> ensuring this functionality is kept for: a) non-upstream mobile 802.11 drivers or
>> upstream 802.11 drivers with slight out-of-tree customizations with a requirements to
>> get calibration data using custom mechanisms b) remote-proc users with huge firmware
>> requirements for which initramfs is not well suited for.
>
> That b) is a lot of devices, I know of a few million phones in the wild
> right now that rely on it.  And millions is a pretty big number :)
>
> Anyway, thanks for addressing my concerns, I'm guessing you will respin
> these remaining patches and resend them as I think there were still some
> comments on them?  I took the first 3 here.

Yeah sure, I will address these comments.

> Is the "drvdata" code ready in your opinion to be merged / reviewed yet?

drvdata stuff is ready as can be but after the sysdata/drvdata rename
change I failed to change one of the p54 files, I also forgot to Cc
Boris on the microcode conversion so I can just respin the drvdata
series again as a separate series right after I address the concerns
for this series.

 Luis