Re: [PATCH v3 02/22] dmaengine: ste_dma40: Fix cyclic transfer residue

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] dmaengine: ste_dma40: unvalidated cyclic period length causes state corruption
- [Medium] dmaengine: ste_dma40: race condition in cyclic residue calculation
--

commit 80bd7a5b773ac08dae8cfb0539ed88005fe2df44
Author: Linus Walleij <linusw@kernel.org>

dmaengine: ste_dma40: Fix cyclic transfer residue

DMA40 reads residue from the element count of the currently active LLI.
For a cyclic transfer this reports at most one period, not the bytes
remaining until the cyclic buffer wraps. This patch tracks the active
period of cyclic descriptors and adds all later periods in the buffer to
the current LLI residue.

> diff --git a/drivers/dma/ste_dma40.c b/drivers/dma/ste_dma40.c
> --- a/drivers/dma/ste_dma40.c
> +++ b/drivers/dma/ste_dma40.c
[ ... ]
> @@ -2108,15 +2120,27 @@ static bool d40_is_paused(struct d40_chan *d40c)
>  
>  }
>  
> -static u32 stedma40_residue(struct dma_chan *chan)
> +static u32 stedma40_residue(struct dma_chan *chan, dma_cookie_t cookie)
>  {
>  	struct d40_chan *d40c =
>  		container_of(chan, struct d40_chan, chan);
> +	struct d40_desc *d40d;
>  	u32 bytes_left;
>  	unsigned long flags;
>  
>  	spin_lock_irqsave(&d40c->lock, flags);
>  	bytes_left = d40_residue(d40c);
> +
> +	d40d = d40_first_active_get(d40c);
> +	/*
> +	 * The hardware residue is for the current LLI. Cyclic transfers use
> +	 * one LLI for each period, so include the later periods in the buffer.
> +	 */
> +	if (d40d && d40d->txd.cookie == cookie && d40d->cyclic &&
> +	    d40d->cyclic_period_len)
> +		bytes_left += d40d->cyclic_buf_len - d40d->cyclic_pos -
> +			      d40d->cyclic_period_len;

[Severity: Medium]
Can this create a race condition when a client asynchronously polls the DMA
residue?

If the hardware has just completed a period and advanced its internal LLI,
but the terminal-count interrupt hasn't updated the software cyclic_pos yet:

dma_tc_handle() {
	...
	if (d40d->cyclic_period_len) {
		d40d->cyclic_pos += d40d->cyclic_period_len;
	...
}

Could stedma40_residue() read the current hardware residue without
synchronizing with the interrupt handler's update, causing the residue (and
pointers for clients like ALSA) to occasionally jump backward and cause audio
glitches?

[ ... ]
> @@ -2524,11 +2548,17 @@ dma40_prep_dma_cyclic(struct dma_chan *chan, dma_addr_t dma_addr,
>  		     size_t buf_len, size_t period_len,
>  		     enum dma_transfer_direction direction, unsigned long flags)
>  {
> -	unsigned int periods = buf_len / period_len;
>  	struct dma_async_tx_descriptor *txd;
> +	struct d40_desc *desc;
>  	struct scatterlist *sg;
>  	int i;
>  
> +	if (!buf_len || !period_len || buf_len % period_len)
> +		return NULL;
> +
> +	periods = buf_len / period_len;

[Severity: High]
Does this also need to validate that period_len is within the hardware's
maximum segment size?

If a cyclic period_len exceeds the maximum segment size (for example,
STEDMA40_MAX_SEG_SIZE * data_width), it seems it would be silently split
into multiple LLIs. For logical channels, every LLI unconditionally
triggers a terminal-count interrupt.

If that happens, wouldn't dma_tc_handle() erroneously advance cyclic_pos
by the full period_len on every LLI interrupt?

dma_tc_handle() {
	...
	if (d40d->cyclic_period_len) {
		d40d->cyclic_pos += d40d->cyclic_period_len;
	...
}

This seems like it would permanently drift the residue tracking in
stedma40_residue() and trigger excessive callbacks if a client requests a
large cyclic buffer.

>  
>  	sg = kzalloc_objs(struct scatterlist, periods + 1, GFP_NOWAIT);
>  	if (!sg)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260918-dma40-fixes-v3-0-8dd8450669e8@kernel.org?part=2