From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754606AbZBWOss (ORCPT ); Mon, 23 Feb 2009 09:48:48 -0500 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1754188AbZBWOsh (ORCPT ); Mon, 23 Feb 2009 09:48:37 -0500 Received: from ogre.sisk.pl ([217.79.144.158]:39934 "EHLO ogre.sisk.pl" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753796AbZBWOsg (ORCPT ); Mon, 23 Feb 2009 09:48:36 -0500 From: "Rafael J. Wysocki" To: Ingo Molnar Subject: Re: [RFC][PATCH 2/2] PM: Rework handling of interrupts during suspend-resume Date: Mon, 23 Feb 2009 15:48:11 +0100 User-Agent: KMail/1.11.0 (Linux/2.6.29-rc5-tst; KDE/4.2.0; x86_64; ; ) Cc: Johannes Berg , Linus Torvalds , LKML , "Eric W. Biederman" , Benjamin Herrenschmidt , Jeremy Fitzhardinge , pm list , Len Brown , Jesse Barnes , Thomas Gleixner References: <200902221837.49396.rjw@sisk.pl> <200902231229.58743.rjw@sisk.pl> <20090223122832.GB31427@elte.hu> In-Reply-To: <20090223122832.GB31427@elte.hu> MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Content-Disposition: inline Message-Id: <200902231548.12821.rjw@sisk.pl> Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Monday 23 February 2009, Ingo Molnar wrote: > > * Rafael J. Wysocki wrote: > > > > > Index: linux-2.6/arch/x86/kernel/apm_32.c > > > > =================================================================== > > > > --- linux-2.6.orig/arch/x86/kernel/apm_32.c > > > > +++ linux-2.6/arch/x86/kernel/apm_32.c > > > > > > > + > > > > + suspend_device_irqs(); > > > > device_power_down(PMSG_SUSPEND); > > > > + > > > > + local_irq_disable(); > > > > > > hm, this is a very repetitive pattern, all around the various > > > suspend/resume variants. Might make sense to make: > > > > > > device_power_down(PMSG_SUSPEND); > > > > > > do the irq line disabling plus the local irq disabling > > > automatically. That also means it cannot be forgotten. The > > > symmetric action should happen for PMSG_RESUME. > > > > > > Is there ever a case where we want a different pattern? > > > > Even if there's no such case, I prefer to call > > local_irq_disable() explicitly in here, so that it's clearly > > known where it happens to anyone reading this code. > > That property can be implied in the function name: > > device_power_down_irq_disable(PMSG_SUSPEND); > > Open-coding it, if it looks the same in all the cases just > increases the chances that someone somewhere copies them > incorrectly. Well, I see your point, but in that case I'd rather couple the disabling of local interrupts on the CPU with sysdev_suspend and the disabling (or whatever Eric would like to call that) of device interrupts with device_power_down(). Thanks, Rafael From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Rafael J. Wysocki" Subject: Re: [RFC][PATCH 2/2] PM: Rework handling of interrupts during suspend-resume Date: Mon, 23 Feb 2009 15:48:11 +0100 Message-ID: <200902231548.12821.rjw@sisk.pl> References: <200902221837.49396.rjw@sisk.pl> <200902231229.58743.rjw@sisk.pl> <20090223122832.GB31427@elte.hu> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20090223122832.GB31427@elte.hu> Content-Disposition: inline List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: linux-pm-bounces@lists.linux-foundation.org Errors-To: linux-pm-bounces@lists.linux-foundation.org To: Ingo Molnar Cc: Jeremy Fitzhardinge , LKML , Jesse Barnes , Thomas Gleixner , "Eric W. Biederman" , Johannes Berg , Linus Torvalds , pm list List-Id: linux-pm@vger.kernel.org On Monday 23 February 2009, Ingo Molnar wrote: > > * Rafael J. Wysocki wrote: > > > > > Index: linux-2.6/arch/x86/kernel/apm_32.c > > > > =================================================================== > > > > --- linux-2.6.orig/arch/x86/kernel/apm_32.c > > > > +++ linux-2.6/arch/x86/kernel/apm_32.c > > > > > > > + > > > > + suspend_device_irqs(); > > > > device_power_down(PMSG_SUSPEND); > > > > + > > > > + local_irq_disable(); > > > > > > hm, this is a very repetitive pattern, all around the various > > > suspend/resume variants. Might make sense to make: > > > > > > device_power_down(PMSG_SUSPEND); > > > > > > do the irq line disabling plus the local irq disabling > > > automatically. That also means it cannot be forgotten. The > > > symmetric action should happen for PMSG_RESUME. > > > > > > Is there ever a case where we want a different pattern? > > > > Even if there's no such case, I prefer to call > > local_irq_disable() explicitly in here, so that it's clearly > > known where it happens to anyone reading this code. > > That property can be implied in the function name: > > device_power_down_irq_disable(PMSG_SUSPEND); > > Open-coding it, if it looks the same in all the cases just > increases the chances that someone somewhere copies them > incorrectly. Well, I see your point, but in that case I'd rather couple the disabling of local interrupts on the CPU with sysdev_suspend and the disabling (or whatever Eric would like to call that) of device interrupts with device_power_down(). Thanks, Rafael