[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <e7a5ee37-993a-4bba-b69e-6c8a7c942af8@redhat.com>
Date: Fri, 29 Aug 2025 16:49:22 +0200
From: Ivan Vecera <ivecera@...hat.com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: netdev@...r.kernel.org, Jiri Pirko <jiri@...nulli.us>,
"David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
Paolo Abeni <pabeni@...hat.com>, Simon Horman <horms@...nel.org>,
Jonathan Corbet <corbet@....net>,
Prathosh Satish <Prathosh.Satish@...rochip.com>, linux-doc@...r.kernel.org,
linux-kernel@...r.kernel.org, Michal Schmidt <mschmidt@...hat.com>,
Petr Oros <poros@...hat.com>, Przemek Kitszel <przemyslaw.kitszel@...el.com>
Subject: Re: [PATCH net-next v3 5/5] dpll: zl3073x: Implement devlink flash
callback
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
Powered by blists - more mailing lists