On Wed, Nov 20, 2024 at 06:30:37PM +0100, Marco Felsch wrote:

Hi,

On 24-11-20, Russ Weight wrote:

quoted

On Tue, Nov 19, 2024 at 11:33:51PM +0100, Marco Felsch wrote:

quoted

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 <redacted>
---
 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.

quoted

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.

quoted

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.

quoted

-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

quoted

Thanks,
- Russ

quoted

 		fw_upload_set_error(fwlp, ret);
 		goto putdev_exit;
 	}

-- 
2.39.5