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

Commit Message

Rafael J. Wysocki June 19, 2017, 9:53 p.m. UTC

From: Rafael J. Wysocki <[e-mail]>

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 <[e-mail]>
---

-> 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(-)

Comments

Lv Zheng June 19, 2017, 11:37 p.m. UTC | #1

Hi, Rafael

> From: [e-mail] [mailto:[e-mail]] 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 <[e-mail]>
> 
> 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 <[e-mail]>
> ---
> 
> -> 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 [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Rafael J. Wysocki June 19, 2017, 11:46 p.m. UTC | #2

On Tue, Jun 20, 2017 at 1:37 AM, Zheng, Lv <[e-mail]> wrote:
> Hi, Rafael
>
>> From: [e-mail] [mailto:[e-mail]] 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 <[e-mail]>
>>
>> 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

Linus Torvalds June 20, 2017, 12:07 a.m. UTC | #3

On Tue, Jun 20, 2017 at 5:53 AM, Rafael J. Wysocki <[e-mail]> 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

Rafael J. Wysocki June 20, 2017, 1:13 a.m. UTC | #4

On Tue, Jun 20, 2017 at 2:07 AM, Linus Torvalds
<[e-mail]> wrote:
> On Tue, Jun 20, 2017 at 5:53 AM, Rafael J. Wysocki <[e-mail]> 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

Linus Torvalds June 20, 2017, 2 a.m. UTC | #5

On Tue, Jun 20, 2017 at 9:13 AM, Rafael J. Wysocki <[e-mail]> 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

Rafael J. Wysocki June 20, 2017, 9:16 p.m. UTC | #6

On Tue, Jun 20, 2017 at 4:00 AM, Linus Torvalds
<[e-mail]> wrote:
> On Tue, Jun 20, 2017 at 9:13 AM, Rafael J. Wysocki <[e-mail]> 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

Lv Zheng June 21, 2017, 1:13 a.m. UTC | #7

Hi, Rafael

> From: [e-mail] [mailto:[e-mail]] 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

> <[e-mail]> wrote:

> > On Tue, Jun 20, 2017 at 5:53 AM, Rafael J. Wysocki <[e-mail]> 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 [e-mail]

> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Lv Zheng June 21, 2017, 1:13 a.m. UTC | #8

Hi,

> From: [e-mail] [mailto:[e-mail]] 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 <[e-mail]> wrote:

> > Hi, Rafael

> >

> >> From: [e-mail] [mailto:[e-mail]] 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 <[e-mail]>

> >>

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

Patch

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