All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/3] input: gpio-keys: Remove extra call to input_sync
@ 2021-03-05 17:01 Paul Cercueil
  2021-03-05 17:01 ` [PATCH 2/3] input: gpio-keys: Use hrtimer for release timer Paul Cercueil
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Paul Cercueil @ 2021-03-05 17:01 UTC (permalink / raw)
  To: Dmitry Torokhov; +Cc: od, linux-input, linux-kernel, Paul Cercueil

The input_sync() function will already be called within
gpio_keys_gpio_report_event(), so there's no need to call it again after
the loop in gpio_keys_report_state().

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 drivers/input/keyboard/gpio_keys.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
index 77bac4ddf324..0be204693ab0 100644
--- a/drivers/input/keyboard/gpio_keys.c
+++ b/drivers/input/keyboard/gpio_keys.c
@@ -641,7 +641,6 @@ static int gpio_keys_setup_key(struct platform_device *pdev,
 
 static void gpio_keys_report_state(struct gpio_keys_drvdata *ddata)
 {
-	struct input_dev *input = ddata->input;
 	int i;
 
 	for (i = 0; i < ddata->pdata->nbuttons; i++) {
@@ -649,7 +648,6 @@ static void gpio_keys_report_state(struct gpio_keys_drvdata *ddata)
 		if (bdata->gpiod)
 			gpio_keys_gpio_report_event(bdata);
 	}
-	input_sync(input);
 }
 
 static int gpio_keys_open(struct input_dev *input)
-- 
2.30.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 2/3] input: gpio-keys: Use hrtimer for release timer
  2021-03-05 17:01 [PATCH 1/3] input: gpio-keys: Remove extra call to input_sync Paul Cercueil
@ 2021-03-05 17:01 ` Paul Cercueil
  2021-03-05 17:01 ` [PATCH 3/3] input: gpio-keys: Use hrtimer for software debounce Paul Cercueil
  2021-03-05 18:40 ` [PATCH 1/3] input: gpio-keys: Remove extra call to input_sync Dmitry Torokhov
  2 siblings, 0 replies; 9+ messages in thread
From: Paul Cercueil @ 2021-03-05 17:01 UTC (permalink / raw)
  To: Dmitry Torokhov; +Cc: od, linux-input, linux-kernel, Paul Cercueil

Dealing with input, timing is important; if the button should be
released in one millisecond, then it should be done in one millisecond
and not a hundred milliseconds.

Therefore, the standard timer API is not really suitable for this task.

Convert the gpio-keys driver to use a hrtimer instead of the standard
timer to address this issue.

Note that by using a hard IRQ for the hrtimer callback, we can get rid
of the spin_lock_irqsave() and spin_unlock_irqrestore().

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 drivers/input/keyboard/gpio_keys.c | 27 ++++++++++++++++-----------
 1 file changed, 16 insertions(+), 11 deletions(-)

diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
index 0be204693ab0..1ab112267aa8 100644
--- a/drivers/input/keyboard/gpio_keys.c
+++ b/drivers/input/keyboard/gpio_keys.c
@@ -8,6 +8,7 @@
 
 #include <linux/module.h>
 
+#include <linux/hrtimer.h>
 #include <linux/init.h>
 #include <linux/fs.h>
 #include <linux/interrupt.h>
@@ -36,7 +37,7 @@ struct gpio_button_data {
 
 	unsigned short *code;
 
-	struct timer_list release_timer;
+	struct hrtimer release_timer;
 	unsigned int release_delay;	/* in msecs, for IRQ-only buttons */
 
 	struct delayed_work work;
@@ -146,7 +147,7 @@ static void gpio_keys_disable_button(struct gpio_button_data *bdata)
 		if (bdata->gpiod)
 			cancel_delayed_work_sync(&bdata->work);
 		else
-			del_timer_sync(&bdata->release_timer);
+			hrtimer_cancel(&bdata->release_timer);
 
 		bdata->disabled = true;
 	}
@@ -415,19 +416,20 @@ static irqreturn_t gpio_keys_gpio_isr(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
-static void gpio_keys_irq_timer(struct timer_list *t)
+static enum hrtimer_restart gpio_keys_irq_timer(struct hrtimer *t)
 {
-	struct gpio_button_data *bdata = from_timer(bdata, t, release_timer);
+	struct gpio_button_data *bdata = container_of(t,
+						      struct gpio_button_data,
+						      release_timer);
 	struct input_dev *input = bdata->input;
-	unsigned long flags;
 
-	spin_lock_irqsave(&bdata->lock, flags);
 	if (bdata->key_pressed) {
 		input_event(input, EV_KEY, *bdata->code, 0);
 		input_sync(input);
 		bdata->key_pressed = false;
 	}
-	spin_unlock_irqrestore(&bdata->lock, flags);
+
+	return HRTIMER_NORESTART;
 }
 
 static irqreturn_t gpio_keys_irq_isr(int irq, void *dev_id)
@@ -457,8 +459,9 @@ static irqreturn_t gpio_keys_irq_isr(int irq, void *dev_id)
 	}
 
 	if (bdata->release_delay)
-		mod_timer(&bdata->release_timer,
-			jiffies + msecs_to_jiffies(bdata->release_delay));
+		hrtimer_start(&bdata->release_timer,
+			      ms_to_ktime(bdata->release_delay),
+			      HRTIMER_MODE_REL_HARD);
 out:
 	spin_unlock_irqrestore(&bdata->lock, flags);
 	return IRQ_HANDLED;
@@ -471,7 +474,7 @@ static void gpio_keys_quiesce_key(void *data)
 	if (bdata->gpiod)
 		cancel_delayed_work_sync(&bdata->work);
 	else
-		del_timer_sync(&bdata->release_timer);
+		hrtimer_cancel(&bdata->release_timer);
 }
 
 static int gpio_keys_setup_key(struct platform_device *pdev,
@@ -595,7 +598,9 @@ static int gpio_keys_setup_key(struct platform_device *pdev,
 		}
 
 		bdata->release_delay = button->debounce_interval;
-		timer_setup(&bdata->release_timer, gpio_keys_irq_timer, 0);
+		hrtimer_init(&bdata->release_timer,
+			     CLOCK_REALTIME, HRTIMER_MODE_REL_HARD);
+		bdata->release_timer.function = gpio_keys_irq_timer;
 
 		isr = gpio_keys_irq_isr;
 		irqflags = 0;
-- 
2.30.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 3/3] input: gpio-keys: Use hrtimer for software debounce
  2021-03-05 17:01 [PATCH 1/3] input: gpio-keys: Remove extra call to input_sync Paul Cercueil
  2021-03-05 17:01 ` [PATCH 2/3] input: gpio-keys: Use hrtimer for release timer Paul Cercueil
@ 2021-03-05 17:01 ` Paul Cercueil
  2021-03-05 18:35   ` Dmitry Torokhov
  2021-03-05 18:40 ` [PATCH 1/3] input: gpio-keys: Remove extra call to input_sync Dmitry Torokhov
  2 siblings, 1 reply; 9+ messages in thread
From: Paul Cercueil @ 2021-03-05 17:01 UTC (permalink / raw)
  To: Dmitry Torokhov; +Cc: od, linux-input, linux-kernel, Paul Cercueil

We want to be able to report the input event as soon as the debounce
delay elapsed. However, the current code does not really ensure that,
as it uses the jiffies-based schedule_delayed_work() API. With a small
enough HZ value (HZ <= 100), this results in some input events being
lost, when a key is quickly pressed then released (on a human's time
scale).

Switching to hrtimers fixes this issue, and will work even on extremely
low HZ values (tested at HZ=24).

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 drivers/input/keyboard/gpio_keys.c | 33 +++++++++++++++---------------
 1 file changed, 17 insertions(+), 16 deletions(-)

diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
index 1ab112267aa8..267ed99e1911 100644
--- a/drivers/input/keyboard/gpio_keys.c
+++ b/drivers/input/keyboard/gpio_keys.c
@@ -22,7 +22,6 @@
 #include <linux/platform_device.h>
 #include <linux/input.h>
 #include <linux/gpio_keys.h>
-#include <linux/workqueue.h>
 #include <linux/gpio.h>
 #include <linux/gpio/consumer.h>
 #include <linux/of.h>
@@ -40,7 +39,7 @@ struct gpio_button_data {
 	struct hrtimer release_timer;
 	unsigned int release_delay;	/* in msecs, for IRQ-only buttons */
 
-	struct delayed_work work;
+	struct hrtimer debounce_timer;
 	unsigned int software_debounce;	/* in msecs, for GPIO-driven buttons */
 
 	unsigned int irq;
@@ -145,7 +144,7 @@ static void gpio_keys_disable_button(struct gpio_button_data *bdata)
 		disable_irq(bdata->irq);
 
 		if (bdata->gpiod)
-			cancel_delayed_work_sync(&bdata->work);
+			hrtimer_cancel(&bdata->debounce_timer);
 		else
 			hrtimer_cancel(&bdata->release_timer);
 
@@ -377,15 +376,18 @@ static void gpio_keys_gpio_report_event(struct gpio_button_data *bdata)
 	input_sync(input);
 }
 
-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);
 
 	if (bdata->button->wakeup)
 		pm_relax(bdata->input->dev.parent);
+
+	return HRTIMER_NORESTART;
 }
 
 static irqreturn_t gpio_keys_gpio_isr(int irq, void *dev_id)
@@ -409,9 +411,9 @@ static irqreturn_t gpio_keys_gpio_isr(int irq, void *dev_id)
 		}
 	}
 
-	mod_delayed_work(system_wq,
-			 &bdata->work,
-			 msecs_to_jiffies(bdata->software_debounce));
+	hrtimer_start(&bdata->debounce_timer,
+		      ms_to_ktime(bdata->software_debounce),
+		      HRTIMER_MODE_REL_SOFT);
 
 	return IRQ_HANDLED;
 }
@@ -472,7 +474,7 @@ static void gpio_keys_quiesce_key(void *data)
 	struct gpio_button_data *bdata = data;
 
 	if (bdata->gpiod)
-		cancel_delayed_work_sync(&bdata->work);
+		hrtimer_cancel(&bdata->debounce_timer);
 	else
 		hrtimer_cancel(&bdata->release_timer);
 }
@@ -562,11 +564,13 @@ static int gpio_keys_setup_key(struct platform_device *pdev,
 			bdata->irq = irq;
 		}
 
-		INIT_DELAYED_WORK(&bdata->work, gpio_keys_gpio_work_func);
-
 		isr = gpio_keys_gpio_isr;
 		irqflags = IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING;
 
+		hrtimer_init(&bdata->debounce_timer,
+			     CLOCK_REALTIME, HRTIMER_MODE_REL_SOFT);
+		bdata->debounce_timer.function = gpio_keys_debounce_timer;
+
 		switch (button->wakeup_event_action) {
 		case EV_ACT_ASSERTED:
 			bdata->wakeup_trigger_type = active_low ?
@@ -615,10 +619,7 @@ static int gpio_keys_setup_key(struct platform_device *pdev,
 	*bdata->code = button->code;
 	input_set_capability(input, button->type ?: EV_KEY, *bdata->code);
 
-	/*
-	 * Install custom action to cancel release timer and
-	 * workqueue item.
-	 */
+	/* Install custom action to cancel timers. */
 	error = devm_add_action(dev, gpio_keys_quiesce_key, bdata);
 	if (error) {
 		dev_err(dev, "failed to register quiesce action, error: %d\n",
-- 
2.30.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH 3/3] input: gpio-keys: Use hrtimer for software debounce
  2021-03-05 17:01 ` [PATCH 3/3] input: gpio-keys: Use hrtimer for software debounce Paul Cercueil
@ 2021-03-05 18:35   ` Dmitry Torokhov
  2021-03-05 20:00     ` Paul Cercueil
  0 siblings, 1 reply; 9+ messages in thread
From: Dmitry Torokhov @ 2021-03-05 18:35 UTC (permalink / raw)
  To: Paul Cercueil; +Cc: od, linux-input, linux-kernel

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).

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.

Thanks.

-- 
Dmitry

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/3] input: gpio-keys: Remove extra call to input_sync
  2021-03-05 17:01 [PATCH 1/3] input: gpio-keys: Remove extra call to input_sync Paul Cercueil
  2021-03-05 17:01 ` [PATCH 2/3] input: gpio-keys: Use hrtimer for release timer Paul Cercueil
  2021-03-05 17:01 ` [PATCH 3/3] input: gpio-keys: Use hrtimer for software debounce Paul Cercueil
@ 2021-03-05 18:40 ` Dmitry Torokhov
  2 siblings, 0 replies; 9+ messages in thread
From: Dmitry Torokhov @ 2021-03-05 18:40 UTC (permalink / raw)
  To: Paul Cercueil; +Cc: od, linux-input, linux-kernel

On Fri, Mar 05, 2021 at 05:01:09PM +0000, Paul Cercueil wrote:
> The input_sync() function will already be called within
> gpio_keys_gpio_report_event(), so there's no need to call it again after
> the loop in gpio_keys_report_state().

I'd probably go other way around and remove the sync from
gpio_keys_report_state() and add one to gpio_keys_gpio_work_func() so
that we do not have to send bunch of input_sync() when there are several
keys/buttons.

Thanks.

-- 
Dmitry

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 3/3] input: gpio-keys: Use hrtimer for software debounce
  2021-03-05 18:35   ` Dmitry Torokhov
@ 2021-03-05 20:00     ` Paul Cercueil
  2021-03-07 20:20       ` Dmitry Torokhov
  0 siblings, 1 reply; 9+ messages in thread
From: Paul Cercueil @ 2021-03-05 20:00 UTC (permalink / raw)
  To: Dmitry Torokhov; +Cc: od, linux-input, linux-kernel

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.

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



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 3/3] input: gpio-keys: Use hrtimer for software debounce
  2021-03-05 20:00     ` Paul Cercueil
@ 2021-03-07 20:20       ` Dmitry Torokhov
  2021-03-07 21:11         ` Paul Cercueil
  0 siblings, 1 reply; 9+ messages in thread
From: Dmitry Torokhov @ 2021-03-07 20:20 UTC (permalink / raw)
  To: Paul Cercueil; +Cc: od, linux-input, linux-kernel

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.

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.

> 
> 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.

Thanks.

-- 
Dmitry

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 3/3] input: gpio-keys: Use hrtimer for software debounce
  2021-03-07 20:20       ` Dmitry Torokhov
@ 2021-03-07 21:11         ` Paul Cercueil
  2021-03-07 21:47           ` Dmitry Torokhov
  0 siblings, 1 reply; 9+ messages in thread
From: Paul Cercueil @ 2021-03-07 21:11 UTC (permalink / raw)
  To: Dmitry Torokhov; +Cc: od, linux-input, linux-kernel

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.

> 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.

Cheers,
-Paul



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 3/3] input: gpio-keys: Use hrtimer for software debounce
  2021-03-07 21:11         ` Paul Cercueil
@ 2021-03-07 21:47           ` Dmitry Torokhov
  0 siblings, 0 replies; 9+ messages in thread
From: Dmitry Torokhov @ 2021-03-07 21:47 UTC (permalink / raw)
  To: Paul Cercueil; +Cc: od, linux-input, linux-kernel

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2021-03-07 21:49 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-05 17:01 [PATCH 1/3] input: gpio-keys: Remove extra call to input_sync Paul Cercueil
2021-03-05 17:01 ` [PATCH 2/3] input: gpio-keys: Use hrtimer for release timer Paul Cercueil
2021-03-05 17:01 ` [PATCH 3/3] input: gpio-keys: Use hrtimer for software debounce Paul Cercueil
2021-03-05 18:35   ` Dmitry Torokhov
2021-03-05 20:00     ` Paul Cercueil
2021-03-07 20:20       ` Dmitry Torokhov
2021-03-07 21:11         ` Paul Cercueil
2021-03-07 21:47           ` Dmitry Torokhov
2021-03-05 18:40 ` [PATCH 1/3] input: gpio-keys: Remove extra call to input_sync Dmitry Torokhov

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.