[PATCH 0/5] PM / sleep: Support power button wakeup from S2I on recent Dell laptops

[PATCH 0/5] PM / sleep: Support power button wakeup from S2I on recent Dell laptops

[Rafael J. Wysocki]

Hi All,

The underlying issue is that on some relatively new Dell laltops, including
Dell XPS13 9360 and 9365, pressing the power button is not sufficient to
wake up the system from suspend-to-idle (it has to be pressed and held
down for around 5 sec for the wakeup to happen) which is not expected
and does not match the Windows' behavior.

This turns out to be a consequence of the way power button events are signaled
on those systems, which is through the Embedded Controller (EC).  Namely,
button events go to the EC which then signals the event through its ACPI GPE
(General Purpose Event), which triggers an ACPI SCI (System Control Interrupt),
whose handler executes a specicif AML control method and triggers a Notify()
targetting a devie object associated with the power button.  The problem with
suspend-to-idle is that the EC GPE is disabled during suspend, because
otherwise all EC events would wake up the system from suspend-to-idle (and
there can be many of them).

The only way to address this is to allow the EC GPE to stay enabled while
suspended to idle, but that cannot be done unconditionally, because of the
possible spurious wakeup events from the EC.  Fortunately, on the Dell systems
in question it is possible to reduce the number of events coming from the EC
while suspended (see the changelog of patch [3/5] for details), but still some
spurious events may happen.  For this reason, when there is an SCI while
suspended, it is not practical to resume all the way up to user space.
Instead, it is better to resume partially up to the point at which the SCI can
be processed by its handler, which is after calling dpm_resume_noirq(), let
the SCI be processed and if no real wakeup events come out of that,
suspend again.  Actually, that can be done in general, because spurious
SCIs do happen while suspended to idle on other systems too, and that's
which it goes as patch [2/5] before the Dell-related changes.

Patch [1/5] is more of a cleanup, but makes the rest look slightly more
straightforward, and patches [4-5/5] update the drivers used for button
events processing on the affected systems so that they signal wakeup
as expected and avoid propagating the wakeup events as button events
to user space.

The series is available from a git branch at

 git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git s2idle-dell-test

and has been included into the testing branch thereof.

Please let me know if there are any concerns.

Thanks,
Rafael

[PATCH 1/5] PM / wakeup: Integrate mechanism to abort transitions in progress

[Rafael J. Wysocki]

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

The system wakeup framework is not very consistent with respect to
the way it handles suspend-to-idle and generally wakeup events
occurring during transitions to system low-power states.

First off, system transitions in progress are aborted by the event
reporting helpers like pm_wakeup_event() only if the wakeup_count
sysfs attribute is in use (as documented), but there are cases in
which system-wide transitions should be aborted even if that is
not the case.  For example, a wakeup signal from a designated
wakeup device during system-wide PM transition, it should cause
the transition to be aborted right away.

Moreover, there is a freeze_wake() call in wakeup_source_activate(),
but that really is only effective after suspend_freeze_state has
been set to FREEZE_STATE_ENTER by freeze_enter().  However, it
is very unlikely that wakeup_source_activate() will ever be called
at that time, as it could only be triggered by a IRQF_NO_SUSPEND
interrupt handler, so wakeups from suspend-to-idle don't really
occur in wakeup_source_activate().

At the same time there is a way to abort a system suspend in
progress (or wake up the system from suspend-to-idle), which is by
calling pm_system_wakeup(), but in turn that doesn't cause any
wakeup source objects to be activated, so it will not be covered
by wakeup source statistics and will not prevent the system from
suspending again immediately (in case autosleep is used, for
example).  Consequently, if anyone wants to abort system transitions
in progress and allow the wakeup_count mechanism to work, they need
to use both pm_system_wakeup() and pm_wakeup_event(), say, at the
same time which is awkward.

For the above reasons, make it possible to trigger
pm_system_wakeup() from within wakeup_source_activate() and
provide a new pm_wakeup_hard_event() helper to do so within the
wakeup framework.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/base/power/wakeup.c |   36 ++++++++++++++++++------------------
 include/linux/pm_wakeup.h   |   25 +++++++++++++++++++++----
 2 files changed, 39 insertions(+), 22 deletions(-)

Index: linux-pm/include/linux/pm_wakeup.h
===================================================================
--- linux-pm.orig/include/linux/pm_wakeup.h
+++ linux-pm/include/linux/pm_wakeup.h
@@ -106,8 +106,8 @@ extern void __pm_stay_awake(struct wakeu
 extern void pm_stay_awake(struct device *dev);
 extern void __pm_relax(struct wakeup_source *ws);
 extern void pm_relax(struct device *dev);
-extern void __pm_wakeup_event(struct wakeup_source *ws, unsigned int msec);
-extern void pm_wakeup_event(struct device *dev, unsigned int msec);
+extern void pm_wakeup_ws_event(struct wakeup_source *ws, unsigned int msec, bool hard);
+extern void pm_wakeup_dev_event(struct device *dev, unsigned int msec, bool hard);
 
 #else /* !CONFIG_PM_SLEEP */
 
@@ -182,9 +182,11 @@ static inline void __pm_relax(struct wak
 
 static inline void pm_relax(struct device *dev) {}
 
-static inline void __pm_wakeup_event(struct wakeup_source *ws, unsigned int msec) {}
+static inline void pm_wakeup_ws_event(struct wakeup_source *ws,
+				      unsigned int msec, bool hard) {}
 
-static inline void pm_wakeup_event(struct device *dev, unsigned int msec) {}
+static inline void pm_wakeup_dev_event(struct device *dev, unsigned int msec,
+				       bool hard) {}
 
 #endif /* !CONFIG_PM_SLEEP */
 
@@ -201,4 +203,19 @@ static inline void wakeup_source_trash(s
 	wakeup_source_drop(ws);
 }
 
+static inline void __pm_wakeup_event(struct wakeup_source *ws, unsigned int msec)
+{
+	return pm_wakeup_ws_event(ws, msec, false);
+}
+
+static inline void pm_wakeup_event(struct device *dev, unsigned int msec)
+{
+	return pm_wakeup_dev_event(dev, msec, false);
+}
+
+static inline void pm_wakeup_hard_event(struct device *dev)
+{
+	return pm_wakeup_dev_event(dev, 0, true);
+}
+
 #endif /* _LINUX_PM_WAKEUP_H */
Index: linux-pm/drivers/base/power/wakeup.c
===================================================================
--- linux-pm.orig/drivers/base/power/wakeup.c
+++ linux-pm/drivers/base/power/wakeup.c
@@ -512,12 +512,13 @@ static bool wakeup_source_not_registered
 /**
  * wakup_source_activate - Mark given wakeup source as active.
  * @ws: Wakeup source to handle.
+ * @hard: If set, abort suspends in progress and wake up from suspend-to-idle.
  *
  * Update the @ws' statistics and, if @ws has just been activated, notify the PM
  * core of the event by incrementing the counter of of wakeup events being
  * processed.
  */
-static void wakeup_source_activate(struct wakeup_source *ws)
+static void wakeup_source_activate(struct wakeup_source *ws, bool hard)
 {
 	unsigned int cec;
 
@@ -525,11 +526,8 @@ static void wakeup_source_activate(struc
 			"unregistered wakeup source\n"))
 		return;
 
-	/*
-	 * active wakeup source should bring the system
-	 * out of PM_SUSPEND_FREEZE state
-	 */
-	freeze_wake();
+	if (hard)
+		pm_system_wakeup();
 
 	ws->active = true;
 	ws->active_count++;
@@ -546,8 +544,9 @@ static void wakeup_source_activate(struc
 /**
  * wakeup_source_report_event - Report wakeup event using the given source.
  * @ws: Wakeup source to report the event for.
+ * @hard: If set, abort suspends in progress and wake up from suspend-to-idle.
  */
-static void wakeup_source_report_event(struct wakeup_source *ws)
+static void wakeup_source_report_event(struct wakeup_source *ws, bool hard)
 {
 	ws->event_count++;
 	/* This is racy, but the counter is approximate anyway. */
@@ -555,7 +554,7 @@ static void wakeup_source_report_event(s
 		ws->wakeup_count++;
 
 	if (!ws->active)
-		wakeup_source_activate(ws);
+		wakeup_source_activate(ws, hard);
 }
 
 /**
@@ -573,7 +572,7 @@ void __pm_stay_awake(struct wakeup_sourc
 
 	spin_lock_irqsave(&ws->lock, flags);
 
-	wakeup_source_report_event(ws);
+	wakeup_source_report_event(ws, false);
 	del_timer(&ws->timer);
 	ws->timer_expires = 0;
 
@@ -739,9 +738,10 @@ static void pm_wakeup_timer_fn(unsigned
 }
 
 /**
- * __pm_wakeup_event - Notify the PM core of a wakeup event.
+ * pm_wakeup_ws_event - Notify the PM core of a wakeup event.
  * @ws: Wakeup source object associated with the event source.
  * @msec: Anticipated event processing time (in milliseconds).
+ * @hard: If set, abort suspends in progress and wake up from suspend-to-idle.
  *
  * Notify the PM core of a wakeup event whose source is @ws that will take
  * approximately @msec milliseconds to be processed by the kernel.  If @ws is
@@ -750,7 +750,7 @@ static void pm_wakeup_timer_fn(unsigned
  *
  * It is safe to call this function from interrupt context.
  */
-void __pm_wakeup_event(struct wakeup_source *ws, unsigned int msec)
+void pm_wakeup_ws_event(struct wakeup_source *ws, unsigned int msec, bool hard)
 {
 	unsigned long flags;
 	unsigned long expires;
@@ -760,7 +760,7 @@ void __pm_wakeup_event(struct wakeup_sou
 
 	spin_lock_irqsave(&ws->lock, flags);
 
-	wakeup_source_report_event(ws);
+	wakeup_source_report_event(ws, hard);
 
 	if (!msec) {
 		wakeup_source_deactivate(ws);
@@ -779,17 +779,17 @@ void __pm_wakeup_event(struct wakeup_sou
  unlock:
 	spin_unlock_irqrestore(&ws->lock, flags);
 }
-EXPORT_SYMBOL_GPL(__pm_wakeup_event);
-
+EXPORT_SYMBOL_GPL(pm_wakeup_ws_event);
 
 /**
  * pm_wakeup_event - Notify the PM core of a wakeup event.
  * @dev: Device the wakeup event is related to.
  * @msec: Anticipated event processing time (in milliseconds).
+ * @hard: If set, abort suspends in progress and wake up from suspend-to-idle.
  *
- * Call __pm_wakeup_event() for the @dev's wakeup source object.
+ * Call pm_wakeup_ws_event() for the @dev's wakeup source object.
  */
-void pm_wakeup_event(struct device *dev, unsigned int msec)
+void pm_wakeup_dev_event(struct device *dev, unsigned int msec, bool hard)
 {
 	unsigned long flags;
 
@@ -797,10 +797,10 @@ void pm_wakeup_event(struct device *dev,
 		return;
 
 	spin_lock_irqsave(&dev->power.lock, flags);
-	__pm_wakeup_event(dev->power.wakeup, msec);
+	pm_wakeup_ws_event(dev->power.wakeup, msec, hard);
 	spin_unlock_irqrestore(&dev->power.lock, flags);
 }
-EXPORT_SYMBOL_GPL(pm_wakeup_event);
+EXPORT_SYMBOL_GPL(pm_wakeup_dev_event);
 
 void pm_print_active_wakeup_sources(void)
 {

[PATCH 2/5] ACPI / sleep: Ignore spurious SCI wakeups from suspend-to-idle

[Rafael J. Wysocki]

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

The ACPI SCI (System Control Interrupt) is set up as a wakeup IRQ
during suspend-to-idle transitions and, consequently, any events
signaled through it wake up the system from that state.  However,
on some systems some of the events signaled via the ACPI SCI while
suspended to idle should not cause the system to wake up.  In fact,
quite often they should just be discarded.

Arguably, systems should not resume entirely on such events, but in
order to decide which events really should cause the system to resume
and which are spurious, it is necessary to resume up to the point
when ACPI SCIs are actually handled and processed, which is after
executing dpm_resume_noirq() in the system resume path.

For this reasons, add a loop around freeze_enter() in which the
platforms can process events signaled via multiplexed IRQ lines
like the ACPI SCI and add suspend-to-idle hooks that can be
used for this purpose to struct platform_freeze_ops.

In the ACPI case, the ->wake hook is used for checking if the SCI
has triggered while suspended and deferring the interrupt-induced
system wakeup until the events signaled through it are actually
processed sufficiently to decide whether or not the system should
resume.  In turn, the ->sync hook allows all of the relevant event
queues to be flushed so as to prevent events from being missed due
to race conditions.

In addition to that, some ACPI code processing wakeup events needs
to be modified to use the "hard" version of wakeup triggers, so that
it will cause a system resume to happen on device-induced wakeup
events even if the "soft" mechanism to prevent the system from
suspending is not enabled (that also helps to catch device-induced
wakeup events occurring during suspend transitions in progress).

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/battery.c      |    2 +-
 drivers/acpi/button.c       |    5 +++--
 drivers/acpi/device_pm.c    |    3 ++-
 drivers/acpi/sleep.c        |   28 ++++++++++++++++++++++++++++
 drivers/base/power/main.c   |    5 -----
 drivers/base/power/wakeup.c |   18 ++++++++++++------
 include/linux/suspend.h     |    7 +++++--
 kernel/power/process.c      |    2 +-
 kernel/power/suspend.c      |   29 +++++++++++++++++++++++++----
 9 files changed, 77 insertions(+), 22 deletions(-)

Index: linux-pm/include/linux/suspend.h
===================================================================
--- linux-pm.orig/include/linux/suspend.h
+++ linux-pm/include/linux/suspend.h
@@ -189,6 +189,8 @@ struct platform_suspend_ops {
 struct platform_freeze_ops {
 	int (*begin)(void);
 	int (*prepare)(void);
+	void (*wake)(void);
+	void (*sync)(void);
 	void (*restore)(void);
 	void (*end)(void);
 };
@@ -428,7 +430,8 @@ extern unsigned int pm_wakeup_irq;
 
 extern bool pm_wakeup_pending(void);
 extern void pm_system_wakeup(void);
-extern void pm_wakeup_clear(void);
+extern void pm_system_cancel_wakeup(void);
+extern void pm_wakeup_clear(bool reset);
 extern void pm_system_irq_wakeup(unsigned int irq_number);
 extern bool pm_get_wakeup_count(unsigned int *count, bool block);
 extern bool pm_save_wakeup_count(unsigned int count);
@@ -478,7 +481,7 @@ static inline int unregister_pm_notifier
 
 static inline bool pm_wakeup_pending(void) { return false; }
 static inline void pm_system_wakeup(void) {}
-static inline void pm_wakeup_clear(void) {}
+static inline void pm_wakeup_clear(bool reset) {}
 static inline void pm_system_irq_wakeup(unsigned int irq_number) {}
 
 static inline void lock_system_sleep(void) {}
Index: linux-pm/kernel/power/suspend.c
===================================================================
--- linux-pm.orig/kernel/power/suspend.c
+++ linux-pm/kernel/power/suspend.c
@@ -72,6 +72,8 @@ static void freeze_begin(void)
 
 static void freeze_enter(void)
 {
+	trace_suspend_resume(TPS("machine_suspend"), PM_SUSPEND_FREEZE, true);
+
 	spin_lock_irq(&suspend_freeze_lock);
 	if (pm_wakeup_pending())
 		goto out;
@@ -98,6 +100,27 @@ static void freeze_enter(void)
  out:
 	suspend_freeze_state = FREEZE_STATE_NONE;
 	spin_unlock_irq(&suspend_freeze_lock);
+
+	trace_suspend_resume(TPS("machine_suspend"), PM_SUSPEND_FREEZE, false);
+}
+
+static void s2idle_loop(void)
+{
+	do {
+		freeze_enter();
+
+		if (freeze_ops && freeze_ops->wake)
+			freeze_ops->wake();
+
+		dpm_resume_noirq(PMSG_RESUME);
+		if (freeze_ops && freeze_ops->sync)
+			freeze_ops->sync();
+
+		if (pm_wakeup_pending())
+			break;
+
+		pm_wakeup_clear(false);
+	} while (!dpm_suspend_noirq(PMSG_SUSPEND));
 }
 
 void freeze_wake(void)
@@ -371,10 +394,8 @@ static int suspend_enter(suspend_state_t
 	 * all the devices are suspended.
 	 */
 	if (state == PM_SUSPEND_FREEZE) {
-		trace_suspend_resume(TPS("machine_suspend"), state, true);
-		freeze_enter();
-		trace_suspend_resume(TPS("machine_suspend"), state, false);
-		goto Platform_wake;
+		s2idle_loop();
+		goto Platform_early_resume;
 	}
 
 	error = disable_nonboot_cpus();
Index: linux-pm/drivers/base/power/main.c
===================================================================
--- linux-pm.orig/drivers/base/power/main.c
+++ linux-pm/drivers/base/power/main.c
@@ -1091,11 +1091,6 @@ static int __device_suspend_noirq(struct
 	if (async_error)
 		goto Complete;
 
-	if (pm_wakeup_pending()) {
-		async_error = -EBUSY;
-		goto Complete;
-	}
-
 	if (dev->power.syscore || dev->power.direct_complete)
 		goto Complete;
 
Index: linux-pm/drivers/base/power/wakeup.c
===================================================================
--- linux-pm.orig/drivers/base/power/wakeup.c
+++ linux-pm/drivers/base/power/wakeup.c
@@ -28,8 +28,8 @@ bool events_check_enabled __read_mostly;
 /* First wakeup IRQ seen by the kernel in the last cycle. */
 unsigned int pm_wakeup_irq __read_mostly;
 
-/* If set and the system is suspending, terminate the suspend. */
-static bool pm_abort_suspend __read_mostly;
+/* If greater than 0 and the system is suspending, terminate the suspend. */
+static atomic_t pm_abort_suspend __read_mostly;
 
 /*
  * Combined counters of registered wakeup events and wakeup events in progress.
@@ -856,20 +856,26 @@ bool pm_wakeup_pending(void)
 		pm_print_active_wakeup_sources();
 	}
 
-	return ret || pm_abort_suspend;
+	return ret || atomic_read(&pm_abort_suspend) > 0;
 }
 
 void pm_system_wakeup(void)
 {
-	pm_abort_suspend = true;
+	atomic_inc(&pm_abort_suspend);
 	freeze_wake();
 }
 EXPORT_SYMBOL_GPL(pm_system_wakeup);
 
-void pm_wakeup_clear(void)
+void pm_system_cancel_wakeup(void)
+{
+	atomic_dec(&pm_abort_suspend);
+}
+
+void pm_wakeup_clear(bool reset)
 {
-	pm_abort_suspend = false;
 	pm_wakeup_irq = 0;
+	if (reset)
+		atomic_set(&pm_abort_suspend, 0);
 }
 
 void pm_system_irq_wakeup(unsigned int irq_number)
Index: linux-pm/kernel/power/process.c
===================================================================
--- linux-pm.orig/kernel/power/process.c
+++ linux-pm/kernel/power/process.c
@@ -132,7 +132,7 @@ int freeze_processes(void)
 	if (!pm_freezing)
 		atomic_inc(&system_freezing_cnt);
 
-	pm_wakeup_clear();
+	pm_wakeup_clear(true);
 	pr_info("Freezing user space processes ... ");
 	pm_freezing = true;
 	error = try_to_freeze_tasks(true);
Index: linux-pm/drivers/acpi/sleep.c
===================================================================
--- linux-pm.orig/drivers/acpi/sleep.c
+++ linux-pm/drivers/acpi/sleep.c
@@ -662,14 +662,40 @@ static int acpi_freeze_prepare(void)
 	acpi_os_wait_events_complete();
 	if (acpi_sci_irq_valid())
 		enable_irq_wake(acpi_sci_irq);
+
 	return 0;
 }
 
+static void acpi_freeze_wake(void)
+{
+	/*
+	 * If IRQD_WAKEUP_ARMED is not set for the SCI at this point, it means
+	 * that the SCI has triggered while suspended, so cancel the wakeup in
+	 * case it has not been a wakeup event (the GPEs will be checked later).
+	 */
+	if (acpi_sci_irq_valid() &&
+	    !irqd_is_wakeup_armed(irq_get_irq_data(acpi_sci_irq)))
+		pm_system_cancel_wakeup();
+}
+
+static void acpi_freeze_sync(void)
+{
+	/*
+	 * Process all pending events in case there are any wakeup ones.
+	 *
+	 * The EC driver uses the system workqueue, so that one needs to be
+	 * flushed too.
+	 */
+	acpi_os_wait_events_complete();
+	flush_scheduled_work();
+}
+
 static void acpi_freeze_restore(void)
 {
 	acpi_disable_wakeup_devices(ACPI_STATE_S0);
 	if (acpi_sci_irq_valid())
 		disable_irq_wake(acpi_sci_irq);
+
 	acpi_enable_all_runtime_gpes();
 }
 
@@ -681,6 +707,8 @@ static void acpi_freeze_end(void)
 static const struct platform_freeze_ops acpi_freeze_ops = {
 	.begin = acpi_freeze_begin,
 	.prepare = acpi_freeze_prepare,
+	.wake = acpi_freeze_wake,
+	.sync = acpi_freeze_sync,
 	.restore = acpi_freeze_restore,
 	.end = acpi_freeze_end,
 };
Index: linux-pm/drivers/acpi/device_pm.c
===================================================================
--- linux-pm.orig/drivers/acpi/device_pm.c
+++ linux-pm/drivers/acpi/device_pm.c
@@ -24,6 +24,7 @@
 #include <linux/pm_qos.h>
 #include <linux/pm_domain.h>
 #include <linux/pm_runtime.h>
+#include <linux/suspend.h>
 
 #include "internal.h"
 
@@ -399,7 +400,7 @@ static void acpi_pm_notify_handler(acpi_
 	mutex_lock(&acpi_pm_notifier_lock);
 
 	if (adev->wakeup.flags.notifier_present) {
-		__pm_wakeup_event(adev->wakeup.ws, 0);
+		pm_wakeup_ws_event(adev->wakeup.ws, 0, true);
 		if (adev->wakeup.context.work.func)
 			queue_pm_work(&adev->wakeup.context.work);
 	}
Index: linux-pm/drivers/acpi/button.c
===================================================================
--- linux-pm.orig/drivers/acpi/button.c
+++ linux-pm/drivers/acpi/button.c
@@ -216,7 +216,7 @@ static int acpi_lid_notify_state(struct
 	}
 
 	if (state)
-		pm_wakeup_event(&device->dev, 0);
+		pm_wakeup_hard_event(&device->dev);
 
 	ret = blocking_notifier_call_chain(&acpi_lid_notifier, state, device);
 	if (ret == NOTIFY_DONE)
@@ -398,7 +398,7 @@ static void acpi_button_notify(struct ac
 		} else {
 			int keycode;
 
-			pm_wakeup_event(&device->dev, 0);
+			pm_wakeup_hard_event(&device->dev);
 			if (button->suspended)
 				break;
 
@@ -530,6 +530,7 @@ static int acpi_button_add(struct acpi_d
 		lid_device = device;
 	}
 
+	device_init_wakeup(&device->dev, true);
 	printk(KERN_INFO PREFIX "%s [%s]\n", name, acpi_device_bid(device));
 	return 0;
 
Index: linux-pm/drivers/acpi/battery.c
===================================================================
--- linux-pm.orig/drivers/acpi/battery.c
+++ linux-pm/drivers/acpi/battery.c
@@ -782,7 +782,7 @@ static int acpi_battery_update(struct ac
 	if ((battery->state & ACPI_BATTERY_STATE_CRITICAL) ||
 	    (test_bit(ACPI_BATTERY_ALARM_PRESENT, &battery->flags) &&
             (battery->capacity_now <= battery->alarm)))
-		pm_wakeup_event(&battery->device->dev, 0);
+		pm_wakeup_hard_event(&battery->device->dev);
 
 	return result;
 }

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

[Rafael J. Wysocki]

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

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

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

Of course, on the Dell systems in question the EC GPE must be enabled
during suspend-to-idle transitions for the button press events to
be signaled while suspended at all.  Fortunately, there is a way to
tell the EC to stop generating the non-wakeup events, which is by
using the _DSM object under the so called micro-PEP (uPEP) device
provided to support Modern Standby in Windows 10.

The expected way to use it is to invoke function 0 from it on system
initialization, functions 3 and 5 during suspend transitions and
functions 4 and 6 during resume transitions (to reverse the actions
carried out by the former).  In particular, function 5 from the uPEP
device _DSM causes the EC to become less verbose (so to speak) on the
affected systems and then its GPE can be enabled as a wakeup source
(then, on resume, function 6 switches it back to the "working state"
mode).

In support of the affected Dell systems, implement the uPEP device
handling as described and allow the EC to generate system wakeup
events if that device is present and behaves as expected.  Enable
that for Dell only, as there are other systems out there in which
the uPEP device is exposed in the ACPI tables and its _DSM appears
to be functional, but it actually isn't, whereas Dell is committed
to supporting it.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/ec.c       |   19 +++++++-
 drivers/acpi/internal.h |    2 
 drivers/acpi/sleep.c    |  110 ++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 130 insertions(+), 1 deletion(-)

Index: linux-pm/drivers/acpi/ec.c
===================================================================
--- linux-pm.orig/drivers/acpi/ec.c
+++ linux-pm/drivers/acpi/ec.c
@@ -40,6 +40,7 @@
 #include <linux/slab.h>
 #include <linux/acpi.h>
 #include <linux/dmi.h>
+#include <linux/suspend.h>
 #include <asm/io.h>
 
 #include "internal.h"
@@ -1493,6 +1494,16 @@ static int acpi_ec_setup(struct acpi_ec
 	acpi_handle_info(ec->handle,
 			 "GPE=0x%lx, EC_CMD/EC_SC=0x%lx, EC_DATA=0x%lx\n",
 			 ec->gpe, ec->command_addr, ec->data_addr);
+
+	/*
+	 * On some platforms the EC GPE is used for waking up the system from
+	 * suspend-to-idle, so mark it as a wakeup one.
+	 *
+	 * This can be done unconditionally, as the setting does not matter
+	 * until acpi_set_gpe_wake_mask() is called for the GPE.
+	 */
+	acpi_mark_gpe_for_wake(NULL, ec->gpe);
+
 	return ret;
 }
 
@@ -1835,8 +1846,11 @@ static int acpi_ec_suspend(struct device
 	struct acpi_ec *ec =
 		acpi_driver_data(to_acpi_device(dev));
 
-	if (ec_freeze_events)
+	if (!pm_suspend_via_firmware() && acpi_sleep_ec_gpe_may_wakeup())
+		acpi_set_gpe_wake_mask(NULL, ec->gpe, ACPI_GPE_ENABLE);
+	else if (ec_freeze_events)
 		acpi_ec_disable_event(ec);
+
 	return 0;
 }
 
@@ -1846,6 +1860,9 @@ static int acpi_ec_resume(struct device
 		acpi_driver_data(to_acpi_device(dev));
 
 	acpi_ec_enable_event(ec);
+	if (!pm_resume_via_firmware() && acpi_sleep_ec_gpe_may_wakeup())
+		acpi_set_gpe_wake_mask(NULL, ec->gpe, ACPI_GPE_DISABLE);
+
 	return 0;
 }
 #endif
Index: linux-pm/drivers/acpi/internal.h
===================================================================
--- linux-pm.orig/drivers/acpi/internal.h
+++ linux-pm/drivers/acpi/internal.h
@@ -199,8 +199,10 @@ void acpi_ec_remove_query_handler(struct
   -------------------------------------------------------------------------- */
 #ifdef CONFIG_ACPI_SYSTEM_POWER_STATES_SUPPORT
 extern int acpi_sleep_init(void);
+bool acpi_sleep_ec_gpe_may_wakeup(void);
 #else
 static inline int acpi_sleep_init(void) { return -ENXIO; }
+static inline bool acpi_sleep_ec_gpe_may_wakeup(void) { return false; }
 #endif
 
 #ifdef CONFIG_ACPI_SLEEP
Index: linux-pm/drivers/acpi/sleep.c
===================================================================
--- linux-pm.orig/drivers/acpi/sleep.c
+++ linux-pm/drivers/acpi/sleep.c
@@ -160,6 +160,10 @@ static int __init init_nvs_nosave(const
 	return 0;
 }
 
+#ifdef CONFIG_SUSPEND
+static int __init init_upep_device(const struct dmi_system_id *d);
+#endif
+
 static struct dmi_system_id acpisleep_dmi_table[] __initdata = {
 	{
 	.callback = init_old_suspend_ordering,
@@ -343,6 +347,15 @@ static struct dmi_system_id acpisleep_dm
 		DMI_MATCH(DMI_PRODUCT_NAME, "80E3"),
 		},
 	},
+#ifdef CONFIG_SUSPEND
+	{
+	 .callback = init_upep_device,
+	 .ident = "All Dell systems",
+	 .matches = {
+		      DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
+		},
+	},
+#endif
 	{},
 };
 
@@ -649,6 +662,94 @@ static const struct platform_suspend_ops
 	.recover = acpi_pm_finish,
 };
 
+/*
+ * The micro-PEP (uPEP) device object is exposed in ACPI tables on systems
+ * supporting Windows 10 with Modern Standby.  The _DSM object under it, if
+ * present, can be used to indicate to the platform that the OS is transitioning
+ * into a low-power state in which certain types of activity are not desirable.
+ */
+static acpi_handle upep_device_handle;
+
+#define ACPI_S2IDLE_SCREEN_OFF	3
+#define ACPI_S2IDLE_SCREEN_ON	4
+#define ACPI_S2IDLE_IR_ENTRY	5
+#define ACPI_S2IDLE_IR_EXIT	6
+
+#define ACPI_S2IDLE_DSM_MASK	((1 << ACPI_S2IDLE_IR_ENTRY) | (1 << ACPI_S2IDLE_IR_EXIT))
+
+static char upep_dsm_func_mask;
+
+/* uPEP device _DSM UUID: c4eb40a0-6cd2-11e2-bcfd-0800200c9a66 */
+static const u8 upep_dsm_uuid[16] = {
+	0xa0, 0x40, 0xeb, 0xc4, 0xd2, 0x6c, 0xe2, 0x11,
+	0xbc, 0xfd, 0x08, 0x00, 0x20, 0x0c, 0x9a, 0x66
+};
+
+static void acpi_sleep_call_upep_dsm(unsigned int func)
+{
+	union acpi_object *out_obj;
+
+	if (!(upep_dsm_func_mask & (1 << func)))
+		return;
+
+	out_obj = acpi_evaluate_dsm(upep_device_handle, upep_dsm_uuid, 1, func,
+				    NULL);
+	ACPI_FREE(out_obj);
+
+	acpi_handle_debug(upep_device_handle, "_DSM function %u evaluation %s\n",
+			  func, out_obj ? "successful" : "failed");
+}
+
+static int upep_device_attach(struct acpi_device *adev,
+			      const struct acpi_device_id *not_used)
+{
+	union acpi_object *out_obj;
+
+	if (upep_device_handle)
+		return 0;
+
+	/* Check if the _DSM is present and as expected. */
+	out_obj = acpi_evaluate_dsm(adev->handle, upep_dsm_uuid, 1, 0, NULL);
+	if (out_obj && out_obj->type == ACPI_TYPE_BUFFER) {
+		char bitmask = *(char *)out_obj->buffer.pointer;
+
+		if (bitmask & ACPI_S2IDLE_DSM_MASK) {
+			upep_dsm_func_mask = bitmask;
+			upep_device_handle = adev->handle;
+		}
+
+		acpi_handle_debug(adev->handle, "_DSM function mask: 0x%x\n",
+				  bitmask);
+	} else {
+		acpi_handle_debug(adev->handle,
+				  "_DSM function 0 evaluation failed\n");
+	}
+	ACPI_FREE(out_obj);
+	return 0;
+}
+
+static const struct acpi_device_id upep_device_ids[] = {
+	{"INT33A1", },
+	{"PNP0D80", },
+	{"", },
+};
+
+static struct acpi_scan_handler upep_handler = {
+	.ids = upep_device_ids,
+	.attach = upep_device_attach,
+};
+
+static int __init init_upep_device(const struct dmi_system_id *d)
+{
+	acpi_scan_add_handler(&upep_handler);
+	return 0;
+}
+
+bool acpi_sleep_ec_gpe_may_wakeup(void)
+{
+	return !!upep_device_handle;
+}
+
 static int acpi_freeze_begin(void)
 {
 	acpi_scan_lock_acquire();
@@ -657,6 +758,8 @@ static int acpi_freeze_begin(void)
 
 static int acpi_freeze_prepare(void)
 {
+	acpi_sleep_call_upep_dsm(ACPI_S2IDLE_SCREEN_OFF);
+	acpi_sleep_call_upep_dsm(ACPI_S2IDLE_IR_ENTRY);
 	acpi_enable_wakeup_devices(ACPI_STATE_S0);
 	acpi_enable_all_wakeup_gpes();
 	acpi_enable_all_runtime_gpes();
@@ -698,6 +801,8 @@ static void acpi_freeze_restore(void)
 		disable_irq_wake(acpi_sci_irq);
 
 	acpi_enable_all_runtime_gpes();
+	acpi_sleep_call_upep_dsm(ACPI_S2IDLE_IR_EXIT);
+	acpi_sleep_call_upep_dsm(ACPI_S2IDLE_SCREEN_ON);
 }
 
 static void acpi_freeze_end(void)
@@ -729,6 +834,11 @@ static void acpi_sleep_suspend_setup(voi
 
 #else /* !CONFIG_SUSPEND */
 static inline void acpi_sleep_suspend_setup(void) {}
+
+bool acpi_sleep_ec_gpe_may_wakeup(void)
+{
+	return false;
+}
 #endif /* !CONFIG_SUSPEND */
 
 #ifdef CONFIG_PM_SLEEP

[PATCH 4/5] platform: x86: intel-vbtn: Wake up the system from suspend-to-idle

[Rafael J. Wysocki]

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

Allow the intel-vbtn driver to wake up the system from suspend-to-idle
by configuring its platform device as a wakeup one by default and
switching it over to a system wakeup events triggering mode during
system suspend transitions.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/platform/x86/intel-vbtn.c |   38 +++++++++++++++++++++++++++++++++++---
 1 file changed, 35 insertions(+), 3 deletions(-)

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
@@ -23,6 +23,7 @@
 #include <linux/platform_device.h>
 #include <linux/input/sparse-keymap.h>
 #include <linux/acpi.h>
+#include <linux/suspend.h>
 #include <acpi/acpi_bus.h>
 
 MODULE_LICENSE("GPL");
@@ -42,6 +43,7 @@ static const struct key_entry intel_vbtn
 
 struct intel_vbtn_priv {
 	struct input_dev *input_dev;
+	bool wakeup_mode;
 };
 
 static int intel_vbtn_input_setup(struct platform_device *device)
@@ -69,9 +71,15 @@ static void notify_handler(acpi_handle h
 	struct platform_device *device = context;
 	struct intel_vbtn_priv *priv = dev_get_drvdata(&device->dev);
 
-	if (!sparse_keymap_report_event(priv->input_dev, event, 1, true))
-		dev_info(&device->dev, "unknown event index 0x%x\n",
-			 event);
+	if (priv->wakeup_mode) {
+		if (sparse_keymap_entry_from_scancode(priv->input_dev, event)) {
+			pm_wakeup_hard_event(&device->dev);
+			return;
+		}
+	} else if (sparse_keymap_report_event(priv->input_dev, event, 1, true)) {
+		return;
+	}
+	dev_info(&device->dev, "unknown event index 0x%x\n", event);
 }
 
 static int intel_vbtn_probe(struct platform_device *device)
@@ -105,6 +113,7 @@ static int intel_vbtn_probe(struct platf
 	if (ACPI_FAILURE(status))
 		return -EBUSY;
 
+	device_init_wakeup(&device->dev, true);
 	return 0;
 }
 
@@ -121,10 +130,33 @@ static int intel_vbtn_remove(struct plat
 	return 0;
 }
 
+static int intel_vbtn_pm_prepare(struct device *dev)
+{
+	struct platform_device *device = to_platform_device(dev);
+	struct intel_vbtn_priv *priv = dev_get_drvdata(&device->dev);
+
+	priv->wakeup_mode = true;
+	return 0;
+}
+
+static void intel_vbtn_pm_complete(struct device *dev)
+{
+	struct platform_device *device = to_platform_device(dev);
+	struct intel_vbtn_priv *priv = dev_get_drvdata(&device->dev);
+
+	priv->wakeup_mode = false;
+}
+
+static const struct dev_pm_ops intel_vbtn_pm_ops = {
+	.prepare = intel_vbtn_pm_prepare,
+	.complete = intel_vbtn_pm_complete,
+};
+
 static struct platform_driver intel_vbtn_pl_driver = {
 	.driver = {
 		.name = "intel-vbtn",
 		.acpi_match_table = intel_vbtn_ids,
+		.pm = &intel_vbtn_pm_ops,
 	},
 	.probe = intel_vbtn_probe,
 	.remove = intel_vbtn_remove,

[PATCH 5/5] platform: x86: intel-hid: Wake up the system from suspend-to-idle

[Rafael J. Wysocki]

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

Allow the intel-hid driver to wake up the system from suspend-to-idle
by configuring its platform device as a wakeup one by default and
switching it over to a system wakeup events triggering mode during
system suspend transitions.

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

This depends on https://patchwork.kernel.org/patch/9685529/

---
 drivers/platform/x86/intel-hid.c |   49 ++++++++++++++++++++++++++++++++++-----
 1 file changed, 43 insertions(+), 6 deletions(-)

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
@@ -23,6 +23,7 @@
 #include <linux/platform_device.h>
 #include <linux/input/sparse-keymap.h>
 #include <linux/acpi.h>
+#include <linux/suspend.h>
 #include <acpi/acpi_bus.h>
 
 MODULE_LICENSE("GPL");
@@ -75,6 +76,7 @@ static const struct key_entry intel_arra
 struct intel_hid_priv {
 	struct input_dev *input_dev;
 	struct input_dev *array;
+	bool wakeup_mode;
 };
 
 static int intel_hid_set_enable(struct device *device, int enable)
@@ -118,23 +120,44 @@ static void intel_button_array_enable(st
 		dev_warn(device, "failed to set button capability\n");
 }
 
-static int intel_hid_pl_suspend_handler(struct device *device)
+static int intel_hid_pm_prepare(struct device *dev)
 {
-	intel_hid_set_enable(device, 0);
-	intel_button_array_enable(device, false);
+	struct platform_device *device = to_platform_device(dev);
+	struct intel_hid_priv *priv = dev_get_drvdata(&device->dev);
 
+	priv->wakeup_mode = true;
 	return 0;
 }
 
-static int intel_hid_pl_resume_handler(struct device *device)
+static void intel_hid_pm_complete(struct device *dev)
+{
+	struct platform_device *device = to_platform_device(dev);
+	struct intel_hid_priv *priv = dev_get_drvdata(&device->dev);
+
+	priv->wakeup_mode = false;
+}
+
+static int intel_hid_pl_suspend_handler(struct device *device)
 {
-	intel_hid_set_enable(device, 1);
-	intel_button_array_enable(device, true);
+	if (pm_suspend_via_firmware()) {
+		intel_hid_set_enable(device, 0);
+		intel_button_array_enable(device, false);
+	}
+	return 0;
+}
 
+static int intel_hid_pl_resume_handler(struct device *device)
+{
+	if (pm_resume_via_firmware()) {
+		intel_hid_set_enable(device, 1);
+		intel_button_array_enable(device, true);
+	}
 	return 0;
 }
 
 static const struct dev_pm_ops intel_hid_pl_pm_ops = {
+	.prepare = intel_hid_pm_prepare,
+	.complete = intel_hid_pm_complete,
 	.freeze  = intel_hid_pl_suspend_handler,
 	.thaw  = intel_hid_pl_resume_handler,
 	.restore  = intel_hid_pl_resume_handler,
@@ -206,6 +229,19 @@ static void notify_handler(acpi_handle h
 	unsigned long long ev_index;
 	acpi_status status;
 
+	if (priv->wakeup_mode) {
+		/* Wake up on 5-button array events only. */
+		if (event == 0xc0 || !priv->array)
+			return;
+
+		if (sparse_keymap_entry_from_scancode(priv->array, event))
+			pm_wakeup_hard_event(&device->dev);
+		else
+			dev_info(&device->dev, "unknown event 0x%x\n", event);
+
+		return;
+	}
+
 	/* 0xC0 is for HID events, other values are for 5 button array */
 	if (event != 0xc0) {
 		if (!priv->array ||
@@ -292,6 +328,7 @@ static int intel_hid_probe(struct platfo
 				 "failed to enable HID power button\n");
 	}
 
+	device_init_wakeup(&device->dev, true);
 	return 0;
 
 err_remove_notify:

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

[Mario.Limonciello]

> -----Original Message-----
> From: Rafael J. Wysocki [mailto:rjw@rjwysocki.net]
> Sent: Wednesday, April 26, 2017 4:24 PM
> To: Linux PM <linux-pm@vger.kernel.org>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>; Darren Hart
> <dvhart@infradead.org>; LKML <linux-kernel@vger.kernel.org>; Linux ACPI <linux-
> acpi@vger.kernel.org>; Srinivas Pandruvada
> <srinivas.pandruvada@linux.intel.com>; Thomas Gleixner <tglx@linutronix.de>;
> Mika Westerberg <mika.westerberg@linux.intel.com>; Limonciello, Mario
> <Mario_Limonciello@Dell.com>
> Subject: [PATCH 3/5] ACPI / sleep: EC-based wakeup from suspend-to-idle on Dell
> systems
> 
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Some recent Dell laptops, including the XPS13 model numbers 9360 and
> 9365, cannot be woken up from suspend-to-idle by pressing the power
> button which is unexpected and makes that feature hardly usable on
> those systems.  However, on the 9365 ACPI S3 (suspend-to-RAM) is not
> expected to be used at all (these systems ship with Windows 10 using
> Modern Standby which never exercises the ACPI S3 path) and
> suspend-to-idle is the only viable system suspend mechanism in there.
> 
> The reason why the power button wakeup from suspend-to-idle doesn't
> work on those systems is because their power button events are
> signaled by the EC (Embedded Controller), whose GPE (General Purpose
> Event) line is disabled during suspend-to-idle transitions in Linux.
> That is done on purpose, because in general the EC tends to generate
> tons of events for various reasons (battery and thermal updates and
> similar, for example) and all of them would kick the CPUs out of deep
> idle states while in suspend-to-idle, which would not be desirable.
> 
> Of course, on the Dell systems in question the EC GPE must be enabled
> during suspend-to-idle transitions for the button press events to
> be signaled while suspended at all.  Fortunately, there is a way to
> tell the EC to stop generating the non-wakeup events, which is by
> using the _DSM object under the so called micro-PEP (uPEP) device
> provided to support Modern Standby in Windows 10.
> 
> The expected way to use it is to invoke function 0 from it on system
> initialization, functions 3 and 5 during suspend transitions and
> functions 4 and 6 during resume transitions (to reverse the actions
> carried out by the former).  In particular, function 5 from the uPEP
> device _DSM causes the EC to become less verbose (so to speak) on the
> affected systems and then its GPE can be enabled as a wakeup source
> (then, on resume, function 6 switches it back to the "working state"
> mode).
> 
> In support of the affected Dell systems, implement the uPEP device
> handling as described and allow the EC to generate system wakeup
> events if that device is present and behaves as expected.  Enable
> that for Dell only, as there are other systems out there in which
> the uPEP device is exposed in the ACPI tables and its _DSM appears
> to be functional, but it actually isn't, whereas Dell is committed
> to supporting it.
> 

I am of course biased in that my priority is for this to work for Dell.
Dell is definitely committed to supporting this on any system with
the low power idle bit in the FADT set.

So I'm fine with the current proposed solution, but have you
dug into what actually breaks on this other system?  Does it actually
work with Modern Standby + the uPEP device on Windows 10?

To my understanding I would think any OEM that is enabling this
uPEP device it should be getting called by the Windows kernel
identically when entering resiliency phases.

This makes me wonder if it should be inverted and a blacklist
of platforms that the uPEP device doesn't work.

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

[Mario.Limonciello]

> -----Original Message-----
> From: Rafael J. Wysocki [mailto:rjw@rjwysocki.net]
> Sent: Wednesday, April 26, 2017 4:24 PM
> To: Linux PM <linux-pm@vger.kernel.org>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>; Darren Hart
> <dvhart@infradead.org>; LKML <linux-kernel@vger.kernel.org>; Linux ACPI <linux-
> acpi@vger.kernel.org>; Srinivas Pandruvada
> <srinivas.pandruvada@linux.intel.com>; Thomas Gleixner <tglx@linutronix.de>;
> Mika Westerberg <mika.westerberg@linux.intel.com>; Limonciello, Mario
> <Mario_Limonciello@Dell.com>
> Subject: [PATCH 3/5] ACPI / sleep: EC-based wakeup from suspend-to-idle on Dell
> systems
> 
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Some recent Dell laptops, including the XPS13 model numbers 9360 and
> 9365, cannot be woken up from suspend-to-idle by pressing the power
> button which is unexpected and makes that feature hardly usable on
> those systems.  However, on the 9365 ACPI S3 (suspend-to-RAM) is not
> expected to be used at all (these systems ship with Windows 10 using
> Modern Standby which never exercises the ACPI S3 path) and
> suspend-to-idle is the only viable system suspend mechanism in there.
> 
> The reason why the power button wakeup from suspend-to-idle doesn't
> work on those systems is because their power button events are
> signaled by the EC (Embedded Controller), whose GPE (General Purpose
> Event) line is disabled during suspend-to-idle transitions in Linux.
> That is done on purpose, because in general the EC tends to generate
> tons of events for various reasons (battery and thermal updates and
> similar, for example) and all of them would kick the CPUs out of deep
> idle states while in suspend-to-idle, which would not be desirable.
> 
> Of course, on the Dell systems in question the EC GPE must be enabled
> during suspend-to-idle transitions for the button press events to
> be signaled while suspended at all.  Fortunately, there is a way to
> tell the EC to stop generating the non-wakeup events, which is by
> using the _DSM object under the so called micro-PEP (uPEP) device
> provided to support Modern Standby in Windows 10.
> 
> The expected way to use it is to invoke function 0 from it on system
> initialization, functions 3 and 5 during suspend transitions and
> functions 4 and 6 during resume transitions (to reverse the actions
> carried out by the former).  In particular, function 5 from the uPEP
> device _DSM causes the EC to become less verbose (so to speak) on the
> affected systems and then its GPE can be enabled as a wakeup source
> (then, on resume, function 6 switches it back to the "working state"
> mode).
> 
> In support of the affected Dell systems, implement the uPEP device
> handling as described and allow the EC to generate system wakeup
> events if that device is present and behaves as expected.  Enable
> that for Dell only, as there are other systems out there in which
> the uPEP device is exposed in the ACPI tables and its _DSM appears
> to be functional, but it actually isn't, whereas Dell is committed
> to supporting it.
> 

I am of course biased in that my priority is for this to work for Dell.
Dell is definitely committed to supporting this on any system with
the low power idle bit in the FADT set.

So I'm fine with the current proposed solution, but have you
dug into what actually breaks on this other system?  Does it actually
work with Modern Standby + the uPEP device on Windows 10?

To my understanding I would think any OEM that is enabling this
uPEP device it should be getting called by the Windows kernel
identically when entering resiliency phases.

This makes me wonder if it should be inverted and a blacklist
of platforms that the uPEP device doesn't work.

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

[Rafael J. Wysocki]

On Thursday, April 27, 2017 02:47:59 PM Mario.Limonciello@dell.com wrote:
> > -----Original Message-----
> > From: Rafael J. Wysocki [mailto:rjw@rjwysocki.net]
> > Sent: Wednesday, April 26, 2017 4:24 PM
> > To: Linux PM <linux-pm@vger.kernel.org>
> > Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>; Darren Hart
> > <dvhart@infradead.org>; LKML <linux-kernel@vger.kernel.org>; Linux ACPI <linux-
> > acpi@vger.kernel.org>; Srinivas Pandruvada
> > <srinivas.pandruvada@linux.intel.com>; Thomas Gleixner <tglx@linutronix.de>;
> > Mika Westerberg <mika.westerberg@linux.intel.com>; Limonciello, Mario
> > <Mario_Limonciello@Dell.com>
> > Subject: [PATCH 3/5] ACPI / sleep: EC-based wakeup from suspend-to-idle on Dell
> > systems
> > 
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > 
> > Some recent Dell laptops, including the XPS13 model numbers 9360 and
> > 9365, cannot be woken up from suspend-to-idle by pressing the power
> > button which is unexpected and makes that feature hardly usable on
> > those systems.  However, on the 9365 ACPI S3 (suspend-to-RAM) is not
> > expected to be used at all (these systems ship with Windows 10 using
> > Modern Standby which never exercises the ACPI S3 path) and
> > suspend-to-idle is the only viable system suspend mechanism in there.
> > 
> > The reason why the power button wakeup from suspend-to-idle doesn't
> > work on those systems is because their power button events are
> > signaled by the EC (Embedded Controller), whose GPE (General Purpose
> > Event) line is disabled during suspend-to-idle transitions in Linux.
> > That is done on purpose, because in general the EC tends to generate
> > tons of events for various reasons (battery and thermal updates and
> > similar, for example) and all of them would kick the CPUs out of deep
> > idle states while in suspend-to-idle, which would not be desirable.
> > 
> > Of course, on the Dell systems in question the EC GPE must be enabled
> > during suspend-to-idle transitions for the button press events to
> > be signaled while suspended at all.  Fortunately, there is a way to
> > tell the EC to stop generating the non-wakeup events, which is by
> > using the _DSM object under the so called micro-PEP (uPEP) device
> > provided to support Modern Standby in Windows 10.
> > 
> > The expected way to use it is to invoke function 0 from it on system
> > initialization, functions 3 and 5 during suspend transitions and
> > functions 4 and 6 during resume transitions (to reverse the actions
> > carried out by the former).  In particular, function 5 from the uPEP
> > device _DSM causes the EC to become less verbose (so to speak) on the
> > affected systems and then its GPE can be enabled as a wakeup source
> > (then, on resume, function 6 switches it back to the "working state"
> > mode).
> > 
> > In support of the affected Dell systems, implement the uPEP device
> > handling as described and allow the EC to generate system wakeup
> > events if that device is present and behaves as expected.  Enable
> > that for Dell only, as there are other systems out there in which
> > the uPEP device is exposed in the ACPI tables and its _DSM appears
> > to be functional, but it actually isn't, whereas Dell is committed
> > to supporting it.
> > 
> 
> I am of course biased in that my priority is for this to work for Dell.
> Dell is definitely committed to supporting this on any system with
> the low power idle bit in the FADT set.
> 
> So I'm fine with the current proposed solution, but have you
> dug into what actually breaks on this other system?  Does it actually
> work with Modern Standby + the uPEP device on Windows 10?
> 
> To my understanding I would think any OEM that is enabling this
> uPEP device it should be getting called by the Windows kernel
> identically when entering resiliency phases.
> 
> This makes me wonder if it should be inverted and a blacklist
> of platforms that the uPEP device doesn't work.

For now I'd prefer to only do it on platforms where the benefit is clear.

The next step may be to extend it to the other ones, but let's avoid making
what is problem mitigation really depend on things that may or may not
work elsewhere to start with.

Thanks,
Rafael

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

[Zheng, Lv]

Hi,

> -----Original Message-----
> From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-owner@vger.kernel.org] On Behalf Of Rafael J.
> Wysocki
> Sent: Friday, April 28, 2017 6:26 AM
> To: Mario.Limonciello@dell.com
> Cc: linux-pm@vger.kernel.org; andriy.shevchenko@linux.intel.com; dvhart@infradead.org; linux-
> kernel@vger.kernel.org; linux-acpi@vger.kernel.org; srinivas.pandruvada@linux.intel.com;
> tglx@linutronix.de; mika.westerberg@linux.intel.com
> Subject: Re: [PATCH 3/5] ACPI / sleep: EC-based wakeup from suspend-to-idle on Dell systems
> 
> On Thursday, April 27, 2017 02:47:59 PM Mario.Limonciello@dell.com wrote:
> > > -----Original Message-----
> > > From: Rafael J. Wysocki [mailto:rjw@rjwysocki.net]
> > > Sent: Wednesday, April 26, 2017 4:24 PM
> > > To: Linux PM <linux-pm@vger.kernel.org>
> > > Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>; Darren Hart
> > > <dvhart@infradead.org>; LKML <linux-kernel@vger.kernel.org>; Linux ACPI <linux-
> > > acpi@vger.kernel.org>; Srinivas Pandruvada
> > > <srinivas.pandruvada@linux.intel.com>; Thomas Gleixner <tglx@linutronix.de>;
> > > Mika Westerberg <mika.westerberg@linux.intel.com>; Limonciello, Mario
> > > <Mario_Limonciello@Dell.com>
> > > Subject: [PATCH 3/5] ACPI / sleep: EC-based wakeup from suspend-to-idle on Dell
> > > systems
> > >
> > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > >
> > > Some recent Dell laptops, including the XPS13 model numbers 9360 and
> > > 9365, cannot be woken up from suspend-to-idle by pressing the power
> > > button which is unexpected and makes that feature hardly usable on
> > > those systems.  However, on the 9365 ACPI S3 (suspend-to-RAM) is not
> > > expected to be used at all (these systems ship with Windows 10 using
> > > Modern Standby which never exercises the ACPI S3 path) and
> > > suspend-to-idle is the only viable system suspend mechanism in there.
> > >
> > > The reason why the power button wakeup from suspend-to-idle doesn't
> > > work on those systems is because their power button events are
> > > signaled by the EC (Embedded Controller), whose GPE (General Purpose
> > > Event) line is disabled during suspend-to-idle transitions in Linux.
> > > That is done on purpose, because in general the EC tends to generate
> > > tons of events for various reasons (battery and thermal updates and
> > > similar, for example) and all of them would kick the CPUs out of deep
> > > idle states while in suspend-to-idle, which would not be desirable.
> > >
> > > Of course, on the Dell systems in question the EC GPE must be enabled
> > > during suspend-to-idle transitions for the button press events to
> > > be signaled while suspended at all.  Fortunately, there is a way to
> > > tell the EC to stop generating the non-wakeup events, which is by
> > > using the _DSM object under the so called micro-PEP (uPEP) device
> > > provided to support Modern Standby in Windows 10.
> > >
> > > The expected way to use it is to invoke function 0 from it on system
> > > initialization, functions 3 and 5 during suspend transitions and
> > > functions 4 and 6 during resume transitions (to reverse the actions
> > > carried out by the former).  In particular, function 5 from the uPEP
> > > device _DSM causes the EC to become less verbose (so to speak) on the
> > > affected systems and then its GPE can be enabled as a wakeup source
> > > (then, on resume, function 6 switches it back to the "working state"
> > > mode).
> > >
> > > In support of the affected Dell systems, implement the uPEP device
> > > handling as described and allow the EC to generate system wakeup
> > > events if that device is present and behaves as expected.  Enable
> > > that for Dell only, as there are other systems out there in which
> > > the uPEP device is exposed in the ACPI tables and its _DSM appears
> > > to be functional, but it actually isn't, whereas Dell is committed
> > > to supporting it.
> > >
> >
> > I am of course biased in that my priority is for this to work for Dell.
> > Dell is definitely committed to supporting this on any system with
> > the low power idle bit in the FADT set.
> >
> > So I'm fine with the current proposed solution, but have you
> > dug into what actually breaks on this other system?  Does it actually
> > work with Modern Standby + the uPEP device on Windows 10?
> >
> > To my understanding I would think any OEM that is enabling this
> > uPEP device it should be getting called by the Windows kernel
> > identically when entering resiliency phases.
> >
> > This makes me wonder if it should be inverted and a blacklist
> > of platforms that the uPEP device doesn't work.
> 
> For now I'd prefer to only do it on platforms where the benefit is clear.
> 
> The next step may be to extend it to the other ones, but let's avoid making
> what is problem mitigation really depend on things that may or may not
> work elsewhere to start with.

Then is it possible to invoke acpi_mark_gpe_for_wake() (and maybe also acpi_unmark_gpe_for_wake()) right after invoking uPEP functions?
So that such platform specific stuffs won't go into ec.c.

Thanks and best regards
Lv

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

[Zheng, Lv]

Hi,

> -----Original Message-----
> From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-owner@vger.kernel.org] On Behalf Of Rafael J.
> Wysocki
> Sent: Friday, April 28, 2017 6:26 AM
> To: Mario.Limonciello@dell.com
> Cc: linux-pm@vger.kernel.org; andriy.shevchenko@linux.intel.com; dvhart@infradead.org; linux-
> kernel@vger.kernel.org; linux-acpi@vger.kernel.org; srinivas.pandruvada@linux.intel.com;
> tglx@linutronix.de; mika.westerberg@linux.intel.com
> Subject: Re: [PATCH 3/5] ACPI / sleep: EC-based wakeup from suspend-to-idle on Dell systems
> 
> On Thursday, April 27, 2017 02:47:59 PM Mario.Limonciello@dell.com wrote:
> > > -----Original Message-----
> > > From: Rafael J. Wysocki [mailto:rjw@rjwysocki.net]
> > > Sent: Wednesday, April 26, 2017 4:24 PM
> > > To: Linux PM <linux-pm@vger.kernel.org>
> > > Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>; Darren Hart
> > > <dvhart@infradead.org>; LKML <linux-kernel@vger.kernel.org>; Linux ACPI <linux-
> > > acpi@vger.kernel.org>; Srinivas Pandruvada
> > > <srinivas.pandruvada@linux.intel.com>; Thomas Gleixner <tglx@linutronix.de>;
> > > Mika Westerberg <mika.westerberg@linux.intel.com>; Limonciello, Mario
> > > <Mario_Limonciello@Dell.com>
> > > Subject: [PATCH 3/5] ACPI / sleep: EC-based wakeup from suspend-to-idle on Dell
> > > systems
> > >
> > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > >
> > > Some recent Dell laptops, including the XPS13 model numbers 9360 and
> > > 9365, cannot be woken up from suspend-to-idle by pressing the power
> > > button which is unexpected and makes that feature hardly usable on
> > > those systems.  However, on the 9365 ACPI S3 (suspend-to-RAM) is not
> > > expected to be used at all (these systems ship with Windows 10 using
> > > Modern Standby which never exercises the ACPI S3 path) and
> > > suspend-to-idle is the only viable system suspend mechanism in there.
> > >
> > > The reason why the power button wakeup from suspend-to-idle doesn't
> > > work on those systems is because their power button events are
> > > signaled by the EC (Embedded Controller), whose GPE (General Purpose
> > > Event) line is disabled during suspend-to-idle transitions in Linux.
> > > That is done on purpose, because in general the EC tends to generate
> > > tons of events for various reasons (battery and thermal updates and
> > > similar, for example) and all of them would kick the CPUs out of deep
> > > idle states while in suspend-to-idle, which would not be desirable.
> > >
> > > Of course, on the Dell systems in question the EC GPE must be enabled
> > > during suspend-to-idle transitions for the button press events to
> > > be signaled while suspended at all.  Fortunately, there is a way to
> > > tell the EC to stop generating the non-wakeup events, which is by
> > > using the _DSM object under the so called micro-PEP (uPEP) device
> > > provided to support Modern Standby in Windows 10.
> > >
> > > The expected way to use it is to invoke function 0 from it on system
> > > initialization, functions 3 and 5 during suspend transitions and
> > > functions 4 and 6 during resume transitions (to reverse the actions
> > > carried out by the former).  In particular, function 5 from the uPEP
> > > device _DSM causes the EC to become less verbose (so to speak) on the
> > > affected systems and then its GPE can be enabled as a wakeup source
> > > (then, on resume, function 6 switches it back to the "working state"
> > > mode).
> > >
> > > In support of the affected Dell systems, implement the uPEP device
> > > handling as described and allow the EC to generate system wakeup
> > > events if that device is present and behaves as expected.  Enable
> > > that for Dell only, as there are other systems out there in which
> > > the uPEP device is exposed in the ACPI tables and its _DSM appears
> > > to be functional, but it actually isn't, whereas Dell is committed
> > > to supporting it.
> > >
> >
> > I am of course biased in that my priority is for this to work for Dell.
> > Dell is definitely committed to supporting this on any system with
> > the low power idle bit in the FADT set.
> >
> > So I'm fine with the current proposed solution, but have you
> > dug into what actually breaks on this other system?  Does it actually
> > work with Modern Standby + the uPEP device on Windows 10?
> >
> > To my understanding I would think any OEM that is enabling this
> > uPEP device it should be getting called by the Windows kernel
> > identically when entering resiliency phases.
> >
> > This makes me wonder if it should be inverted and a blacklist
> > of platforms that the uPEP device doesn't work.
> 
> For now I'd prefer to only do it on platforms where the benefit is clear.
> 
> The next step may be to extend it to the other ones, but let's avoid making
> what is problem mitigation really depend on things that may or may not
> work elsewhere to start with.

Then is it possible to invoke acpi_mark_gpe_for_wake() (and maybe also acpi_unmark_gpe_for_wake()) right after invoking uPEP functions?
So that such platform specific stuffs won't go into ec.c.

Thanks and best regards
Lv

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

[Rafael J. Wysocki]

On Thursday, May 04, 2017 07:58:53 AM Zheng, Lv wrote:
> Hi,
> 
> > -----Original Message-----
> > From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-owner@vger.kernel.org] On Behalf Of Rafael J.
> > Wysocki
> > Sent: Friday, April 28, 2017 6:26 AM
> > To: Mario.Limonciello@dell.com
> > Cc: linux-pm@vger.kernel.org; andriy.shevchenko@linux.intel.com; dvhart@infradead.org; linux-
> > kernel@vger.kernel.org; linux-acpi@vger.kernel.org; srinivas.pandruvada@linux.intel.com;
> > tglx@linutronix.de; mika.westerberg@linux.intel.com
> > Subject: Re: [PATCH 3/5] ACPI / sleep: EC-based wakeup from suspend-to-idle on Dell systems
> > 
> > On Thursday, April 27, 2017 02:47:59 PM Mario.Limonciello@dell.com wrote:
> > > > -----Original Message-----
> > > > From: Rafael J. Wysocki [mailto:rjw@rjwysocki.net]
> > > > Sent: Wednesday, April 26, 2017 4:24 PM
> > > > To: Linux PM <linux-pm@vger.kernel.org>
> > > > Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>; Darren Hart
> > > > <dvhart@infradead.org>; LKML <linux-kernel@vger.kernel.org>; Linux ACPI <linux-
> > > > acpi@vger.kernel.org>; Srinivas Pandruvada
> > > > <srinivas.pandruvada@linux.intel.com>; Thomas Gleixner <tglx@linutronix.de>;
> > > > Mika Westerberg <mika.westerberg@linux.intel.com>; Limonciello, Mario
> > > > <Mario_Limonciello@Dell.com>
> > > > Subject: [PATCH 3/5] ACPI / sleep: EC-based wakeup from suspend-to-idle on Dell
> > > > systems
> > > >
> > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > >
> > > > Some recent Dell laptops, including the XPS13 model numbers 9360 and
> > > > 9365, cannot be woken up from suspend-to-idle by pressing the power
> > > > button which is unexpected and makes that feature hardly usable on
> > > > those systems.  However, on the 9365 ACPI S3 (suspend-to-RAM) is not
> > > > expected to be used at all (these systems ship with Windows 10 using
> > > > Modern Standby which never exercises the ACPI S3 path) and
> > > > suspend-to-idle is the only viable system suspend mechanism in there.
> > > >
> > > > The reason why the power button wakeup from suspend-to-idle doesn't
> > > > work on those systems is because their power button events are
> > > > signaled by the EC (Embedded Controller), whose GPE (General Purpose
> > > > Event) line is disabled during suspend-to-idle transitions in Linux.
> > > > That is done on purpose, because in general the EC tends to generate
> > > > tons of events for various reasons (battery and thermal updates and
> > > > similar, for example) and all of them would kick the CPUs out of deep
> > > > idle states while in suspend-to-idle, which would not be desirable.
> > > >
> > > > Of course, on the Dell systems in question the EC GPE must be enabled
> > > > during suspend-to-idle transitions for the button press events to
> > > > be signaled while suspended at all.  Fortunately, there is a way to
> > > > tell the EC to stop generating the non-wakeup events, which is by
> > > > using the _DSM object under the so called micro-PEP (uPEP) device
> > > > provided to support Modern Standby in Windows 10.
> > > >
> > > > The expected way to use it is to invoke function 0 from it on system
> > > > initialization, functions 3 and 5 during suspend transitions and
> > > > functions 4 and 6 during resume transitions (to reverse the actions
> > > > carried out by the former).  In particular, function 5 from the uPEP
> > > > device _DSM causes the EC to become less verbose (so to speak) on the
> > > > affected systems and then its GPE can be enabled as a wakeup source
> > > > (then, on resume, function 6 switches it back to the "working state"
> > > > mode).
> > > >
> > > > In support of the affected Dell systems, implement the uPEP device
> > > > handling as described and allow the EC to generate system wakeup
> > > > events if that device is present and behaves as expected.  Enable
> > > > that for Dell only, as there are other systems out there in which
> > > > the uPEP device is exposed in the ACPI tables and its _DSM appears
> > > > to be functional, but it actually isn't, whereas Dell is committed
> > > > to supporting it.
> > > >
> > >
> > > I am of course biased in that my priority is for this to work for Dell.
> > > Dell is definitely committed to supporting this on any system with
> > > the low power idle bit in the FADT set.
> > >
> > > So I'm fine with the current proposed solution, but have you
> > > dug into what actually breaks on this other system?  Does it actually
> > > work with Modern Standby + the uPEP device on Windows 10?
> > >
> > > To my understanding I would think any OEM that is enabling this
> > > uPEP device it should be getting called by the Windows kernel
> > > identically when entering resiliency phases.
> > >
> > > This makes me wonder if it should be inverted and a blacklist
> > > of platforms that the uPEP device doesn't work.
> > 
> > For now I'd prefer to only do it on platforms where the benefit is clear.
> > 
> > The next step may be to extend it to the other ones, but let's avoid making
> > what is problem mitigation really depend on things that may or may not
> > work elsewhere to start with.
> 
> Then is it possible to invoke acpi_mark_gpe_for_wake() (and maybe also acpi_unmark_gpe_for_wake()) right after invoking uPEP functions?
> So that such platform specific stuffs won't go into ec.c.

I'm not sure ATM, but it should be doable in theory.

Let me try that approach, but it will need to be re-tested then, obviously.

Thanks,
Rafael

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

[Rafael J. Wysocki]

On Thursday, May 04, 2017 07:58:53 AM Zheng, Lv wrote:
> Hi,
> 
> > -----Original Message-----
> > From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-owner@vger.kernel.org] On Behalf Of Rafael J.
> > Wysocki
> > Sent: Friday, April 28, 2017 6:26 AM
> > To: Mario.Limonciello@dell.com
> > Cc: linux-pm@vger.kernel.org; andriy.shevchenko@linux.intel.com; dvhart@infradead.org; linux-
> > kernel@vger.kernel.org; linux-acpi@vger.kernel.org; srinivas.pandruvada@linux.intel.com;
> > tglx@linutronix.de; mika.westerberg@linux.intel.com
> > Subject: Re: [PATCH 3/5] ACPI / sleep: EC-based wakeup from suspend-to-idle on Dell systems
> > 
> > On Thursday, April 27, 2017 02:47:59 PM Mario.Limonciello@dell.com wrote:
> > > > -----Original Message-----
> > > > From: Rafael J. Wysocki [mailto:rjw@rjwysocki.net]
> > > > Sent: Wednesday, April 26, 2017 4:24 PM
> > > > To: Linux PM <linux-pm@vger.kernel.org>
> > > > Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>; Darren Hart
> > > > <dvhart@infradead.org>; LKML <linux-kernel@vger.kernel.org>; Linux ACPI <linux-
> > > > acpi@vger.kernel.org>; Srinivas Pandruvada
> > > > <srinivas.pandruvada@linux.intel.com>; Thomas Gleixner <tglx@linutronix.de>;
> > > > Mika Westerberg <mika.westerberg@linux.intel.com>; Limonciello, Mario
> > > > <Mario_Limonciello@Dell.com>
> > > > Subject: [PATCH 3/5] ACPI / sleep: EC-based wakeup from suspend-to-idle on Dell
> > > > systems
> > > >
> > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > >
> > > > Some recent Dell laptops, including the XPS13 model numbers 9360 and
> > > > 9365, cannot be woken up from suspend-to-idle by pressing the power
> > > > button which is unexpected and makes that feature hardly usable on
> > > > those systems.  However, on the 9365 ACPI S3 (suspend-to-RAM) is not
> > > > expected to be used at all (these systems ship with Windows 10 using
> > > > Modern Standby which never exercises the ACPI S3 path) and
> > > > suspend-to-idle is the only viable system suspend mechanism in there.
> > > >
> > > > The reason why the power button wakeup from suspend-to-idle doesn't
> > > > work on those systems is because their power button events are
> > > > signaled by the EC (Embedded Controller), whose GPE (General Purpose
> > > > Event) line is disabled during suspend-to-idle transitions in Linux.
> > > > That is done on purpose, because in general the EC tends to generate
> > > > tons of events for various reasons (battery and thermal updates and
> > > > similar, for example) and all of them would kick the CPUs out of deep
> > > > idle states while in suspend-to-idle, which would not be desirable.
> > > >
> > > > Of course, on the Dell systems in question the EC GPE must be enabled
> > > > during suspend-to-idle transitions for the button press events to
> > > > be signaled while suspended at all.  Fortunately, there is a way to
> > > > tell the EC to stop generating the non-wakeup events, which is by
> > > > using the _DSM object under the so called micro-PEP (uPEP) device
> > > > provided to support Modern Standby in Windows 10.
> > > >
> > > > The expected way to use it is to invoke function 0 from it on system
> > > > initialization, functions 3 and 5 during suspend transitions and
> > > > functions 4 and 6 during resume transitions (to reverse the actions
> > > > carried out by the former).  In particular, function 5 from the uPEP
> > > > device _DSM causes the EC to become less verbose (so to speak) on the
> > > > affected systems and then its GPE can be enabled as a wakeup source
> > > > (then, on resume, function 6 switches it back to the "working state"
> > > > mode).
> > > >
> > > > In support of the affected Dell systems, implement the uPEP device
> > > > handling as described and allow the EC to generate system wakeup
> > > > events if that device is present and behaves as expected.  Enable
> > > > that for Dell only, as there are other systems out there in which
> > > > the uPEP device is exposed in the ACPI tables and its _DSM appears
> > > > to be functional, but it actually isn't, whereas Dell is committed
> > > > to supporting it.
> > > >
> > >
> > > I am of course biased in that my priority is for this to work for Dell.
> > > Dell is definitely committed to supporting this on any system with
> > > the low power idle bit in the FADT set.
> > >
> > > So I'm fine with the current proposed solution, but have you
> > > dug into what actually breaks on this other system?  Does it actually
> > > work with Modern Standby + the uPEP device on Windows 10?
> > >
> > > To my understanding I would think any OEM that is enabling this
> > > uPEP device it should be getting called by the Windows kernel
> > > identically when entering resiliency phases.
> > >
> > > This makes me wonder if it should be inverted and a blacklist
> > > of platforms that the uPEP device doesn't work.
> > 
> > For now I'd prefer to only do it on platforms where the benefit is clear.
> > 
> > The next step may be to extend it to the other ones, but let's avoid making
> > what is problem mitigation really depend on things that may or may not
> > work elsewhere to start with.
> 
> Then is it possible to invoke acpi_mark_gpe_for_wake() (and maybe also acpi_unmark_gpe_for_wake()) right after invoking uPEP functions?
> So that such platform specific stuffs won't go into ec.c.

I'm not sure ATM, but it should be doable in theory.

Let me try that approach, but it will need to be re-tested then, obviously.

Thanks,
Rafael

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

[Rafael J. Wysocki]

On Thursday, May 04, 2017 04:23:30 PM Rafael J. Wysocki wrote:
> On Thursday, May 04, 2017 07:58:53 AM Zheng, Lv wrote:
> > Hi,
> > 
> > > -----Original Message-----
> > > From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-owner@vger.kernel.org] On Behalf Of Rafael J.
> > > Wysocki
> > > Sent: Friday, April 28, 2017 6:26 AM
> > > To: Mario.Limonciello@dell.com
> > > Cc: linux-pm@vger.kernel.org; andriy.shevchenko@linux.intel.com; dvhart@infradead.org; linux-
> > > kernel@vger.kernel.org; linux-acpi@vger.kernel.org; srinivas.pandruvada@linux.intel.com;
> > > tglx@linutronix.de; mika.westerberg@linux.intel.com
> > > Subject: Re: [PATCH 3/5] ACPI / sleep: EC-based wakeup from suspend-to-idle on Dell systems
> > > 
> > > On Thursday, April 27, 2017 02:47:59 PM Mario.Limonciello@dell.com wrote:
> > > > > -----Original Message-----
> > > > > From: Rafael J. Wysocki [mailto:rjw@rjwysocki.net]
> > > > > Sent: Wednesday, April 26, 2017 4:24 PM
> > > > > To: Linux PM <linux-pm@vger.kernel.org>
> > > > > Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>; Darren Hart
> > > > > <dvhart@infradead.org>; LKML <linux-kernel@vger.kernel.org>; Linux ACPI <linux-
> > > > > acpi@vger.kernel.org>; Srinivas Pandruvada
> > > > > <srinivas.pandruvada@linux.intel.com>; Thomas Gleixner <tglx@linutronix.de>;
> > > > > Mika Westerberg <mika.westerberg@linux.intel.com>; Limonciello, Mario
> > > > > <Mario_Limonciello@Dell.com>
> > > > > Subject: [PATCH 3/5] ACPI / sleep: EC-based wakeup from suspend-to-idle on Dell
> > > > > systems
> > > > >
> > > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > >
> > > > > Some recent Dell laptops, including the XPS13 model numbers 9360 and
> > > > > 9365, cannot be woken up from suspend-to-idle by pressing the power
> > > > > button which is unexpected and makes that feature hardly usable on
> > > > > those systems.  However, on the 9365 ACPI S3 (suspend-to-RAM) is not
> > > > > expected to be used at all (these systems ship with Windows 10 using
> > > > > Modern Standby which never exercises the ACPI S3 path) and
> > > > > suspend-to-idle is the only viable system suspend mechanism in there.
> > > > >
> > > > > The reason why the power button wakeup from suspend-to-idle doesn't
> > > > > work on those systems is because their power button events are
> > > > > signaled by the EC (Embedded Controller), whose GPE (General Purpose
> > > > > Event) line is disabled during suspend-to-idle transitions in Linux.
> > > > > That is done on purpose, because in general the EC tends to generate
> > > > > tons of events for various reasons (battery and thermal updates and
> > > > > similar, for example) and all of them would kick the CPUs out of deep
> > > > > idle states while in suspend-to-idle, which would not be desirable.
> > > > >
> > > > > Of course, on the Dell systems in question the EC GPE must be enabled
> > > > > during suspend-to-idle transitions for the button press events to
> > > > > be signaled while suspended at all.  Fortunately, there is a way to
> > > > > tell the EC to stop generating the non-wakeup events, which is by
> > > > > using the _DSM object under the so called micro-PEP (uPEP) device
> > > > > provided to support Modern Standby in Windows 10.
> > > > >
> > > > > The expected way to use it is to invoke function 0 from it on system
> > > > > initialization, functions 3 and 5 during suspend transitions and
> > > > > functions 4 and 6 during resume transitions (to reverse the actions
> > > > > carried out by the former).  In particular, function 5 from the uPEP
> > > > > device _DSM causes the EC to become less verbose (so to speak) on the
> > > > > affected systems and then its GPE can be enabled as a wakeup source
> > > > > (then, on resume, function 6 switches it back to the "working state"
> > > > > mode).
> > > > >
> > > > > In support of the affected Dell systems, implement the uPEP device
> > > > > handling as described and allow the EC to generate system wakeup
> > > > > events if that device is present and behaves as expected.  Enable
> > > > > that for Dell only, as there are other systems out there in which
> > > > > the uPEP device is exposed in the ACPI tables and its _DSM appears
> > > > > to be functional, but it actually isn't, whereas Dell is committed
> > > > > to supporting it.
> > > > >
> > > >
> > > > I am of course biased in that my priority is for this to work for Dell.
> > > > Dell is definitely committed to supporting this on any system with
> > > > the low power idle bit in the FADT set.
> > > >
> > > > So I'm fine with the current proposed solution, but have you
> > > > dug into what actually breaks on this other system?  Does it actually
> > > > work with Modern Standby + the uPEP device on Windows 10?
> > > >
> > > > To my understanding I would think any OEM that is enabling this
> > > > uPEP device it should be getting called by the Windows kernel
> > > > identically when entering resiliency phases.
> > > >
> > > > This makes me wonder if it should be inverted and a blacklist
> > > > of platforms that the uPEP device doesn't work.
> > > 
> > > For now I'd prefer to only do it on platforms where the benefit is clear.
> > > 
> > > The next step may be to extend it to the other ones, but let's avoid making
> > > what is problem mitigation really depend on things that may or may not
> > > work elsewhere to start with.
> > 
> > Then is it possible to invoke acpi_mark_gpe_for_wake() (and maybe also acpi_unmark_gpe_for_wake()) right after invoking uPEP functions?
> > So that such platform specific stuffs won't go into ec.c.
> 
> I'm not sure ATM, but it should be doable in theory.

So the problem with that is that the EC GPE number is not known to the sleep.c
code, so it would need to be exported by the EC driver somehow or similar,
which would be uglier than the current patch IMO.

Thanks,
Rafael

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

[Rafael J. Wysocki]

On Thursday, May 04, 2017 04:23:30 PM Rafael J. Wysocki wrote:
> On Thursday, May 04, 2017 07:58:53 AM Zheng, Lv wrote:
> > Hi,
> > 
> > > -----Original Message-----
> > > From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-owner@vger.kernel.org] On Behalf Of Rafael J.
> > > Wysocki
> > > Sent: Friday, April 28, 2017 6:26 AM
> > > To: Mario.Limonciello@dell.com
> > > Cc: linux-pm@vger.kernel.org; andriy.shevchenko@linux.intel.com; dvhart@infradead.org; linux-
> > > kernel@vger.kernel.org; linux-acpi@vger.kernel.org; srinivas.pandruvada@linux.intel.com;
> > > tglx@linutronix.de; mika.westerberg@linux.intel.com
> > > Subject: Re: [PATCH 3/5] ACPI / sleep: EC-based wakeup from suspend-to-idle on Dell systems
> > > 
> > > On Thursday, April 27, 2017 02:47:59 PM Mario.Limonciello@dell.com wrote:
> > > > > -----Original Message-----
> > > > > From: Rafael J. Wysocki [mailto:rjw@rjwysocki.net]
> > > > > Sent: Wednesday, April 26, 2017 4:24 PM
> > > > > To: Linux PM <linux-pm@vger.kernel.org>
> > > > > Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>; Darren Hart
> > > > > <dvhart@infradead.org>; LKML <linux-kernel@vger.kernel.org>; Linux ACPI <linux-
> > > > > acpi@vger.kernel.org>; Srinivas Pandruvada
> > > > > <srinivas.pandruvada@linux.intel.com>; Thomas Gleixner <tglx@linutronix.de>;
> > > > > Mika Westerberg <mika.westerberg@linux.intel.com>; Limonciello, Mario
> > > > > <Mario_Limonciello@Dell.com>
> > > > > Subject: [PATCH 3/5] ACPI / sleep: EC-based wakeup from suspend-to-idle on Dell
> > > > > systems
> > > > >
> > > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > >
> > > > > Some recent Dell laptops, including the XPS13 model numbers 9360 and
> > > > > 9365, cannot be woken up from suspend-to-idle by pressing the power
> > > > > button which is unexpected and makes that feature hardly usable on
> > > > > those systems.  However, on the 9365 ACPI S3 (suspend-to-RAM) is not
> > > > > expected to be used at all (these systems ship with Windows 10 using
> > > > > Modern Standby which never exercises the ACPI S3 path) and
> > > > > suspend-to-idle is the only viable system suspend mechanism in there.
> > > > >
> > > > > The reason why the power button wakeup from suspend-to-idle doesn't
> > > > > work on those systems is because their power button events are
> > > > > signaled by the EC (Embedded Controller), whose GPE (General Purpose
> > > > > Event) line is disabled during suspend-to-idle transitions in Linux.
> > > > > That is done on purpose, because in general the EC tends to generate
> > > > > tons of events for various reasons (battery and thermal updates and
> > > > > similar, for example) and all of them would kick the CPUs out of deep
> > > > > idle states while in suspend-to-idle, which would not be desirable.
> > > > >
> > > > > Of course, on the Dell systems in question the EC GPE must be enabled
> > > > > during suspend-to-idle transitions for the button press events to
> > > > > be signaled while suspended at all.  Fortunately, there is a way to
> > > > > tell the EC to stop generating the non-wakeup events, which is by
> > > > > using the _DSM object under the so called micro-PEP (uPEP) device
> > > > > provided to support Modern Standby in Windows 10.
> > > > >
> > > > > The expected way to use it is to invoke function 0 from it on system
> > > > > initialization, functions 3 and 5 during suspend transitions and
> > > > > functions 4 and 6 during resume transitions (to reverse the actions
> > > > > carried out by the former).  In particular, function 5 from the uPEP
> > > > > device _DSM causes the EC to become less verbose (so to speak) on the
> > > > > affected systems and then its GPE can be enabled as a wakeup source
> > > > > (then, on resume, function 6 switches it back to the "working state"
> > > > > mode).
> > > > >
> > > > > In support of the affected Dell systems, implement the uPEP device
> > > > > handling as described and allow the EC to generate system wakeup
> > > > > events if that device is present and behaves as expected.  Enable
> > > > > that for Dell only, as there are other systems out there in which
> > > > > the uPEP device is exposed in the ACPI tables and its _DSM appears
> > > > > to be functional, but it actually isn't, whereas Dell is committed
> > > > > to supporting it.
> > > > >
> > > >
> > > > I am of course biased in that my priority is for this to work for Dell.
> > > > Dell is definitely committed to supporting this on any system with
> > > > the low power idle bit in the FADT set.
> > > >
> > > > So I'm fine with the current proposed solution, but have you
> > > > dug into what actually breaks on this other system?  Does it actually
> > > > work with Modern Standby + the uPEP device on Windows 10?
> > > >
> > > > To my understanding I would think any OEM that is enabling this
> > > > uPEP device it should be getting called by the Windows kernel
> > > > identically when entering resiliency phases.
> > > >
> > > > This makes me wonder if it should be inverted and a blacklist
> > > > of platforms that the uPEP device doesn't work.
> > > 
> > > For now I'd prefer to only do it on platforms where the benefit is clear.
> > > 
> > > The next step may be to extend it to the other ones, but let's avoid making
> > > what is problem mitigation really depend on things that may or may not
> > > work elsewhere to start with.
> > 
> > Then is it possible to invoke acpi_mark_gpe_for_wake() (and maybe also acpi_unmark_gpe_for_wake()) right after invoking uPEP functions?
> > So that such platform specific stuffs won't go into ec.c.
> 
> I'm not sure ATM, but it should be doable in theory.

So the problem with that is that the EC GPE number is not known to the sleep.c
code, so it would need to be exported by the EC driver somehow or similar,
which would be uglier than the current patch IMO.

Thanks,
Rafael

Re: [PATCH 0/5] PM / sleep: Support power button wakeup from S2I on recent Dell laptops

[Rafael J. Wysocki]

On Wednesday, April 26, 2017 11:21:11 PM Rafael J. Wysocki wrote:
> Hi All,
> 
> The underlying issue is that on some relatively new Dell laltops, including
> Dell XPS13 9360 and 9365, pressing the power button is not sufficient to
> wake up the system from suspend-to-idle (it has to be pressed and held
> down for around 5 sec for the wakeup to happen) which is not expected
> and does not match the Windows' behavior.
> 
> This turns out to be a consequence of the way power button events are signaled
> on those systems, which is through the Embedded Controller (EC).  Namely,
> button events go to the EC which then signals the event through its ACPI GPE
> (General Purpose Event), which triggers an ACPI SCI (System Control Interrupt),
> whose handler executes a specicif AML control method and triggers a Notify()
> targetting a devie object associated with the power button.  The problem with
> suspend-to-idle is that the EC GPE is disabled during suspend, because
> otherwise all EC events would wake up the system from suspend-to-idle (and
> there can be many of them).
> 
> The only way to address this is to allow the EC GPE to stay enabled while
> suspended to idle, but that cannot be done unconditionally, because of the
> possible spurious wakeup events from the EC.  Fortunately, on the Dell systems
> in question it is possible to reduce the number of events coming from the EC
> while suspended (see the changelog of patch [3/5] for details), but still some
> spurious events may happen.  For this reason, when there is an SCI while
> suspended, it is not practical to resume all the way up to user space.
> Instead, it is better to resume partially up to the point at which the SCI can
> be processed by its handler, which is after calling dpm_resume_noirq(), let
> the SCI be processed and if no real wakeup events come out of that,
> suspend again.  Actually, that can be done in general, because spurious
> SCIs do happen while suspended to idle on other systems too, and that's
> which it goes as patch [2/5] before the Dell-related changes.
> 
> Patch [1/5] is more of a cleanup, but makes the rest look slightly more
> straightforward, and patches [4-5/5] update the drivers used for button
> events processing on the affected systems so that they signal wakeup
> as expected and avoid propagating the wakeup events as button events
> to user space.
> 
> The series is available from a git branch at
> 
>  git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git s2idle-dell-test
> 
> and has been included into the testing branch thereof.
> 
> Please let me know if there are any concerns.

Patches [1-2/5] from this series don't seem to be controversial, so I'd like to
go ahead with them, as they are general improvements independent of the
rest of the series.

If anyone has any concerns about that, please let me know.

Thanks,
Rafael

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

[Zheng, Lv]

Hi,

> From: Rafael J. Wysocki [mailto:rjw@rjwysocki.net]
> Subject: Re: [PATCH 3/5] ACPI / sleep: EC-based wakeup from suspend-to-idle on Dell systems
> 
> On Thursday, May 04, 2017 04:23:30 PM Rafael J. Wysocki wrote:
> > On Thursday, May 04, 2017 07:58:53 AM Zheng, Lv wrote:
> > > Hi,
> > >
> > > > -----Original Message-----
> > > > From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-owner@vger.kernel.org] On Behalf Of
> Rafael J.
> > > > Wysocki
> > > > Sent: Friday, April 28, 2017 6:26 AM
> > > > To: Mario.Limonciello@dell.com
> > > > Cc: linux-pm@vger.kernel.org; andriy.shevchenko@linux.intel.com; dvhart@infradead.org; linux-
> > > > kernel@vger.kernel.org; linux-acpi@vger.kernel.org; srinivas.pandruvada@linux.intel.com;
> > > > tglx@linutronix.de; mika.westerberg@linux.intel.com
> > > > Subject: Re: [PATCH 3/5] ACPI / sleep: EC-based wakeup from suspend-to-idle on Dell systems
> > > >
> > > > On Thursday, April 27, 2017 02:47:59 PM Mario.Limonciello@dell.com wrote:
> > > > > > -----Original Message-----
> > > > > > From: Rafael J. Wysocki [mailto:rjw@rjwysocki.net]
> > > > > > Sent: Wednesday, April 26, 2017 4:24 PM
> > > > > > To: Linux PM <linux-pm@vger.kernel.org>
> > > > > > Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>; Darren Hart
> > > > > > <dvhart@infradead.org>; LKML <linux-kernel@vger.kernel.org>; Linux ACPI <linux-
> > > > > > acpi@vger.kernel.org>; Srinivas Pandruvada
> > > > > > <srinivas.pandruvada@linux.intel.com>; Thomas Gleixner <tglx@linutronix.de>;
> > > > > > Mika Westerberg <mika.westerberg@linux.intel.com>; Limonciello, Mario
> > > > > > <Mario_Limonciello@Dell.com>
> > > > > > Subject: [PATCH 3/5] ACPI / sleep: EC-based wakeup from suspend-to-idle on Dell
> > > > > > systems
> > > > > >
> > > > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > > >
> > > > > > Some recent Dell laptops, including the XPS13 model numbers 9360 and
> > > > > > 9365, cannot be woken up from suspend-to-idle by pressing the power
> > > > > > button which is unexpected and makes that feature hardly usable on
> > > > > > those systems.  However, on the 9365 ACPI S3 (suspend-to-RAM) is not
> > > > > > expected to be used at all (these systems ship with Windows 10 using
> > > > > > Modern Standby which never exercises the ACPI S3 path) and
> > > > > > suspend-to-idle is the only viable system suspend mechanism in there.
> > > > > >
> > > > > > The reason why the power button wakeup from suspend-to-idle doesn't
> > > > > > work on those systems is because their power button events are
> > > > > > signaled by the EC (Embedded Controller), whose GPE (General Purpose
> > > > > > Event) line is disabled during suspend-to-idle transitions in Linux.
> > > > > > That is done on purpose, because in general the EC tends to generate
> > > > > > tons of events for various reasons (battery and thermal updates and
> > > > > > similar, for example) and all of them would kick the CPUs out of deep
> > > > > > idle states while in suspend-to-idle, which would not be desirable.
> > > > > >
> > > > > > Of course, on the Dell systems in question the EC GPE must be enabled
> > > > > > during suspend-to-idle transitions for the button press events to
> > > > > > be signaled while suspended at all.  Fortunately, there is a way to
> > > > > > tell the EC to stop generating the non-wakeup events, which is by
> > > > > > using the _DSM object under the so called micro-PEP (uPEP) device
> > > > > > provided to support Modern Standby in Windows 10.
> > > > > >
> > > > > > The expected way to use it is to invoke function 0 from it on system
> > > > > > initialization, functions 3 and 5 during suspend transitions and
> > > > > > functions 4 and 6 during resume transitions (to reverse the actions
> > > > > > carried out by the former).  In particular, function 5 from the uPEP
> > > > > > device _DSM causes the EC to become less verbose (so to speak) on the
> > > > > > affected systems and then its GPE can be enabled as a wakeup source
> > > > > > (then, on resume, function 6 switches it back to the "working state"
> > > > > > mode).
> > > > > >
> > > > > > In support of the affected Dell systems, implement the uPEP device
> > > > > > handling as described and allow the EC to generate system wakeup
> > > > > > events if that device is present and behaves as expected.  Enable
> > > > > > that for Dell only, as there are other systems out there in which
> > > > > > the uPEP device is exposed in the ACPI tables and its _DSM appears
> > > > > > to be functional, but it actually isn't, whereas Dell is committed
> > > > > > to supporting it.
> > > > > >
> > > > >
> > > > > I am of course biased in that my priority is for this to work for Dell.
> > > > > Dell is definitely committed to supporting this on any system with
> > > > > the low power idle bit in the FADT set.
> > > > >
> > > > > So I'm fine with the current proposed solution, but have you
> > > > > dug into what actually breaks on this other system?  Does it actually
> > > > > work with Modern Standby + the uPEP device on Windows 10?
> > > > >
> > > > > To my understanding I would think any OEM that is enabling this
> > > > > uPEP device it should be getting called by the Windows kernel
> > > > > identically when entering resiliency phases.
> > > > >
> > > > > This makes me wonder if it should be inverted and a blacklist
> > > > > of platforms that the uPEP device doesn't work.
> > > >
> > > > For now I'd prefer to only do it on platforms where the benefit is clear.
> > > >
> > > > The next step may be to extend it to the other ones, but let's avoid making
> > > > what is problem mitigation really depend on things that may or may not
> > > > work elsewhere to start with.
> > >
> > > Then is it possible to invoke acpi_mark_gpe_for_wake() (and maybe also acpi_unmark_gpe_for_wake())
> right after invoking uPEP functions?
> > > So that such platform specific stuffs won't go into ec.c.
> >
> > I'm not sure ATM, but it should be doable in theory.
> 
> So the problem with that is that the EC GPE number is not known to the sleep.c
> code, so it would need to be exported by the EC driver somehow or similar,
> which would be uglier than the current patch IMO.

Ah, I see.
Anyway, this is not urgent.
We can just focus on user issue now.

Thanks and best regards
Lv

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

[Zheng, Lv]

Hi,

> From: Rafael J. Wysocki [mailto:rjw@rjwysocki.net]
> Subject: Re: [PATCH 3/5] ACPI / sleep: EC-based wakeup from suspend-to-idle on Dell systems
> 
> On Thursday, May 04, 2017 04:23:30 PM Rafael J. Wysocki wrote:
> > On Thursday, May 04, 2017 07:58:53 AM Zheng, Lv wrote:
> > > Hi,
> > >
> > > > -----Original Message-----
> > > > From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-owner@vger.kernel.org] On Behalf Of
> Rafael J.
> > > > Wysocki
> > > > Sent: Friday, April 28, 2017 6:26 AM
> > > > To: Mario.Limonciello@dell.com
> > > > Cc: linux-pm@vger.kernel.org; andriy.shevchenko@linux.intel.com; dvhart@infradead.org; linux-
> > > > kernel@vger.kernel.org; linux-acpi@vger.kernel.org; srinivas.pandruvada@linux.intel.com;
> > > > tglx@linutronix.de; mika.westerberg@linux.intel.com
> > > > Subject: Re: [PATCH 3/5] ACPI / sleep: EC-based wakeup from suspend-to-idle on Dell systems
> > > >
> > > > On Thursday, April 27, 2017 02:47:59 PM Mario.Limonciello@dell.com wrote:
> > > > > > -----Original Message-----
> > > > > > From: Rafael J. Wysocki [mailto:rjw@rjwysocki.net]
> > > > > > Sent: Wednesday, April 26, 2017 4:24 PM
> > > > > > To: Linux PM <linux-pm@vger.kernel.org>
> > > > > > Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>; Darren Hart
> > > > > > <dvhart@infradead.org>; LKML <linux-kernel@vger.kernel.org>; Linux ACPI <linux-
> > > > > > acpi@vger.kernel.org>; Srinivas Pandruvada
> > > > > > <srinivas.pandruvada@linux.intel.com>; Thomas Gleixner <tglx@linutronix.de>;
> > > > > > Mika Westerberg <mika.westerberg@linux.intel.com>; Limonciello, Mario
> > > > > > <Mario_Limonciello@Dell.com>
> > > > > > Subject: [PATCH 3/5] ACPI / sleep: EC-based wakeup from suspend-to-idle on Dell
> > > > > > systems
> > > > > >
> > > > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > > >
> > > > > > Some recent Dell laptops, including the XPS13 model numbers 9360 and
> > > > > > 9365, cannot be woken up from suspend-to-idle by pressing the power
> > > > > > button which is unexpected and makes that feature hardly usable on
> > > > > > those systems.  However, on the 9365 ACPI S3 (suspend-to-RAM) is not
> > > > > > expected to be used at all (these systems ship with Windows 10 using
> > > > > > Modern Standby which never exercises the ACPI S3 path) and
> > > > > > suspend-to-idle is the only viable system suspend mechanism in there.
> > > > > >
> > > > > > The reason why the power button wakeup from suspend-to-idle doesn't
> > > > > > work on those systems is because their power button events are
> > > > > > signaled by the EC (Embedded Controller), whose GPE (General Purpose
> > > > > > Event) line is disabled during suspend-to-idle transitions in Linux.
> > > > > > That is done on purpose, because in general the EC tends to generate
> > > > > > tons of events for various reasons (battery and thermal updates and
> > > > > > similar, for example) and all of them would kick the CPUs out of deep
> > > > > > idle states while in suspend-to-idle, which would not be desirable.
> > > > > >
> > > > > > Of course, on the Dell systems in question the EC GPE must be enabled
> > > > > > during suspend-to-idle transitions for the button press events to
> > > > > > be signaled while suspended at all.  Fortunately, there is a way to
> > > > > > tell the EC to stop generating the non-wakeup events, which is by
> > > > > > using the _DSM object under the so called micro-PEP (uPEP) device
> > > > > > provided to support Modern Standby in Windows 10.
> > > > > >
> > > > > > The expected way to use it is to invoke function 0 from it on system
> > > > > > initialization, functions 3 and 5 during suspend transitions and
> > > > > > functions 4 and 6 during resume transitions (to reverse the actions
> > > > > > carried out by the former).  In particular, function 5 from the uPEP
> > > > > > device _DSM causes the EC to become less verbose (so to speak) on the
> > > > > > affected systems and then its GPE can be enabled as a wakeup source
> > > > > > (then, on resume, function 6 switches it back to the "working state"
> > > > > > mode).
> > > > > >
> > > > > > In support of the affected Dell systems, implement the uPEP device
> > > > > > handling as described and allow the EC to generate system wakeup
> > > > > > events if that device is present and behaves as expected.  Enable
> > > > > > that for Dell only, as there are other systems out there in which
> > > > > > the uPEP device is exposed in the ACPI tables and its _DSM appears
> > > > > > to be functional, but it actually isn't, whereas Dell is committed
> > > > > > to supporting it.
> > > > > >
> > > > >
> > > > > I am of course biased in that my priority is for this to work for Dell.
> > > > > Dell is definitely committed to supporting this on any system with
> > > > > the low power idle bit in the FADT set.
> > > > >
> > > > > So I'm fine with the current proposed solution, but have you
> > > > > dug into what actually breaks on this other system?  Does it actually
> > > > > work with Modern Standby + the uPEP device on Windows 10?
> > > > >
> > > > > To my understanding I would think any OEM that is enabling this
> > > > > uPEP device it should be getting called by the Windows kernel
> > > > > identically when entering resiliency phases.
> > > > >
> > > > > This makes me wonder if it should be inverted and a blacklist
> > > > > of platforms that the uPEP device doesn't work.
> > > >
> > > > For now I'd prefer to only do it on platforms where the benefit is clear.
> > > >
> > > > The next step may be to extend it to the other ones, but let's avoid making
> > > > what is problem mitigation really depend on things that may or may not
> > > > work elsewhere to start with.
> > >
> > > Then is it possible to invoke acpi_mark_gpe_for_wake() (and maybe also acpi_unmark_gpe_for_wake())
> right after invoking uPEP functions?
> > > So that such platform specific stuffs won't go into ec.c.
> >
> > I'm not sure ATM, but it should be doable in theory.
> 
> So the problem with that is that the EC GPE number is not known to the sleep.c
> code, so it would need to be exported by the EC driver somehow or similar,
> which would be uglier than the current patch IMO.

Ah, I see.
Anyway, this is not urgent.
We can just focus on user issue now.

Thanks and best regards
Lv

[PATCH 0/3] ACPI / sleep: Support power button wakeup from S2I on recent Dell laptops

[Rafael J. Wysocki]

Hi All,

This is a follow-up for a patch series posted some time ago:

http://marc.info/?l=linux-kernel&m=149324246701378&w=2

The first two patches from that series are in 4.12-rc already and the rest
have been rearranged.

The issue at hand is still the same as before:

On Wednesday, April 26, 2017 11:21:11 PM Rafael J. Wysocki wrote:
>
> The underlying issue is that on some relatively new Dell laltops, including
> Dell XPS13 9360 and 9365, pressing the power button is not sufficient to
> wake up the system from suspend-to-idle (it has to be pressed and held
> down for around 5 sec for the wakeup to happen) which is not expected
> and does not match the Windows' behavior.
> 
> This turns out to be a consequence of the way power button events are signaled
> on those systems, which is through the Embedded Controller (EC).  Namely,
> button events go to the EC which then signals the event through its ACPI GPE
> (General Purpose Event), which triggers an ACPI SCI (System Control Interrupt),
> whose handler executes a specicif AML control method and triggers a Notify()
> targetting a devie object associated with the power button.  The problem with
> suspend-to-idle is that the EC GPE is disabled during suspend, because
> otherwise all EC events would wake up the system from suspend-to-idle (and
> there can be many of them).

The first two patches in the current series  update the drivers used for button
events processing on the affected systems so that they signal wakeup as
expected and avoid propagating the wakeup events as button events to user
space.

The third patch allows the EC GPE to become a wakeup GPE on the affected Dell
laptops and finally makes power button events wake up those systems from
suspend-to-idle.

After this series there still is a concern regarding the possible increase of
power draw that may result from the processing of non-wakeup EC events while
suspended which is why the change only affects Dell XPS13 9360 and 9365
for now.

There is no code dependency between patches [1-2/3] and patch [3/3], but all
of them together are necessary for the feature in question to work on both the
affected systems, so IMO they should be applied together.

The series is available from a git branch at
 
  git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git s2idle-dell-test
 
and has been included into the testing branch thereof.
 
If there are any concerns regarding this series, please let me know.

Thanks,
Rafael

[PATCH 1/3] platform: x86: intel-vbtn: Wake up the system from suspend-to-idle

[Rafael J. Wysocki]

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

Allow the intel-vbtn driver to wake up the system from suspend-to-idle
by configuring its platform device as a wakeup one by default and
switching it over to a system wakeup events triggering mode during
system suspend transitions.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/platform/x86/intel-vbtn.c |   39 +++++++++++++++++++++++++++++++++++---
 1 file changed, 36 insertions(+), 3 deletions(-)

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
@@ -23,6 +23,7 @@
 #include <linux/platform_device.h>
 #include <linux/input/sparse-keymap.h>
 #include <linux/acpi.h>
+#include <linux/suspend.h>
 #include <acpi/acpi_bus.h>
 
 MODULE_LICENSE("GPL");
@@ -46,6 +47,7 @@ static const struct key_entry intel_vbtn
 
 struct intel_vbtn_priv {
 	struct input_dev *input_dev;
+	bool wakeup_mode;
 };
 
 static int intel_vbtn_input_setup(struct platform_device *device)
@@ -73,9 +75,15 @@ static void notify_handler(acpi_handle h
 	struct platform_device *device = context;
 	struct intel_vbtn_priv *priv = dev_get_drvdata(&device->dev);
 
-	if (!sparse_keymap_report_event(priv->input_dev, event, 1, true))
-		dev_info(&device->dev, "unknown event index 0x%x\n",
-			 event);
+	if (priv->wakeup_mode) {
+		if (sparse_keymap_entry_from_scancode(priv->input_dev, event)) {
+			pm_wakeup_hard_event(&device->dev);
+			return;
+		}
+	} else if (sparse_keymap_report_event(priv->input_dev, event, 1, true)) {
+		return;
+	}
+	dev_info(&device->dev, "unknown event index 0x%x\n", event);
 }
 
 static int intel_vbtn_probe(struct platform_device *device)
@@ -109,6 +117,7 @@ static int intel_vbtn_probe(struct platf
 	if (ACPI_FAILURE(status))
 		return -EBUSY;
 
+	device_init_wakeup(&device->dev, true);
 	return 0;
 }
 
@@ -125,10 +134,34 @@ static int intel_vbtn_remove(struct plat
 	return 0;
 }
 
+static int intel_vbtn_pm_prepare(struct device *dev)
+{
+	struct intel_vbtn_priv *priv = dev_get_drvdata(dev);
+
+	priv->wakeup_mode = true;
+	return 0;
+}
+
+static int intel_vbtn_pm_resume(struct device *dev)
+{
+	struct intel_vbtn_priv *priv = dev_get_drvdata(dev);
+
+	priv->wakeup_mode = false;
+	return 0;
+}
+
+static const struct dev_pm_ops intel_vbtn_pm_ops = {
+	.prepare = intel_vbtn_pm_prepare,
+	.resume = intel_vbtn_pm_resume,
+	.restore = intel_vbtn_pm_resume,
+	.thaw = intel_vbtn_pm_resume,
+};
+
 static struct platform_driver intel_vbtn_pl_driver = {
 	.driver = {
 		.name = "intel-vbtn",
 		.acpi_match_table = intel_vbtn_ids,
+		.pm = &intel_vbtn_pm_ops,
 	},
 	.probe = intel_vbtn_probe,
 	.remove = intel_vbtn_remove,

[PATCH 2/3] platform: x86: intel-hid: Wake up the system from suspend-to-idle

[Rafael J. Wysocki]

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

Allow the intel-hid driver to wake up the system from suspend-to-idle
by configuring its platform device as a wakeup one by default and
switching it over to a system wakeup events triggering mode during
system suspend transitions.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/platform/x86/intel-hid.c |   40 ++++++++++++++++++++++++++++++++++-----
 1 file changed, 35 insertions(+), 5 deletions(-)

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
@@ -23,6 +23,7 @@
 #include <linux/platform_device.h>
 #include <linux/input/sparse-keymap.h>
 #include <linux/acpi.h>
+#include <linux/suspend.h>
 #include <acpi/acpi_bus.h>
 
 MODULE_LICENSE("GPL");
@@ -75,6 +76,7 @@ static const struct key_entry intel_arra
 struct intel_hid_priv {
 	struct input_dev *input_dev;
 	struct input_dev *array;
+	bool wakeup_mode;
 };
 
 static int intel_hid_set_enable(struct device *device, bool enable)
@@ -116,23 +118,37 @@ static void intel_button_array_enable(st
 		dev_warn(device, "failed to set button capability\n");
 }
 
-static int intel_hid_pl_suspend_handler(struct device *device)
+static int intel_hid_pm_prepare(struct device *device)
 {
-	intel_hid_set_enable(device, false);
-	intel_button_array_enable(device, false);
+	struct intel_hid_priv *priv = dev_get_drvdata(device);
+
+	priv->wakeup_mode = true;
+	return 0;
+}
 
+static int intel_hid_pl_suspend_handler(struct device *device)
+{
+	if (pm_suspend_via_firmware()) {
+		intel_hid_set_enable(device, false);
+		intel_button_array_enable(device, false);
+	}
 	return 0;
 }
 
 static int intel_hid_pl_resume_handler(struct device *device)
 {
-	intel_hid_set_enable(device, true);
-	intel_button_array_enable(device, true);
+	struct intel_hid_priv *priv = dev_get_drvdata(device);
 
+	priv->wakeup_mode = false;
+	if (pm_resume_via_firmware()) {
+		intel_hid_set_enable(device, true);
+		intel_button_array_enable(device, true);
+	}
 	return 0;
 }
 
 static const struct dev_pm_ops intel_hid_pl_pm_ops = {
+	.prepare = intel_hid_pm_prepare,
 	.freeze  = intel_hid_pl_suspend_handler,
 	.thaw  = intel_hid_pl_resume_handler,
 	.restore  = intel_hid_pl_resume_handler,
@@ -186,6 +202,19 @@ static void notify_handler(acpi_handle h
 	unsigned long long ev_index;
 	acpi_status status;
 
+	if (priv->wakeup_mode) {
+		/* Wake up on 5-button array events only. */
+		if (event == 0xc0 || !priv->array)
+			return;
+
+		if (sparse_keymap_entry_from_scancode(priv->array, event))
+			pm_wakeup_hard_event(&device->dev);
+		else
+			dev_info(&device->dev, "unknown event 0x%x\n", event);
+
+		return;
+	}
+
 	/* 0xC0 is for HID events, other values are for 5 button array */
 	if (event != 0xc0) {
 		if (!priv->array ||
@@ -270,6 +299,7 @@ static int intel_hid_probe(struct platfo
 				 "failed to enable HID power button\n");
 	}
 
+	device_init_wakeup(&device->dev, true);
 	return 0;
 
 err_remove_notify:

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

[Rafael J. Wysocki]

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

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

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

Of course, on the Dell systems in question the EC GPE must be enabled
during suspend-to-idle transitions for the button press events to
be signaled while suspended at all.  For this reason, add a DMI
switch to the ACPI system suspend infrastructure to treat the EC
GPE as a wakeup one on the affected Dell systems.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/ec.c       |   19 ++++++++++++++++++-
 drivers/acpi/internal.h |    2 ++
 drivers/acpi/sleep.c    |   34 ++++++++++++++++++++++++++++++++++
 3 files changed, 54 insertions(+), 1 deletion(-)

Index: linux-pm/drivers/acpi/ec.c
===================================================================
--- linux-pm.orig/drivers/acpi/ec.c
+++ linux-pm/drivers/acpi/ec.c
@@ -40,6 +40,7 @@
 #include <linux/slab.h>
 #include <linux/acpi.h>
 #include <linux/dmi.h>
+#include <linux/suspend.h>
 #include <asm/io.h>
 
 #include "internal.h"
@@ -1493,6 +1494,16 @@ static int acpi_ec_setup(struct acpi_ec
 	acpi_handle_info(ec->handle,
 			 "GPE=0x%lx, EC_CMD/EC_SC=0x%lx, EC_DATA=0x%lx\n",
 			 ec->gpe, ec->command_addr, ec->data_addr);
+
+	/*
+	 * On some platforms the EC GPE is used for waking up the system from
+	 * suspend-to-idle, so mark it as a wakeup one.
+	 *
+	 * This can be done unconditionally, as the setting does not matter
+	 * until acpi_set_gpe_wake_mask() is called for the GPE.
+	 */
+	acpi_mark_gpe_for_wake(NULL, ec->gpe);
+
 	return ret;
 }
 
@@ -1835,8 +1846,11 @@ static int acpi_ec_suspend(struct device
 	struct acpi_ec *ec =
 		acpi_driver_data(to_acpi_device(dev));
 
-	if (ec_freeze_events)
+	if (!pm_suspend_via_firmware() && acpi_sleep_ec_gpe_may_wakeup())
+		acpi_set_gpe_wake_mask(NULL, ec->gpe, ACPI_GPE_ENABLE);
+	else if (ec_freeze_events)
 		acpi_ec_disable_event(ec);
+
 	return 0;
 }
 
@@ -1846,6 +1860,9 @@ static int acpi_ec_resume(struct device
 		acpi_driver_data(to_acpi_device(dev));
 
 	acpi_ec_enable_event(ec);
+	if (!pm_resume_via_firmware() && acpi_sleep_ec_gpe_may_wakeup())
+		acpi_set_gpe_wake_mask(NULL, ec->gpe, ACPI_GPE_DISABLE);
+
 	return 0;
 }
 #endif
Index: linux-pm/drivers/acpi/internal.h
===================================================================
--- linux-pm.orig/drivers/acpi/internal.h
+++ linux-pm/drivers/acpi/internal.h
@@ -199,8 +199,10 @@ void acpi_ec_remove_query_handler(struct
   -------------------------------------------------------------------------- */
 #ifdef CONFIG_ACPI_SYSTEM_POWER_STATES_SUPPORT
 extern int acpi_sleep_init(void);
+bool acpi_sleep_ec_gpe_may_wakeup(void);
 #else
 static inline int acpi_sleep_init(void) { return -ENXIO; }
+static inline bool acpi_sleep_ec_gpe_may_wakeup(void) { return false; }
 #endif
 
 #ifdef CONFIG_ACPI_SLEEP
Index: linux-pm/drivers/acpi/sleep.c
===================================================================
--- linux-pm.orig/drivers/acpi/sleep.c
+++ linux-pm/drivers/acpi/sleep.c
@@ -160,6 +160,14 @@ static int __init init_nvs_nosave(const
 	return 0;
 }
 
+static bool ec_gpe_wakeup;
+
+static int __init init_ec_gpe_wakeup(const struct dmi_system_id *d)
+{
+	ec_gpe_wakeup = true;
+	return 0;
+}
+
 static struct dmi_system_id acpisleep_dmi_table[] __initdata = {
 	{
 	.callback = init_old_suspend_ordering,
@@ -343,6 +351,26 @@ static struct dmi_system_id acpisleep_dm
 		DMI_MATCH(DMI_PRODUCT_NAME, "80E3"),
 		},
 	},
+	/*
+	 * Enable the EC to wake up the system from suspend-to-idle to allow
+	 * power button events to it wake up.
+	 */
+	{
+	 .callback = init_ec_gpe_wakeup,
+	 .ident = "Dell XPS 13 9360",
+	 .matches = {
+		DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
+		DMI_MATCH(DMI_PRODUCT_NAME, "XPS 13 9360"),
+		},
+	},
+	{
+	 .callback = init_ec_gpe_wakeup,
+	 .ident = "Dell XPS 13 9365",
+	 .matches = {
+		DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
+		DMI_MATCH(DMI_PRODUCT_NAME, "XPS 13 9365"),
+		},
+	},
 	{},
 };
 
@@ -485,6 +513,7 @@ static void acpi_pm_end(void)
 }
 #else /* !CONFIG_ACPI_SLEEP */
 #define acpi_target_sleep_state	ACPI_STATE_S0
+#define ec_gpe_wakeup		false
 static inline void acpi_sleep_dmi_check(void) {}
 #endif /* CONFIG_ACPI_SLEEP */
 
@@ -731,6 +760,11 @@ static void acpi_sleep_suspend_setup(voi
 static inline void acpi_sleep_suspend_setup(void) {}
 #endif /* !CONFIG_SUSPEND */
 
+bool acpi_sleep_ec_gpe_may_wakeup(void)
+{
+	return ec_gpe_wakeup;
+}
+
 #ifdef CONFIG_PM_SLEEP
 static u32 saved_bm_rld;
 

Re: [PATCH 0/3] ACPI / sleep: Support power button wakeup from S2I on recent Dell laptops

[Andy Shevchenko]

On Thu, 2017-06-01 at 01:23 +0200, Rafael J. Wysocki wrote:
> Hi All,
> 
> This is a follow-up for a patch series posted some time ago:
> 
> http://marc.info/?l=linux-kernel&m=149324246701378&w=2
> 
> The first two patches from that series are in 4.12-rc already and the
> rest
> have been rearranged.
> 

...

> The first two patches in the current series  update the drivers used
> for button
> events processing on the affected systems so that they signal wakeup
> as
> expected and avoid propagating the wakeup events as button events to
> user
> space.

...

> There is no code dependency between patches [1-2/3] and patch [3/3],
> but all
> of them together are necessary for the feature in question to work on
> both the
> affected systems, so IMO they should be applied together.
> 
> The series is available from a git branch at
>  
>   git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git
> s2idle-dell-test
>  
> and has been included into the testing branch thereof.
>  
> If there are any concerns regarding this series, please let me know.

Is this supposed to go via linux PM tree?

I'm fine with the first two:
Acked-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>


-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH 0/3] ACPI / sleep: Support power button wakeup from S2I on recent Dell laptops

[Tom Lanyon]

[resend as text/plain]

On Thu, 2017-06-01 at 01:23 +0200, Rafael J. Wysocki wrote:
> Hi All,
>
> This is a follow-up for a patch series posted some time ago:
>
> http://marc.info/?l=linux-kernel&m=149324246701378&w=2

I've applied Rafael's s2idle-dell-test branch to 4.12.0-rc3 and tested
on a Dell 9365 and, whilst it's significantly improved, it's not yet
working correctly.

Previously I could suspend (s2idle and deep), but it took an awkward
~8 second press of the power button to get it to resume, and I could
never get resume to work when triggering suspend/resume via close/open
of the lid.

With this patchset applied, I can suspend (s2idle) and a momentary
press of the power button resumes successfully.  I can also use the
lid switch to both suspend and resume successfully.

However, the EC events appear to trigger the machine to wake very
frequently whilst it's supposed to be suspended. This is visible via
the kernel messages at the end of this mail (leaving it in a suspended
state for a few hours resulted in many thousands of these messages),
and the high power draw witnessed.

Let me know if there's anything I can do to help debug further.

Tom

[   43.669798] PM: Syncing filesystems ... done.
[   43.675412] PM: Preparing system for sleep (freeze)
[   43.695469] Freezing user space processes ... (elapsed 0.001 seconds) done.
[   43.696769] OOM killer disabled.
[   43.696770] Freezing remaining freezable tasks ... (elapsed 0.001
seconds) done.
[   43.697914] PM: Suspending system (freeze)
[   43.910325] wlan0: deauthenticating from 9c:1c:12:c7:c2:f1 by local
choice (Reason: 3=DEAUTH_LEAVING)
[   44.112944] psmouse serio1: Failed to disable mouse on isa0060/serio1
[   45.721790] PM: suspend of devices complete after 1811.701 msecs
[   45.739769] PM: late suspend of devices complete after 17.956 msecs
[   45.742599] ACPI : EC: interrupt blocked
[   45.744774] xhci_hcd 0000:00:14.0: System wakeup enabled by ACPI
[   45.776254] PM: noirq suspend of devices complete after 34.702 msecs
[   45.776256] PM: suspend-to-idle
[   47.029237] Suspended for 0.327 seconds
[   47.029335] PM: resume from suspend-to-idle
[   47.029587] ACPI : EC: interrupt unblocked
[   47.064470] xhci_hcd 0000:00:14.0: System wakeup disabled by ACPI
[   47.065268] PM: noirq resume of devices complete after 35.894 msecs
[   47.066264] ACPI : EC: interrupt blocked
[   47.079210] xhci_hcd 0000:00:14.0: System wakeup enabled by ACPI
[   47.112461] PM: noirq suspend of devices complete after 47.019 msecs
[   47.112538] ACPI : EC: interrupt unblocked
[   47.147672] xhci_hcd 0000:00:14.0: System wakeup disabled by ACPI
[   47.148516] PM: noirq resume of devices complete after 36.051 msecs
[   47.149610] ACPI : EC: interrupt blocked
[   47.161002] xhci_hcd 0000:00:14.0: System wakeup enabled by ACPI
[   47.192546] PM: noirq suspend of devices complete after 43.955 msecs
[   47.192551] PM: suspend-to-idle
[   48.374756] Suspended for 1.836 seconds
[   48.374870] PM: resume from suspend-to-idle
[   48.375284] ACPI : EC: interrupt unblocked

Re: [PATCH 0/3] ACPI / sleep: Support power button wakeup from S2I on recent Dell laptops

[Rafael J. Wysocki]

Hi,

On Thu, Jun 1, 2017 at 1:50 PM, Tom Lanyon <tom@oneshoeco.com> wrote:
> [resend as text/plain]
>
> On Thu, 2017-06-01 at 01:23 +0200, Rafael J. Wysocki wrote:
>> Hi All,
>>
>> This is a follow-up for a patch series posted some time ago:
>>
>> http://marc.info/?l=linux-kernel&m=149324246701378&w=2
>
> I've applied Rafael's s2idle-dell-test branch to 4.12.0-rc3 and tested
> on a Dell 9365 and, whilst it's significantly improved, it's not yet
> working correctly.
>
> Previously I could suspend (s2idle and deep), but it took an awkward
> ~8 second press of the power button to get it to resume, and I could
> never get resume to work when triggering suspend/resume via close/open
> of the lid.
>
> With this patchset applied, I can suspend (s2idle) and a momentary
> press of the power button resumes successfully.  I can also use the
> lid switch to both suspend and resume successfully.
>
> However, the EC events appear to trigger the machine to wake very
> frequently whilst it's supposed to be suspended. This is visible via
> the kernel messages at the end of this mail (leaving it in a suspended
> state for a few hours resulted in many thousands of these messages),
> and the high power draw witnessed.
>
> Let me know if there's anything I can do to help debug further.

Quoting from my cover letter:

"After this series there still is a concern regarding the possible increase of
power draw that may result from the processing of non-wakeup EC events while
suspended which is why the change only affects Dell XPS13 9360 and 9365
for now."

So that is what happens, unfortunately, and we can't do much about it
at the moment.

The only way to avoid that would be to reconfigure the EC during
suspend to stop generating non-wakeup events, but today we have no
reliable way to do that.

Thanks,
Rafael

Re: [PATCH 0/3] ACPI / sleep: Support power button wakeup from S2I on recent Dell laptops

[Rafael J. Wysocki]

On Thu, Jun 1, 2017 at 12:43 PM, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Thu, 2017-06-01 at 01:23 +0200, Rafael J. Wysocki wrote:
>> Hi All,
>>
>> This is a follow-up for a patch series posted some time ago:
>>
>> http://marc.info/?l=linux-kernel&m=149324246701378&w=2
>>
>> The first two patches from that series are in 4.12-rc already and the
>> rest
>> have been rearranged.
>>
>
> ...
>
>> The first two patches in the current series  update the drivers used
>> for button
>> events processing on the affected systems so that they signal wakeup
>> as
>> expected and avoid propagating the wakeup events as button events to
>> user
>> space.
>
> ...
>
>> There is no code dependency between patches [1-2/3] and patch [3/3],
>> but all
>> of them together are necessary for the feature in question to work on
>> both the
>> affected systems, so IMO they should be applied together.
>>
>> The series is available from a git branch at
>>
>>   git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git
>> s2idle-dell-test
>>
>> and has been included into the testing branch thereof.
>>
>> If there are any concerns regarding this series, please let me know.
>
> Is this supposed to go via linux PM tree?

Yes, it is.

> I'm fine with the first two:
> Acked-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Thanks!

Re: [PATCH 0/3] ACPI / sleep: Support power button wakeup from S2I on recent Dell laptops

[Tom Lanyon]

On 2 June 2017 at 00:59, Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> Quoting from my cover letter:
>
> "After this series there still is a concern regarding the possible increase of
> power draw that may result from the processing of non-wakeup EC events while
> suspended which is why the change only affects Dell XPS13 9360 and 9365
> for now."
>
> So that is what happens, unfortunately, and we can't do much about it
> at the moment.

OK, but at the moment this is a regression in functionality on those
platforms. Without this patchset, I can successfully s2idle
suspend/resume on an XPS 9365 (albeit with a little bit of awkward
fiddling of the power button to resume). After the patchset, I can't
realistically go into s2idle at all.

> The only way to avoid that would be to reconfigure the EC during
> suspend to stop generating non-wakeup events, but today we have no
> reliable way to do that.

I thought I had read one one of the threads that this was possible in
the same way that it is for Windows on these laptops.  What's missing
to make this possible?

Tom

Re: [PATCH 0/3] ACPI / sleep: Support power button wakeup from S2I on recent Dell laptops

[Rafael J. Wysocki]

On Friday, June 02, 2017 11:06:49 AM Tom Lanyon wrote:
> On 2 June 2017 at 00:59, Rafael J. Wysocki <rafael@kernel.org> wrote:
> >
> > Quoting from my cover letter:
> >
> > "After this series there still is a concern regarding the possible increase of
> > power draw that may result from the processing of non-wakeup EC events while
> > suspended which is why the change only affects Dell XPS13 9360 and 9365
> > for now."
> >
> > So that is what happens, unfortunately, and we can't do much about it
> > at the moment.
> 
> OK, but at the moment this is a regression in functionality on those
> platforms. Without this patchset, I can successfully s2idle
> suspend/resume on an XPS 9365 (albeit with a little bit of awkward
> fiddling of the power button to resume). After the patchset, I can't
> realistically go into s2idle at all.

Well, if you think about s2idle as a state in which there's no activity in the
system at all, this is not how it is defined.  In fact, if there is a non-wakeup
edge-triggered interrupt occurring while suspended, for example, it will
cause the IRQ core to run a low-level handler for it even though the action
handler(s) won't be run, which is far from "no activity".

s2idle basically is a state in which all system components are (or at least
should be) in low-power states most of the time and user space has been
frozen and patch [3/3] in this series doesn't change that really, so I really
wouldn't call it a "regression in functionality".  [I guess the messages in the
log are somewhat confusing, but see below.]

Of course, it does increase the amount of activity in the system while
suspended which in turn causes more energy to be used and battery life to
shorten, so it may be regarded as a power regression.  Still, it really is a
tradeoff between functionality (power button wakeups working as expected)
and energy-efficiency and I know about a few people who actually prefer the
functionality to be there.

On top of that, there are a few things that can be optimized slightly.  For
instance, we generally run too much code when those EC wakeups happen,
we print too much to the log (even without debug enabled) etc, which also
should reduce the amout of energy that goes away and I have some patches
going in that direction.  Moreover, the messages printed to the logs are
not quite as accurate as they should be which needs to be fixed too.

What really matters is how much more energy the system uses after that patch
(or how much less time it will stay on battery) while suspended.  Can you please
try to estimate that for your system?

In any case, you have a point that there may be users wanting the systems
to use less energy while suspended to idle at the cost of semi-functional power
button wakeups, so I guess I'll add an acpi_sleep= kernel command line switch
to force-disable EC wakeups even on systems where they would be enabled by
default.

> > The only way to avoid that would be to reconfigure the EC during
> > suspend to stop generating non-wakeup events, but today we have no
> > reliable way to do that.
> 
> I thought I had read one one of the threads that this was possible in
> the same way that it is for Windows on these laptops.  What's missing
> to make this possible?

Let's say there is work in progress to make it possible to use that interface
in Linux, but it may not actually do everything we want it to do, so it may not
help much here on this particular laptop model.

Thanks,
Rafael

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

[Mario.Limonciello]

> +	/*
> +	 * Enable the EC to wake up the system from suspend-to-idle to allow
> +	 * power button events to it wake up.
> +	 */
> +	{
> +	 .callback = init_ec_gpe_wakeup,
> +	 .ident = "Dell XPS 13 9360",
> +	 .matches = {
> +		DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> +		DMI_MATCH(DMI_PRODUCT_NAME, "XPS 13 9360"),
> +		},
> +	},
> +	{
> +	 .callback = init_ec_gpe_wakeup,
> +	 .ident = "Dell XPS 13 9365",
> +	 .matches = {
> +		DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> +		DMI_MATCH(DMI_PRODUCT_NAME, "XPS 13 9365"),
> +		},
> +	},
>  	{},
>  };
> 

Although the XPS 9360 and XPS 9365 are currently the only Dell products on
the market that do the wakeup this way, this is part of Dell's BIOS and EC
codebase.  As other Dell products start to ship Windows 10 with Modern Standby
and (eventually) Linux with Suspend-to-Idle I'd expect this list to be growing.

I know that we talked about this previously in relation to the EC notification
of going in to suspend-to-idle and you had instead marked it across all Dell Inc systems.

Rather than hardcode a whitelist of platform that can do this, could you do the
same thing here and allow EC wakeups across Dell Inc. systems?

Thanks,

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

[Mario.Limonciello]

> +	/*
> +	 * Enable the EC to wake up the system from suspend-to-idle to allow
> +	 * power button events to it wake up.
> +	 */
> +	{
> +	 .callback = init_ec_gpe_wakeup,
> +	 .ident = "Dell XPS 13 9360",
> +	 .matches = {
> +		DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> +		DMI_MATCH(DMI_PRODUCT_NAME, "XPS 13 9360"),
> +		},
> +	},
> +	{
> +	 .callback = init_ec_gpe_wakeup,
> +	 .ident = "Dell XPS 13 9365",
> +	 .matches = {
> +		DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> +		DMI_MATCH(DMI_PRODUCT_NAME, "XPS 13 9365"),
> +		},
> +	},
>  	{},
>  };
> 

Although the XPS 9360 and XPS 9365 are currently the only Dell products on
the market that do the wakeup this way, this is part of Dell's BIOS and EC
codebase.  As other Dell products start to ship Windows 10 with Modern Standby
and (eventually) Linux with Suspend-to-Idle I'd expect this list to be growing.

I know that we talked about this previously in relation to the EC notification
of going in to suspend-to-idle and you had instead marked it across all Dell Inc systems.

Rather than hardcode a whitelist of platform that can do this, could you do the
same thing here and allow EC wakeups across Dell Inc. systems?

Thanks,

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

[Rafael J. Wysocki]

On Mon, Jun 5, 2017 at 5:18 PM,  <Mario.Limonciello@dell.com> wrote:
>> +     /*
>> +      * Enable the EC to wake up the system from suspend-to-idle to allow
>> +      * power button events to it wake up.
>> +      */
>> +     {
>> +      .callback = init_ec_gpe_wakeup,
>> +      .ident = "Dell XPS 13 9360",
>> +      .matches = {
>> +             DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
>> +             DMI_MATCH(DMI_PRODUCT_NAME, "XPS 13 9360"),
>> +             },
>> +     },
>> +     {
>> +      .callback = init_ec_gpe_wakeup,
>> +      .ident = "Dell XPS 13 9365",
>> +      .matches = {
>> +             DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
>> +             DMI_MATCH(DMI_PRODUCT_NAME, "XPS 13 9365"),
>> +             },
>> +     },
>>       {},
>>  };
>>
>
> Although the XPS 9360 and XPS 9365 are currently the only Dell products on
> the market that do the wakeup this way, this is part of Dell's BIOS and EC
> codebase.  As other Dell products start to ship Windows 10 with Modern Standby
> and (eventually) Linux with Suspend-to-Idle I'd expect this list to be growing.

Well, if init_ec_gpe_wakeup() was enabled on all Dell systems at this
point, it would affect all previous generations of them too and that
might not be desirable.

Thanks,
Rafael

[PATCH v2 0/3] ACPI / sleep: Support power button wakeup from S2I on recent Dell laptops

[Rafael J. Wysocki]

Hi All,

On Thursday, June 01, 2017 01:23:43 AM Rafael J. Wysocki wrote:
> 
> This is a follow-up for a patch series posted some time ago:
> 
> http://marc.info/?l=linux-kernel&m=149324246701378&w=2
> 
> The first two patches from that series are in 4.12-rc already and the rest
> have been rearranged.

Here's a v2 of the above.

The first two patches are actually the same as before, only with ACKs from Andy.

The third patch is almost the same, but it adds a new command line switch to
go back to the semi-functional power button wakeup from s2idle if someone
really wants that.

> The issue at hand is still the same as before:
> 
> On Wednesday, April 26, 2017 11:21:11 PM Rafael J. Wysocki wrote:
> >
> > The underlying issue is that on some relatively new Dell laltops, including
> > Dell XPS13 9360 and 9365, pressing the power button is not sufficient to
> > wake up the system from suspend-to-idle (it has to be pressed and held
> > down for around 5 sec for the wakeup to happen) which is not expected
> > and does not match the Windows' behavior.
> > 
> > This turns out to be a consequence of the way power button events are signaled
> > on those systems, which is through the Embedded Controller (EC).  Namely,
> > button events go to the EC which then signals the event through its ACPI GPE
> > (General Purpose Event), which triggers an ACPI SCI (System Control Interrupt),
> > whose handler executes a specicif AML control method and triggers a Notify()
> > targetting a devie object associated with the power button.  The problem with
> > suspend-to-idle is that the EC GPE is disabled during suspend, because
> > otherwise all EC events would wake up the system from suspend-to-idle (and
> > there can be many of them).
> 
> The first two patches in the current series  update the drivers used for button
> events processing on the affected systems so that they signal wakeup as
> expected and avoid propagating the wakeup events as button events to user
> space.
> 
> The third patch allows the EC GPE to become a wakeup GPE on the affected Dell
> laptops and finally makes power button events wake up those systems from
> suspend-to-idle.
> 
> After this series there still is a concern regarding the possible increase of
> power draw that may result from the processing of non-wakeup EC events while
> suspended which is why the change only affects Dell XPS13 9360 and 9365
> for now.
> 
> There is no code dependency between patches [1-2/3] and patch [3/3], but all
> of them together are necessary for the feature in question to work on both the
> affected systems, so IMO they should be applied together.

The series is based on current linux-next.  It is available from a git branch at

  git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git s2idle-dell-test

and has been included into the testing branch thereof.
  
If there are any concerns regarding this series, please let me know.

Thanks,
Rafael

[PATCH v2 1/3] platform: x86: intel-vbtn: Wake up the system from suspend-to-idle

[Rafael J. Wysocki]

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

Allow the intel-vbtn driver to wake up the system from suspend-to-idle
by configuring its platform device as a wakeup one by default and
switching it over to a system wakeup events triggering mode during
system suspend transitions.

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

-> v2: Added tag.

---
 drivers/platform/x86/intel-vbtn.c |   39 +++++++++++++++++++++++++++++++++++---
 1 file changed, 36 insertions(+), 3 deletions(-)

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
@@ -23,6 +23,7 @@
 #include <linux/platform_device.h>
 #include <linux/input/sparse-keymap.h>
 #include <linux/acpi.h>
+#include <linux/suspend.h>
 #include <acpi/acpi_bus.h>
 
 MODULE_LICENSE("GPL");
@@ -46,6 +47,7 @@ static const struct key_entry intel_vbtn
 
 struct intel_vbtn_priv {
 	struct input_dev *input_dev;
+	bool wakeup_mode;
 };
 
 static int intel_vbtn_input_setup(struct platform_device *device)
@@ -73,9 +75,15 @@ static void notify_handler(acpi_handle h
 	struct platform_device *device = context;
 	struct intel_vbtn_priv *priv = dev_get_drvdata(&device->dev);
 
-	if (!sparse_keymap_report_event(priv->input_dev, event, 1, true))
-		dev_info(&device->dev, "unknown event index 0x%x\n",
-			 event);
+	if (priv->wakeup_mode) {
+		if (sparse_keymap_entry_from_scancode(priv->input_dev, event)) {
+			pm_wakeup_hard_event(&device->dev);
+			return;
+		}
+	} else if (sparse_keymap_report_event(priv->input_dev, event, 1, true)) {
+		return;
+	}
+	dev_info(&device->dev, "unknown event index 0x%x\n", event);
 }
 
 static int intel_vbtn_probe(struct platform_device *device)
@@ -109,6 +117,7 @@ static int intel_vbtn_probe(struct platf
 	if (ACPI_FAILURE(status))
 		return -EBUSY;
 
+	device_init_wakeup(&device->dev, true);
 	return 0;
 }
 
@@ -125,10 +134,34 @@ static int intel_vbtn_remove(struct plat
 	return 0;
 }
 
+static int intel_vbtn_pm_prepare(struct device *dev)
+{
+	struct intel_vbtn_priv *priv = dev_get_drvdata(dev);
+
+	priv->wakeup_mode = true;
+	return 0;
+}
+
+static int intel_vbtn_pm_resume(struct device *dev)
+{
+	struct intel_vbtn_priv *priv = dev_get_drvdata(dev);
+
+	priv->wakeup_mode = false;
+	return 0;
+}
+
+static const struct dev_pm_ops intel_vbtn_pm_ops = {
+	.prepare = intel_vbtn_pm_prepare,
+	.resume = intel_vbtn_pm_resume,
+	.restore = intel_vbtn_pm_resume,
+	.thaw = intel_vbtn_pm_resume,
+};
+
 static struct platform_driver intel_vbtn_pl_driver = {
 	.driver = {
 		.name = "intel-vbtn",
 		.acpi_match_table = intel_vbtn_ids,
+		.pm = &intel_vbtn_pm_ops,
 	},
 	.probe = intel_vbtn_probe,
 	.remove = intel_vbtn_remove,

[PATCH v2 2/3] platform: x86: intel-hid: Wake up the system from suspend-to-idle

[Rafael J. Wysocki]

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

Allow the intel-hid driver to wake up the system from suspend-to-idle
by configuring its platform device as a wakeup one by default and
switching it over to a system wakeup events triggering mode during
system suspend transitions.

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

-> v2: Added tag.

---
 drivers/platform/x86/intel-hid.c |   40 ++++++++++++++++++++++++++++++++++-----
 1 file changed, 35 insertions(+), 5 deletions(-)

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
@@ -23,6 +23,7 @@
 #include <linux/platform_device.h>
 #include <linux/input/sparse-keymap.h>
 #include <linux/acpi.h>
+#include <linux/suspend.h>
 #include <acpi/acpi_bus.h>
 
 MODULE_LICENSE("GPL");
@@ -75,6 +76,7 @@ static const struct key_entry intel_arra
 struct intel_hid_priv {
 	struct input_dev *input_dev;
 	struct input_dev *array;
+	bool wakeup_mode;
 };
 
 static int intel_hid_set_enable(struct device *device, bool enable)
@@ -116,23 +118,37 @@ static void intel_button_array_enable(st
 		dev_warn(device, "failed to set button capability\n");
 }
 
-static int intel_hid_pl_suspend_handler(struct device *device)
+static int intel_hid_pm_prepare(struct device *device)
 {
-	intel_hid_set_enable(device, false);
-	intel_button_array_enable(device, false);
+	struct intel_hid_priv *priv = dev_get_drvdata(device);
+
+	priv->wakeup_mode = true;
+	return 0;
+}
 
+static int intel_hid_pl_suspend_handler(struct device *device)
+{
+	if (pm_suspend_via_firmware()) {
+		intel_hid_set_enable(device, false);
+		intel_button_array_enable(device, false);
+	}
 	return 0;
 }
 
 static int intel_hid_pl_resume_handler(struct device *device)
 {
-	intel_hid_set_enable(device, true);
-	intel_button_array_enable(device, true);
+	struct intel_hid_priv *priv = dev_get_drvdata(device);
 
+	priv->wakeup_mode = false;
+	if (pm_resume_via_firmware()) {
+		intel_hid_set_enable(device, true);
+		intel_button_array_enable(device, true);
+	}
 	return 0;
 }
 
 static const struct dev_pm_ops intel_hid_pl_pm_ops = {
+	.prepare = intel_hid_pm_prepare,
 	.freeze  = intel_hid_pl_suspend_handler,
 	.thaw  = intel_hid_pl_resume_handler,
 	.restore  = intel_hid_pl_resume_handler,
@@ -186,6 +202,19 @@ static void notify_handler(acpi_handle h
 	unsigned long long ev_index;
 	acpi_status status;
 
+	if (priv->wakeup_mode) {
+		/* Wake up on 5-button array events only. */
+		if (event == 0xc0 || !priv->array)
+			return;
+
+		if (sparse_keymap_entry_from_scancode(priv->array, event))
+			pm_wakeup_hard_event(&device->dev);
+		else
+			dev_info(&device->dev, "unknown event 0x%x\n", event);
+
+		return;
+	}
+
 	/* 0xC0 is for HID events, other values are for 5 button array */
 	if (event != 0xc0) {
 		if (!priv->array ||
@@ -270,6 +299,7 @@ static int intel_hid_probe(struct platfo
 				 "failed to enable HID power button\n");
 	}
 
+	device_init_wakeup(&device->dev, true);
 	return 0;
 
 err_remove_notify:

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

[Rafael J. Wysocki]

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

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

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

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

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

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

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

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

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

[Zheng, Lv]

Hi, Rafael

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

I have a concern here.

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

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

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

Thanks and best regards
Lv

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

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

[Rafael J. Wysocki]

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

[cut]

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

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

Thanks,
Rafael

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

[Linus Torvalds]

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

This seems a big hacky.

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

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

                Linus

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

[Rafael J. Wysocki]

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

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

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

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

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

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

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

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

Thanks,
Rafael

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

[Linus Torvalds]

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

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

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

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

              Linus

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

[Rafael J. Wysocki]

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

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

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

Thanks,
Rafael

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

[Zheng, Lv]

Hi,

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

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

Best regards
Lv

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

[Zheng, Lv]

Hi, Rafael

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

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

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

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

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

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

This sounds reasonable, but I have a question.

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

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

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

Thanks,
Lv

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

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

[Rafael J. Wysocki]

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

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

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

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

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

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

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

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

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

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

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

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

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

Thanks,
Rafael

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

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

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

[Linus Torvalds]

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

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

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

Anyway - thanks.

             Linus

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

[Zheng, Lv]

Hi, Rafael

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

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

Thanks and best regards
Lv

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

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

[Rafael J. Wysocki]

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

[cut]

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

Yes, it looks like that, thanks!

Rafael

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

[Rafael J. Wysocki]

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

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

Thanks,
Rafael

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

[Rafael J. Wysocki]

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

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

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

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

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

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

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

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

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

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

-> v2:
 Add the s2idle_in_progress variable to distinguish between s2idle and other
 transitions, as pm_suspend_via_firmware() is not entirely sufficient for that,
 and use it in acpi_sleep_no_ec_events().

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

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

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

[Mario.Limonciello]

> -----Original Message-----
> From: Rafael J. Wysocki [mailto:rjw@rjwysocki.net]
> Sent: Friday, June 23, 2017 8:15 AM
> To: Linux ACPI <linux-acpi@vger.kernel.org>
> Cc: Linux PM <linux-pm@vger.kernel.org>; Andy Shevchenko
> <andriy.shevchenko@linux.intel.com>; Darren Hart <dvhart@infradead.org>; LKML
> <linux-kernel@vger.kernel.org>; Srinivas Pandruvada
> <srinivas.pandruvada@linux.intel.com>; Mika Westerberg
> <mika.westerberg@linux.intel.com>; Limonciello, Mario
> <Mario_Limonciello@Dell.com>; Tom Lanyon <tom@oneshoeco.com>; Jérôme de
> Bretagne <jerome.debretagne@gmail.com>; Linus Torvalds <torvalds@linux-
> foundation.org>; Zheng, Lv <lv.zheng@intel.com>
> Subject: [PATCH v2] ACPI / sleep: EC-based wakeup from suspend-to-idle on recent
> systems
> 
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Some recent Dell laptops, including the XPS13 model numbers 9360 and
> 9365, cannot be woken up from suspend-to-idle by pressing the power
> button which is unexpected and makes that feature less usable on
> those systems.  Moreover, on the 9365 ACPI S3 (suspend-to-RAM) is
> not expected to be used at all (the OS these systems ship with never
> exercises the ACPI S3 path in the firmware) and suspend-to-idle is
> the only viable system suspend mechanism there.
> 
> The reason why the power button wakeup from suspend-to-idle doesn't
> work on those systems is because their power button events are
> signaled by the EC (Embedded Controller), whose GPE (General Purpose
> Event) line is disabled during suspend-to-idle transitions in Linux.
> That is done on purpose, because in general the EC tends to be noisy
> for various reasons (battery and thermal updates and similar, for
> example) and all events signaled by it would kick the CPUs out of
> deep idle states while in suspend-to-idle, which effectively might
> defeat its purpose.
> 
> Of course, on the Dell systems in question the EC GPE must be enabled
> during suspend-to-idle transitions for the button press events to
> be signaled while suspended at all, but fortunately there is a way
> out of this puzzle.
> 
> First of all, those systems have the ACPI_FADT_LOW_POWER_S0 flag set
> in their ACPI tables, which means that the OS is expected to prefer
> the "low power S0 idle" system state over ACPI S3 on them.  That
> causes the most recent versions of other OSes to simply ignore ACPI
> S3 on those systems, so it is reasonable to expect that it should not
> be necessary to block GPEs during suspend-to-idle on them.
> 
> Second, in addition to that, the systems in question provide a special
> firmware interface that can be used to indicate to the platform that
> the OS is transitioning into a system-wide low-power state in which
> certain types of activity are not desirable or that it is leaving
> such a state and that (in principle) should allow the platform to
> adjust its operation mode accordingly.
> 
> That interface is a special _DSM object under a System Power
> Management Controller device (PNP0D80).  The expected way to use it
> is to invoke function 0 from it on system initialization, functions
> 3 and 5 during suspend transitions and functions 4 and 6 during
> resume transitions (to reverse the actions carried out by the
> former).  In particular, function 5 from the "Low-Power S0" device
> _DSM is expected to cause the platform to put itself into a low-power
> operation mode which should include making the EC less verbose (so to
> speak).  Next, on resume, function 6 switches the platform back to
> the "working-state" operation mode.
> 
> In accordance with the above, modify the ACPI suspend-to-idle code
> to look for the "Low-Power S0" _DSM interface on platforms with the
> ACPI_FADT_LOW_POWER_S0 flag set in the ACPI tables.  If it's there,
> use it during suspend-to-idle transitions as prescribed and avoid
> changing the GPE configuration in that case.  [That should reflect
> what the most recent versions of other OSes do.]
> 
> Also modify the ACPI EC driver to make it handle events during
> suspend-to-idle in the usual way if the "Low-Power S0" _DSM interface
> is going to be used to make the power button events work while
> suspended on the Dell machines mentioned above
> 
> Link:
> http://www.uefi.org/sites/default/files/resources/Intel_ACPI_Low_Power_S0_Idle.
> pdf
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
> 
> -> v2:
>  Add the s2idle_in_progress variable to distinguish between s2idle and other
>  transitions, as pm_suspend_via_firmware() is not entirely sufficient for that,
>  and use it in acpi_sleep_no_ec_events().
> 
> ---
>  drivers/acpi/ec.c       |    2
>  drivers/acpi/internal.h |    2
>  drivers/acpi/sleep.c    |  113
> ++++++++++++++++++++++++++++++++++++++++++++++--
>  3 files changed, 112 insertions(+), 5 deletions(-)
> 
> Index: linux-pm/drivers/acpi/sleep.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/sleep.c
> +++ linux-pm/drivers/acpi/sleep.c
> @@ -650,18 +650,108 @@ static const struct platform_suspend_ops
>  	.recover = acpi_pm_finish,
>  };
> 
> +static bool s2idle_in_progress;
>  static bool s2idle_wakeup;
> 
> +/*
> + * On platforms supporting the Low Power S0 Idle interface there is an ACPI
> + * device object with the PNP0D80 compatible device ID (System Power
> Management
> + * Controller) and a specific _DSM method under it.  That method, if present,
> + * can be used to indicate to the platform that the OS is transitioning into a
> + * low-power state in which certain types of activity are not desirable or that
> + * it is leaving such a state, which allows the platform to adjust its operation
> + * mode accordingly.
> + */
> +static const struct acpi_device_id lps0_device_ids[] = {
> +	{"PNP0D80", },
> +	{"", },
> +};
> +
> +#define ACPI_LPS0_DSM_UUID	"c4eb40a0-6cd2-11e2-bcfd-0800200c9a66"
> +
> +#define ACPI_LPS0_SCREEN_OFF	3
> +#define ACPI_LPS0_SCREEN_ON	4
> +#define ACPI_LPS0_ENTRY		5
> +#define ACPI_LPS0_EXIT		6
The spec you shared also defines device constraints (function 1). It would be very 
useful if these constraints  could be parsed and compared against the actual power 
states of devices on the system at least for debugging purposes.  I'm not sure if you 
already had a plan for that in a future series.

> +
> +#define ACPI_S2IDLE_FUNC_MASK	((1 << ACPI_LPS0_ENTRY) | (1 <<
> ACPI_LPS0_EXIT))
> +
> +static acpi_handle lps0_device_handle;
> +static guid_t lps0_dsm_guid;
> +static char lps0_dsm_func_mask;
> +
> +static void acpi_sleep_run_lps0_dsm(unsigned int func)
> +{
> +	union acpi_object *out_obj;
> +
> +	if (!(lps0_dsm_func_mask & (1 << func)))
> +		return;
> +
> +	out_obj = acpi_evaluate_dsm(lps0_device_handle, &lps0_dsm_guid, 1,
> func, NULL);
> +	ACPI_FREE(out_obj);
> +
> +	acpi_handle_debug(lps0_device_handle, "_DSM function %u evaluation
> %s\n",
> +			  func, out_obj ? "successful" : "failed");
> +}
> +
> +static int lps0_device_attach(struct acpi_device *adev,
> +			      const struct acpi_device_id *not_used)
> +{
> +	union acpi_object *out_obj;
> +
> +	if (lps0_device_handle)
> +		return 0;
> +
> +	if (!(acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0))
> +		return 0;
Although the spec you shared refers to PNP0D80/INT33A1 in the context 
of LPIT, the PNP0D80 device is not "only" used for low power S0.  It's 
available on systems that don't support modern standby too.

I for example see it on a system running Windows that does not support 
modern standby such as the Precision 5510.

All of the ASL executed in PNP0D80 is guarded with a check 
whether or not that low power idle supported bit has been set and whether
or not running on an OSPM that exported a group of features indicating it 
should support it to ensure its run in the right context.

Since Linux responds as the latest group of Windows features but doesn't 
look at ACPI_FADT_LOW_POWER_S0 yet to determine whether to default to 
suspend to idle i'm a little worried about developing more unexercised code 
paths specific to Linux.

For example:
System has ACPI_FADT_LOW_POWER_S0 set, but also supports S3
(such as XPS 9360)
* On Windows PNP0D80 should be used, all ASL code specific to 
modern standby will be run.
* On Linux (with current patch) if a user invokes S3, PNP0D80 doesn't get used
[Win7 should still be using this PNP0D80 codepath, not used by Linux]

* On Linux (if PNP0D80 was supported on all systems but) a user invoked
S3, PNP0D80 functions would be run.
[This should be an undefined behavior since the ASL would run the modern
standby related code but then go into S3]


And yes I realize have argued both for and against exporting PNP0D80 to more 
systems above.  

I think the proper way to align to Windows behavior is recognize PNP0D80 on
all systems and also look at ACPI_FADT_LOW_POWER_S0 at the same time to
align using S2I instead of S3 by default.

Perhaps this is best placed in a follow up patch that can be easily reverted without 
messing up the wonderful work you've done so far in case my idea ends up causing 
other regressions that are not yet envisioned.

> +
> +	guid_parse(ACPI_LPS0_DSM_UUID, &lps0_dsm_guid);
> +	/* Check if the _DSM is present and as expected. */
> +	out_obj = acpi_evaluate_dsm(adev->handle, &lps0_dsm_guid, 1, 0, NULL);
> +	if (out_obj && out_obj->type == ACPI_TYPE_BUFFER) {
> +		char bitmask = *(char *)out_obj->buffer.pointer;
> +
> +		if ((bitmask & ACPI_S2IDLE_FUNC_MASK) ==
> ACPI_S2IDLE_FUNC_MASK) {
> +			lps0_dsm_func_mask = bitmask;
> +			lps0_device_handle = adev->handle;
> +		}
> +
> +		acpi_handle_debug(adev->handle, "_DSM function mask: 0x%x\n",
> +				  bitmask);
> +	} else {
> +		acpi_handle_debug(adev->handle,
> +				  "_DSM function 0 evaluation failed\n");
> +	}
> +	ACPI_FREE(out_obj);
> +	return 0;
> +}
> +
> +static struct acpi_scan_handler lps0_handler = {
> +	.ids = lps0_device_ids,
> +	.attach = lps0_device_attach,
> +};
> +
>  static int acpi_freeze_begin(void)
>  {
>  	acpi_scan_lock_acquire();
> +	s2idle_in_progress = true;
>  	return 0;
>  }
> 
>  static int acpi_freeze_prepare(void)
>  {
> -	acpi_enable_all_wakeup_gpes();
> -	acpi_os_wait_events_complete();
> +	if (lps0_device_handle) {
> +		acpi_sleep_run_lps0_dsm(ACPI_LPS0_SCREEN_OFF);

Since there is a separate event specifically for the screen being turned off
I think it would make sense to also export this so that the graphics stack 
could potentially also call this in the future.

In the short term it makes sense to me to call it from the ACPI driver immediately
before resiliency like now though.

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

[Mario.Limonciello]

> -----Original Message-----
> From: Rafael J. Wysocki [mailto:rjw@rjwysocki.net]
> Sent: Friday, June 23, 2017 8:15 AM
> To: Linux ACPI <linux-acpi@vger.kernel.org>
> Cc: Linux PM <linux-pm@vger.kernel.org>; Andy Shevchenko
> <andriy.shevchenko@linux.intel.com>; Darren Hart <dvhart@infradead.org>; LKML
> <linux-kernel@vger.kernel.org>; Srinivas Pandruvada
> <srinivas.pandruvada@linux.intel.com>; Mika Westerberg
> <mika.westerberg@linux.intel.com>; Limonciello, Mario
> <Mario_Limonciello@Dell.com>; Tom Lanyon <tom@oneshoeco.com>; Jérôme de
> Bretagne <jerome.debretagne@gmail.com>; Linus Torvalds <torvalds@linux-
> foundation.org>; Zheng, Lv <lv.zheng@intel.com>
> Subject: [PATCH v2] ACPI / sleep: EC-based wakeup from suspend-to-idle on recent
> systems
> 
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Some recent Dell laptops, including the XPS13 model numbers 9360 and
> 9365, cannot be woken up from suspend-to-idle by pressing the power
> button which is unexpected and makes that feature less usable on
> those systems.  Moreover, on the 9365 ACPI S3 (suspend-to-RAM) is
> not expected to be used at all (the OS these systems ship with never
> exercises the ACPI S3 path in the firmware) and suspend-to-idle is
> the only viable system suspend mechanism there.
> 
> The reason why the power button wakeup from suspend-to-idle doesn't
> work on those systems is because their power button events are
> signaled by the EC (Embedded Controller), whose GPE (General Purpose
> Event) line is disabled during suspend-to-idle transitions in Linux.
> That is done on purpose, because in general the EC tends to be noisy
> for various reasons (battery and thermal updates and similar, for
> example) and all events signaled by it would kick the CPUs out of
> deep idle states while in suspend-to-idle, which effectively might
> defeat its purpose.
> 
> Of course, on the Dell systems in question the EC GPE must be enabled
> during suspend-to-idle transitions for the button press events to
> be signaled while suspended at all, but fortunately there is a way
> out of this puzzle.
> 
> First of all, those systems have the ACPI_FADT_LOW_POWER_S0 flag set
> in their ACPI tables, which means that the OS is expected to prefer
> the "low power S0 idle" system state over ACPI S3 on them.  That
> causes the most recent versions of other OSes to simply ignore ACPI
> S3 on those systems, so it is reasonable to expect that it should not
> be necessary to block GPEs during suspend-to-idle on them.
> 
> Second, in addition to that, the systems in question provide a special
> firmware interface that can be used to indicate to the platform that
> the OS is transitioning into a system-wide low-power state in which
> certain types of activity are not desirable or that it is leaving
> such a state and that (in principle) should allow the platform to
> adjust its operation mode accordingly.
> 
> That interface is a special _DSM object under a System Power
> Management Controller device (PNP0D80).  The expected way to use it
> is to invoke function 0 from it on system initialization, functions
> 3 and 5 during suspend transitions and functions 4 and 6 during
> resume transitions (to reverse the actions carried out by the
> former).  In particular, function 5 from the "Low-Power S0" device
> _DSM is expected to cause the platform to put itself into a low-power
> operation mode which should include making the EC less verbose (so to
> speak).  Next, on resume, function 6 switches the platform back to
> the "working-state" operation mode.
> 
> In accordance with the above, modify the ACPI suspend-to-idle code
> to look for the "Low-Power S0" _DSM interface on platforms with the
> ACPI_FADT_LOW_POWER_S0 flag set in the ACPI tables.  If it's there,
> use it during suspend-to-idle transitions as prescribed and avoid
> changing the GPE configuration in that case.  [That should reflect
> what the most recent versions of other OSes do.]
> 
> Also modify the ACPI EC driver to make it handle events during
> suspend-to-idle in the usual way if the "Low-Power S0" _DSM interface
> is going to be used to make the power button events work while
> suspended on the Dell machines mentioned above
> 
> Link:
> http://www.uefi.org/sites/default/files/resources/Intel_ACPI_Low_Power_S0_Idle.
> pdf
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
> 
> -> v2:
>  Add the s2idle_in_progress variable to distinguish between s2idle and other
>  transitions, as pm_suspend_via_firmware() is not entirely sufficient for that,
>  and use it in acpi_sleep_no_ec_events().
> 
> ---
>  drivers/acpi/ec.c       |    2
>  drivers/acpi/internal.h |    2
>  drivers/acpi/sleep.c    |  113
> ++++++++++++++++++++++++++++++++++++++++++++++--
>  3 files changed, 112 insertions(+), 5 deletions(-)
> 
> Index: linux-pm/drivers/acpi/sleep.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/sleep.c
> +++ linux-pm/drivers/acpi/sleep.c
> @@ -650,18 +650,108 @@ static const struct platform_suspend_ops
>  	.recover = acpi_pm_finish,
>  };
> 
> +static bool s2idle_in_progress;
>  static bool s2idle_wakeup;
> 
> +/*
> + * On platforms supporting the Low Power S0 Idle interface there is an ACPI
> + * device object with the PNP0D80 compatible device ID (System Power
> Management
> + * Controller) and a specific _DSM method under it.  That method, if present,
> + * can be used to indicate to the platform that the OS is transitioning into a
> + * low-power state in which certain types of activity are not desirable or that
> + * it is leaving such a state, which allows the platform to adjust its operation
> + * mode accordingly.
> + */
> +static const struct acpi_device_id lps0_device_ids[] = {
> +	{"PNP0D80", },
> +	{"", },
> +};
> +
> +#define ACPI_LPS0_DSM_UUID	"c4eb40a0-6cd2-11e2-bcfd-0800200c9a66"
> +
> +#define ACPI_LPS0_SCREEN_OFF	3
> +#define ACPI_LPS0_SCREEN_ON	4
> +#define ACPI_LPS0_ENTRY		5
> +#define ACPI_LPS0_EXIT		6
The spec you shared also defines device constraints (function 1). It would be very 
useful if these constraints  could be parsed and compared against the actual power 
states of devices on the system at least for debugging purposes.  I'm not sure if you 
already had a plan for that in a future series.

> +
> +#define ACPI_S2IDLE_FUNC_MASK	((1 << ACPI_LPS0_ENTRY) | (1 <<
> ACPI_LPS0_EXIT))
> +
> +static acpi_handle lps0_device_handle;
> +static guid_t lps0_dsm_guid;
> +static char lps0_dsm_func_mask;
> +
> +static void acpi_sleep_run_lps0_dsm(unsigned int func)
> +{
> +	union acpi_object *out_obj;
> +
> +	if (!(lps0_dsm_func_mask & (1 << func)))
> +		return;
> +
> +	out_obj = acpi_evaluate_dsm(lps0_device_handle, &lps0_dsm_guid, 1,
> func, NULL);
> +	ACPI_FREE(out_obj);
> +
> +	acpi_handle_debug(lps0_device_handle, "_DSM function %u evaluation
> %s\n",
> +			  func, out_obj ? "successful" : "failed");
> +}
> +
> +static int lps0_device_attach(struct acpi_device *adev,
> +			      const struct acpi_device_id *not_used)
> +{
> +	union acpi_object *out_obj;
> +
> +	if (lps0_device_handle)
> +		return 0;
> +
> +	if (!(acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0))
> +		return 0;
Although the spec you shared refers to PNP0D80/INT33A1 in the context 
of LPIT, the PNP0D80 device is not "only" used for low power S0.  It's 
available on systems that don't support modern standby too.

I for example see it on a system running Windows that does not support 
modern standby such as the Precision 5510.

All of the ASL executed in PNP0D80 is guarded with a check 
whether or not that low power idle supported bit has been set and whether
or not running on an OSPM that exported a group of features indicating it 
should support it to ensure its run in the right context.

Since Linux responds as the latest group of Windows features but doesn't 
look at ACPI_FADT_LOW_POWER_S0 yet to determine whether to default to 
suspend to idle i'm a little worried about developing more unexercised code 
paths specific to Linux.

For example:
System has ACPI_FADT_LOW_POWER_S0 set, but also supports S3
(such as XPS 9360)
* On Windows PNP0D80 should be used, all ASL code specific to 
modern standby will be run.
* On Linux (with current patch) if a user invokes S3, PNP0D80 doesn't get used
[Win7 should still be using this PNP0D80 codepath, not used by Linux]

* On Linux (if PNP0D80 was supported on all systems but) a user invoked
S3, PNP0D80 functions would be run.
[This should be an undefined behavior since the ASL would run the modern
standby related code but then go into S3]


And yes I realize have argued both for and against exporting PNP0D80 to more 
systems above.  

I think the proper way to align to Windows behavior is recognize PNP0D80 on
all systems and also look at ACPI_FADT_LOW_POWER_S0 at the same time to
align using S2I instead of S3 by default.

Perhaps this is best placed in a follow up patch that can be easily reverted without 
messing up the wonderful work you've done so far in case my idea ends up causing 
other regressions that are not yet envisioned.

> +
> +	guid_parse(ACPI_LPS0_DSM_UUID, &lps0_dsm_guid);
> +	/* Check if the _DSM is present and as expected. */
> +	out_obj = acpi_evaluate_dsm(adev->handle, &lps0_dsm_guid, 1, 0, NULL);
> +	if (out_obj && out_obj->type == ACPI_TYPE_BUFFER) {
> +		char bitmask = *(char *)out_obj->buffer.pointer;
> +
> +		if ((bitmask & ACPI_S2IDLE_FUNC_MASK) ==
> ACPI_S2IDLE_FUNC_MASK) {
> +			lps0_dsm_func_mask = bitmask;
> +			lps0_device_handle = adev->handle;
> +		}
> +
> +		acpi_handle_debug(adev->handle, "_DSM function mask: 0x%x\n",
> +				  bitmask);
> +	} else {
> +		acpi_handle_debug(adev->handle,
> +				  "_DSM function 0 evaluation failed\n");
> +	}
> +	ACPI_FREE(out_obj);
> +	return 0;
> +}
> +
> +static struct acpi_scan_handler lps0_handler = {
> +	.ids = lps0_device_ids,
> +	.attach = lps0_device_attach,
> +};
> +
>  static int acpi_freeze_begin(void)
>  {
>  	acpi_scan_lock_acquire();
> +	s2idle_in_progress = true;
>  	return 0;
>  }
> 
>  static int acpi_freeze_prepare(void)
>  {
> -	acpi_enable_all_wakeup_gpes();
> -	acpi_os_wait_events_complete();
> +	if (lps0_device_handle) {
> +		acpi_sleep_run_lps0_dsm(ACPI_LPS0_SCREEN_OFF);

Since there is a separate event specifically for the screen being turned off
I think it would make sense to also export this so that the graphics stack 
could potentially also call this in the future.

In the short term it makes sense to me to call it from the ACPI driver immediately
before resiliency like now though.

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

[Srinivas Pandruvada]

On Fri, 2017-06-23 at 15:37 +0000, Mario.Limonciello@dell.com wrote:

[...]

> > 
> > +#define ACPI_LPS0_SCREEN_ON	4
> > +#define ACPI_LPS0_ENTRY		5
> > +#define ACPI_LPS0_EXIT		6
> The spec you shared also defines device constraints (function 1). It
> would be very 
> useful if these constraints  could be parsed and compared against the
> actual power 
> states of devices on the system at least for debugging purposes.  I'm
> not sure if you 
> already had a plan for that in a future series.
> 
For debug purpose, I have worked on a patch to dump the constraint
table in debugfs. But in the freeze path whether we meet the
constraints or not will not make any difference, other than for just
debugging.

Thanks,
Srinivas

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

[Mario.Limonciello]

> -----Original Message-----
> From: Srinivas Pandruvada [mailto:srinivas.pandruvada@linux.intel.com]
> Sent: Friday, June 23, 2017 11:06 AM
> To: Limonciello, Mario <Mario_Limonciello@Dell.com>; rjw@rjwysocki.net; linux-
> acpi@vger.kernel.org
> Cc: linux-pm@vger.kernel.org; andriy.shevchenko@linux.intel.com;
> dvhart@infradead.org; linux-kernel@vger.kernel.org;
> mika.westerberg@linux.intel.com; tom@oneshoeco.com;
> jerome.debretagne@gmail.com; torvalds@linux-foundation.org;
> lv.zheng@intel.com
> Subject: Re: [PATCH v2] ACPI / sleep: EC-based wakeup from suspend-to-idle on
> recent systems
> 
> On Fri, 2017-06-23 at 15:37 +0000, Mario.Limonciello@dell.com wrote:
> 
> [...]
> 
> > >
> > > +#define ACPI_LPS0_SCREEN_ON	4
> > > +#define ACPI_LPS0_ENTRY		5
> > > +#define ACPI_LPS0_EXIT		6
> > The spec you shared also defines device constraints (function 1). It
> > would be very
> > useful if these constraints  could be parsed and compared against the
> > actual power
> > states of devices on the system at least for debugging purposes.  I'm
> > not sure if you
> > already had a plan for that in a future series.
> >
> For debug purpose, I have worked on a patch to dump the constraint
> table in debugfs. But in the freeze path whether we meet the
> constraints or not will not make any difference, other than for just
> debugging.
> 
> Thanks,
> Srinivas

Right that was what I thought would be most interesting.  You can potentially
output to syslog as a last step going down what isn't in the right state to match
the constraint table.

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

[Mario.Limonciello]

> -----Original Message-----
> From: Srinivas Pandruvada [mailto:srinivas.pandruvada@linux.intel.com]
> Sent: Friday, June 23, 2017 11:06 AM
> To: Limonciello, Mario <Mario_Limonciello@Dell.com>; rjw@rjwysocki.net; linux-
> acpi@vger.kernel.org
> Cc: linux-pm@vger.kernel.org; andriy.shevchenko@linux.intel.com;
> dvhart@infradead.org; linux-kernel@vger.kernel.org;
> mika.westerberg@linux.intel.com; tom@oneshoeco.com;
> jerome.debretagne@gmail.com; torvalds@linux-foundation.org;
> lv.zheng@intel.com
> Subject: Re: [PATCH v2] ACPI / sleep: EC-based wakeup from suspend-to-idle on
> recent systems
> 
> On Fri, 2017-06-23 at 15:37 +0000, Mario.Limonciello@dell.com wrote:
> 
> [...]
> 
> > >
> > > +#define ACPI_LPS0_SCREEN_ON	4
> > > +#define ACPI_LPS0_ENTRY		5
> > > +#define ACPI_LPS0_EXIT		6
> > The spec you shared also defines device constraints (function 1). It
> > would be very
> > useful if these constraints  could be parsed and compared against the
> > actual power
> > states of devices on the system at least for debugging purposes.  I'm
> > not sure if you
> > already had a plan for that in a future series.
> >
> For debug purpose, I have worked on a patch to dump the constraint
> table in debugfs. But in the freeze path whether we meet the
> constraints or not will not make any difference, other than for just
> debugging.
> 
> Thanks,
> Srinivas

Right that was what I thought would be most interesting.  You can potentially
output to syslog as a last step going down what isn't in the right state to match
the constraint table.

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

[Rafael J. Wysocki]

On Friday, June 23, 2017 03:37:36 PM Mario.Limonciello@dell.com wrote:
> > -----Original Message-----

[cut]

> > Index: linux-pm/drivers/acpi/sleep.c
> > ===================================================================
> > --- linux-pm.orig/drivers/acpi/sleep.c
> > +++ linux-pm/drivers/acpi/sleep.c
> > @@ -650,18 +650,108 @@ static const struct platform_suspend_ops
> >  	.recover = acpi_pm_finish,
> >  };
> > 
> > +static bool s2idle_in_progress;
> >  static bool s2idle_wakeup;
> > 
> > +/*
> > + * On platforms supporting the Low Power S0 Idle interface there is an ACPI
> > + * device object with the PNP0D80 compatible device ID (System Power
> > Management
> > + * Controller) and a specific _DSM method under it.  That method, if present,
> > + * can be used to indicate to the platform that the OS is transitioning into a
> > + * low-power state in which certain types of activity are not desirable or that
> > + * it is leaving such a state, which allows the platform to adjust its operation
> > + * mode accordingly.
> > + */
> > +static const struct acpi_device_id lps0_device_ids[] = {
> > +	{"PNP0D80", },
> > +	{"", },
> > +};
> > +
> > +#define ACPI_LPS0_DSM_UUID	"c4eb40a0-6cd2-11e2-bcfd-0800200c9a66"
> > +
> > +#define ACPI_LPS0_SCREEN_OFF	3
> > +#define ACPI_LPS0_SCREEN_ON	4
> > +#define ACPI_LPS0_ENTRY		5
> > +#define ACPI_LPS0_EXIT		6
> The spec you shared also defines device constraints (function 1). It would be very 
> useful if these constraints  could be parsed and compared against the actual power 
> states of devices on the system at least for debugging purposes.  I'm not sure if you 
> already had a plan for that in a future series.

As Srinivas said, there is a plan to use it for debug purposes going forward.

> 
> > +
> > +#define ACPI_S2IDLE_FUNC_MASK	((1 << ACPI_LPS0_ENTRY) | (1 <<
> > ACPI_LPS0_EXIT))
> > +
> > +static acpi_handle lps0_device_handle;
> > +static guid_t lps0_dsm_guid;
> > +static char lps0_dsm_func_mask;
> > +
> > +static void acpi_sleep_run_lps0_dsm(unsigned int func)
> > +{
> > +	union acpi_object *out_obj;
> > +
> > +	if (!(lps0_dsm_func_mask & (1 << func)))
> > +		return;
> > +
> > +	out_obj = acpi_evaluate_dsm(lps0_device_handle, &lps0_dsm_guid, 1,
> > func, NULL);
> > +	ACPI_FREE(out_obj);
> > +
> > +	acpi_handle_debug(lps0_device_handle, "_DSM function %u evaluation
> > %s\n",
> > +			  func, out_obj ? "successful" : "failed");
> > +}
> > +
> > +static int lps0_device_attach(struct acpi_device *adev,
> > +			      const struct acpi_device_id *not_used)
> > +{
> > +	union acpi_object *out_obj;
> > +
> > +	if (lps0_device_handle)
> > +		return 0;
> > +
> > +	if (!(acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0))
> > +		return 0;
> Although the spec you shared refers to PNP0D80/INT33A1 in the context 
> of LPIT, the PNP0D80 device is not "only" used for low power S0.  It's 
> available on systems that don't support modern standby too.
> 
> I for example see it on a system running Windows that does not support 
> modern standby such as the Precision 5510.
> 
> All of the ASL executed in PNP0D80 is guarded with a check 
> whether or not that low power idle supported bit has been set and whether
> or not running on an OSPM that exported a group of features indicating it 
> should support it to ensure its run in the right context.
> 
> Since Linux responds as the latest group of Windows features but doesn't 
> look at ACPI_FADT_LOW_POWER_S0 yet to determine whether to default to 
> suspend to idle i'm a little worried about developing more unexercised code 
> paths specific to Linux.
> 
> For example:
> System has ACPI_FADT_LOW_POWER_S0 set, but also supports S3
> (such as XPS 9360)
> * On Windows PNP0D80 should be used, all ASL code specific to 
> modern standby will be run.
> * On Linux (with current patch) if a user invokes S3, PNP0D80 doesn't get used
> [Win7 should still be using this PNP0D80 codepath, not used by Linux]

Well, this is the situation already without this patch. :-)

> 
> * On Linux (if PNP0D80 was supported on all systems but) a user invoked
> S3, PNP0D80 functions would be run.
> [This should be an undefined behavior since the ASL would run the modern
> standby related code but then go into S3]
> 
> 
> And yes I realize have argued both for and against exporting PNP0D80 to more 
> systems above.  
> 
> I think the proper way to align to Windows behavior is recognize PNP0D80 on
> all systems and also look at ACPI_FADT_LOW_POWER_S0 at the same time to
> align using S2I instead of S3 by default.
> 
> Perhaps this is best placed in a follow up patch that can be easily reverted without 
> messing up the wonderful work you've done so far in case my idea ends up causing 
> other regressions that are not yet envisioned.

As you correctly pointed out above, the published documentation refers to the
PNP0D80 _DSM in the "low-power S0" context only and this is the context in
which we are expected to use it for the time being.

In principle, its coverage can be extended in the future as far as I'm concerned,
but it woud be good to be able to clearly demonstrate the benefit from doing
that (like fixing suspend issues or reducing the power draw while suspended on
some systems etc).

That clearly is not directly related to the issue targeted by this patch, however.

> > +
> > +	guid_parse(ACPI_LPS0_DSM_UUID, &lps0_dsm_guid);
> > +	/* Check if the _DSM is present and as expected. */
> > +	out_obj = acpi_evaluate_dsm(adev->handle, &lps0_dsm_guid, 1, 0, NULL);
> > +	if (out_obj && out_obj->type == ACPI_TYPE_BUFFER) {
> > +		char bitmask = *(char *)out_obj->buffer.pointer;
> > +
> > +		if ((bitmask & ACPI_S2IDLE_FUNC_MASK) ==
> > ACPI_S2IDLE_FUNC_MASK) {
> > +			lps0_dsm_func_mask = bitmask;
> > +			lps0_device_handle = adev->handle;
> > +		}
> > +
> > +		acpi_handle_debug(adev->handle, "_DSM function mask: 0x%x\n",
> > +				  bitmask);
> > +	} else {
> > +		acpi_handle_debug(adev->handle,
> > +				  "_DSM function 0 evaluation failed\n");
> > +	}
> > +	ACPI_FREE(out_obj);
> > +	return 0;
> > +}
> > +
> > +static struct acpi_scan_handler lps0_handler = {
> > +	.ids = lps0_device_ids,
> > +	.attach = lps0_device_attach,
> > +};
> > +
> >  static int acpi_freeze_begin(void)
> >  {
> >  	acpi_scan_lock_acquire();
> > +	s2idle_in_progress = true;
> >  	return 0;
> >  }
> > 
> >  static int acpi_freeze_prepare(void)
> >  {
> > -	acpi_enable_all_wakeup_gpes();
> > -	acpi_os_wait_events_complete();
> > +	if (lps0_device_handle) {
> > +		acpi_sleep_run_lps0_dsm(ACPI_LPS0_SCREEN_OFF);
> 
> Since there is a separate event specifically for the screen being turned off
> I think it would make sense to also export this so that the graphics stack 
> could potentially also call this in the future.

Agreed.

It would require extra care to avoid invoking the same _DSM function twice in
a row, though.

> In the short term it makes sense to me to call it from the ACPI driver immediately
> before resiliency like now though.

OK

Thanks,
Rafael

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

[Tom Lanyon]

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

Rafael - thanks for your efforts on this.

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

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

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

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

-Tom

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

[Andy Shevchenko]

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

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

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


-- 
With Best Regards,
Andy Shevchenko

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

[Tom Lanyon]

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

Is this something I can determine without CONFIG_ACPI_DEBUG being enabled?

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

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

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

[Mario.Limonciello]

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

Tom,

This is quite useful data points to provide, thanks.

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

Thanks,

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

[Mario.Limonciello]

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

Tom,

This is quite useful data points to provide, thanks.

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

Thanks,

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

[Rafael J. Wysocki]

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

You're welcome!

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

Thanks for the data.

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

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

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

Thanks,
Rafael

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

[Srinivas Pandruvada]

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

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

Thanks,
Srinivas


> 
> Thanks,
> Rafael
> 

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

[Tom Lanyon]

Sorry for the slow response - I've just had a chance to run some more tests.

I tried to disable the SD card reader in the BIOS as suggested earlier
in the thread, but that didn't seem to make a significant change.
More inline below.

On Tue, 2017-06-27 at 17:03 +0200, Rafael J. Wysocki wrote:
> I would carry out s2idle under turbostat to see how much PC10
> residency is there while suspended.  That may be a significant factor.
>
> Most likely there is a device preventing the SoC from reaching its
> deepest low-power states under Linux on your system and it needs to be
> identified and dealt with.

I'm not entirely sure how turbostat records metrics so wasn't sure how
to measure correctly.  I kept turbostat running while performing a
s2idle and recorded the output:

https://gist.githubusercontent.com/tomlanyon/3238e742a155e7fa27658aa0960bdee4/raw/98b5f050e5eb2f88af47b2afd17080e7dd85d20f/turbostat

I'm not familiar with the output format - I see some high percentages
of C10, but nothing in Pkg%pc10. Which is of interest in this
scenario?

On 28 June 2017 at 02:14, Srinivas Pandruvada
<srinivas.pandruvada@linux.intel.com> wrote:
> Also make sure that you have no FW loading error for i915.
> #dmesg | grep i915
> It will display that Guc FW was loaded etc..
> The latest FW can be downloaded from
> https://01.org/linuxgraphics/downloads/firmware
>
> If you don't see PC10 residency, we can try something more.

I've confirmed that there's no FW errors for the i915.

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

[Srinivas Pandruvada]

On Thu, 2017-07-06 at 21:41 +1000, Tom Lanyon wrote:
> Sorry for the slow response - I've just had a chance to run some more
> tests.
> 
> I tried to disable the SD card reader in the BIOS as suggested
> earlier
> in the thread, but that didn't seem to make a significant change.
> More inline below.
> 
> On Tue, 2017-06-27 at 17:03 +0200, Rafael J. Wysocki wrote:
> > 
> > I would carry out s2idle under turbostat to see how much PC10
> > residency is there while suspended.  That may be a significant
> > factor.
> > 
> > Most likely there is a device preventing the SoC from reaching its
> > deepest low-power states under Linux on your system and it needs to
> > be
> > identified and dealt with.
> I'm not entirely sure how turbostat records metrics so wasn't sure
> how
> to measure correctly.  I kept turbostat running while performing a
> s2idle and recorded the output:
> 
> https://gist.githubusercontent.com/tomlanyon/3238e742a155e7fa27658aa0
> 960bdee4/raw/98b5f050e5eb2f88af47b2afd17080e7dd85d20f/turbostat
> 
> I'm not familiar with the output format - I see some high percentages
> of C10, but nothing in Pkg%pc10. Which is of interest in this
> scenario?
I see that you do have Pkg%pc10 of 86.56 in one interval. The system
was in Pkg%pc8 for multiple intervals before Pkg%pc10. So can you try
this also before your test?

# cd /sys/kernel/debug/pmc_core
# for i in {0..32}; do echo $i > ltr_ignore; done

Thanks,
Srinivas	

> 
> On 28 June 2017 at 02:14, Srinivas Pandruvada
> <srinivas.pandruvada@linux.intel.com> wrote:
> > 
> > Also make sure that you have no FW loading error for i915.
> > #dmesg | grep i915
> > It will display that Guc FW was loaded etc..
> > The latest FW can be downloaded from
> > https://01.org/linuxgraphics/downloads/firmware
> > 
> > If you don't see PC10 residency, we can try something more.
> I've confirmed that there's no FW errors for the i915.