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

[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
---
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.

> +}

[ ... ]

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