Re: [PATCH v2] Fix freeze in lm8333 i2c keyboard driver

[Jeff LaBundy <jeff@labundy.com>]

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