diff mbox series

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

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

Commit Message

Yuanshen Cao June 19, 2026, 4:53 a.m. UTC 
The previous implementation of `sun6i-dma` had some implicit assumptions
about the number of channels per interrupt register. Specifically,
functions like `sun6i_kill_tasklet` were hardcoded to only disable
interrupts for IRQ 0 and 1. `DMA_MAX_CHANNELS` is also not in used in
the past, 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. This patch introduces
`num_channels_per_reg` to the `sun6i_dma_config`, similar to BSP, to
make the interrupt handling logic hardware-agnostic. It also sets
`DMA_MAX_CHANNELS` to 16 to align with the new BSP code and ensure loops
over interrupts are correctly bounded.

Changes:
- Change `DMA_MAX_CHANNELS` definition to 16.
- Added `num_channels_per_reg` to `struct sun6i_dma_config`.
- Replaced hardcoded IRQ register calculations with values from
  `sdev->cfg->num_channels_per_reg`.
- Updated `sun6i_kill_tasklet` to loop through all possible interrupt
  registers based on `DMA_MAX_CHANNELS` and the configuration.

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

Comments

sashiko-bot@kernel.org June 19, 2026, 5:08 a.m. UTC | #1 
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Incorrect physical channel indexing in `sun6i_dma_interrupt` ignores the register index, applying status to the wrong physical channel.
- [High] Integer division truncation in the `sun6i_dma_interrupt` loop bound skips polling for trailing channels.
- [High] Time-of-Check to Time-of-Use (TOCTOU) race condition in `sun6i_dma_interrupt` risks a NULL pointer dereference.
--

commit 3bfcb64969bffa1792e80eaa282fc15450f3870b
Author: Yuanshen Cao <alex.caoys@gmail.com>

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

This commit introduces num_channels_per_reg to the sun6i_dma_config to
handle SoCs with varying channels per interrupt register. It modifies
the interrupt logic to use this new configuration value and bounds checking.

> diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
> index 059455425e19c..fb1c1a28744bb 100644
> --- a/drivers/dma/sun6i-dma.c
> +++ b/drivers/dma/sun6i-dma.c

[ ... ]

> @@ -574,7 +575,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 is a pre-existing issue, but does this loop condition truncate the
division and miss trailing channels?

If sdev->num_pchans is not perfectly divisible by num_channels_per_reg
(for example, 12 channels with 8 channels per register), the integer
division truncates the remainder. This means the loop will skip the second
register entirely, permanently missing interrupts for the trailing channels.

>  		status = sdev->cfg->read_irq_stat(sdev, i);
>  		if (!status)
>  			continue;
> @@ -584,7 +585,7 @@ static irqreturn_t sun6i_dma_interrupt(int irq, void *dev_id)
>  
>  		sdev->cfg->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: Critical]
This is a pre-existing issue, but does this apply the status to the wrong
physical channel?

The array lookup uses only the local offset j, discarding the register
index i. For any register i > 0, it accesses sdev->pchans[j] instead of
sdev->pchans[i * num_channels_per_reg + j].

With the newly added hardware where num_channels_per_reg is 1, j is always
0, which seems like it would cause all channels to incorrectly map to
channel 0.

>  			vchan = pchan->vchan;
>  			if (vchan && (status & vchan->irq_type)) {

[Severity: High]
This is a pre-existing issue, but is there a race condition here that could
lead to a NULL pointer dereference?

The interrupt handler reads vchan = pchan->vchan locklessly, and eventually
blocks to acquire vchan->vc.lock. Concurrently, sun6i_dma_terminate_all()
could acquire the same lock, nullify pchan->desc, and free the descriptors.

When the interrupt handler successfully acquires the lock:

spin_lock(&vchan->vc.lock);
vchan_cookie_complete(&pchan->desc->vd);

it does not re-verify pchan->desc before passing &pchan->desc->vd to
vchan_cookie_complete(), which would crash if pchan->desc was concurrently
set to NULL.
Frank Li June 19, 2026, 3:46 p.m. UTC | #2 
On Fri, Jun 19, 2026 at 04:53:32AM +0000, Yuanshen Cao wrote:
>
> The previous implementation of `sun6i-dma` had some implicit assumptions
> about the number of channels per interrupt register. Specifically,
> functions like `sun6i_kill_tasklet` were hardcoded to only disable
> interrupts for IRQ 0 and 1. `DMA_MAX_CHANNELS` is also not in used in
> the past, 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. This patch introduces
> `num_channels_per_reg` to the `sun6i_dma_config`, similar to BSP, to
> make the interrupt handling logic hardware-agnostic. It also sets
> `DMA_MAX_CHANNELS` to 16 to align with the new BSP code and ensure loops
> over interrupts are correctly bounded.
>
> Changes:
> - Change `DMA_MAX_CHANNELS` definition to 16.
> - Added `num_channels_per_reg` to `struct sun6i_dma_config`.
> - Replaced hardcoded IRQ register calculations with values from
>   `sdev->cfg->num_channels_per_reg`.
> - Updated `sun6i_kill_tasklet` to loop through all possible interrupt
>   registers based on `DMA_MAX_CHANNELS` and the configuration.
>
> Signed-off-by: Yuanshen Cao <alex.caoys@gmail.com>
> ---
>  drivers/dma/sun6i-dma.c | 25 ++++++++++++++++++-------
>  1 file changed, 18 insertions(+), 7 deletions(-)
>
...
> @@ -1171,6 +1174,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),
> +       .num_channels_per_reg = DMA_IRQ_CHAN_NR,

if previous patch have MACRO, you can put it to there

Frank
diff mbox series

Patch

diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
index 059455425e19..fb1c1a28744b 100644
--- a/drivers/dma/sun6i-dma.c
+++ b/drivers/dma/sun6i-dma.c
@@ -41,7 +41,7 @@ 
 #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
@@ -151,6 +151,7 @@  struct sun6i_dma_config {
 	u32 src_addr_widths;
 	u32 dst_addr_widths;
 	bool has_mbus_clk;
+	u32 num_channels_per_reg;
 };
 
 /*
@@ -481,8 +482,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;
 
@@ -574,7 +575,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 = sdev->cfg->read_irq_stat(sdev, i);
 		if (!status)
 			continue;
@@ -584,7 +585,7 @@  static irqreturn_t sun6i_dma_interrupt(int irq, void *dev_id)
 
 		sdev->cfg->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)) {
@@ -1108,9 +1109,11 @@  static struct dma_chan *sun6i_dma_of_xlate(struct of_phandle_args *dma_spec,
 
 static inline void sun6i_kill_tasklet(struct sun6i_dma_dev *sdev)
 {
+	int i;
+
 	/* Disable all interrupts from DMA */
-	writel(0, sdev->base + DMA_IRQ_EN(0));
-	writel(0, sdev->base + DMA_IRQ_EN(1));
+	for (i = 0; i < DMA_MAX_CHANNELS / sdev->cfg->num_channels_per_reg; i++)
+		sdev->cfg->write_irq_en(sdev, i, 0);
 
 	/* Prevent spurious interrupts from scheduling the tasklet */
 	atomic_inc(&sdev->tasklet_shutdown);
@@ -1171,6 +1174,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),
+	.num_channels_per_reg = DMA_IRQ_CHAN_NR,
 };
 
 /*
@@ -1200,6 +1204,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),
+	.num_channels_per_reg = DMA_IRQ_CHAN_NR,
 };
 
 static struct sun6i_dma_config sun8i_a83t_dma_cfg = {
@@ -1224,6 +1229,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),
+	.num_channels_per_reg = DMA_IRQ_CHAN_NR,
 };
 
 /*
@@ -1257,6 +1263,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),
+	.num_channels_per_reg = DMA_IRQ_CHAN_NR,
 };
 
 /*
@@ -1284,6 +1291,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),
+	.num_channels_per_reg = DMA_IRQ_CHAN_NR,
 };
 
 /*
@@ -1311,6 +1319,7 @@  static struct sun6i_dma_config sun50i_a100_dma_cfg = {
 			     BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) |
 			     BIT(DMA_SLAVE_BUSWIDTH_4_BYTES) |
 			     BIT(DMA_SLAVE_BUSWIDTH_8_BYTES),
+	.num_channels_per_reg = DMA_IRQ_CHAN_NR,
 	.has_mbus_clk = true,
 };
 
@@ -1339,6 +1348,7 @@  static struct sun6i_dma_config sun50i_h6_dma_cfg = {
 			     BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) |
 			     BIT(DMA_SLAVE_BUSWIDTH_4_BYTES) |
 			     BIT(DMA_SLAVE_BUSWIDTH_8_BYTES),
+	.num_channels_per_reg = DMA_IRQ_CHAN_NR,
 	.has_mbus_clk = true,
 };
 
@@ -1369,6 +1379,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),
+	.num_channels_per_reg = DMA_IRQ_CHAN_NR,
 };
 
 static const struct of_device_id sun6i_dma_match[] = {