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

[PATCH 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>
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 edce1d76ce29..955c11d6ff9d 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

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

[Julia Lawall]

On Mon, 12 Dec 2016, 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.
>
> 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>
> Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>

Acked-by: Julia.Lawall@lip6.fr


> ---
>  .../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 edce1d76ce29..955c11d6ff9d 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
>
>

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

[Jacek Anaszewski]

Hi Milo,

Could you please verify if leds-lp55xx-common.c driver
really needs a custom firmware loading fallback mechanism?
See [0] to gain some background knowledge, especially patch 3/5 seems
to provide a big amount of information.

[0] https://lkml.org/lkml/2016/12/12/717

Thanks,
Jacek Anaszewski

On 12/13/2016 04:08 AM, 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.
>
> 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>
> 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 edce1d76ce29..955c11d6ff9d 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")
>

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

[Milo Kim]

Hi Jacek,

On 12/13/2016 06:44 PM, Jacek Anaszewski wrote:
>
> Could you please verify if leds-lp55xx-common.c driver
> really needs a custom firmware loading fallback mechanism?

Thanks for sharing this. The lp55xx-common uses this mechanism to load 
and run LED effect manually, so this could be a misuse case.
I think the right solution is providing device attributes.

At this moment, four drivers use lp55xx-common code.

- lp5521, lp5523: OK if we do not support FW loading fallback mechanism
- lp5562, lp8501: need to create additional sysfs alternatively.

However, we should be careful because I'm not sure this modification 
will generate the regression (breaking the user-space) or not.

Best regards,
Milo

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

[Luis R. Rodriguez]

On Wed, Dec 14, 2016 at 10:48:37AM +0900, Milo Kim wrote:
> Hi Jacek,
> 
> On 12/13/2016 06:44 PM, Jacek Anaszewski wrote:
> > 
> > Could you please verify if leds-lp55xx-common.c driver
> > really needs a custom firmware loading fallback mechanism?
> 
> Thanks for sharing this. The lp55xx-common uses this mechanism to load and
> run LED effect manually, so this could be a misuse case.
> I think the right solution is providing device attributes.

Run LED manually using request_firmware()? What the.. yes this sounds odd.

> At this moment, four drivers use lp55xx-common code.
> 
> - lp5521, lp5523: OK if we do not support FW loading fallback mechanism
> - lp5562, lp8501: need to create additional sysfs alternatively.

Phew!

> However, we should be careful because I'm not sure this modification will
> generate the regression (breaking the user-space) or not.

Right so not breaking old userspace is critical.

  Luis

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

[Luis R. Rodriguez]

On Fri, Dec 16, 2016 at 10:29:52AM +0100, Luis R. Rodriguez wrote:
> On Wed, Dec 14, 2016 at 10:48:37AM +0900, Milo Kim wrote:
> > Hi Jacek,
> > 
> > On 12/13/2016 06:44 PM, Jacek Anaszewski wrote:
> > > 
> > > Could you please verify if leds-lp55xx-common.c driver
> > > really needs a custom firmware loading fallback mechanism?
> > 
> > Thanks for sharing this. The lp55xx-common uses this mechanism to load and
> > run LED effect manually, so this could be a misuse case.
> > I think the right solution is providing device attributes.
> 
> Run LED manually using request_firmware()? What the.. yes this sounds odd.
> 
> > At this moment, four drivers use lp55xx-common code.
> > 
> > - lp5521, lp5523: OK if we do not support FW loading fallback mechanism
> > - lp5562, lp8501: need to create additional sysfs alternatively.
> 
> Phew!
> 
> > However, we should be careful because I'm not sure this modification will
> > generate the regression (breaking the user-space) or not.
> 
> Right so not breaking old userspace is critical.

So to recap:

What you can do is add support for the new interface and recommend folks
to use it. We just cannot break old userspace relying on the same module.

So the currently proposed DECLARE_FW_CUSTOM_FALLBACK() is still justified
for this driver. As stupid as the original implementation may have been
its there.. and we just have to deal with it. I would seriously appreciate
a revamp update on the Documentation/leds/leds-lp55xx.txt documentation though
with everything that we have discussed.

  Luis

[PATCH 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>
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 955c11d6ff9d..b51673e40439 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 5/5] firmware: add DECLARE_FW_CUSTOM_FALLBACK() annotation

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1542 bytes --]

Hi!

> 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.

> --- 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;

The driver does:

static void lp55xx_firmware_loaded(const struct firmware *fw, void
*context)
{
	struct lp55xx_chip *chip = context;
	        struct device *dev = &chip->cl->dev;
		        enum lp55xx_engine_index idx =
			chip->engine_idx;

        if (!fw) {
	                dev_err(dev, "firmware request failed\n");
			                goto out;
					     }
        ...
out:
        /* firmware should be released for other channel use */
	        release_firmware(chip->fw);
}


Does that match the "custom fallback" definition?


-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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

[Luis R. Rodriguez]

On Tue, Dec 13, 2016 at 08:04:29PM +0100, Pavel Machek wrote:
> Hi!
> 
> > 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.
> 
> > --- 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;
> 
> The driver does:
> 
> static void lp55xx_firmware_loaded(const struct firmware *fw, void
> *context)
> {
> 	struct lp55xx_chip *chip = context;
> 	        struct device *dev = &chip->cl->dev;
> 		        enum lp55xx_engine_index idx =
> 			chip->engine_idx;
> 
>         if (!fw) {
> 	                dev_err(dev, "firmware request failed\n");
> 			                goto out;
> 					     }
>         ...
> out:
>         /* firmware should be released for other channel use */
> 	        release_firmware(chip->fw);
> }
> 
> 
> Does that match the "custom fallback" definition?

Refer to the documentation I supplied, and also to the grammar rule, in
particular the patch "firmware: add SmPL report for custom fallback mechanism",
it captures the SmPL form for the custom fallback mechanism as:

@ 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)   
)     

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

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 2148 bytes --]

On Fri 2016-12-16 10:22:41, Luis R. Rodriguez wrote:
> On Tue, Dec 13, 2016 at 08:04:29PM +0100, Pavel Machek wrote:
> > Hi!
> > 
> > > 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.
> > 
> > > --- 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;
> > 
> > The driver does:
> > 
> > static void lp55xx_firmware_loaded(const struct firmware *fw, void
> > *context)
> > {
> > 	struct lp55xx_chip *chip = context;
> > 	        struct device *dev = &chip->cl->dev;
> > 		        enum lp55xx_engine_index idx =
> > 			chip->engine_idx;
> > 
> >         if (!fw) {
> > 	                dev_err(dev, "firmware request failed\n");
> > 			                goto out;
> > 					     }
> >         ...
> > out:
> >         /* firmware should be released for other channel use */
> > 	        release_firmware(chip->fw);
> > }
> > 
> > 
> > Does that match the "custom fallback" definition?
> 
> Refer to the documentation I supplied, and also to the grammar rule, in
> particular the patch "firmware: add SmPL report for custom fallback mechanism",
> it captures the SmPL form for the custom fallback mechanism as:

I don't much care what the rule says. If you believe the code is
buggy, submit a patch.

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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

[Luis R. Rodriguez]

On Fri, Dec 16, 2016 at 10:29:20AM +0100, Pavel Machek wrote:
> On Fri 2016-12-16 10:22:41, Luis R. Rodriguez wrote:
> > On Tue, Dec 13, 2016 at 08:04:29PM +0100, Pavel Machek wrote:
> > > Hi!
> > > 
> > > > 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.
> > > 
> > > > --- 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;
> > > 
> > > The driver does:
> > > 
> > > static void lp55xx_firmware_loaded(const struct firmware *fw, void
> > > *context)
> > > {
> > > 	struct lp55xx_chip *chip = context;
> > > 	        struct device *dev = &chip->cl->dev;
> > > 		        enum lp55xx_engine_index idx =
> > > 			chip->engine_idx;
> > > 
> > >         if (!fw) {
> > > 	                dev_err(dev, "firmware request failed\n");
> > > 			                goto out;
> > > 					     }
> > >         ...
> > > out:
> > >         /* firmware should be released for other channel use */
> > > 	        release_firmware(chip->fw);
> > > }
> > > 
> > > 
> > > Does that match the "custom fallback" definition?
> > 
> > Refer to the documentation I supplied, and also to the grammar rule, in
> > particular the patch "firmware: add SmPL report for custom fallback mechanism",
> > it captures the SmPL form for the custom fallback mechanism as:
> 
> I don't much care what the rule says. If you believe the code is
> buggy, submit a patch.

Huh? No, its an old API and valid uses are scarce. The point is to avoid folks
adding yet other users by mistake by using grammar to help white-list actual
valid users.

  Luis

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

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 2838 bytes --]

On Fri 2016-12-16 10:59:06, Luis R. Rodriguez wrote:
> On Fri, Dec 16, 2016 at 10:29:20AM +0100, Pavel Machek wrote:
> > On Fri 2016-12-16 10:22:41, Luis R. Rodriguez wrote:
> > > On Tue, Dec 13, 2016 at 08:04:29PM +0100, Pavel Machek wrote:
> > > > Hi!
> > > > 
> > > > > 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.
> > > > 
> > > > > --- 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;
> > > > 
> > > > The driver does:
> > > > 
> > > > static void lp55xx_firmware_loaded(const struct firmware *fw, void
> > > > *context)
> > > > {
> > > > 	struct lp55xx_chip *chip = context;
> > > > 	        struct device *dev = &chip->cl->dev;
> > > > 		        enum lp55xx_engine_index idx =
> > > > 			chip->engine_idx;
> > > > 
> > > >         if (!fw) {
> > > > 	                dev_err(dev, "firmware request failed\n");
> > > > 			                goto out;
> > > > 					     }
> > > >         ...
> > > > out:
> > > >         /* firmware should be released for other channel use */
> > > > 	        release_firmware(chip->fw);
> > > > }
> > > > 
> > > > 
> > > > Does that match the "custom fallback" definition?
> > > 
> > > Refer to the documentation I supplied, and also to the grammar rule, in
> > > particular the patch "firmware: add SmPL report for custom fallback mechanism",
> > > it captures the SmPL form for the custom fallback mechanism as:
> > 
> > I don't much care what the rule says. If you believe the code is
> > buggy, submit a patch.
> 
> Huh? No, its an old API and valid uses are scarce. The point is to avoid folks
> adding yet other users by mistake by using grammar to help white-list actual
> valid users.

Well, I was asking if the above snipped looks like valid use. Because
AFAICT, the "custom fallback" is just dev_err(), see above. Coccinelle
rules don't help me...
									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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

[Luis R. Rodriguez]

On Fri, Dec 16, 2016 at 11:14:05AM +0100, Pavel Machek wrote:
> 
> Well, I was asking if the above snipped looks like valid use. Because
> AFAICT, the "custom fallback" is just dev_err(), see above. Coccinelle
> rules don't help me...

Its not. Its when you ask for no uevent. Only 2 drivers do this.

  Luis

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

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 640 bytes --]

On Fri 2016-12-16 11:56:48, Luis R. Rodriguez wrote:
> On Fri, Dec 16, 2016 at 11:14:05AM +0100, Pavel Machek wrote:
> > 
> > Well, I was asking if the above snipped looks like valid use. Because
> > AFAICT, the "custom fallback" is just dev_err(), see above. Coccinelle
> > rules don't help me...
> 
> Its not. Its when you ask for no uevent. Only 2 drivers do this.

That was one of two you listed. If that is not valid use, perhaps it
should be removed, not annotated?
									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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

[Luis R. Rodriguez]

On Fri, Dec 16, 2016 at 5:27 AM, Pavel Machek <pavel@ucw.cz> wrote:
> On Fri 2016-12-16 11:56:48, Luis R. Rodriguez wrote:
>> On Fri, Dec 16, 2016 at 11:14:05AM +0100, Pavel Machek wrote:
>> >
>> > Well, I was asking if the above snipped looks like valid use. Because
>> > AFAICT, the "custom fallback" is just dev_err(), see above. Coccinelle
>> > rules don't help me...
>>
>> Its not. Its when you ask for no uevent. Only 2 drivers do this.
>
> That was one of two you listed. If that is not valid use, perhaps it
> should be removed, not annotated?

Pavel, the annotation was added on top of:

static int lp55xx_request_firmware(struct lp55xx_chip *chip)
{
        const char *name = chip->cl->name;
        struct device *dev = &chip->cl->dev;

        return request_firmware_nowait(THIS_MODULE, false, name, dev,
                                GFP_KERNEL, chip, lp55xx_firmware_loaded);
}

Note the second argument is false. This matches the grammar and the
definition for a custom fallback mechanism since uevents are not used.
What am I missing?

 Luis

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

[Luis R. Rodriguez]

On Fri, Dec 16, 2016 at 12:27:00PM +0100, Pavel Machek wrote:
> On Fri 2016-12-16 11:56:48, Luis R. Rodriguez wrote:
> > On Fri, Dec 16, 2016 at 11:14:05AM +0100, Pavel Machek wrote:
> > > 
> > > Well, I was asking if the above snipped looks like valid use. Because
> > > AFAICT, the "custom fallback" is just dev_err(), see above. Coccinelle
> > > rules don't help me...
> > 
> > Its not. Its when you ask for no uevent. Only 2 drivers do this.
> 
> That was one of two you listed. If that is not valid use, perhaps it
> should be removed, not annotated?

Ah, well Milo Kim replied and described that the custom fallback is used as to
help load LED effect manually, and suggested a sysfs interface is more ideal [0]. I
agree however its also may be too late, and it depends how wide spread this "userspace"
that relies on this is, we just can't break it. Granted the custom fallback
mechanism was broken since v4.0 (see the fix "firmware: fix usermode helper
fallback loading") so one may argue no one seems to care...

So this is a judgement call, and the declaration is to point to documentation
to white list uses, as terrible as this one is userspace exists for it. but
more importantly to also help the SmPL grammar report to avoid reporting
already vetted cases. The alarm / cases for the 2 drivers has been issueed,
moving forward the lack of declaration with the custom fallback should trigger
a rant through 0-day so we don't run into the same stupid situation.

[0] https://marc.info/?l=linux-kernel&m=148168024112445

  Luis

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

[Luis R. Rodriguez]

On Fri, Dec 16, 2016 at 05:10:18PM +0100, Luis R. Rodriguez wrote:
> Ah, well Milo Kim replied and described that the custom fallback is used as to
> help load LED effect manually, and suggested a sysfs interface is more ideal [0]. I
> agree however its also may be too late, and it depends how wide spread this "userspace"
> that relies on this is, we just can't break it. Granted the custom fallback
> mechanism was broken since v4.0 (see the fix "firmware: fix usermode helper
> fallback loading") so one may argue no one seems to care...
> 
> So this is a judgement call, and the declaration is to point to documentation
> to white list uses, as terrible as this one is userspace exists for it. but
> more importantly to also help the SmPL grammar report to avoid reporting
> already vetted cases. The alarm / cases for the 2 drivers has been issueed,
> moving forward the lack of declaration with the custom fallback should trigger
> a rant through 0-day so we don't run into the same stupid situation.
> 
> [0] https://marc.info/?l=linux-kernel&m=148168024112445

Milo if sysfs is used can't the old userspace be mapped to use the new 
sysfs interface through a wrapper of some sort ? What exactly would be
needed to ensure old userspace will not break? Why has no one cried
after the v4.0 custom fallback mechanism breaking ? How wide spread is
this custom userspace ?

  Luis

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

[Milo Kim]

Hi Luis,

On 12/17/2016 01:14 AM, Luis R. Rodriguez wrote:
> Milo if sysfs is used can't the old userspace be mapped to use the new
> sysfs interface through a wrapper of some sort ? What exactly would be
> needed to ensure old userspace will not break?

LP5521 and LP5523 have two ways to load hex code from the userspace - 
the sysfs and firmware I/F. So user program supports both interfaces. 
Even if the firmware I/F is not available, user can still run LED effect 
through the sysfs.

However, LP5562 and LP8501 support only single way which is the firmware 
I/F. So user-space program for LP5562/8501 should be modified if lp55xx 
removes the interface. My idea is

   Phase 1)
   - create sysfs in LP5562 and LP8501
   - use new sysfs inside the firmware I/F loading callback
   - mark the firmware callback as a deprecated interface

   Phase 2)
   - remove the firmware I/F after all user program fixes the interface
     (but the problem is how can we get to know when this is done?)

 > Why has no one cried
 > after the v4.0 custom fallback mechanism breaking ?

Well, I don't know the reason exactly but my guess is they maybe still 
using old kernel.

 > How wide spread is this custom userspace ?

Device manufactures in Asia & North America requested lp55xx drivers, 
but I don't know how many vendors uses the firmware I/F. Some vendors 
embeds the binary code inside the driver instead of using user-program.

I understood it's a kind of troublesome work in terms of the 
maintenance. Sorry for that. I hope we have a consensus to resolve it.
Thanks!

Best regards,
Milo

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

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 2139 bytes --]

Hi!

> On 12/17/2016 01:14 AM, Luis R. Rodriguez wrote:
> >Milo if sysfs is used can't the old userspace be mapped to use the new
> >sysfs interface through a wrapper of some sort ? What exactly would be
> >needed to ensure old userspace will not break?
> 
> LP5521 and LP5523 have two ways to load hex code from the userspace - the
> sysfs and firmware I/F. So user program supports both interfaces. Even if
> the firmware I/F is not available, user can still run LED effect through the
> sysfs.
> 
> However, LP5562 and LP8501 support only single way which is the firmware
> I/F. So user-space program for LP5562/8501 should be modified if lp55xx
> removes the interface. My idea is

Actually... it would be good to have some reasonable interface for RGB
LEDs. This way, we need separate "firmware" for each LED
controller. It would be good to have common format for LED effects.

>   Phase 1)
>   - create sysfs in LP5562 and LP8501
>   - use new sysfs inside the firmware I/F loading callback
>   - mark the firmware callback as a deprecated interface

Phase 1a)

stick WARN_ON() in the firmware callback.

>   Phase 2)
>   - remove the firmware I/F after all user program fixes the interface
>     (but the problem is how can we get to know when this is done?)
> 
> > Why has no one cried
> > after the v4.0 custom fallback mechanism breaking ?
> 
> Well, I don't know the reason exactly but my guess is they maybe still using
> old kernel.
> 
> > How wide spread is this custom userspace ?
> 
> Device manufactures in Asia & North America requested lp55xx drivers, but I
> don't know how many vendors uses the firmware I/F. Some vendors embeds the
> binary code inside the driver instead of using user-program.

Nokia N900 uses lp55xx, and I have custom scripts interfacing sysfs.

Maemo uses the LEDs, too, but maemo is not open source.

So no, I don't think there's anything important that could be broken.

Best regards,

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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

[Jacek Anaszewski]

On 12/19/2016 09:08 PM, Pavel Machek wrote:
> Hi!
> 
>> On 12/17/2016 01:14 AM, Luis R. Rodriguez wrote:
>>> Milo if sysfs is used can't the old userspace be mapped to use the new
>>> sysfs interface through a wrapper of some sort ? What exactly would be
>>> needed to ensure old userspace will not break?
>>
>> LP5521 and LP5523 have two ways to load hex code from the userspace - the
>> sysfs and firmware I/F. So user program supports both interfaces. Even if
>> the firmware I/F is not available, user can still run LED effect through the
>> sysfs.
>>
>> However, LP5562 and LP8501 support only single way which is the firmware
>> I/F. So user-space program for LP5562/8501 should be modified if lp55xx
>> removes the interface. My idea is
> 
> Actually... it would be good to have some reasonable interface for RGB
> LEDs. This way, we need separate "firmware" for each LED
> controller. It would be good to have common format for LED effects.

We still haven't tried trigger approach discussed over half a year ago.
If we used firmware approach we would still have to overcome the problem
of defining the LED class drivers affected by the firmware program.

>>   Phase 1)
>>   - create sysfs in LP5562 and LP8501
>>   - use new sysfs inside the firmware I/F loading callback
>>   - mark the firmware callback as a deprecated interface
> 
> Phase 1a)
> 
> stick WARN_ON() in the firmware callback.
> 
>>   Phase 2)
>>   - remove the firmware I/F after all user program fixes the interface
>>     (but the problem is how can we get to know when this is done?)
>>
>>> Why has no one cried
>>> after the v4.0 custom fallback mechanism breaking ?
>>
>> Well, I don't know the reason exactly but my guess is they maybe still using
>> old kernel.
>>
>>> How wide spread is this custom userspace ?
>>
>> Device manufactures in Asia & North America requested lp55xx drivers, but I
>> don't know how many vendors uses the firmware I/F. Some vendors embeds the
>> binary code inside the driver instead of using user-program.
> 
> Nokia N900 uses lp55xx, and I have custom scripts interfacing sysfs.
> 
> Maemo uses the LEDs, too, but maemo is not open source.
> 
> So no, I don't think there's anything important that could be broken.

We can't guarantee that. Is there any problem in just using the
currently introduced DECLARE_FW_CUSTOM_FALLBACK() in
drivers/leds/leds-lp55xx-common.c?

-- 
Best regards,
Jacek Anaszewski

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

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 2168 bytes --]

Hi!

> >>> Milo if sysfs is used can't the old userspace be mapped to use the new
> >>> sysfs interface through a wrapper of some sort ? What exactly would be
> >>> needed to ensure old userspace will not break?
> >>
> >> LP5521 and LP5523 have two ways to load hex code from the userspace - the
> >> sysfs and firmware I/F. So user program supports both interfaces. Even if
> >> the firmware I/F is not available, user can still run LED effect through the
> >> sysfs.
> >>
> >> However, LP5562 and LP8501 support only single way which is the firmware
> >> I/F. So user-space program for LP5562/8501 should be modified if lp55xx
> >> removes the interface. My idea is
> > 
> > Actually... it would be good to have some reasonable interface for RGB
> > LEDs. This way, we need separate "firmware" for each LED
> > controller. It would be good to have common format for LED effects.
> 
> We still haven't tried trigger approach discussed over half a year ago.
> If we used firmware approach we would still have to overcome the problem
> of defining the LED class drivers affected by the firmware program.

The firmware approach is in the tree today :-(.

> >> Device manufactures in Asia & North America requested lp55xx drivers, but I
> >> don't know how many vendors uses the firmware I/F. Some vendors embeds the
> >> binary code inside the driver instead of using user-program.
> > 
> > Nokia N900 uses lp55xx, and I have custom scripts interfacing sysfs.
> > 
> > Maemo uses the LEDs, too, but maemo is not open source.
> > 
> > So no, I don't think there's anything important that could be broken.
> 
> We can't guarantee that. Is there any problem in just using the
> currently introduced DECLARE_FW_CUSTOM_FALLBACK() in
> drivers/leds/leds-lp55xx-common.c?

Well, it would be good to get rid of the custom fallback
functionality. And no, we don't need to "guarantee" that.  Removing
obscure functionality noone uses is far game... providing noone
complains ;-).

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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

[Jacek Anaszewski]

Hi,

On 12/21/2016 07:49 PM, Pavel Machek wrote:
> Hi!
> 
>>>>> Milo if sysfs is used can't the old userspace be mapped to use the new
>>>>> sysfs interface through a wrapper of some sort ? What exactly would be
>>>>> needed to ensure old userspace will not break?
>>>>
>>>> LP5521 and LP5523 have two ways to load hex code from the userspace - the
>>>> sysfs and firmware I/F. So user program supports both interfaces. Even if
>>>> the firmware I/F is not available, user can still run LED effect through the
>>>> sysfs.
>>>>
>>>> However, LP5562 and LP8501 support only single way which is the firmware
>>>> I/F. So user-space program for LP5562/8501 should be modified if lp55xx
>>>> removes the interface. My idea is
>>>
>>> Actually... it would be good to have some reasonable interface for RGB
>>> LEDs. This way, we need separate "firmware" for each LED
>>> controller. It would be good to have common format for LED effects.
>>
>> We still haven't tried trigger approach discussed over half a year ago.
>> If we used firmware approach we would still have to overcome the problem
>> of defining the LED class drivers affected by the firmware program.
> 
> The firmware approach is in the tree today :-(.

to RGB LEDs? What exactly do you have on mind?

> 
>>>> Device manufactures in Asia & North America requested lp55xx drivers, but I
>>>> don't know how many vendors uses the firmware I/F. Some vendors embeds the
>>>> binary code inside the driver instead of using user-program.
>>>
>>> Nokia N900 uses lp55xx, and I have custom scripts interfacing sysfs.
>>>
>>> Maemo uses the LEDs, too, but maemo is not open source.
>>>
>>> So no, I don't think there's anything important that could be broken.
>>
>> We can't guarantee that. Is there any problem in just using the
>> currently introduced DECLARE_FW_CUSTOM_FALLBACK() in
>> drivers/leds/leds-lp55xx-common.c?
> 
> Well, it would be good to get rid of the custom fallback
> functionality. And no, we don't need to "guarantee" that.  Removing
> obscure functionality noone uses is far game... providing noone
> complains ;-).

As Milo explained:

> Why has no one cried
> after the v4.0 custom fallback mechanism breaking ?

"Well, I don't know the reason exactly but my guess is they maybe still
using old kernel."

and after that:

"Device manufactures in Asia & North America requested lp55xx drivers"

These should be sufficient arguments for us for keeping the API
unchanged. If the users decided to upgrade their kernel then they
would be surprised by the API change.

DECLARE_FW_CUSTOM_FALLBACK macro seems to have been designed for
handling exactly this type of cases.

-- 
Best regards,
Jacek Anaszewski

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

[Jacek Anaszewski]

Hi Luis,

Thanks for the patch.

On 12/13/2016 04:08 AM, 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>
> 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 955c11d6ff9d..b51673e40439 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::

double colon at the end of line

>
>          $ 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)
>  {

For this LED class driver:

Acked-by: Jacek Anaszewski <j.anaszewski@samsung.com>


>  	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;
>  @@
>
>

-- 
Best regards,
Jacek Anaszewski

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

[Luis R. Rodriguez]

On Thu, Dec 15, 2016 at 10:32:12AM +0100, Jacek Anaszewski wrote:
> > diff --git a/Documentation/driver-api/firmware/fallback-mechanisms.rst b/Documentation/driver-api/firmware/fallback-mechanisms.rst
> > index 955c11d6ff9d..b51673e40439 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::
> 
> double colon at the end of line

That is on purpose for rst files, for use with the new trendy hipster
Sphinx documentation format.

> > 
> >          $ export COCCI=scripts/coccinelle/api/request_firmware-avoid-init-probe-init.cocci
> >          $ make coccicheck MODE=report

It will kind of blockquote the above.

> > 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)
> >  {
> 
> For this LED class driver:
> 
> Acked-by: Jacek Anaszewski <j.anaszewski@samsung.com>

Thanks, amended!

  Luis