linux-kernel - Re: [PATCH 2/5] firmware_loader: add support to handle FW_UPLOAD_ERR

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20241121084125.3ldkyd7xotjvuq2r@pengutronix.de>
Date: Thu, 21 Nov 2024 09:41:25 +0100
From: Marco Felsch <m.felsch@...gutronix.de>
To: Russ Weight <russ.weight@...ux.dev>
Cc: Luis Chamberlain <mcgrof@...nel.org>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	"Rafael J. Wysocki" <rafael@...nel.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Dmitry Torokhov <dmitry.torokhov@...il.com>,
	Kamel Bouhara <kamel.bouhara@...tlin.com>,
	Marco Felsch <kernel@...gutronix.de>,
	Henrik Rydberg <rydberg@...math.org>,
	Danilo Krummrich <dakr@...hat.com>, linux-kernel@...r.kernel.org,
	devicetree@...r.kernel.org, linux-input@...r.kernel.org
Subject: Re: [PATCH 2/5] firmware_loader: add support to handle
 FW_UPLOAD_ERR_SKIP

On 24-11-20, Russ Weight wrote:
> 
> On Wed, Nov 20, 2024 at 06:30:37PM +0100, Marco Felsch wrote:
> > Hi,
> > 
> > On 24-11-20, Russ Weight wrote:
> > > On Tue, Nov 19, 2024 at 11:33:51PM +0100, Marco Felsch wrote:
> > > > It's no error if a driver indicates that the firmware is already
> > > > up-to-date and the update can be skipped.
> > > > 
> > > > Signed-off-by: Marco Felsch <m.felsch@...gutronix.de>
> > > > ---
> > > >  drivers/base/firmware_loader/sysfs_upload.c | 4 ++++
> > > >  1 file changed, 4 insertions(+)
> > > > 
> > > > diff --git a/drivers/base/firmware_loader/sysfs_upload.c b/drivers/base/firmware_loader/sysfs_upload.c
> > > > index b3cbe5b156e3..44f3d8fa5e64 100644
> > > > --- a/drivers/base/firmware_loader/sysfs_upload.c
> > > > +++ b/drivers/base/firmware_loader/sysfs_upload.c
> > > > @@ -174,6 +174,10 @@ static void fw_upload_main(struct work_struct *work)
> > > >  	fw_upload_update_progress(fwlp, FW_UPLOAD_PROG_PREPARING);
> > > >  	ret = fwlp->ops->prepare(fwl, fwlp->data, fwlp->remaining_size);
> > > >  	if (ret != FW_UPLOAD_ERR_NONE) {
> > > > +		if (ret == FW_UPLOAD_ERR_SKIP) {
> > > > +			dev_info(fw_dev, "firmware already up-to-date, skip update\n");
> > > > +			ret = FW_UPLOAD_ERR_NONE;
> > > > +		}
> > > 
> > > If you change the error-code from FW_UPLOAD_ERR_SKIP to
> > > FW_UPLOAD_ERR_NONE, then the "skip" string provided in the previous
> > > patch will never be seen. There are currently no other instances where
> > 
> > Do we really need to set it? As explained within the commit message,
> > it's no error if FW_UPLOAD_ERR_SKIP is returned. The previous patch just
> > added all pieces which may be required later on.
> > 
> > > an error code requires special-case modifications to the fw_upload
> > > code and I don't think it is necessary to add it here.
> > 
> > Because at the moment no one is checking it except for the gb-beagleplay
> > driver. This driver prints a dev_warn() string and returns a failure.
> > Now the userspace needs some heuristic by parsing dmesg to check the
> > reason. This is rather complex and very error prone as the sting can be
> > changed in the future.
> > 
> > Therefore I added the support to have a simple error code which can be
> > returned by a driver. I'm open to return "skip" as error instead of
> > casting it to none. Both is fine for me since both allow the userspace
> > to easily check if the error is a 'real' error or if the fw-update was
> > just skipped due to already-up-to-date.
> 
> Are you saying that you intend for the user-space code to see "skip"?
> Because in the current implementation, I don't think the user-space
> code would see "skip". If you ultimately return FW_UPLOAD_ERR_NONE,
> then cat'ing the error file should result in an empty file.

I know.

> > I wouldn't say that this is a special case, it is very common but no one
> > is performing a fw-version check. Therefore I added this to the common
> > code, to make it easier for driver devs.
> 
> By "special case" I meant to say that this is the first time this
> core code has had to know about any error codes other than
> FW_UPLOAD_ERR_NONE - and the first time that an error type alters
> the code flow.
> 
> I understand that other drivers may also want to abort if the
> firmware being loaded is a duplicate.

:)

> > > The dev_info() message above can be provided by the device driver
> > > that is using this API.
> > > 
> > > I think you can either:
> > > 
> > > (1) allow "skip" to be treated as an error. The update didn't happen...
> > 
> > Please see above.
> > 
> > > -or-
> > > 
> > > (2) The prepare function could detect the situation and set
> > >     a flag in the same device driver. Your write function could
> > >     set *written to the full data size and return without writing
> > >     anything. Your poll_complete handler could also return
> > >     FW_UPLOAD_ERR_NONE. Then you don't need to add FW_UPLOAD_ERR_SKIP
> > >     at all. You would get the info message from the device driver
> > >     and fw_upload would exit without an error.
> > 
> > Please see above. I don't think that this is special case and why making
> > the life hard for driver devs instead of having a well known fw
> > behaviour?
> 
> If you are not opposed to treating it as an error, then all you need
> to add are the error code and the string to go with it.

Yep this works for me.

> Instead of FW_UPLOAD_ERR_SKIP -> "skip", how about
> FW_UPLOAD_ERR_DUPLICATE -> "duplicate_firmware"?

Fine by me :) I can use this if no one else comes up with a better idea
for the string.

Regards,
  Marco