Re: Seeking clarity on IRQCHIP_MASK_ON_SUSPEND

[NeilBrown]

On Tue, 11 Sep 2012 15:20:53 +0200 (CEST) Thomas Gleixner
<tglx@xxxxxxxxxxxxx> wrote:

> On Tue, 11 Sep 2012, NeilBrown wrote:
> > On Mon, 10 Sep 2012 12:28:35 +0200 (CEST) Thomas Gleixner
> > <tglx@xxxxxxxxxxxxx> wrote:
> > > You might be looking for a different functionality. Can you explain
> > > what you need?
> > 
> > I want as particular GPIO interrupt to be masked before entering suspend.
> > I produced code to get the ->suspend() callback to perform this masking.
> > Another developer (Santosh) felt that IRQCHIP_MASK_ON_SUSPEND was the
> > preferred way to do this and on the surface this looks like it should be
> > correct.  However it doesn't work as explained previously.
> > I want a resolution to this difference of opinion that doesn't just sweep the
> > issue under that carpet but provides a clear answer to this sort of issue.
> > 
> > My current opinion is that IRQCHIP_MASK_ON_SUSPEND should be discarded.  The
> > patch which introduced it says:
> > 
> >     Rather than working around this in the affected interrupt chip
> >     implementations we can solve this elegant in the core code itself.
> >     
> > It appears that the solution in core code, while elegant, is wrong.  It
> > happens too late to be generally usable.  It is easy enough to handle this
> 
> Sigh. The flag was invented with the semantics to keep the current
> "check for any interrupt" pending functionality alive and then mask it
> right before going down, so only the designated wakeup interrupts can
> wake the device. That was the result of the discussion back then and
> that was what the developer wanted to achieve with his driver suspend
> hackery.
> 
> > issue in the interrupt chip drivers so maybe that is the best place
> > to handle it.
> 
> And have the same "keep track of wakeup interrupts" code over and over
> in the drivers.

We could have a 'mask my non-wakeup interrupts' library call that interested
drivers could call when appropriate, rather than having a flag to be set when
appropriate.

In the case of the gpio_omap driver, simply disabling wakeup requires less
talking to hardware than a full 'irq_mask' call.  So if we give the driver
more information about what is happening, it can implement it more
efficiently.

(I generally prefer providing code to call rather than flags to set.  It
gives more control with introducing more complexity).

>  
> > The the very least I think we need a big comment saying the
> > IRQCHIP_MASK_ON_SUSPEND can only be used for irqchips which can always be
> > programmed, even when they are suspended from an runtime-PM perspective,
> > and that those chips must handle masking in their 'suspend' callback.
> 
> Sigh, no. Either we make IRQCHIP_MASK_ON_SUSPEND into an
> implementation which masks the interrupts early, if the existing users
> find this acceptable or have a separate IRQCHIP_MASK_BEFORE_SUSPEND
> flag.

Something like this may be?  Just compile-tested so far.
I'll see if I can understand the two current users of IRQCHIP_MASK_ON_SUSPEND
well enough to see if they should work with this flag.  I suspect they will.

Thanks,
NeilBrown



diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
index e68a8e5..d46f6c1 100644
--- a/include/linux/interrupt.h
+++ b/include/linux/interrupt.h
@@ -228,8 +228,12 @@ extern void suspend_device_irqs(void);
 extern void resume_device_irqs(void);
 #ifdef CONFIG_PM_SLEEP
 extern int check_wakeup_irqs(void);
+void mask_non_wakeup_irqs(void);
+void unmask_non_wakeup_irqs(void);
 #else
 static inline int check_wakeup_irqs(void) { return 0; }
+static inline void mask_non_wakeup_irqs(void) { return 0; }
+static inline void unmask_non_wakeup_irqs(void) { return 0; }
 #endif
 #else
 static inline void suspend_device_irqs(void) { };
diff --git a/include/linux/irq.h b/include/linux/irq.h
index a5261e3..fbd9d7b 100644
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -340,7 +340,10 @@ struct irq_chip {
  *
  * IRQCHIP_SET_TYPE_MASKED:	Mask before calling chip.irq_set_type()
  * IRQCHIP_EOI_IF_HANDLED:	Only issue irq_eoi() when irq was handled
- * IRQCHIP_MASK_ON_SUSPEND:	Mask non wake irqs in the suspend path
+ * IRQCHIP_MASK_ON_SUSPEND:	Mask non wake irqs late in the suspend path
+ * IRQCHIP_MASK_PRE_SUSPEND:	Mask non wake irqs early in the suspend path
+ *				before devices are powered off or interrupts are
+ *				disabled.
  * IRQCHIP_ONOFFLINE_ENABLED:	Only call irq_on/off_line callbacks
  *				when irq enabled
  * IRQCHIP_SKIP_SET_WAKE:	Skip chip.irq_set_wake(), for this irq chip
@@ -351,6 +354,7 @@ enum {
 	IRQCHIP_MASK_ON_SUSPEND		= (1 <<  2),
 	IRQCHIP_ONOFFLINE_ENABLED	= (1 <<  3),
 	IRQCHIP_SKIP_SET_WAKE		= (1 <<  4),
+	IRQCHIP_MASK_PRE_SUSPEND	= (1 <<  5),
 };
 
 /* This include will go away once we isolated irq_desc usage to core code */
diff --git a/kernel/irq/pm.c b/kernel/irq/pm.c
index 3728c97..836737b 100644
--- a/kernel/irq/pm.c
+++ b/kernel/irq/pm.c
@@ -128,3 +128,35 @@ int check_wakeup_irqs(void)
 
 	return 0;
 }
+
+/**
+ * mask_non_wakeup_irqs - irqs that should not wake from suspend should be
+ *       masked now.
+ * This is called after devices have been suspended, but before suspend_late
+ * and before interrupts are disabled.  This means it should still be possible
+ * to talk to the interrupt controller to effect the mask.
+ */
+void mask_non_wakeup_irqs(void)
+{
+	struct irq_desc *desc;
+	int irq;
+
+	for_each_irq_desc(irq, desc)
+		if ((irq_desc_get_chip(desc)->flags & IRQCHIP_MASK_PRE_SUSPEND)
+		    && !irqd_is_wakeup_set(&desc->irq_data)
+		    && !irqd_irq_masked(&desc->irq_data))
+			mask_irq(desc);
+}
+
+void unmask_non_wakeup_irqs(void)
+{
+	struct irq_desc *desc;
+	int irq;
+
+	for_each_irq_desc(irq, desc)
+		if ((irq_desc_get_chip(desc)->flags & IRQCHIP_MASK_PRE_SUSPEND)
+		    && !irqd_is_wakeup_set(&desc->irq_data)
+		    && irqd_irq_masked(&desc->irq_data)
+		    && !irqd_irq_disabled(&desc->irq_data))
+			unmask_irq(desc);
+}
diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
index 16a0f77..05054a4 100644
--- a/kernel/power/suspend.c
+++ b/kernel/power/suspend.c
@@ -23,6 +23,7 @@
 #include <linux/slab.h>
 #include <linux/export.h>
 #include <linux/suspend.h>
+#include <linux/interrupt.h>
 #include <linux/syscore_ops.h>
 #include <trace/events/power.h>
 
@@ -222,11 +223,12 @@ int suspend_devices_and_enter(suspend_state_t state)
 	if (suspend_test(TEST_DEVICES))
 		goto Recover_platform;
 
+	mask_non_wakeup_irqs();
 	do {
 		error = suspend_enter(state, &wakeup);
 	} while (!error && !wakeup
 		&& suspend_ops->suspend_again && suspend_ops->suspend_again());
-
+	unmask_non_wakeup_irqs();
  Resume_devices:
 	suspend_test_start();
 	dpm_resume_end(PMSG_RESUME);

Attachment: signature.asc
Description: PGP signature