Re: [PATCH v8 08/19] dmaengine: ti: k3-udma: move udma utility functions to k3-udma-common.c

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

Pre-existing issues:
- [High] Infinite loop in `udma_check_tx_completion` when channel is terminated concurrently.
- [High] DMA mappings are permanently leaked in `udma_setup_rx_flush` on error paths and during normal teardown.
- [High] RX flush buffer is mapped with `DMA_TO_DEVICE` instead of `DMA_FROM_DEVICE`, causing IOMMU faults.
- [High] Double free of `uc->terminated_desc` due to a TOCTOU race condition between `udma_ring_irq_handler` and `udma_reset_rings`.
--

--- Patch [8]: [PATCH v8 08/19] dmaengine: ti: k3-udma: move udma utility functions to k3-udma-common.c ---
commit 1751c5be6aa08d5072c4cebf22d4b8c37ab58711
Author: Sai Sree Kartheek Adivi <s-adivi@ti.com>
Date:   Mon Aug 10 15:14:02 2026 +0000

    dmaengine: ti: k3-udma: move udma utility functions to k3-udma-common.c
    
    Relocate udma utility functions from k3-udma.c to k3-udma-common.c file.
    
    The implementation of these functions is largely shared between K3 UDMA
    and K3 UDMA v2. This refactor improves code reuse and maintainability
    across multiple variants.
    
    No functional changes intended.

> diff --git a/drivers/dma/ti/k3-udma-common.c b/drivers/dma/ti/k3-udma-common.c
> index 3ec5cc932280c..892b88a220f18 100644
> --- a/drivers/dma/ti/k3-udma-common.c
> +++ b/drivers/dma/ti/k3-udma-common.c

[ ... ]

> @@ -1342,5 +1365,531 @@ void udma_reset_rings(struct udma_chan *uc)
> +void udma_check_tx_completion(struct work_struct *work)
> +{
> +	struct udma_chan *uc = container_of(work, typeof(*uc),
> +					    tx_drain.work.work);
> +	bool desc_done = true;
> +	u32 residue_diff;
> +	ktime_t time_diff;
> +	unsigned long delay;
> +	unsigned long flags;
> +
> +	while (1) {
> +		spin_lock_irqsave(&uc->vc.lock, flags);
> +
> +		if (uc->desc) {
> +			/* Get previous residue and time stamp */
> +			residue_diff = uc->tx_drain.residue;
> +			time_diff = uc->tx_drain.tstamp;
> +			/*
> +			 * Get current residue and time stamp or see if
> +			 * transfer is complete
> +			 */
> +			desc_done = udma_is_desc_really_done(uc, uc->desc);
> +		}
> +
> +		if (!desc_done) {
> +			/*
> +			 * Find the time delta and residue delta w.r.t
> +			 * previous poll
> +			 */
> +			time_diff = ktime_sub(uc->tx_drain.tstamp,
> +					      time_diff) + 1;
> +			residue_diff -= uc->tx_drain.residue;
> +			if (residue_diff) {
> +				/*
> +				 * Try to guess when we should check
> +				 * next time by calculating rate at
> +				 * which data is being drained at the
> +				 * peer device
> +				 */
> +				delay = (time_diff / residue_diff) *
> +					uc->tx_drain.residue;
> +			} else {
> +				/* No progress, check again in 1 second  */
> +				schedule_delayed_work(&uc->tx_drain.work, HZ);
> +				break;
> +			}
> +
> +			spin_unlock_irqrestore(&uc->vc.lock, flags);
> +
> +			usleep_range(ktime_to_us(delay),
> +				     ktime_to_us(delay) + 10);
> +			continue;

[Severity: High]
This is a pre-existing issue, but can this worker thread spin indefinitely
if the channel is terminated concurrently?

If udma_terminate_all() executes while this worker is sleeping, it sets
uc->desc = NULL. When the worker wakes up and reacquires the lock, it
skips the if (uc->desc) block, leaving the local variable desc_done as
false.

It then enters the if (!desc_done) block, repeatedly subtracting the
now-stale uc->tx_drain.residue from the local residue_diff. Because the
state is never refreshed, does the loop repeat endlessly until an eventual
integer underflow allows it to break millions of iterations later?

> +		}
> +
> +		if (uc->desc) {
> +			struct udma_desc *d = uc->desc;
> +
> +			uc->ud->decrement_byte_counters(uc, d->residue);
> +			uc->ud->start(uc);
> +			vchan_cookie_complete(&d->vd);
> +			break;
> +		}
> +
> +		break;
> +	}
> +
> +	spin_unlock_irqrestore(&uc->vc.lock, flags);
> +}

[ ... ]

> +void udma_synchronize(struct dma_chan *chan)
> +{
> +	struct udma_chan *uc = to_udma_chan(chan);
> +	unsigned long timeout = msecs_to_jiffies(1000);
> +
> +	vchan_synchronize(&uc->vc);
> +
> +	if (uc->state == UDMA_CHAN_IS_TERMINATING) {
> +		timeout = wait_for_completion_timeout(&uc->teardown_completed,
> +						      timeout);
> +		if (!timeout) {
> +			dev_warn(uc->ud->dev, "chan%d teardown timeout!\n",
> +				 uc->id);
> +			udma_dump_chan_stdata(uc);
> +			uc->ud->reset_chan(uc, true);
> +		}
> +	}
> +
> +	uc->ud->reset_chan(uc, false);
> +	if (udma_is_chan_running(uc))
> +		dev_warn(uc->ud->dev, "chan%d refused to stop!\n", uc->id);
> +
> +	cancel_delayed_work_sync(&uc->tx_drain.work);
> +	udma_reset_rings(uc);

[Severity: High]
This isn't a bug introduced by this patch, but is there a TOCTOU race
condition between udma_synchronize() calling udma_reset_rings() and the
udma_ring_irq_handler()?

In udma_ring_irq_handler(), when a teardown completion message is processed,
complete_all(&uc->teardown_completed) is called before uc->terminated_desc
is freed and cleared:

    if (cppi5_desc_is_tdcm(paddr)) {
        complete_all(&uc->teardown_completed);

        if (uc->terminated_desc) {
            udma_desc_free(&uc->terminated_desc->vd);
            uc->terminated_desc = NULL;
        }

Calling complete_all() immediately wakes up the thread waiting here in
udma_synchronize(). The woken thread then executes udma_reset_rings() which
performs an unlocked check of uc->terminated_desc.

In an SMP system, could the woken thread observe uc->terminated_desc as
non-NULL before the IRQ handler on the original CPU sets it to NULL, causing
both threads to concurrently attempt a double-free?

> +}

[ ... ]

> +int udma_setup_rx_flush(struct udma_dev *ud)
> +{
> +	struct udma_rx_flush *rx_flush = &ud->rx_flush;
> +	struct cppi5_desc_hdr_t *tr_desc;
> +	struct cppi5_tr_type1_t *tr_req;
> +	struct cppi5_host_desc_t *desc;
> +	struct device *dev = ud->dev;
> +	struct udma_hwdesc *hwdesc;
> +	size_t tr_size;
> +
> +	/* Allocate 1K buffer for discarded data on RX channel teardown */
> +	rx_flush->buffer_size = SZ_1K;
> +	rx_flush->buffer_vaddr = devm_kzalloc(dev, rx_flush->buffer_size,
> +					      GFP_KERNEL);
> +	if (!rx_flush->buffer_vaddr)
> +		return -ENOMEM;
> +
> +	rx_flush->buffer_paddr = dma_map_single(dev, rx_flush->buffer_vaddr,
> +						rx_flush->buffer_size,
> +						DMA_TO_DEVICE);

[Severity: High]
This is a pre-existing issue, but is DMA_TO_DEVICE the correct direction
for this mapping?

This buffer seems to act as a memory sink for discarded incoming data
when tearing down a DEV_TO_MEM channel. Because the DMA engine writes data
to this buffer, the IOMMU must grant write access to the device. However,
mapping it with DMA_TO_DEVICE configures the IOMMU to allow only device reads.

Will a strict IOMMU trap the write and trigger a fault?

> +	if (dma_mapping_error(dev, rx_flush->buffer_paddr))
> +		return -ENOMEM;
> +
> +	/* Set up descriptor to be used for TR mode */
> +	hwdesc = &rx_flush->hwdescs[0];
> +	tr_size = sizeof(struct cppi5_tr_type1_t);
> +	hwdesc->cppi5_desc_size = cppi5_trdesc_calc_size(tr_size, 1);
> +	hwdesc->cppi5_desc_size = ALIGN(hwdesc->cppi5_desc_size,
> +					ud->desc_align);
> +
> +	hwdesc->cppi5_desc_vaddr = devm_kzalloc(dev, hwdesc->cppi5_desc_size,
> +						GFP_KERNEL);
> +	if (!hwdesc->cppi5_desc_vaddr)
> +		return -ENOMEM;

[Severity: High]
This isn't a bug introduced by this patch, but does this error path permanently
leak the DMA mapping (IOVA space)?

If the allocation for hwdesc->cppi5_desc_vaddr fails, the function returns
-ENOMEM without calling dma_unmap_single() for rx_flush->buffer_paddr.
While devres frees the memory, IOVA space mapped via dma_map_single() must
be explicitly unmapped.

Additionally, is there a corresponding dma_unmap_single() invoked during
normal driver teardown?

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