[PATCH 0/2] Fix some more fallout from GPIOs from _CRS

[PATCH 0/2] Fix some more fallout from GPIOs from _CRS

[Mario Limonciello]

Raul's series that let GPIOs be enabled based on ACPI tables
caused some fallout on systems that don't support s2idle.

When systems were suspended they either immediately woke up
or never (appeared) to enter suspend.

This affected at least 2 System76 systems (pang10/pang11) as
well as two Lenovo laptops (X13 G2a/T14 G2a).

Initially the solution was developed as a quirk for these
4 systems, but then it was discovered the systems are ONLY
affected when set to S3 instead of s2idle in BIOS setup.

To fix the regression, don't set wake capable for those GPIOs
unless the system claims to support low power idle in the FADT.

This effectively restores the behavior from before
commit 1796f808e4bb ("HID: i2c-hid: acpi: Stop setting wakeup_capable")
but only when utilized with S3.

Mario Limonciello (2):
  pinctrl: amd: Fix debug output for debounce time
  gpiolib-acpi: Don't set GPIOs for wakeup in S3 mode

 drivers/gpio/gpiolib-acpi.c   | 3 ++-
 drivers/pinctrl/pinctrl-amd.c | 1 +
 2 files changed, 3 insertions(+), 1 deletion(-)

-- 
2.34.1

[PATCH 1/2] pinctrl: amd: Fix debug output for debounce time

[Mario Limonciello]

If one GPIO has debounce enabled but future GPIOs in the list don't
have debounce the time never gets reset and shows wrong value.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/pinctrl/pinctrl-amd.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/pinctrl/pinctrl-amd.c b/drivers/pinctrl/pinctrl-amd.c
index 9bc6e3922e78e..32c3edaf90385 100644
--- a/drivers/pinctrl/pinctrl-amd.c
+++ b/drivers/pinctrl/pinctrl-amd.c
@@ -365,6 +365,7 @@ static void amd_gpio_dbg_show(struct seq_file *s, struct gpio_chip *gc)
 
 			} else {
 				debounce_enable = "  ∅";
+				time = 0;
 			}
 			snprintf(debounce_value, sizeof(debounce_value), "%u", time * unit);
 			seq_printf(s, "debounce %s (🕑 %sus)| ", debounce_enable, debounce_value);
-- 
2.34.1

Re: [PATCH 1/2] pinctrl: amd: Fix debug output for debounce time

[Linus Walleij]

On Sat, Jan 21, 2023 at 2:48 PM Mario Limonciello
<mario.limonciello@amd.com> wrote:

> If one GPIO has debounce enabled but future GPIOs in the list don't
> have debounce the time never gets reset and shows wrong value.
>
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>

Patch applied for fixes.

Yours,
Lijnus Walleij

[PATCH 2/2] gpiolib-acpi: Don't set GPIOs for wakeup in S3 mode

[Mario Limonciello]

commit 1796f808e4bb ("HID: i2c-hid: acpi: Stop setting wakeup_capable")
adjusted the policy to enable wakeup by default if the ACPI tables
indicated that a device was wake capable.

It was reported however that this broke suspend on at least two System76
systems in S3 mode and two Lenovo Gen2a systems, but only with S3.
When the machines are set to s2idle, wakeup behaves properly.

Configuring the GPIOs for wakeup with S3 doesn't work properly, so only
set it when the system supports low power idle.

Fixes: 1796f808e4bb ("HID: i2c-hid: acpi: Stop setting wakeup_capable")
Fixes: b38f2d5d9615c ("i2c: acpi: Use ACPI wake capability bit to set wake_irq")
Link: https://gitlab.freedesktop.org/drm/amd/-/issues/2357
Link: https://bugzilla.redhat.com/show_bug.cgi?id=2162013
Reported-by: Nathan Smythe <ncsmythe@scruboak.org>
Tested-by: Nathan Smythe <ncsmythe@scruboak.org>
Suggested-by: Raul Rangel <rrangel@chromium.org>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/gpio/gpiolib-acpi.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index 9ef0f5641b521..17c53f484280f 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -1104,7 +1104,8 @@ int acpi_dev_gpio_irq_wake_get_by(struct acpi_device *adev, const char *name, in
 				dev_dbg(&adev->dev, "IRQ %d already in use\n", irq);
 			}
 
-			if (wake_capable)
+			/* avoid suspend issues with GPIOs when systems are using S3 */
+			if (wake_capable && acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0)
 				*wake_capable = info.wake_capable;
 
 			return irq;
-- 
2.34.1

Re: [PATCH 2/2] gpiolib-acpi: Don't set GPIOs for wakeup in S3 mode

[Andy Shevchenko]

On Sat, Jan 21, 2023 at 07:48:11AM -0600, Mario Limonciello wrote:
> commit 1796f808e4bb ("HID: i2c-hid: acpi: Stop setting wakeup_capable")
> adjusted the policy to enable wakeup by default if the ACPI tables
> indicated that a device was wake capable.
> 
> It was reported however that this broke suspend on at least two System76
> systems in S3 mode and two Lenovo Gen2a systems, but only with S3.
> When the machines are set to s2idle, wakeup behaves properly.
> 
> Configuring the GPIOs for wakeup with S3 doesn't work properly, so only
> set it when the system supports low power idle.

Fine by me,
Acked-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> Fixes: 1796f808e4bb ("HID: i2c-hid: acpi: Stop setting wakeup_capable")
> Fixes: b38f2d5d9615c ("i2c: acpi: Use ACPI wake capability bit to set wake_irq")
> Link: https://gitlab.freedesktop.org/drm/amd/-/issues/2357
> Link: https://bugzilla.redhat.com/show_bug.cgi?id=2162013
> Reported-by: Nathan Smythe <ncsmythe@scruboak.org>
> Tested-by: Nathan Smythe <ncsmythe@scruboak.org>
> Suggested-by: Raul Rangel <rrangel@chromium.org>
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>  drivers/gpio/gpiolib-acpi.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
> index 9ef0f5641b521..17c53f484280f 100644
> --- a/drivers/gpio/gpiolib-acpi.c
> +++ b/drivers/gpio/gpiolib-acpi.c
> @@ -1104,7 +1104,8 @@ int acpi_dev_gpio_irq_wake_get_by(struct acpi_device *adev, const char *name, in
>  				dev_dbg(&adev->dev, "IRQ %d already in use\n", irq);
>  			}
>  
> -			if (wake_capable)
> +			/* avoid suspend issues with GPIOs when systems are using S3 */
> +			if (wake_capable && acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0)
>  				*wake_capable = info.wake_capable;
>  
>  			return irq;
> -- 
> 2.34.1
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/2] gpiolib-acpi: Don't set GPIOs for wakeup in S3 mode

[Bartosz Golaszewski]

On Sat, Jan 21, 2023 at 2:48 PM Mario Limonciello
<mario.limonciello@amd.com> wrote:
>
> commit 1796f808e4bb ("HID: i2c-hid: acpi: Stop setting wakeup_capable")
> adjusted the policy to enable wakeup by default if the ACPI tables
> indicated that a device was wake capable.
>
> It was reported however that this broke suspend on at least two System76
> systems in S3 mode and two Lenovo Gen2a systems, but only with S3.
> When the machines are set to s2idle, wakeup behaves properly.
>
> Configuring the GPIOs for wakeup with S3 doesn't work properly, so only
> set it when the system supports low power idle.
>
> Fixes: 1796f808e4bb ("HID: i2c-hid: acpi: Stop setting wakeup_capable")
> Fixes: b38f2d5d9615c ("i2c: acpi: Use ACPI wake capability bit to set wake_irq")
> Link: https://gitlab.freedesktop.org/drm/amd/-/issues/2357
> Link: https://bugzilla.redhat.com/show_bug.cgi?id=2162013
> Reported-by: Nathan Smythe <ncsmythe@scruboak.org>
> Tested-by: Nathan Smythe <ncsmythe@scruboak.org>
> Suggested-by: Raul Rangel <rrangel@chromium.org>
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>  drivers/gpio/gpiolib-acpi.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
> index 9ef0f5641b521..17c53f484280f 100644
> --- a/drivers/gpio/gpiolib-acpi.c
> +++ b/drivers/gpio/gpiolib-acpi.c
> @@ -1104,7 +1104,8 @@ int acpi_dev_gpio_irq_wake_get_by(struct acpi_device *adev, const char *name, in
>                                 dev_dbg(&adev->dev, "IRQ %d already in use\n", irq);
>                         }
>
> -                       if (wake_capable)
> +                       /* avoid suspend issues with GPIOs when systems are using S3 */
> +                       if (wake_capable && acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0)
>                                 *wake_capable = info.wake_capable;
>
>                         return irq;
> --
> 2.34.1
>

Applied, thanks!

Bart

Re: [PATCH 2/2] gpiolib-acpi: Don't set GPIOs for wakeup in S3 mode

[Raul Rangel]

On Mon, Jan 23, 2023 at 8:03 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> On Sat, Jan 21, 2023 at 2:48 PM Mario Limonciello
> <mario.limonciello@amd.com> wrote:
> >
> > commit 1796f808e4bb ("HID: i2c-hid: acpi: Stop setting wakeup_capable")
> > adjusted the policy to enable wakeup by default if the ACPI tables
> > indicated that a device was wake capable.
> >
> > It was reported however that this broke suspend on at least two System76
> > systems in S3 mode and two Lenovo Gen2a systems, but only with S3.
> > When the machines are set to s2idle, wakeup behaves properly.
> >
> > Configuring the GPIOs for wakeup with S3 doesn't work properly, so only
> > set it when the system supports low power idle.
> >
> > Fixes: 1796f808e4bb ("HID: i2c-hid: acpi: Stop setting wakeup_capable")
> > Fixes: b38f2d5d9615c ("i2c: acpi: Use ACPI wake capability bit to set wake_irq")
> > Link: https://gitlab.freedesktop.org/drm/amd/-/issues/2357
> > Link: https://bugzilla.redhat.com/show_bug.cgi?id=2162013
> > Reported-by: Nathan Smythe <ncsmythe@scruboak.org>
> > Tested-by: Nathan Smythe <ncsmythe@scruboak.org>
> > Suggested-by: Raul Rangel <rrangel@chromium.org>
> > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> > ---
> >  drivers/gpio/gpiolib-acpi.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
> > index 9ef0f5641b521..17c53f484280f 100644
> > --- a/drivers/gpio/gpiolib-acpi.c
> > +++ b/drivers/gpio/gpiolib-acpi.c
> > @@ -1104,7 +1104,8 @@ int acpi_dev_gpio_irq_wake_get_by(struct acpi_device *adev, const char *name, in
> >                                 dev_dbg(&adev->dev, "IRQ %d already in use\n", irq);
> >                         }
> >
> > -                       if (wake_capable)
> > +                       /* avoid suspend issues with GPIOs when systems are using S3 */
> > +                       if (wake_capable && acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0)
> >                                 *wake_capable = info.wake_capable;
> >
> >                         return irq;
> > --
> > 2.34.1
> >
>
> Applied, thanks!
>
> Bart


We still need to figure out a proper fix for this. If you read my post
here: https://gitlab.freedesktop.org/drm/amd/-/issues/2357#note_1732372
I think we misinterpreted what the SharedAndWake bit is used for. To
me it sounds like it's only valid for HW Reduced ACPI platforms, and
S0ix. My changes made it so we call `dev_pm_set_wake_irq` when the
Wake bit is set. Does anyone have any additional context on the Wake
bit? I think we either need to make `dev_pm_set_wake_irq` (or a
variant) only enable the wake on S0i3, or we can teach the ACPI
subsystem to manage arming the IRQ's wake bit. Kind of like we already
manage the GPE events for the device.

RE: [PATCH 2/2] gpiolib-acpi: Don't set GPIOs for wakeup in S3 mode

[Limonciello, Mario]

[Public]



> -----Original Message-----
> From: Raul Rangel <rrangel@chromium.org>
> Sent: Monday, January 23, 2023 09:55
> To: Bartosz Golaszewski <brgl@bgdev.pl>
> Cc: Limonciello, Mario <Mario.Limonciello@amd.com>; Mika Westerberg
> <mika.westerberg@linux.intel.com>; Andy Shevchenko
> <andriy.shevchenko@linux.intel.com>; Linus Walleij
> <linus.walleij@linaro.org>; Dmitry Torokhov <dmitry.torokhov@gmail.com>;
> Benjamin Tissoires <benjamin.tissoires@redhat.com>; Wolfram Sang
> <wsa@kernel.org>; Rafael J. Wysocki <rafael.j.wysocki@intel.com>; Nathan
> Smythe <ncsmythe@scruboak.org>; linux-gpio@vger.kernel.org; linux-
> acpi@vger.kernel.org; linux-kernel@vger.kernel.org; Mark Hasemeyer
> <markhas@chromium.org>
> Subject: Re: [PATCH 2/2] gpiolib-acpi: Don't set GPIOs for wakeup in S3 mode
> 
> On Mon, Jan 23, 2023 at 8:03 AM Bartosz Golaszewski <brgl@bgdev.pl>
> wrote:
> >
> > On Sat, Jan 21, 2023 at 2:48 PM Mario Limonciello
> > <mario.limonciello@amd.com> wrote:
> > >
> > > commit 1796f808e4bb ("HID: i2c-hid: acpi: Stop setting wakeup_capable")
> > > adjusted the policy to enable wakeup by default if the ACPI tables
> > > indicated that a device was wake capable.
> > >
> > > It was reported however that this broke suspend on at least two
> System76
> > > systems in S3 mode and two Lenovo Gen2a systems, but only with S3.
> > > When the machines are set to s2idle, wakeup behaves properly.
> > >
> > > Configuring the GPIOs for wakeup with S3 doesn't work properly, so only
> > > set it when the system supports low power idle.
> > >
> > > Fixes: 1796f808e4bb ("HID: i2c-hid: acpi: Stop setting wakeup_capable")
> > > Fixes: b38f2d5d9615c ("i2c: acpi: Use ACPI wake capability bit to set
> wake_irq")
> > > Link: https://gitlab.freedesktop.org/drm/amd/-/issues/2357
> > > Link: https://bugzilla.redhat.com/show_bug.cgi?id=2162013
> > > Reported-by: Nathan Smythe <ncsmythe@scruboak.org>
> > > Tested-by: Nathan Smythe <ncsmythe@scruboak.org>
> > > Suggested-by: Raul Rangel <rrangel@chromium.org>
> > > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> > > ---
> > >  drivers/gpio/gpiolib-acpi.c | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
> > > index 9ef0f5641b521..17c53f484280f 100644
> > > --- a/drivers/gpio/gpiolib-acpi.c
> > > +++ b/drivers/gpio/gpiolib-acpi.c
> > > @@ -1104,7 +1104,8 @@ int acpi_dev_gpio_irq_wake_get_by(struct
> acpi_device *adev, const char *name, in
> > >                                 dev_dbg(&adev->dev, "IRQ %d already in use\n", irq);
> > >                         }
> > >
> > > -                       if (wake_capable)
> > > +                       /* avoid suspend issues with GPIOs when systems are using
> S3 */
> > > +                       if (wake_capable && acpi_gbl_FADT.flags &
> ACPI_FADT_LOW_POWER_S0)
> > >                                 *wake_capable = info.wake_capable;
> > >
> > >                         return irq;
> > > --
> > > 2.34.1
> > >
> >
> > Applied, thanks!
> >
> > Bart
> 
> 
> We still need to figure out a proper fix for this. If you read my post
> here: https://gitlab.freedesktop.org/drm/amd/-/issues/2357#note_1732372
> I think we misinterpreted what the SharedAndWake bit is used for. To
> me it sounds like it's only valid for HW Reduced ACPI platforms, and
> S0ix. My changes made it so we call `dev_pm_set_wake_irq` when the
> Wake bit is set. Does anyone have any additional context on the Wake
> bit? I think we either need to make `dev_pm_set_wake_irq` (or a
> variant) only enable the wake on S0i3, or we can teach the ACPI
> subsystem to manage arming the IRQ's wake bit. Kind of like we already
> manage the GPE events for the device.

There is an FADT flag for HW reduced (ACPI_FADT_HW_REDUCED).  So
maybe something on top of my change to look at that too?

IE:
if (wake_capable && (acpi_gbl_FADT.flags & (ACPI_FADT_LOW_POWER_S0 | ACPI_FADT_HW_REDUCED)

Re: [PATCH 2/2] gpiolib-acpi: Don't set GPIOs for wakeup in S3 mode

[Raul Rangel]

On Mon, Jan 23, 2023 at 9:07 AM Limonciello, Mario
<Mario.Limonciello@amd.com> wrote:
>
> [Public]
>
>
>
> > -----Original Message-----
> > From: Raul Rangel <rrangel@chromium.org>
> > Sent: Monday, January 23, 2023 09:55
> > To: Bartosz Golaszewski <brgl@bgdev.pl>
> > Cc: Limonciello, Mario <Mario.Limonciello@amd.com>; Mika Westerberg
> > <mika.westerberg@linux.intel.com>; Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com>; Linus Walleij
> > <linus.walleij@linaro.org>; Dmitry Torokhov <dmitry.torokhov@gmail.com>;
> > Benjamin Tissoires <benjamin.tissoires@redhat.com>; Wolfram Sang
> > <wsa@kernel.org>; Rafael J. Wysocki <rafael.j.wysocki@intel.com>; Nathan
> > Smythe <ncsmythe@scruboak.org>; linux-gpio@vger.kernel.org; linux-
> > acpi@vger.kernel.org; linux-kernel@vger.kernel.org; Mark Hasemeyer
> > <markhas@chromium.org>
> > Subject: Re: [PATCH 2/2] gpiolib-acpi: Don't set GPIOs for wakeup in S3 mode
> >
> > On Mon, Jan 23, 2023 at 8:03 AM Bartosz Golaszewski <brgl@bgdev.pl>
> > wrote:
> > >
> > > On Sat, Jan 21, 2023 at 2:48 PM Mario Limonciello
> > > <mario.limonciello@amd.com> wrote:
> > > >
> > > > commit 1796f808e4bb ("HID: i2c-hid: acpi: Stop setting wakeup_capable")
> > > > adjusted the policy to enable wakeup by default if the ACPI tables
> > > > indicated that a device was wake capable.
> > > >
> > > > It was reported however that this broke suspend on at least two
> > System76
> > > > systems in S3 mode and two Lenovo Gen2a systems, but only with S3.
> > > > When the machines are set to s2idle, wakeup behaves properly.
> > > >
> > > > Configuring the GPIOs for wakeup with S3 doesn't work properly, so only
> > > > set it when the system supports low power idle.
> > > >
> > > > Fixes: 1796f808e4bb ("HID: i2c-hid: acpi: Stop setting wakeup_capable")
> > > > Fixes: b38f2d5d9615c ("i2c: acpi: Use ACPI wake capability bit to set
> > wake_irq")
> > > > Link: https://gitlab.freedesktop.org/drm/amd/-/issues/2357
> > > > Link: https://bugzilla.redhat.com/show_bug.cgi?id=2162013
> > > > Reported-by: Nathan Smythe <ncsmythe@scruboak.org>
> > > > Tested-by: Nathan Smythe <ncsmythe@scruboak.org>
> > > > Suggested-by: Raul Rangel <rrangel@chromium.org>
> > > > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> > > > ---
> > > >  drivers/gpio/gpiolib-acpi.c | 3 ++-
> > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
> > > > index 9ef0f5641b521..17c53f484280f 100644
> > > > --- a/drivers/gpio/gpiolib-acpi.c
> > > > +++ b/drivers/gpio/gpiolib-acpi.c
> > > > @@ -1104,7 +1104,8 @@ int acpi_dev_gpio_irq_wake_get_by(struct
> > acpi_device *adev, const char *name, in
> > > >                                 dev_dbg(&adev->dev, "IRQ %d already in use\n", irq);
> > > >                         }
> > > >
> > > > -                       if (wake_capable)
> > > > +                       /* avoid suspend issues with GPIOs when systems are using
> > S3 */
> > > > +                       if (wake_capable && acpi_gbl_FADT.flags &
> > ACPI_FADT_LOW_POWER_S0)
> > > >                                 *wake_capable = info.wake_capable;
> > > >
> > > >                         return irq;
> > > > --
> > > > 2.34.1
> > > >
> > >
> > > Applied, thanks!
> > >
> > > Bart
> >
> >
> > We still need to figure out a proper fix for this. If you read my post
> > here: https://gitlab.freedesktop.org/drm/amd/-/issues/2357#note_1732372
> > I think we misinterpreted what the SharedAndWake bit is used for. To
> > me it sounds like it's only valid for HW Reduced ACPI platforms, and
> > S0ix. My changes made it so we call `dev_pm_set_wake_irq` when the
> > Wake bit is set. Does anyone have any additional context on the Wake
> > bit? I think we either need to make `dev_pm_set_wake_irq` (or a
> > variant) only enable the wake on S0i3, or we can teach the ACPI
> > subsystem to manage arming the IRQ's wake bit. Kind of like we already
> > manage the GPE events for the device.
>
> There is an FADT flag for HW reduced (ACPI_FADT_HW_REDUCED).  So
> maybe something on top of my change to look at that too?
>
> IE:
> if (wake_capable && (acpi_gbl_FADT.flags & (ACPI_FADT_LOW_POWER_S0 | ACPI_FADT_HW_REDUCED)

The problem with the ACPI_FADT_LOW_POWER_S0 FADT flag is that it
defines if S0ix is supported. That's not mutually exclusive with S3.
So we really need a runtime check to see which suspend mode we are
entering.

Re: [PATCH 2/2] gpiolib-acpi: Don't set GPIOs for wakeup in S3 mode

[Andy Shevchenko]

On Mon, Jan 23, 2023 at 04:06:59PM +0000, Limonciello, Mario wrote:
> > From: Raul Rangel <rrangel@chromium.org>
> > Sent: Monday, January 23, 2023 09:55
> > On Mon, Jan 23, 2023 at 8:03 AM Bartosz Golaszewski <brgl@bgdev.pl>
> > wrote:
> > > On Sat, Jan 21, 2023 at 2:48 PM Mario Limonciello
> > > <mario.limonciello@amd.com> wrote:

...

> > > > +                       /* avoid suspend issues with GPIOs when systems are using
> > S3 */
> > > > +                       if (wake_capable && acpi_gbl_FADT.flags &
> > ACPI_FADT_LOW_POWER_S0)
> > > >                                 *wake_capable = info.wake_capable;
> > > >
> > > >                         return irq;

...

> > We still need to figure out a proper fix for this. If you read my post
> > here: https://gitlab.freedesktop.org/drm/amd/-/issues/2357#note_1732372
> > I think we misinterpreted what the SharedAndWake bit is used for. To
> > me it sounds like it's only valid for HW Reduced ACPI platforms, and
> > S0ix. My changes made it so we call `dev_pm_set_wake_irq` when the
> > Wake bit is set. Does anyone have any additional context on the Wake
> > bit? I think we either need to make `dev_pm_set_wake_irq` (or a
> > variant) only enable the wake on S0i3, or we can teach the ACPI
> > subsystem to manage arming the IRQ's wake bit. Kind of like we already
> > manage the GPE events for the device.
> 
> There is an FADT flag for HW reduced (ACPI_FADT_HW_REDUCED).  So
> maybe something on top of my change to look at that too?
> 
> IE:
> if (wake_capable && (acpi_gbl_FADT.flags & (ACPI_FADT_LOW_POWER_S0 | ACPI_FADT_HW_REDUCED)

I'm not sure why we are talking about HW reduced case?
In HP reduced case IIRC the GPE are absent as a class.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/2] gpiolib-acpi: Don't set GPIOs for wakeup in S3 mode

[Andy Shevchenko]

On Mon, Jan 23, 2023 at 08:55:02AM -0700, Raul Rangel wrote:
> On Mon, Jan 23, 2023 at 8:03 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> >
> > On Sat, Jan 21, 2023 at 2:48 PM Mario Limonciello
> > <mario.limonciello@amd.com> wrote:
> > >
> > > commit 1796f808e4bb ("HID: i2c-hid: acpi: Stop setting wakeup_capable")
> > > adjusted the policy to enable wakeup by default if the ACPI tables
> > > indicated that a device was wake capable.
> > >
> > > It was reported however that this broke suspend on at least two System76
> > > systems in S3 mode and two Lenovo Gen2a systems, but only with S3.
> > > When the machines are set to s2idle, wakeup behaves properly.
> > >
> > > Configuring the GPIOs for wakeup with S3 doesn't work properly, so only
> > > set it when the system supports low power idle.
> > >
> > > Fixes: 1796f808e4bb ("HID: i2c-hid: acpi: Stop setting wakeup_capable")
> > > Fixes: b38f2d5d9615c ("i2c: acpi: Use ACPI wake capability bit to set wake_irq")
> > > Link: https://gitlab.freedesktop.org/drm/amd/-/issues/2357
> > > Link: https://bugzilla.redhat.com/show_bug.cgi?id=2162013
> > > Reported-by: Nathan Smythe <ncsmythe@scruboak.org>
> > > Tested-by: Nathan Smythe <ncsmythe@scruboak.org>
> > > Suggested-by: Raul Rangel <rrangel@chromium.org>
> > > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> > > ---
> > >  drivers/gpio/gpiolib-acpi.c | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
> > > index 9ef0f5641b521..17c53f484280f 100644
> > > --- a/drivers/gpio/gpiolib-acpi.c
> > > +++ b/drivers/gpio/gpiolib-acpi.c
> > > @@ -1104,7 +1104,8 @@ int acpi_dev_gpio_irq_wake_get_by(struct acpi_device *adev, const char *name, in
> > >                                 dev_dbg(&adev->dev, "IRQ %d already in use\n", irq);
> > >                         }
> > >
> > > -                       if (wake_capable)
> > > +                       /* avoid suspend issues with GPIOs when systems are using S3 */
> > > +                       if (wake_capable && acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0)
> > >                                 *wake_capable = info.wake_capable;
> > >
> > >                         return irq;
> > > --
> > > 2.34.1
> > >
> >
> > Applied, thanks!
> >
> > Bart
> 
> 
> We still need to figure out a proper fix for this. If you read my post
> here: https://gitlab.freedesktop.org/drm/amd/-/issues/2357#note_1732372
> I think we misinterpreted what the SharedAndWake bit is used for. To
> me it sounds like it's only valid for HW Reduced ACPI platforms, and
> S0ix. My changes made it so we call `dev_pm_set_wake_irq` when the
> Wake bit is set. Does anyone have any additional context on the Wake
> bit? I think we either need to make `dev_pm_set_wake_irq` (or a
> variant) only enable the wake on S0i3, or we can teach the ACPI
> subsystem to manage arming the IRQ's wake bit. Kind of like we already
> manage the GPE events for the device.

From the spec:

Shared is an optional argument and can be one of Shared, Exclusive,
SharedAndWake or ExclusiveAndWake. If not specified, Exclusive is assumed.
The “Wake” designation indicates that the interrupt is capable of waking
the system from a low-power idle state or a system sleep state. The bit
field name _SHR is automatically created to refer to this portion of
the resource descriptor.


Note: "...a low-power idle state or a system sleep state.". I believe it
applies to both.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/2] gpiolib-acpi: Don't set GPIOs for wakeup in S3 mode

[Raul Rangel]

On Mon, Jan 23, 2023 at 10:30 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Mon, Jan 23, 2023 at 08:55:02AM -0700, Raul Rangel wrote:
> > On Mon, Jan 23, 2023 at 8:03 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > >
> > > On Sat, Jan 21, 2023 at 2:48 PM Mario Limonciello
> > > <mario.limonciello@amd.com> wrote:
> > > >
> > > > commit 1796f808e4bb ("HID: i2c-hid: acpi: Stop setting wakeup_capable")
> > > > adjusted the policy to enable wakeup by default if the ACPI tables
> > > > indicated that a device was wake capable.
> > > >
> > > > It was reported however that this broke suspend on at least two System76
> > > > systems in S3 mode and two Lenovo Gen2a systems, but only with S3.
> > > > When the machines are set to s2idle, wakeup behaves properly.
> > > >
> > > > Configuring the GPIOs for wakeup with S3 doesn't work properly, so only
> > > > set it when the system supports low power idle.
> > > >
> > > > Fixes: 1796f808e4bb ("HID: i2c-hid: acpi: Stop setting wakeup_capable")
> > > > Fixes: b38f2d5d9615c ("i2c: acpi: Use ACPI wake capability bit to set wake_irq")
> > > > Link: https://gitlab.freedesktop.org/drm/amd/-/issues/2357
> > > > Link: https://bugzilla.redhat.com/show_bug.cgi?id=2162013
> > > > Reported-by: Nathan Smythe <ncsmythe@scruboak.org>
> > > > Tested-by: Nathan Smythe <ncsmythe@scruboak.org>
> > > > Suggested-by: Raul Rangel <rrangel@chromium.org>
> > > > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> > > > ---
> > > >  drivers/gpio/gpiolib-acpi.c | 3 ++-
> > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
> > > > index 9ef0f5641b521..17c53f484280f 100644
> > > > --- a/drivers/gpio/gpiolib-acpi.c
> > > > +++ b/drivers/gpio/gpiolib-acpi.c
> > > > @@ -1104,7 +1104,8 @@ int acpi_dev_gpio_irq_wake_get_by(struct acpi_device *adev, const char *name, in
> > > >                                 dev_dbg(&adev->dev, "IRQ %d already in use\n", irq);
> > > >                         }
> > > >
> > > > -                       if (wake_capable)
> > > > +                       /* avoid suspend issues with GPIOs when systems are using S3 */
> > > > +                       if (wake_capable && acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0)
> > > >                                 *wake_capable = info.wake_capable;
> > > >
> > > >                         return irq;
> > > > --
> > > > 2.34.1
> > > >
> > >
> > > Applied, thanks!
> > >
> > > Bart
> >
> >
> > We still need to figure out a proper fix for this. If you read my post
> > here: https://gitlab.freedesktop.org/drm/amd/-/issues/2357#note_1732372
> > I think we misinterpreted what the SharedAndWake bit is used for. To
> > me it sounds like it's only valid for HW Reduced ACPI platforms, and
> > S0ix. My changes made it so we call `dev_pm_set_wake_irq` when the
> > Wake bit is set. Does anyone have any additional context on the Wake
> > bit? I think we either need to make `dev_pm_set_wake_irq` (or a
> > variant) only enable the wake on S0i3, or we can teach the ACPI
> > subsystem to manage arming the IRQ's wake bit. Kind of like we already
> > manage the GPE events for the device.
>
> From the spec:
>
> Shared is an optional argument and can be one of Shared, Exclusive,
> SharedAndWake or ExclusiveAndWake. If not specified, Exclusive is assumed.
> The “Wake” designation indicates that the interrupt is capable of waking
> the system from a low-power idle state or a system sleep state. The bit
> field name _SHR is automatically created to refer to this portion of
> the resource descriptor.
>
>
> Note: "...a low-power idle state or a system sleep state.". I believe it
> applies to both.

Without the _PRW, how do we determine the valid system sleep states
the device can wake the system from?

>
> --
> With Best Regards,
> Andy Shevchenko
>
>

Re: [PATCH 2/2] gpiolib-acpi: Don't set GPIOs for wakeup in S3 mode

[Andy Shevchenko]

On Mon, Jan 23, 2023 at 10:54:29AM -0700, Raul Rangel wrote:
> On Mon, Jan 23, 2023 at 10:30 AM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Mon, Jan 23, 2023 at 08:55:02AM -0700, Raul Rangel wrote:
> > > On Mon, Jan 23, 2023 at 8:03 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > > > On Sat, Jan 21, 2023 at 2:48 PM Mario Limonciello
> > > > <mario.limonciello@amd.com> wrote:

...

> > > We still need to figure out a proper fix for this. If you read my post
> > > here: https://gitlab.freedesktop.org/drm/amd/-/issues/2357#note_1732372
> > > I think we misinterpreted what the SharedAndWake bit is used for. To
> > > me it sounds like it's only valid for HW Reduced ACPI platforms, and
> > > S0ix. My changes made it so we call `dev_pm_set_wake_irq` when the
> > > Wake bit is set. Does anyone have any additional context on the Wake
> > > bit? I think we either need to make `dev_pm_set_wake_irq` (or a
> > > variant) only enable the wake on S0i3, or we can teach the ACPI
> > > subsystem to manage arming the IRQ's wake bit. Kind of like we already
> > > manage the GPE events for the device.
> >
> > From the spec:
> >
> > Shared is an optional argument and can be one of Shared, Exclusive,
> > SharedAndWake or ExclusiveAndWake. If not specified, Exclusive is assumed.
> > The “Wake” designation indicates that the interrupt is capable of waking
> > the system from a low-power idle state or a system sleep state. The bit
> > field name _SHR is automatically created to refer to this portion of
> > the resource descriptor.
> >
> >
> > Note: "...a low-power idle state or a system sleep state.". I believe it
> > applies to both.
> 
> Without the _PRW, how do we determine the valid system sleep states
> the device can wake the system from?

Good question. I believe you need to ask somebody from ASWG for the
clarifications.

-- 
With Best Regards,
Andy Shevchenko