Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1540809 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 4/5] firmware: add SmPL report for custom fallback mechanism "Luis R. Rodriguez" <mcgrof@kernel.org> - 2016-12-13 04:10 +0100
    Re: [PATCH 4/5] firmware: add SmPL report for custom fallback  mechanism Julia Lawall <julia.lawall@lip6.fr> - 2016-12-13 07:20 +0100
    Re: [PATCH 4/5] firmware: add SmPL report for custom fallback mechanism Jacek Anaszewski <j.anaszewski@samsung.com> - 2016-12-13 10:50 +0100
      Re: [PATCH 4/5] firmware: add SmPL report for custom fallback  mechanism Milo Kim <woogyom.kim@gmail.com> - 2016-12-14 03:00 +0100
        Re: [PATCH 4/5] firmware: add SmPL report for custom fallback  mechanism "Luis R. Rodriguez" <mcgrof@kernel.org> - 2016-12-16 10:40 +0100

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

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

[toc] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

#1540944

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web