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

[sashiko-bot@kernel.org]

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

New issues:
- [High] Misuse of the `read_poll_timeout` macro causes an unlocked read-modify-write data race on `chan->status`, which can overwrite the interrupt handler's state completion and lead to an infinite loop.

Pre-existing issues:
- [High] The `dw_edma_device_terminate_all()` function performs read and write access to the state machine variables `chan->status` and `chan->request` without acquiring the required `chan->vc.lock` spinlock.
--

--- Patch [1]: [PATCH v2 1/3] dmaengine: dw-edma: Implement device_synchronize() callback ---
commit dad772ddd69c60671b5bb40802d893826d2fa9c5
Author: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>

dmaengine: dw-edma: Implement device_synchronize() callback

This commit implements the device_synchronize() callback for the dw-edma
driver. It uses read_poll_timeout() to wait for all in-flight DMA operations
to complete by polling the channel status. Once complete, it drains the
tasklet using vchan_synchronize().

> diff --git a/drivers/dma/dw-edma/dw-edma-core.c b/drivers/dma/dw-edma/dw-edma-core.c
> index c2feb3adc79fa..df0d1a946ed06 100644
> --- a/drivers/dma/dw-edma/dw-edma-core.c
> +++ b/drivers/dma/dw-edma/dw-edma-core.c
[ ... ]
> @@ -331,6 +332,20 @@ static int dw_edma_device_terminate_all(struct dma_chan *dchan)
>  	return err;
>  }

[Severity: High]
This isn't a bug introduced by this patch, but does
dw_edma_device_terminate_all() perform read and write accesses to state
machine variables without acquiring the required chan->vc.lock spinlock?

Looking at dw_edma_device_terminate_all(), it modifies chan->status and
chan->request locklessly:

    } else if (chan->request > EDMA_REQ_PAUSE) {
        err = -EPERM;
    } else {
        chan->request = EDMA_REQ_STOP;
    }

Since the DMA engine API allows asynchronous termination, could this race with
dw_edma_done_interrupt() or dw_edma_device_issue_pending(), both of which hold
the spinlock while modifying the same fields? If the interrupt handler reads
chan->request right before terminate_all writes it, it might mistakenly start
the next pending transfer instead of stopping it.

>  
> +static void dw_edma_device_synchronize(struct dma_chan *dchan)
> +{
> +	struct dw_edma_chan *chan = dchan2dw_edma_chan(dchan);
> +
> +	/*
> +	 * Make sure all the in-flight DMA operations are completed before
> +	 * draining the tasklet using vchan_synchronize().
> +	 */
> +	read_poll_timeout(READ_ONCE, chan->status, chan->status != EDMA_ST_BUSY,
> +			  10, 0, false, chan->status);

[Severity: High]
Could this read_poll_timeout() macro usage introduce a regression by causing
an unlocked read-modify-write data race on chan->status?

The read_poll_timeout() macro expands to (val) = op(args). By passing
chan->status as the val parameter, it expands to:

    (chan->status) = READ_ONCE(chan->status)

This loop executes without the chan->vc.lock spinlock. If the hardware
interrupt handler dw_edma_done_interrupt() fires exactly after READ_ONCE()
reads EDMA_ST_BUSY but before the assignment back to chan->status, it will
cleanly set chan->status = EDMA_ST_IDLE. 

Would the polling thread then overwrite the newly set EDMA_ST_IDLE state with
its stale EDMA_ST_BUSY value? Because the timeout parameter is 0, if this
corrupted EDMA_ST_BUSY state is written back, no further interrupts will arrive
to clear it, which could result in a permanent hang.

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260714-mhi-ep-flush-v2-0-b6a9db011e85@oss.qualcomm.com?part=1