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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Incorrect pairing of pm_wakeup_event(dev, 0) and pm_relax(dev) fails to keep the system awake.
- [Medium] Missing cleanup for dev_pm_set_wake_irq() causes resource leak and -EEXIST on rebind.
--

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

Input: snvs_pwrkey - make use of dev_err_probe()

This patch updates the imx_snvs_pwrkey_probe function to use dev_err_probe()
on its error paths. This provides more concise error logging and helps
identify issues more easily during probe.

> diff --git a/drivers/input/keyboard/snvs_pwrkey.c b/drivers/input/keyboard/snvs_pwrkey.c
> index 954055aaf6e29..748196fcce75b 100644
> --- a/drivers/input/keyboard/snvs_pwrkey.c
> +++ b/drivers/input/keyboard/snvs_pwrkey.c

[ ... ]

> @@ -209,16 +203,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 since dev_pm_set_wake_irq() is called here
without a corresponding cleanup mechanism, does this cause a resource leak?

When the driver is unbound via sysfs, the allocated wake_irq structure is
leaked and the device remains marked with a wakeirq because there is no
.remove callback or devm action to call dev_pm_clear_wake_irq().

Will a subsequent rebind cause dev_pm_set_wake_irq() to fail with -EEXIST
because dev->power.wakeirq is still populated?

This is also a pre-existing issue, but I noticed something else regarding the
wake event handling.

drivers/input/keyboard/snvs_pwrkey.c:imx_snvs_pwrkey_interrupt() {
    ...
	pm_wakeup_event(input->dev.parent, 0);
    ...
}

And later in the driver:

drivers/input/keyboard/snvs_pwrkey.c:imx_imx_snvs_check_for_events() {
    ...
		pm_relax(pdata->input->dev.parent);
    ...
}

[Severity: High]
Does this incorrect pairing of pm_wakeup_event(dev, 0) and pm_relax(dev) fail
to keep the system awake?

Passing a 0 ms timeout to pm_wakeup_event() activates and immediately
deactivates the wakeup source. Because the wakeup source is already inactive,
the later call to pm_relax() acts as a no-op.

Does this mean the system is free to enter auto-sleep immediately after the
interrupt and before the timer executes, potentially delaying or dropping the
power key event delivery? Would it be better to use pm_stay_awake() here
instead?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260604-pwrkey-cleanup-v2-0-59fa0ddf0d7c@nxp.com?part=1