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

["Rao, Nikhil" <nikhirao@amd.com>]

On 7/16/2026 12:20 AM, Simon Horman wrote:
> 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)
>> +{
> 
The free is not premature. The devcmd interface processes one command at 
a time in firmware. A (non-EAGAIN/non-ETIMEDOUT) completion of any 
devcmd proves the prior DMA is quiesced.

> 
>> @@ -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.
This is a valid race. Since this is a pre-existing issue, I will submit 
a fix to -net.

Thanks,
Nikhil