irq: Rework IRQF_NO_SUSPENDED

Subject: irq: Rework IRQF_NO_SUSPENDED
From: Peter Zijlstra <[email protected]>
Date: Thu Jul 24 22:34:50 CEST 2014

Typically when devices are suspended they're quiesced such that they will not generate any further interrupts.

However some devices should still generate interrupts, even when suspended, typically used to wake the machine back up.

Such logic should ideally be contained inside each driver, if it can generate interrupts when suspended, it knows about this and the interrupt handler can deal with it.

Except of course for shared interrupts, when such a wakeup device is sharing an interrupt line with a device that does not expect interrupts while suspended things can go funny.

This is where IRQF_NO_SUSPEND comes in, the idea is that drivers that have the capability to wake the machine set IRQF_NO_SUSPEND and their handler will still be called, even when the IRQ subsystem is formally suspended. Handlers without IRQF_NO_SUSPEND will not be called.

This replaced the prior implementation of IRQF_NO_SUSPEND which had a number of fatal issues in that it didn't actually work for the shared case, exactly the case it should be helping.

There is still enable_irq_wake()/IRQD_WAKEUP_STATE that tries to serve a similar purpose but is equially wrecked for shared interrupts, ideally this would be removed.

Cc [email protected]

Signed-off-by Peter Zijlstra <[email protected]>

---
 kernel/irq/chip.c      |    4 +---
 kernel/irq/handle.c    |   24 +++++++++++++++++++++---
 kernel/irq/internals.h |    6 ++++--
 kernel/irq/manage.c    |   31 ++++++-------------------------
 kernel/irq/pm.c        |   17 +++++++++--------
 5 files changed, 41 insertions(+), 41 deletions(-)

kernel/irq/chip.c +1 -3

@@ -677,9 +677,7 @@ void handle_percpu_devid_irq(unsigned in
 	if (chip->irq_ack)
 		chip->irq_ack(&desc->irq_data);
 
-	trace_irq_handler_entry(irq, action);
-	res = action->handler(irq, dev_id);
-	trace_irq_handler_exit(irq, action, res);
+	res = do_irqaction(desc, action, irq, dev_id);
 
 	if (chip->irq_eoi)
 		chip->irq_eoi(&desc->irq_data);

kernel/irq/handle.c +21 -3

@@ -131,6 +131,23 @@ void __irq_wake_thread(struct irq_desc *
 }
 
 irqreturn_t
+do_irqaction(struct irq_desc *desc, struct irqaction *action,
+	     unsigned int irq, void *dev_id)
+{
+	irqreturn_t ret;
+
+	if (unlikely((desc->istate & IRQS_SUSPENDED) &&
+		     !(action->flags & IRQF_NO_SUSPEND)))
+		return IRQ_NONE;
+
+	trace_irq_handler_entry(irq, action);
+	ret = action->handler(irq, dev_id);
+	trace_irq_handler_exit(irq, action, ret);
+
+	return ret;
+}
+
+irqreturn_t
 handle_irq_event_percpu(struct irq_desc *desc, struct irqaction *action)
 {
 	irqreturn_t retval = IRQ_NONE;

@@ -139,9 +156,7 @@ handle_irq_event_percpu(struct irq_desc
 	do {
 		irqreturn_t res;
 
-		trace_irq_handler_entry(irq, action);
-		res = action->handler(irq, action->dev_id);
-		trace_irq_handler_exit(irq, action, res);
+		res = do_irqaction(desc, action, irq, action->dev_id);
 
 		if (WARN_ONCE(!irqs_disabled(),"irq %u handler %pF enabled interrupts\n",
 			      irq, action->handler))

@@ -175,6 +190,9 @@ handle_irq_event_percpu(struct irq_desc
 
 	add_interrupt_randomness(irq, flags);
 
+	if (unlikely(desc->istate & IRQS_SUSPENDED) && retval == IRQ_NONE)
+		retval = IRQ_HANDLED;
+
 	if (!noirqdebug)
 		note_interrupt(irq, desc, retval);
 	return retval;

kernel/irq/internals.h +4 -2

@@ -63,8 +63,10 @@ enum {
 
 extern int __irq_set_trigger(struct irq_desc *desc, unsigned int irq,
 		unsigned long flags);
-extern void __disable_irq(struct irq_desc *desc, unsigned int irq, bool susp);
-extern void __enable_irq(struct irq_desc *desc, unsigned int irq, bool resume);
+
+extern irqreturn_t
+do_irqaction(struct irq_desc *desc, struct irqaction *action,
+	     unsigned int irq, void *dev_id);
 
 extern int irq_startup(struct irq_desc *desc, bool resend);
 extern void irq_shutdown(struct irq_desc *desc);

kernel/irq/manage.c +6 -25

@@ -382,15 +382,8 @@ setup_affinity(unsigned int irq, struct
 }
 #endif
 
-void __disable_irq(struct irq_desc *desc, unsigned int irq, bool suspend)
+void __disable_irq(struct irq_desc *desc, unsigned int irq)
 {
-	if (suspend) {
-		if (!desc->action || (desc->action->flags & IRQF_NO_SUSPEND) ||
-		    irqd_has_set(&desc->irq_data, IRQD_WAKEUP_STATE))
-			return;
-		desc->istate |= IRQS_SUSPENDED;
-	}
-
 	if (!desc->depth++)
 		irq_disable(desc);
 }

@@ -402,7 +395,7 @@ static int __disable_irq_nosync(unsigned
 
 	if (!desc)
 		return -EINVAL;
-	__disable_irq(desc, irq, false);
+	__disable_irq(desc, irq);
 	irq_put_desc_busunlock(desc, flags);
 	return 0;
 }

@@ -443,20 +436,8 @@ void disable_irq(unsigned int irq)
 }
 EXPORT_SYMBOL(disable_irq);
 
-void __enable_irq(struct irq_desc *desc, unsigned int irq, bool resume)
+void __enable_irq(struct irq_desc *desc, unsigned int irq)
 {
-	if (resume) {
-		if (!(desc->istate & IRQS_SUSPENDED)) {
-			if (!desc->action)
-				return;
-			if (!(desc->action->flags & IRQF_FORCE_RESUME))
-				return;
-			/* Pretend that it got disabled ! */
-			desc->depth++;
-		}
-		desc->istate &= ~IRQS_SUSPENDED;
-	}
-
 	switch (desc->depth) {
 	case 0:
  err_out:

@@ -498,7 +479,7 @@ void enable_irq(unsigned int irq)
 		 KERN_ERR "enable_irq before setup/request_irq: irq %u\n", irq))
 		goto out;
 
-	__enable_irq(desc, irq, false);
+	__enable_irq(desc, irq);
 out:
 	irq_put_desc_busunlock(desc, flags);
 }

@@ -1079,7 +1060,7 @@ __setup_irq(unsigned int irq, struct irq
 		 */
 
 #define IRQF_MISMATCH \
-	(IRQF_TRIGGER_MASK | IRQF_ONESHOT | IRQF_NO_SUSPEND)
+	(IRQF_TRIGGER_MASK | IRQF_ONESHOT)
 
 		if (!((old->flags & new->flags) & IRQF_SHARED) ||
 		    ((old->flags ^ new->flags) & IRQF_MISMATCH))

@@ -1232,7 +1213,7 @@ __setup_irq(unsigned int irq, struct irq
 	 */
 	if (shared && (desc->istate & IRQS_SPURIOUS_DISABLED)) {
 		desc->istate &= ~IRQS_SPURIOUS_DISABLED;
-		__enable_irq(desc, irq, false);
+		__enable_irq(desc, irq);
 	}
 
 	raw_spin_unlock_irqrestore(&desc->lock, flags);

kernel/irq/pm.c +9 -9

@@ -29,14 +29,20 @@ void suspend_device_irqs(void)
 	for_each_irq_desc(irq, desc) {
 		unsigned long flags;
 
+		/*
+		 * Ideally this would be a global state, but we cannot
+		 * for the trainwreck that is IRQD_WAKEUP_STATE.
+		 */
 		raw_spin_lock_irqsave(&desc->lock, flags);
-		__disable_irq(desc, irq, true);
+		if (!irqd_has_set(&desc->irq_data, IRQD_WAKEUP_STATE))
+			desc->istate |= IRQS_SUSPENDED;
 		raw_spin_unlock_irqrestore(&desc->lock, flags);
 	}
 
-	for_each_irq_desc(irq, desc)
+	for_each_irq_desc(irq, desc) {
 		if (desc->istate & IRQS_SUSPENDED)
 			synchronize_irq(irq);
+	}
 }
 EXPORT_SYMBOL_GPL(suspend_device_irqs);
 

@@ -47,14 +53,9 @@ static void resume_irqs(bool want_early)
 
 	for_each_irq_desc(irq, desc) {
 		unsigned long flags;
-		bool is_early = desc->action &&
-			desc->action->flags & IRQF_EARLY_RESUME;
-
-		if (!is_early && want_early)
-			continue;
 
 		raw_spin_lock_irqsave(&desc->lock, flags);
-		__enable_irq(desc, irq, true);
+		desc->istate &= ~IRQS_SUSPENDED;
 		raw_spin_unlock_irqrestore(&desc->lock, flags);
 	}
 }
--

To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Subject: irq: Rework IRQF_NO_SUSPENDED
From: Peter Zijlstra <[email protected]>
Date: Thu Jul 24 22:34:50 CEST 2014

Typically when devices are suspended they're quiesced such that they will not generate any further interrupts.

However some devices should still generate interrupts, even when suspended, typically used to wake the machine back up.

Such logic should ideally be contained inside each driver, if it can generate interrupts when suspended, it knows about this and the interrupt handler can deal with it.

Except of course for shared interrupts, when such a wakeup device is sharing an interrupt line with a device that does not expect interrupts while suspended things can go funny.

This is where IRQF_NO_SUSPEND comes in, the idea is that drivers that have the capability to wake the machine set IRQF_NO_SUSPEND and their handler will still be called, even when the IRQ subsystem is formally suspended. Handlers without IRQF_NO_SUSPEND will not be called.

This replaced the prior implementation of IRQF_NO_SUSPEND which had a number of fatal issues in that it didn't actually work for the shared case, exactly the case it should be helping.

There is still enable_irq_wake()/IRQD_WAKEUP_STATE that tries to serve a similar purpose but is equially wrecked for shared interrupts, ideally this would be removed.

Cc [email protected]

Signed-off-by Peter Zijlstra <[email protected]>

---
 kernel/irq/chip.c      |    4 +---
 kernel/irq/handle.c    |   24 +++++++++++++++++++++---
 kernel/irq/internals.h |    6 ++++--
 kernel/irq/manage.c    |   31 ++++++-------------------------
 kernel/irq/pm.c        |   17 +++++++++--------
 5 files changed, 41 insertions(+), 41 deletions(-)

kernel/irq/chip.c +1 -3

@@ -677,9 +677,7 @@ void handle_percpu_devid_irq(unsigned in
 	if (chip->irq_ack)
 		chip->irq_ack(&desc->irq_data);
 
-	trace_irq_handler_entry(irq, action);
-	res = action->handler(irq, dev_id);
-	trace_irq_handler_exit(irq, action, res);
+	res = do_irqaction(desc, action, irq, dev_id);
 
 	if (chip->irq_eoi)
 		chip->irq_eoi(&desc->irq_data);

kernel/irq/handle.c +21 -3

@@ -131,6 +131,23 @@ void __irq_wake_thread(struct irq_desc *
 }
 
 irqreturn_t
+do_irqaction(struct irq_desc *desc, struct irqaction *action,
+	     unsigned int irq, void *dev_id)
+{
+	irqreturn_t ret;
+
+	if (unlikely((desc->istate & IRQS_SUSPENDED) &&
+		     !(action->flags & IRQF_NO_SUSPEND)))
+		return IRQ_NONE;
+
+	trace_irq_handler_entry(irq, action);
+	ret = action->handler(irq, dev_id);
+	trace_irq_handler_exit(irq, action, ret);
+
+	return ret;
+}
+
+irqreturn_t
 handle_irq_event_percpu(struct irq_desc *desc, struct irqaction *action)
 {
 	irqreturn_t retval = IRQ_NONE;

@@ -139,9 +156,7 @@ handle_irq_event_percpu(struct irq_desc
 	do {
 		irqreturn_t res;
 
-		trace_irq_handler_entry(irq, action);
-		res = action->handler(irq, action->dev_id);
-		trace_irq_handler_exit(irq, action, res);
+		res = do_irqaction(desc, action, irq, action->dev_id);
 
 		if (WARN_ONCE(!irqs_disabled(),"irq %u handler %pF enabled interrupts\n",
 			      irq, action->handler))

@@ -175,6 +190,9 @@ handle_irq_event_percpu(struct irq_desc
 
 	add_interrupt_randomness(irq, flags);
 
+	if (unlikely(desc->istate & IRQS_SUSPENDED) && retval == IRQ_NONE)
+		retval = IRQ_HANDLED;
+
 	if (!noirqdebug)
 		note_interrupt(irq, desc, retval);
 	return retval;

kernel/irq/internals.h +4 -2

@@ -63,8 +63,10 @@ enum {
 
 extern int __irq_set_trigger(struct irq_desc *desc, unsigned int irq,
 		unsigned long flags);
-extern void __disable_irq(struct irq_desc *desc, unsigned int irq, bool susp);
-extern void __enable_irq(struct irq_desc *desc, unsigned int irq, bool resume);
+
+extern irqreturn_t
+do_irqaction(struct irq_desc *desc, struct irqaction *action,
+	     unsigned int irq, void *dev_id);
 
 extern int irq_startup(struct irq_desc *desc, bool resend);
 extern void irq_shutdown(struct irq_desc *desc);

kernel/irq/manage.c +6 -25

@@ -382,15 +382,8 @@ setup_affinity(unsigned int irq, struct
 }
 #endif
 
-void __disable_irq(struct irq_desc *desc, unsigned int irq, bool suspend)
+void __disable_irq(struct irq_desc *desc, unsigned int irq)
 {
-	if (suspend) {
-		if (!desc->action || (desc->action->flags & IRQF_NO_SUSPEND) ||
-		    irqd_has_set(&desc->irq_data, IRQD_WAKEUP_STATE))
-			return;
-		desc->istate |= IRQS_SUSPENDED;
-	}
-
 	if (!desc->depth++)
 		irq_disable(desc);
 }

@@ -402,7 +395,7 @@ static int __disable_irq_nosync(unsigned
 
 	if (!desc)
 		return -EINVAL;
-	__disable_irq(desc, irq, false);
+	__disable_irq(desc, irq);
 	irq_put_desc_busunlock(desc, flags);
 	return 0;
 }

@@ -443,20 +436,8 @@ void disable_irq(unsigned int irq)
 }
 EXPORT_SYMBOL(disable_irq);
 
-void __enable_irq(struct irq_desc *desc, unsigned int irq, bool resume)
+void __enable_irq(struct irq_desc *desc, unsigned int irq)
 {
-	if (resume) {
-		if (!(desc->istate & IRQS_SUSPENDED)) {
-			if (!desc->action)
-				return;
-			if (!(desc->action->flags & IRQF_FORCE_RESUME))
-				return;
-			/* Pretend that it got disabled ! */
-			desc->depth++;
-		}
-		desc->istate &= ~IRQS_SUSPENDED;
-	}
-
 	switch (desc->depth) {
 	case 0:
  err_out:

@@ -498,7 +479,7 @@ void enable_irq(unsigned int irq)
 		 KERN_ERR "enable_irq before setup/request_irq: irq %u\n", irq))
 		goto out;
 
-	__enable_irq(desc, irq, false);
+	__enable_irq(desc, irq);
 out:
 	irq_put_desc_busunlock(desc, flags);
 }

@@ -1079,7 +1060,7 @@ __setup_irq(unsigned int irq, struct irq
 		 */
 
 #define IRQF_MISMATCH \
-	(IRQF_TRIGGER_MASK | IRQF_ONESHOT | IRQF_NO_SUSPEND)
+	(IRQF_TRIGGER_MASK | IRQF_ONESHOT)
 
 		if (!((old->flags & new->flags) & IRQF_SHARED) ||
 		    ((old->flags ^ new->flags) & IRQF_MISMATCH))

@@ -1232,7 +1213,7 @@ __setup_irq(unsigned int irq, struct irq
 	 */
 	if (shared && (desc->istate & IRQS_SPURIOUS_DISABLED)) {
 		desc->istate &= ~IRQS_SPURIOUS_DISABLED;
-		__enable_irq(desc, irq, false);
+		__enable_irq(desc, irq);
 	}
 
 	raw_spin_unlock_irqrestore(&desc->lock, flags);

kernel/irq/pm.c +9 -9

@@ -29,14 +29,20 @@ void suspend_device_irqs(void)
 	for_each_irq_desc(irq, desc) {
 		unsigned long flags;
 
+		/*
+		 * Ideally this would be a global state, but we cannot
+		 * for the trainwreck that is IRQD_WAKEUP_STATE.
+		 */
 		raw_spin_lock_irqsave(&desc->lock, flags);
-		__disable_irq(desc, irq, true);
+		if (!irqd_has_set(&desc->irq_data, IRQD_WAKEUP_STATE))
+			desc->istate |= IRQS_SUSPENDED;
 		raw_spin_unlock_irqrestore(&desc->lock, flags);
 	}
 
-	for_each_irq_desc(irq, desc)
+	for_each_irq_desc(irq, desc) {
 		if (desc->istate & IRQS_SUSPENDED)
 			synchronize_irq(irq);
+	}
 }
 EXPORT_SYMBOL_GPL(suspend_device_irqs);
 

@@ -47,14 +53,9 @@ static void resume_irqs(bool want_early)
 
 	for_each_irq_desc(irq, desc) {
 		unsigned long flags;
-		bool is_early = desc->action &&
-			desc->action->flags & IRQF_EARLY_RESUME;
-
-		if (!is_early && want_early)
-			continue;
 
 		raw_spin_lock_irqsave(&desc->lock, flags);
-		__enable_irq(desc, irq, true);
+		desc->istate &= ~IRQS_SUSPENDED;
 		raw_spin_unlock_irqrestore(&desc->lock, flags);
 	}
 }
--

To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Quoted 3 lines

There is still enable_irq_wake()/IRQD_WAKEUP_STATE that tries to serve a similar purpose but is equially wrecked for shared interrupts, ideally this would be removed.

Let me comment about this particular thing.

I had a discussion with Dmitry about that and his argument was that enable_irq_wake() should imply IRQF_NO_SUSPEND, because drivers that set up interrupts for system wakeup should expect those interrupts to trigger at any time, including system suspend. Hence the patch that added the IRQD_WAKEUP_STATE check to __disable_irq().

However, in the face of the problem that is being addressed here I'm not really sure that this argument is valid, because if the driver calling enable_irq_wake() is sharing the IRQ with another one, the other driver may not actually know that the IRQ will be a wakeup one and still may not expect interrupts to come to it during system suspend/resume.

Yes, drivers using enable_irq_wake() will likely want IRQF_NO_SUSPEND to be set for their irqactions, but that should not imply "no suspend" for all irqactions sharing the same desc. So I guess it may be better to go forth and use a global "interrupts suspended" state variable instead of the IRQS_SUSPENDED flag for each desc and throw away the IRQD_WAKEUP_STATE check from suspend_device_irqs() entirely.

Peter, it looks like you'd prefer that?

Quoted 7 lines · Rafael J. Wysocki

Yes, drivers using enable_irq_wake() will likely want IRQF_NO_SUSPEND to be set for their irqactions, but that should not imply "no suspend" for all irqactions sharing the same desc. So I guess it may be better to go forth and use a global "interrupts suspended" state variable instead of the IRQS_SUSPENDED flag for each desc and throw away the IRQD_WAKEUP_STATE check from suspend_device_irqs() entirely.

Quoted 1 line

Peter, it looks like you'd prefer that?

My preference would be to shoot enable_irq_wake() in the head, its fundamentally broken.

I'm curious what you mean. Are you referring to the fact that its input is simply an IRQ number (regardless of whether the IRQ is shared), not something that identifies the particular handler (e.g., struct irqaction)?

Quoted 4 lines · Brian Norris

I'm curious what you mean. Are you referring to the fact that its input is simply an IRQ number (regardless of whether the IRQ is shared), not something that identifies the particular handler (e.g., struct irqaction)?

Yes. I know that shared stuff is a massive head-ache, but I feel we should not introduce primitives that do not work with it.

Quoted 23 lines · Peter Zijlstra

Subject: irq: Rework IRQF_NO_SUSPENDED
From: Peter Zijlstra <[email protected]>
Date: Thu Jul 24 22:34:50 CEST 2014

Typically when devices are suspended they're quiesced such that they will not generate any further interrupts.

However some devices should still generate interrupts, even when suspended, typically used to wake the machine back up.

Such logic should ideally be contained inside each driver, if it can generate interrupts when suspended, it knows about this and the interrupt handler can deal with it.

Except of course for shared interrupts, when such a wakeup device is sharing an interrupt line with a device that does not expect interrupts while suspended things can go funny.

This is where IRQF_NO_SUSPEND comes in, the idea is that drivers that have the capability to wake the machine set IRQF_NO_SUSPEND and their handler will still be called, even when the IRQ subsystem is formally suspended. Handlers without IRQF_NO_SUSPEND will not be called.

Quoted 7 lines

This replaced the prior implementation of IRQF_NO_SUSPEND which had a number of fatal issues in that it didn't actually work for the shared case, exactly the case it should be helping.

There is still enable_irq_wake()/IRQD_WAKEUP_STATE that tries to serve a similar purpose but is equially wrecked for shared interrupts, ideally this would be removed.

Let me comment about this particular thing.

I had a discussion with Dmitry about that and his argument was that enable_irq_wake() should imply IRQF_NO_SUSPEND, because drivers that set up interrupts for system wakeup should expect those interrupts to trigger at any time, including system suspend. Hence the patch that added the IRQD_WAKEUP_STATE check to __disable_irq().

However, in the face of the problem that is being addressed here I'm not really sure that this argument is valid, because if the driver calling enable_irq_wake() is sharing the IRQ with another one, the other driver may not actually know that the IRQ will be a wakeup one and still may not expect interrupts to come to it during system suspend/resume.

Yes, drivers using enable_irq_wake() will likely want IRQF_NO_SUSPEND to be set for their irqactions, but that should not imply "no suspend" for all irqactions sharing the same desc. So I guess it may be better to go forth and use a global "interrupts suspended" state variable instead of the IRQS_SUSPENDED flag for each desc and throw away the IRQD_WAKEUP_STATE check from suspend_device_irqs() entirely.

Quoted 47 lines · Rafael J. Wysocki

Quoted 23 lines · Peter Zijlstra

Subject: irq: Rework IRQF_NO_SUSPENDED
From: Peter Zijlstra <[email protected]>
Date: Thu Jul 24 22:34:50 CEST 2014

Typically when devices are suspended they're quiesced such that they will not generate any further interrupts.

However some devices should still generate interrupts, even when suspended, typically used to wake the machine back up.

Such logic should ideally be contained inside each driver, if it can generate interrupts when suspended, it knows about this and the interrupt handler can deal with it.

Except of course for shared interrupts, when such a wakeup device is sharing an interrupt line with a device that does not expect interrupts while suspended things can go funny.

This is where IRQF_NO_SUSPEND comes in, the idea is that drivers that have the capability to wake the machine set IRQF_NO_SUSPEND and their handler will still be called, even when the IRQ subsystem is formally suspended. Handlers without IRQF_NO_SUSPEND will not be called.

Quoted 7 lines

This replaced the prior implementation of IRQF_NO_SUSPEND which had a number of fatal issues in that it didn't actually work for the shared case, exactly the case it should be helping.

There is still enable_irq_wake()/IRQD_WAKEUP_STATE that tries to serve a similar purpose but is equially wrecked for shared interrupts, ideally this would be removed.

Let me comment about this particular thing.

I had a discussion with Dmitry about that and his argument was that enable_irq_wake() should imply IRQF_NO_SUSPEND, because drivers that set up interrupts for system wakeup should expect those interrupts to trigger at any time, including system suspend. Hence the patch that added the IRQD_WAKEUP_STATE check to __disable_irq().

However, in the face of the problem that is being addressed here I'm not really sure that this argument is valid, because if the driver calling enable_irq_wake() is sharing the IRQ with another one, the other driver may not actually know that the IRQ will be a wakeup one and still may not expect interrupts to come to it during system suspend/resume.

Quoted 6 lines

Yes, drivers using enable_irq_wake() will likely want IRQF_NO_SUSPEND to be set for their irqactions, but that should not imply "no suspend" for all irqactions sharing the same desc. So I guess it may be better to go forth and use a global "interrupts suspended" state variable instead of the IRQS_SUSPENDED flag for each desc and throw away the IRQD_WAKEUP_STATE check from suspend_device_irqs() entirely.

How should that global state work?

If the face of your remark about the level-triggered interrupts that's all moot.

kernel/irq/pm.c +5 -1

@@ -29,14 +29,20 @@ void suspend_device_irqs(void)
 	for_each_irq_desc(irq, desc) {
 		unsigned long flags;
 
+		/*
+		 * Ideally this would be a global state, but we cannot
+		 * for the trainwreck that is IRQD_WAKEUP_STATE.
+		 */
 		raw_spin_lock_irqsave(&desc->lock, flags);
-		__disable_irq(desc, irq, true);
+		if (!irqd_has_set(&desc->irq_data, IRQD_WAKEUP_STATE))

Quoted 11 lines · Peter Zijlstra

(quoted diff) +5 -1

@@ -29,14 +29,20 @@ void suspend_device_irqs(void)
 	for_each_irq_desc(irq, desc) {
 		unsigned long flags;
 
+		/*
+		 * Ideally this would be a global state, but we cannot
+		 * for the trainwreck that is IRQD_WAKEUP_STATE.
+		 */
 		raw_spin_lock_irqsave(&desc->lock, flags);
-		__disable_irq(desc, irq, true);
+		if (!irqd_has_set(&desc->irq_data, IRQD_WAKEUP_STATE))

Review void suspend_device_irqs(void) · line 46

Quoted 10 lines

(quoted diff) +3 -1

+			desc->istate |= IRQS_SUSPENDED;
 		raw_spin_unlock_irqrestore(&desc->lock, flags);
 	}
 
-	for_each_irq_desc(irq, desc)
+	for_each_irq_desc(irq, desc) {
 		if (desc->istate & IRQS_SUSPENDED)
 			synchronize_irq(irq);
+	}
 }

So, instead of disabling the interrupt you just mark it suspended. Good luck with level triggered interrupt lines then.

Assume the interrupt fires after you marked it suspended. Then the flow handler will call handle_irq_event() which will do nothing and return handled. So the flow handler will reenable the interrupt line, which will cause the interrupt to fire immediately again after the RETI. Guess how much progress the system is going to make when that happens.

So it looks like we really need the "suspend" thing to either disable the interrupt entirely (in which case all handlers for all actions will not be invoked after it's been suspended) or leave it enabled (causing all handlers to be invoked all the time).

I'm not sure if we can do much beyond what's already in your tree about that, then.

Well, perhaps the patch failing the irq request in case of the IRQF_NO_SUSPEND mismatch is a bit too drastic. Instead, we could just print a warning in __setup_irq() in that case and then check IRQF_NO_SUSPEND for all actions in __disable_irq().

What about this?

Quoted 11 lines · Peter Zijlstra

kernel/irq/pm.c +5 -1

@@ -29,14 +29,20 @@ void suspend_device_irqs(void)
 	for_each_irq_desc(irq, desc) {
 		unsigned long flags;
 
+		/*
+		 * Ideally this would be a global state, but we cannot
+		 * for the trainwreck that is IRQD_WAKEUP_STATE.
+		 */
 		raw_spin_lock_irqsave(&desc->lock, flags);
-		__disable_irq(desc, irq, true);
+		if (!irqd_has_set(&desc->irq_data, IRQD_WAKEUP_STATE))

kernel/irq/pm.c void suspend_device_irqs(void) · line 46

Quoted 10 lines

kernel/irq/pm.c +3 -1

+			desc->istate |= IRQS_SUSPENDED;
 		raw_spin_unlock_irqrestore(&desc->lock, flags);
 	}
 
-	for_each_irq_desc(irq, desc)
+	for_each_irq_desc(irq, desc) {
 		if (desc->istate & IRQS_SUSPENDED)
 			synchronize_irq(irq);
+	}
 }

So, instead of disabling the interrupt you just mark it suspended. Good luck with level triggered interrupt lines then.

Assume the interrupt fires after you marked it suspended. Then the flow handler will call handle_irq_event() which will do nothing and return handled. So the flow handler will reenable the interrupt line, which will cause the interrupt to fire immediately again after the RETI. Guess how much progress the system is going to make when that happens.

Quoted 26 lines · Thomas Gleixner

Quoted 11 lines · Peter Zijlstra

kernel/irq/pm.c +5 -1

@@ -29,14 +29,20 @@ void suspend_device_irqs(void)
 	for_each_irq_desc(irq, desc) {
 		unsigned long flags;
 
+		/*
+		 * Ideally this would be a global state, but we cannot
+		 * for the trainwreck that is IRQD_WAKEUP_STATE.
+		 */
 		raw_spin_lock_irqsave(&desc->lock, flags);
-		__disable_irq(desc, irq, true);
+		if (!irqd_has_set(&desc->irq_data, IRQD_WAKEUP_STATE))

kernel/irq/pm.c void suspend_device_irqs(void) · line 46

Quoted 10 lines

kernel/irq/pm.c +3 -1

+			desc->istate |= IRQS_SUSPENDED;
 		raw_spin_unlock_irqrestore(&desc->lock, flags);
 	}
 
-	for_each_irq_desc(irq, desc)
+	for_each_irq_desc(irq, desc) {
 		if (desc->istate & IRQS_SUSPENDED)
 			synchronize_irq(irq);
+	}
 }

So, instead of disabling the interrupt you just mark it suspended. Good luck with level triggered interrupt lines then.

Quoted 6 lines

Assume the interrupt fires after you marked it suspended. Then the flow handler will call handle_irq_event() which will do nothing and return handled. So the flow handler will reenable the interrupt line, which will cause the interrupt to fire immediately again after the RETI. Guess how much progress the system is going to make when that happens.

Urgh, right. I knew it was too easy. Can we have do_irqhandler() ACK the interrupt and not call the handler?

Quoted 34 lines · Peter Zijlstra

Quoted 32 lines · Thomas Gleixner

Quoted 11 lines · Peter Zijlstra

kernel/irq/pm.c +5 -1

@@ -29,14 +29,20 @@ void suspend_device_irqs(void)
 	for_each_irq_desc(irq, desc) {
 		unsigned long flags;
 
+		/*
+		 * Ideally this would be a global state, but we cannot
+		 * for the trainwreck that is IRQD_WAKEUP_STATE.
+		 */
 		raw_spin_lock_irqsave(&desc->lock, flags);
-		__disable_irq(desc, irq, true);
+		if (!irqd_has_set(&desc->irq_data, IRQD_WAKEUP_STATE))

kernel/irq/pm.c void suspend_device_irqs(void) · line 46

Quoted 10 lines

kernel/irq/pm.c +3 -1

+			desc->istate |= IRQS_SUSPENDED;
 		raw_spin_unlock_irqrestore(&desc->lock, flags);
 	}
 
-	for_each_irq_desc(irq, desc)
+	for_each_irq_desc(irq, desc) {
 		if (desc->istate & IRQS_SUSPENDED)
 			synchronize_irq(irq);
+	}
 }

So, instead of disabling the interrupt you just mark it suspended. Good luck with level triggered interrupt lines then.

Assume the interrupt fires after you marked it suspended. Then the flow handler will call handle_irq_event() which will do nothing and return handled. So the flow handler will reenable the interrupt line, which will cause the interrupt to fire immediately again after the RETI. Guess how much progress the system is going to make when that happens.

Quoted 2 lines

Urgh, right. I knew it was too easy. Can we have do_irqhandler() ACK the interrupt and not call the handler?

OK, so Rafael said there's devices that keep on raising their interrupt until they get attention. Ideally this won't happen because the device is suspended etc.. But I'm sure there's some broken piece of hardware out there that'll make it go boom.

So here's an idea.

What about returning IRQ_NONE rather than IRQ_HANDLED for "suspended" interrupts (after all, that's what a sane driver would do for a suspended device I suppose)?

If the line is really shared and the interrupt is taken care of by the other guy sharing the line, we'll be all fine.

If that is not the case, on the other hand, and something's really broken, we'll end up disabling the interrupt and marking it as IRQS_SPURIOUS_DISABLED (if I understand things correctly).

But then, we can re-enable it and clear the IRQS_SPURIOUS_DISABLED flag at the resume_device_irqs() time so the driver can use it again. And we'll have a trace of the breakage in dmesg, so possibly we can go forth and fix the bad guy.

Would that make sense?

Quoted 4 lines · Peter Zijlstra

OK, so Rafael said there's devices that keep on raising their interrupt until they get attention. Ideally this won't happen because the device is suspended etc.. But I'm sure there's some broken piece of hardware out there that'll make it go boom.

So here's an idea.

What about returning IRQ_NONE rather than IRQ_HANDLED for "suspended" interrupts (after all, that's what a sane driver would do for a suspended device I suppose)?

If the line is really shared and the interrupt is taken care of by the other guy sharing the line, we'll be all fine.

If that is not the case, on the other hand, and something's really broken, we'll end up disabling the interrupt and marking it as IRQS_SPURIOUS_DISABLED (if I understand things correctly).

Quoted 15 lines · Rafael J. Wysocki

Quoted 4 lines · Peter Zijlstra

OK, so Rafael said there's devices that keep on raising their interrupt until they get attention. Ideally this won't happen because the device is suspended etc.. But I'm sure there's some broken piece of hardware out there that'll make it go boom.

So here's an idea.

What about returning IRQ_NONE rather than IRQ_HANDLED for "suspended" interrupts (after all, that's what a sane driver would do for a suspended device I suppose)?

If the line is really shared and the interrupt is taken care of by the other guy sharing the line, we'll be all fine.

Quoted 3 lines

If that is not the case, on the other hand, and something's really broken, we'll end up disabling the interrupt and marking it as IRQS_SPURIOUS_DISABLED (if I understand things correctly).

We should not wait 100k unhandled interrupts in that case. We know already at the first unhandled interrupt that the shit hit the fan.

I'll have a deeper look how we can sanitize the whole wake/no_suspend logic vs. shared interrupts.

Need to look at the usage sites first.

Quoted 20 lines · Thomas Gleixner

Quoted 18 lines · Rafael J. Wysocki

Quoted 4 lines · Peter Zijlstra

OK, so Rafael said there's devices that keep on raising their interrupt until they get attention. Ideally this won't happen because the device is suspended etc.. But I'm sure there's some broken piece of hardware out there that'll make it go boom.

So here's an idea.

What about returning IRQ_NONE rather than IRQ_HANDLED for "suspended" interrupts (after all, that's what a sane driver would do for a suspended device I suppose)?

If the line is really shared and the interrupt is taken care of by the other guy sharing the line, we'll be all fine.

If that is not the case, on the other hand, and something's really broken, we'll end up disabling the interrupt and marking it as IRQS_SPURIOUS_DISABLED (if I understand things correctly).

Quoted 2 lines

We should not wait 100k unhandled interrupts in that case. We know already at the first unhandled interrupt that the shit hit the fan.

The first one may be a bus glitch or some such. Also I guess we still need to allow the legitimate "no suspend" guy to handle his interrupts until it gets too worse.

Also does it really hurt to rely on the generic mechanism here? We regard it as fine at all other times after all.

Quoted 2 lines

I'll have a deeper look how we can sanitize the whole wake/no_suspend logic vs. shared interrupts.

Cool, thanks!

Quoted 1 line

Need to look at the usage sites first.

There will be more of them, like this:

https://patchwork.kernel.org/patch/4618531/

Essentially, all wakeup interrupts will need at least one no_suspend irqaction going forward.

Below is my take on this (untested) in case it is useful for anything.

It is targeted at the problematic case (that is, a shared interrupt with at least one irqaction that has IRQF_NO_SUSPEND set and at least one that doesn't) only and is not supposed to change behavior in the other cases (the do_irqaction thing shamelessly stolen from the Peter's patch). It drops the IRQD_WAKEUP_STATE check, because that has the same problem with shared interrupts as no_suspend.

---
 kernel/irq/handle.c |   21 ++++++++++++++++++---
 kernel/irq/manage.c |   30 +++++++++++++++++++++++++-----
 2 files changed, 43 insertions(+), 8 deletions(-)

Index: linux-pm/kernel/irq/manage.c
===================================================================

kernel/irq/manage.c +0 -0

Quoted 16 lines · Thomas Gleixner

Quoted 15 lines · Rafael J. Wysocki

Quoted 4 lines · Peter Zijlstra

OK, so Rafael said there's devices that keep on raising their interrupt until they get attention. Ideally this won't happen because the device is suspended etc.. But I'm sure there's some broken piece of hardware out there that'll make it go boom.

So here's an idea.

What about returning IRQ_NONE rather than IRQ_HANDLED for "suspended" interrupts (after all, that's what a sane driver would do for a suspended device I suppose)?

If the line is really shared and the interrupt is taken care of by the other guy sharing the line, we'll be all fine.

Quoted 6 lines

Quoted 3 lines

If that is not the case, on the other hand, and something's really broken, we'll end up disabling the interrupt and marking it as IRQS_SPURIOUS_DISABLED (if I understand things correctly).

We should not wait 100k unhandled interrupts in that case. We know already at the first unhandled interrupt that the shit hit the fan.

The first one may be a bus glitch or some such. Also I guess we still need to allow the legitimate "no suspend" guy to handle his interrupts until it gets too worse.

Also does it really hurt to rely on the generic mechanism here? We regard it as fine at all other times after all.

Quoted 2 lines

I'll have a deeper look how we can sanitize the whole wake/no_suspend logic vs. shared interrupts.

Quoted 24 lines · Rafael J. Wysocki

Quoted 16 lines · Thomas Gleixner

Quoted 15 lines · Rafael J. Wysocki

Quoted 4 lines · Peter Zijlstra

OK, so Rafael said there's devices that keep on raising their interrupt until they get attention. Ideally this won't happen because the device is suspended etc.. But I'm sure there's some broken piece of hardware out there that'll make it go boom.

So here's an idea.

What about returning IRQ_NONE rather than IRQ_HANDLED for "suspended" interrupts (after all, that's what a sane driver would do for a suspended device I suppose)?

If the line is really shared and the interrupt is taken care of by the other guy sharing the line, we'll be all fine.

Quoted 6 lines

Quoted 3 lines

If that is not the case, on the other hand, and something's really broken, we'll end up disabling the interrupt and marking it as IRQS_SPURIOUS_DISABLED (if I understand things correctly).

We should not wait 100k unhandled interrupts in that case. We know already at the first unhandled interrupt that the shit hit the fan.

Quoted 9 lines

The first one may be a bus glitch or some such. Also I guess we still need to allow the legitimate "no suspend" guy to handle his interrupts until it gets too worse.

Also does it really hurt to rely on the generic mechanism here? We regard it as fine at all other times after all.

Quoted 2 lines

I'll have a deeper look how we can sanitize the whole wake/no_suspend logic vs. shared interrupts.

One more idea, on top of the prototype patch that I posted
(https://patchwork.kernel.org/patch/4625921/).

The problem with enable_irq_wake() is that it only takes one argument, so if that's a shared interrupt, we can't really say which irqaction is supposed to handle wakeup interrupts should they occur.

To address this we can introduce enable_device_irq_wake() that will take an additional dev_id argument (that must be the one passed to request_irq() or the operation will fail) that can be used to identify the irqaction for handling the wakeup interrupts. It can work by setting IRQF_NO_SUSPEND for the irqaction in question and doing the rest along the lines of irq_set_irq_wake(irq, 1). disable_device_irq_wake() will then clear IRQF_NO_SUSPEND (it also has to be passed the dev_id argument).

If we have that, the guys who need to set up device interrupts (ie. interrupts normally used for signaling input events etc) for system wakeup will need to use enable_device_irq_wake() and that should just work.

One more idea, on top of the prototype patch that I posted
(https://patchwork.kernel.org/patch/4625921/).

The problem with enable_irq_wake() is that it only takes one argument, so if that's a shared interrupt, we can't really say which irqaction is supposed to handle wakeup interrupts should they occur.

To address this we can introduce enable_device_irq_wake() that will take an additional dev_id argument (that must be the one passed to request_irq() or the operation will fail) that can be used to identify the irqaction for handling the wakeup interrupts. It can work by setting IRQF_NO_SUSPEND for the irqaction in question and doing the rest along the lines of irq_set_irq_wake(irq, 1). disable_device_irq_wake() will then clear IRQF_NO_SUSPEND (it also has to be passed the dev_id argument).

If we have that, the guys who need to set up device interrupts (ie. interrupts normally used for signaling input events etc) for system wakeup will need to use enable_device_irq_wake() and that should just work.

Quoted 6 lines · Rafael J. Wysocki

One more idea, on top of the prototype patch that I posted
(https://patchwork.kernel.org/patch/4625921/).

The problem with enable_irq_wake() is that it only takes one argument, so if that's a shared interrupt, we can't really say which irqaction is supposed to handle wakeup interrupts should they occur.

Right.

Quoted 11 lines

To address this we can introduce enable_device_irq_wake() that will take an additional dev_id argument (that must be the one passed to request_irq() or the operation will fail) that can be used to identify the irqaction for handling the wakeup interrupts. It can work by setting IRQF_NO_SUSPEND for the irqaction in question and doing the rest along the lines of irq_set_irq_wake(irq, 1). disable_device_irq_wake() will then clear IRQF_NO_SUSPEND (it also has to be passed the dev_id argument).

If we have that, the guys who need to set up device interrupts (ie. interrupts normally used for signaling input events etc) for system wakeup will need to use enable_device_irq_wake() and that should just work.

So in the patch I posted I described why NO_SUSPEND is useful, I still have to hear a solid reason for why we also need enable_irq_wake()? What does it do that cannot be achieved with NO_SUSPEND?

I realize its dynamic, but that's crap, at device registration time it pretty much already knows if its a wakeup source or not, right?

Quoted 2 lines

I realize its dynamic, but that's crap, at device registration time it pretty much already knows if its a wakeup source or not, right?

It does, but that doesn't make it crap. There are situations where you want certain wakeup sources enabled or disabled and you can't do that with IRQF_NO_SUSPEND. And to support this, you need the wake_depth counter as well. And that's what we always had.

I'd rather say, that the "I can wakeup the machine and will do so no matter what flag" is more stupid than the wake mechanism.

So we are not going to make everything a single stupid flag and limit the usability of existing code. We rather go and try to remove the stupid flag before it becomes more wide spread.

And we cannot treat the wakeup thing the same way as the IRQF_NO_SUSPEND flag, because there is hardware where the irq line must be disabled at the normal (non suspend) interrupt controller, and the wake mechanism tells the PM microcontroller to monitor the interrupt line and kick the machine back to life.

So we need to very carefully look at all the existing cases instead of yelling crap and inflicting x86 specific horror on everyone. I said on friday, that I need to look at ALL use cases first and I meant it.

Quoted 8 lines · Peter Zijlstra

Quoted 6 lines · Rafael J. Wysocki

One more idea, on top of the prototype patch that I posted
(https://patchwork.kernel.org/patch/4625921/).

The problem with enable_irq_wake() is that it only takes one argument, so if that's a shared interrupt, we can't really say which irqaction is supposed to handle wakeup interrupts should they occur.

Quoted 1 line

Right.

Historically no hardware manufacturer was so stupid to have wakeup sources on shared interrupts. Just because x86 is brain damaged is no reason to claim that stuff that worked fine for years is crap.

We never needed this until now, so we simply go and do what we've done always in such situations. We look for a solution which copes with the new hardware brain farts and keeps the existing stuff working. It's that simple.

Quoted 12 lines

Quoted 11 lines

To address this we can introduce enable_device_irq_wake() that will take an additional dev_id argument (that must be the one passed to request_irq() or the operation will fail) that can be used to identify the irqaction for handling the wakeup interrupts. It can work by setting IRQF_NO_SUSPEND for the irqaction in question and doing the rest along the lines of irq_set_irq_wake(irq, 1). disable_device_irq_wake() will then clear IRQF_NO_SUSPEND (it also has to be passed the dev_id argument).

If we have that, the guys who need to set up device interrupts (ie. interrupts normally used for signaling input events etc) for system wakeup will need to use enable_device_irq_wake() and that should just work.

Quoted 6 lines

So in the patch I posted I described why NO_SUSPEND is useful, I still have to hear a solid reason for why we also need enable_irq_wake()? What does it do that cannot be achieved with NO_SUSPEND?

I realize its dynamic, but that's crap, at device registration time it pretty much already knows if its a wakeup source or not, right?

It does, but that doesn't make it crap. There are situations where you want certain wakeup sources enabled or disabled and you can't do that with IRQF_NO_SUSPEND. And to support this, you need the wake_depth counter as well. And that's what we always had.

I'd rather say, that the "I can wakeup the machine and will do so no matter what flag" is more stupid than the wake mechanism.

It was added to support XEN wreckage and I wish we've never done that in the first place.

So we are not going to make everything a single stupid flag and limit the usability of existing code. We rather go and try to remove the stupid flag before it becomes more wide spread.

And we cannot treat the wakeup thing the same way as the IRQF_NO_SUSPEND flag, because there is hardware where the irq line must be disabled at the normal (non suspend) interrupt controller, and the wake mechanism tells the PM microcontroller to monitor the interrupt line and kick the machine back to life.

So we need to very carefully look at all the existing cases instead of yelling crap and inflicting x86 specific horror on everyone. I said on friday, that I need to look at ALL use cases first and I meant it.

Quoted 2 lines · Thomas Gleixner

Quoted 1 line · Peter Zijlstra

On Sat, Jul 26, 2014 at 01:49:17PM +0200, Rafael J. Wysocki wrote:

Quoted 10 lines

So we are not going to make everything a single stupid flag and limit the usability of existing code. We rather go and try to remove the stupid flag before it becomes more wide spread.

And we cannot treat the wakeup thing the same way as the IRQF_NO_SUSPEND flag, because there is hardware where the irq line must be disabled at the normal (non suspend) interrupt controller, and the wake mechanism tells the PM microcontroller to monitor the interrupt line and kick the machine back to life.

Quoted 3 lines

So we need to very carefully look at all the existing cases instead of yelling crap and inflicting x86 specific horror on everyone. I said on friday, that I need to look at ALL use cases first and I meant it.

Regardless of the use case, I don't think it is necessary to manipulate the interrupt controller settings before the syscore_suspend stage, because if an interrupt happens earlier, we need to handle it pretty much in a normal way, unless it has been suspended.

So I'd argue for not using anything like enable_irq_wake() that goes all the way to the hardware in drivers. Instead, we could allow drivers to mark interrupts as "set this up for system wakeup" and really do the setup right before putting the platform into the final "suspended" state. And that is totally independend of the IRQF_NO_SUSPEND thing.

Quoted 3 lines · Rafael J. Wysocki

Quoted 2 lines · Thomas Gleixner

Quoted 1 line · Peter Zijlstra

On Sat, Jul 26, 2014 at 01:49:17PM +0200, Rafael J. Wysocki wrote:

[cut]

Quoted 19 lines

Quoted 10 lines

So we are not going to make everything a single stupid flag and limit the usability of existing code. We rather go and try to remove the stupid flag before it becomes more wide spread.

And we cannot treat the wakeup thing the same way as the IRQF_NO_SUSPEND flag, because there is hardware where the irq line must be disabled at the normal (non suspend) interrupt controller, and the wake mechanism tells the PM microcontroller to monitor the interrupt line and kick the machine back to life.

Quoted 3 lines

So we need to very carefully look at all the existing cases instead of yelling crap and inflicting x86 specific horror on everyone. I said on friday, that I need to look at ALL use cases first and I meant it.

Regardless of the use case, I don't think it is necessary to manipulate the interrupt controller settings before the syscore_suspend stage, because if an interrupt happens earlier, we need to handle it pretty much in a normal way, unless it has been suspended.

Quoted 5 lines

So I'd argue for not using anything like enable_irq_wake() that goes all the way to the hardware in drivers. Instead, we could allow drivers to mark interrupts as "set this up for system wakeup" and really do the setup right before putting the platform into the final "suspended" state. And that is totally independend of the IRQF_NO_SUSPEND thing.

In addition to that we need the interrupt handler of the driver that requested the irq to be set up for system wakeup to be invoked after suspend_device_irqs() in case there are interrupts that should abort the suspend transition or we can lose a wakeup event. So whatever interface we decide to use it has to affect suspend/resume_device_irqs() pretty much in the same way as the IRQF_NO_SUSPEND flag.

Quoted 32 lines · Rafael J. Wysocki

Quoted 3 lines · Rafael J. Wysocki

Quoted 2 lines · Thomas Gleixner

Quoted 1 line · Peter Zijlstra

On Sat, Jul 26, 2014 at 01:49:17PM +0200, Rafael J. Wysocki wrote:

[cut]

Quoted 19 lines

Quoted 10 lines

So we are not going to make everything a single stupid flag and limit the usability of existing code. We rather go and try to remove the stupid flag before it becomes more wide spread.

And we cannot treat the wakeup thing the same way as the IRQF_NO_SUSPEND flag, because there is hardware where the irq line must be disabled at the normal (non suspend) interrupt controller, and the wake mechanism tells the PM microcontroller to monitor the interrupt line and kick the machine back to life.

Quoted 3 lines

So we need to very carefully look at all the existing cases instead of yelling crap and inflicting x86 specific horror on everyone. I said on friday, that I need to look at ALL use cases first and I meant it.

Regardless of the use case, I don't think it is necessary to manipulate the interrupt controller settings before the syscore_suspend stage, because if an interrupt happens earlier, we need to handle it pretty much in a normal way, unless it has been suspended.

Quoted 5 lines

So I'd argue for not using anything like enable_irq_wake() that goes all the way to the hardware in drivers. Instead, we could allow drivers to mark interrupts as "set this up for system wakeup" and really do the setup right before putting the platform into the final "suspended" state. And that is totally independend of the IRQF_NO_SUSPEND thing.

Quoted 6 lines

In addition to that we need the interrupt handler of the driver that requested the irq to be set up for system wakeup to be invoked after suspend_device_irqs() in case there are interrupts that should abort the suspend transition or we can lose a wakeup event. So whatever interface we decide to use it has to affect suspend/resume_device_irqs() pretty much in the same way as the IRQF_NO_SUSPEND flag.

Right, that's a different issue. We probably want that even for the existing irq_wake() users.

Quoted 41 lines · Thomas Gleixner

Quoted 32 lines · Rafael J. Wysocki

Quoted 3 lines · Rafael J. Wysocki

Quoted 2 lines · Thomas Gleixner

Quoted 1 line · Peter Zijlstra

On Sat, Jul 26, 2014 at 01:49:17PM +0200, Rafael J. Wysocki wrote:

[cut]

Quoted 19 lines

Quoted 10 lines

So we are not going to make everything a single stupid flag and limit the usability of existing code. We rather go and try to remove the stupid flag before it becomes more wide spread.

And we cannot treat the wakeup thing the same way as the IRQF_NO_SUSPEND flag, because there is hardware where the irq line must be disabled at the normal (non suspend) interrupt controller, and the wake mechanism tells the PM microcontroller to monitor the interrupt line and kick the machine back to life.

Quoted 3 lines

So we need to very carefully look at all the existing cases instead of yelling crap and inflicting x86 specific horror on everyone. I said on friday, that I need to look at ALL use cases first and I meant it.

Regardless of the use case, I don't think it is necessary to manipulate the interrupt controller settings before the syscore_suspend stage, because if an interrupt happens earlier, we need to handle it pretty much in a normal way, unless it has been suspended.

Quoted 5 lines

So I'd argue for not using anything like enable_irq_wake() that goes all the way to the hardware in drivers. Instead, we could allow drivers to mark interrupts as "set this up for system wakeup" and really do the setup right before putting the platform into the final "suspended" state. And that is totally independend of the IRQF_NO_SUSPEND thing.

Quoted 6 lines

In addition to that we need the interrupt handler of the driver that requested the irq to be set up for system wakeup to be invoked after suspend_device_irqs() in case there are interrupts that should abort the suspend transition or we can lose a wakeup event. So whatever interface we decide to use it has to affect suspend/resume_device_irqs() pretty much in the same way as the IRQF_NO_SUSPEND flag.

Quoted 2 lines

Right, that's a different issue. We probably want that even for the existing irq_wake() users.

I agree.

There's one more thing to consider here. Going forward we'll want to avoid touching runtime-suspended devices during system suspend. Then, system wakeup devices will need to mark their IRQs for system wakeup at the runtime suspend time and I'm not sure if that's the right time for calling enable_irq_wake().

From: Rafael J. Wysocki <[email protected]>

Device drivers currently use enable_irq_wake() to configure their interrupts for system wakeup, but that API is not particularly well suited for this purpose, because it goes directly all the way to the hardware and attempts to change the IRQ configuration at the chip level.

The first problem with this approach is that the IRQ subsystem is not told which interrupt handler is supposed to handle interrupts from the wakeup line should they occur during system suspend or resume. That is problematic if the IRQ is shared and the other devices sharing it with the wakeup device in question are not wakeup devices. In that case their drivers may not be prepared to handle interrupts after the devices have been powered down and they may expect suspend_device_irqs() to disable the interrupt. For this reason, the IRQ should not be left enabled by suspend_device_irqs() in that case. On the other hand, though, it needs to be left enabled to prevent wakeup events occuring after suspend_device_irqs() has returned from being lost.

On Wed, 30 Jul 2014, Rafael J. Wysocki wrote:
Before this code changes in any way I want to see:

1) a description of the existing semantics and their background

2) a description of the short comings of the existing semantics w/o considering the new fangled (hardware) use cases

3) a description how to mitigate the short comings described in #2 w/o breaking the world and some more and introducing hard to decode regressions

4) a description why the improvements introduced by #3 are not sufficient for the new fangled (hardware) use cases

5) a description how to mitigate the short comings described in #4 w/o breaking the world and some more and introducing hard to decode regressions

Quoted 4 lines · Thomas Gleixner

On Wed, 30 Jul 2014, Rafael J. Wysocki wrote:
Before this code changes in any way I want to see:

1) a description of the existing semantics and their background

Quoted 7 lines

2) a description of the short comings of the existing semantics w/o considering the new fangled (hardware) use cases

3) a description how to mitigate the short comings described in #2 w/o breaking the world and some more and introducing hard to decode regressions

Quoted 6 lines

4) a description why the improvements introduced by #3 are not sufficient for the new fangled (hardware) use cases

5) a description how to mitigate the short comings described in #4 w/o breaking the world and some more and introducing hard to decode regressions

Aside of that I want to see a coherent explanation why a shared MSI interrupt makes any sense at all.

25: 1 <....> 0 PCI-MSI-edge aerdrv, PCIe PME

AFAICT, that's the primary reason why you insist to support wakeup sources on shared irq lines. And to be honest, it's utter bullshit.

The whole purpose of MSI is to avoid interrupt sharing, but of course if that particular interrupt source has two potential causes, i.e. the AER and the PME one and the stupid hardware does not support different vectors or the drivers are not able to do so, it's conveniant to make them shared instead of thinking about them what they really are:

Separate interrupts on a secondary interrupt controller connected to the primary (MSI) one.

That's what is in fact. Simply because you can enable/disable them at the same IP block quite contrary to stuff which is physically shared by hard wired electrical connections.

Quoted 5 lines · Thomas Gleixner

Quoted 4 lines · Thomas Gleixner

On Wed, 30 Jul 2014, Rafael J. Wysocki wrote:
Before this code changes in any way I want to see:

1) a description of the existing semantics and their background

On that one, the meaning of IRQF_NO_SUSPEND is quite simple to me.

If it is set (for the first irqaction in a given irq_desc) suspend_device_irqs() will leave that IRQ alone (that is, will not disable it and will not mark it as IRQS_SUSPENDED).

As a result, if an interrupt for that irq_desc happens after suspend_device_irqs(), the interrupt handlers from all of its irqactions will be invoked.

So this basically means "ignore that IRQ" to suspend_device_irqs() and that's its *only* meaning.

It's primary users are timers, per-CPU IRQs, ACPI SCI, via-pmu, Xen. There also is a bunch of drivers that use it for wakeup stuff, but they shouldn't in my opinion.

The background I'm aware of was primarily timers (we wanted to be able to msleep() during PCI PM transitions in the noirq phases of system suspend and resume among other things), and we want per-CPU stuff to work all the time too. ACPI uses it for signaling various types of events (including battery and thermal) that need to be handled all the time. I'm not sure why Xen needs it exactly, but that seems to be IPI-related.

The current handling of IRQF_NO_SUSPEND has a problem vs shared interrupts which I've tried to address by https://patchwork.kernel.org/patch/4643871/
and which is described in the changelog of that patch. Unfortunately, some existing users pass IRQF_SHARED along with IRQF_NO_SUSPEND which is the main motivation for that. Many of them use it for wakeup purposes, but for one example (that doesn't use it for wakeup only) the ACPI SCI is shareable by design.

Quoted 7 lines

Aside of that I want to see a coherent explanation why a shared MSI interrupt makes any sense at all.

25: 1 <....> 0 PCI-MSI-edge aerdrv, PCIe PME

AFAICT, that's the primary reason why you insist to support wakeup sources on shared irq lines. And to be honest, it's utter bullshit.

No, this isn't the primary reason.

Here's /proc/interrupts from my MSI Wind system and, as you can see, PCIe PME are (a) not MSI and (b) shared with some interesting things (USB, WiFi and the GPU).

 16:       5217          0   IO-APIC-fasteoi   PCIe PME, uhci_hcd:usb4, i915
 17:      13964          0   IO-APIC-fasteoi   PCIe PME, rtl818x_pci

Quoted 17 lines · Rafael J. Wysocki

Quoted 5 lines · Thomas Gleixner

Quoted 4 lines · Thomas Gleixner

On Wed, 30 Jul 2014, Rafael J. Wysocki wrote:
Before this code changes in any way I want to see:

1) a description of the existing semantics and their background

On that one, the meaning of IRQF_NO_SUSPEND is quite simple to me.

If it is set (for the first irqaction in a given irq_desc) suspend_device_irqs() will leave that IRQ alone (that is, will not disable it and will not mark it as IRQS_SUSPENDED).

As a result, if an interrupt for that irq_desc happens after suspend_device_irqs(), the interrupt handlers from all of its irqactions will be invoked.

Quoted 6 lines

So this basically means "ignore that IRQ" to suspend_device_irqs() and that's its *only* meaning.

It's primary users are timers, per-CPU IRQs, ACPI SCI, via-pmu, Xen. There also is a bunch of drivers that use it for wakeup stuff, but they shouldn't in my opinion.

Indeed.

Quoted 6 lines

The background I'm aware of was primarily timers (we wanted to be able to msleep() during PCI PM transitions in the noirq phases of system suspend and resume among other things), and we want per-CPU stuff to work all the time too. ACPI uses it for signaling various types of events (including battery and thermal) that need to be handled all the time. I'm not sure why Xen needs it exactly, but that seems to be IPI-related.

So none of these interrupts is used to abort suspend or wakeup.

They are kept functional because they are required for the suspend/resume transitions itself.

What's this PCIe PME handler doing? Is it required functionality for the suspend/resume path or is it a wakeup/abort mechanism.

Quoted 3 lines

The current handling of IRQF_NO_SUSPEND has a problem vs shared interrupts which I've tried to address by https://patchwork.kernel.org/patch/4643871/
and which is described in the changelog of that patch.

Quoted 5 lines

Unfortunately, some
existing users pass IRQF_SHARED along with IRQF_NO_SUSPEND which is the main motivation for that. Many of them use it for wakeup purposes, but for one example (that doesn't use it for wakeup only) the ACPI SCI is shareable by design.

So many of them use it for wakeup purposes. Why so and how is that supposed to work?

The mechanism for wakeup sources is:

1) Lazy disable the interrupt

2) Do the transition into suspend with interrupts enabled

3) Check whether one of the wakeup sources has triggered. If yes, abort. Otherwise suspend.

The ones marked IRQF_NO_SUSPEND are not part of the above scheme, because they are not checked. So they use different mechanisms to abort the suspend?

Quoted 13 lines

Quoted 7 lines

Aside of that I want to see a coherent explanation why a shared MSI interrupt makes any sense at all.

25: 1 <....> 0 PCI-MSI-edge aerdrv, PCIe PME

AFAICT, that's the primary reason why you insist to support wakeup sources on shared irq lines. And to be honest, it's utter bullshit.

No, this isn't the primary reason.

Here's /proc/interrupts from my MSI Wind system and, as you can see, PCIe PME are (a) not MSI and (b) shared with some interesting things (USB, WiFi and the GPU).

Quoted 2 lines

 16:       5217          0   IO-APIC-fasteoi   PCIe PME, uhci_hcd:usb4, i915
 17:      13964          0   IO-APIC-fasteoi   PCIe PME, rtl818x_pci

So the obvious question is: WHY are they not using MSI?

Just because MSI fucked up the MSI configuration of the device or is there any sane explanation for it?

And before we enter the wakeup handling slippery slope, let me make a note that this problem is bothering me quite a bit at the moment. In my opinion we need to address it somehow regardless of the wakeup issues and I'm not sure if failing __setup_irq() when there's a mismatch (that is, there are existing actions for the given irq_desc and their IRQF_NO_SUSPEND settings are not consistent with the new one) is the right way to do that, because it may make things behave a bit randomly (it will always fail the second guy, but that need not be the one who's requested IRQF_NO_SUSPEND and it depends on the ordering between them).

Quoted 9 lines · Rafael J. Wysocki

And before we enter the wakeup handling slippery slope, let me make a note that this problem is bothering me quite a bit at the moment. In my opinion we need to address it somehow regardless of the wakeup issues and I'm not sure if failing __setup_irq() when there's a mismatch (that is, there are existing actions for the given irq_desc and their IRQF_NO_SUSPEND settings are not consistent with the new one) is the right way to do that, because it may make things behave a bit randomly (it will always fail the second guy, but that need not be the one who's requested IRQF_NO_SUSPEND and it depends on the ordering between them).

Pardon me for sticking my nose into the middle of the conversation, but here's what it looks like to me:

The entire no_irq phase of suspend/resume is starting to seem like a mistake. We should never have done it.

Alternatively, it might be
okay to disable _all_ interrupts during the no_irq phase provided they are then _all_ enabled again before entering the sysdev and platform-specific parts of suspend (or the final freeze).

As I understand it, the idea behind the no_irq phase was to make life easier for drivers. They wouldn't have to worry about strange things cropping up while they're in the middle of powering down their devices or afterward.

Well, guess what? It turns out that they do have to worry about it after all. Timers can still fire during suspend transitions, and if an IRQ line is shared with a wakeup source then it won't be disabled.

The fact is, drivers should not rely on disabled interrupts to prevent untimely interrupt requests or wakeups. They should configure their devices not to generate any interrupt requests at all, apart from wakeup requests. And their interrupt handlers shouldn't mind being invoked for a shared IRQ, even after their devices are turned off.

Any driver that leaves its device capable of generating non-wakeup interrupt requests during suspend, and relies on interrupts being globally disabled to avoid problems, is most likely broken. Yes, it may be acceptable in cases where the IRQ line isn't shared, but it's still a bad design.

Quoted 5 lines · Alan Stern

Pardon me for sticking my nose into the middle of the conversation, but here's what it looks like to me:

The entire no_irq phase of suspend/resume is starting to seem like a mistake. We should never have done it.

In hindsight, I totally agree. Question is what we can do about it now.

So how can we eliminate the noirq phase in a workable way?

Quoted 8 lines · Rafael J. Wysocki

Quoted 5 lines · Alan Stern

Pardon me for sticking my nose into the middle of the conversation, but here's what it looks like to me:

The entire no_irq phase of suspend/resume is starting to seem like a mistake. We should never have done it.

In hindsight, I totally agree. Question is what we can do about it now.

<SNIP>

Quoted 1 line

So how can we eliminate the noirq phase in a workable way?

The straight way to do that is breaking the world and some more and then fix up a gazillion of device drivers by doing a massive voodoo debugging effort simply because in most cases we do not get any useful information out of the system once the shit hits the fan.

We could add instrumentation to the core code about interrupts which are coming in unexpectedly during suspend, but that does not solve anything.

We really cannot call any device handler at that point as clocks might be turned off already and any access to a device register might simply cause a full undebuggable stall of the CPU.

And there is no way to prove that there is no chance of a spurious interrupt for a given device.

So if we cannot handle it at the infrastructure level, we need to make sure that every fricking device driver interrupt handler has a

     if (dev->suspended)
     	return CRAP;

conditional as the first line of code in it.

What is that buying us?

Nothing than a shitload of hard to understand problems, really. The only sensible way to handle this is at the core level.

#1 There is no way that you can rely on random drivers to do the Right
Thing.

#2 There is no way that all hardware is implemented in a sane way.

#3 You CANNOT educate the people who are tasked to implement something
which "does the job" to understand all the subtle details of suspend/resume or whatever.

Anyone who thinks that this can and should be solved at the driver level is simply taking the wrong drugs or ran out of supply of the proper ones. Either call your shrink or your drug dealer to get out of that.

Quoted 2 lines · Thomas Gleixner

What's this PCIe PME handler doing? Is it required functionality for the suspend/resume path or is it a wakeup/abort mechanism.

It is a wakeup/abort mechanism.

And before we enter the wakeup handling slippery slope, let me make a note that this problem is bothering me quite a bit at the moment. In my opinion we need to address it somehow regardless of the wakeup issues and I'm not sure if failing __setup_irq() when there's a mismatch (that is, there are existing actions for the given irq_desc and their IRQF_NO_SUSPEND settings are not consistent with the new one) is the right way to do that, because it may make things behave a bit randomly (it will always fail the second guy, but that need not be the one who's requested IRQF_NO_SUSPEND and it depends on the ordering between them).

I had a couple of ideas, but none of them was particularly clean. Ideally, IRQF_NO_SUSPEND should always be requested without IRQF_SHARED, but I'm afraid that we can't really do that for the ACPI SCI, because that may cause problems to happen on some older systems where that interrupt is actually shared. On all systems I have immediate access to it isn't shared, but I remember seeing some where it was. On those systems the ACPI SCI itself would not be affected, because it is requested quite early during system init, but the other guys wanting to share the line with it would take a hit.

One thing I was thinking about was to return an error from suspend_device_irqs() if there was a mismatch between IRQF_NO_SUSPEND settings for different irqactions in the same irq_desc. That would make system suspend fail on systems where it is potentially unsafe, but at least any other functionality would not be affected.

Quoted 2 lines

So many of them use it for wakeup purposes. Why so and how is that supposed to work?

Quite frankly, I'm not sure why they use it. These are mostly drivers I'm not familiar with on platforms I'm not familiar with. My guess is that the lazy disable mechanism is not sufficient for them for some reason.

Quoted 9 lines

The mechanism for wakeup sources is:

1) Lazy disable the interrupt

2) Do the transition into suspend with interrupts enabled

3) Check whether one of the wakeup sources has triggered. If yes, abort. Otherwise suspend.

Quoted 3 lines

The ones marked IRQF_NO_SUSPEND are not part of the above scheme, because they are not checked. So they use different mechanisms to abort the suspend?

Well, if you look at the tegra_kbc driver, for example, it uses both enable_irq_wake() and IRQF_NO_SUSPEND. Why it does that, I don't know.

Other ones seem to be using pm_wakeup_event(), but that will only abort suspend when it is enabled upfront (it need not be). Moreover, it wasn't intended to be used that way.

It generally looks like things are used not as intended in the wakeup area, sadly. Perhaps that's my fault, because I wasn't looking carefully enough every time, but I wasn't directly involved in any of them IIRC.

I guess that's an opportunity to clean that up ...

And now there's one more piece of it which is suspend-to-idle (aka "freeze"). That doesn't go all the way to syscore_suspend(), but basically stops after finishing the "noirq" phase of suspending devices. Then, it idles the CPUs and waits for interrupts that will take them out of idle. Only some of those interrupts are wakeup events, so it only resumes when __pm_wakeup_event() or __pm_relax() is called in the process of handling the interrupts.

Of course, it could be implemented differently, but that was the simplest way to do that. It still can be changed, but I'd really like it not to have to go through all of the disabling nonboot CPUs and sysfore_suspend(), because that simply isn't necessary (and it avoids a metric ton of problems with CPU offline/online). And of course, it has to work on x86 too.

On x86, however, enable_irq_wake() always returns -ENXIO and nothing happens, because the chip doesn't have an ->irq_set_wake callback and doesn't flag itself as IRQCHIP_SKIP_SET_WAKE, so even if we found a way to do something equivalent to check_wakeup_irqs() for suspend-to-idle, it still wouldn't work on x86 for that reason.

Also, I wouldn't like to make suspend-to-idle more special than it really has to be, so it would be ideal if it could use as much of the same mechanics as regular platform-supported suspend as reasonably possible. The handling of wakeup events is crucial here, because it's needed to make suspend-to-idle really work and I'd like to make it as consistent as possible with the "regular" suspend. Now, that can be done in a couple of ways and some of them may be better than others for reasons that aren't known to me.

Quoted 5 lines · Rafael J. Wysocki

Quoted 2 lines · Thomas Gleixner

What's this PCIe PME handler doing? Is it required functionality for the suspend/resume path or is it a wakeup/abort mechanism.

It is a wakeup/abort mechanism.

So why is it using IRQF_NO_SUSPEND in the first place?

Just because x86 does not have irq wake implemented or flagged it's irq chips with IRQCHIP_SKIP_SET_WAKE.

Right, so instead of thinking about a proper solution driver folks just slap the next available thing on it w/o thinking about the consequences. But, thats partly our own fault due to lack of proper documentation.

Quoted 9 lines

And before we enter the wakeup handling slippery slope, let me make a note that this problem is bothering me quite a bit at the moment. In my opinion we need to address it somehow regardless of the wakeup issues and I'm not sure if failing __setup_irq() when there's a mismatch (that is, there are existing actions for the given irq_desc and their IRQF_NO_SUSPEND settings are not consistent with the new one) is the right way to do that, because it may make things behave a bit randomly (it will always fail the second guy, but that need not be the one who's requested IRQF_NO_SUSPEND and it depends on the ordering between them).

I totally agree that we want to fix it and I'm going to help. Though I really wanted to have a clear picture of the stuff before making decisions.

Quoted 9 lines

I had a couple of ideas, but none of them was particularly clean. Ideally, IRQF_NO_SUSPEND should always be requested without IRQF_SHARED, but I'm afraid that we can't really do that for the ACPI SCI, because that may cause problems to happen on some older systems where that interrupt is actually shared. On all systems I have immediate access to it isn't shared, but I remember seeing some where it was. On those systems the ACPI SCI itself would not be affected, because it is requested quite early during system init, but the other guys wanting to share the line with it would take a hit.

Quoted 5 lines

One thing I was thinking about was to return an error from suspend_device_irqs() if there was a mismatch between IRQF_NO_SUSPEND settings for different irqactions in the same irq_desc. That would make system suspend fail on systems where it is potentially unsafe, but at least any other functionality would not be affected.

That's one possible solution. See below.

Quoted 6 lines

Quoted 2 lines

So many of them use it for wakeup purposes. Why so and how is that supposed to work?

Quite frankly, I'm not sure why they use it. These are mostly drivers I'm not familiar with on platforms I'm not familiar with. My guess is that the lazy disable mechanism is not sufficient for them for some reason.

Looking at a few of them I fear the reason is that the developer did not understand the wakeup mechanism at all. Again that's probably our fault, because the whole business including the irq part lacks proper in depth documentation.

Quoted 9 lines

Quoted 9 lines

The mechanism for wakeup sources is:

1) Lazy disable the interrupt

2) Do the transition into suspend with interrupts enabled

3) Check whether one of the wakeup sources has triggered. If yes, abort. Otherwise suspend.

Quoted 6 lines

Quoted 3 lines

The ones marked IRQF_NO_SUSPEND are not part of the above scheme, because they are not checked. So they use different mechanisms to abort the suspend?

Well, if you look at the tegra_kbc driver, for example, it uses both enable_irq_wake() and IRQF_NO_SUSPEND. Why it does that, I don't know.

That doesn't make sense at all.

Quoted 3 lines

Other ones seem to be using pm_wakeup_event(), but that will only abort suspend when it is enabled upfront (it need not be). Moreover, it wasn't intended to be used that way.

Right. We should kill that before more people copy it blindly.

Quoted 5 lines

It generally looks like things are used not as intended in the wakeup area, sadly. Perhaps that's my fault, because I wasn't looking carefully enough every time, but I wasn't directly involved in any of them IIRC.

I guess that's an opportunity to clean that up ...

Agreed. I'm not frightened to do a tree wide sweep. Seems to become a habit :)

Quoted 7 lines

And now there's one more piece of it which is suspend-to-idle (aka "freeze"). That doesn't go all the way to syscore_suspend(), but basically stops after finishing the "noirq" phase of suspending devices. Then, it idles the CPUs and waits for interrupts that will take them out of idle. Only some of those interrupts are wakeup events, so it only resumes when __pm_wakeup_event() or __pm_relax() is called in the process of handling the interrupts.

Quoted 5 lines

Of course, it could be implemented differently, but that was the simplest way to do that. It still can be changed, but I'd really like it not to have to go through all of the disabling nonboot CPUs and sysfore_suspend(), because that simply isn't necessary (and it avoids a metric ton of problems with CPU offline/online). And of course, it has to work on x86 too.

Right. So one thing which would make the situation more clear is that the interrupt handler needs to tell the core code about this by returning something like IRQ_HANDLED_PMWAKE and the core kicks the suspend-to-idle logic back to life. I'm not sure whether the extra return value is actually necessary, we might even map it to IRQ_HANDLED in the core as we know that we are in the suspend state.

Quoted 5 lines

On x86, however, enable_irq_wake() always returns -ENXIO and nothing happens, because the chip doesn't have an ->irq_set_wake callback and doesn't flag itself as IRQCHIP_SKIP_SET_WAKE, so even if we found a way to do something equivalent to check_wakeup_irqs() for suspend-to-idle, it still wouldn't work on x86 for that reason.

There is no reason, why we can't flag the affected irq chips with IRQCHIP_SKIP_SET_WAKE. That was introduced to handle irq controllers which provide wakeup functionality but do not have this special extra magic which is implemented in ->irq_set_wake, i.e. talking to some PM controller directly.

Quoted 7 lines

Also, I wouldn't like to make suspend-to-idle more special than it really has to be, so it would be ideal if it could use as much of the same mechanics as regular platform-supported suspend as reasonably possible. The handling of wakeup events is crucial here, because it's needed to make suspend-to-idle really work and I'd like to make it as consistent as possible with the "regular" suspend. Now, that can be done in a couple of ways and some of them may be better than others for reasons that aren't known to me.

Sure. From the recent discussion I can see the following things we need to address:

1) IRQF_NO_SUSPEND:

- Investigate the (ab)use
- Fix the offending call sites
- Provide proper documentation when and why it is sane to use

2) check_wakeup_irqs()

Call it in the suspend-to-idle case and deal with the detected suspend abort from there. If we don't do that, we basically force people to hack creative workarounds into their driver, be it abusing IRQF_NO_SUSPEND or other completely brainwrecked modifications.

The approach of doing:

1) lazy disable
2) transition into suspend
3) check abort conditions

is the only sane way to keep the real suspend and the suspend-to-idle stuff in line.

Any other approach will create major headache, simply because by avoiding the conditional call in the PM core you force the driver developers to work around it and have conditional handling for that case at the driver level without having proper information about the actual suspend target, i.e. idle vs. real. I can predict the mess caused by that halfways precisely without using a crystal
ball: With the expected popularity of the suspend-to-idle approach this is going to approach infinite level rapidly.

Seriously we cannot tell people to use A for real suspend and B for suspend-to-idle. It must be the same mechanism. If that costs the price of some core code restructuring, we better pay that than dealing with the fallout of the other variant.

3) IRQF_SHARED

- The ideal solution for this would be to provide seperate virtual irq numbers for each device which sits on a shared interrupt line and have a pseudo interrupt chip handling the devices and the real interrupt line providing the "demux" function.

That would make life way simpler as we'd always deal with a single interrupt number per device. So the whole chained action pain would simply go away and the wake/no suspend functionality would not have to care at all about this whole shared interrupt trainwreck. You'd get per device interrupt stats for free as a bonus.

Unfortunately we can't pull that off w/o major surgery to a gazillion of device drivers, but I'm tempted to provide the infrastructure for this sooner than later.

Though for the PCIe root hub thing which has a shared MSI handler !?! we really want to implement that at the device level today. Sharing a MSI interrupt is just ass backwards.

- So we need to bite the bullet and handle the action chain as it is.

And we need to support it for the IRQF_NO_SUSPEND and the wake functionality.

For that we have two choices:

1) Use the separate pointer in the irq desc which stores the actions which are either not flagged IRQF_NO_SUSPEND or not declared as actual wakeup sources.

2) Have a separate handler function pointer in struct irqaction, which is used to store the actual device handler and have a core stub handler as the primary handler.

That stub handler would basically do:

if (!irqs_suspended)
return action->real_handler();
return IRQ_NONE;

The switch over of the handler can be done at setup/request_irq time when a shared non consistent flagged handler is detected or at enable_wake() time.

There is an advantage to #2:

The suspend/resume path does not care about any of this. It only penalizes the few shared handlers which are affected by this nonsense with the extra conditional in the hot handling path. Bad luck for the device which has to share the interrupt
line: complain to the morons who still think that shared interrupt lines are a brilliant idea.

Now for the enable/disable_wake() stuff we really need to have the mechanism which has the extra *dev_id argument to identify the proper handler. The existing interface should simply hand in NULL and if it's used on a shared interrupt line it should yell loudly and maybe even prevent suspend. Combined with the above we burden all the nonsense of handling shared interrupts in that context to the slow path.

Of course we need handling for the stupid case where the non wakeup device decides to fire interrupts on the shared line.

We certainly need a detection mechanism for this, but your current proposal of accelerating the spurious detection has a serious downside:

If there is a spurious interrupt on a shared edge type interrupt, which is crap by definition, but unfortunately reality, we never trigger the spurious detector, but we might render the device useless in the following case:

disk irq shares line with wakeup button irq

suspend
spurious interrupt caused by disk device

resume from a differnt wakeup source

Disk device interrupt is lost and therefor the disk is disfunctional until someone presses the wakeup button.

Admittedly a constructed scenario, but we had our share of pleasure to deal with lost edge type interrupts in the past and I can assure you it's not fun to debug.

So we rather error out on the safe side and disable the interrupt line right away, mark it pending and resume or abort suspend and tell why.

If we don't do that upfront we'll add
that feature after a painful debug session which starts with this information: Sporadically after suspend I have to hard reset the machine by pressing the power button for 10 seconds or removing the battery....

Note: This only applies to shared interrupt lines. We still have a sane spurious handling for proper designed systems which avoid the shared brainfarts.

4) Move the handling of resume/abort to the core

We really want either an explicit return value from the device interrupt handler which indicates that we should resume or abort the suspend transition or handle IRQ_HANDLED automatically at the core level in case of suspend.

If you let drivers do that, they will create a gazillion of solutions to abort/resume which will kill your ability to change any of the PM core internal mechanisms w/o doing a tree wide cleanup. Let's do it now before we get into the unavoidable exponential copy and paste phase.

5) Documentation

Add a metric ton of it!

Thoughts?

Quoted 2 lines · Thomas Gleixner

Quoted 1 line · Rafael J. Wysocki

On Thursday, July 31, 2014 12:44:24 PM Thomas Gleixner wrote:

Except for a couple of points where I'm not sure I understand you correctly (commented above), all of that sounds good to me.

I'm not sure about the ordering, though. It would be good to have a working replacement for the IRQF_NO_SUSPEND things that we'll be removing in 1, for example. So since we need to do 3) IRQF_SHARED for both IRQF_NO_SUSPEND and wakeup, as you said, would it be practical to start with that one?

Quoted 3 lines · Thomas Gleixner

Right, so instead of thinking about a proper solution driver folks just slap the next available thing on it w/o thinking about the consequences. But, thats partly our own fault due to lack of proper documentation.

Prototyping with the next available thing is not generally wrong in my view as long as this doesn't get to the code base without consideration.

Quoted 13 lines

Quoted 7 lines

And now there's one more piece of it which is suspend-to-idle (aka "freeze"). That doesn't go all the way to syscore_suspend(), but basically stops after finishing the "noirq" phase of suspending devices. Then, it idles the CPUs and waits for interrupts that will take them out of idle. Only some of those interrupts are wakeup events, so it only resumes when __pm_wakeup_event() or __pm_relax() is called in the process of handling the interrupts.

Quoted 5 lines

Of course, it could be implemented differently, but that was the simplest way to do that. It still can be changed, but I'd really like it not to have to go through all of the disabling nonboot CPUs and sysfore_suspend(), because that simply isn't necessary (and it avoids a metric ton of problems with CPU offline/online). And of course, it has to work on x86 too.

Quoted 7 lines

Right. So one thing which would make the situation more clear is that the interrupt handler needs to tell the core code about this by returning something like IRQ_HANDLED_PMWAKE and the core kicks the suspend-to-idle logic back to life. I'm not sure whether the extra return value is actually necessary, we might even map it to IRQ_HANDLED in the core as we know that we are in the suspend state.

I'm not sure I'm following you here. Do you mean that upon receiving IRQ_HANDLED_PMWAKE from an interrupt handler the core will know that this was a wakeup event and will trigger a resume from suspend-to-idle?

Quoted 5 lines

There is no reason, why we can't flag the affected irq chips with IRQCHIP_SKIP_SET_WAKE. That was introduced to handle irq controllers which provide wakeup functionality but do not have this special extra magic which is implemented in ->irq_set_wake, i.e. talking to some PM controller directly.

That may help, but then it won't prevent wakeup IRQs from being disabled by suspend_device_irqs(). Do I think correctly that we can re-enable them in the suspend-to-idle loop, before idling the CPUs?

Quoted 8 lines

2) check_wakeup_irqs()

Call it in the suspend-to-idle case and deal with the detected suspend abort from there. If we don't do that, we basically force people to hack creative workarounds into their driver, be it abusing IRQF_NO_SUSPEND or other completely brainwrecked modifications.

Quoted 8 lines

The approach of doing:

1) lazy disable
2) transition into suspend
3) check abort conditions

is the only sane way to keep the real suspend and the suspend-to-idle stuff in line.

Well, suspend-to-idle is a loop of the type

(1) wait for an interrupt
(2) iterrupt happens -> if wakeup, resume; else goto (1)

so I'm not sure how to match that with lazy disable? We basically would need to do something like:

(1) enable all wakeup interrupts
(2) wait for an interrupt
(3) interrupt happens
(4) disable all interrupts enabled in (1)
(5) if wakeup, resume (we'll resume interrupts later); else goto (1)

Is that what you mean?

Quoted 3 lines

Though for the PCIe root hub thing which has a shared MSI handler !?! we really want to implement that at the device level today. Sharing a MSI interrupt is just ass backwards.

There are more reasons for doing that. A PCIe port is really one device with multiple things to handle and they really need more coordination with each other than we have in the current code. In my opinion we should just use one PCIe port driver doing all of these things together instead of three separate drivers handling multiple artificially cut out devices on a virtual bus.

Problem is, people either have no time or are too scared to do that.

Quoted 10 lines

If there is a spurious interrupt on a shared edge type interrupt, which is crap by definition, but unfortunately reality, we never trigger the spurious detector, but we might render the device useless in the following case:

disk irq shares line with wakeup button irq

suspend
spurious interrupt caused by disk device

Quoted 4 lines

resume from a differnt wakeup source

Disk device interrupt is lost and therefor the disk is disfunctional until someone presses the wakeup button.

But the disk was supposed to be suspended, right? So its driver should not expect to receive any interrupts at that point anyway. Or am I missing anything?

I'm not sure about the ordering, though. It would be good to have a working replacement for the IRQF_NO_SUSPEND things that we'll be removing in 1, for example. So since we need to do 3) IRQF_SHARED for both IRQF_NO_SUSPEND and wakeup, as you said, would it be practical to start with that one?

Quoted 1 line · Rafael J. Wysocki

On Friday, August 01, 2014 12:16:23 AM Thomas Gleixner wrote:

Quoted 21 lines

Quoted 13 lines

Quoted 7 lines

And now there's one more piece of it which is suspend-to-idle (aka "freeze"). That doesn't go all the way to syscore_suspend(), but basically stops after finishing the "noirq" phase of suspending devices. Then, it idles the CPUs and waits for interrupts that will take them out of idle. Only some of those interrupts are wakeup events, so it only resumes when __pm_wakeup_event() or __pm_relax() is called in the process of handling the interrupts.

Quoted 5 lines

Of course, it could be implemented differently, but that was the simplest way to do that. It still can be changed, but I'd really like it not to have to go through all of the disabling nonboot CPUs and sysfore_suspend(), because that simply isn't necessary (and it avoids a metric ton of problems with CPU offline/online). And of course, it has to work on x86 too.

Quoted 7 lines

Right. So one thing which would make the situation more clear is that the interrupt handler needs to tell the core code about this by returning something like IRQ_HANDLED_PMWAKE and the core kicks the suspend-to-idle logic back to life. I'm not sure whether the extra return value is actually necessary, we might even map it to IRQ_HANDLED in the core as we know that we are in the suspend state.

Quoted 3 lines

I'm not sure I'm following you here. Do you mean that upon receiving IRQ_HANDLED_PMWAKE from an interrupt handler the core will know that this was a wakeup event and will trigger a resume from suspend-to-idle?

Correct. Whether we need the extra return code is debatable. But yes, we want to talk to the PM/suspend/resume thing at core level instead of letting drivers use random interfaces which happen to be exported for one reason or the other, but definitely not for the purpose of random driver.

Quoted 4 lines

I'm not sure about the ordering, though. It would be good to have a working replacement for the IRQF_NO_SUSPEND things that we'll be removing in 1, for example. So since we need to do 3) IRQF_SHARED for both IRQF_NO_SUSPEND and wakeup, as you said, would it be practical to start with that one?

The numbering was not meant as ordering, it was just to seperate the various issues which we need to look at.

OK, I guess "IRQ_HANDLED from a wakeup interrupt" may be interpreted as IRQ_HANDLED_PMWAKE. On the other hand, if that's going to be handled in handle_irq_event_percpu(), then using a special return code would save us a brach for IRQ_HANDLED interrupts. We could convert it to IRQ_HANDLED immediately then.

OK, I'll take a stab at the IRQF_SHARED thing if you don't mind.

Here's my current understanding of what can be done for IRQF_NO_SUSPEND.

In suspend_device_irqs():

(1) If all actions in the list have the same setting (eg. IRQF_NO_SUSPEND unset), keep the current behavior.
(2) If the actions have different settings:
- Actions with IRQF_NO_SUSPEND set are not modified.
- Actions with IRQF_NO_SUSPEND unset are switched over to a stub handler.
- IRQS_SUSPEND_MODE (new flag) is set for the IRQ.

Quoted 5 lines · Rafael J. Wysocki

OK, I guess "IRQ_HANDLED from a wakeup interrupt" may be interpreted as IRQ_HANDLED_PMWAKE. On the other hand, if that's going to be handled in handle_irq_event_percpu(), then using a special return code would save us a brach for IRQ_HANDLED interrupts. We could convert it to IRQ_HANDLED immediately then.

We can handle it at the end of the function by calling note_interrupt() unconditionally do the following there:

      if (suspended) {
      	 if (ret == IRQ_NONE) {
	    if (shared)
	       yell_and_abort_or_resume();
         } else {
	    abort_or_resume();
         }
      }
      if (noirqdebug)
      	 return;

Quoted 1 line

OK, I'll take a stab at the IRQF_SHARED thing if you don't mind.

Definitely not :)

Quoted 10 lines

Here's my current understanding of what can be done for IRQF_NO_SUSPEND.

In suspend_device_irqs():

(1) If all actions in the list have the same setting (eg. IRQF_NO_SUSPEND unset), keep the current behavior.
(2) If the actions have different settings:
- Actions with IRQF_NO_SUSPEND set are not modified.
- Actions with IRQF_NO_SUSPEND unset are switched over to a stub handler.
- IRQS_SUSPEND_MODE (new flag) is set for the IRQ.

Can we please do that in setup_irq() and let the shared ones always run through the stub? That keeps suspend/resume_device_irqs() simple.

Quoted 10 lines · Thomas Gleixner

Quoted 5 lines · Rafael J. Wysocki

OK, I guess "IRQ_HANDLED from a wakeup interrupt" may be interpreted as IRQ_HANDLED_PMWAKE. On the other hand, if that's going to be handled in handle_irq_event_percpu(), then using a special return code would save us a brach for IRQ_HANDLED interrupts. We could convert it to IRQ_HANDLED immediately then.

We can handle it at the end of the function by calling note_interrupt() unconditionally do the following there:

Quoted 10 lines

      if (suspended) {
      	 if (ret == IRQ_NONE) {
	    if (shared)
	       yell_and_abort_or_resume();
         } else {
	    abort_or_resume();
         }
      }
      if (noirqdebug)
      	 return;

I see.

Quoted 15 lines

Quoted 1 line

OK, I'll take a stab at the IRQF_SHARED thing if you don't mind.

Definitely not :)

Quoted 10 lines

Here's my current understanding of what can be done for IRQF_NO_SUSPEND.

In suspend_device_irqs():

(1) If all actions in the list have the same setting (eg. IRQF_NO_SUSPEND unset), keep the current behavior.
(2) If the actions have different settings:
- Actions with IRQF_NO_SUSPEND set are not modified.
- Actions with IRQF_NO_SUSPEND unset are switched over to a stub handler.
- IRQS_SUSPEND_MODE (new flag) is set for the IRQ.

Quoted 2 lines

Can we please do that in setup_irq() and let the shared ones always run through the stub? That keeps suspend/resume_device_irqs() simple.

OK

Quoted 39 lines · Rafael J. Wysocki

Quoted 10 lines · Thomas Gleixner

Quoted 5 lines · Rafael J. Wysocki

OK, I guess "IRQ_HANDLED from a wakeup interrupt" may be interpreted as IRQ_HANDLED_PMWAKE. On the other hand, if that's going to be handled in handle_irq_event_percpu(), then using a special return code would save us a brach for IRQ_HANDLED interrupts. We could convert it to IRQ_HANDLED immediately then.

We can handle it at the end of the function by calling note_interrupt() unconditionally do the following there:

Quoted 10 lines

      if (suspended) {
      	 if (ret == IRQ_NONE) {
	    if (shared)
	       yell_and_abort_or_resume();
         } else {
	    abort_or_resume();
         }
      }
      if (noirqdebug)
      	 return;

I see.

Quoted 15 lines

Quoted 1 line

OK, I'll take a stab at the IRQF_SHARED thing if you don't mind.

Definitely not :)

Quoted 10 lines

Here's my current understanding of what can be done for IRQF_NO_SUSPEND.

In suspend_device_irqs():

(1) If all actions in the list have the same setting (eg. IRQF_NO_SUSPEND unset), keep the current behavior.
(2) If the actions have different settings:
- Actions with IRQF_NO_SUSPEND set are not modified.
- Actions with IRQF_NO_SUSPEND unset are switched over to a stub handler.
- IRQS_SUSPEND_MODE (new flag) is set for the IRQ.

Quoted 4 lines

Quoted 2 lines

Can we please do that in setup_irq() and let the shared ones always run through the stub? That keeps suspend/resume_device_irqs() simple.

OK

I've tried to do that, but encountered a problem. The stub handler is called with irq, dev_id as arguments and for the "interrupts are not suspended" case (common) it has to use irq to find the irq_desc and then it has to use dev_id to find the irqaction to call the original handler from there. That seemed to be a bit too much of a penalty to me. Especially for people who never suspend their machines. :-)

For this reason, I went for changing handlers in suspend_device_irqs() and back in resume_device_irqs(). That's not terribly complicated (the restoration in particular is pretty simple) and it should be easily reusable for the wakeup interrupts case. resume_device_irqs() wouldn't need any more changes for that, for example. It minimally affects systems that don't suspend too.

I also ended up adding a new interrupt handler return code (IRQ_SUSPENDED). I could add a new irq_desc flag instead, but then the new code in suspend_device_irqs() and the new check in note_interrupt() would need to be slightly more complicated.

Quoted 58 lines · Rafael J. Wysocki

Quoted 39 lines · Rafael J. Wysocki

Quoted 10 lines · Thomas Gleixner

Quoted 5 lines · Rafael J. Wysocki

OK, I guess "IRQ_HANDLED from a wakeup interrupt" may be interpreted as IRQ_HANDLED_PMWAKE. On the other hand, if that's going to be handled in handle_irq_event_percpu(), then using a special return code would save us a brach for IRQ_HANDLED interrupts. We could convert it to IRQ_HANDLED immediately then.

We can handle it at the end of the function by calling note_interrupt() unconditionally do the following there:

Quoted 10 lines

      if (suspended) {
      	 if (ret == IRQ_NONE) {
	    if (shared)
	       yell_and_abort_or_resume();
         } else {
	    abort_or_resume();
         }
      }
      if (noirqdebug)
      	 return;

I see.

Quoted 15 lines

Quoted 1 line

OK, I'll take a stab at the IRQF_SHARED thing if you don't mind.

Definitely not :)

Quoted 10 lines

Here's my current understanding of what can be done for IRQF_NO_SUSPEND.

In suspend_device_irqs():

(1) If all actions in the list have the same setting (eg. IRQF_NO_SUSPEND unset), keep the current behavior.
(2) If the actions have different settings:
- Actions with IRQF_NO_SUSPEND set are not modified.
- Actions with IRQF_NO_SUSPEND unset are switched over to a stub handler.
- IRQS_SUSPEND_MODE (new flag) is set for the IRQ.

Quoted 4 lines

Quoted 2 lines

Can we please do that in setup_irq() and let the shared ones always run through the stub? That keeps suspend/resume_device_irqs() simple.

OK

I've tried to do that, but encountered a problem. The stub handler is called with irq, dev_id as arguments and for the "interrupts are not suspended" case (common) it has to use irq to find the irq_desc and then it has to use dev_id to find the irqaction to call the original handler from there. That seemed to be a bit too much of a penalty to me. Especially for people who never suspend their machines. :-)

For this reason, I went for changing handlers in suspend_device_irqs() and back in resume_device_irqs(). That's not terribly complicated (the restoration in particular is pretty simple) and it should be easily reusable for the wakeup interrupts case. resume_device_irqs() wouldn't need any more changes for that, for example. It minimally affects systems that don't suspend too.

Quoted 3 lines

I also ended up adding a new interrupt handler return code (IRQ_SUSPENDED). I could add a new irq_desc flag instead, but then the new code in suspend_device_irqs() and the new check in note_interrupt() would need to be slightly more complicated.

Actually, if we have a global irqs_suspended state variable, it will be much simpler to handle wakeup interrupts going forward, because in that case we'll be able to keep their original interrupt handlers and IRQ_HANDLED from them will be interpreted as a wakeup event automatically.

Quoted 10 lines · Thomas Gleixner

Quoted 5 lines · Rafael J. Wysocki

OK, I guess "IRQ_HANDLED from a wakeup interrupt" may be interpreted as IRQ_HANDLED_PMWAKE. On the other hand, if that's going to be handled in handle_irq_event_percpu(), then using a special return code would save us a brach for IRQ_HANDLED interrupts. We could convert it to IRQ_HANDLED immediately then.

We can handle it at the end of the function by calling note_interrupt() unconditionally do the following there:

Quoted 10 lines

      if (suspended) {
      	 if (ret == IRQ_NONE) {
	    if (shared)
	       yell_and_abort_or_resume();
         } else {
	    abort_or_resume();
         }
      }
      if (noirqdebug)
      	 return;

I see.

Quoted 15 lines

Quoted 1 line

OK, I'll take a stab at the IRQF_SHARED thing if you don't mind.

Definitely not :)

Quoted 10 lines

Here's my current understanding of what can be done for IRQF_NO_SUSPEND.

In suspend_device_irqs():

(1) If all actions in the list have the same setting (eg. IRQF_NO_SUSPEND unset), keep the current behavior.
(2) If the actions have different settings:
- Actions with IRQF_NO_SUSPEND set are not modified.
- Actions with IRQF_NO_SUSPEND unset are switched over to a stub handler.
- IRQS_SUSPEND_MODE (new flag) is set for the IRQ.

Quoted 2 lines

Can we please do that in setup_irq() and let the shared ones always run through the stub? That keeps suspend/resume_device_irqs() simple.

OK