[PATCH v1] firmware loader: introduce module parameter to customize fw search path

[PATCH v1] firmware loader: introduce module parameter to customize fw search path

[Ming Lei]

This patch introduces one module parameter of 'path' in firmware_class
to support customizing firmware image search path, so that people can
use its own firmware path if the default built-in paths can't meet their
demand[1], and the typical usage is passing the below from kernel command
parameter when 'firmware_class' is built in kernel:

	firmware_class.path=$CUSTOMIZED_PATH

[1], https://lkml.org/lkml/2012/10/11/337

Cc: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
V1:
	- remove kernel boot parameter and only support the feature by
	module parameter as suggested by Greg

 Documentation/firmware_class/README |    5 +++++
 drivers/base/firmware_class.c       |   23 +++++++++++++++++++++--
 2 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/Documentation/firmware_class/README b/Documentation/firmware_class/README
index 815b711..ce02744 100644
--- a/Documentation/firmware_class/README
+++ b/Documentation/firmware_class/README
@@ -22,12 +22,17 @@
 	- calls request_firmware(&fw_entry, $FIRMWARE, device)
 	- kernel searchs the fimware image with name $FIRMWARE directly
 	in the below search path of root filesystem:
+		User customized search path by module parameter 'path'[1]
 		"/lib/firmware/updates/" UTS_RELEASE,
 		"/lib/firmware/updates",
 		"/lib/firmware/" UTS_RELEASE,
 		"/lib/firmware"
 	- If found, goto 7), else goto 2)
 
+	[1], the 'path' is a string parameter which length should be less
+	than 256, user should pass 'firmware.path=$CUSTOMIZED_PATH' if
+	firmware_class is built in kernel(the general situation)
+
  2), userspace:
  	- /sys/class/firmware/xxx/{loading,data} appear.
 	- hotplug gets called with a firmware identifier in $FIRMWARE
diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 8945f4e..b363103 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -274,6 +274,16 @@ static const char *fw_path[] = {
 	"/lib/firmware"
 };
 
+static char fw_path_para[256];
+
+/*
+ * Typical usage is that passing 'firmware_class.path=$CUSTOMIZED_PATH'
+ * from kernel command because firmware_class is generally built in
+ * kernel instead of module.
+ */
+module_param_string(path, fw_path_para, sizeof(fw_path_para), 0644);
+MODULE_PARM_DESC(path, "customized firmware image search path with a higher priority than default path");
+
 /* Don't inline this: 'struct kstat' is biggish */
 static noinline long fw_file_size(struct file *file)
 {
@@ -313,9 +323,18 @@ static bool fw_get_filesystem_firmware(struct firmware_buf *buf)
 	bool success = false;
 	char *path = __getname();
 
-	for (i = 0; i < ARRAY_SIZE(fw_path); i++) {
+	for (i = -1; i < ARRAY_SIZE(fw_path); i++) {
 		struct file *file;
-		snprintf(path, PATH_MAX, "%s/%s", fw_path[i], buf->fw_id);
+
+		if (i < 0) {
+			if (!fw_path_para[0])	/* No customized path */
+				continue;
+			snprintf(path, PATH_MAX, "%s/%s", fw_path_para,
+				 buf->fw_id);
+		} else {
+			snprintf(path, PATH_MAX, "%s/%s", fw_path[i],
+				 buf->fw_id);
+		}
 
 		file = filp_open(path, O_RDONLY, 0);
 		if (IS_ERR(file))
-- 
1.7.9.5

Re: [PATCH v1] firmware loader: introduce module parameter to customize fw search path

[Linus Torvalds]

On Thu, Oct 25, 2012 at 5:46 PM, Ming Lei <ming.lei@canonical.com> wrote:
>                 struct file *file;
> -               snprintf(path, PATH_MAX, "%s/%s", fw_path[i], buf->fw_id);
> +
> +               if (i < 0) {
> +                       if (!fw_path_para[0])   /* No customized path */
> +                               continue;
> +                       snprintf(path, PATH_MAX, "%s/%s", fw_path_para,
> +                                buf->fw_id);
> +               } else {
> +                       snprintf(path, PATH_MAX, "%s/%s", fw_path[i],
> +                                buf->fw_id);
> +               }

Ugh. This is just disgusting.

Please just make "fw_path[0]" just be the pointer to fw_path_para[]
(which sounds like the cleanest fix) and get rid of the negative 'i'
and conditional entirely.

Or if there is some odd reason you don't want to do that, at least
make the conditional much smaller, without the snprintf() in both arms
(ie make the if-statement just set a "const char *dir" variable to
either fw_path[i] or fw_path_para or whatever).

Sure, the compiler *may* merge them (gcc does, but I've seen it miss
them too), but even if the compiler might fix up ugly code, that's not
a reason for it to be ugly in the source code anyway.

          Linus

Re: [PATCH v1] firmware loader: introduce module parameter to customize fw search path

[Ming Lei]

On Fri, Oct 26, 2012 at 10:32 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> Please just make "fw_path[0]" just be the pointer to fw_path_para[]
> (which sounds like the cleanest fix) and get rid of the negative 'i'
> and conditional entirely.

Yes, it should be the cleanest, I don't do it because I thought that might
have caused one compile warning('const char *' points to memory
without 'const', like below)

	static char fw_path_para[256];
	static const char *fw_path[] = {
		fw_path_para,
		"/lib/firmware/updates/" UTS_RELEASE,
		"/lib/firmware/updates",
		"/lib/firmware/" UTS_RELEASE,
		"/lib/firmware"
	};

but in fact there isn't any warning with above change and it does work, still
don't know why? :-(

Thanks,
--
Ming Lei

Re: [PATCH v1] firmware loader: introduce module parameter to customize fw search path

[Linus Torvalds]

On Thu, Oct 25, 2012 at 8:12 PM, Ming Lei <ming.lei@canonical.com> wrote:
>
> Yes, it should be the cleanest, I don't do it because I thought that might
> have caused one compile warning('const char *' points to memory
> without 'const', like below)

You can just keep the const.

In fact, you could even add one, and make it be

   static const char * const fw_path[] = {

We currently don't mark fw_path[] itself const (even though it is),
only the strings it points to.

> but in fact there isn't any warning with above change and it does work, still
> don't know why? :-(

It's valid to cast a non-const pointer to a const one. It's the
*other* way around that is invalid.

So marking fw_path[] as having 'const char *' elements just means that
we won't be changing those elements through the fw_path[] array
(correct: we only read them). The fact that one of those same pointers
is then also available through a non-const pointer variable means that
they can change through *that* pointer, but that doesn't change the
fact that fw_path[] itself contains const pointers.

Remember: in C, a "const pointer" does *not* mean that the thing it
points to cannot change. It only means that it cannot change through
*that* pointer.

           Linus

Re: [PATCH v1] firmware loader: introduce module parameter to customize fw search path

[Al Viro]

On Thu, Oct 25, 2012 at 08:38:25PM -0700, Linus Torvalds wrote:

> It's valid to cast a non-const pointer to a const one. It's the
> *other* way around that is invalid.
> 
> So marking fw_path[] as having 'const char *' elements just means that
> we won't be changing those elements through the fw_path[] array
> (correct: we only read them). The fact that one of those same pointers
> is then also available through a non-const pointer variable means that
> they can change through *that* pointer, but that doesn't change the
> fact that fw_path[] itself contains const pointers.
> 
> Remember: in C, a "const pointer" does *not* mean that the thing it
> points to cannot change. It only means that it cannot change through
> *that* pointer.

It's a bit trickier, unfortunately - pointer to pointer to const char
and pointer to pointer to char do not mix.  Just for fun, try to constify
envp and argv arguments of call_usermodehelper()...

Re: [PATCH v1] firmware loader: introduce module parameter to customize fw search path

[Kevin Easton]

Quoting Al Viro <viro@ZenIV.linux.org.uk>:

> On Thu, Oct 25, 2012 at 08:38:25PM -0700, Linus Torvalds wrote:
>
>> It's valid to cast a non-const pointer to a const one. It's the
>> *other* way around that is invalid.
>>
>> So marking fw_path[] as having 'const char *' elements just means that
>> we won't be changing those elements through the fw_path[] array
>> (correct: we only read them). The fact that one of those same pointers
>> is then also available through a non-const pointer variable means that
>> they can change through *that* pointer, but that doesn't change the
>> fact that fw_path[] itself contains const pointers.
>>
>> Remember: in C, a "const pointer" does *not* mean that the thing it
>> points to cannot change. It only means that it cannot change through
>> *that* pointer.
>
> It's a bit trickier, unfortunately - pointer to pointer to const char
> and pointer to pointer to char do not mix.  Just for fun, try to constify
> envp and argv arguments of call_usermodehelper()...

That's because if it _was_ allowed, you could use it to silently  
launder the const away:

const char *c = "rodata";
char *x;
const char **y;

y = &x;
*y = c;

/* We now have (const char) values accessible through a (char *) pointer x */