Re: [PATCH 2/5 v3] irq / PM: Make wakeup interrupts work with suspend-to-idle

From: Thomas Gleixner
Date: Wed Aug 27 2014 - 16:32:36 EST

On Wed, 27 Aug 2014, Rafael J. Wysocki wrote:
> The line of reasoning leading to that is as follows.
> The way suspend_device_irqs() works and the existing code in
> check_wakeup_irqs(), called by syscore_suspend(), imply that:
> (1) Interrupt handlers are not invoked for wakeup interrupts
> after suspend_device_irqs().
> (2) All interrups from system wakeup IRQs received after\
> suspend_device_irqs() cause full system suspends to be aborted.
> In addition to the above, there is the requirement that
> (3) System wakeup interrupts should wake up the system from
> suspend-to-idle.
> It immediately follows from (1) and (2) that no effort is made to
> distinguish "genuine" wakeup interrupts from "spurious" ones. They
> all are treated in the same way. Since (3) means that "genuine"
> wakeup interrupts are supposed to wake up the system from
> suspend-to-idle too, consistency with (1) and (2) requires that
> "spurious" wakeup interrupts should do the same thing. Thus there is
> no reason to invoke interrupt handlers for wakeup interrups after
> suspend_device_irqs() in the suspend-to-idle case. Moreover, doing
> so would go against rule (1).

I agree with that, but I disagree with the implementation.

We now have two separate mechanisms to abort suspend:

1) The existing suspend_device_irqs() / check_wakeup_irqs()

2) The new suspend_device_irqs() /

So why do we need those two mechanisms in the first place?

AFAICT there is no reason why we cant use the abort_suspend mechanics
to replace the suspend_device_irqs() / check_wakeup_irqs() pair.

All it needs is to do the handler substitution in
suspend_device_irqs() right away and replace the loop in
check_wakeup_irqs() with a check for abort_suspend == true. The roll
back of the handler substitution can happen in resume_device_irqs()
for both scenarios.

Aside of that the whole irqaction based substitution is silly. What's
wrong with doing it at the real interrupt handler level?

static void handle_wakeup_irq(unsigned int irq, struct irq_desc *desc)



void suspend_device_irqs(void)
for_each_irq_desc(irq, desc) {
/* Disable the interrupt unconditionally */

/* Is the irq a wakeup source? */
if (!irqd_is_wakeup_set(&desc->irq_data))

/* Replace the handler */
raw_spin_lock_irqsave(&desc->lock, flags);
desc->saved_handler = desc->handler;
desc->handler = handle_wakeup_irq;
raw_spin_unlock_irqrestore(&desc->lock, flags);

/* Reenable the wakeup irq */

/* Move that into the pm core code */
bool check_wakeup_irqs(void)
return abort_suspend;

void resume_device_irqs(void)
for_each_irq_desc(irq, desc) {

/* Prevent the wakeup handler from running */

raw_spin_lock_irqsave(&desc->lock, flags);

/* Do we need to restore the handler? */
if (desc->handler == handle_wakeup_irq)
desc->handler = desc->saved_handler;

/* Is the irq a wakeup source? */
if (!irqd_is_wakeup_set(&desc->irq_data))
__enable_irq(irq, desc);

/* Did it get disabled in the wakeup handler? */
else if (desc->istate & IRQS_SUSPENDED)
__enable_irq(irq, desc);

raw_spin_unlock_irqrestore(&desc->lock, flags);



One thing we might think about is having flow specific
handle_wakeup_irq variants as some hardware might require an ack or
eoi, but that's a simple to solve problem and way simpler than
fiddling with the irqaction chain and avoids the whole mess of
sprinkling irq_pm_saved_id() and irq_pm_restore_handler() calls all
over the place. I wonder why you added them to __free_irq() at all,
but no, we dont want that.



To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at
Please read the FAQ at