Re: [PATCH v3 1/5] dmaengine: sun6i-dma: Refactor to support A733 interrupt and register handling

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Integer division truncates the channel check in the interrupt handler, causing interrupts for channels >= 8 to be completely ignored on SoCs where the number of channels is not a multiple of 8.
- [High] Incorrect calculation of the physical channel pointer in the interrupt handler aliases upper hardware channels to lower software channels, causing Use-After-Free and data corruption.
- [High] A Time-of-Check to Time-of-Use (TOCTOU) race condition in `sun6i_kill_tasklet` allows a concurrent tasklet to re-enable interrupts after they were intentionally disabled, causing an unhandled interrupt storm after the IRQ is freed.
--

commit 4e47cdc0e212cbdd5c17510fa923b572eb14fb54
Author: Yuanshen Cao <alex.caoys@gmail.com>

dmaengine: sun6i-dma: Refactor to support A733 interrupt and register handling

Refactor to support the Allwinner A733 DMA controller by moving interrupt
handling and register dumping functions into the sun6i_dma_config structure.
This allows a polymorphic approach determined by hardware configuration
during device probing.

> diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
> index a9a254dbf8cb9..ef3052c4ab363 100644
> --- a/drivers/dma/sun6i-dma.c
> +++ b/drivers/dma/sun6i-dma.c
> @@ -347,6 +352,26 @@ static void sun6i_set_mode_h6(u32 *p_cfg, s8 src_mode, s8 dst_mode)
[ ... ]
> +static void sun6i_write_irq_en(struct sun6i_dma_dev *sdev, u32 irq_reg, u32 irq_val)
> +{
> +	writel(irq_val, sdev->base + DMA_IRQ_EN(irq_reg));
> +}

[Severity: High]
This is a pre-existing issue, but does disabling interrupts leave a race
condition?

In sun6i_kill_tasklet(), hardware interrupts are disabled via write_irq_en(0)
before tasklet_kill() is called:

sun6i_kill_tasklet()
    for (i = 0; i < ...; i++)
        sdev->cfg->write_irq_en(sdev, i, 0);
    atomic_inc(&sdev->tasklet_shutdown);
    devm_free_irq(sdev->slave.dev, sdev->irq, sdev);
    tasklet_kill(&sdev->task);

If sun6i_dma_tasklet() runs concurrently and calls sun6i_dma_start_desc(),
could it perform a read-modify-write on the IRQ_EN register without a lock?

This could re-enable the interrupt after sun6i_kill_tasklet() explicitly
disabled it. When devm_free_irq() is called, an active DMA channel might
then assert an unhandled IRQ, leading to an interrupt storm.

[ ... ]
> @@ -549,14 +574,14 @@ static irqreturn_t sun6i_dma_interrupt(int irq, void *dev_id)
>  	u32 status;
>  
>  	for (i = 0; i < sdev->num_pchans / DMA_IRQ_CHAN_NR; i++) {

[Severity: High]
This is a pre-existing issue, but does integer division truncate the channel
check here?

If num_pchans is not a multiple of 8 (like 12 on H3), dividing by 8 yields 1.
This would mean the loop only checks channels 0-7 and ignores the second
interrupt register for channels 8-11.

Could this cause unacknowledged level-triggered interrupts from those upper
channels, leading to an interrupt storm?

> -		status = readl(sdev->base + DMA_IRQ_STAT(i));
> +		status = sdev->cfg->read_irq_stat(sdev, i);
>  		if (!status)
>  			continue;
>  
>  		dev_dbg(sdev->slave.dev, "DMA irq status %s: 0x%x\n",
>  			str_high_low(i), status);
>  
> -		writel(status, sdev->base + DMA_IRQ_STAT(i));
> +		sdev->cfg->write_irq_stat(sdev, i, status);
>  
>  		for (j = 0; (j < DMA_IRQ_CHAN_NR) && status; j++) {
>  			pchan = sdev->pchans + j;

[Severity: High]
This isn't a bug introduced by this patch, but does the physical channel
pointer calculation alias upper hardware channels to lower software channels?

The calculation sdev->pchans + j ignores the register index i.

For upper channels (where i = 1, meaning channels 8-15), would the handler
map the active hardware interrupt back to software channels 0-7?

It seems this might cause the driver to process and complete descriptors for
the wrong channels while the hardware is still writing to them, potentially
causing data corruption and use-after-free conditions.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260622-sun60i-a733-dma-v3-0-f697ef296cbc@gmail.com?part=1