Re: [PATCH 2/4] mfd: rk8xx-core: keep the PMIC interrupt disabled during system suspend

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] mfd: rk8xx-core: disable_irq() in rk8xx_suspend() breaks s2idle wakeup
--

commit 1642fdff105aee2817fbf318588a972d4d3d49a4
Author: Jakob Unterwurzacher <jakob.unterwurzacher@cherry.de>

mfd: rk8xx-core: keep the PMIC interrupt disabled during system suspend

This commit disables the PMIC interrupt at the end of rk8xx_suspend() and
re-enables it at the start of rk8xx_resume() so that wakeup interrupts are
not handled before the parent bus controller has resumed.

> diff --git a/drivers/mfd/rk8xx-core.c b/drivers/mfd/rk8xx-core.c
> index 3dcf6abfda74f..3a68a7974ed06 100644
> --- a/drivers/mfd/rk8xx-core.c
> +++ b/drivers/mfd/rk8xx-core.c

[ ... ]

> @@ -929,7 +931,20 @@ int rk8xx_suspend(struct device *dev)
>  		break;
>  	}
>  
> -	return ret;
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Keep the interrupt disabled while the bus to the PMIC may be
> +	 * suspended: otherwise a wakeup event (e.g. the power key) is handled
> +	 * as soon as interrupts are re-enabled on resume, before the bus has
> +	 * resumed, and reading/acking the interrupt status fails. If the
> +	 * interrupt is a wakeup source, it stays armed, and the pending event
> +	 * is replayed by enable_irq() on resume.
> +	 */
> +	disable_irq(rk808->irq);

[Severity: High]
Does calling disable_irq(rk808->irq) in rk8xx_suspend() break wakeup from
suspend-to-idle (s2idle) on Rockchip platforms?

On Rockchip SoCs, the PMIC interrupt parent is the Rockchip GPIO irqchip in
drivers/gpio/gpio-rockchip.c, which implements rockchip_irq_disable() to mask
the pin in hardware and does not set the irqchip wakeup-on-suspend flag:

drivers/gpio/gpio-rockchip.c:rockchip_irq_disable() {
	irq_gc_mask_set_bit(d);
}

When disable_irq(rk808->irq) runs during dpm_suspend() -> rk8xx_suspend(),
it invokes __disable_irq() -> irq_disable() -> __irq_disable() ->
rockchip_irq_disable(), masking the interrupt line in the GPIO int_mask
register and marking the descriptor disabled.

Later in dpm_suspend_noirq() -> suspend_device_irqs(),
kernel/irq/pm.c:suspend_device_irq() only re-enables a disabled wakeup
interrupt in the irqchip if that flag is set on the chip:

kernel/irq/pm.c:suspend_device_irq() {
	if (irqd_is_wakeup_set(irqd)) {
		irqd_set(irqd, IRQD_WAKEUP_ARMED);

		if ((chipflags & IRQCHIP_ENABLE_WAKEUP_ON_SUSPEND) &&
		     irqd_irq_disabled(irqd)) {
			__enable_irq(desc);
			irqd_set(irqd, IRQD_IRQ_ENABLED_ON_SUSPEND);
		}
}

During suspend-to-RAM, syscore_suspend() subsequently runs irq_gc_suspend() ->
rockchip_irq_suspend(), which writes ~gc->wake_active to int_mask:

drivers/gpio/gpio-rockchip.c:rockchip_irq_suspend() {
	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
	struct rockchip_pin_bank *bank = gc->private;

	bank->saved_masks = irq_reg_readl(gc, bank->gpio_regs->int_mask);
	irq_reg_writel(gc, ~gc->wake_active, bank->gpio_regs->int_mask);
}

However, during suspend-to-idle, kernel/power/suspend.c:suspend_enter() enters
s2idle_loop() and jumps to Platform_wake without calling syscore_suspend():

kernel/power/suspend.c:suspend_enter() {
	if (state == PM_SUSPEND_TO_IDLE) {
		s2idle_loop();
		goto Platform_wake;
	}
}

Wouldn't that leave rk808->irq masked in the GPIO hardware throughout
s2idle_loop(), preventing the PMIC power key or RTC alarm from waking the
system from s2idle?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261029-dev-tiger-v7-4-pre-20261028-tiger-suspend-b4-v1-0-cb12621c1fb4@cherry.de?part=2