[PATCH] firmware loader: log path to loaded firmwares

[PATCH] firmware loader: log path to loaded firmwares

[Drew DeVault]

This is useful for users who are trying to identify the firmwares in use
on their system.

Signed-off-by: Drew DeVault <sir@cmpwn.com>
---
 drivers/base/firmware_loader/main.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/base/firmware_loader/main.c b/drivers/base/firmware_loader/main.c
index bf44c79beae9..139d40a2f423 100644
--- a/drivers/base/firmware_loader/main.c
+++ b/drivers/base/firmware_loader/main.c
@@ -34,6 +34,7 @@
 #include <linux/reboot.h>
 #include <linux/security.h>
 #include <linux/xz.h>
+#include <linux/kernel.h>
 
 #include <generated/utsrelease.h>
 
@@ -504,6 +505,7 @@ fw_get_filesystem_firmware(struct device *device, struct fw_priv *fw_priv,
 					 path);
 			continue;
 		}
+		printk(KERN_INFO "Loading firmware from %s\n", path);
 		if (decompress) {
 			dev_dbg(device, "f/w decompressing %s\n",
 				fw_priv->fw_name);
-- 
2.23.0

Re: [PATCH] firmware loader: log path to loaded firmwares

[Greg Kroah-Hartman]

On Sun, Nov 03, 2019 at 12:38:38PM -0500, Drew DeVault wrote:
> This is useful for users who are trying to identify the firmwares in use
> on their system.
> 
> Signed-off-by: Drew DeVault <sir@cmpwn.com>
> ---
>  drivers/base/firmware_loader/main.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/base/firmware_loader/main.c b/drivers/base/firmware_loader/main.c
> index bf44c79beae9..139d40a2f423 100644
> --- a/drivers/base/firmware_loader/main.c
> +++ b/drivers/base/firmware_loader/main.c
> @@ -34,6 +34,7 @@
>  #include <linux/reboot.h>
>  #include <linux/security.h>
>  #include <linux/xz.h>
> +#include <linux/kernel.h>
>  
>  #include <generated/utsrelease.h>
>  
> @@ -504,6 +505,7 @@ fw_get_filesystem_firmware(struct device *device, struct fw_priv *fw_priv,
>  					 path);
>  			continue;
>  		}
> +		printk(KERN_INFO "Loading firmware from %s\n", path);

And it's totally noisy :(

Also, if you have a 'struct device' you should always use the dev_*()
calls instead, which will show you exactly what device is asking for
what.

Please just make this a debug call, that way you can turn it on
dynamically if you really want to see what firmware is attempting to be
loaded.

thanks,

greg k-h

Re: [PATCH] firmware loader: log path to loaded firmwares

[Drew DeVault]

On Sun Nov 3, 2019 at 6:50 PM Greg Kroah-Hartman wrote:
> And it's totally noisy :(
> 
> Please just make this a debug call, that way you can turn it on
> dynamically if you really want to see what firmware is attempting to be
> loaded.

The typical setup won't need more than say, 10-20 firmwares? On my
system I need 13, and 12 of them are just for AMDGPU. In the 20 minutes
since I rebooted to this kernel, it constitutes less than 1% of my dmesg
volume, and will only get less so over time unless I start hotplugging
stuff (in which case, their respective drivers are likely to make noise,
too). In practice, I don't think it'll be especially noisy.

On the other hand, enabling debug logs just to get this information
would generate heaps of noise for a little bit of signal. This use-case
isn't the exceptional case for me, on my systems I only install the
firmwares I need so this is something I would reach for every time I set
up a new system.

> Also, if you have a 'struct device' you should always use the dev_*()
> calls instead, which will show you exactly what device is asking for
> what.

Understood.

Re: [PATCH] firmware loader: log path to loaded firmwares

[Greg Kroah-Hartman]

On Sun, Nov 03, 2019 at 12:55:18PM -0500, Drew DeVault wrote:
> On Sun Nov 3, 2019 at 6:50 PM Greg Kroah-Hartman wrote:
> > And it's totally noisy :(
> > 
> > Please just make this a debug call, that way you can turn it on
> > dynamically if you really want to see what firmware is attempting to be
> > loaded.
> 
> The typical setup won't need more than say, 10-20 firmwares? On my
> system I need 13, and 12 of them are just for AMDGPU. In the 20 minutes
> since I rebooted to this kernel, it constitutes less than 1% of my dmesg
> volume, and will only get less so over time unless I start hotplugging
> stuff (in which case, their respective drivers are likely to make noise,
> too). In practice, I don't think it'll be especially noisy.
> 
> On the other hand, enabling debug logs just to get this information
> would generate heaps of noise for a little bit of signal. This use-case
> isn't the exceptional case for me, on my systems I only install the
> firmwares I need so this is something I would reach for every time I set
> up a new system.

A "normal" system should not have any messages printing to the kernel
log.  All of this "look ma, I'm now loaded!" doesn't help anyone.
Printing errors is the key, that way if you see a message, it means you
need to take care of it.

If a firmware file is NOT found that is asked for, then yes, a message
should be printed out.  Isn't that the case if you do not have the
correct firmware file present?

Please never print out "Here is what the code is doing now!" type
messages to the info log, that's just noise.

thanks,

greg k-h

Re: [PATCH] firmware loader: log path to loaded firmwares

[Drew DeVault]

Fair enough, v2 sent. Thanks for the feedback!