linux-kernel - Re: [PATCH 3/3] input: gpio-keys: Use hrtimer for software debounce

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: <79IIPQ.DQ7JNXZ0OI5Q2@crapouillou.net>
Date:   Fri, 05 Mar 2021 20:00:43 +0000
From:   Paul Cercueil <paul@...pouillou.net>
To:     Dmitry Torokhov <dmitry.torokhov@...il.com>
Cc:     od@...c.me, linux-input@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/3] input: gpio-keys: Use hrtimer for software debounce

Hi Dmitry,

Le ven. 5 mars 2021 à 10:35, Dmitry Torokhov 
<dmitry.torokhov@...il.com> a écrit :
> Hi Paul,
> 
> On Fri, Mar 05, 2021 at 05:01:11PM +0000, Paul Cercueil wrote:
>>  -static void gpio_keys_gpio_work_func(struct work_struct *work)
>>  +static enum hrtimer_restart gpio_keys_debounce_timer(struct 
>> hrtimer *t)
>>   {
>>  -	struct gpio_button_data *bdata =
>>  -		container_of(work, struct gpio_button_data, work.work);
>>  +	struct gpio_button_data *bdata = container_of(t,
>>  +						      struct gpio_button_data,
>>  +						      debounce_timer);
>> 
>>   	gpio_keys_gpio_report_event(bdata);
> 
> I am not sure how this works. As far as I know, even
> HRTIMER_MODE_REL_SOFT do not allow sleeping in the timer handlers, and
> gpio_keys_gpio_report_event() use sleeping variant of GPIOD API (and
> that is not going to change).

Quoting <linux/hrtimers.h>, the "timer callback will be executed in 
soft irq context", so sleeping should be possible.

But I guess in this case I can use HRTIMER_MODE_REL.

> It seems to me that if you want to use software debounce in gpio keys
> driver you need to set up sufficiently high HZ for your system. Maybe 
> we
> could thrown a warning when we see low debounce delay and low HZ to
> alert system developer.

This is exactly what we should not do. I certainly don't want to have 
250+ timer interrupts per second just so that input events aren't lost, 
to work around a sucky debounce implementation. Besides, if you 
consider the hrtimers doc (Documentation/timers/hrtimers.rst), hrtimers 
really are what should be used here.

-Paul