[PATCH] Fix freeze in lm8333 i2c keyboard driver

[PATCH] Fix freeze in lm8333 i2c keyboard driver

[Tomas Mudrunka]

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;
-- 
2.40.0

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

[Jeff LaBundy]

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

[PATCH v2] Fix freeze in lm8333 i2c keyboard driver

[Tomas Mudrunka]

LM8333 uses gpio interrupt line which is activated by falling edge.
When button is pressed before driver is loaded,
driver will miss the edge and never respond again.
To fix this we clear the interrupt via i2c after registering IRQ.

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..9a810ca00 100644
--- a/drivers/input/keyboard/lm8333.c
+++ b/drivers/input/keyboard/lm8333.c
@@ -184,6 +184,8 @@ static int lm8333_probe(struct i2c_client *client)
 	if (err)
 		goto free_mem;
 
+	lm8333_read8(lm8333, LM8333_READ_INT);
+
 	err = input_register_device(input);
 	if (err)
 		goto free_irq;
-- 
2.40.0

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

[Jeff LaBundy]

Hi Tomas,

On Tue, Apr 25, 2023 at 06:49:03PM +0200, Tomas Mudrunka wrote:
> LM8333 uses gpio interrupt line which is activated by falling edge.
> When button is pressed before driver is loaded,
> driver will miss the edge and never respond again.
> To fix this we clear the interrupt via i2c after registering IRQ.
> 
> 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..9a810ca00 100644
> --- a/drivers/input/keyboard/lm8333.c
> +++ b/drivers/input/keyboard/lm8333.c
> @@ -184,6 +184,8 @@ static int lm8333_probe(struct i2c_client *client)
>  	if (err)
>  		goto free_mem;
>  
> +	lm8333_read8(lm8333, LM8333_READ_INT);
> +

This is the right idea. I am sort of splitting hairs here, however I
think it makes sense to place this read before the IRQ is requested
and not after.

As written, there is room for an ever-so-tiny race condition wherein
the IRQ is asserted just after it is requested. Before the threaded
handler has run however, the new read in probe swallows the IRQ status
before the threaded handler can read it and react to errors.

Also, I think you should at least capture and evaluate lm8333_read8()'s
return value as is already done for the calls to lm8333_write8().

>  	err = input_register_device(input);
>  	if (err)
>  		goto free_irq;
> -- 
> 2.40.0

Kind regards,
Jeff LaBundy

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

[Tomáš Mudruňka]

Hello, thanks for your notes.

> This is the right idea. I am sort of splitting hairs here, however I
> think it makes sense to place this read before the IRQ is requested
> and not after.
>
>
> As written, there is room for an ever-so-tiny race condition wherein
> the IRQ is asserted just after it is requested. Before the threaded
> handler has run however, the new read in probe swallows the IRQ status
> before the threaded handler can read it and react to errors.

In fact i believe quite the opposite case to be true.
If i read before registering IRQ there will be ever-so-tiny race condition that
would allow to miss the edge (exactly the bug this patch is fixing,
but limited).

In the case you describe the worst scenario is likely that the interrupt handler
will be called only to re-read status and immediately return on this condition:

if (!status) return IRQ_NONE;

> Also, I think you should at least capture and evaluate lm8333_read8()'s
> return value as is already done for the calls to lm8333_write8().

Well. If you think this will bring any benefits, i might as well just call
lm8333_irq_thread() instead of lm8333_read8()
Would that be acceptable solution?

Tom.

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

[Jeff LaBundy]

Hi Tomas,

On Thu, Apr 27, 2023 at 10:13:22AM +0200, Tomáš Mudruňka wrote:
> Hello, thanks for your notes.
> 
> > This is the right idea. I am sort of splitting hairs here, however I
> > think it makes sense to place this read before the IRQ is requested
> > and not after.
> >
> >
> > As written, there is room for an ever-so-tiny race condition wherein
> > the IRQ is asserted just after it is requested. Before the threaded
> > handler has run however, the new read in probe swallows the IRQ status
> > before the threaded handler can read it and react to errors.
> 
> In fact i believe quite the opposite case to be true.
> If i read before registering IRQ there will be ever-so-tiny race condition that
> would allow to miss the edge (exactly the bug this patch is fixing,
> but limited).

I thought the original problem is that the IRQ is already low by the time
the driver loads. Since a high-to-low transition (i.e. falling edge) is
never witnessed, the handler is never called to read the status and allow
the IRQ to go high again. Therefore, key events are gone forever.

The concern you mention is simply that of not responding to key events
until the interrupt handler is registered; there is no way around that.
Any event that occurs before then is off the table. Instead, we can only
make sure that none of those prior events place us in a bad state.

> 
> In the case you describe the worst scenario is likely that the interrupt handler
> will be called only to re-read status and immediately return on this condition:
> 
> if (!status) return IRQ_NONE;

Right, I am simply saying this is one key press that could have been
preserved. As a matter of principle, once the interrupt handler is live,
you should not disturb the precious read-on-clear registers on your own
without concurrency protection. It is much more common to clear suprious
interrupts and _then_ make the handler go live.

> 
> > Also, I think you should at least capture and evaluate lm8333_read8()'s
> > return value as is already done for the calls to lm8333_write8().
> 
> Well. If you think this will bring any benefits, i might as well just call
> lm8333_irq_thread() instead of lm8333_read8()
> Would that be acceptable solution?

Looking at the datasheet, it seems this devices builds up scan codes in
a FIFO. To protect against the rare case in which this dummy read includes
actual data, perhaps it is better to call lm8333_irq_thread() instead of
lm8333_read8() so that the FIFO is flushed.

> 
> Tom.

Kind regards,
Jeff LaBundy

[PATCH v3] Fix freeze in lm8333 i2c keyboard driver

[Tomas Mudrunka]

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 after 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..9a810ca00 100644
--- a/drivers/input/keyboard/lm8333.c
+++ b/drivers/input/keyboard/lm8333.c
@@ -184,6 +184,8 @@ static int lm8333_probe(struct i2c_client *client)
 	if (err)
 		goto free_mem;
 
+	lm8333_irq_thread(client->irq, (void *) lm8333);
+
 	err = input_register_device(input);
 	if (err)
 		goto free_irq;
-- 
2.40.0

[PATCH v4] Fix freeze in lm8333 i2c keyboard driver

[Tomas Mudrunka]

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 after 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..9a810ca00 100644
--- a/drivers/input/keyboard/lm8333.c
+++ b/drivers/input/keyboard/lm8333.c
@@ -184,6 +184,8 @@ static int lm8333_probe(struct i2c_client *client)
 	if (err)
 		goto free_mem;
 
+	lm8333_irq_thread(client->irq, lm8333);
+
 	err = input_register_device(input);
 	if (err)
 		goto free_irq;
-- 
2.40.0

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

[Jeff LaBundy]

Hi Tomas,

On Fri, Apr 28, 2023 at 12:20:15PM +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 after 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..9a810ca00 100644
> --- a/drivers/input/keyboard/lm8333.c
> +++ b/drivers/input/keyboard/lm8333.c
> @@ -184,6 +184,8 @@ static int lm8333_probe(struct i2c_client *client)
>  	if (err)
>  		goto free_mem;
>  
> +	lm8333_irq_thread(client->irq, lm8333);

Just to clarify, my stance is that this call should go _before_ the handler
is registered. Your earlier statement that doing so would steal any pending
status from the handler is correct; however, it is a moot point because the
handler cannot do anything with that status until the input device has been
registered anyway.

Any events that come before then are off the table, and this is OK because
user space isn't going to start consuming key events until well after this
driver has probed anyway.

The reason behind my assertion is that as a matter of best practice, you
should not have two asynchronous threads that can in theory access the same
register. You are correct that the handler would simply return IRQ_NONE in
such a race, but it sets a bad precedent and opens room for bugs in case
this driver is modified in the future. It also creates one unnecessary I2C
read.

This is why it is much more common to register the handler _after_ manually
accessing read-to-clear registers; the register access remains synchronous.
In case you feel I have misunderstood, please let me know.

> +
>  	err = input_register_device(input);
>  	if (err)
>  		goto free_irq;
> -- 
> 2.40.0

Kind regards,
Jeff LaBundy

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

[Tomáš Mudruňka]

> Just to clarify, my stance is that this call should go _before_ the handler
> is registered.

Ok, i will fix the patch later today.

> Any events that come before then are off the table, and this is OK because
> user space isn't going to start consuming key events until well after this
> driver has probed anyway.

Well, that was never my point. I don't care about capturing events
that happen before driver was properly loaded.
My only concern was to limit possibility of deadlock which happened previously.
Because that makes device unusable till the IC is power cycled.
Which might be especially annoying on devices that have power button
implemented using this exact IC :-)

> The reason behind my assertion is that as a matter of best practice, you
> should not have two asynchronous threads that can in theory access the same
> register.

Yeah, this makes bit more sense now. Didn't realized IRQ might
interrupt that lm8333_irq_thread() call immediately.
While not very likely to cause problems like deadlock of the driver, i
think it's a valid point.

After all this is what happens with IRQF_ONESHOT anyway right? Each
time the IRQ is triggered it's disabled, lm8333_irq_thread() is run
and then it's enabled immediately after that. So i guess the behaviour
on each keypress is very similar to calling lm8333_irq_thread() before
registering the IRQ handler, which gives me some confidence there
might not be huge chance for deadlock under normal circumstances.

Though i wonder what would happen if some EMI burst would create rapid
train of randomly timed keypresses, that might just hit the
unfortunate sweetspot after while... Might test that later in the lab,
since i really need the software to remain operational after such
condition had passed.

[PATCH v5] Fix freeze in lm8333 i2c keyboard driver

[Tomas Mudrunka]

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

[Jeff LaBundy]

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

[Dmitry Torokhov]

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

[Jeff LaBundy]

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

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

[Tomáš Mudruňka]

> 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

[Jeff LaBundy]

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

[Tomas Mudrunka]

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

[Jeff LaBundy]

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

[Tomas Mudrunka]

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

[Tomas Mudrunka]

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

Best regards. Tom

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

[Dmitry Torokhov]

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.

-- 
Dmitry

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@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

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

[Tomáš Mudruňka]

> 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.

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

[Jeff LaBundy]

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