Re: [PATCH v8 18/19] dmaengine: ti: k3-udma-v2: Update glue layer to support PKTDMA V2

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Hardware TX and RX channels are left enabled if the V2 autopairing process times out, leading to potential DMA Use-After-Free.
- [Low] Unmet direct dependency for `TI_K3_RINGACC` when `TI_K3_UDMA_V2` is selected.
--

--- Patch [18]: [PATCH v8 18/19] dmaengine: ti: k3-udma-v2: Update glue layer to support PKTDMA V2 ---
commit 679ff99789e75f58a08b38ae95963dd08f423c94
Author: Sai Sree Kartheek Adivi <[email protected]>

dmaengine: ti: k3-udma-v2: Update glue layer to support PKTDMA V2
    
Update glue layer to support PKTDMA V2 for non DMAengine users.
The updates include handling absence of TISCI, direct IRQs, and
Autopair: Lack of PSIL pair.

> diff --git a/drivers/dma/ti/Kconfig b/drivers/dma/ti/Kconfig
> index 40713bd1e8e9b..ada2ea8aca4b0 100644
> --- a/drivers/dma/ti/Kconfig
> +++ b/drivers/dma/ti/Kconfig
> @@ -68,7 +68,7 @@ config TI_K3_UDMA_COMMON
>  config TI_K3_UDMA_GLUE_LAYER
>  	tristate "Texas Instruments UDMA Glue layer for non DMAengine users"
>  	depends on ARCH_K3 || COMPILE_TEST
> -	depends on TI_K3_UDMA
> +	depends on TI_K3_UDMA || TI_K3_UDMA_V2

[Severity: Low]
Does selecting TI_K3_UDMA_V2 cause an unmet dependency warning?

TI_K3_UDMA_V2 selects TI_K3_RINGACC but lacks the depends on
TI_SCI_INTA_IRQCHIP dependency required by TI_K3_RINGACC.

>  	help
>  	  Say y here to support the K3 NAVSS DMA glue interface
>  	  If unsure, say N.

[ ... ]

> diff --git a/drivers/dma/ti/k3-udma-glue.c b/drivers/dma/ti/k3-udma-glue.c
> index bec4deb480e60..9b9921e904bc2 100644
> --- a/drivers/dma/ti/k3-udma-glue.c
> +++ b/drivers/dma/ti/k3-udma-glue.c

[ ... ]

> @@ -525,21 +539,38 @@ int k3_udma_glue_enable_tx_chn(struct k3_udma_glue_tx_channel *tx_chn)
>  {
>  	int ret;
>  
> -	ret = xudma_navss_psil_pair(tx_chn->common.udmax,
> -				    tx_chn->common.src_thread,
> -				    tx_chn->common.dst_thread);
> -	if (ret) {
> -		dev_err(tx_chn->common.dev, "PSI-L request err %d\n", ret);
> -		return ret;
> -	}
> +	if (tx_chn->common.udmax->match_data->version == K3_UDMA_V1) {

[ ... ]

> +	} else {
> +		u32 val;
> +
> +		xudma_tchanrt_write(tx_chn->udma_tchanx, UDMA_CHAN_RT_CTL_REG,
> +				    UDMA_CHAN_RT_CTL_AUTOPAIR | UDMA_CHAN_RT_CTL_EN);
> +
> +		ret = read_poll_timeout(xudma_tchanrt_read, val,
> +					val & (UDMA_CHAN_RT_CTL_PAIR_COMPLETE |
> +					       UDMA_CHAN_RT_CTL_PAIR_TIMEOUT),
> +					100, 500, false,
> +					tx_chn->udma_tchanx, UDMA_CHAN_RT_CTL_REG);
> +		if (ret || (val & UDMA_CHAN_RT_CTL_PAIR_TIMEOUT)) {
> +			dev_err(tx_chn->common.dev, "TX channel autopair timeout\n");
> +			return -ETIMEDOUT;
> +		}

[Severity: High]
If the autopairing times out, does this leave the TX channel enabled?

Since UDMA_CHAN_RT_CTL_EN was set before the timeout and is not cleared on
this error path, could the hardware eventually pair and perform DMA accesses
to freed memory if callers assume a failure means the channel is inactive?

> +	}
>  
>  	k3_udma_glue_dump_tx_rt_chn(tx_chn, "txchn en");
>  	return 0;

[ ... ]

> @@ -1406,21 +1481,38 @@ int k3_udma_glue_enable_rx_chn(struct k3_udma_glue_rx_channel *rx_chn)
>  	if (rx_chn->flows_ready < rx_chn->flow_num)
>  		return -EINVAL;
>  
> -	ret = xudma_navss_psil_pair(rx_chn->common.udmax,
> -				    rx_chn->common.src_thread,
> -				    rx_chn->common.dst_thread);
> -	if (ret) {
> -		dev_err(rx_chn->common.dev, "PSI-L request err %d\n", ret);
> -		return ret;
> -	}
> +	if (rx_chn->common.udmax->match_data->version == K3_UDMA_V1) {

[ ... ]

> +	} else {
> +		u32 val;
> +
> +		xudma_rchanrt_write(rx_chn->udma_rchanx, UDMA_CHAN_RT_CTL_REG,
> +				    UDMA_CHAN_RT_CTL_AUTOPAIR | UDMA_CHAN_RT_CTL_EN);
> +
> +		ret = read_poll_timeout(xudma_rchanrt_read, val,
> +					val & (UDMA_CHAN_RT_CTL_PAIR_COMPLETE |
> +					       UDMA_CHAN_RT_CTL_PAIR_TIMEOUT),
> +					100, 500, false,
> +					rx_chn->udma_rchanx, UDMA_CHAN_RT_CTL_REG);
> +		if (ret || (val & UDMA_CHAN_RT_CTL_PAIR_TIMEOUT)) {
> +			dev_err(rx_chn->common.dev, "RX channel autopair timeout\n");
> +			return -ETIMEDOUT;
> +		}

[Severity: High]
Similarly for the RX channel, if the autopairing times out, does this return
-ETIMEDOUT without disabling the hardware channel?

Could this lead to memory corruption if the hardware operates after the driver
has freed the associated ring buffers?

> +	}
>  
>  	k3_udma_glue_dump_rx_rt_chn(rx_chn, "rxrt en");
>  	return 0;

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