Hi Jakub,
On 19. 08. 25 4:29 dop., Jakub Kicinski wrote:
On Wed, 13 Aug 2025 19:44:08 +0200 Ivan Vecera wrote:
+ struct zl3073x_dev *zldev = devlink_priv(devlink);
+ struct zl3073x_fw_component *util;
+ struct zl3073x_fw *zlfw;
+ int rc = 0;
+
+ /* Load firmware */
Please drop the comments which more or less repeat the name
of the function called.
Will do.
+ zlfw = zl3073x_fw_load(zldev, params->fw->data, params->fw->size,
+ extack);
+ if (IS_ERR(zlfw))
+ return PTR_ERR(zlfw);
+
+ util = zlfw->component[ZL_FW_COMPONENT_UTIL];
+ if (!util) {
+ zl3073x_devlink_flash_notify(zldev,
+ "Utility is missing in firmware",
+ NULL, 0, 0);
+ rc = -EOPNOTSUPP;
I'd think -EINVAL would be more appropriate.
If you want to be fancy maybe ENOEXEC ?
OK, will use -ENOEXEC.
+ goto error;
+ }
+
+ /* Stop normal operation during flash */
+ zl3073x_dev_stop(zldev);
+
+ /* Enter flashing mode */
+ rc = zl3073x_flash_mode_enter(zldev, util->data, util->size, extack);
+ if (!rc) {
+ /* Flash the firmware */
+ rc = zl3073x_fw_flash(zldev, zlfw, extack);
this error code seems to be completely ignored, no?
Yep, you are right, this should be propagated to the caller.
+ /* Leave flashing mode */
+ zl3073x_flash_mode_leave(zldev, extack);
+ }
+
+ /* Restart normal operation */
+ rc = zl3073x_dev_start(zldev, true);
+ if (rc)
+ dev_warn(zldev->dev, "Failed to re-start normal operation\n");
And also we can't really cleanly handle the failure case.
This is why I was speculating about implementing the down/up portion
in the devlink core. Add a flag that the driver requires reload_down
to be called before the flashing operation, and reload_up after.
This way not only core handles some of the error handling, but also
it can mark the device as reload_failed if things go sideways, which
is a nicer way to surface this sort of permanent error state.
This makes sense... The question is if this should reuse existing
.reload_down and .reload_up callbacks let's say with new devlink action
DEVLINK_RELOAD_ACTION_FW_UPDATE or rather introduce new callbacks
.flash_update_down/_up() to avoid confusions.
Thanks,
Ivan
