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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20241120185611.43soqjcyruztby4f@4VRSMR2-DT.corp.robot.car>
Date: Wed, 20 Nov 2024 10:56:11 -0800
From: Russ Weight <russ.weight@...ux.dev>
To: Marco Felsch <m.felsch@...gutronix.de>
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 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 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.

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

Thanks,
- Russ

> 
> Regards,
>   Marco
> 
> > 
> > Thanks,
> > - Russ
> > 
> > >  		fw_upload_set_error(fwlp, ret);
> > >  		goto putdev_exit;
> > >  	}
> > > 
> > > -- 
> > > 2.39.5
> > > 
> >