[PATCH 0/3] pinctrl-amd powerbtn handling regression

[PATCH 0/3] pinctrl-amd powerbtn handling regression

[Mario Limonciello]

AMD GPIO controllers have a bit in one of the registers that sets debounce
to meet Windows certification requirements.

As many platforms ship Windows they enable this by default.
Linux was not using it until a bug fix commit a855724dc08b ("pinctrl: amd:
Fix mistake in handling clearing pins at startup") aligned the register
with the intended values.

On systems that program GPIOs through _AEI and handle interrupts in the
GPIO controller driver this makes them behave like Windows.

On systems that don't program GPIOs in _AEI, the interrupts aren't handled
by the GPIO controller driver this causes the GPIO to get "stuck".

It's stuck because according to the spec the interrupt is supposed to be
cleared by driver when the button is pressed less than 2s.

However as the GPIO doesn't trigger an interrupt, it can't be cleared
until another GPIO that is handled by the driver (such as touchpad).

This series adds handling behavior from the spec, but as it's not ideal
on the reported platform from the lack of interrupt to handle set a quirk
to revert the debounce behavior back to old behavior.

A module parameter is also added to let anyone else affected by this debug
it.

Mario Limonciello (3):
  pinctrl: amd: Clear `Less2secSts` and `Less10secSts` for GPIO0
  pinctrl: amd: Add a module parameter to configure power button
    behavior
  pinctrl: amd: Add a quirk for Lenovo Ideapad 5

 drivers/pinctrl/pinctrl-amd.c | 70 +++++++++++++++++++++++++++++++++++
 drivers/pinctrl/pinctrl-amd.h |  2 +
 2 files changed, 72 insertions(+)


base-commit: b4e880a8d840e2b64937ab47ad518185c07747e3
-- 
2.34.1

[PATCH 1/3] pinctrl: amd: Clear `Less2secSts` and `Less10secSts` for GPIO0

[Mario Limonciello]

On systems that don't program GPIO0 through _AEI may still control
the power button via this GPIO.  When the GPIO master controller
register has bit 15 configured for `EnWinBlueBtn` this will cause
GPIO 0 to behave differently. If the user presses the button for
less than 2 seconds or less than 10 seconds then it is expected
that interrupt status must be cleared by the GPIO driver.

Reported-by: Luca Pigliacampo <lucapgl2001@gmail.com>
Closes: https://bugzilla.kernel.org/show_bug.cgi?id=217833
Fixes: a855724dc08b ("pinctrl: amd: Fix mistake in handling clearing pins at startup")
Link: https://www.amd.com/content/dam/amd/en/documents/archived-tech-docs/programmer-references/55072_AMD_Family_15h_Models_70h-7Fh_BKDG.pdf p948
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/pinctrl/pinctrl-amd.c | 9 +++++++++
 drivers/pinctrl/pinctrl-amd.h | 2 ++
 2 files changed, 11 insertions(+)

diff --git a/drivers/pinctrl/pinctrl-amd.c b/drivers/pinctrl/pinctrl-amd.c
index 74241b2ff21e..37d64fa55b66 100644
--- a/drivers/pinctrl/pinctrl-amd.c
+++ b/drivers/pinctrl/pinctrl-amd.c
@@ -590,6 +590,7 @@ static const struct irq_chip amd_gpio_irqchip = {
 	GPIOCHIP_IRQ_RESOURCE_HELPERS,
 };
 
+#define GPIO0_LESS_TIME	(BIT(GPIO0_LESS_2_OFF) | BIT(GPIO0_LESS_10_OFF))
 #define PIN_IRQ_PENDING	(BIT(INTERRUPT_STS_OFF) | BIT(WAKE_STS_OFF))
 
 static bool do_amd_gpio_irq_handler(int irq, void *dev_id)
@@ -610,6 +611,14 @@ static bool do_amd_gpio_irq_handler(int irq, void *dev_id)
 	status |= readl(gpio_dev->base + WAKE_INT_STATUS_REG0);
 	raw_spin_unlock_irqrestore(&gpio_dev->lock, flags);
 
+	/* check GPIO0 specifically for Less2 and Less10 */
+	regval = readl(gpio_dev->base);
+	if (regval & GPIO0_LESS_TIME) {
+		pm_pr_dbg("GPIO0 was pressed for less than %d seconds\n",
+			  regval & BIT(GPIO0_LESS_10_OFF) ? 10 : 2);
+		writel(regval | BIT(INTERRUPT_STS_OFF), gpio_dev->base);
+	}
+
 	/* Bit 0-45 contain the relevant status bits */
 	status &= (1ULL << 46) - 1;
 	regs = gpio_dev->base;
diff --git a/drivers/pinctrl/pinctrl-amd.h b/drivers/pinctrl/pinctrl-amd.h
index 34c5c3e71fb2..3ea9a5275845 100644
--- a/drivers/pinctrl/pinctrl-amd.h
+++ b/drivers/pinctrl/pinctrl-amd.h
@@ -45,6 +45,8 @@
 #define WAKECNTRL_Z_OFF			27
 #define INTERRUPT_STS_OFF		28
 #define WAKE_STS_OFF			29
+#define GPIO0_LESS_2_OFF		30
+#define GPIO0_LESS_10_OFF		31
 
 #define DB_TMR_OUT_MASK	0xFUL
 #define DB_CNTRl_MASK	0x3UL
-- 
2.34.1

[PATCH 2/3] pinctrl: amd: Add a module parameter to configure power button behavior

[Mario Limonciello]

On pinctrl-amd supported systems there is a bit in the GPIO controller
register that controls the debounce behavior. The traditional debounce
behavior is configured by software, whereas the 'WinBlue' debounce behavior
is handled internally in the GPIO controller and conforms to Microsoft
requirements.

For any users that find this behavior not desirable, add a module parameter
to force the traditional debounce behavior.

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

diff --git a/drivers/pinctrl/pinctrl-amd.c b/drivers/pinctrl/pinctrl-amd.c
index 37d64fa55b66..a2468a988be3 100644
--- a/drivers/pinctrl/pinctrl-amd.c
+++ b/drivers/pinctrl/pinctrl-amd.c
@@ -36,6 +36,11 @@
 #include "pinctrl-utils.h"
 #include "pinctrl-amd.h"
 
+static int powerbtn = -1;
+module_param(powerbtn, int, 0444);
+MODULE_PARM_DESC(powerbtn,
+		 "Power button debouncing: 0=traditional, 1=windows, -1=auto");
+
 static int amd_gpio_get_direction(struct gpio_chip *gc, unsigned offset)
 {
 	unsigned long flags;
@@ -1075,6 +1080,30 @@ static void amd_get_iomux_res(struct amd_gpio *gpio_dev)
 	desc->pmxops = NULL;
 }
 
+static void handle_powerbtn(struct amd_gpio *gpio_dev)
+{
+	u32 pin_reg;
+
+	if (powerbtn == -1)
+		return;
+
+	pin_reg = readl(gpio_dev->base + WAKE_INT_MASTER_REG);
+	switch (powerbtn) {
+	case 0:
+		pin_reg &= ~INTERNAL_GPIO0_DEBOUNCE;
+		break;
+	case 1:
+		pin_reg |= INTERNAL_GPIO0_DEBOUNCE;
+		break;
+	default:
+		dev_err(&gpio_dev->pdev->dev, "Invalid module parameter %d\n",
+			powerbtn);
+		return;
+	}
+
+	writel(pin_reg, gpio_dev->base + WAKE_INT_MASTER_REG);
+}
+
 static int amd_gpio_probe(struct platform_device *pdev)
 {
 	int ret = 0;
@@ -1095,6 +1124,8 @@ static int amd_gpio_probe(struct platform_device *pdev)
 		return PTR_ERR(gpio_dev->base);
 	}
 
+	handle_powerbtn(gpio_dev);
+
 	gpio_dev->irq = platform_get_irq(pdev, 0);
 	if (gpio_dev->irq < 0)
 		return gpio_dev->irq;
-- 
2.34.1

[PATCH 3/3] pinctrl: amd: Add a quirk for Lenovo Ideapad 5

[Mario Limonciello]

Lenovo ideapad 5 doesn't use interrupts for GPIO 0, and so internally
debouncing with WinBlue debounce behavior means that the GPIO doesn't
clear until a separate GPIO is used (such as touchpad).

Prefer to use legacy debouncing to avoid problems.

Reported-by: Luca Pigliacampo <lucapgl2001@gmail.com>
Closes: https://bugzilla.kernel.org/show_bug.cgi?id=217833
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/pinctrl/pinctrl-amd.c | 34 ++++++++++++++++++++++++++++++++--
 1 file changed, 32 insertions(+), 2 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-amd.c b/drivers/pinctrl/pinctrl-amd.c
index a2468a988be3..2e1721a9249a 100644
--- a/drivers/pinctrl/pinctrl-amd.c
+++ b/drivers/pinctrl/pinctrl-amd.c
@@ -8,6 +8,7 @@
  *
  */
 
+#include <linux/dmi.h>
 #include <linux/err.h>
 #include <linux/bug.h>
 #include <linux/kernel.h>
@@ -41,6 +42,27 @@ module_param(powerbtn, int, 0444);
 MODULE_PARM_DESC(powerbtn,
 		 "Power button debouncing: 0=traditional, 1=windows, -1=auto");
 
+struct pinctrl_amd_dmi_quirk {
+	int powerbtn;
+};
+
+static const struct dmi_system_id pinctrl_amd_dmi_quirks[] __initconst = {
+	{
+		/*
+		 * Lenovo Ideapad 5
+		 * Power button GPIO not cleared until touchpad movement
+		 * https://bugzilla.kernel.org/show_bug.cgi?id=217833
+		 */
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
+			DMI_MATCH(DMI_PRODUCT_NAME, "82LM"),
+		},
+		.driver_data = &(struct pinctrl_amd_dmi_quirk) {
+			.powerbtn = 0,
+		},
+	}
+};
+
 static int amd_gpio_get_direction(struct gpio_chip *gc, unsigned offset)
 {
 	unsigned long flags;
@@ -1084,8 +1106,16 @@ static void handle_powerbtn(struct amd_gpio *gpio_dev)
 {
 	u32 pin_reg;
 
-	if (powerbtn == -1)
-		return;
+	if (powerbtn == -1) {
+		const struct pinctrl_amd_dmi_quirk *quirk = NULL;
+		const struct dmi_system_id *id;
+
+		id = dmi_first_match(pinctrl_amd_dmi_quirks);
+		if (!id)
+			return;
+		quirk = id->driver_data;
+		powerbtn = quirk->powerbtn;
+	}
 
 	pin_reg = readl(gpio_dev->base + WAKE_INT_MASTER_REG);
 	switch (powerbtn) {
-- 
2.34.1

Re: [PATCH 3/3] pinctrl: amd: Add a quirk for Lenovo Ideapad 5

[Hans de Goede]

Hi Mario,

On 8/29/23 18:56, Mario Limonciello wrote:
> Lenovo ideapad 5 doesn't use interrupts for GPIO 0, and so internally
> debouncing with WinBlue debounce behavior means that the GPIO doesn't
> clear until a separate GPIO is used (such as touchpad).
> 
> Prefer to use legacy debouncing to avoid problems.
> 
> Reported-by: Luca Pigliacampo <lucapgl2001@gmail.com>
> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=217833
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>

I'm not happy to see yet another DMI quirk solution here.

and I guess you're not happy with this either...

Are we sure there is no other way? Did you check an acpidump
for the laptop and specifically for its ACPI powerbutton handling?

I would expect the ACPI powerbutton handler to somehow clear
the bit, like how patch 1/3 clears it from the GPIO chip's
own IRQ handler.

I see that drivers/acpi/button.c does:

static u32 acpi_button_event(void *data)
{
        acpi_os_execute(OSL_NOTIFY_HANDLER, acpi_button_notify_run, data);
        return ACPI_INTERRUPT_HANDLED;
}

So unless I'm misreading something here, there is some AML being
executed on power-button events. So maybe there is something wrong
with how Linux interprets that AML ?

Regards,

Hans




> ---
>  drivers/pinctrl/pinctrl-amd.c | 34 ++++++++++++++++++++++++++++++++--
>  1 file changed, 32 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pinctrl/pinctrl-amd.c b/drivers/pinctrl/pinctrl-amd.c
> index a2468a988be3..2e1721a9249a 100644
> --- a/drivers/pinctrl/pinctrl-amd.c
> +++ b/drivers/pinctrl/pinctrl-amd.c
> @@ -8,6 +8,7 @@
>   *
>   */
>  
> +#include <linux/dmi.h>
>  #include <linux/err.h>
>  #include <linux/bug.h>
>  #include <linux/kernel.h>
> @@ -41,6 +42,27 @@ module_param(powerbtn, int, 0444);
>  MODULE_PARM_DESC(powerbtn,
>  		 "Power button debouncing: 0=traditional, 1=windows, -1=auto");
>  
> +struct pinctrl_amd_dmi_quirk {
> +	int powerbtn;
> +};
> +
> +static const struct dmi_system_id pinctrl_amd_dmi_quirks[] __initconst = {
> +	{
> +		/*
> +		 * Lenovo Ideapad 5
> +		 * Power button GPIO not cleared until touchpad movement
> +		 * https://bugzilla.kernel.org/show_bug.cgi?id=217833
> +		 */
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "82LM"),
> +		},
> +		.driver_data = &(struct pinctrl_amd_dmi_quirk) {
> +			.powerbtn = 0,
> +		},
> +	}
> +};
> +
>  static int amd_gpio_get_direction(struct gpio_chip *gc, unsigned offset)
>  {
>  	unsigned long flags;
> @@ -1084,8 +1106,16 @@ static void handle_powerbtn(struct amd_gpio *gpio_dev)
>  {
>  	u32 pin_reg;
>  
> -	if (powerbtn == -1)
> -		return;
> +	if (powerbtn == -1) {
> +		const struct pinctrl_amd_dmi_quirk *quirk = NULL;
> +		const struct dmi_system_id *id;
> +
> +		id = dmi_first_match(pinctrl_amd_dmi_quirks);
> +		if (!id)
> +			return;
> +		quirk = id->driver_data;
> +		powerbtn = quirk->powerbtn;
> +	}
>  
>  	pin_reg = readl(gpio_dev->base + WAKE_INT_MASTER_REG);
>  	switch (powerbtn) {

Re: [PATCH 3/3] pinctrl: amd: Add a quirk for Lenovo Ideapad 5

[Mario Limonciello]

On 8/29/2023 14:54, Hans de Goede wrote:
> Hi Mario,
> 
> On 8/29/23 18:56, Mario Limonciello wrote:
>> Lenovo ideapad 5 doesn't use interrupts for GPIO 0, and so internally
>> debouncing with WinBlue debounce behavior means that the GPIO doesn't
>> clear until a separate GPIO is used (such as touchpad).
>>
>> Prefer to use legacy debouncing to avoid problems.
>>
>> Reported-by: Luca Pigliacampo <lucapgl2001@gmail.com>
>> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=217833
>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> 
> I'm not happy to see yet another DMI quirk solution here.
> 
> and I guess you're not happy with this either...

Yeah I was really hoping the first patch was enough for the issue.

If we can't come up with anything else we can potentially drop patches 2 
and 3. Even patch 1 alone will "significantly" improve the situation.

The other option I considered is to hardcode WinBlue debounce behavior 
"off" in Linux.

I don't think this is a good idea though though because we will likely 
trade bugs because the debounce values in the AML for systems using _AEI 
aren't actually used in Windows and might not have good values.

> 
> Are we sure there is no other way? Did you check an acpidump
> for the laptop and specifically for its ACPI powerbutton handling?

I'm not sure there is another way or not, but yes there is an acpidump 
attached to the bug in case you or anyone else has some ideas.

> 
> I would expect the ACPI powerbutton handler to somehow clear
> the bit, like how patch 1/3 clears it from the GPIO chip's
> own IRQ handler.
> 
> I see that drivers/acpi/button.c does:
> 
> static u32 acpi_button_event(void *data)
> {
>          acpi_os_execute(OSL_NOTIFY_HANDLER, acpi_button_notify_run, data);
>          return ACPI_INTERRUPT_HANDLED;
> }
> 
> So unless I'm misreading something here, there is some AML being
> executed on power-button events. So maybe there is something wrong
> with how Linux interprets that AML ?
> 
The relevant ACPI spec section is here:

https://uefi.org/htmlspecs/ACPI_Spec_6_4_html/04_ACPI_Hardware_Specification/ACPI_Hardware_Specification.html#control-method-power-button

I did look at the acpidump.  GPE 08 notifies \_SB.PWRB (a PNP0C0C 
device) with 0x2.  According to the spec this is specifically for 
letting the system know the power button is waking up the system from G1.

I don't see any notifications with data 0x80 in the AML.  So I'm 
wondering if the AML designer made an assumption that pressing the power 
button caused the system to suspend or turn off.  That's the Windows 
behavior, isn't it?

The spec even says:

"While the system is in the working state, a power button press is a 
user request to transition the system into either the sleeping (G1) or 
soft-off state (G2)."

I think it would be a really good data point whether this happens with 
Windows if possible too.  Then we can know if we're looking at a Linux 
bug or a platform bug.

Re: [PATCH 3/3] pinctrl: amd: Add a quirk for Lenovo Ideapad 5

[Hans de Goede]

Hi,

On 8/29/23 23:37, Mario Limonciello wrote:
> On 8/29/2023 14:54, Hans de Goede wrote:
>> Hi Mario,
>>
>> On 8/29/23 18:56, Mario Limonciello wrote:
>>> Lenovo ideapad 5 doesn't use interrupts for GPIO 0, and so internally
>>> debouncing with WinBlue debounce behavior means that the GPIO doesn't
>>> clear until a separate GPIO is used (such as touchpad).
>>>
>>> Prefer to use legacy debouncing to avoid problems.
>>>
>>> Reported-by: Luca Pigliacampo <lucapgl2001@gmail.com>
>>> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=217833
>>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>>
>> I'm not happy to see yet another DMI quirk solution here.
>>
>> and I guess you're not happy with this either...
> 
> Yeah I was really hoping the first patch was enough for the issue.
> 
> If we can't come up with anything else we can potentially drop patches 2 and 3. Even patch 1 alone will "significantly" improve the situation.
> 
> The other option I considered is to hardcode WinBlue debounce behavior "off" in Linux.
> 
> I don't think this is a good idea though though because we will likely trade bugs because the debounce values in the AML for systems using _AEI aren't actually used in Windows and might not have good values.

What if we turn off the WinBlue debounce behavior for GPIO0 and then just hardcode some sane debounce values for it, overriding whatever the DSDT _AEI entries contain ?

>> Are we sure there is no other way? Did you check an acpidump
>> for the laptop and specifically for its ACPI powerbutton handling?
> 
> I'm not sure there is another way or not, but yes there is an acpidump attached to the bug in case you or anyone else has some ideas.
> 
>>
>> I would expect the ACPI powerbutton handler to somehow clear
>> the bit, like how patch 1/3 clears it from the GPIO chip's
>> own IRQ handler.
>>
>> I see that drivers/acpi/button.c does:
>>
>> static u32 acpi_button_event(void *data)
>> {
>>          acpi_os_execute(OSL_NOTIFY_HANDLER, acpi_button_notify_run, data);
>>          return ACPI_INTERRUPT_HANDLED;
>> }
>>
>> So unless I'm misreading something here, there is some AML being
>> executed on power-button events. So maybe there is something wrong
>> with how Linux interprets that AML ?
>>
> The relevant ACPI spec section is here:
> 
> https://uefi.org/htmlspecs/ACPI_Spec_6_4_html/04_ACPI_Hardware_Specification/ACPI_Hardware_Specification.html#control-method-power-button
> 
> I did look at the acpidump.  GPE 08 notifies \_SB.PWRB (a PNP0C0C device) with 0x2.  According to the spec this is specifically for letting the system know the power button is waking up the system from G1.

Sorry, the acpi_os_execute() function name gave me the impression that this would actually call some ACPI defined function, since normally in acpi speak execute refers to an ACPI table defined method.

But that is not the case here it is just a wrapper to deferred-exec the passed in function pointer.

To be clear I was hoping that there was an ACPI defined (AML code) function which would maybe clear the GPIO for us and that that was maybe not working due to e.g. some opregion not being implemented by Linux. But no AML code is being executed at all, so this is all a red herring.

Regards,

Hans

Re: [PATCH 3/3] pinctrl: amd: Add a quirk for Lenovo Ideapad 5

[Mario Limonciello]

On 8/30/2023 10:37, Hans de Goede wrote:
> Hi,
> 
> On 8/29/23 23:37, Mario Limonciello wrote:
>> On 8/29/2023 14:54, Hans de Goede wrote:
>>> Hi Mario,
>>>
>>> On 8/29/23 18:56, Mario Limonciello wrote:
>>>> Lenovo ideapad 5 doesn't use interrupts for GPIO 0, and so internally
>>>> debouncing with WinBlue debounce behavior means that the GPIO doesn't
>>>> clear until a separate GPIO is used (such as touchpad).
>>>>
>>>> Prefer to use legacy debouncing to avoid problems.
>>>>
>>>> Reported-by: Luca Pigliacampo <lucapgl2001@gmail.com>
>>>> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=217833
>>>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>>>
>>> I'm not happy to see yet another DMI quirk solution here.
>>>
>>> and I guess you're not happy with this either...
>>
>> Yeah I was really hoping the first patch was enough for the issue.
>>
>> If we can't come up with anything else we can potentially drop patches 2 and 3. Even patch 1 alone will "significantly" improve the situation.
>>
>> The other option I considered is to hardcode WinBlue debounce behavior "off" in Linux.
>>
>> I don't think this is a good idea though though because we will likely trade bugs because the debounce values in the AML for systems using _AEI aren't actually used in Windows and might not have good values.
> 
> What if we turn off the WinBlue debounce behavior for GPIO0 and then just hardcode some sane debounce values for it, overriding whatever the DSDT _AEI entries contain ?
> 

I don't think this is a good idea.

Some vendors GPIO0 doesn't connect to the power button but instead to 
the EC.  If it's connected to the EC, the EC might instead trigger GPIO0 
for lid or power button or whatever they decided for a design specific way.

I'd worry that we're going to end up with inconsistent results if they 
have their own debouncing put in place in the EC *because* they were 
relying upon the Winblue debounce behavior.

After all - this was fixed because of 
https://bugzilla.kernel.org/show_bug.cgi?id=217315

>>> Are we sure there is no other way? Did you check an acpidump
>>> for the laptop and specifically for its ACPI powerbutton handling?
>>
>> I'm not sure there is another way or not, but yes there is an acpidump attached to the bug in case you or anyone else has some ideas.
>>
>>>
>>> I would expect the ACPI powerbutton handler to somehow clear
>>> the bit, like how patch 1/3 clears it from the GPIO chip's
>>> own IRQ handler.
>>>
>>> I see that drivers/acpi/button.c does:
>>>
>>> static u32 acpi_button_event(void *data)
>>> {
>>>           acpi_os_execute(OSL_NOTIFY_HANDLER, acpi_button_notify_run, data);
>>>           return ACPI_INTERRUPT_HANDLED;
>>> }
>>>
>>> So unless I'm misreading something here, there is some AML being
>>> executed on power-button events. So maybe there is something wrong
>>> with how Linux interprets that AML ?
>>>
>> The relevant ACPI spec section is here:
>>
>> https://uefi.org/htmlspecs/ACPI_Spec_6_4_html/04_ACPI_Hardware_Specification/ACPI_Hardware_Specification.html#control-method-power-button
>>
>> I did look at the acpidump.  GPE 08 notifies \_SB.PWRB (a PNP0C0C device) with 0x2.  According to the spec this is specifically for letting the system know the power button is waking up the system from G1.
> 
> Sorry, the acpi_os_execute() function name gave me the impression that this would actually call some ACPI defined function, since normally in acpi speak execute refers to an ACPI table defined method.
> 
> But that is not the case here it is just a wrapper to deferred-exec the passed in function pointer.
> 
> To be clear I was hoping that there was an ACPI defined (AML code) function which would maybe clear the GPIO for us and that that was maybe not working due to e.g. some opregion not being implemented by Linux. But no AML code is being executed at all, so this is all a red herring.
> 
> Regards,
> 
> Hans
> 
> 

Something we could do is add an extra callback for ACPI button driver to 
call the GPIO controller IRQ handler.  Worst case the IRQ handler does 
nothing, best case it fixes this issue.
I'm not sure how we'd tie it to something spec compliant.

Re: [PATCH 3/3] pinctrl: amd: Add a quirk for Lenovo Ideapad 5

[Hans de Goede]

Hi,

On 8/30/23 17:47, Mario Limonciello wrote:
> On 8/30/2023 10:37, Hans de Goede wrote:
>> Hi,
>>
>> On 8/29/23 23:37, Mario Limonciello wrote:
>>> On 8/29/2023 14:54, Hans de Goede wrote:
>>>> Hi Mario,
>>>>
>>>> On 8/29/23 18:56, Mario Limonciello wrote:
>>>>> Lenovo ideapad 5 doesn't use interrupts for GPIO 0, and so internally
>>>>> debouncing with WinBlue debounce behavior means that the GPIO doesn't
>>>>> clear until a separate GPIO is used (such as touchpad).
>>>>>
>>>>> Prefer to use legacy debouncing to avoid problems.
>>>>>
>>>>> Reported-by: Luca Pigliacampo <lucapgl2001@gmail.com>
>>>>> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=217833
>>>>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>>>>
>>>> I'm not happy to see yet another DMI quirk solution here.
>>>>
>>>> and I guess you're not happy with this either...
>>>
>>> Yeah I was really hoping the first patch was enough for the issue.
>>>
>>> If we can't come up with anything else we can potentially drop patches 2 and 3. Even patch 1 alone will "significantly" improve the situation.
>>>
>>> The other option I considered is to hardcode WinBlue debounce behavior "off" in Linux.
>>>
>>> I don't think this is a good idea though though because we will likely trade bugs because the debounce values in the AML for systems using _AEI aren't actually used in Windows and might not have good values.
>>
>> What if we turn off the WinBlue debounce behavior for GPIO0 and then just hardcode some sane debounce values for it, overriding whatever the DSDT _AEI entries contain ?
>>
> 
> I don't think this is a good idea.
> 
> Some vendors GPIO0 doesn't connect to the power button but instead to the EC.  If it's connected to the EC, the EC might instead trigger GPIO0 for lid or power button or whatever they decided for a design specific way.
> 
> I'd worry that we're going to end up with inconsistent results if they have their own debouncing put in place in the EC *because* they were relying upon the Winblue debounce behavior.
> 
> After all - this was fixed because of https://bugzilla.kernel.org/show_bug.cgi?id=217315

Ok, that is fair.


>>>> Are we sure there is no other way? Did you check an acpidump
>>>> for the laptop and specifically for its ACPI powerbutton handling?
>>>
>>> I'm not sure there is another way or not, but yes there is an acpidump attached to the bug in case you or anyone else has some ideas.
>>>
>>>>
>>>> I would expect the ACPI powerbutton handler to somehow clear
>>>> the bit, like how patch 1/3 clears it from the GPIO chip's
>>>> own IRQ handler.
>>>>
>>>> I see that drivers/acpi/button.c does:
>>>>
>>>> static u32 acpi_button_event(void *data)
>>>> {
>>>>           acpi_os_execute(OSL_NOTIFY_HANDLER, acpi_button_notify_run, data);
>>>>           return ACPI_INTERRUPT_HANDLED;
>>>> }
>>>>
>>>> So unless I'm misreading something here, there is some AML being
>>>> executed on power-button events. So maybe there is something wrong
>>>> with how Linux interprets that AML ?
>>>>
>>> The relevant ACPI spec section is here:
>>>
>>> https://uefi.org/htmlspecs/ACPI_Spec_6_4_html/04_ACPI_Hardware_Specification/ACPI_Hardware_Specification.html#control-method-power-button
>>>
>>> I did look at the acpidump.  GPE 08 notifies \_SB.PWRB (a PNP0C0C device) with 0x2.  According to the spec this is specifically for letting the system know the power button is waking up the system from G1.
>>
>> Sorry, the acpi_os_execute() function name gave me the impression that this would actually call some ACPI defined function, since normally in acpi speak execute refers to an ACPI table defined method.
>>
>> But that is not the case here it is just a wrapper to deferred-exec the passed in function pointer.
>>
>> To be clear I was hoping that there was an ACPI defined (AML code) function which would maybe clear the GPIO for us and that that was maybe not working due to e.g. some opregion not being implemented by Linux. But no AML code is being executed at all, so this is all a red herring.
>>
>> Regards,
>>
>> Hans
>>
>>
> 
> Something we could do is add an extra callback for ACPI button driver to call the GPIO controller IRQ handler.  Worst case the IRQ handler does nothing, best case it fixes this issue.
> I'm not sure how we'd tie it to something spec compliant.

I was actually thinking the same thing. I think we should discuss going this route
(ACPI button driver to call the GPIO controller IRQ handler) with Rafael.

We can use the existing acpi_notifier_call_chain() mechanism and:

1. Have the button code call acpi_notifier_call_chain() on the PNP0C0C
event on button presses.

2. Have the AMD pinctrl driver register a notifier_block with
register_acpi_notifier() and then check if the source is a PNP0C0C
device based on the device_class of the acpi_bus_event struct
passed to the notifier and if it is check if GPIO0 needs
clearing.

I think this is preferable over the DMI quirk route, because this should
fix the same issue also on other affected models which we don't know
about.

Regards,

Hans

Re: [PATCH 3/3] pinctrl: amd: Add a quirk for Lenovo Ideapad 5

[Mario Limonciello]

On 8/30/2023 11:18, Hans de Goede wrote:
> Hi,
> 
> On 8/30/23 17:47, Mario Limonciello wrote:
>> On 8/30/2023 10:37, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 8/29/23 23:37, Mario Limonciello wrote:
>>>> On 8/29/2023 14:54, Hans de Goede wrote:
>>>>> Hi Mario,
>>>>>
>>>>> On 8/29/23 18:56, Mario Limonciello wrote:
>>>>>> Lenovo ideapad 5 doesn't use interrupts for GPIO 0, and so internally
>>>>>> debouncing with WinBlue debounce behavior means that the GPIO doesn't
>>>>>> clear until a separate GPIO is used (such as touchpad).
>>>>>>
>>>>>> Prefer to use legacy debouncing to avoid problems.
>>>>>>
>>>>>> Reported-by: Luca Pigliacampo <lucapgl2001@gmail.com>
>>>>>> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=217833
>>>>>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>>>>>
>>>>> I'm not happy to see yet another DMI quirk solution here.
>>>>>
>>>>> and I guess you're not happy with this either...
>>>>
>>>> Yeah I was really hoping the first patch was enough for the issue.
>>>>
>>>> If we can't come up with anything else we can potentially drop patches 2 and 3. Even patch 1 alone will "significantly" improve the situation.
>>>>
>>>> The other option I considered is to hardcode WinBlue debounce behavior "off" in Linux.
>>>>
>>>> I don't think this is a good idea though though because we will likely trade bugs because the debounce values in the AML for systems using _AEI aren't actually used in Windows and might not have good values.
>>>
>>> What if we turn off the WinBlue debounce behavior for GPIO0 and then just hardcode some sane debounce values for it, overriding whatever the DSDT _AEI entries contain ?
>>>
>>
>> I don't think this is a good idea.
>>
>> Some vendors GPIO0 doesn't connect to the power button but instead to the EC.  If it's connected to the EC, the EC might instead trigger GPIO0 for lid or power button or whatever they decided for a design specific way.
>>
>> I'd worry that we're going to end up with inconsistent results if they have their own debouncing put in place in the EC *because* they were relying upon the Winblue debounce behavior.
>>
>> After all - this was fixed because of https://bugzilla.kernel.org/show_bug.cgi?id=217315
> 
> Ok, that is fair.
> 
> 
>>>>> Are we sure there is no other way? Did you check an acpidump
>>>>> for the laptop and specifically for its ACPI powerbutton handling?
>>>>
>>>> I'm not sure there is another way or not, but yes there is an acpidump attached to the bug in case you or anyone else has some ideas.
>>>>
>>>>>
>>>>> I would expect the ACPI powerbutton handler to somehow clear
>>>>> the bit, like how patch 1/3 clears it from the GPIO chip's
>>>>> own IRQ handler.
>>>>>
>>>>> I see that drivers/acpi/button.c does:
>>>>>
>>>>> static u32 acpi_button_event(void *data)
>>>>> {
>>>>>            acpi_os_execute(OSL_NOTIFY_HANDLER, acpi_button_notify_run, data);
>>>>>            return ACPI_INTERRUPT_HANDLED;
>>>>> }
>>>>>
>>>>> So unless I'm misreading something here, there is some AML being
>>>>> executed on power-button events. So maybe there is something wrong
>>>>> with how Linux interprets that AML ?
>>>>>
>>>> The relevant ACPI spec section is here:
>>>>
>>>> https://uefi.org/htmlspecs/ACPI_Spec_6_4_html/04_ACPI_Hardware_Specification/ACPI_Hardware_Specification.html#control-method-power-button
>>>>
>>>> I did look at the acpidump.  GPE 08 notifies \_SB.PWRB (a PNP0C0C device) with 0x2.  According to the spec this is specifically for letting the system know the power button is waking up the system from G1.
>>>
>>> Sorry, the acpi_os_execute() function name gave me the impression that this would actually call some ACPI defined function, since normally in acpi speak execute refers to an ACPI table defined method.
>>>
>>> But that is not the case here it is just a wrapper to deferred-exec the passed in function pointer.
>>>
>>> To be clear I was hoping that there was an ACPI defined (AML code) function which would maybe clear the GPIO for us and that that was maybe not working due to e.g. some opregion not being implemented by Linux. But no AML code is being executed at all, so this is all a red herring.
>>>
>>> Regards,
>>>
>>> Hans
>>>
>>>
>>
>> Something we could do is add an extra callback for ACPI button driver to call the GPIO controller IRQ handler.  Worst case the IRQ handler does nothing, best case it fixes this issue.
>> I'm not sure how we'd tie it to something spec compliant.
> 
> I was actually thinking the same thing. I think we should discuss going this route
> (ACPI button driver to call the GPIO controller IRQ handler) with Rafael.
> 
> We can use the existing acpi_notifier_call_chain() mechanism and:
> 
> 1. Have the button code call acpi_notifier_call_chain() on the PNP0C0C
> event on button presses.
> 
> 2. Have the AMD pinctrl driver register a notifier_block with
> register_acpi_notifier() and then check if the source is a PNP0C0C
> device based on the device_class of the acpi_bus_event struct
> passed to the notifier and if it is check if GPIO0 needs
> clearing.
> 
> I think this is preferable over the DMI quirk route, because this should
> fix the same issue also on other affected models which we don't know
> about.
> 

Linus - please disregard version 1.

I provided Luca a new series that implements this approach that Hans and 
I discussed and they confirmed it works.

I have some minor modifications to it to narrow where it's applied so we 
don't have needless notifications and will send it for review after the 
new modifications are tested as well.

Re: [PATCH 3/3] pinctrl: amd: Add a quirk for Lenovo Ideapad 5

[Linus Walleij]

On Thu, Aug 31, 2023 at 7:53 PM Mario Limonciello
<mario.limonciello@amd.com> wrote:

> Linus - please disregard version 1.

OK!

> I provided Luca a new series that implements this approach that Hans and
> I discussed and they confirmed it works.
>
> I have some minor modifications to it to narrow where it's applied so we
> don't have needless notifications and will send it for review after the
> new modifications are tested as well.

OK standing by, I'll wait for Hans' ACK and then merge it for fixes.

Yours,
Linus Walleij

Re: [PATCH 3/3] pinctrl: amd: Add a quirk for Lenovo Ideapad 5

[Hans de Goede]

Hi,

On 9/12/23 09:08, Linus Walleij wrote:
> On Thu, Aug 31, 2023 at 7:53 PM Mario Limonciello
> <mario.limonciello@amd.com> wrote:
> 
>> Linus - please disregard version 1.
> 
> OK!
> 
>> I provided Luca a new series that implements this approach that Hans and
>> I discussed and they confirmed it works.
>>
>> I have some minor modifications to it to narrow where it's applied so we
>> don't have needless notifications and will send it for review after the
>> new modifications are tested as well.
> 
> OK standing by, I'll wait for Hans' ACK and then merge it for fixes.

AFAICT Mario has not posted a new version (yet),
so there is nothing for me to ack (yet).

Regards,

Hans

Re: [PATCH 3/3] pinctrl: amd: Add a quirk for Lenovo Ideapad 5

[Mario Limonciello]

On 9/12/2023 03:58, Hans de Goede wrote:
> Hi,
> 
> On 9/12/23 09:08, Linus Walleij wrote:
>> On Thu, Aug 31, 2023 at 7:53 PM Mario Limonciello
>> <mario.limonciello@amd.com> wrote:
>>
>>> Linus - please disregard version 1.
>>
>> OK!
>>
>>> I provided Luca a new series that implements this approach that Hans and
>>> I discussed and they confirmed it works.
>>>
>>> I have some minor modifications to it to narrow where it's applied so we
>>> don't have needless notifications and will send it for review after the
>>> new modifications are tested as well.
>>
>> OK standing by, I'll wait for Hans' ACK and then merge it for fixes.
> 
> AFAICT Mario has not posted a new version (yet),
> so there is nothing for me to ack (yet).
> 
> Regards,
> 
> Hans
> 
> 

Yeah it looked like we were about to have a new version to post last 
week but there are some inconsistent results that we need to understand 
still, especially with comparing to Windows.

Re: [PATCH 3/3] pinctrl: amd: Add a quirk for Lenovo Ideapad 5

[Mario Limonciello]

On 9/12/2023 13:21, Mario Limonciello wrote:
> On 9/12/2023 03:58, Hans de Goede wrote:
>> Hi,
>>
>> On 9/12/23 09:08, Linus Walleij wrote:
>>> On Thu, Aug 31, 2023 at 7:53 PM Mario Limonciello
>>> <mario.limonciello@amd.com> wrote:
>>>
>>>> Linus - please disregard version 1.
>>>
>>> OK!
>>>
>>>> I provided Luca a new series that implements this approach that Hans 
>>>> and
>>>> I discussed and they confirmed it works.
>>>>
>>>> I have some minor modifications to it to narrow where it's applied 
>>>> so we
>>>> don't have needless notifications and will send it for review after the
>>>> new modifications are tested as well.
>>>
>>> OK standing by, I'll wait for Hans' ACK and then merge it for fixes.
>>
>> AFAICT Mario has not posted a new version (yet),
>> so there is nothing for me to ack (yet).
>>
>> Regards,
>>
>> Hans
>>
>>
> 
> Yeah it looked like we were about to have a new version to post last 
> week but there are some inconsistent results that we need to understand 
> still, especially with comparing to Windows.

I've got two pieces of news to share on this issue.

1. In further testing Luca confirmed that the issue was behaving 
identically in Windows.  We were bug compliant.

2. In better news updating the BIOS fixed the issue in both Linux and 
Windows, no kernel patches needed.

So no further work will be done on this series.

Re: [PATCH 3/3] pinctrl: amd: Add a quirk for Lenovo Ideapad 5

[Linus Walleij]

On Wed, Sep 13, 2023 at 11:21 PM Mario Limonciello
<mario.limonciello@amd.com> wrote:

> 2. In better news updating the BIOS fixed the issue in both Linux and
> Windows, no kernel patches needed.
>
> So no further work will be done on this series.

Is it easy for users to update BIOS? I.e. does
fwupdmgr update work?

Or does it require flashing special USB drives with FAT filesystems...?

Because I'm not sure all users will do that. Or even be aware that
they should. In that case detecting the situation and emitting
a dev_err() telling the user to update their BIOS would be
desirable I think?

Yours,
Linus Walleij

Re: [PATCH 3/3] pinctrl: amd: Add a quirk for Lenovo Ideapad 5

[Luca Pigliacampo]

On 9/14/23 10:43, Linus Walleij wrote:

> On Wed, Sep 13, 2023 at 11:21 PM Mario Limonciello
> <mario.limonciello@amd.com> wrote:
>
>> 2. In better news updating the BIOS fixed the issue in both Linux and
>> Windows, no kernel patches needed.
>>
>> So no further work will be done on this series.
> Is it easy for users to update BIOS? I.e. does
> fwupdmgr update work?
>
> Or does it require flashing special USB drives with FAT filesystems...?
>
> Because I'm not sure all users will do that. Or even be aware that
> they should. In that case detecting the situation and emitting
> a dev_err() telling the user to update their BIOS would be
> desirable I think?
>
> Yours,
> Linus Walleij

sadly it's not convenient,

the only way lenovo offers to update the bios

is an executable to run on windows.


So a user should either have a dual boot

or install windows on an external drive and boot from that,

also the update process might wipe every boot entry beside windows.


I read that some bios updaters also run on freedos, but i didn't try

Re: [PATCH 3/3] pinctrl: amd: Add a quirk for Lenovo Ideapad 5

[Mario Limonciello]

On 9/14/2023 04:08, Luca Pigliacampo wrote:
> On 9/14/23 10:43, Linus Walleij wrote:
> 
>> On Wed, Sep 13, 2023 at 11:21 PM Mario Limonciello
>> <mario.limonciello@amd.com> wrote:
>>
>>> 2. In better news updating the BIOS fixed the issue in both Linux and
>>> Windows, no kernel patches needed.
>>>
>>> So no further work will be done on this series.
>> Is it easy for users to update BIOS? I.e. does
>> fwupdmgr update work?
>>
>> Or does it require flashing special USB drives with FAT filesystems...?
>>
>> Because I'm not sure all users will do that. Or even be aware that
>> they should. In that case detecting the situation and emitting
>> a dev_err() telling the user to update their BIOS would be
>> desirable I think?
>>

I'm not sure how to detect it without giving false positives to users 
with no problems.

>> Yours,
>> Linus Walleij
> 
> sadly it's not convenient,
> 
> the only way lenovo offers to update the bios
> 
> is an executable to run on windows.
> 
> 
> So a user should either have a dual boot
> 
> or install windows on an external drive and boot from that,
> 
> also the update process might wipe every boot entry beside windows.
> 
> 
> I read that some bios updaters also run on freedos, but i didn't try
> 

On some systems Lenovo offers native updates for Linux, but I guess not 
this one.