Re: [PATCH v3 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 4 potential issue(s) to consider:

New issues:
- [High] Hardcoded loop bound in `sun6i_kill_tasklet` leads to out-of-bounds MMIO register writes.

Pre-existing issues:
- [Critical] Incorrect physical channel indexing in `sun6i_dma_interrupt` misroutes hardware interrupts to the wrong software channels.
- [High] Integer division truncation in IRQ register loops silently drops interrupts for leftover channels.
- [High] A TOCTOU race exists between `sun6i_dma_interrupt` and `sun6i_dma_terminate_all`, leading to a NULL pointer dereference.
--

commit 901a8f4f5bfa274478e14bdc97eb09faba518f9f
Author: Yuanshen Cao <alex.caoys@gmail.com>

dmaengine: sun6i-dma: Add support for Allwinner A733 DMA controller
    
Support Allwinner A733 DMA controller. Define new register offsets,
bitfield mappings and dma_config required for the A733, which slightly
differs from the older sun6i DMA controllers.

> 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
> @@ -257,6 +268,23 @@ static inline void sun6i_dma_dump_com_regs(struct sun6i_dma_dev *sdev)
[ ... ]
> +static inline void sun6i_dma_dump_com_regs_a733(struct sun6i_dma_dev *sdev)
> +{
> +	int i;
> +
> +	for (i = 0; i < sdev->num_pchans / sdev->cfg->num_channels_per_reg; i++) {

[Severity: High]
This is a pre-existing issue, but I noticed this loop limit might cause integer
division truncation.

If the total number of channels is not an exact multiple of the channels
per register, the fractional remainder is truncated. For example, if a
platform has 12 channels and 8 per register, the loop only iterates once,
completely ignoring the second register. 

Does this mean interrupts for the leftover channels are never read or
cleared, leading to an unhandled interrupt storm? Should this use 
DIV_ROUND_UP instead? This buggy logic also seems to be present in
sun6i_dma_interrupt().

> +		dev_dbg(sdev->slave.dev, "Common register:\n"
[ ... ]
> @@ -1339,6 +1398,33 @@ static struct sun6i_dma_config sun50i_h6_dma_cfg = {
[ ... ]
> +static struct sun6i_dma_config sun60i_a733_dma_cfg = {
> +	.clock_autogate_enable = sun6i_enable_clock_autogate_h3,
[ ... ]
> +	.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: Critical]
This is a pre-existing issue, but setting num_channels_per_reg to 1 for
the A733 controller appears to completely break the DMA controller due
to an issue in sun6i_dma_interrupt().

In sun6i_dma_interrupt(), the physical channel is resolved as:

    pchan = sdev->pchans + j;

Since num_channels_per_reg is 1, j is always 0. Does this route every
hardware interrupt to software channel 0, regardless of the actual
triggering channel? Should the calculation instead be updated to properly
account for the register index:

    pchan = sdev->pchans + i * sdev->cfg->num_channels_per_reg + j;

[Severity: High]
I also noticed an issue in sun6i_kill_tasklet() that might be triggered
by this configuration.

sun6i_kill_tasklet() calculates its iteration bounds using:

    for (i = 0; i < DMA_MAX_CHANNELS / sdev->cfg->num_channels_per_reg; i++)
        sdev->cfg->write_irq_en(sdev, i, 0);

With DMA_MAX_CHANNELS set to 16 and num_channels_per_reg set to 1, this
loop will unconditionally execute 16 times. If the dynamically initialized
channel count for the SoC is fewer than 16, will these writes target
unmapped peripheral memory space and potentially trigger a synchronous
external abort? Should the loop bound depend on the actual initialized
channel count (sdev->num_pchans) instead?

> +	.has_mbus_clk = true,
> +};

[Severity: High]
This is a pre-existing issue, but could there be a TOCTOU race condition
between sun6i_dma_interrupt() and sun6i_dma_terminate_all() that leads to
a NULL pointer dereference?

In sun6i_dma_terminate_all(), the channel is stopped and pointers are cleared:

    vchan->phy = NULL;
    pchan->vchan = NULL;
    pchan->desc = NULL;
    pchan->done = NULL;

However, in sun6i_dma_interrupt(), the pointers are accessed like this:

    pchan = sdev->pchans + j;
    vchan = pchan->vchan;
    if (vchan && (status & vchan->irq_type)) {
        if (vchan->cyclic) {
            vchan_cyclic_callback(&pchan->desc->vd);
        } else {
            spin_lock(&vchan->vc.lock);
            vchan_cookie_complete(&pchan->desc->vd);

Since the interrupt handler checks vchan = pchan->vchan outside the lock,
could a concurrent call to sun6i_dma_terminate_all() clear pchan->desc
before it is dereferenced inside the interrupt handler, resulting in a
near-NULL pointer dereference?

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