[PATCH v5 0/4] snvs_pwrkey - code improvements and add report event

[PATCH v5 0/4] snvs_pwrkey - code improvements and add report event

[joy.zou]

This patch series improves the snvs_pwrkey driver with better code quality
and add report press event.

The main improvements include:
1. Clean up the code by using local device pointers and dev_err_probe() for
better readability and easier debugging.

2. Fix potential event loss during system suspend by reporting key press events
in the timer callback.

Signed-off-by: Joy Zou <joy.zou@oss.nxp.com>
---
Changes in v5:
- Replace SIMPLE_DEV_PM_OPS with DEFINE_SIMPLE_DEV_PM_OPS and remove
  __maybe_unused from suspend/resume callbacks for patch #4.
- Use pm_ptr() to wrap pm_ops pointer in platform_driver for patch #4.
- Replace suspended flag check in interrupt handler with a pending_press
  latch: set pending_press in hardirq context, consume and report the
  press event from the timer callback in softirq context for patch #4.
- Link to v4: https://patch.msgid.link/20260618-b4-pwrkey-v4-0-4bfda105bdf3@oss.nxp.com

Changes in v4:
- Use dev_err_probe() for devm_input_allocate_device() to simplify error handling
  for patch #1.
- Add Reviewed-by tag for patch #2 and #3.
- Modify the subject description for patch #2.
- Link to v3: https://patch.msgid.link/20260615-b4-pwrkey-v3-0-9510b1173f6e@oss.nxp.com

Changes in v3:
- Add spinlock for pdata->keystate and pdata->suspended per AI review comments.
- Replace hardcode value 1 with local variable keystate in input_report_key()
  under suspended.
- Link to v2: https://lore.kernel.org/r/20260604-pwrkey-cleanup-v2-0-59fa0ddf0d7c@nxp.com

Changes in v2:
- Drop dev_err_probe() change for platform_get_irq() per AI review comments.
- Add new patch #2 replace hardcoding -EINVAL with original error code per
  AI review comments.
- Use dev instead of &pdev->dev for devm_input_allocate_device(), which was
  missed in patch v1 per AI review comments.
- Add a boolean variable suspended and PM callback functions to replace
  the use of the is_suspended field per AI review comments.
- Move event report handle to else branch in suspended state, since the
  pdata->minor_rev == 0 branch has no debounce detection per AI review
  comments.
- Modify patch #3 and #4 commit message.
- Add Reviewed-by tag for patch #1.
- Link to v1: https://lore.kernel.org/r/20260326-pwrkey-cleanup-v1-0-d85d7c0bf275@nxp.com

To: Dmitry Torokhov <dmitry.torokhov@gmail.com>
To: Frank Li <Frank.Li@nxp.com>
To: Bough Chen <haibo.chen@nxp.com>
To: Peng Fan <peng.fan@nxp.com>
To: Jacky Bai <ping.bai@nxp.com>
To: Ye Li <ye.li@nxp.com>
Cc: imx@lists.linux.dev
Cc: linux-input@vger.kernel.org
Cc: linux-kernel@vger.kernel.org

---
Joy Zou (4):
      Input: snvs_pwrkey - make use of dev_err_probe()
      Input: snvs_pwrkey - propagate error code of platform_get_irq()
      Input: snvs_pwrkey - use local device pointer to simple code
      Input: snvs_pwrkey - add press event reporting to avoid event loss during suspend

 drivers/input/keyboard/snvs_pwrkey.c | 143 ++++++++++++++++++++++++-----------
 1 file changed, 98 insertions(+), 45 deletions(-)
---
base-commit: 34cf6dafc47441dfb6b356a095b89c3585a93714
change-id: 20260615-b4-pwrkey-a60b2043cd29

Best regards,
--  
Joy Zou <joy.zou@oss.nxp.com>

[PATCH v5 1/4] Input: snvs_pwrkey - make use of dev_err_probe()

[joy.zou]

From: Joy Zou <joy.zou@nxp.com>

Add dev_err_probe() at return path of probe() to support users to
identify issues easier.

Reviewed-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Joy Zou <joy.zou@nxp.com>
---
Changes in v4:
1. Use dev_err_probe() for devm_input_allocate_device() to simplify
   error handling.

Changes in v2:
1. Drop dev_err_probe() change for platform_get_irq() per AI review comments.
2. Add Reviewed-by tag.
---
 drivers/input/keyboard/snvs_pwrkey.c | 44 +++++++++++++-----------------------
 1 file changed, 16 insertions(+), 28 deletions(-)

diff --git a/drivers/input/keyboard/snvs_pwrkey.c b/drivers/input/keyboard/snvs_pwrkey.c
index 954055aaf6e2..8cc6863d26ed 100644
--- a/drivers/input/keyboard/snvs_pwrkey.c
+++ b/drivers/input/keyboard/snvs_pwrkey.c
@@ -124,17 +124,15 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
 	/* Get SNVS register Page */
 	np = pdev->dev.of_node;
 	if (!np)
-		return -ENODEV;
+		return dev_err_probe(&pdev->dev, -ENODEV, "Device tree node not found\n");
 
 	pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
 	if (!pdata)
 		return -ENOMEM;
 
 	pdata->snvs = syscon_regmap_lookup_by_phandle(np, "regmap");
-	if (IS_ERR(pdata->snvs)) {
-		dev_err(&pdev->dev, "Can't get snvs syscon\n");
-		return PTR_ERR(pdata->snvs);
-	}
+	if (IS_ERR(pdata->snvs))
+		return dev_err_probe(&pdev->dev, PTR_ERR(pdata->snvs), "Can't get snvs syscon\n");
 
 	if (of_property_read_u32(np, "linux,keycode", &pdata->keycode)) {
 		pdata->keycode = KEY_POWER;
@@ -142,10 +140,9 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
 	}
 
 	clk = devm_clk_get_optional_enabled(&pdev->dev, NULL);
-	if (IS_ERR(clk)) {
-		dev_err(&pdev->dev, "Failed to get snvs clock (%pe)\n", clk);
-		return PTR_ERR(clk);
-	}
+	if (IS_ERR(clk))
+		return dev_err_probe(&pdev->dev, PTR_ERR(clk),
+				     "Failed to get snvs clock (%pe)\n", clk);
 
 	pdata->wakeup = of_property_read_bool(np, "wakeup-source");
 
@@ -165,9 +162,8 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
 			bpt = (val / 5) - 1;
 			break;
 		default:
-			dev_err(&pdev->dev,
-				"power-off-time-sec %d out of range\n", val);
-			return -EINVAL;
+			return dev_err_probe(&pdev->dev, -EINVAL,
+					     "power-off-time-sec %d out of range\n", val);
 		}
 
 		regmap_update_bits(pdata->snvs, SNVS_LPCR_REG, SNVS_LPCR_BPT_MASK,
@@ -185,10 +181,8 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
 	timer_setup(&pdata->check_timer, imx_imx_snvs_check_for_events, 0);
 
 	input = devm_input_allocate_device(&pdev->dev);
-	if (!input) {
-		dev_err(&pdev->dev, "failed to allocate the input device\n");
-		return -ENOMEM;
-	}
+	if (!input)
+		return dev_err_probe(&pdev->dev, -ENOMEM, "failed to allocate the input device\n");
 
 	input->name = pdev->name;
 	input->phys = "snvs-pwrkey/input0";
@@ -198,10 +192,8 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
 
 	/* input customer action to cancel release timer */
 	error = devm_add_action(&pdev->dev, imx_snvs_pwrkey_act, pdata);
-	if (error) {
-		dev_err(&pdev->dev, "failed to register remove action\n");
-		return error;
-	}
+	if (error)
+		return dev_err_probe(&pdev->dev, error, "failed to register remove action\n");
 
 	pdata->input = input;
 	platform_set_drvdata(pdev, pdata);
@@ -209,16 +201,12 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
 	error = devm_request_irq(&pdev->dev, pdata->irq,
 			       imx_snvs_pwrkey_interrupt,
 			       0, pdev->name, pdev);
-	if (error) {
-		dev_err(&pdev->dev, "interrupt not available.\n");
-		return error;
-	}
+	if (error)
+		return dev_err_probe(&pdev->dev, error, "interrupt not available.\n");
 
 	error = input_register_device(input);
-	if (error < 0) {
-		dev_err(&pdev->dev, "failed to register input device\n");
-		return error;
-	}
+	if (error < 0)
+		return dev_err_probe(&pdev->dev, error, "failed to register input device\n");
 
 	device_init_wakeup(&pdev->dev, pdata->wakeup);
 	error = dev_pm_set_wake_irq(&pdev->dev, pdata->irq);

-- 
2.34.1

[PATCH v5 2/4] Input: snvs_pwrkey - propagate error code of platform_get_irq()

[joy.zou]

From: Joy Zou <joy.zou@nxp.com>

Hardcoding -EINVAL discards the actual error code, which breaks probe
deferral (-EPROBE_DEFER) and loses critical diagnostic information
needed for proper kernel error handling.

Reviewed-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Joy Zou <joy.zou@nxp.com>
---
Changes in v4:
1. modify the subject description.
2. add Reviewed-by tag.
---
 drivers/input/keyboard/snvs_pwrkey.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/input/keyboard/snvs_pwrkey.c b/drivers/input/keyboard/snvs_pwrkey.c
index 8cc6863d26ed..d58bbbe9fd58 100644
--- a/drivers/input/keyboard/snvs_pwrkey.c
+++ b/drivers/input/keyboard/snvs_pwrkey.c
@@ -148,7 +148,7 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
 
 	pdata->irq = platform_get_irq(pdev, 0);
 	if (pdata->irq < 0)
-		return -EINVAL;
+		return pdata->irq;
 
 	error = of_property_read_u32(np, "power-off-time-sec", &val);
 	if (!error) {

-- 
2.34.1

[PATCH v5 3/4] Input: snvs_pwrkey - use local device pointer to simple code

[joy.zou]

From: Joy Zou <joy.zou@nxp.com>

Use local struct device pointer to avoid reference the platform_device
pointer every time.

No functional change.

Reviewed-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Joy Zou <joy.zou@nxp.com>
---
Changes for v4:
1. Add Reviewed-by tag.

Changes for v2:
1. Use dev instead of &pdev->dev for devm_input_allocate_device(),
   which was missed in patch v1 per AI review comments.
2. Modify commit message.
---
 drivers/input/keyboard/snvs_pwrkey.c | 41 ++++++++++++++++++------------------
 1 file changed, 21 insertions(+), 20 deletions(-)

diff --git a/drivers/input/keyboard/snvs_pwrkey.c b/drivers/input/keyboard/snvs_pwrkey.c
index d58bbbe9fd58..cbe44a38d2b3 100644
--- a/drivers/input/keyboard/snvs_pwrkey.c
+++ b/drivers/input/keyboard/snvs_pwrkey.c
@@ -112,6 +112,7 @@ static void imx_snvs_pwrkey_act(void *pdata)
 
 static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
 {
+	struct device *dev = &pdev->dev;
 	struct pwrkey_drv_data *pdata;
 	struct input_dev *input;
 	struct device_node *np;
@@ -122,26 +123,26 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
 	u32 vid;
 
 	/* Get SNVS register Page */
-	np = pdev->dev.of_node;
+	np = dev->of_node;
 	if (!np)
-		return dev_err_probe(&pdev->dev, -ENODEV, "Device tree node not found\n");
+		return dev_err_probe(dev, -ENODEV, "Device tree node not found\n");
 
-	pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
+	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
 	if (!pdata)
 		return -ENOMEM;
 
 	pdata->snvs = syscon_regmap_lookup_by_phandle(np, "regmap");
 	if (IS_ERR(pdata->snvs))
-		return dev_err_probe(&pdev->dev, PTR_ERR(pdata->snvs), "Can't get snvs syscon\n");
+		return dev_err_probe(dev, PTR_ERR(pdata->snvs), "Can't get snvs syscon\n");
 
 	if (of_property_read_u32(np, "linux,keycode", &pdata->keycode)) {
 		pdata->keycode = KEY_POWER;
-		dev_warn(&pdev->dev, "KEY_POWER without setting in dts\n");
+		dev_warn(dev, "KEY_POWER without setting in dts\n");
 	}
 
-	clk = devm_clk_get_optional_enabled(&pdev->dev, NULL);
+	clk = devm_clk_get_optional_enabled(dev, NULL);
 	if (IS_ERR(clk))
-		return dev_err_probe(&pdev->dev, PTR_ERR(clk),
+		return dev_err_probe(dev, PTR_ERR(clk),
 				     "Failed to get snvs clock (%pe)\n", clk);
 
 	pdata->wakeup = of_property_read_bool(np, "wakeup-source");
@@ -162,7 +163,7 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
 			bpt = (val / 5) - 1;
 			break;
 		default:
-			return dev_err_probe(&pdev->dev, -EINVAL,
+			return dev_err_probe(dev, -EINVAL,
 					     "power-off-time-sec %d out of range\n", val);
 		}
 
@@ -180,9 +181,9 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
 
 	timer_setup(&pdata->check_timer, imx_imx_snvs_check_for_events, 0);
 
-	input = devm_input_allocate_device(&pdev->dev);
+	input = devm_input_allocate_device(dev);
 	if (!input)
-		return dev_err_probe(&pdev->dev, -ENOMEM, "failed to allocate the input device\n");
+		return dev_err_probe(dev, -ENOMEM, "failed to allocate the input device\n");
 
 	input->name = pdev->name;
 	input->phys = "snvs-pwrkey/input0";
@@ -191,27 +192,27 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
 	input_set_capability(input, EV_KEY, pdata->keycode);
 
 	/* input customer action to cancel release timer */
-	error = devm_add_action(&pdev->dev, imx_snvs_pwrkey_act, pdata);
+	error = devm_add_action(dev, imx_snvs_pwrkey_act, pdata);
 	if (error)
-		return dev_err_probe(&pdev->dev, error, "failed to register remove action\n");
+		return dev_err_probe(dev, error, "failed to register remove action\n");
 
 	pdata->input = input;
 	platform_set_drvdata(pdev, pdata);
 
-	error = devm_request_irq(&pdev->dev, pdata->irq,
-			       imx_snvs_pwrkey_interrupt,
-			       0, pdev->name, pdev);
+	error = devm_request_irq(dev, pdata->irq,
+				 imx_snvs_pwrkey_interrupt,
+				 0, pdev->name, pdev);
 	if (error)
-		return dev_err_probe(&pdev->dev, error, "interrupt not available.\n");
+		return dev_err_probe(dev, error, "interrupt not available.\n");
 
 	error = input_register_device(input);
 	if (error < 0)
-		return dev_err_probe(&pdev->dev, error, "failed to register input device\n");
+		return dev_err_probe(dev, error, "failed to register input device\n");
 
-	device_init_wakeup(&pdev->dev, pdata->wakeup);
-	error = dev_pm_set_wake_irq(&pdev->dev, pdata->irq);
+	device_init_wakeup(dev, pdata->wakeup);
+	error = dev_pm_set_wake_irq(dev, pdata->irq);
 	if (error)
-		dev_err(&pdev->dev, "irq wake enable failed.\n");
+		dev_err(dev, "irq wake enable failed.\n");
 
 	return 0;
 }

-- 
2.34.1

[PATCH v5 4/4] Input: snvs_pwrkey - add press event reporting to avoid event loss during suspend

[joy.zou]

From: Joy Zou <joy.zou@nxp.com>

The driver implements debounce protection using a timer-based mechanism:
when a key interrupt occurs, a timer is scheduled to verify the key state
after DEBOUNCE_TIME before reporting the event. This works well during
normal operation.

However, key press events can be lost during system resume on platforms
like i.MX8MQ-EVK because:
1. During the no_irq resume phase, PCIe driver restoration can take up to
200ms with IRQs disabled.
2. The power key interrupt remains pending during the no_irq phase.
3. If the key is released before IRQs are re-enabled, the timer eventually
runs but sees the key as released and skips reporting the event.

To prevent event loss during system suspend, set a pending_press flag in
the interrupt handler and report the press event from the timer callback
when the flag is set. This avoids out-of-order event delivery and keeps
the existing timer-based debounce mechanism for normal operation.

Signed-off-by: Joy Zou <joy.zou@nxp.com>
---
Changes for v5:
1. Replace SIMPLE_DEV_PM_OPS with DEFINE_SIMPLE_DEV_PM_OPS and remove
   __maybe_unused from suspend/resume callbacks.
2. Use pm_ptr() to wrap pm_ops pointer in platform_driver.
3. Replace suspended flag check in interrupt handler with a pending_press
   latch: set pending_press in hardirq context, consume and report the
   press event from the timer callback in softirq context.

Changes for v3:
1. Add spinlock for pdata->keystate and pdata->suspended per AI review
   comments.
2. Replace hardcode value 1 with local variable keystate in input_report_key()
   under suspended.

Changes for v2:
1. Add a boolean variable suspended and PM callback functions to replace
   the use of the is_suspended field per AI review comments.
2. Move event report handle to else branch in suspended state, since the
   pdata->minor_rev == 0 branch has no debounce detection per AI review
   comments.
3. Modify the commit message.
---
 drivers/input/keyboard/snvs_pwrkey.c | 72 ++++++++++++++++++++++++++++++++++--
 1 file changed, 68 insertions(+), 4 deletions(-)

diff --git a/drivers/input/keyboard/snvs_pwrkey.c b/drivers/input/keyboard/snvs_pwrkey.c
index cbe44a38d2b3..970c7d65bd57 100644
--- a/drivers/input/keyboard/snvs_pwrkey.c
+++ b/drivers/input/keyboard/snvs_pwrkey.c
@@ -39,6 +39,9 @@ struct pwrkey_drv_data {
 	int keycode;
 	int keystate;  /* 1:pressed */
 	int wakeup;
+	bool suspended;     /* Track suspend state */
+	bool pending_press; /* Key pressed during suspend, report from timer callback */
+	spinlock_t lock;    /* Protects keystate, suspended and pending_press */
 	struct timer_list check_timer;
 	struct input_dev *input;
 	u8 minor_rev;
@@ -49,14 +52,38 @@ static void imx_imx_snvs_check_for_events(struct timer_list *t)
 	struct pwrkey_drv_data *pdata = timer_container_of(pdata, t,
 							   check_timer);
 	struct input_dev *input = pdata->input;
+	bool state_changed = false;
+	bool pending_press;
 	u32 state;
 
 	regmap_read(pdata->snvs, SNVS_HPSR_REG, &state);
 	state = state & SNVS_HPSR_BTN ? 1 : 0;
 
-	/* only report new event if status changed */
-	if (state ^ pdata->keystate) {
-		pdata->keystate = state;
+	scoped_guard(spinlock_irqsave, &pdata->lock) {
+		pending_press = pdata->pending_press;
+		if (pending_press) {
+			pdata->pending_press = false;
+			pdata->keystate = 1;
+		}
+		/* only report new event if status changed */
+		if (state ^ pdata->keystate) {
+			pdata->keystate = state;
+			state_changed = true;
+		}
+	}
+
+	/*
+	 * Report a press event latched during suspend. If the key is still
+	 * held, state_changed will be 0 (keystate already set to 1 above),
+	 * so no duplicate press is reported. If already released,
+	 * state_changed will fire next to report the release.
+	 */
+	if (pending_press) {
+		input_report_key(input, pdata->keycode, 1);
+		input_sync(input);
+	}
+
+	if (state_changed) {
 		input_event(input, EV_KEY, pdata->keycode, state);
 		input_sync(input);
 		pm_relax(pdata->input->dev.parent);
@@ -92,8 +119,17 @@ static irqreturn_t imx_snvs_pwrkey_interrupt(int irq, void *dev_id)
 			input_sync(input);
 			pm_relax(input->dev.parent);
 		} else {
+			/*
+			 * If the key is pressed during suspend, latch it so
+			 * the timer callback can report the press event in
+			 * softirq context, avoiding out-of-order events.
+			 */
+			scoped_guard(spinlock_irqsave, &pdata->lock) {
+				if (pdata->suspended)
+					pdata->pending_press = true;
+			}
 			mod_timer(&pdata->check_timer,
-			          jiffies + msecs_to_jiffies(DEBOUNCE_TIME));
+				  jiffies + msecs_to_jiffies(DEBOUNCE_TIME));
 		}
 	}
 
@@ -151,6 +187,7 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
 	if (pdata->irq < 0)
 		return pdata->irq;
 
+	spin_lock_init(&pdata->lock);
 	error = of_property_read_u32(np, "power-off-time-sec", &val);
 	if (!error) {
 		switch (val) {
@@ -217,6 +254,32 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
 	return 0;
 }
 
+static int imx_snvs_pwrkey_suspend(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct pwrkey_drv_data *pdata = platform_get_drvdata(pdev);
+
+	scoped_guard(spinlock_irqsave, &pdata->lock)
+		pdata->suspended = true;
+
+	return 0;
+}
+
+static int imx_snvs_pwrkey_resume(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct pwrkey_drv_data *pdata = platform_get_drvdata(pdev);
+
+	scoped_guard(spinlock_irqsave, &pdata->lock)
+		pdata->suspended = false;
+
+	return 0;
+}
+
+static DEFINE_SIMPLE_DEV_PM_OPS(imx_snvs_pwrkey_pm_ops,
+				imx_snvs_pwrkey_suspend,
+				imx_snvs_pwrkey_resume);
+
 static const struct of_device_id imx_snvs_pwrkey_ids[] = {
 	{ .compatible = "fsl,sec-v4.0-pwrkey" },
 	{ /* sentinel */ }
@@ -227,6 +290,7 @@ static struct platform_driver imx_snvs_pwrkey_driver = {
 	.driver = {
 		.name = "snvs_pwrkey",
 		.of_match_table = imx_snvs_pwrkey_ids,
+		.pm = pm_ptr(&imx_snvs_pwrkey_pm_ops),
 	},
 	.probe = imx_snvs_pwrkey_probe,
 };

-- 
2.34.1

Re: [PATCH v5 3/4] Input: snvs_pwrkey - use local device pointer to simple code

[sashiko-bot]

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

Pre-existing issues:
- [High] Resource Leak / Use-After-Free due to unmanaged dev_pm_set_wake_irq()
--

--- Patch [3]: [PATCH v5 3/4] Input: snvs_pwrkey - use local device pointer to simple code ---
commit a69cd5d0da1072f6e3d82c6e22dfe1b937e29159
Author: Joy Zou <joy.zou@nxp.com>

Input: snvs_pwrkey - use local device pointer to simple code

This commit introduces a local device pointer in imx_snvs_pwrkey_probe() to
simplify the code. It replaces repeated references to &pdev->dev with a
single local variable.

> diff --git a/drivers/input/keyboard/snvs_pwrkey.c b/drivers/input/keyboard/snvs_pwrkey.c
> index d58bbbe9fd58f..cbe44a38d2b3d 100644
> --- a/drivers/input/keyboard/snvs_pwrkey.c
> +++ b/drivers/input/keyboard/snvs_pwrkey.c
[ ... ]
> @@ -191,27 +192,27 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
[ ... ]
> -	device_init_wakeup(&pdev->dev, pdata->wakeup);
> -	error = dev_pm_set_wake_irq(&pdev->dev, pdata->irq);
> +	device_init_wakeup(dev, pdata->wakeup);
> +	error = dev_pm_set_wake_irq(dev, pdata->irq);

[Severity: High]
This is a pre-existing issue, but does this code leak the wakeirq structure
when the driver is unbound?

Because the driver provides no remove callback or devm action to call
dev_pm_clear_wake_irq(), the allocated wake_irq structure appears to be left
attached to dev->power.wakeirq on unbind. If the driver is re-probed, could
this cause dev_pm_set_wake_irq() to fail with -EEXIST?

Also, since the underlying IRQ is managed by devm_request_irq() and freed on
unbind, if the system enters suspend after unbinding, could the PM core
attempt to call enable_irq_wake() using the stale, freed IRQ number and
cause an oops or use-after-free?

>  	if (error)
> -		dev_err(&pdev->dev, "irq wake enable failed.\n");
> +		dev_err(dev, "irq wake enable failed.\n");
>  
>  	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260715-b4-pwrkey-v5-0-07e7353c319e@oss.nxp.com?part=3

Re: [PATCH v5 2/4] Input: snvs_pwrkey - propagate error code of platform_get_irq()

[sashiko-bot]

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

Pre-existing issues:
- [High] The driver uses `dev_pm_set_wake_irq()` and `device_init_wakeup()` during probe but provides no `.remove` callback to clean them up, causing a resource leak and permanently breaking driver rebind.
- [High] The driver ignores the return value of `regmap_read()`, leading to the use of uninitialized stack variables which can trigger spurious system shutdowns.
--

--- Patch [2]: [PATCH v5 2/4] Input: snvs_pwrkey - propagate error code of platform_get_irq() ---
commit 353345395cd42cd8e38b32a7bbc24dfad430ba99
Author: Joy Zou <joy.zou@nxp.com>

Input: snvs_pwrkey - propagate error code of platform_get_irq()

This commit updates the driver to propagate the actual error code from
platform_get_irq() instead of hardcoding -EINVAL. This ensures probe
deferral and other diagnostic information are preserved.

> diff --git a/drivers/input/keyboard/snvs_pwrkey.c b/drivers/input/keyboard/snvs_pwrkey.c
> index 8cc6863d26ed8..d58bbbe9fd58f 100644
> --- a/drivers/input/keyboard/snvs_pwrkey.c
> +++ b/drivers/input/keyboard/snvs_pwrkey.c
> @@ -148,7 +148,7 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
>  
>  	pdata->irq = platform_get_irq(pdev, 0);
>  	if (pdata->irq < 0)
> -		return -EINVAL;
> +		return pdata->irq;
>  
>  	error = of_property_read_u32(np, "power-off-time-sec", &val);
>  	if (!error) {

[Severity: High]
This is a pre-existing issue, but does this driver leak the wake IRQ upon
unbind?

Looking at imx_snvs_pwrkey_probe() in drivers/input/keyboard/snvs_pwrkey.c,
the driver sets up a wake IRQ:

    error = dev_pm_set_wake_irq(dev, pdata->irq);
    if (error)
        dev_err(dev, "irq wake enable failed.\n");

However, there is no corresponding .remove callback in the platform_driver
struct (imx_snvs_pwrkey_driver) to call dev_pm_clear_wake_irq() or
device_init_wakeup(..., false):

    static struct platform_driver imx_snvs_pwrkey_driver = {
        .driver = {
            .name = "snvs_pwrkey",
            .of_match_table = imx_snvs_pwrkey_ids,
        },
        .probe = imx_snvs_pwrkey_probe,
    };

If the driver is unbound via sysfs and rebound, does this leave a dangling
wakeup source and cause the subsequent bind to fail with -EEXIST?

[Severity: High]
This is a pre-existing issue, but can unhandled regmap_read() errors cause
spurious system shutdowns?

In drivers/input/keyboard/snvs_pwrkey.c, imx_snvs_pwrkey_interrupt() reads
the status register without checking the return value:

    regmap_read(pdata->snvs, SNVS_LPSR_REG, &lp_status);
    if (lp_status & SNVS_LPSR_SPO) {

A similar pattern exists in imx_imx_snvs_check_for_events():

    regmap_read(pdata->snvs, SNVS_HPSR_REG, &state);
    state = state & SNVS_HPSR_BTN ? 1 : 0;

And also in imx_snvs_pwrkey_probe():

    regmap_read(pdata->snvs, SNVS_HPVIDR1_REG, &vid);
    pdata->minor_rev = vid & 0xff;

If regmap_read() fails due to a clock glitch or bus fault, won't these
stack variables remain uninitialized? Could an uninitialized variable
randomly evaluate to true and be interpreted as a button press,
immediately propagating a spurious power key event?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260715-b4-pwrkey-v5-0-07e7353c319e@oss.nxp.com?part=2

Re: [PATCH v5 4/4] Input: snvs_pwrkey - add press event reporting to avoid event loss during suspend

[sashiko-bot]

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

Pre-existing issues:
- [High] Missing cleanup for wake IRQ leads to memory leak and device re-bind failure.
--

--- Patch [4]: [PATCH v5 4/4] Input: snvs_pwrkey - add press event reporting to avoid event loss during suspend ---
commit 0a87e611a13956d492552759681ef44d587017b8
Author: Joy Zou <joy.zou@nxp.com>

Input: snvs_pwrkey - add press event reporting to avoid event loss during suspend

This commit introduces a pending_press flag and a spinlock to handle power
key events during system resume phases where IRQs might be disabled. It
ensures that if the key is released before IRQs are re-enabled, a press
event is still reported via the timer callback.

> diff --git a/drivers/input/keyboard/snvs_pwrkey.c b/drivers/input/keyboard/snvs_pwrkey.c
> --- a/drivers/input/keyboard/snvs_pwrkey.c
> +++ b/drivers/input/keyboard/snvs_pwrkey.c

[ ... ]

> @@ -217,6 +254,32 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
>  	return 0;
>  }

[Severity: High]
This is a pre-existing issue, but I noticed the wake IRQ setup just prior to
this return in imx_snvs_pwrkey_probe() lacks a cleanup path:

	error = dev_pm_set_wake_irq(dev, pdata->irq);

Since there is no .remove callback or manual cleanup, does this leak the
struct wake_irq memory when the device is unbound?

Additionally, since dev->power.wakeirq is never cleared, would re-binding
the driver trigger the dev_WARN_ONCE ("wake irq already initialized") in
dev_pm_attach_wake_irq(), potentially causing a system panic if
panic_on_warn is enabled?

Could this be resolved by switching to the managed devm_pm_set_wake_irq()
helper?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260715-b4-pwrkey-v5-0-07e7353c319e@oss.nxp.com?part=4

Re: [PATCH v5 4/4] Input: snvs_pwrkey - add press event reporting to avoid event loss during suspend

[Frank Li]

On Wed, Jul 15, 2026 at 05:33:54PM +0800, joy.zou@oss.nxp.com wrote:
> From: Joy Zou <joy.zou@nxp.com>
>
> The driver implements debounce protection using a timer-based mechanism:
> when a key interrupt occurs, a timer is scheduled to verify the key state
> after DEBOUNCE_TIME before reporting the event. This works well during
> normal operation.
>
> However, key press events can be lost during system resume on platforms
> like i.MX8MQ-EVK because:
> 1. During the no_irq resume phase, PCIe driver restoration can take up to
> 200ms with IRQs disabled.
> 2. The power key interrupt remains pending during the no_irq phase.
> 3. If the key is released before IRQs are re-enabled, the timer eventually
> runs but sees the key as released and skips reporting the event.
>
> To prevent event loss during system suspend, set a pending_press flag in
> the interrupt handler and report the press event from the timer callback
> when the flag is set. This avoids out-of-order event delivery and keeps
> the existing timer-based debounce mechanism for normal operation.
>
> Signed-off-by: Joy Zou <joy.zou@nxp.com>
> ---
> Changes for v5:
> 1. Replace SIMPLE_DEV_PM_OPS with DEFINE_SIMPLE_DEV_PM_OPS and remove
>    __maybe_unused from suspend/resume callbacks.
> 2. Use pm_ptr() to wrap pm_ops pointer in platform_driver.
> 3. Replace suspended flag check in interrupt handler with a pending_press
>    latch: set pending_press in hardirq context, consume and report the
>    press event from the timer callback in softirq context.
>
> Changes for v3:
> 1. Add spinlock for pdata->keystate and pdata->suspended per AI review
>    comments.
> 2. Replace hardcode value 1 with local variable keystate in input_report_key()
>    under suspended.
>
> Changes for v2:
> 1. Add a boolean variable suspended and PM callback functions to replace
>    the use of the is_suspended field per AI review comments.
> 2. Move event report handle to else branch in suspended state, since the
>    pdata->minor_rev == 0 branch has no debounce detection per AI review
>    comments.
> 3. Modify the commit message.
> ---

Reviewed-by: Frank Li <Frank.Li@nxp.com>

>  drivers/input/keyboard/snvs_pwrkey.c | 72 ++++++++++++++++++++++++++++++++++--
>  1 file changed, 68 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/input/keyboard/snvs_pwrkey.c b/drivers/input/keyboard/snvs_pwrkey.c
> index cbe44a38d2b3..970c7d65bd57 100644
> --- a/drivers/input/keyboard/snvs_pwrkey.c
> +++ b/drivers/input/keyboard/snvs_pwrkey.c
> @@ -39,6 +39,9 @@ struct pwrkey_drv_data {
>  	int keycode;
>  	int keystate;  /* 1:pressed */
>  	int wakeup;
> +	bool suspended;     /* Track suspend state */
> +	bool pending_press; /* Key pressed during suspend, report from timer callback */
> +	spinlock_t lock;    /* Protects keystate, suspended and pending_press */
>  	struct timer_list check_timer;
>  	struct input_dev *input;
>  	u8 minor_rev;
> @@ -49,14 +52,38 @@ static void imx_imx_snvs_check_for_events(struct timer_list *t)
>  	struct pwrkey_drv_data *pdata = timer_container_of(pdata, t,
>  							   check_timer);
>  	struct input_dev *input = pdata->input;
> +	bool state_changed = false;
> +	bool pending_press;
>  	u32 state;
>
>  	regmap_read(pdata->snvs, SNVS_HPSR_REG, &state);
>  	state = state & SNVS_HPSR_BTN ? 1 : 0;
>
> -	/* only report new event if status changed */
> -	if (state ^ pdata->keystate) {
> -		pdata->keystate = state;
> +	scoped_guard(spinlock_irqsave, &pdata->lock) {
> +		pending_press = pdata->pending_press;
> +		if (pending_press) {
> +			pdata->pending_press = false;
> +			pdata->keystate = 1;
> +		}
> +		/* only report new event if status changed */
> +		if (state ^ pdata->keystate) {
> +			pdata->keystate = state;
> +			state_changed = true;
> +		}
> +	}
> +
> +	/*
> +	 * Report a press event latched during suspend. If the key is still
> +	 * held, state_changed will be 0 (keystate already set to 1 above),
> +	 * so no duplicate press is reported. If already released,
> +	 * state_changed will fire next to report the release.
> +	 */
> +	if (pending_press) {
> +		input_report_key(input, pdata->keycode, 1);
> +		input_sync(input);
> +	}
> +
> +	if (state_changed) {
>  		input_event(input, EV_KEY, pdata->keycode, state);
>  		input_sync(input);
>  		pm_relax(pdata->input->dev.parent);
> @@ -92,8 +119,17 @@ static irqreturn_t imx_snvs_pwrkey_interrupt(int irq, void *dev_id)
>  			input_sync(input);
>  			pm_relax(input->dev.parent);
>  		} else {
> +			/*
> +			 * If the key is pressed during suspend, latch it so
> +			 * the timer callback can report the press event in
> +			 * softirq context, avoiding out-of-order events.
> +			 */
> +			scoped_guard(spinlock_irqsave, &pdata->lock) {
> +				if (pdata->suspended)
> +					pdata->pending_press = true;
> +			}
>  			mod_timer(&pdata->check_timer,
> -			          jiffies + msecs_to_jiffies(DEBOUNCE_TIME));
> +				  jiffies + msecs_to_jiffies(DEBOUNCE_TIME));
>  		}
>  	}
>
> @@ -151,6 +187,7 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
>  	if (pdata->irq < 0)
>  		return pdata->irq;
>
> +	spin_lock_init(&pdata->lock);
>  	error = of_property_read_u32(np, "power-off-time-sec", &val);
>  	if (!error) {
>  		switch (val) {
> @@ -217,6 +254,32 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
>  	return 0;
>  }
>
> +static int imx_snvs_pwrkey_suspend(struct device *dev)
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct pwrkey_drv_data *pdata = platform_get_drvdata(pdev);
> +
> +	scoped_guard(spinlock_irqsave, &pdata->lock)
> +		pdata->suspended = true;
> +
> +	return 0;
> +}
> +
> +static int imx_snvs_pwrkey_resume(struct device *dev)
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct pwrkey_drv_data *pdata = platform_get_drvdata(pdev);
> +
> +	scoped_guard(spinlock_irqsave, &pdata->lock)
> +		pdata->suspended = false;
> +
> +	return 0;
> +}
> +
> +static DEFINE_SIMPLE_DEV_PM_OPS(imx_snvs_pwrkey_pm_ops,
> +				imx_snvs_pwrkey_suspend,
> +				imx_snvs_pwrkey_resume);
> +
>  static const struct of_device_id imx_snvs_pwrkey_ids[] = {
>  	{ .compatible = "fsl,sec-v4.0-pwrkey" },
>  	{ /* sentinel */ }
> @@ -227,6 +290,7 @@ static struct platform_driver imx_snvs_pwrkey_driver = {
>  	.driver = {
>  		.name = "snvs_pwrkey",
>  		.of_match_table = imx_snvs_pwrkey_ids,
> +		.pm = pm_ptr(&imx_snvs_pwrkey_pm_ops),
>  	},
>  	.probe = imx_snvs_pwrkey_probe,
>  };
>
> --
> 2.34.1
>
>