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

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

New issues:
- [High] The driver incorrectly enforces a mandatory clock using the optional `clk_bulk` API, breaking probe for `!CONFIG_HAVE_CLK` configurations and systems using dummy clocks.

Pre-existing issues:
- [High] The runtime PM usage counter is leaked and PM runtime is left enabled if `pm_runtime_get_sync()` fails in `sprd_dma_probe()`.
- [High] The interrupt handler `dma_irq_handle` can be invoked before `sdev->glb_base` is mapped, leading to a NULL pointer dereference.
- [High] The driver improperly unlinks DMA channels before calling `dma_async_device_unregister()`, breaking the DMA engine core cleanup.
- [High] Failing `pm_runtime_get_sync()` calls do not decrement the usage count in `sprd_dma_alloc_chan_resources()`, leading to PM runtime counter leaks.
--

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

The commit converts the driver to use the clk_bulk API for managing its clocks
to fix a clock imbalance. This ensures both clocks are properly disabled on
failure paths. The optional ashb_eb clock is fetched alongside the mandatory
enable clock via devm_clk_bulk_get_optional().

> diff --git a/drivers/dma/sprd-dma.c b/drivers/dma/sprd-dma.c
> index 087fea3af2e41..749cabd984ebd 100644
> --- a/drivers/dma/sprd-dma.c
> +++ b/drivers/dma/sprd-dma.c

[ ... ]

> @@ -1140,16 +1120,17 @@ 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);
> -	}
> +	/* The ashb_eb clock is optional and only present for AGCP DMA */
> +	sdev->clks[0].id = "enable";
> +	sdev->clks[1].id = "ashb_eb";
> +	ret = devm_clk_bulk_get_optional(&pdev->dev, ARRAY_SIZE(sdev->clks),
> +					 sdev->clks);
> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret, "get clocks 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");
> +	if (!sdev->clks[0].clk)
> +		return dev_err_probe(&pdev->dev, -ENOENT,
> +				     "get enable clock failed\n");

[Severity: High]
Does this manual NULL check break probe for systems without CONFIG_HAVE_CLK or
when dummy clocks are used?

When using devm_clk_bulk_get_optional(), if CONFIG_HAVE_CLK is disabled, the
API returns 0 but sets the clock pointer to NULL. The previous IS_ERR() check
would safely ignore this, but the new strict NULL check will unconditionally
fail with -ENOENT.

>  	/*
>  	 * We have three DMA controllers: AP DMA, AON DMA and AGCP DMA. For AGCP


While reviewing this patch, I also noticed a few pre-existing issues in the file.

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);

[Severity: High]
This is a pre-existing issue, but could this cause a NULL pointer dereference
if the interrupt fires immediately?

The interrupt handler dma_irq_handle() dereferences sdev->glb_base to read
SPRD_DMA_GLB_INT_MSK_STS. Because devm_request_irq() is called before
devm_platform_ioremap_resource(), an early interrupt would access the unmapped
glb_base which is still NULL.


Also in sprd_dma_probe():

> 	pm_runtime_set_active(&pdev->dev);
> 	pm_runtime_enable(&pdev->dev);
> 
> 	ret = pm_runtime_get_sync(&pdev->dev);
> 	if (ret < 0)
> 		goto err_rpm;
[ ... ]
> err_register:
> 	pm_runtime_put_noidle(&pdev->dev);
> 	pm_runtime_disable(&pdev->dev);
> err_rpm:
> 	sprd_dma_disable(sdev);
> 	return ret;

[Severity: High]
This isn't a bug introduced by this patch, but does this error path leak the
runtime PM usage counter?

If pm_runtime_get_sync() fails, it still increments the usage count.
Jumping directly to err_rpm skips pm_runtime_put_noidle() and
pm_runtime_disable(), leaving PM runtime enabled and the counter unbalanced.


In sprd_dma_alloc_chan_resources():

> static int sprd_dma_alloc_chan_resources(struct dma_chan *chan)
> {
> 	return pm_runtime_get_sync(chan->device->dev);
> }

[Severity: High]
This is a pre-existing issue, but does this leak the PM usage count if it fails?

Because pm_runtime_get_sync() increments the counter even on failure, directly
returning its negative error code without calling pm_runtime_put_noidle() leaves
the usage count permanently elevated, preventing the device from suspending.
Should this use pm_runtime_resume_and_get() instead?


In sprd_dma_remove():

> 	list_for_each_entry_safe(c, cn, &sdev->dma_dev.channels,
> 				 vc.chan.device_node) {
> 		list_del(&c->vc.chan.device_node);
> 		tasklet_kill(&c->vc.task);
> 	}
> 
> 	of_dma_controller_free(pdev->dev.of_node);
> 	dma_async_device_unregister(&sdev->dma_dev);

[Severity: High]
This is a pre-existing issue, but does explicitly removing the channels from
the list here break the DMA engine core cleanup?

The channels are unlinked with list_del() before calling
dma_async_device_unregister(). When the core DMA engine loops over the
channels to free resources (such as sysfs entries), the list is empty, resulting
in leaked channel resources.

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