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