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

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

On Sun, Mar 07, 2021 at 09:11:18PM +0000, Paul Cercueil wrote:
> Hi Dmitry,
> 
> Le dim. 7 mars 2021 à 12:20, Dmitry Torokhov <dmitry.torokhov@gmail.com> a
> écrit :
> > On Fri, Mar 05, 2021 at 08:00:43PM +0000, Paul Cercueil wrote:
> > >  Hi Dmitry,
> > > 
> > >  Le ven. 5 mars 2021 à 10:35, Dmitry Torokhov
> > > <dmitry.torokhov@gmail.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.
> > 
> > I am afraid you misunderstand what soft irq context is, as softirqs and
> > tasklets still run in interrupt context and therefore can not sleep,
> > only code running in process context may sleep.
> 
> I probably do. My understanding of "softirq" is that the callback runs in a
> threaded interrupt handler.

No, you are thinking about threaded interrupts, which are separate
beasts. Softirqs are traditional bottom halfs that run after exit of
hard interrupt, but still are non-preemptible so sleeping is not
allowed.

> 
> > You can test it yourself by sticking "msleep(1)" in
> > gpio_keys_debounce_timer() and see if you will get "scheduling while
> > atomic" in logs.
> 
> I tested it, it locks up.
> 
> > > 
> > >  But I guess in this case I can use HRTIMER_MODE_REL.
> > 
> > This changes selected clock source, but has no effect on whether timer
> > handler can sleep or not.
> > 
> > > 
> > >  > 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.
> > 
> > I explained why they can't. They could be if you restrict gpio_keys to
> > only be used with GPIOs that do not require sleep to read their state,
> > but I am not willing to accept such restriction. You either need to have
> > longer debounce, higher HZ, or see if you can use GPIO controller that
> > supports debounce handling. See also if you can enable dynamic
> > ticks/NO_HZ to limit number of timer interrupts on idle system.
> 
> We can also use the hrtimer approach if the GPIO doesn't require sleep, and
> fall back to the standard timer if it does. It's possible to detect that
> with gpiod_cansleep(). The diff would be pretty slim. Would you accept
> something like that?
> 
> Switching from HZ=250 to HZ=24 leads to a 3% overall performance increase
> across all apps on our system, so a pretty big optimization, and this is the
> only blocker.

Let me take a look at the updated patch and we will see.

Thanks.

-- 
Dmitry