Re: [PATCH 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
---
--- Patch [3]: [PATCH 3/6] pds_core: add PLDM firmware update support via devlink flash ---
commit 865af5a5208d8bbf12e2bc16faaacfc3c3239978
Author: Brett Creeley <brett.creeley@amd.com>

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.

> 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)
> +{

[ ... ]

> @@ -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;
> +}