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.
> + 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 ?
> + 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?
> + /* 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.
Not feeling strongly about it, but I think it'd be cleaner, so bringing
it up in case my previous comment from a while back wasn't clear.
> +error:
> + /* Free flash context */
> + zl3073x_fw_free(zlfw);
> +
> + zl3073x_devlink_flash_notify(zldev,
> + rc ? "Flashing failed" : "Flashing done",
> + NULL, 0, 0);
