linux-kernel - Re: [PATCH v3 4/4] pinctrl: amd: remove debounce filter setting in IRQ type setting

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]

Message-ID: <fa67aa70-2a14-35af-632b-b0e86dc4b436@redhat.com>
Date:   Mon, 9 Nov 2020 14:52:17 +0100
From:   Hans de Goede <hdegoede@...hat.com>
To:     Coiby Xu <coiby.xu@...il.com>,
        Linus Walleij <linus.walleij@...aro.org>
Cc:     Andy Shevchenko <andy.shevchenko@...il.com>,
        linux-gpio@...r.kernel.org,
        Benjamin Tissoires <benjamin.tissoires@...hat.com>,
        stable@...r.kernel.org, open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 4/4] pinctrl: amd: remove debounce filter setting in
 IRQ type setting

Hi,

On 11/6/20 12:19 AM, Coiby Xu wrote:
> Debounce filter setting should be independent from IRQ type setting
> because according to the ACPI specs, there are separate arguments for
> specifying debounce timeout and IRQ type in GpioIo() and GpioInt().
> 
> This will fix broken touchpads for laptops whose BIOS set the debounce
> timeout to a relatively large value. For example, the BIOS of Lenovo
> Legion-5 AMD gaming laptops including 15ARH05 (R7000) and R7000P set
> the debounce timeout to 124.8ms. This led to the kernel receiving only
> ~7 HID reports per second from the Synaptics touchpad
> (MSFT0001:00 06CB:7F28). Existing touchpads like [1][2] are not troubled
> by this bug because the debounce timeout has been set to 0 by the BIOS
> before enabling the debounce filter in setting IRQ type.
> 
> [1] https://github.com/Syniurge/i2c-amd-mp2/issues/11#issuecomment-721331582
> [2] https://forum.manjaro.org/t/random-short-touchpad-freezes/30832/28
> 
> Cc: Hans de Goede <hdegoede@...hat.com>
> Cc: Andy Shevchenko <andy.shevchenko@...il.com>
> Cc: Benjamin Tissoires <benjamin.tissoires@...hat.com>
> Cc: stable@...r.kernel.org
> Reviewed-by: Andy Shevchenko <andy.shevchenko@...il.com>
> BugLink: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1887190
> Link: https://lore.kernel.org/linux-gpio/CAHp75VcwiGREBUJ0A06EEw-SyabqYsp%2Bdqs2DpSrhaY-2GVdAA%40mail.gmail.com/
> Signed-off-by: Coiby Xu <coiby.xu@...il.com>

I'm not entirely sure about this patch. This is consistent with how we
already stopped touching the debounce timeout setting during init, so
that speaks in favor of this change.

Still I'm worried a bit that this might have undesirable side effects.

I guess this should be landed together with Andy's series to apply
the debounce setting from the ACPI GPIO resources.

Regards,

Hans




> ---
>  drivers/pinctrl/pinctrl-amd.c | 7 -------
>  1 file changed, 7 deletions(-)
> 
> diff --git a/drivers/pinctrl/pinctrl-amd.c b/drivers/pinctrl/pinctrl-amd.c
> index e9b761c2b77a..2d4acf21117c 100644
> --- a/drivers/pinctrl/pinctrl-amd.c
> +++ b/drivers/pinctrl/pinctrl-amd.c
> @@ -468,7 +468,6 @@ static int amd_gpio_irq_set_type(struct irq_data *d, unsigned int type)
>  		pin_reg &= ~BIT(LEVEL_TRIG_OFF);
>  		pin_reg &= ~(ACTIVE_LEVEL_MASK << ACTIVE_LEVEL_OFF);
>  		pin_reg |= ACTIVE_HIGH << ACTIVE_LEVEL_OFF;
> -		pin_reg |= DB_TYPE_REMOVE_GLITCH << DB_CNTRL_OFF;
>  		irq_set_handler_locked(d, handle_edge_irq);
>  		break;
>  
> @@ -476,7 +475,6 @@ static int amd_gpio_irq_set_type(struct irq_data *d, unsigned int type)
>  		pin_reg &= ~BIT(LEVEL_TRIG_OFF);
>  		pin_reg &= ~(ACTIVE_LEVEL_MASK << ACTIVE_LEVEL_OFF);
>  		pin_reg |= ACTIVE_LOW << ACTIVE_LEVEL_OFF;
> -		pin_reg |= DB_TYPE_REMOVE_GLITCH << DB_CNTRL_OFF;
>  		irq_set_handler_locked(d, handle_edge_irq);
>  		break;
>  
> @@ -484,7 +482,6 @@ static int amd_gpio_irq_set_type(struct irq_data *d, unsigned int type)
>  		pin_reg &= ~BIT(LEVEL_TRIG_OFF);
>  		pin_reg &= ~(ACTIVE_LEVEL_MASK << ACTIVE_LEVEL_OFF);
>  		pin_reg |= BOTH_EADGE << ACTIVE_LEVEL_OFF;
> -		pin_reg |= DB_TYPE_REMOVE_GLITCH << DB_CNTRL_OFF;
>  		irq_set_handler_locked(d, handle_edge_irq);
>  		break;
>  
> @@ -492,8 +489,6 @@ static int amd_gpio_irq_set_type(struct irq_data *d, unsigned int type)
>  		pin_reg |= LEVEL_TRIGGER << LEVEL_TRIG_OFF;
>  		pin_reg &= ~(ACTIVE_LEVEL_MASK << ACTIVE_LEVEL_OFF);
>  		pin_reg |= ACTIVE_HIGH << ACTIVE_LEVEL_OFF;
> -		pin_reg &= ~(DB_CNTRl_MASK << DB_CNTRL_OFF);
> -		pin_reg |= DB_TYPE_PRESERVE_LOW_GLITCH << DB_CNTRL_OFF;
>  		irq_set_handler_locked(d, handle_level_irq);
>  		break;
>  
> @@ -501,8 +496,6 @@ static int amd_gpio_irq_set_type(struct irq_data *d, unsigned int type)
>  		pin_reg |= LEVEL_TRIGGER << LEVEL_TRIG_OFF;
>  		pin_reg &= ~(ACTIVE_LEVEL_MASK << ACTIVE_LEVEL_OFF);
>  		pin_reg |= ACTIVE_LOW << ACTIVE_LEVEL_OFF;
> -		pin_reg &= ~(DB_CNTRl_MASK << DB_CNTRL_OFF);
> -		pin_reg |= DB_TYPE_PRESERVE_HIGH_GLITCH << DB_CNTRL_OFF;
>  		irq_set_handler_locked(d, handle_level_irq);
>  		break;
>  
>