Re: [PATCH net-next v5 03/10] ethtool: Add an interface for flashing transceiver modules' firmware

[Jakub Kicinski]

On Wed, 24 Apr 2024 16:30:16 +0300 Danielle Ratson wrote:
> +MODULE_FW_FLASH_ACT
> +===================
> +
> +Flashes transceiver module firmware.
> +
> +Request contents:
> +
> +  =======================================  ======  ===========================
> +  ``ETHTOOL_A_MODULE_FW_FLASH_HEADER``     nested  request header
> +  ``ETHTOOL_A_MODULE_FW_FLASH_FILE_NAME``  string  firmware image file name
> +  ``ETHTOOL_A_MODULE_FW_FLASH_PASSWORD``   u32     transceiver module password
> +  =======================================  ======  ===========================
> +
> +The firmware update process consists of three logical steps:
> +
> +1. Downloading a firmware image to the transceiver module and validating it.
> +2. Running the firmware image.
> +3. Committing the firmware image so that it is run upon reset.
> +
> +When flash command is given, those three steps are taken in that order.
> +
> +The ``ETHTOOL_A_MODULE_FW_FLASH_FILE_NAME`` attribute encodes the firmware
> +image file name. The firmware image is downloaded to the transceiver module,
> +validated, run and committed.
> +
> +The optional ``ETHTOOL_A_MODULE_FW_FLASH_PASSWORD`` attribute encodes a password
> +that might be required as part of the transceiver module firmware update
> +process.
> +
> +The firmware update process can take several minutes to complete. Therefore,
> +during the update process notifications are emitted from the kernel to user
> +space updating it about the status and progress.

We should spell out that the request only kicks off the process.
devlink flashing blocks the netlink socket send() until it's completed,
and the notifications have to be received on another socket.
So someone who has devlink experience may expect the same behavior here.

> +Notification contents:
> +
> + +---------------------------------------------------+--------+----------------+
> + | ``ETHTOOL_A_MODULE_FW_FLASH_HEADER``              | nested | reply header   |
> + +---------------------------------------------------+--------+----------------+
> + | ``ETHTOOL_A_MODULE_FW_FLASH_STATUS``              | u32    | status         |
> + +---------------------------------------------------+--------+----------------+
> + | ``ETHTOOL_A_MODULE_FW_FLASH_STATUS_MSG``          | string | status message |
> + +---------------------------------------------------+--------+----------------+
> + | ``ETHTOOL_A_MODULE_FW_FLASH_DONE``                | uint   | progress       |
> + +---------------------------------------------------+--------+----------------+
> + | ``ETHTOOL_A_MODULE_FW_FLASH_TOTAL``               | uint   | total          |
> + +---------------------------------------------------+--------+----------------+

> +enum {
> +	ETHTOOL_A_MODULE_FW_FLASH_UNSPEC,
> +	ETHTOOL_A_MODULE_FW_FLASH_HEADER,		/* nest - _A_HEADER_* */
> +	ETHTOOL_A_MODULE_FW_FLASH_FILE_NAME,		/* string */
> +	ETHTOOL_A_MODULE_FW_FLASH_PASSWORD,		/* u32 */
> +	ETHTOOL_A_MODULE_FW_FLASH_PAD,

uint doesn't need pad, and pad is not specified in YAML, so 
the following attribute IDs will be all off by one in YNL.

> +	ETHTOOL_A_MODULE_FW_FLASH_STATUS,		/* u32 */
> +	ETHTOOL_A_MODULE_FW_FLASH_STATUS_MSG,		/* string */
> +	ETHTOOL_A_MODULE_FW_FLASH_DONE,			/* uint */
> +	ETHTOOL_A_MODULE_FW_FLASH_TOTAL,		/* uint */
> +
> +	/* add new constants above here */
> +	__ETHTOOL_A_MODULE_FW_FLASH_CNT,
> +	ETHTOOL_A_MODULE_FW_FLASH_MAX = (__ETHTOOL_A_MODULE_FW_FLASH_CNT - 1)