Hi Tomas, On Fri, May 12, 2023 at 06:55:08PM +0200, Tomáš Mudruňka wrote: > > So this is still racy, isn't it? The interrupt may come after read is > > done, but before we register the handler. > > Well. It is. But please see the rest of the thread, where we've > already discussed this. > > Every time the interrupt handler runs, the interrupt is disabled and > then reenabled after i2c communication is done. Which means this exact > thing happens on each keypress anyway. So i don't think it's a > necessarily huge deal. It might not be perfect solution, but it makes > things much better. window in which the deadlock condition can happen > is now in range of few ms (or us), instead of ~10 seconds (previously > it included bootloader and basicaly any moment from power up to driver > load) Right, but the point is that there are some alternatives to reduce the range to zero. You posted one already, but I mistakenly advised against it due to my own oversight :) > > Another solution would be to trigger on LOW instead of FALLING as > proposed in initial version of the patch. That would be safer in terms > of lm8333 deadlock, but Jeff was concerned about possibility of > interrupt storm taking down whole system in case the IRQ line gets > stuck in LOW for some reason... Just to clarify, this is not my concern; all bets are off in case of gross hardware failure such as this. Rather, my recommendations are: 1. Level (or edge) sensitivity should be specified in dts, not hard-coded in the driver. 2. If you open support for level-triggered interrupts, you should verify on a scope whether there is any chance that the IRQ line may still be in the process of rising at the moment the read is completed. The datasheet is ambiguous here. > > Tom > > pá 12. 5. 2023 v 1:44 odesílatel Dmitry Torokhov > <dmitry.torokhov@xxxxxxxxx> napsal: > > > > On Wed, May 03, 2023 at 08:44:06PM -0500, Jeff LaBundy wrote: > > > Hi Tomas, > > > > > > On Wed, May 03, 2023 at 05:32:31PM +0200, Tomas Mudrunka wrote: > > > > LM8333 uses gpio interrupt line which is triggered by falling edge. > > > > When button is pressed before driver is loaded, > > > > driver will miss the edge and never respond again. > > > > To fix this we run the interrupt handler before registering IRQ > > > > to clear the interrupt via i2c command. > > > > > > > > Signed-off-by: Tomas Mudrunka <tomas.mudrunka@xxxxxxxxx> > > > > --- > > > > > > Reviewed-by: Jeff LaBundy <jeff@xxxxxxxxxxx> > > > > > > > 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..52108c370 100644 > > > > --- a/drivers/input/keyboard/lm8333.c > > > > +++ b/drivers/input/keyboard/lm8333.c > > > > @@ -178,6 +178,8 @@ static int lm8333_probe(struct i2c_client *client) > > > > dev_warn(&client->dev, "Unable to set active time\n"); > > > > } > > > > > > > > + lm8333_irq_thread(client->irq, lm8333); > > > > So this is still racy, isn't it? The interrupt may come after read is > > done, but before we register the handler. > > > > > > + > > > > err = request_threaded_irq(client->irq, NULL, lm8333_irq_thread, > > > > IRQF_TRIGGER_FALLING | IRQF_ONESHOT, > > > > "lm8333", lm8333); > > > > -- > > > > 2.40.1 > > > > > > > Thanks. > > > > -- > > Dmitry Kind regards, Jeff LaBundy
