Re: [PATCH net-next v2 6/6] pds_core: add debugfs support for host backed memory

[Jakub Kicinski <kuba@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.
---
pds_core: add debugfs support for host backed memory

This adds a debugfs file (host_mem) under the pds_core device directory
that displays the tag, size, order, and physical address of each host
memory request tracked in pdsc->host_mem_reqs.

> diff --git a/drivers/net/ethernet/amd/pds_core/debugfs.c b/drivers/net/ethernet/amd/pds_core/debugfs.c
> index 04c5e3abd8d7..058071f6f17e 100644
> --- a/drivers/net/ethernet/amd/pds_core/debugfs.c
> +++ b/drivers/net/ethernet/amd/pds_core/debugfs.c
> @@ -173,3 +173,53 @@ void pdsc_debugfs_del_qcq(struct pdsc_qcq *qcq)
>  	debugfs_remove_recursive(qcq->dentry);
>  	qcq->dentry = NULL;
>  }
> +
> +static int host_mem_show(struct seq_file *seq, void *v)
> +{
> +	struct pdsc *pdsc = seq->private;
> +	struct pdsc_host_mem *hm;
> +	int i;
> +
> +	if (!pdsc->host_mem_reqs || pdsc->num_host_mem_reqs == 0) {
> +		seq_puts(seq, "No host memory allocated\n");
> +		return 0;
> +	}

[High]
Can host_mem_show() race with pdsc_host_mem_free() and end up
dereferencing freed memory?

The reader takes no lock, but the writers in pdsc_host_mem_add() and
pdsc_host_mem_free() are serialized by pdsc->config_lock and run from
pdsc_fw_down()/pdsc_fw_up() during PCI reset_prepare/reset_done, AER
recovery, and the watchdog-triggered pdsc_health_thread workqueue.
pdsc_teardown() ultimately calls pdsc_host_mem_free(), which kfree()s
host_mem_reqs and sets the pointer to NULL.

The initial check is also TOCTOU:

    if (!pdsc->host_mem_reqs || pdsc->num_host_mem_reqs == 0) {

After this passes, host_mem_reqs can be freed and nulled before the
loop body dereferences &pdsc->host_mem_reqs[i].

Could this be addressed by holding pdsc->config_lock around the
iteration, by removing the host_mem dentry from pdsc_fw_down() /
pdsc_teardown() before pdsc_host_mem_free(), or by switching the
array to RCU?

> +
> +	seq_printf(seq, "Host memory requests: %d\n\n",
> +		   pdsc->num_host_mem_reqs);
> +	seq_puts(seq, "Tag    Size         Order  PA\n");
> +	seq_puts(seq, "---    ----         -----  --\n");
> +
> +	for (i = 0; i < pdsc->num_host_mem_reqs; i++) {
> +		hm = &pdsc->host_mem_reqs[i];
> +
> +		if (!hm->pg)
> +			continue;
> +
> +		seq_printf(seq, "%-6d %-12u %-6d 0x%llx\n",
> +			   hm->tag, hm->size, hm->order,
> +			   (unsigned long long)hm->pa);

[Low, Low]
hm->tag is u16 and hm->order is u8, but they are printed with the
signed %-6d specifier while hm->size (u32) uses %-12u. Should the
unsigned types use %u here as well?

Also, the canonical format specifier for dma_addr_t is %pad. Could
hm->pa be printed with &hm->pa and %pad rather than cast to unsigned
long long with 0x%llx?

> +	}
> +
> +	return 0;
> +}
> +DEFINE_SHOW_ATTRIBUTE(host_mem);
> +
> +void pdsc_debugfs_add_host_mem(struct pdsc *pdsc)
> +{
> +	struct dentry *dentry;
> +
> +	if (!(pdsc->dev_ident.capabilities &
> +	     cpu_to_le64(PDS_CORE_DEV_CAP_HOST_MEM)))
> +		return;
> +
> +	/* Check if file already exists (e.g., during reset recovery) */
> +	dentry = debugfs_lookup("host_mem", pdsc->dentry);
> +	if (!IS_ERR_OR_NULL(dentry)) {
> +		dput(dentry);
> +		return;
> +	}
> +
> +	debugfs_create_file("host_mem", 0400, pdsc->dentry,
> +			    pdsc, &host_mem_fops);
> +}

[Low]
Is the comment "Check if file already exists (e.g., during reset
recovery)" accurate here?

pdsc_debugfs_add_host_mem() is only called from pdsc_init_pf(), which
is itself only called from pdsc_probe(). The reset and recovery paths
go pdsc_fw_up() -> pdsc_setup() -> pdsc_start() -> pdsc_host_mem_add()
and never re-invoke pdsc_debugfs_add_host_mem().

By contrast, pdsc_debugfs_add_ident() is genuinely called during
recovery via pdsc_dev_init() from pdsc_setup(), so the same idiom
there is meaningful.

Should the dead defensive branch be dropped, or was the intent to
also wire pdsc_debugfs_add_host_mem() into the recovery path so the
file reflects the post-recovery host_mem_reqs array?

> diff --git a/drivers/net/ethernet/amd/pds_core/main.c b/drivers/net/ethernet/amd/pds_core/main.c
> index 58b4d77f6eca..aa7b5ce6f623 100644
> --- a/drivers/net/ethernet/amd/pds_core/main.c
> +++ b/drivers/net/ethernet/amd/pds_core/main.c
> @@ -266,6 +266,8 @@ static int pdsc_init_pf(struct pdsc *pdsc)
>  
>  	mutex_unlock(&pdsc->config_lock);
>  
> +	pdsc_debugfs_add_host_mem(pdsc);
> +
>  	err = pdsc_auxbus_dev_add(pdsc, pdsc, PDS_DEV_TYPE_FWCTL, &pdsc->padev);
>  	if (err)
>  		goto err_out_stop;