Re: [PATCHv3 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:
- [High] The patch adds deferred probe support for per-channel IRQs but neglects the per-controller IRQ.
- [Medium] Negative error codes from `of_irq_get()` are incorrectly logged and passed to the IRQ mapping subsystem.

Pre-existing issues:
- [High] Missing `tasklet_kill()` leads to potential use-after-free of the channel structure.
--

commit 2b8b79cb3c43b58e4e60c937bb6a8f2ff23ac916
Author: Rosen Penev <rosenp@gmail.com>

dmaengine: fsldma: replace irq_of_parse_and_map with of_irq_get
    
Use of_irq_get() which returns a negative error code on failure
instead of silently returning 0. Split the IRQ validation check
in fsldma_request_irqs to handle three cases.

> The fsldma_free_irqs() function's !chan->irq check is unchanged
> since both 0 and negative values mean no IRQ to free.

[Severity: Medium]
Is this description accurate? In C, negative values like -EPROBE_DEFER
evaluate to true. This means !chan->irq will be false for negative error
codes, skipping the intended check, and potentially passing a negative
value to free_irq() or irq_dispose_mapping() later.

> Assisted-by: opencode:big-pickle
> Signed-off-by: Rosen Penev <rosenp@gmail.com>

> 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);

[Severity: High]
Does this change miss updating the per-controller IRQ in fsldma_of_probe()?

If irq_of_parse_and_map() remains in fsldma_of_probe() to set fdev->irq:

    fdev->irq = irq_of_parse_and_map(op->dev.of_node, 0);

It will silently return 0 on deferral. The driver then falls back to
checking per-channel IRQs. If they don't exist, probe fails completely
with -EINVAL instead of correctly deferring, preventing deferred probe
from working for the standard per-controller IRQ configuration.

[Severity: Medium]
Will this logging logic incorrectly evaluate negative error codes? In
fsl_dma_chan_probe(), just after this block:

    dev_info(fdev->dev, "#%d (%s), irq %d\n", chan->id, compatible,
             chan->irq ? chan->irq : fdev->irq);

Since chan->irq now stores negative error codes from of_irq_get(), the
ternary operator will evaluate them as true and log the error code
instead of falling back to the controller IRQ.

Additionally, is it safe to unconditionally pass a negative error code to
irq_dispose_mapping() in fsl_dma_chan_remove() if chan->irq failed?

    static void fsl_dma_chan_remove(struct fsldma_chan *chan)
    {
        irq_dispose_mapping(chan->irq);

[Severity: High]
This isn't a bug introduced by this patch, but is it possible for a
tasklet to trigger a use-after-free here?

During teardown, fsl_dma_chan_remove() does:

    static void fsl_dma_chan_remove(struct fsldma_chan *chan)
    {
        irq_dispose_mapping(chan->irq);
        list_del(&chan->common.device_node);
        iounmap(chan->regs);
        kfree(chan);
    }

It frees the channel memory without first calling
tasklet_kill(&chan->tasklet). If a tasklet was scheduled and remains
pending in the softirq queue, it will execute dma_do_tasklet() after the
chan structure has been freed, dereferencing freed memory.

>  
>  	/* Add the channel to DMA device channel list */
>  	list_add_tail(&chan->common.device_node, &fdev->common.channels);

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