Re: [PATCH v4 5/6] pds_core: add host backed memory support for firmware

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

On 6/15/2026 9:19 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
> ---
> pds_core: add host backed memory support for firmware
> 
> This commit adds support for host-backed DMA memory to provide additional
> memory for firmware control plane components on newer AMD/Pensando cards.
> 
>> diff --git a/drivers/net/ethernet/amd/pds_core/core.c b/drivers/net/ethernet/amd/pds_core/core.c
>> index 705cab7b07273..d1695ca95440c 100644
>> --- a/drivers/net/ethernet/amd/pds_core/core.c
>> +++ b/drivers/net/ethernet/amd/pds_core/core.c
> [ ... ]
> 
>> @@ -658,3 +660,161 @@ void pdsc_health_thread(struct work_struct *work)
>>   out_unlock:
>>        mutex_unlock(&pdsc->config_lock);
>>   }
>> +
>> +static void pdsc_host_mem_del_one(struct pdsc *pdsc, u16 tag, u8 reason)
>> +{
>> +     union pds_core_dev_comp comp = {};
>> +     union pds_core_dev_cmd cmd = {
>> +             .host_mem.opcode = PDS_CORE_CMD_HOST_MEM,
>> +             .host_mem.oper = PDS_CORE_HOST_MEM_DEL,
>> +             .host_mem.tag = cpu_to_le16(tag),
>> +             .host_mem.reason = reason,
>> +     };
> [ ... ]
> 
>> +     hm->pa = dma_map_page(pdsc->dev, hm->pg, 0, hm->size,
>> +                           DMA_BIDIRECTIONAL);
>> +     if (dma_mapping_error(pdsc->dev, hm->pa)) {
>> +             dev_err(pdsc->dev, "dma map failed for tag %d size %d\n",
>> +                     hm->tag, hm->size);
>> +             __free_pages(hm->pg, hm->order);
>> +             hm->pg = NULL;
>> +             err = -EIO;
>> +             goto err_del;
>> +     }
>> +
>> +     /* Track this allocation so pdsc_host_mem_free() can clean it up */
>> +     pdsc->num_host_mem_reqs++;
>> +
>> +     memset(&cmd, 0, sizeof(cmd));
>> +     memset(&comp, 0, sizeof(comp));
>> +     cmd.host_mem.opcode = PDS_CORE_CMD_HOST_MEM;
>> +     cmd.host_mem.oper = PDS_CORE_HOST_MEM_ADD;
>> +     cmd.host_mem.tag = cpu_to_le16(hm->tag);
>> +     cmd.host_mem.size = cpu_to_le32(hm->size);
>> +     cmd.host_mem.buf_pa = cpu_to_le64(hm->pa);
>> +
>> +     dev_dbg(pdsc->dev, "Sending devcmd for mem add tag %d size %d pa %pad\n",
>> +             hm->tag, hm->size, &hm->pa);
>> +     err = pdsc_devcmd(pdsc, &cmd, &comp, pdsc->devcmd_timeout);
>> +     if (err || comp.status != PDS_RC_SUCCESS) {
>> +             dev_err(pdsc->dev, "mem add failed err %d status %d for tag %d\n",
>> +                     err, comp.status, hm->tag);
>> +             err = err ? err : -EIO;
>> +             goto err_del;
>> +     }
>> +     dev_dbg(pdsc->dev, "mem add completed for tag %d\n", hm->tag);
>> +
>> +     return 0;
>> +
>> +err_del:
>> +     /* After MEM_QUERY succeeds, firmware expects MEM_ADD or MEM_DEL */
>> +     pdsc_host_mem_del_one(pdsc, hm->tag, PDS_RC_ENOMEM);
>> +     return err;
> [Severity: Medium]
> Does this leave the failed memory allocation mapped and wasted for the
> lifetime of the driver?
> 
> Because pdsc->num_host_mem_reqs is incremented before sending the MEM_ADD
> command, jumping to the err_del label here skips the DMA unmap and page free
> steps. The failed request is tracked and will eventually be cleaned up by
> pdsc_host_mem_free() during device teardown.
> 
> However, the caller pdsc_start() ignores the failure, so the driver operates
> normally while potentially tying up to 4MB of DMA-mapped memory until unload
> or reset.

This is intentional. In the timeout case, firmware could still be
accessing the memory (slow response, not actual failure). Freeing
immediately could cause firmware to DMA into freed memory. The memory
is freed at teardown regardless of MEM_ADD result.

> 
>> +}
> [ ... ]
> 
>> +void pdsc_host_mem_free(struct pdsc *pdsc)
>> +{
>> +     int i;
>> +
>> +     if (!pdsc->host_mem_reqs)
>> +             return;
>> +
>> +     for (i = 0; i < pdsc->num_host_mem_reqs; i++) {
>> +             dma_unmap_page(pdsc->dev, pdsc->host_mem_reqs[i].pa,
>> +                            pdsc->host_mem_reqs[i].size,
>> +                            DMA_BIDIRECTIONAL);
>> +             __free_pages(pdsc->host_mem_reqs[i].pg,
>> +                          pdsc->host_mem_reqs[i].order);
>> +     }
> [Severity: Critical]
> Is there a missing notification to the firmware that this memory is being
> unmapped?
> 
> This loop unmaps the DMA regions and frees the pages, but it doesn't send a
> PDS_CORE_HOST_MEM_DEL command. >
> For physical functions, pdsc_devcmd_reset() runs before this during
> teardown, which might clear hardware state. However, virtual functions
> explicitly bypass the reset in pdsc_teardown():
> 
> pdsc_teardown() {
>      if (!pdsc->pdev->is_virtfn)
>          pdsc_devcmd_reset(pdsc);
>      ...
> 
> If a virtual function uses host memory, skipping the MEM_DEL command means
> the firmware could be completely unaware the buffers were returned to the
> OS. Could this lead to the device performing DMA into freed memory and
> causing memory corruption or IOMMU faults?
> 
>> +
>> +     kfree(pdsc->host_mem_reqs);
>> +     pdsc->host_mem_reqs = NULL;
>> +     pdsc->num_host_mem_reqs = 0;
>> +}

For PFs: pdsc_devcmd_reset() runs before pdsc_host_mem_free() in 
teardown. The reset clears all firmware state including host memory 
state, so individual MEM_DEL commands are not needed.

For VFs: They never allocate host memory. VF init path (pdsc_init_vf())
doesn't call pdsc_setup() or pdsc_start(). pdsc_host_mem_add() is only
called from pdsc_start() in pdsc_init_pf().

Thanks,
Nikhil