Re: [PATCH v3 4/4] irqchip/imx-irqsteer: Allow building as module

[sashiko-bot@kernel.org]

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

New issues:
- [High] Missing explicit runtime PM management in `remove()` causes a hardware access crash and a clock enable count underflow during devres cleanup.

Pre-existing issues:
- [High] The chained IRQ handler accesses hardware registers unconditionally without ensuring the device's clock is enabled, which can crash the system on spurious interrupts.
--

--- Patch [4]: [PATCH v3 4/4] irqchip/imx-irqsteer: Allow building as module ---
commit d2409da6cb54e5e7df533908b454c22db10f15a7
Author: Jindong Yue <jindong.yue@nxp.com>

irqchip/imx-irqsteer: Allow building as module

Make the driver buildable as a module by turning the Kconfig symbol into
a tristate and using module_platform_driver() instead of
builtin_platform_driver(). Let the driver core own the clock and runtime PM
lifetime so that remove() does not have to hand-balance them.

> diff --git a/drivers/irqchip/irq-imx-irqsteer.c b/drivers/irqchip/irq-imx-irqsteer.c
> index 0c9c99f1141a9..e62862617b8d1 100644
> --- a/drivers/irqchip/irq-imx-irqsteer.c
> +++ b/drivers/irqchip/irq-imx-irqsteer.c

[ ... ]

> @@ -266,15 +260,22 @@ static int imx_irqsteer_probe(struct platform_device *pdev)
>  
>  	platform_set_drvdata(pdev, data);
>  
> -	pm_runtime_set_active(&pdev->dev);
> -	pm_runtime_enable(&pdev->dev);
> +	ret = devm_pm_runtime_set_active_enabled(&pdev->dev);
> +	if (ret)
> +		goto err_irq;

[Severity: High]
This is a pre-existing issue, but does the chained IRQ handler access hardware
registers unconditionally without ensuring the device's clock is enabled?

If no child interrupts are requested by client drivers, the irqsteer device
drops its PM usage count to 0 and enters runtime suspend, disabling ipg_clk.
However, the parent interrupt handler imx_irqsteer_irq_handler remains
registered and active with the core IRQ subsystem.

If a spurious interrupt arrives while the device is suspended, the handler
executes and directly reads the CHANSTATUS register:

drivers/irqchip/irq-imx-irqsteer.c:imx_irqsteer_irq_handler() {
	...
	irqmap = readl_relaxed(data->regs + CHANSTATUS(idx, data->reg_num));
	...
}

Accessing memory-mapped registers of an IP block with a gated interface clock
can trigger a hardware fault (external abort / bus fault).

Should the handler use pm_runtime_get_if_active() to verify the power state
before accessing hardware?

[ ... ]

> @@ -302,8 +303,6 @@ static void imx_irqsteer_remove(struct platform_device *pdev)
>  						     hwirq));
>  
>  	irq_domain_remove(irqsteer_data->domain);
> -
> -	clk_disable_unprepare(irqsteer_data->ipg_clk);
>  }

[Severity: High]
Does this change cause a hardware access crash during driver unbind?

On driver unbind, the driver core drops its PM reference by calling
pm_runtime_put_sync() before invoking the driver's remove callback. Since no
children are holding PM references, the device suspends synchronously, and
imx_irqsteer_suspend() disables ipg_clk.

When imx_irqsteer_remove() executes, it writes to CHANMASK hardware registers
with a disabled clock, which can trigger a fatal bus fault:

drivers/irqchip/irq-imx-irqsteer.c:imx_irqsteer_remove() {
	...
	for (i = 0; i < irqsteer_data->reg_num; i++)
		writel_relaxed(0, irqsteer_data->regs +
			       CHANMASK(i, irqsteer_data->reg_num));
	...
}

Additionally, devres teardown runs after remove() in LIFO order and unwinds
devm_clk_get_enabled() by calling clk_disable_unprepare(). This decrements
the already-zero clock enable count, throwing a WARN and corrupting the clock
tree.

Should the driver explicitly wake the device in remove() (e.g., via
pm_runtime_get_sync()) before accessing registers?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260807072346.1222389-1-Zhipeng.wang_1@oss.nxp.com?part=4