From patchwork Wed Feb 25 21:59:36 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: 5883651 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 F2FEC9F37F for ; Wed, 25 Feb 2015 21:38:57 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id DD7DE20381 for ; Wed, 25 Feb 2015 21:38:56 +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 D2F962037F for ; Wed, 25 Feb 2015 21:38:55 +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 1YQjdN-0002UH-4X; Wed, 25 Feb 2015 21:36:45 +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 1YQjdE-0002Jh-Pk for linux-arm-kernel@lists.infradead.org; Wed, 25 Feb 2015 21:36:38 +0000 Received: from aatv252.neoplus.adsl.tpnet.pl (83.6.3.252) (HELO vostro.rjw.lan) by serwer1319399.home.pl (79.96.170.134) with SMTP (IdeaSmtpServer v0.80) id a2c9ef565a95402f; Wed, 25 Feb 2015 22:36:09 +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: Wed, 25 Feb 2015 22:59:36 +0100 Message-ID: <2154999.DTKV1bGBcx@vostro.rjw.lan> User-Agent: KMail/4.11.5 (Linux/3.19.0+; KDE/4.11.5; x86_64; ; ) In-Reply-To: <1424771762-16343-1-git-send-email-boris.brezillon@free-electrons.com> References: <1424771762-16343-1-git-send-email-boris.brezillon@free-electrons.com> MIME-Version: 1.0 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20150225_133637_130961_BFF0EB31 X-CRM114-Status: GOOD ( 26.89 ) 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 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. So your patch [3/3] could be redone on top of this AFAICS. 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;