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

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

[Rosen Penev]

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.

Moved before anything else in order to handle -EPROBE_DEFER and to avoid
doing extra work. Simplifies the if statements as well.

Assisted-by: opencode:big-pickle
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 v2: move to the front of probe.
 drivers/dma/fsldma.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/dma/fsldma.c b/drivers/dma/fsldma.c
index fb19c60540be..9ee8ae1aaf40 100644
--- a/drivers/dma/fsldma.c
+++ b/drivers/dma/fsldma.c
@@ -1224,8 +1224,13 @@ static int fsldma_of_probe(struct platform_device *op)
 	struct fsldma_device *fdev;
 	struct device_node *child;
 	unsigned int i;
+	int irq;
 	int err;
 
+	irq = platform_get_irq_optional(op, 0);
+	if (irq == -EPROBE_DEFER)
+		return irq;
+
 	fdev = kzalloc_obj(*fdev);
 	if (!fdev) {
 		err = -ENOMEM;
@@ -1246,7 +1251,9 @@ static int fsldma_of_probe(struct platform_device *op)
 	}
 
 	/* map the channel IRQ if it exists, but don't hookup the handler yet */
-	fdev->irq = irq_of_parse_and_map(op->dev.of_node, 0);
+	fdev->irq = irq;
+	if (fdev->irq < 0)
+		fdev->irq = 0;
 
 	dma_cap_set(DMA_MEMCPY, fdev->common.cap_mask);
 	dma_cap_set(DMA_SLAVE, fdev->common.cap_mask);
@@ -1308,7 +1315,7 @@ static int fsldma_of_probe(struct platform_device *op)
 		if (fdev->chan[i])
 			fsl_dma_chan_remove(fdev->chan[i]);
 	}
-	irq_dispose_mapping(fdev->irq);
+out_iounmap:
 	iounmap(fdev->regs);
 out_free:
 	kfree(fdev);
@@ -1330,7 +1337,6 @@ static void fsldma_of_remove(struct platform_device *op)
 		if (fdev->chan[i])
 			fsl_dma_chan_remove(fdev->chan[i]);
 	}
-	irq_dispose_mapping(fdev->irq);
 
 	iounmap(fdev->regs);
 	kfree(fdev);
-- 
2.55.0

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

[sashiko-bot]

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

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

[Frank Li]

On Thu, Sep 10, 2026 at 08:58:44PM +0000, sashiko-bot@kernel.org wrote:
> 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.

Did you build it?

Frank

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