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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] ti: k3-psil: Data race when reading/writing shared static endpoint configuration
- [Low] ti: k3-psil: Misleading error log prints channel ID instead of thread ID
--

commit fa7e3efdc8ec449e5c46f7824fe82e5b4d465743
Author: Sai Sree Kartheek Adivi <s-adivi@ti.com>
Subject: 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 endpoint type for multiple selectable FIFOs along with a new
fifo_id field.

> diff --git a/drivers/dma/ti/k3-psil.c b/drivers/dma/ti/k3-psil.c
> index c4b6f0df46861..153ed5fc79e53 100644
> --- a/drivers/dma/ti/k3-psil.c
> +++ b/drivers/dma/ti/k3-psil.c
> @@ -71,12 +72,68 @@ 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->src) {
> +		for (i = 0; i < soc_ep_map->src_count; i++) {
> +			struct psil_ep *ep = &soc_ep_map->src[i];
> +
> +			if (ep->ep_config.mapped_channel_id != (s16)channel_id)
> +				continue;
> +			if ((bool)ep->ep_config.pkt_mode != is_pktdma)
> +				continue;

[Severity: Medium]
Could this iteration cause a data race?

This loop reads fields from the shared ep_config struct while holding
ep_map_mutex. However, in psil_set_new_ep_config(), the exact same
ep_config fields are mutated locklessly via:

    memcpy(dst_ep_config, ep_config, sizeof(*dst_ep_config));

Since this iteration reads fields inside ep_config while the memcpy()
overwrites them locklessly, a race condition appears to exist that could
lead to torn reads of endpoint configurations like mapped_channel_id or
the pkt_mode bitfield.

[ ... ]
> @@ -90,9 +147,34 @@ int psil_set_new_ep_config(struct device *dev, const char *name,
>  				       index, &dma_spec))
>  		return -ENOENT;
>  
> -	thread_id = dma_spec.args[0];
> +	if (of_device_is_compatible(dma_spec.np, "ti,am62l-dmss-pktdma")) {
> +		bool dev_to_mem;
> +
> +		if (dma_spec.args_count < 1) {
> +			of_node_put(dma_spec.np);
> +			return -EINVAL;
> +		}
>  
> -	dst_ep_config = psil_get_ep_config(thread_id);
> +		thread_id = dma_spec.args[0];
> +		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;
> +
> +		if (dma_spec.args_count < 3) {
> +			of_node_put(dma_spec.np);
> +			return -EINVAL;
> +		}
> +
> +		thread_id = dma_spec.args[2];
> +		dst_ep_config = psil_get_ep_config_by_id(dma_spec.args[2],
> +							  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: Low]
Is this error message misleading?

If psil_get_ep_config_by_id() fails to find a matching channel for the
ti,am62l-dmss-pktdma or ti,am62l-dmss-bcdma bindings, it returns an error
pointer without modifying the thread_id variable.

At this point, the thread_id variable still holds the raw channel ID
extracted from the device tree (such as dma_spec.args[0] or
dma_spec.args[2]). Printing it as "PSIL: thread ID 0x%04x not defined in
map" displays the channel ID as if it were a thread ID, which could cause
confusion during device tree debugging.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260928025749.1055383-1-s-adivi@ti.com?part=15