Re: [PATCH v9 net-next 07/14] pds_core: add FW update feature to devlink

[Simon Horman <simon.horman@corigine.com>]

On Thu, Apr 06, 2023 at 04:41:36PM -0700, Shannon Nelson wrote:
> Add in the support for doing firmware updates.  Of the two
> main banks available, a and b, this updates the one not in
> use and then selects it for the next boot.
> 
> Example:
>     devlink dev flash pci/0000:b2:00.0 \
> 	    file pensando/dsc_fw_1.63.0-22.tar
> 
> Signed-off-by: Shannon Nelson <shannon.nelson@amd.com>

Hi Shannon,

some minor feedback from my side.

...

> diff --git a/Documentation/networking/device_drivers/ethernet/amd/pds_core.rst b/Documentation/networking/device_drivers/ethernet/amd/pds_core.rst
> index 265d948a8c02..6faf46390f5f 100644
> --- a/Documentation/networking/device_drivers/ethernet/amd/pds_core.rst
> +++ b/Documentation/networking/device_drivers/ethernet/amd/pds_core.rst
> @@ -73,6 +73,16 @@ The ``pds_core`` driver reports the following versions
>       - fixed
>       - The revision of the ASIC for this device
>  
> +Firmware Management
> +===================
> +
> +The ``flash`` command can update a the DSC firmware.  The downloaded firmware
> +will be saved into either of firmware bank 1 or bank 2, whichever is not
> +currrently in use, and that bank will used for the next boot::

nit: s/currrently/currently/

...

> diff --git a/drivers/net/ethernet/amd/pds_core/fw.c b/drivers/net/ethernet/amd/pds_core/fw.c

...

> +int pdsc_firmware_update(struct pdsc *pdsc, const struct firmware *fw,
> +			 struct netlink_ext_ack *extack)
> +{
> +	u32 buf_sz, copy_sz, offset;
> +	struct devlink *dl;
> +	int next_interval;
> +	u64 data_addr;
> +	int err = 0;
> +	u8 fw_slot;
> +
> +	dev_info(pdsc->dev, "Installing firmware\n");
> +
> +	dl = priv_to_devlink(pdsc);
> +	devlink_flash_update_status_notify(dl, "Preparing to flash",
> +					   NULL, 0, 0);
> +
> +	buf_sz = sizeof(pdsc->cmd_regs->data);
> +
> +	dev_dbg(pdsc->dev,
> +		"downloading firmware - size %d part_sz %d nparts %lu\n",
> +		(int)fw->size, buf_sz, DIV_ROUND_UP(fw->size, buf_sz));
> +
> +	offset = 0;
> +	next_interval = 0;
> +	data_addr = offsetof(struct pds_core_dev_cmd_regs, data);
> +	while (offset < fw->size) {
> +		if (offset >= next_interval) {
> +			devlink_flash_update_status_notify(dl, "Downloading",
> +							   NULL, offset,
> +							   fw->size);
> +			next_interval = offset +
> +					(fw->size / PDSC_FW_INTERVAL_FRACTION);
> +		}
> +
> +		copy_sz = min_t(unsigned int, buf_sz, fw->size - offset);
> +		mutex_lock(&pdsc->devcmd_lock);
> +		memcpy_toio(&pdsc->cmd_regs->data, fw->data + offset, copy_sz);
> +		err = pdsc_devcmd_fw_download_locked(pdsc, data_addr,
> +						     offset, copy_sz);
> +		mutex_unlock(&pdsc->devcmd_lock);
> +		if (err) {
> +			dev_err(pdsc->dev,
> +				"download failed offset 0x%x addr 0x%llx len 0x%x: %pe\n",
> +				offset, data_addr, copy_sz, ERR_PTR(err));
> +			NL_SET_ERR_MSG_MOD(extack, "Segment download failed");
> +			goto err_out;
> +		}
> +		offset += copy_sz;
> +	}
> +	devlink_flash_update_status_notify(dl, "Downloading", NULL,
> +					   fw->size, fw->size);
> +
> +	devlink_flash_update_timeout_notify(dl, "Installing", NULL,
> +					    PDSC_FW_INSTALL_TIMEOUT);
> +
> +	fw_slot = pdsc_devcmd_fw_install(pdsc);
> +	if (fw_slot < 0) {

The type of fs_slot is u8.
But the return type of pdsc_devcmd_fw_install is int,
(I think) it can return negative error values,
and that case is checked on the line above.

Perhaps the type of fw_slot should be int?

Flagged by Coccinelle as:

drivers/net/ethernet/amd/pds_core/fw.c:154:5-12: WARNING: Unsigned expression compared with zero: fw_slot < 0

And Smatch as:

drivers/net/ethernet/amd/pds_core/fw.c:154 pdsc_firmware_update() warn: impossible condition '(fw_slot < 0) => (0-255 < 0)'

> +		err = fw_slot;
> +		dev_err(pdsc->dev, "install failed: %pe\n", ERR_PTR(err));
> +		NL_SET_ERR_MSG_MOD(extack, "Failed to start firmware install");
> +		goto err_out;
> +	}

...