Seeking clarity on IRQCHIP_MASK_ON_SUSPEND

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

...

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

Thomas,

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.

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

So for this thing you need a IRQCHIP_MASK_BEFORE_SUSPEND variant?

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.

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.

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

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.

And have the same "keep track of wakeup interrupts" code over and over in the drivers.

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

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

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

index e68a8e5..d46f6c1 100644
@@ -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) { };

index a5261e3..fbd9d7b 100644
@@ -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 */

index 3728c97..836737b 100644
@@ -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);
+}

index 16a0f77..05054a4 100644
@@ -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);