[RFC PATCH 0/3] genirq: mixing IRQF_NO_SUSPEND and wakeup sources on shared IRQs

[RFC PATCH 0/3] genirq: mixing IRQF_NO_SUSPEND and wakeup sources on shared IRQs

[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 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(-)

-- 
1.9.1

[RFC PATCH 1/3] genirq: prevent system wakeup when dealing with IRQF_NO_SUSPEND IRQs

[Boris Brezillon]

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.

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
 kernel/irq/pm.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/kernel/irq/pm.c b/kernel/irq/pm.c
index 3ca5325..1743162 100644
--- a/kernel/irq/pm.c
+++ b/kernel/irq/pm.c
@@ -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++;
-- 
1.9.1

[RFC PATCH 1/3] genirq: prevent system wakeup when dealing with IRQF_NO_SUSPEND IRQs

[Rafael J. Wysocki]

On Tuesday, February 24, 2015 10:56:00 AM Boris Brezillon wrote:
> 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.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> ---
>  kernel/irq/pm.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/irq/pm.c b/kernel/irq/pm.c
> index 3ca5325..1743162 100644
> --- a/kernel/irq/pm.c
> +++ b/kernel/irq/pm.c
> @@ -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()).


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

[RFC PATCH 1/3] genirq: prevent system wakeup when dealing with IRQF_NO_SUSPEND IRQs

[Boris Brezillon]

On Wed, 25 Feb 2015 23:01:31 +0100
"Rafael J. Wysocki" <rjw@rjwysocki.net> wrote:

> On Tuesday, February 24, 2015 10:56:00 AM Boris Brezillon wrote:
> > 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.
> > 
> > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > ---
> >  kernel/irq/pm.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/kernel/irq/pm.c b/kernel/irq/pm.c
> > index 3ca5325..1743162 100644
> > --- a/kernel/irq/pm.c
> > +++ b/kernel/irq/pm.c
> > @@ -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()).
> 
> 

Indeed, it seems I overlooked this test in suspend_device_irq, and this
makes my irq_is_wakeup_armed test useless.


-- 
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

[Boris Brezillon]

Add 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.

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
 include/linux/interrupt.h |  3 +++
 kernel/irq/manage.c       | 16 ++++++++++++++++
 kernel/irq/pm.c           |  6 ++++++
 3 files changed, 25 insertions(+)

diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
index d9b05b5..052a3b2 100644
--- a/include/linux/interrupt.h
+++ b/include/linux/interrupt.h
@@ -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;
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index 196a06f..5424be0 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -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
diff --git a/kernel/irq/pm.c b/kernel/irq/pm.c
index 1743162..1110a37 100644
--- a/kernel/irq/pm.c
+++ b/kernel/irq/pm.c
@@ -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.
-- 
1.9.1

[RFC PATCH 2/3] genirq: add helper functions to deal with wakeup on shared IRQF_NO_SUSPEND IRQs

[Rafael J. Wysocki]

On Tuesday, February 24, 2015 10:56:01 AM Boris Brezillon wrote:
> Add 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.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> ---
>  include/linux/interrupt.h |  3 +++
>  kernel/irq/manage.c       | 16 ++++++++++++++++
>  kernel/irq/pm.c           |  6 ++++++
>  3 files changed, 25 insertions(+)
> 
> diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
> index d9b05b5..052a3b2 100644
> --- a/include/linux/interrupt.h
> +++ b/include/linux/interrupt.h
> @@ -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;
> diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
> index 196a06f..5424be0 100644
> --- a/kernel/irq/manage.c
> +++ b/kernel/irq/manage.c
> @@ -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);

Ugly that is ...

> +
>  /*
>   * Internal function that tells the architecture code whether a
>   * particular irq has been exclusively allocated or is available
> diff --git a/kernel/irq/pm.c b/kernel/irq/pm.c
> index 1743162..1110a37 100644
> --- a/kernel/irq/pm.c
> +++ b/kernel/irq/pm.c
> @@ -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?

> +
>  /*
>   * Called from __setup_irq() with desc->lock held after @action has
>   * been installed in the action chain.
> 


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

[RFC PATCH 2/3] genirq: add helper functions to deal with wakeup on shared IRQF_NO_SUSPEND IRQs

[Boris Brezillon]

On Wed, 25 Feb 2015 23:03:14 +0100
"Rafael J. Wysocki" <rjw@rjwysocki.net> wrote:

> On Tuesday, February 24, 2015 10:56:01 AM Boris Brezillon wrote:
> > Add 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.
> > 
> > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > ---
> >  include/linux/interrupt.h |  3 +++
> >  kernel/irq/manage.c       | 16 ++++++++++++++++
> >  kernel/irq/pm.c           |  6 ++++++
> >  3 files changed, 25 insertions(+)
> > 
> > diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
> > index d9b05b5..052a3b2 100644
> > --- a/include/linux/interrupt.h
> > +++ b/include/linux/interrupt.h
> > @@ -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;
> > diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
> > index 196a06f..5424be0 100644
> > --- a/kernel/irq/manage.c
> > +++ b/kernel/irq/manage.c
> > @@ -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);
> 
> 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.

> 
> > +
> >  /*
> >   * Internal function that tells the architecture code whether a
> >   * particular irq has been exclusively allocated or is available
> > diff --git a/kernel/irq/pm.c b/kernel/irq/pm.c
> > index 1743162..1110a37 100644
> > --- a/kernel/irq/pm.c
> > +++ b/kernel/irq/pm.c
> > @@ -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?

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.


-- 
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

[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 with IRQF_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 <boris.brezillon@free-electrons.com>
---
 drivers/rtc/rtc-at91sam9.c | 62 ++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 51 insertions(+), 11 deletions(-)

diff --git a/drivers/rtc/rtc-at91sam9.c b/drivers/rtc/rtc-at91sam9.c
index 2183fd2..8cf9c1b 100644
--- a/drivers/rtc/rtc-at91sam9.c
+++ b/drivers/rtc/rtc-at91sam9.c
@@ -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;
-- 
1.9.1

[RFC PATCH 3/3] rtc: at91sam9: properly act when IRQ handler is called in suspended state

[Rafael J. Wysocki]

On Tuesday, February 24, 2015 10:56:02 AM Boris Brezillon wrote:
> 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 with IRQF_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 <boris.brezillon@free-electrons.com>
> ---
>  drivers/rtc/rtc-at91sam9.c | 62 ++++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 51 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-at91sam9.c b/drivers/rtc/rtc-at91sam9.c
> index 2183fd2..8cf9c1b 100644
> --- a/drivers/rtc/rtc-at91sam9.c
> +++ b/drivers/rtc/rtc-at91sam9.c
> @@ -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)) {

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.

> +		/* 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;
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

[RFC PATCH 3/3] rtc: at91sam9: properly act when IRQ handler is called in suspended state

[Boris Brezillon]

On Wed, 25 Feb 2015 23:05:36 +0100
"Rafael J. Wysocki" <rjw@rjwysocki.net> wrote:

> On Tuesday, February 24, 2015 10:56:02 AM Boris Brezillon wrote:
> > 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 with IRQF_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 <boris.brezillon@free-electrons.com>
> > ---
> >  drivers/rtc/rtc-at91sam9.c | 62 ++++++++++++++++++++++++++++++++++++++--------
> >  1 file changed, 51 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/rtc/rtc-at91sam9.c b/drivers/rtc/rtc-at91sam9.c
> > index 2183fd2..8cf9c1b 100644
> > --- a/drivers/rtc/rtc-at91sam9.c
> > +++ b/drivers/rtc/rtc-at91sam9.c
> > @@ -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)) {
> 
> 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

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[RFC PATCH 0/3] genirq: mixing IRQF_NO_SUSPEND and wakeup sources on shared IRQs

[Rafael J. Wysocki]

On Tuesday, February 24, 2015 10:55:59 AM Boris Brezillon wrote:
> 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.

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(-)

Index: linux-pm/include/linux/interrupt.h
===================================================================
--- linux-pm.orig/include/linux/interrupt.h
+++ linux-pm/include/linux/interrupt.h
@@ -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
===================================================================
--- linux-pm.orig/kernel/irq/pm.c
+++ linux-pm/kernel/irq/pm.c
@@ -43,9 +43,6 @@ void irq_pm_install_action(struct irq_de
 
 	if (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;

[RFC PATCH 0/3] genirq: mixing IRQF_NO_SUSPEND and wakeup sources on shared IRQs

[Boris Brezillon]

Hi Rafael,

On Wed, 25 Feb 2015 22:59:36 +0100
"Rafael J. Wysocki" <rjw@rjwysocki.net> wrote:

> On Tuesday, February 24, 2015 10:55:59 AM Boris Brezillon wrote:
> > 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.
> 
> 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.

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.

> 
> So your patch [3/3] could be redone on top of this AFAICS.

Absolutely.

Thanks.

Boris

> 
> 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
> ===================================================================
> --- linux-pm.orig/include/linux/interrupt.h
> +++ linux-pm/include/linux/interrupt.h
> @@ -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
> ===================================================================
> --- linux-pm.orig/kernel/irq/pm.c
> +++ linux-pm/kernel/irq/pm.c
> @@ -43,9 +43,6 @@ void irq_pm_install_action(struct irq_de
>  
>  	if (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;
> 



-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[RFC PATCH 0/3] genirq: mixing IRQF_NO_SUSPEND and wakeup sources on shared IRQs

[Rafael J. Wysocki]

On Thursday, February 26, 2015 09:03:47 AM Boris Brezillon wrote:
> Hi Rafael,
> 
> On Wed, 25 Feb 2015 22:59:36 +0100
> "Rafael J. Wysocki" <rjw@rjwysocki.net> wrote:
> 
> > On Tuesday, February 24, 2015 10:55:59 AM Boris Brezillon wrote:
> > > 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.
> > 
> > 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.
> 
> 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?

> 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.

> 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. :-)


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

[RFC PATCH 0/3] genirq: mixing IRQF_NO_SUSPEND and wakeup sources on shared IRQs

[Boris Brezillon]

On Thu, 26 Feb 2015 16:44:16 +0100
"Rafael J. Wysocki" <rjw@rjwysocki.net> wrote:

> On Thursday, February 26, 2015 09:03:47 AM Boris Brezillon wrote:
> > Hi Rafael,
> > 
> > On Wed, 25 Feb 2015 22:59:36 +0100
> > "Rafael J. Wysocki" <rjw@rjwysocki.net> wrote:
> > 
> > > On Tuesday, February 24, 2015 10:55:59 AM Boris Brezillon wrote:
> > > > 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.
> > > 
> > > 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.
> > 
> > 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).

> 
> > 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 ;-).




-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[RFC PATCH 0/3] genirq: mixing IRQF_NO_SUSPEND and wakeup sources on shared IRQs

[Rafael J. Wysocki]

On Thursday, February 26, 2015 04:47:24 PM Boris Brezillon wrote:
> On Thu, 26 Feb 2015 16:44:16 +0100
> "Rafael J. Wysocki" <rjw@rjwysocki.net> wrote:
> 
> > On Thursday, February 26, 2015 09:03:47 AM Boris Brezillon wrote:
> > > Hi Rafael,
> > > 
> > > On Wed, 25 Feb 2015 22:59:36 +0100
> > > "Rafael J. Wysocki" <rjw@rjwysocki.net> wrote:
> > > 
> > > > On Tuesday, February 24, 2015 10:55:59 AM Boris Brezillon wrote:
> > > > > 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.
> > > > 
> > > > 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.
> > > 
> > > 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.

The same idea I had while talking to Peter over IRC.  So the patch below
implements that.

> 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).

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(-)

Index: linux-pm/include/linux/interrupt.h
===================================================================
--- linux-pm.orig/include/linux/interrupt.h
+++ linux-pm/include/linux/interrupt.h
@@ -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)
 
Index: linux-pm/include/linux/irqdesc.h
===================================================================
--- linux-pm.orig/include/linux/irqdesc.h
+++ linux-pm/include/linux/irqdesc.h
@@ -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
Index: linux-pm/kernel/irq/pm.c
===================================================================
--- linux-pm.orig/kernel/irq/pm.c
+++ linux-pm/kernel/irq/pm.c
@@ -43,9 +43,12 @@ void irq_pm_install_action(struct irq_de
 
 	if (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_des
 
 	if (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)
Index: linux-pm/kernel/irq/manage.c
===================================================================
--- linux-pm.orig/kernel/irq/manage.c
+++ linux-pm/kernel/irq/manage.c
@@ -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);

[RFC PATCH 0/3] genirq: mixing IRQF_NO_SUSPEND and wakeup sources on shared IRQs

[Boris Brezillon]

On Thu, 26 Feb 2015 19:17:03 +0100
"Rafael J. Wysocki" <rjw@rjwysocki.net> wrote:

> On Thursday, February 26, 2015 04:47:24 PM Boris Brezillon wrote:
> > On Thu, 26 Feb 2015 16:44:16 +0100
> > "Rafael J. Wysocki" <rjw@rjwysocki.net> wrote:

[...]

> > > 
> > > 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.
> 
> 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.


-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[RFC PATCH 0/3] genirq: mixing IRQF_NO_SUSPEND and wakeup sources on shared IRQs

[Rafael J. Wysocki]

On Thursday, February 26, 2015 07:17:24 PM Boris Brezillon wrote:
> On Thu, 26 Feb 2015 19:17:03 +0100
> "Rafael J. Wysocki" <rjw@rjwysocki.net> wrote:
> 
> > On Thursday, February 26, 2015 04:47:24 PM Boris Brezillon wrote:
> > > On Thu, 26 Feb 2015 16:44:16 +0100
> > > "Rafael J. Wysocki" <rjw@rjwysocki.net> wrote:
> 
> [...]
> 
> > > > 
> > > > 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.
> > 
> > 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.


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

[PATCH] genirq / PM: Add flag for shared NO_SUSPEND interrupt lines

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

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.

Link: http://marc.info/?l=linux-kernel&m=142252777602084&w=2
Link: http://marc.info/?t=142252775300011&r=1&w=2
Linx: https://lkml.org/lkml/2014/12/15/552
Reported-by: Boris Brezillon <boris.brezillon@free-electrons.com>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 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
===================================================================
--- linux-pm.orig/include/linux/interrupt.h
+++ linux-pm/include/linux/interrupt.h
@@ -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)
 
Index: linux-pm/include/linux/irqdesc.h
===================================================================
--- linux-pm.orig/include/linux/irqdesc.h
+++ linux-pm/include/linux/irqdesc.h
@@ -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
Index: linux-pm/kernel/irq/pm.c
===================================================================
--- linux-pm.orig/kernel/irq/pm.c
+++ linux-pm/kernel/irq/pm.c
@@ -43,9 +43,12 @@ void irq_pm_install_action(struct irq_de
 
 	if (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_des
 
 	if (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)
Index: linux-pm/kernel/irq/manage.c
===================================================================
--- linux-pm.orig/kernel/irq/manage.c
+++ linux-pm/kernel/irq/manage.c
@@ -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);

[PATCH] genirq / PM: Add flag for shared NO_SUSPEND interrupt lines

[Peter Zijlstra]

On Fri, Feb 27, 2015 at 12:07:55AM +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> 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.
> 
> Link: http://marc.info/?l=linux-kernel&m=142252777602084&w=2
> Link: http://marc.info/?t=142252775300011&r=1&w=2
> Linx: https://lkml.org/lkml/2014/12/15/552
> Reported-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

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?

[PATCH] genirq / PM: Add flag for shared NO_SUSPEND interrupt lines

[Rafael J. Wysocki]

On Friday, February 27, 2015 09:38:59 AM Peter Zijlstra wrote:
> On Fri, Feb 27, 2015 at 12:07:55AM +0100, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > 
> > 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.
> > 
> > Link: http://marc.info/?l=linux-kernel&m=142252777602084&w=2
> > Link: http://marc.info/?t=142252775300011&r=1&w=2
> > Linx: https://lkml.org/lkml/2014/12/15/552
> > Reported-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> 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. :-)

> Also, should we warn if people use enable_irq_wake() where there is only
> a single descriptor with NO_SUSPEND?

We probably should do that, but that would be a separate patch IMO?

[PATCH] genirq / PM: Add flag for shared NO_SUSPEND interrupt lines

[Peter Zijlstra]

On Fri, Feb 27, 2015 at 11:13:57PM +0100, Rafael J. Wysocki wrote:
> On Friday, February 27, 2015 09:38:59 AM Peter Zijlstra wrote:

> > 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. :-)

Works for me,

Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>

> > Also, should we warn if people use enable_irq_wake() where there is only
> > a single descriptor with NO_SUSPEND?
> 
> We probably should do that, but that would be a separate patch IMO?

Agreed. Just wanted to raise the point.

[PATCH] genirq / PM: Add flag for shared NO_SUSPEND interrupt lines

[Mark Rutland]

Hi Rafael,

I'm a little late to the party here, but I have just a couple of minor
comments...

[...]

> Link: http://marc.info/?l=linux-kernel&m=142252777602084&w=2
> Link: http://marc.info/?t=142252775300011&r=1&w=2
> Linx: https://lkml.org/lkml/2014/12/15/552

s/x/k/ ?

> Reported-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  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
> ===================================================================
> --- linux-pm.orig/include/linux/interrupt.h
> +++ linux-pm/include/linux/interrupt.h
> @@ -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!

Acked-by: Mark Rutland <mark.rutland@arm.com>

Thanks,
Mark.

[PATCH] genirq: describe IRQF_COND_SUSPEND

[Mark Rutland]

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.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Boris Brezillon <boris.brezillon@free-electrons.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 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.

diff --git a/Documentation/power/suspend-and-interrupts.txt b/Documentation/power/suspend-and-interrupts.txt
index 50493c9..8afb29a 100644
--- a/Documentation/power/suspend-and-interrupts.txt
+++ b/Documentation/power/suspend-and-interrupts.txt
@@ -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.
-- 
1.9.1

[PATCH] genirq: describe IRQF_COND_SUSPEND

[Rafael J. Wysocki]

On Wednesday, March 04, 2015 08:00:40 PM Mark Rutland wrote:
> 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.
> 
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Cc: Boris Brezillon <boris.brezillon@free-electrons.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  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?

Yes, it does, thanks!

I'll queue it up along with the rest of the IRQF_COND_SUSPEND patches.


> diff --git a/Documentation/power/suspend-and-interrupts.txt b/Documentation/power/suspend-and-interrupts.txt
> index 50493c9..8afb29a 100644
> --- a/Documentation/power/suspend-and-interrupts.txt
> +++ b/Documentation/power/suspend-and-interrupts.txt
> @@ -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.
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

[PATCH] genirq: describe IRQF_COND_SUSPEND

[Alexandre Belloni]

tiny tiny nitpick:

On 04/03/2015 at 20:00:40 +0000, Mark Rutland wrote :
> 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
  ^ an


> +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 ?

-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[PATCH] genirq: describe IRQF_COND_SUSPEND

[Arnd Bergmann]

On Wednesday 04 March 2015 23:17:29 Alexandre Belloni wrote:
> On 04/03/2015 at 20:00:40 +0000, Mark Rutland wrote :
> > 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
>   ^ an
> 
> 
> > +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 ?

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

[PATCH] genirq: describe IRQF_COND_SUSPEND

[Mark Rutland]

On Wed, Mar 04, 2015 at 10:17:29PM +0000, Alexandre Belloni wrote:
> tiny tiny nitpick:
> 
> On 04/03/2015 at 20:00:40 +0000, Mark Rutland wrote :
> > 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
>   ^ an

Whoops.

Rafael, are you happy to fix that up, or would you like me to resend?

> > +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 ?

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.

[PATCH] genirq: describe IRQF_COND_SUSPEND

[Alexandre Belloni]

On 05/03/2015 at 11:04:11 +0000, Mark Rutland wrote :
> > > +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 ?
> 
> 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.


-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[PATCH] genirq: describe IRQF_COND_SUSPEND

[Mark Rutland]

On Thu, Mar 05, 2015 at 11:33:06AM +0000, Alexandre Belloni wrote:
> On 05/03/2015 at 11:04:11 +0000, Mark Rutland wrote :
> > > > +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 ?
> > 
> > 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.

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.

[PATCH] genirq: describe IRQF_COND_SUSPEND

[Rafael J. Wysocki]

On Thursday, March 05, 2015 11:04:11 AM Mark Rutland wrote:
> On Wed, Mar 04, 2015 at 10:17:29PM +0000, Alexandre Belloni wrote:
> > tiny tiny nitpick:
> > 
> > On 04/03/2015 at 20:00:40 +0000, Mark Rutland wrote :
> > > 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
> >   ^ an
> 
> Whoops.
> 
> Rafael, are you happy to fix that up, or would you like me to resend?

Fixed, thanks!


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

[PATCH] genirq / PM: Add flag for shared NO_SUSPEND interrupt lines

[Rafael J. Wysocki]

On Wednesday, March 04, 2015 07:42:46 PM Mark Rutland wrote:
> Hi Rafael,
> 
> I'm a little late to the party here, but I have just a couple of minor
> comments...
> 
> [...]
> 
> > Link: http://marc.info/?l=linux-kernel&m=142252777602084&w=2
> > Link: http://marc.info/?t=142252775300011&r=1&w=2
> > Linx: https://lkml.org/lkml/2014/12/15/552
> 
> s/x/k/ ?

Yes, thanks!

> > Reported-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >  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
> > ===================================================================
> > --- linux-pm.orig/include/linux/interrupt.h
> > +++ linux-pm/include/linux/interrupt.h
> > @@ -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!
> 
> Acked-by: Mark Rutland <mark.rutland@arm.com>

Thanks!