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() / reenable_stuff_and_fiddle_with_irq_action() 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) { raw_spin_lock(&desc->lock); desc->istate |= IRQS_SUSPENDED | IRQS_PENDING; desc->depth++; irq_disable(desc); pm_system_wakeup(); raw_spin_unlock(&desc->lock); } void suspend_device_irqs(void) { for_each_irq_desc(irq, desc) { /* Disable the interrupt unconditionally */ disable_irq(irq); /* Is the irq a wakeup source? */ if (!irqd_is_wakeup_set(&desc->irq_data)) continue; /* 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 */ enable_irq(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 */ disable_irq(); 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); enable_irq(); } } Hmm? 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. Thanks, tglx -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html