On Thursday, February 26, 2015 04:47:24 PM Boris Brezillon wrote:

On Thu, 26 Feb 2015 16:44:16 +0100
"Rafael J. Wysocki" [off-list ref] wrote:

quoted

On Thursday, February 26, 2015 09:03:47 AM Boris Brezillon wrote:

quoted

Hi Rafael,

On Wed, 25 Feb 2015 22:59:36 +0100
"Rafael J. Wysocki" [off-list ref] wrote:

quoted

On Tuesday, February 24, 2015 10:55:59 AM Boris Brezillon wrote:

quoted

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