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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 29 Aug 2023 21:54:25 +0200
From:   Hans de Goede <hdegoede@...hat.com>
To:     Mario Limonciello <mario.limonciello@....com>,
        linus.walleij@...aro.org
Cc:     Shyam-sundar.S-k@....com, Basavaraj.Natikar@....com,
        linux-gpio@...r.kernel.org, linux-kernel@...r.kernel.org,
        regressions@...ts.linux.dev, lucapgl2001@...il.com
Subject: Re: [PATCH 3/3] pinctrl: amd: Add a quirk for Lenovo Ideapad 5

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@...il.com>
> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=217833
> Signed-off-by: Mario Limonciello <mario.limonciello@....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) {