| Message ID | 20250526-gpio_keys_preempt_rt-v1-1-09ddadf8e19d@foss.st.com |
|---|---|
| State | New |
| Headers | show |
| Series | Input: gpio-keys - fix a sleep while atomic with PREEMPT_RT | expand |
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
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
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
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
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;