On Wed, Aug 21, 2019 at 10:06:09AM +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
>
> Commit 10a08fd65ec1 ("ACPI: PM: Set up EC GPE for system wakeup from
> drivers that need it") assumed that the EC GPE would only need to be
> set up for system wakeup if either the intel-hid or the intel-vbtn
> driver was in use, but that turns out to be incorrect. In particular,
> on ASUS Zenbook UX430UNR/i7-8550U, if the EC GPE is not enabled while
> suspended, the system cannot be woken up by opening the lid or
> pressing a key, and that machine doesn't use any of the drivers
> mentioned above.
>
> For this reason, always set up the EC GPE for system wakeup from
> suspend-to-idle by setting and clearing its wake mask in the ACPI
> suspend-to-idle callbacks.
Acked-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
Couple of minor comments below.
> Fixes: 10a08fd65ec1 ("ACPI: PM: Set up EC GPE for system wakeup from drivers that need it")
> Reported-by: Kristian Klausen <kristian@xxxxxxxxxx>
> Tested-by: Kristian Klausen <kristian@xxxxxxxxxx>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> ---
>
> Commit 10a08fd65ec1 is present in linux-next.
>
> ---
> drivers/acpi/ec.c | 1 -
> drivers/acpi/sleep.c | 15 +++++++++++++--
> drivers/platform/x86/intel-hid.c | 5 +----
> drivers/platform/x86/intel-vbtn.c | 5 +----
> 4 files changed, 15 insertions(+), 11 deletions(-)
>
> Index: linux-pm/drivers/acpi/sleep.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/sleep.c
> +++ linux-pm/drivers/acpi/sleep.c
> @@ -938,6 +938,13 @@ static int lps0_device_attach(struct acp
> if (mem_sleep_default > PM_SUSPEND_MEM && !acpi_sleep_default_s3)
> mem_sleep_current = PM_SUSPEND_TO_IDLE;
>
> + /*
> + * Some LPS0 systems, like ASUS Zenbook UX430UNR/i7-8550U, require the
> + * EC GPE to be enabled while suspended for certain wakeup devices to
> + * work, so mark it as wakeup-capable.
> + */
> + acpi_ec_mark_gpe_for_wake();
> +
> return 0;
> }
>
> @@ -954,8 +961,10 @@ static int acpi_s2idle_begin(void)
>
> static int acpi_s2idle_prepare(void)
> {
> - if (acpi_sci_irq_valid())
> + if (acpi_sci_irq_valid()) {
> enable_irq_wake(acpi_sci_irq);
> + acpi_ec_set_gpe_wake_mask(ACPI_GPE_ENABLE);
> + }
>
> acpi_enable_wakeup_devices(ACPI_STATE_S0);
>
> @@ -1034,8 +1043,10 @@ static void acpi_s2idle_restore(void)
>
> acpi_disable_wakeup_devices(ACPI_STATE_S0);
>
> - if (acpi_sci_irq_valid())
> + if (acpi_sci_irq_valid()) {
> + acpi_ec_set_gpe_wake_mask(ACPI_GPE_DISABLE);
> disable_irq_wake(acpi_sci_irq);
> + }
> }
>
> static void acpi_s2idle_end(void)
> Index: linux-pm/drivers/platform/x86/intel-hid.c
> ===================================================================
> --- linux-pm.orig/drivers/platform/x86/intel-hid.c
> +++ linux-pm/drivers/platform/x86/intel-hid.c
> @@ -257,7 +257,6 @@ static int intel_hid_pm_prepare(struct d
> struct intel_hid_priv *priv = dev_get_drvdata(device);
>
> priv->wakeup_mode = true;
> - acpi_ec_set_gpe_wake_mask(ACPI_GPE_ENABLE);
> }
> return 0;
> }
> @@ -266,10 +265,8 @@ static void intel_hid_pm_complete(struct
> {
> struct intel_hid_priv *priv = dev_get_drvdata(device);
>
> - if (priv->wakeup_mode) {
> - acpi_ec_set_gpe_wake_mask(ACPI_GPE_DISABLE);
> + if (priv->wakeup_mode)
But this now seems does not add any value and we can assign unconditionally.
> priv->wakeup_mode = false;
> - }
> }
>
> static int intel_hid_pl_suspend_handler(struct device *device)
> Index: linux-pm/drivers/platform/x86/intel-vbtn.c
> ===================================================================
> --- linux-pm.orig/drivers/platform/x86/intel-vbtn.c
> +++ linux-pm/drivers/platform/x86/intel-vbtn.c
> @@ -205,7 +205,6 @@ static int intel_vbtn_pm_prepare(struct
> struct intel_vbtn_priv *priv = dev_get_drvdata(dev);
>
> priv->wakeup_mode = true;
> - acpi_ec_set_gpe_wake_mask(ACPI_GPE_ENABLE);
> }
> return 0;
> }
> @@ -214,10 +213,8 @@ static void intel_vbtn_pm_complete(struc
> {
> struct intel_vbtn_priv *priv = dev_get_drvdata(dev);
>
> - if (priv->wakeup_mode) {
> - acpi_ec_set_gpe_wake_mask(ACPI_GPE_DISABLE);
> + if (priv->wakeup_mode)
Ditto.
> priv->wakeup_mode = false;
> - }
> }
>
> static int intel_vbtn_pm_resume(struct device *dev)
> Index: linux-pm/drivers/acpi/ec.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/ec.c
> +++ linux-pm/drivers/acpi/ec.c
> @@ -1970,7 +1970,6 @@ void acpi_ec_set_gpe_wake_mask(u8 action
> if (pm_suspend_no_platform() && first_ec && !ec_no_wakeup)
> acpi_set_gpe_wake_mask(NULL, first_ec->gpe, action);
> }
> -EXPORT_SYMBOL_GPL(acpi_ec_set_gpe_wake_mask);
>
> bool acpi_ec_dispatch_gpe(void)
> {
--
With Best Regards,
Andy Shevchenko
