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

[Jeff LaBundy]

Hi Dmitry,

On Wed, Apr 26, 2023 at 04:16:01PM -0700, Dmitry Torokhov wrote:
> Hi Jeff, Tomas,
> 
> On Tue, Apr 25, 2023 at 10:39:49AM -0500, Jeff LaBundy wrote:
> > Hi Tomas,
> > 
> > On Tue, Apr 25, 2023 at 03:00:53PM +0200, Tomas Mudrunka wrote:
> > > LM8333 uses gpio interrupt line which is active-low.
> > > When interrupt is set to FALLING edge and button is pressed
> > > before driver loads, driver will miss the edge and never respond.
> > > To fix this we handle ONESHOT LOW interrupt rather than edge.
> > > 
> > > Signed-off-by: Tomas Mudrunka <tomas.mudrunka@xxxxxxxxx>
> > > ---
> > >  drivers/input/keyboard/lm8333.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/input/keyboard/lm8333.c b/drivers/input/keyboard/lm8333.c
> > > index 7457c3220..c5770ebb2 100644
> > > --- a/drivers/input/keyboard/lm8333.c
> > > +++ b/drivers/input/keyboard/lm8333.c
> > > @@ -179,7 +179,7 @@ static int lm8333_probe(struct i2c_client *client)
> > >  	}
> > >  
> > >  	err = request_threaded_irq(client->irq, NULL, lm8333_irq_thread,
> > > -				   IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> > > +				   IRQF_TRIGGER_LOW | IRQF_ONESHOT,
> > >  				   "lm8333", lm8333);
> > >  	if (err)
> > >  		goto free_mem;
> > 
> > Thanks for the patch, but this is a NAK in my opinion.
> > 
> > First of all, we should not be hard-coding interrupt polarity in the
> > first place; that is an existing piece of technical debt in this driver.
> 
> Yes, I wonder if the original hardware was limited to the edge
> interrupts.
> 
> > 
> > Second, changing from edge-triggered to level-triggered interrupts runs
> > the risk of creating an interrupt storm depending on the time it takes
> > the device to deassert the irq following the I2C read and the point at
> > which the threaded handler returns. Have you measured this?
> 
> IRQF_ONESHOT ensures that the level interrupt is unmasked only when the
> threaded handler returns.

Yes that's correct; what I mean to say is that depending on the nature of
the read-to-clear mechanism in the part, there is a chance that the IRQ
has not been deasserted by the time the threaded handler returns. On some
devices for example, the IRQ is not deasserted until some time after the
read's stop condition.

For these cases, I consider it best practice to measure the I2C and IRQ
lines on a scope and if necessary, add a small delay before the interrupt
handler returns. This is especially true for open-drain interrupts that
may need a few hundred extra us for the pin to rise.

> 
> > 
> > Can we not simply read the interrupt status registers once at start-up
> > to clear any pending status? This is essentially what your change does
> > anyway, albeit indirectly.
> > 
> 
> Thanks.
> 
> -- 
> Dmitry

Kind regards,
Jeff LaBundy