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.

v2: typos and small fixes

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

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.

Quoted 3 lines · 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.

Well, the biggest reason is that some distros still rely on this. I've seen new products being made that rely on it,

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

We can't get rid of it,

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

Quoted 7 lines · Greg KH

Quoted 3 lines · 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.

Well, the biggest reason is that some distros still rely on this. I've seen new products being made that rely on it,

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

Quoted 9 lines · Luis R. Rodriguez

Quoted 7 lines · Greg KH

Quoted 3 lines · 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.

Well, the biggest reason is that some distros still rely on this. I've seen new products being made that rely on it,

Quoted 6 lines

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

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

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

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

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

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 +5 -2

index b87a292153c6..73f509a8d2de 100644
@@ -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

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 +1 -0

index 5377f22ff994..04161428ee3b 100644
@@ -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;

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

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;

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

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

2.10.1

Quoted 92 lines · 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 <[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 +5 -2

index b87a292153c6..73f509a8d2de 100644
@@ -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

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 +1 -0

index 5377f22ff994..04161428ee3b 100644
@@ -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;

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

index c7598cfc4780..68cacab35b76 100644
@@ -17,6 +17,13 @@
 virtual report
 virtual context
 

scripts/coccinelle/api/request_firmware-custom-fallback.cocci position p; · line 37

Quoted 12 lines

scripts/coccinelle/api/request_firmware-custom-fallback.cocci +7 -0

+@ r0 depends on report || context @
+declarer name DECLARE_FW_CUSTOM_FALLBACK;
+expression E;
+@@
+
+DECLARE_FW_CUSTOM_FALLBACK(E);
+
 @ r1 depends on report || context @
 expression mod, name, dev, gfp, drv, cb;
 position p;

@@ -30,7 +37,7 @@ position p;
 *request_firmware_nowait@p(mod, FW_ACTION_NOHOTPLUG, name, dev, gfp, drv, cb)

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

Quoted 3 lines · Julia Lawall

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

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

Quoted 96 lines · Julia Lawall

Quoted 92 lines · 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 <[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 +5 -2

index b87a292153c6..73f509a8d2de 100644
@@ -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

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 +1 -0

index 5377f22ff994..04161428ee3b 100644
@@ -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;

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

index c7598cfc4780..68cacab35b76 100644
@@ -17,6 +17,13 @@
 virtual report
 virtual context
 

Quoted 16 lines

scripts/coccinelle/api/request_firmware-custom-fallback.cocci position p; · line 37

Quoted 12 lines

scripts/coccinelle/api/request_firmware-custom-fallback.cocci +7 -0

+@ r0 depends on report || context @
+declarer name DECLARE_FW_CUSTOM_FALLBACK;
+expression E;
+@@
+
+DECLARE_FW_CUSTOM_FALLBACK(E);
+
 @ r1 depends on report || context @
 expression mod, name, dev, gfp, drv, cb;
 position p;

@@ -30,7 +37,7 @@ position p;
 *request_firmware_nowait@p(mod, FW_ACTION_NOHOTPLUG, name, dev, gfp, drv, cb)

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

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

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

index 68cacab35b76..9548e5be9c0e 100644
@@ -24,7 +24,7 @@ expression E;
 
 DECLARE_FW_CUSTOM_FALLBACK(E);
 
-@ r1 depends on report || context @
+@ r1 depends on !r0 && report || context @
 expression mod, name, dev, gfp, drv, cb;
 position p;
 @@

Quoted 130 lines · Luis R. Rodriguez

Quoted 96 lines · Julia Lawall

Quoted 92 lines · 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 <[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 +5 -2

index b87a292153c6..73f509a8d2de 100644
@@ -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

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 +1 -0

index 5377f22ff994..04161428ee3b 100644
@@ -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;

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

index c7598cfc4780..68cacab35b76 100644
@@ -17,6 +17,13 @@
 virtual report
 virtual context
 

Quoted 16 lines

scripts/coccinelle/api/request_firmware-custom-fallback.cocci position p; · line 37

Quoted 12 lines

scripts/coccinelle/api/request_firmware-custom-fallback.cocci +7 -0

+@ r0 depends on report || context @
+declarer name DECLARE_FW_CUSTOM_FALLBACK;
+expression E;
+@@
+
+DECLARE_FW_CUSTOM_FALLBACK(E);
+
 @ r1 depends on report || context @
 expression mod, name, dev, gfp, drv, cb;
 position p;

@@ -30,7 +37,7 @@ position p;
 *request_firmware_nowait@p(mod, FW_ACTION_NOHOTPLUG, name, dev, gfp, drv, cb)

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

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

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

index 68cacab35b76..9548e5be9c0e 100644
@@ -24,7 +24,7 @@ expression E;
 
 DECLARE_FW_CUSTOM_FALLBACK(E);
 
-@ r1 depends on report || context @
+@ r1 depends on !r0 && report || context @
 expression mod, name, dev, gfp, drv, cb;
 position p;
 @@

scripts/coccinelle/api/request_firmware-custom-fallback.cocci position p; · line 42

Quoted 7 lines

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

@@ -37,7 +37,7 @@ position p;
 *request_firmware_nowait@p(mod, FW_ACTION_NOHOTPLUG, name, dev, gfp, drv, cb)
 )
 
-@script:python depends on report && !r0 @
+@script:python depends on report && r1 @
 p << r1.p;
 @@

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

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