From patchwork Tue Sep 11 23:21:53 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: NeilBrown X-Patchwork-Id: 1440131 Return-Path: X-Original-To: patchwork-linux-omap@patchwork.kernel.org Delivered-To: patchwork-process-083081@patchwork1.kernel.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by patchwork1.kernel.org (Postfix) with ESMTP id 10A714025E for ; Tue, 11 Sep 2012 23:23:04 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1759054Ab2IKXWk (ORCPT ); Tue, 11 Sep 2012 19:22:40 -0400 Received: from cantor2.suse.de ([195.135.220.15]:33458 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754306Ab2IKXWI (ORCPT ); Tue, 11 Sep 2012 19:22:08 -0400 Received: from relay1.suse.de (unknown [195.135.220.254]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by mx2.suse.de (Postfix) with ESMTP id CFD9BA38EA; Wed, 12 Sep 2012 01:22:02 +0200 (CEST) Date: Wed, 12 Sep 2012 09:21:53 +1000 From: NeilBrown To: Thomas Gleixner Cc: "Rafael J. Wysocki" , "Shilimkar, Santosh" , lkml , linux-omap@vger.kernel.org, Kevin Hilman , Abhijeet Dharmapurikar Subject: Re: Seeking clarity on IRQCHIP_MASK_ON_SUSPEND Message-ID: <20120912092153.02f3a7e5@notabene.brown> In-Reply-To: References: <20120910165127.37dd07f3@notabene.brown> <20120911094252.0e78a8f9@notabene.brown> X-Mailer: Claws Mail 3.7.10 (GTK+ 2.24.7; x86_64-suse-linux-gnu) Mime-Version: 1.0 Sender: linux-omap-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-omap@vger.kernel.org On Tue, 11 Sep 2012 15:20:53 +0200 (CEST) Thomas Gleixner wrote: > On Tue, 11 Sep 2012, NeilBrown wrote: > > On Mon, 10 Sep 2012 12:28:35 +0200 (CEST) Thomas Gleixner > > wrote: > > > You might be looking for a different functionality. Can you explain > > > what you need? > > > > I want as particular GPIO interrupt to be masked before entering suspend. > > I produced code to get the ->suspend() callback to perform this masking. > > Another developer (Santosh) felt that IRQCHIP_MASK_ON_SUSPEND was the > > preferred way to do this and on the surface this looks like it should be > > correct. However it doesn't work as explained previously. > > I want a resolution to this difference of opinion that doesn't just sweep the > > issue under that carpet but provides a clear answer to this sort of issue. > > > > My current opinion is that IRQCHIP_MASK_ON_SUSPEND should be discarded. The > > patch which introduced it says: > > > > Rather than working around this in the affected interrupt chip > > implementations we can solve this elegant in the core code itself. > > > > It appears that the solution in core code, while elegant, is wrong. It > > happens too late to be generally usable. It is easy enough to handle this > > Sigh. The flag was invented with the semantics to keep the current > "check for any interrupt" pending functionality alive and then mask it > right before going down, so only the designated wakeup interrupts can > wake the device. That was the result of the discussion back then and > that was what the developer wanted to achieve with his driver suspend > hackery. > > > issue in the interrupt chip drivers so maybe that is the best place > > to handle it. > > And have the same "keep track of wakeup interrupts" code over and over > in the drivers. We could have a 'mask my non-wakeup interrupts' library call that interested drivers could call when appropriate, rather than having a flag to be set when appropriate. In the case of the gpio_omap driver, simply disabling wakeup requires less talking to hardware than a full 'irq_mask' call. So if we give the driver more information about what is happening, it can implement it more efficiently. (I generally prefer providing code to call rather than flags to set. It gives more control with introducing more complexity). > > > The the very least I think we need a big comment saying the > > IRQCHIP_MASK_ON_SUSPEND can only be used for irqchips which can always be > > programmed, even when they are suspended from an runtime-PM perspective, > > and that those chips must handle masking in their 'suspend' callback. > > Sigh, no. Either we make IRQCHIP_MASK_ON_SUSPEND into an > implementation which masks the interrupts early, if the existing users > find this acceptable or have a separate IRQCHIP_MASK_BEFORE_SUSPEND > flag. Something like this may be? Just compile-tested so far. I'll see if I can understand the two current users of IRQCHIP_MASK_ON_SUSPEND well enough to see if they should work with this flag. I suspect they will. Thanks, NeilBrown diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h index e68a8e5..d46f6c1 100644 --- a/include/linux/interrupt.h +++ b/include/linux/interrupt.h @@ -228,8 +228,12 @@ extern void suspend_device_irqs(void); extern void resume_device_irqs(void); #ifdef CONFIG_PM_SLEEP extern int check_wakeup_irqs(void); +void mask_non_wakeup_irqs(void); +void unmask_non_wakeup_irqs(void); #else static inline int check_wakeup_irqs(void) { return 0; } +static inline void mask_non_wakeup_irqs(void) { return 0; } +static inline void unmask_non_wakeup_irqs(void) { return 0; } #endif #else static inline void suspend_device_irqs(void) { }; diff --git a/include/linux/irq.h b/include/linux/irq.h index a5261e3..fbd9d7b 100644 --- a/include/linux/irq.h +++ b/include/linux/irq.h @@ -340,7 +340,10 @@ struct irq_chip { * * IRQCHIP_SET_TYPE_MASKED: Mask before calling chip.irq_set_type() * IRQCHIP_EOI_IF_HANDLED: Only issue irq_eoi() when irq was handled - * IRQCHIP_MASK_ON_SUSPEND: Mask non wake irqs in the suspend path + * IRQCHIP_MASK_ON_SUSPEND: Mask non wake irqs late in the suspend path + * IRQCHIP_MASK_PRE_SUSPEND: Mask non wake irqs early in the suspend path + * before devices are powered off or interrupts are + * disabled. * IRQCHIP_ONOFFLINE_ENABLED: Only call irq_on/off_line callbacks * when irq enabled * IRQCHIP_SKIP_SET_WAKE: Skip chip.irq_set_wake(), for this irq chip @@ -351,6 +354,7 @@ enum { IRQCHIP_MASK_ON_SUSPEND = (1 << 2), IRQCHIP_ONOFFLINE_ENABLED = (1 << 3), IRQCHIP_SKIP_SET_WAKE = (1 << 4), + IRQCHIP_MASK_PRE_SUSPEND = (1 << 5), }; /* This include will go away once we isolated irq_desc usage to core code */ diff --git a/kernel/irq/pm.c b/kernel/irq/pm.c index 3728c97..836737b 100644 --- a/kernel/irq/pm.c +++ b/kernel/irq/pm.c @@ -128,3 +128,35 @@ int check_wakeup_irqs(void) return 0; } + +/** + * mask_non_wakeup_irqs - irqs that should not wake from suspend should be + * masked now. + * This is called after devices have been suspended, but before suspend_late + * and before interrupts are disabled. This means it should still be possible + * to talk to the interrupt controller to effect the mask. + */ +void mask_non_wakeup_irqs(void) +{ + struct irq_desc *desc; + int irq; + + for_each_irq_desc(irq, desc) + if ((irq_desc_get_chip(desc)->flags & IRQCHIP_MASK_PRE_SUSPEND) + && !irqd_is_wakeup_set(&desc->irq_data) + && !irqd_irq_masked(&desc->irq_data)) + mask_irq(desc); +} + +void unmask_non_wakeup_irqs(void) +{ + struct irq_desc *desc; + int irq; + + for_each_irq_desc(irq, desc) + if ((irq_desc_get_chip(desc)->flags & IRQCHIP_MASK_PRE_SUSPEND) + && !irqd_is_wakeup_set(&desc->irq_data) + && irqd_irq_masked(&desc->irq_data) + && !irqd_irq_disabled(&desc->irq_data)) + unmask_irq(desc); +} diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c index 16a0f77..05054a4 100644 --- a/kernel/power/suspend.c +++ b/kernel/power/suspend.c @@ -23,6 +23,7 @@ #include #include #include +#include #include #include @@ -222,11 +223,12 @@ int suspend_devices_and_enter(suspend_state_t state) if (suspend_test(TEST_DEVICES)) goto Recover_platform; + mask_non_wakeup_irqs(); do { error = suspend_enter(state, &wakeup); } while (!error && !wakeup && suspend_ops->suspend_again && suspend_ops->suspend_again()); - + unmask_non_wakeup_irqs(); Resume_devices: suspend_test_start(); dpm_resume_end(PMSG_RESUME);