Re: [PATCH 04/26] dmaengine: mediatek-mtk-cqdma: Remove redundant dev_err()/dev_err_probe()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] IRQ is requested and enabled before tasklets are initialized, leading to a crash.
- [High] The IRQ handler accesses unallocated channel structures, causing a NULL pointer dereference.
- [High] Device registers are accessed before clocks and power management are enabled, due to premature device registration.
- [High] Missing OF DMA controller deregistration on hardware initialization failure.
- [High] Missing cleanup for virtual channel tasklets on the probe error path leads to a potential Use-After-Free.
- [High] Teardown asymmetry in the device removal routine allows concurrent access to destroyed state.
--

--- Patch [4]: [PATCH 04/26] dmaengine: mediatek-mtk-cqdma: Remove redundant dev_err()/dev_err_probe() ---
commit 04c062c96653e2015152bd44faa0dd41edc7a398
Author: Pan Chuang <panchuang@vivo.com>

dmaengine: mediatek-mtk-cqdma: Remove redundant dev_err()/dev_err_probe()

This commit removes redundant dev_err() calls when devm_request_irq() fails
in mtk_cqdma_probe(), relying on the core's built-in logging instead. It also
simplifies the error path by returning -EINVAL directly.

> diff --git a/drivers/dma/mediatek/mtk-cqdma.c b/drivers/dma/mediatek/mtk-cqdma.c
> index 80791e30aec24..4dbe84954de6b 100644
> --- a/drivers/dma/mediatek/mtk-cqdma.c
> +++ b/drivers/dma/mediatek/mtk-cqdma.c
> @@ -828,11 +828,8 @@ static int mtk_cqdma_probe(struct platform_device *pdev)
>  		err = devm_request_irq(&pdev->dev, cqdma->pc[i]->irq,
>  				       mtk_cqdma_irq, 0, dev_name(&pdev->dev),
>  				       cqdma);

[Severity: High]
This is a pre-existing issue, but is it safe to request and enable the IRQ
here before the required tasklet is initialized?

If a pending hardware interrupt fires before tasklet_setup() is called at
the end of the probe function, won't the interrupt handler mtk_cqdma_irq()
execute and call tasklet_schedule() on a zero-initialized tasklet structure?

[Severity: High]
This is also a pre-existing issue, but does the IRQ handler safely handle
unallocated channels?

Physical channels (cqdma->pc[i]) are allocated and their IRQs requested
sequentially in this loop. If an interrupt fires for an earlier channel
(e.g., i=0) before the loop allocates the remaining channels,
mtk_cqdma_irq() executes and blindly iterates over all channels
(0 to cqdma->dma_channels - 1) attempting to lock cqdma->pc[i]->lock.

Could this result in a NULL pointer dereference for the unallocated
channels?

> -		if (err) {
> -			dev_err(&pdev->dev,
> -				"request_irq failed with err %d\n", err);
> +		if (err)
>  			return -EINVAL;
> -		}
>  	}
>  
>  	/* allocate resource for VCs */

While reviewing this patch, I noticed a few other pre-existing issues in
this driver:

drivers/dma/mediatek/mtk-cqdma.c:mtk_cqdma_probe() {
    ...
    err = dma_async_device_register(dd);
    ...
    err = mtk_cqdma_hw_init(cqdma);
    ...
}

[Severity: High]
This isn't a bug introduced by this patch, but are device registers accessed
before clocks and power management are enabled?

dma_async_device_register() exposes the DMA engine to clients. If a client
immediately requests a channel, mtk_cqdma_alloc_chan_resources() is called,
which invokes mtk_cqdma_hard_reset() to write to device registers.

However, mtk_cqdma_hw_init(), which enables the device clocks and power
management, is called after registration.

Can the register writes occur with disabled clocks causing a synchronous
external abort?


drivers/dma/mediatek/mtk-cqdma.c:mtk_cqdma_probe() {
    ...
    err = of_dma_controller_register(pdev->dev.of_node,
                                     of_dma_xlate_by_chan_id, cqdma);
    ...
err_unregister:
    dma_async_device_unregister(dd);

    return err;
}

[Severity: High]
This isn't a bug introduced by this patch, but does the error path leak the
OF DMA controller registration?

If mtk_cqdma_hw_init() fails, the code jumps to err_unregister. The error
path calls dma_async_device_unregister() but fails to call
of_dma_controller_free(pdev->dev.of_node).

Could this leave the device registered in the OF DMA list with a freed
driver context?


drivers/dma/mediatek/mtk-cqdma.c:mtk_cqdma_probe() {
    ...
    for (i = 0; i < cqdma->dma_requests; i++) {
        vc = &cqdma->vc[i];
        vc->vc.desc_free = mtk_cqdma_vdesc_free;
        vchan_init(&vc->vc, dd);
        init_completion(&vc->issue_completion);
    }
    ...
err_unregister:
    dma_async_device_unregister(dd);

    return err;
}

[Severity: High]
This isn't a bug introduced by this patch, but is there missing cleanup for
virtual channel tasklets on the probe error path?

vchan_init() sets up virtual channel tasklets. If a subsequent
initialization step (like mtk_cqdma_hw_init) fails, the err_unregister path
does not call tasklet_kill() for these channels.

Because the device was temporarily exposed via dma_async_device_register(),
a client could have scheduled a transaction, leaving the tasklet active.

When devres frees the device memory, could the executing tasklet trigger a
use-after-free?


drivers/dma/mediatek/mtk-cqdma.c:mtk_cqdma_remove() {
    ...
    /* internal driver state torn down here */
    ...
    dma_async_device_unregister(&cqdma->ddev);
    of_dma_controller_free(pdev->dev.of_node);
}

[Severity: High]
This isn't a bug introduced by this patch, but does the teardown asymmetry
in the device removal routine allow concurrent access to destroyed state?

In mtk_cqdma_remove(), internal driver state (tasklets and hardware
clocks/interrupts) is torn down before the subsystem interfaces
(dma_async_device_unregister and of_dma_controller_free) are unregistered.

Does this teardown inversion allow clients to issue new DMA requests while
the driver is actively being destroyed?

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