[v2,1/5] dmaengine: sun6i-dma: Refactor to support A733 interrupt and register handling

Refactor to support the Allwinner A733 DMA controller. Currently, the
`sun6i-dma` driver has several functions related to interrupt handling
(reading/writing interrupt enable and status registers) and register
dumping that are hardcoded.

To support the A733, which has different register layouts and interrupt
handling logic, these functions are being moved into the
`sun6i_dma_config` structure as function pointers. This allows the
driver to use a polymorphic approach where the specific implementation
is determined by the hardware configuration assigned during device
probing.

Changes:
- Added function pointers to `struct sun6i_dma_config` for:
    - `dump_com_regs`
    - `read_irq_en`
    - `write_irq_en`
    - `read_irq_stat`
    - `write_irq_stat`
- Implemented generic `sun6i_read/write_irq_*` functions for existing
  hardware.
- Added a macro and updated existing `sun6i_dma_config` instances (A31,
  A23, H3, A64, A100, H6, V3S) to use these new function pointers.

Signed-off-by: Yuanshen Cao <alex.caoys@gmail.com>
---
 drivers/dma/sun6i-dma.c | 50 ++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 45 insertions(+), 5 deletions(-)
  

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.
  

On Sun, Jun 21, 2026 at 09:40:54PM +0000, Yuanshen Cao wrote:
> Refactor to support the Allwinner A733 DMA controller. Currently, the
> `sun6i-dma` driver has several functions related to interrupt handling
> (reading/writing interrupt enable and status registers) and register
> dumping that are hardcoded.
>
> To support the A733, which has different register layouts and interrupt
> handling logic, these functions are being moved into the
> `sun6i_dma_config` structure as function pointers. This allows the
> driver to use a polymorphic approach where the specific implementation
> is determined by the hardware configuration assigned during device
> probing.
>
> Changes:
> - Added function pointers to `struct sun6i_dma_config` for:
>     - `dump_com_regs`
>     - `read_irq_en`
>     - `write_irq_en`
>     - `read_irq_stat`
>     - `write_irq_stat`
> - Implemented generic `sun6i_read/write_irq_*` functions for existing
>   hardware.
> - Added a macro and updated existing `sun6i_dma_config` instances (A31,
>   A23, H3, A64, A100, H6, V3S) to use these new function pointers.
>
> Signed-off-by: Yuanshen Cao <alex.caoys@gmail.com>
> ---

Reviewed-by: Frank Li <Frank.Li@nxp.com>

>  drivers/dma/sun6i-dma.c | 50 ++++++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 45 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
> index a9a254dbf8cb..ef3052c4ab36 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 irq_reg);
> +	void (*write_irq_en)(struct sun6i_dma_dev *sdev, u32 irq_reg, u32 irq_val);
> +	u32 (*read_irq_stat)(struct sun6i_dma_dev *sdev, u32 irq_reg);
> +	void (*write_irq_stat)(struct sun6i_dma_dev *sdev, u32 irq_reg, u32 status);
>  	u32 src_burst_lengths;
>  	u32 dst_burst_lengths;
>  	u32 src_addr_widths;
> @@ -347,6 +352,26 @@ static void sun6i_set_mode_h6(u32 *p_cfg, s8 src_mode, s8 dst_mode)
>  		  DMA_CHAN_CFG_DST_MODE_H6(dst_mode);
>  }
>
> +static u32 sun6i_read_irq_en(struct sun6i_dma_dev *sdev, u32 irq_reg)
> +{
> +	return readl(sdev->base + DMA_IRQ_EN(irq_reg));
> +}
> +
> +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));
> +}
> +
> +static u32 sun6i_read_irq_stat(struct sun6i_dma_dev *sdev, u32 irq_reg)
> +{
> +	return readl(sdev->base + DMA_IRQ_STAT(irq_reg));
> +}
> +
> +static void sun6i_write_irq_stat(struct sun6i_dma_dev *sdev, u32 irq_reg, u32 status)
> +{
> +	writel(status, sdev->base + DMA_IRQ_STAT(irq_reg));
> +}
> +
>  static size_t sun6i_get_chan_size(struct sun6i_pchan *pchan)
>  {
>  	struct sun6i_desc *txd = pchan->desc;
> @@ -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);
>
>  	writel(pchan->desc->p_lli, pchan->base + DMA_CHAN_LLI_ADDR);
>  	writel(DMA_CHAN_ENABLE_START, pchan->base + DMA_CHAN_ENABLE);
>
> -	sun6i_dma_dump_com_regs(sdev);
> +	sdev->cfg->dump_com_regs(sdev);
>  	sun6i_dma_dump_chan_regs(sdev, pchan);
>
>  	return 0;
> @@ -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);
>  		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;
> @@ -1101,6 +1126,13 @@ static inline void sun6i_dma_free(struct sun6i_dma_dev *sdev)
>  	}
>  }
>
> +#define SUN6I_DMA_IRQ_A31_COMMON_OPS	\
> +	.dump_com_regs    = sun6i_dma_dump_com_regs,	\
> +	.read_irq_en      = sun6i_read_irq_en,	\
> +	.write_irq_en     = sun6i_write_irq_en,	\
> +	.read_irq_stat    = sun6i_read_irq_stat,	\
> +	.write_irq_stat   = sun6i_write_irq_stat,
> +
>  /*
>   * For A31:
>   *
> @@ -1132,6 +1164,7 @@ static struct sun6i_dma_config sun6i_a31_dma_cfg = {
>  	.dst_addr_widths   = BIT(DMA_SLAVE_BUSWIDTH_1_BYTE) |
>  			     BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) |
>  			     BIT(DMA_SLAVE_BUSWIDTH_4_BYTES),
> +	SUN6I_DMA_IRQ_A31_COMMON_OPS
>  };
>
>  /*
> @@ -1155,6 +1188,7 @@ static struct sun6i_dma_config sun8i_a23_dma_cfg = {
>  	.dst_addr_widths   = BIT(DMA_SLAVE_BUSWIDTH_1_BYTE) |
>  			     BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) |
>  			     BIT(DMA_SLAVE_BUSWIDTH_4_BYTES),
> +	SUN6I_DMA_IRQ_A31_COMMON_OPS
>  };
>
>  static struct sun6i_dma_config sun8i_a83t_dma_cfg = {
> @@ -1173,6 +1207,7 @@ static struct sun6i_dma_config sun8i_a83t_dma_cfg = {
>  	.dst_addr_widths   = BIT(DMA_SLAVE_BUSWIDTH_1_BYTE) |
>  			     BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) |
>  			     BIT(DMA_SLAVE_BUSWIDTH_4_BYTES),
> +	SUN6I_DMA_IRQ_A31_COMMON_OPS
>  };
>
>  /*
> @@ -1200,6 +1235,7 @@ static struct sun6i_dma_config sun8i_h3_dma_cfg = {
>  			     BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) |
>  			     BIT(DMA_SLAVE_BUSWIDTH_4_BYTES) |
>  			     BIT(DMA_SLAVE_BUSWIDTH_8_BYTES),
> +	SUN6I_DMA_IRQ_A31_COMMON_OPS
>  };
>
>  /*
> @@ -1221,6 +1257,7 @@ static struct sun6i_dma_config sun50i_a64_dma_cfg = {
>  			     BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) |
>  			     BIT(DMA_SLAVE_BUSWIDTH_4_BYTES) |
>  			     BIT(DMA_SLAVE_BUSWIDTH_8_BYTES),
> +	SUN6I_DMA_IRQ_A31_COMMON_OPS
>  };
>
>  /*
> @@ -1244,6 +1281,7 @@ static struct sun6i_dma_config sun50i_a100_dma_cfg = {
>  			     BIT(DMA_SLAVE_BUSWIDTH_8_BYTES),
>  	.has_high_addr = true,
>  	.has_mbus_clk = true,
> +	SUN6I_DMA_IRQ_A31_COMMON_OPS
>  };
>
>  /*
> @@ -1266,6 +1304,7 @@ static struct sun6i_dma_config sun50i_h6_dma_cfg = {
>  			     BIT(DMA_SLAVE_BUSWIDTH_4_BYTES) |
>  			     BIT(DMA_SLAVE_BUSWIDTH_8_BYTES),
>  	.has_mbus_clk = true,
> +	SUN6I_DMA_IRQ_A31_COMMON_OPS
>  };
>
>  /*
> @@ -1289,6 +1328,7 @@ static struct sun6i_dma_config sun8i_v3s_dma_cfg = {
>  	.dst_addr_widths   = BIT(DMA_SLAVE_BUSWIDTH_1_BYTE) |
>  			     BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) |
>  			     BIT(DMA_SLAVE_BUSWIDTH_4_BYTES),
> +	SUN6I_DMA_IRQ_A31_COMMON_OPS
>  };
>
>  static const struct of_device_id sun6i_dma_match[] = {
>
> --
> 2.54.0
>