| Message ID | 20230425130054.591007-1-tomas.mudrunka@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Fix freeze in lm8333 i2c keyboard driver | expand |
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@gmail.com> > --- > 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. 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? 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. > -- > 2.40.0 > Kind regards, Jeff LaBundy
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@gmail.com> > > --- > > 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. > > 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.
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@gmail.com> > > > --- > > > 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
> 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. Well before posting the patch i did some testing. I was watching the /proc/interrupts and checked that IRQ counter for lm8333 matches number of keypresses. Which i've only tested for like 20-30 times, but haven't seem any glitch. But i still recognize the fact that the gpio line getting stuck for some reason (short circuit on PCB?) might cause troubles by unnecessarily loading the CPU, while with edge trigger it's more likely to affect only the function of keyboard itself rather than bringing down whole system. But i am not sure if this case is supposed to be expected and handled in SW.
Hi Tomas, On Thu, Apr 27, 2023 at 10:19:38AM +0200, Tomáš Mudruňka wrote: > > 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. > > Well before posting the patch i did some testing. > I was watching the /proc/interrupts and checked that IRQ counter for > lm8333 matches number of keypresses. > Which i've only tested for like 20-30 times, but haven't seem any glitch. > > But i still recognize the fact that the gpio line getting stuck for > some reason (short circuit on PCB?) might cause troubles by > unnecessarily loading the CPU, while with edge trigger it's more > likely to affect only the function of keyboard itself rather than > bringing down whole system. But i am not sure if this case is supposed > to be expected and handled in SW. In the case of short circuit, the hardware has failed and there is nothing we can do. My point is that different devices deassert their IRQ at different points in the read-to-clear operation. For some devices, the IRQ is deasserted immediately after the register address is latched, so we can be confident that the IRQ has already gone high by the time the I2C read operation and hence the interrupt handler return. On others however, the IRQ may still remain low for 10's or even 100's of us after the read is complete. In some cases, the threaded handler could have already returned by then. Since you did not find any unexplained IRQ counts, perhaps that is not the case for this device. Kind regards, Jeff LaBundy
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;
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@gmail.com> --- drivers/input/keyboard/lm8333.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)