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

[sashiko-bot@kernel.org]

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