Skip to contentSkip to conversation

IRQ: don't suspend nested_thread irqs over system suspend.

PATCH In review NeilBrown 3 participants 3 messages May 17, 2015 – May 18, 2015 IRQ

lore
Share & tools
lore

NeilBrown

author
#0 lore raw

PATCH 1/1 IRQ: don't suspend nested_thread irqs over system suspend.

Nested IRQs can only fire when the parent irq fires. So when the parent is suspended, there is no need to suspend the child irq.

Suspending nested irqs can cause a problem is they are suspended or resumed in the wrong order.
If an interrupt fires while the parent is active but the child is suspended, then the interrupt will not be acknowledged properly and so an interrupt storm can result.
This is particularly likely if the parent is resumed before the child, and the interrupt was raised during suspend.

Ensuring correct ordering would be possible, but it is simpler to just never suspend nested interrupts.

Signed-off-by NeilBrown <[email protected]>

---
At Rafael's request I'm resending this. No response last time (except from Rafael), no change in the code.

This change will fix a bug so that the work-around introduced by
8b41669ceba0 ("mfd: twl4030: Fix chained irq handling on resume from suspend")
can be reverted. This work-around misuses the IRQF_EARLY_RESUME flag, and triggers a warning on resume:

[   56.095825] WARNING: CPU: 0 PID: 3 at ../kernel/irq/manage.c:661 irq_nested_primary_handler+0x18/0x28()

(though the line number might have changed).

NeilBrown

kernel/irq/pm.c +4 -0
index 5204a6d1b985..d22786a6dbde 100644
@@ -123,6 +123,8 @@ void suspend_device_irqs(void)
 		unsigned long flags;
 		bool sync;
 
+		if (irq_settings_is_nested_thread(desc))
+			continue;
 		raw_spin_lock_irqsave(&desc->lock, flags);
 		sync = suspend_device_irq(desc, irq);
 		raw_spin_unlock_irqrestore(&desc->lock, flags);

@@ -163,6 +165,8 @@ static void resume_irqs(bool want_early)
 
 		if (!is_early && want_early)
 			continue;
+		if (irq_settings_is_nested_thread(desc))
+			continue;
 
 		raw_spin_lock_irqsave(&desc->lock, flags);
 		resume_irq(desc, irq);
Acked-by on v1 1/1
#1 lore raw
Quoted 14 lines · NeilBrown

Nested IRQs can only fire when the parent irq fires. So when the parent is suspended, there is no need to suspend the child irq.

Suspending nested irqs can cause a problem is they are suspended or resumed in the wrong order.
If an interrupt fires while the parent is active but the child is suspended, then the interrupt will not be acknowledged properly and so an interrupt storm can result.
This is particularly likely if the parent is resumed before the child, and the interrupt was raised during suspend.

Quoted 2 lines

Ensuring correct ordering would be possible, but it is simpler to just never suspend nested interrupts.

Looks sane to me, but it's Thomas' call.

FWIW:

Acked-by Peter Zijlstra (Intel) <[email protected]>
Quoted 25 lines
Signed-off-by NeilBrown <[email protected]>

---

kernel/irq/pm.c +4 -0
index 5204a6d1b985..d22786a6dbde 100644
@@ -123,6 +123,8 @@ void suspend_device_irqs(void)
 		unsigned long flags;
 		bool sync;
 
+		if (irq_settings_is_nested_thread(desc))
+			continue;
 		raw_spin_lock_irqsave(&desc->lock, flags);
 		sync = suspend_device_irq(desc, irq);
 		raw_spin_unlock_irqrestore(&desc->lock, flags);

@@ -163,6 +165,8 @@ static void resume_irqs(bool want_early)
 
 		if (!is_early && want_early)
 			continue;
+		if (irq_settings_is_nested_thread(desc))
+			continue;
 
 		raw_spin_lock_irqsave(&desc->lock, flags);
 		resume_irq(desc, irq);
merge bot
#2 lore raw

[tip:irq/core] genirq: Don' t suspend nested_thread irqs over system suspend

In reply to NeilBrown
genirq: Don't suspend nested_thread irqs over system suspend
Commit-ID:  3c646f2c6aa9e918d7fc77867df7f430059f9ccc
Gitweb:     http://git.kernel.org/tip/3c646f2c6aa9e918d7fc77867df7f430059f9ccc
Author:     NeilBrown <[email protected]>
AuthorDate: Sun, 17 May 2015 15:19:34 +1000
Committer:  Thomas Gleixner <[email protected]>
CommitDate: Mon, 18 May 2015 17:23:47 +0200

genirq: Don't suspend nested_thread irqs over system suspend

Nested IRQs can only fire when the parent irq fires. So when the parent is suspended, there is no need to suspend the child irq.

Suspending nested irqs can cause a problem is they are suspended or resumed in the wrong order. If an interrupt fires while the parent is active but the child is suspended, then the interrupt will not be acknowledged properly and so an interrupt storm can result. This is particularly likely if the parent is resumed before the child, and the interrupt was raised during suspend.

Ensuring correct ordering would be possible, but it is simpler to just never suspend nested interrupts.

Signed-off-by NeilBrown <[email protected]>
Cc GTA04 owners <[email protected]>
Cc Kalle Jokiniemi <[email protected]>
Acked-by Peter Zijlstra <[email protected]>
Cc Rafael J. Wysocki <[email protected]>
Link http://lkml.kernel.org/r/[email protected] open here
Signed-off-by Thomas Gleixner <[email protected]>
---
 kernel/irq/pm.c | 4 ++++
 1 file changed, 4 insertions(+)
kernel/irq/pm.c +4 -0
index 5204a6d..d22786a 100644
@@ -123,6 +123,8 @@ void suspend_device_irqs(void)
 		unsigned long flags;
 		bool sync;
 
+		if (irq_settings_is_nested_thread(desc))
+			continue;
 		raw_spin_lock_irqsave(&desc->lock, flags);
 		sync = suspend_device_irq(desc, irq);
 		raw_spin_unlock_irqrestore(&desc->lock, flags);

@@ -163,6 +165,8 @@ static void resume_irqs(bool want_early)
 
 		if (!is_early && want_early)
 			continue;
+		if (irq_settings_is_nested_thread(desc))
+			continue;
 
 		raw_spin_lock_irqsave(&desc->lock, flags);
 		resume_irq(desc, irq);