Re: [PATCH 2/2] Input: tca8418_keypad - fix race condition in interrupt handler

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The patch fails to resolve the lost-interrupt race condition and introduces a 16-iteration busy loop due to misunderstanding the Write-1-to-Clear (W1C) interrupt status register.
- [High] Returning IRQ_HANDLED on I2C read failure bypasses clearing the interrupt, which could lead to an IRQ storm.
- [Medium] The outer loop and its 16-iteration limit are redundant and based on a flawed assumption about the FIFO reading logic.
--

commit b07bd14b2796c804f03cf9efdf000891fe041cc5
Author: Zhian Liang <liangzhan5dev@gmail.com>

Input: tca8418_keypad - fix race condition in interrupt handler

This patch modifies the tca8418 interrupt handler to re-read the
interrupt status register and loop if the interrupt bit is still set.
This is intended to prevent lost key events when new data arrives
after the FIFO is drained but before the interrupt status is cleared.

> diff --git a/drivers/input/keyboard/tca8418_keypad.c b/drivers/input/keyboard/tca8418_keypad.c
> index d917de848a213..be700137b0fb7 100644
> --- a/drivers/input/keyboard/tca8418_keypad.c
> +++ b/drivers/input/keyboard/tca8418_keypad.c

[ ... ]

> @@ -214,9 +215,19 @@ static irqreturn_t tca8418_irq_handler(int irq, void *dev_id)
>  
>  	if (reg & INT_STAT_OVR_FLOW_INT)
>  		dev_warn(&keypad_data->client->dev, "overflow occurred\n");
> +	do {
> +		if (reg & INT_STAT_K_INT)
> +			tca8418_read_keypad(keypad_data);
> +
> +		/* Re-read interrupt status to check for new events */
> +		error = tca8418_read_byte(keypad_data, REG_INT_STAT, &reg);
> +		if (error) {
> +			dev_err(&keypad_data->client->dev,
> +				"unable to re-read REG_INT_STAT\n");
> +			return IRQ_HANDLED;

[Severity: High]
If the I2C read fails inside the new loop, returning IRQ_HANDLED bypasses the
interrupt clear step at the end of the function. Could this leave the interrupt
line asserted in hardware? If the interrupt is level-triggered or shared,
might this cause an IRQ storm because the kernel is told the interrupt was
handled while it remains asserted?

> +		}
>  
> -	if (reg & INT_STAT_K_INT)
> -		tca8418_read_keypad(keypad_data);
> +	} while ((reg & INT_STAT_K_INT) && --max_loops);

[Severity: High]
The REG_INT_STAT register is Write-1-to-Clear. Because reading it does not
clear the interrupt bit by itself, wouldn't reg & INT_STAT_K_INT evaluate to
true on every iteration, causing the handler to always spin for the full
16-iteration limit on every valid interrupt?

[Severity: Medium]
Is the max_loops limit of 16 necessary to drain the 10-event FIFO? Looking at
tca8418_read_keypad(), it has its own do/while loop that continuously reads
until the FIFO reports empty. It seems a single call fully drains the FIFO,
making the outer loop based on FIFO depth redundant.

>  
>  	/* Clear all interrupts, even IRQs we didn't check (GPI, CAD, LCK) */
>  	reg = 0xff;
>  	error = tca8418_write_byte(keypad_data, REG_INT_STAT, reg);

[Severity: High]
Does this unconditional write of 0xff clear any new interrupts that triggered
just after the loop finished? If a new event sets the interrupt flag between
the end of the loop and this write, it looks like it will be cleared without
reading the corresponding event. Could this permanently drop events and
preserve the race condition this patch aims to fix?

>  	if (error)
>  		dev_err(&keypad_data->client->dev,
>  			"unable to clear REG_INT_STAT\n");

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260903192942.21906-1-liangzhan5dev@gmail.com?part=2