[PATCH v3 1/3] input: gpio-keys: Remove extra call to input_sync

[PATCH v3 1/3] input: gpio-keys: Remove extra call to input_sync

[Paul Cercueil]

The input_sync() function is already called after the loop in
gpio_keys_report_state(), so it does not need to be called after each
iteration within gpio_keys_gpio_report_event().

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---

Notes:
    v2: Keep the input_sync() within gpio_keys_report_state() so that it's
        not called at every iteration of the loop.
    
    v3: No change

 drivers/input/keyboard/gpio_keys.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
index 77bac4ddf324..7fcb2c35c5cc 100644
--- a/drivers/input/keyboard/gpio_keys.c
+++ b/drivers/input/keyboard/gpio_keys.c
@@ -373,7 +373,6 @@ static void gpio_keys_gpio_report_event(struct gpio_button_data *bdata)
 	} else {
 		input_event(input, type, *bdata->code, state);
 	}
-	input_sync(input);
 }
 
 static void gpio_keys_gpio_work_func(struct work_struct *work)
@@ -382,6 +381,7 @@ static void gpio_keys_gpio_work_func(struct work_struct *work)
 		container_of(work, struct gpio_button_data, work.work);
 
 	gpio_keys_gpio_report_event(bdata);
+	input_sync(bdata->input);
 
 	if (bdata->button->wakeup)
 		pm_relax(bdata->input->dev.parent);
-- 
2.30.1

[PATCH v3 2/3] input: gpio-keys: Use hrtimer for release timer

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

Notes:
    v2-v3: No change

 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 7fcb2c35c5cc..4b92f49decef 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

Re: [PATCH v3 2/3] input: gpio-keys: Use hrtimer for release timer

[Dmitry Torokhov]

On Sun, Mar 07, 2021 at 10:22:39PM +0000, Paul Cercueil wrote:
> 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>

Applied, thank you.

-- 
Dmitry

[PATCH v3 3/3] input: gpio-keys: Use hrtimer for software debounce, if possible

[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). This is however only possible if
reading the GPIO is possible without sleeping. If this condition is not
met, the previous approach of using a jiffies-based timer is taken.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---

Notes:
    v2: HRTIMER_MODE_REL_SOFT -> HRTIMER_MODE_REL
    
    v3: Only use a hrtimer-based timer if we know that reading the GPIO
        will never sleep.

 drivers/input/keyboard/gpio_keys.c | 69 ++++++++++++++++++++++++------
 1 file changed, 56 insertions(+), 13 deletions(-)

diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
index 4b92f49decef..046d9dffa171 100644
--- a/drivers/input/keyboard/gpio_keys.c
+++ b/drivers/input/keyboard/gpio_keys.c
@@ -41,6 +41,7 @@ struct gpio_button_data {
 	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;
@@ -49,6 +50,7 @@ struct gpio_button_data {
 	bool disabled;
 	bool key_pressed;
 	bool suspended;
+	bool debounce_use_hrtimer;
 };
 
 struct gpio_keys_drvdata {
@@ -144,10 +146,12 @@ static void gpio_keys_disable_button(struct gpio_button_data *bdata)
 		 */
 		disable_irq(bdata->irq);
 
-		if (bdata->gpiod)
-			cancel_delayed_work_sync(&bdata->work);
-		else
+		if (!bdata->gpiod)
 			hrtimer_cancel(&bdata->release_timer);
+		else if (bdata->debounce_use_hrtimer)
+			hrtimer_cancel(&bdata->debounce_timer);
+		else
+			cancel_delayed_work_sync(&bdata->work);
 
 		bdata->disabled = true;
 	}
@@ -361,7 +365,10 @@ static void gpio_keys_gpio_report_event(struct gpio_button_data *bdata)
 	unsigned int type = button->type ?: EV_KEY;
 	int state;
 
-	state = gpiod_get_value_cansleep(bdata->gpiod);
+	if (bdata->debounce_use_hrtimer)
+		state = gpiod_get_value(bdata->gpiod);
+	else
+		state = gpiod_get_value_cansleep(bdata->gpiod);
 	if (state < 0) {
 		dev_err(input->dev.parent,
 			"failed to get gpio state: %d\n", state);
@@ -376,11 +383,8 @@ static void gpio_keys_gpio_report_event(struct gpio_button_data *bdata)
 	}
 }
 
-static void gpio_keys_gpio_work_func(struct work_struct *work)
+static void gpio_keys_debounce_event(struct gpio_button_data *bdata)
 {
-	struct gpio_button_data *bdata =
-		container_of(work, struct gpio_button_data, work.work);
-
 	gpio_keys_gpio_report_event(bdata);
 	input_sync(bdata->input);
 
@@ -388,6 +392,26 @@ static void gpio_keys_gpio_work_func(struct work_struct *work)
 		pm_relax(bdata->input->dev.parent);
 }
 
+static void gpio_keys_gpio_work_func(struct work_struct *work)
+{
+	struct gpio_button_data *bdata = container_of(work,
+						      struct gpio_button_data,
+						      work.work);
+
+	gpio_keys_debounce_event(bdata);
+}
+
+static enum hrtimer_restart gpio_keys_debounce_timer(struct hrtimer *t)
+{
+	struct gpio_button_data *bdata = container_of(t,
+						      struct gpio_button_data,
+						      debounce_timer);
+
+	gpio_keys_debounce_event(bdata);
+
+	return HRTIMER_NORESTART;
+}
+
 static irqreturn_t gpio_keys_gpio_isr(int irq, void *dev_id)
 {
 	struct gpio_button_data *bdata = dev_id;
@@ -409,9 +433,15 @@ 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));
+	if (bdata->debounce_use_hrtimer) {
+		hrtimer_start(&bdata->debounce_timer,
+			      ms_to_ktime(bdata->software_debounce),
+			      HRTIMER_MODE_REL);
+	} else {
+		mod_delayed_work(system_wq,
+				 &bdata->work,
+				 msecs_to_jiffies(bdata->software_debounce));
+	}
 
 	return IRQ_HANDLED;
 }
@@ -471,10 +501,12 @@ static void gpio_keys_quiesce_key(void *data)
 {
 	struct gpio_button_data *bdata = data;
 
-	if (bdata->gpiod)
+	if (bdata->gpiod) {
+		hrtimer_cancel(&bdata->debounce_timer);
 		cancel_delayed_work_sync(&bdata->work);
-	else
+	} else {
 		hrtimer_cancel(&bdata->release_timer);
+	}
 }
 
 static int gpio_keys_setup_key(struct platform_device *pdev,
@@ -546,6 +578,13 @@ static int gpio_keys_setup_key(struct platform_device *pdev,
 			if (error < 0)
 				bdata->software_debounce =
 						button->debounce_interval;
+
+			/*
+			 * If reading the GPIO won't sleep, we can use a hrtimer
+			 * instead of a standard timer for the software
+			 * debounce, to reduce the latency as much as possible.
+			 */
+			bdata->debounce_use_hrtimer = !gpiod_cansleep(bdata->gpiod);
 		}
 
 		if (button->irq) {
@@ -564,6 +603,10 @@ static int gpio_keys_setup_key(struct platform_device *pdev,
 
 		INIT_DELAYED_WORK(&bdata->work, gpio_keys_gpio_work_func);
 
+		hrtimer_init(&bdata->debounce_timer,
+			     CLOCK_REALTIME, HRTIMER_MODE_REL);
+		bdata->debounce_timer.function = gpio_keys_debounce_timer;
+
 		isr = gpio_keys_gpio_isr;
 		irqflags = IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING;
 
-- 
2.30.1

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

[Dmitry Torokhov]

On Sun, Mar 07, 2021 at 10:22:40PM +0000, Paul Cercueil wrote:
> 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). This is however only possible if
> reading the GPIO is possible without sleeping. If this condition is not
> met, the previous approach of using a jiffies-based timer is taken.
> 
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>

Applied with minor edits to make more use of debounce_use_hrtimer flag.

Thanks.

-- 
Dmitry

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

[Tony Lindgren]

Hi,

* Dmitry Torokhov <dmitry.torokhov@gmail.com> [700101 02:00]:
> On Sun, Mar 07, 2021 at 10:22:40PM +0000, Paul Cercueil wrote:
> > 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). This is however only possible if
> > reading the GPIO is possible without sleeping. If this condition is not
> > met, the previous approach of using a jiffies-based timer is taken.
> > 
> > Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> 
> Applied with minor edits to make more use of debounce_use_hrtimer flag.

While testing Linux next I noticed that this patch causes a null pointer
dereference at least when unbinding a gpio-keys instance, see below.

Regards,

Tony

8< -----------------
Unable to handle kernel NULL pointer dereference at virtual address 0000000c
...
PC is at hrtimer_active+0xc/0x98
LR is at hrtimer_try_to_cancel+0x24/0x140
...
[<c01c43b8>] (hrtimer_active) from [<c01c50f4>] (hrtimer_try_to_cancel+0x24/0x140)
[<c01c50f4>] (hrtimer_try_to_cancel) from [<c01c5224>] (hrtimer_cancel+0x14/0x4c)
[<c01c5224>] (hrtimer_cancel) from [<bf1cae24>] (gpio_keys_attr_store_helper+0x1b8/0x1d8 [gpio_keys])
[<bf1cae24>] (gpio_keys_attr_store_helper [gpio_keys]) from [<bf1cae80>] (gpio_keys_store_disabled_keys+0x18/0x24 [gpio_keys])
[<bf1cae80>] (gpio_keys_store_disabled_keys [gpio_keys]) from [<c038ec7c>] (kernfs_fop_write_iter+0x10c/0x1cc)
[<c038ec7c>] (kernfs_fop_write_iter) from [<c02df858>] (vfs_write+0x2ac/0x404)
[<c02df858>] (vfs_write) from [<c02dfaf4>] (ksys_write+0x64/0xdc)
[<c02dfaf4>] (ksys_write) from [<c0100080>] (ret_fast_syscall+0x0/0x58)

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

[Dmitry Torokhov]

On Tue, Apr 06, 2021 at 11:37:07AM +0300, Tony Lindgren wrote:
> Hi,
> 
> * Dmitry Torokhov <dmitry.torokhov@gmail.com> [700101 02:00]:
> > On Sun, Mar 07, 2021 at 10:22:40PM +0000, Paul Cercueil wrote:
> > > 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). This is however only possible if
> > > reading the GPIO is possible without sleeping. If this condition is not
> > > met, the previous approach of using a jiffies-based timer is taken.
> > > 
> > > Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> > 
> > Applied with minor edits to make more use of debounce_use_hrtimer flag.
> 
> While testing Linux next I noticed that this patch causes a null pointer
> dereference at least when unbinding a gpio-keys instance, see below.

Ugh, my "minor edits" did screw things up ;( as I mixed up release and
debounce timers. I'll fix it up.

> 
> Regards,
> 
> Tony
> 
> 8< -----------------
> Unable to handle kernel NULL pointer dereference at virtual address 0000000c
> ...
> PC is at hrtimer_active+0xc/0x98
> LR is at hrtimer_try_to_cancel+0x24/0x140
> ...
> [<c01c43b8>] (hrtimer_active) from [<c01c50f4>] (hrtimer_try_to_cancel+0x24/0x140)
> [<c01c50f4>] (hrtimer_try_to_cancel) from [<c01c5224>] (hrtimer_cancel+0x14/0x4c)
> [<c01c5224>] (hrtimer_cancel) from [<bf1cae24>] (gpio_keys_attr_store_helper+0x1b8/0x1d8 [gpio_keys])
> [<bf1cae24>] (gpio_keys_attr_store_helper [gpio_keys]) from [<bf1cae80>] (gpio_keys_store_disabled_keys+0x18/0x24 [gpio_keys])
> [<bf1cae80>] (gpio_keys_store_disabled_keys [gpio_keys]) from [<c038ec7c>] (kernfs_fop_write_iter+0x10c/0x1cc)
> [<c038ec7c>] (kernfs_fop_write_iter) from [<c02df858>] (vfs_write+0x2ac/0x404)
> [<c02df858>] (vfs_write) from [<c02dfaf4>] (ksys_write+0x64/0xdc)
> [<c02dfaf4>] (ksys_write) from [<c0100080>] (ret_fast_syscall+0x0/0x58)
> 

-- 
Dmitry

Re: [PATCH v3 1/3] input: gpio-keys: Remove extra call to input_sync

[Dmitry Torokhov]

On Sun, Mar 07, 2021 at 10:22:38PM +0000, Paul Cercueil wrote:
> The input_sync() function is already called after the loop in
> gpio_keys_report_state(), so it does not need to be called after each
> iteration within gpio_keys_gpio_report_event().
> 
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>

Applied, thank you.

-- 
Dmitry