linux-acpi.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 0/8] PM / ACPI: sleep: Additional changes related to suspend-to-idle
@ 2019-08-02 10:33 Rafael J. Wysocki
  2019-08-02 10:38 ` [PATCH v3 1/8] ACPI: PM: Set up EC GPE for system wakeup from drivers that need it Rafael J. Wysocki
                   ` (10 more replies)
  0 siblings, 11 replies; 25+ messages in thread
From: Rafael J. Wysocki @ 2019-08-02 10:33 UTC (permalink / raw)
  To: Linux ACPI
  Cc: Linux PM, LKML, Zhang Rui, Rajneesh Bhardwaj, Andy Shevchenko,
	Mario Limonciello, Kai-Heng Feng

Hi All,

> > On top of the "Simplify the suspend-to-idle control flow" patch series
> > posted previously:
> > 
> > https://lore.kernel.org/lkml/71085220.z6FKkvYQPX@kreacher/
> > 
> > sanitize the suspend-to-idle flow even further.
> > 
> > First off, decouple EC wakeup from the LPS0 _DSM processing (patch 1).
> > 
> > Next, reorder the code to invoke LPS0 _DSM Functions 5 and 6 in the
> > specification-compliant order with respect to suspending and resuming
> > devices (patch 2).
> > 
> > Finally, rearrange lps0_device_attach() (patch 3) and add a command line
> > switch to prevent the LPS0 _DSM from being used.\0
> 
> The v2 is because I found a (minor) bug in patch 1, decided to use a module
> parameter instead of a kernel command line option in patch 4.  Also, there
> are 4 new patches:
> 
> Patch 5: Switch the EC over to polling during "noirq" suspend and back
> during "noirq" resume.
> 
> Patch 6: Eliminate acpi_sleep_no_ec_events().
> 
> Patch 7: Consolidate some EC code depending on PM_SLEEP.
> 
> Patch 8: Add EC GPE dispatching debug message.\0

The v3 is just a rearranged v2 so as to move the post sensitive patch (previous patch 2)
to the end of the series.   [After applying the full series the code is the same as before.]

For easier testing, the series (along with some previous patches depended on by it)
is available in the pm-s2idle-testing branch of the linux-pm.git tree at kernel.org:

https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git/log/?h=pm-s2idle-testing

Please refer to the changelogs for details.

Thanks,
Rafael




^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH v3 1/8] ACPI: PM: Set up EC GPE for system wakeup from drivers that need it
  2019-08-02 10:33 [PATCH v3 0/8] PM / ACPI: sleep: Additional changes related to suspend-to-idle Rafael J. Wysocki
@ 2019-08-02 10:38 ` Rafael J. Wysocki
  2019-08-02 10:38 ` [PATCH v3 2/8] ACPI: PM: s2idle: Rearrange lps0_device_attach() Rafael J. Wysocki
                   ` (9 subsequent siblings)
  10 siblings, 0 replies; 25+ messages in thread
From: Rafael J. Wysocki @ 2019-08-02 10:38 UTC (permalink / raw)
  To: Linux ACPI
  Cc: Linux PM, LKML, Zhang Rui, Rajneesh Bhardwaj, Andy Shevchenko,
	Mario Limonciello, Kai-Heng Feng

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

The EC GPE needs to be set up for system wakeup only if there is a
driver depending on it, either intel-hid or intel-vbtn, bound to a
button device that is expected to wake up the system from sleep (such
as the power button on some Dell systems, like the XPS13 9360).  It
doesn't need to be set up for waking up the system from sleep in any
other cases and whether or not it is expected to wake up the system
from sleep doesn't depend on whether or not the LPS0 device is
present in the ACPI namespace.

For this reason, rearrange the ACPI suspend-to-idle code to make the
drivers depending on the EC GPE wakeup take care of setting it up and
decouple that from the LPS0 device handling.

While at it, make intel-hid and intel-vbtn prepare for system wakeup
only if they are allowed to wake up the system from sleep by user
space (via sysfs).

[Note that acpi_ec_mark_gpe_for_wake() and acpi_ec_set_gpe_wake_mask()
 are there to prevent the EC GPE from being disabled by the
 acpi_enable_all_wakeup_gpes() call in acpi_s2idle_prepare(), so on
 systems with either intel-hid or intel-vbtn this change doesn't
 affect any interactions with the hardware or platform firmware.]

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---

No changes in v3.

---
 drivers/acpi/ec.c                 |    7 ++++++-
 drivers/acpi/internal.h           |    2 --
 drivers/acpi/sleep.c              |   13 ++-----------
 drivers/platform/x86/intel-hid.c  |   20 ++++++++++++++++----
 drivers/platform/x86/intel-vbtn.c |   20 ++++++++++++++++----
 include/linux/acpi.h              |    4 ++++
 include/linux/suspend.h           |    1 +
 7 files changed, 45 insertions(+), 22 deletions(-)

Index: linux-pm/drivers/acpi/internal.h
===================================================================
--- linux-pm.orig/drivers/acpi/internal.h
+++ linux-pm/drivers/acpi/internal.h
@@ -194,8 +194,6 @@ void acpi_ec_ecdt_probe(void);
 void acpi_ec_dsdt_probe(void);
 void acpi_ec_block_transactions(void);
 void acpi_ec_unblock_transactions(void);
-void acpi_ec_mark_gpe_for_wake(void);
-void acpi_ec_set_gpe_wake_mask(u8 action);
 bool acpi_ec_dispatch_gpe(void);
 int acpi_ec_add_query_handler(struct acpi_ec *ec, u8 query_bit,
 			      acpi_handle handle, acpi_ec_query_func func,
Index: linux-pm/drivers/platform/x86/intel-hid.c
===================================================================
--- linux-pm.orig/drivers/platform/x86/intel-hid.c
+++ linux-pm/drivers/platform/x86/intel-hid.c
@@ -253,9 +253,12 @@ static void intel_button_array_enable(st
 
 static int intel_hid_pm_prepare(struct device *device)
 {
-	struct intel_hid_priv *priv = dev_get_drvdata(device);
+	if (device_may_wakeup(device)) {
+		struct intel_hid_priv *priv = dev_get_drvdata(device);
 
-	priv->wakeup_mode = true;
+		priv->wakeup_mode = true;
+		acpi_ec_set_gpe_wake_mask(ACPI_GPE_ENABLE);
+	}
 	return 0;
 }
 
@@ -270,9 +273,12 @@ static int intel_hid_pl_suspend_handler(
 
 static int intel_hid_pl_resume_handler(struct device *device)
 {
-	struct intel_hid_priv *priv = dev_get_drvdata(device);
+	if (device_may_wakeup(device)) {
+		struct intel_hid_priv *priv = dev_get_drvdata(device);
 
-	priv->wakeup_mode = false;
+		acpi_ec_set_gpe_wake_mask(ACPI_GPE_DISABLE);
+		priv->wakeup_mode = false;
+	}
 	if (pm_resume_via_firmware()) {
 		intel_hid_set_enable(device, true);
 		intel_button_array_enable(device, true);
@@ -491,6 +497,12 @@ static int intel_hid_probe(struct platfo
 	}
 
 	device_init_wakeup(&device->dev, true);
+	/*
+	 * In order for system wakeup to work, the EC GPE has to be marked as
+	 * a wakeup one, so do that here (this setting will persist, but it has
+	 * no effect until the wakeup mask is set for the EC GPE).
+	 */
+	acpi_ec_mark_gpe_for_wake();
 	return 0;
 
 err_remove_notify:
Index: linux-pm/drivers/platform/x86/intel-vbtn.c
===================================================================
--- linux-pm.orig/drivers/platform/x86/intel-vbtn.c
+++ linux-pm/drivers/platform/x86/intel-vbtn.c
@@ -176,6 +176,12 @@ static int intel_vbtn_probe(struct platf
 		return -EBUSY;
 
 	device_init_wakeup(&device->dev, true);
+	/*
+	 * In order for system wakeup to work, the EC GPE has to be marked as
+	 * a wakeup one, so do that here (this setting will persist, but it has
+	 * no effect until the wakeup mask is set for the EC GPE).
+	 */
+	acpi_ec_mark_gpe_for_wake();
 	return 0;
 }
 
@@ -195,17 +201,23 @@ static int intel_vbtn_remove(struct plat
 
 static int intel_vbtn_pm_prepare(struct device *dev)
 {
-	struct intel_vbtn_priv *priv = dev_get_drvdata(dev);
+	if (device_may_wakeup(dev)) {
+		struct intel_vbtn_priv *priv = dev_get_drvdata(dev);
 
-	priv->wakeup_mode = true;
+		priv->wakeup_mode = true;
+		acpi_ec_set_gpe_wake_mask(ACPI_GPE_ENABLE);
+	}
 	return 0;
 }
 
 static int intel_vbtn_pm_resume(struct device *dev)
 {
-	struct intel_vbtn_priv *priv = dev_get_drvdata(dev);
+	if (device_may_wakeup(dev)) {
+		struct intel_vbtn_priv *priv = dev_get_drvdata(dev);
 
-	priv->wakeup_mode = false;
+		acpi_ec_set_gpe_wake_mask(ACPI_GPE_DISABLE);
+		priv->wakeup_mode = false;
+	}
 	return 0;
 }
 
Index: linux-pm/include/linux/acpi.h
===================================================================
--- linux-pm.orig/include/linux/acpi.h
+++ linux-pm/include/linux/acpi.h
@@ -931,6 +931,8 @@ int acpi_subsys_suspend_noirq(struct dev
 int acpi_subsys_suspend(struct device *dev);
 int acpi_subsys_freeze(struct device *dev);
 int acpi_subsys_poweroff(struct device *dev);
+void acpi_ec_mark_gpe_for_wake(void);
+void acpi_ec_set_gpe_wake_mask(u8 action);
 #else
 static inline int acpi_subsys_prepare(struct device *dev) { return 0; }
 static inline void acpi_subsys_complete(struct device *dev) {}
@@ -939,6 +941,8 @@ static inline int acpi_subsys_suspend_no
 static inline int acpi_subsys_suspend(struct device *dev) { return 0; }
 static inline int acpi_subsys_freeze(struct device *dev) { return 0; }
 static inline int acpi_subsys_poweroff(struct device *dev) { return 0; }
+static inline void acpi_ec_mark_gpe_for_wake(void) {}
+static inline void acpi_ec_set_gpe_wake_mask(u8 action) {}
 #endif
 
 #ifdef CONFIG_ACPI
Index: linux-pm/drivers/acpi/ec.c
===================================================================
--- linux-pm.orig/drivers/acpi/ec.c
+++ linux-pm/drivers/acpi/ec.c
@@ -25,6 +25,7 @@
 #include <linux/list.h>
 #include <linux/spinlock.h>
 #include <linux/slab.h>
+#include <linux/suspend.h>
 #include <linux/acpi.h>
 #include <linux/dmi.h>
 #include <asm/io.h>
@@ -1048,17 +1049,21 @@ void acpi_ec_unblock_transactions(void)
 		acpi_ec_start(first_ec, true);
 }
 
+#ifdef CONFIG_PM_SLEEP
 void acpi_ec_mark_gpe_for_wake(void)
 {
 	if (first_ec && !ec_no_wakeup)
 		acpi_mark_gpe_for_wake(NULL, first_ec->gpe);
 }
+EXPORT_SYMBOL_GPL(acpi_ec_mark_gpe_for_wake);
 
 void acpi_ec_set_gpe_wake_mask(u8 action)
 {
-	if (first_ec && !ec_no_wakeup)
+	if (pm_suspend_no_platform() && first_ec && !ec_no_wakeup)
 		acpi_set_gpe_wake_mask(NULL, first_ec->gpe, action);
 }
+EXPORT_SYMBOL_GPL(acpi_ec_set_gpe_wake_mask);
+#endif
 
 bool acpi_ec_dispatch_gpe(void)
 {
Index: linux-pm/drivers/acpi/sleep.c
===================================================================
--- linux-pm.orig/drivers/acpi/sleep.c
+++ linux-pm/drivers/acpi/sleep.c
@@ -930,8 +930,6 @@ static int lps0_device_attach(struct acp
 
 		acpi_handle_debug(adev->handle, "_DSM function mask: 0x%x\n",
 				  bitmask);
-
-		acpi_ec_mark_gpe_for_wake();
 	} else {
 		acpi_handle_debug(adev->handle,
 				  "_DSM function 0 evaluation failed\n");
@@ -960,8 +958,6 @@ static int acpi_s2idle_prepare(void)
 	if (lps0_device_handle) {
 		acpi_sleep_run_lps0_dsm(ACPI_LPS0_SCREEN_OFF);
 		acpi_sleep_run_lps0_dsm(ACPI_LPS0_ENTRY);
-
-		acpi_ec_set_gpe_wake_mask(ACPI_GPE_ENABLE);
 	}
 
 	if (acpi_sci_irq_valid())
@@ -979,10 +975,7 @@ static int acpi_s2idle_prepare(void)
 
 static void acpi_s2idle_wake(void)
 {
-	if (!lps0_device_handle)
-		return;
-
-	if (pm_debug_messages_on)
+	if (lps0_device_handle && pm_debug_messages_on)
 		lpi_check_constraints();
 
 	/*
@@ -1031,8 +1024,6 @@ static void acpi_s2idle_restore(void)
 		disable_irq_wake(acpi_sci_irq);
 
 	if (lps0_device_handle) {
-		acpi_ec_set_gpe_wake_mask(ACPI_GPE_DISABLE);
-
 		acpi_sleep_run_lps0_dsm(ACPI_LPS0_EXIT);
 		acpi_sleep_run_lps0_dsm(ACPI_LPS0_SCREEN_ON);
 	}
@@ -1081,7 +1072,7 @@ bool acpi_s2idle_wakeup(void)
 
 bool acpi_sleep_no_ec_events(void)
 {
-	return !s2idle_in_progress || !lps0_device_handle;
+	return !s2idle_in_progress;
 }
 
 #ifdef CONFIG_PM_SLEEP
Index: linux-pm/include/linux/suspend.h
===================================================================
--- linux-pm.orig/include/linux/suspend.h
+++ linux-pm/include/linux/suspend.h
@@ -335,6 +335,7 @@ static inline void pm_set_suspend_via_fi
 static inline void pm_set_resume_via_firmware(void) {}
 static inline bool pm_suspend_via_firmware(void) { return false; }
 static inline bool pm_resume_via_firmware(void) { return false; }
+static inline bool pm_suspend_no_platform(void) { return false; }
 static inline bool pm_suspend_default_s2idle(void) { return false; }
 
 static inline void suspend_set_ops(const struct platform_suspend_ops *ops) {}




^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH v3 2/8] ACPI: PM: s2idle: Rearrange lps0_device_attach()
  2019-08-02 10:33 [PATCH v3 0/8] PM / ACPI: sleep: Additional changes related to suspend-to-idle Rafael J. Wysocki
  2019-08-02 10:38 ` [PATCH v3 1/8] ACPI: PM: Set up EC GPE for system wakeup from drivers that need it Rafael J. Wysocki
@ 2019-08-02 10:38 ` Rafael J. Wysocki
  2019-08-02 10:38 ` [PATCH v3 3/8] ACPI: PM: s2idle: Add acpi.sleep_no_lps0 module parameter Rafael J. Wysocki
                   ` (8 subsequent siblings)
  10 siblings, 0 replies; 25+ messages in thread
From: Rafael J. Wysocki @ 2019-08-02 10:38 UTC (permalink / raw)
  To: Linux ACPI
  Cc: Linux PM, LKML, Zhang Rui, Rajneesh Bhardwaj, Andy Shevchenko,
	Mario Limonciello, Kai-Heng Feng

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

To allow a subsequent change to be simpler, rearrange the code in
lps0_device_attach() to reduce the indentation level and (while
at it) make it avoid calling lpi_device_get_constraints() when
lps0_device_handle is not going to be set.

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

In v2 this was patch 3.

---
 drivers/acpi/sleep.c |   32 +++++++++++++++++---------------
 1 file changed, 17 insertions(+), 15 deletions(-)

Index: linux-pm/drivers/acpi/sleep.c
===================================================================
--- linux-pm.orig/drivers/acpi/sleep.c
+++ linux-pm/drivers/acpi/sleep.c
@@ -916,28 +916,30 @@ static int lps0_device_attach(struct acp
 	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;
-
-		lps0_dsm_func_mask = bitmask;
-		lps0_device_handle = adev->handle;
-		/*
-		 * Use suspend-to-idle by default if the default
-		 * suspend mode was not set from the command line.
-		 */
-		if (mem_sleep_default > PM_SUSPEND_MEM)
-			mem_sleep_current = PM_SUSPEND_TO_IDLE;
-
-		acpi_handle_debug(adev->handle, "_DSM function mask: 0x%x\n",
-				  bitmask);
-	} else {
+	if (!out_obj || out_obj->type != ACPI_TYPE_BUFFER) {
 		acpi_handle_debug(adev->handle,
 				  "_DSM function 0 evaluation failed\n");
+		return 0;
 	}
+
+	lps0_dsm_func_mask = *(char *)out_obj->buffer.pointer;
+
 	ACPI_FREE(out_obj);
 
+	acpi_handle_debug(adev->handle, "_DSM function mask: 0x%x\n",
+			  lps0_dsm_func_mask);
+
+	lps0_device_handle = adev->handle;
+
 	lpi_device_get_constraints();
 
+	/*
+	 * Use suspend-to-idle by default if the default suspend mode was not
+	 * set from the command line.
+	 */
+	if (mem_sleep_default > PM_SUSPEND_MEM)
+		mem_sleep_current = PM_SUSPEND_TO_IDLE;
+
 	return 0;
 }
 




^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH v3 3/8] ACPI: PM: s2idle: Add acpi.sleep_no_lps0 module parameter
  2019-08-02 10:33 [PATCH v3 0/8] PM / ACPI: sleep: Additional changes related to suspend-to-idle Rafael J. Wysocki
  2019-08-02 10:38 ` [PATCH v3 1/8] ACPI: PM: Set up EC GPE for system wakeup from drivers that need it Rafael J. Wysocki
  2019-08-02 10:38 ` [PATCH v3 2/8] ACPI: PM: s2idle: Rearrange lps0_device_attach() Rafael J. Wysocki
@ 2019-08-02 10:38 ` Rafael J. Wysocki
  2019-08-02 10:40 ` [PATCH v3 4/8] ACPI: PM: s2idle: Switch EC over to polling during "noirq" suspend Rafael J. Wysocki
                   ` (7 subsequent siblings)
  10 siblings, 0 replies; 25+ messages in thread
From: Rafael J. Wysocki @ 2019-08-02 10:38 UTC (permalink / raw)
  To: Linux ACPI
  Cc: Linux PM, LKML, Zhang Rui, Rajneesh Bhardwaj, Andy Shevchenko,
	Mario Limonciello, Kai-Heng Feng

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

Add a module parameter to prevent the ACPI LPS0 _DSM functions
from being invoked (if need be) and rework the suspend-to-idle
blacklist entries in acpisleep_dmi_table[] to make them simply
prevent suspend-to-idle from being used by default on the systems
in question (which really is the original purpose of those entries).

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

In v2 this was patch 4.

---
 drivers/acpi/sleep.c |   31 +++++++++++++++----------------
 1 file changed, 15 insertions(+), 16 deletions(-)

Index: linux-pm/drivers/acpi/sleep.c
===================================================================
--- linux-pm.orig/drivers/acpi/sleep.c
+++ linux-pm/drivers/acpi/sleep.c
@@ -89,6 +89,10 @@ bool acpi_sleep_state_supported(u8 sleep
 }
 
 #ifdef CONFIG_ACPI_SLEEP
+static bool sleep_no_lps0 __read_mostly;
+module_param(sleep_no_lps0, bool, 0644);
+MODULE_PARM_DESC(sleep_no_lps0, "Do not use the special LPS0 device interface");
+
 static u32 acpi_target_sleep_state = ACPI_STATE_S0;
 
 u32 acpi_target_system_state(void)
@@ -158,11 +162,11 @@ static int __init init_nvs_nosave(const
 	return 0;
 }
 
-static bool acpi_sleep_no_lps0;
+static bool acpi_sleep_default_s3;
 
-static int __init init_no_lps0(const struct dmi_system_id *d)
+static int __init init_default_s3(const struct dmi_system_id *d)
 {
-	acpi_sleep_no_lps0 = true;
+	acpi_sleep_default_s3 = true;
 	return 0;
 }
 
@@ -363,7 +367,7 @@ static const struct dmi_system_id acpisl
 	 * S0 Idle firmware interface.
 	 */
 	{
-	.callback = init_no_lps0,
+	.callback = init_default_s3,
 	.ident = "Dell XPS13 9360",
 	.matches = {
 		DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
@@ -376,7 +380,7 @@ static const struct dmi_system_id acpisl
 	 * https://bugzilla.kernel.org/show_bug.cgi?id=199057).
 	 */
 	{
-	.callback = init_no_lps0,
+	.callback = init_default_s3,
 	.ident = "ThinkPad X1 Tablet(2016)",
 	.matches = {
 		DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
@@ -524,8 +528,9 @@ static void acpi_pm_end(void)
 	acpi_sleep_tts_switch(acpi_target_sleep_state);
 }
 #else /* !CONFIG_ACPI_SLEEP */
+#define sleep_no_lps0	(1)
 #define acpi_target_sleep_state	ACPI_STATE_S0
-#define acpi_sleep_no_lps0	(false)
+#define acpi_sleep_default_s3	(1)
 static inline void acpi_sleep_dmi_check(void) {}
 #endif /* CONFIG_ACPI_SLEEP */
 
@@ -904,12 +909,6 @@ static int lps0_device_attach(struct acp
 	if (lps0_device_handle)
 		return 0;
 
-	if (acpi_sleep_no_lps0) {
-		acpi_handle_info(adev->handle,
-				 "Low Power S0 Idle interface disabled\n");
-		return 0;
-	}
-
 	if (!(acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0))
 		return 0;
 
@@ -937,7 +936,7 @@ static int lps0_device_attach(struct acp
 	 * Use suspend-to-idle by default if the default suspend mode was not
 	 * set from the command line.
 	 */
-	if (mem_sleep_default > PM_SUSPEND_MEM)
+	if (mem_sleep_default > PM_SUSPEND_MEM && !acpi_sleep_default_s3)
 		mem_sleep_current = PM_SUSPEND_TO_IDLE;
 
 	return 0;
@@ -957,7 +956,7 @@ static int acpi_s2idle_begin(void)
 
 static int acpi_s2idle_prepare(void)
 {
-	if (lps0_device_handle) {
+	if (lps0_device_handle && !sleep_no_lps0) {
 		acpi_sleep_run_lps0_dsm(ACPI_LPS0_SCREEN_OFF);
 		acpi_sleep_run_lps0_dsm(ACPI_LPS0_ENTRY);
 	}
@@ -977,7 +976,7 @@ static int acpi_s2idle_prepare(void)
 
 static void acpi_s2idle_wake(void)
 {
-	if (lps0_device_handle && pm_debug_messages_on)
+	if (lps0_device_handle && !sleep_no_lps0 && pm_debug_messages_on)
 		lpi_check_constraints();
 
 	/*
@@ -1025,7 +1024,7 @@ static void acpi_s2idle_restore(void)
 	if (acpi_sci_irq_valid())
 		disable_irq_wake(acpi_sci_irq);
 
-	if (lps0_device_handle) {
+	if (lps0_device_handle && !sleep_no_lps0) {
 		acpi_sleep_run_lps0_dsm(ACPI_LPS0_EXIT);
 		acpi_sleep_run_lps0_dsm(ACPI_LPS0_SCREEN_ON);
 	}




^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH v3 4/8] ACPI: PM: s2idle: Switch EC over to polling during "noirq" suspend
  2019-08-02 10:33 [PATCH v3 0/8] PM / ACPI: sleep: Additional changes related to suspend-to-idle Rafael J. Wysocki
                   ` (2 preceding siblings ...)
  2019-08-02 10:38 ` [PATCH v3 3/8] ACPI: PM: s2idle: Add acpi.sleep_no_lps0 module parameter Rafael J. Wysocki
@ 2019-08-02 10:40 ` Rafael J. Wysocki
  2019-08-02 10:41 ` [PATCH v3 5/8] ACPI: PM: s2idle: Eliminate acpi_sleep_no_ec_events() Rafael J. Wysocki
                   ` (6 subsequent siblings)
  10 siblings, 0 replies; 25+ messages in thread
From: Rafael J. Wysocki @ 2019-08-02 10:40 UTC (permalink / raw)
  To: Linux ACPI
  Cc: Linux PM, LKML, Zhang Rui, Rajneesh Bhardwaj, Andy Shevchenko,
	Mario Limonciello, Kai-Heng Feng

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

Since the ACPI SCI is set up for system wakeup before the "noirq"
suspend of devices, it is better to make suspend-to-idle follow
suspend-to-RAM (S3) and switch over the EC to polling during "noirq"
suspend (and back to interrupt-based flow during "noirq" resume).

The frequency of spurious wakeup interrupts from the EC may be
reduced this way.

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

In v2 this was patch 5.

---
 drivers/acpi/ec.c |    6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

Index: linux-pm/drivers/acpi/ec.c
===================================================================
--- linux-pm.orig/drivers/acpi/ec.c
+++ linux-pm/drivers/acpi/ec.c
@@ -1958,8 +1958,7 @@ static int acpi_ec_suspend_noirq(struct
 	    ec->reference_count >= 1)
 		acpi_set_gpe(NULL, ec->gpe, ACPI_GPE_DISABLE);
 
-	if (acpi_sleep_no_ec_events())
-		acpi_ec_enter_noirq(ec);
+	acpi_ec_enter_noirq(ec);
 
 	return 0;
 }
@@ -1968,8 +1967,7 @@ static int acpi_ec_resume_noirq(struct d
 {
 	struct acpi_ec *ec = acpi_driver_data(to_acpi_device(dev));
 
-	if (acpi_sleep_no_ec_events())
-		acpi_ec_leave_noirq(ec);
+	acpi_ec_leave_noirq(ec);
 
 	if (ec_no_wakeup && test_bit(EC_FLAGS_STARTED, &ec->flags) &&
 	    ec->reference_count >= 1)




^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH v3 5/8] ACPI: PM: s2idle: Eliminate acpi_sleep_no_ec_events()
  2019-08-02 10:33 [PATCH v3 0/8] PM / ACPI: sleep: Additional changes related to suspend-to-idle Rafael J. Wysocki
                   ` (3 preceding siblings ...)
  2019-08-02 10:40 ` [PATCH v3 4/8] ACPI: PM: s2idle: Switch EC over to polling during "noirq" suspend Rafael J. Wysocki
@ 2019-08-02 10:41 ` Rafael J. Wysocki
  2019-08-02 10:43 ` [PATCH v3 6/8] ACPI: EC: PM: Consolidate some code depending on PM_SLEEP Rafael J. Wysocki
                   ` (5 subsequent siblings)
  10 siblings, 0 replies; 25+ messages in thread
From: Rafael J. Wysocki @ 2019-08-02 10:41 UTC (permalink / raw)
  To: Linux ACPI
  Cc: Linux PM, LKML, Zhang Rui, Rajneesh Bhardwaj, Andy Shevchenko,
	Mario Limonciello, Kai-Heng Feng

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

Change acpi_ec_suspend() to use pm_suspend_no_platform() instead of
acpi_sleep_no_ec_events(), which allows the latter to be eliminated
along with the s2idle_in_progress variable which is only used by it.

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

In v2 this was patch 6.

---
 drivers/acpi/ec.c       |    2 +-
 drivers/acpi/internal.h |    2 --
 drivers/acpi/sleep.c    |    9 ---------
 3 files changed, 1 insertion(+), 12 deletions(-)

Index: linux-pm/drivers/acpi/ec.c
===================================================================
--- linux-pm.orig/drivers/acpi/ec.c
+++ linux-pm/drivers/acpi/ec.c
@@ -1941,7 +1941,7 @@ static int acpi_ec_suspend(struct device
 	struct acpi_ec *ec =
 		acpi_driver_data(to_acpi_device(dev));
 
-	if (acpi_sleep_no_ec_events() && ec_freeze_events)
+	if (!pm_suspend_no_platform() && ec_freeze_events)
 		acpi_ec_disable_event(ec);
 	return 0;
 }
Index: linux-pm/drivers/acpi/sleep.c
===================================================================
--- linux-pm.orig/drivers/acpi/sleep.c
+++ linux-pm/drivers/acpi/sleep.c
@@ -696,7 +696,6 @@ static const struct platform_suspend_ops
 	.recover = acpi_pm_finish,
 };
 
-static bool s2idle_in_progress;
 static bool s2idle_wakeup;
 
 /*
@@ -950,7 +949,6 @@ static struct acpi_scan_handler lps0_han
 static int acpi_s2idle_begin(void)
 {
 	acpi_scan_lock_acquire();
-	s2idle_in_progress = true;
 	return 0;
 }
 
@@ -1032,7 +1030,6 @@ static void acpi_s2idle_restore(void)
 
 static void acpi_s2idle_end(void)
 {
-	s2idle_in_progress = false;
 	acpi_scan_lock_release();
 }
 
@@ -1060,7 +1057,6 @@ static void acpi_sleep_suspend_setup(voi
 }
 
 #else /* !CONFIG_SUSPEND */
-#define s2idle_in_progress	(false)
 #define s2idle_wakeup		(false)
 #define lps0_device_handle	(NULL)
 static inline void acpi_sleep_suspend_setup(void) {}
@@ -1071,11 +1067,6 @@ bool acpi_s2idle_wakeup(void)
 	return s2idle_wakeup;
 }
 
-bool acpi_sleep_no_ec_events(void)
-{
-	return !s2idle_in_progress;
-}
-
 #ifdef CONFIG_PM_SLEEP
 static u32 saved_bm_rld;
 
Index: linux-pm/drivers/acpi/internal.h
===================================================================
--- linux-pm.orig/drivers/acpi/internal.h
+++ linux-pm/drivers/acpi/internal.h
@@ -210,11 +210,9 @@ void acpi_ec_flush_work(void);
   -------------------------------------------------------------------------- */
 #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
 




^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH v3 6/8] ACPI: EC: PM: Consolidate some code depending on PM_SLEEP
  2019-08-02 10:33 [PATCH v3 0/8] PM / ACPI: sleep: Additional changes related to suspend-to-idle Rafael J. Wysocki
                   ` (4 preceding siblings ...)
  2019-08-02 10:41 ` [PATCH v3 5/8] ACPI: PM: s2idle: Eliminate acpi_sleep_no_ec_events() Rafael J. Wysocki
@ 2019-08-02 10:43 ` Rafael J. Wysocki
  2019-08-02 10:44 ` [PATCH v3 7/8] ACPI: EC: PM: Make acpi_ec_dispatch_gpe() print debug message Rafael J. Wysocki
                   ` (4 subsequent siblings)
  10 siblings, 0 replies; 25+ messages in thread
From: Rafael J. Wysocki @ 2019-08-02 10:43 UTC (permalink / raw)
  To: Linux ACPI
  Cc: Linux PM, LKML, Zhang Rui, Rajneesh Bhardwaj, Andy Shevchenko,
	Mario Limonciello, Kai-Heng Feng

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

Move some routines, including acpi_ec_dispatch_gpe(), that are only
used if CONFIG_PM_SLEEP is set to the #ifdef block containing the EC
suspend and resume callbacks, to make the "full EC PM picture" easier
to follow.

While at it, move the header of acpi_ec_dispatch_gpe() in the
header file to a CONFIG_PM_SLEEP #ifdef block.

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

In v2 this was patch 7.

---
 drivers/acpi/ec.c       |   54 +++++++++++++++++++++++-------------------------
 drivers/acpi/internal.h |    2 -
 2 files changed, 27 insertions(+), 29 deletions(-)

Index: linux-pm/drivers/acpi/internal.h
===================================================================
--- linux-pm.orig/drivers/acpi/internal.h
+++ linux-pm/drivers/acpi/internal.h
@@ -194,7 +194,6 @@ void acpi_ec_ecdt_probe(void);
 void acpi_ec_dsdt_probe(void);
 void acpi_ec_block_transactions(void);
 void acpi_ec_unblock_transactions(void);
-bool acpi_ec_dispatch_gpe(void);
 int acpi_ec_add_query_handler(struct acpi_ec *ec, u8 query_bit,
 			      acpi_handle handle, acpi_ec_query_func func,
 			      void *data);
@@ -202,6 +201,7 @@ void acpi_ec_remove_query_handler(struct
 
 #ifdef CONFIG_PM_SLEEP
 void acpi_ec_flush_work(void);
+bool acpi_ec_dispatch_gpe(void);
 #endif
 
 
Index: linux-pm/drivers/acpi/ec.c
===================================================================
--- linux-pm.orig/drivers/acpi/ec.c
+++ linux-pm/drivers/acpi/ec.c
@@ -1049,33 +1049,6 @@ void acpi_ec_unblock_transactions(void)
 		acpi_ec_start(first_ec, true);
 }
 
-#ifdef CONFIG_PM_SLEEP
-void acpi_ec_mark_gpe_for_wake(void)
-{
-	if (first_ec && !ec_no_wakeup)
-		acpi_mark_gpe_for_wake(NULL, first_ec->gpe);
-}
-EXPORT_SYMBOL_GPL(acpi_ec_mark_gpe_for_wake);
-
-void acpi_ec_set_gpe_wake_mask(u8 action)
-{
-	if (pm_suspend_no_platform() && first_ec && !ec_no_wakeup)
-		acpi_set_gpe_wake_mask(NULL, first_ec->gpe, action);
-}
-EXPORT_SYMBOL_GPL(acpi_ec_set_gpe_wake_mask);
-#endif
-
-bool acpi_ec_dispatch_gpe(void)
-{
-	u32 ret;
-
-	if (!first_ec)
-		return false;
-
-	ret = acpi_dispatch_gpe(NULL, first_ec->gpe);
-	return ret == ACPI_INTERRUPT_HANDLED;
-}
-
 /* --------------------------------------------------------------------------
                                 Event Management
    -------------------------------------------------------------------------- */
@@ -1984,7 +1957,32 @@ static int acpi_ec_resume(struct device
 	acpi_ec_enable_event(ec);
 	return 0;
 }
-#endif
+
+void acpi_ec_mark_gpe_for_wake(void)
+{
+	if (first_ec && !ec_no_wakeup)
+		acpi_mark_gpe_for_wake(NULL, first_ec->gpe);
+}
+EXPORT_SYMBOL_GPL(acpi_ec_mark_gpe_for_wake);
+
+void acpi_ec_set_gpe_wake_mask(u8 action)
+{
+	if (pm_suspend_no_platform() && first_ec && !ec_no_wakeup)
+		acpi_set_gpe_wake_mask(NULL, first_ec->gpe, action);
+}
+EXPORT_SYMBOL_GPL(acpi_ec_set_gpe_wake_mask);
+
+bool acpi_ec_dispatch_gpe(void)
+{
+	u32 ret;
+
+	if (!first_ec)
+		return false;
+
+	ret = acpi_dispatch_gpe(NULL, first_ec->gpe);
+	return ret == ACPI_INTERRUPT_HANDLED;
+}
+#endif /* CONFIG_PM_SLEEP */
 
 static const struct dev_pm_ops acpi_ec_pm = {
 	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(acpi_ec_suspend_noirq, acpi_ec_resume_noirq)




^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH v3 7/8] ACPI: EC: PM: Make acpi_ec_dispatch_gpe() print debug message
  2019-08-02 10:33 [PATCH v3 0/8] PM / ACPI: sleep: Additional changes related to suspend-to-idle Rafael J. Wysocki
                   ` (5 preceding siblings ...)
  2019-08-02 10:43 ` [PATCH v3 6/8] ACPI: EC: PM: Consolidate some code depending on PM_SLEEP Rafael J. Wysocki
@ 2019-08-02 10:44 ` Rafael J. Wysocki
  2019-08-02 10:45 ` [PATCH v3 8/8] ACPI: PM: s2idle: Execute LPS0 _DSM functions with suspended devices Rafael J. Wysocki
                   ` (3 subsequent siblings)
  10 siblings, 0 replies; 25+ messages in thread
From: Rafael J. Wysocki @ 2019-08-02 10:44 UTC (permalink / raw)
  To: Linux ACPI
  Cc: Linux PM, LKML, Zhang Rui, Rajneesh Bhardwaj, Andy Shevchenko,
	Mario Limonciello, Kai-Heng Feng

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

Add a pm_pr_dbg() debug statement to acpi_ec_dispatch_gpe() to print
a message when the EC GPE has been dispatched (because its status
was set).

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

In v2 this was patch 8.

---
 drivers/acpi/ec.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Index: linux-pm/drivers/acpi/ec.c
===================================================================
--- linux-pm.orig/drivers/acpi/ec.c
+++ linux-pm/drivers/acpi/ec.c
@@ -1980,7 +1980,11 @@ bool acpi_ec_dispatch_gpe(void)
 		return false;
 
 	ret = acpi_dispatch_gpe(NULL, first_ec->gpe);
-	return ret == ACPI_INTERRUPT_HANDLED;
+	if (ret == ACPI_INTERRUPT_HANDLED) {
+		pm_pr_dbg("EC GPE dispatched\n");
+		return true;
+	}
+	return false;
 }
 #endif /* CONFIG_PM_SLEEP */
 




^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH v3 8/8] ACPI: PM: s2idle: Execute LPS0 _DSM functions with suspended devices
  2019-08-02 10:33 [PATCH v3 0/8] PM / ACPI: sleep: Additional changes related to suspend-to-idle Rafael J. Wysocki
                   ` (6 preceding siblings ...)
  2019-08-02 10:44 ` [PATCH v3 7/8] ACPI: EC: PM: Make acpi_ec_dispatch_gpe() print debug message Rafael J. Wysocki
@ 2019-08-02 10:45 ` Rafael J. Wysocki
       [not found]   ` <CGME20190809120052eucas1p11b56806662ef4f4efb82a152ad651481@eucas1p1.samsung.com>
  2019-08-05 16:25 ` [PATCH v3 0/8] PM / ACPI: sleep: Additional changes related to suspend-to-idle Kai-Heng Feng
                   ` (2 subsequent siblings)
  10 siblings, 1 reply; 25+ messages in thread
From: Rafael J. Wysocki @ 2019-08-02 10:45 UTC (permalink / raw)
  To: Linux ACPI
  Cc: Linux PM, LKML, Zhang Rui, Rajneesh Bhardwaj, Andy Shevchenko,
	Mario Limonciello, Kai-Heng Feng

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

According to Section 3.5 of the "Intel Low Power S0 Idle" document [1],
Function 5 of the LPS0 _DSM is expected to be invoked when the system
configuration matches the criteria for entering the target low-power
state of the platform.  In particular, this means that all devices
should be suspended and in low-power states already when that function
is invoked.

This is not the case currently, however, because Function 5 of the
LPS0 _DSM is invoked by it before the "noirq" phase of device suspend,
which means that some devices may not have been put into low-power
states yet at that point.  That is a consequence of the previous
design of the suspend-to-idle flow that allowed the "noirq" phase of
device suspend and the "noirq" phase of device resume to be carried
out for multiple times while "suspended" (if any spurious wakeup
events were detected) and the point of the LPS0 _DSM Function 5
invocation was chosen so as to call it (and LPS0 _DSM Function 6
analogously) once per suspend-resume cycle (regardless of how many
times the "noirq" phases of device suspend and resume were carried
out while "suspended").

Now that the suspend-to-idle flow has been redesigned to carry out
the "noirq" phases of device suspend and resume once in each cycle,
the code can be reordered to follow the specification that it is
based on more closely.

For this purpose, add ->prepare_late and ->restore_early platform
callbacks for suspend-to-idle, to be executed, respectively, after
the "noirq" phase of suspending devices and before the "noirq"
phase of resuming them and make ACPI use them for the invocation
of LPS0 _DSM functions as appropriate.

While at it, move the LPS0 entry requirements check to be made
before invoking Functions 3 and 5 of the LPS0 _DSM (also once
per cycle) as follows from the specification [1].

Link: https://uefi.org/sites/default/files/resources/Intel_ACPI_Low_Power_S0_Idle.pdf # [1]
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

In v2 this was patch 2.

---
 drivers/acpi/sleep.c    |   36 ++++++++++++++++++++++++------------
 include/linux/suspend.h |    2 ++
 kernel/power/suspend.c  |   12 +++++++++---
 3 files changed, 35 insertions(+), 15 deletions(-)

Index: linux-pm/drivers/acpi/sleep.c
===================================================================
--- linux-pm.orig/drivers/acpi/sleep.c
+++ linux-pm/drivers/acpi/sleep.c
@@ -954,11 +954,6 @@ static int acpi_s2idle_begin(void)
 
 static int acpi_s2idle_prepare(void)
 {
-	if (lps0_device_handle && !sleep_no_lps0) {
-		acpi_sleep_run_lps0_dsm(ACPI_LPS0_SCREEN_OFF);
-		acpi_sleep_run_lps0_dsm(ACPI_LPS0_ENTRY);
-	}
-
 	if (acpi_sci_irq_valid())
 		enable_irq_wake(acpi_sci_irq);
 
@@ -972,11 +967,22 @@ static int acpi_s2idle_prepare(void)
 	return 0;
 }
 
-static void acpi_s2idle_wake(void)
+static int acpi_s2idle_prepare_late(void)
 {
-	if (lps0_device_handle && !sleep_no_lps0 && pm_debug_messages_on)
+	if (!lps0_device_handle || sleep_no_lps0)
+		return 0;
+
+	if (pm_debug_messages_on)
 		lpi_check_constraints();
 
+	acpi_sleep_run_lps0_dsm(ACPI_LPS0_SCREEN_OFF);
+	acpi_sleep_run_lps0_dsm(ACPI_LPS0_ENTRY);
+
+	return 0;
+}
+
+static void acpi_s2idle_wake(void)
+{
 	/*
 	 * If IRQD_WAKEUP_ARMED is set for the SCI at this point, the SCI has
 	 * not triggered while suspended, so bail out.
@@ -1011,6 +1017,15 @@ static void acpi_s2idle_wake(void)
 	rearm_wake_irq(acpi_sci_irq);
 }
 
+static void acpi_s2idle_restore_early(void)
+{
+	if (!lps0_device_handle || sleep_no_lps0)
+		return;
+
+	acpi_sleep_run_lps0_dsm(ACPI_LPS0_EXIT);
+	acpi_sleep_run_lps0_dsm(ACPI_LPS0_SCREEN_ON);
+}
+
 static void acpi_s2idle_restore(void)
 {
 	s2idle_wakeup = false;
@@ -1021,11 +1036,6 @@ static void acpi_s2idle_restore(void)
 
 	if (acpi_sci_irq_valid())
 		disable_irq_wake(acpi_sci_irq);
-
-	if (lps0_device_handle && !sleep_no_lps0) {
-		acpi_sleep_run_lps0_dsm(ACPI_LPS0_EXIT);
-		acpi_sleep_run_lps0_dsm(ACPI_LPS0_SCREEN_ON);
-	}
 }
 
 static void acpi_s2idle_end(void)
@@ -1036,7 +1046,9 @@ static void acpi_s2idle_end(void)
 static const struct platform_s2idle_ops acpi_s2idle_ops = {
 	.begin = acpi_s2idle_begin,
 	.prepare = acpi_s2idle_prepare,
+	.prepare_late = acpi_s2idle_prepare_late,
 	.wake = acpi_s2idle_wake,
+	.restore_early = acpi_s2idle_restore_early,
 	.restore = acpi_s2idle_restore,
 	.end = acpi_s2idle_end,
 };
Index: linux-pm/kernel/power/suspend.c
===================================================================
--- linux-pm.orig/kernel/power/suspend.c
+++ linux-pm/kernel/power/suspend.c
@@ -253,13 +253,19 @@ static int platform_suspend_prepare_late
 
 static int platform_suspend_prepare_noirq(suspend_state_t state)
 {
-	return state != PM_SUSPEND_TO_IDLE && suspend_ops->prepare_late ?
-		suspend_ops->prepare_late() : 0;
+	if (state == PM_SUSPEND_TO_IDLE) {
+		if (s2idle_ops && s2idle_ops->prepare_late)
+			return s2idle_ops->prepare_late();
+	}
+	return suspend_ops->prepare_late ? suspend_ops->prepare_late() : 0;
 }
 
 static void platform_resume_noirq(suspend_state_t state)
 {
-	if (state != PM_SUSPEND_TO_IDLE && suspend_ops->wake)
+	if (state == PM_SUSPEND_TO_IDLE) {
+		if (s2idle_ops && s2idle_ops->restore_early)
+			s2idle_ops->restore_early();
+	} else if (suspend_ops->wake)
 		suspend_ops->wake();
 }
 
Index: linux-pm/include/linux/suspend.h
===================================================================
--- linux-pm.orig/include/linux/suspend.h
+++ linux-pm/include/linux/suspend.h
@@ -190,7 +190,9 @@ struct platform_suspend_ops {
 struct platform_s2idle_ops {
 	int (*begin)(void);
 	int (*prepare)(void);
+	int (*prepare_late)(void);
 	void (*wake)(void);
+	void (*restore_early)(void);
 	void (*restore)(void);
 	void (*end)(void);
 };




^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v3 0/8] PM / ACPI: sleep: Additional changes related to suspend-to-idle
  2019-08-02 10:33 [PATCH v3 0/8] PM / ACPI: sleep: Additional changes related to suspend-to-idle Rafael J. Wysocki
                   ` (7 preceding siblings ...)
  2019-08-02 10:45 ` [PATCH v3 8/8] ACPI: PM: s2idle: Execute LPS0 _DSM functions with suspended devices Rafael J. Wysocki
@ 2019-08-05 16:25 ` Kai-Heng Feng
  2019-08-12 13:55 ` Bhardwaj, Rajneesh
  2019-08-16 20:20 ` Kristian Klausen
  10 siblings, 0 replies; 25+ messages in thread
From: Kai-Heng Feng @ 2019-08-05 16:25 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Linux ACPI, Linux PM, LKML, Zhang Rui, Rajneesh Bhardwaj,
	Andy Shevchenko, Mario Limonciello

at 18:33, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:

> Hi All,
>
>>> On top of the "Simplify the suspend-to-idle control flow" patch series
>>> posted previously:
>>>
>>> https://lore.kernel.org/lkml/71085220.z6FKkvYQPX@kreacher/
>>>
>>> sanitize the suspend-to-idle flow even further.
>>>
>>> First off, decouple EC wakeup from the LPS0 _DSM processing (patch 1).
>>>
>>> Next, reorder the code to invoke LPS0 _DSM Functions 5 and 6 in the
>>> specification-compliant order with respect to suspending and resuming
>>> devices (patch 2).
>>>
>>> Finally, rearrange lps0_device_attach() (patch 3) and add a command line
>>> switch to prevent the LPS0 _DSM from being used.
>>
>> The v2 is because I found a (minor) bug in patch 1, decided to use a  
>> module
>> parameter instead of a kernel command line option in patch 4.  Also, there
>> are 4 new patches:
>>
>> Patch 5: Switch the EC over to polling during "noirq" suspend and back
>> during "noirq" resume.
>>
>> Patch 6: Eliminate acpi_sleep_no_ec_events().
>>
>> Patch 7: Consolidate some EC code depending on PM_SLEEP.
>>
>> Patch 8: Add EC GPE dispatching debug message.
>
> The v3 is just a rearranged v2 so as to move the post sensitive patch  
> (previous patch 2)
> to the end of the series.   [After applying the full series the code is  
> the same as before.]
>
> For easier testing, the series (along with some previous patches depended  
> on by it)
> is available in the pm-s2idle-testing branch of the linux-pm.git tree at  
> kernel.org:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git/log/?h=pm-s2idle-testing

I’ve just tested the full series on Latitude 5300, and the additional  
spurious wake up is gone.

Tested-by: Kai-Heng Feng <kai.heng.feng@canonical.com>

>
> Please refer to the changelogs for details.
>
> Thanks,
> Rafael



^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v3 8/8] ACPI: PM: s2idle: Execute LPS0 _DSM functions with suspended devices
       [not found]   ` <CGME20190809120052eucas1p11b56806662ef4f4efb82a152ad651481@eucas1p1.samsung.com>
@ 2019-08-09 12:00     ` Marek Szyprowski
  2019-08-09 12:15       ` Rafael J. Wysocki
  2019-08-10 11:24       ` [PATCH] PM: suspend: Fix platform_suspend_prepare_noirq() Rafael J. Wysocki
  0 siblings, 2 replies; 25+ messages in thread
From: Marek Szyprowski @ 2019-08-09 12:00 UTC (permalink / raw)
  To: Rafael J. Wysocki, Linux ACPI
  Cc: Linux PM, LKML, Zhang Rui, Rajneesh Bhardwaj, Andy Shevchenko,
	Mario Limonciello, Kai-Heng Feng

Hi Rafael,

On 2019-08-02 12:45, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> According to Section 3.5 of the "Intel Low Power S0 Idle" document [1],
> Function 5 of the LPS0 _DSM is expected to be invoked when the system
> configuration matches the criteria for entering the target low-power
> state of the platform.  In particular, this means that all devices
> should be suspended and in low-power states already when that function
> is invoked.
>
> This is not the case currently, however, because Function 5 of the
> LPS0 _DSM is invoked by it before the "noirq" phase of device suspend,
> which means that some devices may not have been put into low-power
> states yet at that point.  That is a consequence of the previous
> design of the suspend-to-idle flow that allowed the "noirq" phase of
> device suspend and the "noirq" phase of device resume to be carried
> out for multiple times while "suspended" (if any spurious wakeup
> events were detected) and the point of the LPS0 _DSM Function 5
> invocation was chosen so as to call it (and LPS0 _DSM Function 6
> analogously) once per suspend-resume cycle (regardless of how many
> times the "noirq" phases of device suspend and resume were carried
> out while "suspended").
>
> Now that the suspend-to-idle flow has been redesigned to carry out
> the "noirq" phases of device suspend and resume once in each cycle,
> the code can be reordered to follow the specification that it is
> based on more closely.
>
> For this purpose, add ->prepare_late and ->restore_early platform
> callbacks for suspend-to-idle, to be executed, respectively, after
> the "noirq" phase of suspending devices and before the "noirq"
> phase of resuming them and make ACPI use them for the invocation
> of LPS0 _DSM functions as appropriate.
>
> While at it, move the LPS0 entry requirements check to be made
> before invoking Functions 3 and 5 of the LPS0 _DSM (also once
> per cycle) as follows from the specification [1].
>
> Link: https://uefi.org/sites/default/files/resources/Intel_ACPI_Low_Power_S0_Idle.pdf # [1]
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>
> In v2 this was patch 2.
>
> ---
>   drivers/acpi/sleep.c    |   36 ++++++++++++++++++++++++------------
>   include/linux/suspend.h |    2 ++
>   kernel/power/suspend.c  |   12 +++++++++---
>   3 files changed, 35 insertions(+), 15 deletions(-)
>
> Index: linux-pm/drivers/acpi/sleep.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/sleep.c
> +++ linux-pm/drivers/acpi/sleep.c
> @@ -954,11 +954,6 @@ static int acpi_s2idle_begin(void)
>   
>   static int acpi_s2idle_prepare(void)
>   {
> -	if (lps0_device_handle && !sleep_no_lps0) {
> -		acpi_sleep_run_lps0_dsm(ACPI_LPS0_SCREEN_OFF);
> -		acpi_sleep_run_lps0_dsm(ACPI_LPS0_ENTRY);
> -	}
> -
>   	if (acpi_sci_irq_valid())
>   		enable_irq_wake(acpi_sci_irq);
>   
> @@ -972,11 +967,22 @@ static int acpi_s2idle_prepare(void)
>   	return 0;
>   }
>   
> -static void acpi_s2idle_wake(void)
> +static int acpi_s2idle_prepare_late(void)
>   {
> -	if (lps0_device_handle && !sleep_no_lps0 && pm_debug_messages_on)
> +	if (!lps0_device_handle || sleep_no_lps0)
> +		return 0;
> +
> +	if (pm_debug_messages_on)
>   		lpi_check_constraints();
>   
> +	acpi_sleep_run_lps0_dsm(ACPI_LPS0_SCREEN_OFF);
> +	acpi_sleep_run_lps0_dsm(ACPI_LPS0_ENTRY);
> +
> +	return 0;
> +}
> +
> +static void acpi_s2idle_wake(void)
> +{
>   	/*
>   	 * If IRQD_WAKEUP_ARMED is set for the SCI at this point, the SCI has
>   	 * not triggered while suspended, so bail out.
> @@ -1011,6 +1017,15 @@ static void acpi_s2idle_wake(void)
>   	rearm_wake_irq(acpi_sci_irq);
>   }
>   
> +static void acpi_s2idle_restore_early(void)
> +{
> +	if (!lps0_device_handle || sleep_no_lps0)
> +		return;
> +
> +	acpi_sleep_run_lps0_dsm(ACPI_LPS0_EXIT);
> +	acpi_sleep_run_lps0_dsm(ACPI_LPS0_SCREEN_ON);
> +}
> +
>   static void acpi_s2idle_restore(void)
>   {
>   	s2idle_wakeup = false;
> @@ -1021,11 +1036,6 @@ static void acpi_s2idle_restore(void)
>   
>   	if (acpi_sci_irq_valid())
>   		disable_irq_wake(acpi_sci_irq);
> -
> -	if (lps0_device_handle && !sleep_no_lps0) {
> -		acpi_sleep_run_lps0_dsm(ACPI_LPS0_EXIT);
> -		acpi_sleep_run_lps0_dsm(ACPI_LPS0_SCREEN_ON);
> -	}
>   }
>   
>   static void acpi_s2idle_end(void)
> @@ -1036,7 +1046,9 @@ static void acpi_s2idle_end(void)
>   static const struct platform_s2idle_ops acpi_s2idle_ops = {
>   	.begin = acpi_s2idle_begin,
>   	.prepare = acpi_s2idle_prepare,
> +	.prepare_late = acpi_s2idle_prepare_late,
>   	.wake = acpi_s2idle_wake,
> +	.restore_early = acpi_s2idle_restore_early,
>   	.restore = acpi_s2idle_restore,
>   	.end = acpi_s2idle_end,
>   };
> Index: linux-pm/kernel/power/suspend.c
> ===================================================================
> --- linux-pm.orig/kernel/power/suspend.c
> +++ linux-pm/kernel/power/suspend.c
> @@ -253,13 +253,19 @@ static int platform_suspend_prepare_late
>   
>   static int platform_suspend_prepare_noirq(suspend_state_t state)
>   {
> -	return state != PM_SUSPEND_TO_IDLE && suspend_ops->prepare_late ?
> -		suspend_ops->prepare_late() : 0;
> +	if (state == PM_SUSPEND_TO_IDLE) {
> +		if (s2idle_ops && s2idle_ops->prepare_late)
> +			return s2idle_ops->prepare_late();
> +	}
> +	return suspend_ops->prepare_late ? suspend_ops->prepare_late() : 0;

This unconditionally references suspend_ops here, what wasn't done 
earlier. On one of my test boards (OdroidXU) it causes following NULL 
pointer dereference since Linux next-20190809 (the first -next, which 
contains this patch):

root@target:~# time rtcwake -s10 -mmem
rtcwake: wakeup from "mem" using /dev/rtc0 at Fri Aug  9 12:21:43 2019
PM: suspend entry (s2idle)
Filesystems sync: 0.001 seconds
Freezing user space processes ... (elapsed 0.002 seconds) done.
OOM killer disabled.
Freezing remaining freezable tasks ... (elapsed 0.001 seconds) done.
smsc95xx 1-2:1.0 eth0: entering SUSPEND2 mode
wake enabled for irq 111
usb3503 4-0008: switched to STANDBY mode
samsung-pinctrl 13400000.pinctrl: No retention data configured bank with 
external wakeup interrupt. Wake-up mask will not be set.
8<--- cut here ---
Unable to handle kernel NULL pointer dereference at virtual address 0000000c
pgd = ac9cf656
[0000000c] *pgd=00000000
Internal error: Oops: 5 [#1] PREEMPT SMP ARM
Modules linked in:
CPU: 1 PID: 2027 Comm: rtcwake Not tainted 5.3.0-rc3-next-20190809 #6373
Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
PC is at suspend_devices_and_enter+0x330/0xf2c
LR is at lock_is_held_type+0x80/0x108
pc : [<c0195b78>]    lr : [<c01883dc>]    psr: 60000013
sp : e73dfe20  ip : 00000001  fp : 00000001
r10: c10b9394  r9 : c10ce36c  r8 : c1007648
r7 : c16d26c8  r6 : 00000001  r5 : 00000000  r4 : c16d26dc
r3 : 00000000  r2 : 00000000  r1 : c1014220  r0 : 00000000
Flags: nZCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
Control: 10c5387d  Table: 671ac06a  DAC: 00000051
Process rtcwake (pid: 2027, stack limit = 0x59af371a)
Stack: (0xe73dfe20 to 0xe73e0000)
...
[<c0195b78>] (suspend_devices_and_enter) from [<c0196d0c>] 
(pm_suspend+0x598/0xcb8)
[<c0196d0c>] (pm_suspend) from [<c0194848>] (state_store+0x6c/0xc8)
[<c0194848>] (state_store) from [<c0330100>] (kernfs_fop_write+0x100/0x1e0)
[<c0330100>] (kernfs_fop_write) from [<c02a1594>] (__vfs_write+0x30/0x1d0)
[<c02a1594>] (__vfs_write) from [<c02a4090>] (vfs_write+0xa4/0x180)
[<c02a4090>] (vfs_write) from [<c02a42f0>] (ksys_write+0x60/0xdc)
[<c02a42f0>] (ksys_write) from [<c0101000>] (ret_fast_syscall+0x0/0x28)
Exception stack(0xe73dffa8 to 0xe73dfff0)
...
---[ end trace 627069b7dfd482c9 ]---

>   }
>   
>   static void platform_resume_noirq(suspend_state_t state)
>   {
> -	if (state != PM_SUSPEND_TO_IDLE && suspend_ops->wake)
> +	if (state == PM_SUSPEND_TO_IDLE) {
> +		if (s2idle_ops && s2idle_ops->restore_early)
> +			s2idle_ops->restore_early();
> +	} else if (suspend_ops->wake)
>   		suspend_ops->wake();
>   }
>   
> Index: linux-pm/include/linux/suspend.h
> ===================================================================
> --- linux-pm.orig/include/linux/suspend.h
> +++ linux-pm/include/linux/suspend.h
> @@ -190,7 +190,9 @@ struct platform_suspend_ops {
>   struct platform_s2idle_ops {
>   	int (*begin)(void);
>   	int (*prepare)(void);
> +	int (*prepare_late)(void);
>   	void (*wake)(void);
> +	void (*restore_early)(void);
>   	void (*restore)(void);
>   	void (*end)(void);
>   };

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v3 8/8] ACPI: PM: s2idle: Execute LPS0 _DSM functions with suspended devices
  2019-08-09 12:00     ` Marek Szyprowski
@ 2019-08-09 12:15       ` Rafael J. Wysocki
  2019-08-10 11:24       ` [PATCH] PM: suspend: Fix platform_suspend_prepare_noirq() Rafael J. Wysocki
  1 sibling, 0 replies; 25+ messages in thread
From: Rafael J. Wysocki @ 2019-08-09 12:15 UTC (permalink / raw)
  To: Marek Szyprowski
  Cc: Rafael J. Wysocki, Linux ACPI, Linux PM, LKML, Zhang Rui,
	Rajneesh Bhardwaj, Andy Shevchenko, Mario Limonciello,
	Kai-Heng Feng

On Fri, Aug 9, 2019 at 2:00 PM Marek Szyprowski
<m.szyprowski@samsung.com> wrote:
>
> Hi Rafael,
>
> On 2019-08-02 12:45, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > According to Section 3.5 of the "Intel Low Power S0 Idle" document [1],
> > Function 5 of the LPS0 _DSM is expected to be invoked when the system
> > configuration matches the criteria for entering the target low-power
> > state of the platform.  In particular, this means that all devices
> > should be suspended and in low-power states already when that function
> > is invoked.
> >
> > This is not the case currently, however, because Function 5 of the
> > LPS0 _DSM is invoked by it before the "noirq" phase of device suspend,
> > which means that some devices may not have been put into low-power
> > states yet at that point.  That is a consequence of the previous
> > design of the suspend-to-idle flow that allowed the "noirq" phase of
> > device suspend and the "noirq" phase of device resume to be carried
> > out for multiple times while "suspended" (if any spurious wakeup
> > events were detected) and the point of the LPS0 _DSM Function 5
> > invocation was chosen so as to call it (and LPS0 _DSM Function 6
> > analogously) once per suspend-resume cycle (regardless of how many
> > times the "noirq" phases of device suspend and resume were carried
> > out while "suspended").
> >
> > Now that the suspend-to-idle flow has been redesigned to carry out
> > the "noirq" phases of device suspend and resume once in each cycle,
> > the code can be reordered to follow the specification that it is
> > based on more closely.
> >
> > For this purpose, add ->prepare_late and ->restore_early platform
> > callbacks for suspend-to-idle, to be executed, respectively, after
> > the "noirq" phase of suspending devices and before the "noirq"
> > phase of resuming them and make ACPI use them for the invocation
> > of LPS0 _DSM functions as appropriate.
> >
> > While at it, move the LPS0 entry requirements check to be made
> > before invoking Functions 3 and 5 of the LPS0 _DSM (also once
> > per cycle) as follows from the specification [1].
> >
> > Link: https://uefi.org/sites/default/files/resources/Intel_ACPI_Low_Power_S0_Idle.pdf # [1]
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >
> > In v2 this was patch 2.
> >
> > ---
> >   drivers/acpi/sleep.c    |   36 ++++++++++++++++++++++++------------
> >   include/linux/suspend.h |    2 ++
> >   kernel/power/suspend.c  |   12 +++++++++---
> >   3 files changed, 35 insertions(+), 15 deletions(-)
> >
> > Index: linux-pm/drivers/acpi/sleep.c
> > ===================================================================
> > --- linux-pm.orig/drivers/acpi/sleep.c
> > +++ linux-pm/drivers/acpi/sleep.c
> > @@ -954,11 +954,6 @@ static int acpi_s2idle_begin(void)
> >
> >   static int acpi_s2idle_prepare(void)
> >   {
> > -     if (lps0_device_handle && !sleep_no_lps0) {
> > -             acpi_sleep_run_lps0_dsm(ACPI_LPS0_SCREEN_OFF);
> > -             acpi_sleep_run_lps0_dsm(ACPI_LPS0_ENTRY);
> > -     }
> > -
> >       if (acpi_sci_irq_valid())
> >               enable_irq_wake(acpi_sci_irq);
> >
> > @@ -972,11 +967,22 @@ static int acpi_s2idle_prepare(void)
> >       return 0;
> >   }
> >
> > -static void acpi_s2idle_wake(void)
> > +static int acpi_s2idle_prepare_late(void)
> >   {
> > -     if (lps0_device_handle && !sleep_no_lps0 && pm_debug_messages_on)
> > +     if (!lps0_device_handle || sleep_no_lps0)
> > +             return 0;
> > +
> > +     if (pm_debug_messages_on)
> >               lpi_check_constraints();
> >
> > +     acpi_sleep_run_lps0_dsm(ACPI_LPS0_SCREEN_OFF);
> > +     acpi_sleep_run_lps0_dsm(ACPI_LPS0_ENTRY);
> > +
> > +     return 0;
> > +}
> > +
> > +static void acpi_s2idle_wake(void)
> > +{
> >       /*
> >        * If IRQD_WAKEUP_ARMED is set for the SCI at this point, the SCI has
> >        * not triggered while suspended, so bail out.
> > @@ -1011,6 +1017,15 @@ static void acpi_s2idle_wake(void)
> >       rearm_wake_irq(acpi_sci_irq);
> >   }
> >
> > +static void acpi_s2idle_restore_early(void)
> > +{
> > +     if (!lps0_device_handle || sleep_no_lps0)
> > +             return;
> > +
> > +     acpi_sleep_run_lps0_dsm(ACPI_LPS0_EXIT);
> > +     acpi_sleep_run_lps0_dsm(ACPI_LPS0_SCREEN_ON);
> > +}
> > +
> >   static void acpi_s2idle_restore(void)
> >   {
> >       s2idle_wakeup = false;
> > @@ -1021,11 +1036,6 @@ static void acpi_s2idle_restore(void)
> >
> >       if (acpi_sci_irq_valid())
> >               disable_irq_wake(acpi_sci_irq);
> > -
> > -     if (lps0_device_handle && !sleep_no_lps0) {
> > -             acpi_sleep_run_lps0_dsm(ACPI_LPS0_EXIT);
> > -             acpi_sleep_run_lps0_dsm(ACPI_LPS0_SCREEN_ON);
> > -     }
> >   }
> >
> >   static void acpi_s2idle_end(void)
> > @@ -1036,7 +1046,9 @@ static void acpi_s2idle_end(void)
> >   static const struct platform_s2idle_ops acpi_s2idle_ops = {
> >       .begin = acpi_s2idle_begin,
> >       .prepare = acpi_s2idle_prepare,
> > +     .prepare_late = acpi_s2idle_prepare_late,
> >       .wake = acpi_s2idle_wake,
> > +     .restore_early = acpi_s2idle_restore_early,
> >       .restore = acpi_s2idle_restore,
> >       .end = acpi_s2idle_end,
> >   };
> > Index: linux-pm/kernel/power/suspend.c
> > ===================================================================
> > --- linux-pm.orig/kernel/power/suspend.c
> > +++ linux-pm/kernel/power/suspend.c
> > @@ -253,13 +253,19 @@ static int platform_suspend_prepare_late
> >
> >   static int platform_suspend_prepare_noirq(suspend_state_t state)
> >   {
> > -     return state != PM_SUSPEND_TO_IDLE && suspend_ops->prepare_late ?
> > -             suspend_ops->prepare_late() : 0;
> > +     if (state == PM_SUSPEND_TO_IDLE) {
> > +             if (s2idle_ops && s2idle_ops->prepare_late)
> > +                     return s2idle_ops->prepare_late();

This should be

return s2idle_ops && s2idle_ops->prepare_late ? s2idle_ops->prepare_late() : 0;

> > +     }
> > +     return suspend_ops->prepare_late ? suspend_ops->prepare_late() : 0;
>
> This unconditionally references suspend_ops here, what wasn't done
> earlier. On one of my test boards (OdroidXU) it causes following NULL
> pointer dereference since Linux next-20190809 (the first -next, which
> contains this patch):

Sorry about this, will fix early next week.

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH] PM: suspend: Fix platform_suspend_prepare_noirq()
  2019-08-09 12:00     ` Marek Szyprowski
  2019-08-09 12:15       ` Rafael J. Wysocki
@ 2019-08-10 11:24       ` Rafael J. Wysocki
  1 sibling, 0 replies; 25+ messages in thread
From: Rafael J. Wysocki @ 2019-08-10 11:24 UTC (permalink / raw)
  To: Linux PM
  Cc: Marek Szyprowski, Linux ACPI, LKML, Zhang Rui, Rajneesh Bhardwaj,
	Andy Shevchenko, Mario Limonciello, Kai-Heng Feng

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

After commit ac9eafbe930a ("ACPI: PM: s2idle: Execute LPS0 _DSM
functions with suspended devices"), a NULL pointer may be dereferenced
if suspend-to-idle is attempted on a platform without "traditional"
suspend support due to invalid fall-through in
platform_suspend_prepare_noirq().

Fix that and while at it add missing braces in platform_resume_noirq().

Fixes: ac9eafbe930a ("ACPI: PM: s2idle: Execute LPS0 _DSM functions with suspended devices")
Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

Commit ac9eafbe930a is in linux-next only at this point.

---
 kernel/power/suspend.c |   11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

Index: linux-pm/kernel/power/suspend.c
===================================================================
--- linux-pm.orig/kernel/power/suspend.c
+++ linux-pm/kernel/power/suspend.c
@@ -253,10 +253,10 @@ static int platform_suspend_prepare_late
 
 static int platform_suspend_prepare_noirq(suspend_state_t state)
 {
-	if (state == PM_SUSPEND_TO_IDLE) {
-		if (s2idle_ops && s2idle_ops->prepare_late)
-			return s2idle_ops->prepare_late();
-	}
+	if (state == PM_SUSPEND_TO_IDLE)
+		return s2idle_ops && s2idle_ops->prepare_late ?
+			s2idle_ops->prepare_late() : 0;
+
 	return suspend_ops->prepare_late ? suspend_ops->prepare_late() : 0;
 }
 
@@ -265,8 +265,9 @@ static void platform_resume_noirq(suspen
 	if (state == PM_SUSPEND_TO_IDLE) {
 		if (s2idle_ops && s2idle_ops->restore_early)
 			s2idle_ops->restore_early();
-	} else if (suspend_ops->wake)
+	} else if (suspend_ops->wake) {
 		suspend_ops->wake();
+	}
 }
 
 static void platform_resume_early(suspend_state_t state)




^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v3 0/8] PM / ACPI: sleep: Additional changes related to suspend-to-idle
  2019-08-02 10:33 [PATCH v3 0/8] PM / ACPI: sleep: Additional changes related to suspend-to-idle Rafael J. Wysocki
                   ` (8 preceding siblings ...)
  2019-08-05 16:25 ` [PATCH v3 0/8] PM / ACPI: sleep: Additional changes related to suspend-to-idle Kai-Heng Feng
@ 2019-08-12 13:55 ` Bhardwaj, Rajneesh
  2019-08-12 21:32   ` Rafael J. Wysocki
  2019-08-16 20:20 ` Kristian Klausen
  10 siblings, 1 reply; 25+ messages in thread
From: Bhardwaj, Rajneesh @ 2019-08-12 13:55 UTC (permalink / raw)
  To: Rafael J. Wysocki, Linux ACPI
  Cc: Linux PM, LKML, Zhang Rui, Andy Shevchenko, Mario Limonciello,
	Kai-Heng Feng

Hi Rafael

On 02-Aug-19 4:03 PM, Rafael J. Wysocki wrote:
> Hi All,
>
>>> On top of the "Simplify the suspend-to-idle control flow" patch series
>>> posted previously:
>>>
>>> https://lore.kernel.org/lkml/71085220.z6FKkvYQPX@kreacher/
>>>
>>> sanitize the suspend-to-idle flow even further.
>>>
>>> First off, decouple EC wakeup from the LPS0 _DSM processing (patch 1).
>>>
>>> Next, reorder the code to invoke LPS0 _DSM Functions 5 and 6 in the
>>> specification-compliant order with respect to suspending and resuming
>>> devices (patch 2).
>>>
>>> Finally, rearrange lps0_device_attach() (patch 3) and add a command line
>>> switch to prevent the LPS0 _DSM from being used.
>> The v2 is because I found a (minor) bug in patch 1, decided to use a module
>> parameter instead of a kernel command line option in patch 4.  Also, there
>> are 4 new patches:
>>
>> Patch 5: Switch the EC over to polling during "noirq" suspend and back
>> during "noirq" resume.
>>
>> Patch 6: Eliminate acpi_sleep_no_ec_events().
>>
>> Patch 7: Consolidate some EC code depending on PM_SLEEP.
>>
>> Patch 8: Add EC GPE dispatching debug message.
> The v3 is just a rearranged v2 so as to move the post sensitive patch (previous patch 2)
> to the end of the series.   [After applying the full series the code is the same as before.]
>
> For easier testing, the series (along with some previous patches depended on by it)
> is available in the pm-s2idle-testing branch of the linux-pm.git tree at kernel.org:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git/log/?h=pm-s2idle-testing
>
> Please refer to the changelogs for details.


I have tested both pm-s2idle-testing and pm-s2idle-rework branches 
including recently introduced commit "PM: suspend: Fix 
platform_suspend_prepare_noirq()".

Works fine for me on Ice Lake platform.

  Acked-by: Rajneesh Bhardwaj <rajneesh.bhardwaj@linux.intel.com>

Tested-by: Rajneesh Bhardwaj <rajneesh.bhardwaj@linux.intel.com>


> Thanks,
> Rafael
>
>
>

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v3 0/8] PM / ACPI: sleep: Additional changes related to suspend-to-idle
  2019-08-12 13:55 ` Bhardwaj, Rajneesh
@ 2019-08-12 21:32   ` Rafael J. Wysocki
  0 siblings, 0 replies; 25+ messages in thread
From: Rafael J. Wysocki @ 2019-08-12 21:32 UTC (permalink / raw)
  To: Bhardwaj, Rajneesh
  Cc: Rafael J. Wysocki, Linux ACPI, Linux PM, LKML, Zhang Rui,
	Andy Shevchenko, Mario Limonciello, Kai-Heng Feng

On Mon, Aug 12, 2019 at 3:55 PM Bhardwaj, Rajneesh
<rajneesh.bhardwaj@linux.intel.com> wrote:
>
> Hi Rafael
>
> On 02-Aug-19 4:03 PM, Rafael J. Wysocki wrote:
> > Hi All,
> >
> >>> On top of the "Simplify the suspend-to-idle control flow" patch series
> >>> posted previously:
> >>>
> >>> https://lore.kernel.org/lkml/71085220.z6FKkvYQPX@kreacher/
> >>>
> >>> sanitize the suspend-to-idle flow even further.
> >>>
> >>> First off, decouple EC wakeup from the LPS0 _DSM processing (patch 1).
> >>>
> >>> Next, reorder the code to invoke LPS0 _DSM Functions 5 and 6 in the
> >>> specification-compliant order with respect to suspending and resuming
> >>> devices (patch 2).
> >>>
> >>> Finally, rearrange lps0_device_attach() (patch 3) and add a command line
> >>> switch to prevent the LPS0 _DSM from being used.
> >> The v2 is because I found a (minor) bug in patch 1, decided to use a module
> >> parameter instead of a kernel command line option in patch 4.  Also, there
> >> are 4 new patches:
> >>
> >> Patch 5: Switch the EC over to polling during "noirq" suspend and back
> >> during "noirq" resume.
> >>
> >> Patch 6: Eliminate acpi_sleep_no_ec_events().
> >>
> >> Patch 7: Consolidate some EC code depending on PM_SLEEP.
> >>
> >> Patch 8: Add EC GPE dispatching debug message.
> > The v3 is just a rearranged v2 so as to move the post sensitive patch (previous patch 2)
> > to the end of the series.   [After applying the full series the code is the same as before.]
> >
> > For easier testing, the series (along with some previous patches depended on by it)
> > is available in the pm-s2idle-testing branch of the linux-pm.git tree at kernel.org:
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git/log/?h=pm-s2idle-testing
> >
> > Please refer to the changelogs for details.
>
>
> I have tested both pm-s2idle-testing and pm-s2idle-rework branches
> including recently introduced commit "PM: suspend: Fix
> platform_suspend_prepare_noirq()".
>
> Works fine for me on Ice Lake platform.
>
>   Acked-by: Rajneesh Bhardwaj <rajneesh.bhardwaj@linux.intel.com>
>
> Tested-by: Rajneesh Bhardwaj <rajneesh.bhardwaj@linux.intel.com>

Thanks!

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v3 0/8] PM / ACPI: sleep: Additional changes related to suspend-to-idle
  2019-08-02 10:33 [PATCH v3 0/8] PM / ACPI: sleep: Additional changes related to suspend-to-idle Rafael J. Wysocki
                   ` (9 preceding siblings ...)
  2019-08-12 13:55 ` Bhardwaj, Rajneesh
@ 2019-08-16 20:20 ` Kristian Klausen
  2019-08-19  7:59   ` Rafael J. Wysocki
  10 siblings, 1 reply; 25+ messages in thread
From: Kristian Klausen @ 2019-08-16 20:20 UTC (permalink / raw)
  To: Rafael J. Wysocki, Linux ACPI
  Cc: Linux PM, LKML, Zhang Rui, Rajneesh Bhardwaj, Andy Shevchenko,
	Mario Limonciello, Kai-Heng Feng

On 02.08.2019 12.33, Rafael J. Wysocki wrote:
> Hi All,
>
>>> On top of the "Simplify the suspend-to-idle control flow" patch series
>>> posted previously:
>>>
>>> https://lore.kernel.org/lkml/71085220.z6FKkvYQPX@kreacher/
>>>
>>> sanitize the suspend-to-idle flow even further.
>>>
>>> First off, decouple EC wakeup from the LPS0 _DSM processing (patch 1).
>>>
>>> Next, reorder the code to invoke LPS0 _DSM Functions 5 and 6 in the
>>> specification-compliant order with respect to suspending and resuming
>>> devices (patch 2).
>>>
>>> Finally, rearrange lps0_device_attach() (patch 3) and add a command line
>>> switch to prevent the LPS0 _DSM from being used.
>> The v2 is because I found a (minor) bug in patch 1, decided to use a module
>> parameter instead of a kernel command line option in patch 4.  Also, there
>> are 4 new patches:
>>
>> Patch 5: Switch the EC over to polling during "noirq" suspend and back
>> during "noirq" resume.
>>
>> Patch 6: Eliminate acpi_sleep_no_ec_events().
>>
>> Patch 7: Consolidate some EC code depending on PM_SLEEP.
>>
>> Patch 8: Add EC GPE dispatching debug message.
> The v3 is just a rearranged v2 so as to move the post sensitive patch (previous patch 2)
> to the end of the series.   [After applying the full series the code is the same as before.]
>
> For easier testing, the series (along with some previous patches depended on by it)
> is available in the pm-s2idle-testing branch of the linux-pm.git tree at kernel.org:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git/log/?h=pm-s2idle-testing
It was just testing this patch series(461fc1caed55), to see if it would 
fix my charging issue 
(https://bugzilla.kernel.org/show_bug.cgi?id=201307), which it didn't.

I did however notice that my laptop (ASUS Zenbook UX430UNR/i7-8550U) 
won't wake when opening the lid or pressing a key, the only way to wake 
the laptop is pressing the power button.

I also tested mainline (5.3.0-rc4 b7e7c85dc7b0) and 5.2.8 and the laptop 
wakes without issue when the lid is opened or a key is presed.
> Please refer to the changelogs for details.
>
> Thanks,
> Rafael
>
>
>


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v3 0/8] PM / ACPI: sleep: Additional changes related to suspend-to-idle
  2019-08-16 20:20 ` Kristian Klausen
@ 2019-08-19  7:59   ` Rafael J. Wysocki
  2019-08-19  9:05     ` Rafael J. Wysocki
  0 siblings, 1 reply; 25+ messages in thread
From: Rafael J. Wysocki @ 2019-08-19  7:59 UTC (permalink / raw)
  To: Kristian Klausen
  Cc: Rafael J. Wysocki, Linux ACPI, Linux PM, LKML, Zhang Rui,
	Rajneesh Bhardwaj, Andy Shevchenko, Mario Limonciello,
	Kai-Heng Feng

On Fri, Aug 16, 2019 at 10:26 PM Kristian Klausen <kristian@klausen.dk> wrote:
>
> On 02.08.2019 12.33, Rafael J. Wysocki wrote:
> > Hi All,
> >
> >>> On top of the "Simplify the suspend-to-idle control flow" patch series
> >>> posted previously:
> >>>
> >>> https://lore.kernel.org/lkml/71085220.z6FKkvYQPX@kreacher/
> >>>
> >>> sanitize the suspend-to-idle flow even further.
> >>>
> >>> First off, decouple EC wakeup from the LPS0 _DSM processing (patch 1).
> >>>
> >>> Next, reorder the code to invoke LPS0 _DSM Functions 5 and 6 in the
> >>> specification-compliant order with respect to suspending and resuming
> >>> devices (patch 2).
> >>>
> >>> Finally, rearrange lps0_device_attach() (patch 3) and add a command line
> >>> switch to prevent the LPS0 _DSM from being used.
> >> The v2 is because I found a (minor) bug in patch 1, decided to use a module
> >> parameter instead of a kernel command line option in patch 4.  Also, there
> >> are 4 new patches:
> >>
> >> Patch 5: Switch the EC over to polling during "noirq" suspend and back
> >> during "noirq" resume.
> >>
> >> Patch 6: Eliminate acpi_sleep_no_ec_events().
> >>
> >> Patch 7: Consolidate some EC code depending on PM_SLEEP.
> >>
> >> Patch 8: Add EC GPE dispatching debug message.
> > The v3 is just a rearranged v2 so as to move the post sensitive patch (previous patch 2)
> > to the end of the series.   [After applying the full series the code is the same as before.]
> >
> > For easier testing, the series (along with some previous patches depended on by it)
> > is available in the pm-s2idle-testing branch of the linux-pm.git tree at kernel.org:
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git/log/?h=pm-s2idle-testing
> It was just testing this patch series(461fc1caed55), to see if it would
> fix my charging issue
> (https://bugzilla.kernel.org/show_bug.cgi?id=201307), which it didn't.

It is unlikely to help in that case.

> I did however notice that my laptop (ASUS Zenbook UX430UNR/i7-8550U)
> won't wake when opening the lid or pressing a key, the only way to wake
> the laptop is pressing the power button.
>
> I also tested mainline (5.3.0-rc4 b7e7c85dc7b0) and 5.2.8 and the laptop
> wakes without issue when the lid is opened or a key is presed.
> > Please refer to the changelogs for details.

Thanks for your report.

I seem to see a similar issue with respect to the lid on one of my
test machines, looking into it right now.

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v3 0/8] PM / ACPI: sleep: Additional changes related to suspend-to-idle
  2019-08-19  7:59   ` Rafael J. Wysocki
@ 2019-08-19  9:05     ` Rafael J. Wysocki
  2019-08-19 15:45       ` Kristian Klausen
  0 siblings, 1 reply; 25+ messages in thread
From: Rafael J. Wysocki @ 2019-08-19  9:05 UTC (permalink / raw)
  To: Kristian Klausen
  Cc: Linux ACPI, Linux PM, LKML, Zhang Rui, Rajneesh Bhardwaj,
	Andy Shevchenko, Mario Limonciello, Kai-Heng Feng

On Monday, August 19, 2019 9:59:02 AM CEST Rafael J. Wysocki wrote:
> On Fri, Aug 16, 2019 at 10:26 PM Kristian Klausen <kristian@klausen.dk> wrote:
> >
> > On 02.08.2019 12.33, Rafael J. Wysocki wrote:
> > > Hi All,
> > >
> > >>> On top of the "Simplify the suspend-to-idle control flow" patch series
> > >>> posted previously:
> > >>>
> > >>> https://lore.kernel.org/lkml/71085220.z6FKkvYQPX@kreacher/
> > >>>
> > >>> sanitize the suspend-to-idle flow even further.
> > >>>
> > >>> First off, decouple EC wakeup from the LPS0 _DSM processing (patch 1).
> > >>>
> > >>> Next, reorder the code to invoke LPS0 _DSM Functions 5 and 6 in the
> > >>> specification-compliant order with respect to suspending and resuming
> > >>> devices (patch 2).
> > >>>
> > >>> Finally, rearrange lps0_device_attach() (patch 3) and add a command line
> > >>> switch to prevent the LPS0 _DSM from being used.
> > >> The v2 is because I found a (minor) bug in patch 1, decided to use a module
> > >> parameter instead of a kernel command line option in patch 4.  Also, there
> > >> are 4 new patches:
> > >>
> > >> Patch 5: Switch the EC over to polling during "noirq" suspend and back
> > >> during "noirq" resume.
> > >>
> > >> Patch 6: Eliminate acpi_sleep_no_ec_events().
> > >>
> > >> Patch 7: Consolidate some EC code depending on PM_SLEEP.
> > >>
> > >> Patch 8: Add EC GPE dispatching debug message.
> > > The v3 is just a rearranged v2 so as to move the post sensitive patch (previous patch 2)
> > > to the end of the series.   [After applying the full series the code is the same as before.]
> > >
> > > For easier testing, the series (along with some previous patches depended on by it)
> > > is available in the pm-s2idle-testing branch of the linux-pm.git tree at kernel.org:
> > >
> > > https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git/log/?h=pm-s2idle-testing
> > It was just testing this patch series(461fc1caed55), to see if it would
> > fix my charging issue
> > (https://bugzilla.kernel.org/show_bug.cgi?id=201307), which it didn't.
> 
> It is unlikely to help in that case.
> 
> > I did however notice that my laptop (ASUS Zenbook UX430UNR/i7-8550U)
> > won't wake when opening the lid or pressing a key, the only way to wake
> > the laptop is pressing the power button.
> >
> > I also tested mainline (5.3.0-rc4 b7e7c85dc7b0) and 5.2.8 and the laptop
> > wakes without issue when the lid is opened or a key is presed.
> > > Please refer to the changelogs for details.
> 
> Thanks for your report.
> 
> I seem to see a similar issue with respect to the lid on one of my
> test machines, looking into it right now.

Well, my lid issue seems to be unrelated as it doesn't result from any patches in the
series in question.

First off, please clone 5.3-rc5 from kernel.org and double check if the issue is not
present in that one.

If that's not the case, merge the pm-s2idle-rework branch from my tree on top of it
and retest.

If you still see the issue then, apply the appended patch (on top of the pm-s2idle-reqork
branch ) and, after starting the kernel, do

# echo 1 > /sys/power/pm_debug_messages

suspend the system and try to wake it up through all of the ways that stopped working.

Then, wake it up with the power button, save the output of dmesg and send it to me.

Thanks!

---
 drivers/acpi/sleep.c        |    4 ++--
 drivers/base/power/wakeup.c |    2 ++
 kernel/irq/pm.c             |    2 ++
 3 files changed, 6 insertions(+), 2 deletions(-)

Index: linux-pm/drivers/acpi/sleep.c
===================================================================
--- linux-pm.orig/drivers/acpi/sleep.c
+++ linux-pm/drivers/acpi/sleep.c
@@ -1012,9 +1012,9 @@ static void acpi_s2idle_wake(void)
 		acpi_os_wait_events_complete(); /* synchronize EC GPE processing */
 		acpi_ec_flush_work();
 		acpi_os_wait_events_complete(); /* synchronize Notify handling */
-	}
 
-	rearm_wake_irq(acpi_sci_irq);
+		rearm_wake_irq(acpi_sci_irq);
+	}
 }
 
 static void acpi_s2idle_restore_early(void)
Index: linux-pm/drivers/base/power/wakeup.c
===================================================================
--- linux-pm.orig/drivers/base/power/wakeup.c
+++ linux-pm/drivers/base/power/wakeup.c
@@ -871,6 +871,8 @@ void pm_wakeup_clear(bool reset)
 
 void pm_system_irq_wakeup(unsigned int irq_number)
 {
+	pm_pr_dbg("IRQ wakeup: IRQ %u\n", irq_number);
+
 	if (pm_wakeup_irq == 0) {
 		pm_wakeup_irq = irq_number;
 		pm_system_wakeup();
Index: linux-pm/kernel/irq/pm.c
===================================================================
--- linux-pm.orig/kernel/irq/pm.c
+++ linux-pm/kernel/irq/pm.c
@@ -15,6 +15,8 @@
 
 bool irq_pm_check_wakeup(struct irq_desc *desc)
 {
+	pm_pr_dbg("%s: IRQ %u\n", __func__, irq_desc_get_irq(desc));
+
 	if (irqd_is_wakeup_armed(&desc->irq_data)) {
 		irqd_clear(&desc->irq_data, IRQD_WAKEUP_ARMED);
 		desc->istate |= IRQS_SUSPENDED | IRQS_PENDING;






^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v3 0/8] PM / ACPI: sleep: Additional changes related to suspend-to-idle
  2019-08-19  9:05     ` Rafael J. Wysocki
@ 2019-08-19 15:45       ` Kristian Klausen
  2019-08-19 20:41         ` Rafael J. Wysocki
  0 siblings, 1 reply; 25+ messages in thread
From: Kristian Klausen @ 2019-08-19 15:45 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Linux ACPI, Linux PM, LKML, Zhang Rui, Rajneesh Bhardwaj,
	Andy Shevchenko, Mario Limonciello, Kai-Heng Feng

On 19.08.2019 11.05, Rafael J. Wysocki wrote:
> On Monday, August 19, 2019 9:59:02 AM CEST Rafael J. Wysocki wrote:
>> On Fri, Aug 16, 2019 at 10:26 PM Kristian Klausen <kristian@klausen.dk> wrote:
>>> On 02.08.2019 12.33, Rafael J. Wysocki wrote:
>>>> Hi All,
>>>>
>>>>>> On top of the "Simplify the suspend-to-idle control flow" patch series
>>>>>> posted previously:
>>>>>>
>>>>>> https://lore.kernel.org/lkml/71085220.z6FKkvYQPX@kreacher/
>>>>>>
>>>>>> sanitize the suspend-to-idle flow even further.
>>>>>>
>>>>>> First off, decouple EC wakeup from the LPS0 _DSM processing (patch 1).
>>>>>>
>>>>>> Next, reorder the code to invoke LPS0 _DSM Functions 5 and 6 in the
>>>>>> specification-compliant order with respect to suspending and resuming
>>>>>> devices (patch 2).
>>>>>>
>>>>>> Finally, rearrange lps0_device_attach() (patch 3) and add a command line
>>>>>> switch to prevent the LPS0 _DSM from being used.
>>>>> The v2 is because I found a (minor) bug in patch 1, decided to use a module
>>>>> parameter instead of a kernel command line option in patch 4.  Also, there
>>>>> are 4 new patches:
>>>>>
>>>>> Patch 5: Switch the EC over to polling during "noirq" suspend and back
>>>>> during "noirq" resume.
>>>>>
>>>>> Patch 6: Eliminate acpi_sleep_no_ec_events().
>>>>>
>>>>> Patch 7: Consolidate some EC code depending on PM_SLEEP.
>>>>>
>>>>> Patch 8: Add EC GPE dispatching debug message.
>>>> The v3 is just a rearranged v2 so as to move the post sensitive patch (previous patch 2)
>>>> to the end of the series.   [After applying the full series the code is the same as before.]
>>>>
>>>> For easier testing, the series (along with some previous patches depended on by it)
>>>> is available in the pm-s2idle-testing branch of the linux-pm.git tree at kernel.org:
>>>>
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git/log/?h=pm-s2idle-testing
>>> It was just testing this patch series(461fc1caed55), to see if it would
>>> fix my charging issue
>>> (https://bugzilla.kernel.org/show_bug.cgi?id=201307), which it didn't.
>> It is unlikely to help in that case.
Do you have any idea what the issue could be?
>>
>>> I did however notice that my laptop (ASUS Zenbook UX430UNR/i7-8550U)
>>> won't wake when opening the lid or pressing a key, the only way to wake
>>> the laptop is pressing the power button.
>>>
>>> I also tested mainline (5.3.0-rc4 b7e7c85dc7b0) and 5.2.8 and the laptop
>>> wakes without issue when the lid is opened or a key is presed.
>>>> Please refer to the changelogs for details.
>> Thanks for your report.
>>
>> I seem to see a similar issue with respect to the lid on one of my
>> test machines, looking into it right now.
> Well, my lid issue seems to be unrelated as it doesn't result from any patches in the
> series in question.
>
> First off, please clone 5.3-rc5 from kernel.org and double check if the issue is not
> present in that one.
>
> If that's not the case, merge the pm-s2idle-rework branch from my tree on top of it
> and retest.
>
> If you still see the issue then, apply the appended patch (on top of the pm-s2idle-reqork
> branch ) and, after starting the kernel, do
>
> # echo 1 > /sys/power/pm_debug_messages
>
> suspend the system and try to wake it up through all of the ways that stopped working.
>
> Then, wake it up with the power button, save the output of dmesg and send it to me.
>
> Thanks!

With 5.3-rc5 the laptops wakes up without any issue when pressing a key 
or opening the lid.
With v5.3-rc5+pm-s2idle-testing I can only wake the laptop by pressing 
the power button.

dmesg with pm_debug_messages=1 and your patch:
[   55.646109] PM: suspend entry (s2idle)
[   55.698559] Filesystems sync: 0.052 seconds
[   55.698561] PM: Preparing system for sleep (s2idle)
[   55.700661] Freezing user space processes ... (elapsed 0.210 seconds) 
done.
[   55.911494] OOM killer disabled.
[   55.911495] Freezing remaining freezable tasks ... (elapsed 0.001 
seconds) done.
[   55.913192] PM: Suspending system (s2idle)
[   55.913195] printk: Suspending console(s) (use no_console_suspend to 
debug)
[   55.914778] [drm] CT: disabled
[   55.916057] wlan0: deauthenticating from 64:70:02:a5:fd:02 by local 
choice (Reason: 3=DEAUTH_LEAVING)
[   56.045634] sd 2:0:0:0: [sda] Synchronizing SCSI cache
[   56.046650] sd 2:0:0:0: [sda] Stopping disk
[   56.287622] PM: suspend of devices complete after 371.285 msecs
[   56.287627] PM: start suspend of devices complete after 373.684 msecs
[   56.307155] PM: late suspend of devices complete after 19.477 msecs
[   56.312479] ACPI: EC: interrupt blocked
[   56.352761] PM: noirq suspend of devices complete after 45.205 msecs
[   56.352770] ACPI: \_PR_.PR00: LPI: Device not power manageable
[   56.352774] ACPI: \_PR_.PR01: LPI: Device not power manageable
[   56.352776] ACPI: \_PR_.PR02: LPI: Device not power manageable
[   56.352779] ACPI: \_PR_.PR03: LPI: Device not power manageable
[   56.352782] ACPI: \_PR_.PR04: LPI: Device not power manageable
[   56.352785] ACPI: \_PR_.PR05: LPI: Device not power manageable
[   56.352788] ACPI: \_PR_.PR06: LPI: Device not power manageable
[   56.352790] ACPI: \_PR_.PR07: LPI: Device not power manageable
[   56.352793] ACPI: \_SB_.PCI0.GFX0: LPI: Device not power manageable
[   56.352800] ACPI: \_SB_.PCI0.RP06.PXSX: LPI: Device not power manageable
[   56.357057] PM: suspend-to-idle
[   69.338656] PM: Timekeeping suspended for 12.178 seconds
[   69.338701] PM: irq_pm_check_wakeup: IRQ 9
[   69.338704] PM: IRQ wakeup: IRQ 9
[   69.338879] PM: resume from suspend-to-idle
[   69.371406] ACPI: EC: interrupt unblocked
[   69.514126] PM: noirq resume of devices complete after 142.668 msecs
[   69.516007] PM: early resume of devices complete after 1.773 msecs
[   69.517579] [drm] HuC: Loaded firmware i915/kbl_huc_ver02_00_1810.bin 
(version 2.0)
[   69.521691] [drm] GuC: Loaded firmware i915/kbl_guc_32.0.3.bin 
(version 32.0)
[   69.521764] [drm] CT: enabled
[   69.521850] i915 0000:00:02.0: GuC firmware version 32.0
[   69.521853] i915 0000:00:02.0: GuC submission disabled
[   69.521855] i915 0000:00:02.0: HuC enabled
[   69.527165] sd 2:0:0:0: [sda] Starting disk
[   69.528076] iwlwifi 0000:02:00.0: Applying debug destination 
EXTERNAL_DRAM
[   69.661997] iwlwifi 0000:02:00.0: Applying debug destination 
EXTERNAL_DRAM
[   69.729645] iwlwifi 0000:02:00.0: FW already configured (0) - 
re-configuring
[   69.842657] ata3: SATA link up 6.0 Gbps (SStatus 133 SControl 300)
[   69.844600] ata3.00: configured for UDMA/133
[   69.949032] PM: resume of devices complete after 432.157 msecs
[   69.949770] PM: Finishing wakeup.
[   69.949771] OOM killer enabled.
[   69.949772] Restarting tasks ...
[   69.953029] mei_hdcp mei::b638ab7e-94e2-4ea2-a552-d1c54b627f04:01: 
bound 0000:00:02.0 (ops i915_hdcp_component_ops [i915])
[   69.953521] done.
[   70.012592] PM: suspend exit
>
> ---
>   drivers/acpi/sleep.c        |    4 ++--
>   drivers/base/power/wakeup.c |    2 ++
>   kernel/irq/pm.c             |    2 ++
>   3 files changed, 6 insertions(+), 2 deletions(-)
>
> Index: linux-pm/drivers/acpi/sleep.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/sleep.c
> +++ linux-pm/drivers/acpi/sleep.c
> @@ -1012,9 +1012,9 @@ static void acpi_s2idle_wake(void)
>   		acpi_os_wait_events_complete(); /* synchronize EC GPE processing */
>   		acpi_ec_flush_work();
>   		acpi_os_wait_events_complete(); /* synchronize Notify handling */
> -	}
>   
> -	rearm_wake_irq(acpi_sci_irq);
> +		rearm_wake_irq(acpi_sci_irq);
> +	}
>   }
>   
>   static void acpi_s2idle_restore_early(void)
> Index: linux-pm/drivers/base/power/wakeup.c
> ===================================================================
> --- linux-pm.orig/drivers/base/power/wakeup.c
> +++ linux-pm/drivers/base/power/wakeup.c
> @@ -871,6 +871,8 @@ void pm_wakeup_clear(bool reset)
>   
>   void pm_system_irq_wakeup(unsigned int irq_number)
>   {
> +	pm_pr_dbg("IRQ wakeup: IRQ %u\n", irq_number);
> +
>   	if (pm_wakeup_irq == 0) {
>   		pm_wakeup_irq = irq_number;
>   		pm_system_wakeup();
> Index: linux-pm/kernel/irq/pm.c
> ===================================================================
> --- linux-pm.orig/kernel/irq/pm.c
> +++ linux-pm/kernel/irq/pm.c
> @@ -15,6 +15,8 @@
>   
>   bool irq_pm_check_wakeup(struct irq_desc *desc)
>   {
> +	pm_pr_dbg("%s: IRQ %u\n", __func__, irq_desc_get_irq(desc));
> +
>   	if (irqd_is_wakeup_armed(&desc->irq_data)) {
>   		irqd_clear(&desc->irq_data, IRQD_WAKEUP_ARMED);
>   		desc->istate |= IRQS_SUSPENDED | IRQS_PENDING;
>
>
>
>
>


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v3 0/8] PM / ACPI: sleep: Additional changes related to suspend-to-idle
  2019-08-19 15:45       ` Kristian Klausen
@ 2019-08-19 20:41         ` Rafael J. Wysocki
  2019-08-20 13:10           ` Kristian Klausen
  0 siblings, 1 reply; 25+ messages in thread
From: Rafael J. Wysocki @ 2019-08-19 20:41 UTC (permalink / raw)
  To: Kristian Klausen
  Cc: Rafael J. Wysocki, Linux ACPI, Linux PM, LKML, Zhang Rui,
	Rajneesh Bhardwaj, Andy Shevchenko, Mario Limonciello,
	Kai-Heng Feng

On Mon, Aug 19, 2019 at 5:47 PM Kristian Klausen <kristian@klausen.dk> wrote:
>
> On 19.08.2019 11.05, Rafael J. Wysocki wrote:
> > On Monday, August 19, 2019 9:59:02 AM CEST Rafael J. Wysocki wrote:
> >> On Fri, Aug 16, 2019 at 10:26 PM Kristian Klausen <kristian@klausen.dk> wrote:
> >>> On 02.08.2019 12.33, Rafael J. Wysocki wrote:
> >>>> Hi All,
> >>>>
> >>>>>> On top of the "Simplify the suspend-to-idle control flow" patch series
> >>>>>> posted previously:
> >>>>>>
> >>>>>> https://lore.kernel.org/lkml/71085220.z6FKkvYQPX@kreacher/
> >>>>>>
> >>>>>> sanitize the suspend-to-idle flow even further.
> >>>>>>
> >>>>>> First off, decouple EC wakeup from the LPS0 _DSM processing (patch 1).
> >>>>>>
> >>>>>> Next, reorder the code to invoke LPS0 _DSM Functions 5 and 6 in the
> >>>>>> specification-compliant order with respect to suspending and resuming
> >>>>>> devices (patch 2).
> >>>>>>
> >>>>>> Finally, rearrange lps0_device_attach() (patch 3) and add a command line
> >>>>>> switch to prevent the LPS0 _DSM from being used.
> >>>>> The v2 is because I found a (minor) bug in patch 1, decided to use a module
> >>>>> parameter instead of a kernel command line option in patch 4.  Also, there
> >>>>> are 4 new patches:
> >>>>>
> >>>>> Patch 5: Switch the EC over to polling during "noirq" suspend and back
> >>>>> during "noirq" resume.
> >>>>>
> >>>>> Patch 6: Eliminate acpi_sleep_no_ec_events().
> >>>>>
> >>>>> Patch 7: Consolidate some EC code depending on PM_SLEEP.
> >>>>>
> >>>>> Patch 8: Add EC GPE dispatching debug message.
> >>>> The v3 is just a rearranged v2 so as to move the post sensitive patch (previous patch 2)
> >>>> to the end of the series.   [After applying the full series the code is the same as before.]
> >>>>
> >>>> For easier testing, the series (along with some previous patches depended on by it)
> >>>> is available in the pm-s2idle-testing branch of the linux-pm.git tree at kernel.org:
> >>>>
> >>>> https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git/log/?h=pm-s2idle-testing
> >>> It was just testing this patch series(461fc1caed55), to see if it would
> >>> fix my charging issue
> >>> (https://bugzilla.kernel.org/show_bug.cgi?id=201307), which it didn't.
> >> It is unlikely to help in that case.
> Do you have any idea what the issue could be?

Basically, there are two possibilities: either the OS is expected to
handle the AC/battery switching events, or the platform firmware
should take care of them.  In the former case, the EC should generate
events to be handled by the OS and in the latter one there needs to be
a way to let the platform firmware that it needs to take care of those
events going forward.

In either case there may be a platform-specific action to be carried
out during suspend and resume to set this up as expected which may be
missing.

> >>
> >>> I did however notice that my laptop (ASUS Zenbook UX430UNR/i7-8550U)
> >>> won't wake when opening the lid or pressing a key, the only way to wake
> >>> the laptop is pressing the power button.
> >>>
> >>> I also tested mainline (5.3.0-rc4 b7e7c85dc7b0) and 5.2.8 and the laptop
> >>> wakes without issue when the lid is opened or a key is presed.
> >>>> Please refer to the changelogs for details.
> >> Thanks for your report.
> >>
> >> I seem to see a similar issue with respect to the lid on one of my
> >> test machines, looking into it right now.
> > Well, my lid issue seems to be unrelated as it doesn't result from any patches in the
> > series in question.
> >
> > First off, please clone 5.3-rc5 from kernel.org and double check if the issue is not
> > present in that one.
> >
> > If that's not the case, merge the pm-s2idle-rework branch from my tree on top of it
> > and retest.
> >
> > If you still see the issue then, apply the appended patch (on top of the pm-s2idle-reqork
> > branch ) and, after starting the kernel, do
> >
> > # echo 1 > /sys/power/pm_debug_messages
> >
> > suspend the system and try to wake it up through all of the ways that stopped working.
> >
> > Then, wake it up with the power button, save the output of dmesg and send it to me.
> >
> > Thanks!
>
> With 5.3-rc5 the laptops wakes up without any issue when pressing a key
> or opening the lid.
> With v5.3-rc5+pm-s2idle-testing I can only wake the laptop by pressing
> the power button.

OK, thanks for verifying.

So it is unclear to me how the series can cause an issue like that to appear.

> dmesg with pm_debug_messages=1 and your patch:
> [   55.646109] PM: suspend entry (s2idle)
> [   55.698559] Filesystems sync: 0.052 seconds
> [   55.698561] PM: Preparing system for sleep (s2idle)
> [   55.700661] Freezing user space processes ... (elapsed 0.210 seconds)
> done.
> [   55.911494] OOM killer disabled.
> [   55.911495] Freezing remaining freezable tasks ... (elapsed 0.001
> seconds) done.
> [   55.913192] PM: Suspending system (s2idle)
> [   55.913195] printk: Suspending console(s) (use no_console_suspend to
> debug)
> [   55.914778] [drm] CT: disabled
> [   55.916057] wlan0: deauthenticating from 64:70:02:a5:fd:02 by local
> choice (Reason: 3=DEAUTH_LEAVING)
> [   56.045634] sd 2:0:0:0: [sda] Synchronizing SCSI cache
> [   56.046650] sd 2:0:0:0: [sda] Stopping disk
> [   56.287622] PM: suspend of devices complete after 371.285 msecs
> [   56.287627] PM: start suspend of devices complete after 373.684 msecs
> [   56.307155] PM: late suspend of devices complete after 19.477 msecs
> [   56.312479] ACPI: EC: interrupt blocked
> [   56.352761] PM: noirq suspend of devices complete after 45.205 msecs
> [   56.352770] ACPI: \_PR_.PR00: LPI: Device not power manageable
> [   56.352774] ACPI: \_PR_.PR01: LPI: Device not power manageable
> [   56.352776] ACPI: \_PR_.PR02: LPI: Device not power manageable
> [   56.352779] ACPI: \_PR_.PR03: LPI: Device not power manageable
> [   56.352782] ACPI: \_PR_.PR04: LPI: Device not power manageable
> [   56.352785] ACPI: \_PR_.PR05: LPI: Device not power manageable
> [   56.352788] ACPI: \_PR_.PR06: LPI: Device not power manageable
> [   56.352790] ACPI: \_PR_.PR07: LPI: Device not power manageable
> [   56.352793] ACPI: \_SB_.PCI0.GFX0: LPI: Device not power manageable
> [   56.352800] ACPI: \_SB_.PCI0.RP06.PXSX: LPI: Device not power manageable
> [   56.357057] PM: suspend-to-idle
> [   69.338656] PM: Timekeeping suspended for 12.178 seconds
> [   69.338701] PM: irq_pm_check_wakeup: IRQ 9
> [   69.338704] PM: IRQ wakeup: IRQ 9

This clearly is the power button event causing the system to wake up.
The other actions, whatever they were, didn't cause any interrupts to
be triggered.

I suspect that the issue is related to the EC, so please try to revert commit

fcd0a04267ac ACPI: PM: s2idle: Switch EC over to polling during "noirq" suspend

and see if that makes any difference (should revert cleanly).

If that doesn't make any difference, please also try to revert commits
(on top of the above revert)

11f26633cccb PM: suspend: Fix platform_suspend_prepare_noirq()
ac9eafbe930a ACPI: PM: s2idle: Execute LPS0 _DSM functions with
suspended devices

(in this order) and retest.

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v3 0/8] PM / ACPI: sleep: Additional changes related to suspend-to-idle
  2019-08-19 20:41         ` Rafael J. Wysocki
@ 2019-08-20 13:10           ` Kristian Klausen
  2019-08-20 13:29             ` Rafael J. Wysocki
  0 siblings, 1 reply; 25+ messages in thread
From: Kristian Klausen @ 2019-08-20 13:10 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Rafael J. Wysocki, Linux ACPI, Linux PM, LKML, Zhang Rui,
	Rajneesh Bhardwaj, Andy Shevchenko, Mario Limonciello,
	Kai-Heng Feng

On 19.08.2019 22.41, Rafael J. Wysocki wrote:
> On Mon, Aug 19, 2019 at 5:47 PM Kristian Klausen <kristian@klausen.dk> wrote:
>> On 19.08.2019 11.05, Rafael J. Wysocki wrote:
>>> On Monday, August 19, 2019 9:59:02 AM CEST Rafael J. Wysocki wrote:
>>>> On Fri, Aug 16, 2019 at 10:26 PM Kristian Klausen <kristian@klausen.dk> wrote:
>>>>> On 02.08.2019 12.33, Rafael J. Wysocki wrote:
>>>>>> Hi All,
>>>>>>
>>>>>>>> On top of the "Simplify the suspend-to-idle control flow" patch series
>>>>>>>> posted previously:
>>>>>>>>
>>>>>>>> https://lore.kernel.org/lkml/71085220.z6FKkvYQPX@kreacher/
>>>>>>>>
>>>>>>>> sanitize the suspend-to-idle flow even further.
>>>>>>>>
>>>>>>>> First off, decouple EC wakeup from the LPS0 _DSM processing (patch 1).
>>>>>>>>
>>>>>>>> Next, reorder the code to invoke LPS0 _DSM Functions 5 and 6 in the
>>>>>>>> specification-compliant order with respect to suspending and resuming
>>>>>>>> devices (patch 2).
>>>>>>>>
>>>>>>>> Finally, rearrange lps0_device_attach() (patch 3) and add a command line
>>>>>>>> switch to prevent the LPS0 _DSM from being used.
>>>>>>> The v2 is because I found a (minor) bug in patch 1, decided to use a module
>>>>>>> parameter instead of a kernel command line option in patch 4.  Also, there
>>>>>>> are 4 new patches:
>>>>>>>
>>>>>>> Patch 5: Switch the EC over to polling during "noirq" suspend and back
>>>>>>> during "noirq" resume.
>>>>>>>
>>>>>>> Patch 6: Eliminate acpi_sleep_no_ec_events().
>>>>>>>
>>>>>>> Patch 7: Consolidate some EC code depending on PM_SLEEP.
>>>>>>>
>>>>>>> Patch 8: Add EC GPE dispatching debug message.
>>>>>> The v3 is just a rearranged v2 so as to move the post sensitive patch (previous patch 2)
>>>>>> to the end of the series.   [After applying the full series the code is the same as before.]
>>>>>>
>>>>>> For easier testing, the series (along with some previous patches depended on by it)
>>>>>> is available in the pm-s2idle-testing branch of the linux-pm.git tree at kernel.org:
>>>>>>
>>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git/log/?h=pm-s2idle-testing
>>>>> It was just testing this patch series(461fc1caed55), to see if it would
>>>>> fix my charging issue
>>>>> (https://bugzilla.kernel.org/show_bug.cgi?id=201307), which it didn't.
>>>> It is unlikely to help in that case.
>> Do you have any idea what the issue could be?
> Basically, there are two possibilities: either the OS is expected to
> handle the AC/battery switching events, or the platform firmware
> should take care of them.  In the former case, the EC should generate
> events to be handled by the OS and in the latter one there needs to be
> a way to let the platform firmware that it needs to take care of those
> events going forward.
>
> In either case there may be a platform-specific action to be carried
> out during suspend and resume to set this up as expected which may be
> missing.
Thanks for the explanation. I don't think I have the expertise to solve 
the issue, but at least now I'm one step closer.
>
>>>>> I did however notice that my laptop (ASUS Zenbook UX430UNR/i7-8550U)
>>>>> won't wake when opening the lid or pressing a key, the only way to wake
>>>>> the laptop is pressing the power button.
>>>>>
>>>>> I also tested mainline (5.3.0-rc4 b7e7c85dc7b0) and 5.2.8 and the laptop
>>>>> wakes without issue when the lid is opened or a key is presed.
>>>>>> Please refer to the changelogs for details.
>>>> Thanks for your report.
>>>>
>>>> I seem to see a similar issue with respect to the lid on one of my
>>>> test machines, looking into it right now.
>>> Well, my lid issue seems to be unrelated as it doesn't result from any patches in the
>>> series in question.
>>>
>>> First off, please clone 5.3-rc5 from kernel.org and double check if the issue is not
>>> present in that one.
>>>
>>> If that's not the case, merge the pm-s2idle-rework branch from my tree on top of it
>>> and retest.
>>>
>>> If you still see the issue then, apply the appended patch (on top of the pm-s2idle-reqork
>>> branch ) and, after starting the kernel, do
>>>
>>> # echo 1 > /sys/power/pm_debug_messages
>>>
>>> suspend the system and try to wake it up through all of the ways that stopped working.
>>>
>>> Then, wake it up with the power button, save the output of dmesg and send it to me.
>>>
>>> Thanks!
>> With 5.3-rc5 the laptops wakes up without any issue when pressing a key
>> or opening the lid.
>> With v5.3-rc5+pm-s2idle-testing I can only wake the laptop by pressing
>> the power button.
> OK, thanks for verifying.
>
> So it is unclear to me how the series can cause an issue like that to appear.
>
>> dmesg with pm_debug_messages=1 and your patch:
>> [   55.646109] PM: suspend entry (s2idle)
>> [   55.698559] Filesystems sync: 0.052 seconds
>> [   55.698561] PM: Preparing system for sleep (s2idle)
>> [   55.700661] Freezing user space processes ... (elapsed 0.210 seconds)
>> done.
>> [   55.911494] OOM killer disabled.
>> [   55.911495] Freezing remaining freezable tasks ... (elapsed 0.001
>> seconds) done.
>> [   55.913192] PM: Suspending system (s2idle)
>> [   55.913195] printk: Suspending console(s) (use no_console_suspend to
>> debug)
>> [   55.914778] [drm] CT: disabled
>> [   55.916057] wlan0: deauthenticating from 64:70:02:a5:fd:02 by local
>> choice (Reason: 3=DEAUTH_LEAVING)
>> [   56.045634] sd 2:0:0:0: [sda] Synchronizing SCSI cache
>> [   56.046650] sd 2:0:0:0: [sda] Stopping disk
>> [   56.287622] PM: suspend of devices complete after 371.285 msecs
>> [   56.287627] PM: start suspend of devices complete after 373.684 msecs
>> [   56.307155] PM: late suspend of devices complete after 19.477 msecs
>> [   56.312479] ACPI: EC: interrupt blocked
>> [   56.352761] PM: noirq suspend of devices complete after 45.205 msecs
>> [   56.352770] ACPI: \_PR_.PR00: LPI: Device not power manageable
>> [   56.352774] ACPI: \_PR_.PR01: LPI: Device not power manageable
>> [   56.352776] ACPI: \_PR_.PR02: LPI: Device not power manageable
>> [   56.352779] ACPI: \_PR_.PR03: LPI: Device not power manageable
>> [   56.352782] ACPI: \_PR_.PR04: LPI: Device not power manageable
>> [   56.352785] ACPI: \_PR_.PR05: LPI: Device not power manageable
>> [   56.352788] ACPI: \_PR_.PR06: LPI: Device not power manageable
>> [   56.352790] ACPI: \_PR_.PR07: LPI: Device not power manageable
>> [   56.352793] ACPI: \_SB_.PCI0.GFX0: LPI: Device not power manageable
>> [   56.352800] ACPI: \_SB_.PCI0.RP06.PXSX: LPI: Device not power manageable
>> [   56.357057] PM: suspend-to-idle
>> [   69.338656] PM: Timekeeping suspended for 12.178 seconds
>> [   69.338701] PM: irq_pm_check_wakeup: IRQ 9
>> [   69.338704] PM: IRQ wakeup: IRQ 9
> This clearly is the power button event causing the system to wake up.
> The other actions, whatever they were, didn't cause any interrupts to
> be triggered.
>
> I suspect that the issue is related to the EC, so please try to revert commit
>
> fcd0a04267ac ACPI: PM: s2idle: Switch EC over to polling during "noirq" suspend
>
> and see if that makes any difference (should revert cleanly).
>
> If that doesn't make any difference, please also try to revert commits
> (on top of the above revert)
>
> 11f26633cccb PM: suspend: Fix platform_suspend_prepare_noirq()
> ac9eafbe930a ACPI: PM: s2idle: Execute LPS0 _DSM functions with
> suspended devices
>
> (in this order) and retest.
Reverting the following commits, didn't fix the issue:
fcd0a04267ac ACPI: PM: s2idle: Switch EC over to polling during "noirq" 
suspend
6e86633a791f ACPI: PM: s2idle: Eliminate acpi_sleep_no_ec_events()
11f26633cccb PM: suspend: Fix platform_suspend_prepare_noirq()
ac9eafbe930a ACPI: PM: s2idle: Execute LPS0 _DSM functions with 
suspended devices

I didn't bother reverting all the commits, so I did a checkout of:
b605c44c30b5 PM: sleep: Drop dpm_noirq_begin() and dpm_noirq_end()
and everything works, then I did a checkout of:
10a08fd65ec1 ACPI: PM: Set up EC GPE for system wakeup from drivers that 
need it
and the laptop won't wake when opening the lid or pressing a key.

So 10a08fd65ec1 must be the culprit.

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v3 0/8] PM / ACPI: sleep: Additional changes related to suspend-to-idle
  2019-08-20 13:10           ` Kristian Klausen
@ 2019-08-20 13:29             ` Rafael J. Wysocki
  2019-08-20 21:38               ` Rafael J. Wysocki
  0 siblings, 1 reply; 25+ messages in thread
From: Rafael J. Wysocki @ 2019-08-20 13:29 UTC (permalink / raw)
  To: Kristian Klausen
  Cc: Rafael J. Wysocki, Rafael J. Wysocki, Linux ACPI, Linux PM, LKML,
	Zhang Rui, Rajneesh Bhardwaj, Andy Shevchenko, Mario Limonciello,
	Kai-Heng Feng

On Tue, Aug 20, 2019 at 3:10 PM Kristian Klausen <kristian@klausen.dk> wrote:
>
> On 19.08.2019 22.41, Rafael J. Wysocki wrote:
> > On Mon, Aug 19, 2019 at 5:47 PM Kristian Klausen <kristian@klausen.dk> wrote:
> >> On 19.08.2019 11.05, Rafael J. Wysocki wrote:
> >>> On Monday, August 19, 2019 9:59:02 AM CEST Rafael J. Wysocki wrote:
> >>>> On Fri, Aug 16, 2019 at 10:26 PM Kristian Klausen <kristian@klausen.dk> wrote:
> >>>>> On 02.08.2019 12.33, Rafael J. Wysocki wrote:
> >>>>>> Hi All,
> >>>>>>
> >>>>>>>> On top of the "Simplify the suspend-to-idle control flow" patch series
> >>>>>>>> posted previously:
> >>>>>>>>
> >>>>>>>> https://lore.kernel.org/lkml/71085220.z6FKkvYQPX@kreacher/
> >>>>>>>>
> >>>>>>>> sanitize the suspend-to-idle flow even further.
> >>>>>>>>
> >>>>>>>> First off, decouple EC wakeup from the LPS0 _DSM processing (patch 1).
> >>>>>>>>
> >>>>>>>> Next, reorder the code to invoke LPS0 _DSM Functions 5 and 6 in the
> >>>>>>>> specification-compliant order with respect to suspending and resuming
> >>>>>>>> devices (patch 2).
> >>>>>>>>
> >>>>>>>> Finally, rearrange lps0_device_attach() (patch 3) and add a command line
> >>>>>>>> switch to prevent the LPS0 _DSM from being used.
> >>>>>>> The v2 is because I found a (minor) bug in patch 1, decided to use a module
> >>>>>>> parameter instead of a kernel command line option in patch 4.  Also, there
> >>>>>>> are 4 new patches:
> >>>>>>>
> >>>>>>> Patch 5: Switch the EC over to polling during "noirq" suspend and back
> >>>>>>> during "noirq" resume.
> >>>>>>>
> >>>>>>> Patch 6: Eliminate acpi_sleep_no_ec_events().
> >>>>>>>
> >>>>>>> Patch 7: Consolidate some EC code depending on PM_SLEEP.
> >>>>>>>
> >>>>>>> Patch 8: Add EC GPE dispatching debug message.
> >>>>>> The v3 is just a rearranged v2 so as to move the post sensitive patch (previous patch 2)
> >>>>>> to the end of the series.   [After applying the full series the code is the same as before.]
> >>>>>>
> >>>>>> For easier testing, the series (along with some previous patches depended on by it)
> >>>>>> is available in the pm-s2idle-testing branch of the linux-pm.git tree at kernel.org:
> >>>>>>
> >>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git/log/?h=pm-s2idle-testing
> >>>>> It was just testing this patch series(461fc1caed55), to see if it would
> >>>>> fix my charging issue
> >>>>> (https://bugzilla.kernel.org/show_bug.cgi?id=201307), which it didn't.
> >>>> It is unlikely to help in that case.
> >> Do you have any idea what the issue could be?
> > Basically, there are two possibilities: either the OS is expected to
> > handle the AC/battery switching events, or the platform firmware
> > should take care of them.  In the former case, the EC should generate
> > events to be handled by the OS and in the latter one there needs to be
> > a way to let the platform firmware that it needs to take care of those
> > events going forward.
> >
> > In either case there may be a platform-specific action to be carried
> > out during suspend and resume to set this up as expected which may be
> > missing.
> Thanks for the explanation. I don't think I have the expertise to solve
> the issue, but at least now I'm one step closer.
> >
> >>>>> I did however notice that my laptop (ASUS Zenbook UX430UNR/i7-8550U)
> >>>>> won't wake when opening the lid or pressing a key, the only way to wake
> >>>>> the laptop is pressing the power button.
> >>>>>
> >>>>> I also tested mainline (5.3.0-rc4 b7e7c85dc7b0) and 5.2.8 and the laptop
> >>>>> wakes without issue when the lid is opened or a key is presed.
> >>>>>> Please refer to the changelogs for details.
> >>>> Thanks for your report.
> >>>>
> >>>> I seem to see a similar issue with respect to the lid on one of my
> >>>> test machines, looking into it right now.
> >>> Well, my lid issue seems to be unrelated as it doesn't result from any patches in the
> >>> series in question.
> >>>
> >>> First off, please clone 5.3-rc5 from kernel.org and double check if the issue is not
> >>> present in that one.
> >>>
> >>> If that's not the case, merge the pm-s2idle-rework branch from my tree on top of it
> >>> and retest.
> >>>
> >>> If you still see the issue then, apply the appended patch (on top of the pm-s2idle-reqork
> >>> branch ) and, after starting the kernel, do
> >>>
> >>> # echo 1 > /sys/power/pm_debug_messages
> >>>
> >>> suspend the system and try to wake it up through all of the ways that stopped working.
> >>>
> >>> Then, wake it up with the power button, save the output of dmesg and send it to me.
> >>>
> >>> Thanks!
> >> With 5.3-rc5 the laptops wakes up without any issue when pressing a key
> >> or opening the lid.
> >> With v5.3-rc5+pm-s2idle-testing I can only wake the laptop by pressing
> >> the power button.
> > OK, thanks for verifying.
> >
> > So it is unclear to me how the series can cause an issue like that to appear.
> >
> >> dmesg with pm_debug_messages=1 and your patch:
> >> [   55.646109] PM: suspend entry (s2idle)
> >> [   55.698559] Filesystems sync: 0.052 seconds
> >> [   55.698561] PM: Preparing system for sleep (s2idle)
> >> [   55.700661] Freezing user space processes ... (elapsed 0.210 seconds)
> >> done.
> >> [   55.911494] OOM killer disabled.
> >> [   55.911495] Freezing remaining freezable tasks ... (elapsed 0.001
> >> seconds) done.
> >> [   55.913192] PM: Suspending system (s2idle)
> >> [   55.913195] printk: Suspending console(s) (use no_console_suspend to
> >> debug)
> >> [   55.914778] [drm] CT: disabled
> >> [   55.916057] wlan0: deauthenticating from 64:70:02:a5:fd:02 by local
> >> choice (Reason: 3=DEAUTH_LEAVING)
> >> [   56.045634] sd 2:0:0:0: [sda] Synchronizing SCSI cache
> >> [   56.046650] sd 2:0:0:0: [sda] Stopping disk
> >> [   56.287622] PM: suspend of devices complete after 371.285 msecs
> >> [   56.287627] PM: start suspend of devices complete after 373.684 msecs
> >> [   56.307155] PM: late suspend of devices complete after 19.477 msecs
> >> [   56.312479] ACPI: EC: interrupt blocked
> >> [   56.352761] PM: noirq suspend of devices complete after 45.205 msecs
> >> [   56.352770] ACPI: \_PR_.PR00: LPI: Device not power manageable
> >> [   56.352774] ACPI: \_PR_.PR01: LPI: Device not power manageable
> >> [   56.352776] ACPI: \_PR_.PR02: LPI: Device not power manageable
> >> [   56.352779] ACPI: \_PR_.PR03: LPI: Device not power manageable
> >> [   56.352782] ACPI: \_PR_.PR04: LPI: Device not power manageable
> >> [   56.352785] ACPI: \_PR_.PR05: LPI: Device not power manageable
> >> [   56.352788] ACPI: \_PR_.PR06: LPI: Device not power manageable
> >> [   56.352790] ACPI: \_PR_.PR07: LPI: Device not power manageable
> >> [   56.352793] ACPI: \_SB_.PCI0.GFX0: LPI: Device not power manageable
> >> [   56.352800] ACPI: \_SB_.PCI0.RP06.PXSX: LPI: Device not power manageable
> >> [   56.357057] PM: suspend-to-idle
> >> [   69.338656] PM: Timekeeping suspended for 12.178 seconds
> >> [   69.338701] PM: irq_pm_check_wakeup: IRQ 9
> >> [   69.338704] PM: IRQ wakeup: IRQ 9
> > This clearly is the power button event causing the system to wake up.
> > The other actions, whatever they were, didn't cause any interrupts to
> > be triggered.
> >
> > I suspect that the issue is related to the EC, so please try to revert commit
> >
> > fcd0a04267ac ACPI: PM: s2idle: Switch EC over to polling during "noirq" suspend
> >
> > and see if that makes any difference (should revert cleanly).
> >
> > If that doesn't make any difference, please also try to revert commits
> > (on top of the above revert)
> >
> > 11f26633cccb PM: suspend: Fix platform_suspend_prepare_noirq()
> > ac9eafbe930a ACPI: PM: s2idle: Execute LPS0 _DSM functions with
> > suspended devices
> >
> > (in this order) and retest.
> Reverting the following commits, didn't fix the issue:
> fcd0a04267ac ACPI: PM: s2idle: Switch EC over to polling during "noirq"
> suspend
> 6e86633a791f ACPI: PM: s2idle: Eliminate acpi_sleep_no_ec_events()
> 11f26633cccb PM: suspend: Fix platform_suspend_prepare_noirq()
> ac9eafbe930a ACPI: PM: s2idle: Execute LPS0 _DSM functions with
> suspended devices
>
> I didn't bother reverting all the commits, so I did a checkout of:
> b605c44c30b5 PM: sleep: Drop dpm_noirq_begin() and dpm_noirq_end()
> and everything works, then I did a checkout of:
> 10a08fd65ec1 ACPI: PM: Set up EC GPE for system wakeup from drivers that
> need it
> and the laptop won't wake when opening the lid or pressing a key.
>
> So 10a08fd65ec1 must be the culprit.

Good job, thanks!

The assumption in there was that the EC GPE would not need to be set
up for wakeup unless it is needed either by the intel-hid or by the
intel-vbtn driver.  On your platform it needs to be set up for wakeup
even though neither of these drivers is in use.

Let me cut a fix patch and get back to you when it's ready.

Thanks!

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v3 0/8] PM / ACPI: sleep: Additional changes related to suspend-to-idle
  2019-08-20 13:29             ` Rafael J. Wysocki
@ 2019-08-20 21:38               ` Rafael J. Wysocki
  2019-08-20 22:47                 ` Kristian Klausen
  0 siblings, 1 reply; 25+ messages in thread
From: Rafael J. Wysocki @ 2019-08-20 21:38 UTC (permalink / raw)
  To: Kristian Klausen
  Cc: Linux ACPI, Linux PM, LKML, Zhang Rui, Rajneesh Bhardwaj,
	Andy Shevchenko, Mario Limonciello, Kai-Heng Feng

On Tuesday, August 20, 2019 3:29:48 PM CEST Rafael J. Wysocki wrote:
> On Tue, Aug 20, 2019 at 3:10 PM Kristian Klausen <kristian@klausen.dk> wrote:
> >
> > On 19.08.2019 22.41, Rafael J. Wysocki wrote:
> > > On Mon, Aug 19, 2019 at 5:47 PM Kristian Klausen <kristian@klausen.dk> wrote:
> > >> On 19.08.2019 11.05, Rafael J. Wysocki wrote:
> > >>> On Monday, August 19, 2019 9:59:02 AM CEST Rafael J. Wysocki wrote:
> > >>>> On Fri, Aug 16, 2019 at 10:26 PM Kristian Klausen <kristian@klausen.dk> wrote:
> > >>>>> On 02.08.2019 12.33, Rafael J. Wysocki wrote:
> > >>>>>> Hi All,
> > >>>>>>
> > >>>>>>>> On top of the "Simplify the suspend-to-idle control flow" patch series
> > >>>>>>>> posted previously:
> > >>>>>>>>
> > >>>>>>>> https://lore.kernel.org/lkml/71085220.z6FKkvYQPX@kreacher/
> > >>>>>>>>
> > >>>>>>>> sanitize the suspend-to-idle flow even further.
> > >>>>>>>>
> > >>>>>>>> First off, decouple EC wakeup from the LPS0 _DSM processing (patch 1).
> > >>>>>>>>
> > >>>>>>>> Next, reorder the code to invoke LPS0 _DSM Functions 5 and 6 in the
> > >>>>>>>> specification-compliant order with respect to suspending and resuming
> > >>>>>>>> devices (patch 2).
> > >>>>>>>>
> > >>>>>>>> Finally, rearrange lps0_device_attach() (patch 3) and add a command line
> > >>>>>>>> switch to prevent the LPS0 _DSM from being used.
> > >>>>>>> The v2 is because I found a (minor) bug in patch 1, decided to use a module
> > >>>>>>> parameter instead of a kernel command line option in patch 4.  Also, there
> > >>>>>>> are 4 new patches:
> > >>>>>>>
> > >>>>>>> Patch 5: Switch the EC over to polling during "noirq" suspend and back
> > >>>>>>> during "noirq" resume.
> > >>>>>>>
> > >>>>>>> Patch 6: Eliminate acpi_sleep_no_ec_events().
> > >>>>>>>
> > >>>>>>> Patch 7: Consolidate some EC code depending on PM_SLEEP.
> > >>>>>>>
> > >>>>>>> Patch 8: Add EC GPE dispatching debug message.
> > >>>>>> The v3 is just a rearranged v2 so as to move the post sensitive patch (previous patch 2)
> > >>>>>> to the end of the series.   [After applying the full series the code is the same as before.]
> > >>>>>>
> > >>>>>> For easier testing, the series (along with some previous patches depended on by it)
> > >>>>>> is available in the pm-s2idle-testing branch of the linux-pm.git tree at kernel.org:
> > >>>>>>
> > >>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git/log/?h=pm-s2idle-testing
> > >>>>> It was just testing this patch series(461fc1caed55), to see if it would
> > >>>>> fix my charging issue
> > >>>>> (https://bugzilla.kernel.org/show_bug.cgi?id=201307), which it didn't.
> > >>>> It is unlikely to help in that case.
> > >> Do you have any idea what the issue could be?
> > > Basically, there are two possibilities: either the OS is expected to
> > > handle the AC/battery switching events, or the platform firmware
> > > should take care of them.  In the former case, the EC should generate
> > > events to be handled by the OS and in the latter one there needs to be
> > > a way to let the platform firmware that it needs to take care of those
> > > events going forward.
> > >
> > > In either case there may be a platform-specific action to be carried
> > > out during suspend and resume to set this up as expected which may be
> > > missing.
> > Thanks for the explanation. I don't think I have the expertise to solve
> > the issue, but at least now I'm one step closer.
> > >
> > >>>>> I did however notice that my laptop (ASUS Zenbook UX430UNR/i7-8550U)
> > >>>>> won't wake when opening the lid or pressing a key, the only way to wake
> > >>>>> the laptop is pressing the power button.
> > >>>>>
> > >>>>> I also tested mainline (5.3.0-rc4 b7e7c85dc7b0) and 5.2.8 and the laptop
> > >>>>> wakes without issue when the lid is opened or a key is presed.
> > >>>>>> Please refer to the changelogs for details.
> > >>>> Thanks for your report.
> > >>>>
> > >>>> I seem to see a similar issue with respect to the lid on one of my
> > >>>> test machines, looking into it right now.
> > >>> Well, my lid issue seems to be unrelated as it doesn't result from any patches in the
> > >>> series in question.
> > >>>
> > >>> First off, please clone 5.3-rc5 from kernel.org and double check if the issue is not
> > >>> present in that one.
> > >>>
> > >>> If that's not the case, merge the pm-s2idle-rework branch from my tree on top of it
> > >>> and retest.
> > >>>
> > >>> If you still see the issue then, apply the appended patch (on top of the pm-s2idle-reqork
> > >>> branch ) and, after starting the kernel, do
> > >>>
> > >>> # echo 1 > /sys/power/pm_debug_messages
> > >>>
> > >>> suspend the system and try to wake it up through all of the ways that stopped working.
> > >>>
> > >>> Then, wake it up with the power button, save the output of dmesg and send it to me.
> > >>>
> > >>> Thanks!
> > >> With 5.3-rc5 the laptops wakes up without any issue when pressing a key
> > >> or opening the lid.
> > >> With v5.3-rc5+pm-s2idle-testing I can only wake the laptop by pressing
> > >> the power button.
> > > OK, thanks for verifying.
> > >
> > > So it is unclear to me how the series can cause an issue like that to appear.
> > >
> > >> dmesg with pm_debug_messages=1 and your patch:
> > >> [   55.646109] PM: suspend entry (s2idle)
> > >> [   55.698559] Filesystems sync: 0.052 seconds
> > >> [   55.698561] PM: Preparing system for sleep (s2idle)
> > >> [   55.700661] Freezing user space processes ... (elapsed 0.210 seconds)
> > >> done.
> > >> [   55.911494] OOM killer disabled.
> > >> [   55.911495] Freezing remaining freezable tasks ... (elapsed 0.001
> > >> seconds) done.
> > >> [   55.913192] PM: Suspending system (s2idle)
> > >> [   55.913195] printk: Suspending console(s) (use no_console_suspend to
> > >> debug)
> > >> [   55.914778] [drm] CT: disabled
> > >> [   55.916057] wlan0: deauthenticating from 64:70:02:a5:fd:02 by local
> > >> choice (Reason: 3=DEAUTH_LEAVING)
> > >> [   56.045634] sd 2:0:0:0: [sda] Synchronizing SCSI cache
> > >> [   56.046650] sd 2:0:0:0: [sda] Stopping disk
> > >> [   56.287622] PM: suspend of devices complete after 371.285 msecs
> > >> [   56.287627] PM: start suspend of devices complete after 373.684 msecs
> > >> [   56.307155] PM: late suspend of devices complete after 19.477 msecs
> > >> [   56.312479] ACPI: EC: interrupt blocked
> > >> [   56.352761] PM: noirq suspend of devices complete after 45.205 msecs
> > >> [   56.352770] ACPI: \_PR_.PR00: LPI: Device not power manageable
> > >> [   56.352774] ACPI: \_PR_.PR01: LPI: Device not power manageable
> > >> [   56.352776] ACPI: \_PR_.PR02: LPI: Device not power manageable
> > >> [   56.352779] ACPI: \_PR_.PR03: LPI: Device not power manageable
> > >> [   56.352782] ACPI: \_PR_.PR04: LPI: Device not power manageable
> > >> [   56.352785] ACPI: \_PR_.PR05: LPI: Device not power manageable
> > >> [   56.352788] ACPI: \_PR_.PR06: LPI: Device not power manageable
> > >> [   56.352790] ACPI: \_PR_.PR07: LPI: Device not power manageable
> > >> [   56.352793] ACPI: \_SB_.PCI0.GFX0: LPI: Device not power manageable
> > >> [   56.352800] ACPI: \_SB_.PCI0.RP06.PXSX: LPI: Device not power manageable
> > >> [   56.357057] PM: suspend-to-idle
> > >> [   69.338656] PM: Timekeeping suspended for 12.178 seconds
> > >> [   69.338701] PM: irq_pm_check_wakeup: IRQ 9
> > >> [   69.338704] PM: IRQ wakeup: IRQ 9
> > > This clearly is the power button event causing the system to wake up.
> > > The other actions, whatever they were, didn't cause any interrupts to
> > > be triggered.
> > >
> > > I suspect that the issue is related to the EC, so please try to revert commit
> > >
> > > fcd0a04267ac ACPI: PM: s2idle: Switch EC over to polling during "noirq" suspend
> > >
> > > and see if that makes any difference (should revert cleanly).
> > >
> > > If that doesn't make any difference, please also try to revert commits
> > > (on top of the above revert)
> > >
> > > 11f26633cccb PM: suspend: Fix platform_suspend_prepare_noirq()
> > > ac9eafbe930a ACPI: PM: s2idle: Execute LPS0 _DSM functions with
> > > suspended devices
> > >
> > > (in this order) and retest.
> > Reverting the following commits, didn't fix the issue:
> > fcd0a04267ac ACPI: PM: s2idle: Switch EC over to polling during "noirq"
> > suspend
> > 6e86633a791f ACPI: PM: s2idle: Eliminate acpi_sleep_no_ec_events()
> > 11f26633cccb PM: suspend: Fix platform_suspend_prepare_noirq()
> > ac9eafbe930a ACPI: PM: s2idle: Execute LPS0 _DSM functions with
> > suspended devices
> >
> > I didn't bother reverting all the commits, so I did a checkout of:
> > b605c44c30b5 PM: sleep: Drop dpm_noirq_begin() and dpm_noirq_end()
> > and everything works, then I did a checkout of:
> > 10a08fd65ec1 ACPI: PM: Set up EC GPE for system wakeup from drivers that
> > need it
> > and the laptop won't wake when opening the lid or pressing a key.
> >
> > So 10a08fd65ec1 must be the culprit.
> 
> Good job, thanks!
> 
> The assumption in there was that the EC GPE would not need to be set
> up for wakeup unless it is needed either by the intel-hid or by the
> intel-vbtn driver.  On your platform it needs to be set up for wakeup
> even though neither of these drivers is in use.
> 
> Let me cut a fix patch and get back to you when it's ready.

The appended patch should help, so please apply it (on top of
v5.3-rc5+pm-s2idle-testing) and test.

---
 drivers/acpi/ec.c                 |    1 -
 drivers/acpi/sleep.c              |   15 +++++++++++++--
 drivers/platform/x86/intel-hid.c  |    5 +----
 drivers/platform/x86/intel-vbtn.c |    5 +----
 4 files changed, 15 insertions(+), 11 deletions(-)

Index: linux-pm/drivers/acpi/sleep.c
===================================================================
--- linux-pm.orig/drivers/acpi/sleep.c
+++ linux-pm/drivers/acpi/sleep.c
@@ -938,6 +938,13 @@ static int lps0_device_attach(struct acp
 	if (mem_sleep_default > PM_SUSPEND_MEM && !acpi_sleep_default_s3)
 		mem_sleep_current = PM_SUSPEND_TO_IDLE;
 
+	/*
+	 * Some LPS0 systems, like ASUS Zenbook UX430UNR/i7-8550U, require the
+	 * EC GPE to be enabled while suspended for certain wakeup devices to
+	 * work, so mark it as wakeup-capable.
+	 */
+	acpi_ec_mark_gpe_for_wake();
+
 	return 0;
 }
 
@@ -954,8 +961,10 @@ static int acpi_s2idle_begin(void)
 
 static int acpi_s2idle_prepare(void)
 {
-	if (acpi_sci_irq_valid())
+	if (acpi_sci_irq_valid()) {
 		enable_irq_wake(acpi_sci_irq);
+		acpi_ec_set_gpe_wake_mask(ACPI_GPE_ENABLE);
+	}
 
 	acpi_enable_wakeup_devices(ACPI_STATE_S0);
 
@@ -1034,8 +1043,10 @@ static void acpi_s2idle_restore(void)
 
 	acpi_disable_wakeup_devices(ACPI_STATE_S0);
 
-	if (acpi_sci_irq_valid())
+	if (acpi_sci_irq_valid()) {
+		acpi_ec_set_gpe_wake_mask(ACPI_GPE_DISABLE);
 		disable_irq_wake(acpi_sci_irq);
+	}
 }
 
 static void acpi_s2idle_end(void)
Index: linux-pm/drivers/platform/x86/intel-hid.c
===================================================================
--- linux-pm.orig/drivers/platform/x86/intel-hid.c
+++ linux-pm/drivers/platform/x86/intel-hid.c
@@ -257,7 +257,6 @@ static int intel_hid_pm_prepare(struct d
 		struct intel_hid_priv *priv = dev_get_drvdata(device);
 
 		priv->wakeup_mode = true;
-		acpi_ec_set_gpe_wake_mask(ACPI_GPE_ENABLE);
 	}
 	return 0;
 }
@@ -266,10 +265,8 @@ static void intel_hid_pm_complete(struct
 {
 	struct intel_hid_priv *priv = dev_get_drvdata(device);
 
-	if (priv->wakeup_mode) {
-		acpi_ec_set_gpe_wake_mask(ACPI_GPE_DISABLE);
+	if (priv->wakeup_mode)
 		priv->wakeup_mode = false;
-	}
 }
 
 static int intel_hid_pl_suspend_handler(struct device *device)
Index: linux-pm/drivers/platform/x86/intel-vbtn.c
===================================================================
--- linux-pm.orig/drivers/platform/x86/intel-vbtn.c
+++ linux-pm/drivers/platform/x86/intel-vbtn.c
@@ -205,7 +205,6 @@ static int intel_vbtn_pm_prepare(struct
 		struct intel_vbtn_priv *priv = dev_get_drvdata(dev);
 
 		priv->wakeup_mode = true;
-		acpi_ec_set_gpe_wake_mask(ACPI_GPE_ENABLE);
 	}
 	return 0;
 }
@@ -214,10 +213,8 @@ static void intel_vbtn_pm_complete(struc
 {
 	struct intel_vbtn_priv *priv = dev_get_drvdata(dev);
 
-	if (priv->wakeup_mode) {
-		acpi_ec_set_gpe_wake_mask(ACPI_GPE_DISABLE);
+	if (priv->wakeup_mode)
 		priv->wakeup_mode = false;
-	}
 }
 
 static int intel_vbtn_pm_resume(struct device *dev)
Index: linux-pm/drivers/acpi/ec.c
===================================================================
--- linux-pm.orig/drivers/acpi/ec.c
+++ linux-pm/drivers/acpi/ec.c
@@ -1970,7 +1970,6 @@ void acpi_ec_set_gpe_wake_mask(u8 action
 	if (pm_suspend_no_platform() && first_ec && !ec_no_wakeup)
 		acpi_set_gpe_wake_mask(NULL, first_ec->gpe, action);
 }
-EXPORT_SYMBOL_GPL(acpi_ec_set_gpe_wake_mask);
 
 bool acpi_ec_dispatch_gpe(void)
 {




^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v3 0/8] PM / ACPI: sleep: Additional changes related to suspend-to-idle
  2019-08-20 21:38               ` Rafael J. Wysocki
@ 2019-08-20 22:47                 ` Kristian Klausen
  2019-08-21  7:30                   ` Rafael J. Wysocki
  0 siblings, 1 reply; 25+ messages in thread
From: Kristian Klausen @ 2019-08-20 22:47 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Linux ACPI, Linux PM, LKML, Zhang Rui, Rajneesh Bhardwaj,
	Andy Shevchenko, Mario Limonciello, Kai-Heng Feng

On 20.08.2019 23.38, Rafael J. Wysocki wrote:
> On Tuesday, August 20, 2019 3:29:48 PM CEST Rafael J. Wysocki wrote:
>> On Tue, Aug 20, 2019 at 3:10 PM Kristian Klausen <kristian@klausen.dk> wrote:
>>> On 19.08.2019 22.41, Rafael J. Wysocki wrote:
>>>> On Mon, Aug 19, 2019 at 5:47 PM Kristian Klausen <kristian@klausen.dk> wrote:
>>>>> On 19.08.2019 11.05, Rafael J. Wysocki wrote:
>>>>>> On Monday, August 19, 2019 9:59:02 AM CEST Rafael J. Wysocki wrote:
>>>>>>> On Fri, Aug 16, 2019 at 10:26 PM Kristian Klausen <kristian@klausen.dk> wrote:
>>>>>>>> On 02.08.2019 12.33, Rafael J. Wysocki wrote:
>>>>>>>>> Hi All,
>>>>>>>>>
>>>>>>>>>>> On top of the "Simplify the suspend-to-idle control flow" patch series
>>>>>>>>>>> posted previously:
>>>>>>>>>>>
>>>>>>>>>>> https://lore.kernel.org/lkml/71085220.z6FKkvYQPX@kreacher/
>>>>>>>>>>>
>>>>>>>>>>> sanitize the suspend-to-idle flow even further.
>>>>>>>>>>>
>>>>>>>>>>> First off, decouple EC wakeup from the LPS0 _DSM processing (patch 1).
>>>>>>>>>>>
>>>>>>>>>>> Next, reorder the code to invoke LPS0 _DSM Functions 5 and 6 in the
>>>>>>>>>>> specification-compliant order with respect to suspending and resuming
>>>>>>>>>>> devices (patch 2).
>>>>>>>>>>>
>>>>>>>>>>> Finally, rearrange lps0_device_attach() (patch 3) and add a command line
>>>>>>>>>>> switch to prevent the LPS0 _DSM from being used.
>>>>>>>>>> The v2 is because I found a (minor) bug in patch 1, decided to use a module
>>>>>>>>>> parameter instead of a kernel command line option in patch 4.  Also, there
>>>>>>>>>> are 4 new patches:
>>>>>>>>>>
>>>>>>>>>> Patch 5: Switch the EC over to polling during "noirq" suspend and back
>>>>>>>>>> during "noirq" resume.
>>>>>>>>>>
>>>>>>>>>> Patch 6: Eliminate acpi_sleep_no_ec_events().
>>>>>>>>>>
>>>>>>>>>> Patch 7: Consolidate some EC code depending on PM_SLEEP.
>>>>>>>>>>
>>>>>>>>>> Patch 8: Add EC GPE dispatching debug message.
>>>>>>>>> The v3 is just a rearranged v2 so as to move the post sensitive patch (previous patch 2)
>>>>>>>>> to the end of the series.   [After applying the full series the code is the same as before.]
>>>>>>>>>
>>>>>>>>> For easier testing, the series (along with some previous patches depended on by it)
>>>>>>>>> is available in the pm-s2idle-testing branch of the linux-pm.git tree at kernel.org:
>>>>>>>>>
>>>>>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git/log/?h=pm-s2idle-testing
>>>>>>>> It was just testing this patch series(461fc1caed55), to see if it would
>>>>>>>> fix my charging issue
>>>>>>>> (https://bugzilla.kernel.org/show_bug.cgi?id=201307), which it didn't.
>>>>>>> It is unlikely to help in that case.
>>>>> Do you have any idea what the issue could be?
>>>> Basically, there are two possibilities: either the OS is expected to
>>>> handle the AC/battery switching events, or the platform firmware
>>>> should take care of them.  In the former case, the EC should generate
>>>> events to be handled by the OS and in the latter one there needs to be
>>>> a way to let the platform firmware that it needs to take care of those
>>>> events going forward.
>>>>
>>>> In either case there may be a platform-specific action to be carried
>>>> out during suspend and resume to set this up as expected which may be
>>>> missing.
>>> Thanks for the explanation. I don't think I have the expertise to solve
>>> the issue, but at least now I'm one step closer.
>>>>>>>> I did however notice that my laptop (ASUS Zenbook UX430UNR/i7-8550U)
>>>>>>>> won't wake when opening the lid or pressing a key, the only way to wake
>>>>>>>> the laptop is pressing the power button.
>>>>>>>>
>>>>>>>> I also tested mainline (5.3.0-rc4 b7e7c85dc7b0) and 5.2.8 and the laptop
>>>>>>>> wakes without issue when the lid is opened or a key is presed.
>>>>>>>>> Please refer to the changelogs for details.
>>>>>>> Thanks for your report.
>>>>>>>
>>>>>>> I seem to see a similar issue with respect to the lid on one of my
>>>>>>> test machines, looking into it right now.
>>>>>> Well, my lid issue seems to be unrelated as it doesn't result from any patches in the
>>>>>> series in question.
>>>>>>
>>>>>> First off, please clone 5.3-rc5 from kernel.org and double check if the issue is not
>>>>>> present in that one.
>>>>>>
>>>>>> If that's not the case, merge the pm-s2idle-rework branch from my tree on top of it
>>>>>> and retest.
>>>>>>
>>>>>> If you still see the issue then, apply the appended patch (on top of the pm-s2idle-reqork
>>>>>> branch ) and, after starting the kernel, do
>>>>>>
>>>>>> # echo 1 > /sys/power/pm_debug_messages
>>>>>>
>>>>>> suspend the system and try to wake it up through all of the ways that stopped working.
>>>>>>
>>>>>> Then, wake it up with the power button, save the output of dmesg and send it to me.
>>>>>>
>>>>>> Thanks!
>>>>> With 5.3-rc5 the laptops wakes up without any issue when pressing a key
>>>>> or opening the lid.
>>>>> With v5.3-rc5+pm-s2idle-testing I can only wake the laptop by pressing
>>>>> the power button.
>>>> OK, thanks for verifying.
>>>>
>>>> So it is unclear to me how the series can cause an issue like that to appear.
>>>>
>>>>> dmesg with pm_debug_messages=1 and your patch:
>>>>> [   55.646109] PM: suspend entry (s2idle)
>>>>> [   55.698559] Filesystems sync: 0.052 seconds
>>>>> [   55.698561] PM: Preparing system for sleep (s2idle)
>>>>> [   55.700661] Freezing user space processes ... (elapsed 0.210 seconds)
>>>>> done.
>>>>> [   55.911494] OOM killer disabled.
>>>>> [   55.911495] Freezing remaining freezable tasks ... (elapsed 0.001
>>>>> seconds) done.
>>>>> [   55.913192] PM: Suspending system (s2idle)
>>>>> [   55.913195] printk: Suspending console(s) (use no_console_suspend to
>>>>> debug)
>>>>> [   55.914778] [drm] CT: disabled
>>>>> [   55.916057] wlan0: deauthenticating from 64:70:02:a5:fd:02 by local
>>>>> choice (Reason: 3=DEAUTH_LEAVING)
>>>>> [   56.045634] sd 2:0:0:0: [sda] Synchronizing SCSI cache
>>>>> [   56.046650] sd 2:0:0:0: [sda] Stopping disk
>>>>> [   56.287622] PM: suspend of devices complete after 371.285 msecs
>>>>> [   56.287627] PM: start suspend of devices complete after 373.684 msecs
>>>>> [   56.307155] PM: late suspend of devices complete after 19.477 msecs
>>>>> [   56.312479] ACPI: EC: interrupt blocked
>>>>> [   56.352761] PM: noirq suspend of devices complete after 45.205 msecs
>>>>> [   56.352770] ACPI: \_PR_.PR00: LPI: Device not power manageable
>>>>> [   56.352774] ACPI: \_PR_.PR01: LPI: Device not power manageable
>>>>> [   56.352776] ACPI: \_PR_.PR02: LPI: Device not power manageable
>>>>> [   56.352779] ACPI: \_PR_.PR03: LPI: Device not power manageable
>>>>> [   56.352782] ACPI: \_PR_.PR04: LPI: Device not power manageable
>>>>> [   56.352785] ACPI: \_PR_.PR05: LPI: Device not power manageable
>>>>> [   56.352788] ACPI: \_PR_.PR06: LPI: Device not power manageable
>>>>> [   56.352790] ACPI: \_PR_.PR07: LPI: Device not power manageable
>>>>> [   56.352793] ACPI: \_SB_.PCI0.GFX0: LPI: Device not power manageable
>>>>> [   56.352800] ACPI: \_SB_.PCI0.RP06.PXSX: LPI: Device not power manageable
>>>>> [   56.357057] PM: suspend-to-idle
>>>>> [   69.338656] PM: Timekeeping suspended for 12.178 seconds
>>>>> [   69.338701] PM: irq_pm_check_wakeup: IRQ 9
>>>>> [   69.338704] PM: IRQ wakeup: IRQ 9
>>>> This clearly is the power button event causing the system to wake up.
>>>> The other actions, whatever they were, didn't cause any interrupts to
>>>> be triggered.
>>>>
>>>> I suspect that the issue is related to the EC, so please try to revert commit
>>>>
>>>> fcd0a04267ac ACPI: PM: s2idle: Switch EC over to polling during "noirq" suspend
>>>>
>>>> and see if that makes any difference (should revert cleanly).
>>>>
>>>> If that doesn't make any difference, please also try to revert commits
>>>> (on top of the above revert)
>>>>
>>>> 11f26633cccb PM: suspend: Fix platform_suspend_prepare_noirq()
>>>> ac9eafbe930a ACPI: PM: s2idle: Execute LPS0 _DSM functions with
>>>> suspended devices
>>>>
>>>> (in this order) and retest.
>>> Reverting the following commits, didn't fix the issue:
>>> fcd0a04267ac ACPI: PM: s2idle: Switch EC over to polling during "noirq"
>>> suspend
>>> 6e86633a791f ACPI: PM: s2idle: Eliminate acpi_sleep_no_ec_events()
>>> 11f26633cccb PM: suspend: Fix platform_suspend_prepare_noirq()
>>> ac9eafbe930a ACPI: PM: s2idle: Execute LPS0 _DSM functions with
>>> suspended devices
>>>
>>> I didn't bother reverting all the commits, so I did a checkout of:
>>> b605c44c30b5 PM: sleep: Drop dpm_noirq_begin() and dpm_noirq_end()
>>> and everything works, then I did a checkout of:
>>> 10a08fd65ec1 ACPI: PM: Set up EC GPE for system wakeup from drivers that
>>> need it
>>> and the laptop won't wake when opening the lid or pressing a key.
>>>
>>> So 10a08fd65ec1 must be the culprit.
>> Good job, thanks!
>>
>> The assumption in there was that the EC GPE would not need to be set
>> up for wakeup unless it is needed either by the intel-hid or by the
>> intel-vbtn driver.  On your platform it needs to be set up for wakeup
>> even though neither of these drivers is in use.
>>
>> Let me cut a fix patch and get back to you when it's ready.
> The appended patch should help, so please apply it (on top of
> v5.3-rc5+pm-s2idle-testing) and test.
It works, pressing a key or opening the lid the laptop wakes instantly.
Thanks!
>
> ---
>   drivers/acpi/ec.c                 |    1 -
>   drivers/acpi/sleep.c              |   15 +++++++++++++--
>   drivers/platform/x86/intel-hid.c  |    5 +----
>   drivers/platform/x86/intel-vbtn.c |    5 +----
>   4 files changed, 15 insertions(+), 11 deletions(-)
>
> Index: linux-pm/drivers/acpi/sleep.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/sleep.c
> +++ linux-pm/drivers/acpi/sleep.c
> @@ -938,6 +938,13 @@ static int lps0_device_attach(struct acp
>   	if (mem_sleep_default > PM_SUSPEND_MEM && !acpi_sleep_default_s3)
>   		mem_sleep_current = PM_SUSPEND_TO_IDLE;
>   
> +	/*
> +	 * Some LPS0 systems, like ASUS Zenbook UX430UNR/i7-8550U, require the
> +	 * EC GPE to be enabled while suspended for certain wakeup devices to
> +	 * work, so mark it as wakeup-capable.
> +	 */
> +	acpi_ec_mark_gpe_for_wake();
> +
>   	return 0;
>   }
>   
> @@ -954,8 +961,10 @@ static int acpi_s2idle_begin(void)
>   
>   static int acpi_s2idle_prepare(void)
>   {
> -	if (acpi_sci_irq_valid())
> +	if (acpi_sci_irq_valid()) {
>   		enable_irq_wake(acpi_sci_irq);
> +		acpi_ec_set_gpe_wake_mask(ACPI_GPE_ENABLE);
> +	}
>   
>   	acpi_enable_wakeup_devices(ACPI_STATE_S0);
>   
> @@ -1034,8 +1043,10 @@ static void acpi_s2idle_restore(void)
>   
>   	acpi_disable_wakeup_devices(ACPI_STATE_S0);
>   
> -	if (acpi_sci_irq_valid())
> +	if (acpi_sci_irq_valid()) {
> +		acpi_ec_set_gpe_wake_mask(ACPI_GPE_DISABLE);
>   		disable_irq_wake(acpi_sci_irq);
> +	}
>   }
>   
>   static void acpi_s2idle_end(void)
> Index: linux-pm/drivers/platform/x86/intel-hid.c
> ===================================================================
> --- linux-pm.orig/drivers/platform/x86/intel-hid.c
> +++ linux-pm/drivers/platform/x86/intel-hid.c
> @@ -257,7 +257,6 @@ static int intel_hid_pm_prepare(struct d
>   		struct intel_hid_priv *priv = dev_get_drvdata(device);
>   
>   		priv->wakeup_mode = true;
> -		acpi_ec_set_gpe_wake_mask(ACPI_GPE_ENABLE);
>   	}
>   	return 0;
>   }
> @@ -266,10 +265,8 @@ static void intel_hid_pm_complete(struct
>   {
>   	struct intel_hid_priv *priv = dev_get_drvdata(device);
>   
> -	if (priv->wakeup_mode) {
> -		acpi_ec_set_gpe_wake_mask(ACPI_GPE_DISABLE);
> +	if (priv->wakeup_mode)
>   		priv->wakeup_mode = false;
> -	}
>   }
>   
>   static int intel_hid_pl_suspend_handler(struct device *device)
> Index: linux-pm/drivers/platform/x86/intel-vbtn.c
> ===================================================================
> --- linux-pm.orig/drivers/platform/x86/intel-vbtn.c
> +++ linux-pm/drivers/platform/x86/intel-vbtn.c
> @@ -205,7 +205,6 @@ static int intel_vbtn_pm_prepare(struct
>   		struct intel_vbtn_priv *priv = dev_get_drvdata(dev);
>   
>   		priv->wakeup_mode = true;
> -		acpi_ec_set_gpe_wake_mask(ACPI_GPE_ENABLE);
>   	}
>   	return 0;
>   }
> @@ -214,10 +213,8 @@ static void intel_vbtn_pm_complete(struc
>   {
>   	struct intel_vbtn_priv *priv = dev_get_drvdata(dev);
>   
> -	if (priv->wakeup_mode) {
> -		acpi_ec_set_gpe_wake_mask(ACPI_GPE_DISABLE);
> +	if (priv->wakeup_mode)
>   		priv->wakeup_mode = false;
> -	}
>   }
>   
>   static int intel_vbtn_pm_resume(struct device *dev)
> Index: linux-pm/drivers/acpi/ec.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/ec.c
> +++ linux-pm/drivers/acpi/ec.c
> @@ -1970,7 +1970,6 @@ void acpi_ec_set_gpe_wake_mask(u8 action
>   	if (pm_suspend_no_platform() && first_ec && !ec_no_wakeup)
>   		acpi_set_gpe_wake_mask(NULL, first_ec->gpe, action);
>   }
> -EXPORT_SYMBOL_GPL(acpi_ec_set_gpe_wake_mask);
>   
>   bool acpi_ec_dispatch_gpe(void)
>   {
>
>
>


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v3 0/8] PM / ACPI: sleep: Additional changes related to suspend-to-idle
  2019-08-20 22:47                 ` Kristian Klausen
@ 2019-08-21  7:30                   ` Rafael J. Wysocki
  0 siblings, 0 replies; 25+ messages in thread
From: Rafael J. Wysocki @ 2019-08-21  7:30 UTC (permalink / raw)
  To: Kristian Klausen
  Cc: Rafael J. Wysocki, Linux ACPI, Linux PM, LKML, Zhang Rui,
	Rajneesh Bhardwaj, Andy Shevchenko, Mario Limonciello,
	Kai-Heng Feng

On Wed, Aug 21, 2019 at 12:47 AM Kristian Klausen <kristian@klausen.dk> wrote:
>
> On 20.08.2019 23.38, Rafael J. Wysocki wrote:
> > On Tuesday, August 20, 2019 3:29:48 PM CEST Rafael J. Wysocki wrote:
> >> On Tue, Aug 20, 2019 at 3:10 PM Kristian Klausen <kristian@klausen.dk> wrote:
> >>> On 19.08.2019 22.41, Rafael J. Wysocki wrote:
> >>>> On Mon, Aug 19, 2019 at 5:47 PM Kristian Klausen <kristian@klausen.dk> wrote:
> >>>>> On 19.08.2019 11.05, Rafael J. Wysocki wrote:
> >>>>>> On Monday, August 19, 2019 9:59:02 AM CEST Rafael J. Wysocki wrote:
> >>>>>>> On Fri, Aug 16, 2019 at 10:26 PM Kristian Klausen <kristian@klausen.dk> wrote:
> >>>>>>>> On 02.08.2019 12.33, Rafael J. Wysocki wrote:
> >>>>>>>>> Hi All,
> >>>>>>>>>
> >>>>>>>>>>> On top of the "Simplify the suspend-to-idle control flow" patch series
> >>>>>>>>>>> posted previously:
> >>>>>>>>>>>
> >>>>>>>>>>> https://lore.kernel.org/lkml/71085220.z6FKkvYQPX@kreacher/
> >>>>>>>>>>>
> >>>>>>>>>>> sanitize the suspend-to-idle flow even further.
> >>>>>>>>>>>
> >>>>>>>>>>> First off, decouple EC wakeup from the LPS0 _DSM processing (patch 1).
> >>>>>>>>>>>
> >>>>>>>>>>> Next, reorder the code to invoke LPS0 _DSM Functions 5 and 6 in the
> >>>>>>>>>>> specification-compliant order with respect to suspending and resuming
> >>>>>>>>>>> devices (patch 2).
> >>>>>>>>>>>
> >>>>>>>>>>> Finally, rearrange lps0_device_attach() (patch 3) and add a command line
> >>>>>>>>>>> switch to prevent the LPS0 _DSM from being used.
> >>>>>>>>>> The v2 is because I found a (minor) bug in patch 1, decided to use a module
> >>>>>>>>>> parameter instead of a kernel command line option in patch 4.  Also, there
> >>>>>>>>>> are 4 new patches:
> >>>>>>>>>>
> >>>>>>>>>> Patch 5: Switch the EC over to polling during "noirq" suspend and back
> >>>>>>>>>> during "noirq" resume.
> >>>>>>>>>>
> >>>>>>>>>> Patch 6: Eliminate acpi_sleep_no_ec_events().
> >>>>>>>>>>
> >>>>>>>>>> Patch 7: Consolidate some EC code depending on PM_SLEEP.
> >>>>>>>>>>
> >>>>>>>>>> Patch 8: Add EC GPE dispatching debug message.
> >>>>>>>>> The v3 is just a rearranged v2 so as to move the post sensitive patch (previous patch 2)
> >>>>>>>>> to the end of the series.   [After applying the full series the code is the same as before.]
> >>>>>>>>>
> >>>>>>>>> For easier testing, the series (along with some previous patches depended on by it)
> >>>>>>>>> is available in the pm-s2idle-testing branch of the linux-pm.git tree at kernel.org:
> >>>>>>>>>
> >>>>>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git/log/?h=pm-s2idle-testing
> >>>>>>>> It was just testing this patch series(461fc1caed55), to see if it would
> >>>>>>>> fix my charging issue
> >>>>>>>> (https://bugzilla.kernel.org/show_bug.cgi?id=201307), which it didn't.
> >>>>>>> It is unlikely to help in that case.
> >>>>> Do you have any idea what the issue could be?
> >>>> Basically, there are two possibilities: either the OS is expected to
> >>>> handle the AC/battery switching events, or the platform firmware
> >>>> should take care of them.  In the former case, the EC should generate
> >>>> events to be handled by the OS and in the latter one there needs to be
> >>>> a way to let the platform firmware that it needs to take care of those
> >>>> events going forward.
> >>>>
> >>>> In either case there may be a platform-specific action to be carried
> >>>> out during suspend and resume to set this up as expected which may be
> >>>> missing.
> >>> Thanks for the explanation. I don't think I have the expertise to solve
> >>> the issue, but at least now I'm one step closer.
> >>>>>>>> I did however notice that my laptop (ASUS Zenbook UX430UNR/i7-8550U)
> >>>>>>>> won't wake when opening the lid or pressing a key, the only way to wake
> >>>>>>>> the laptop is pressing the power button.
> >>>>>>>>
> >>>>>>>> I also tested mainline (5.3.0-rc4 b7e7c85dc7b0) and 5.2.8 and the laptop
> >>>>>>>> wakes without issue when the lid is opened or a key is presed.
> >>>>>>>>> Please refer to the changelogs for details.
> >>>>>>> Thanks for your report.
> >>>>>>>
> >>>>>>> I seem to see a similar issue with respect to the lid on one of my
> >>>>>>> test machines, looking into it right now.
> >>>>>> Well, my lid issue seems to be unrelated as it doesn't result from any patches in the
> >>>>>> series in question.
> >>>>>>
> >>>>>> First off, please clone 5.3-rc5 from kernel.org and double check if the issue is not
> >>>>>> present in that one.
> >>>>>>
> >>>>>> If that's not the case, merge the pm-s2idle-rework branch from my tree on top of it
> >>>>>> and retest.
> >>>>>>
> >>>>>> If you still see the issue then, apply the appended patch (on top of the pm-s2idle-reqork
> >>>>>> branch ) and, after starting the kernel, do
> >>>>>>
> >>>>>> # echo 1 > /sys/power/pm_debug_messages
> >>>>>>
> >>>>>> suspend the system and try to wake it up through all of the ways that stopped working.
> >>>>>>
> >>>>>> Then, wake it up with the power button, save the output of dmesg and send it to me.
> >>>>>>
> >>>>>> Thanks!
> >>>>> With 5.3-rc5 the laptops wakes up without any issue when pressing a key
> >>>>> or opening the lid.
> >>>>> With v5.3-rc5+pm-s2idle-testing I can only wake the laptop by pressing
> >>>>> the power button.
> >>>> OK, thanks for verifying.
> >>>>
> >>>> So it is unclear to me how the series can cause an issue like that to appear.
> >>>>
> >>>>> dmesg with pm_debug_messages=1 and your patch:
> >>>>> [   55.646109] PM: suspend entry (s2idle)
> >>>>> [   55.698559] Filesystems sync: 0.052 seconds
> >>>>> [   55.698561] PM: Preparing system for sleep (s2idle)
> >>>>> [   55.700661] Freezing user space processes ... (elapsed 0.210 seconds)
> >>>>> done.
> >>>>> [   55.911494] OOM killer disabled.
> >>>>> [   55.911495] Freezing remaining freezable tasks ... (elapsed 0.001
> >>>>> seconds) done.
> >>>>> [   55.913192] PM: Suspending system (s2idle)
> >>>>> [   55.913195] printk: Suspending console(s) (use no_console_suspend to
> >>>>> debug)
> >>>>> [   55.914778] [drm] CT: disabled
> >>>>> [   55.916057] wlan0: deauthenticating from 64:70:02:a5:fd:02 by local
> >>>>> choice (Reason: 3=DEAUTH_LEAVING)
> >>>>> [   56.045634] sd 2:0:0:0: [sda] Synchronizing SCSI cache
> >>>>> [   56.046650] sd 2:0:0:0: [sda] Stopping disk
> >>>>> [   56.287622] PM: suspend of devices complete after 371.285 msecs
> >>>>> [   56.287627] PM: start suspend of devices complete after 373.684 msecs
> >>>>> [   56.307155] PM: late suspend of devices complete after 19.477 msecs
> >>>>> [   56.312479] ACPI: EC: interrupt blocked
> >>>>> [   56.352761] PM: noirq suspend of devices complete after 45.205 msecs
> >>>>> [   56.352770] ACPI: \_PR_.PR00: LPI: Device not power manageable
> >>>>> [   56.352774] ACPI: \_PR_.PR01: LPI: Device not power manageable
> >>>>> [   56.352776] ACPI: \_PR_.PR02: LPI: Device not power manageable
> >>>>> [   56.352779] ACPI: \_PR_.PR03: LPI: Device not power manageable
> >>>>> [   56.352782] ACPI: \_PR_.PR04: LPI: Device not power manageable
> >>>>> [   56.352785] ACPI: \_PR_.PR05: LPI: Device not power manageable
> >>>>> [   56.352788] ACPI: \_PR_.PR06: LPI: Device not power manageable
> >>>>> [   56.352790] ACPI: \_PR_.PR07: LPI: Device not power manageable
> >>>>> [   56.352793] ACPI: \_SB_.PCI0.GFX0: LPI: Device not power manageable
> >>>>> [   56.352800] ACPI: \_SB_.PCI0.RP06.PXSX: LPI: Device not power manageable
> >>>>> [   56.357057] PM: suspend-to-idle
> >>>>> [   69.338656] PM: Timekeeping suspended for 12.178 seconds
> >>>>> [   69.338701] PM: irq_pm_check_wakeup: IRQ 9
> >>>>> [   69.338704] PM: IRQ wakeup: IRQ 9
> >>>> This clearly is the power button event causing the system to wake up.
> >>>> The other actions, whatever they were, didn't cause any interrupts to
> >>>> be triggered.
> >>>>
> >>>> I suspect that the issue is related to the EC, so please try to revert commit
> >>>>
> >>>> fcd0a04267ac ACPI: PM: s2idle: Switch EC over to polling during "noirq" suspend
> >>>>
> >>>> and see if that makes any difference (should revert cleanly).
> >>>>
> >>>> If that doesn't make any difference, please also try to revert commits
> >>>> (on top of the above revert)
> >>>>
> >>>> 11f26633cccb PM: suspend: Fix platform_suspend_prepare_noirq()
> >>>> ac9eafbe930a ACPI: PM: s2idle: Execute LPS0 _DSM functions with
> >>>> suspended devices
> >>>>
> >>>> (in this order) and retest.
> >>> Reverting the following commits, didn't fix the issue:
> >>> fcd0a04267ac ACPI: PM: s2idle: Switch EC over to polling during "noirq"
> >>> suspend
> >>> 6e86633a791f ACPI: PM: s2idle: Eliminate acpi_sleep_no_ec_events()
> >>> 11f26633cccb PM: suspend: Fix platform_suspend_prepare_noirq()
> >>> ac9eafbe930a ACPI: PM: s2idle: Execute LPS0 _DSM functions with
> >>> suspended devices
> >>>
> >>> I didn't bother reverting all the commits, so I did a checkout of:
> >>> b605c44c30b5 PM: sleep: Drop dpm_noirq_begin() and dpm_noirq_end()
> >>> and everything works, then I did a checkout of:
> >>> 10a08fd65ec1 ACPI: PM: Set up EC GPE for system wakeup from drivers that
> >>> need it
> >>> and the laptop won't wake when opening the lid or pressing a key.
> >>>
> >>> So 10a08fd65ec1 must be the culprit.
> >> Good job, thanks!
> >>
> >> The assumption in there was that the EC GPE would not need to be set
> >> up for wakeup unless it is needed either by the intel-hid or by the
> >> intel-vbtn driver.  On your platform it needs to be set up for wakeup
> >> even though neither of these drivers is in use.
> >>
> >> Let me cut a fix patch and get back to you when it's ready.
> > The appended patch should help, so please apply it (on top of
> > v5.3-rc5+pm-s2idle-testing) and test.
> It works, pressing a key or opening the lid the laptop wakes instantly.
> Thanks!

Thanks for the confirmation!

I'll resend it with a changelog and tags, then.

^ permalink raw reply	[flat|nested] 25+ messages in thread

end of thread, other threads:[~2019-08-21  7:31 UTC | newest]

Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-02 10:33 [PATCH v3 0/8] PM / ACPI: sleep: Additional changes related to suspend-to-idle Rafael J. Wysocki
2019-08-02 10:38 ` [PATCH v3 1/8] ACPI: PM: Set up EC GPE for system wakeup from drivers that need it Rafael J. Wysocki
2019-08-02 10:38 ` [PATCH v3 2/8] ACPI: PM: s2idle: Rearrange lps0_device_attach() Rafael J. Wysocki
2019-08-02 10:38 ` [PATCH v3 3/8] ACPI: PM: s2idle: Add acpi.sleep_no_lps0 module parameter Rafael J. Wysocki
2019-08-02 10:40 ` [PATCH v3 4/8] ACPI: PM: s2idle: Switch EC over to polling during "noirq" suspend Rafael J. Wysocki
2019-08-02 10:41 ` [PATCH v3 5/8] ACPI: PM: s2idle: Eliminate acpi_sleep_no_ec_events() Rafael J. Wysocki
2019-08-02 10:43 ` [PATCH v3 6/8] ACPI: EC: PM: Consolidate some code depending on PM_SLEEP Rafael J. Wysocki
2019-08-02 10:44 ` [PATCH v3 7/8] ACPI: EC: PM: Make acpi_ec_dispatch_gpe() print debug message Rafael J. Wysocki
2019-08-02 10:45 ` [PATCH v3 8/8] ACPI: PM: s2idle: Execute LPS0 _DSM functions with suspended devices Rafael J. Wysocki
     [not found]   ` <CGME20190809120052eucas1p11b56806662ef4f4efb82a152ad651481@eucas1p1.samsung.com>
2019-08-09 12:00     ` Marek Szyprowski
2019-08-09 12:15       ` Rafael J. Wysocki
2019-08-10 11:24       ` [PATCH] PM: suspend: Fix platform_suspend_prepare_noirq() Rafael J. Wysocki
2019-08-05 16:25 ` [PATCH v3 0/8] PM / ACPI: sleep: Additional changes related to suspend-to-idle Kai-Heng Feng
2019-08-12 13:55 ` Bhardwaj, Rajneesh
2019-08-12 21:32   ` Rafael J. Wysocki
2019-08-16 20:20 ` Kristian Klausen
2019-08-19  7:59   ` Rafael J. Wysocki
2019-08-19  9:05     ` Rafael J. Wysocki
2019-08-19 15:45       ` Kristian Klausen
2019-08-19 20:41         ` Rafael J. Wysocki
2019-08-20 13:10           ` Kristian Klausen
2019-08-20 13:29             ` Rafael J. Wysocki
2019-08-20 21:38               ` Rafael J. Wysocki
2019-08-20 22:47                 ` Kristian Klausen
2019-08-21  7:30                   ` Rafael J. Wysocki

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).