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

Pre-existing issues:
- [High] Integer division truncation in `sun6i_dma_interrupt` loop bound silently drops interrupts for higher-numbered channels.
- [High] Incorrect physical channel indexing in `sun6i_dma_interrupt` misroutes interrupts for higher channels.
- [High] TOCTOU race in `sun6i_dma_interrupt` leads to a NULL pointer dereference on channel teardown.
--

commit 0015717509538d0aa3c622981170b196ad6d4d64
Author: Yuanshen Cao <alex.caoys@gmail.com>

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

The `sun6i-dma` driver previously assumed a fixed number of channels per
interrupt register. The A733 has a different interrupt structure where the
number of channels per register may differ, so add `num_channels_per_reg` to
`struct sun6i_dma_config` to make the interrupt handling logic hardware-agnostic.

> diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
> index 53c54161e32a9..b96d7d90f6b53 100644
> --- a/drivers/dma/sun6i-dma.c
> +++ b/drivers/dma/sun6i-dma.c

[ ... ]

> @@ -585,7 +587,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 isn't a bug introduced by this patch, but does this loop silently ignore
higher-numbered channels in sun6i_dma_interrupt() due to integer division
truncation?

If the total number of channels is not perfectly divisible by the channels per
register (e.g. Allwinner H3 has 12 channels, 8 per register), this calculation
truncates the remainder. For H3, 12 / 8 truncates to 1, causing the loop to
only process i = 0 (channels 0-7) and skip i = 1 (channels 8-11), stalling
those higher channels.

The patch correctly uses DIV_ROUND_UP() in sun6i_dma_dump_com_regs(), should
it be used here as well?

>  		status = sun6i_read_irq_stat(sdev, i);
>  		if (!status)
>  			continue;
> @@ -595,7 +597,7 @@ static irqreturn_t sun6i_dma_interrupt(int irq, void *dev_id)
>  
>  		sun6i_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 array indexing misroute interrupts
for channels in subsequent registers?

The index calculation in sun6i_dma_interrupt() completely ignores the register
index `i`. For instance, if an interrupt fires on channel 8 (which is in
register i=1, offset j=0), this will retrieve sdev->pchans[0] instead of
sdev->pchans[8], erroneously completing channel 0's active transfer.
Shouldn't this incorporate i * sdev->cfg->num_channels_per_reg?

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

[Severity: High]
This is a pre-existing issue, but does this lockless check introduce a TOCTOU
race leading to a NULL pointer dereference on channel teardown?

If a client driver concurrently terminates the channel (e.g., during device
removal) and sun6i_dma_terminate_all() sets pchan->desc to NULL, this code
can evaluate the following block where pchan->desc->vd is dereferenced:

                if (vchan->cyclic) {
                    vchan_cyclic_callback(&pchan->desc->vd);
                } else {
                    spin_lock(&vchan->vc.lock);
                    vchan_cookie_complete(&pchan->desc->vd);

Since the cyclic path takes no locks and the non-cyclic path doesn't re-verify
pchan->desc after taking the lock, could this crash when the pointer becomes
NULL mid-execution?

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