Re: [PATCH 06/26] dmaengine: mmp_pdma: 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] Potential NULL pointer dereference in IRQ handlers (`mmp_pdma_chan_handler` / `mmp_pdma_int_handler`) when handling spurious or shared interrupts before initialization is complete or when the PHY is inactive.
- [High] Missing `tasklet_kill()` on device removal and probe error paths causes a use-after-free if the tasklet runs after `devres` frees the channel memory.
- [Medium] Negative IRQ error codes like `-EPROBE_DEFER` are incorrectly passed to `devm_request_irq()` and masked as `-EINVAL`.
--

--- Patch [6]: [PATCH 06/26] dmaengine: mmp_pdma: Remove redundant dev_err()/dev_err_probe() ---
commit f205944d00824c4ac830efbda0d25e06f33f54a9
Author: Pan Chuang <panchuang@vivo.com>

dmaengine: mmp_pdma: Remove redundant dev_err()/dev_err_probe()

The devm_request_irq() function automatically logs detailed error messages on
failure. This commit eliminates the need for driver-specific dev_err() and
dev_err_probe() calls that previously printed generic messages.

> diff --git a/drivers/dma/mmp_pdma.c b/drivers/dma/mmp_pdma.c
> index 386e85cd4882a..c2a638d554883 100644
> --- a/drivers/dma/mmp_pdma.c
> +++ b/drivers/dma/mmp_pdma.c
> @@ -1161,10 +1161,8 @@ static int mmp_pdma_chan_init(struct mmp_pdma_device *pdev, int idx, int irq)
>  	if (irq) {

[Severity: Medium]
This is a pre-existing issue, but could irq be a negative error code here?

If platform_get_irq() returns -EPROBE_DEFER, it evaluates as true in this
condition. It would then be passed directly to devm_request_irq() below,
which implicitly casts it to an unsigned int and fails with -EINVAL, permanently
failing the probe instead of deferring it.

>  		ret = devm_request_irq(pdev->dev, irq, mmp_pdma_chan_handler,
>  				       IRQF_SHARED, "pdma", phy);

[Severity: High]
This isn't a bug introduced by this patch, but is there a risk of a NULL
pointer dereference with these shared IRQs?

In mmp_pdma_probe(), a shared interrupt is requested before the loop that
initializes phy->base in mmp_pdma_chan_init(). If a shared interrupt triggers
immediately, mmp_pdma_int_handler() calls mmp_pdma_chan_handler() and then
clear_chan_irq(), which dereferences phy->base:

drivers/dma/mmp_pdma.c:clear_chan_irq() {
    ...
    u32 dint = readl(phy->base + DINT);
    ...
}

Additionally, if a spurious interrupt fires while phy->vchan is NULL,
clear_chan_irq() can still return 0. This causes mmp_pdma_chan_handler() to
unconditionally execute tasklet_schedule(&phy->vchan->tasklet), which
dereferences NULL.

> -		if (ret) {
> -			dev_err(pdev->dev, "channel request irq fail!\n");
> +		if (ret)
>  			return ret;
> -		}
>  	}
>  
>  	spin_lock_init(&chan->desc_lock);

[Severity: High]
This is a pre-existing issue, but does this driver leave tasklets running
after the channel memory is freed?

A few lines below here in mmp_pdma_chan_init(), a tasklet is set up via
tasklet_setup(). However, mmp_pdma_remove() does not appear to call
tasklet_kill(). 

Since free_irq() synchronizes with running interrupt handlers but not with
scheduled tasklets, if a tasklet was scheduled right before the IRQ was freed,
it could execute after devres has freed the chan memory, causing a
use-after-free.

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