Re: [PATCH 11/26] dmaengine: sf-pdma-sf-pdma: Remove redundant dev_err()/dev_err_probe()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The driver masks standard error codes with -EINVAL, permanently breaking probe deferral.
- [High] Interrupt handlers are registered before channel structures are initialized, causing an immediate kernel panic on spurious interrupts.
- [High] Tasklets are not killed on probe error paths, leading to use-after-free list corruption.
- [High] Tasklets access `chan->desc` without checking for NULL, leading to crashes when racing with channel termination.
--

--- Patch [11]: [PATCH 11/26] dmaengine: sf-pdma-sf-pdma: Remove redundant dev_err()/dev_err_probe() ---
commit 09662195d630e22b4643f58a4176a95df1d14255
Author: Pan Chuang <panchuang@vivo.com>

dmaengine: sf-pdma-sf-pdma: 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/sf-pdma/sf-pdma.c b/drivers/dma/sf-pdma/sf-pdma.c
> index b3cba11b62032..93611e335a075 100644
> --- a/drivers/dma/sf-pdma/sf-pdma.c
> +++ b/drivers/dma/sf-pdma/sf-pdma.c
> @@ -412,10 +412,8 @@ static int sf_pdma_irq_init(struct platform_device *pdev, struct sf_pdma *pdma)
>  
>  		r = devm_request_irq(&pdev->dev, irq, sf_pdma_done_isr, 0,
>  				     dev_name(&pdev->dev), (void *)chan);
> -		if (r) {
> -			dev_err(&pdev->dev, "Fail to attach done ISR: %d\n", r);
> +		if (r)
>  			return -EINVAL;

[Severity: High]
This is a pre-existing issue, but does this mask standard error codes?

If platform_get_irq() or devm_request_irq() returns -EPROBE_DEFER, returning
-EINVAL will cause the driver core to treat the probe as a permanent failure
instead of deferring it. Should the original error code be propagated instead?

>  
>  		chan->txirq = irq;
>  
> @@ -425,10 +423,8 @@ static int sf_pdma_irq_init(struct platform_device *pdev, struct sf_pdma *pdma)
>  
>  		r = devm_request_irq(&pdev->dev, irq, sf_pdma_err_isr, 0,
>  				     dev_name(&pdev->dev), (void *)chan);
> -		if (r) {
> -			dev_err(&pdev->dev, "Fail to attach err ISR: %d\n", r);
> +		if (r)
>  			return -EINVAL;

[Severity: High]
This isn't a bug introduced by this patch, but does this also drop the
actual error code returned by devm_request_irq()?

Propagating the original error code 'r' would ensure probe deferrals and
other specific failure reasons are handled correctly by the driver core.

[ ... ]

[Severity: High]
This is a pre-existing issue, but can spurious interrupts cause a panic
during probe?

In drivers/dma/sf-pdma/sf-pdma.c, sf_pdma_probe() calls sf_pdma_irq_init()
before sf_pdma_setup_chans():

sf_pdma_probe() {
    ...
    ret = sf_pdma_irq_init(pdev, pdma);
    if (ret)
        return ret;

    sf_pdma_setup_chans(pdma);
    ...
}

If an interrupt fires immediately after devm_request_irq(),
sf_pdma_done_isr() will read from the uninitialized chan->regs.ctrl:

sf_pdma_done_isr() {
    ...
    writel((readl(regs->ctrl)) & ~PDMA_DONE_STATUS_MASK, regs->ctrl);
    ...
}

Would it be safer to call sf_pdma_setup_chans() before requesting interrupts?

[Severity: High]
This is a pre-existing issue, but do we leave tasklets running if the probe
fails later on?

If an interrupt fires during probe and schedules done_tasklet, and a
subsequent step like dma_async_device_register() fails:

sf_pdma_probe() {
    ...
err_unregister:
    dma_async_device_unregister(&pdma->dma_dev);

    return ret;
}

The devres framework will free the device memory and IRQs, but
devm_free_irq() does not kill pending tasklets. Could the scheduled tasklet
execute later using freed memory?

[Severity: High]
This isn't a bug introduced by this patch, but is there a potential NULL
pointer dereference if tasklets race with channel termination?

sf_pdma_terminate_all() frees chan->desc and sets it to NULL without
synchronizing with the tasklets:

sf_pdma_terminate_all() {
    ...
    kfree(chan->desc);
    chan->desc = NULL;
    ...
}

If done_tasklet runs immediately after, it acquires the lock and blindly
dereferences chan->desc:

sf_pdma_donebh_tasklet() {
    ...
    spin_lock_irqsave(&chan->vchan.lock, flags);
    list_del(&chan->desc->vdesc.node);
    ...
}

Should chan->desc be checked for NULL, or should the tasklets be killed
during termination?

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