Re: [PATCHv4 13/15] dmaengine: fsldma: replace irq_of_parse_and_map with of_irq_get

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] Diagnostic print incorrectly evaluates negative error codes, preventing fallback to the controller IRQ.

Pre-existing issues:
- [High] The driver does not kill the channel tasklet before freeing the channel structure, risking a use-after-free.
- [High] The return value of dma_async_device_register() is ignored.
--

commit b6458c2f1ed9edcf2ad0905771c28bd59e93cd71
Author: Rosen Penev <rosenp@gmail.com>
Subject: dmaengine: fsldma: replace irq_of_parse_and_map with of_irq_get

This commit replaces irq_of_parse_and_map() with of_irq_get() in the fsldma
driver. It also modifies the error handling in fsldma_request_irqs() to
properly propagate negative error codes, such as -EPROBE_DEFER, instead of
just checking for zero.

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

[ ... ]

> @@ -1185,7 +1191,7 @@ static int fsl_dma_chan_probe(struct fsldma_device *fdev,
>  	dma_cookie_init(&chan->common);
>  
>  	/* find the IRQ line, if it exists in the device tree */
> -	chan->irq = irq_of_parse_and_map(node, 0);
> +	chan->irq = of_irq_get(node, 0);
>  
>  	/* Add the channel to DMA device channel list */
>  	list_add_tail(&chan->common.device_node, &fdev->common.channels);
>  
>  	dev_info(fdev->dev, "#%d (%s), irq %d\n", chan->id, compatible,
>  		 chan->irq ? chan->irq : fdev->irq);

[Severity: Medium]
Since chan->irq can now be a negative error code on failure, will the ternary
operator (chan->irq ? ...) evaluate to true? If so, does this mean the driver
will print the negative error code instead of falling back to the controller's
IRQ (fdev->irq) in the kernel log?

[Severity: High]
This is a pre-existing issue and was not introduced by this patch, but
should the driver kill the channel tasklet before freeing the channel
structure?

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

The driver schedules chan->tasklet in its interrupt handler, but during
device removal or probe error unwinding, fsl_dma_chan_remove() unmaps
I/O and frees chan without calling tasklet_kill(&chan->tasklet). Could
a tasklet scheduled just before teardown execute after chan has been
freed, causing a use-after-free?

[Severity: High]
This is a pre-existing issue and was not introduced by this patch, but
is it safe to ignore the return value of dma_async_device_register()?

drivers/dma/fsldma.c:fsldma_of_probe() {
    ...
	err = fsldma_request_irqs(fdev);
	if (err) {
		dev_err(fdev->dev, "unable to request IRQs\n");
		goto out_free_fdev;
	}

	dma_async_device_register(&fdev->common);
	return 0;

out_free_fdev:
    ...
}

If registration fails (e.g., due to memory allocation failure or
validation issues), fsldma_of_probe() will still return 0 (success).
This leaves the driver bound to an unregistered DMA device. When the
device is later removed, dma_async_device_unregister() will be called
on it, which might lead to undefined behavior.

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