Re: [PATCH 3/6] pds_core: add PLDM firmware update support via devlink flash
From: Simon Horman <horms@kernel.org>
Date: 2026-07-16 07:22:47
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 ---
--- Patch [3]: [PATCH 3/6] pds_core: add PLDM firmware update support via devlink flash ---commit 865af5a5208d8bbf12e2bc16faaacfc3c3239978 Author: Brett Creeley [off-list ref] pds_core: add PLDM firmware update support via devlink flash This commit implements PLDM firmware update support in the pds_core driver using the upstream pldmfw API. It 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/dev.c b/drivers/net/ethernet/amd/pds_core/dev.c index 84ea502ecb128..b149d29bd2560 100644 --- a/drivers/net/ethernet/amd/pds_core/dev.c +++ b/drivers/net/ethernet/amd/pds_core/dev.c@@ -206,15 +206,56 @@ static int __pdsc_devcmd_locked(struct pdsc *pdsc, union pds_core_dev_cmd *cmd, else memcpy_fromio(comp, &pdsc->cmd_regs->comp, sizeof(*comp)); + if (err != -ETIMEDOUT && err != -EAGAIN) + pdsc_deferred_dma_free(pdsc); +
[Severity: High] Can pdsc_deferred_dma_free() prematurely free DMA memory that the device firmware is still actively accessing? If a DMA-based devcmd like pdsc_devcmd_send_component() times out after polling and returns -EAGAIN, the DMA mapping is added to pdsc->deferred_dma_list in pdsc_flash_component_chunk(). The firmware update aborts on the host, but the device might still be processing the command in the background. If an unrelated devcmd (e.g., from a periodic health check) executes and succeeds shortly after, this logic will unconditionally unmap and free all deferred DMA memory. Does this lead to memory corruption or IOMMU faults if the firmware continues to access the freed memory?
return err;
}
+void pdsc_deferred_dma_add(struct pdsc *pdsc, struct pdsc_deferred_dma *entry,
+ dma_addr_t dma_addr, void *va, size_t size,
+ enum dma_data_direction dir)
+{[ ... ]
quoted hunk ↗ jump to hunk
@@ -227,6 +268,47 @@ int pdsc_devcmd(struct pdsc *pdsc, union pds_core_dev_cmd *cmd, return err; } +static int __pdsc_devcmd_with_data(struct pdsc *pdsc, + union pds_core_dev_cmd *cmd, + const void *data, size_t data_len, + union pds_core_dev_comp *comp, + int max_seconds, bool do_msg) +{ + int err; + + mutex_lock(&pdsc->devcmd_lock); + if (!pdsc->cmd_regs) { + err = -ENXIO; + goto unlock; + } + if (data_len > sizeof(pdsc->cmd_regs->data)) { + err = -ENOSPC; + goto unlock; + } + memcpy_toio(&pdsc->cmd_regs->data, data, data_len);
[Severity: High] This is a pre-existing issue, but can a concurrent PCI reset unmap the device BARs while the firmware update is actively copying data to MMIO here? When devlink flash initiates a firmware update, __pdsc_devcmd_with_data() acquires devcmd_lock and begins copying up to 2KB of data via memcpy_toio(). Concurrently, a PCI reset (e.g., via sysfs or AER) triggers pdsc_reset_prepare(), which calls pdsc_unmap_bars() without acquiring devcmd_lock. pdsc_unmap_bars() sets pdsc->cmd_regs = NULL and calls pci_iounmap(). Could this race cause a kernel panic if the firmware update thread dereferences the now-NULL pdsc->cmd_regs pointer or accesses unmapped MMIO memory? The long polling firmware update that repeatedly copies large data blocks might exacerbate this race window.
+ err = __pdsc_devcmd_locked(pdsc, cmd, comp, max_seconds, do_msg); +unlock: + mutex_unlock(&pdsc->devcmd_lock); + + return err; +}