[PATCH] ACPI: PM: Revert "Only mark EC GPE for wakeup on Intel systems"

[PATCH] ACPI: PM: Revert "Only mark EC GPE for wakeup on Intel systems"

[Mario Limonciello]

Testing on various upcoming OEM systems shows commit 7b167c4cb48e ("ACPI:
PM: Only mark EC GPE for wakeup on Intel systems") was short
sighted and the symptoms were indicative of other problems. Some OEMs
do have the dedicated GPIOs for the power button but also rely upon
an interrupt to the EC SCI to let the lid work.

The original commit showed spurious activity on Lenovo systems:
     * On both Lenovo T14 and P14s the keyboard wakeup doesn't work, and
       sometimes the power button event doesn't work.

This was confirmed on my end at that time.

However further development in the kernel showed that the issue was
actually the IRQ for the GPIO controller was also shared with the EC SCI.
This was actually fixed by commit 2d54067fcd23 ("pinctrl: amd: Fix
wakeups when IRQ is shared with SCI").

The original commit also showed problems with AC adapter:
     * On HP 635 G7 detaching or attaching AC during suspend will cause
       the system not to wakeup
     * On Asus vivobook to prevent detaching AC causing resume problems
     * On Lenovo 14ARE05 to prevent detaching AC causing resume problems
     * On HP ENVY x360  to prevent detaching AC causing resume problems

Detaching AC adapter causing problems appears to have been a problem
because the EC SCI went off to notify the OS of the power adapter change
but the SCI was ignored and there was no other way to wake up this system
since GPIO controller wasn't properly enabled.  The wakeups were fixed by
enabling the GPIO controller in commit acd47b9f28e5 ("pinctrl: amd: Handle
wake-up interrupt").

I've confirmed on a variety of OEM notebooks with the following test
1) echo 1 | sudo tee /sys/power/pm_debug_messages
2) sudo systemctl suspend
3) unplug AC adapter, make sure system is still asleep
4) wake system from lid (which is provided by ACPI SCI on some of them)
5) dmesg
   a) see the EC GPE dispatched, timekeeping for X seconds (matching ~time
      until AC adapter plug out)
   b) see timekeeping for Y seconds until woke (matching ~time from AC
      adapter until lid event)
6) Look at /sys/kernel/debug/amd_pmc/s0ix_stats
   "Time (in us) in S0i3" = X + Y - firmware processing time

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/acpi/x86/s2idle.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/acpi/x86/s2idle.c b/drivers/acpi/x86/s2idle.c
index a2f16d4ecbae..665a89e2c940 100644
--- a/drivers/acpi/x86/s2idle.c
+++ b/drivers/acpi/x86/s2idle.c
@@ -427,15 +427,11 @@ static int lps0_device_attach(struct acpi_device *adev,
 		mem_sleep_current = PM_SUSPEND_TO_IDLE;
 
 	/*
-	 * Some Intel based LPS0 systems, like ASUS Zenbook UX430UNR/i7-8550U don't
-	 * use intel-hid or intel-vbtn but require the EC GPE to be enabled while
-	 * suspended for certain wakeup devices to work, so mark it as wakeup-capable.
-	 *
-	 * Only enable on !AMD as enabling this universally causes problems for a number
-	 * of AMD based systems.
+	 * 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.
 	 */
-	if (!acpi_s2idle_vendor_amd())
-		acpi_ec_mark_gpe_for_wake();
+	acpi_ec_mark_gpe_for_wake();
 
 	return 0;
 }
-- 
2.25.1

Re: [PATCH] ACPI: PM: Revert "Only mark EC GPE for wakeup on Intel systems"

[Kai-Heng Feng]

On Sat, Jan 29, 2022 at 4:34 AM Mario Limonciello
<mario.limonciello@amd.com> wrote:
>
> Testing on various upcoming OEM systems shows commit 7b167c4cb48e ("ACPI:
> PM: Only mark EC GPE for wakeup on Intel systems") was short
> sighted and the symptoms were indicative of other problems. Some OEMs
> do have the dedicated GPIOs for the power button but also rely upon
> an interrupt to the EC SCI to let the lid work.
>
> The original commit showed spurious activity on Lenovo systems:
>      * On both Lenovo T14 and P14s the keyboard wakeup doesn't work, and
>        sometimes the power button event doesn't work.
>
> This was confirmed on my end at that time.
>
> However further development in the kernel showed that the issue was
> actually the IRQ for the GPIO controller was also shared with the EC SCI.
> This was actually fixed by commit 2d54067fcd23 ("pinctrl: amd: Fix
> wakeups when IRQ is shared with SCI").
>
> The original commit also showed problems with AC adapter:
>      * On HP 635 G7 detaching or attaching AC during suspend will cause
>        the system not to wakeup
>      * On Asus vivobook to prevent detaching AC causing resume problems
>      * On Lenovo 14ARE05 to prevent detaching AC causing resume problems
>      * On HP ENVY x360  to prevent detaching AC causing resume problems
>
> Detaching AC adapter causing problems appears to have been a problem
> because the EC SCI went off to notify the OS of the power adapter change
> but the SCI was ignored and there was no other way to wake up this system
> since GPIO controller wasn't properly enabled.  The wakeups were fixed by
> enabling the GPIO controller in commit acd47b9f28e5 ("pinctrl: amd: Handle
> wake-up interrupt").
>
> I've confirmed on a variety of OEM notebooks with the following test
> 1) echo 1 | sudo tee /sys/power/pm_debug_messages
> 2) sudo systemctl suspend
> 3) unplug AC adapter, make sure system is still asleep
> 4) wake system from lid (which is provided by ACPI SCI on some of them)
> 5) dmesg
>    a) see the EC GPE dispatched, timekeeping for X seconds (matching ~time
>       until AC adapter plug out)
>    b) see timekeeping for Y seconds until woke (matching ~time from AC
>       adapter until lid event)
> 6) Look at /sys/kernel/debug/amd_pmc/s0ix_stats
>    "Time (in us) in S0i3" = X + Y - firmware processing time
>
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>

Fixes LID wakeup on several AMD based HP ProBook and EliteBook.
Tested-by: Kai-Heng Feng <kai.heng.feng@canonical.com>

> ---
>  drivers/acpi/x86/s2idle.c | 12 ++++--------
>  1 file changed, 4 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/acpi/x86/s2idle.c b/drivers/acpi/x86/s2idle.c
> index a2f16d4ecbae..665a89e2c940 100644
> --- a/drivers/acpi/x86/s2idle.c
> +++ b/drivers/acpi/x86/s2idle.c
> @@ -427,15 +427,11 @@ static int lps0_device_attach(struct acpi_device *adev,
>                 mem_sleep_current = PM_SUSPEND_TO_IDLE;
>
>         /*
> -        * Some Intel based LPS0 systems, like ASUS Zenbook UX430UNR/i7-8550U don't
> -        * use intel-hid or intel-vbtn but require the EC GPE to be enabled while
> -        * suspended for certain wakeup devices to work, so mark it as wakeup-capable.
> -        *
> -        * Only enable on !AMD as enabling this universally causes problems for a number
> -        * of AMD based systems.
> +        * 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.
>          */
> -       if (!acpi_s2idle_vendor_amd())
> -               acpi_ec_mark_gpe_for_wake();
> +       acpi_ec_mark_gpe_for_wake();
>
>         return 0;
>  }
> --
> 2.25.1
>

Re: [PATCH] ACPI: PM: Revert "Only mark EC GPE for wakeup on Intel systems"

[Rafael J. Wysocki]

On Sat, Jan 29, 2022 at 8:22 AM Kai-Heng Feng
<kai.heng.feng@canonical.com> wrote:
>
> On Sat, Jan 29, 2022 at 4:34 AM Mario Limonciello
> <mario.limonciello@amd.com> wrote:
> >
> > Testing on various upcoming OEM systems shows commit 7b167c4cb48e ("ACPI:
> > PM: Only mark EC GPE for wakeup on Intel systems") was short
> > sighted and the symptoms were indicative of other problems. Some OEMs
> > do have the dedicated GPIOs for the power button but also rely upon
> > an interrupt to the EC SCI to let the lid work.
> >
> > The original commit showed spurious activity on Lenovo systems:
> >      * On both Lenovo T14 and P14s the keyboard wakeup doesn't work, and
> >        sometimes the power button event doesn't work.
> >
> > This was confirmed on my end at that time.
> >
> > However further development in the kernel showed that the issue was
> > actually the IRQ for the GPIO controller was also shared with the EC SCI.
> > This was actually fixed by commit 2d54067fcd23 ("pinctrl: amd: Fix
> > wakeups when IRQ is shared with SCI").
> >
> > The original commit also showed problems with AC adapter:
> >      * On HP 635 G7 detaching or attaching AC during suspend will cause
> >        the system not to wakeup
> >      * On Asus vivobook to prevent detaching AC causing resume problems
> >      * On Lenovo 14ARE05 to prevent detaching AC causing resume problems
> >      * On HP ENVY x360  to prevent detaching AC causing resume problems
> >
> > Detaching AC adapter causing problems appears to have been a problem
> > because the EC SCI went off to notify the OS of the power adapter change
> > but the SCI was ignored and there was no other way to wake up this system
> > since GPIO controller wasn't properly enabled.  The wakeups were fixed by
> > enabling the GPIO controller in commit acd47b9f28e5 ("pinctrl: amd: Handle
> > wake-up interrupt").
> >
> > I've confirmed on a variety of OEM notebooks with the following test
> > 1) echo 1 | sudo tee /sys/power/pm_debug_messages
> > 2) sudo systemctl suspend
> > 3) unplug AC adapter, make sure system is still asleep
> > 4) wake system from lid (which is provided by ACPI SCI on some of them)
> > 5) dmesg
> >    a) see the EC GPE dispatched, timekeeping for X seconds (matching ~time
> >       until AC adapter plug out)
> >    b) see timekeeping for Y seconds until woke (matching ~time from AC
> >       adapter until lid event)
> > 6) Look at /sys/kernel/debug/amd_pmc/s0ix_stats
> >    "Time (in us) in S0i3" = X + Y - firmware processing time
> >
> > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>
> Fixes LID wakeup on several AMD based HP ProBook and EliteBook.
> Tested-by: Kai-Heng Feng <kai.heng.feng@canonical.com>

Applied as 5.17-rc material, thanks!

> > ---
> >  drivers/acpi/x86/s2idle.c | 12 ++++--------
> >  1 file changed, 4 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/acpi/x86/s2idle.c b/drivers/acpi/x86/s2idle.c
> > index a2f16d4ecbae..665a89e2c940 100644
> > --- a/drivers/acpi/x86/s2idle.c
> > +++ b/drivers/acpi/x86/s2idle.c
> > @@ -427,15 +427,11 @@ static int lps0_device_attach(struct acpi_device *adev,
> >                 mem_sleep_current = PM_SUSPEND_TO_IDLE;
> >
> >         /*
> > -        * Some Intel based LPS0 systems, like ASUS Zenbook UX430UNR/i7-8550U don't
> > -        * use intel-hid or intel-vbtn but require the EC GPE to be enabled while
> > -        * suspended for certain wakeup devices to work, so mark it as wakeup-capable.
> > -        *
> > -        * Only enable on !AMD as enabling this universally causes problems for a number
> > -        * of AMD based systems.
> > +        * 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.
> >          */
> > -       if (!acpi_s2idle_vendor_amd())
> > -               acpi_ec_mark_gpe_for_wake();
> > +       acpi_ec_mark_gpe_for_wake();
> >
> >         return 0;
> >  }
> > --
> > 2.25.1
> >