From mboxrd@z Thu Jan 1 00:00:00 1970 From: Alexandra Yates Subject: Re: [PATCH V8] Report interrupt that caused system wakeup Date: Sat, 12 Sep 2015 14:41:31 -0700 Message-ID: <55F49C0B.3090903@linux.intel.com> References: <1441929860-5488-1-git-send-email-alexandra.yates@linux.intel.com> <2235143.PitGJtzR6R@vostro.rjw.lan> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Return-path: Received: from mga02.intel.com ([134.134.136.20]:40893 "EHLO mga02.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751914AbbILVj2 (ORCPT ); Sat, 12 Sep 2015 17:39:28 -0400 In-Reply-To: <2235143.PitGJtzR6R@vostro.rjw.lan> Sender: linux-pm-owner@vger.kernel.org List-Id: linux-pm@vger.kernel.org To: "Rafael J. Wysocki" Cc: tglx@linutronix.de, kristen.c.accardi@intel.com, linux-pm@vger.kernel.org Hi Rafael, On 09/11/2015 04:36 PM, Rafael J. Wysocki wrote: > On Thursday, September 10, 2015 05:04:20 PM Alexandra Yates wrote: >> This feature reports the IRQ that was armed for system wakeup after >> the system was last time suspended. It adds a new sysfs attribute >> under /sys/power/ named: pm_last_wakeup_irq Sounds good > What about changing it this way: > > Add a sysfs attribute, /sys/power/pm_wakeup_irq, reporting the IRQ number of > the first wakeup interrupt (that is, the first interrupt from an IRQ line > armed for system wakeup) seen by the kernel during the most recent system > suspend/resume cycle. > >> This feature will be useful for system wakeup diagnostics of >> spurious wakeup interrupts. >> >> Signed-off-by: Alexandra Yates >> --- >> Documentation/ABI/testing/sysfs-power | 11 +++++++++++ >> drivers/base/power/wakeup.c | 12 ++++++++++++ >> include/linux/suspend.h | 8 +++++++- >> kernel/irq/pm.c | 4 ++-- >> kernel/power/main.c | 19 +++++++++++++++++++ >> 5 files changed, 51 insertions(+), 3 deletions(-) >> >> diff --git a/Documentation/ABI/testing/sysfs-power b/Documentation/ABI/testing/sysfs-power >> index f455181..75f27ed 100644 >> --- a/Documentation/ABI/testing/sysfs-power >> +++ b/Documentation/ABI/testing/sysfs-power >> @@ -256,3 +256,14 @@ Description: >> Writing a "1" enables this printing while writing a "0" >> disables it. The default value is "0". Reading from this file >> will display the current value. >> + >> +What: /sys/power/pm_last_wakeup_irq > I'd change that to /sys/power/pm_wakeup_irq > >> +Date: April 2015 >> +Contact: Alexandra Yates >> +Description: >> + The /sys/power/pm_last_wakeup_irq file reports to user space >> + the IRQ that was armed for system wakeup and occurred since >> + suspend_device_irqs() was last called. > And here I'd say: > > The /sys/power/pm_wakeup_irq file reports to user space the IRQ number of > the first wakeup interrupt (that is, the first interrupt from an IRQ line > armed for system wakeup) seen by the kernel during the most recent system > suspend/resume cycle. > >> + >> + This output is useful for system wakeup diagnostics of spurious >> + wakeup interrupts. >> diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c >> index 51f15bc..f663fb1 100644 >> --- a/drivers/base/power/wakeup.c >> +++ b/drivers/base/power/wakeup.c >> @@ -870,6 +870,18 @@ void pm_wakeup_clear(void) >> pm_abort_suspend = false; >> } >> >> +void pm_system_irq_wakeup(unsigned int irq_number) >> +{ >> + if (wakeup_irq == 0) >> + wakeup_irq = irq_number; >> + pm_system_wakeup(); > I'm wondering if we need to call pm_system_wakeup() in the wakeup_irq != 0 case? > > If wakeup_irq != 0, pm_system_wakeup() has been called (at least) once already > and it is not necessary to call it more than once. Or am I missing anything? Here I'm following Thomas advice to create pm_system_irq_wakeup to handle the storage of wakeup_irq I'm simply calling/wrapping pm_system_wakeup() since it was originally called from pm_system_irq_wakeup()with out other additional conditionals. I think doing anything else here may have unwanted results. The core functionality of the code can be changed but I think that should be another patch. one targeted towards performance optimization. Please let me know and I could add this to my to do list. >> +} >> + >> +void pm_last_wakeup_irq_reset(void) >> +{ >> + wakeup_irq = 0; >> +} Will do, good idea. > Why don't you clear wakeup_irq in pm_wakeup_clear()? Can it actually change > between the time when pm_wakeup_clear() is called and when suspend_device_irqs() > runs? > >> + >> /** >> * pm_get_wakeup_count - Read the number of registered wakeup events. >> * @count: Address to store the value at. >> diff --git a/include/linux/suspend.h b/include/linux/suspend.h >> index 5efe743..eb5ad3b 100644 >> --- a/include/linux/suspend.h >> +++ b/include/linux/suspend.h >> @@ -371,6 +371,9 @@ static inline bool hibernation_available(void) { return false; } >> >> extern struct mutex pm_mutex; >> >> +/* IRQ number which causes system wakeup */ >> +extern unsigned int wakeup_irq; >> + >> #ifdef CONFIG_PM_SLEEP >> void save_processor_state(void); >> void restore_processor_state(void); >> @@ -378,7 +381,7 @@ void restore_processor_state(void); >> /* kernel/power/main.c */ >> extern int register_pm_notifier(struct notifier_block *nb); >> extern int unregister_pm_notifier(struct notifier_block *nb); >> - >> +extern void pm_last_wakeup_irq_reset(void); >> #define pm_notifier(fn, pri) { \ >> static struct notifier_block fn##_nb = \ >> { .notifier_call = fn, .priority = pri }; \ >> @@ -391,6 +394,7 @@ extern bool events_check_enabled; >> extern bool pm_wakeup_pending(void); >> extern void pm_system_wakeup(void); >> extern void pm_wakeup_clear(void); >> +extern void pm_system_irq_wakeup(unsigned int); >> extern bool pm_get_wakeup_count(unsigned int *count, bool block); >> extern bool pm_save_wakeup_count(unsigned int count); >> extern void pm_wakep_autosleep_enabled(bool set); >> @@ -440,6 +444,8 @@ static inline int unregister_pm_notifier(struct notifier_block *nb) >> static inline bool pm_wakeup_pending(void) { return false; } >> static inline void pm_system_wakeup(void) {} >> static inline void pm_wakeup_clear(void) {} >> +static inline void pm_system_irq_wakeup(unsigned int) {} >> +static inline void pm_last_wakeup_irq_reset(void){}; >> >> static inline void lock_system_sleep(void) {} >> static inline void unlock_system_sleep(void) {} >> diff --git a/kernel/irq/pm.c b/kernel/irq/pm.c >> index d22786a..18178d7 100644 >> --- a/kernel/irq/pm.c >> +++ b/kernel/irq/pm.c >> @@ -21,7 +21,7 @@ bool irq_pm_check_wakeup(struct irq_desc *desc) >> desc->istate |= IRQS_SUSPENDED | IRQS_PENDING; >> desc->depth++; >> irq_disable(desc); >> - pm_system_wakeup(); >> + pm_system_irq_wakeup(irq_desc_get_irq(desc)); >> return true; >> } >> return false; >> @@ -118,7 +118,7 @@ void suspend_device_irqs(void) >> { >> struct irq_desc *desc; >> int irq; >> - >> + pm_last_wakeup_irq_reset(); >> for_each_irq_desc(irq, desc) { >> unsigned long flags; >> bool sync; >> diff --git a/kernel/power/main.c b/kernel/power/main.c >> index 63d395b..d2bd164 100644 >> --- a/kernel/power/main.c >> +++ b/kernel/power/main.c >> @@ -272,6 +272,24 @@ static inline void pm_print_times_init(void) >> { >> pm_print_times_enabled = !!initcall_debug; >> } >> + >> +unsigned int wakeup_irq; >> +EXPORT_SYMBOL_GPL(wakeup_irq); > It doesn't have to be exported to modules. > >> +static ssize_t pm_last_wakeup_irq_show(struct kobject *kobj, >> + struct kobj_attribute *attr, >> + char *buf) >> +{ >> + return wakeup_irq ? sprintf(buf, "%u\n", wakeup_irq) : -EINVAL; > -EINVAL means "invalid argument", which is not the case here. > > I'd use -ENODATA. > >> +} >> + >> +static ssize_t pm_last_wakeup_irq_store(struct kobject *kobj, >> + struct kobj_attribute *attr, >> + const char *buf, size_t n) >> +{ >> + return -EINVAL; >> +} >> +power_attr(pm_last_wakeup_irq); >> + >> #else /* !CONFIG_PM_SLEEP_DEBUG */ >> static inline void pm_print_times_init(void) {} >> #endif /* CONFIG_PM_SLEEP_DEBUG */ >> @@ -604,6 +622,7 @@ static struct attribute * g[] = { >> #endif >> #ifdef CONFIG_PM_SLEEP_DEBUG >> &pm_print_times_attr.attr, >> + &pm_last_wakeup_irq_attr.attr, >> #endif >> #endif >> #ifdef CONFIG_FREEZER >> > Thanks, > Rafael > > I'm sending patch v9 for your review. -- Thank you,