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

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] NULL pointer dereference in psil_get_ep_config_by_id() if the SoC is not matched.
- [High] psil_set_new_ep_config() incorrectly checks the client device node instead of the DMA controller node for compatibility.
- [Medium] Uninitialized stack variable 'thread_id' is logged on the error path, leaking stack memory.
--

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

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

Add PSIL and PDMA data for AM62Lx SoC.

Introduce new PSIL_EP_PDMA_XYMF type for channels that have multiple
selectable fifos. Also add a new fifo_id field to psil_endpoint_config
to specify the selectable fifo_id.

> diff --git a/drivers/dma/ti/k3-psil.c b/drivers/dma/ti/k3-psil.c
> index c4b6f0df46861..ed9c92d5a7b2b 100644
> --- a/drivers/dma/ti/k3-psil.c
> +++ b/drivers/dma/ti/k3-psil.c
[ ... ]
> @@ -71,6 +72,57 @@ struct psil_endpoint_config *psil_get_ep_config(u32 thread_id)
>  }
>  EXPORT_SYMBOL_GPL(psil_get_ep_config);
>  
> +struct psil_endpoint_config *psil_get_ep_config_by_id(u32 channel_id,
> +						       bool is_pktdma,
> +						       u32 *thread_id,
> +						       bool *dev_to_mem)
> +{
> +	int i;
> +
> +	mutex_lock(&ep_map_mutex);
> +	if (!soc_ep_map) {
> +		const struct soc_device_attribute *soc;
> +
> +		soc = soc_device_match(k3_soc_devices);
> +		if (soc)
> +			soc_ep_map = soc->data;
> +	}
> +
> +	if (soc_ep_map->src) {

[Severity: High]
If soc_device_match() fails to find a match, soc_ep_map is not populated and
remains NULL. Will this unconditionally dereference soc_ep_map here?

> +		for (i = 0; i < soc_ep_map->src_count; i++) {
> +			struct psil_ep *ep = &soc_ep_map->src[i];
[ ... ]
> @@ -90,9 +142,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(dev->of_node, "ti,am62l-dmss-pktdma")) {

[Severity: High]
Are we checking the compatibility of the correct device node here?

dev->of_node is the client device's node, which won't match the DMA
controller's compatible string. Should this check dma_spec.np instead?

> +		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(dev->of_node, "ti,am62l-dmss-bcdma")) {

[Severity: High]
This check also seems to use the client device node (dev->of_node) instead of
the DMA controller node.

> +		bool dev_to_mem;
> +
> +		dst_ep_config = psil_get_ep_config_by_id(dma_spec.args[0],
> +							  false,
> +							  &thread_id, &dev_to_mem);
> +	} 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]
If psil_get_ep_config_by_id() fails to find a channel and returns an error
pointer, thread_id is left uninitialized. Does this cause the error log to
print uninitialized stack memory?

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

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