diff mbox series

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

Message ID 20260826-sun60i-a733-dma-v5-3-abc5229b441e@gmail.com (mailing list archive)
State New
Headers
Series dmaengine: sun6i-dma: Add support for Allwinner A733 DMA controller |

Commit Message

Yuanshen Cao Aug. 26, 2026, 2:40 a.m. UTC 
The `sun6i-dma` driver previously assumed a fixed number of channels
per interrupt register. For example, `sun6i_kill_tasklet` was hardcoded
to disable interrupts only for registers 0 and 1. `DMA_MAX_CHANNELS` was
also not in used previously, and the old SoCs never has more than 16
channels.

The A733 has a different interrupt structure where the number of
channels per register may differ. Add `num_channels_per_reg` to `struct
sun6i_dma_config` to make the interrupt handling logic handware-agnostic
Update `sun6i_dma_interrupt`, `sun6i_dma_start_desc`, and
`sun6i_kill_tasklet` to use this value.

Additionally, set `DMA_MAX_CHANNELS` to 16 to ensure loops over
interrupts are correctly bounded, aligning with the hardware
specifications.

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

Comments

sashiko-bot@kernel.org Aug. 26, 2026, 2:53 a.m. UTC | #1 
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Integer division truncates the interrupt register loop bound, permanently ignoring interrupts for channels that do not divide evenly into num_channels_per_reg.
- [High] Incorrect array indexing for physical channels misroutes interrupts to the wrong DMA channels, breaking functionality and A733 support.
--

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

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

This commit updates the sun6i-dma driver to use a flexible 
num_channels_per_reg configuration rather than a hardcoded value. This makes 
the interrupt handling hardware-agnostic, enabling support for the A733 SoC 
which has a different interrupt structure.

> diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
> index 1ffd870d95942..ffb63212bea71 100644
> --- a/drivers/dma/sun6i-dma.c
> +++ b/drivers/dma/sun6i-dma.c

[ ... ]

> @@ -582,7 +584,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 isn't a bug introduced by this patch, but it appears to be a pre-existing
issue where integer division truncates the interrupt register loop bound. On
hardware where the number of physical channels is not an exact multiple of
num_channels_per_reg, will this loop prematurely exit and abandon unaligned
physical channels?

For example, if a SoC has 12 physical channels and num_channels_per_reg is 8,
the bound evaluates to 12 / 8 = 1, meaning the second interrupt register is
never checked. Should this be using DIV_ROUND_UP(sdev->num_pchans,
sdev->cfg->num_channels_per_reg) to match how it is handled in
sun6i_dma_dump_com_regs()?

>  		status = sun6i_read_irq_stat(sdev, i);
>  		if (!status)
>  			continue;
> @@ -592,7 +594,7 @@ static irqreturn_t sun6i_dma_interrupt(int irq, void *dev_id)
>  
>  		sun6i_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: High]
This is also a pre-existing issue, but the incorrect array indexing here
becomes particularly critical with the newly introduced A733 support. Does
this array access ignore the outer register index i, aliasing all
higher-channel interrupts to the first register's channels?

Since the newly supported A733 SoC has num_channels_per_reg = 1, j will
always evaluate to 0. Will this catastrophically misroute all interrupts
across all channels to channel 0?

It looks like the correct calculation needs to include the register offset,
such as: pchan = sdev->pchans + (i * sdev->cfg->num_channels_per_reg) + j;

Note that this might also require an explicit array bounds check against
sdev->num_pchans to prevent out-of-bounds access for trailing unaligned
channels.
diff mbox series

Patch

diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
index 1ffd870d9594..ffb63212bea7 100644
--- a/drivers/dma/sun6i-dma.c
+++ b/drivers/dma/sun6i-dma.c
@@ -32,14 +32,13 @@ 
 #define DMA_IRQ_PKG			BIT(1)
 #define DMA_IRQ_QUEUE			BIT(2)
 
-#define DMA_IRQ_CHAN_NR			8
 #define DMA_IRQ_CHAN_WIDTH		4
 
 
 #define DMA_STAT		0x30
 
 /* Offset between DMA_IRQ_EN and DMA_IRQ_STAT limits number of channels */
-#define DMA_MAX_CHANNELS	(DMA_IRQ_CHAN_NR * 0x10 / 4)
+#define DMA_MAX_CHANNELS	16
 
 /*
  * sun8i specific registers
@@ -58,6 +57,8 @@ 
 #define DMA_IRQ_EN_OFFSET_A31		0x00
 #define DMA_IRQ_STAT_OFFSET_A31		0x10
 
+#define DMA_IRQ_CHAN_NR_A31		8
+
 /*
  * Channels specific registers
  */
@@ -154,6 +155,7 @@  struct sun6i_dma_config {
 	u32 irq_stride;
 	u32 irq_en_offset;
 	u32 irq_stat_offset;
+	u32 num_channels_per_reg;
 };
 
 /*
@@ -268,7 +270,7 @@  static inline void sun6i_dma_dump_com_regs(struct sun6i_dma_dev *sdev)
 {
 	int i;
 
-	for (i = 0; i < 2; i++) {
+	for (i = 0; i < DIV_ROUND_UP(sdev->num_pchans, sdev->cfg->num_channels_per_reg); i++) {
 		dev_dbg(sdev->slave.dev, "Common register:\n"
 			"chan num %d\n"
 			"\tmask(%04x): 0x%08x\n"
@@ -489,8 +491,8 @@  static int sun6i_dma_start_desc(struct sun6i_vchan *vchan)
 
 	sun6i_dma_dump_lli(vchan, pchan->desc->v_lli, pchan->desc->p_lli);
 
-	irq_reg = pchan->idx / DMA_IRQ_CHAN_NR;
-	irq_offset = pchan->idx % DMA_IRQ_CHAN_NR;
+	irq_reg = pchan->idx / sdev->cfg->num_channels_per_reg;
+	irq_offset = pchan->idx % sdev->cfg->num_channels_per_reg;
 
 	vchan->irq_type = vchan->cyclic ? DMA_IRQ_PKG : DMA_IRQ_QUEUE;
 
@@ -582,7 +584,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++) {
 		status = sun6i_read_irq_stat(sdev, i);
 		if (!status)
 			continue;
@@ -592,7 +594,7 @@  static irqreturn_t sun6i_dma_interrupt(int irq, void *dev_id)
 
 		sun6i_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;
 			vchan = pchan->vchan;
 			if (vchan && (status & vchan->irq_type)) {
@@ -1110,7 +1112,7 @@  static inline void sun6i_kill_tasklet(struct sun6i_dma_dev *sdev)
 	int i;
 
 	/* Disable all interrupts from DMA */
-	for (i = 0; i < 2; i++)
+	for (i = 0; i < DMA_MAX_CHANNELS / sdev->cfg->num_channels_per_reg; i++)
 		sun6i_write_irq_en(sdev, i, 0);
 
 	/* Prevent spurious interrupts from scheduling the tasklet */
@@ -1138,7 +1140,8 @@  static inline void sun6i_dma_free(struct sun6i_dma_dev *sdev)
 #define SUN6I_DMA_IRQ_A31_COMMON_CFG	\
 	.irq_stride      = DMA_IRQ_STRIDE_A31,	\
 	.irq_en_offset   = DMA_IRQ_EN_OFFSET_A31,	\
-	.irq_stat_offset = DMA_IRQ_STAT_OFFSET_A31,
+	.irq_stat_offset = DMA_IRQ_STAT_OFFSET_A31,	\
+	.num_channels_per_reg = DMA_IRQ_CHAN_NR_A31,
 
 /*
  * For A31: