Re: [PATCH 3/3] Input: samsung-keypad - use pm_runtime_active guard

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing Error Handling for pm_runtime_get_sync() implicitly called by guard(pm_runtime_active)
--

--- Patch [3]: [PATCH 3/3] Input: samsung-keypad - use pm_runtime_active guard ---
commit dabb4cc7edb73abb241db678d97750949e93ef10
Author: Dmitry Torokhov <dmitry.torokhov@gmail.com>

    Input: samsung-keypad - use pm_runtime_active guard
    
    Simplify the driver by using the block-scope guard(pm_runtime_active)
    instead of manually invoking pm_runtime_get_sync() and
    pm_runtime_put().

> diff --git a/drivers/input/keyboard/samsung-keypad.c b/drivers/input/keyboard/samsung-keypad.c
> index 88e733c1eabea..5f6535b49761c 100644
> --- a/drivers/input/keyboard/samsung-keypad.c
> +++ b/drivers/input/keyboard/samsung-keypad.c
> @@ -142,7 +142,7 @@ static irqreturn_t samsung_keypad_irq(int irq, void *dev_id)
>  	unsigned int row_state[SAMSUNG_MAX_COLS];
>  	bool key_down;
>  
> -	pm_runtime_get_sync(&keypad->pdev->dev);
> +	guard(pm_runtime_active)(&keypad->pdev->dev);
>  
>  	do {
>  		readl(keypad->base + SAMSUNG_KEYIFSTSCLR);

[Severity: High]
This isn't a bug introduced by this patch, but does ignoring the return value
of the implicit pm_runtime_get_sync() call here leave the hardware access
vulnerable to crashes?

If resuming the device fails (e.g., due to a clock or regulator error), the
driver proceeds to access memory-mapped registers like SAMSUNG_KEYIFSTSCLR.
Reading or writing these registers while the power domain is unpowered can
trigger a Data Abort (kernel panic) on ARM.

[ ... ]

> @@ -167,7 +165,7 @@ static void samsung_keypad_start(struct samsung_keypad *keypad)
>  {
>  	unsigned int val;
>  
> -	pm_runtime_get_sync(&keypad->pdev->dev);
> +	guard(pm_runtime_active)(&keypad->pdev->dev);
>  
>  	/* Tell IRQ thread that it may poll the device. */
>  	keypad->stopped = false;
> @@ -182,8 +180,6 @@ static void samsung_keypad_start(struct samsung_keypad *keypad)
>  	/* KEYIFCOL reg clear. */
>  	writel(0, keypad->base + SAMSUNG_KEYIFCOL);

[Severity: High]
This is a pre-existing issue, but this function is reachable via userspace
opening the input device. If the resume fails, could the subsequent hardware
accesses like writel() to SAMSUNG_KEYIFCOL trigger a crash for the same
reason?

[ ... ]

> @@ -191,7 +187,7 @@ static void samsung_keypad_stop(struct samsung_keypad *keypad)
>  {
>  	unsigned int val;
>  
> -	pm_runtime_get_sync(&keypad->pdev->dev);
> +	guard(pm_runtime_active)(&keypad->pdev->dev);
>  
>  	/* Signal IRQ thread to stop polling and disable the handler. */
>  	keypad->stopped = true;
> @@ -207,8 +203,6 @@ static void samsung_keypad_stop(struct samsung_keypad *keypad)
>  	writel(val, keypad->base + SAMSUNG_KEYIFCON);
>  
>  	clk_disable(keypad->clk);
> -
> -	pm_runtime_put(&keypad->pdev->dev);
>  }

[Severity: High]
This is also a pre-existing issue, but does this face the same risk when
writing to SAMSUNG_KEYIFCON if the device failed to wake?

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