Date:	Tue, 05 Aug 2014 17:25:42 +0200
From:	"Rafael J. Wysocki" <rjw@...ysocki.net>
To:	Thomas Gleixner <tglx@...utronix.de>
Cc:	Peter Zijlstra <peterz@...radead.org>,
	linux-kernel@...r.kernel.org,
	Linux PM list <linux-pm@...r.kernel.org>,
	Dmitry Torokhov <dtor@...gle.com>
Subject: [PATCH 2/5] irq / PM: Fix IRQF_NO_SUSPEND problem with shared interrupts

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

Since __disable_irq() only checks IRQF_NO_SUSPEND for the first
irqaction in a given irq_desc, that value of that bit for the
first irqaction affects all of the other irqactions sharing the
interrupt with it.  This is problematic in two cases.

First, if IRQF_NO_SUSPEND is set in the first irqaction and unset
in at least one of the other irqactions sharing the same interrupt,
the interrupt handlers in the other irqactions with IRQF_NO_SUSPEND
unset will be invoked after suspend_device_irqs() has returned even
though they are not supposed to.  That shouldn't be a problem if
those interrupt handlers are implemented correctly and the
corresponding devices are properly suspended, but it may lead to
functional issues otherwise.

Second, if IRQF_NO_SUSPEND is unset in the first irqaction and set
in at least one of the other irqactions sharing the same interrupt,
the interrupt handlers in the other irqactions with IRQF_NO_SUSPEND
set will not be invoked after suspend_device_irqs() has returned, but
they are supposed to be invoked then.  That may be a problem if the
corresponding devices are wakeup devices or if their interrupts have
to be properly handled during system suspend/resume too for other
reasons, which is the case for timer interrupts or the ACPI SCI for
example.

To address this, introduce a wrapper interrupt handler for shared
interrupts, irq_shared_wrapper_handler(), and make __setup_irq()
use it for shared interrupts by default.  That wrapper handler
simply invokes the original handler supplied while requesting the
IRQ if the (new) irqs_suspended global variable is unset.  Otherwise
it returns IRQ_NONE.  The wrapper handler is used for all irqactions
sharing IRQs except for irqactions with IRQF_NO_SUSPEND set that
retain their original handlers.

The irqs_suspended variable is set by suspend_device_irqs() and
unset by resume_device_irqs().  It is checked by note_interrupt()
(now called unconditionally) and if set, any unhandled interrupt
will cause the system to wake up (or system suspend in progress
to be aborted).

The additional (new) skip_suspend field in struct irq_desc is
used to track if any irqactions in the given desc have
IRQF_NO_SUSPEND set.  It is used by suspend_device_irqs() to
decide which IRQs to disable during system suspends.  This way,
all IRQs with at least one irqaction having IRQF_NO_SUSPEND set
(skip_suspend greater than 0) will stay enabled after
suspend_device_irqs(), but if any irqactions belonging to them
have IRQF_NO_SUSPEND unset, they will use the wrapper handler
that will return IRQ_NONE for them (causing system suspend to
be aborted or system wakeup from suspend-to-idle to happen).

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
---
 include/linux/interrupt.h |    4 +++
 include/linux/irqdesc.h   |    1 
 kernel/irq/handle.c       |    3 --
 kernel/irq/internals.h    |    3 ++
 kernel/irq/manage.c       |   52 ++++++++++++++++++++++++++++++++++++++++++++--
 kernel/irq/pm.c           |   10 +++++---
 kernel/irq/spurious.c     |   13 +++++++++++
 7 files changed, 78 insertions(+), 8 deletions(-)

Index: linux-pm/kernel/irq/manage.c
===================================================================
--- linux-pm.orig/kernel/irq/manage.c
+++ linux-pm/kernel/irq/manage.c
@@ -385,7 +385,7 @@ setup_affinity(unsigned int irq, struct
 void __disable_irq(struct irq_desc *desc, unsigned int irq, bool suspend)
 {
 	if (suspend) {
-		if (!desc->action || (desc->action->flags & IRQF_NO_SUSPEND))
+		if (!desc->action || desc->skip_suspend)
 			return;
 		desc->istate |= IRQS_SUSPENDED;
 	}
@@ -658,6 +658,43 @@ int irq_set_parent(int irq, int parent_i
 }
 #endif
 
+bool irqs_suspended __read_mostly;
+
+void set_irqs_suspended(bool on)
+{
+	struct irq_desc *desc;
+	int irq;
+
+	irqs_suspended = on;
+	for_each_irq_desc(irq, desc)
+		synchronize_irq(irq);
+}
+
+/*
+ * Wrapper handler for shared interrupts.
+ */
+static irqreturn_t irq_shared_wrapper_handler(int irq, void *dev_id)
+{
+	struct irqaction *action = dev_id;
+
+	if (unlikely(irqs_suspended))
+		return IRQ_NONE;
+
+	return action->s_handler(irq, action->s_dev_id);
+}
+
+static void irq_shared_wrap_handler(struct irqaction *action)
+{
+	if (action->handler == irq_shared_wrapper_handler ||
+	    (action->flags & IRQF_NO_SUSPEND))
+		return;
+
+	action->s_handler = action->handler;
+	action->handler = irq_shared_wrapper_handler;
+	action->s_dev_id = action->dev_id;
+	action->dev_id = action;
+}
+
 /*
  * Default primary interrupt handler for threaded interrupts. Is
  * assigned as primary handler when request_threaded_irq is called
@@ -1088,6 +1125,7 @@ __setup_irq(unsigned int irq, struct irq
 
 		/* add new interrupt at end of irq queue */
 		do {
+			irq_shared_wrap_handler(old);
 			/*
 			 * Or all existing action->thread_mask bits,
 			 * so we can find the next zero bit for this
@@ -1097,6 +1135,7 @@ __setup_irq(unsigned int irq, struct irq
 			old_ptr = &old->next;
 			old = *old_ptr;
 		} while (old);
+		irq_shared_wrap_handler(new);
 		shared = 1;
 	}
 
@@ -1222,6 +1261,9 @@ __setup_irq(unsigned int irq, struct irq
 	desc->irq_count = 0;
 	desc->irqs_unhandled = 0;
 
+	if (new->flags & IRQF_NO_SUSPEND)
+		desc->skip_suspend++;
+
 	/*
 	 * Check whether we disabled the irq via the spurious handler
 	 * before. Reenable it and give it another chance.
@@ -1328,13 +1370,19 @@ static struct irqaction *__free_irq(unsi
 			return NULL;
 		}
 
-		if (action->dev_id == dev_id)
+		if (action->dev_id == dev_id || action->s_dev_id == dev_id)
 			break;
 		action_ptr = &action->next;
 	}
 
 	/* Found it - now remove it from the list of entries: */
 	*action_ptr = action->next;
+	if (action->flags & IRQF_NO_SUSPEND) {
+		desc->skip_suspend--;
+	} else if (action->handler == irq_shared_wrapper_handler) {
+		action->handler = action->s_handler;
+		action->dev_id = action->s_dev_id;
+	}
 
 	/* If this was the last handler, shut down the IRQ line: */
 	if (!desc->action) {
Index: linux-pm/include/linux/interrupt.h
===================================================================
--- linux-pm.orig/include/linux/interrupt.h
+++ linux-pm/include/linux/interrupt.h
@@ -90,8 +90,10 @@ typedef irqreturn_t (*irq_handler_t)(int
 /**
  * struct irqaction - per interrupt action descriptor
  * @handler:	interrupt handler function
+ * @s_handler:	interrupt handler function for shared interrupts
  * @name:	name of the device
  * @dev_id:	cookie to identify the device
+ * @s_dev_id:	cookie to identify the device for shared interrupts
  * @percpu_dev_id:	cookie to identify the device
  * @next:	pointer to the next irqaction for shared interrupts
  * @irq:	interrupt number
@@ -105,6 +107,8 @@ typedef irqreturn_t (*irq_handler_t)(int
 struct irqaction {
 	irq_handler_t		handler;
 	void			*dev_id;
+	irq_handler_t		s_handler;
+	void			*s_dev_id;
 	void __percpu		*percpu_dev_id;
 	struct irqaction	*next;
 	irq_handler_t		thread_fn;
Index: linux-pm/kernel/irq/handle.c
===================================================================
--- linux-pm.orig/kernel/irq/handle.c
+++ linux-pm/kernel/irq/handle.c
@@ -175,8 +175,7 @@ handle_irq_event_percpu(struct irq_desc
 
 	add_interrupt_randomness(irq, flags);
 
-	if (!noirqdebug)
-		note_interrupt(irq, desc, retval);
+	note_interrupt(irq, desc, retval);
 	return retval;
 }
 
Index: linux-pm/kernel/irq/spurious.c
===================================================================
--- linux-pm.orig/kernel/irq/spurious.c
+++ linux-pm/kernel/irq/spurious.c
@@ -13,6 +13,7 @@
 #include <linux/interrupt.h>
 #include <linux/moduleparam.h>
 #include <linux/timer.h>
+#include <linux/suspend.h>
 
 #include "internals.h"
 
@@ -275,6 +276,18 @@ try_misrouted_irq(unsigned int irq, stru
 void note_interrupt(unsigned int irq, struct irq_desc *desc,
 		    irqreturn_t action_ret)
 {
+	if (unlikely(irqs_suspended && action_ret == IRQ_NONE)) {
+		pr_err("IRQ %d: Unhandled while suspended\n", irq);
+		desc->istate |= IRQS_SUSPENDED;
+		desc->depth++;
+		irq_disable(desc);
+		pm_system_wakeup();
+		return;
+	}
+
+	if (noirqdebug)
+		return;
+
 	if (desc->istate & IRQS_POLL_INPROGRESS ||
 	    irq_settings_is_polled(desc))
 		return;
Index: linux-pm/kernel/irq/pm.c
===================================================================
--- linux-pm.orig/kernel/irq/pm.c
+++ linux-pm/kernel/irq/pm.c
@@ -10,6 +10,7 @@
 #include <linux/module.h>
 #include <linux/interrupt.h>
 #include <linux/syscore_ops.h>
+#include <linux/suspend.h>
 
 #include "internals.h"
 
@@ -33,10 +34,7 @@ void suspend_device_irqs(void)
 		__disable_irq(desc, irq, true);
 		raw_spin_unlock_irqrestore(&desc->lock, flags);
 	}
-
-	for_each_irq_desc(irq, desc)
-		if (desc->istate & IRQS_SUSPENDED)
-			synchronize_irq(irq);
+	set_irqs_suspended(true);
 }
 EXPORT_SYMBOL_GPL(suspend_device_irqs);
 
@@ -90,6 +88,7 @@ device_initcall(irq_pm_init_ops);
  */
 void resume_device_irqs(void)
 {
+	set_irqs_suspended(false);
 	resume_irqs(false);
 }
 EXPORT_SYMBOL_GPL(resume_device_irqs);
@@ -102,6 +101,9 @@ int check_wakeup_irqs(void)
 	struct irq_desc *desc;
 	int irq;
 
+	if (pm_wakeup_pending())
+		return -EBUSY;
+
 	for_each_irq_desc(irq, desc) {
 		/*
 		 * Only interrupts which are marked as wakeup source
Index: linux-pm/kernel/irq/internals.h
===================================================================
--- linux-pm.orig/kernel/irq/internals.h
+++ linux-pm/kernel/irq/internals.h
@@ -17,6 +17,9 @@
 #define istate core_internal_state__do_not_mess_with_it
 
 extern bool noirqdebug;
+extern bool irqs_suspended;
+
+extern void set_irqs_suspended(bool on);
 
 /*
  * Bits used by threaded handlers:
Index: linux-pm/include/linux/irqdesc.h
===================================================================
--- linux-pm.orig/include/linux/irqdesc.h
+++ linux-pm/include/linux/irqdesc.h
@@ -51,6 +51,7 @@ struct irq_desc {
 	unsigned int		core_internal_state__do_not_mess_with_it;
 	unsigned int		depth;		/* nested irq disables */
 	unsigned int		wake_depth;	/* nested wake enables */
+	unsigned int		skip_suspend;
 	unsigned int		irq_count;	/* For detecting broken IRQs */
 	unsigned long		last_unhandled;	/* Aging timer for unhandled count */
 	unsigned int		irqs_unhandled;

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists