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

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 <[email protected]>

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;

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