diff mbox series

Input: gpio-keys - Cancel delayed work only in case of GPIO

Message ID 20220513132500.32395-1-prabhakar.mahadev-lad.rj@bp.renesas.com (mailing list archive)
State Superseded
Headers show
Series Input: gpio-keys - Cancel delayed work only in case of GPIO | expand

Commit Message

Lad Prabhakar May 13, 2022, 1:25 p.m. UTC
gpio_keys module can either accept gpios or interrupts. The module
initializes delayed work in case of gpios only and not for interrupts,
so make sure cancel_delayed_work_sync() is called only when bdata->gpiod
is true.

This fixes the issue seen below when the gpio_keys module is unloaded and
an interrupt pin is used instead of GPIO:

[  360.297569] ------------[ cut here ]------------
[  360.302303] WARNING: CPU: 0 PID: 237 at kernel/workqueue.c:3066 __flush_work+0x414/0x470
[  360.310531] Modules linked in: gpio_keys(-)
[  360.314797] CPU: 0 PID: 237 Comm: rmmod Not tainted 5.18.0-rc5-arm64-renesas-00116-g73636105874d-dirty #166
[  360.324662] Hardware name: Renesas SMARC EVK based on r9a07g054l2 (DT)
[  360.331270] pstate: 60400005 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[  360.338318] pc : __flush_work+0x414/0x470
[  360.342385] lr : __cancel_work_timer+0x140/0x1b0
[  360.347065] sp : ffff80000a7fba00
[  360.350423] x29: ffff80000a7fba00 x28: ffff000012b9c5c0 x27: 0000000000000000
[  360.357664] x26: ffff80000a7fbb80 x25: ffff80000954d0a8 x24: 0000000000000001
[  360.364904] x23: ffff800009757000 x22: 0000000000000000 x21: ffff80000919b000
[  360.372143] x20: ffff00000f5974e0 x19: ffff00000f5974e0 x18: ffff8000097fcf48
[  360.379382] x17: 0000000000000000 x16: 0000000000000000 x15: 0000000000053f40
[  360.386622] x14: ffff800009850e88 x13: 0000000000000002 x12: 000000000000a60c
[  360.393861] x11: 000000000000a610 x10: 0000000000000000 x9 : 0000000000000008
[  360.401100] x8 : 0101010101010101 x7 : 00000000a473c394 x6 : 0080808080808080
[  360.408339] x5 : 0000000000000001 x4 : 0000000000000000 x3 : ffff80000919b458
[  360.415578] x2 : ffff8000097577f0 x1 : 0000000000000001 x0 : 0000000000000000
[  360.422818] Call trace:
[  360.425299]  __flush_work+0x414/0x470
[  360.429012]  __cancel_work_timer+0x140/0x1b0
[  360.433340]  cancel_delayed_work_sync+0x10/0x18
[  360.437931]  gpio_keys_quiesce_key+0x28/0x58 [gpio_keys]
[  360.443327]  devm_action_release+0x10/0x18
[  360.447481]  release_nodes+0x8c/0x1a0
[  360.451194]  devres_release_all+0x90/0x100
[  360.455346]  device_unbind_cleanup+0x14/0x60
[  360.459677]  device_release_driver_internal+0xe8/0x168
[  360.464883]  driver_detach+0x4c/0x90
[  360.468509]  bus_remove_driver+0x54/0xb0
[  360.472485]  driver_unregister+0x2c/0x58
[  360.476462]  platform_driver_unregister+0x10/0x18
[  360.481230]  gpio_keys_exit+0x14/0x828 [gpio_keys]
[  360.486088]  __arm64_sys_delete_module+0x1e0/0x270
[  360.490945]  invoke_syscall+0x40/0xf8
[  360.494661]  el0_svc_common.constprop.3+0xf0/0x110
[  360.499515]  do_el0_svc+0x20/0x78
[  360.502877]  el0_svc+0x48/0xf8
[  360.505977]  el0t_64_sync_handler+0x88/0xb0
[  360.510216]  el0t_64_sync+0x148/0x14c
[  360.513930] irq event stamp: 4306
[  360.517288] hardirqs last  enabled at (4305): [<ffff8000080b0300>] __cancel_work_timer+0x130/0x1b0
[  360.526359] hardirqs last disabled at (4306): [<ffff800008d194fc>] el1_dbg+0x24/0x88
[  360.534204] softirqs last  enabled at (4278): [<ffff8000080104a0>] _stext+0x4a0/0x5e0
[  360.542133] softirqs last disabled at (4267): [<ffff8000080932ac>] irq_exit_rcu+0x18c/0x1b0
[  360.550591] ---[ end trace 0000000000000000 ]---

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 drivers/input/keyboard/gpio_keys.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Geert Uytterhoeven May 17, 2022, 7:33 a.m. UTC | #1
On Fri, May 13, 2022 at 6:44 PM Lad Prabhakar
<prabhakar.mahadev-lad.rj@bp.renesas.com> wrote:
> gpio_keys module can either accept gpios or interrupts. The module
> initializes delayed work in case of gpios only and not for interrupts,
> so make sure cancel_delayed_work_sync() is called only when bdata->gpiod
> is true.
>
> This fixes the issue seen below when the gpio_keys module is unloaded and
> an interrupt pin is used instead of GPIO:
>
> [  360.297569] ------------[ cut here ]------------
> [  360.302303] WARNING: CPU: 0 PID: 237 at kernel/workqueue.c:3066 __flush_work+0x414/0x470
> [  360.310531] Modules linked in: gpio_keys(-)
> [  360.314797] CPU: 0 PID: 237 Comm: rmmod Not tainted 5.18.0-rc5-arm64-renesas-00116-g73636105874d-dirty #166
> [  360.324662] Hardware name: Renesas SMARC EVK based on r9a07g054l2 (DT)
> [  360.331270] pstate: 60400005 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> [  360.338318] pc : __flush_work+0x414/0x470
> [  360.342385] lr : __cancel_work_timer+0x140/0x1b0
> [  360.347065] sp : ffff80000a7fba00
> [  360.350423] x29: ffff80000a7fba00 x28: ffff000012b9c5c0 x27: 0000000000000000
> [  360.357664] x26: ffff80000a7fbb80 x25: ffff80000954d0a8 x24: 0000000000000001
> [  360.364904] x23: ffff800009757000 x22: 0000000000000000 x21: ffff80000919b000
> [  360.372143] x20: ffff00000f5974e0 x19: ffff00000f5974e0 x18: ffff8000097fcf48
> [  360.379382] x17: 0000000000000000 x16: 0000000000000000 x15: 0000000000053f40
> [  360.386622] x14: ffff800009850e88 x13: 0000000000000002 x12: 000000000000a60c
> [  360.393861] x11: 000000000000a610 x10: 0000000000000000 x9 : 0000000000000008
> [  360.401100] x8 : 0101010101010101 x7 : 00000000a473c394 x6 : 0080808080808080
> [  360.408339] x5 : 0000000000000001 x4 : 0000000000000000 x3 : ffff80000919b458
> [  360.415578] x2 : ffff8000097577f0 x1 : 0000000000000001 x0 : 0000000000000000
> [  360.422818] Call trace:
> [  360.425299]  __flush_work+0x414/0x470
> [  360.429012]  __cancel_work_timer+0x140/0x1b0
> [  360.433340]  cancel_delayed_work_sync+0x10/0x18
> [  360.437931]  gpio_keys_quiesce_key+0x28/0x58 [gpio_keys]
> [  360.443327]  devm_action_release+0x10/0x18
> [  360.447481]  release_nodes+0x8c/0x1a0
> [  360.451194]  devres_release_all+0x90/0x100
> [  360.455346]  device_unbind_cleanup+0x14/0x60
> [  360.459677]  device_release_driver_internal+0xe8/0x168
> [  360.464883]  driver_detach+0x4c/0x90
> [  360.468509]  bus_remove_driver+0x54/0xb0
> [  360.472485]  driver_unregister+0x2c/0x58
> [  360.476462]  platform_driver_unregister+0x10/0x18
> [  360.481230]  gpio_keys_exit+0x14/0x828 [gpio_keys]
> [  360.486088]  __arm64_sys_delete_module+0x1e0/0x270
> [  360.490945]  invoke_syscall+0x40/0xf8
> [  360.494661]  el0_svc_common.constprop.3+0xf0/0x110
> [  360.499515]  do_el0_svc+0x20/0x78
> [  360.502877]  el0_svc+0x48/0xf8
> [  360.505977]  el0t_64_sync_handler+0x88/0xb0
> [  360.510216]  el0t_64_sync+0x148/0x14c
> [  360.513930] irq event stamp: 4306
> [  360.517288] hardirqs last  enabled at (4305): [<ffff8000080b0300>] __cancel_work_timer+0x130/0x1b0
> [  360.526359] hardirqs last disabled at (4306): [<ffff800008d194fc>] el1_dbg+0x24/0x88
> [  360.534204] softirqs last  enabled at (4278): [<ffff8000080104a0>] _stext+0x4a0/0x5e0
> [  360.542133] softirqs last disabled at (4267): [<ffff8000080932ac>] irq_exit_rcu+0x18c/0x1b0
> [  360.550591] ---[ end trace 0000000000000000 ]---
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
Dmitry Torokhov May 18, 2022, 4:46 a.m. UTC | #2
Hi Lad,

On Fri, May 13, 2022 at 02:25:00PM +0100, Lad Prabhakar wrote:
> gpio_keys module can either accept gpios or interrupts. The module
> initializes delayed work in case of gpios only and not for interrupts,
> so make sure cancel_delayed_work_sync() is called only when bdata->gpiod
> is true.
...
> diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
> index d75a8b179a8a..ec9d50ddda42 100644
> --- a/drivers/input/keyboard/gpio_keys.c
> +++ b/drivers/input/keyboard/gpio_keys.c
> @@ -133,7 +133,7 @@ static void gpio_keys_quiesce_key(void *data)
>  		hrtimer_cancel(&bdata->release_timer);
>  	if (bdata->debounce_use_hrtimer)
>  		hrtimer_cancel(&bdata->debounce_timer);
> -	else
> +	else if (bdata->gpiod)
>  		cancel_delayed_work_sync(&bdata->work);

We already have a check for bdata->gpiod a couple lines above. I think
the chunk should look like this:

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

since we use debounce timer/work only when we deal with gpio-backed
keys.

Thanks.
Lad, Prabhakar May 19, 2022, 4:58 a.m. UTC | #3
Hi Dmitry,

Thank you for the review.

On Wed, May 18, 2022 at 5:46 AM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> Hi Lad,
>
> On Fri, May 13, 2022 at 02:25:00PM +0100, Lad Prabhakar wrote:
> > gpio_keys module can either accept gpios or interrupts. The module
> > initializes delayed work in case of gpios only and not for interrupts,
> > so make sure cancel_delayed_work_sync() is called only when bdata->gpiod
> > is true.
> ...
> > diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
> > index d75a8b179a8a..ec9d50ddda42 100644
> > --- a/drivers/input/keyboard/gpio_keys.c
> > +++ b/drivers/input/keyboard/gpio_keys.c
> > @@ -133,7 +133,7 @@ static void gpio_keys_quiesce_key(void *data)
> >               hrtimer_cancel(&bdata->release_timer);
> >       if (bdata->debounce_use_hrtimer)
> >               hrtimer_cancel(&bdata->debounce_timer);
> > -     else
> > +     else if (bdata->gpiod)
> >               cancel_delayed_work_sync(&bdata->work);
>
> We already have a check for bdata->gpiod a couple lines above. I think
> the chunk should look like this:
>
>         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);
>
> since we use debounce timer/work only when we deal with gpio-backed
> keys.
>
Agreed, will fix that in v2.

Cheers,
Prabhakar
diff mbox series

Patch

diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
index d75a8b179a8a..ec9d50ddda42 100644
--- a/drivers/input/keyboard/gpio_keys.c
+++ b/drivers/input/keyboard/gpio_keys.c
@@ -133,7 +133,7 @@  static void gpio_keys_quiesce_key(void *data)
 		hrtimer_cancel(&bdata->release_timer);
 	if (bdata->debounce_use_hrtimer)
 		hrtimer_cancel(&bdata->debounce_timer);
-	else
+	else if (bdata->gpiod)
 		cancel_delayed_work_sync(&bdata->work);
 }