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

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

[Dmitry Torokhov]

When checking if the device can wake the system, we should pull the
device_may_wakeup() check to the caller instead of repeating it inside
the toggle_wakeup() handler. Furthermore, when configuring the wakeup,
we should safely ensure we write to the registers in the correct order:
configure the interrupt receiver before enabling the peripheral's wake
functionality, and vice-versa.

Assisted-by: Antigravity:gemini-3.1-pro
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/keyboard/samsung-keypad.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/input/keyboard/samsung-keypad.c b/drivers/input/keyboard/samsung-keypad.c
index 17127269e3f0..a578f429d100 100644
--- a/drivers/input/keyboard/samsung-keypad.c
+++ b/drivers/input/keyboard/samsung-keypad.c
@@ -492,15 +492,14 @@ static void samsung_keypad_toggle_wakeup(struct samsung_keypad *keypad,
 
 	val = readl(keypad->base + SAMSUNG_KEYIFCON);
 	if (enable) {
+		enable_irq_wake(keypad->irq);
 		val |= SAMSUNG_KEYIFCON_WAKEUPEN;
-		if (device_may_wakeup(&keypad->pdev->dev))
-			enable_irq_wake(keypad->irq);
+		writel(val, keypad->base + SAMSUNG_KEYIFCON);
 	} else {
 		val &= ~SAMSUNG_KEYIFCON_WAKEUPEN;
-		if (device_may_wakeup(&keypad->pdev->dev))
-			disable_irq_wake(keypad->irq);
+		writel(val, keypad->base + SAMSUNG_KEYIFCON);
+		disable_irq_wake(keypad->irq);
 	}
-	writel(val, keypad->base + SAMSUNG_KEYIFCON);
 
 	clk_disable(keypad->clk);
 }
@@ -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;
 }
@@ -529,7 +529,8 @@ static int samsung_keypad_resume(struct device *dev)
 
 	guard(mutex)(&input_dev->mutex);
 
-	samsung_keypad_toggle_wakeup(keypad, false);
+	if (device_may_wakeup(dev))
+		samsung_keypad_toggle_wakeup(keypad, false);
 
 	if (input_device_enabled(input_dev))
 		samsung_keypad_start(keypad);
-- 
2.55.0.795.g602f6c329a-goog

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

[Dmitry Torokhov]

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. Enable the interrupt in samsung_keypad_start()
when the device is opened and ready, and disable it in
samsung_keypad_stop() when the device is closed. Remove the redundant
re-enabling of the interrupt at the end of samsung_keypad_stop().

Additionally, manually clear the pending interrupt status during system
resume when the device is closed to avoid immediate resume.

Fixes: 0fffed27f92d ("Input: samsung-keypad - Add samsung keypad driver")
Assisted-by: Antigravity:gemini-3.1-pro
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/keyboard/samsung-keypad.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/input/keyboard/samsung-keypad.c b/drivers/input/keyboard/samsung-keypad.c
index a578f429d100..88e733c1eabe 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);
+
+	enable_irq(keypad->irq);
 }
 
 static void samsung_keypad_stop(struct samsung_keypad *keypad)
@@ -206,12 +208,6 @@ static void samsung_keypad_stop(struct samsung_keypad *keypad)
 
 	clk_disable(keypad->clk);
 
-	/*
-	 * Now that chip should not generate interrupts we can safely
-	 * re-enable the handler.
-	 */
-	enable_irq(keypad->irq);
-
 	pm_runtime_put(&keypad->pdev->dev);
 }
 
@@ -412,7 +408,8 @@ static int samsung_keypad_probe(struct platform_device *pdev)
 	}
 
 	error = devm_request_threaded_irq(&pdev->dev, keypad->irq, NULL,
-					  samsung_keypad_irq, IRQF_ONESHOT,
+					  samsung_keypad_irq,
+					  IRQF_ONESHOT | IRQF_NO_AUTOEN,
 					  dev_name(&pdev->dev), keypad);
 	if (error) {
 		dev_err(&pdev->dev, "failed to register keypad interrupt\n");
@@ -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);
 	}
 
 	clk_disable(keypad->clk);
-- 
2.55.0.795.g602f6c329a-goog

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

[sashiko-bot]

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

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

[Dmitry Torokhov]

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

Assisted-by: Antigravity:gemini-3.5-flash
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/keyboard/samsung-keypad.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/drivers/input/keyboard/samsung-keypad.c b/drivers/input/keyboard/samsung-keypad.c
index 88e733c1eabe..5f6535b49761 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);
@@ -158,8 +158,6 @@ static irqreturn_t samsung_keypad_irq(int irq, void *dev_id)
 
 	} while (key_down && !keypad->stopped);
 
-	pm_runtime_put(&keypad->pdev->dev);
-
 	return IRQ_HANDLED;
 }
 
@@ -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);
 
-	pm_runtime_put(&keypad->pdev->dev);
-
 	enable_irq(keypad->irq);
 }
 
@@ -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);
 }
 
 static int samsung_keypad_open(struct input_dev *input_dev)
-- 
2.55.0.795.g602f6c329a-goog

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

[sashiko-bot]

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