Seeking clarity on IRQCHIP_MASK_ON_SUSPEND

Seeking clarity on IRQCHIP_MASK_ON_SUSPEND

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 3026 bytes --]


The IRQCHIP_MASK_ON_SUSPEND flag seems to be hard to use correctly, so either
I'm understanding it wrongly, or it could be made easier to use.
If the first case, I'm hoping that some improvement to documentation might
result.  If the second, then maybe we can fix the code.

As I understand it, the need for IRQCHIP_MASK_ON_SUSPEND comes from the fact
that interrupt masking happens lazily.  When an interrupt is disabled the
hardware isn't told about that immediately, only internal data structures are
updated.  If/when the interrupt actually happens, only then is it masked and
ignored.
This cannot really work in suspend as masking after the interrupt fires means
that we have already woken from suspend.

So suspend_device_irqs() (called between the suspend_late handlers and the
suspend_noirq handlers) just disables all interrupts and check_wakeup_irqs()
(which is called very late) is left to talk to the hardware and actually mask
those which should not cause a wake-from-suspend.

The problem is that check_wakeup_irqs() is called so late that it might not be
possible to talk to the hardware.  For example, on the OMAP3 platform,
runtime power management will gate the 'iclk' (interface clock) and possibly
other clocks so that it is no longer possible to talk over an i2c bus or even
directly to the GPIO SoC module to effect the masking.
As runtime PM is disabled at this stage of the suspend cycle it is not even
possible to turn the iclk back on, mask the interrupt, then turn it off
again.

So it seems to me that either:

 1/ irq_chip devices need to mask any non-wakeup interrupts in ->suspend or
    possibly in ->suspend_late before runtimePM has switched the interface
    off, making IRQCHIP_MASK_ON_SUSPEND essentially useless except for some
    rare cases.
or
 2/ IRQCHIP_MASK_ON_SUSPEND needs to happen much earlier, probably before
    the suspend_late callbacks.

It might be tempting to change suspend_device_irqs() to disable interrupts in
such a way that the 'mask' happen synchronously (for non-wakeup interrupts).
However I don't think that would work as it happens after suspend_late and I
think suspend_late is allowed to power_down devices (and iclks).

I *think* the correct answer is '1'.  In that case I would love to know when
IRQCHIP_MASK_ON_SUSPEND can be used correctly (if ever).  I'm hoping someone
who works with interrupts and power management more than I can help me
understand...

There are currently only two drivers that set IRQCHIP_MASK_ON_SUSPEND: 
  arch/arm/mach-omap2/omap-wakeupgen.c
and 
  drivers/mfd/pm8xxx-irq.c

The former is in an 'always-on' power domain so the interaction with runtime
PM presumably doesn't affect it.

The later ... I don't think will compile.  It is only used by pm8921-core.c,
and that requires #include <linux/msm_ssbi.h>, which doesn't exist in
mainline.

Is anyone able to give a definitive answer on this?  Should
IRQCHIP_MASK_ON_SUSPEND be removed?


Thanks,
NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: Seeking clarity on IRQCHIP_MASK_ON_SUSPEND

[Thomas Gleixner]

On Mon, 10 Sep 2012, NeilBrown wrote:
> 
> The IRQCHIP_MASK_ON_SUSPEND flag seems to be hard to use correctly, so either
> I'm understanding it wrongly, or it could be made easier to use.
> If the first case, I'm hoping that some improvement to documentation might
> result.  If the second, then maybe we can fix the code.
... 
> Is anyone able to give a definitive answer on this?  Should
> IRQCHIP_MASK_ON_SUSPEND be removed?

The whole point of IRQCHIP_MASK_ON_SUSPEND is to deal with hardware
designed by geniuses.

Most SoCs have a way to mark the interrupts which serve as a wake up
source as such. All other interrupts are magically "masked" on entry
to suspend.

Now there is hardware which is missing such a control, so we need to
mask the non wakeup interrupts right before going into suspend.

That's what IRQCHIP_MASK_ON_SUSPEND does. Not more, not less. See
commit d209a699a0b for more ugly details.

You might be looking for a different functionality. Can you explain
what you need?

Thanks,

	tglx

Re: Seeking clarity on IRQCHIP_MASK_ON_SUSPEND

[Shilimkar, Santosh]

Thomas,

On Mon, Sep 10, 2012 at 3:58 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> On Mon, 10 Sep 2012, NeilBrown wrote:
>>
>> The IRQCHIP_MASK_ON_SUSPEND flag seems to be hard to use correctly, so either
>> I'm understanding it wrongly, or it could be made easier to use.
>> If the first case, I'm hoping that some improvement to documentation might
>> result.  If the second, then maybe we can fix the code.
> ...
>> Is anyone able to give a definitive answer on this?  Should
>> IRQCHIP_MASK_ON_SUSPEND be removed?
>
> The whole point of IRQCHIP_MASK_ON_SUSPEND is to deal with hardware
> designed by geniuses.
>
> Most SoCs have a way to mark the interrupts which serve as a wake up
> source as such. All other interrupts are magically "masked" on entry
> to suspend.
>
Just to support this, IRQCHIP_MASK_ON_SUSPEND does work with quite
a few ARM platforms too. OMAP already uses it in mainline and I have
seen patches for U500 and Tegra SOCs. Most of these usages are with
IRQ chips who doesn't have any driver run time PM supported and
the IRQ CHIP itself is shutdown when the CPU/CPU cluster gets
power down. So as far as functionality concerned with the flag,
it does what it suppose to do.

> Now there is hardware which is missing such a control, so we need to
> mask the non wakeup interrupts right before going into suspend.
>
> That's what IRQCHIP_MASK_ON_SUSPEND does. Not more, not less. See
> commit d209a699a0b for more ugly details.
>
> You might be looking for a different functionality. Can you explain
> what you need?
>
Neil's email came from a discussion on the usage of this flag for
OMAP GPIO irqchip which I proposed. With the flag, when the
lazy check_irq routine is called, the GPIO driver is runtime suspended
and hence the late mask/unmask calls take abort(clocks are already gated).
GPIO IRQCHIP is secondary IRQCHIP connected to 1 interrupt line
per bank(32 GPIOs) to the primary interrupt controller IRQCHIP.

Hope this gives bit more clarity to the discussed problem.

Regards
Santosh

Re: Seeking clarity on IRQCHIP_MASK_ON_SUSPEND

[Thomas Gleixner]

On Mon, 10 Sep 2012, Shilimkar, Santosh wrote:
> Thomas,
> 
> On Mon, Sep 10, 2012 at 3:58 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> > On Mon, 10 Sep 2012, NeilBrown wrote:
> >>
> >> The IRQCHIP_MASK_ON_SUSPEND flag seems to be hard to use correctly, so either
> >> I'm understanding it wrongly, or it could be made easier to use.
> >> If the first case, I'm hoping that some improvement to documentation might
> >> result.  If the second, then maybe we can fix the code.
> > ...
> >> Is anyone able to give a definitive answer on this?  Should
> >> IRQCHIP_MASK_ON_SUSPEND be removed?
> >
> > The whole point of IRQCHIP_MASK_ON_SUSPEND is to deal with hardware
> > designed by geniuses.
> >
> > Most SoCs have a way to mark the interrupts which serve as a wake up
> > source as such. All other interrupts are magically "masked" on entry
> > to suspend.
> >
> Just to support this, IRQCHIP_MASK_ON_SUSPEND does work with quite
> a few ARM platforms too. OMAP already uses it in mainline and I have
> seen patches for U500 and Tegra SOCs. Most of these usages are with
> IRQ chips who doesn't have any driver run time PM supported and
> the IRQ CHIP itself is shutdown when the CPU/CPU cluster gets
> power down. So as far as functionality concerned with the flag,
> it does what it suppose to do.
> 
> > Now there is hardware which is missing such a control, so we need to
> > mask the non wakeup interrupts right before going into suspend.
> >
> > That's what IRQCHIP_MASK_ON_SUSPEND does. Not more, not less. See
> > commit d209a699a0b for more ugly details.
> >
> > You might be looking for a different functionality. Can you explain
> > what you need?
> >
> Neil's email came from a discussion on the usage of this flag for
> OMAP GPIO irqchip which I proposed. With the flag, when the
> lazy check_irq routine is called, the GPIO driver is runtime suspended
> and hence the late mask/unmask calls take abort(clocks are already gated).
> GPIO IRQCHIP is secondary IRQCHIP connected to 1 interrupt line
> per bank(32 GPIOs) to the primary interrupt controller IRQCHIP.

So for this thing you need a IRQCHIP_MASK_BEFORE_SUSPEND variant? 

Re: Seeking clarity on IRQCHIP_MASK_ON_SUSPEND

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 3745 bytes --]

On Mon, 10 Sep 2012 12:28:35 +0200 (CEST) Thomas Gleixner
<tglx@linutronix.de> wrote:

> On Mon, 10 Sep 2012, NeilBrown wrote:
> > 
> > The IRQCHIP_MASK_ON_SUSPEND flag seems to be hard to use correctly, so either
> > I'm understanding it wrongly, or it could be made easier to use.
> > If the first case, I'm hoping that some improvement to documentation might
> > result.  If the second, then maybe we can fix the code.
> ... 
> > Is anyone able to give a definitive answer on this?  Should
> > IRQCHIP_MASK_ON_SUSPEND be removed?
> 
> The whole point of IRQCHIP_MASK_ON_SUSPEND is to deal with hardware
> designed by geniuses.
> 
> Most SoCs have a way to mark the interrupts which serve as a wake up
> source as such. All other interrupts are magically "masked" on entry
> to suspend.
> 
> Now there is hardware which is missing such a control, so we need to
> mask the non wakeup interrupts right before going into suspend.

Even allowing for the obvious sarcasm, I'm having a lot of trouble
understanding the nature of this 'bad' hardware.

My understand of the SoC world is that the goal is for 'suspend' to be no
different from deep runtime PM (from a hardware perspective).  So where there
is a way to mark an interrupt as a wakeup source, this is used both to support
wakeup from suspend, and to support wakeup from deep runtime-PM

When it is used to wake-from-runtime-PM, the lazy interrupt masking is
sufficient to mask it when needed - the extra wakeup isn't a big cost.
When it is used to wake-from-suspend - lazy interrupt masking can cause an
unwanted wake from suspend which is much more expensive.

So even with sane hardware that supports marking interrupts as wakeup
sources, we still need do extra explicit masking for the suspend case, and
IRQCHIP_MASK_ON_SUSPEND looks like the tool to use ... except that as
described previously it doesn't work.

> 
> That's what IRQCHIP_MASK_ON_SUSPEND does. Not more, not less. See
> commit d209a699a0b for more ugly details.

One detail that I get from that commit is that it was 'Reviewed-by' someone
from 'codeaurora', and that name appears in the pm8xxx-irq.c driver which is
one of the two users.  However that driver can only be used if
CONFIG_MSM_SSBI, and nothing ever devices that so it all seems completely
theoretical.

> 
> 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
issue in the interrupt chip drivers so maybe that is the best place to handle
it.

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.

Thank,
NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: Seeking clarity on IRQCHIP_MASK_ON_SUSPEND

[Thomas Gleixner]

On Tue, 11 Sep 2012, NeilBrown wrote:
> On Mon, 10 Sep 2012 12:28:35 +0200 (CEST) Thomas Gleixner
> <tglx@linutronix.de> 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.
 
> 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.

This GPIO driver at hand is probably not the last one which requires
this and we really want to do stuff in the core code instead of having
random implementations of the same stuff all over the place.

Thanks,

	tglx

Re: Seeking clarity on IRQCHIP_MASK_ON_SUSPEND

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 6878 bytes --]

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

> On Tue, 11 Sep 2012, NeilBrown wrote:
> > On Mon, 10 Sep 2012 12:28:35 +0200 (CEST) Thomas Gleixner
> > <tglx@linutronix.de> 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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]