Re: [PATCH 02/26] dmaengine: fsl-qdma: 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:

New issues:
- [Medium] Incomplete Refactoring / Commit Message Mismatch

Pre-existing issues:
- [High] Driver fails to halt the hardware DMA engine on teardown and error paths
- [High] Inverted teardown order: driver destroys internal resources before unregistering the device
- [High] vchan tasklets are not killed on probe failure
--

--- Patch [2]: [PATCH 02/26] dmaengine: fsl-qdma: Remove redundant dev_err()/dev_err_probe() ---
commit a6f00a613e01411f16385ac0c92bca0027dc654d
Author: Pan Chuang <panchuang@vivo.com>

dmaengine: fsl-qdma: Remove redundant dev_err()/dev_err_probe()

This commit removes a redundant error log following a call to
devm_request_irq, as the core already logs failures. It intends to clean up
the error paths in the qDMA driver.

> 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.

[Severity: Medium]
Is the commit message accurate regarding dev_err_probe? Looking at the
patch, no dev_err_probe calls are actually removed.

Also, does this patch miss the second devm_request_irq in the same
function?

drivers/dma/fsl-qdma.c:fsl_qdma_irq_init() {
[ ... ]
        ret = devm_request_irq(&pdev->dev,
                               fsl_qdma->queue_irq[i],
                               fsl_qdma_queue_handler,
                               0,
                               "qDMA queue",
                               fsl_qdma);
        if (ret) {
                dev_err(&pdev->dev,
                        "Can't register qDMA queue IRQ.\n");
                return  ret;
        }
[ ... ]
}

> diff --git a/drivers/dma/fsl-qdma.c b/drivers/dma/fsl-qdma.c
> index df843fad0ece5..7f0d69b99289e 100644
> --- a/drivers/dma/fsl-qdma.c
> +++ b/drivers/dma/fsl-qdma.c
> @@ -818,10 +818,8 @@ fsl_qdma_irq_init(struct platform_device *pdev,
>  	ret = devm_request_irq(&pdev->dev, fsl_qdma->error_irq,
>  			       fsl_qdma_error_handler, 0,
>  			       "qDMA error", fsl_qdma);
> -	if (ret) {
> -		dev_err(&pdev->dev, "Can't register qDMA controller IRQ.\n");
> +	if (ret)
>  		return  ret;
> -	}
>  
>  	for (i = 0; i < fsl_qdma->block_number; i++) {
>  		sprintf(irq_name, "qdma-queue%d", i);

[Severity: High]
This is a pre-existing issue, but does the driver fail to halt the hardware
DMA engine on teardown and error paths?

fsl_qdma_reg_init() explicitly enables the DMA hardware and its interrupts.
If probe() fails later or when the driver is unbound, the driver never 
invokes fsl_qdma_halt() to stop the device:

drivers/dma/fsl-qdma.c:fsl_qdma_remove() {
        fsl_qdma_irq_exit(pdev, fsl_qdma);
        fsl_qdma_cleanup_vchan(&fsl_qdma->dma_dev);
        of_dma_controller_free(np);
        dma_async_device_unregister(&fsl_qdma->dma_dev);
}

Because the DMA buffers are allocated via devres (e.g., dmam_alloc_coherent),
they are freed immediately upon driver removal or probe failure. Can the
unhalted hardware continue to perform DMA operations into this freed memory?

[Severity: High]
This is a pre-existing issue, but does fsl_qdma_remove() have an inverted
teardown order?

drivers/dma/fsl-qdma.c:fsl_qdma_remove() {
        fsl_qdma_irq_exit(pdev, fsl_qdma);
        fsl_qdma_cleanup_vchan(&fsl_qdma->dma_dev);
        of_dma_controller_free(np);
        dma_async_device_unregister(&fsl_qdma->dma_dev);
}

fsl_qdma_cleanup_vchan() is called before dma_async_device_unregister(). This
creates a window where the DMA controller is still visible to clients via the
DMA engine API, but its backend is destroyed.

Furthermore, fsl_qdma_cleanup_vchan() removes channels from the channels
list. Will dma_async_device_unregister() then iterate over an empty list and
fail to deregister the channels from sysfs?

[Severity: High]
This is a pre-existing issue, but are vchan tasklets properly killed on
probe failure?

drivers/dma/fsl-qdma.c:fsl_qdma_probe() {
[ ... ]
        ret = fsl_qdma_irq_init(pdev, fsl_qdma);
        if (ret)
                return ret;

        ret = dma_async_device_register(&fsl_qdma->dma_dev);
        if (ret)
                return dev_err_probe(&pdev->dev, ret,
                                     "Can't register NXP Layerscape qDMA engine.\n");
[ ... ]
}

vchan_init() initializes the vchan tasklets for each channel. If a subsequent
step like fsl_qdma_irq_init() or dma_async_device_register() fails, probe()
returns the error code directly without calling tasklet_kill().

Because the hardware is already enabled, could an interrupt fire and schedule
the tasklet? When devres frees the fsl_qdma->chans array upon probe failure,
would the scheduled tasklet execute against freed memory?

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