linux-kernel - Re: [PATCH v2 3/3] Input: soc_button_array: Only debounce cherryview and baytrail systems

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: <57e9b1d5-faf1-4c7a-87fc-047e0dc102f9@kernel.org>
Date: Wed, 25 Jun 2025 14:23:55 -0500
From: Mario Limonciello <superm1@...nel.org>
To: Hans de Goede <hansg@...nel.org>, Mika Westerberg <westeri@...nel.org>,
 Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
 Linus Walleij <linus.walleij@...aro.org>, Bartosz Golaszewski
 <brgl@...ev.pl>, Dmitry Torokhov <dmitry.torokhov@...il.com>
Cc: "open list:GPIO ACPI SUPPORT" <linux-gpio@...r.kernel.org>,
 "open list:GPIO ACPI SUPPORT" <linux-acpi@...r.kernel.org>,
 open list <linux-kernel@...r.kernel.org>,
 "open list:INPUT (KEYBOARD, MOUSE, JOYSTICK, TOUCHSCREEN)..."
 <linux-input@...r.kernel.org>, Mario Limonciello <mario.limonciello@....com>
Subject: Re: [PATCH v2 3/3] Input: soc_button_array: Only debounce cherryview
 and baytrail systems

On 6/25/25 2:03 PM, Hans de Goede wrote:
> Hi,
> 
> On 25-Jun-25 8:13 PM, Mario Limonciello wrote:
>> From: Mario Limonciello <mario.limonciello@....com>
>>
>> commit 5c4fa2a6da7fb ("Input: soc_button_array - debounce the buttons")
>> hardcoded all soc-button-array devices to use a 50ms debounce timeout
>> but this doesn't work on all hardware.  The hardware I have on hand
>> actually prescribes in the ASL that the timeout should be 0:
>>
>> GpioInt (Edge, ActiveBoth, Exclusive, PullUp, 0x0000,
>>           "\\_SB.GPIO", 0x00, ResourceConsumer, ,)
>> {   // Pin list
>>      0x0000
>> }
>>
>> Many cherryview and baytrail systems don't have accurate values in the
>> ASL for debouncing and thus use software debouncing in gpio_keys. The
>> value to use is programmed in soc_button_array.  Detect Cherry View
>> and Baytrail using ACPI HID IDs used for those GPIO controllers and apply
>> the 50ms only for those systems.
>>
>> Cc: Hans de Goede <hansg@...nel.org>
>> Fixes: 5c4fa2a6da7fb ("Input: soc_button_array - debounce the buttons")
>> Signed-off-by: Mario Limonciello <mario.limonciello@....com>
> 
> I'm not a fan of this approach, I believe that we need to always debounce
> when dealing with mechanical buttons otherwise we will get unreliable /
> spurious input events.
> 
> My suggestion to deal with the issue where setting up debouncing at
> the GPIO controller level is causing issues is to always use software
> debouncing (which I suspect is what Windows does).
> 
> Let me copy and pasting my reply from the v1 thread with
> a bit more detail on my proposal:
> 
> My proposal is to add a "no_hw_debounce" flag to
> struct gpio_keys_platform_data and make the soc_button_array
> driver set that regardless of which platform it is running on.
> 
> And then in gpio_keys.c do something like this:
> 
> diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
> index f9db86da0818..2788d1e5782c 100644
> --- a/drivers/input/keyboard/gpio_keys.c
> +++ b/drivers/input/keyboard/gpio_keys.c
> @@ -552,8 +552,11 @@ static int gpio_keys_setup_key(struct platform_device *pdev,
>   		bool active_low = gpiod_is_active_low(bdata->gpiod);
>   
>   		if (button->debounce_interval) {
> -			error = gpiod_set_debounce(bdata->gpiod,
> -					button->debounce_interval * 1000);
> +			if (ddata->pdata->no_hw_debounce)
> +				error = -EINVAL;
> +			else
> +				error = gpiod_set_debounce(bdata->gpiod,
> +						button->debounce_interval * 1000);
>   			/* use timer if gpiolib doesn't provide debounce */
>   			if (error < 0)
>   				bdata->software_debounce =
> 
> So keep debouncing, as that will always be necessary when dealing with
> mechanical buttons, but always use software debouncing to avoid issues
> like the issue you are seeing.
> 
> My mention of the BYT/CHT behavior in my previous email was to point
> out that those already always use software debouncing for the 50 ms
> debounce-period. It was *not* my intention to suggest to solve this
> with platform specific quirks/behavior.
> 
> Regards,
> 
> Hans

I mentioned on the v1 too, but let's shift conversation here.

So essentially all platforms using soc_button_array would always turn on 
software debouncing of 50ms?

In that case what happens if the hardware debounce was ALSO set from the 
ASL?  You end up with double debouncing I would expect.

Shouldn't you only turn on software debouncing when it's required?

I'm wondering if considering the first two patches we should have 
gpio-keys look up if hardware can support debounce, and then "only if it 
can't" we program the value from soc button array.

It can be done by having gpio_keys do a "get()" on debounce.  Iff the 
driver returns -ENOTSUPP /then/ program the software debounce.