Re: [PATCH v2 12/19] dmaengine: dw-edma: Recover stopped channels from tx_status()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] Polling and restarting a stopped channel in tx_status races with the hardware STOP interrupt handler, causing the newly running transfer to be misinterpreted as stopped, leading to premature completion and data corruption.
--

--- Patch [12]: [PATCH v2 12/19] dmaengine: dw-edma: Recover stopped channels from tx_status() ---
commit b34ccd5c57949fabf1e4daf7d4e7cfdaa0576cc4
Author: Koichiro Den <den@valinux.co.jp>

    dmaengine: dw-edma: Recover stopped channels from tx_status()

This commit allows tx_status() to consume IRQ-recorded progress under
vc.lock. If a channel is stopped with pending entries, it reconciles a
fresh LLP boundary and restarts the channel.

> diff --git a/drivers/dma/dw-edma/dw-edma-core.c b/drivers/dma/dw-edma/dw-edma-core.c
> index 9489ca7e70fda..3fc929a6bcf52 100644
> --- a/drivers/dma/dw-edma/dw-edma-core.c
> +++ b/drivers/dma/dw-edma/dw-edma-core.c

[ ... ]

> @@ -654,6 +662,36 @@ dw_edma_device_tx_status(struct dma_chan *dchan, dma_cookie_t cookie,
>  	if (ret == DMA_COMPLETE)
>  		return ret;
>  
> +	if (!chan->non_ll) {
> +		scoped_guard(spinlock_irqsave, &chan->vc.lock) {
> +			enum dma_status ch_status;
> +
> +			ch_status = dw_edma_core_ch_status(chan);
> +
> +			/*
> +			 * Do not treat raw DMA_LLP polling as normal progress here. Only
> +			 * recycle progress recorded by the IRQ pass that consumed the
> +			 * status event.
> +			 */
> +			if (chan->request == EDMA_REQ_NONE &&
> +			    chan->status != EDMA_ST_PAUSE &&
> +			    ch_status == DMA_COMPLETE &&
> +			    dw_edma_ll_consume_progress(chan)) {
> +				dw_edma_start_transfer(chan);
> +				chan->status = dw_edma_ll_pending(chan) ?
> +					       EDMA_ST_BUSY : EDMA_ST_IDLE;
> +			}
> +
> +			if (dw_edma_ll_recoverable_pending(chan, ch_status))
> +				chan->status = EDMA_ST_BUSY;
> +			dw_edma_core_ch_maybe_doorbell(chan);

[Severity: Critical]
Can this race with the hard IRQ handler and cause data corruption?

If a transfer completes and the hardware stops, the hard IRQ handler 
(e.g., dw_hdma_v0_core_handle_int()) will read and clear the STOP status,
then call done(chan, true) which queues the deferred handler with stopped=true.

Before the deferred handler acquires chan->vc.lock, could a client polling
dw_edma_device_tx_status() acquire the lock, observe DMA_COMPLETE, and restart
the channel by hitting the doorbell here?

If so, when the deferred handler finally acquires the lock in
dw_edma_record_irq_idx(), it samples the LLP which is now moving since the
channel has been restarted. Because it uses the stale stopped=true flag,
dw_edma_ll_recycle_idx() returns the exact moving LLP index without applying
the idx - 1 safety margin. 

Could this cause descriptors to be completed prematurely while the hardware is
still actively transferring data?

> +		}
> +
> +		/* dw_edma_ll_clean_pending() may have completed this cookie. */
> +		ret = dma_cookie_status(dchan, cookie, txstate);
> +		if (ret == DMA_COMPLETE)
> +			return ret;
> +	}
>  	if (ret == DMA_IN_PROGRESS && chan->status == EDMA_ST_PAUSE)
>  		ret = DMA_PAUSED;
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260723084150.521366-1-den@valinux.co.jp?part=12