Re: [PATCH 2/3] Input: samsung-keypad - keep interrupt disabled while closed

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] The patch unconditionally clears the pending hardware interrupt during system resume even when the keypad is open, discarding legitimate wakeup key events.

Pre-existing issues:
- [High] Missing check for pm_runtime_get_sync() return value in hardware access paths leads to clock-gated hardware access.
--

--- Patch [2]: [PATCH 2/3] Input: samsung-keypad - keep interrupt disabled while closed ---
commit 4f3c0f99ee50c72647a3ac611009b6d19d9c230d
Author: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Input: samsung-keypad - keep interrupt disabled while closed

The driver requests the interrupt during probe, which by default enables
it. If the bootloader left the keypad interrupts enabled, or if a
spurious interrupt fires early before the driver is fully initialized
and clocks are enabled, the interrupt handler will attempt to read
registers and may cause a synchronous external abort. Fix this by
requesting the interrupt with IRQF_NO_AUTOEN, keeping it disabled during
probe.

> diff --git a/drivers/input/keyboard/samsung-keypad.c b/drivers/input/keyboard/samsung-keypad.c
> index a578f429d1009..88e733c1eabea 100644
> --- a/drivers/input/keyboard/samsung-keypad.c
> +++ b/drivers/input/keyboard/samsung-keypad.c
> @@ -183,6 +183,8 @@ static void samsung_keypad_start(struct samsung_keypad *keypad)
>  	writel(0, keypad->base + SAMSUNG_KEYIFCOL);
>  
>  	pm_runtime_put(&keypad->pdev->dev);

[Severity: High]
This is a pre-existing issue, but I noticed that earlier in
samsung_keypad_start() (as well as in samsung_keypad_stop() and
samsung_keypad_irq()), we seem to call pm_runtime_get_sync() without
checking its return value.

If runtime PM fails to resume the device (for example, due to a power
domain failure), the device clock remains disabled.

Could this lead to a kernel panic from a synchronous external abort when
the driver subsequently accesses hardware registers via readl() and writel()
on disabled hardware?

> +
> +	enable_irq(keypad->irq);
>  }

[ ... ]

> @@ -499,6 +496,9 @@ static void samsung_keypad_toggle_wakeup(struct samsung_keypad *keypad,
>  		val &= ~SAMSUNG_KEYIFCON_WAKEUPEN;
>  		writel(val, keypad->base + SAMSUNG_KEYIFCON);
>  		disable_irq_wake(keypad->irq);
> +
> +		if (keypad->stopped)
> +			writel(~0x0, keypad->base + SAMSUNG_KEYIFSTSCLR);

[Severity: High]
Does this unconditionally clear the pending hardware interrupt during system
resume, even for open devices, potentially discarding legitimate wakeup
events?

During system suspend, samsung_keypad_suspend() stops open devices, which
sets keypad->stopped = true:

samsung_keypad_suspend()
  if (input_device_enabled(input_dev))
    samsung_keypad_stop(keypad);

On resume, samsung_keypad_resume() calls samsung_keypad_toggle_wakeup(..., false)
before restarting the device:

samsung_keypad_resume()
  if (device_may_wakeup(dev))
    samsung_keypad_toggle_wakeup(keypad, false);
  if (input_device_enabled(input_dev))
    samsung_keypad_start(keypad);

Because keypad->stopped is still true here for both closed and open devices,
the if (keypad->stopped) condition evaluates to true.

Will this execute writel(~0x0, keypad->base + SAMSUNG_KEYIFSTSCLR) and
silently discard the keystroke that woke the system before the IRQ is
re-enabled by samsung_keypad_start()?

>  	}
>  
>  	clk_disable(keypad->clk);

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