Re: [PATCH AUTOSEL 6.18-5.15] dmaengine: dw-axi-dmac: fix PM for system sleep and channel alloc

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The shared interrupt handler accesses hardware registers without checking if the device is active, and the suspend callback fails to synchronize IRQs.
--

commit 11e86e3a3829f6e35823522311eef18dacb1afef
Author: Tze Yee Ng <tze.yee.ng@altera.com>

dmaengine: dw-axi-dmac: fix PM for system sleep and channel alloc
    
The driver only had runtime PM callbacks. If a channel stayed allocated
across system suspend/resume, the runtime usage count could remain
non-zero while hardware state (DMAC_CFG, clocks) was lost, and
axi_dma_runtime_resume() would not run to restore it.

Add system-sleep PM ops that use pm_runtime_force_suspend() and
pm_runtime_force_resume() so suspend/resume reuses the existing
axi_dma_suspend() and axi_dma_resume() paths.

Link: https://patch.msgid.link/18bf778a3a1cc2f377ef8eb0d1508d8ac6371896.1779688569.git.tze.yee.ng@altera.com

> 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 b23536645ff7c..4fd77f5b67c47 100644
> --- a/drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c
> +++ b/drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c

[ ... ]

> @@ -1654,6 +1659,8 @@ static void dw_remove(struct platform_device *pdev)
>  }
>  
>  static const struct dev_pm_ops dw_axi_dma_pm_ops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
> +				pm_runtime_force_resume)
>  	SET_RUNTIME_PM_OPS(axi_dma_runtime_suspend, axi_dma_runtime_resume, NULL)
>  };

[Severity: High]
This is a pre-existing issue, but does the shared interrupt handler have a
potential race condition with device suspend?

The driver requests a shared interrupt:

drivers/dma/dw-axi-dmac/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);
    ...
}

However, the interrupt handler accesses hardware registers unconditionally:

drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c:dw_axi_dma_interrupt() {
    ...
	axi_dma_irq_disable(chip);
    ...
}

If another device on the same shared IRQ line triggers an interrupt while this
DMA controller is runtime suspended (with clocks disabled), could this cause a
bus hang or system crash when accessing unclocked registers?

Additionally, the suspend routine cuts clocks without synchronizing IRQs:

drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c:axi_dma_suspend() {
    ...
	clk_disable_unprepare(chip->core_clk);
	clk_disable_unprepare(chip->cfgr_clk);
    ...
}

Could an in-flight IRQ handler crash when the clocks are cut mid-execution?

Should the interrupt handler use pm_runtime_get_if_active() to check if the
device is active before proceeding, and should the suspend callback call
synchronize_irq() to ensure no handler is executing before disabling the clocks?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831133314.4125787-209-sashal@kernel.org?part=1