From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754603AbbIYGmx (ORCPT ); Fri, 25 Sep 2015 02:42:53 -0400 Received: from mga03.intel.com ([134.134.136.65]:27152 "EHLO mga03.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752305AbbIYGmt (ORCPT ); Fri, 25 Sep 2015 02:42:49 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.17,585,1437462000"; d="scan'208";a="652000017" From: "Chen, Yu C" To: "Rafael J. Wysocki" CC: "Wysocki, Rafael J" , "jiang.liu@linux.intel.com" , "Zhang, Rui" , "Brown, Len" , "linux-kernel@vger.kernel.org" , "linux-pm@vger.kernel.org" Subject: RE: [PATCH][RFC] ACPI / PM: Fix incorrect wakeup irq setting before suspend-to-idle Thread-Topic: [PATCH][RFC] ACPI / PM: Fix incorrect wakeup irq setting before suspend-to-idle Thread-Index: AQHQ0xFmaJpZnOghfE+M3UXyZPqqKJ5MNWgAgADQUsA= Date: Fri, 25 Sep 2015 06:42:41 +0000 Message-ID: <36DF59CE26D8EE47B0655C516E9CE64026586EF4@SHSMSX101.ccr.corp.intel.com> References: <1439172686-16360-1-git-send-email-yu.c.chen@intel.com> <1921486.iFXFBYijSj@vostro.rjw.lan> In-Reply-To: <1921486.iFXFBYijSj@vostro.rjw.lan> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from base64 to 8bit by mail.home.local id t8P6gvW5028008 Hi,Rafael, thanks a lot for your review, will resend v2 version. > -----Original Message----- > From: Rafael J. Wysocki [mailto:rjw@rjwysocki.net] > Sent: Friday, September 25, 2015 9:24 AM > To: Chen, Yu C > Cc: Wysocki, Rafael J; jiang.liu@linux.intel.com; Zhang, Rui; Brown, Len; linux- > kernel@vger.kernel.org; linux-pm@vger.kernel.org > Subject: Re: [PATCH][RFC] ACPI / PM: Fix incorrect wakeup irq setting before > suspend-to-idle > > On Monday, August 10, 2015 10:11:26 AM Chen Yu wrote: > > Signed-off-by: Chen Yu > > That would only really matter if GPE devices were used, but I've never seen a > system using them in practice, so this is more of a theoretical issue. > I haven't encountered this problem, just find this suspicious when I was doing some other debugging. > > --- > > drivers/acpi/osl.c | 5 ++++- > > drivers/acpi/sleep.c | 20 ++++++++++++++++++-- drivers/acpi/sleep.h > > | 5 +++++ > > 3 files changed, 27 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c index > > 3b8963f..8e1420a 100644 > > --- a/drivers/acpi/osl.c > > +++ b/drivers/acpi/osl.c > > @@ -850,7 +851,9 @@ acpi_os_install_interrupt_handler(u32 gsi, > acpi_osd_handler handler, > > gsi); > > return AE_OK; > > } > > - > > +#ifdef CONFIG_SUSPEND > > + set_wake_irq_freeze(irq); > > +#endif > > Please don't use #ifdefs in function bodies. You can use IS_ENABLED() for > that. > OK, will do. > > acpi_irq_handler = handler; > > acpi_irq_context = context; > > if (request_irq(irq, acpi_irq, IRQF_SHARED, "acpi", acpi_irq)) { > > diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c index > > 2f0d4db..9e7b54e 100644 > > --- a/drivers/acpi/sleep.c > > +++ b/drivers/acpi/sleep.c > > @@ -620,6 +620,22 @@ static const struct platform_suspend_ops > acpi_suspend_ops_old = { > > .end = acpi_pm_end, > > .recover = acpi_pm_finish, > > }; > > +static int wake_irq_freeze = -EINVAL; > > There may be more than one of these in theory. > Oh, do you mean the naming for this variable is un-suitable? OK, I'll change it to acpi_freeze_wake_irq > > + > > +int get_wake_irq_freeze(void) > > +{ > > + if (IS_ERR_VALUE(wake_irq_freeze)) > > + return acpi_gbl_FADT.sci_interrupt; > > + else > > + return wake_irq_freeze; > > That would look better this way IMO: > > return IS_ERR_VALUE(wake_irq_freeze) ? > acpi_gbl_FADT.sci_interrupt : wake_irq_freeze; > OK, will do. > > diff --git a/drivers/acpi/sleep.h b/drivers/acpi/sleep.h index > > c797ffa..eca4fda 100644 > > --- a/drivers/acpi/sleep.h > > +++ b/drivers/acpi/sleep.h > > @@ -6,3 +6,8 @@ extern struct list_head acpi_wakeup_device_list; > > extern struct mutex acpi_device_lock; > > > > extern void acpi_resume_power_resources(void); > > + > > +#ifdef CONFIG_SUSPEND > > +extern int get_wake_irq_freeze(void); extern void > > +set_wake_irq_freeze(unsigned int irq); #endif > > Is the #ifdef needed here at all? > Will delete the #ifdef Best Regards, Yu {.n++%ݶw{.n+{G{ayʇڙ,jfhz_(階ݢj"mG?&~iOzv^m ?I