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

[PATCH v4 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
directly in the interrupt handler.

Signed-off-by: Joy Zou <joy.zou@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 - report press event in interrupt handler

 drivers/input/keyboard/snvs_pwrkey.c | 131 +++++++++++++++++++++++------------
 1 file changed, 87 insertions(+), 44 deletions(-)
---
base-commit: f7af91adc230aa99e23330ecf85bc9badd9780ad
change-id: 20260615-b4-pwrkey-a60b2043cd29

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

[PATCH v4 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

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

[sashiko-bot]

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

Pre-existing issues:
- [Medium] Memory leak of struct wake_irq on device unbind
--

commit 469e290fcf154d6638767e1e60cb2427638bea98
Author: Joy Zou <joy.zou@nxp.com>

Input: snvs_pwrkey - make use of dev_err_probe()

This commit updates the probe function of the snvs_pwrkey driver to
use dev_err_probe() for error handling. This simplifies the code and
improves logging for deferred probes.

> diff --git a/drivers/input/keyboard/snvs_pwrkey.c b/drivers/input/keyboard/snvs_pwrkey.c
> index 954055aaf6e29..8cc6863d26ed8 100644
> --- a/drivers/input/keyboard/snvs_pwrkey.c
> +++ b/drivers/input/keyboard/snvs_pwrkey.c
[ ... ]
> @@ -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);

[Severity: Medium]
This is a pre-existing issue, but does this dev_pm_set_wake_irq() allocation
leak when the driver is unbound?

Because this driver relies entirely on devres for resource management and
does not implement a .remove callback, there is no explicit call to
dev_pm_clear_wake_irq() upon driver unbind.

The device core does not automatically free dev->power.wakeirq during
device_pm_remove(), so this allocated wake_irq structure appears to leak
every time the driver is unbound.

Could this be fixed by switching to the managed variant,
devm_pm_set_wake_irq()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260618-b4-pwrkey-v4-0-4bfda105bdf3@oss.nxp.com?part=1

[PATCH v4 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 v4 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

Re: [PATCH v4 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] Missing cleanup for device wakeup and wake IRQ on driver unbind causes memory leaks and re-bind warnings.
--

commit aed6a36738ed05979b6e5650a6efcf08cfb05a1d
Author: Joy Zou <joy.zou@nxp.com>

Input: snvs_pwrkey - use local device pointer to simple code

This patch introduces a local device pointer in imx_snvs_pwrkey_probe()
to simplify the code. It replaces repeated references to the platform device
pointer with the new local variable.

> 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

[ ... ]

> @@ -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 since there is no remove callback in this
driver, does this leak the wake IRQ allocation when the driver is unbound?

When the driver is unbound via sysfs, the wake_irq structure allocated by
dev_pm_set_wake_irq() seems to remain attached to dev->power.wakeirq.

If the driver is subsequently rebound, will dev_pm_set_wake_irq() fail with
-EEXIST and trigger a kernel warning about the wake irq already being
initialized?

Furthermore, because device_del() does not automatically free
dev->power.wakeirq, could this memory permanently leak when the platform
device is destroyed?

Would it be better to use devm_pm_set_wake_irq() here to ensure proper
cleanup?

>  	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/20260618-b4-pwrkey-v4-0-4bfda105bdf3@oss.nxp.com?part=3

[PATCH v4 4/4] Input: snvs_pwrkey - report press event in interrupt handler

[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.

Report key press events directly in interrupt handler to prevent event
loss during system suspend. This is safe because:

1. Only one event is reported per suspend cycle.
2. Normal operation retains the existing timer-based debounce mechanism.

Signed-off-by: Joy Zou <joy.zou@nxp.com>
---
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 | 60 ++++++++++++++++++++++++++++++++++--
 1 file changed, 57 insertions(+), 3 deletions(-)

diff --git a/drivers/input/keyboard/snvs_pwrkey.c b/drivers/input/keyboard/snvs_pwrkey.c
index cbe44a38d2b3..98c33d0bb1ec 100644
--- a/drivers/input/keyboard/snvs_pwrkey.c
+++ b/drivers/input/keyboard/snvs_pwrkey.c
@@ -39,6 +39,8 @@ struct pwrkey_drv_data {
 	int keycode;
 	int keystate;  /* 1:pressed */
 	int wakeup;
+	bool suspended; /* Track suspend state */
+	spinlock_t lock; /* Protects keystate and suspended */
 	struct timer_list check_timer;
 	struct input_dev *input;
 	u8 minor_rev;
@@ -49,14 +51,21 @@ 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;
 	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) {
+		/* only report new event if status changed */
+		if (state ^ pdata->keystate) {
+			pdata->keystate = state;
+			state_changed = true;
+		}
+	}
+
+	if (state_changed) {
 		input_event(input, EV_KEY, pdata->keycode, state);
 		input_sync(input);
 		pm_relax(pdata->input->dev.parent);
@@ -74,6 +83,8 @@ static irqreturn_t imx_snvs_pwrkey_interrupt(int irq, void *dev_id)
 	struct platform_device *pdev = dev_id;
 	struct pwrkey_drv_data *pdata = platform_get_drvdata(pdev);
 	struct input_dev *input = pdata->input;
+	bool suspended;
+	int keystate;
 	u32 lp_status;
 
 	pm_wakeup_event(input->dev.parent, 0);
@@ -92,6 +103,21 @@ static irqreturn_t imx_snvs_pwrkey_interrupt(int irq, void *dev_id)
 			input_sync(input);
 			pm_relax(input->dev.parent);
 		} else {
+			/*
+			 * Report key press events directly in interrupt handler to prevent event
+			 * loss during system suspend.
+			 */
+			scoped_guard(spinlock_irqsave, &pdata->lock) {
+				suspended = pdata->suspended;
+				if (suspended) {
+					pdata->keystate = 1;
+					keystate = pdata->keystate;
+				}
+			}
+			if (suspended) {
+				input_report_key(input, pdata->keycode, keystate);
+				input_sync(input);
+			}
 			mod_timer(&pdata->check_timer,
 			          jiffies + msecs_to_jiffies(DEBOUNCE_TIME));
 		}
@@ -151,6 +177,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 +244,32 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
 	return 0;
 }
 
+static int __maybe_unused 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 __maybe_unused 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 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 +280,7 @@ static struct platform_driver imx_snvs_pwrkey_driver = {
 	.driver = {
 		.name = "snvs_pwrkey",
 		.of_match_table = imx_snvs_pwrkey_ids,
+		.pm   = &imx_snvs_pwrkey_pm_ops,
 	},
 	.probe = imx_snvs_pwrkey_probe,
 };

-- 
2.34.1

Re: [PATCH v4 4/4] Input: snvs_pwrkey - report press event in interrupt handler

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The spinlock scope is too narrow, allowing the timer softirq and interrupt handler to interleave their event reporting, which can leave the key permanently stuck in the pressed state.
--

commit 115416424bcbaae3f95f1195f01f6e828f579e10
Author: Joy Zou <joy.zou@nxp.com>

Input: snvs_pwrkey - report press event in interrupt handler

This patch modifies the driver to report key press events directly in the
interrupt handler during system suspend to prevent event loss. It also adds a
spinlock to protect the keystate and suspended tracking flag.

> 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

[ ... ]

> @@ -49,14 +51,21 @@ 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;
>  	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) {
> +		/* only report new event if status changed */
> +		if (state ^ pdata->keystate) {
> +			pdata->keystate = state;
> +			state_changed = true;
> +		}
> +	}
> +
> +	if (state_changed) {
>  		input_event(input, EV_KEY, pdata->keycode, state);

[Severity: High]
Is the spinlock scope here too narrow? 

Because the lock is dropped before calling input_event(), it seems the
interrupt handler could preempt this thread and reorder the event delivery
to the input core.

>  		input_sync(input);
>  		pm_relax(pdata->input->dev.parent);

[ ... ]

> @@ -92,6 +103,21 @@ static irqreturn_t imx_snvs_pwrkey_interrupt(int irq, void *dev_id)
>  			input_sync(input);
>  			pm_relax(input->dev.parent);
>  		} else {
> +			/*
> +			 * Report key press events directly in interrupt handler to prevent event
> +			 * loss during system suspend.
> +			 */
> +			scoped_guard(spinlock_irqsave, &pdata->lock) {
> +				suspended = pdata->suspended;
> +				if (suspended) {
> +					pdata->keystate = 1;
> +					keystate = pdata->keystate;
> +				}
> +			}
> +			if (suspended) {
> +				input_report_key(input, pdata->keycode, keystate);

[Severity: High]
Could dropping the lock before calling input_report_key() lead to a race
with the timer softirq emitting its own input events?

For example, if the timer softirq reads state as 0 in
imx_imx_snvs_check_for_events(), updates pdata->keystate to 0, and drops
the lock, an interrupt could then preempt it:

CPU0 (timer softirq)
    scoped_guard(...) {
        pdata->keystate = 0;
    }

CPU0 (hardirq preempts before input_event is called)
    scoped_guard(...) {
        pdata->keystate = 1;
    }
    input_report_key(..., 1); // PRESS emitted

CPU0 (timer softirq resumes)
    input_event(..., 0);      // RELEASE emitted

In this scenario, the input core would receive PRESS followed by RELEASE,
leaving the final input state as RELEASE (0). However, pdata->keystate
remains PRESS (1).

Would this cause the next physical key press to be ignored since the timer
will see state=1, but the XOR against keystate (already 1) evaluates to 0,
effectively losing the event permanently?

> +				input_sync(input);
> +			}
>  			mod_timer(&pdata->check_timer,
>  			          jiffies + msecs_to_jiffies(DEBOUNCE_TIME));
>  		}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260618-b4-pwrkey-v4-0-4bfda105bdf3@oss.nxp.com?part=4