Re: [PATCH 08/10] 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 2 potential issue(s) to consider:

New issues:
- [High] The patch replaces `irq_of_parse_and_map()` with `of_irq_get()`, which can correctly return negative error codes such as `-EPROBE_DEFER` when the interrupt controller is not yet ready. However, in `fsldma_request_irqs()`, the updated error handling block catches these negative codes but incorrectly hardcodes the return value to `-ENODEV`. This squashes the actual error code and permanently fails the probe process, breaking probe deferral.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but DMA client callbacks are invoked with the channel lock (`chan->desc_lock`) held, which causes an AB-BA self-deadlock if the client submits a new transaction from the callback. The DMAengine API explicitly allows clients to submit new transactions from their completion callbacks. When a client does this, `fsl_dma_tx_submit()` is called, which unconditionally attempts to acquire `spin_lock_bh(&chan->desc_lock)`. Since `fsldma_cleanup_descriptors()` already holds this non-recursive spinlock when invoking the callback via `dmaengine_desc_get_callback_invoke()`, the system will deadlock.
--

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

dmaengine: fsldma: replace irq_of_parse_and_map with of_irq_get

This commit replaces irq_of_parse_and_map() with of_irq_get() to handle
negative error codes appropriately. It updates the IRQ validation checks in
fsldma_request_irqs() to handle both 0 and negative error returns correctly.

> diff --git a/drivers/dma/fsldma.c b/drivers/dma/fsldma.c
> index 0d73ce3dbfe6a..79a268139b9f8 100644
> --- a/drivers/dma/fsldma.c
> +++ b/drivers/dma/fsldma.c
> @@ -1067,7 +1067,7 @@ static int fsldma_request_irqs(struct fsldma_device *fdev)
>  		if (!chan)
>  			continue;
>  
> -		if (!chan->irq) {
> +		if (chan->irq <= 0) {
>  			chan_err(chan, "interrupts property missing in device tree\n");
>  			ret = -ENODEV;

[Severity: High]
Does overriding the return value here to -ENODEV cause a regression with
probe deferral?

If of_irq_get() returns a valid negative error code like -EPROBE_DEFER
when the interrupt controller is not yet ready, catching it here but
hardcoding the return value to -ENODEV permanently squashes the actual
error code. This will cause the DMA controller to permanently fail to probe
if it initializes before its interrupt controller.

>  			goto out_unwind;
>  		}

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but I noticed an existing locking
issue with how DMA client callbacks are invoked.

Looking at fsldma_run_tx_complete_actions(), dmaengine_desc_get_callback_invoke()
is called synchronously without dropping chan->desc_lock.

If a client submits a new transaction from their completion callback, it will
call into fsl_dma_tx_submit() which unconditionally attempts to acquire
spin_lock_bh(&chan->desc_lock). Since this non-recursive spinlock is already
held, this creates an AB-BA self-deadlock.

Could this be updated to drop the lock before invoking the callback, as the
DMAengine API explicitly allows clients to queue new transfers from their
completion callbacks?

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