Re: [PATCHv2] dmaengine: fsldma: convert to platform_get_irq_optional()

[sashiko-bot@kernel.org]

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

New issues:
- [Low] The patch introduces an unused label `out_iounmap:`, which will cause a compiler warning and break kernel builds configured with -Werror.

Pre-existing issues:
- [Critical] Missing `tasklet_kill()` during channel removal can lead to a use-after-free of the channel structure memory.
- [High] The driver ignores the return value of `dma_async_device_register()`, leading to a silent failure state and potential kernel oops on module removal.
- [High] The error path in `fsldma_suspend_late` unlocks `desc_lock` for channels that were already unlocked, leading to a double-unlock kernel panic.
--

commit 8cf71591d829b94cb515a6922619751103de2f0d
Author: Rosen Penev <rosenp@gmail.com>

dmaengine: fsldma: convert to platform_get_irq_optional()

Replace the per-controller irq_of_parse_and_map() call with
platform_get_irq_optional(). The controller IRQ is optional when absent
and the driver falls back to per-channel IRQs. The corresponding
irq_dispose_mapping() calls in the probe error path and remove function
are removed.

> diff --git a/drivers/dma/fsldma.c b/drivers/dma/fsldma.c
> index 22d62d958abd9..0637a7b2e14d0 100644
> --- a/drivers/dma/fsldma.c
> +++ b/drivers/dma/fsldma.c

[ ... ]

> @@ -1300,7 +1307,7 @@ static int fsldma_of_probe(struct platform_device *op)
>  		if (fdev->chan[i])
>  			fsl_dma_chan_remove(fdev->chan[i]);

[Severity: Critical]
This is a pre-existing issue, but does fsl_dma_chan_remove() need to call
tasklet_kill() before freeing the channel memory?

drivers/dma/fsldma.c:fsl_dma_chan_remove() {
	irq_dispose_mapping(chan->irq);
	list_del(&chan->common.device_node);
	iounmap(chan->regs);
	kfree(chan);
}

A tasklet scheduled by the device's IRQ handler can execute after this frees
the channel memory via kfree(chan). The free_irq() function waits for
executing IRQ handlers but does not wait for or kill scheduled tasklets,
potentially leading to a use-after-free.

>  	}
> -	irq_dispose_mapping(fdev->irq);
> +out_iounmap:

[Severity: Low]
Does this introduce an unused label?

The patch introduces out_iounmap:, but nothing seems to jump to it, which
will cause a compiler warning and break kernel builds configured with -Werror.

>  	iounmap(fdev->regs);
>  out_free:
>  	kfree(fdev);

[ ... ]


[Severity: High]
This is a pre-existing issue, but does the driver ignore the return value of
dma_async_device_register() in fsldma_of_probe()?

drivers/dma/fsldma.c:fsldma_of_probe() {
    ...
	dma_async_device_register(&fdev->common);
	return 0;

out_free_fdev:
    ...
}

If dma_async_device_register() fails, it returns a negative error code,
leaving fields like the device IDA and refcount uninitialized or zeroed.
Because the driver ignores this failure, it will complete the probe
successfully.

During module removal, dma_async_device_unregister() will execute on these
uninitialized internal fields, potentially triggering an oops.


[Severity: High]
This is a pre-existing issue, but does fsldma_suspend_late() unlock
desc_lock for channels that were already unlocked?

drivers/dma/fsldma.c:fsldma_suspend_late() {
    ...
	for (i = 0; i < FSL_DMA_MAX_CHANS_PER_DEVICE; i++) {
        ...
		spin_lock_bh(&chan->desc_lock);
		if (unlikely(!chan->idle))
			goto out;
        ...
		spin_unlock_bh(&chan->desc_lock);
	}
	return 0;

out:
	for (; i >= 0; i--) {
		chan = fdev->chan[i];
		if (!chan)
			continue;
		chan->pm_state = RUNNING;
		spin_unlock_bh(&chan->desc_lock);
	}
	return -EBUSY;
}

In the forward loop, spin_unlock_bh(&chan->desc_lock) is correctly called on
successful iterations.

If a subsequent channel is not idle, the code jumps to out and iterates
backwards from the failing channel down to 0, unconditionally calling
spin_unlock_bh(&chan->desc_lock) for all previously processed channels.

Can this unlock locks that were already released during the forward loop's
successful iterations, leading to a double-unlock panic?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260910204625.55367-1-rosenp@gmail.com?part=1