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

[PATCH] [v2] 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. Meanwhile this patch
fixes the same problems inside acpi_os_remove_interrupt_handler
and acpi_os_wait_events_complete respectively.

Signed-off-by: Chen Yu <yu.c.chen@intel.com>
---
v2:
 - 1.Define a global acpi_inuse_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.
   2.Also fix the same problems inside acpi_os_remove_interrupt_handler,
   acpi_os_wait_events_complete.
---
 drivers/acpi/osl.c   |  9 ++++++++-
 drivers/acpi/sleep.c | 10 ++++++++--
 include/linux/acpi.h |  3 +++
 3 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
index 739a4a6..97507fc 100644
--- a/drivers/acpi/osl.c
+++ b/drivers/acpi/osl.c
@@ -81,6 +81,7 @@ static struct workqueue_struct *kacpid_wq;
 static struct workqueue_struct *kacpi_notify_wq;
 static struct workqueue_struct *kacpi_hotplug_wq;
 static bool acpi_os_initialized;
+unsigned int acpi_inuse_irq = INVALID_ACPI_IRQ;
 
 /*
  * This list of permanent mappings is for memory that may be accessed from
@@ -856,6 +857,7 @@ acpi_os_install_interrupt_handler(u32 gsi, acpi_osd_handler handler,
 		acpi_irq_handler = NULL;
 		return AE_NOT_ACQUIRED;
 	}
+	acpi_inuse_irq = irq;
 
 	return AE_OK;
 }
@@ -865,6 +867,9 @@ acpi_status acpi_os_remove_interrupt_handler(u32 irq, acpi_osd_handler handler)
 	if (irq != acpi_gbl_FADT.sci_interrupt)
 		return AE_BAD_PARAMETER;
 
+	if (!IS_INVALID_ACPI_IRQ(acpi_inuse_irq))
+		irq = acpi_inuse_irq;
+
 	free_irq(irq, acpi_irq);
 	acpi_irq_handler = NULL;
 
@@ -1176,12 +1181,14 @@ EXPORT_SYMBOL(acpi_os_execute);
 
 void acpi_os_wait_events_complete(void)
 {
+	unsigned int irq = IS_INVALID_ACPI_IRQ(acpi_inuse_irq) ?
+		acpi_gbl_FADT.sci_interrupt : acpi_inuse_irq;
 	/*
 	 * Make sure the GPE handler or the fixed event handler is not used
 	 * on another CPU after removal.
 	 */
 	if (acpi_irq_handler)
-		synchronize_hardirq(acpi_gbl_FADT.sci_interrupt);
+		synchronize_hardirq(irq);
 	flush_workqueue(kacpid_wq);
 	flush_workqueue(kacpi_notify_wq);
 }
diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
index 2f0d4db..a7a467b 100644
--- a/drivers/acpi/sleep.c
+++ b/drivers/acpi/sleep.c
@@ -629,17 +629,23 @@ static int acpi_freeze_begin(void)
 
 static int acpi_freeze_prepare(void)
 {
+	unsigned int irq = IS_INVALID_ACPI_IRQ(acpi_inuse_irq) ?
+		acpi_gbl_FADT.sci_interrupt : acpi_inuse_irq;
+
 	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(irq);
 	return 0;
 }
 
 static void acpi_freeze_restore(void)
 {
+	unsigned int irq = IS_INVALID_ACPI_IRQ(acpi_inuse_irq) ?
+		acpi_gbl_FADT.sci_interrupt : acpi_inuse_irq;
+
 	acpi_disable_wakeup_devices(ACPI_STATE_S0);
-	disable_irq_wake(acpi_gbl_FADT.sci_interrupt);
+	disable_irq_wake(irq);
 	acpi_enable_all_runtime_gpes();
 }
 
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 7235c48..df8ed19 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -193,6 +193,9 @@ int acpi_ioapic_registered(acpi_handle handle, u32 gsi_base);
 void acpi_irq_stats_init(void);
 extern u32 acpi_irq_handled;
 extern u32 acpi_irq_not_handled;
+extern unsigned int acpi_inuse_irq;
+#define INVALID_ACPI_IRQ  ((unsigned)-1)
+#define IS_INVALID_ACPI_IRQ(x) unlikely((x) == INVALID_ACPI_IRQ)
 
 extern int sbf_port;
 extern unsigned long acpi_realmode_flags;
-- 
1.8.4.2

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

[Rafael J. Wysocki]

On Sunday, September 27, 2015 09:23:10 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. Meanwhile this patch
> fixes the same problems inside acpi_os_remove_interrupt_handler
> and acpi_os_wait_events_complete respectively.
> 
> Signed-off-by: Chen Yu <yu.c.chen@intel.com>
> ---
> v2:
>  - 1.Define a global acpi_inuse_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.
>    2.Also fix the same problems inside acpi_os_remove_interrupt_handler,
>    acpi_os_wait_events_complete.
> ---
>  drivers/acpi/osl.c   |  9 ++++++++-
>  drivers/acpi/sleep.c | 10 ++++++++--
>  include/linux/acpi.h |  3 +++
>  3 files changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
> index 739a4a6..97507fc 100644
> --- a/drivers/acpi/osl.c
> +++ b/drivers/acpi/osl.c
> @@ -81,6 +81,7 @@ static struct workqueue_struct *kacpid_wq;
>  static struct workqueue_struct *kacpi_notify_wq;
>  static struct workqueue_struct *kacpi_hotplug_wq;
>  static bool acpi_os_initialized;
> +unsigned int acpi_inuse_irq = INVALID_ACPI_IRQ;

What about calling it acpi_sci_irq instead?

>  
>  /*
>   * This list of permanent mappings is for memory that may be accessed from
> @@ -856,6 +857,7 @@ acpi_os_install_interrupt_handler(u32 gsi, acpi_osd_handler handler,
>  		acpi_irq_handler = NULL;
>  		return AE_NOT_ACQUIRED;
>  	}
> +	acpi_inuse_irq = irq;
>  
>  	return AE_OK;
>  }
> @@ -865,6 +867,9 @@ acpi_status acpi_os_remove_interrupt_handler(u32 irq, acpi_osd_handler handler)
>  	if (irq != acpi_gbl_FADT.sci_interrupt)
>  		return AE_BAD_PARAMETER;
>  
> +	if (!IS_INVALID_ACPI_IRQ(acpi_inuse_irq))
> +		irq = acpi_inuse_irq;

I'd think that we should return from here if acpi_inuse_irq is invalid?

It surely can't be invalid if acpi_os_install_interrupt_handler() has succeeded,
right?

> +
>  	free_irq(irq, acpi_irq);
>  	acpi_irq_handler = NULL;
>  
> @@ -1176,12 +1181,14 @@ EXPORT_SYMBOL(acpi_os_execute);
>  
>  void acpi_os_wait_events_complete(void)
>  {
> +	unsigned int irq = IS_INVALID_ACPI_IRQ(acpi_inuse_irq) ?
> +		acpi_gbl_FADT.sci_interrupt : acpi_inuse_irq;

That, again.  If acpi_inuse_irq is invalid, acpi_os_install_interrupt_handler()
has failed, so we have nothing to synchronize here, or am I missing anything?

>  	/*
>  	 * Make sure the GPE handler or the fixed event handler is not used
>  	 * on another CPU after removal.
>  	 */
>  	if (acpi_irq_handler)
> -		synchronize_hardirq(acpi_gbl_FADT.sci_interrupt);
> +		synchronize_hardirq(irq);
>  	flush_workqueue(kacpid_wq);
>  	flush_workqueue(kacpi_notify_wq);
>  }
> diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
> index 2f0d4db..a7a467b 100644
> --- a/drivers/acpi/sleep.c
> +++ b/drivers/acpi/sleep.c
> @@ -629,17 +629,23 @@ static int acpi_freeze_begin(void)
>  
>  static int acpi_freeze_prepare(void)
>  {
> +	unsigned int irq = IS_INVALID_ACPI_IRQ(acpi_inuse_irq) ?
> +		acpi_gbl_FADT.sci_interrupt : acpi_inuse_irq;

Same comment as above.

Plus, you seem to be duplicating code from acpi_os_wait_events_complete()
here, so what about putting it into a separate routine (maybe static inline)?

> +
>  	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(irq);
>  	return 0;
>  }
>  
>  static void acpi_freeze_restore(void)
>  {
> +	unsigned int irq = IS_INVALID_ACPI_IRQ(acpi_inuse_irq) ?
> +		acpi_gbl_FADT.sci_interrupt : acpi_inuse_irq;
> +

Ditto.

>  	acpi_disable_wakeup_devices(ACPI_STATE_S0);
> -	disable_irq_wake(acpi_gbl_FADT.sci_interrupt);
> +	disable_irq_wake(irq);
>  	acpi_enable_all_runtime_gpes();
>  }
>  
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index 7235c48..df8ed19 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -193,6 +193,9 @@ int acpi_ioapic_registered(acpi_handle handle, u32 gsi_base);
>  void acpi_irq_stats_init(void);
>  extern u32 acpi_irq_handled;
>  extern u32 acpi_irq_not_handled;
> +extern unsigned int acpi_inuse_irq;
> +#define INVALID_ACPI_IRQ  ((unsigned)-1)
> +#define IS_INVALID_ACPI_IRQ(x) unlikely((x) == INVALID_ACPI_IRQ)
>  
>  extern int sbf_port;
>  extern unsigned long acpi_realmode_flags;

Thanks,
Rafael

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

[Chen, Yu C]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 3564 bytes --]

Hi, Rafael
Sorry for my late response, just came back from home:)

> -----Original Message-----
> From: linux-pm-owner@vger.kernel.org [mailto:linux-pm-
> owner@vger.kernel.org] On Behalf Of Rafael J. Wysocki
> Sent: Thursday, October 01, 2015 9:11 AM
> To: Chen, Yu C
> Cc: lenb@kernel.org; Zhang, Rui; jiang.liu@linux.intel.com; linux-
> pm@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] [v2] ACPI / PM: Fix incorrect wakeup irq setting before
> suspend-to-idle
> 
> On Sunday, September 27, 2015 09:23:10 AM Chen Yu wrote:
> > diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c index
> > 739a4a6..97507fc 100644
> > --- a/drivers/acpi/osl.c
> > +++ b/drivers/acpi/osl.c
> > @@ -81,6 +81,7 @@ static struct workqueue_struct *kacpid_wq;  static
> > struct workqueue_struct *kacpi_notify_wq;  static struct
> > workqueue_struct *kacpi_hotplug_wq;  static bool acpi_os_initialized;
> > +unsigned int acpi_inuse_irq = INVALID_ACPI_IRQ;
> 
> What about calling it acpi_sci_irq instead?
> 
OK. 
> > @@ -865,6 +867,9 @@ acpi_status
> acpi_os_remove_interrupt_handler(u32 irq, acpi_osd_handler handler)
> >  	if (irq != acpi_gbl_FADT.sci_interrupt)
> >  		return AE_BAD_PARAMETER;
> >
> > +	if (!IS_INVALID_ACPI_IRQ(acpi_inuse_irq))
> > +		irq = acpi_inuse_irq;
> 
> I'd think that we should return from here if acpi_inuse_irq is invalid?
> 
> It surely can't be invalid if acpi_os_install_interrupt_handler() has succeeded,
> right?
> 
Yes, there is no need to remove the handler if it is not registered, will rewite it.

> > +
> >  	free_irq(irq, acpi_irq);
> >  	acpi_irq_handler = NULL;
> >
> > @@ -1176,12 +1181,14 @@ EXPORT_SYMBOL(acpi_os_execute);
> >
> >  void acpi_os_wait_events_complete(void)
> >  {
> > +	unsigned int irq = IS_INVALID_ACPI_IRQ(acpi_inuse_irq) ?
> > +		acpi_gbl_FADT.sci_interrupt : acpi_inuse_irq;
> 
> That, again.  If acpi_inuse_irq is invalid, acpi_os_install_interrupt_handler()
> has failed, so we have nothing to synchronize here, or am I missing anything?
> 
Right, will optimize this.
> >  static int acpi_freeze_prepare(void)
> >  {
> > +	unsigned int irq = IS_INVALID_ACPI_IRQ(acpi_inuse_irq) ?
> > +		acpi_gbl_FADT.sci_interrupt : acpi_inuse_irq;
> 
> Same comment as above.
> 
> Plus, you seem to be duplicating code from acpi_os_wait_events_complete()
> here, so what about putting it into a separate routine (maybe static inline)?
> 

I've sent out another version 3 patch, which simplified this logic that, 
the code flow will firstly  check if the acpi irq is registered,
If not, we simply bypass the code. So the duplicating code would become
one line:
" if (!IS_INVALID_ACPI_IRQ(acpi_sci_irq))" 

v3:
 - 1.Rename acpi_inuse_irq to acpi_sci_irq for better understanding.

   2.If the irq handler is not registered, skip the synchronize_hardirq
     in acpi_os_wait_events_complete and return immediately in
     acpi_os_remove_interrupt_handler.

   3.For acpi_freeze_prepare and acpi_freeze_restore, if the acpi irq
     handler is not properly registered, we do not leverage acpi irq
     to wake up the system, but expect other peripherals(such as PCI devices
     and USB devices)to invoke enable_irq_wake for us.

> Thanks,
> Rafael
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in the
> body of a message to majordomo@vger.kernel.org More majordomo info at
> http://vger.kernel.org/majordomo-info.html
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥