| Message ID | 20230425164903.610455-1-tomas.mudrunka@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] Fix freeze in lm8333 i2c keyboard driver | expand |
Hi Tomas, On Tue, Apr 25, 2023 at 06:49:03PM +0200, Tomas Mudrunka wrote: > LM8333 uses gpio interrupt line which is activated by falling edge. > When button is pressed before driver is loaded, > driver will miss the edge and never respond again. > To fix this we clear the interrupt via i2c after registering IRQ. > > Signed-off-by: Tomas Mudrunka <tomas.mudrunka@gmail.com> > --- > drivers/input/keyboard/lm8333.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/input/keyboard/lm8333.c b/drivers/input/keyboard/lm8333.c > index 7457c3220..9a810ca00 100644 > --- a/drivers/input/keyboard/lm8333.c > +++ b/drivers/input/keyboard/lm8333.c > @@ -184,6 +184,8 @@ static int lm8333_probe(struct i2c_client *client) > if (err) > goto free_mem; > > + lm8333_read8(lm8333, LM8333_READ_INT); > + This is the right idea. I am sort of splitting hairs here, however I think it makes sense to place this read before the IRQ is requested and not after. As written, there is room for an ever-so-tiny race condition wherein the IRQ is asserted just after it is requested. Before the threaded handler has run however, the new read in probe swallows the IRQ status before the threaded handler can read it and react to errors. Also, I think you should at least capture and evaluate lm8333_read8()'s return value as is already done for the calls to lm8333_write8(). > err = input_register_device(input); > if (err) > goto free_irq; > -- > 2.40.0 Kind regards, Jeff LaBundy
Hello, thanks for your notes. > This is the right idea. I am sort of splitting hairs here, however I > think it makes sense to place this read before the IRQ is requested > and not after. > > > As written, there is room for an ever-so-tiny race condition wherein > the IRQ is asserted just after it is requested. Before the threaded > handler has run however, the new read in probe swallows the IRQ status > before the threaded handler can read it and react to errors. In fact i believe quite the opposite case to be true. If i read before registering IRQ there will be ever-so-tiny race condition that would allow to miss the edge (exactly the bug this patch is fixing, but limited). In the case you describe the worst scenario is likely that the interrupt handler will be called only to re-read status and immediately return on this condition: if (!status) return IRQ_NONE; > Also, I think you should at least capture and evaluate lm8333_read8()'s > return value as is already done for the calls to lm8333_write8(). Well. If you think this will bring any benefits, i might as well just call lm8333_irq_thread() instead of lm8333_read8() Would that be acceptable solution? Tom.
Hi Tomas, On Thu, Apr 27, 2023 at 10:13:22AM +0200, Tomáš Mudruňka wrote: > Hello, thanks for your notes. > > > This is the right idea. I am sort of splitting hairs here, however I > > think it makes sense to place this read before the IRQ is requested > > and not after. > > > > > > As written, there is room for an ever-so-tiny race condition wherein > > the IRQ is asserted just after it is requested. Before the threaded > > handler has run however, the new read in probe swallows the IRQ status > > before the threaded handler can read it and react to errors. > > In fact i believe quite the opposite case to be true. > If i read before registering IRQ there will be ever-so-tiny race condition that > would allow to miss the edge (exactly the bug this patch is fixing, > but limited). I thought the original problem is that the IRQ is already low by the time the driver loads. Since a high-to-low transition (i.e. falling edge) is never witnessed, the handler is never called to read the status and allow the IRQ to go high again. Therefore, key events are gone forever. The concern you mention is simply that of not responding to key events until the interrupt handler is registered; there is no way around that. Any event that occurs before then is off the table. Instead, we can only make sure that none of those prior events place us in a bad state. > > In the case you describe the worst scenario is likely that the interrupt handler > will be called only to re-read status and immediately return on this condition: > > if (!status) return IRQ_NONE; Right, I am simply saying this is one key press that could have been preserved. As a matter of principle, once the interrupt handler is live, you should not disturb the precious read-on-clear registers on your own without concurrency protection. It is much more common to clear suprious interrupts and _then_ make the handler go live. > > > Also, I think you should at least capture and evaluate lm8333_read8()'s > > return value as is already done for the calls to lm8333_write8(). > > Well. If you think this will bring any benefits, i might as well just call > lm8333_irq_thread() instead of lm8333_read8() > Would that be acceptable solution? Looking at the datasheet, it seems this devices builds up scan codes in a FIFO. To protect against the rare case in which this dummy read includes actual data, perhaps it is better to call lm8333_irq_thread() instead of lm8333_read8() so that the FIFO is flushed. > > Tom. Kind regards, Jeff LaBundy
diff --git a/drivers/input/keyboard/lm8333.c b/drivers/input/keyboard/lm8333.c index 7457c3220..9a810ca00 100644 --- a/drivers/input/keyboard/lm8333.c +++ b/drivers/input/keyboard/lm8333.c @@ -184,6 +184,8 @@ static int lm8333_probe(struct i2c_client *client) if (err) goto free_mem; + lm8333_read8(lm8333, LM8333_READ_INT); + err = input_register_device(input); if (err) goto free_irq;
LM8333 uses gpio interrupt line which is activated by falling edge. When button is pressed before driver is loaded, driver will miss the edge and never respond again. To fix this we clear the interrupt via i2c after registering IRQ. Signed-off-by: Tomas Mudrunka <tomas.mudrunka@gmail.com> --- drivers/input/keyboard/lm8333.c | 2 ++ 1 file changed, 2 insertions(+)