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@xxxxxxxxx> 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@xxxxxxxxx> 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
