Re: [PATCH v4 03/23] dmaengine: ste_dma40: Recover coalesced cyclic callbacks

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] dmaengine: ste_dma40: double-interrupt race causes full buffer spurious callbacks
- [High] dmaengine: ste_dma40: duplicate cyclic callbacks on fallback pointer sampling failure
--

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

dmaengine: ste_dma40: Recover coalesced cyclic callbacks

This commit addresses lost period notifications in the DMA40 driver caused
by coalesced cyclic callbacks. It calculates elapsed periods by comparing
the current memory-side pointer against the last queued callback position,
accounting for elapsed periods and potential full buffer laps.

> diff --git a/drivers/dma/ste_dma40.c b/drivers/dma/ste_dma40.c
> index c9983e600dafc..b992471e05f29 100644
> --- a/drivers/dma/ste_dma40.c
> +++ b/drivers/dma/ste_dma40.c

[ ... ]

> @@ -1484,6 +1488,40 @@ static bool d40_cyclic_offset(struct d40_chan *d40c, struct d40_desc *d40d,
>  	return false;
>  }
>  
> +static unsigned int d40_cyclic_periods_elapsed(struct d40_chan *d40c,
> +					       struct d40_desc *d40d)
> +{
> +	size_t current_pos;
> +	size_t offset;
> +	unsigned int periods;
> +
> +	if (!d40d->cyclic_period_len ||
> +	    !d40_cyclic_offset(d40c, d40d, &offset))
> +		return 1;

[Severity: High]
Could this fallback path lead to duplicate callbacks on the next interrupt?

If d40_cyclic_offset fails to reliably read the hardware pointer (for
example, returning false after retry failures due to concurrent register
updates), this returns 1 to legitimately report a single period completion.

However, since d40d->cyclic_callback_pos is left unmodified here, won't the
next successful interrupt recalculate the elapsed periods using the stale
cyclic_callback_pos, effectively recounting the period that was already
reported by this fallback path?

> +
> +	current_pos = rounddown(offset, d40d->cyclic_period_len);
> +	if (!d40_residue(d40c) && current_pos != offset)
> +		current_pos += d40d->cyclic_period_len;
> +	if (current_pos == d40d->cyclic_buf_len)
> +		current_pos = 0;
> +
> +	if (current_pos > d40d->cyclic_callback_pos) {
> +		periods = (current_pos - d40d->cyclic_callback_pos) /
> +			d40d->cyclic_period_len;
> +	} else if (current_pos < d40d->cyclic_callback_pos) {
> +		periods = (d40d->cyclic_buf_len -
> +			d40d->cyclic_callback_pos + current_pos) /
> +			d40d->cyclic_period_len;
> +	} else {
> +		/* At least one interrupt occurred, so assume one buffer lap. */
> +		periods = d40d->cyclic_buf_len / d40d->cyclic_period_len;
> +	}

[Severity: High]
Is it possible for this else branch to be triggered by a double-interrupt
race rather than a full buffer lap?

If a second or ghost interrupt fires immediately after the first, before the
hardware pointer has advanced to the next boundary, current_pos will exactly
equal cyclic_callback_pos. This branch appears to interpret that condition
strictly as a full buffer lap caused by CPU starvation.

Could this extremely common double-interrupt race cause a massive burst of
spurious callbacks (a full buffer's worth) to be queued to the cyclic DMA
client, potentially leading to severe state desynchronization like ALSA
audio underruns or pointer jumps?

> +
> +	d40d->cyclic_callback_pos = current_pos;
> +
> +	return periods;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260920-dma40-fixes-v4-0-d751b2d9c23f@kernel.org?part=3