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@xxxxxxxxxxxxxx>
> > > ---
> > > 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
> > >
> >
