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

Zheng, Lv
2017-06-21T01:13:33Z

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