[PATCH][RFC] ACPI / PM: Fix incorrect wakeup irq setting before suspend-to-idle

[PATCH][RFC] ACPI / PM: Fix incorrect wakeup irq setting before suspend-to-idle

[Chen Yu]

For ACPI compatible system, SCI(ACPI System Control
Interrupt) is used to wake system up from suspend-to-idle.
Once CPU is woken up by SCI, interrupt handler will
firstly checks if current interrupt is legal to wake up
the whole system, thus irq_pm_check_wakeup is invoked
to validate the irq number. However, before suspend-to-idle,
acpi_gbl_FADT.sci_interrupt is marked rather than actual
irq number in acpi_freeze_prepare, this might lead to unable
to wake up the system.

This patch fixes this problem by marking the irq number
return by acpi_gsi_to_irq as IRQD_WAKEUP_STATE, rather than
marking the acpi_gbl_FADT.sci_interrupt.

Signed-off-by: Chen Yu <yu.c.chen@intel.com>
---
 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
@@ -49,6 +49,7 @@
 #include <asm/uaccess.h>
 
 #include "internal.h"
+#include "sleep.h"
 
 #define _COMPONENT		ACPI_OS_SERVICES
 ACPI_MODULE_NAME("osl");
@@ -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
 	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;
+
+int get_wake_irq_freeze(void)
+{
+	if (IS_ERR_VALUE(wake_irq_freeze))
+		return acpi_gbl_FADT.sci_interrupt;
+	else
+		return wake_irq_freeze;
+}
+EXPORT_SYMBOL_GPL(get_wake_irq_freeze);
+
+void set_wake_irq_freeze(unsigned int irq)
+{
+	wake_irq_freeze = (int)irq;
+}
+EXPORT_SYMBOL_GPL(set_wake_irq_freeze);
 
 static int acpi_freeze_begin(void)
 {
@@ -632,14 +648,14 @@ static int acpi_freeze_prepare(void)
 	acpi_enable_wakeup_devices(ACPI_STATE_S0);
 	acpi_enable_all_wakeup_gpes();
 	acpi_os_wait_events_complete();
-	enable_irq_wake(acpi_gbl_FADT.sci_interrupt);
+	enable_irq_wake(get_wake_irq_freeze());
 	return 0;
 }
 
 static void acpi_freeze_restore(void)
 {
 	acpi_disable_wakeup_devices(ACPI_STATE_S0);
-	disable_irq_wake(acpi_gbl_FADT.sci_interrupt);
+	disable_irq_wake(get_wake_irq_freeze());
 	acpi_enable_all_runtime_gpes();
 }
 
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
-- 
2.1.4

Re: [PATCH][RFC] ACPI / PM: Fix incorrect wakeup irq setting before suspend-to-idle

[Rafael J. Wysocki]

On Monday, August 10, 2015 10:11:26 AM Chen Yu wrote:
> For ACPI compatible system, SCI(ACPI System Control
> Interrupt) is used to wake system up from suspend-to-idle.
> Once CPU is woken up by SCI, interrupt handler will
> firstly checks if current interrupt is legal to wake up
> the whole system, thus irq_pm_check_wakeup is invoked
> to validate the irq number. However, before suspend-to-idle,
> acpi_gbl_FADT.sci_interrupt is marked rather than actual
> irq number in acpi_freeze_prepare, this might lead to unable
> to wake up the system.
> 
> This patch fixes this problem by marking the irq number
> return by acpi_gsi_to_irq as IRQD_WAKEUP_STATE, rather than
> marking the acpi_gbl_FADT.sci_interrupt.
> 
> Signed-off-by: Chen Yu <yu.c.chen@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.

> ---
>  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
> @@ -49,6 +49,7 @@
>  #include <asm/uaccess.h>
>  
>  #include "internal.h"
> +#include "sleep.h"
>  
>  #define _COMPONENT		ACPI_OS_SERVICES
>  ACPI_MODULE_NAME("osl");
> @@ -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.

>  	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.

> +
> +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;

> +}
> +EXPORT_SYMBOL_GPL(get_wake_irq_freeze);
> +
> +void set_wake_irq_freeze(unsigned int irq)
> +{
> +	wake_irq_freeze = (int)irq;
> +}
> +EXPORT_SYMBOL_GPL(set_wake_irq_freeze);
>  
>  static int acpi_freeze_begin(void)
>  {
> @@ -632,14 +648,14 @@ static int acpi_freeze_prepare(void)
>  	acpi_enable_wakeup_devices(ACPI_STATE_S0);
>  	acpi_enable_all_wakeup_gpes();
>  	acpi_os_wait_events_complete();
> -	enable_irq_wake(acpi_gbl_FADT.sci_interrupt);
> +	enable_irq_wake(get_wake_irq_freeze());
>  	return 0;
>  }
>  
>  static void acpi_freeze_restore(void)
>  {
>  	acpi_disable_wakeup_devices(ACPI_STATE_S0);
> -	disable_irq_wake(acpi_gbl_FADT.sci_interrupt);
> +	disable_irq_wake(get_wake_irq_freeze());
>  	acpi_enable_all_runtime_gpes();
>  }
>  
> 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?

Thanks,
Rafael

RE: [PATCH][RFC] ACPI / PM: Fix incorrect wakeup irq setting before suspend-to-idle

[Chen, Yu C]

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 <yu.c.chen@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

Re: [PATCH][RFC] ACPI / PM: Fix incorrect wakeup irq setting before suspend-to-idle

[Rafael J. Wysocki]

On Friday, September 25, 2015 06:42:41 AM Chen, Yu C wrote:
> 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 <yu.c.chen@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.

In fact what I said was incorrect.  You might encounter this problem if the
ACPI interrupt has been remapped and not when GPE devices are used.

> > > ---
> > >  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.

Alternatively, you can define an empty static inline stub of set_wake_irq_freeze()
for CONFIG_SUSPEND and avoid using IS_ENABLED() even.

But I'd rather define a global acpi_irq variable, store irq in it and access it
directly from acpi_freeze_prepare().  And it doesn't have to depend on CONFIG_SUSPEND
as it is just the IRQ number actually used by ACPI.

BTW, I wonder if there are other places using acpi_gbl_FADT.sci_interrupt directly
which they shouldn't 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 

No.  What I mean is that in theory there may be multiple ACPI interrupts.

But you need not care about this case because of the

	if (gsi != acpi_gbl_FADT.sci_interrupt)
		return AE_BAD_PARAMETER;

check in acpi_os_install_interrupt_handler().

Thanks,
Rafael

RE: [PATCH][RFC] ACPI / PM: Fix incorrect wakeup irq setting before suspend-to-idle

[Chen, Yu C]

Hi, Rafael,

> -----Original Message-----
> From: linux-pm-owner@vger.kernel.org [mailto:linux-pm-
> owner@vger.kernel.org] On Behalf Of Rafael J. Wysocki
> Sent: Friday, September 25, 2015 9:57 PM
> 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 Friday, September 25, 2015 06:42:41 AM Chen, Yu C wrote:
> > 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:
> > > > +#ifdef CONFIG_SUSPEND
> > > > +	set_wake_irq_freeze(irq);
> > > > +#endif
> 
> Alternatively, you can define an empty static inline stub of
> set_wake_irq_freeze() for CONFIG_SUSPEND and avoid using IS_ENABLED()
> even.
> 
> But I'd rather define a global acpi_irq variable, store irq in it and access it
> directly from acpi_freeze_prepare().  And it doesn't have to depend on
> CONFIG_SUSPEND as it is just the IRQ number actually used by ACPI.
> 
OK, I've convert it to a global variable acpi_inuse_irq.

> BTW, I wonder if there are other places using acpi_gbl_FADT.sci_interrupt
> directly which they shouldn't do?
> 
I searched the code and found there are two other potential misuse of 
acpi_gbl_FADT.sci_interrupt, they are in acpi_os_remove_interrupt_handler
and acpi_os_wait_events_complete, so I sent out a version 2 patch
for them.

> > Oh, do you mean the naming for this variable is un-suitable? OK, I'll
> > change it to acpi_freeze_wake_irq
> 
> No.  What I mean is that in theory there may be multiple ACPI interrupts.
> 
> But you need not care about this case because of the
> 
> 	if (gsi != acpi_gbl_FADT.sci_interrupt)
> 		return AE_BAD_PARAMETER;
> check in acpi_os_install_interrupt_handler().
> 
OK, thanks!

Best Regards,
Yu

Re: [PATCH][RFC] ACPI / PM: Fix incorrect wakeup irq setting before suspend-to-idle

[Rafael J. Wysocki]

On Saturday, September 26, 2015 02:37:19 PM Chen, Yu C wrote:
> Hi, Rafael,
> 
> > -----Original Message-----
> > From: linux-pm-owner@vger.kernel.org [mailto:linux-pm-
> > owner@vger.kernel.org] On Behalf Of Rafael J. Wysocki
> > Sent: Friday, September 25, 2015 9:57 PM
> > 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 Friday, September 25, 2015 06:42:41 AM Chen, Yu C wrote:
> > > 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:
> > > > > +#ifdef CONFIG_SUSPEND
> > > > > +	set_wake_irq_freeze(irq);
> > > > > +#endif
> > 
> > Alternatively, you can define an empty static inline stub of
> > set_wake_irq_freeze() for CONFIG_SUSPEND and avoid using IS_ENABLED()
> > even.
> > 
> > But I'd rather define a global acpi_irq variable, store irq in it and access it
> > directly from acpi_freeze_prepare().  And it doesn't have to depend on
> > CONFIG_SUSPEND as it is just the IRQ number actually used by ACPI.
> > 
> OK, I've convert it to a global variable acpi_inuse_irq.

Why do you need the "inuse" part?  Why is acpi_irq not sufficient?

> > BTW, I wonder if there are other places using acpi_gbl_FADT.sci_interrupt
> > directly which they shouldn't do?
> > 
> I searched the code and found there are two other potential misuse of 
> acpi_gbl_FADT.sci_interrupt, they are in acpi_os_remove_interrupt_handler
> and acpi_os_wait_events_complete, so I sent out a version 2 patch
> for them.

OK, thanks!

Rafael

RE: [PATCH][RFC] ACPI / PM: Fix incorrect wakeup irq setting before suspend-to-idle

[Chen, Yu C]

Hi, Rafael,

> -----Original Message-----
> From: linux-pm-owner@vger.kernel.org [mailto:linux-pm-
> owner@vger.kernel.org] On Behalf Of Rafael J. Wysocki
> Sent: Sunday, September 27, 2015 9:30 PM
> 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 Saturday, September 26, 2015 02:37:19 PM Chen, Yu C wrote:
> > Hi, Rafael,
> >
> > > -----Original Message-----
> > > From: linux-pm-owner@vger.kernel.org [mailto:linux-pm-
> > > owner@vger.kernel.org] On Behalf Of Rafael J. Wysocki
> > > Sent: Friday, September 25, 2015 9:57 PM
> > > 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 Friday, September 25, 2015 06:42:41 AM Chen, Yu C wrote:
> > > > 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:
> > >
> > > But I'd rather define a global acpi_irq variable, store irq in it
> > > and access it directly from acpi_freeze_prepare().  And it doesn't
> > > have to depend on CONFIG_SUSPEND as it is just the IRQ number
> actually used by ACPI.
> > >
> > OK, I've convert it to a global variable acpi_inuse_irq.
> 
> Why do you need the "inuse" part?  Why is acpi_irq not sufficient?
Because the name of acpi_irq is already used by acpi irq handler at
drivers/acpi/osl.c:
request_irq(irq, acpi_irq, IRQF_SHARED, "acpi", acpi_irq)

Thanks
Yu

Re: [PATCH][RFC] ACPI / PM: Fix incorrect wakeup irq setting before suspend-to-idle

[Rafael J. Wysocki]

On Monday, September 28, 2015 01:51:09 AM Chen, Yu C wrote:
> Hi, Rafael,
> 
> > -----Original Message-----
> > From: linux-pm-owner@vger.kernel.org [mailto:linux-pm-
> > owner@vger.kernel.org] On Behalf Of Rafael J. Wysocki
> > Sent: Sunday, September 27, 2015 9:30 PM
> > 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 Saturday, September 26, 2015 02:37:19 PM Chen, Yu C wrote:
> > > Hi, Rafael,
> > >
> > > > -----Original Message-----
> > > > From: linux-pm-owner@vger.kernel.org [mailto:linux-pm-
> > > > owner@vger.kernel.org] On Behalf Of Rafael J. Wysocki
> > > > Sent: Friday, September 25, 2015 9:57 PM
> > > > 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 Friday, September 25, 2015 06:42:41 AM Chen, Yu C wrote:
> > > > > 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:
> > > >
> > > > But I'd rather define a global acpi_irq variable, store irq in it
> > > > and access it directly from acpi_freeze_prepare().  And it doesn't
> > > > have to depend on CONFIG_SUSPEND as it is just the IRQ number
> > actually used by ACPI.
> > > >
> > > OK, I've convert it to a global variable acpi_inuse_irq.
> > 
> > Why do you need the "inuse" part?  Why is acpi_irq not sufficient?
> Because the name of acpi_irq is already used by acpi irq handler at
> drivers/acpi/osl.c:
> request_irq(irq, acpi_irq, IRQF_SHARED, "acpi", acpi_irq)

Ah, this is the name of the handler.  I forgot about that, sorry.

Well, it might be worth renaming the handler to something like acpi_interrupt(), then.

Thanks,
Rafael

RE: [PATCH][RFC] ACPI / PM: Fix incorrect wakeup irq setting before suspend-to-idle

[Chen, Yu C]

Hi,

> -----Original Message-----
> From: Rafael J. Wysocki [mailto:rjw@rjwysocki.net]
> Sent: Monday, September 28, 2015 8:56 PM
> 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, September 28, 2015 01:51:09 AM Chen, Yu C wrote:
> > Hi, Rafael,
> >
> > > -----Original Message-----
> > > From: linux-pm-owner@vger.kernel.org [mailto:linux-pm-
> > > owner@vger.kernel.org] On Behalf Of Rafael J. Wysocki
> > > Sent: Sunday, September 27, 2015 9:30 PM
> > > 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 Saturday, September 26, 2015 02:37:19 PM Chen, Yu C wrote:
> > > > Hi, Rafael,
> > > >
> > > > > -----Original Message-----
> > > > > From: linux-pm-owner@vger.kernel.org [mailto:linux-pm-
> > > > > owner@vger.kernel.org] On Behalf Of Rafael J. Wysocki
> > > > > Sent: Friday, September 25, 2015 9:57 PM
> > > > > 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 Friday, September 25, 2015 06:42:41 AM Chen, Yu C wrote:
> > > > > > 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:
> > > > >
> > Because the name of acpi_irq is already used by acpi irq handler at
> > drivers/acpi/osl.c:
> > request_irq(irq, acpi_irq, IRQF_SHARED, "acpi", acpi_irq)
> 
> Ah, this is the name of the handler.  I forgot about that, sorry.
> 
> Well, it might be worth renaming the handler to something like
> acpi_interrupt(), then.
OK, will rewrite the patch, Thanks!

Best Regards,
Yu

Re: [PATCH][RFC] ACPI / PM: Fix incorrect wakeup irq setting before suspend-to-idle

[Rafael J. Wysocki]

Hi,

On Mon, Sep 28, 2015 at 7:56 PM, Chen, Yu C <yu.c.chen@intel.com> wrote:
> Hi,
>
>> -----Original Message-----
>> From: Rafael J. Wysocki [mailto:rjw@rjwysocki.net]
>> Sent: Monday, September 28, 2015 8:56 PM
>> 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, September 28, 2015 01:51:09 AM Chen, Yu C wrote:
>> > Hi, Rafael,
>> >
>> > > -----Original Message-----
>> > > From: linux-pm-owner@vger.kernel.org [mailto:linux-pm-
>> > > owner@vger.kernel.org] On Behalf Of Rafael J. Wysocki
>> > > Sent: Sunday, September 27, 2015 9:30 PM
>> > > 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 Saturday, September 26, 2015 02:37:19 PM Chen, Yu C wrote:
>> > > > Hi, Rafael,
>> > > >
>> > > > > -----Original Message-----
>> > > > > From: linux-pm-owner@vger.kernel.org [mailto:linux-pm-
>> > > > > owner@vger.kernel.org] On Behalf Of Rafael J. Wysocki
>> > > > > Sent: Friday, September 25, 2015 9:57 PM
>> > > > > 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 Friday, September 25, 2015 06:42:41 AM Chen, Yu C wrote:
>> > > > > > 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:
>> > > > >
>> > Because the name of acpi_irq is already used by acpi irq handler at
>> > drivers/acpi/osl.c:
>> > request_irq(irq, acpi_irq, IRQF_SHARED, "acpi", acpi_irq)
>>
>> Ah, this is the name of the handler.  I forgot about that, sorry.
>>
>> Well, it might be worth renaming the handler to something like
>> acpi_interrupt(), then.
> OK, will rewrite the patch, Thanks!

Alternatively, you can call the new variable acpi_sci_irq.

Thanks,
Rafael

Re: [PATCH][RFC] ACPI / PM: Fix incorrect wakeup irq setting before suspend-to-idle

[Rafael J. Wysocki]

On Mon, Sep 28, 2015 at 10:05 PM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> Hi,
>
> On Mon, Sep 28, 2015 at 7:56 PM, Chen, Yu C <yu.c.chen@intel.com> wrote:

[cut]

>>> > Because the name of acpi_irq is already used by acpi irq handler at
>>> > drivers/acpi/osl.c:
>>> > request_irq(irq, acpi_irq, IRQF_SHARED, "acpi", acpi_irq)
>>>
>>> Ah, this is the name of the handler.  I forgot about that, sorry.
>>>
>>> Well, it might be worth renaming the handler to something like
>>> acpi_interrupt(), then.
>> OK, will rewrite the patch, Thanks!
>
> Alternatively, you can call the new variable acpi_sci_irq.

BTW, please CC ACPI-related patches to linux-acpi too.

Thanks,
Rafael