Re: [PATCH 2/5] firmware_loader: add support to handle FW_UPLOAD_ERR_SKIP

[Marco Felsch <m.felsch@pengutronix.de>]

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@pengutronix.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.

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.

> 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?

Regards,
  Marco

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