[PATCH v2 0/3] Input: samsung-keypad - interrupt and PM fixes

[PATCH v2 0/3] Input: samsung-keypad - interrupt and PM fixes

[Dmitry Torokhov]

This series improves the Samsung keypad driver's interrupt handling
and power management.

Changes in v2:
- Fix incorrect clearing of keypad status register during system resume
  which caused wakeup key events to be discarded. We now only clear it
  if the device is closed.
- We do not expect runtime suspend/resume to fail in this driver, and
  power domains are not expected to interfere, so no additional PM
  error handling is introduced.

Link to v1:
https://lore.kernel.org/r/20260714015309.2140694-1-dmitry.torokhov@gmail.com

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
Dmitry Torokhov (3):
      Input: samsung-keypad - clean up wakeup configuration logic
      Input: samsung-keypad - keep interrupt disabled while closed
      Input: samsung-keypad - use pm_runtime_active guard

 drivers/input/keyboard/samsung-keypad.c | 39 ++++++++++++++-------------------
 1 file changed, 17 insertions(+), 22 deletions(-)
---
base-commit: 49362394dad7df66c274c867a271394c10ca2bb8
change-id: 20260713-samsung-kp-irq-d9b2652f0b88

Thanks.

-- 
Dmitry

[PATCH v2 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 v2 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..a51f0f639e9e 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 (!input_device_enabled(keypad->input_dev))
+			writel(~0x0, keypad->base + SAMSUNG_KEYIFSTSCLR);
 	}
 
 	clk_disable(keypad->clk);

-- 
2.55.0.795.g602f6c329a-goog

[PATCH v2 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 a51f0f639e9e..deffa98749c0 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 v2 1/3] Input: samsung-keypad - clean up wakeup configuration logic

[sashiko-bot]

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