Hi,Rafael, thanks a lot for your review, will resend v2 version. > -----Original Message----- > From: Rafael J. Wysocki [mailto:r...@rjwysocki.net] > Sent: Friday, September 25, 2015 9:24 AM > To: Chen, Yu C > Cc: Wysocki, Rafael J; jiang....@linux.intel.com; Zhang, Rui; Brown, Len; > linux- > ker...@vger.kernel.org; linux...@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 <yu.c.c...@intel.com> > > 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
