Re: [PATCH v2 5/5] dmaengine: sun6i-dma: Implement support for Allwinner A733 DMA controller

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Incorrect physical channel indexing in the interrupt handler maps all A733 physical channel interrupts to `sdev->pchans[0]`.
- [High] Integer division truncation in the interrupt handler's outer loop condition causes upper DMA channels to be completely ignored.
--

commit 5f14cc2c94bb065f85b3b101369ba127ddd3a538
Author: Yuanshen Cao <alex.caoys@gmail.com>

dmaengine: sun6i-dma: Implement support for Allwinner A733 DMA controller

This patch adds support for the Allwinner A733 DMA controller by defining
its specific register macros and interrupt structures. It introduces
a new sun60i_a733_dma_cfg with A733-specific configuration, notably setting
the number of channels per interrupt register to 1.

> diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
> index 196a0d73b2212..4808015934ccb 100644
> --- a/drivers/dma/sun6i-dma.c
> +++ b/drivers/dma/sun6i-dma.c

[ ... ]

> @@ -1339,6 +1398,33 @@ static struct sun6i_dma_config sun50i_h6_dma_cfg = {
>  	SUN6I_DMA_IRQ_A31_COMMON_OPS
>  };
>  
> +/*
> + * The A733 binding uses the number of dma channels from the
> + * device tree node.
> + */
> +static struct sun6i_dma_config sun60i_a733_dma_cfg = {
> +	.clock_autogate_enable = sun6i_enable_clock_autogate_h3,
> +	.set_burst_length = sun6i_set_burst_length_h3,
> +	.set_drq          = sun6i_set_drq_h6,
> +	.set_mode         = sun6i_set_mode_h6,
> +	.set_addr         = sun6i_dma_set_addr_a733,
> +	.dump_com_regs    = sun6i_dma_dump_com_regs_a733,
> +	.read_irq_en      = sun6i_read_irq_en_a733,
> +	.write_irq_en     = sun6i_write_irq_en_a733,
> +	.read_irq_stat    = sun6i_read_irq_stat_a733,
> +	.write_irq_stat   = sun6i_write_irq_stat_a733,
> +	.src_burst_lengths = BIT(1) | BIT(4) | BIT(8) | BIT(16),
> +	.dst_burst_lengths = BIT(1) | BIT(4) | BIT(8) | BIT(16),
> +	.src_addr_widths   = BIT(DMA_SLAVE_BUSWIDTH_1_BYTE) |
> +			     BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) |
> +			     BIT(DMA_SLAVE_BUSWIDTH_4_BYTES),
> +	.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_A733,

[Severity: High]
This is a pre-existing issue, but I wanted to ask about how the current
interrupt handling interacts with num_channels_per_reg.

In sun6i_dma_interrupt() in drivers/dma/sun6i-dma.c, the outer loop
determines the number of registers to check using integer division:

    for (i = 0; i < sdev->num_pchans / sdev->cfg->num_channels_per_reg; i++) {
        status = sdev->cfg->read_irq_stat(sdev, i);

If the number of physical channels is not a perfectly divisible multiple
(e.g., a controller with 12 channels and 8 channels per register), the
division truncates to 1. Will this cause the handler to ignore the upper
registers entirely, leading to silent timeouts on upper channels?

[Severity: Critical]
This is also a pre-existing issue, but how does sun6i_dma_interrupt()
resolve the physical channel when i > 0?

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

The calculation pchan = sdev->pchans + j ignores the register index i.

Because DMA_IRQ_CHAN_NR_A733 is 1, j will always be 0. Doesn't this mean
every interrupt on the A733 controller will incorrectly map to channel 0,
causing channel 0 to complete spuriously while the actual triggering
channel hangs?

> +	.has_mbus_clk = true,
> +};
> +

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