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