[v5] Fix freeze in lm8333 i2c keyboard driver

[PATCH v5] Fix freeze in lm8333 i2c keyboard driver

Posted by Tomas Mudrunka 2 years, 9 months ago

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(+)

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

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

Posted by Jeff LaBundy 2 years, 9 months ago

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

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

Posted by Dmitry Torokhov 2 years, 9 months ago

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

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

Posted by Tomáš Mudruňka 2 years, 9 months ago

> 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

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

Posted by Jeff LaBundy 2 years, 9 months ago

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

[PATCH v6] Fix freeze in lm8333 i2c keyboard driver

Posted by Tomas Mudrunka 2 years, 2 months ago

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 should handle ONESHOT LOW interrupt rather than edge.

Rather than hardcoding this, we simply remove the override from
driver by calling request_threaded_irq() with IRQF_TRIGGER_NONE flag.
This will keep interrupt trigger configuration as per devicetree. eg.:

	lm8333@51 {
		compatible = "ti,lm8333";
		interrupt-parent = <&gpio1>;
		interrupts = <12 IRQ_TYPE_LEVEL_LOW>;
		...
	}

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_NONE | IRQF_ONESHOT,
 				   "lm8333", lm8333);
 	if (err)
 		goto free_mem;
-- 
2.40.0

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

Posted by Jeff LaBundy 2 years, 1 month ago

Hi Tomas,

On Tue, Nov 14, 2023 at 01:30:23PM +0100, 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 should handle ONESHOT LOW interrupt rather than edge.
> 
> Rather than hardcoding this, we simply remove the override from
> driver by calling request_threaded_irq() with IRQF_TRIGGER_NONE flag.
> This will keep interrupt trigger configuration as per devicetree. eg.:
> 
> 	lm8333@51 {
> 		compatible = "ti,lm8333";
> 		interrupt-parent = <&gpio1>;
> 		interrupts = <12 IRQ_TYPE_LEVEL_LOW>;
> 		...
> 	}
> 
> 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_NONE | IRQF_ONESHOT,

This seems like the best approach; it solves the original problem, and
adopts the correct design pattern of allowing the dts to specify details
about the interrupt polarity and sensitivity.

My only feedback is that I think you can simply drop IRQF_TRIGGER_FALLING
altogether instead of replacing it with IRQF_TRIGGER_NONE; it is pointless
to bitwise OR against zero, and almost no drivers do this. It really should
only be used unless there are quite literally no flags to use. Passing only
IRQF_ONESHOT is sufficient here.

Assuming you agree with this change, please feel free to add the following
for v7:

Reviewed-by: Jeff LaBundy <jeff@labundy.com>

>  				   "lm8333", lm8333);
>  	if (err)
>  		goto free_mem;
> -- 
> 2.40.0

Kind regards,
Jeff LaBundy

[PATCH v7] Fix freeze in lm8333 i2c keyboard driver

Posted by Tomas Mudrunka 2 years, 1 month ago

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 should handle ONESHOT LOW interrupt rather than edge.

Rather than hardcoding this, we simply remove the override from
driver by calling request_threaded_irq() without specifying trigger.
This will keep interrupt trigger configuration as per devicetree. eg.:

	lm8333@51 {
		compatible = "ti,lm8333";
		interrupt-parent = <&gpio1>;
		interrupts = <12 IRQ_TYPE_LEVEL_LOW>;
		...
	}

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, 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_ONESHOT,
 				   "lm8333", lm8333);
 	if (err)
 		goto free_mem;
-- 
2.40.0

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

Posted by Tomas Mudrunka 11 months, 3 weeks ago

Hi guys! Been a while. Is there anything else blocking this
from being merged?

Best regards. Tom

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

Posted by Jeff LaBundy 2 years, 9 months ago

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