* [PATCH v1] firmware loader: introduce module parameter to customize fw search path
@ 2012-10-26 0:46 Ming Lei
2012-10-26 2:32 ` Linus Torvalds
0 siblings, 1 reply; 6+ messages in thread
From: Ming Lei @ 2012-10-26 0:46 UTC (permalink / raw)
To: Greg Kroah-Hartman; +Cc: linux-kernel, Ming Lei, Linus Torvalds
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v1] firmware loader: introduce module parameter to customize fw search path
2012-10-26 0:46 [PATCH v1] firmware loader: introduce module parameter to customize fw search path Ming Lei
@ 2012-10-26 2:32 ` Linus Torvalds
2012-10-26 3:12 ` Ming Lei
0 siblings, 1 reply; 6+ messages in thread
From: Linus Torvalds @ 2012-10-26 2:32 UTC (permalink / raw)
To: Ming Lei; +Cc: Greg Kroah-Hartman, linux-kernel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v1] firmware loader: introduce module parameter to customize fw search path
2012-10-26 2:32 ` Linus Torvalds
@ 2012-10-26 3:12 ` Ming Lei
2012-10-26 3:38 ` Linus Torvalds
0 siblings, 1 reply; 6+ messages in thread
From: Ming Lei @ 2012-10-26 3:12 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Greg Kroah-Hartman, linux-kernel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v1] firmware loader: introduce module parameter to customize fw search path
2012-10-26 3:12 ` Ming Lei
@ 2012-10-26 3:38 ` Linus Torvalds
2012-10-26 3:52 ` Al Viro
0 siblings, 1 reply; 6+ messages in thread
From: Linus Torvalds @ 2012-10-26 3:38 UTC (permalink / raw)
To: Ming Lei; +Cc: Greg Kroah-Hartman, linux-kernel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v1] firmware loader: introduce module parameter to customize fw search path
2012-10-26 3:38 ` Linus Torvalds
@ 2012-10-26 3:52 ` Al Viro
2012-10-26 5:57 ` Kevin Easton
0 siblings, 1 reply; 6+ messages in thread
From: Al Viro @ 2012-10-26 3:52 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Ming Lei, Greg Kroah-Hartman, linux-kernel
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()...
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v1] firmware loader: introduce module parameter to customize fw search path
2012-10-26 3:52 ` Al Viro
@ 2012-10-26 5:57 ` Kevin Easton
0 siblings, 0 replies; 6+ messages in thread
From: Kevin Easton @ 2012-10-26 5:57 UTC (permalink / raw)
To: Al Viro; +Cc: Linus Torvalds, Ming Lei, Greg Kroah-Hartman, linux-kernel
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 */
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-10-26 6:06 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-10-26 0:46 [PATCH v1] firmware loader: introduce module parameter to customize fw search path Ming Lei
2012-10-26 2:32 ` Linus Torvalds
2012-10-26 3:12 ` Ming Lei
2012-10-26 3:38 ` Linus Torvalds
2012-10-26 3:52 ` Al Viro
2012-10-26 5:57 ` Kevin Easton
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.