Re: [PATCH v8 15/19] dmaengine: ti: k3-psil-am62l: Add AM62Lx PSIL and PDMA data

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] psil_set_new_ep_config() uses the incorrect dma-cells index for BCDMA channel ID lookup.
- [Medium] Misleading error message on lookup failure in psil_set_new_ep_config() logs '0x0000'.
--

--- Patch [15]: [PATCH v8 15/19] dmaengine: ti: k3-psil-am62l: Add AM62Lx PSIL and PDMA data ---
commit 341f27f741b8359a8b0d9ea36665be0d14b7d6b0
Author: Sai Sree Kartheek Adivi <[email protected]>

dmaengine: ti: k3-psil-am62l: Add AM62Lx PSIL and PDMA data

This commit adds PSIL and PDMA data for the AM62Lx SoC. It also introduces a
new PSIL_EP_PDMA_XYMF type for channels that have multiple selectable fifos
and adds a fifo_id field to the psil_endpoint_config structure.

> diff --git a/drivers/dma/ti/k3-psil.c b/drivers/dma/ti/k3-psil.c
> index c4b6f0df46861..3a9b43e179d1e 100644
> --- a/drivers/dma/ti/k3-psil.c
> +++ b/drivers/dma/ti/k3-psil.c

[ ... ]

> @@ -90,9 +147,22 @@ int psil_set_new_ep_config(struct device *dev, const char *name,
>  				       index, &dma_spec))
>  		return -ENOENT;
>  
> -	thread_id = dma_spec.args[0];
> -
> -	dst_ep_config = psil_get_ep_config(thread_id);
> +	if (of_device_is_compatible(dma_spec.np, "ti,am62l-dmss-pktdma")) {
> +		bool dev_to_mem;
> +
> +		dst_ep_config = psil_get_ep_config_by_id(dma_spec.args[0],
> +							  true,
> +							  &thread_id, &dev_to_mem);
> +	} else if (of_device_is_compatible(dma_spec.np, "ti,am62l-dmss-bcdma")) {
> +		bool dev_to_mem;
> +
> +		dst_ep_config = psil_get_ep_config_by_id(dma_spec.args[0],
> +							  false,
> +							  &thread_id, &dev_to_mem);

[Severity: High]
For devices matching ti,am62l-dmss-bcdma, does this code pass the trigger type
instead of the mapped channel ID to psil_get_ep_config_by_id?

If dma_spec.args[0] is the trigger type for a BCDMA node (which has 4 cells),
this lookup might consistently fail with -ENOENT since it will not match a
valid mapped channel ID. Would passing dma_spec.args[2] be the intended
approach here?

> +	} else {
> +		thread_id = dma_spec.args[0];
> +		dst_ep_config = psil_get_ep_config(thread_id);
> +	}
>  	if (IS_ERR(dst_ep_config)) {
>  		pr_err("PSIL: thread ID 0x%04x not defined in map\n",
>  		       thread_id);

[Severity: Medium]
When psil_get_ep_config_by_id fails to find the endpoint, it returns
ERR_PTR(-ENOENT) without updating the thread_id out-parameter.

Since thread_id is initialized to 0 at the start of psil_set_new_ep_config,
would this error path print an uninitialized thread ID (0x0000) upon a lookup
failure? This might hide the actual failing channel or thread ID and make
debugging difficult.

>  		of_node_put(dma_spec.np);
>  		return PTR_ERR(dst_ep_config);
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=15