Re: [PATCH v2] dmaengine: sprd: use clk_bulk API to fix clock imbalance

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

Pre-existing issues:
- [High] Runtime PM usage counter and enablement state leak on error paths.
- [High] Device interrupt requested before hardware IO memory is mapped and spinlocks are initialized.
--

--- Patch [1]: [PATCH v2] dmaengine: sprd: use clk_bulk API to fix clock imbalance ---
commit ed070633d7ec91a8fe862c830443d077ba0540c0
Author: Chaithanya Lagisetty <[email protected]>

dmaengine: sprd: use clk_bulk API to fix clock imbalance

This patch converts the driver to use the clk_bulk API to handle clock
enabling and disabling. It simplifies error paths and fixes a clock
imbalance issue where sdev->clk was leaked if the optional ashb_clk failed
to enable.

> diff --git a/drivers/dma/sprd-dma.c b/drivers/dma/sprd-dma.c
> index 087fea3af2e41..ac6fe6118ad19 100644
> --- a/drivers/dma/sprd-dma.c
> +++ b/drivers/dma/sprd-dma.c
[ ... ]
> @@ -1140,16 +1120,18 @@ static int sprd_dma_probe(struct platform_device *pdev)
>  	if (!sdev)
>  		return -ENOMEM;
>  
> -	sdev->clk = devm_clk_get(&pdev->dev, "enable");
> -	if (IS_ERR(sdev->clk)) {
> -		dev_err(&pdev->dev, "get enable clock failed\n");
> -		return PTR_ERR(sdev->clk);
> -	}
> +	sdev->clks[0].id = "enable";
> +	sdev->clks[0].clk = devm_clk_get(&pdev->dev, "enable");
> +	if (IS_ERR(sdev->clks[0].clk))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(sdev->clks[0].clk),
> +				     "get enable clock failed\n");
>  
>  	/* ashb clock is optional for AGCP DMA */
> -	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");
> +	sdev->clks[1].id = "ashb_eb";
> +	sdev->clks[1].clk = devm_clk_get_optional(&pdev->dev, "ashb_eb");
> +	if (IS_ERR(sdev->clks[1].clk))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(sdev->clks[1].clk),
> +				     "get ashb_eb clock failed\n");
>  
>  	/*
>  	 * We have three DMA controllers: AP DMA, AON DMA and AGCP DMA. For AGCP

[Severity: High]
This is a pre-existing issue, but does this code request the interrupt too
early?

Looking at the sequence further down in sprd_dma_probe():

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

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

The interrupt is requested before sdev->glb_base is mapped and before the
virtual channels are initialized. If an interrupt is pending (for example,
left over from a bootloader or kexec), could the IRQ handler dma_irq_handle()
fire immediately?

If it fires early, the handler will attempt to read from sdev->glb_base:

drivers/dma/sprd-dma.c:dma_irq_handle() {
	...
	u32 irq_status = readl(sdev->glb_base + SPRD_DMA_GLB_INT_MSK_STS);
	...
}

Since sdev was allocated with devm_kzalloc(), sdev->glb_base is still NULL,
which would lead to a NULL pointer dereference. The handler might also lock
schan->vc.lock before it has been initialized by vchan_init(). Would it be
safer to request the interrupt after all necessary resources are mapped and
initialized?


[Severity: High]
This isn't a bug introduced by this patch, but is there a PM usage counter
leak on the error path further down in sprd_dma_probe()?

When pm_runtime_get_sync() fails, the code jumps to err_rpm:

	ret = pm_runtime_get_sync(&pdev->dev);
	if (ret < 0)
		goto err_rpm;
...
err_rpm:
	sprd_dma_disable(sdev);
	return ret;

This skips calling pm_runtime_put_noidle() and pm_runtime_disable(), which
could leave the runtime PM state unbalanced and prevent the device from ever
suspending if the probe is retried.

A similar issue appears to exist in sprd_dma_alloc_chan_resources(), where
it directly returns the result of pm_runtime_get_sync():

drivers/dma/sprd-dma.c:sprd_dma_alloc_chan_resources() {
	...
	return pm_runtime_get_sync(chan->device->dev);
}

Could this also leak the usage counter if pm_runtime_get_sync() fails?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1