[PATCH] genirq: Fix gpio irq will fail to be resend under certain conditions

[PATCH] genirq: Fix gpio irq will fail to be resend under certain conditions

[Zhenze Zhuang]

When a gpio irq is disable and the wakeup function is enable, and
the device enters suspend, the irq wakeup is triggered but then enters
suspend, the IRQS_REPLAY flag will be set, but the IRQS_REPLAY will
not be cleared because the irq_may_run() condition is not met. After
the gpio irq is enabled and the suspend is entered again, after the
gpio irq is triggered, the check_irq_resend() execution will fail
because the IRQS_REPLAY is asserted, resulting in the interrupt not
being resned.

Signed-off-by: Zhenze Zhuang <luuiiruo@163.com>
---
 kernel/irq/chip.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
index dc94e0bf2c94..8800db8d655c 100644
--- a/kernel/irq/chip.c
+++ b/kernel/irq/chip.c
@@ -690,6 +690,8 @@ void handle_fasteoi_irq(struct irq_desc *desc)
 
 	raw_spin_lock(&desc->lock);
 
+	desc->istate &= ~(IRQS_REPLAY | IRQS_WAITING);
+
 	/*
 	 * When an affinity change races with IRQ handling, the next interrupt
 	 * can arrive on the new CPU before the original CPU has completed
@@ -701,8 +703,6 @@ void handle_fasteoi_irq(struct irq_desc *desc)
 		goto out;
 	}
 
-	desc->istate &= ~(IRQS_REPLAY | IRQS_WAITING);
-
 	/*
 	 * If its disabled or no action available
 	 * then mask it and get out of here:
-- 
2.34.1

Re: [PATCH] genirq: Fix gpio irq will fail to be resend under certain conditions

[Thomas Gleixner]

Zhenze!

On Sun, Jun 09 2024 at 00:36, Zhenze Zhuang wrote:

This has nothing to do with GPIO interrupts. It's a general problem
vs. resend, no?

> When a gpio irq is disable and the wakeup function is enable, and
> the device enters suspend, the irq wakeup is triggered but then enters
> suspend, the IRQS_REPLAY flag will be set, but the IRQS_REPLAY will
> not be cleared because the irq_may_run() condition is not met. After
> the gpio irq is enabled and the suspend is entered again, after the
> gpio irq is triggered, the check_irq_resend() execution will fail
> because the IRQS_REPLAY is asserted, resulting in the interrupt not
> being resned.

The concept of separate sentences and paragrahs exists for a reason.

  https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#changelog

No let me break that world salad apart and digest it.

> When a gpio irq is disable and the wakeup function is enable, and
> the device enters suspend, the irq wakeup is triggered but then enters
> suspend, the IRQS_REPLAY flag will be set, but the IRQS_REPLAY will
> not be cleared because the irq_may_run() condition is not met.

So what you are saying is:

An interrupt is disabled, but the wakeup function of the interrupt is
enabled, right?

Now the system enters suspend and the interrupt is raised, which
triggers the wakeup function, right?

Now the system enters suspend nevertheless. How can that happen?

Due to that the IRQS_REPLAY flag will be set. How so? IRQS_REPLAY is
only set from the resend function.

Due to that IRQS_REPLAY will not be cleared because the irq_may_run()
condition is not cleared. Sure, but how does any of this happen in the
first place?

> After the gpio irq is enabled and the suspend is entered again, after
> the gpio irq is triggered, the check_irq_resend() execution will fail
> because the IRQS_REPLAY is asserted, resulting in the interrupt not
> being resned.

This sentence really makes my brain go in circles. None of this change
log makes any sense.

Before you send me a decipherable description of the problem, let me ask
you a few obvious questions:

  1) Why is the interrupt disabled _and_ marked as wakeup interrupt?

  2) If it should still wakeup the system even if disabled, then the
     interrupt chip should have the IRQCHIP_ENABLE_WAKEUP_ON_SUSPEND
     flag set. That's not the case, right?

Thanks,

        tglx

Re:Re: [PATCH] genirq: Fix gpio irq will fail to be resend under certain conditions

[zzz]

tglx,
    I'm sorry for my vague description. First, let me answer your questions.

>This has nothing to do with GPIO interrupts. It's a general problem
>vs. resend, no?
Yes, i think it's a general interrupts problem, but i found it while testing gpio interrupts.

>So what you are saying is:
>An interrupt is disabled, but the wakeup function of the interrupt is
>enabled, right?
Yes.

>Now the system enters suspend and the interrupt is raised, which
>triggers the wakeup function, right?
Yes.

>Now the system enters suspend nevertheless. How can that happen?
Although the device is woken up by gpio irq, it may re-enter suspend after some time.

>Due to that the IRQS_REPLAY flag will be set. How so? IRQS_REPLAY is
>only set from the resend function.
When entering suspend , the path is as follows:
suspend_device_irqs()
    suspend_device_irq()
        __enable_irq()
            irq_startup()
                check_irq_resend()   --IRQS_REPLAY flag is set in this function.

>Due to that IRQS_REPLAY will not be cleared because the irq_may_run()
>condition is not cleared. Sure, but how does any of this happen in the
>first place?
I will show you a complete process.

>Before you send me a decipherable description of the problem, let me ask
>you a few obvious questions:
>  1) Why is the interrupt disabled _and_ marked as wakeup interrupt?
I have a pin named DATA.
When there is no connection, DATA needs to be a recognition function, the 
interrupt enabled and wakeup are used here.
When there is connection, DATA is used communication function, the interrupt 
disabled and wakeup are used here.

>  2) If it should still wakeup the system even if disabled, then the
>     interrupt chip should have the IRQCHIP_ENABLE_WAKEUP_ON_SUSPEND
>     flag set. That's not the case, right?
No, IRQCHIP_ENABLE_WAKEUP_ON_SUSPEND flag will be set.


Second, I wil show you complete process by timeline:

1.device is suspend, irq disable, wakeup enable, every state is normal now;

2.device is suspend, irq disable, wakeup enable, irq raised and trigger wakeup, 
IRQS_PENDING flag will be set;

3.device is active, irq disable, wakeup enable, devices enter suspend afte some 
time, this step will executes check_irq_resend(), IRQS_REPLAY flag will be set. 
Please refer to the paths listed above.
And check_irq_resend() will call handle_fasteoi_irq(), but irq_may_run() return false,
so IRQS_REPLAY flag not be clear.

4.device is suspend, irq disable, wakeup enable, device wakeup again, we set irq 
enable, and let device enter suspend;

5.device is suspend, irq enable, wakeup enable, irq raised and trigger wakeup, will 
executes check_irq_resend(), but IRQS_REPLAY flag is set, so irq thread_fn() will not 
be executes.


Thanks!
zzz