From patchwork Thu Feb 26 18:17:03 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: "Rafael J. Wysocki" X-Patchwork-Id: 5895241 Return-Path: X-Original-To: patchwork-linux-arm@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork1.web.kernel.org (Postfix) with ESMTP id F04B99F399 for ; Thu, 26 Feb 2015 17:56:25 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id D52C9203A4 for ; Thu, 26 Feb 2015 17:56:24 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.9]) (using TLSv1.2 with cipher DHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id C0A73202F2 for ; Thu, 26 Feb 2015 17:56:23 +0000 (UTC) Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.80.1 #2 (Red Hat Linux)) id 1YR2dU-0001VQ-Ql; Thu, 26 Feb 2015 17:54:08 +0000 Received: from v094114.home.net.pl ([79.96.170.134]) by bombadil.infradead.org with smtp (Exim 4.80.1 #2 (Red Hat Linux)) id 1YR2dJ-0001MG-EE for linux-arm-kernel@lists.infradead.org; Thu, 26 Feb 2015 17:53:59 +0000 Received: from afcl80.neoplus.adsl.tpnet.pl (95.49.63.80) (HELO vostro.rjw.lan) by serwer1319399.home.pl (79.96.170.134) with SMTP (IdeaSmtpServer v0.80) id ac7b3e236d0e3cd6; Thu, 26 Feb 2015 18:53:35 +0100 From: "Rafael J. Wysocki" To: Boris Brezillon Subject: Re: [RFC PATCH 0/3] genirq: mixing IRQF_NO_SUSPEND and wakeup sources on shared IRQs Date: Thu, 26 Feb 2015 19:17:03 +0100 Message-ID: <8151717.nkhnGBri9h@vostro.rjw.lan> User-Agent: KMail/4.11.5 (Linux/3.19.0+; KDE/4.11.5; x86_64; ; ) In-Reply-To: <20150226164724.4f3c5921@bbrezillon> References: <1424771762-16343-1-git-send-email-boris.brezillon@free-electrons.com> <20964379.cuQSlojCVH@vostro.rjw.lan> <20150226164724.4f3c5921@bbrezillon> MIME-Version: 1.0 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20150226_095357_700617_3377E745 X-CRM114-Status: GOOD ( 38.99 ) X-Spam-Score: -0.0 (/) Cc: Mark Rutland , Jason Cooper , Peter Zijlstra , Nicolas Ferre , linux-kernel@vger.kernel.org, Alexandre Belloni , Thomas Gleixner , Jean-Christophe Plagniol-Villard , linux-arm-kernel@lists.infradead.org X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org X-Spam-Status: No, score=-4.2 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_MED, T_RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On Thursday, February 26, 2015 04:47:24 PM Boris Brezillon wrote: > On Thu, 26 Feb 2015 16:44:16 +0100 > "Rafael J. Wysocki" 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" 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. The same idea I had while talking to Peter over IRC. So the patch below implements that. > 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). Right. --- include/linux/interrupt.h | 5 +++++ include/linux/irqdesc.h | 1 + kernel/irq/manage.c | 7 ++++++- kernel/irq/pm.c | 7 ++++++- 4 files changed, 18 insertions(+), 2 deletions(-) Index: linux-pm/include/linux/interrupt.h =================================================================== --- linux-pm.orig/include/linux/interrupt.h +++ linux-pm/include/linux/interrupt.h @@ -57,6 +57,10 @@ * 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, execute this + * interrupt handler after suspending interrupts. For system + * wakeup devices users need to implement wakeup detection in + * their interrupt handlers. */ #define IRQF_DISABLED 0x00000020 #define IRQF_SHARED 0x00000080 @@ -70,6 +74,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) Index: linux-pm/include/linux/irqdesc.h =================================================================== --- linux-pm.orig/include/linux/irqdesc.h +++ linux-pm/include/linux/irqdesc.h @@ -78,6 +78,7 @@ struct irq_desc { #ifdef CONFIG_PM_SLEEP unsigned int nr_actions; unsigned int no_suspend_depth; + unsigned int cond_suspend_depth; unsigned int force_resume_depth; #endif #ifdef CONFIG_PROC_FS Index: linux-pm/kernel/irq/pm.c =================================================================== --- linux-pm.orig/kernel/irq/pm.c +++ linux-pm/kernel/irq/pm.c @@ -43,9 +43,12 @@ void irq_pm_install_action(struct irq_de if (action->flags & IRQF_NO_SUSPEND) desc->no_suspend_depth++; + else if (action->flags & IRQF_COND_SUSPEND) + desc->cond_suspend_depth++; WARN_ON_ONCE(desc->no_suspend_depth && - desc->no_suspend_depth != desc->nr_actions); + (desc->no_suspend_depth + + desc->cond_suspend_depth) != desc->nr_actions); } /* @@ -61,6 +64,8 @@ void irq_pm_remove_action(struct irq_des if (action->flags & IRQF_NO_SUSPEND) desc->no_suspend_depth--; + else if (action->flags & IRQF_COND_SUSPEND) + desc->cond_suspend_depth--; } static bool suspend_device_irq(struct irq_desc *desc, int irq) Index: linux-pm/kernel/irq/manage.c =================================================================== --- linux-pm.orig/kernel/irq/manage.c +++ linux-pm/kernel/irq/manage.c @@ -1474,8 +1474,13 @@ int request_threaded_irq(unsigned int ir * otherwise we'll have trouble later trying to figure out * which interrupt is which (messes up the interrupt freeing * logic etc). + * + * Also IRQF_COND_SUSPEND only makes sense for shared interrupts and + * it is mutually exclusive with IRQF_NO_SUSPEND. */ - if ((irqflags & IRQF_SHARED) && !dev_id) + if (((irqflags & IRQF_SHARED) && !dev_id) || + (!(irqflags & IRQF_SHARED) && (irqflags & IRQF_COND_SUSPEND)) || + ((irqflags & IRQF_NO_SUSPEND) && (irqflags & IRQF_COND_SUSPEND))) return -EINVAL; desc = irq_to_desc(irq);