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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Loop bound truncation in `sun6i_dma_interrupt` ignores higher physical channels.
- [High] The physical channel index computation in `sun6i_dma_interrupt` completely ignores the register offset.
--

commit 60cc97cd5f987f5b25e3c1a483094c243658c5e5
Author: Yuanshen Cao <alex.caoys@gmail.com>

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

The patch introduces num_channels_per_reg to sun6i_dma_config to support
different SoC interrupt structures where channels per register may differ.
It replaces hardcoded IRQ register calculations and sets DMA_MAX_CHANNELS
to 16.

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

[ ... ]

> @@ -575,7 +576,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
channel count if it isn't an exact multiple of the channels per register?

For example, if an SoC has 12 channels and 8 channels per register, integer
division (12 / 8) yields 1, causing the loop to only process the first
register (channels 0-7) and skip the second register (channels 8-11).

If a transfer completes on a higher channel, could this result in an
unhandled interrupt storm since the status is never read or cleared? Should
this use DIV_ROUND_UP(sdev->num_pchans, sdev->cfg->num_channels_per_reg)?

>  		status = sdev->cfg->read_irq_stat(sdev, i);
>  		if (!status)
>  			continue;
> @@ -585,7 +586,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: High]
This is a pre-existing issue, but does this physical channel lookup ignore
the register offset from the outer loop?

When processing the second interrupt register (i = 1, channels 8-15), the
inner loop accesses pchan indices 0 through 7 instead of offsetting by
i * num_channels_per_reg.

Could a legitimate hardware interrupt on channel 8 spuriously trigger the
completion callback for the client mapped to channel 0, while channel 8's
client waits indefinitely?

Should the lookup be:
pchan = sdev->pchans + (i * sdev->cfg->num_channels_per_reg) + j;

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

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