| Message ID | 2154999.DTKV1bGBcx@vostro.rjw.lan (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Rafael, On Wed, 25 Feb 2015 22:59:36 +0100 "Rafael J. Wysocki" <rjw@rjwysocki.net> wrote: > On Tuesday, February 24, 2015 10:55:59 AM Boris Brezillon wrote: > > Hello, > > > > I put the IRQF_NO_SUSPEND_SAFE/IRQF_TIMER_SIBLING_OK/WHATEVER_NAME_YOU_CHOOSE > > debate aside to concentrate on another problem pointed out by Rafael and > > Mark: the fact that we cannot mix IRQF_NO_SUSPEND and wakeup sources on > > a shared IRQ line. > > > > This is because the wakeup code is prevailing the IRQF_NO_SUSPEND case > > and will trigger a system wakeup as soon as the IRQ line is tagged as a > > wakeup source. > > > > This series propose an approach to deal with such cases by doing the > > following: > > 1/ Prevent any system wakeup when at least one of the IRQ user has set > > the IRQF_NO_SUSPEND flag > > 2/ Adapt IRQ handlers so that they can safely be called in suspended > > state > > 3/ Let drivers decide when the system should be woken up > > > > Let me know what you think of this approach. > > So I have the appended patch that should deal with all that too (it doesn't > rework drivers that need to share NO_SUSPEND IRQs and do wakeup, but that > can be done on top of it in a straightforward way). > > The idea is quite simple. By default, the core replaces the interrupt handlers > of everyone sharing NO_SUSPEND lines and not using IRQF_NO_SUSPEND with a null > handler always returning IRQ_NONE at the suspend_device_irqs() time (the > rationale being that if you don't use IRQF_NO_SUSPEND, then your device has > no reason to generate interrupts after that point). The original handlers are > then restored by resume_device_irqs(). > > However, if the IRQ is configured for wakeup, there may be a reason to generate > interrupts from a device not using IRQF_NO_SUSPEND. For that, the patch adds > IRQF_COND_SUSPEND that, if set, will prevent the default behavior described > above from being applied to irqactions using it if the IRQs in question are > configured for wakeup. Of course, the users of IRQF_COND_SUSPEND are supposed > to implement wakeup detection in their interrupt handlers and then call > pm_system_wakeup() if necessary. That patch sounds good to me. Could you send it on its own to the appropriate MLs ? Thomas, Peter, Mark, could you share you opinion ? I know I'm a bit insistent on this fix, but I'd really like to get away from this warning backtrace (and the associated problems behind it) as soon as possible. > > So your patch [3/3] could be redone on top of this AFAICS. Absolutely. Thanks. Boris > > Rafael > > > --- > include/linux/interrupt.h | 10 +++++++++ > kernel/irq/pm.c | 49 ++++++++++++++++++++++++++++++++++++++++++---- > 2 files changed, 55 insertions(+), 4 deletions(-) > > Index: linux-pm/include/linux/interrupt.h > =================================================================== > --- linux-pm.orig/include/linux/interrupt.h > +++ linux-pm/include/linux/interrupt.h > @@ -57,6 +57,11 @@ > * IRQF_NO_THREAD - Interrupt cannot be threaded > * IRQF_EARLY_RESUME - Resume IRQ early during syscore instead of at device > * resume time. > + * IRQF_COND_SUSPEND - If the IRQ is shared with a NO_SUSPEND user and it is > + * configured for system wakeup, execute this interrup handler > + * after suspending interrupts as it may be necessary to detect > + * wakeup. Users need to implement system wakeup detection in > + * their interrupt handlers. > */ > #define IRQF_DISABLED 0x00000020 > #define IRQF_SHARED 0x00000080 > @@ -70,6 +75,7 @@ > #define IRQF_FORCE_RESUME 0x00008000 > #define IRQF_NO_THREAD 0x00010000 > #define IRQF_EARLY_RESUME 0x00020000 > +#define IRQF_COND_SUSPEND 0x00040000 > > #define IRQF_TIMER (__IRQF_TIMER | IRQF_NO_SUSPEND | IRQF_NO_THREAD) > > @@ -101,6 +107,7 @@ 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 > + * @saved_handler: address of the original interrupt handler function > */ > struct irqaction { > irq_handler_t handler; > @@ -115,6 +122,9 @@ struct irqaction { > unsigned long thread_mask; > const char *name; > struct proc_dir_entry *dir; > +#ifdef CONFIG_PM_SLEEP > + irq_handler_t saved_handler; > +#endif > } ____cacheline_internodealigned_in_smp; > > extern irqreturn_t no_action(int cpl, void *dev_id); > Index: linux-pm/kernel/irq/pm.c > =================================================================== > --- linux-pm.orig/kernel/irq/pm.c > +++ linux-pm/kernel/irq/pm.c > @@ -43,9 +43,6 @@ void irq_pm_install_action(struct irq_de > > if (action->flags & IRQF_NO_SUSPEND) > desc->no_suspend_depth++; > - > - WARN_ON_ONCE(desc->no_suspend_depth && > - desc->no_suspend_depth != desc->nr_actions); > } > > /* > @@ -63,11 +60,53 @@ void irq_pm_remove_action(struct irq_des > desc->no_suspend_depth--; > } > > +static irqreturn_t irq_pm_null_handler(int irq, void *context) > +{ > + return IRQ_NONE; > +} > + > +static void prepare_no_suspend_irq(struct irq_desc *desc) > +{ > + struct irqaction *action; > + > + for (action = desc->action; action; action = action->next) { > + if (action->flags & IRQF_NO_SUSPEND) > + continue; > + > + if ((action->flags & IRQF_COND_SUSPEND) && > + irqd_is_wakeup_set(&desc->irq_data)) > + continue; > + > + action->saved_handler = action->handler; > + action->handler = irq_pm_null_handler; > + } > +} > + > +static void restore_no_suspend_irq(struct irq_desc *desc) > +{ > + struct irqaction *action; > + > + for (action = desc->action; action; action = action->next) { > + if (action->flags & IRQF_NO_SUSPEND) > + continue; > + > + if (action->saved_handler) { > + action->handler = action->saved_handler; > + action->saved_handler = NULL; > + } > + } > +} > + > static bool suspend_device_irq(struct irq_desc *desc, int irq) > { > - if (!desc->action || desc->no_suspend_depth) > + if (!desc->action) > return false; > > + if (desc->no_suspend_depth) { > + prepare_no_suspend_irq(desc); > + return false; > + } > + > if (irqd_is_wakeup_set(&desc->irq_data)) { > irqd_set(&desc->irq_data, IRQD_WAKEUP_ARMED); > /* > @@ -135,6 +174,8 @@ static void resume_irq(struct irq_desc * > if (desc->istate & IRQS_SUSPENDED) > goto resume; > > + restore_no_suspend_irq(desc); > + > /* Force resume the interrupt? */ > if (!desc->force_resume_depth) > return; >
On Thursday, February 26, 2015 09:03:47 AM Boris Brezillon wrote: > Hi Rafael, > > On Wed, 25 Feb 2015 22:59:36 +0100 > "Rafael J. Wysocki" <rjw@rjwysocki.net> wrote: > > > On Tuesday, February 24, 2015 10:55:59 AM Boris Brezillon wrote: > > > Hello, > > > > > > I put the IRQF_NO_SUSPEND_SAFE/IRQF_TIMER_SIBLING_OK/WHATEVER_NAME_YOU_CHOOSE > > > debate aside to concentrate on another problem pointed out by Rafael and > > > Mark: the fact that we cannot mix IRQF_NO_SUSPEND and wakeup sources on > > > a shared IRQ line. > > > > > > This is because the wakeup code is prevailing the IRQF_NO_SUSPEND case > > > and will trigger a system wakeup as soon as the IRQ line is tagged as a > > > wakeup source. > > > > > > This series propose an approach to deal with such cases by doing the > > > following: > > > 1/ Prevent any system wakeup when at least one of the IRQ user has set > > > the IRQF_NO_SUSPEND flag > > > 2/ Adapt IRQ handlers so that they can safely be called in suspended > > > state > > > 3/ Let drivers decide when the system should be woken up > > > > > > Let me know what you think of this approach. > > > > So I have the appended patch that should deal with all that too (it doesn't > > rework drivers that need to share NO_SUSPEND IRQs and do wakeup, but that > > can be done on top of it in a straightforward way). > > > > The idea is quite simple. By default, the core replaces the interrupt handlers > > of everyone sharing NO_SUSPEND lines and not using IRQF_NO_SUSPEND with a null > > handler always returning IRQ_NONE at the suspend_device_irqs() time (the > > rationale being that if you don't use IRQF_NO_SUSPEND, then your device has > > no reason to generate interrupts after that point). The original handlers are > > then restored by resume_device_irqs(). > > > > However, if the IRQ is configured for wakeup, there may be a reason to generate > > interrupts from a device not using IRQF_NO_SUSPEND. For that, the patch adds > > IRQF_COND_SUSPEND that, if set, will prevent the default behavior described > > above from being applied to irqactions using it if the IRQs in question are > > configured for wakeup. Of course, the users of IRQF_COND_SUSPEND are supposed > > to implement wakeup detection in their interrupt handlers and then call > > pm_system_wakeup() if necessary. > > That patch sounds good to me. But it is still a bit risky. Namely, if the driver in question is sufficiently broken (eg. it may not suspend the device and rely on the fact that its interrupt handler will be run just because it is sharing a "no suspend" IRQ), we may get an interrupt storm. Isn't that a problem? > Could you send it on its own to the appropriate MLs ? Sure, I can do that, but I'd like to hear from the other people on the CC first. > Thomas, Peter, Mark, could you share you opinion ? > I know I'm a bit insistent on this fix, but I'd really like to get away > from this warning backtrace (and the associated problems behind it) as > soon as possible. Yeah, me too. :-)
On Thu, 26 Feb 2015 16:44:16 +0100 "Rafael J. Wysocki" <rjw@rjwysocki.net> wrote: > On Thursday, February 26, 2015 09:03:47 AM Boris Brezillon wrote: > > Hi Rafael, > > > > On Wed, 25 Feb 2015 22:59:36 +0100 > > "Rafael J. Wysocki" <rjw@rjwysocki.net> wrote: > > > > > On Tuesday, February 24, 2015 10:55:59 AM Boris Brezillon wrote: > > > > Hello, > > > > > > > > I put the IRQF_NO_SUSPEND_SAFE/IRQF_TIMER_SIBLING_OK/WHATEVER_NAME_YOU_CHOOSE > > > > debate aside to concentrate on another problem pointed out by Rafael and > > > > Mark: the fact that we cannot mix IRQF_NO_SUSPEND and wakeup sources on > > > > a shared IRQ line. > > > > > > > > This is because the wakeup code is prevailing the IRQF_NO_SUSPEND case > > > > and will trigger a system wakeup as soon as the IRQ line is tagged as a > > > > wakeup source. > > > > > > > > This series propose an approach to deal with such cases by doing the > > > > following: > > > > 1/ Prevent any system wakeup when at least one of the IRQ user has set > > > > the IRQF_NO_SUSPEND flag > > > > 2/ Adapt IRQ handlers so that they can safely be called in suspended > > > > state > > > > 3/ Let drivers decide when the system should be woken up > > > > > > > > Let me know what you think of this approach. > > > > > > So I have the appended patch that should deal with all that too (it doesn't > > > rework drivers that need to share NO_SUSPEND IRQs and do wakeup, but that > > > can be done on top of it in a straightforward way). > > > > > > The idea is quite simple. By default, the core replaces the interrupt handlers > > > of everyone sharing NO_SUSPEND lines and not using IRQF_NO_SUSPEND with a null > > > handler always returning IRQ_NONE at the suspend_device_irqs() time (the > > > rationale being that if you don't use IRQF_NO_SUSPEND, then your device has > > > no reason to generate interrupts after that point). The original handlers are > > > then restored by resume_device_irqs(). > > > > > > However, if the IRQ is configured for wakeup, there may be a reason to generate > > > interrupts from a device not using IRQF_NO_SUSPEND. For that, the patch adds > > > IRQF_COND_SUSPEND that, if set, will prevent the default behavior described > > > above from being applied to irqactions using it if the IRQs in question are > > > configured for wakeup. Of course, the users of IRQF_COND_SUSPEND are supposed > > > to implement wakeup detection in their interrupt handlers and then call > > > pm_system_wakeup() if necessary. > > > > That patch sounds good to me. > > But it is still a bit risky. Namely, if the driver in question is sufficiently > broken (eg. it may not suspend the device and rely on the fact that its interrupt > handler will be run just because it is sharing a "no suspend" IRQ), we may get > an interrupt storm. > > Isn't that a problem? For me no (I'll fix all the drivers to handle wakeup, and they are all already masking interrupts coming from their side in the suspend callback). I can't talk for other people though. The only problem I see here is that you're not informing people that they are erroneously mixing IRQF_NO_SUSPEND and !IRQF_NO_SUSPEND anymore (you removed the warning backtrace). Moreover, you are replacing their handler by a stub when entering suspend, so they might end-up receiving spurious interrupts when suspended without knowing why ? How about checking if the number of actions registered with IRQF_NO_SUSPEND + those registered with IRQF_COND_SUSPEND (or another flag stating that the handler can safely be called in suspended state even if it didn't ask for NO_SUSPEND) are equal to the total number of registered actions, and complain if it's not the case. If some actions are offending this rule, you could keep the previous behavior by leaving its handler in place when entering suspend so that existing drivers/systems will keep working (but with a warning backtrace). > > > Could you send it on its own to the appropriate MLs ? > > Sure, I can do that, but I'd like to hear from the other people on the CC first. Sure, but that doesn't prevent sending it as an RFC, you'll still have the same review ;-).
Index: linux-pm/include/linux/interrupt.h =================================================================== --- linux-pm.orig/include/linux/interrupt.h +++ linux-pm/include/linux/interrupt.h @@ -57,6 +57,11 @@ * IRQF_NO_THREAD - Interrupt cannot be threaded * IRQF_EARLY_RESUME - Resume IRQ early during syscore instead of at device * resume time. + * IRQF_COND_SUSPEND - If the IRQ is shared with a NO_SUSPEND user and it is + * configured for system wakeup, execute this interrup handler + * after suspending interrupts as it may be necessary to detect + * wakeup. Users need to implement system wakeup detection in + * their interrupt handlers. */ #define IRQF_DISABLED 0x00000020 #define IRQF_SHARED 0x00000080 @@ -70,6 +75,7 @@ #define IRQF_FORCE_RESUME 0x00008000 #define IRQF_NO_THREAD 0x00010000 #define IRQF_EARLY_RESUME 0x00020000 +#define IRQF_COND_SUSPEND 0x00040000 #define IRQF_TIMER (__IRQF_TIMER | IRQF_NO_SUSPEND | IRQF_NO_THREAD) @@ -101,6 +107,7 @@ 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 + * @saved_handler: address of the original interrupt handler function */ struct irqaction { irq_handler_t handler; @@ -115,6 +122,9 @@ struct irqaction { unsigned long thread_mask; const char *name; struct proc_dir_entry *dir; +#ifdef CONFIG_PM_SLEEP + irq_handler_t saved_handler; +#endif } ____cacheline_internodealigned_in_smp; extern irqreturn_t no_action(int cpl, void *dev_id); Index: linux-pm/kernel/irq/pm.c =================================================================== --- linux-pm.orig/kernel/irq/pm.c +++ linux-pm/kernel/irq/pm.c @@ -43,9 +43,6 @@ void irq_pm_install_action(struct irq_de if (action->flags & IRQF_NO_SUSPEND) desc->no_suspend_depth++; - - WARN_ON_ONCE(desc->no_suspend_depth && - desc->no_suspend_depth != desc->nr_actions); } /* @@ -63,11 +60,53 @@ void irq_pm_remove_action(struct irq_des desc->no_suspend_depth--; } +static irqreturn_t irq_pm_null_handler(int irq, void *context) +{ + return IRQ_NONE; +} + +static void prepare_no_suspend_irq(struct irq_desc *desc) +{ + struct irqaction *action; + + for (action = desc->action; action; action = action->next) { + if (action->flags & IRQF_NO_SUSPEND) + continue; + + if ((action->flags & IRQF_COND_SUSPEND) && + irqd_is_wakeup_set(&desc->irq_data)) + continue; + + action->saved_handler = action->handler; + action->handler = irq_pm_null_handler; + } +} + +static void restore_no_suspend_irq(struct irq_desc *desc) +{ + struct irqaction *action; + + for (action = desc->action; action; action = action->next) { + if (action->flags & IRQF_NO_SUSPEND) + continue; + + if (action->saved_handler) { + action->handler = action->saved_handler; + action->saved_handler = NULL; + } + } +} + static bool suspend_device_irq(struct irq_desc *desc, int irq) { - if (!desc->action || desc->no_suspend_depth) + if (!desc->action) return false; + if (desc->no_suspend_depth) { + prepare_no_suspend_irq(desc); + return false; + } + if (irqd_is_wakeup_set(&desc->irq_data)) { irqd_set(&desc->irq_data, IRQD_WAKEUP_ARMED); /* @@ -135,6 +174,8 @@ static void resume_irq(struct irq_desc * if (desc->istate & IRQS_SUSPENDED) goto resume; + restore_no_suspend_irq(desc); + /* Force resume the interrupt? */ if (!desc->force_resume_depth) return;