Re: [PATCH v4 3/6] pds_core: add PLDM firmware update support via devlink flash

[Simon Horman <horms@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev
---
pds_core: add PLDM firmware update support via devlink flash

Implement PLDM FW Update in the pds_core driver using the upstream
pldmfw API. This allows updating an entire PLDM FW package at once
or updating specific firmware components by name.

> diff --git a/drivers/net/ethernet/amd/pds_core/fw.c b/drivers/net/ethernet/amd/pds_core/fw.c
> --- a/drivers/net/ethernet/amd/pds_core/fw.c
> +++ b/drivers/net/ethernet/amd/pds_core/fw.c
> [ ... ]
> @@ -195,3 +239,720 @@ int pdsc_firmware_update(struct pdsc *pdsc, const struct firmware *fw,
> [ ... ]
> +static int pdsc_flash_component_chunk(struct pdsc *pdsc, struct device *dev,
> [ ... ]
> +	err = pdsc_devcmd_send_component(pdsc, info, info_sz, dma_addr,
> +					 copy_sz, offset, slot_id, comp);
> +	if (err == -ETIMEDOUT || err == -EAGAIN) {
> +		pdsc_deferred_dma_add(pdsc, deferred, dma_addr,
> +				      component_data, copy_sz, DMA_TO_DEVICE);
> +		return err;
> +	}

[Severity: High]
Can this lead to a race condition that leaks the DMA mapping and memory?

If pdsc_devcmd_send_component() encounters a timeout in the lower level
__pdsc_devcmd_locked() function, it will queue pdsc->health_work. The wrapper
function __pdsc_devcmd_with_data_nomsg() will then release the devcmd_lock
before returning the timeout error back here.

If the scheduled health_work runs and acquires devcmd_lock before this code
reaches pdsc_deferred_dma_add(), it could execute a recovery devcmd and
call pdsc_deferred_dma_free() on an empty list. When this thread resumes,
it adds the timed-out buffer to the deferred DMA list, but its cleanup
trigger has already fired, leaving the buffer leaked.

> [ ... ]
> +int pdsc_firmware_update(struct pdsc *pdsc,
> +			 struct devlink_flash_update_params *params,
> +			 struct netlink_ext_ack *extack)
> +{
> +	int err;
> +
> +	if (pdsc->dev_ident.version >= PDS_CORE_IDENTITY_VERSION_2 &&
> +	    pdsc->dev_ident.capabilities &
> +		cpu_to_le64(PDS_CORE_DEV_CAP_PLDM_FW_UPDATE))
> +		err = pdsc_pldm_firmware_update(pdsc, params, extack,
> +						params->fw);
> +	else
> +		err = pdsc_legacy_firmware_update(pdsc, params->fw, extack);

[Severity: High]
Does this fallback path ignore the requested component name?

If the device does not support PLDM, the driver falls back to
pdsc_legacy_firmware_update() which only takes the params->fw pointer.
If a user requested a targeted component update using devlink, this would
ignore the component filter and flash the entire monolithic image onto
the device.

Should this explicitly check if params->component is set before falling
back, and return -EOPNOTSUPP if a component is specified on legacy
firmware?