Re: [PATCH v3 2/4] Input: snvs_pwrkey - replace hardcoding -EINVAL with original error code

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing cleanup for dev_pm_set_wake_irq() causes memory leak and state corruption on driver unbind.
- [High] Systematic failure to check regmap_read() return values leads to consuming uninitialized stack memory, risking spurious system shutdown.
--

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

Input: snvs_pwrkey - replace hardcoding -EINVAL with original error code

This commit replaces a hardcoded -EINVAL with the actual error code returned
by platform_get_irq(). This preserves valuable diagnostic information and
allows probe deferral (-EPROBE_DEFER) to work correctly.

> diff --git a/drivers/input/keyboard/snvs_pwrkey.c b/drivers/input/keyboard/snvs_pwrkey.c
> index 748196fcce75b..a291812e6d22e 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 I noticed a potential lifecycle regression
further down in imx_snvs_pwrkey_probe():

drivers/input/keyboard/snvs_pwrkey.c:imx_snvs_pwrkey_probe() {
    ...
	error = dev_pm_set_wake_irq(dev, pdata->irq);
	if (error)
		dev_err(dev, "irq wake enable failed.\n");
    ...
}

The driver configures a wake IRQ using dev_pm_set_wake_irq(), which
allocates a wake_irq structure and attaches it to dev->power.wakeirq.
However, there doesn't appear to be a paired remove callback to call
dev_pm_clear_wake_irq() during driver unbind.

Does this permanently leak memory if the driver is unbound via sysfs?

Additionally, since the underlying IRQ is managed by devm_request_irq(), it
will be freed on unbind while dev->power.wakeirq remains configured. If the
system later attempts to suspend, could the PM core evaluate
device_may_wakeup() (which remains true since device_init_wakeup() isn't
reverted) and call enable_irq_wake() on a freed IRQ?

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but I also noticed that
regmap_read() return values are not checked in several places before
consuming the variables passed by reference:

drivers/input/keyboard/snvs_pwrkey.c:imx_snvs_pwrkey_interrupt() {
    ...
	u32 lp_status;

	pm_wakeup_event(input->dev.parent, 0);

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

It also happens in the timer handler:

drivers/input/keyboard/snvs_pwrkey.c:imx_imx_snvs_check_for_events() {
    ...
	u32 state;

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

If regmap_read() fails (e.g., due to an underlying clock enable failure or
MMIO bus error), it returns an error code and leaves the target stack
variables (lp_status, state) uninitialized.

Since these uninitialized stack variables are then used in logic checks,
could this lead to unpredictable behavior? For example, in the interrupt
and timer handlers, if the uninitialized stack memory randomly has the
SNVS_LPSR_SPO or SNVS_HPSR_BTN bits set, would this trigger a spurious
KEY_POWER event and an unintended system shutdown?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260615-b4-pwrkey-v3-0-9510b1173f6e@oss.nxp.com?part=2