[PATCH] PM: wakup: export wakeup irq when system aborts suspend

[PATCH] PM: wakup: export wakeup irq when system aborts suspend

[Zhang Rui]

When suspend is aborted because of an ARMED irq, we can not get any
useful information about which irq is triggered.

Export the wakeup irq via suspend_resume trace event in this case.

Signed-off-by: Brandt, Todd E <todd.e.brandt@intel.com>
Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
 drivers/base/power/wakeup.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
index bb1ae17..d32bd91 100644
--- a/drivers/base/power/wakeup.c
+++ b/drivers/base/power/wakeup.c
@@ -875,6 +875,7 @@ void pm_system_irq_wakeup(unsigned int irq_number)
 {
 	if (pm_wakeup_irq == 0) {
 		pm_wakeup_irq = irq_number;
+		trace_suspend_resume(TPS("irq_wakeup"), irq_number, true);
 		pm_system_wakeup();
 	}
 }
-- 
2.7.4

Re: [PATCH] PM: wakup: export wakeup irq when system aborts suspend

[Rafael J. Wysocki]

On Wed, Apr 24, 2019 at 8:59 AM Zhang Rui <rui.zhang@intel.com> wrote:
>
> When suspend is aborted because of an ARMED irq, we can not get any
> useful information about which irq is triggered.

That should be provided via the /sys/power/pm_wakeup_irq attribute.

> Export the wakeup irq via suspend_resume trace event in this case.
>
> Signed-off-by: Brandt, Todd E <todd.e.brandt@intel.com>
> Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> ---
>  drivers/base/power/wakeup.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
> index bb1ae17..d32bd91 100644
> --- a/drivers/base/power/wakeup.c
> +++ b/drivers/base/power/wakeup.c
> @@ -875,6 +875,7 @@ void pm_system_irq_wakeup(unsigned int irq_number)
>  {
>         if (pm_wakeup_irq == 0) {
>                 pm_wakeup_irq = irq_number;
> +               trace_suspend_resume(TPS("irq_wakeup"), irq_number, true);

pm_wakeup_irq can be read from /sys/power/pm_wakeup_irq, so why is it
necessary to trace it additionally?

>                 pm_system_wakeup();
>         }
>  }
> --

Re: [PATCH] PM: wakup: export wakeup irq when system aborts suspend

[Zhang Rui]

On 三, 2019-04-24 at 09:29 +0200, Rafael J. Wysocki wrote:
> On Wed, Apr 24, 2019 at 8:59 AM Zhang Rui <rui.zhang@intel.com>
> wrote:
> > 
> > 
> > When suspend is aborted because of an ARMED irq, we can not get any
> > useful information about which irq is triggered.
> That should be provided via the /sys/power/pm_wakeup_irq attribute.
> 
another case is that,
say we got spurious wakeup interrupts in the idle loop, and then waked
by a real wakeup interrupt, say, rtc.
/sys/power/pm_wakeup_irq only shows the latest one, and we can not find
out the spurious wakeup interrupts.

I should add this in the changelog as well.

thanks,
rui

> > 
> > Export the wakeup irq via suspend_resume trace event in this case.
> > 
> > Signed-off-by: Brandt, Todd E <todd.e.brandt@intel.com>
> > Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> > ---
> >  drivers/base/power/wakeup.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/base/power/wakeup.c
> > b/drivers/base/power/wakeup.c
> > index bb1ae17..d32bd91 100644
> > --- a/drivers/base/power/wakeup.c
> > +++ b/drivers/base/power/wakeup.c
> > @@ -875,6 +875,7 @@ void pm_system_irq_wakeup(unsigned int
> > irq_number)
> >  {
> >         if (pm_wakeup_irq == 0) {
> >                 pm_wakeup_irq = irq_number;
> > +               trace_suspend_resume(TPS("irq_wakeup"), irq_number,
> > true);
> pm_wakeup_irq can be read from /sys/power/pm_wakeup_irq, so why is it
> necessary to trace it additionally?
> 
> > 
> >                 pm_system_wakeup();
> >         }
> >  }
> > --

Re: [PATCH] PM: wakup: export wakeup irq when system aborts suspend

[Rafael J. Wysocki]

On Wed, Apr 24, 2019 at 9:46 AM Zhang Rui <rui.zhang@intel.com> wrote:
>
> On 三, 2019-04-24 at 09:29 +0200, Rafael J. Wysocki wrote:
> > On Wed, Apr 24, 2019 at 8:59 AM Zhang Rui <rui.zhang@intel.com>
> > wrote:
> > >
> > >
> > > When suspend is aborted because of an ARMED irq, we can not get any
> > > useful information about which irq is triggered.
> > That should be provided via the /sys/power/pm_wakeup_irq attribute.
> >
> another case is that,
> say we got spurious wakeup interrupts in the idle loop, and then waked
> by a real wakeup interrupt, say, rtc.
> /sys/power/pm_wakeup_irq only shows the latest one,

No it doesn't, it shows the *first* one.

Please look at the code: pm_wakeup_irq is only set when it is zero
which is unless it has been set before.

> and we can not find out the spurious wakeup interrupts.

The wakeup interrupts that have occurred after the first one are not
recorded, but your patch doesn't change that.

Your patch causes pm_wakeup_irq to be added to the trace only when it
is set, but that is the /sys/power/pm_wakeup_irq which is my point.

Re: [PATCH] PM: wakup: export wakeup irq when system aborts suspend

[Zhang Rui]

On 三, 2019-04-24 at 10:32 +0200, Rafael J. Wysocki wrote:
> On Wed, Apr 24, 2019 at 9:46 AM Zhang Rui <rui.zhang@intel.com>
> wrote:
> > 
> > 
> > On 三, 2019-04-24 at 09:29 +0200, Rafael J. Wysocki wrote:
> > > 
> > > On Wed, Apr 24, 2019 at 8:59 AM Zhang Rui <rui.zhang@intel.com>
> > > wrote:
> > > > 
> > > > 
> > > > 
> > > > When suspend is aborted because of an ARMED irq, we can not get
> > > > any
> > > > useful information about which irq is triggered.
> > > That should be provided via the /sys/power/pm_wakeup_irq
> > > attribute.
> > > 
> > another case is that,
> > say we got spurious wakeup interrupts in the idle loop, and then
> > waked
> > by a real wakeup interrupt, say, rtc.
> > /sys/power/pm_wakeup_irq only shows the latest one,
> No it doesn't, it shows the *first* one.
> 
> Please look at the code: pm_wakeup_irq is only set when it is zero
> which is unless it has been set before.
> 

hmmm, by reading the code, pm_wakeup_clear() clears pm_wakeup_irq, and it is invoked inside the s2idle loop. So I think pm_wakeup_irq is cleared and set again if there is a second spurious interrupt.

> > and we can not find out the spurious wakeup interrupts.
> The wakeup interrupts that have occurred after the first one are not
> recorded, but your patch doesn't change that.
> 
> Your patch causes pm_wakeup_irq to be added to the trace only when it
> is set, but that is the /sys/power/pm_wakeup_irq which is my point.

yes, the first case (suspend aborted) can be covered
by /sys/power/pm_wakeup_irq.
For the spurious wakeup case, I think I observed the behavior before,
e.g, pm_wakeup_irq keeps on showing 9 because of spurious GPE, and then
 returns 8 because of RTC wakeup event. Anyway, let me do some
experimental to confirm and get back to you then.

thanks,
rui

Re: [PATCH] PM: wakup: export wakeup irq when system aborts suspend

[Rafael J. Wysocki]

On Wed, Apr 24, 2019 at 10:58 AM Zhang Rui <rui.zhang@intel.com> wrote:
>
> On 三, 2019-04-24 at 10:32 +0200, Rafael J. Wysocki wrote:
> > On Wed, Apr 24, 2019 at 9:46 AM Zhang Rui <rui.zhang@intel.com>
> > wrote:
> > >
> > >
> > > On 三, 2019-04-24 at 09:29 +0200, Rafael J. Wysocki wrote:
> > > >
> > > > On Wed, Apr 24, 2019 at 8:59 AM Zhang Rui <rui.zhang@intel.com>
> > > > wrote:
> > > > >
> > > > >
> > > > >
> > > > > When suspend is aborted because of an ARMED irq, we can not get
> > > > > any
> > > > > useful information about which irq is triggered.
> > > > That should be provided via the /sys/power/pm_wakeup_irq
> > > > attribute.
> > > >
> > > another case is that,
> > > say we got spurious wakeup interrupts in the idle loop, and then
> > > waked
> > > by a real wakeup interrupt, say, rtc.
> > > /sys/power/pm_wakeup_irq only shows the latest one,
> > No it doesn't, it shows the *first* one.
> >
> > Please look at the code: pm_wakeup_irq is only set when it is zero
> > which is unless it has been set before.
> >
>
> hmmm, by reading the code, pm_wakeup_clear() clears pm_wakeup_irq, and it is invoked inside the s2idle loop. So I think pm_wakeup_irq is cleared and set again if there is a second spurious interrupt.

If that is a concern, I would suggest adding a debug printout to
pm_wakeup_clear() to print the current value of pm_wakeup_irq before
clearing it (if not zero already).