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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZXpnbifDOQ/eF5jb@nixie71>
Date:   Wed, 13 Dec 2023 20:24:46 -0600
From:   Jeff LaBundy <jeff@...undy.com>
To:     Tomas Mudrunka <tomas.mudrunka@...il.com>
Cc:     dmitry.torokhov@...il.com, linux-input@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v6] Fix freeze in lm8333 i2c keyboard driver

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@...il.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@...undy.com>

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

Kind regards,
Jeff LaBundy