Re: [PATCH 3/5] dmaengine: sun6i-dma: Add num_channels_per_reg for flexible interrupt mapping

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Incorrect physical channel indexing in `sun6i_dma_interrupt` ignores the register index, applying status to the wrong physical channel.
- [High] Integer division truncation in the `sun6i_dma_interrupt` loop bound skips polling for trailing channels.
- [High] Time-of-Check to Time-of-Use (TOCTOU) race condition in `sun6i_dma_interrupt` risks a NULL pointer dereference.
--

commit 3bfcb64969bffa1792e80eaa282fc15450f3870b
Author: Yuanshen Cao <alex.caoys@gmail.com>

dmaengine: sun6i-dma: Add num_channels_per_reg for flexible interrupt mapping

This commit introduces num_channels_per_reg to the sun6i_dma_config to
handle SoCs with varying channels per interrupt register. It modifies
the interrupt logic to use this new configuration value and bounds checking.

> diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
> index 059455425e19c..fb1c1a28744bb 100644
> --- a/drivers/dma/sun6i-dma.c
> +++ b/drivers/dma/sun6i-dma.c

[ ... ]

> @@ -574,7 +575,7 @@ static irqreturn_t sun6i_dma_interrupt(int irq, void *dev_id)
>  	int i, j, ret = IRQ_NONE;
>  	u32 status;
>  
> -	for (i = 0; i < sdev->num_pchans / DMA_IRQ_CHAN_NR; i++) {
> +	for (i = 0; i < sdev->num_pchans / sdev->cfg->num_channels_per_reg; i++) {

[Severity: High]
This is a pre-existing issue, but does this loop condition truncate the
division and miss trailing channels?

If sdev->num_pchans is not perfectly divisible by num_channels_per_reg
(for example, 12 channels with 8 channels per register), the integer
division truncates the remainder. This means the loop will skip the second
register entirely, permanently missing interrupts for the trailing channels.

>  		status = sdev->cfg->read_irq_stat(sdev, i);
>  		if (!status)
>  			continue;
> @@ -584,7 +585,7 @@ static irqreturn_t sun6i_dma_interrupt(int irq, void *dev_id)
>  
>  		sdev->cfg->write_irq_stat(sdev, i, status);
>  
> -		for (j = 0; (j < DMA_IRQ_CHAN_NR) && status; j++) {
> +		for (j = 0; (j < sdev->cfg->num_channels_per_reg) && status; j++) {
>  			pchan = sdev->pchans + j;

[Severity: Critical]
This is a pre-existing issue, but does this apply the status to the wrong
physical channel?

The array lookup uses only the local offset j, discarding the register
index i. For any register i > 0, it accesses sdev->pchans[j] instead of
sdev->pchans[i * num_channels_per_reg + j].

With the newly added hardware where num_channels_per_reg is 1, j is always
0, which seems like it would cause all channels to incorrectly map to
channel 0.

>  			vchan = pchan->vchan;
>  			if (vchan && (status & vchan->irq_type)) {

[Severity: High]
This is a pre-existing issue, but is there a race condition here that could
lead to a NULL pointer dereference?

The interrupt handler reads vchan = pchan->vchan locklessly, and eventually
blocks to acquire vchan->vc.lock. Concurrently, sun6i_dma_terminate_all()
could acquire the same lock, nullify pchan->desc, and free the descriptors.

When the interrupt handler successfully acquires the lock:

spin_lock(&vchan->vc.lock);
vchan_cookie_complete(&pchan->desc->vd);

it does not re-verify pchan->desc before passing &pchan->desc->vd to
vchan_cookie_complete(), which would crash if pchan->desc was concurrently
set to NULL.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260619-sun60i-a733-dma-v1-0-da4b649fc72a@gmail.com?part=3