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@xxxxxxxxxxxxxxx > Signed-off-by: Ben Hutchings <ben.hutchings@xxxxxxxxxxxxxxx> > --- > 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. -- Dmitry -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
