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

[Paolo Abeni <pabeni@redhat.com>]

On 6/9/26 12:32 AM, Nikhil P. Rao wrote:
> +static int pdsc_host_mem_add_one(struct pdsc *pdsc, int index)
> +{
> +	struct pdsc_host_mem *hm = &pdsc->host_mem_reqs[index];
> +	union pds_core_dev_comp comp = {};
> +	union pds_core_dev_cmd cmd = {};
> +	int err;
> +
> +	memset(hm, 0, sizeof(*hm));

Minor nit: the buffer is kzmalloced() no need to zero it again.

> +	cmd.host_mem.opcode = PDS_CORE_CMD_HOST_MEM;
> +	cmd.host_mem.oper = PDS_CORE_HOST_MEM_QUERY;
> +	cmd.host_mem.index = cpu_to_le16(index);
> +	dev_dbg(pdsc->dev, "Sending devcmd for mem query index %d\n", index);
> +	err = pdsc_devcmd(pdsc, &cmd, &comp, pdsc->devcmd_timeout);
> +	if (err || comp.status != PDS_RC_SUCCESS) {
> +		dev_err(pdsc->dev, "mem query failed err %d status %d\n",
> +			err, comp.status);
> +		return err ? err : -EIO;
> +	}
> +	hm->size = le32_to_cpu(comp.host_mem.size);
> +	hm->tag = le16_to_cpu(comp.host_mem.tag);
> +	dev_dbg(pdsc->dev, "mem query returned size %d tag %d\n",
> +		hm->size, hm->tag);
> +
> +	if (!hm->size || hm->size > PDSC_HOST_MEM_MAX_CONTIG) {
> +		dev_err(pdsc->dev, "invalid size %d for tag %d\n",
> +			hm->size, hm->tag);
> +		err = -EINVAL;
> +		goto err_del;
> +	}
> +
> +	hm->order = get_order(hm->size);
> +	hm->pg = alloc_pages(GFP_KERNEL | __GFP_ZERO | __GFP_NOWARN, hm->order);
> +	if (!hm->pg) {
> +		dev_err(pdsc->dev, "alloc order %d failed for tag %d\n",
> +			hm->order, hm->tag);

Minor nit: as this is a gracefully handled failure, possibly dev_warn()
is more suited and the message itself should be less alarming.

[...]
> +void pdsc_host_mem_add(struct pdsc *pdsc)
> +{
> +	union pds_core_dev_comp comp = {};
> +	union pds_core_dev_cmd cmd = {};
> +	u16 count;
> +	int err;
> +	int i;
> +
> +	if (!(pdsc->dev_ident.capabilities &
> +	     cpu_to_le64(PDS_CORE_DEV_CAP_HOST_MEM)))
> +		return;
> +
> +	cmd.host_mem.opcode = PDS_CORE_CMD_HOST_MEM;
> +	cmd.host_mem.oper = PDS_CORE_HOST_MEM_GET_COUNT;
> +	cmd.host_mem.index = cpu_to_le16(PDSC_HOST_MEM_MAX_COUNT);
> +	cmd.host_mem.max_contig = cpu_to_le32(PDSC_HOST_MEM_MAX_CONTIG);
> +	dev_dbg(pdsc->dev, "Sending devcmd for mem get count max_contig %u\n",
> +		PDSC_HOST_MEM_MAX_CONTIG);
> +	err = pdsc_devcmd(pdsc, &cmd, &comp, pdsc->devcmd_timeout);
> +	if (err || comp.status != PDS_RC_SUCCESS) {
> +		dev_err(pdsc->dev, "mem get count failed err %d status %d\n",
> +			err, comp.status);
> +		return;
> +	}
> +
> +	count = min(le16_to_cpu(comp.host_mem.count),
> +		    PDSC_HOST_MEM_MAX_COUNT);
> +	dev_dbg(pdsc->dev, "mem get count returned count %d\n", count);
> +	if (count == 0)
> +		return;
> +
> +	pdsc->host_mem_reqs = kzalloc_objs(*pdsc->host_mem_reqs, count,
> +					   GFP_KERNEL);
> +	if (!pdsc->host_mem_reqs) {
> +		dev_err(pdsc->dev, "failed to alloc host_mem_reqs array\n");
> +		return;
> +	}
> +
> +	for (i = 0; i < count; i++) {
> +		err = pdsc_host_mem_add_one(pdsc, i);
> +		if (err)
> +			break;

When pdsc_host_mem_add_one() returns an error, pdsc->host_mem_reqs[i].pg
will be zero. Errors are not propagate to the caller...

> +	}
> +}
> +
> +void pdsc_host_mem_free(struct pdsc *pdsc)
> +{
> +	int i;
> +
> +	if (!pdsc->host_mem_reqs)
> +		return;

... and this function will be unconditionally called on shutdown with
non zero host_mem_reqs.

> +
> +	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,

so  pdsc->host_mem_reqs[i].pg can be zero here. AFAICS at least
__free_pages() -> ___free_pages -> put_page_testzero ->
page_ref_count(0) will oops.

I find strange AI did not catch the above, so I'm possibly missing
something?!?

/P