Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1669992 > unrolled thread

[PATCH v2 3/3] ACPI / sleep: EC-based wakeup from suspend-to-idle on recent Dell systems

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v2 3/3] ACPI / sleep: EC-based wakeup from suspend-to-idle on recent Dell systems "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2017-06-20 00:10 +0200
    RE: [PATCH v2 3/3] ACPI / sleep: EC-based wakeup from  suspend-to-idle on recent Dell systems "Zheng, Lv" <lv.zheng@intel.com> - 2017-06-20 01:40 +0200
      Re: [PATCH v2 3/3] ACPI / sleep: EC-based wakeup from suspend-to-idle  on recent Dell systems "Rafael J. Wysocki" <rafael@kernel.org> - 2017-06-20 01:50 +0200
        RE: [PATCH v2 3/3] ACPI / sleep: EC-based wakeup from  suspend-to-idle on recent Dell systems "Zheng, Lv" <lv.zheng@intel.com> - 2017-06-21 03:20 +0200
    Re: [PATCH v2 3/3] ACPI / sleep: EC-based wakeup from suspend-to-idle  on recent Dell systems Linus Torvalds <torvalds@linux-foundation.org> - 2017-06-20 02:10 +0200
      Re: [PATCH v2 3/3] ACPI / sleep: EC-based wakeup from suspend-to-idle  on recent Dell systems "Rafael J. Wysocki" <rafael@kernel.org> - 2017-06-20 03:20 +0200
        Re: [PATCH v2 3/3] ACPI / sleep: EC-based wakeup from suspend-to-idle  on recent Dell systems Linus Torvalds <torvalds@linux-foundation.org> - 2017-06-20 04:10 +0200
          Re: [PATCH v2 3/3] ACPI / sleep: EC-based wakeup from suspend-to-idle  on recent Dell systems "Rafael J. Wysocki" <rafael@kernel.org> - 2017-06-20 23:20 +0200
        RE: [PATCH v2 3/3] ACPI / sleep: EC-based wakeup from  suspend-to-idle on recent Dell systems "Zheng, Lv" <lv.zheng@intel.com> - 2017-06-21 03:20 +0200
    [PATCH] ACPI / sleep: EC-based wakeup from suspend-to-idle on recent systems "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2017-06-23 02:10 +0200
      Re: [PATCH] ACPI / sleep: EC-based wakeup from suspend-to-idle on  recent systems Linus Torvalds <torvalds@linux-foundation.org> - 2017-06-23 04:50 +0200
        Re: [PATCH] ACPI / sleep: EC-based wakeup from suspend-to-idle on  recent systems Tom Lanyon <tom@oneshoeco.com> - 2017-06-27 08:00 +0200
          Re: [PATCH] ACPI / sleep: EC-based wakeup from suspend-to-idle on  recent systems Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-06-27 08:50 +0200
            Re: [PATCH] ACPI / sleep: EC-based wakeup from suspend-to-idle on  recent systems Tom Lanyon <tom@oneshoeco.com> - 2017-06-27 13:00 +0200
          RE: [PATCH] ACPI / sleep: EC-based wakeup from suspend-to-idle on  recent systems <Mario.Limonciello@dell.com> - 2017-06-27 13:20 +0200
          Re: [PATCH] ACPI / sleep: EC-based wakeup from suspend-to-idle on recent systems "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2017-06-27 17:30 +0200
            Re: [PATCH] ACPI / sleep: EC-based wakeup from suspend-to-idle on  recent systems Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> - 2017-06-27 18:20 +0200
      RE: [PATCH] ACPI / sleep: EC-based wakeup from suspend-to-idle on  recent systems "Zheng, Lv" <lv.zheng@intel.com> - 2017-06-23 08:40 +0200
        Re: [PATCH] ACPI / sleep: EC-based wakeup from suspend-to-idle on recent systems "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2017-06-23 14:30 +0200
          Re: [PATCH] ACPI / sleep: EC-based wakeup from suspend-to-idle on recent systems "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2017-06-23 14:40 +0200
      [PATCH v2] ACPI / sleep: EC-based wakeup from suspend-to-idle on recent systems "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2017-06-23 15:30 +0200
        RE: [PATCH v2] ACPI / sleep: EC-based wakeup from suspend-to-idle on  recent systems <Mario.Limonciello@dell.com> - 2017-06-23 17:40 +0200
          Re: [PATCH v2] ACPI / sleep: EC-based wakeup from suspend-to-idle  on recent systems Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> - 2017-06-23 18:10 +0200
            RE: [PATCH v2] ACPI / sleep: EC-based wakeup from suspend-to-idle on  recent systems <Mario.Limonciello@dell.com> - 2017-06-23 20:10 +0200
          Re: [PATCH v2] ACPI / sleep: EC-based wakeup from suspend-to-idle on recent systems "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2017-06-24 03:00 +0200

Page 1 of 2  [1] 2  Next page →

#1669992 — [PATCH v2 3/3] ACPI / sleep: EC-based wakeup from suspend-to-idle on recent Dell systems

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Some recent Dell laptops, including the XPS13 model numbers 9360 and
9365, cannot be woken up from suspend-to-idle by pressing the power
button which is unexpected and makes that feature less usable on
those systems.  Moreover, on the 9365 ACPI S3 (suspend-to-RAM) is
not expected to be used at all (the OS these systems ship with never
exercises the ACPI S3 path) and suspend-to-idle is the only viable
system suspend mechanism in there.

The reason why the power button wakeup from suspend-to-idle doesn't
work on those systems is because their power button events are
signaled by the EC (Embedded Controller), whose GPE (General Purpose
Event) line is disabled during suspend-to-idle transitions in Linux.
That is done on purpose, because in general the EC tends to generate
tons of events for various reasons (battery and thermal updates and
similar, for example) and all of them would kick the CPUs out of deep
idle states while in suspend-to-idle, which effectively would defeat
its purpose.

Of course, on the Dell systems in question the EC GPE must be enabled
during suspend-to-idle transitions for the button press events to
be signaled while suspended at all.  For this reason, add a DMI
switch to the ACPI system suspend infrastructure to treat the EC
GPE as a wakeup one on the affected Dell systems.  In case the
users would prefer not to do that after all, add a new kernel
command line switch, acpi_sleep=no_ec_wakeup, to disable that new
behavior.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

-> v2: Added acpi_sleep=no_ec_wakeup to prevent EC events from waking up
          the system from s2idle on systems where they do that by default.

---
 Documentation/admin-guide/kernel-parameters.txt |    6 ++-
 arch/x86/kernel/acpi/sleep.c                    |    2 +
 drivers/acpi/ec.c                               |   19 ++++++++++
 drivers/acpi/internal.h                         |    2 +
 drivers/acpi/sleep.c                            |   43 ++++++++++++++++++++++++
 include/linux/acpi.h                            |    1 
 6 files changed, 71 insertions(+), 2 deletions(-)

Index: linux-pm/drivers/acpi/ec.c
===================================================================
--- linux-pm.orig/drivers/acpi/ec.c
+++ linux-pm/drivers/acpi/ec.c
@@ -40,6 +40,7 @@
 #include <linux/slab.h>
 #include <linux/acpi.h>
 #include <linux/dmi.h>
+#include <linux/suspend.h>
 #include <asm/io.h>
 
 #include "internal.h"
@@ -1493,6 +1494,16 @@ static int acpi_ec_setup(struct acpi_ec
 	acpi_handle_info(ec->handle,
 			 "GPE=0x%lx, EC_CMD/EC_SC=0x%lx, EC_DATA=0x%lx\n",
 			 ec->gpe, ec->command_addr, ec->data_addr);
+
+	/*
+	 * On some platforms the EC GPE is used for waking up the system from
+	 * suspend-to-idle, so mark it as a wakeup one.
+	 *
+	 * This can be done unconditionally, as the setting does not matter
+	 * until acpi_set_gpe_wake_mask() is called for the GPE.
+	 */
+	acpi_mark_gpe_for_wake(NULL, ec->gpe);
+
 	return ret;
 }
 
@@ -1835,8 +1846,11 @@ static int acpi_ec_suspend(struct device
 	struct acpi_ec *ec =
 		acpi_driver_data(to_acpi_device(dev));
 
-	if (ec_freeze_events)
+	if (!pm_suspend_via_firmware() && acpi_sleep_ec_gpe_may_wakeup())
+		acpi_set_gpe_wake_mask(NULL, ec->gpe, ACPI_GPE_ENABLE);
+	else if (ec_freeze_events)
 		acpi_ec_disable_event(ec);
+
 	return 0;
 }
 
@@ -1846,6 +1860,9 @@ static int acpi_ec_resume(struct device
 		acpi_driver_data(to_acpi_device(dev));
 
 	acpi_ec_enable_event(ec);
+	if (!pm_resume_via_firmware() && acpi_sleep_ec_gpe_may_wakeup())
+		acpi_set_gpe_wake_mask(NULL, ec->gpe, ACPI_GPE_DISABLE);
+
 	return 0;
 }
 #endif
Index: linux-pm/drivers/acpi/internal.h
===================================================================
--- linux-pm.orig/drivers/acpi/internal.h
+++ linux-pm/drivers/acpi/internal.h
@@ -199,9 +199,11 @@ void acpi_ec_remove_query_handler(struct
   -------------------------------------------------------------------------- */
 #ifdef CONFIG_ACPI_SYSTEM_POWER_STATES_SUPPORT
 extern bool acpi_s2idle_wakeup(void);
+extern bool acpi_sleep_ec_gpe_may_wakeup(void);
 extern int acpi_sleep_init(void);
 #else
 static inline bool acpi_s2idle_wakeup(void) { return false; }
+static inline bool acpi_sleep_ec_gpe_may_wakeup(void) { return false; }
 static inline int acpi_sleep_init(void) { return -ENXIO; }
 #endif
 
Index: linux-pm/drivers/acpi/sleep.c
===================================================================
--- linux-pm.orig/drivers/acpi/sleep.c
+++ linux-pm/drivers/acpi/sleep.c
@@ -160,6 +160,23 @@ static int __init init_nvs_nosave(const
 	return 0;
 }
 
+/* If set, it is allowed to use the EC GPE to wake up the system. */
+static bool ec_gpe_wakeup_allowed __initdata = true;
+
+void __init acpi_disable_ec_gpe_wakeup(void)
+{
+	ec_gpe_wakeup_allowed = false;
+}
+
+/* If set, the EC GPE will be configured to wake up the system. */
+static bool ec_gpe_wakeup;
+
+static int __init init_ec_gpe_wakeup(const struct dmi_system_id *d)
+{
+	ec_gpe_wakeup = ec_gpe_wakeup_allowed;
+	return 0;
+}
+
 static struct dmi_system_id acpisleep_dmi_table[] __initdata = {
 	{
 	.callback = init_old_suspend_ordering,
@@ -343,6 +360,26 @@ static struct dmi_system_id acpisleep_dm
 		DMI_MATCH(DMI_PRODUCT_NAME, "80E3"),
 		},
 	},
+	/*
+	 * Enable the EC to wake up the system from suspend-to-idle to allow
+	 * power button events to it wake up.
+	 */
+	{
+	 .callback = init_ec_gpe_wakeup,
+	 .ident = "Dell XPS 13 9360",
+	 .matches = {
+		DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
+		DMI_MATCH(DMI_PRODUCT_NAME, "XPS 13 9360"),
+		},
+	},
+	{
+	 .callback = init_ec_gpe_wakeup,
+	 .ident = "Dell XPS 13 9365",
+	 .matches = {
+		DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
+		DMI_MATCH(DMI_PRODUCT_NAME, "XPS 13 9365"),
+		},
+	},
 	{},
 };
 
@@ -485,6 +522,7 @@ static void acpi_pm_end(void)
 }
 #else /* !CONFIG_ACPI_SLEEP */
 #define acpi_target_sleep_state	ACPI_STATE_S0
+#define ec_gpe_wakeup		false
 static inline void acpi_sleep_dmi_check(void) {}
 #endif /* CONFIG_ACPI_SLEEP */
 
@@ -740,6 +778,11 @@ bool acpi_s2idle_wakeup(void)
 	return s2idle_wakeup;
 }
 
+bool acpi_sleep_ec_gpe_may_wakeup(void)
+{
+	return ec_gpe_wakeup;
+}
+
 #ifdef CONFIG_PM_SLEEP
 static u32 saved_bm_rld;
 
Index: linux-pm/arch/x86/kernel/acpi/sleep.c
===================================================================
--- linux-pm.orig/arch/x86/kernel/acpi/sleep.c
+++ linux-pm/arch/x86/kernel/acpi/sleep.c
@@ -137,6 +137,8 @@ static int __init acpi_sleep_setup(char
 			acpi_nvs_nosave_s3();
 		if (strncmp(str, "old_ordering", 12) == 0)
 			acpi_old_suspend_ordering();
+		if (strncmp(str, "no_ec_wakeup", 12) == 0)
+			acpi_disable_ec_gpe_wakeup();
 		str = strchr(str, ',');
 		if (str != NULL)
 			str += strspn(str, ", \t");
Index: linux-pm/include/linux/acpi.h
===================================================================
--- linux-pm.orig/include/linux/acpi.h
+++ linux-pm/include/linux/acpi.h
@@ -448,6 +448,7 @@ void __init acpi_no_s4_hw_signature(void
 void __init acpi_old_suspend_ordering(void);
 void __init acpi_nvs_nosave(void);
 void __init acpi_nvs_nosave_s3(void);
+void __init acpi_disable_ec_gpe_wakeup(void);
 #endif /* CONFIG_PM_SLEEP */
 
 struct acpi_osc_context {
Index: linux-pm/Documentation/admin-guide/kernel-parameters.txt
===================================================================
--- linux-pm.orig/Documentation/admin-guide/kernel-parameters.txt
+++ linux-pm/Documentation/admin-guide/kernel-parameters.txt
@@ -223,7 +223,8 @@
 
 	acpi_sleep=	[HW,ACPI] Sleep options
 			Format: { s3_bios, s3_mode, s3_beep, s4_nohwsig,
-				  old_ordering, nonvs, sci_force_enable }
+				  old_ordering, nonvs, sci_force_enable,
+				  no_ec_wakeup }
 			See Documentation/power/video.txt for information on
 			s3_bios and s3_mode.
 			s3_beep is for debugging; it makes the PC's speaker beep
@@ -239,6 +240,9 @@
 			sci_force_enable causes the kernel to set SCI_EN directly
 			on resume from S1/S3 (which is against the ACPI spec,
 			but some broken systems don't work without it).
+			no_ec_wakeup prevents the EC GPE from being configured
+			to wake up the system on platforms where that is done by
+			default.
 
 	acpi_use_timer_override [HW,ACPI]
 			Use timer override. For some broken Nvidia NF5 boards

[toc] | [next] | [standalone]

#1670036 — RE: [PATCH v2 3/3] ACPI / sleep: EC-based wakeup from suspend-to-idle on recent Dell systems

Hi, Rafael

> From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-owner@vger.kernel.org] On Behalf Of Rafael J.
> Wysocki
> Subject: [PATCH v2 3/3] ACPI / sleep: EC-based wakeup from suspend-to-idle on recent Dell systems
> 
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Some recent Dell laptops, including the XPS13 model numbers 9360 and
> 9365, cannot be woken up from suspend-to-idle by pressing the power
> button which is unexpected and makes that feature less usable on
> those systems.  Moreover, on the 9365 ACPI S3 (suspend-to-RAM) is
> not expected to be used at all (the OS these systems ship with never
> exercises the ACPI S3 path) and suspend-to-idle is the only viable
> system suspend mechanism in there.
> 
> The reason why the power button wakeup from suspend-to-idle doesn't
> work on those systems is because their power button events are
> signaled by the EC (Embedded Controller), whose GPE (General Purpose
> Event) line is disabled during suspend-to-idle transitions in Linux.
> That is done on purpose, because in general the EC tends to generate
> tons of events for various reasons (battery and thermal updates and
> similar, for example) and all of them would kick the CPUs out of deep
> idle states while in suspend-to-idle, which effectively would defeat
> its purpose.
> 
> Of course, on the Dell systems in question the EC GPE must be enabled
> during suspend-to-idle transitions for the button press events to
> be signaled while suspended at all.  For this reason, add a DMI
> switch to the ACPI system suspend infrastructure to treat the EC
> GPE as a wakeup one on the affected Dell systems.  In case the
> users would prefer not to do that after all, add a new kernel
> command line switch, acpi_sleep=no_ec_wakeup, to disable that new
> behavior.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
> 
> -> v2: Added acpi_sleep=no_ec_wakeup to prevent EC events from waking up
>           the system from s2idle on systems where they do that by default.
> 
> ---
>  Documentation/admin-guide/kernel-parameters.txt |    6 ++-
>  arch/x86/kernel/acpi/sleep.c                    |    2 +
>  drivers/acpi/ec.c                               |   19 ++++++++++
>  drivers/acpi/internal.h                         |    2 +
>  drivers/acpi/sleep.c                            |   43 ++++++++++++++++++++++++
>  include/linux/acpi.h                            |    1
>  6 files changed, 71 insertions(+), 2 deletions(-)
> 
> Index: linux-pm/drivers/acpi/ec.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/ec.c
> +++ linux-pm/drivers/acpi/ec.c
> @@ -40,6 +40,7 @@
>  #include <linux/slab.h>
>  #include <linux/acpi.h>
>  #include <linux/dmi.h>
> +#include <linux/suspend.h>
>  #include <asm/io.h>
> 
>  #include "internal.h"
> @@ -1493,6 +1494,16 @@ static int acpi_ec_setup(struct acpi_ec
>  	acpi_handle_info(ec->handle,
>  			 "GPE=0x%lx, EC_CMD/EC_SC=0x%lx, EC_DATA=0x%lx\n",
>  			 ec->gpe, ec->command_addr, ec->data_addr);
> +
> +	/*
> +	 * On some platforms the EC GPE is used for waking up the system from
> +	 * suspend-to-idle, so mark it as a wakeup one.
> +	 *
> +	 * This can be done unconditionally, as the setting does not matter
> +	 * until acpi_set_gpe_wake_mask() is called for the GPE.
> +	 */
> +	acpi_mark_gpe_for_wake(NULL, ec->gpe);
> +
>  	return ret;
>  }
> 
> @@ -1835,8 +1846,11 @@ static int acpi_ec_suspend(struct device
>  	struct acpi_ec *ec =
>  		acpi_driver_data(to_acpi_device(dev));
> 
> -	if (ec_freeze_events)
> +	if (!pm_suspend_via_firmware() && acpi_sleep_ec_gpe_may_wakeup())
> +		acpi_set_gpe_wake_mask(NULL, ec->gpe, ACPI_GPE_ENABLE);
> +	else if (ec_freeze_events)
>  		acpi_ec_disable_event(ec);
> +
>  	return 0;
>  }
> 
> @@ -1846,6 +1860,9 @@ static int acpi_ec_resume(struct device
>  		acpi_driver_data(to_acpi_device(dev));
> 
>  	acpi_ec_enable_event(ec);
> +	if (!pm_resume_via_firmware() && acpi_sleep_ec_gpe_may_wakeup())
> +		acpi_set_gpe_wake_mask(NULL, ec->gpe, ACPI_GPE_DISABLE);
> +
>  	return 0;
>  }
>  #endif
> Index: linux-pm/drivers/acpi/internal.h
> ===================================================================
> --- linux-pm.orig/drivers/acpi/internal.h
> +++ linux-pm/drivers/acpi/internal.h
> @@ -199,9 +199,11 @@ void acpi_ec_remove_query_handler(struct
>    -------------------------------------------------------------------------- */
>  #ifdef CONFIG_ACPI_SYSTEM_POWER_STATES_SUPPORT
>  extern bool acpi_s2idle_wakeup(void);
> +extern bool acpi_sleep_ec_gpe_may_wakeup(void);
>  extern int acpi_sleep_init(void);
>  #else
>  static inline bool acpi_s2idle_wakeup(void) { return false; }
> +static inline bool acpi_sleep_ec_gpe_may_wakeup(void) { return false; }
>  static inline int acpi_sleep_init(void) { return -ENXIO; }
>  #endif
> 
> Index: linux-pm/drivers/acpi/sleep.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/sleep.c
> +++ linux-pm/drivers/acpi/sleep.c
> @@ -160,6 +160,23 @@ static int __init init_nvs_nosave(const
>  	return 0;
>  }
> 
> +/* If set, it is allowed to use the EC GPE to wake up the system. */
> +static bool ec_gpe_wakeup_allowed __initdata = true;
> +
> +void __init acpi_disable_ec_gpe_wakeup(void)
> +{
> +	ec_gpe_wakeup_allowed = false;
> +}
> +
> +/* If set, the EC GPE will be configured to wake up the system. */
> +static bool ec_gpe_wakeup;
> +
> +static int __init init_ec_gpe_wakeup(const struct dmi_system_id *d)
> +{
> +	ec_gpe_wakeup = ec_gpe_wakeup_allowed;
> +	return 0;
> +}
> +
>  static struct dmi_system_id acpisleep_dmi_table[] __initdata = {
>  	{
>  	.callback = init_old_suspend_ordering,
> @@ -343,6 +360,26 @@ static struct dmi_system_id acpisleep_dm
>  		DMI_MATCH(DMI_PRODUCT_NAME, "80E3"),
>  		},
>  	},
> +	/*
> +	 * Enable the EC to wake up the system from suspend-to-idle to allow
> +	 * power button events to it wake up.
> +	 */
> +	{
> +	 .callback = init_ec_gpe_wakeup,
> +	 .ident = "Dell XPS 13 9360",
> +	 .matches = {
> +		DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> +		DMI_MATCH(DMI_PRODUCT_NAME, "XPS 13 9360"),
> +		},
> +	},
> +	{
> +	 .callback = init_ec_gpe_wakeup,
> +	 .ident = "Dell XPS 13 9365",
> +	 .matches = {
> +		DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> +		DMI_MATCH(DMI_PRODUCT_NAME, "XPS 13 9365"),
> +		},
> +	},
>  	{},
>  };
> 

I have a concern here.

ACPI spec has already defined a mechanism to statically
Mark GPEs as wake-capable and enable it, it is done via
_PRW. We may call it a "static wakeup GPE" mechanism.

Now the problem might be on some platforms, _PRW cannot be
prepared unconditionally. And the platform designers wants
a "dynamic wakeup GPE" mechanism to dynamically
mark/enable GPEs as wakeup GPE after having done some
platform specific behaviors (ex., after/before
saving/restoring some firmware configurations).

From this point of view, can we prepare several APIs in
sleep.c to allow dynamically mark/enable wakeup GPEs and
export EC information via a new API from ec.c, ex.,
acpi_ec_get_attributes(), or just publish struct acpi_ec
and first_ec in acpi_ec.h to the other drivers.
So that all such kinds of platforms drivers can use both
interfaces to dynamically achieve this, which can help
to avoid introducing quirk tables here.

Thanks and best regards
Lv

> @@ -485,6 +522,7 @@ static void acpi_pm_end(void)
>  }
>  #else /* !CONFIG_ACPI_SLEEP */
>  #define acpi_target_sleep_state	ACPI_STATE_S0
> +#define ec_gpe_wakeup		false
>  static inline void acpi_sleep_dmi_check(void) {}
>  #endif /* CONFIG_ACPI_SLEEP */
> 
> @@ -740,6 +778,11 @@ bool acpi_s2idle_wakeup(void)
>  	return s2idle_wakeup;
>  }
> 
> +bool acpi_sleep_ec_gpe_may_wakeup(void)
> +{
> +	return ec_gpe_wakeup;
> +}
> +
>  #ifdef CONFIG_PM_SLEEP
>  static u32 saved_bm_rld;
> 
> Index: linux-pm/arch/x86/kernel/acpi/sleep.c
> ===================================================================
> --- linux-pm.orig/arch/x86/kernel/acpi/sleep.c
> +++ linux-pm/arch/x86/kernel/acpi/sleep.c
> @@ -137,6 +137,8 @@ static int __init acpi_sleep_setup(char
>  			acpi_nvs_nosave_s3();
>  		if (strncmp(str, "old_ordering", 12) == 0)
>  			acpi_old_suspend_ordering();
> +		if (strncmp(str, "no_ec_wakeup", 12) == 0)
> +			acpi_disable_ec_gpe_wakeup();
>  		str = strchr(str, ',');
>  		if (str != NULL)
>  			str += strspn(str, ", \t");
> Index: linux-pm/include/linux/acpi.h
> ===================================================================
> --- linux-pm.orig/include/linux/acpi.h
> +++ linux-pm/include/linux/acpi.h
> @@ -448,6 +448,7 @@ void __init acpi_no_s4_hw_signature(void
>  void __init acpi_old_suspend_ordering(void);
>  void __init acpi_nvs_nosave(void);
>  void __init acpi_nvs_nosave_s3(void);
> +void __init acpi_disable_ec_gpe_wakeup(void);
>  #endif /* CONFIG_PM_SLEEP */
> 
>  struct acpi_osc_context {
> Index: linux-pm/Documentation/admin-guide/kernel-parameters.txt
> ===================================================================
> --- linux-pm.orig/Documentation/admin-guide/kernel-parameters.txt
> +++ linux-pm/Documentation/admin-guide/kernel-parameters.txt
> @@ -223,7 +223,8 @@
> 
>  	acpi_sleep=	[HW,ACPI] Sleep options
>  			Format: { s3_bios, s3_mode, s3_beep, s4_nohwsig,
> -				  old_ordering, nonvs, sci_force_enable }
> +				  old_ordering, nonvs, sci_force_enable,
> +				  no_ec_wakeup }
>  			See Documentation/power/video.txt for information on
>  			s3_bios and s3_mode.
>  			s3_beep is for debugging; it makes the PC's speaker beep
> @@ -239,6 +240,9 @@
>  			sci_force_enable causes the kernel to set SCI_EN directly
>  			on resume from S1/S3 (which is against the ACPI spec,
>  			but some broken systems don't work without it).
> +			no_ec_wakeup prevents the EC GPE from being configured
> +			to wake up the system on platforms where that is done by
> +			default.
> 
>  	acpi_use_timer_override [HW,ACPI]
>  			Use timer override. For some broken Nvidia NF5 boards
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[toc] | [prev] | [next] | [standalone]

#1670042 — Re: [PATCH v2 3/3] ACPI / sleep: EC-based wakeup from suspend-to-idle on recent Dell systems

On Tue, Jun 20, 2017 at 1:37 AM, Zheng, Lv <lv.zheng@intel.com> wrote:
> Hi, Rafael
>
>> From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-owner@vger.kernel.org] On Behalf Of Rafael J.
>> Wysocki
>> Subject: [PATCH v2 3/3] ACPI / sleep: EC-based wakeup from suspend-to-idle on recent Dell systems
>>
>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>
>> Some recent Dell laptops, including the XPS13 model numbers 9360 and
>> 9365, cannot be woken up from suspend-to-idle by pressing the power
>> button which is unexpected and makes that feature less usable on
>> those systems.  Moreover, on the 9365 ACPI S3 (suspend-to-RAM) is
>> not expected to be used at all (the OS these systems ship with never
>> exercises the ACPI S3 path) and suspend-to-idle is the only viable
>> system suspend mechanism in there.
>>
>> The reason why the power button wakeup from suspend-to-idle doesn't
>> work on those systems is because their power button events are
>> signaled by the EC (Embedded Controller), whose GPE (General Purpose
>> Event) line is disabled during suspend-to-idle transitions in Linux.
>> That is done on purpose, because in general the EC tends to generate
>> tons of events for various reasons (battery and thermal updates and
>> similar, for example) and all of them would kick the CPUs out of deep
>> idle states while in suspend-to-idle, which effectively would defeat
>> its purpose.
>>
>> Of course, on the Dell systems in question the EC GPE must be enabled
>> during suspend-to-idle transitions for the button press events to
>> be signaled while suspended at all.  For this reason, add a DMI
>> switch to the ACPI system suspend infrastructure to treat the EC
>> GPE as a wakeup one on the affected Dell systems.  In case the
>> users would prefer not to do that after all, add a new kernel
>> command line switch, acpi_sleep=no_ec_wakeup, to disable that new
>> behavior.
>>

[cut]

>>
>> Index: linux-pm/drivers/acpi/sleep.c
>> ===================================================================
>> --- linux-pm.orig/drivers/acpi/sleep.c
>> +++ linux-pm/drivers/acpi/sleep.c
>> @@ -160,6 +160,23 @@ static int __init init_nvs_nosave(const
>>       return 0;
>>  }
>>
>> +/* If set, it is allowed to use the EC GPE to wake up the system. */
>> +static bool ec_gpe_wakeup_allowed __initdata = true;
>> +
>> +void __init acpi_disable_ec_gpe_wakeup(void)
>> +{
>> +     ec_gpe_wakeup_allowed = false;
>> +}
>> +
>> +/* If set, the EC GPE will be configured to wake up the system. */
>> +static bool ec_gpe_wakeup;
>> +
>> +static int __init init_ec_gpe_wakeup(const struct dmi_system_id *d)
>> +{
>> +     ec_gpe_wakeup = ec_gpe_wakeup_allowed;
>> +     return 0;
>> +}
>> +
>>  static struct dmi_system_id acpisleep_dmi_table[] __initdata = {
>>       {
>>       .callback = init_old_suspend_ordering,
>> @@ -343,6 +360,26 @@ static struct dmi_system_id acpisleep_dm
>>               DMI_MATCH(DMI_PRODUCT_NAME, "80E3"),
>>               },
>>       },
>> +     /*
>> +      * Enable the EC to wake up the system from suspend-to-idle to allow
>> +      * power button events to it wake up.
>> +      */
>> +     {
>> +      .callback = init_ec_gpe_wakeup,
>> +      .ident = "Dell XPS 13 9360",
>> +      .matches = {
>> +             DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
>> +             DMI_MATCH(DMI_PRODUCT_NAME, "XPS 13 9360"),
>> +             },
>> +     },
>> +     {
>> +      .callback = init_ec_gpe_wakeup,
>> +      .ident = "Dell XPS 13 9365",
>> +      .matches = {
>> +             DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
>> +             DMI_MATCH(DMI_PRODUCT_NAME, "XPS 13 9365"),
>> +             },
>> +     },
>>       {},
>>  };
>>
>
> I have a concern here.
>
> ACPI spec has already defined a mechanism to statically
> Mark GPEs as wake-capable and enable it, it is done via
> _PRW. We may call it a "static wakeup GPE" mechanism.
>
> Now the problem might be on some platforms, _PRW cannot be
> prepared unconditionally. And the platform designers wants
> a "dynamic wakeup GPE" mechanism to dynamically
> mark/enable GPEs as wakeup GPE after having done some
> platform specific behaviors (ex., after/before
> saving/restoring some firmware configurations).
>
> From this point of view, can we prepare several APIs in
> sleep.c to allow dynamically mark/enable wakeup GPEs and
> export EC information via a new API from ec.c, ex.,
> acpi_ec_get_attributes(), or just publish struct acpi_ec
> and first_ec in acpi_ec.h to the other drivers.
> So that all such kinds of platforms drivers can use both
> interfaces to dynamically achieve this, which can help
> to avoid introducing quirk tables here.

I'm not sure how this is related to the patch.

Thanks,
Rafael

[toc] | [prev] | [next] | [standalone]

#1671275 — RE: [PATCH v2 3/3] ACPI / sleep: EC-based wakeup from suspend-to-idle on recent Dell systems

Hi,

> From: rjwysocki@gmail.com [mailto:rjwysocki@gmail.com] On Behalf Of Rafael J. Wysocki
> Subject: Re: [PATCH v2 3/3] ACPI / sleep: EC-based wakeup from suspend-to-idle on recent Dell systems
> 
> On Tue, Jun 20, 2017 at 1:37 AM, Zheng, Lv <lv.zheng@intel.com> wrote:
> > Hi, Rafael
> >
> >> From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-owner@vger.kernel.org] On Behalf Of
> Rafael J.
> >> Wysocki
> >> Subject: [PATCH v2 3/3] ACPI / sleep: EC-based wakeup from suspend-to-idle on recent Dell systems
> >>
> >> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >>
> >> Some recent Dell laptops, including the XPS13 model numbers 9360 and
> >> 9365, cannot be woken up from suspend-to-idle by pressing the power
> >> button which is unexpected and makes that feature less usable on
> >> those systems.  Moreover, on the 9365 ACPI S3 (suspend-to-RAM) is
> >> not expected to be used at all (the OS these systems ship with never
> >> exercises the ACPI S3 path) and suspend-to-idle is the only viable
> >> system suspend mechanism in there.
> >>
> >> The reason why the power button wakeup from suspend-to-idle doesn't
> >> work on those systems is because their power button events are
> >> signaled by the EC (Embedded Controller), whose GPE (General Purpose
> >> Event) line is disabled during suspend-to-idle transitions in Linux.
> >> That is done on purpose, because in general the EC tends to generate
> >> tons of events for various reasons (battery and thermal updates and
> >> similar, for example) and all of them would kick the CPUs out of deep
> >> idle states while in suspend-to-idle, which effectively would defeat
> >> its purpose.
> >>
> >> Of course, on the Dell systems in question the EC GPE must be enabled
> >> during suspend-to-idle transitions for the button press events to
> >> be signaled while suspended at all.  For this reason, add a DMI
> >> switch to the ACPI system suspend infrastructure to treat the EC
> >> GPE as a wakeup one on the affected Dell systems.  In case the
> >> users would prefer not to do that after all, add a new kernel
> >> command line switch, acpi_sleep=no_ec_wakeup, to disable that new
> >> behavior.
> >>
> 
> [cut]
> 
> >>
> >> Index: linux-pm/drivers/acpi/sleep.c
> >> ===================================================================
> >> --- linux-pm.orig/drivers/acpi/sleep.c
> >> +++ linux-pm/drivers/acpi/sleep.c
> >> @@ -160,6 +160,23 @@ static int __init init_nvs_nosave(const
> >>       return 0;
> >>  }
> >>
> >> +/* If set, it is allowed to use the EC GPE to wake up the system. */
> >> +static bool ec_gpe_wakeup_allowed __initdata = true;
> >> +
> >> +void __init acpi_disable_ec_gpe_wakeup(void)
> >> +{
> >> +     ec_gpe_wakeup_allowed = false;
> >> +}
> >> +
> >> +/* If set, the EC GPE will be configured to wake up the system. */
> >> +static bool ec_gpe_wakeup;
> >> +
> >> +static int __init init_ec_gpe_wakeup(const struct dmi_system_id *d)
> >> +{
> >> +     ec_gpe_wakeup = ec_gpe_wakeup_allowed;
> >> +     return 0;
> >> +}
> >> +
> >>  static struct dmi_system_id acpisleep_dmi_table[] __initdata = {
> >>       {
> >>       .callback = init_old_suspend_ordering,
> >> @@ -343,6 +360,26 @@ static struct dmi_system_id acpisleep_dm
> >>               DMI_MATCH(DMI_PRODUCT_NAME, "80E3"),
> >>               },
> >>       },
> >> +     /*
> >> +      * Enable the EC to wake up the system from suspend-to-idle to allow
> >> +      * power button events to it wake up.
> >> +      */
> >> +     {
> >> +      .callback = init_ec_gpe_wakeup,
> >> +      .ident = "Dell XPS 13 9360",
> >> +      .matches = {
> >> +             DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> >> +             DMI_MATCH(DMI_PRODUCT_NAME, "XPS 13 9360"),
> >> +             },
> >> +     },
> >> +     {
> >> +      .callback = init_ec_gpe_wakeup,
> >> +      .ident = "Dell XPS 13 9365",
> >> +      .matches = {
> >> +             DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> >> +             DMI_MATCH(DMI_PRODUCT_NAME, "XPS 13 9365"),
> >> +             },
> >> +     },
> >>       {},
> >>  };
> >>
> >
> > I have a concern here.
> >
> > ACPI spec has already defined a mechanism to statically
> > Mark GPEs as wake-capable and enable it, it is done via
> > _PRW. We may call it a "static wakeup GPE" mechanism.
> >
> > Now the problem might be on some platforms, _PRW cannot be
> > prepared unconditionally. And the platform designers wants
> > a "dynamic wakeup GPE" mechanism to dynamically
> > mark/enable GPEs as wakeup GPE after having done some
> > platform specific behaviors (ex., after/before
> > saving/restoring some firmware configurations).
> >
> > From this point of view, can we prepare several APIs in
> > sleep.c to allow dynamically mark/enable wakeup GPEs and
> > export EC information via a new API from ec.c, ex.,
> > acpi_ec_get_attributes(), or just publish struct acpi_ec
> > and first_ec in acpi_ec.h to the other drivers.
> > So that all such kinds of platforms drivers can use both
> > interfaces to dynamically achieve this, which can help
> > to avoid introducing quirk tables here.
> 
> I'm not sure how this is related to the patch.

Sorry, I was thinking this is still related to uPEP.

Best regards
Lv

[toc] | [prev] | [next] | [standalone]

#1670079 — Re: [PATCH v2 3/3] ACPI / sleep: EC-based wakeup from suspend-to-idle on recent Dell systems

On Tue, Jun 20, 2017 at 5:53 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>
> -> v2: Added acpi_sleep=no_ec_wakeup to prevent EC events from waking up
>           the system from s2idle on systems where they do that by default.

This seems a big hacky.

Is there no way to simply make acpi_ec_suspend() smarter while going
to sleep? Instead of just unconditionally disabling every EC GPE, can
we see that "this gpe is the power botton" somehow?

Disabling the power button event sounds fundamentally broken, and it
sounds like Windows doesn't do that. I doubt Windows has some hacky
whitelist. So I'd rather fix a deeper issue than have these kinds of
hacks, if at all possible.

                Linus

[toc] | [prev] | [next] | [standalone]

#1670142 — Re: [PATCH v2 3/3] ACPI / sleep: EC-based wakeup from suspend-to-idle on recent Dell systems

On Tue, Jun 20, 2017 at 2:07 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Tue, Jun 20, 2017 at 5:53 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>>
>> -> v2: Added acpi_sleep=no_ec_wakeup to prevent EC events from waking up
>>           the system from s2idle on systems where they do that by default.
>
> This seems a big hacky.
>
> Is there no way to simply make acpi_ec_suspend() smarter while going
> to sleep? Instead of just unconditionally disabling every EC GPE, can
> we see that "this gpe is the power botton" somehow?

Unfortunately, the connection between the GPE and the power button is
not direct.

The EC GPE handler has no idea that it will generate power button
events.  It simply executes an AML method doing that.

The AML method, in turn, executes Notify(power button device) and the
"power button device" driver has to register a notify handler that
will recognize and process the events.  It doesn't know in principle
where the events will come from, though.  They may come from the EC or
from a different GPE etc.

Neither the EC driver, nor the "power button device" driver can figure
out that the connection is there.

> Disabling the power button event sounds fundamentally broken, and it
> sounds like Windows doesn't do that. I doubt Windows has some hacky
> whitelist. So I'd rather fix a deeper issue than have these kinds of
> hacks, if at all possible.

My understanding is that Windows uses the ACPI_FADT_LOW_POWER_S0 flag.
It generally enables non-S3 suspend/resume when this flag is set and
it doesn't touch S3 then.  Keeping the EC GPE (and other GPEs for that
matter) enabled over suspend/resume is part of that if my
understanding is correct.

During suspend we generally disable all GPEs that are not expected to
generate wakeup events in order to avoid spurious wakeups, but we can
try to keep them enabled if ACPI_FADT_LOW_POWER_S0 is set.  That will
reduce the ugliness, but the cost may be more energy used while
suspended on some systems.

Thanks,
Rafael

[toc] | [prev] | [next] | [standalone]

#1670200 — Re: [PATCH v2 3/3] ACPI / sleep: EC-based wakeup from suspend-to-idle on recent Dell systems

On Tue, Jun 20, 2017 at 9:13 AM, Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> My understanding is that Windows uses the ACPI_FADT_LOW_POWER_S0 flag.
> It generally enables non-S3 suspend/resume when this flag is set and
> it doesn't touch S3 then.  Keeping the EC GPE (and other GPEs for that
> matter) enabled over suspend/resume is part of that if my
> understanding is correct.
>
> During suspend we generally disable all GPEs that are not expected to
> generate wakeup events in order to avoid spurious wakeups, but we can
> try to keep them enabled if ACPI_FADT_LOW_POWER_S0 is set.  That will
> reduce the ugliness, but the cost may be more energy used while
> suspended on some systems.

I think trying to do something similar to what windows does is likely
the right thing, since that is (sadly) the only thing that tends to
get extensive testing still.

Of course, different versions of Windows then probably do different
things, but I guess ACPI_FADT_LOW_POWER_S0 ends up being a good sign
of "new machine designed for windows 10", so it's probably a good
thing to trigger that behavior on.

So I suspect it's worth testing, particularly if we're going to be in
the situation that a lot of machines are going to do this going
forward (ie the "all Dell" may end up being more than just Dell too?
Dell usually doesn't do particularly odd and out-of-the-norm design
choices like some vendors do).

              Linus

[toc] | [prev] | [next] | [standalone]

#1671134 — Re: [PATCH v2 3/3] ACPI / sleep: EC-based wakeup from suspend-to-idle on recent Dell systems

On Tue, Jun 20, 2017 at 4:00 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Tue, Jun 20, 2017 at 9:13 AM, Rafael J. Wysocki <rafael@kernel.org> wrote:
>>
>> My understanding is that Windows uses the ACPI_FADT_LOW_POWER_S0 flag.
>> It generally enables non-S3 suspend/resume when this flag is set and
>> it doesn't touch S3 then.  Keeping the EC GPE (and other GPEs for that
>> matter) enabled over suspend/resume is part of that if my
>> understanding is correct.
>>
>> During suspend we generally disable all GPEs that are not expected to
>> generate wakeup events in order to avoid spurious wakeups, but we can
>> try to keep them enabled if ACPI_FADT_LOW_POWER_S0 is set.  That will
>> reduce the ugliness, but the cost may be more energy used while
>> suspended on some systems.
>
> I think trying to do something similar to what windows does is likely
> the right thing, since that is (sadly) the only thing that tends to
> get extensive testing still.
>
> Of course, different versions of Windows then probably do different
> things, but I guess ACPI_FADT_LOW_POWER_S0 ends up being a good sign
> of "new machine designed for windows 10", so it's probably a good
> thing to trigger that behavior on.
>
> So I suspect it's worth testing, particularly if we're going to be in
> the situation that a lot of machines are going to do this going
> forward (ie the "all Dell" may end up being more than just Dell too?
> Dell usually doesn't do particularly odd and out-of-the-norm design
> choices like some vendors do).

Well, involving the EC in power button events processing has not been
a common practice so far.

Anyway, I will replace this patch with something that ought to be more
in line with what Windows does.

Thanks,
Rafael

[toc] | [prev] | [next] | [standalone]

#1671274 — RE: [PATCH v2 3/3] ACPI / sleep: EC-based wakeup from suspend-to-idle on recent Dell systems

Hi, Rafael

> From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-owner@vger.kernel.org] On Behalf Of Rafael J.
> Wysocki
> Subject: Re: [PATCH v2 3/3] ACPI / sleep: EC-based wakeup from suspend-to-idle on recent Dell systems
> 
> On Tue, Jun 20, 2017 at 2:07 AM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> > On Tue, Jun 20, 2017 at 5:53 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> >>
> >> -> v2: Added acpi_sleep=no_ec_wakeup to prevent EC events from waking up
> >>           the system from s2idle on systems where they do that by default.
> >
> > This seems a big hacky.
> >
> > Is there no way to simply make acpi_ec_suspend() smarter while going
> > to sleep? Instead of just unconditionally disabling every EC GPE, can
> > we see that "this gpe is the power botton" somehow?
> 
> Unfortunately, the connection between the GPE and the power button is
> not direct.
> 
> The EC GPE handler has no idea that it will generate power button
> events.  It simply executes an AML method doing that.
> 
> The AML method, in turn, executes Notify(power button device) and the
> "power button device" driver has to register a notify handler that
> will recognize and process the events.  It doesn't know in principle
> where the events will come from, though.  They may come from the EC or
> from a different GPE etc.
> 
> Neither the EC driver, nor the "power button device" driver can figure
> out that the connection is there.

The EC driver can only get an event number after querying the firmware.
And it has no idea whether handling this event by executing _Exx where
Xx is the number of the event can result in Notify(power button device).

Traditional ACPI power button events are ACPI fixed events, not EC GPE:

Power button signal
A power button can be supplied in two ways.
 One way is to simply use the fixed status bit, and
 The other uses the declaration of an ACPI power device and AML code to
  determine the event.
For more information about the alternate-device based power button, see
Section 4.8.2.2.1.2, Control Method Power Button.”

If it is not designed as fixed event, OS has no idea what GPE, or
EC event number is related to the power button.

> 
> > Disabling the power button event sounds fundamentally broken, and it
> > sounds like Windows doesn't do that. I doubt Windows has some hacky
> > whitelist. So I'd rather fix a deeper issue than have these kinds of
> > hacks, if at all possible.
> 
> My understanding is that Windows uses the ACPI_FADT_LOW_POWER_S0 flag.
> It generally enables non-S3 suspend/resume when this flag is set and
> it doesn't touch S3 then.  Keeping the EC GPE (and other GPEs for that
> matter) enabled over suspend/resume is part of that if my
> understanding is correct.

This sounds reasonable, but I have a question.

On Surface notebooks, an EC GPE wake capable setting is prepared:
                    Device (EC0)
                    {
                        Name (_HID, EisaId ("PNP0C09"))  // _HID: Hardware ID
                        ...
                        Method (_STA, 0, NotSerialized)  // _STA: Status
                        {
                            ...
                            Return (0x0F)
                        }
                        Name (_GPE, 0x38)  // _GPE: General Purpose Events
                        Name (_PRW, Package (0x02)  // _PRW: Power Resources for Wake
                        {
                            0x38, 
                            0x03
                        })

The _PRW means GPE 0x38 (EC GPE) can wake-up the system from S3-S0.
And the platform only supports s2idle.
Decoding its FADT, we can see the flag is set:
[070h 0112   4]        Flags (decoded below) : 002384B5
...
                      Low Power S0 Idle (V5) : 1

If EC GPE should always be enabled when the flag is set, why MS
(surface pros are manufactured by MS) prepares _PRW for its EC?

Thanks,
Lv

> 
> During suspend we generally disable all GPEs that are not expected to
> generate wakeup events in order to avoid spurious wakeups, but we can
> try to keep them enabled if ACPI_FADT_LOW_POWER_S0 is set.  That will
> reduce the ugliness, but the cost may be more energy used while
> suspended on some systems.
> 
> Thanks,
> Rafael
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[toc] | [prev] | [next] | [standalone]

#1673141 — [PATCH] ACPI / sleep: EC-based wakeup from suspend-to-idle on recent systems

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Some recent Dell laptops, including the XPS13 model numbers 9360 and
9365, cannot be woken up from suspend-to-idle by pressing the power
button which is unexpected and makes that feature less usable on
those systems.  Moreover, on the 9365 ACPI S3 (suspend-to-RAM) is
not expected to be used at all (the OS these systems ship with never
exercises the ACPI S3 path in the firmware) and suspend-to-idle is
the only viable system suspend mechanism there.

The reason why the power button wakeup from suspend-to-idle doesn't
work on those systems is because their power button events are
signaled by the EC (Embedded Controller), whose GPE (General Purpose
Event) line is disabled during suspend-to-idle transitions in Linux.
That is done on purpose, because in general the EC tends to be noisy
for various reasons (battery and thermal updates and similar, for
example) and all events signaled by it would kick the CPUs out of
deep idle states while in suspend-to-idle, which effectively might
defeat its purpose.

Of course, on the Dell systems in question the EC GPE must be enabled
during suspend-to-idle transitions for the button press events to
be signaled while suspended at all, but fortunately there is a way
out of this puzzle.

First of all, those systems have the ACPI_FADT_LOW_POWER_S0 flag set
in their ACPI tables, which means that the OS is expected to prefer
the "low power S0 idle" system state over ACPI S3 on them.  That
causes the most recent versions of other OSes to simply ignore ACPI
S3 on those systems, so it is reasonable to expect that it should not
be necessary to block GPEs during suspend-to-idle on them.

Second, in addition to that, the systems in question provide a special
firmware interface that can be used to indicate to the platform that
the OS is transitioning into a system-wide low-power state in which
certain types of activity are not desirable or that it is leaving
such a state and that (in principle) should allow the platform to
adjust its operation mode accordingly.

That interface is a special _DSM object under a System Power
Management Controller device (PNP0D80).  The expected way to use it
is to invoke function 0 from it on system initialization, functions
3 and 5 during suspend transitions and functions 4 and 6 during
resume transitions (to reverse the actions carried out by the
former).  In particular, function 5 from the "Low-Power S0" device
_DSM is expected to cause the platform to put itself into a low-power
operation mode which should include making the EC less verbose (so to
speak).  Next, on resume, function 6 switches the platform back to
the "working-state" operation mode.

In accordance with the above, modify the ACPI suspend-to-idle code
to look for the "Low-Power S0" _DSM interface on platforms with the
ACPI_FADT_LOW_POWER_S0 flag set in the ACPI tables.  If it's there,
use it during suspend-to-idle transitions as prescribed and avoid
changing the GPE configuration in that case.  [That should reflect
what the most recent versions of other OSes do.]

Also modify the ACPI EC driver to make it handle events during
suspend-to-idle in the usual way if the "Low-Power S0" _DSM interface
is going to be used to make the power button events work while
suspended on the Dell machines mentioned above

Link: http://www.uefi.org/sites/default/files/resources/Intel_ACPI_Low_Power_S0_Idle.pdf
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

This is a replacement for https://patchwork.kernel.org/patch/9797909/

The changelog describes what is going on (and now the "Low-Power S0" _DSM
specification is public, so it can be used officially here) and it gets the job
done on the XPS13 9360.  [The additional sort of "bonus" is that the machine
looks "suspended" in s2idle now, as one of the effects of the _DSM appears
to be turning off the lights in a quite literal sense.]

The patch is based on https://patchwork.kernel.org/patch/9797913/ and
https://patchwork.kernel.org/patch/9797903/ on top of the current linux-next.

Thanks,
Rafael

---
 drivers/acpi/ec.c       |    2 
 drivers/acpi/internal.h |    2 
 drivers/acpi/sleep.c    |  107 ++++++++++++++++++++++++++++++++++++++++++++++--
 3 files changed, 107 insertions(+), 4 deletions(-)

Index: linux-pm/drivers/acpi/sleep.c
===================================================================
--- linux-pm.orig/drivers/acpi/sleep.c
+++ linux-pm/drivers/acpi/sleep.c
@@ -652,6 +652,84 @@ static const struct platform_suspend_ops
 
 static bool s2idle_wakeup;
 
+/*
+ * On platforms supporting the Low Power S0 Idle interface there is an ACPI
+ * device object with the PNP0D80 compatible device ID (System Power Management
+ * Controller) and a specific _DSM method under it.  That method, if present,
+ * can be used to indicate to the platform that the OS is transitioning into a
+ * low-power state in which certain types of activity are not desirable or that
+ * it is leaving such a state, which allows the platform to adjust its operation
+ * mode accordingly.
+ */
+static const struct acpi_device_id lps0_device_ids[] = {
+	{"PNP0D80", },
+	{"", },
+};
+
+#define ACPI_LPS0_DSM_UUID	"c4eb40a0-6cd2-11e2-bcfd-0800200c9a66"
+
+#define ACPI_LPS0_SCREEN_OFF	3
+#define ACPI_LPS0_SCREEN_ON	4
+#define ACPI_LPS0_ENTRY		5
+#define ACPI_LPS0_EXIT		6
+
+#define ACPI_S2IDLE_FUNC_MASK	((1 << ACPI_LPS0_ENTRY) | (1 << ACPI_LPS0_EXIT))
+
+static acpi_handle lps0_device_handle;
+static guid_t lps0_dsm_guid;
+static char lps0_dsm_func_mask;
+
+static void acpi_sleep_run_lps0_dsm(unsigned int func)
+{
+	union acpi_object *out_obj;
+
+	if (!(lps0_dsm_func_mask & (1 << func)))
+		return;
+
+	out_obj = acpi_evaluate_dsm(lps0_device_handle, &lps0_dsm_guid, 1, func, NULL);
+	ACPI_FREE(out_obj);
+
+	acpi_handle_debug(lps0_device_handle, "_DSM function %u evaluation %s\n",
+			  func, out_obj ? "successful" : "failed");
+}
+
+static int lps0_device_attach(struct acpi_device *adev,
+			      const struct acpi_device_id *not_used)
+{
+	union acpi_object *out_obj;
+
+	if (lps0_device_handle)
+		return 0;
+
+	if (!(acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0))
+		return 0;
+
+	guid_parse(ACPI_LPS0_DSM_UUID, &lps0_dsm_guid);
+	/* Check if the _DSM is present and as expected. */
+	out_obj = acpi_evaluate_dsm(adev->handle, &lps0_dsm_guid, 1, 0, NULL);
+	if (out_obj && out_obj->type == ACPI_TYPE_BUFFER) {
+		char bitmask = *(char *)out_obj->buffer.pointer;
+
+		if ((bitmask & ACPI_S2IDLE_FUNC_MASK) == ACPI_S2IDLE_FUNC_MASK) {
+			lps0_dsm_func_mask = bitmask;
+			lps0_device_handle = adev->handle;
+		}
+
+		acpi_handle_debug(adev->handle, "_DSM function mask: 0x%x\n",
+				  bitmask);
+	} else {
+		acpi_handle_debug(adev->handle,
+				  "_DSM function 0 evaluation failed\n");
+	}
+	ACPI_FREE(out_obj);
+	return 0;
+}
+
+static struct acpi_scan_handler lps0_handler = {
+	.ids = lps0_device_ids,
+	.attach = lps0_device_attach,
+};
+
 static int acpi_freeze_begin(void)
 {
 	acpi_scan_lock_acquire();
@@ -660,8 +738,18 @@ static int acpi_freeze_begin(void)
 
 static int acpi_freeze_prepare(void)
 {
-	acpi_enable_all_wakeup_gpes();
-	acpi_os_wait_events_complete();
+	if (lps0_device_handle) {
+		acpi_sleep_run_lps0_dsm(ACPI_LPS0_SCREEN_OFF);
+		acpi_sleep_run_lps0_dsm(ACPI_LPS0_ENTRY);
+	} else {
+		/*
+		 * The configuration of GPEs is changed here to avoid spurious
+		 * wakeups, but that should not be necessary if this is a
+		 * "low-power S0" platform and the low-power S0 _DSM is present.
+		 */
+		acpi_enable_all_wakeup_gpes();
+		acpi_os_wait_events_complete();
+	}
 	if (acpi_sci_irq_valid())
 		enable_irq_wake(acpi_sci_irq);
 
@@ -700,7 +788,12 @@ static void acpi_freeze_restore(void)
 	if (acpi_sci_irq_valid())
 		disable_irq_wake(acpi_sci_irq);
 
-	acpi_enable_all_runtime_gpes();
+	if (lps0_device_handle) {
+		acpi_sleep_run_lps0_dsm(ACPI_LPS0_EXIT);
+		acpi_sleep_run_lps0_dsm(ACPI_LPS0_SCREEN_ON);
+	} else {
+		acpi_enable_all_runtime_gpes();
+	}
 }
 
 static void acpi_freeze_end(void)
@@ -727,11 +820,14 @@ static void acpi_sleep_suspend_setup(voi
 
 	suspend_set_ops(old_suspend_ordering ?
 		&acpi_suspend_ops_old : &acpi_suspend_ops);
+
+	acpi_scan_add_handler(&lps0_handler);
 	freeze_set_ops(&acpi_freeze_ops);
 }
 
 #else /* !CONFIG_SUSPEND */
 #define s2idle_wakeup	(false)
+#define lps0_device_handle	(NULL)
 static inline void acpi_sleep_suspend_setup(void) {}
 #endif /* !CONFIG_SUSPEND */
 
@@ -740,6 +836,11 @@ bool acpi_s2idle_wakeup(void)
 	return s2idle_wakeup;
 }
 
+bool acpi_sleep_no_ec_events(void)
+{
+	return pm_suspend_via_firmware() || !lps0_device_handle;
+}
+
 #ifdef CONFIG_PM_SLEEP
 static u32 saved_bm_rld;
 
Index: linux-pm/drivers/acpi/ec.c
===================================================================
--- linux-pm.orig/drivers/acpi/ec.c
+++ linux-pm/drivers/acpi/ec.c
@@ -1835,7 +1835,7 @@ static int acpi_ec_suspend(struct device
 	struct acpi_ec *ec =
 		acpi_driver_data(to_acpi_device(dev));
 
-	if (ec_freeze_events)
+	if (acpi_sleep_no_ec_events() && ec_freeze_events)
 		acpi_ec_disable_event(ec);
 	return 0;
 }
Index: linux-pm/drivers/acpi/internal.h
===================================================================
--- linux-pm.orig/drivers/acpi/internal.h
+++ linux-pm/drivers/acpi/internal.h
@@ -199,9 +199,11 @@ void acpi_ec_remove_query_handler(struct
   -------------------------------------------------------------------------- */
 #ifdef CONFIG_ACPI_SYSTEM_POWER_STATES_SUPPORT
 extern bool acpi_s2idle_wakeup(void);
+extern bool acpi_sleep_no_ec_events(void);
 extern int acpi_sleep_init(void);
 #else
 static inline bool acpi_s2idle_wakeup(void) { return false; }
+static inline bool acpi_sleep_no_ec_events(void) { return true; }
 static inline int acpi_sleep_init(void) { return -ENXIO; }
 #endif
 

[toc] | [prev] | [next] | [standalone]

#1673181 — Re: [PATCH] ACPI / sleep: EC-based wakeup from suspend-to-idle on recent systems

On Thu, Jun 22, 2017 at 4:56 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>
> Some recent Dell laptops, including the XPS13 model numbers 9360 and
> 9365, cannot be woken up from suspend-to-idle by pressing the power
> button which is unexpected and makes that feature less usable on
> those systems.  [ details removed ]

This looks much more reasonable and more likely to work on future machines too.

Of course, who knows what broken machines it will cause problems on,
but it sounds like the code now does what it's supposed to and what
Win10 does, so maybe it JustWorks(tm). Hah.

Anyway - thanks.

             Linus

[toc] | [prev] | [next] | [standalone]

#1675309 — Re: [PATCH] ACPI / sleep: EC-based wakeup from suspend-to-idle on recent systems

On 23 June 2017 at 12:40, Linus Torvalds <torvalds@linux-foundation.org> wrote:
> On Thu, Jun 22, 2017 at 4:56 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>>
>> Some recent Dell laptops, including the XPS13 model numbers 9360 and
>> 9365, cannot be woken up from suspend-to-idle by pressing the power
>> button which is unexpected and makes that feature less usable on
>> those systems.  [ details removed ]
>
> This looks much more reasonable and more likely to work on future machines too.
>
> Of course, who knows what broken machines it will cause problems on,
> but it sounds like the code now does what it's supposed to and what
> Win10 does, so maybe it JustWorks(tm). Hah.

Rafael - thanks for your efforts on this.

I wanted to provide some feedback from some quick and naive tests on
an XPS 13 9365 in case it was useful, as it seems like there's still
some way to go before matching Win10's behaviour.

    Linux idling w/ screen ON => 17% battery drain per hour.
    Linux idling w/ screen OFF => 12% battery drain per hour.
    Linux during s2idle => 6% battery drain per hour.
    Win10 during sleep => 1% battery drain per hour.

where Linux = 4.12-rc6 + the latest patch from your acpi-pm-test branch.

So whilst s2idle halves the battery drain compared to the machine
staying powered on, it's still significantly more draining than Win10.
Let me know if there's any more useful analysis I can do.

-Tom

[toc] | [prev] | [next] | [standalone]

#1675330 — Re: [PATCH] ACPI / sleep: EC-based wakeup from suspend-to-idle on recent systems

On Tue, Jun 27, 2017 at 8:50 AM, Tom Lanyon <tom@oneshoeco.com> wrote:
> On 23 June 2017 at 12:40, Linus Torvalds <torvalds@linux-foundation.org> wrote:

>     Linux during s2idle => 6% battery drain per hour.
>     Win10 during sleep => 1% battery drain per hour.
>
> where Linux = 4.12-rc6 + the latest patch from your acpi-pm-test branch.
>
> So whilst s2idle halves the battery drain compared to the machine
> staying powered on, it's still significantly more draining than Win10.
> Let me know if there's any more useful analysis I can do.

Tom, thanks for this.
I would speculate that the problem might be in the certain device
drivers. It would be nice to get statistics which wakeup source
generates more hits.


-- 
With Best Regards,
Andy Shevchenko

[toc] | [prev] | [next] | [standalone]

#1675546 — Re: [PATCH] ACPI / sleep: EC-based wakeup from suspend-to-idle on recent systems

On 27 June 2017 at 16:47, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> Tom, thanks for this.
> I would speculate that the problem might be in the certain device
> drivers. It would be nice to get statistics which wakeup source
> generates more hits.

Is this something I can determine without CONFIG_ACPI_DEBUG being enabled?

I don't get anything in dmesg other than indications that it's
resuming and then sleeping again every second or so.

[   45.463907] PM: Suspending system (freeze)
[   47.703216] PM: suspend of devices complete after 2028.170 msecs
[   47.721329] PM: late suspend of devices complete after 18.108 msecs
[   47.723153] ACPI : EC: interrupt blocked
[   47.757801] PM: noirq suspend of devices complete after 35.746 msecs
[   47.757802] PM: suspend-to-idle
[   48.944708] Suspended for 0.779 seconds
[   48.945030] ACPI : EC: interrupt unblocked
[   48.980728] PM: noirq resume of devices complete after 35.924 msecs
[   48.982265] ACPI : EC: interrupt blocked
[   49.027946] PM: noirq suspend of devices complete after 47.016 msecs

[toc] | [prev] | [next] | [standalone]

#1675568 — RE: [PATCH] ACPI / sleep: EC-based wakeup from suspend-to-idle on recent systems

> -----Original Message-----
> From: Tom Lanyon [mailto:tom@oneshoeco.com]
> Sent: Tuesday, June 27, 2017 12:51 AM
> To: Rafael J. Wysocki <rjw@rjwysocki.net>
> Cc: Linux ACPI <linux-acpi@vger.kernel.org>; Linux PM <linux-
> pm@vger.kernel.org>; Andy Shevchenko <andriy.shevchenko@linux.intel.com>;
> Darren Hart <dvhart@infradead.org>; LKML <linux-kernel@vger.kernel.org>;
> Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>; Mika Westerberg
> <mika.westerberg@linux.intel.com>; Limonciello, Mario
> <Mario_Limonciello@Dell.com>; Jérôme de Bretagne
> <jerome.debretagne@gmail.com>; Zheng, Lv <lv.zheng@intel.com>; Linus Torvalds
> <torvalds@linux-foundation.org>
> Subject: Re: [PATCH] ACPI / sleep: EC-based wakeup from suspend-to-idle on recent
> systems
> 
> On 23 June 2017 at 12:40, Linus Torvalds <torvalds@linux-foundation.org> wrote:
> > On Thu, Jun 22, 2017 at 4:56 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> >>
> >> Some recent Dell laptops, including the XPS13 model numbers 9360 and
> >> 9365, cannot be woken up from suspend-to-idle by pressing the power
> >> button which is unexpected and makes that feature less usable on
> >> those systems.  [ details removed ]
> >
> > This looks much more reasonable and more likely to work on future machines too.
> >
> > Of course, who knows what broken machines it will cause problems on,
> > but it sounds like the code now does what it's supposed to and what
> > Win10 does, so maybe it JustWorks(tm). Hah.
> 
> Rafael - thanks for your efforts on this.
> 
> I wanted to provide some feedback from some quick and naive tests on
> an XPS 13 9365 in case it was useful, as it seems like there's still
> some way to go before matching Win10's behaviour.
> 
>     Linux idling w/ screen ON => 17% battery drain per hour.
>     Linux idling w/ screen OFF => 12% battery drain per hour.
>     Linux during s2idle => 6% battery drain per hour.
>     Win10 during sleep => 1% battery drain per hour.
> 
> where Linux = 4.12-rc6 + the latest patch from your acpi-pm-test branch.
> 
> So whilst s2idle halves the battery drain compared to the machine
> staying powered on, it's still significantly more draining than Win10.
> Let me know if there's any more useful analysis I can do.
> 
> -Tom

Tom,

This is quite useful data points to provide, thanks.

Would you mind doing on more test in your Linux comparison of turning off SD card reader in the BIOS setup?

Thanks,

[toc] | [prev] | [next] | [standalone]

#1675890 — Re: [PATCH] ACPI / sleep: EC-based wakeup from suspend-to-idle on recent systems

On Tuesday, June 27, 2017 03:50:33 PM Tom Lanyon wrote:
> On 23 June 2017 at 12:40, Linus Torvalds <torvalds@linux-foundation.org> wrote:
> > On Thu, Jun 22, 2017 at 4:56 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> >>
> >> Some recent Dell laptops, including the XPS13 model numbers 9360 and
> >> 9365, cannot be woken up from suspend-to-idle by pressing the power
> >> button which is unexpected and makes that feature less usable on
> >> those systems.  [ details removed ]
> >
> > This looks much more reasonable and more likely to work on future machines too.
> >
> > Of course, who knows what broken machines it will cause problems on,
> > but it sounds like the code now does what it's supposed to and what
> > Win10 does, so maybe it JustWorks(tm). Hah.
> 
> Rafael - thanks for your efforts on this.

You're welcome!

> I wanted to provide some feedback from some quick and naive tests on
> an XPS 13 9365 in case it was useful, as it seems like there's still
> some way to go before matching Win10's behaviour.
> 
>     Linux idling w/ screen ON => 17% battery drain per hour.
>     Linux idling w/ screen OFF => 12% battery drain per hour.
>     Linux during s2idle => 6% battery drain per hour.
>     Win10 during sleep => 1% battery drain per hour.
> 
> where Linux = 4.12-rc6 + the latest patch from your acpi-pm-test branch.
> 
> So whilst s2idle halves the battery drain compared to the machine
> staying powered on, it's still significantly more draining than Win10.

Thanks for the data.

> Let me know if there's any more useful analysis I can do.

I would carry out s2idle under turbostat to see how much PC10 residency is
there while suspended.  That may be a significant factor.

Most likely there is a device preventing the SoC from reaching its deepest
low-power states under Linux on your system and it needs to be identified
and dealt with.

Thanks,
Rafael

[toc] | [prev] | [next] | [standalone]

#1675971 — Re: [PATCH] ACPI / sleep: EC-based wakeup from suspend-to-idle on recent systems

On Tue, 2017-06-27 at 17:03 +0200, Rafael J. Wysocki wrote:
> On Tuesday, June 27, 2017 03:50:33 PM Tom Lanyon wrote:
> > 
> > On 23 June 2017 at 12:40, Linus Torvalds <torvalds@linux-foundation
> > .org> wrote:
> > > 
> > > On Thu, Jun 22, 2017 at 4:56 PM, Rafael J. Wysocki <rjw@rjwysocki
> > > .net> wrote:
> > > > 
> > > > 
> > > > Some recent Dell laptops, including the XPS13 model numbers
> > > > 9360 and
> > > > 9365, cannot be woken up from suspend-to-idle by pressing the
> > > > power
> > > > button which is unexpected and makes that feature less usable
> > > > on
> > > > those systems.  [ details removed ]
> > > 
> > > This looks much more reasonable and more likely to work on future
> > > machines too.
> > > 
> > > Of course, who knows what broken machines it will cause problems
> > > on,
> > > but it sounds like the code now does what it's supposed to and
> > > what
> > > Win10 does, so maybe it JustWorks(tm). Hah.
> > 
> > Rafael - thanks for your efforts on this.
> 
> You're welcome!
> 
> > 
> > I wanted to provide some feedback from some quick and naive tests
> > on
> > an XPS 13 9365 in case it was useful, as it seems like there's
> > still
> > some way to go before matching Win10's behaviour.
> > 
> >     Linux idling w/ screen ON => 17% battery drain per hour.
> >     Linux idling w/ screen OFF => 12% battery drain per hour.
> >     Linux during s2idle => 6% battery drain per hour.
> >     Win10 during sleep => 1% battery drain per hour.
> > 
> > where Linux = 4.12-rc6 + the latest patch from your acpi-pm-test
> > branch.
> > 
> > So whilst s2idle halves the battery drain compared to the machine
> > staying powered on, it's still significantly more draining than
> > Win10.
> 
> Thanks for the data.
> 
> > 
> > Let me know if there's any more useful analysis I can do.
> 
> I would carry out s2idle under turbostat to see how much PC10
> residency is
> there while suspended.  That may be a significant factor.
> 
> Most likely there is a device preventing the SoC from reaching its
> deepest
> low-power states under Linux on your system and it needs to be
> identified
> and dealt with.
Also make sure that you have no FW loading error for i915.
#dmesg | grep i915
It will display that Guc FW was loaded etc..
The latest FW can be downloaded from
https://01.org/linuxgraphics/downloads/firmware

If you don't see PC10 residency, we can try something more.

Thanks,
Srinivas


> 
> Thanks,
> Rafael
> 

[toc] | [prev] | [next] | [standalone]

#1673291 — RE: [PATCH] ACPI / sleep: EC-based wakeup from suspend-to-idle on recent systems

Hi, Rafael

> From: Rafael J. Wysocki [mailto:rjw@rjwysocki.net]
> Subject: [PATCH] ACPI / sleep: EC-based wakeup from suspend-to-idle on recent systems
> 
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Some recent Dell laptops, including the XPS13 model numbers 9360 and
> 9365, cannot be woken up from suspend-to-idle by pressing the power
> button which is unexpected and makes that feature less usable on
> those systems.  Moreover, on the 9365 ACPI S3 (suspend-to-RAM) is
> not expected to be used at all (the OS these systems ship with never
> exercises the ACPI S3 path in the firmware) and suspend-to-idle is
> the only viable system suspend mechanism there.
> 
> The reason why the power button wakeup from suspend-to-idle doesn't
> work on those systems is because their power button events are
> signaled by the EC (Embedded Controller), whose GPE (General Purpose
> Event) line is disabled during suspend-to-idle transitions in Linux.
> That is done on purpose, because in general the EC tends to be noisy
> for various reasons (battery and thermal updates and similar, for
> example) and all events signaled by it would kick the CPUs out of
> deep idle states while in suspend-to-idle, which effectively might
> defeat its purpose.
> 
> Of course, on the Dell systems in question the EC GPE must be enabled
> during suspend-to-idle transitions for the button press events to
> be signaled while suspended at all, but fortunately there is a way
> out of this puzzle.
> 
> First of all, those systems have the ACPI_FADT_LOW_POWER_S0 flag set
> in their ACPI tables, which means that the OS is expected to prefer
> the "low power S0 idle" system state over ACPI S3 on them.  That
> causes the most recent versions of other OSes to simply ignore ACPI
> S3 on those systems, so it is reasonable to expect that it should not
> be necessary to block GPEs during suspend-to-idle on them.
> 
> Second, in addition to that, the systems in question provide a special
> firmware interface that can be used to indicate to the platform that
> the OS is transitioning into a system-wide low-power state in which
> certain types of activity are not desirable or that it is leaving
> such a state and that (in principle) should allow the platform to
> adjust its operation mode accordingly.
> 
> That interface is a special _DSM object under a System Power
> Management Controller device (PNP0D80).  The expected way to use it
> is to invoke function 0 from it on system initialization, functions
> 3 and 5 during suspend transitions and functions 4 and 6 during
> resume transitions (to reverse the actions carried out by the
> former).  In particular, function 5 from the "Low-Power S0" device
> _DSM is expected to cause the platform to put itself into a low-power
> operation mode which should include making the EC less verbose (so to
> speak).  Next, on resume, function 6 switches the platform back to
> the "working-state" operation mode.
> 
> In accordance with the above, modify the ACPI suspend-to-idle code
> to look for the "Low-Power S0" _DSM interface on platforms with the
> ACPI_FADT_LOW_POWER_S0 flag set in the ACPI tables.  If it's there,
> use it during suspend-to-idle transitions as prescribed and avoid
> changing the GPE configuration in that case.  [That should reflect
> what the most recent versions of other OSes do.]
> 
> Also modify the ACPI EC driver to make it handle events during
> suspend-to-idle in the usual way if the "Low-Power S0" _DSM interface
> is going to be used to make the power button events work while
> suspended on the Dell machines mentioned above
> 
> Link: http://www.uefi.org/sites/default/files/resources/Intel_ACPI_Low_Power_S0_Idle.pdf
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
> 
> This is a replacement for https://patchwork.kernel.org/patch/9797909/
> 
> The changelog describes what is going on (and now the "Low-Power S0" _DSM
> specification is public, so it can be used officially here) and it gets the job
> done on the XPS13 9360.  [The additional sort of "bonus" is that the machine
> looks "suspended" in s2idle now, as one of the effects of the _DSM appears
> to be turning off the lights in a quite literal sense.]
> 
> The patch is based on https://patchwork.kernel.org/patch/9797913/ and
> https://patchwork.kernel.org/patch/9797903/ on top of the current linux-next.
> 
> Thanks,
> Rafael
> 
> ---
>  drivers/acpi/ec.c       |    2
>  drivers/acpi/internal.h |    2
>  drivers/acpi/sleep.c    |  107 ++++++++++++++++++++++++++++++++++++++++++++++--
>  3 files changed, 107 insertions(+), 4 deletions(-)
> 
> Index: linux-pm/drivers/acpi/sleep.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/sleep.c
> +++ linux-pm/drivers/acpi/sleep.c
> @@ -652,6 +652,84 @@ static const struct platform_suspend_ops
> 
>  static bool s2idle_wakeup;
> 
> +/*
> + * On platforms supporting the Low Power S0 Idle interface there is an ACPI
> + * device object with the PNP0D80 compatible device ID (System Power Management
> + * Controller) and a specific _DSM method under it.  That method, if present,
> + * can be used to indicate to the platform that the OS is transitioning into a
> + * low-power state in which certain types of activity are not desirable or that
> + * it is leaving such a state, which allows the platform to adjust its operation
> + * mode accordingly.
> + */
> +static const struct acpi_device_id lps0_device_ids[] = {
> +	{"PNP0D80", },
> +	{"", },
> +};
> +
> +#define ACPI_LPS0_DSM_UUID	"c4eb40a0-6cd2-11e2-bcfd-0800200c9a66"
> +
> +#define ACPI_LPS0_SCREEN_OFF	3
> +#define ACPI_LPS0_SCREEN_ON	4
> +#define ACPI_LPS0_ENTRY		5
> +#define ACPI_LPS0_EXIT		6
> +
> +#define ACPI_S2IDLE_FUNC_MASK	((1 << ACPI_LPS0_ENTRY) | (1 << ACPI_LPS0_EXIT))
> +
> +static acpi_handle lps0_device_handle;
> +static guid_t lps0_dsm_guid;
> +static char lps0_dsm_func_mask;
> +
> +static void acpi_sleep_run_lps0_dsm(unsigned int func)
> +{
> +	union acpi_object *out_obj;
> +
> +	if (!(lps0_dsm_func_mask & (1 << func)))
> +		return;
> +
> +	out_obj = acpi_evaluate_dsm(lps0_device_handle, &lps0_dsm_guid, 1, func, NULL);
> +	ACPI_FREE(out_obj);
> +
> +	acpi_handle_debug(lps0_device_handle, "_DSM function %u evaluation %s\n",
> +			  func, out_obj ? "successful" : "failed");
> +}
> +
> +static int lps0_device_attach(struct acpi_device *adev,
> +			      const struct acpi_device_id *not_used)
> +{
> +	union acpi_object *out_obj;
> +
> +	if (lps0_device_handle)
> +		return 0;
> +
> +	if (!(acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0))
> +		return 0;
> +
> +	guid_parse(ACPI_LPS0_DSM_UUID, &lps0_dsm_guid);
> +	/* Check if the _DSM is present and as expected. */
> +	out_obj = acpi_evaluate_dsm(adev->handle, &lps0_dsm_guid, 1, 0, NULL);
> +	if (out_obj && out_obj->type == ACPI_TYPE_BUFFER) {
> +		char bitmask = *(char *)out_obj->buffer.pointer;
> +
> +		if ((bitmask & ACPI_S2IDLE_FUNC_MASK) == ACPI_S2IDLE_FUNC_MASK) {
> +			lps0_dsm_func_mask = bitmask;
> +			lps0_device_handle = adev->handle;
> +		}
> +
> +		acpi_handle_debug(adev->handle, "_DSM function mask: 0x%x\n",
> +				  bitmask);
> +	} else {
> +		acpi_handle_debug(adev->handle,
> +				  "_DSM function 0 evaluation failed\n");
> +	}
> +	ACPI_FREE(out_obj);
> +	return 0;
> +}
> +
> +static struct acpi_scan_handler lps0_handler = {
> +	.ids = lps0_device_ids,
> +	.attach = lps0_device_attach,
> +};
> +
>  static int acpi_freeze_begin(void)
>  {
>  	acpi_scan_lock_acquire();
> @@ -660,8 +738,18 @@ static int acpi_freeze_begin(void)
> 
>  static int acpi_freeze_prepare(void)
>  {
> -	acpi_enable_all_wakeup_gpes();
> -	acpi_os_wait_events_complete();
> +	if (lps0_device_handle) {
> +		acpi_sleep_run_lps0_dsm(ACPI_LPS0_SCREEN_OFF);
> +		acpi_sleep_run_lps0_dsm(ACPI_LPS0_ENTRY);
> +	} else {
> +		/*
> +		 * The configuration of GPEs is changed here to avoid spurious
> +		 * wakeups, but that should not be necessary if this is a
> +		 * "low-power S0" platform and the low-power S0 _DSM is present.
> +		 */
> +		acpi_enable_all_wakeup_gpes();
> +		acpi_os_wait_events_complete();
> +	}
>  	if (acpi_sci_irq_valid())
>  		enable_irq_wake(acpi_sci_irq);
> 
> @@ -700,7 +788,12 @@ static void acpi_freeze_restore(void)
>  	if (acpi_sci_irq_valid())
>  		disable_irq_wake(acpi_sci_irq);
> 
> -	acpi_enable_all_runtime_gpes();
> +	if (lps0_device_handle) {
> +		acpi_sleep_run_lps0_dsm(ACPI_LPS0_EXIT);
> +		acpi_sleep_run_lps0_dsm(ACPI_LPS0_SCREEN_ON);
> +	} else {
> +		acpi_enable_all_runtime_gpes();
> +	}
>  }
> 
>  static void acpi_freeze_end(void)
> @@ -727,11 +820,14 @@ static void acpi_sleep_suspend_setup(voi
> 
>  	suspend_set_ops(old_suspend_ordering ?
>  		&acpi_suspend_ops_old : &acpi_suspend_ops);
> +
> +	acpi_scan_add_handler(&lps0_handler);
>  	freeze_set_ops(&acpi_freeze_ops);
>  }
> 
>  #else /* !CONFIG_SUSPEND */
>  #define s2idle_wakeup	(false)
> +#define lps0_device_handle	(NULL)
>  static inline void acpi_sleep_suspend_setup(void) {}
>  #endif /* !CONFIG_SUSPEND */
> 
> @@ -740,6 +836,11 @@ bool acpi_s2idle_wakeup(void)
>  	return s2idle_wakeup;
>  }
> 
> +bool acpi_sleep_no_ec_events(void)
> +{
> +	return pm_suspend_via_firmware() || !lps0_device_handle;
> +}
> +
>  #ifdef CONFIG_PM_SLEEP
>  static u32 saved_bm_rld;
> 
> Index: linux-pm/drivers/acpi/ec.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/ec.c
> +++ linux-pm/drivers/acpi/ec.c
> @@ -1835,7 +1835,7 @@ static int acpi_ec_suspend(struct device
>  	struct acpi_ec *ec =
>  		acpi_driver_data(to_acpi_device(dev));
> 
> -	if (ec_freeze_events)
> +	if (acpi_sleep_no_ec_events() && ec_freeze_events)
>  		acpi_ec_disable_event(ec);
>  	return 0;
>  }

I just notice a slight pontential issue.
Should we add a similar change to acpi_ec_stop()?
acpi_ec_stop() will be invoked by acpi_block_transactions(). When
ec_freeze_events=Y, acpi_ec_suspend() takes care of disabling
event before noirq stage - I introduced this recently in order to
avoid implementing event polling mode in noirq stage while still
can fix event loss issue.
When ec_freeze_events=N, acpi_block_transactions() takes care of
disabling event after noirq stage - old EC driver logic, risking
event loss issues on some platforms.

Thanks and best regards
Lv

> Index: linux-pm/drivers/acpi/internal.h
> ===================================================================
> --- linux-pm.orig/drivers/acpi/internal.h
> +++ linux-pm/drivers/acpi/internal.h
> @@ -199,9 +199,11 @@ void acpi_ec_remove_query_handler(struct
>    -------------------------------------------------------------------------- */
>  #ifdef CONFIG_ACPI_SYSTEM_POWER_STATES_SUPPORT
>  extern bool acpi_s2idle_wakeup(void);
> +extern bool acpi_sleep_no_ec_events(void);
>  extern int acpi_sleep_init(void);
>  #else
>  static inline bool acpi_s2idle_wakeup(void) { return false; }
> +static inline bool acpi_sleep_no_ec_events(void) { return true; }
>  static inline int acpi_sleep_init(void) { return -ENXIO; }
>  #endif
> 

[toc] | [prev] | [next] | [standalone]

#1673507 — Re: [PATCH] ACPI / sleep: EC-based wakeup from suspend-to-idle on recent systems

On Friday, June 23, 2017 06:30:35 AM Zheng, Lv wrote:
> Hi, Rafael
> 

[cut]

> > 
> > Index: linux-pm/drivers/acpi/ec.c
> > ===================================================================
> > --- linux-pm.orig/drivers/acpi/ec.c
> > +++ linux-pm/drivers/acpi/ec.c
> > @@ -1835,7 +1835,7 @@ static int acpi_ec_suspend(struct device
> >  	struct acpi_ec *ec =
> >  		acpi_driver_data(to_acpi_device(dev));
> > 
> > -	if (ec_freeze_events)
> > +	if (acpi_sleep_no_ec_events() && ec_freeze_events)
> >  		acpi_ec_disable_event(ec);
> >  	return 0;
> >  }
> 
> I just notice a slight pontential issue.
> Should we add a similar change to acpi_ec_stop()?

Yes, it looks like that, thanks!

Rafael

[toc] | [prev] | [next] | [standalone]

#1673515 — Re: [PATCH] ACPI / sleep: EC-based wakeup from suspend-to-idle on recent systems

On Friday, June 23, 2017 02:13:57 PM Rafael J. Wysocki wrote:
> On Friday, June 23, 2017 06:30:35 AM Zheng, Lv wrote:
> > Hi, Rafael
> > 
> 
> [cut]
> 
> > > 
> > > Index: linux-pm/drivers/acpi/ec.c
> > > ===================================================================
> > > --- linux-pm.orig/drivers/acpi/ec.c
> > > +++ linux-pm/drivers/acpi/ec.c
> > > @@ -1835,7 +1835,7 @@ static int acpi_ec_suspend(struct device
> > >  	struct acpi_ec *ec =
> > >  		acpi_driver_data(to_acpi_device(dev));
> > > 
> > > -	if (ec_freeze_events)
> > > +	if (acpi_sleep_no_ec_events() && ec_freeze_events)
> > >  		acpi_ec_disable_event(ec);
> > >  	return 0;
> > >  }
> > 
> > I just notice a slight pontential issue.
> > Should we add a similar change to acpi_ec_stop()?
> 
> Yes, it looks like that, thanks!

Actually, no, I don't think so, because acpi_ec_block_transactions() is not
used for suspend-to-idle, but I need a separate variable for that, because
pm_suspend_via_firmware() also returns "false" for hibernation.

Thanks,
Rafael

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web