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

New issues:
- [Low] The parameter `chan_num` in the newly introduced `read_irq_en` and `write_irq_en` callbacks is highly misleading because it expects a register index, not a channel number.

Pre-existing issues:
- [Critical] `sun6i_dma_interrupt` incorrectly maps interrupts for higher channels to the wrong physical channel structs because it ignores the outer register index.
- [High] Integer division truncation in the `sun6i_dma_interrupt` loop bound completely ignores interrupts for higher channels on SoCs where the channel count is not a multiple of the channels per register.
- [High] `sun6i_dma_terminate_all` and `sun6i_dma_pause`/`resume` acquire `sdev->lock` without disabling softirqs, creating a deadlock with the DMA tasklet.
- [High] `sun6i_dma_interrupt` accesses `vchan->cyclic` and dereferences `pchan->desc` without holding `vc.lock`, racing with channel teardown.
--

commit 47f7d45b0543fc62a4eb37d06b46eeffe4dd06c6
Author: Yuanshen Cao <alex.caoys@gmail.com>

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

This patch refactors the sun6i-dma driver to support the Allwinner A733 DMA
controller. It moves interrupt handling and register dumping functions into
the sun6i_dma_config structure to handle different register layouts.

> diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
> index a9a254dbf8cb9..d92e702320d9f 100644
> --- a/drivers/dma/sun6i-dma.c
> +++ b/drivers/dma/sun6i-dma.c
> @@ -138,6 +138,11 @@ struct sun6i_dma_config {
>  	void (*set_burst_length)(u32 *p_cfg, s8 src_burst, s8 dst_burst);
>  	void (*set_drq)(u32 *p_cfg, s8 src_drq, s8 dst_drq);
>  	void (*set_mode)(u32 *p_cfg, s8 src_mode, s8 dst_mode);
> +	void (*dump_com_regs)(struct sun6i_dma_dev *sdev);
> +	u32 (*read_irq_en)(struct sun6i_dma_dev *sdev, u32 chan_num);
> +	void (*write_irq_en)(struct sun6i_dma_dev *sdev, u32 chan_num, u32 irq_val);

[Severity: Low]
Is the chan_num parameter name misleading here?

Looking at sun6i_dma_start_desc(), the value passed to these callbacks is
actually the register index (irq_reg), not the physical channel number.

Could this cause confusion for future implementers who might try to use it
as a literal channel index?

[ ... ]

> @@ -549,14 +573,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 this loop bound properly handle
platforms where the channel count is not a multiple of the channels per
register?

Because integer division truncates, a platform like sun8i-h3 (which defines
nr_max_channels = 12) will evaluate 12 / 8 as 1.

Does this mean the loop only runs for i = 0 (handling channels 0-7) and
completely drops interrupts for channels 8-11, potentially leading to an
unhandled hardware interrupt storm? Should this use DIV_ROUND_UP?

> -		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: Critical]
This is a pre-existing issue, but does this correctly calculate the physical
channel pointer for higher channels?

Inside this interrupt loop, i represents the interrupt register index and j
is the local bit offset within that register. By calculating the physical
channel as sdev->pchans + j, it completely ignores the outer register index
offset.

If an interrupt triggers on physical channel 8 (i=1, j=0), won't the code
incorrectly assign it to sdev->pchans + 0?

Could this incorrectly complete the active transfer for channel 0, causing
use-after-free issues and DMA state corruption? Should the assignment be
sdev->pchans + j + i * sdev->cfg->num_channels_per_reg?


[Severity: High]
This is a pre-existing issue, but does sun6i_dma_interrupt() have a race
condition with channel teardown?

In the hardirq handler sun6i_dma_interrupt(), vchan->cyclic is checked
locklessly:

drivers/dma/sun6i-dma.c:sun6i_dma_interrupt() {
	...
	if (vchan->cyclic) {
		vchan_cyclic_callback(&pchan->desc->vd);
	}
	...
}

Concurrently, sun6i_dma_terminate_all() can run on another CPU, acquire
vchan->vc.lock, set vchan->cyclic = false, set pchan->desc = NULL, and free
the descriptors:

drivers/dma/sun6i-dma.c:sun6i_dma_terminate_all() {
	...
	vchan->cyclic = false;
	...
}

If the IRQ handler checks vchan->cyclic just before it is modified, could it
proceed to dereference pchan->desc after it has been nulled or freed?


[Severity: High]
This is a pre-existing issue, but do sun6i_dma_terminate_all(),
sun6i_dma_pause(), and sun6i_dma_resume() have a deadlock window?

These functions acquire sdev->lock using a plain spin_lock() from process
context:

drivers/dma/sun6i-dma.c:sun6i_dma_terminate_all() {
	...
	spin_lock(&sdev->lock);
	...
}

However, sdev->lock is also acquired by sun6i_dma_tasklet(), which runs in
softirq context.

If a hard interrupt occurs on the same CPU while this lock is held, and the
interrupt handler returns and processes pending softirqs (including the
scheduled sun6i_dma_tasklet()), won't the tasklet attempt to acquire
sdev->lock using spin_lock_irq(), resulting in a same-CPU ABBA deadlock?

Should process context use spin_lock_bh() or spin_lock_irqsave() when locking
resources shared with softirqs?

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