Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Mon, 18 Nov 2019 18:53:12 +0100
From:   Greg KH <gregkh@linuxfoundation.org>
To:     "Robin H. Johnson" <robbat2@gentoo.org>
Cc:     mcgrof@kernel.org, linux-kernel@vger.kernel.org, sir@cmpwn.com,
        ~sircmpwn/public-inbox@lists.sr.ht, rafael@kernel.org
Subject: Re: [PATCH v3] firmware: log name & outcome of loaded firmware
Message-ID: <20191118175312.GA602012@kroah.com>
References: <robbat2-20191113T195158-869302266Z@orbis-terrarum.net>
 <20191117234734.27101-1-robbat2@gentoo.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20191117234734.27101-1-robbat2@gentoo.org>
User-Agent: Mutt/1.12.2 (2019-09-21)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Sun, Nov 17, 2019 at 03:47:34PM -0800, Robin H. Johnson wrote:
> It's non-trivial to figure out names of firmware that was actually
> loaded, add a debug statement at the end of _request_firmware that logs
> the name & result of each firmware.
> 
> This is esp. valuable early in boot, before logging of UEVENT is
> available.
> 
> v3:
> - Log at dev_dbg level per maintainer.
> - HOWTO: Enable at boot via kernel boot param
>   dyndbg="func _request_firmware +p"
> - Credit to Drew DeVault for parallel creation and help promoting the
>   idea.

These "v3.." lines need to go below the --- line.

> 
> Alternate-Creation: Drew DeVault <sir@cmpwn.com>

Is that a valid tag?  You can have co-developed-by (or something like
that, read the documentation for the real name), but I have never seen
this one before.

> Signed-off-by: Robin H. Johnson <robbat2@gentoo.org>
> ---
>  drivers/base/firmware_loader/main.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/base/firmware_loader/main.c b/drivers/base/firmware_loader/main.c
> index bf44c79beae9..84a879608ca4 100644
> --- a/drivers/base/firmware_loader/main.c
> +++ b/drivers/base/firmware_loader/main.c
> @@ -791,6 +791,13 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
>  		fw = NULL;
>  	}
>  
> +	/* Provide a consistent way to capture the result of trying to load any
> +	 * firmware. As a potential future improvement, this might include
> +	 * persistent state that firmware is loaded (or failed to load for some
> +	 * reason). See Message-ID: <20191113205010.GY11244@42.do-not-panic.com>

Just provide a lore link with the message id if you really want this.

But really, this type of thing belongs in the changelog text, not in a
comment, right?

> +	 * for background */
> +	dev_dbg(device, "%s %s ret=%d\n", __func__, name, ret);

That does not provide any real information as to what is going on, why
doesn't ftrace suffice for this?

thanks,

greg k-h