Re: [PATCH 01/26] dmaengine: fsl-edma-main: Remove redundant dev_err()/dev_err_probe()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The txirq_16_31 interrupt is not freed before vchan cleanup, leading to a race condition and potential use-after-free on device unbind.
- [High] Double disable/unprepare of devm-managed clocks during the probe failure path.
- [High] Probe deferral is permanently broken due to unconditionally overriding negative error codes from `platform_get_irq()` with `-EINVAL` or `-ENXIO`.
--

--- Patch [1]: [PATCH 01/26] dmaengine: fsl-edma-main: Remove redundant dev_err()/dev_err_probe() ---
commit 0e4d4c4909fc6ed60a790a7b9fecb3bf5b716c3b
Author: Pan Chuang <panchuang@vivo.com>

    dmaengine: fsl-edma-main: Remove redundant dev_err()/dev_err_probe()
    
    The devm_request_irq() now automatically logs detailed error messages on
    failure. This eliminates the need for driver-specific dev_err() and
    dev_err_probe() calls that previously printed generic messages.

> diff --git a/drivers/dma/fsl-edma-main.c b/drivers/dma/fsl-edma-main.c
> index 36155ab1602a9..0881f4f36b3f4 100644
> --- a/drivers/dma/fsl-edma-main.c
> +++ b/drivers/dma/fsl-edma-main.c
[ ... ]
> @@ -418,7 +412,7 @@ static int fsl_edma3_irq_init(struct platform_device *pdev, struct fsl_edma_engi
>  		ret = devm_request_irq(&pdev->dev, fsl_edma->errirq, fsl_edma3_err_handler_shared,
>  				       0, errirq_name, fsl_edma);
>  		if (ret)
> -			return dev_err_probe(&pdev->dev, ret, "Can't register eDMA err IRQ.\n");
> +			return ret;

[Severity: High]
This is a pre-existing issue, but probe deferral appears to be permanently
broken for the interrupt requests in this function and fsl_edma2_irq_init().

In fsl_edma3_irq_init(), if platform_get_irq() returns -EPROBE_DEFER,
the error code is discarded and replaced with -EINVAL:

    fsl_chan->txirq = platform_get_irq(pdev, i);
    if (fsl_chan->txirq < 0)
        return -EINVAL;

Similarly in fsl_edma2_irq_init(), it replaces the error with -ENXIO:

    irq = platform_get_irq(pdev, i);
    ret = 0;
    if (irq < 0)
        return -ENXIO;

Could this prevent the DMA engine from probing on systems where the
interrupt controller is not yet ready?

>  	}
>  
>  	return 0;
> @@ -445,24 +439,21 @@ static int fsl_edma3_or_irq_init(struct platform_device *pdev,
>  			       fsl_edma3_tx_0_15_handler, 0, "eDMA tx0_15",
>  			       fsl_edma);
>  	if (ret)
> -		return dev_err_probe(&pdev->dev, ret,
> -			       "Can't register eDMA tx0_15 IRQ.\n");
> +		return ret;
>  
>  	if (fsl_edma->n_chans > 16) {
>  		ret = devm_request_irq(&pdev->dev, fsl_edma->txirq_16_31,
>  				       fsl_edma3_tx_16_31_handler, 0,
>  				       "eDMA tx16_31", fsl_edma);
>  		if (ret)
> -			return dev_err_probe(&pdev->dev, ret,
> -					"Can't register eDMA tx16_31 IRQ.\n");
> +			return ret;

[Severity: High]
This isn't a bug introduced by this patch, but does the handling of
txirq_16_31 create a race condition leading to a potential use-after-free?

When the driver is unbound, fsl_edma_remove() executes:

    fsl_edma_irq_exit(pdev, fsl_edma);
    fsl_edma_cleanup_vchan(&fsl_edma->dma_dev);

However, fsl_edma_irq_exit() manually frees txirq and errirq to stop
interrupts before vchan cleanup, but it misses txirq_16_31:

    if (fsl_edma->txirq == fsl_edma->errirq) {
        if (fsl_edma->txirq >= 0)
            devm_free_irq(&pdev->dev, fsl_edma->txirq, fsl_edma);
    } else {
        if (fsl_edma->txirq >= 0)
            devm_free_irq(&pdev->dev, fsl_edma->txirq, fsl_edma);
        if (fsl_edma->errirq >= 0)
            devm_free_irq(&pdev->dev, fsl_edma->errirq, fsl_edma);
    }

Since txirq_16_31 is not freed, is it possible for the interrupt handler
to still run during or after fsl_edma_cleanup_vchan() destroys the vchan
tasklets?

>  	}
>  
>  	ret = devm_request_irq(&pdev->dev, fsl_edma->errirq,
>  			       fsl_edma3_or_err_handler, 0, "eDMA err",
>  			       fsl_edma);
>  	if (ret)
> -		return dev_err_probe(&pdev->dev, ret,
> -				     "Can't register eDMA err IRQ.\n");
> +		return ret;
>  
>  	return 0;
>  }

[Severity: High]
This isn't a bug introduced by this patch, but is there a double disable of
devm-managed clocks during the probe failure path?

In fsl_edma_probe(), the mux clocks are acquired using the devm variant:

    fsl_edma->muxclk[i] = devm_clk_get_enabled(&pdev->dev, clkname);

If resource mapping fails for a subsequent mux base, the error path manually
disables the clocks:

    if (IS_ERR(fsl_edma->muxbase[i])) {
        /* on error: disable all previously enabled clks */
        fsl_disable_clocks(fsl_edma, i);
        return PTR_ERR(fsl_edma->muxbase[i]);
    }

Because the clocks were acquired with devm_clk_get_enabled(), won't devres
also try to disable them when cleaning up the probe failure, resulting in a
double-disable?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709135846.97972-1-panchuang@vivo.com?part=1