firmware: doc revamp

Understanding this code is getting out of control without any notes. Give the firmware_class driver a much needed documentation love, and while at it convert it to the new sphinx documentation format.

Documentation/driver-api/firmware/fallback-mechanisms.rst new file +11 -0

new file mode 100644
index 000000000000..edce1d76ce29
@@ -0,0 +1,195 @@
+===================
+Fallback mechanisms
+===================
+
+A fallback mechanism is supported to allow to overcome failures to do a direct
+filesystem lookup on the root filesystem or when the firmware simply cannot be
+installed for practical reasons on the root filesystem. The kernel
+configuration options related to supporting the firmware fallback mechanism are:
+
+  * CONFIG_FW_LOADER_USER_HELPER: enables building the firmware fallback
+    mechanism. Most distributions enable this option today. If enabled but

Quoted 3 lines · Luis R. Rodriguez

Understanding this code is getting out of control without any notes. Give the firmware_class driver a much needed documentation love, and while at it convert it to the new sphinx documentation format.

It does help to understand the class/API, thank you!

Documentation/driver-api/firmware/built-in-fw.rst line 31

Quoted 7 lines

Documentation/driver-api/firmware/built-in-fw.rst new file +7 -0

+Even if you have these needs there are a few reasons why you may not be
+able to make use of built-in firmware:
+
+* Legalese - firmware is non-GPL compatible
+* Some firmware may be optional
+* Firmware upgrades are possible, therefore a new firmware would implicate
+  a complete firmware rebuild.

Could it be you meant "kernel rebuild" or "vmlinux rebuild" here?

Quoted 21 lines

Documentation/driver-api/firmware/fallback-mechanisms.rst new file +15 -0

new file mode 100644
index 000000000000..edce1d76ce29
@@ -0,0 +1,195 @@
+===================
+Fallback mechanisms
+===================
+
+A fallback mechanism is supported to allow to overcome failures to do a direct
+filesystem lookup on the root filesystem or when the firmware simply cannot be
+installed for practical reasons on the root filesystem. The kernel
+configuration options related to supporting the firmware fallback mechanism are:
+
+  * CONFIG_FW_LOADER_USER_HELPER: enables building the firmware fallback
+    mechanism. Most distributions enable this option today. If enabled but
+    CONFIG_FW_LOADER_USER_HELPER_FALLBACK is disabled, only the custom fallback
+    mechanism is available and for the request_firmware_nowait() call.
+  * CONFIG_FW_LOADER_USER_HELPER_FALLBACK: force enables each request to
+    enable the kobject uevent fallback mechanism on all firmare API calls

Documentation/driver-api/firmware/fallback-mechanisms.rst line 21

Quoted 6 lines

Documentation/driver-api/firmware/fallback-mechanisms.rst new file +6 -0

+    except request_firmware_direct(). Most distributions disable this option
+    today. The call request_firmware_nowait() allows for one alternative
+    fallback mechanism: if this kconfig option is enabled and your second
+    argument to request_firmware_nowait(), uevent, is set to false you are
+    informing the kernel that you have a custom fallback mechanism and it will
+    manually load the firmware. Read below for more details.

Yeah, it really asks for API simplification ;)

+which enable firmware to be built as part of the kernel build process. This

Documentation/driver-api/firmware/built-in-fw.rst new file +7 -0

+option, CONFIG_FIRMWARE_IN_KERNEL, will build all firmware for all drivers
+enabled which ship its firmware inside the Linux kernel source tree.
+
+There are a few reasons why you might want to consider building your firmware
+into the kernel with CONFIG_EXTRA_FIRMWARE though:
+
+* Speed

+++ b/Documentation/driver-api/firmware/fallback-mechanisms.rst

Documentation/driver-api/firmware/fallback-mechanisms.rst new file +5 -0

@@ -0,0 +1,195 @@
+===================
+Fallback mechanisms
+===================
+
+A fallback mechanism is supported to allow to overcome failures to do a direct

Documentation/driver-api/firmware/fallback-mechanisms.rst new file +26 -0

+    except request_firmware_direct(). Most distributions disable this option
+    today. The call request_firmware_nowait() allows for one alternative
+    fallback mechanism: if this kconfig option is enabled and your second
+    argument to request_firmware_nowait(), uevent, is set to false you are
+    informing the kernel that you have a custom fallback mechanism and it will
+    manually load the firmware. Read below for more details.
+
+Note that this means when having this configuration:
+
+CONFIG_FW_LOADER_USER_HELPER=y
+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.
+
+Justifying the firmware fallback mechanism
+==========================================
+
+Direct filesystem lookups may fail for a variety of reasons. Known reasons for
+this are worth itemizing and documenting as it justifies the need for the
+fallback mechanism:
+
+* Race against access with the root filesystem upon bootup.
+
+* Races upon resume from suspend. This is resolved by the firmware cache, but
+  the firmware cache is only supported if you use uevents, and its not

Quoted 6 lines

+* Some firmware files may be really large in size. The remote-proc subsystem

(quoted diff) +2 -0

+  is an example subsystem which deals with these sorts of firmware
+* The firmware may need to be scraped out from some device specific

location
+ dynamically, an example is calibration data for for some WiFi chipsets.

Maybe it is worth to mention, that the calibration data is unique to a given chip, so it is individual. That is you would need to built for each device you sell its own kernel.

Quoted 11 lines · Daniel Wagner

Quoted 6 lines

+* Some firmware files may be really large in size. The remote-proc subsystem

(quoted diff) +2 -0

+  is an example subsystem which deals with these sorts of firmware
+* The firmware may need to be scraped out from some device specific

location
+ dynamically, an example is calibration data for for some WiFi chipsets.

Maybe it is worth to mention, that the calibration data is unique to a given chip, so it is individual. That is you would need to built for each device you sell its own kernel.

It's commonly unique to the device model, not a chip. The same chip can be used with different power amplifiers or different antennas. That's why you need model (board) specific calibration data.

From what I recall in my 802.11 days this can be often very specific

Quoted 7 lines

Quoted 3 lines

Maybe it is worth to mention, that the calibration data is unique to a given chip, so it is individual. That is you would need to built for each device you sell its own kernel.

It's commonly unique to the device model, not a chip. The same chip can be used with different power amplifiers or different antennas. That's why you need model (board) specific calibration data.

From what I recall in my 802.11 days this can be often very specific per *batch* of cards not just chip/device model, so this depends on the manufacturing process and date.

Documentation/driver-api/firmware/built-in-fw.rst line 7

Quoted 9 lines

+++ b/Documentation/driver-api/firmware/built-in-fw.rst

Documentation/driver-api/firmware/built-in-fw.rst new file +7 -0

@@ -0,0 +1,36 @@
+=================
+Built-in firmware
+=================
+
+Firmware can be built-in to the kernel, that is built-in to vmlinux,
+to enable firmware lookups to avoid having to look for firmware from
+the filesystem.

I find the second part of the sentence a bit confusing in the wording.

Documentation/driver-api/firmware/built-in-fw.rst line 13

Quoted 7 lines

You can enable built-in firmware using the kernel

Documentation/driver-api/firmware/built-in-fw.rst new file +6 -0

+configuration options:
+
+  * CONFIG_EXTRA_FIRMWARE
+  * CONFIG_EXTRA_FIRMWARE_DIR
+
+The should not be confused with CONFIG_FIRMWARE_IN_KERNEL, this is for drivers

s/The/This/ ?

Quoted 1 line

+which enable firmware to be built as part of the kernel build process. This

enables?

Quoted 11 lines

Documentation/driver-api/firmware/built-in-fw.rst new file +11 -0

+option, CONFIG_FIRMWARE_IN_KERNEL, will build all firmware for all drivers
+enabled which ship its firmware inside the Linux kernel source tree.
+
+There are a few reasons why you might want to consider building your firmware
+into the kernel with CONFIG_EXTRA_FIRMWARE though:
+
+* Speed
+* Firmware is needed for accessing the boot device, and the user doesn't
+  want to stuff the firmware into the boot initramfs.
+
+Even if you have these needs there are a few reasons why you may not be

Documentation/driver-api/firmware/built-in-fw.rst line 31

Quoted 6 lines

Documentation/driver-api/firmware/built-in-fw.rst new file +6 -0

+able to make use of built-in firmware:
+
+* Legalese - firmware is non-GPL compatible
+* Some firmware may be optional
+* Firmware upgrades are possible, therefore a new firmware would implicate
+  a complete firmware rebuild.

kernel rebuild?

Documentation/driver-api/firmware/built-in-fw.rst line 35

Quoted 4 lines

Documentation/driver-api/firmware/built-in-fw.rst new file +4 -0

+* Some firmware files may be really large in size. The remote-proc subsystem
+  is an example subsystem which deals with these sorts of firmware
+* The firmware may need to be scraped out from some device specific location
+  dynamically, an example is calibration data for for some WiFi chipsets.

Maybe it is worth to mention, that the calibration data is unique to a given chip, so it is individual. That is you would need to built for each device you sell its own kernel.

[...]

Quoted 11 lines

+++ b/Documentation/driver-api/firmware/fallback-mechanisms.rst

Documentation/driver-api/firmware/fallback-mechanisms.rst new file +9 -0

@@ -0,0 +1,195 @@
+===================
+Fallback mechanisms
+===================
+
+A fallback mechanism is supported to allow to overcome failures to do a direct
+filesystem lookup on the root filesystem or when the firmware simply cannot be
+installed for practical reasons on the root filesystem. The kernel
+configuration options related to supporting the firmware fallback mechanism are:
+

Documentation/driver-api/firmware/fallback-mechanisms.rst line 15

Quoted 6 lines

Documentation/driver-api/firmware/fallback-mechanisms.rst new file +6 -0

+  * CONFIG_FW_LOADER_USER_HELPER: enables building the firmware fallback
+    mechanism. Most distributions enable this option today. If enabled but
+    CONFIG_FW_LOADER_USER_HELPER_FALLBACK is disabled, only the custom fallback
+    mechanism is available and for the request_firmware_nowait() call.
+  * CONFIG_FW_LOADER_USER_HELPER_FALLBACK: force enables each request to
+    enable the kobject uevent fallback mechanism on all firmare API calls

s/firmare/firmware/

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.

v2: change dependencies on rules make more sense, and fixed
context mode

Cc Fengguang Wu <[email protected]>

Cc Richard Purdie <[email protected]>

Cc Jacek Anaszewski <[email protected]>

Cc [email protected]

Cc Abhay Salunke <[email protected]>

Acked-by Jacek Anaszewski <[email protected]>

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

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

Documentation/driver-api/firmware/fallback-mechanisms.rst +0 -0

index b87a292153c6..73f509a8d2de 100644

drivers/leds/leds-lp55xx-common.c +0 -0

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

include/linux/firmware.h +0 -0

index b1f9f0ccb8ac..e6ca19c03dcc 100644

Quoted 1 line · Luis R. Rodriguez

+Invalid users of the custom fallback mechanism can be policed using::

Ick, no, why? Why not just add a checkpatch rule instead?

Quoted 11 lines

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

drivers/firmware/dell_rbu.c +0 -0

index 2f452f1f7c8a..3f2aa35bc54d 100644
@@ -586,6 +586,7 @@ static ssize_t read_rbu_image_type(struct file *filp, struct kobject *kobj,
 	return size;
 }
 

drivers/firmware/dell_rbu.c static ssize_t read_rbu_image_type(struct file *filp, struct kobject *kobj, · line 589

Quoted 1 line

drivers/firmware/dell_rbu.c +1 -0

+DECLARE_FW_CUSTOM_FALLBACK("Documentation/dell_rbu.txt");

That's a pain.

Quoted 8 lines

include/linux/firmware.h +0 -0

index b1f9f0ccb8ac..e6ca19c03dcc 100644
@@ -8,6 +8,13 @@
 #define FW_ACTION_NOHOTPLUG 0
 #define FW_ACTION_HOTPLUG 1
 

include/linux/firmware.h line 16

Quoted 6 lines

include/linux/firmware.h +6 -0

+/*
+ * 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)

So you really don't need to put anything "valid" in the define argument? This feels like such a horrid hack, I really don't like it, especially as we don't do it anywhere else in the kernel, right? Why start now?

Quoted 4 lines · Greg KH

Quoted 1 line · Luis R. Rodriguez

+Invalid users of the custom fallback mechanism can be policed using::

Ick, no, why? Why not just add a checkpatch rule instead?

If its easy to do, how would we do that?

Quoted 10 lines

Quoted 10 lines

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

drivers/firmware/dell_rbu.c +0 -0

index 2f452f1f7c8a..3f2aa35bc54d 100644
@@ -586,6 +586,7 @@ static ssize_t read_rbu_image_type(struct file *filp, struct kobject *kobj,
 	return size;
 }
 

Quoted 3 lines

drivers/firmware/dell_rbu.c static ssize_t read_rbu_image_type(struct file *filp, struct kobject *kobj, · line 589

Quoted 1 line

drivers/firmware/dell_rbu.c +1 -0

+DECLARE_FW_CUSTOM_FALLBACK("Documentation/dell_rbu.txt");

That's a pain.

It is easier with checkpatch?

In short, don't worry about this, it's not a big deal nor really worth the effort.

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 <[email protected]>

Cc Jacek Anaszewski <[email protected]>

Cc [email protected]

Cc Abhay Salunke <[email protected]>

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

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

Documentation/driver-api/firmware/fallback-mechanisms.rst +17 -0

index edce1d76ce29..955c11d6ff9d 100644
@@ -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
 =========================
 

scripts/coccinelle/api/request_firmware-custom-fallback.cocci new file +37 -1

new file mode 100644
index 000000000000..c7598cfc4780
@@ -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 <[email protected]> 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

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 <[email protected]>

Cc Jacek Anaszewski <[email protected]>

Cc [email protected]

Cc Abhay Salunke <[email protected]>

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

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

Documentation/driver-api/firmware/fallback-mechanisms.rst +17 -0

index edce1d76ce29..955c11d6ff9d 100644
@@ -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
 =========================
 

scripts/coccinelle/api/request_firmware-custom-fallback.cocci new file +37 -0

new file mode 100644
index 000000000000..c7598cfc4780
@@ -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 <[email protected]> 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")

Could you please verify if leds-lp55xx-common.c driver really needs a custom firmware loading fallback mechanism?

Hi Jacek,

Quoted 3 lines · Jacek Anaszewski

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.

Quoted 10 lines · Milo Kim

Hi Jacek,

Quoted 3 lines · Jacek Anaszewski

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.

Quoted 4 lines

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!

Quoted 2 lines

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.

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.

Hi!

Quoted 10 lines

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.

drivers/leds/leds-lp55xx-common.c static void lp55xx_firmware_loaded(const struct firmware *fw, void *context) · line 225

Quoted 7 lines

drivers/leds/leds-lp55xx-common.c +1 -0

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

Quoted 41 lines · Pavel Machek

Hi!

Quoted 10 lines

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.

drivers/leds/leds-lp55xx-common.c static void lp55xx_firmware_loaded(const struct firmware *fw, void *context) · line 225

Quoted 7 lines

drivers/leds/leds-lp55xx-common.c +1 -0

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

Quoted 1 line

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:

Quoted 42 lines · Luis R. Rodriguez

Quoted 41 lines · Pavel Machek

Hi!

Quoted 10 lines

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.

drivers/leds/leds-lp55xx-common.c static void lp55xx_firmware_loaded(const struct firmware *fw, void *context) · line 225

Quoted 7 lines

drivers/leds/leds-lp55xx-common.c +1 -0

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

Quoted 5 lines

Quoted 1 line

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.

Quoted 49 lines · Pavel Machek

Quoted 47 lines · Luis R. Rodriguez

Quoted 41 lines · Pavel Machek

Hi!

Quoted 10 lines

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.

drivers/leds/leds-lp55xx-common.c static void lp55xx_firmware_loaded(const struct firmware *fw, void *context) · line 225

Quoted 7 lines

drivers/leds/leds-lp55xx-common.c +1 -0

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

Quoted 1 line

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:

Quoted 2 lines

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

Quoted 4 lines · Pavel Machek

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.

Quoted 7 lines · Luis R. Rodriguez

Quoted 4 lines · Pavel Machek

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?

Quoted 7 lines · Luis R. Rodriguez

Quoted 4 lines · Pavel Machek

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

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 ?

Quoted 3 lines

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

Quoted 2 lines

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.

Quoted 1 line

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.

Hi!

Quoted 10 lines · Luis R. Rodriguez

Quoted 3 lines

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.

Quoted 3 lines

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.

Quoted 4 lines

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.

Quoted 7 lines

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

Quoted 2 lines

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

Quoted 8 lines

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

Quoted 1 line

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.

Quoted 13 lines

Quoted 12 lines

Quoted 3 lines

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

Quoted 3 lines

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.

Quoted 6 lines

Quoted 3 lines

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.

Quoted 3 lines

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?

Hi!

Quoted 17 lines

Quoted 16 lines

Quoted 4 lines

Quoted 3 lines

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?

Quoted 8 lines

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.

Quoted 3 lines

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

Quoted 11 lines

Quoted 10 lines

Quoted 4 lines

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.

Quoted 3 lines

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

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

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 <[email protected]>

Cc Richard Purdie <[email protected]>

Cc Jacek Anaszewski <[email protected]>

Cc [email protected]

Cc Abhay Salunke <[email protected]>

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

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

Documentation/driver-api/firmware/fallback-mechanisms.rst +0 -0

index 955c11d6ff9d..b51673e40439 100644

Documentation/driver-api/firmware/fallback-mechanisms.rst +0 -0

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

drivers/firmware/dell_rbu.c +1 -0

index 2f452f1f7c8a..3f2aa35bc54d 100644
@@ -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)

drivers/leds/leds-lp55xx-common.c +0 -0

index 5377f22ff994..04161428ee3b 100644

drivers/leds/leds-lp55xx-common.c +0 -0

 	const char *name = chip->cl->name;

include/linux/firmware.h +7 -0

index b1f9f0ccb8ac..e6ca19c03dcc 100644
@@ -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;

scripts/coccinelle/api/request_firmware-custom-fallback.cocci +8 -1

index c7598cfc4780..68cacab35b76 100644
@@ -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;
 @@
 

Quoted 6 lines

Documentation/driver-api/firmware/fallback-mechanisms.rst +0 -0

index 955c11d6ff9d..b51673e40439 100644
@@ -184,8 +184,11 @@ load firmware for you through a custom path.
 

Documentation/driver-api/firmware/fallback-mechanisms.rst load firmware for you through a custom path. · line 191

Quoted 9 lines

Documentation/driver-api/firmware/fallback-mechanisms.rst +5 -2

 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

Documentation/driver-api/firmware/fallback-mechanisms.rst load firmware for you through a custom path. · line 194

Quoted 3 lines

Documentation/driver-api/firmware/fallback-mechanisms.rst +0 -0

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

Quoted 8 lines

drivers/leds/leds-lp55xx-common.c +0 -0

index 5377f22ff994..04161428ee3b 100644
@@ -219,6 +219,7 @@ static void lp55xx_firmware_loaded(const struct firmware *fw, void *context)
 	release_firmware(chip->fw);
 }
 

drivers/leds/leds-lp55xx-common.c static void lp55xx_firmware_loaded(const struct firmware *fw, void *context) · line 224

Quoted 3 lines

drivers/leds/leds-lp55xx-common.c +1 -0

+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 <[email protected]>

You may notice I've dropped the SmPL patches which complain on use of the API on init and probe -- although valid the context was off given the only valid use case was if you don't use initramfs, and that's a corner case. Fortunatley Daniel Wagner and Tom Gundersen have come up with some ideas that should help correct these issues, so I've dropped that grammar patch.