genirq: mixing IRQF_NO_SUSPEND and wakeup sources on shared IRQs
cover genirq: mixing IRQF_NO_SUSPEND and wakeup sources on shared IRQs
Hello,
I put the IRQF_NO_SUSPEND_SAFE/IRQF_TIMER_SIBLING_OK/WHATEVER_NAME_YOU_CHOOSE debate aside to concentrate on another problem pointed out by Rafael and
Mark: the fact that we cannot mix IRQF_NO_SUSPEND and wakeup sources on a shared IRQ line.
This is because the wakeup code is prevailing the IRQF_NO_SUSPEND case and will trigger a system wakeup as soon as the IRQ line is tagged as a wakeup source.
This series propose an approach to deal with such cases by doing the following:
1/ Prevent any system wakeup when at least one of the IRQ user has set
the IRQF_NO_SUSPEND flag
2/ Adapt IRQ handlers so that they can safely be called in suspended
state
3/ Let drivers decide when the system should be woken up
Let me know what you think of this approach.
Thanks,
Boris
Boris Brezillon (3):
genirq: prevent system wakeup when dealing with IRQF_NO_SUSPEND IRQs
genirq: add helper functions to deal with wakeup on shared
IRQF_NO_SUSPEND IRQs
rtc: at91sam9: properly act when IRQ handler is called in suspended
state
drivers/rtc/rtc-at91sam9.c | 62 ++++++++++++++++++++++++++++++++++++++-------- include/linux/interrupt.h | 3 +++ kernel/irq/manage.c | 16 ++++++++++++ kernel/irq/pm.c | 9 ++++++- 4 files changed, 78 insertions(+), 12 deletions(-)
Signature · 1 line
1.9.1
RFC PATCH 1/3 genirq: prevent system wakeup when dealing with IRQF_NO_SUSPEND IRQs
Mixing IRQF_NO_SUSPEND and !IRQF_NO_SUSPEND on the same IRQ line is highly discouraged, but in some cases (IRQ shared by a timer and other devices) you don't have any other choice.
Since some devices sharing the IRQ line might tag it as a wakeup source, you might end up with your handler that requested IRQF_NO_SUSPEND not being called in suspended state, or invalid system wakeup (the system is woken up without any wakeup source actually requesting it).
To deal with such unlikely situations, you'll have to:
1/ prevent any automatic wakeup when at least one of the IRQ users
registered with IRQF_NO_SUSPEND
2/ let IRQ users decide if/when they should wake the system up
This patch is taking care of 1.
--- kernel/irq/pm.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
kernel/irq/pm.c +2 -1
@@ -16,7 +16,8 @@bool irq_pm_check_wakeup (struct irq_desc *desc) { -if (irqd_is_wakeup_armed (&desc->irq_data)) { +if (irqd_is_wakeup_armed (&desc->irq_data) && + !desc->no_suspend_depth) {irqd_clear (&desc->irq_data,IRQD_WAKEUP_ARMED ); desc->istate |=IRQS_SUSPENDED |IRQS_PENDING ; desc->depth++;
Signature · 1 line
1.9.1
Re: [RFC PATCH 1/3] genirq: prevent system wakeup when dealing with IRQF_NO_SUSPEND IRQs
Quoted 26 lines · Boris Brezillon
Mixing
IRQF_NO_SUSPENDand !IRQF_NO_SUSPEND on the same IRQ line is highly discouraged, but in some cases (IRQ shared by a timer and other devices) you don't have any other choice.
Since some devices sharing the IRQ line might tag it as a wakeup source, you might end up with your handler that requestedIRQF_NO_SUSPENDnot being called in suspended state, or invalid system wakeup (the system is woken up without any wakeup source actually requesting it).To deal with such unlikely situations, you'll have to:
1/ prevent any automatic wakeup when at least one of the IRQ users
registered withIRQF_NO_SUSPEND
2/ let IRQ users decide if/when they should wake the system upThis patch is taking care of 1.
Signed-off-by Boris Brezillon <[email protected]>--- kernel/irq/pm.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)kernel/irq/pm.c +0
-0
kernel/irq/pm.c line 23
Quoted 9 lines
kernel/irq/pm.c +2
-1@@ -16,7 +16,8 @@bool irq_pm_check_wakeup (struct irq_desc *desc) { -if (irqd_is_wakeup_armed (&desc->irq_data)) { +if (irqd_is_wakeup_armed (&desc->irq_data) && + !desc->no_suspend_depth) {irqd_clear (&desc->irq_data,IRQD_WAKEUP_ARMED ); desc->istate |=IRQS_SUSPENDED |IRQS_PENDING ; desc->depth++;
I'm not sure how this helps, because irqd_is_wakeup_armed() is false for IRQs having no_suspend_depth different from zero (please see the first check in suspend_device_irq()).
Signature · 2 lines
I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center.
Re: [RFC PATCH 1/3] genirq: prevent system wakeup when dealing with IRQF_NO_SUSPEND IRQs
Quoted 27 lines · On Wed, 25 Feb 2015 23:01:31 +0100 "Rafael J. Wysocki" <[email protected]>…
Quoted 26 lines · Boris Brezillon
Mixing
IRQF_NO_SUSPENDand !IRQF_NO_SUSPEND on the same IRQ line is highly discouraged, but in some cases (IRQ shared by a timer and other devices) you don't have any other choice.
Since some devices sharing the IRQ line might tag it as a wakeup source, you might end up with your handler that requestedIRQF_NO_SUSPENDnot being called in suspended state, or invalid system wakeup (the system is woken up without any wakeup source actually requesting it).To deal with such unlikely situations, you'll have to:
1/ prevent any automatic wakeup when at least one of the IRQ users
registered withIRQF_NO_SUSPEND
2/ let IRQ users decide if/when they should wake the system upThis patch is taking care of 1.
Signed-off-by Boris Brezillon <[email protected]>--- kernel/irq/pm.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)kernel/irq/pm.c +0
-0@@ -16,7 +16,8 @@
Quoted 15 lines
kernel/irq/pm.c line 23
Quoted 9 lines
kernel/irq/pm.c +2
-1bool irq_pm_check_wakeup (struct irq_desc *desc) { -if (irqd_is_wakeup_armed (&desc->irq_data)) { +if (irqd_is_wakeup_armed (&desc->irq_data) && + !desc->no_suspend_depth) {irqd_clear (&desc->irq_data,IRQD_WAKEUP_ARMED ); desc->istate |=IRQS_SUSPENDED |IRQS_PENDING ; desc->depth++;I'm not sure how this helps, because
irqd_is_wakeup_armed()is false for IRQs havingno_suspend_depthdifferent from zero (please see the first check insuspend_device_irq()).
Indeed, it seems I overlooked this test in suspend_device_irq, and this makes my irq_is_wakeup_armed test useless.
Signature · 3 lines
Boris Brezillon, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com
RFC PATCH 2/3 genirq: add helper functions to deal with wakeup on shared IRQF_NO_SUSPEND IRQs
In reply to Boris BrezillonAdd two helper functions to help drivers that are sharing IRQs with timer devices (or other devices setting the IRQF_NO_SUSPEND flag) deal with system wakeup and state detection.
Such drivers should expect their IRQ handler to be called in 2 different contexts:
1/ the system is resumed and the handler should act normally 2/ the system is suspended and the handler should wake it up, and
potentially save the received events so that they could be taken care of when the resume callback is called
irq_is_wakeup_armed provides mean to test the current state (true means the system is suspended).
irq_pm_force_wakeup provides mean to force a system wakeup.
--- include/linux/interrupt.h | 3 +++ kernel/irq/manage.c | 16 ++++++++++++++++ kernel/irq/pm.c | 6 ++++++ 3 files changed, 25 insertions(+)
include/linux/interrupt.h +3 -0
@@ -356,6 +356,9 @@ static inline int disable_irq_wake(unsigned int irq)return irq_set_irq_wake (irq,0 ); } +bool irq_is_wakeup_armed (unsigned int irq); + +void irq_pm_force_wakeup (void );#ifdef CONFIG_IRQ_FORCED_THREADING extern bool force_irqthreads;
kernel/irq/manage.c +16 -0
@@ -551,6 +551,22 @@ int irq_set_irq_wake(unsigned int irq, unsigned int on) }EXPORT_SYMBOL (irq_set_irq_wake); +bool irq_is_wakeup_armed (unsigned int irq) +{ +unsigned long flags; +struct irq_desc *desc =irq_get_desc_buslock (irq, &flags,IRQ_GET_DESC_CHECK_GLOBAL ); +bool ret; + +if (!desc) +return false; + + ret =irqd_is_wakeup_armed (&desc->irq_data); + +irq_put_desc_busunlock (desc, flags); +return ret; +} +EXPORT_SYMBOL (irq_is_wakeup_armed); +/* * Internal function that tells the architecture code whether a * particular irq has been exclusively allocated or is available
kernel/irq/pm.c +6 -0
@@ -28,6 +28,12 @@ bool irq_pm_check_wakeup(struct irq_desc *desc)return false; } +void irq_pm_force_wakeup (void ) +{ +pm_system_wakeup (); +} +EXPORT_SYMBOL_GPL (irq_pm_force_wakeup); +/* * Called from __setup_irq() with desc->lock held after @action has * been installed in the action chain.
Signature · 1 line
1.9.1
Re: [RFC PATCH 2/3] genirq: add helper functions to deal with wakeup on shared IRQF_NO_SUSPEND IRQs
Quoted 50 lines · Boris Brezillon
Add two helper functions to help drivers that are sharing IRQs with timer devices (or other devices setting the
IRQF_NO_SUSPENDflag) deal with system wakeup and state detection.Such drivers should expect their IRQ handler to be called in 2 different contexts:
1/ the system is resumed and the handler should act normally 2/ the system is suspended and the handler should wake it up, and
potentially save the received events so that they could be taken care of when the resume callback is called
irq_is_wakeup_armedprovides mean to test the current state (true means the system is suspended).
irq_pm_force_wakeupprovides mean to force a system wakeup.Signed-off-by Boris Brezillon <[email protected]>--- include/linux/interrupt.h | 3 +++ kernel/irq/manage.c | 16 ++++++++++++++++ kernel/irq/pm.c | 6 ++++++ 3 files changed, 25 insertions(+)include/linux/interrupt.h +3
-0@@ -356,6 +356,9 @@ static inline int disable_irq_wake(unsigned int irq)return irq_set_irq_wake (irq,0 ); } +bool irq_is_wakeup_armed (unsigned int irq); + +void irq_pm_force_wakeup (void );#ifdef CONFIG_IRQ_FORCED_THREADING extern bool force_irqthreads;kernel/irq/manage.c +5
-0@@ -551,6 +551,22 @@ int irq_set_irq_wake(unsigned int irq, unsigned int on) }EXPORT_SYMBOL (irq_set_irq_wake); +bool irq_is_wakeup_armed (unsigned int irq) +{ +unsigned long flags; +struct irq_desc *desc =irq_get_desc_buslock (irq, &flags,IRQ_GET_DESC_CHECK_GLOBAL ); +bool ret;
kernel/irq/manage.c int irq_set_irq_wake(unsigned int irq, unsigned int on) · line 568
Quoted 10 lines
kernel/irq/manage.c +10
-0+ +if (!desc) +return false; + + ret =irqd_is_wakeup_armed (&desc->irq_data); + +irq_put_desc_busunlock (desc, flags); +return ret; +} +EXPORT_SYMBOL (irq_is_wakeup_armed);
Ugly that is ...
Quoted 9 lines
kernel/irq/manage.c +1
-0+/* * Internal function that tells the architecture code whether a * particular irq has been exclusively allocated or is available kernel/irq/pm.c +0
-0
kernel/irq/pm.c bool irq_pm_check_wakeup(struct irq_desc *desc) · line 35
Quoted 8 lines
kernel/irq/pm.c +5
-0@@ -28,6 +28,12 @@ bool irq_pm_check_wakeup(struct irq_desc *desc)return false; } +void irq_pm_force_wakeup (void ) +{ +pm_system_wakeup (); +} +EXPORT_SYMBOL_GPL (irq_pm_force_wakeup);
Why don't you export pm_system_wakeup() instead and use it directly?
Quoted 5 lines
kernel/irq/pm.c +1
-0+/* * Called from __setup_irq() with desc->lock held after @action has * been installed in the action chain.
Signature · 2 lines
I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center.
Re: [RFC PATCH 2/3] genirq: add helper functions to deal with wakeup on shared IRQF_NO_SUSPEND IRQs
Quoted 55 lines · On Wed, 25 Feb 2015 23:03:14 +0100 "Rafael J. Wysocki" <[email protected]>…
Quoted 54 lines · Boris Brezillon
Add two helper functions to help drivers that are sharing IRQs with timer devices (or other devices setting the
IRQF_NO_SUSPENDflag) deal with system wakeup and state detection.Such drivers should expect their IRQ handler to be called in 2 different contexts:
1/ the system is resumed and the handler should act normally 2/ the system is suspended and the handler should wake it up, and
potentially save the received events so that they could be taken care of when the resume callback is called
irq_is_wakeup_armedprovides mean to test the current state (true means the system is suspended).
irq_pm_force_wakeupprovides mean to force a system wakeup.Signed-off-by Boris Brezillon <[email protected]>--- include/linux/interrupt.h | 3 +++ kernel/irq/manage.c | 16 ++++++++++++++++ kernel/irq/pm.c | 6 ++++++ 3 files changed, 25 insertions(+)include/linux/interrupt.h +3
-0@@ -356,6 +356,9 @@ static inline int disable_irq_wake(unsigned int irq)return irq_set_irq_wake (irq,0 ); } +bool irq_is_wakeup_armed (unsigned int irq); + +void irq_pm_force_wakeup (void );#ifdef CONFIG_IRQ_FORCED_THREADING extern bool force_irqthreads;kernel/irq/manage.c +9
-0@@ -551,6 +551,22 @@ int irq_set_irq_wake(unsigned int irq, unsigned int on) }EXPORT_SYMBOL (irq_set_irq_wake); +bool irq_is_wakeup_armed (unsigned int irq) +{ +unsigned long flags; +struct irq_desc *desc =irq_get_desc_buslock (irq, &flags,IRQ_GET_DESC_CHECK_GLOBAL ); +bool ret; + +if (!desc) +return false; +
Quoted 8 lines
kernel/irq/manage.c int irq_set_irq_wake(unsigned int irq, unsigned int on) · line 568
Quoted 6 lines
kernel/irq/manage.c +6
-0+ ret =irqd_is_wakeup_armed (&desc->irq_data); + +irq_put_desc_busunlock (desc, flags); +return ret; +} +EXPORT_SYMBOL (irq_is_wakeup_armed);Ugly that is ...
Yes, I'm not proud of it, but I wasn't sure when the handler was supposed to start acting as a 'suspended' handler, and I thought testing for this WAKEUP_ARMED bit was safer.
Anyway, as you pointed out in your previous answer, this bit is not set when the IRQ line is not suspended, so this function is pretty much useless.
Quoted 13 lines
Quoted 13 lines
kernel/irq/manage.c +1
-0+/* * Internal function that tells the architecture code whether a * particular irq has been exclusively allocated or is available kernel/irq/pm.c +0
-0@@ -28,6 +28,12 @@ bool irq_pm_check_wakeup(struct irq_desc *desc)return false; }
Quoted 7 lines
kernel/irq/pm.c bool irq_pm_check_wakeup(struct irq_desc *desc) · line 35
Quoted 5 lines
kernel/irq/pm.c +5
-0+void irq_pm_force_wakeup (void ) +{ +pm_system_wakeup (); +} +EXPORT_SYMBOL_GPL (irq_pm_force_wakeup);Why don't you export
pm_system_wakeup()instead and use it directly?
Yep, I wasn't sure if pm_system_wakeup was reserved for core kernel parts.
I don't have any concern exporting pm_system_wakeup instead.
Signature · 3 lines
Boris Brezillon, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com
RFC PATCH 3/3 rtc: at91sam9: properly act when IRQ handler is called in suspended state
In reply to Boris BrezillonThe IRQ line used by the RTC device is often shared with the system timer (PIT) on at91 platforms.
Since timers are registering their handlers with IRQF_NO_SUSPEND, we should expect being called in suspended state, and properly wake the system up when this is the case.
--- drivers/rtc/rtc-at91sam9.c | 62 ++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 51 insertions(+), 11 deletions(-)
drivers/rtc/rtc-at91sam9.c +51 -11
@@ -77,6 +77,8 @@ struct sam9_rtc {unsigned int gpbr_offset;int irq;struct clk *sclk; +unsigned long events; +spinlock_t lock; };#define rtt_readl (rtc, field) \
@@ -271,14 +273,9 @@ static int at91_rtc_proc(struct device *dev, struct seq_file *seq)return 0 ; } -/* -* IRQ handler for the RTC -*/ -static irqreturn_t at91_rtc_interrupt (int irq,void *_rtc) +static irqreturn_t at91_rtc_cache_events (struct sam9_rtc *rtc) { -struct sam9_rtc *rtc = _rtc;u32 sr, mr; -unsigned long events =0 ;/* Shared interrupt may be for another device. Note: reading * SR clears it, so we must only read it in this irq handler!
@@ -290,18 +287,54 @@ static irqreturn_t at91_rtc_interrupt(int irq, void *_rtc)/* alarm status */ if (sr &AT91_RTT_ALMS ) - events |= (RTC_AF |RTC_IRQF ); + rtc->events |= (RTC_AF |RTC_IRQF );/* timer update/increment */ if (sr &AT91_RTT_RTTINC ) - events |= (RTC_UF |RTC_IRQF ); + rtc->events |= (RTC_UF |RTC_IRQF ); + +return IRQ_HANDLED ; +} + +static void at91_rtc_flush_events (struct sam9_rtc *rtc) +{ +if (!rtc->events) +return ; -rtc_update_irq (rtc->rtcdev,1 , events); +rtc_update_irq (rtc->rtcdev,1 , rtc->events); + rtc->events =0 ;pr_debug ("%s: num=%ld, events=0x%02lx\n" , __func__, - events >>8 , events &0x000000FF ); + rtc->events >>8 , rtc->events &0x000000FF ); +} -return IRQ_HANDLED ; +/* +* IRQ handler for the RTC +*/ +static irqreturn_t at91_rtc_interrupt (int irq,void *_rtc) +{ +struct sam9_rtc *rtc = _rtc; +int ret; + +spin_lock (&rtc->lock); + + ret =at91_rtc_cache_events (rtc); + +/* We're called in suspended state */ +if (irq_is_wakeup_armed (irq)) { +/* Mask irqs coming from this peripheral */ +rtt_writel (rtc,MR , +rtt_readl (rtc,MR ) & + ~(AT91_RTT_ALMIEN |AT91_RTT_RTTINCIEN )); +/* Trigger a system wakeup */ +irq_pm_force_wakeup (); + }else { +at91_rtc_flush_events (rtc); + } + +spin_unlock (&rtc->lock); + +return ret; }static const struct rtc_class_ops at91_rtc_ops = {
@@ -499,10 +532,17 @@ static int at91_rtc_resume(struct device *dev)u32 mr;if (rtc->imr) { +unsigned long flags; +if (device_may_wakeup (dev))disable_irq_wake (rtc->irq); mr =rtt_readl (rtc,MR );rtt_writel (rtc,MR , mr | rtc->imr); + +spin_lock_irqsave (&rtc->lock, flags); +at91_rtc_cache_events (rtc); +at91_rtc_flush_events (rtc); +spin_unlock_irqrestore (&rtc->lock, flags); }return 0 ;
Signature · 1 line
1.9.1
Re: [RFC PATCH 3/3] rtc: at91sam9: properly act when IRQ handler is called in suspended state
Quoted 74 lines · Boris Brezillon
The IRQ line used by the RTC device is often shared with the system timer (PIT) on at91 platforms.
Since timers are registering their handlers withIRQF_NO_SUSPEND, we should expect being called in suspended state, and properly wake the system up when this is the case.Signed-off-by Boris Brezillon <[email protected]>--- drivers/rtc/rtc-at91sam9.c | 62 ++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 51 insertions(+), 11 deletions(-)drivers/rtc/rtc-at91sam9.c +21
-11@@ -77,6 +77,8 @@ struct sam9_rtc {unsigned int gpbr_offset;int irq;struct clk *sclk; +unsigned long events; +spinlock_t lock; };#define rtt_readl (rtc, field) \@@ -271,14 +273,9 @@ static int at91_rtc_proc(struct device *dev, struct seq_file *seq)return 0 ; } -/* -* IRQ handler for the RTC -*/ -static irqreturn_t at91_rtc_interrupt (int irq,void *_rtc) +static irqreturn_t at91_rtc_cache_events (struct sam9_rtc *rtc) { -struct sam9_rtc *rtc = _rtc;u32 sr, mr; -unsigned long events =0 ;/* Shared interrupt may be for another device. Note: reading * SR clears it, so we must only read it in this irq handler! @@ -290,18 +287,54 @@ static irqreturn_t at91_rtc_interrupt(int irq, void *_rtc)/* alarm status */ if (sr &AT91_RTT_ALMS ) - events |= (RTC_AF |RTC_IRQF ); + rtc->events |= (RTC_AF |RTC_IRQF );/* timer update/increment */ if (sr &AT91_RTT_RTTINC ) - events |= (RTC_UF |RTC_IRQF ); + rtc->events |= (RTC_UF |RTC_IRQF ); + +return IRQ_HANDLED ; +} + +static void at91_rtc_flush_events (struct sam9_rtc *rtc) +{ +if (!rtc->events) +return ; -rtc_update_irq (rtc->rtcdev,1 , events); +rtc_update_irq (rtc->rtcdev,1 , rtc->events); + rtc->events =0 ;pr_debug ("%s: num=%ld, events=0x%02lx\n" , __func__, - events >>8 , events &0x000000FF ); + rtc->events >>8 , rtc->events &0x000000FF ); +} -return IRQ_HANDLED ; +/* +* IRQ handler for the RTC +*/ +static irqreturn_t at91_rtc_interrupt (int irq,void *_rtc)
drivers/rtc/rtc-at91sam9.c static irqreturn_t at91_rtc_interrupt(int irq, void *_rtc) · line 324
Quoted 10 lines
drivers/rtc/rtc-at91sam9.c +10
-0+{ +struct sam9_rtc *rtc = _rtc; +int ret; + +spin_lock (&rtc->lock); + + ret =at91_rtc_cache_events (rtc); + +/* We're called in suspended state */ +if (irq_is_wakeup_armed (irq)) {
Instead of doing this, I would set a flag in the driver's ->suspend callback (or in ->suspend_late, whichever is more convenient) and check that flag here.
Quoted 35 lines
drivers/rtc/rtc-at91sam9.c +20
-0+/* Mask irqs coming from this peripheral */ +rtt_writel (rtc,MR , +rtt_readl (rtc,MR ) & + ~(AT91_RTT_ALMIEN |AT91_RTT_RTTINCIEN )); +/* Trigger a system wakeup */ +irq_pm_force_wakeup (); + }else { +at91_rtc_flush_events (rtc); + } + +spin_unlock (&rtc->lock); + +return ret; }static const struct rtc_class_ops at91_rtc_ops = {@@ -499,10 +532,17 @@ static int at91_rtc_resume(struct device *dev)u32 mr;if (rtc->imr) { +unsigned long flags; +if (device_may_wakeup (dev))disable_irq_wake (rtc->irq); mr =rtt_readl (rtc,MR );rtt_writel (rtc,MR , mr | rtc->imr); + +spin_lock_irqsave (&rtc->lock, flags); +at91_rtc_cache_events (rtc); +at91_rtc_flush_events (rtc); +spin_unlock_irqrestore (&rtc->lock, flags); }return 0 ;
Signature · 2 lines
I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center.
Re: [RFC PATCH 3/3] rtc: at91sam9: properly act when IRQ handler is called in suspended state
Quoted 81 lines · On Wed, 25 Feb 2015 23:05:36 +0100 "Rafael J. Wysocki" <[email protected]>…
Quoted 80 lines · Boris Brezillon
The IRQ line used by the RTC device is often shared with the system timer (PIT) on at91 platforms.
Since timers are registering their handlers withIRQF_NO_SUSPEND, we should expect being called in suspended state, and properly wake the system up when this is the case.Signed-off-by Boris Brezillon <[email protected]>--- drivers/rtc/rtc-at91sam9.c | 62 ++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 51 insertions(+), 11 deletions(-)drivers/rtc/rtc-at91sam9.c +27
-11@@ -77,6 +77,8 @@ struct sam9_rtc {unsigned int gpbr_offset;int irq;struct clk *sclk; +unsigned long events; +spinlock_t lock; };#define rtt_readl (rtc, field) \@@ -271,14 +273,9 @@ static int at91_rtc_proc(struct device *dev, struct seq_file *seq)return 0 ; } -/* -* IRQ handler for the RTC -*/ -static irqreturn_t at91_rtc_interrupt (int irq,void *_rtc) +static irqreturn_t at91_rtc_cache_events (struct sam9_rtc *rtc) { -struct sam9_rtc *rtc = _rtc;u32 sr, mr; -unsigned long events =0 ;/* Shared interrupt may be for another device. Note: reading * SR clears it, so we must only read it in this irq handler! @@ -290,18 +287,54 @@ static irqreturn_t at91_rtc_interrupt(int irq, void *_rtc)/* alarm status */ if (sr &AT91_RTT_ALMS ) - events |= (RTC_AF |RTC_IRQF ); + rtc->events |= (RTC_AF |RTC_IRQF );/* timer update/increment */ if (sr &AT91_RTT_RTTINC ) - events |= (RTC_UF |RTC_IRQF ); + rtc->events |= (RTC_UF |RTC_IRQF ); + +return IRQ_HANDLED ; +} + +static void at91_rtc_flush_events (struct sam9_rtc *rtc) +{ +if (!rtc->events) +return ; -rtc_update_irq (rtc->rtcdev,1 , events); +rtc_update_irq (rtc->rtcdev,1 , rtc->events); + rtc->events =0 ;pr_debug ("%s: num=%ld, events=0x%02lx\n" , __func__, - events >>8 , events &0x000000FF ); + rtc->events >>8 , rtc->events &0x000000FF ); +} -return IRQ_HANDLED ; +/* +* IRQ handler for the RTC +*/ +static irqreturn_t at91_rtc_interrupt (int irq,void *_rtc) +{ +struct sam9_rtc *rtc = _rtc; +int ret; + +spin_lock (&rtc->lock); +
Quoted 8 lines
drivers/rtc/rtc-at91sam9.c static irqreturn_t at91_rtc_interrupt(int irq, void *_rtc) · line 324
Quoted 4 lines
drivers/rtc/rtc-at91sam9.c +4
-0+ ret =at91_rtc_cache_events (rtc); + +/* We're called in suspended state */ +if (irq_is_wakeup_armed (irq)) {Instead of doing this, I would set a flag in the driver's ->suspend callback (or in ->suspend_late, whichever is more convenient) and check that flag here.
Sure, if I can start acting as a suspended handler (in other words, if I can safely call pm_system_wakeup) as soon as my suspend callback has been called, then I'm fine adding a boolean to store the device state.
Thanks for your review.
Boris
Signature · 3 lines
Boris Brezillon, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com
Quoted 19 lines · Boris Brezillon
Hello,
I put the IRQF_NO_SUSPEND_SAFE/IRQF_TIMER_SIBLING_OK/WHATEVER_NAME_YOU_CHOOSE debate aside to concentrate on another problem pointed out by Rafael and
Mark: the fact that we cannot mixIRQF_NO_SUSPENDand wakeup sources on a shared IRQ line.This is because the wakeup code is prevailing the
IRQF_NO_SUSPENDcase and will trigger a system wakeup as soon as the IRQ line is tagged as a wakeup source.This series propose an approach to deal with such cases by doing the following:
1/ Prevent any system wakeup when at least one of the IRQ user has set
theIRQF_NO_SUSPENDflag
2/ Adapt IRQ handlers so that they can safely be called in suspended
state
3/ Let drivers decide when the system should be woken up
Quoted 1 line
Let me know what you think of this approach.
So I have the appended patch that should deal with all that too (it doesn't rework drivers that need to share NO_SUSPEND IRQs and do wakeup, but that can be done on top of it in a straightforward way).
The idea is quite simple. By default, the core replaces the interrupt handlers of everyone sharing NO_SUSPEND lines and not using IRQF_NO_SUSPEND with a null handler always returning IRQ_NONE at the suspend_device_irqs() time (the rationale being that if you don't use IRQF_NO_SUSPEND, then your device has no reason to generate interrupts after that point). The original handlers are then restored by resume_device_irqs().
However, if the IRQ is configured for wakeup, there may be a reason to generate interrupts from a device not using IRQF_NO_SUSPEND. For that, the patch adds IRQF_COND_SUSPEND that, if set, will prevent the default behavior described above from being applied to irqactions using it if the IRQs in question are configured for wakeup. Of course, the users of IRQF_COND_SUSPEND are supposed to implement wakeup detection in their interrupt handlers and then call pm_system_wakeup() if necessary.
So your patch [3/3] could be redone on top of this AFAICS.
Rafael
--- include/linux/interrupt.h | 10 +++++++++ kernel/irq/pm.c | 49 ++++++++++++++++++++++++++++++++++++++++++---- 2 files changed, 55 insertions(+), 4 deletions(-)
include/linux/interrupt.h +10 -0
@@ -57,6 +57,11 @@* IRQF_NO_THREAD - Interrupt cannot be threaded * IRQF_EARLY_RESUME - Resume IRQ early during syscore instead of at device * resume time. +* IRQF_COND_SUSPEND - If the IRQ is shared with a NO_SUSPEND user and it is +* configured for system wakeup, execute this interrup handler +* after suspending interrupts as it may be necessary to detect +* wakeup. Users need to implement system wakeup detection in +* their interrupt handlers. */ #define IRQF_DISABLED 0x00000020#define IRQF_SHARED 0x00000080
@@ -70,6 +75,7 @@#define IRQF_FORCE_RESUME 0x00008000#define IRQF_NO_THREAD 0x00010000#define IRQF_EARLY_RESUME 0x00020000 +#define IRQF_COND_SUSPEND 0x00040000#define IRQF_TIMER (__IRQF_TIMER | IRQF_NO_SUSPEND | IRQF_NO_THREAD)
@@ -101,6 +107,7 @@ typedef irqreturn_t (*irq_handler_t)(int* @thread_flags: flags related to @thread * @thread_mask: bitmask for keeping track of @thread activity * @dir: pointer to the proc/irq/NN/name entry +* @saved_handler: address of the original interrupt handler function */ struct irqaction {irq_handler_t handler;
@@ -115,6 +122,9 @@ struct irqaction {unsigned long thread_mask;const char *name;struct proc_dir_entry *dir; +#ifdef CONFIG_PM_SLEEP +irq_handler_t saved_handler; +#endif }____cacheline_internodealigned_in_smp ;extern irqreturn_t no_action (int cpl,void *dev_id);
kernel/irq/pm.c +45 -4
71 lines not on this page · raw message
Hi Rafael,
Quoted 33 lines · On Wed, 25 Feb 2015 22:59:36 +0100 "Rafael J. Wysocki" <[email protected]>…
Quoted 19 lines · Boris Brezillon
Hello,
I put the IRQF_NO_SUSPEND_SAFE/IRQF_TIMER_SIBLING_OK/WHATEVER_NAME_YOU_CHOOSE debate aside to concentrate on another problem pointed out by Rafael and
Mark: the fact that we cannot mixIRQF_NO_SUSPENDand wakeup sources on a shared IRQ line.This is because the wakeup code is prevailing the
IRQF_NO_SUSPENDcase and will trigger a system wakeup as soon as the IRQ line is tagged as a wakeup source.This series propose an approach to deal with such cases by doing the following:
1/ Prevent any system wakeup when at least one of the IRQ user has set
theIRQF_NO_SUSPENDflag
2/ Adapt IRQ handlers so that they can safely be called in suspended
state
3/ Let drivers decide when the system should be woken upQuoted 1 line
Let me know what you think of this approach.
So I have the appended patch that should deal with all that too (it doesn't rework drivers that need to share
NO_SUSPENDIRQs and do wakeup, but that can be done on top of it in a straightforward way).The idea is quite simple. By default, the core replaces the interrupt handlers of everyone sharing
NO_SUSPENDlines and not usingIRQF_NO_SUSPENDwith a null handler always returningIRQ_NONEat thesuspend_device_irqs()time (the rationale being that if you don't useIRQF_NO_SUSPEND, then your device has no reason to generate interrupts after that point). The original handlers are then restored byresume_device_irqs().
Quoted 7 lines
However, if the IRQ is configured for wakeup, there may be a reason to generate interrupts from a device not using
IRQF_NO_SUSPEND. For that, the patch addsIRQF_COND_SUSPENDthat, if set, will prevent the default behavior described above from being applied to irqactions using it if the IRQs in question are configured for wakeup. Of course, the users ofIRQF_COND_SUSPENDare supposed to implement wakeup detection in their interrupt handlers and then callpm_system_wakeup()if necessary.
That patch sounds good to me.
Could you send it on its own to the appropriate MLs ?
Thomas, Peter, Mark, could you share you opinion ?
I know I'm a bit insistent on this fix, but I'd really like to get away from this warning backtrace (and the associated problems behind it) as soon as possible.
Quoted 2 lines
So your patch [3/3] could be redone on top of this AFAICS.
Absolutely.
Thanks.
Boris
Quoted 130 lines
Rafael
--- include/linux/interrupt.h | 10 +++++++++ kernel/irq/pm.c | 49 ++++++++++++++++++++++++++++++++++++++++++---- 2 files changed, 55 insertions(+), 4 deletions(-)Index:
linux-pm/include/linux/interrupt.h
===================================================================include/linux/interrupt.h +10
-0@@ -57,6 +57,11 @@* IRQF_NO_THREAD - Interrupt cannot be threaded * IRQF_EARLY_RESUME - Resume IRQ early during syscore instead of at device * resume time. +* IRQF_COND_SUSPEND - If the IRQ is shared with a NO_SUSPEND user and it is +* configured for system wakeup, execute this interrup handler +* after suspending interrupts as it may be necessary to detect +* wakeup. Users need to implement system wakeup detection in +* their interrupt handlers. */ #define IRQF_DISABLED 0x00000020#define IRQF_SHARED 0x00000080@@ -70,6 +75,7 @@#define IRQF_FORCE_RESUME 0x00008000#define IRQF_NO_THREAD 0x00010000#define IRQF_EARLY_RESUME 0x00020000 +#define IRQF_COND_SUSPEND 0x00040000#define IRQF_TIMER (__IRQF_TIMER | IRQF_NO_SUSPEND | IRQF_NO_THREAD)@@ -101,6 +107,7 @@ typedef irqreturn_t (*irq_handler_t)(int* @thread_flags: flags related to @thread * @thread_mask: bitmask for keeping track of @thread activity * @dir: pointer to the proc/irq/NN/name entry +* @saved_handler: address of the original interrupt handler function */ struct irqaction {irq_handler_t handler;@@ -115,6 +122,9 @@ struct irqaction {unsigned long thread_mask;const char *name;struct proc_dir_entry *dir; +#ifdef CONFIG_PM_SLEEP +irq_handler_t saved_handler; +#endif }____cacheline_internodealigned_in_smp ;extern irqreturn_t no_action (int cpl,void *dev_id);Index:
linux-pm/kernel/irq/pm.c
===================================================================kernel/irq/pm.c +45
-4@@ -43,9 +43,6 @@ void irq_pm_install_action(struct irq_deif (action->flags &IRQF_NO_SUSPEND ) desc->no_suspend_depth++; - -WARN_ON_ONCE (desc->no_suspend_depth && - desc->no_suspend_depth != desc->nr_actions); }/* @@ -63,11 +60,53 @@ void irq_pm_remove_action(struct irq_des desc->no_suspend_depth--; } +static irqreturn_t irq_pm_null_handler (int irq,void *context) +{ +return IRQ_NONE ; +} + +static void prepare_no_suspend_irq (struct irq_desc *desc) +{ +struct irqaction *action; + +for (action = desc->action; action; action = action->next) { +if (action->flags &IRQF_NO_SUSPEND ) +continue ; + +if ((action->flags &IRQF_COND_SUSPEND ) && +irqd_is_wakeup_set (&desc->irq_data)) +continue ; + + action->saved_handler = action->handler; + action->handler = irq_pm_null_handler; + } +} + +static void restore_no_suspend_irq (struct irq_desc *desc) +{ +struct irqaction *action; + +for (action = desc->action; action; action = action->next) { +if (action->flags &IRQF_NO_SUSPEND ) +continue ; + +if (action->saved_handler) { + action->handler = action->saved_handler; + action->saved_handler =NULL ; + } + } +} +static bool suspend_device_irq (struct irq_desc *desc,int irq) { -if (!desc->action || desc->no_suspend_depth) +if (!desc->action)return false; +if (desc->no_suspend_depth) { +prepare_no_suspend_irq (desc); +return false; + } +if (irqd_is_wakeup_set (&desc->irq_data)) {irqd_set (&desc->irq_data,IRQD_WAKEUP_ARMED );/* @@ -135,6 +174,8 @@ static void resume_irq(struct irq_desc *if (desc->istate &IRQS_SUSPENDED ) goto resume; +restore_no_suspend_irq (desc); +/* Force resume the interrupt? */ if (!desc->force_resume_depth)return ;
Signature · 3 lines
Boris Brezillon, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com
Quoted 46 lines · Boris Brezillon
Hi Rafael,
Quoted 33 lines · On Wed, 25 Feb 2015 22:59:36 +0100 "Rafael J. Wysocki" <[email protected]>…
Quoted 19 lines · Boris Brezillon
Hello,
I put the IRQF_NO_SUSPEND_SAFE/IRQF_TIMER_SIBLING_OK/WHATEVER_NAME_YOU_CHOOSE debate aside to concentrate on another problem pointed out by Rafael and
Mark: the fact that we cannot mixIRQF_NO_SUSPENDand wakeup sources on a shared IRQ line.This is because the wakeup code is prevailing the
IRQF_NO_SUSPENDcase and will trigger a system wakeup as soon as the IRQ line is tagged as a wakeup source.This series propose an approach to deal with such cases by doing the following:
1/ Prevent any system wakeup when at least one of the IRQ user has set
theIRQF_NO_SUSPENDflag
2/ Adapt IRQ handlers so that they can safely be called in suspended
state
3/ Let drivers decide when the system should be woken upQuoted 1 line
Let me know what you think of this approach.
So I have the appended patch that should deal with all that too (it doesn't rework drivers that need to share
NO_SUSPENDIRQs and do wakeup, but that can be done on top of it in a straightforward way).The idea is quite simple. By default, the core replaces the interrupt handlers of everyone sharing
NO_SUSPENDlines and not usingIRQF_NO_SUSPENDwith a null handler always returningIRQ_NONEat thesuspend_device_irqs()time (the rationale being that if you don't useIRQF_NO_SUSPEND, then your device has no reason to generate interrupts after that point). The original handlers are then restored byresume_device_irqs().Quoted 7 lines
However, if the IRQ is configured for wakeup, there may be a reason to generate interrupts from a device not using
IRQF_NO_SUSPEND. For that, the patch addsIRQF_COND_SUSPENDthat, if set, will prevent the default behavior described above from being applied to irqactions using it if the IRQs in question are configured for wakeup. Of course, the users ofIRQF_COND_SUSPENDare supposed to implement wakeup detection in their interrupt handlers and then callpm_system_wakeup()if necessary.
Quoted 1 line
That patch sounds good to me.
But it is still a bit risky. Namely, if the driver in question is sufficiently broken (eg. it may not suspend the device and rely on the fact that its interrupt handler will be run just because it is sharing a "no suspend" IRQ), we may get an interrupt storm.
Isn't that a problem?
Quoted 1 line
Could you send it on its own to the appropriate MLs ?
Sure, I can do that, but I'd like to hear from the other people on the CC first.
Quoted 4 lines
Thomas, Peter, Mark, could you share you opinion ?
I know I'm a bit insistent on this fix, but I'd really like to get away from this warning backtrace (and the associated problems behind it) as soon as possible.
Yeah, me too. :-)
Signature · 2 lines
I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center.
Quoted 47 lines · On Thu, 26 Feb 2015 16:44:16 +0100 "Rafael J. Wysocki" <[email protected]>…
Quoted 46 lines · Boris Brezillon
Hi Rafael,
Quoted 40 lines · On Wed, 25 Feb 2015 22:59:36 +0100 "Rafael J. Wysocki" <[email protected]>…
Quoted 19 lines · Boris Brezillon
Hello,
I put the IRQF_NO_SUSPEND_SAFE/IRQF_TIMER_SIBLING_OK/WHATEVER_NAME_YOU_CHOOSE debate aside to concentrate on another problem pointed out by Rafael and
Mark: the fact that we cannot mixIRQF_NO_SUSPENDand wakeup sources on a shared IRQ line.This is because the wakeup code is prevailing the
IRQF_NO_SUSPENDcase and will trigger a system wakeup as soon as the IRQ line is tagged as a wakeup source.This series propose an approach to deal with such cases by doing the following:
1/ Prevent any system wakeup when at least one of the IRQ user has set
theIRQF_NO_SUSPENDflag
2/ Adapt IRQ handlers so that they can safely be called in suspended
state
3/ Let drivers decide when the system should be woken upQuoted 1 line
Let me know what you think of this approach.
So I have the appended patch that should deal with all that too (it doesn't rework drivers that need to share
NO_SUSPENDIRQs and do wakeup, but that can be done on top of it in a straightforward way).The idea is quite simple. By default, the core replaces the interrupt handlers of everyone sharing
NO_SUSPENDlines and not usingIRQF_NO_SUSPENDwith a null handler always returningIRQ_NONEat thesuspend_device_irqs()time (the rationale being that if you don't useIRQF_NO_SUSPEND, then your device has no reason to generate interrupts after that point). The original handlers are then restored byresume_device_irqs().However, if the IRQ is configured for wakeup, there may be a reason to generate interrupts from a device not using
IRQF_NO_SUSPEND. For that, the patch addsIRQF_COND_SUSPENDthat, if set, will prevent the default behavior described above from being applied to irqactions using it if the IRQs in question are configured for wakeup. Of course, the users ofIRQF_COND_SUSPENDare supposed to implement wakeup detection in their interrupt handlers and then callpm_system_wakeup()if necessary.
Quoted 8 lines
Quoted 1 line
That patch sounds good to me.
But it is still a bit risky. Namely, if the driver in question is sufficiently broken (eg. it may not suspend the device and rely on the fact that its interrupt handler will be run just because it is sharing a "no suspend" IRQ), we may get an interrupt storm.
Isn't that a problem?
For me no (I'll fix all the drivers to handle wakeup, and they are all already masking interrupts coming from their side in the suspend callback).
I can't talk for other people though.
The only problem I see here is that you're not informing people that they are erroneously mixing IRQF_NO_SUSPEND and !IRQF_NO_SUSPEND anymore (you removed the warning backtrace).
Moreover, you are replacing their handler by a stub when entering suspend, so they might end-up receiving spurious interrupts when suspended without knowing why ?
How about checking if the number of actions registered with IRQF_NO_SUSPEND + those registered with IRQF_COND_SUSPEND (or another flag stating that the handler can safely be called in suspended state even if it didn't ask for NO_SUSPEND) are equal to the total number of registered actions, and complain if it's not the case.
If some actions are offending this rule, you could keep the previous behavior by leaving its handler in place when entering suspend so that existing drivers/systems will keep working (but with a warning backtrace).
Quoted 4 lines
Quoted 2 lines
Could you send it on its own to the appropriate MLs ?
Sure, I can do that, but I'd like to hear from the other people on the CC first.
Sure, but that doesn't prevent sending it as an RFC, you'll still have the same review ;-).
Signature · 3 lines
Boris Brezillon, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com
Quoted 70 lines · Boris Brezillon
Quoted 47 lines · On Thu, 26 Feb 2015 16:44:16 +0100 "Rafael J. Wysocki" <[email protected]>…
Quoted 46 lines · Boris Brezillon
Hi Rafael,
Quoted 40 lines · On Wed, 25 Feb 2015 22:59:36 +0100 "Rafael J. Wysocki" <[email protected]>…
Quoted 19 lines · Boris Brezillon
Hello,
I put the IRQF_NO_SUSPEND_SAFE/IRQF_TIMER_SIBLING_OK/WHATEVER_NAME_YOU_CHOOSE debate aside to concentrate on another problem pointed out by Rafael and
Mark: the fact that we cannot mixIRQF_NO_SUSPENDand wakeup sources on a shared IRQ line.This is because the wakeup code is prevailing the
IRQF_NO_SUSPENDcase and will trigger a system wakeup as soon as the IRQ line is tagged as a wakeup source.This series propose an approach to deal with such cases by doing the following:
1/ Prevent any system wakeup when at least one of the IRQ user has set
theIRQF_NO_SUSPENDflag
2/ Adapt IRQ handlers so that they can safely be called in suspended
state
3/ Let drivers decide when the system should be woken upQuoted 1 line
Let me know what you think of this approach.
So I have the appended patch that should deal with all that too (it doesn't rework drivers that need to share
NO_SUSPENDIRQs and do wakeup, but that can be done on top of it in a straightforward way).The idea is quite simple. By default, the core replaces the interrupt handlers of everyone sharing
NO_SUSPENDlines and not usingIRQF_NO_SUSPENDwith a null handler always returningIRQ_NONEat thesuspend_device_irqs()time (the rationale being that if you don't useIRQF_NO_SUSPEND, then your device has no reason to generate interrupts after that point). The original handlers are then restored byresume_device_irqs().However, if the IRQ is configured for wakeup, there may be a reason to generate interrupts from a device not using
IRQF_NO_SUSPEND. For that, the patch addsIRQF_COND_SUSPENDthat, if set, will prevent the default behavior described above from being applied to irqactions using it if the IRQs in question are configured for wakeup. Of course, the users ofIRQF_COND_SUSPENDare supposed to implement wakeup detection in their interrupt handlers and then callpm_system_wakeup()if necessary.Quoted 8 lines
Quoted 1 line
That patch sounds good to me.
But it is still a bit risky. Namely, if the driver in question is sufficiently broken (eg. it may not suspend the device and rely on the fact that its interrupt handler will be run just because it is sharing a "no suspend" IRQ), we may get an interrupt storm.
Isn't that a problem?
For me no (I'll fix all the drivers to handle wakeup, and they are all already masking interrupts coming from their side in the suspend callback).
I can't talk for other people though.
The only problem I see here is that you're not informing people that they are erroneously mixingIRQF_NO_SUSPENDand !IRQF_NO_SUSPEND anymore (you removed the warning backtrace).
Moreover, you are replacing their handler by a stub when entering suspend, so they might end-up receiving spurious interrupts when suspended without knowing why ?
Quoted 5 lines
How about checking if the number of actions registered with
IRQF_NO_SUSPEND+ those registered withIRQF_COND_SUSPEND(or another flag stating that the handler can safely be called in suspended state even if it didn't ask forNO_SUSPEND) are equal to the total number of registered actions, and complain if it's not the case.
The same idea I had while talking to Peter over IRC. So the patch below implements that.
Quoted 4 lines
If some actions are offending this rule, you could keep the previous behavior by leaving its handler in place when entering suspend so that existing
drivers/systemswill keep working (but with a warning backtrace).
Right.
--- include/linux/interrupt.h | 5 +++++ include/linux/irqdesc.h | 1 + kernel/irq/manage.c | 7 ++++++- kernel/irq/pm.c | 7 ++++++- 4 files changed, 18 insertions(+), 2 deletions(-)
include/linux/interrupt.h +5 -0
@@ -57,6 +57,10 @@* IRQF_NO_THREAD - Interrupt cannot be threaded * IRQF_EARLY_RESUME - Resume IRQ early during syscore instead of at device * resume time. +* IRQF_COND_SUSPEND - If the IRQ is shared with a NO_SUSPEND user, execute this +* interrupt handler after suspending interrupts. For system +* wakeup devices users need to implement wakeup detection in +* their interrupt handlers. */ #define IRQF_DISABLED 0x00000020#define IRQF_SHARED 0x00000080
@@ -70,6 +74,7 @@#define IRQF_FORCE_RESUME 0x00008000#define IRQF_NO_THREAD 0x00010000#define IRQF_EARLY_RESUME 0x00020000 +#define IRQF_COND_SUSPEND 0x00040000#define IRQF_TIMER (__IRQF_TIMER | IRQF_NO_SUSPEND | IRQF_NO_THREAD)
include/linux/irqdesc.h +1 -0
@@ -78,6 +78,7 @@ struct irq_desc {#ifdef CONFIG_PM_SLEEP unsigned int nr_actions;unsigned int no_suspend_depth; +unsigned int cond_suspend_depth;unsigned int force_resume_depth;#endif #ifdef CONFIG_PROC_FS
kernel/irq/pm.c +6 -1
@@ -43,9 +43,12 @@ void irq_pm_install_action(struct irq_deif (action->flags &IRQF_NO_SUSPEND ) desc->no_suspend_depth++; +else if (action->flags &IRQF_COND_SUSPEND ) + desc->cond_suspend_depth++;WARN_ON_ONCE (desc->no_suspend_depth && - desc->no_suspend_depth != desc->nr_actions); + (desc->no_suspend_depth + + desc->cond_suspend_depth) != desc->nr_actions); }/*
@@ -61,6 +64,8 @@ void irq_pm_remove_action(struct irq_desif (action->flags &IRQF_NO_SUSPEND ) desc->no_suspend_depth--; +else if (action->flags &IRQF_COND_SUSPEND ) + desc->cond_suspend_depth--; }static bool suspend_device_irq (struct irq_desc *desc,int irq)
kernel/irq/manage.c +6 -1
@@ -1474,8 +1474,13 @@ int request_threaded_irq(unsigned int ir* otherwise we'll have trouble later trying to figure out * which interrupt is which (messes up the interrupt freeing * logic etc). +* +* Also IRQF_COND_SUSPEND only makes sense for shared interrupts and +* it is mutually exclusive with IRQF_NO_SUSPEND. */ -if ((irqflags &IRQF_SHARED ) && !dev_id) +if (((irqflags &IRQF_SHARED ) && !dev_id) || + (!(irqflags &IRQF_SHARED ) && (irqflags &IRQF_COND_SUSPEND )) || + ((irqflags &IRQF_NO_SUSPEND ) && (irqflags &IRQF_COND_SUSPEND )))return -EINVAL ; desc =irq_to_desc (irq);
Quoted 3 lines · On Thu, 26 Feb 2015 19:17:03 +0100 "Rafael J. Wysocki" <[email protected]>…
Quoted 2 lines · Boris Brezillon
On Thu, 26 Feb 2015 16:44:16 +0100
"Rafael J. Wysocki" <[email protected]> wrote:
[...]
Quoted 19 lines
Quoted 19 lines
Quoted 7 lines
But it is still a bit risky. Namely, if the driver in question is sufficiently broken (eg. it may not suspend the device and rely on the fact that its interrupt handler will be run just because it is sharing a "no suspend" IRQ), we may get an interrupt storm.
Isn't that a problem?
For me no (I'll fix all the drivers to handle wakeup, and they are all already masking interrupts coming from their side in the suspend callback).
I can't talk for other people though.
The only problem I see here is that you're not informing people that they are erroneously mixingIRQF_NO_SUSPENDand !IRQF_NO_SUSPEND anymore (you removed the warning backtrace).
Moreover, you are replacing their handler by a stub when entering suspend, so they might end-up receiving spurious interrupts when suspended without knowing why ?
Quoted 8 lines
Quoted 5 lines
How about checking if the number of actions registered with
IRQF_NO_SUSPEND+ those registered withIRQF_COND_SUSPEND(or another flag stating that the handler can safely be called in suspended state even if it didn't ask forNO_SUSPEND) are equal to the total number of registered actions, and complain if it's not the case.The same idea I had while talking to Peter over IRC. So the patch below implements that.
Yep, that's what I had in mind.
Signature · 3 lines
Boris Brezillon, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com
Quoted 34 lines · Boris Brezillon
Quoted 3 lines · On Thu, 26 Feb 2015 19:17:03 +0100 "Rafael J. Wysocki" <[email protected]>…
Quoted 2 lines · Boris Brezillon
On Thu, 26 Feb 2015 16:44:16 +0100
"Rafael J. Wysocki" <[email protected]> wrote:[...]
Quoted 25 lines
Quoted 24 lines
Quoted 7 lines
But it is still a bit risky. Namely, if the driver in question is sufficiently broken (eg. it may not suspend the device and rely on the fact that its interrupt handler will be run just because it is sharing a "no suspend" IRQ), we may get an interrupt storm.
Isn't that a problem?
For me no (I'll fix all the drivers to handle wakeup, and they are all already masking interrupts coming from their side in the suspend callback).
I can't talk for other people though.
The only problem I see here is that you're not informing people that they are erroneously mixingIRQF_NO_SUSPENDand !IRQF_NO_SUSPEND anymore (you removed the warning backtrace).
Moreover, you are replacing their handler by a stub when entering suspend, so they might end-up receiving spurious interrupts when suspended without knowing why ?How about checking if the number of actions registered with
IRQF_NO_SUSPEND+ those registered withIRQF_COND_SUSPEND(or another flag stating that the handler can safely be called in suspended state even if it didn't ask forNO_SUSPEND) are equal to the total number of registered actions, and complain if it's not the case.
Quoted 4 lines
Quoted 2 lines
The same idea I had while talking to Peter over IRC. So the patch below implements that.
Yep, that's what I had in mind.
OK, thanks.
I'll submit it formally, then.
Signature · 2 lines
I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center.
PATCH 1/1 genirq / PM: Add flag for shared NO_SUSPEND interrupt lines
In reply to Boris BrezillonFrom: Rafael J. Wysocki <[email protected]>
It currently is required that all users of NO_SUSPEND interrupt lines pass the IRQF_NO_SUSPEND flag when requesting the IRQ or the WARN_ON_ONCE() in irq_pm_install_action() will trigger. That is done to warn about situations in which unprepared interrupt handlers may be run unnecessarily for suspended devices and may attempt to access those devices by mistake. However, it may cause drivers that have no technical reasons for using IRQF_NO_SUSPEND to set that flag just because they happen to share the interrupt line with something like a timer.
Moreover, the generic handling of wakeup interrupts introduced by commit 9ce7a25849e8 (genirq: Simplify wakeup mechanism) only works for IRQs without any NO_SUSPEND users, so the drivers of wakeup devices needing to use shared NO_SUSPEND interrupt lines for signaling system wakeup generally have to detect wakeup in their interrupt handlers. Thus if they happen to share an interrupt line with a NO_SUSPEND user, they also need to request that their interrupt handlers be run after suspend_device_irqs().
In both cases the reason for using IRQF_NO_SUSPEND is not because the driver in question has a genuine need to run its interrupt handler after suspend_device_irqs(), but because it happens to share the line with some other NO_SUSPEND user. Otherwise, the driver would do without IRQF_NO_SUSPEND just fine.
To make it possible to specify that condition explicitly, introduce a new IRQ action handler flag for shared IRQs, IRQF_COND_SUSPEND, that, when set, will indicate to the IRQ core that the interrupt user is generally fine with suspending the IRQ, but it also can tolerate handler invocations after suspend_device_irqs() and, in particular, it is capable of detecting system wakeup and triggering it as appropriate from its interrupt handler.
That will allow us to work around a problem with a shared timer interrupt line on at91 platforms.
Linx: https://lkml.org/lkml/2014/12/15/552
--- include/linux/interrupt.h | 5 +++++ include/linux/irqdesc.h | 1 + kernel/irq/manage.c | 7 ++++++- kernel/irq/pm.c | 7 ++++++- 4 files changed, 18 insertions(+), 2 deletions(-)
include/linux/interrupt.h +5 -0
@@ -57,6 +57,10 @@* IRQF_NO_THREAD - Interrupt cannot be threaded * IRQF_EARLY_RESUME - Resume IRQ early during syscore instead of at device * resume time. +* IRQF_COND_SUSPEND - If the IRQ is shared with a NO_SUSPEND user, execute this +* interrupt handler after suspending interrupts. For system +* wakeup devices users need to implement wakeup detection in +* their interrupt handlers. */ #define IRQF_DISABLED 0x00000020#define IRQF_SHARED 0x00000080
@@ -70,6 +74,7 @@#define IRQF_FORCE_RESUME 0x00008000#define IRQF_NO_THREAD 0x00010000#define IRQF_EARLY_RESUME 0x00020000 +#define IRQF_COND_SUSPEND 0x00040000#define IRQF_TIMER (__IRQF_TIMER | IRQF_NO_SUSPEND | IRQF_NO_THREAD)
include/linux/irqdesc.h +1 -0
@@ -78,6 +78,7 @@ struct irq_desc {#ifdef CONFIG_PM_SLEEP unsigned int nr_actions;unsigned int no_suspend_depth; +unsigned int cond_suspend_depth;unsigned int force_resume_depth;#endif #ifdef CONFIG_PROC_FS
kernel/irq/pm.c +6 -1
@@ -43,9 +43,12 @@ void irq_pm_install_action(struct irq_deif (action->flags &IRQF_NO_SUSPEND ) desc->no_suspend_depth++; +else if (action->flags &IRQF_COND_SUSPEND ) + desc->cond_suspend_depth++;WARN_ON_ONCE (desc->no_suspend_depth && - desc->no_suspend_depth != desc->nr_actions); + (desc->no_suspend_depth + + desc->cond_suspend_depth) != desc->nr_actions); }/*
@@ -61,6 +64,8 @@ void irq_pm_remove_action(struct irq_desif (action->flags &IRQF_NO_SUSPEND ) desc->no_suspend_depth--; +else if (action->flags &IRQF_COND_SUSPEND ) + desc->cond_suspend_depth--; }static bool suspend_device_irq (struct irq_desc *desc,int irq)
kernel/irq/manage.c +6 -1
@@ -1474,8 +1474,13 @@ int request_threaded_irq(unsigned int ir* otherwise we'll have trouble later trying to figure out * which interrupt is which (messes up the interrupt freeing * logic etc). +* +* Also IRQF_COND_SUSPEND only makes sense for shared interrupts and +* it cannot be set along with IRQF_NO_SUSPEND. */ -if ((irqflags &IRQF_SHARED ) && !dev_id) +if (((irqflags &IRQF_SHARED ) && !dev_id) || + (!(irqflags &IRQF_SHARED ) && (irqflags &IRQF_COND_SUSPEND )) || + ((irqflags &IRQF_NO_SUSPEND ) && (irqflags &IRQF_COND_SUSPEND )))return -EINVAL ; desc =irq_to_desc (irq);
Re: [PATCH] genirq / PM: Add flag for shared NO_SUSPEND interrupt lines
Quoted 35 lines · Rafael J. Wysocki
From: Rafael J. Wysocki <[email protected]>
It currently is required that all users of
NO_SUSPENDinterrupt lines pass theIRQF_NO_SUSPENDflag when requesting the IRQ or theWARN_ON_ONCE()inirq_pm_install_action()will trigger. That is done to warn about situations in which unprepared interrupt handlers may be run unnecessarily for suspended devices and may attempt to access those devices by mistake. However, it may cause drivers that have no technical reasons for usingIRQF_NO_SUSPENDto set that flag just because they happen to share the interrupt line with something like a timer.Moreover, the generic handling of wakeup interrupts introduced by commit
9ce7a25849e8(genirq: Simplify wakeup mechanism) only works for IRQs without anyNO_SUSPENDusers, so the drivers of wakeup devices needing to use sharedNO_SUSPENDinterrupt lines for signaling system wakeup generally have to detect wakeup in their interrupt handlers. Thus if they happen to share an interrupt line with aNO_SUSPENDuser, they also need to request that their interrupt handlers be run aftersuspend_device_irqs().In both cases the reason for using
IRQF_NO_SUSPENDis not because the driver in question has a genuine need to run its interrupt handler aftersuspend_device_irqs(), but because it happens to share the line with some otherNO_SUSPENDuser. Otherwise, the driver would do withoutIRQF_NO_SUSPENDjust fine.To make it possible to specify that condition explicitly, introduce a new IRQ action handler flag for shared IRQs,
IRQF_COND_SUSPEND, that, when set, will indicate to the IRQ core that the interrupt user is generally fine with suspending the IRQ, but it also can tolerate handler invocations aftersuspend_device_irqs()and, in particular, it is capable of detecting system wakeup and triggering it as appropriate from its interrupt handler.
Quoted 8 lines
That will allow us to work around a problem with a shared timer interrupt line on at91 platforms.
Linx: https://lkml.org/lkml/2014/12/15/552
Reported-by Boris Brezillon <[email protected]>Signed-off-by Rafael J. Wysocki <[email protected]>
Seems good to me. Should I take this through tip/irq ?
Also, should we warn if people use enable_irq_wake() where there is only a single descriptor with NO_SUSPEND?
Re: [PATCH] genirq / PM: Add flag for shared NO_SUSPEND interrupt lines
Quoted 39 lines · Peter Zijlstra
Quoted 38 lines · Rafael J. Wysocki
From: Rafael J. Wysocki <[email protected]>
It currently is required that all users of
NO_SUSPENDinterrupt lines pass theIRQF_NO_SUSPENDflag when requesting the IRQ or theWARN_ON_ONCE()inirq_pm_install_action()will trigger. That is done to warn about situations in which unprepared interrupt handlers may be run unnecessarily for suspended devices and may attempt to access those devices by mistake. However, it may cause drivers that have no technical reasons for usingIRQF_NO_SUSPENDto set that flag just because they happen to share the interrupt line with something like a timer.Moreover, the generic handling of wakeup interrupts introduced by commit
9ce7a25849e8(genirq: Simplify wakeup mechanism) only works for IRQs without anyNO_SUSPENDusers, so the drivers of wakeup devices needing to use sharedNO_SUSPENDinterrupt lines for signaling system wakeup generally have to detect wakeup in their interrupt handlers. Thus if they happen to share an interrupt line with aNO_SUSPENDuser, they also need to request that their interrupt handlers be run aftersuspend_device_irqs().In both cases the reason for using
IRQF_NO_SUSPENDis not because the driver in question has a genuine need to run its interrupt handler aftersuspend_device_irqs(), but because it happens to share the line with some otherNO_SUSPENDuser. Otherwise, the driver would do withoutIRQF_NO_SUSPENDjust fine.To make it possible to specify that condition explicitly, introduce a new IRQ action handler flag for shared IRQs,
IRQF_COND_SUSPEND, that, when set, will indicate to the IRQ core that the interrupt user is generally fine with suspending the IRQ, but it also can tolerate handler invocations aftersuspend_device_irqs()and, in particular, it is capable of detecting system wakeup and triggering it as appropriate from its interrupt handler.That will allow us to work around a problem with a shared timer interrupt line on at91 platforms.
Quoted 7 lines
Quoted 5 lines
Linx: https://lkml.org/lkml/2014/12/15/552
Reported-by Boris Brezillon <[email protected]>Signed-off-by Rafael J. Wysocki <[email protected]>Seems good to me. Should I take this through tip/irq ?
I can apply it along with the previous IRQF_NO_SUSPEND documentation patch from Mark Rutland if you ACK it for me. :-)
Quoted 2 lines
Also, should we warn if people use
enable_irq_wake()where there is only a single descriptor withNO_SUSPEND?
We probably should do that, but that would be a separate patch IMO?
Peter Zijlstra
Acked-byRe: [PATCH] genirq / PM: Add flag for shared NO_SUSPEND interrupt lines
Quoted 6 lines · Rafael J. Wysocki
Quoted 1 line · Peter Zijlstra
Seems good to me. Should I take this through tip/irq ?
I can apply it along with the previous
IRQF_NO_SUSPENDdocumentation patch from Mark Rutland if you ACK it for me. :-)
Works for me,
Quoted 4 lines
Quoted 2 lines
Also, should we warn if people use
enable_irq_wake()where there is only a single descriptor withNO_SUSPEND?We probably should do that, but that would be a separate patch IMO?
Agreed. Just wanted to raise the point.
Re: [PATCH] genirq / PM: Add flag for shared NO_SUSPEND interrupt lines
In reply to Rafael J. WysockiHi Rafael,
I'm a little late to the party here, but I have just a couple of minor comments...
[...]
Quoted 3 lines
s/x/k/ ?
Quoted 14 lines
Reported-by Boris Brezillon <[email protected]>Signed-off-by Rafael J. Wysocki <[email protected]>--- include/linux/interrupt.h | 5 +++++ include/linux/irqdesc.h | 1 + kernel/irq/manage.c | 7 ++++++- kernel/irq/pm.c | 7 ++++++- 4 files changed, 18 insertions(+), 2 deletions(-)Index:
linux-pm/include/linux/interrupt.h
===================================================================
include/linux/interrupt.h line 63
Quoted 7 lines
include/linux/interrupt.h +4
-0@@ -57,6 +57,10 @@* IRQF_NO_THREAD - Interrupt cannot be threaded * IRQF_EARLY_RESUME - Resume IRQ early during syscore instead of at device * resume time. +* IRQF_COND_SUSPEND - If the IRQ is shared with a NO_SUSPEND user, execute this +* interrupt handler after suspending interrupts. For system +* wakeup devices users need to implement wakeup detection in +* their interrupt handlers.
It's probably worth documenting this in suspend-and-interrupts.txt, as this invalidates some of the "IRQF_NO_SUSPEND and enable_irq_wake()" section. I'll send a patch momentarily to that effect.
Otherwise, this patch looks good, thanks for handling this!
Thanks,
Mark.
PATCH 1/1 genirq: describe IRQF_COND_SUSPEND
With certain restrictions it is possible for a wakeup device to share and IRQ with an IRQF_NO_SUSPEND user, and the warnings introduced by commit cab303be91dc47942bc25de33dc1140123540800 are spurious. The new IRQF_COND_SUSPEND flag allows drivers to tell the core when these restrictions are met, allowing spurious warnings to be silenced.
This patch documents how IRQF_COND_SUSPEND is expected to be used, updating some of the text now made invalid by its addition.
--- Documentation/power/suspend-and-interrupts.txt | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-)
As promised previously, take IRQF_COND_SUSPEND into account in the documentation.
Rafael, does this look OK to you?
Thanks,
Mark.
Documentation/power/suspend-and-interrupts.txt +13 -3
@@ -112,8 +112,9 @@ any special interrupt handling logic for it to work. IRQF_NO_SUSPEND and enable_irq_wake() ------------------------------------- -There are no valid reasons to use both enable_irq_wake() and the IRQF_NO_SUSPEND -flag on the same IRQ. +There are very few valid reasons to use both enable_irq_wake() and the +IRQF_NO_SUSPEND flag on the same IRQ, and it is never valid to use both for the +same device. First of all, if the IRQ is not shared, the rules for handling IRQF_NO_SUSPEND interrupts (interrupt handlers are invoked after suspend_device_irqs()) are
@@ -122,4 +123,13 @@ handlers are not invoked after suspend_device_irqs()). Second, both enable_irq_wake() and IRQF_NO_SUSPEND apply to entire IRQs and not to individual interrupt handlers, so sharing an IRQ between a system wakeup -interrupt source and an IRQF_NO_SUSPEND interrupt source does not make sense. +interrupt source and an IRQF_NO_SUSPEND interrupt source does not generally +make sense. + +In rare cases an IRQ can be shared between a wakeup device driver and an +IRQF_NO_SUSPEND user. In order for this to be safe, the wakeup device driver +must be able to discern spurious IRQs from genuine wakeup events (signalling +the latter to the core with pm_system_wakeup()), must use enable_irq_wake() to +ensure that the IRQ will function as a wakeup source, and must request the IRQ +with IRQF_COND_SUSPEND to tell the core that it meets these requirements. If +these requirements are not met, it is not valid to use IRQF_COND_SUSPEND.
Signature · 1 line
1.9.1
Re: [PATCH] genirq: describe IRQF_COND_SUSPEND
Quoted 13 lines · Mark Rutland
With certain restrictions it is possible for a wakeup device to share and IRQ with an
IRQF_NO_SUSPENDuser, and the warnings introduced by commitcab303be91dc47942bc25de33dc1140123540800are spurious. The newIRQF_COND_SUSPENDflag allows drivers to tell the core when these restrictions are met, allowing spurious warnings to be silenced.This patch documents how
IRQF_COND_SUSPENDis expected to be used, updating some of the text now made invalid by its addition.Signed-off-by Mark Rutland <[email protected]>Cc Boris Brezillon <[email protected]>Cc Peter Zijlstra <[email protected]>Cc Rafael J. Wysocki <[email protected]>
Quoted 8 lines
--- Documentation/power/suspend-and-interrupts.txt | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-)As promised previously, take
IRQF_COND_SUSPENDinto account in the documentation.Rafael, does this look OK to you?
Yes, it does, thanks!
I'll queue it up along with the rest of the IRQF_COND_SUSPEND patches.
Quoted 32 lines
Documentation/power/suspend-and-interrupts.txt +13
-3@@ -112,8 +112,9 @@ any special interrupt handling logic for it to work. IRQF_NO_SUSPEND and enable_irq_wake() ------------------------------------- -There are no valid reasons to use both enable_irq_wake() and the IRQF_NO_SUSPEND -flag on the same IRQ. +There are very few valid reasons to use both enable_irq_wake() and the +IRQF_NO_SUSPEND flag on the same IRQ, and it is never valid to use both for the +same device. First of all, if the IRQ is not shared, the rules for handling IRQF_NO_SUSPEND interrupts (interrupt handlers are invoked after suspend_device_irqs()) are@@ -122,4 +123,13 @@ handlers are not invoked after suspend_device_irqs()). Second, both enable_irq_wake() and IRQF_NO_SUSPEND apply to entire IRQs and not to individual interrupt handlers, so sharing an IRQ between a system wakeup -interrupt source and an IRQF_NO_SUSPEND interrupt source does not make sense. +interrupt source and an IRQF_NO_SUSPEND interrupt source does not generally +make sense. + +In rare cases an IRQ can be shared between a wakeup device driver and an +IRQF_NO_SUSPEND user. In order for this to be safe, the wakeup device driver +must be able to discern spurious IRQs from genuine wakeup events (signalling +the latter to the core with pm_system_wakeup()), must use enable_irq_wake() to +ensure that the IRQ will function as a wakeup source, and must request the IRQ +with IRQF_COND_SUSPEND to tell the core that it meets these requirements. If +these requirements are not met, it is not valid to use IRQF_COND_SUSPEND.
Signature · 2 lines
I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center.
Re: [PATCH] genirq: describe IRQF_COND_SUSPEND
In reply to Mark Rutlandtiny tiny nitpick:
Quoted 2 lines · Mark Rutland
With certain restrictions it is possible for a wakeup device to share and IRQ with an
IRQF_NO_SUSPENDuser, and the warnings introduced by
^ an
Documentation/power/suspend-and-interrupts.txt handlers are not invoked after suspend_device_irqs()). · line 131
Quoted 3 lines
Documentation/power/suspend-and-interrupts.txt +3
-0+In rare cases an IRQ can be shared between a wakeup device driver and an +IRQF_NO_SUSPEND user. In order for this to be safe, the wakeup device driver +must be able to discern spurious IRQs from genuine wakeup events (signalling
And genuine question, should we use British English or American English or we don't care ?
Signature · 3 lines
Alexandre Belloni, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com
Re: [PATCH] genirq: describe IRQF_COND_SUSPEND
Quoted 6 lines · On Wednesday 04 March 2015 23:17:29 Alexandre Belloni wrote:
Quoted 2 lines · Mark Rutland
With certain restrictions it is possible for a wakeup device to share and IRQ with an
IRQF_NO_SUSPENDuser, and the warnings introduced by^ an
Quoted 6 lines
Documentation/power/suspend-and-interrupts.txt handlers are not invoked after suspend_device_irqs()). · line 131
Quoted 3 lines
Documentation/power/suspend-and-interrupts.txt +3
-0+In rare cases an IRQ can be shared between a wakeup device driver and an +IRQF_NO_SUSPEND user. In order for this to be safe, the wakeup device driver +must be able to discern spurious IRQs from genuine wakeup events (signallingAnd genuine question, should we use British English or American English or we don't care ?
I believe most developers use Incorrect English, the second most common is American English, followed by Indian English and British English. ;-)
More seriously, just try to be consistent within one file or subsystem.
Arnd
Re: [PATCH] genirq: describe IRQF_COND_SUSPEND
In reply to Alexandre BelloniQuoted 6 lines · Alexandre Belloni
tiny tiny nitpick:
Quoted 2 lines · Mark Rutland
With certain restrictions it is possible for a wakeup device to share and IRQ with an
IRQF_NO_SUSPENDuser, and the warnings introduced by^ an
Whoops.
Rafael, are you happy to fix that up, or would you like me to resend?
Quoted 6 lines
Documentation/power/suspend-and-interrupts.txt handlers are not invoked after suspend_device_irqs()). · line 131
Quoted 3 lines
Documentation/power/suspend-and-interrupts.txt +3
-0+In rare cases an IRQ can be shared between a wakeup device driver and an +IRQF_NO_SUSPEND user. In order for this to be safe, the wakeup device driver +must be able to discern spurious IRQs from genuine wakeup events (signallingAnd genuine question, should we use British English or American English or we don't care ?
Have I written something that isn't valid American English there? I read over this a few times and failed to spot anything obvious.
I'm happy to change for consistency, I generally assume that's the most important thing.
Mark.
Re: [PATCH] genirq: describe IRQF_COND_SUSPEND
Quoted 7 lines · Mark Rutland
Quoted 6 lines
Documentation/power/suspend-and-interrupts.txt handlers are not invoked after suspend_device_irqs()). · line 131
Quoted 3 lines
Documentation/power/suspend-and-interrupts.txt +3
-0+In rare cases an IRQ can be shared between a wakeup device driver and an +IRQF_NO_SUSPEND user. In order for this to be safe, the wakeup device driver +must be able to discern spurious IRQs from genuine wakeup events (signallingAnd genuine question, should we use British English or American English or we don't care ?
Quoted 5 lines
Have I written something that isn't valid American English there? I read over this a few times and failed to spot anything obvious.
I'm happy to change for consistency, I generally assume that's the most important thing.
I'd say signalling vs signaling. I actually had to look up which one was correct. I'm personally using Incorrect/Broken English so I'm definitely not here to give lessons.
Signature · 3 lines
Alexandre Belloni, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com
Re: [PATCH] genirq: describe IRQF_COND_SUSPEND
Quoted 11 lines · Alexandre Belloni
Quoted 10 lines · Mark Rutland
Quoted 6 lines
Documentation/power/suspend-and-interrupts.txt handlers are not invoked after suspend_device_irqs()). · line 131
Quoted 3 lines
Documentation/power/suspend-and-interrupts.txt +3
-0+In rare cases an IRQ can be shared between a wakeup device driver and an +IRQF_NO_SUSPEND user. In order for this to be safe, the wakeup device driver +must be able to discern spurious IRQs from genuine wakeup events (signallingAnd genuine question, should we use British English or American English or we don't care ?
Have I written something that isn't valid American English there? I read over this a few times and failed to spot anything obvious.
Quoted 6 lines
Quoted 2 lines
I'm happy to change for consistency, I generally assume that's the most important thing.
I'd say signalling vs signaling. I actually had to look up which one was correct. I'm personally using Incorrect/Broken English so I'm definitely not here to give lessons.
Easy option to keep everyone happy: s/signalling/indicating/
That should be valid for the English variants I'm aware of, and it has the same number of characters so we don't need to reflow the text.
Mark.
Re: [PATCH] genirq: describe IRQF_COND_SUSPEND
In reply to Mark RutlandQuoted 11 lines · Mark Rutland
Quoted 6 lines · Alexandre Belloni
tiny tiny nitpick:
Quoted 2 lines · Mark Rutland
With certain restrictions it is possible for a wakeup device to share and IRQ with an
IRQF_NO_SUSPENDuser, and the warnings introduced by^ an
Whoops.
Rafael, are you happy to fix that up, or would you like me to resend?
Fixed, thanks!
Signature · 2 lines
I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center.
Re: [PATCH] genirq / PM: Add flag for shared NO_SUSPEND interrupt lines
In reply to Mark RutlandQuoted 5 lines · Mark Rutland
Hi Rafael,
I'm a little late to the party here, but I have just a couple of minor comments...
Quoted 7 lines
[...]
Quoted 3 lines
s/x/k/ ?
Yes, thanks!
Quoted 26 lines
Quoted 14 lines
Reported-by Boris Brezillon <[email protected]>Signed-off-by Rafael J. Wysocki <[email protected]>--- include/linux/interrupt.h | 5 +++++ include/linux/irqdesc.h | 1 + kernel/irq/manage.c | 7 ++++++- kernel/irq/pm.c | 7 ++++++- 4 files changed, 18 insertions(+), 2 deletions(-)Index:
linux-pm/include/linux/interrupt.h
===================================================================include/linux/interrupt.h line 63
Quoted 7 lines
include/linux/interrupt.h +4
-0@@ -57,6 +57,10 @@* IRQF_NO_THREAD - Interrupt cannot be threaded * IRQF_EARLY_RESUME - Resume IRQ early during syscore instead of at device * resume time. +* IRQF_COND_SUSPEND - If the IRQ is shared with a NO_SUSPEND user, execute this +* interrupt handler after suspending interrupts. For system +* wakeup devices users need to implement wakeup detection in +* their interrupt handlers. It's probably worth documenting this in suspend-and-interrupts.txt, as this invalidates some of the "
IRQF_NO_SUSPENDandenable_irq_wake()" section. I'll send a patch momentarily to that effect.
Quoted 3 lines
Otherwise, this patch looks good, thanks for handling this!
Acked-by Mark Rutland <[email protected]>
Thanks!