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

[sashiko-bot@kernel.org]

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

New issues:
- [High] Missing mandatory function pointers in sun60i_a733_dma_cfg leads to unconditionally dereferencing NULL pointers during DMA preparation.

Pre-existing issues:
- [High] Incorrect physical channel indexing in the DMA interrupt handler maps interrupts for all registers to the first register's channels.
- [High] Missed interrupts due to integer division truncation in the DMA interrupt handler causes higher channel interrupts to be ignored.
--

commit 9fc6c319ac37c3c86b13dd65e2affac0589606b0
Author: Yuanshen Cao <alex.caoys@gmail.com>

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

Implement support for the Allwinner A733 DMA controller by defining
A733-specific register offsets, bitfield masks, and the
sun60i_a733_dma_cfg configuration structure.

> diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
> index b96d7d90f6b53..fb371b57d792a 100644
> --- a/drivers/dma/sun6i-dma.c
> +++ b/drivers/dma/sun6i-dma.c
[ ... ]
> @@ -1321,6 +1327,29 @@ static struct sun6i_dma_config sun50i_h6_dma_cfg = {
>  	SUN6I_DMA_IRQ_A31_COMMON_CFG
>  };
>  
> +/*
> + * 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,
> +	.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),
> +	.src_high_addr_mask = SRC_HIGH_ADDR_32G_MASK,
> +	.dst_high_addr_mask = DST_HIGH_ADDR_32G_MASK,
> +	.has_mbus_clk = true,
> +	.irq_stride      = DMA_IRQ_STRIDE_A733,
> +	.irq_en_offset   = DMA_IRQ_EN_OFFSET_A733,
> +	.irq_stat_offset = DMA_IRQ_STAT_OFFSET_A733,
> +	.num_channels_per_reg = DMA_IRQ_CHAN_NR_A733,
> +};
> +

[Severity: High]
Does sun60i_a733_dma_cfg omit the set_burst_length, set_drq, and set_mode
function pointers?

When sun6i_dma_prep_dma_memcpy(), sun6i_dma_prep_slave_sg(), or
sun6i_dma_prep_dma_cyclic() are called, they unconditionally invoke these
function pointers:

    sdev->cfg->set_burst_length(&v_lli->cfg, burst, burst);
    sdev->cfg->set_drq(&v_lli->cfg, DRQ_SDRAM, DRQ_SDRAM);
    sdev->cfg->set_mode(&v_lli->cfg, LINEAR_MODE, LINEAR_MODE);

Could this result in a NULL pointer dereference on the A733 platform?


[Severity: High]
This is a pre-existing issue, but does setting num_channels_per_reg to 1 for
the A733 platform expose a physical channel indexing issue in
sun6i_dma_interrupt()?

When an interrupt occurs on a secondary IRQ register, sun6i_dma_interrupt()
reads the correct status register i, but calculates the physical channel
pointer as:

    pchan = sdev->pchans + j;

Since it omits the i * sdev->cfg->num_channels_per_reg offset, hardware
interrupts for higher channels appear to be routed to lower channels. On
A733, this would map all channel interrupts to channel 0.


[Severity: High]
This is a pre-existing issue, but does the loop bound in
sun6i_dma_interrupt() truncate due to integer division on platforms with
non-multiple channel counts?

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

If a platform has 12 physical channels and 8 channels per register (like H3),
the division evaluates to 1, causing the loop to only read interrupts for
channels 0-7 and ignore the final register. Could this cause transfers on the
highest channels to time out?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260826-sun60i-a733-dma-v4-0-dc2f745f40dc@gmail.com?part=5