| Message ID | 20180312225147.okm6w3gzaq7yt6ur@xylophone.i.decadent.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Mar 12, 2018 at 10:51:48PM +0000, Ben Hutchings wrote: > matrix_keypad_stop() now serialises with matrix_keypad_interrupt() > using spin_lock_irq() while setting the "stopped" flag, but it drops > the lock before disabling the interrupt sources. If an interrupt is > asserted at this point, matrix_keypad_interrupt() will ignore it > because stopped is true. The interrupt will remain asserted and it > may be called repeatedly, potentially leading to a hard lockup on a UP > system. No, it will not. We acknowledge interrupt (return IRQ_HANDLED) even when keypad is stopped, and because we are dealing with a bunch of GPIOs (and not some controller that has internal state and would not deassert the attention line until we read or otherwise clear the state) the interrupts will not be re-assered (unless there is a keypress, but even then we'll simply re-acknowledge interrupt again). > > Instead, matrix_keypad_stop() should disable the interrupt sources > (asynchronously) while still holding the lock. However, we have to > take care because at this point they may already be disabled due to a > pending scan. To avoid mismatched enable/disable calls, we should > maintain the invariant that they are enabled if and only if > !(scan_pending || stopped). Therefore: > > - Set the scan_pending flag in matrix_keypad_start(), since it does > schedule a scan and doesn't enable interrupts > - Only disable interrupts in matrix_keypad_stop() if scan_pending is > false > - Only enable interrupts in matrix_keypad_scan() if stopped is false > > Cc: stable@vger.kernel.org > Signed-off-by: Ben Hutchings <ben.hutchings@codethink.co.uk> > --- > This is untested and completely based on code review. It's possible > that the problem I identified can't really happen for reasons that > aren't obvious to me. In this case I'd appreciate if the patch was titled RFC instead of preemptively tagging it for stable. Thanks.
On Tue, 2018-03-13 at 11:09 -0700, Dmitry Torokhov wrote: > On Mon, Mar 12, 2018 at 10:51:48PM +0000, Ben Hutchings wrote: > > matrix_keypad_stop() now serialises with matrix_keypad_interrupt() > > using spin_lock_irq() while setting the "stopped" flag, but it drops > > the lock before disabling the interrupt sources. If an interrupt is > > asserted at this point, matrix_keypad_interrupt() will ignore it > > because stopped is true. The interrupt will remain asserted and it > > may be called repeatedly, potentially leading to a hard lockup on a UP > > system. > > No, it will not. We acknowledge interrupt (return IRQ_HANDLED) even when > keypad is stopped, and because we are dealing with a bunch of GPIOs (and > not some controller that has internal state and would not deassert the > attention line until we read or otherwise clear the state) the > interrupts will not be re-assered (unless there is a keypress, but even > then we'll simply re-acknowledge interrupt again). [...] Returning IRQ_HANDLED has no effect on the hardware; it's mostly used for spurious interrupt detection. But I think what you're saying that these interrupts will always be edge-triggered, not level-triggered - is that right? In which case I agree that screaming interrupts won't be a problem. Ben.
On Wed, Mar 14, 2018 at 04:17:33PM +0000, Ben Hutchings wrote: > On Tue, 2018-03-13 at 11:09 -0700, Dmitry Torokhov wrote: > > On Mon, Mar 12, 2018 at 10:51:48PM +0000, Ben Hutchings wrote: > > > matrix_keypad_stop() now serialises with matrix_keypad_interrupt() > > > using spin_lock_irq() while setting the "stopped" flag, but it drops > > > the lock before disabling the interrupt sources. If an interrupt is > > > asserted at this point, matrix_keypad_interrupt() will ignore it > > > because stopped is true. The interrupt will remain asserted and it > > > may be called repeatedly, potentially leading to a hard lockup on a UP > > > system. > > > > No, it will not. We acknowledge interrupt (return IRQ_HANDLED) even when > > keypad is stopped, and because we are dealing with a bunch of GPIOs (and > > not some controller that has internal state and would not deassert the > > attention line until we read or otherwise clear the state) the > > interrupts will not be re-assered (unless there is a keypress, but even > > then we'll simply re-acknowledge interrupt again). > [...] > > Returning IRQ_HANDLED has no effect on the hardware; it's mostly used > for spurious interrupt detection. Yes. You still need to acknowledge that interrupt was handled. I.e. if the keypad driver was returning 0 (IRQ_NONE) while it is "stopped", that would be wrong and the system could consider interrupt spurious one. > But I think what you're saying that > these interrupts will always be edge-triggered, not level-triggered - > is that right? In which case I agree that screaming interrupts won't > be a problem. Yes, level-triggered interrupts are not usable whenever something is based on GPIOs as while that "something" is active you'd have interrupt constantly retriggered as soon as you done processing it. We explicitly request interrupts as IRQG_TRIGGER_FALLING | IRQF_TRIGGER_RISING in matrix-keypad driver. Thanks.
On Wed, 2018-03-14 at 10:23 -0700, Dmitry Torokhov wrote: [...] > > But I think what you're saying that > > these interrupts will always be edge-triggered, not level-triggered - > > is that right? In which case I agree that screaming interrupts won't > > be a problem. > > Yes, level-triggered interrupts are not usable whenever something is > based on GPIOs as while that "something" is active you'd have interrupt > constantly retriggered as soon as you done processing it. > > We explicitly request interrupts as IRQG_TRIGGER_FALLING | > IRQF_TRIGGER_RISING in matrix-keypad driver. This is not enforced in the clustered IRQ case. Would it make sense to combine those with the specified clustered_irq_flags, or to mention this assumption in the struct matrix_keypad_platform_data doc-comment? Ben.
diff --git a/drivers/input/keyboard/matrix_keypad.c b/drivers/input/keyboard/matrix_keypad.c index 41614c185918..e765950d3491 100644 --- a/drivers/input/keyboard/matrix_keypad.c +++ b/drivers/input/keyboard/matrix_keypad.c @@ -169,7 +169,8 @@ static void matrix_keypad_scan(struct work_struct *work) /* Enable IRQs again */ spin_lock_irq(&keypad->lock); keypad->scan_pending = false; - enable_row_irqs(keypad); + if (!keypad->stopped) + enable_row_irqs(keypad); spin_unlock_irq(&keypad->lock); } @@ -203,6 +204,7 @@ static int matrix_keypad_start(struct input_dev *dev) struct matrix_keypad *keypad = input_get_drvdata(dev); keypad->stopped = false; + keypad->scan_pending = true; mb(); /* @@ -220,14 +222,16 @@ static void matrix_keypad_stop(struct input_dev *dev) spin_lock_irq(&keypad->lock); keypad->stopped = true; + if (!keypad->scan_pending) { + /* + * matrix_keypad_scan() will leave IRQs enabled; + * we should disable them now. + */ + disable_row_irqs(keypad); + } spin_unlock_irq(&keypad->lock); flush_work(&keypad->work.work); - /* - * matrix_keypad_scan() will leave IRQs enabled; - * we should disable them now. - */ - disable_row_irqs(keypad); } #ifdef CONFIG_PM_SLEEP
matrix_keypad_stop() now serialises with matrix_keypad_interrupt() using spin_lock_irq() while setting the "stopped" flag, but it drops the lock before disabling the interrupt sources. If an interrupt is asserted at this point, matrix_keypad_interrupt() will ignore it because stopped is true. The interrupt will remain asserted and it may be called repeatedly, potentially leading to a hard lockup on a UP system. Instead, matrix_keypad_stop() should disable the interrupt sources (asynchronously) while still holding the lock. However, we have to take care because at this point they may already be disabled due to a pending scan. To avoid mismatched enable/disable calls, we should maintain the invariant that they are enabled if and only if !(scan_pending || stopped). Therefore: - Set the scan_pending flag in matrix_keypad_start(), since it does schedule a scan and doesn't enable interrupts - Only disable interrupts in matrix_keypad_stop() if scan_pending is false - Only enable interrupts in matrix_keypad_scan() if stopped is false Cc: stable@vger.kernel.org Signed-off-by: Ben Hutchings <ben.hutchings@codethink.co.uk> --- This is untested and completely based on code review. It's possible that the problem I identified can't really happen for reasons that aren't obvious to me. Ben. drivers/input/keyboard/matrix_keypad.c | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-)