Re: [PATCH v10 17/19] dmaengine: ti: k3-udma-v2: Add support for PKTDMA V2

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] dmaengine: ti: k3-udma: inconsistent IRQ lookup keys for PKTDMA V2
- [High] dmaengine: ti: k3-udma: ring reset races with interrupt handler
--

commit 9f976c6183eaa9059066ffde518addf6969bc382
Author: Sai Sree Kartheek Adivi <s-adivi@ti.com>
Subject: dmaengine: ti: k3-udma-v2: Add support for PKTDMA V2

The PKTDMA V2 is different than the existing PKTDMA supported by the
k3-udma driver.

> diff --git a/drivers/dma/ti/k3-udma-private.c b/drivers/dma/ti/k3-udma-private.c
> index 44c097fff5ee6..525155d3bb513 100644
> --- a/drivers/dma/ti/k3-udma-private.c
> +++ b/drivers/dma/ti/k3-udma-private.c
> @@ -174,16 +174,30 @@ EXPORT_SYMBOL(xudma_is_pktdma);
>  
>  int xudma_pktdma_tflow_get_irq(struct udma_dev *ud, int udma_tflow_id)
>  {
> -	const struct udma_oes_offsets *oes = &ud->soc_data->oes;
> +	if (ud->match_data->version == K3_UDMA_V1) {
> +		const struct udma_oes_offsets *oes = &ud->soc_data->oes;
>  
> -	return msi_get_virq(ud->dev, udma_tflow_id + oes->pktdma_tchan_flow);
> +		return msi_get_virq(ud->dev, udma_tflow_id + oes->pktdma_tchan_flow);
> +	}
> +	struct platform_device *pdev = to_platform_device(ud->dev);
> +	char irq_name[10];
> +
> +	snprintf(irq_name, sizeof(irq_name), "tx-%u", udma_tflow_id);

[Severity: High]
Does this cause an inconsistent IRQ lookup? In
pktdma_v2_alloc_chan_resources(), the driver formats the interrupt name
using mapped_channel_id. However, here in the glue layer helper, the same
string format is used with the flow ID (udma_tflow_id).

Since flow ID and mapped channel ID can differ in PKTDMA V2 (for example,
tflow_cnt=112 vs chan_cnt=97), they could look up different strings for the
exact same device node, leading to either the DMA engine or the glue layer
client acquiring the wrong interrupt.

> +	return platform_get_irq_byname(pdev, irq_name);
>  }
>  EXPORT_SYMBOL(xudma_pktdma_tflow_get_irq);

[ ... ]

> diff --git a/drivers/dma/ti/k3-udma-v2.c b/drivers/dma/ti/k3-udma-v2.c
> index ba3042e866cac..a71471aa7707a 100644
> --- a/drivers/dma/ti/k3-udma-v2.c
> +++ b/drivers/dma/ti/k3-udma-v2.c

[ ... ]

> +static int pktdma_v2_alloc_chan_resources(struct dma_chan *chan)
> +{

[ ... ]

> +	if (uc->config.dir == DMA_MEM_TO_DEV)
> +		snprintf(irq_name, sizeof(irq_name), "tx-%u", irq_ring_idx);
> +	else
> +		snprintf(irq_name, sizeof(irq_name), "rx-%u", irq_ring_idx);
> +	uc->irq_num_ring = platform_get_irq_byname(pdev, irq_name);
> +	if (uc->irq_num_ring < 0) {
> +		ret = uc->irq_num_ring;
> +		goto err_res_free;
> +	}
> +
> +	ret = request_irq(uc->irq_num_ring, udma_v2_ring_irq_handler,
> +			  IRQF_SHARED | IRQF_TRIGGER_HIGH, uc->name, uc);
> +
> +	if (ret) {
> +		dev_err(ud->dev, "chan%d: ring irq request failed\n", uc->id);
> +		goto err_irq_free;
> +	}
> +
> +	uc->irq_num_udma = 0;
> +
> +	udma_reset_rings(uc);

[Severity: High]
Does this create a race condition where the interrupt handler might execute
before the ring state is fully reset?

If the shared IRQ line fires early or a stale interrupt is already pending,
udma_v2_ring_irq_handler() could be called before udma_reset_rings()
completes. Would it be safer to reset the hardware state before requesting
the IRQ?

> +
> +	if (uc->tchan)
> +		dev_dbg(ud->dev,
> +			"chan%d: tchan%d, tflow%d, Remote thread: 0x%04x\n",
> +			uc->id, uc->tchan->id, uc->tchan->tflow_id,
> +			uc->config.remote_thread_id);

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