Message-ID: <e6cd77a7-bc18-4e0c-9536-5fb107ec4db4@redhat.com>
Date: Mon, 1 Sep 2025 18:34:14 +0200
From: Ivan Vecera <ivecera@...hat.com>
To: Jakub Kicinski <kuba@...nel.org>, Jiri Pirko <jiri@...nulli.us>
Cc: netdev@...r.kernel.org, "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 Kuba and Jiri,

On 30. 08. 25 1:56 dop., Jakub Kicinski wrote:
> On Fri, 29 Aug 2025 16:49:22 +0200 Ivan Vecera wrote:
>>>> +		/* 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.
> 
> Whatever makes sense for your driver, for now. I'm assuming both ops
> are the same, otherwise you wouldn't be asking? It should be trivial
> for someone add the extra ops later, and just hook them both up to the
> same functions in existing drivers.

Things are a little bit complicated after further investigation...

Some internal flashing backround first:
The zl3073x HW needs an external program called "flash utility"
to access an internal flash inside the chip. This utility provides
flash API over I2C/SPI bus that is different from the FW API provided
by the normal firmware. So to access the flash memory the driver has
to stop the device's CPU, to load the utility into chip RAM and resume
the CPU to execute the utility. At this point normal FW API is not
accessible so the driver has to stop the normal operation (unregister
DPLL devices, pins etc.). Then it updates flash using flash API and
after flash operations it has to reset device's CPU to restart newly
flashed firmware. Finally when normal FW is available it resumes
the normal operation (re-register DPLL devices etc.).

Current steps in this patch:
1. Load given FW file and verify that utility is present
2. Stop normal operations
3. Stop CPU, download utility to device, resume CPU
4. Flash components from the FW file
5. Unconditionally reset device's CPU to load normal FW
6. Resume normal operations

I found 4 possible options how to handle:

1. Introduce DEVLINK_RELOAD_ACTION_FW_UPDATE devlink action and reuse
    .reload_down/up() callbacks and call them prior and after
    .flash_update callback.

At first glance, it looks the most elegant... The zl3073x driver stops
during .reload_down() normal operation, then in .flash_update will
switch the device to flash mode, performs flash update and finally
during .reload_up() will resume normal operation.

Issues:
- a problematic case, when the given firmware file does not contain
   utility... During .reload_down() this cannot be checked as the
   firmware is not available during .reload_down() callback.
- DEVLINK_RELOAD_ACTION_FW_UPDATE has to be placed in devlink UAPI
   and but this reload action should be handled as internal one as
   there should not be possible to initiate it from the userspace

   e.g. devlink dev reload DEV action fw_update

2. Add new .flash_down/up() or .flash_prepare/done() optional callbacks
    that are called prior and after .flash_update if they are provided by
    a driver.

This looks also good and very similar to previous option. Could resolve
the 1st issue as we can pass 'devlink_flash_update_params' to both
new callbacks, so the driver can parse firmware file during
.flash_down and check for utility presence.

Issues:
- the firmware has to be parsed twice - during .flash_down() and
   .flash_update()
   This could be resolved by extending devlink_flash_update_params
   structure by 'void *priv' field that could be used by a driver
   during flash operation.
   It could be also useful to add flash_failed flag similar to
   reload_failed that would record a status reported by .flash_up()
   callback.

3. Keep my original approach but without restarting normal operation
    (re-register DPLL devices and pins). User has to use explicitly
    devlink reload fw_activate to restart normal operation.

This could be reasonable but introduces some kind of asymmetry because
the driver stops normal operation during .flash_update() and left
the device in intermediate state (devlink interface is working but
DPLL devices and pins are not registered). Only upon explicit user
request (fw_activate) would it restore normal mode (re-registration).

4. Keep my original approach, fix the ignored error code reported by
    Jakub and pass "re-start normal operation failure" via devlink
    notification.

 From my POV better than previous one as the driver will do its best to
resume device state prior the flashing. Only corner case where the
firmware is unresponsive after reset will cause that normal operation
won't be resumed -> could be handled by health reporting?

Thanks for opinions and advises.

Ivan

Powered by blists - more mailing lists