Input: gpio-keys - fix a sleep while atomic with PREEMPT_RT

[PATCH] Input: gpio-keys - fix a sleep while atomic with PREEMPT_RT

Posted by Gatien Chevallier 6 months, 3 weeks ago

From: Fabrice Gasnier <fabrice.gasnier@foss.st.com>

When enabling PREEMPT_RT, the gpio_keys_irq_timer() callback runs in
hard irq context, but the input_event() takes a spin_lock, which isn't
allowed there as it is converted to a rt_spin_lock().

[ 4054.289999] BUG: sleeping function called from invalid context at kernel/locking/spinlock_rt.c:48
[ 4054.290028] in_atomic(): 1, irqs_disabled(): 1, non_block: 0, pid: 0, name: swapper/0
...
[ 4054.290195]  __might_resched+0x13c/0x1f4
[ 4054.290209]  rt_spin_lock+0x54/0x11c
[ 4054.290219]  input_event+0x48/0x80
[ 4054.290230]  gpio_keys_irq_timer+0x4c/0x78
[ 4054.290243]  __hrtimer_run_queues+0x1a4/0x438
[ 4054.290257]  hrtimer_interrupt+0xe4/0x240
[ 4054.290269]  arch_timer_handler_phys+0x2c/0x44
[ 4054.290283]  handle_percpu_devid_irq+0x8c/0x14c
[ 4054.290297]  handle_irq_desc+0x40/0x58
[ 4054.290307]  generic_handle_domain_irq+0x1c/0x28
[ 4054.290316]  gic_handle_irq+0x44/0xcc

Considering the gpio_keys_irq_isr() can run in any context, e.g. it can
be threaded, it seems there's no point in requesting the timer isr to
run in hard irq context.

So relax the hrtimer not to use the hard context. This requires the
spin_lock to be added back in gpio_keys_irq_timer().

Fixes: 019002f20cb5 ("Input: gpio-keys - use hrtimer for release timer")
Suggested-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
Signed-off-by: Gatien Chevallier <gatien.chevallier@foss.st.com>
---
 drivers/input/keyboard/gpio_keys.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
index 5c39a217b94c8ad03a8542380eed741fccdca5da..99fc7d3c29ea5809604915782525fecacb151612 100644
--- a/drivers/input/keyboard/gpio_keys.c
+++ b/drivers/input/keyboard/gpio_keys.c
@@ -448,12 +448,15 @@ static enum hrtimer_restart gpio_keys_irq_timer(struct hrtimer *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_report_key(input, *bdata->code, 0);
 		input_sync(input);
 		bdata->key_pressed = false;
 	}
+	spin_unlock_irqrestore(&bdata->lock, flags);
 
 	return HRTIMER_NORESTART;
 }
@@ -486,7 +489,7 @@ static irqreturn_t gpio_keys_irq_isr(int irq, void *dev_id)
 	if (bdata->release_delay)
 		hrtimer_start(&bdata->release_timer,
 			      ms_to_ktime(bdata->release_delay),
-			      HRTIMER_MODE_REL_HARD);
+			      HRTIMER_MODE_REL);
 out:
 	return IRQ_HANDLED;
 }
@@ -628,7 +631,7 @@ static int gpio_keys_setup_key(struct platform_device *pdev,
 
 		bdata->release_delay = button->debounce_interval;
 		hrtimer_setup(&bdata->release_timer, gpio_keys_irq_timer,
-			      CLOCK_REALTIME, HRTIMER_MODE_REL_HARD);
+			      CLOCK_REALTIME, HRTIMER_MODE_REL);
 
 		isr = gpio_keys_irq_isr;
 		irqflags = 0;

---
base-commit: 0ff41df1cb268fc69e703a08a57ee14ae967d0ca
change-id: 20250526-gpio_keys_preempt_rt-10619c8fa916

Best regards,
-- 
Gatien Chevallier <gatien.chevallier@foss.st.com>

Re: [PATCH] Input: gpio-keys - fix a sleep while atomic with PREEMPT_RT

Posted by Sebastian Andrzej Siewior 6 months, 3 weeks ago

On 2025-05-26 15:56:29 [+0200], Gatien Chevallier wrote:
> From: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
> 
> When enabling PREEMPT_RT, the gpio_keys_irq_timer() callback runs in
> hard irq context, but the input_event() takes a spin_lock, which isn't
> allowed there as it is converted to a rt_spin_lock().
> 
> [ 4054.289999] BUG: sleeping function called from invalid context at kernel/locking/spinlock_rt.c:48
> [ 4054.290028] in_atomic(): 1, irqs_disabled(): 1, non_block: 0, pid: 0, name: swapper/0
> ...
> [ 4054.290195]  __might_resched+0x13c/0x1f4
> [ 4054.290209]  rt_spin_lock+0x54/0x11c
> [ 4054.290219]  input_event+0x48/0x80
> [ 4054.290230]  gpio_keys_irq_timer+0x4c/0x78
> [ 4054.290243]  __hrtimer_run_queues+0x1a4/0x438
> [ 4054.290257]  hrtimer_interrupt+0xe4/0x240
> [ 4054.290269]  arch_timer_handler_phys+0x2c/0x44
> [ 4054.290283]  handle_percpu_devid_irq+0x8c/0x14c
> [ 4054.290297]  handle_irq_desc+0x40/0x58
> [ 4054.290307]  generic_handle_domain_irq+0x1c/0x28
> [ 4054.290316]  gic_handle_irq+0x44/0xcc
> 
> Considering the gpio_keys_irq_isr() can run in any context, e.g. it can
> be threaded, it seems there's no point in requesting the timer isr to
> run in hard irq context.
> 
> So relax the hrtimer not to use the hard context. This requires the
> spin_lock to be added back in gpio_keys_irq_timer().

Why does it? This needs to be explained or it deserves an independent
patch/ fix. This flag change makes not difference on !PREEMPT_RT and so
should be the requirements for locking here.

> Fixes: 019002f20cb5 ("Input: gpio-keys - use hrtimer for release timer")
> Suggested-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
> Signed-off-by: Gatien Chevallier <gatien.chevallier@foss.st.com>

Sebastian

Re: [PATCH] Input: gpio-keys - fix a sleep while atomic with PREEMPT_RT

Posted by Gatien CHEVALLIER 6 months, 3 weeks ago

Hello Sebastian,

On 5/26/25 16:13, Sebastian Andrzej Siewior wrote:
> On 2025-05-26 15:56:29 [+0200], Gatien Chevallier wrote:
>> From: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
>>
>> When enabling PREEMPT_RT, the gpio_keys_irq_timer() callback runs in
>> hard irq context, but the input_event() takes a spin_lock, which isn't
>> allowed there as it is converted to a rt_spin_lock().
>>
>> [ 4054.289999] BUG: sleeping function called from invalid context at kernel/locking/spinlock_rt.c:48
>> [ 4054.290028] in_atomic(): 1, irqs_disabled(): 1, non_block: 0, pid: 0, name: swapper/0
>> ...
>> [ 4054.290195]  __might_resched+0x13c/0x1f4
>> [ 4054.290209]  rt_spin_lock+0x54/0x11c
>> [ 4054.290219]  input_event+0x48/0x80
>> [ 4054.290230]  gpio_keys_irq_timer+0x4c/0x78
>> [ 4054.290243]  __hrtimer_run_queues+0x1a4/0x438
>> [ 4054.290257]  hrtimer_interrupt+0xe4/0x240
>> [ 4054.290269]  arch_timer_handler_phys+0x2c/0x44
>> [ 4054.290283]  handle_percpu_devid_irq+0x8c/0x14c
>> [ 4054.290297]  handle_irq_desc+0x40/0x58
>> [ 4054.290307]  generic_handle_domain_irq+0x1c/0x28
>> [ 4054.290316]  gic_handle_irq+0x44/0xcc
>>
>> Considering the gpio_keys_irq_isr() can run in any context, e.g. it can
>> be threaded, it seems there's no point in requesting the timer isr to
>> run in hard irq context.
>>
>> So relax the hrtimer not to use the hard context. This requires the
>> spin_lock to be added back in gpio_keys_irq_timer().
> 
> Why does it? This needs to be explained or it deserves an independent
> patch/ fix. This flag change makes not difference on !PREEMPT_RT and so
> should be the requirements for locking here.
> 

Can you elaborate on "This flag change makes not difference on
!PREEMPT_RT" please? IIUC,this makes the callback not run in hard IRQ
context, even in !PREEMPT_RT, no?

Regarding the need of the spin_lock: gpio_keys_irq_timer() and
gpio_keys_irq_isr() appear to access the same resources. Can't we
have a concurrent access on it from:
HR timer interrupt // GPIO interrupt?

But looking back at the patch, this situation does not depend on
the HRTIMER_MODE_REL_HARD flag. So maybe it should be addressed
separately.

On the other hand, I should use the new
guard(spinlock_irqsave)(&bdata->lock);

>> Fixes: 019002f20cb5 ("Input: gpio-keys - use hrtimer for release timer")
>> Suggested-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
>> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
>> Signed-off-by: Gatien Chevallier <gatien.chevallier@foss.st.com>
> 
> Sebastian

Re: [PATCH] Input: gpio-keys - fix a sleep while atomic with PREEMPT_RT

Posted by Sebastian Andrzej Siewior 6 months, 3 weeks ago

On 2025-05-27 15:36:37 [+0200], Gatien CHEVALLIER wrote:
> Hello Sebastian,
Hello Gatien,

> Can you elaborate on "This flag change makes not difference on
> !PREEMPT_RT" please? IIUC,this makes the callback not run in hard IRQ
> context, even in !PREEMPT_RT, no?

If you set
- HRTIMER_MODE_REL_HARD
  then the callback runs in
  - hardirq context on !PREEMPT_RT
  - hardirq context on PREEMPT_RT.

- HRTIMER_MODE_REL
  then the callback runs in
  - hardirq context on !PREEMPT_RT
  - preemptible softirq on PREEMPT_RT.

- HRTIMER_MODE_REL_SOFT
  then the callback runs in
  - softirq context on !PREEMPT_RT
  - preemptible softirq on PREEMPT_RT.

Therefore if you switch HRTIMER_MODE_REL_HARD -> HRTIMER_MODE_REL then
it is a nop on !PREEMPT_RT.

> Regarding the need of the spin_lock: gpio_keys_irq_timer() and
> gpio_keys_irq_isr() appear to access the same resources. Can't we
> have a concurrent access on it from:
> HR timer interrupt // GPIO interrupt?

Yes, it could.

> But looking back at the patch, this situation does not depend on
> the HRTIMER_MODE_REL_HARD flag. So maybe it should be addressed
> separately.

Yes, please.

> On the other hand, I should use the new
> guard(spinlock_irqsave)(&bdata->lock);

Yes, please. The other instance already does so.

Sebastian

Re: [PATCH] Input: gpio-keys - fix a sleep while atomic with PREEMPT_RT

Posted by Gatien CHEVALLIER 6 months, 3 weeks ago

Hello Sebastian,

On 5/27/25 16:41, Sebastian Andrzej Siewior wrote:
> On 2025-05-27 15:36:37 [+0200], Gatien CHEVALLIER wrote:
>> Hello Sebastian,
> Hello Gatien,
> 
>> Can you elaborate on "This flag change makes not difference on
>> !PREEMPT_RT" please? IIUC,this makes the callback not run in hard IRQ
>> context, even in !PREEMPT_RT, no?
> 
> If you set
> - HRTIMER_MODE_REL_HARD
>    then the callback runs in
>    - hardirq context on !PREEMPT_RT
>    - hardirq context on PREEMPT_RT.
> 
> - HRTIMER_MODE_REL
>    then the callback runs in
>    - hardirq context on !PREEMPT_RT
>    - preemptible softirq on PREEMPT_RT.
> 
> - HRTIMER_MODE_REL_SOFT
>    then the callback runs in
>    - softirq context on !PREEMPT_RT
>    - preemptible softirq on PREEMPT_RT.
> 
> Therefore if you switch HRTIMER_MODE_REL_HARD -> HRTIMER_MODE_REL then
> it is a nop on !PREEMPT_RT.
> 

Thank you for the details.

>> Regarding the need of the spin_lock: gpio_keys_irq_timer() and
>> gpio_keys_irq_isr() appear to access the same resources. Can't we
>> have a concurrent access on it from:
>> HR timer interrupt // GPIO interrupt?
> 
> Yes, it could.
> 
>> But looking back at the patch, this situation does not depend on
>> the HRTIMER_MODE_REL_HARD flag. So maybe it should be addressed
>> separately.
> 
> Yes, please.
> 

Ok, I will do that in V2

>> On the other hand, I should use the new
>> guard(spinlock_irqsave)(&bdata->lock);
> 
> Yes, please. The other instance already does so.
> 
> Sebastian

Ok

Best regards,
Gatien