diff mbox series

[v3,05/11] genirq: Shutdown irq chips in suspend/resume during hibernation

Message ID d9bcd552c946ac56f3f17cc0c1be57247d4a3004.1598042152.git.anchalag@amazon.com
State Not Applicable
Delegated to: David Miller
Headers show
Series Fix PM hibernation in Xen guests | expand

Commit Message

Thomas Gleixner Aug. 21, 2020, 10:27 p.m. UTC 
Many legacy device drivers do not implement power management (PM)
functions which means that interrupts requested by these drivers stay
in active state when the kernel is hibernated.

This does not matter on bare metal and on most hypervisors because the
interrupt is restored on resume without any noticable side effects as
it stays connected to the same physical or virtual interrupt line.

The XEN interrupt mechanism is different as it maintains a mapping
between the Linux interrupt number and a XEN event channel. If the
interrupt stays active on hibernation this mapping is preserved but
there is unfortunately no guarantee that on resume the same event
channels are reassigned to these devices. This can result in event
channel conflicts which prevent the affected devices from being
restored correctly.

One way to solve this would be to add the necessary power management
functions to all affected legacy device drivers, but that's a
questionable effort which does not provide any benefits on non-XEN
environments.

The least intrusive and most efficient solution is to provide a
mechanism which allows the core interrupt code to tear down these
interrupts on hibernation and bring them back up again on resume. This
allows the XEN event channel mechanism to assign an arbitrary event
channel on resume without affecting the functionality of these
devices.

Fortunately all these device interrupts are handled by a dedicated XEN
interrupt chip so the chip can be marked that all interrupts connected
to it are handled this way. This is pretty much in line with the other
interrupt chip specific quirks, e.g. IRQCHIP_MASK_ON_SUSPEND.

Add a new quirk flag IRQCHIP_SHUTDOWN_ON_SUSPEND and add support for
it the core interrupt suspend/resume paths.

Changelog:
v1->v2: Corrected the author's name to tglx@
Signed-off-by: Anchal Agarwal <anchalag@amazon.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 drivers/xen/events/events_base.c |  1 +
 include/linux/irq.h              |  2 ++
 kernel/irq/chip.c                |  2 +-
 kernel/irq/internals.h           |  1 +
 kernel/irq/pm.c                  | 31 ++++++++++++++++++++++---------
 5 files changed, 27 insertions(+), 10 deletions(-)

Comments

Thomas Gleixner Aug. 22, 2020, 12:36 a.m. UTC | #1 
On Fri, Aug 21 2020 at 22:27, Thomas Gleixner wrote:
> Add a new quirk flag IRQCHIP_SHUTDOWN_ON_SUSPEND and add support for
> it the core interrupt suspend/resume paths.
>
> Changelog:
> v1->v2: Corrected the author's name to tglx@

Can you please move that Changelog part below the --- seperator next
time because that's really not part of the final commit messaage and the
maintainer has then to strip it off manually
 
> Signed-off-by: Anchal Agarwal <anchalag@amazon.com>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

These SOB lines are just wrongly ordered as they suggest:

     Anchal has authored the patch and Thomas transported it

which is clearly not the case. So the right order is:

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Anchal Agarwal <anchalag@amazon.com>

And that needs another tweak at the top of the change log. The first
line in the mail body wants to be:

From: Thomas Gleixner <tglx@linutronix.de>

followed by an empty new line before the actual changelog text
starts. That way the attribution of the patch when applying it will be
correct.

Documentation/process/ is there for a reason and following the few
simple rules to get that straight is not rocket science.

Thanks,

        tglx
Anchal Agarwal Aug. 24, 2020, 5:25 p.m. UTC | #2 
On Sat, Aug 22, 2020 at 02:36:37AM +0200, Thomas Gleixner wrote:
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
> 
> 
> 
> On Fri, Aug 21 2020 at 22:27, Thomas Gleixner wrote:
> > Add a new quirk flag IRQCHIP_SHUTDOWN_ON_SUSPEND and add support for
> > it the core interrupt suspend/resume paths.
> >
> > Changelog:
> > v1->v2: Corrected the author's name to tglx@
> 
> Can you please move that Changelog part below the --- seperator next
> time because that's really not part of the final commit messaage and the
> maintainer has then to strip it off manually
> 
Ack.
> > Signed-off-by: Anchal Agarwal <anchalag@amazon.com>
> > Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> 
> These SOB lines are just wrongly ordered as they suggest:
> 
>      Anchal has authored the patch and Thomas transported it
> 
> which is clearly not the case. So the right order is:
> 
I must admit I wasn't aware of that. Will fix.
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: Anchal Agarwal <anchalag@amazon.com>
> 
> And that needs another tweak at the top of the change log. The first
> line in the mail body wants to be:
> 
> From: Thomas Gleixner <tglx@linutronix.de>
Yes I accidentally missed that in this patch.
Others have that line on all the patches and even v2 for this patch
has. Will fix.
> 
> followed by an empty new line before the actual changelog text
> starts. That way the attribution of the patch when applying it will be
> correct.
> 
> Documentation/process/ is there for a reason and following the few
> simple rules to get that straight is not rocket science.
> 
> Thanks,
> 
>         tglx
> 
> 
Anchal
Christoph Hellwig Aug. 25, 2020, 1:20 p.m. UTC | #3 
On Sat, Aug 22, 2020 at 02:36:37AM +0200, Thomas Gleixner wrote:
> From: Thomas Gleixner <tglx@linutronix.de>
> 
> followed by an empty new line before the actual changelog text
> starts. That way the attribution of the patch when applying it will be
> correct.

The way he sent it attribution will be correct as he managed to get his
MTU to send out the mail claiming to be from you.  But yes, it needs
the second From line, _and_ the first from line needs to be fixed to
be from him.
Thomas Gleixner Aug. 25, 2020, 3:25 p.m. UTC | #4 
On Tue, Aug 25 2020 at 14:20, Christoph Hellwig wrote:
> On Sat, Aug 22, 2020 at 02:36:37AM +0200, Thomas Gleixner wrote:
>> From: Thomas Gleixner <tglx@linutronix.de>
>> 
>> followed by an empty new line before the actual changelog text
>> starts. That way the attribution of the patch when applying it will be
>> correct.
>
> The way he sent it attribution will be correct as he managed to get his
> MTU to send out the mail claiming to be from you.

Which is even worse as that spammed my inbox with mail delivery rejects
for SPF and whatever violations. And those came mostly from Amazon
servers which sent out that wrong stuff in the first place ....

> But yes, it needs the second From line, _and_ the first from line
> needs to be fixed to be from him.

Thanks,

        tglx
diff mbox series

Patch

diff --git a/drivers/xen/events/events_base.c b/drivers/xen/events/events_base.c
index 140c7bf33a98..958dea2a4916 100644
--- a/drivers/xen/events/events_base.c
+++ b/drivers/xen/events/events_base.c
@@ -1611,6 +1611,7 @@  static struct irq_chip xen_pirq_chip __read_mostly = {
 	.irq_set_affinity	= set_affinity_irq,
 
 	.irq_retrigger		= retrigger_dynirq,
+	.flags                  = IRQCHIP_SHUTDOWN_ON_SUSPEND,
 };
 
 static struct irq_chip xen_percpu_chip __read_mostly = {
diff --git a/include/linux/irq.h b/include/linux/irq.h
index 1b7f4dfee35b..9340eec4a5a6 100644
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -555,6 +555,7 @@  struct irq_chip {
  * IRQCHIP_EOI_THREADED:	Chip requires eoi() on unmask in threaded mode
  * IRQCHIP_SUPPORTS_LEVEL_MSI	Chip can provide two doorbells for Level MSIs
  * IRQCHIP_SUPPORTS_NMI:	Chip can deliver NMIs, only for root irqchips
+ * IRQCHIP_SHUTDOWN_ON_SUSPEND: Shutdown non wake irqs in the suspend path
  */
 enum {
 	IRQCHIP_SET_TYPE_MASKED		= (1 <<  0),
@@ -566,6 +567,7 @@  enum {
 	IRQCHIP_EOI_THREADED		= (1 <<  6),
 	IRQCHIP_SUPPORTS_LEVEL_MSI	= (1 <<  7),
 	IRQCHIP_SUPPORTS_NMI		= (1 <<  8),
+	IRQCHIP_SHUTDOWN_ON_SUSPEND     = (1 <<  9),
 };
 
 #include <linux/irqdesc.h>
diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
index 857f5f4c8098..136e3ebe996f 100644
--- a/kernel/irq/chip.c
+++ b/kernel/irq/chip.c
@@ -233,7 +233,7 @@  __irq_startup_managed(struct irq_desc *desc, struct cpumask *aff, bool force)
 }
 #endif
 
-static int __irq_startup(struct irq_desc *desc)
+int __irq_startup(struct irq_desc *desc)
 {
 	struct irq_data *d = irq_desc_get_irq_data(desc);
 	int ret = 0;
diff --git a/kernel/irq/internals.h b/kernel/irq/internals.h
index 7db284b10ac9..b6fca5eacff7 100644
--- a/kernel/irq/internals.h
+++ b/kernel/irq/internals.h
@@ -80,6 +80,7 @@  extern void __enable_irq(struct irq_desc *desc);
 extern int irq_activate(struct irq_desc *desc);
 extern int irq_activate_and_startup(struct irq_desc *desc, bool resend);
 extern int irq_startup(struct irq_desc *desc, bool resend, bool force);
+extern int __irq_startup(struct irq_desc *desc);
 
 extern void irq_shutdown(struct irq_desc *desc);
 extern void irq_shutdown_and_deactivate(struct irq_desc *desc);
diff --git a/kernel/irq/pm.c b/kernel/irq/pm.c
index c6c7e187ae74..3c4ffb2b6ef2 100644
--- a/kernel/irq/pm.c
+++ b/kernel/irq/pm.c
@@ -85,16 +85,25 @@  static bool suspend_device_irq(struct irq_desc *desc)
 	}
 
 	desc->istate |= IRQS_SUSPENDED;
-	__disable_irq(desc);
-
 	/*
-	 * Hardware which has no wakeup source configuration facility
-	 * requires that the non wakeup interrupts are masked at the
-	 * chip level. The chip implementation indicates that with
-	 * IRQCHIP_MASK_ON_SUSPEND.
+	 * Some irq chips (e.g. XEN PIRQ) require a full shutdown on suspend
+	 * as some of the legacy drivers(e.g. floppy) do nothing during the
+	 * suspend path
 	 */
-	if (irq_desc_get_chip(desc)->flags & IRQCHIP_MASK_ON_SUSPEND)
-		mask_irq(desc);
+	if (irq_desc_get_chip(desc)->flags & IRQCHIP_SHUTDOWN_ON_SUSPEND) {
+		irq_shutdown(desc);
+	} else {
+		__disable_irq(desc);
+
+	       /*
+		* Hardware which has no wakeup source configuration facility
+		* requires that the non wakeup interrupts are masked at the
+		* chip level. The chip implementation indicates that with
+		* IRQCHIP_MASK_ON_SUSPEND.
+		*/
+		if (irq_desc_get_chip(desc)->flags & IRQCHIP_MASK_ON_SUSPEND)
+			mask_irq(desc);
+	}
 	return true;
 }
 
@@ -152,7 +161,11 @@  static void resume_irq(struct irq_desc *desc)
 	irq_state_set_masked(desc);
 resume:
 	desc->istate &= ~IRQS_SUSPENDED;
-	__enable_irq(desc);
+
+	if (irq_desc_get_chip(desc)->flags & IRQCHIP_SHUTDOWN_ON_SUSPEND)
+		__irq_startup(desc);
+	else
+		__enable_irq(desc);
 }
 
 static void resume_irqs(bool want_early)