| Message ID | 20230503153231.1136114-1-tomas.mudrunka@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v5] Fix freeze in lm8333 i2c keyboard driver | expand |
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@gmail.com> > --- Reviewed-by: Jeff LaBundy <jeff@labundy.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..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); > + > err = request_threaded_irq(client->irq, NULL, lm8333_irq_thread, > IRQF_TRIGGER_FALLING | IRQF_ONESHOT, > "lm8333", lm8333); > -- > 2.40.1 Thank you for the productive discussion. Kind regards, Jeff LaBundy
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@gmail.com> > > --- > > Reviewed-by: Jeff LaBundy <jeff@labundy.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..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.
Hi Dmitry, On Thu, May 11, 2023 at 04:44:08PM -0700, Dmitry Torokhov wrote: > 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@gmail.com> > > > --- > > > > Reviewed-by: Jeff LaBundy <jeff@labundy.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..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. You're absolutely correct; I had not considered this corner case. Apologies for the churn Tomas. In that case, it seems the solution is to either move the dummy read after the handler is registered as in v4, or remove the hard-coded flag and allow dts to specify level sensitivity. > > > > + > > > 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
> 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) 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... Tom pá 12. 5. 2023 v 1:44 odesílatel Dmitry Torokhov <dmitry.torokhov@gmail.com> 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@gmail.com> > > > --- > > > > Reviewed-by: Jeff LaBundy <jeff@labundy.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..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
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@gmail.com> 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@gmail.com> > > > > --- > > > > > > Reviewed-by: Jeff LaBundy <jeff@labundy.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..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
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); + err = request_threaded_irq(client->irq, NULL, lm8333_irq_thread, IRQF_TRIGGER_FALLING | IRQF_ONESHOT, "lm8333", lm8333);
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@gmail.com> --- drivers/input/keyboard/lm8333.c | 2 ++ 1 file changed, 2 insertions(+)