Re: [PATCH 1/3] dmaengine: dw-edma: Implement device_synchronize() callback

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] dw_edma_device_synchronize() silently fails if the timeout is reached, returning to the caller while the hardware channel is still active.
- [High] dw_edma_device_synchronize() implements a 5000ms busy-wait loop using cpu_relax() in a sleepable context.
- [Medium] Lockless read of chan->status without READ_ONCE() or memory barriers in dw_edma_device_synchronize().

Pre-existing issues:
- [High] dw_edma_device_terminate_all() fails to flush pending virt-dma descriptor queues, causing memory leaks and the risk of stale transfers resuming.
- [High] Virt-DMA descriptors are permanently leaked during channel teardown because vchan_free_chan_resources() is not called.
- [High] dw_edma_device_terminate_all() modifies channel state without holding the required spinlock.
--

commit dde223ef8e9950c9d18889da8638ba0fd963f5d8
Author: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>

dmaengine: dw-edma: Implement device_synchronize() callback

device_synchronize() callback is required by the client drivers to ensure
all the DMA operations are completed so that they can free the memory
associated with the complete callbacks.

So implement this callback by first making sure that all the in-flight DMA
operations are completed and then call vchan_synchronize() to drain the
DMA tasklet.

> diff --git a/drivers/dma/dw-edma/dw-edma-core.c b/drivers/dma/dw-edma/dw-edma-core.c
> index c2feb3adc79fa..7b12dfe8cfd3e 100644
> --- a/drivers/dma/dw-edma/dw-edma-core.c
> +++ b/drivers/dma/dw-edma/dw-edma-core.c
> @@ -331,6 +331,21 @@ static int dw_edma_device_terminate_all(struct dma_chan *dchan)

[Severity: High]
This is a pre-existing issue, but looking at dw_edma_device_terminate_all(),
it seems to lack the necessary virt-dma calls (like vchan_get_all_descriptors)
to extract and free pending descriptors:

    } else {
        chan->request = EDMA_REQ_STOP;
    }

Since dmaengine_terminate_all() requires all active and pending descriptors
to be aborted and removed from the channel's queues, could setting this
software flag without flushing the virt-dma descriptor lists cause queued
descriptors to remain in vc->desc_issued and vc->desc_submitted?

If a client later submits new transfers, might the hardware unintentionally
resume processing the old, supposedly terminated descriptors?

[Severity: High]
This is a pre-existing issue, but does dw_edma_device_terminate_all()
modify channel state without holding the required spinlock?

    } else if (chan->status == EDMA_ST_PAUSE) {
        chan->status = EDMA_ST_IDLE;
        chan->configured = false;

Since chan->status, chan->request, and chan->configured are concurrently
accessed and updated by the interrupt handler and issue pending paths under
chan->vc.lock, does modifying them unlocked introduce a race condition?

Could this corrupt the channel's internal state machine when a client
calls dmaengine_terminate_all() during interrupt processing?

>  	return err;
>  }
>  
> +static void dw_edma_device_synchronize(struct dma_chan *dchan)
> +{
> +	struct dw_edma_chan *chan = dchan2dw_edma_chan(dchan);
> +	unsigned long timeout = jiffies + msecs_to_jiffies(5000);
> +
> +	/*
> +	 * Make sure all the in-flight DMA operations are completed before
> +	 * draining the tasklet using vchan_synchronize().
> +	 */
> +	while (chan->status == EDMA_ST_BUSY && time_before(jiffies, timeout))

[Severity: Medium]
Since chan->status is concurrently mutated by the interrupt handler under
vc->lock, does reading it locklessly in this while loop without READ_ONCE()
or memory barriers constitute a data race?

Could this cause synchronization bypass if the state update is observed out
of order relative to the tasklet scheduling?

> +		cpu_relax();

[Severity: High]
Is it safe to implement a 5000ms busy-wait loop using cpu_relax() in a
sleepable context like dw_edma_device_synchronize()?

Since busy-waiting for up to 5 seconds blocks a CPU core entirely, could
this starve other tasks and trigger soft lockups when a client driver calls
dmaengine_synchronize() to wait for DMA completion? Since this callback is
explicitly allowed to sleep, should a sleeping wait like wait_event_timeout()
or msleep() be used instead?

> +
> +	vchan_synchronize(&chan->vc);

[Severity: Critical]
Does dw_edma_device_synchronize() silently fail if the timeout is reached?

If the 5-second timeout expires while the channel is still busy, the loop
exits and the function returns to the caller without stopping the ongoing
hardware transfer or warning the client.

Since the client relies on device_synchronize() to ensure all active DMA
operations have fully completed before unmapping and freeing memory, could
this lead to silent data corruption and a use-after-free if the hardware
is still actively transferring data?

Should the function either wait indefinitely or forcefully abort the hardware
and emit a warning on timeout?

> +}

[ ... ]

[Severity: High]
This is a pre-existing issue, but does dw_edma_free_chan_resources()
permanently leak virt-dma descriptors during channel teardown?

Looking at dw_edma_free_chan_resources():

    static void dw_edma_free_chan_resources(struct dma_chan *dchan)
    {
        ...
        // No call to vchan_free_chan_resources(&chan->vc)
    }

When a DMA channel is released via dma_release_channel(), this teardown
function is called. Because dw_edma_device_terminate_all() leaves
uncompleted descriptors on the virt-dma lists, and this function fails to
invoke vchan_free_chan_resources(), are any pending or submitted
descriptors permanently leaked during a client driver unbind operation?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260629-mhi-ep-flush-v1-0-714e0d56e87c@oss.qualcomm.com?part=1