Re: [PATCH 09/26] dmaengine: pxa_dma: Remove redundant dev_err()/dev_err_probe()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The shared interrupt handler panics on `BUG_ON(!chan)` for spurious or shared interrupts.
- [High] Tasklets are not explicitly destroyed if `of_dma_controller_register()` fails, leading to a potential use-after-free.
--

--- Patch [9]: [PATCH 09/26] dmaengine: pxa_dma: Remove redundant dev_err()/dev_err_probe() ---
commit f2ce223752ea303da12586b72cd70a589be5a08b
Author: Pan Chuang <panchuang@vivo.com>

dmaengine: pxa_dma: 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/pxa_dma.c b/drivers/dma/pxa_dma.c
> index fa2ee0b3e09f8..7bdcc5e6a3d8f 100644
> --- a/drivers/dma/pxa_dma.c
> +++ b/drivers/dma/pxa_dma.c
> @@ -1260,12 +1260,8 @@ static int pxad_init_phys(struct platform_device *op,
>  			ret = devm_request_irq(&op->dev, irq0,
>  					       pxad_int_handler,
>  					       IRQF_SHARED, "pxa-dma", pdev);
> -		if (ret) {
> -			dev_err(pdev->slave.dev,
> -				"%s(): can't request irq %d:%d\n", __func__,
> -				irq, ret);
> +		if (ret)
>  			return ret;

[Severity: High]
This isn't a bug introduced by this patch, but registering the IRQ handlers
with IRQF_SHARED exposes a potential kernel panic.

If another device sharing the IRQ line generates an interrupt, or if a
spurious interrupt occurs, the kernel calls pxad_chan_handler():

drivers/dma/pxa_dma.c:pxad_chan_handler() {
    struct pxad_phy *phy = dev_id;
    struct pxad_chan *chan = phy->vchan;
    ...
    BUG_ON(!chan);
    ...
}

If no DMA transfers are currently active, phy->vchan is NULL, leading to a
deterministic kernel panic. 

Should the shared interrupt handler return IRQ_NONE instead of panicking
when the device has not generated an interrupt?


[Severity: High]
This is another pre-existing issue, but returning directly on failure here
or in the probe function can cause a use-after-free.

During probe, the driver exposes DMA channels by calling
dmaenginem_async_device_register() inside pxad_init_dmadev(). If the
subsequent of_dma_controller_register() call fails, it returns directly:

drivers/dma/pxa_dma.c:pxad_probe() {
    ...
    ret = pxad_init_dmadev(op, pdev, dma_channels, nb_requestors);
    if (ret) { ... }

    if (op->dev.of_node) {
        ret = of_dma_controller_register(op->dev.of_node,
                                         pxad_dma_xlate, pdev);
        if (ret < 0) {
            dev_err(pdev->slave.dev,
                "of_dma_controller_register failed\n");
            return ret;
        }
    ...
}

Because the DMA engine was temporarily registered, a concurrent non-DT
client could have requested a channel and scheduled its tasklet. When devres
automatically frees the struct pxad_chan memory upon probe failure, the
scheduled tasklet remains on the system's tasklet list.

When the tasklet executes, it will access the freed channel structure. 

Does the error path need an explicit call to pxad_free_channels() to kill
the tasklets, similar to what is done in pxad_remove()?

> -		}
>  	}
>  
>  	return 0;

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