Re: [PATCH 16/26] dmaengine: sprd-dma: 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] Incorrect Optional Resource Handling / Ignored Probe Deferral
- [High] Premature IRQ Request Causes Potential NULL Pointer Dereference
- [High] Missing Tasklet Cleanup on Probe Failure Leads to Use-After-Free
--

--- Patch [16]: [PATCH 16/26] dmaengine: sprd-dma: Remove redundant dev_err()/dev_err_probe() ---
commit e390b8344f4bfbd854daa3d432e12e9099d1828c
Author: Pan Chuang <panchuang@vivo.com>

dmaengine: sprd-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/sprd-dma.c b/drivers/dma/sprd-dma.c
> index 087fea3af2e41..dbf52ff88b47e 100644
> --- a/drivers/dma/sprd-dma.c
> +++ b/drivers/dma/sprd-dma.c
> @@ -1162,10 +1162,8 @@ static int sprd_dma_probe(struct platform_device *pdev)
>  	if (sdev->irq > 0) {

[Severity: High]
This is a pre-existing issue, but does this correctly handle probe deferral?

If platform_get_irq() returns -EPROBE_DEFER, the check if (sdev->irq > 0) 
ignores it and the driver will start without its IRQ. 

A similar issue exists earlier in this function with the ashb_eb clock:

	sdev->ashb_clk = devm_clk_get(&pdev->dev, "ashb_eb");
	if (IS_ERR(sdev->ashb_clk))
		dev_warn(&pdev->dev, "no optional ashb eb clock\n");

If devm_clk_get() returns -EPROBE_DEFER, the driver merely prints a warning
and continues. If the hardware actually requires this clock, accessing the 
DMA registers unclocked could cause a bus error and freeze the system.

Should this code use devm_clk_get_optional() and platform_get_irq_optional(),
and explicitly propagate errors like -EPROBE_DEFER?

>  		ret = devm_request_irq(&pdev->dev, sdev->irq, dma_irq_handle,
>  				       0, "sprd_dma", (void *)sdev);

[Severity: High]
This is a pre-existing issue, but can this early IRQ request cause a NULL
pointer dereference?

The interrupt is requested and enabled here, but sdev->glb_base is mapped 
afterwards:

	sdev->glb_base = devm_platform_ioremap_resource(pdev, 0);

If the IRQ line is already active (e.g., from the bootloader or kexec),
dma_irq_handle() will trigger immediately and try to read from glb_base:

	struct sprd_dma_dev *sdev = (struct sprd_dma_dev *)dev_id;
	u32 irq_status = readl(sdev->glb_base + SPRD_DMA_GLB_INT_MSK_STS);

Since sdev->glb_base hasn't been mapped yet, this will dereference a NULL
pointer. Should the IRQ request be deferred until all hardware mappings and
internal channel structures are fully initialized?

> -		if (ret < 0) {
> -			dev_err(&pdev->dev, "request dma irq failed\n");
> +		if (ret < 0)
>  			return ret;
> -		}
>  	} else {
>  		dev_warn(&pdev->dev, "no interrupts for the dma controller\n");
>  	}

[Severity: High]
This is a pre-existing issue, but does the probe error path leak scheduled
tasklets leading to a use-after-free?

During vchan_init(), tasklets are set up for each channel. If a late probe 
step fails (like of_dma_controller_register()), the error path handles
cleanup:

err_of_register:
	dma_async_device_unregister(&sdev->dma_dev);
err_register:
	pm_runtime_put_noidle(&pdev->dev);
	pm_runtime_disable(&pdev->dev);
err_rpm:
	sprd_dma_disable(sdev);
	return ret;

However, this doesn't call tasklet_kill() for the channels. If an interrupt 
fired or a channel was used, the tasklet remains scheduled. When devres 
subsequently frees the sdev memory, the scheduled tasklet will execute and
access freed memory, causing a panic. 

Should the error rollback path manually call tasklet_kill() for all 
initialized channels?

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