From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758415AbZEFAiz (ORCPT ); Tue, 5 May 2009 20:38:55 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1754240AbZEFAip (ORCPT ); Tue, 5 May 2009 20:38:45 -0400 Received: from rv-out-0506.google.com ([209.85.198.227]:17849 "EHLO rv-out-0506.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754159AbZEFAio convert rfc822-to-8bit (ORCPT ); Tue, 5 May 2009 20:38:44 -0400 To: "Rafael J. Wysocki" Cc: Arve =?iso-8859-1?Q?Hj=F8nnev=E5g?= , Andrew Morton , linux-pm@lists.linux-foundation.org, linux-kernel@vger.kernel.org, Linus Torvalds , Ingo Molnar Subject: Re: [PATCH] PM: suspend_device_irqs(): don't disable wakeup IRQs References: <1241483224-7113-1-git-send-email-khilman@deeprootsystems.com> <87eiv35eg6.fsf@deeprootsystems.com> <200905060127.24777.rjw@sisk.pl> <87y6tb3x6r.fsf@deeprootsystems.com> From: Kevin Hilman Organization: Deep Root Systems, LLC Date: Tue, 05 May 2009 17:38:43 -0700 In-Reply-To: <87y6tb3x6r.fsf@deeprootsystems.com> (Kevin Hilman's message of "Tue\, 05 May 2009 17\:13\:48 -0700") Message-ID: <878wlb3w18.fsf@deeprootsystems.com> User-Agent: Gnus/5.11 (Gnus v5.11) Emacs/22.2 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Kevin Hilman writes: > "Rafael J. Wysocki" writes: > >> On Wednesday 06 May 2009, Kevin Hilman wrote: >>> Arve Hjønnevåg writes: >>> >>> > On Tue, May 5, 2009 at 8:52 AM, Kevin Hilman >>> > wrote: >>> >> Andrew Morton writes: >>> >> >>> >>> On Mon, 4 May 2009 17:27:04 -0700 Kevin Hilman wrote: >>> >>> >>> >>>> Interrupts that are flagged as wakeup sources via set_irq_wake() >>> >>>> should not be disabled for suspend. >>> >>>> >>> >>> >>> >>> Why not? >>> >>> >>> >> >>> >> If an interrupt is a wakeup source, and it is disabled at the chip >>> >> level, it will no longer generate interrupts, and thus no longer wake >>> >> up the system. >>> >> >>> >> I'd be interested in hearing why wakeup interrupts should be disabled >>> >> during suspend. >> >> That depends on whether or not they are used for anything else than wake-up. >> >>> >>> [...] >>> >>> >>> >>> >>> If this fixes some bug then please provide a description of that bug? >>> >> >>> >> The bug is that on TI OMAP, interrupts that are used for wakeup events >>> >> are disabled by this code causing the system to no longer wake up. >>> > >>> > What do you do if the interrupt triggers right after your driver has >>> > returned from its late suspend hook? >>> >>> If it's a wakeup IRQ, I assume you want it to prevent suspend. >>> >>> But I don't see how that can happen in the current code. IIUC, by the >>> time your late suspend hook is run, your device IRQ is already >>> disabled, so it won't trigger an interrupt that will be caught by >>> check_wakeup_irqs() anyways. >> >> My understanding of __disable_irq() was that it didn't actually disable the >> IRQ at the hardware level, allowing the CPU to actually receive the interrupt >> and acknowledge it, but preventing the device driver for receiving it. > > Hmm, that's not normally what I think of as disabled. ;) > >> Does it work differently on the affected systems? > > Yes. > > __disable_irq() calls the irq_chip's disable method which is platform > specific. On OMAP, this masks the IRQ at the hardware level > preventing the CPU from seeing the interrupt. So just as a test, I just removed the 'disable' hook from my platforms irq_chip and this allows me to wakeup without using my proposed patch, although I'm not sure it is the right behavior either. The 'struct irq_chip' comments are a bit misleading here as it says * @disable: disable the interrupt (defaults to chip->mask if NULL) And since my irq_chip->disable was doing basically the same thing as my irq_chip->mask, I didn't expect it to change behavior. But in kernel/irq/chip.c, disable gets set to an empty default_disable if the irq_chip's version is NULL. The result is that if irq_chip->disable == NULL, suspend_device_irqs() is a big NOP, albiet one that does lots of locking. :) So, should the irq_chip code be fixed to match the comment? Something like the patch below? If I fix the IRQ chip code, then I'm back to needing my patch since my irq_chip mask function still masks the IRQ at the hardware. Kevin commit f9b534f23ac7835eead99fb0a9cec7c505fe1e85 Author: Kevin Hilman Date: Tue May 5 17:32:59 2009 -0700 IRQ: chip->disable should default to chip->mask if NULL The struct irq_chip comments in state: * @disable: disable the interrupt (defaults to chip->mask if NULL) However, the code in kernel/irq/chip.c does otherwise by setting a NULL disable hook to an empty default_disable function. This patch makes the default_disable function call the ->mask hook to match the comments. Signed-off-by: Kevin Hilman diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c index c687ba4..0fb690a 100644 --- a/kernel/irq/chip.c +++ b/kernel/irq/chip.c @@ -238,6 +238,10 @@ static void default_enable(unsigned int irq) */ static void default_disable(unsigned int irq) { + struct irq_desc *desc = irq_to_desc(irq); + + desc->chip->mask(irq); + desc->status |= IRQ_MASKED; } /*