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 <[email protected]>
Signed-off-by: Ming Lei <[email protected]>
---
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
On Thu, Oct 25, 2012 at 5:46 PM, Ming Lei <[email protected]> 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
On Fri, Oct 26, 2012 at 10:32 AM, Linus Torvalds
<[email protected]> 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
On Thu, Oct 25, 2012 at 8:12 PM, Ming Lei <[email protected]> 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
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()...
Quoting Al Viro <[email protected]>:
> 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 */