Re: [PATCH v2 1/3] Input: samsung-keypad - clean up wakeup configuration logic

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The threaded IRQ handler accesses memory-mapped registers without ensuring the peripheral clock is enabled if a wakeup interrupt fires while the device is closed, leading to a kernel crash.
--

--- Patch [1]: [PATCH v2 1/3] Input: samsung-keypad - clean up wakeup configuration logic ---
commit c263408953e10fb025a3702953499e150bde4016
Author: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Input: samsung-keypad - clean up wakeup configuration logic
    
This commit refactors the wakeup configuration logic in the samsung-keypad
driver. It moves the device_may_wakeup check to the caller and adjusts the
ordering of register writes and interrupt configuration.

> diff --git a/drivers/input/keyboard/samsung-keypad.c b/drivers/input/keyboard/samsung-keypad.c
> index 17127269e3f05..a578f429d1009 100644
> --- a/drivers/input/keyboard/samsung-keypad.c
> +++ b/drivers/input/keyboard/samsung-keypad.c

[ ... ]

> @@ -516,7 +515,8 @@ static int samsung_keypad_suspend(struct device *dev)
>  	if (input_device_enabled(input_dev))
>  		samsung_keypad_stop(keypad);
>  
> -	samsung_keypad_toggle_wakeup(keypad, true);
> +	if (device_may_wakeup(dev))
> +		samsung_keypad_toggle_wakeup(keypad, true);
>  
>  	return 0;
>  }

[Severity: High]
This is a pre-existing issue, but could a wakeup interrupt firing while the
device is closed cause an unclocked register access and crash the system?

If the device is closed by all users, samsung_keypad_stop() sets
keypad->stopped to true. When the system suspends, the code above enables
wakeup if device_may_wakeup() returns true.

If a key press triggers a wakeup interrupt, upon resume the primary IRQ
handler schedules the threaded handler samsung_keypad_irq():

drivers/input/keyboard/samsung-keypad.c:samsung_keypad_irq() {
	...
	pm_runtime_get_sync(&keypad->pdev->dev);

	do {
		readl(keypad->base + SAMSUNG_KEYIFSTSCLR);
	...
}

However, pm_runtime_get_sync() calls samsung_keypad_runtime_resume(), which
skips enabling the clock because keypad->stopped is true:

drivers/input/keyboard/samsung-keypad.c:samsung_keypad_runtime_resume() {
	...
	if (keypad->stopped)
		return 0;

	clk_enable(keypad->clk);
	...
}

Will the subsequent readl() in samsung_keypad_irq() trigger a synchronous
external abort since the peripheral clock was never enabled?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260713-samsung-kp-irq-v2-0-acc84b6daf9a@gmail.com?part=1