| Message ID | 16387974.EqoNYrShmO@vostro.rjw.lan (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday, August 27, 2014 10:32:23 PM Thomas Gleixner wrote: > 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. We can do that of course. > Aside of that the whole irqaction based substitution is silly. What's > wrong with doing it at the real interrupt handler level? Nothing I suppose. :-) > 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); We still need to skip the IRQF_NO_SUSPEND stuff (eg. timers), so I guess everything left disabled here needs to be IRQS_SUSPENDED, so we know which ones to re-enable in resume_device_irqs(). > > /* 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; Hmm. There's no handler field in struct irq_desc (/me is puzzled). Did you mean handle_irq (I think you did)? > 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? OK There is quite some ugliness related to resume_irqs(), the want_early thing and IRQF_EARLY_RESUME / IRQF_FORCE_RESUME. I guess that needs to be preserved? > 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. I was concerned about the (unlikely) possibility of freeing an interrupt having a temporary handler. Never mind. Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 28 Aug 2014, Rafael J. Wysocki wrote: > On Wednesday, August 27, 2014 10:32:23 PM Thomas Gleixner wrote: > > void suspend_device_irqs(void) > > { > > for_each_irq_desc(irq, desc) { > > /* Disable the interrupt unconditionally */ > > disable_irq(irq); > > We still need to skip the IRQF_NO_SUSPEND stuff (eg. timers), so I guess > everything left disabled here needs to be IRQS_SUSPENDED, so we know which > ones to re-enable in resume_device_irqs(). Right. I skipped that one for simplicity. I wanted to look into the whole maze today again with brain awake. I think it's simple to integrate the no suspend magic here and have a separate handler for it. > > > > /* 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; > > Hmm. There's no handler field in struct irq_desc (/me is puzzled). > > Did you mean handle_irq (I think you did)? Yup. > There is quite some ugliness related to resume_irqs(), the want_early thing > and IRQF_EARLY_RESUME / IRQF_FORCE_RESUME. I guess that needs to be preserved? Probably. Did not look into the madness of that yet. Thanks, tglx -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thursday, August 28, 2014 11:23:11 AM Thomas Gleixner wrote: > On Thu, 28 Aug 2014, Rafael J. Wysocki wrote: > > On Wednesday, August 27, 2014 10:32:23 PM Thomas Gleixner wrote: > > > void suspend_device_irqs(void) > > > { > > > for_each_irq_desc(irq, desc) { > > > /* Disable the interrupt unconditionally */ > > > disable_irq(irq); > > > > We still need to skip the IRQF_NO_SUSPEND stuff (eg. timers), so I guess > > everything left disabled here needs to be IRQS_SUSPENDED, so we know which > > ones to re-enable in resume_device_irqs(). > > Right. I skipped that one for simplicity. I wanted to look into the > whole maze today again with brain awake. I think it's simple to > integrate the no suspend magic here and have a separate handler for > it. Well, I've already read your message about this particular thing. :-) Before that I was about to say that I'd rather leave the no suspend stuff as is for now until we have working wakeup IRQs for suspend-to-idle at least. > > > > > > /* 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; > > > > Hmm. There's no handler field in struct irq_desc (/me is puzzled). > > > > Did you mean handle_irq (I think you did)? > > Yup. So I got that to work earlier today, but with some more stuff in the handler which doesn't look particularly generic to me. Namely, my (working) wakeup handler looks like this at the moment: static void handle_wakeup_irq(unsigned int irq, struct irq_desc *desc) { struct irq_chip *chip = irq_desc_get_chip(desc); raw_spin_lock(&desc->lock); desc->istate &= ~(IRQS_REPLAY | IRQS_WAITING); kstat_incr_irqs_this_cpu(irq, desc); if (irqd_irq_disabled(&desc->irq_data)) { desc->istate |= IRQS_PENDING; mask_irq(desc); } else { desc->istate |= IRQS_SUSPENDED | IRQS_PENDING; desc->depth++; irq_disable(desc); pm_system_wakeup(); } if (chip->irq_eoi && !(chip->flags & IRQCHIP_EOI_IF_HANDLED)) chip->irq_eoi(&desc->irq_data); raw_spin_unlock(&desc->lock); } but it only works on a particular machine with a particular wakeup interrupt. I had to add the if (chip->irq_eoi && !(chip->flags & IRQCHIP_EOI_IF_HANDLED)) chip->irq_eoi(&desc->irq_data); check, because otherwise it hanged the system solid once the interrupt happened. I also had to add this: if (irqd_irq_disabled(&desc->irq_data)) { desc->istate |= IRQS_PENDING; mask_irq(desc); } ... because otherwise the IRQ didn't work correctly after resume. However, both those things appear to be needed just because that's a fastEOI interrupt and some other manipulations would need to be done for edge interrupts etc. I'm not sure, then, if we really can come up with a perfectly generic handle_irq wakeup interrupt handler that would work with all kinds of IRQs. It looks like the part that could be replaced in all of them in principle is the handle_irq_event() call, but the rest seems to be too specific to me. So I'm not sure what to do at this point. It is tempting to do the following: (1) Add a handle_event callback to struct irq_desc. (2) Rename the existing handle_irq_event() to handle_irq_event_common(). (3) Point desc->handle_event to handle_irq_event_common() for every desc at init. (4) Make (new, possibly static inline) handle_irq_event() invoke desc->handle_event(). (5) During suspend_device_irqs() point desc->handle_event to handle_irq_event_wakeup() for all wakeup IRQs. (6) During resume_device_irqs() point desc->handle_event back to handle_irq_event_common() for everybody. Then, for CONFIG_PM_SLEEP unset, handle_irq_event() can be defined as handle_irq_event_common(). Of course, that adds a function pointer dereference overhead to every interrupt event, so I'm not sure if it's acceptable. One alternative may be to add a handle_wakeup_irq pointer to struct irq_desc and check in handle_irq_event() if that is present and call it instead of the usual stuff if that's the case. Another way may be to introduce an irq_desc flag that will be checked by handle_irq_event() and will indicate "wakeup mode" to it, in which it will do the simplified wakeup handling instead of the usual stuff. This, however, like the previous one, will add a branch to handle_irq_event() that will be checked every time even though it only is really necessary during system suspend. And handlers can be replaced at the irqaction level in a couple of ways too. Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Index: linux-pm/include/linux/interrupt.h =================================================================== --- linux-pm.orig/include/linux/interrupt.h +++ linux-pm/include/linux/interrupt.h @@ -101,6 +101,8 @@ typedef irqreturn_t (*irq_handler_t)(int * @thread_flags: flags related to @thread * @thread_mask: bitmask for keeping track of @thread activity * @dir: pointer to the proc/irq/NN/name entry + * @s_handler: original interrupt handler for wakeup mode interrupts + * @s_dev_id: original device identification cookie for wakeup mode */ struct irqaction { irq_handler_t handler; @@ -115,6 +117,10 @@ struct irqaction { unsigned long thread_mask; const char *name; struct proc_dir_entry *dir; +#ifdef CONFIG_PM_SLEEP + irq_handler_t s_handler; + void *s_dev_id; +#endif } ____cacheline_internodealigned_in_smp; extern irqreturn_t no_action(int cpl, void *dev_id); @@ -193,6 +199,7 @@ extern void irq_wake_thread(unsigned int /* The following three functions are for the core kernel use only. */ extern void suspend_device_irqs(void); extern void resume_device_irqs(void); +extern void wakeup_mode_for_irqs(bool enable); #ifdef CONFIG_PM_SLEEP extern int check_wakeup_irqs(void); #else Index: linux-pm/kernel/irq/internals.h =================================================================== --- linux-pm.orig/kernel/irq/internals.h +++ linux-pm/kernel/irq/internals.h @@ -194,3 +194,17 @@ static inline void kstat_incr_irqs_this_ __this_cpu_inc(*desc->kstat_irqs); __this_cpu_inc(kstat.irqs_sum); } + +#ifdef CONFIG_PM_SLEEP +static inline bool irq_pm_saved_id(struct irqaction *action, void *dev_id) +{ + return action->s_dev_id == dev_id; +} +extern void irq_pm_restore_handler(struct irqaction *action); +#else +static inline bool irq_pm_saved_id(struct irqaction *action, void *dev_id) +{ + return false; +} +static inline void irq_pm_restore_handler(struct irqaction *action) {} +#endif Index: linux-pm/kernel/irq/manage.c =================================================================== --- linux-pm.orig/kernel/irq/manage.c +++ linux-pm/kernel/irq/manage.c @@ -1328,7 +1328,7 @@ static struct irqaction *__free_irq(unsi return NULL; } - if (action->dev_id == dev_id) + if (action->dev_id == dev_id || irq_pm_saved_id(action, dev_id)) break; action_ptr = &action->next; } @@ -1336,6 +1336,8 @@ static struct irqaction *__free_irq(unsi /* Found it - now remove it from the list of entries: */ *action_ptr = action->next; + irq_pm_restore_handler(action); + /* If this was the last handler, shut down the IRQ line: */ if (!desc->action) { irq_shutdown(desc); Index: linux-pm/kernel/irq/pm.c =================================================================== --- linux-pm.orig/kernel/irq/pm.c +++ linux-pm/kernel/irq/pm.c @@ -9,10 +9,95 @@ #include <linux/irq.h> #include <linux/module.h> #include <linux/interrupt.h> +#include <linux/suspend.h> #include <linux/syscore_ops.h> #include "internals.h" +void irq_pm_restore_handler(struct irqaction *action) +{ + if (action->s_handler) { + action->handler = action->s_handler; + action->s_handler = NULL; + action->dev_id = action->s_dev_id; + action->s_dev_id = NULL; + } +} + +static void irq_pm_substitute_handler(struct irqaction *action, + irq_handler_t new_handler) +{ + if (!action->s_handler) { + action->s_handler = action->handler; + action->handler = new_handler; + action->s_dev_id = action->dev_id; + action->dev_id = action; + } +} + +static irqreturn_t irq_wakeup_mode_handler(int irq, void *dev_id) +{ + struct irqaction *action = dev_id; + struct irq_desc *desc; + + if (action->next) + return IRQ_NONE; + + desc = irq_to_desc(irq); + desc->istate |= IRQS_SUSPENDED | IRQS_PENDING; + desc->depth++; + irq_disable(desc); + pm_system_wakeup(); + return IRQ_HANDLED; +} + +static void irq_pm_wakeup_mode(struct irq_desc *desc) +{ + struct irqaction *action; + + for (action = desc->action; action; action = action->next) + irq_pm_substitute_handler(action, irq_wakeup_mode_handler); +} + +static void irq_pm_normal_mode(struct irq_desc *desc) +{ + struct irqaction *action; + + for (action = desc->action; action; action = action->next) + irq_pm_restore_handler(action); +} + +void wakeup_mode_for_irqs(bool enable) +{ + struct irq_desc *desc; + int irq; + + for_each_irq_desc(irq, desc) { + struct irqaction *action = desc->action; + unsigned long flags; + + raw_spin_lock_irqsave(&desc->lock, flags); + + if (action && irqd_is_wakeup_set(&desc->irq_data)) { + if (enable) { + if (desc->istate & IRQS_SUSPENDED) { + irq_pm_wakeup_mode(desc); + desc->istate &= ~IRQS_SUSPENDED; + __enable_irq(desc, irq, false); + } + } else { + if (!(desc->istate & IRQS_SUSPENDED)) { + __disable_irq(desc, irq, false); + desc->istate |= IRQS_SUSPENDED; + } + irq_pm_normal_mode(desc); + } + } + + raw_spin_unlock_irqrestore(&desc->lock, flags); + } +} + /** * suspend_device_irqs - disable all currently enabled interrupt lines * Index: linux-pm/kernel/power/suspend.c =================================================================== --- linux-pm.orig/kernel/power/suspend.c +++ linux-pm/kernel/power/suspend.c @@ -28,6 +28,7 @@ #include <linux/ftrace.h> #include <trace/events/power.h> #include <linux/compiler.h> +#include <linux/interrupt.h> #include "power.h" @@ -55,7 +56,9 @@ static void freeze_enter(void) { cpuidle_use_deepest_state(true); cpuidle_resume(); + wakeup_mode_for_irqs(true); wait_event(suspend_freeze_wait_head, suspend_freeze_wake); + wakeup_mode_for_irqs(false); cpuidle_pause(); cpuidle_use_deepest_state(false); }