| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20150226090347.777c47ec@bbrezillon>
Date: Thu, 26 Feb 2015 09:03:47 +0100
From: Boris Brezillon <boris.brezillon@...e-electrons.com>
To: "Rafael J. Wysocki" <rjw@...ysocki.net>
Cc: Thomas Gleixner <tglx@...utronix.de>,
Jason Cooper <jason@...edaemon.net>,
Peter Zijlstra <peterz@...radead.org>,
Mark Rutland <mark.rutland@....com>,
linux-kernel@...r.kernel.org,
Nicolas Ferre <nicolas.ferre@...el.com>,
Jean-Christophe Plagniol-Villard <plagnioj@...osoft.com>,
Alexandre Belloni <alexandre.belloni@...e-electrons.com>,
linux-arm-kernel@...ts.infradead.org
Subject: Re: [RFC PATCH 0/3] genirq: mixing IRQF_NO_SUSPEND and wakeup
sources on shared IRQs
Hi Rafael,
On Wed, 25 Feb 2015 22:59:36 +0100
"Rafael J. Wysocki" <rjw@...ysocki.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;
>
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists