~sircmpwn/public-inbox

This thread contains a patchset. You're looking at the original emails, but you may wish to use the patch review UI. Review patch
4 3

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

Details
Message ID
<20191103180646.34880-1-sir@cmpwn.com>
DKIM signature
pass
Download raw message
Patch: +1 -0
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>
---
v2 uses dev_dbg instead of printk(KERN_INFO)

 drivers/base/firmware_loader/main.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/base/firmware_loader/main.c b/drivers/base/firmware_loader/main.c
index bf44c79beae9..2537da43a572 100644
--- a/drivers/base/firmware_loader/main.c
+++ b/drivers/base/firmware_loader/main.c
@@ -504,6 +504,7 @@ fw_get_filesystem_firmware(struct device *device, struct fw_priv *fw_priv,
 					 path);
 			continue;
 		}
+		dev_dbg(device, "Loading firmware from %s\n", path);
 		if (decompress) {
 			dev_dbg(device, "f/w decompressing %s\n",
 				fw_priv->fw_name);
-- 
2.23.0
Luis Chamberlain
Details
Message ID
<20191113005628.GT11244@42.do-not-panic.com>
In-Reply-To
<20191103180646.34880-1-sir@cmpwn.com> (view parent)
DKIM signature
missing
Download raw message
On Sun, Nov 03, 2019 at 01:06:46PM -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>
> ---
> v2 uses dev_dbg instead of printk(KERN_INFO)
> 
>  drivers/base/firmware_loader/main.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/base/firmware_loader/main.c b/drivers/base/firmware_loader/main.c
> index bf44c79beae9..2537da43a572 100644
> --- a/drivers/base/firmware_loader/main.c
> +++ b/drivers/base/firmware_loader/main.c
> @@ -504,6 +504,7 @@ fw_get_filesystem_firmware(struct device *device, struct fw_priv *fw_priv,
>  					 path);
>  			continue;
>  		}
> +		dev_dbg(device, "Loading firmware from %s\n", path);

Because this is dev_dbg() I'm willing to consider it, so that its not
always enabled. However its not in the right place, the code path you
are addressing is only for direct filesystem lookups. If that fails
some systems do a fallback call out to userspace. To cover both cases,
you want it at the end of _request_firmware() on the success path. Can
you send a new patch?

  Luis

>  		if (decompress) {
>  			dev_dbg(device, "f/w decompressing %s\n",
>  				fw_priv->fw_name);
> -- 
> 2.23.0
> 
Details
Message ID
<BYETRH5DWPRO.5M04O9X5SJ6Y@homura>
In-Reply-To
<20191113005628.GT11244@42.do-not-panic.com> (view parent)
DKIM signature
pass
Download raw message
Sure, no problem. Thanks for clarifying.
Robin H. Johnson
Details
Message ID
<robbat2-20191113T195158-869302266Z@orbis-terrarum.net>
In-Reply-To
<20191113005628.GT11244@42.do-not-panic.com> (view parent)
DKIM signature
missing
Download raw message
On Wed, Nov 13, 2019 at 12:56:28AM +0000, Luis Chamberlain wrote:
> On Sun, Nov 03, 2019 at 01:06:46PM -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>
> > ---
> > v2 uses dev_dbg instead of printk(KERN_INFO)
> > 
> >  drivers/base/firmware_loader/main.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/base/firmware_loader/main.c b/drivers/base/firmware_loader/main.c
> > index bf44c79beae9..2537da43a572 100644
> > --- a/drivers/base/firmware_loader/main.c
> > +++ b/drivers/base/firmware_loader/main.c
> > @@ -504,6 +504,7 @@ fw_get_filesystem_firmware(struct device *device, struct fw_priv *fw_priv,
> >  					 path);
> >  			continue;
> >  		}
> > +		dev_dbg(device, "Loading firmware from %s\n", path);
> 
> Because this is dev_dbg() I'm willing to consider it, so that its not
> always enabled. However its not in the right place, the code path you
> are addressing is only for direct filesystem lookups. If that fails
> some systems do a fallback call out to userspace. To cover both cases,
> you want it at the end of _request_firmware() on the success path. Can
> you send a new patch?
As the author of a separate patch that predates Drew's patch (originally
in July, with a later version sent to the list last week):
https://pastebin.com/Tf09x3ed (v1)
https://lkml.org/lkml/2019/11/7/800 (v2)

This already uses the _request_firmware path to fire after the firmware
was successfully loaded. The commit message is also specific that it's
to cover early boot situations, before UEVENT can be logged.

dev_dbg means that the loglevel must have been set to debug BEFORE the
firmware load took place, but this means either setting system-wide
debug spam or requiring debugfs, which is annoying for boot stuff (not
impossible, just annoying).

I have two uses cases overall:
- log so you know exactly when it's loaded successfully (great if
  loading a firmware causes your system to lock up a few seconds later)
- at some point in the future, being able to query what firmware was
  loaded in the past, and esp. exactly what version/data was in that
  firmware file.

-- 
Robin Hugh Johnson
Gentoo Linux: Dev, Infra Lead, Foundation Treasurer
E-Mail   : robbat2@gentoo.org
GnuPG FP : 11ACBA4F 4778E3F6 E4EDF38E B27B944E 34884E85
GnuPG FP : 7D0B3CEB E9B85B1F 825BCECF EE05E6F6 A48F6136
Luis Chamberlain
Details
Message ID
<20191113205010.GY11244@42.do-not-panic.com>
In-Reply-To
<robbat2-20191113T195158-869302266Z@orbis-terrarum.net> (view parent)
DKIM signature
missing
Download raw message
On Wed, Nov 13, 2019 at 08:19:07PM +0000, Robin H. Johnson wrote:
> I have two uses cases overall:
> - log so you know exactly when it's loaded successfully (great if
>   loading a firmware causes your system to lock up a few seconds later)

Then you can change the driver to confirm this, not impose every driver
to do the same.

> - at some point in the future, being able to query what firmware was
>   loaded in the past, and esp. exactly what version/data was in that
>   firmware file.

Firmware data is opaque to the firmware loader, as such details to
extract generic information about firmware details can only be done
by the driver, which could decode the firmware information. Many
drivers print these details themselves already, if they want it.

A generic interface to let us query *all* devices and currently loaded
firmware through the firmware loader would only be possible today for
firmware which requests (the default) caching of firmware upon
suspend/resume given that we keep the device / firmware name pair
around prior to suspend. For those devices it could be possible to
extend the firmware loader with a driver callback which can extract
firmware details in a generic codified way. To support *all* drivers
though, in a more clean way for this, a separate but similar list
could be kept which enables one to do this. Such items would be
torn down upon driver removal. But that would then be an opt-in
new mechanism.

  Luis