diff mbox series

[v3,1/2] dmaengine: sun6i: fix non-atomic read of DMA position registers

Message ID 20260817135723.12807-2-christian.lugnberg@soundtrack.io (mailing list archive)
State New
Headers
Series dmaengine: sun6i: Fix two bugs in the DMA status path |

Commit Message

Christian Lugnberg Aug. 17, 2026, 1:51 p.m. UTC 
sun6i_get_chan_size() reads DMA_CHAN_LLI_ADDR and DMA_CHAN_CUR_CNT in two
separate readl() calls with no synchronisation between them:

    pos   = readl(pchan->base + DMA_CHAN_LLI_ADDR);
    bytes = readl(pchan->base + DMA_CHAN_CUR_CNT);

DMA_CHAN_LLI_ADDR holds the physical address of the *next* descriptor the
engine will load once the current one completes. DMA_CHAN_CUR_CNT holds the
remaining byte count for the *current* descriptor. If the DMA engine
advances to the next LLI entry between the two reads, pos becomes stale: it
still points to what was the next descriptor at the time of the first read,
but that descriptor is now the current one and CUR_CNT reflects its initial
(full) byte count. The subsequent virtual-chain walk starts one entry too
early and accumulates an extra full period's worth of bytes into the
residue estimate.

Fix this by re-reading DMA_CHAN_LLI_ADDR after DMA_CHAN_CUR_CNT and
retrying if the value changed. This double-read pattern guarantees that
both registers were sampled during the same descriptor interval. The cost
is at most one extra readl() pair per call in the racy case, which occurs
only at descriptor boundaries (~every 2 ms) and is negligible.

Fixes: a90e173f3faf ("dmaengine: sun6i: Add cyclic capability")
Cc: stable@vger.kernel.org
Assisted-by: Claude:claude-sonnet-4-6
Signed-off-by: Christian Lugnberg <christian.lugnberg@soundtrack.io>
---
 drivers/dma/sun6i-dma.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Comments

sashiko-bot@kernel.org Aug. 17, 2026, 2:07 p.m. UTC | #1 
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] The `sun6i_dma_tx_status` function reads and caches `vchan->phy` into a local `pchan` variable before acquiring the `vchan->vc.lock`. If the DMA transfer completes concurrently and the tasklet reassigns this physical channel to another virtual channel, `tx_status` uses the stale `pchan` pointer to calculate the DMA residue. This results in accessing another channel's hardware registers and descriptor lists.
--

--- Patch [1]: [PATCH v3 1/2] dmaengine: sun6i: fix non-atomic read of DMA position registers ---
commit bea51abd69d8527a2183678d78b9d1b2e28b24a1
Author: Christian Lugnberg <christian.lugnberg@soundtrack.io>

dmaengine: sun6i: fix non-atomic read of DMA position registers

sun6i_get_chan_size() reads DMA_CHAN_LLI_ADDR and DMA_CHAN_CUR_CNT in two
separate readl() calls with no synchronisation between them. The subsequent
virtual-chain walk starts one entry too early and accumulates an extra full
period's worth of bytes into the residue estimate. Fix this by re-reading
DMA_CHAN_LLI_ADDR after DMA_CHAN_CUR_CNT and retrying if the value changed.

> diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
> index a9a254dbf8cb9..eb060ba2d4738 100644
> --- a/drivers/dma/sun6i-dma.c
> +++ b/drivers/dma/sun6i-dma.c
> @@ -354,8 +354,10 @@ static size_t sun6i_get_chan_size(struct sun6i_pchan *pchan)
>  	size_t bytes;
>  	dma_addr_t pos;
>  
> -	pos = readl(pchan->base + DMA_CHAN_LLI_ADDR);
> -	bytes = readl(pchan->base + DMA_CHAN_CUR_CNT);
> +	do {
> +		pos = readl(pchan->base + DMA_CHAN_LLI_ADDR);
> +		bytes = readl(pchan->base + DMA_CHAN_CUR_CNT);
> +	} while (pos != readl(pchan->base + DMA_CHAN_LLI_ADDR));

[Severity: High]
This is a pre-existing issue, but since sun6i_get_chan_size() uses the
pchan pointer to read hardware registers, can it be called with a stale
pchan pointer from sun6i_dma_tx_status()?

In sun6i_dma_tx_status(), pchan is cached outside the lock:

drivers/dma/sun6i-dma.c:sun6i_dma_tx_status() {
	struct sun6i_pchan *pchan = vchan->phy;
	...
	spin_lock_irqsave(&vchan->vc.lock, flags);
	...
	} else {
		bytes = sun6i_get_chan_size(pchan);
	}
	...
}

If the transfer finishes and the tasklet reassigns this physical channel
to another virtual channel concurrently, sun6i_dma_tx_status() could use
the stale pchan. This would lead to calculating the residue using another
channel's hardware state and descriptor lists.

Should vchan->phy be re-read inside the lock?

>  
>  	if (pos == LLI_LAST_ITEM)
>  		return bytes;
diff mbox series

Patch

diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
index f47a326dd7ff..04fe1f5042e9 100644
--- a/drivers/dma/sun6i-dma.c
+++ b/drivers/dma/sun6i-dma.c
@@ -354,8 +354,10 @@  static size_t sun6i_get_chan_size(struct sun6i_pchan *pchan)
 	size_t bytes;
 	dma_addr_t pos;
 
-	pos = readl(pchan->base + DMA_CHAN_LLI_ADDR);
-	bytes = readl(pchan->base + DMA_CHAN_CUR_CNT);
+	do {
+		pos = readl(pchan->base + DMA_CHAN_LLI_ADDR);
+		bytes = readl(pchan->base + DMA_CHAN_CUR_CNT);
+	} while (pos != readl(pchan->base + DMA_CHAN_LLI_ADDR));
 
 	if (pos == LLI_LAST_ITEM)
 		return bytes;