| Message ID | 1556089181-4246-1-git-send-email-rui.zhang@intel.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | PM: wakup: export wakeup irq when system aborts suspend | expand |
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(); > } > } > --
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(); > > } > > } > > --
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.
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
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).
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(); } }