Re: [PATCH v2] dmaengine: dw-axi-dmac: convert clock handling to clk_bulk API

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

New issues:
- [High] The patch silently reverses the required clock enable and disable order, which can cause hardware initialization failures.

Pre-existing issues:
- [High] The interrupt handler unconditionally accesses unclocked device registers if an interrupt fires on a shared line while the device is in PM runtime suspend.
--

--- Patch [1]: [PATCH v2] dmaengine: dw-axi-dmac: convert clock handling to clk_bulk API ---
commit 9a52e2da75664a18bf3bb34d3bc595717db1bddc
Author: Chaithanya Lagisetty <[email protected]>
Subject: dmaengine: dw-axi-dmac: convert clock handling to clk_bulk API

This commit converts the driver to use the clk_bulk API for managing its two
mandatory clocks. It consolidates the clock getting, enabling, and disabling
logic to simplify error handling and fix a clock refcount imbalance.

Link: https://lore.kernel.org/all/[email protected]/

> diff --git a/drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c b/drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c
> index bcefaff03b5c6..254167a558ff7 100644
> --- a/drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c
> +++ b/drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c

[ ... ]

> @@ -1524,13 +1519,11 @@ static int dw_probe(struct platform_device *pdev)
>  
>  	chip->dw->hdata->use_cfg2 = !!(flags & AXI_DMA_FLAG_USE_CFG2);
>  
> -	chip->core_clk = devm_clk_get(chip->dev, "core-clk");
> -	if (IS_ERR(chip->core_clk))
> -		return PTR_ERR(chip->core_clk);
> -
> -	chip->cfgr_clk = devm_clk_get(chip->dev, "cfgr-clk");
> -	if (IS_ERR(chip->cfgr_clk))
> -		return PTR_ERR(chip->cfgr_clk);
> +	chip->clks[0].id = "core-clk";
> +	chip->clks[1].id = "cfgr-clk";

[Severity: High]
Does this array initialization unintentionally reverse the clock enable and
disable sequence?

The old driver correctly enabled cfgr_clk before core_clk in axi_dma_resume(),
and disabled core_clk before cfgr_clk in axi_dma_suspend().

Because clk_bulk_prepare_enable() iterates forwards and
clk_bulk_disable_unprepare() iterates backwards, mapping core-clk to index 0
and cfgr-clk to index 1 applies the reverse hardware sequence for both.  Can
this cause hardware lockups or bus faults?

> +	ret = devm_clk_bulk_get(chip->dev, ARRAY_SIZE(chip->clks), chip->clks);
> +	if (ret)
> +		return dev_err_probe(chip->dev, ret, "failed to get clocks\n");

[Severity: High]
This is a pre-existing issue, but does the interrupt handler unconditionally
access unclocked device registers if an interrupt fires on a shared line
while the device is in PM runtime suspend?

In dw_probe(), the IRQs are requested using IRQF_SHARED:

dw-axi-dmac-platform.c:axi_req_irqs() {
	...
	ret = devm_request_irq(chip->dev, chip->irq[i], dw_axi_dma_interrupt,
			       IRQF_SHARED, KBUILD_MODNAME, chip);
	...
}

If the DMA controller is idle, runtime PM suspends the device, which disables
the clocks. If another device sharing the same IRQ line triggers an
interrupt, the handler is called:

dw-axi-dmac-platform.c:dw_axi_dma_interrupt() {
	...
	/* Disable DMAC interrupts. We'll enable them after processing channels */
	axi_dma_irq_disable(chip);
	...
}

Since axi_dma_irq_disable() accesses DMAC_CFG without checking if the device
is active, can this unclocked register read trigger a synchronous external
abort (kernel panic)?

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