Re: [PATCH v4 3/6] pds_core: add PLDM firmware update support via devlink flash
From: Simon Horman <horms@kernel.org>
Date: 2026-06-15 15:36:30
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.
quoted hunk ↗ jump to hunk
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?