Re: [PATCH v3 3/3] input: gpio-keys: Use hrtimer for software debounce, if possible
On Tue, Apr 06, 2021 at 11:37:07AM +0300, Tony Lindgren wrote: > Hi, > > * Dmitry Torokhov [700101 02:00]: > > On Sun, Mar 07, 2021 at 10:22:40PM +, 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 > > > > 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 000c > ... > PC is at hrtimer_active+0xc/0x98 > LR is at hrtimer_try_to_cancel+0x24/0x140 > ... > [] (hrtimer_active) from [] > (hrtimer_try_to_cancel+0x24/0x140) > [] (hrtimer_try_to_cancel) from [] > (hrtimer_cancel+0x14/0x4c) > [] (hrtimer_cancel) from [] > (gpio_keys_attr_store_helper+0x1b8/0x1d8 [gpio_keys]) > [] (gpio_keys_attr_store_helper [gpio_keys]) from [] > (gpio_keys_store_disabled_keys+0x18/0x24 [gpio_keys]) > [] (gpio_keys_store_disabled_keys [gpio_keys]) from [] > (kernfs_fop_write_iter+0x10c/0x1cc) > [] (kernfs_fop_write_iter) from [] (vfs_write+0x2ac/0x404) > [] (vfs_write) from [] (ksys_write+0x64/0xdc) > [] (ksys_write) from [] (ret_fast_syscall+0x0/0x58) > -- Dmitry
Re: [PATCH v3 3/3] input: gpio-keys: Use hrtimer for software debounce, if possible
Hi, * Dmitry Torokhov [700101 02:00]: > On Sun, Mar 07, 2021 at 10:22:40PM +, 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 > > 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 000c ... PC is at hrtimer_active+0xc/0x98 LR is at hrtimer_try_to_cancel+0x24/0x140 ... [] (hrtimer_active) from [] (hrtimer_try_to_cancel+0x24/0x140) [] (hrtimer_try_to_cancel) from [] (hrtimer_cancel+0x14/0x4c) [] (hrtimer_cancel) from [] (gpio_keys_attr_store_helper+0x1b8/0x1d8 [gpio_keys]) [] (gpio_keys_attr_store_helper [gpio_keys]) from [] (gpio_keys_store_disabled_keys+0x18/0x24 [gpio_keys]) [] (gpio_keys_store_disabled_keys [gpio_keys]) from [] (kernfs_fop_write_iter+0x10c/0x1cc) [] (kernfs_fop_write_iter) from [] (vfs_write+0x2ac/0x404) [] (vfs_write) from [] (ksys_write+0x64/0xdc) [] (ksys_write) from [] (ret_fast_syscall+0x0/0x58)
Re: [PATCH v3 3/3] input: gpio-keys: Use hrtimer for software debounce, if possible
On Sun, Mar 07, 2021 at 10:22:40PM +, 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 Applied with minor edits to make more use of debounce_use_hrtimer flag. Thanks. -- Dmitry
[PATCH v3 3/3] input: gpio-keys: Use hrtimer for software debounce, if possible
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
---
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
