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

[Zheng, Lv]

Hi,

> From: rjwysocki@xxxxxxxxx [mailto:rjwysocki@xxxxxxxxx] 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@xxxxxxxxx> wrote:
> > Hi, Rafael
> >
> >> From: linux-acpi-owner@xxxxxxxxxxxxxxx [mailto:linux-acpi-owner@xxxxxxxxxxxxxxx] 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@xxxxxxxxx>
> >>
> >> 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