| Message ID | 1441929860-5488-1-git-send-email-alexandra.yates@linux.intel.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
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 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 <alexandra.yates@linux.intel.com> > --- > 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 <alexandra.yates@linux.intel.org> > +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? > +} > + > +void pm_last_wakeup_irq_reset(void) > +{ > + wakeup_irq = 0; > +} 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 -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <alexandra.yates@linux.intel.com> >> --- >> 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 <alexandra.yates@linux.intel.org> >> +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.
On Saturday, September 12, 2015 02:41:31 PM Alexandra Yates wrote: > 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 <alexandra.yates@linux.intel.com> > >> --- > >> 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 <alexandra.yates@linux.intel.org> > >> +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. My point is that you can do if (wakeup_irq == 0) { wakeup_irq = irq_number; pm_system_wakeup(); } instead of calling pm_system_wakeup() unconditionally even if wakeup_irq != 0, can't you? Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09/14/2015 06:56 AM, Rafael J. Wysocki wrote: > On Saturday, September 12, 2015 02:41:31 PM Alexandra Yates wrote: >> 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 <alexandra.yates@linux.intel.com> >>>> --- >>>> 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 <alexandra.yates@linux.intel.org> >>>> +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. > My point is that you can do > > if (wakeup_irq == 0) { > wakeup_irq = irq_number; > pm_system_wakeup(); > } > > instead of calling pm_system_wakeup() unconditionally even if wakeup_irq != 0, > can't you? My concern is how this change would impact any other IRQs that were armed. Apparently from my testing there is no further impact. The patch was sent with version10. In case there are issues patch V09 doesn't include this change. > Thanks, > Rafael > >
On 09/14/2015 06:56 AM, Rafael J. Wysocki wrote: > On Saturday, September 12, 2015 02:41:31 PM Alexandra Yates wrote: >> 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 <alexandra.yates@linux.intel.com> >>>> --- >>>> 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 <alexandra.yates@linux.intel.org> >>>> +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. My concern is how this change would impact any other IRQs that were armed. Apparently from my testing there is no further impact. The patch was sent with version10. In case there are issues patch V09 doesn't include this change. > My point is that you can do > > if (wakeup_irq == 0) { > wakeup_irq = irq_number; > pm_system_wakeup(); > } > > instead of calling pm_system_wakeup() unconditionally even if wakeup_irq != 0, > can't you? > > Thanks, > Rafael > > -- > To unsubscribe from this list: send the line "unsubscribe linux-pm" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
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 +Date: April 2015 +Contact: Alexandra Yates <alexandra.yates@linux.intel.org> +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. + + 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(); +} + +void pm_last_wakeup_irq_reset(void) +{ + wakeup_irq = 0; +} + /** * 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); +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; +} + +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
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 This feature will be useful for system wakeup diagnostics of spurious wakeup interrupts. Signed-off-by: Alexandra Yates <alexandra.yates@linux.intel.com> --- 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(-)