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

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

Pre-existing issues:
- [Critical] Unprotected access to `uc->terminated_desc` in `udma_reset_rings` races with `udma_ring_irq_handler`, causing a potential double-free or use-after-free.
- [High] The delayed work `udma_check_tx_completion` can enter a runaway loop and incorrectly reschedule itself if its channel is terminated concurrently, leaving a rogue timer active.
--

--- Patch [8]: [PATCH v7 08/19] dmaengine: ti: k3-udma: move udma utility functions to k3-udma-common.c ---
commit a1188aac2ed37ae50666251d86303353027b5fbb
Author: Sai Sree Kartheek Adivi <[email protected]>
Subject: dmaengine: ti: k3-udma: move udma utility functions to k3-udma-common.c

This commit relocates several utility functions from k3-udma.c to
k3-udma-common.c to improve code reuse and maintainability between K3 UDMA
and K3 UDMA v2 variants. There are no intended functional changes.

diff --git a/drivers/dma/ti/k3-udma-common.c b/drivers/dma/ti/k3-udma-common.c
index fc5e2c99ee28a..01aea77c25715 100644
--- a/drivers/dma/ti/k3-udma-common.c
+++ b/drivers/dma/ti/k3-udma-common.c
[ ... ]
> +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) {

[Severity: High]
This isn't a bug introduced by this patch, but is a pre-existing logic
issue. If uc->desc becomes NULL during the worker's sleep (e.g., if
udma_terminate_all() is called concurrently):

udma_terminate_all() {
	if (uc->desc) {
		uc->terminated_desc = uc->desc;
		uc->desc = NULL;
...

could the worker wake up, bypass the if (uc->desc) check, and see
desc_done still false from the previous iteration?

It seems this would use stale residue_diff values on every subsequent loop
iteration, continually sleeping until residue_diff wraps around to 0:

[ ... ]
> +			} else {
> +				/* No progress, check again in 1 second  */
> +				schedule_delayed_work(&uc->tx_drain.work, HZ);
> +				break;
> +			}

and then incorrectly reschedule itself, leaving a rogue delayed work armed
despite the channel being terminated?

[ ... ]
> +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);
[ ... ]
> +	}
> +
> +	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: Critical]
This is a pre-existing issue, but does udma_reset_rings() race with
udma_ring_irq_handler() and potentially cause a double-free?

When a DMA channel is terminated, udma_synchronize() waits for the teardown
completion. The hardware raises an interrupt, and udma_ring_irq_handler()
signals completion before it finishes clearing uc->terminated_desc:

drivers/dma/ti/k3-udma.c:udma_ring_irq_handler() {
	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;
		}
...

This immediately wakes up udma_synchronize(), which proceeds to call
udma_reset_rings() without holding uc->vc.lock:

drivers/dma/ti/k3-udma-common.c:udma_reset_rings() {
...
	if (uc->terminated_desc) {
		udma_desc_free(&uc->terminated_desc->vd);
		uc->terminated_desc = NULL;
	}
}

If the awakened thread reaches udma_reset_rings() while the IRQ handler
is still executing udma_desc_free(), couldn't both threads invoke
udma_desc_free() on the same descriptor?

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