Re: [PATCH] irqchip: imx-irqsteer: Call pm_runtime_disable() on remove

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

Pre-existing issues:
- [High] `imx_irqsteer_remove()` unconditionally calls `clk_disable_unprepare()`, which causes a shared clock double-disable and underflow if the device is already runtime-suspended.
- [High] The chained interrupt handler `imx_irqsteer_irq_handler()` accesses memory-mapped registers without checking the runtime PM state, risking a system panic.
--

--- Patch [1]: [PATCH] irqchip: imx-irqsteer: Call pm_runtime_disable() on remove ---
commit 9f44afe2cbf39115bfc35bfc96ec7644902b8a9a
Author: Fabio Estevam <[email protected]>

irqchip: imx-irqsteer: Call pm_runtime_disable() on remove
    
imx_irqsteer_probe() enables runtime PM, but imx_irqsteer_remove()
does not disable it. This commit adds pm_runtime_disable() when
removing the device to prevent an unbalanced pm_runtime_enable.

Fixes: 4730d2233311 ("irqchip/imx-irqsteer: Add runtime PM support")
Signed-off-by: Fabio Estevam <[email protected]>

> diff --git a/drivers/irqchip/irq-imx-irqsteer.c b/drivers/irqchip/irq-imx-irqsteer.c
> index 87b07f517be35..a695ba31db9a2 100644
> --- a/drivers/irqchip/irq-imx-irqsteer.c
> +++ b/drivers/irqchip/irq-imx-irqsteer.c
> @@ -276,6 +276,8 @@ static void imx_irqsteer_remove(struct platform_device *pdev)
>  	struct irqsteer_data *irqsteer_data = platform_get_drvdata(pdev);
>  	int i;
>  
> +	pm_runtime_disable(&pdev->dev);
> +

[Severity: High]
This isn't a bug introduced by this patch, but this sequence appears to cause
a double-disable of the clock if the device is already runtime-suspended.

If the device is in RPM_SUSPENDED state when unbound via sysfs,
pm_runtime_disable() locks the PM state without resuming the device.

The clk_disable_unprepare() call at the end of imx_irqsteer_remove() is then
executed unconditionally:

drivers/irqchip/irq-imx-irqsteer.c:imx_irqsteer_remove() {
	...
	pm_runtime_disable(&pdev->dev);
	...
	clk_disable_unprepare(irqsteer_data->ipg_clk);
}

Could this decrease the clock enable count twice for a single prepare/enable
pair, stealing references from other active peripherals on the same bus?


[Severity: High]
This is a pre-existing issue, but the chained interrupt handler appears to
access memory-mapped registers without checking the runtime PM state.

If no child interrupts are requested, the device's PM usage count drops to 0,
disabling ipg_clk.

If a spurious interrupt arrives on the parent line,
imx_irqsteer_irq_handler() unconditionally accesses the registers:

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

Accessing these registers with ipg_clk disabled can cause a synchronous
external abort.

Should this handler check the PM state using pm_runtime_get_if_active()
before accessing the hardware?

>  	for (i = 0; i < irqsteer_data->irq_count; i++) {
>  		if (!irqsteer_data->irq[i])
>  			break;

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