Re: [PATCH v2 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 4 potential issue(s) to consider:

Pre-existing issues:
- [High] The interrupt handler incorrectly handles SoCs with more than 8 channels due to loop truncation and incorrect physical channel index calculations, causing missed interrupts and state confusion.
- [High] The driver disables hardware clocks and asserts reset before killing the virtual channel tasklets, which can lead to a bus fault via Use-After-Free hardware access.
- [High] Potential deadlock in `sun6i_dma_terminate_all` due to acquiring `sdev->lock` with `spin_lock()` in process context while the lock is also used in softirq context.
- [High] A race condition between `sun6i_kill_tasklet` and `sun6i_dma_start_desc` can inadvertently re-enable hardware interrupts right before the IRQ handler is freed.
--

commit f22370211098277a88244ad75b31ec31efb6ead0
Author: Yuanshen Cao <alex.caoys@gmail.com>
Subject: dmaengine: sun6i-dma: Refactor to support A733 interrupt and register handling

This commit refactors the sun6i-dma driver to support the Allwinner A733 DMA
controller. It abstracts hardcoded interrupt handling and register dumping
into function pointers within the sun6i_dma_config structure. This allows
a polymorphic approach determined by the hardware configuration.

> 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
[ ... ]
> @@ -460,16 +485,16 @@ static int sun6i_dma_start_desc(struct sun6i_vchan *vchan)
>  
>  	vchan->irq_type = vchan->cyclic ? DMA_IRQ_PKG : DMA_IRQ_QUEUE;
>  
> -	irq_val = readl(sdev->base + DMA_IRQ_EN(irq_reg));
> +	irq_val = sdev->cfg->read_irq_en(sdev, irq_reg);
>  	irq_val &= ~((DMA_IRQ_HALF | DMA_IRQ_PKG | DMA_IRQ_QUEUE) <<
>  			(irq_offset * DMA_IRQ_CHAN_WIDTH));
>  	irq_val |= vchan->irq_type << (irq_offset * DMA_IRQ_CHAN_WIDTH);
> -	writel(irq_val, sdev->base + DMA_IRQ_EN(irq_reg));
> +	sdev->cfg->write_irq_en(sdev, irq_reg, irq_val);

[Severity: High]
This isn't a bug introduced by this patch, but is there a race condition
between tasklet teardown and interrupt enabling?

During driver teardown, sun6i_kill_tasklet() writes 0 to the hardware
registers without locks to serialize against the tasklet:

sun6i_kill_tasklet() {
    for (i = 0; i < DMA_MAX_CHANNELS / sdev->cfg->num_channels_per_reg; i++)
        sdev->cfg->write_irq_en(sdev, i, 0);
...

If the tasklet concurrently executes sun6i_dma_start_desc(), it performs a
read-modify-write on the same register here.

If the tasklet reads the register just before sun6i_kill_tasklet() writes 0,
and then writes back its modified value here, could it overwrite the 0 and
re-enable the hardware interrupt right before devm_free_irq() removes the
handler, leading to an unhandled interrupt storm?

>  
>  	writel(pchan->desc->p_lli, pchan->base + DMA_CHAN_LLI_ADDR);
>  	writel(DMA_CHAN_ENABLE_START, pchan->base + DMA_CHAN_ENABLE);
[ ... ]
> @@ -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++) {
> -		status = readl(sdev->base + DMA_IRQ_STAT(i));
> +		status = sdev->cfg->read_irq_stat(sdev, i);

[Severity: High]
This is a pre-existing issue, but does this interrupt handler correctly handle
SoCs with more than 8 channels?

For SoCs with num_pchans > 8 (such as H3 with 12 channels), the outer loop
uses integer division.

For 12 channels, 12 / 8 evaluates to 1, causing the handler to ignore the
second interrupt register and miss all interrupts for channels 8-11.

Additionally, in the inner loop:

for (j = 0; (j < DMA_IRQ_CHAN_NR) && status; j++) {
        pchan = sdev->pchans + j;

This calculates the physical channel pointer without accounting for the outer
loop index i. If an interrupt occurs on channel 8, it maps to physical
channel 0 instead of 8, which clears the wrong completion state and leaks
channel 8's descriptor.

>  		if (!status)
>  			continue;
[ ... ]


[Severity: High]
This is a pre-existing issue in the driver, but is there a potential deadlock
in sun6i_dma_terminate_all() regarding sdev->lock?

sun6i_dma_terminate_all() acquires the lock in process context without
disabling interrupts:

sun6i_dma_terminate_all() {
    spin_lock(&sdev->lock);
    list_del_init(&vchan->node);
    spin_unlock(&sdev->lock);

If a softirq fires on the same CPU while this lock is held, it will execute
sun6i_dma_tasklet(), which attempts to acquire the exact same lock using
spin_lock_irq(&sdev->lock). This would cause the tasklet to spin forever.
Should sun6i_dma_terminate_all() use spin_lock_bh() or spin_lock_irqsave()
instead?


[Severity: High]
This is a pre-existing issue, but does the teardown sequence in
sun6i_dma_remove() safely handle tasklets?

The driver disables hardware clocks and asserts reset before calling
sun6i_dma_free(), which kills the virtual channel tasklets:

sun6i_dma_remove() {
    clk_disable_unprepare(sdc->clk);
    reset_control_assert(sdc->rstc);

    sun6i_dma_free(sdc);

If a tasklet is scheduled by an interrupt just before removal, could it run
concurrently or immediately after the hardware is powered down? If the client
callback within the tasklet calls a DMA engine API, it will attempt to access
DMA hardware registers, which could trigger a bus fault since the clocks are
disabled and reset is asserted.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260621-sun60i-a733-dma-v2-0-340f205891cc@gmail.com?part=1