Re: [PATCH v4 2/2] Input: gpio-keys - support wakeup pinctrl state on suspend

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] input: spurious key press on resume due to LIFO violation in pinctrl teardown
--

commit 77748a917235cbadd6477449603ad11920308ecf
Author: Kendall Willis <k-willis@ti.com>

Input: gpio-keys - support wakeup pinctrl state on suspend

This commit adds support for selecting a wakeup pinctrl state on suspend and
restoring the default state on resume for GPIO devices. This enables wakeup
support on certain platforms where the GPIO controller is powered down during
suspend.

> diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
> index e196174856796..b8ecaaded6a44 100644
> --- a/drivers/input/keyboard/gpio_keys.c
> +++ b/drivers/input/keyboard/gpio_keys.c
[ ... ]
> @@ -1022,6 +1042,9 @@ gpio_keys_enable_wakeup(struct gpio_keys_drvdata *ddata)
>  	return 0;
>  
>  err_out:
> +	if (!IS_ERR_OR_NULL(ddata->pinctrl_wakeup))
> +		pinctrl_pm_select_default_state(ddata->input->dev.parent);
> +
[Severity: Medium]
Could this error path in gpio_keys_enable_wakeup() in
drivers/input/keyboard/gpio_keys.c trigger a spurious IRQ?

The default pinctrl state is restored before the successfully configured
wakeup IRQs are disabled. If switching the pinctrl state causes a glitch
on the GPIO line, an IRQ might fire prematurely.

>  	while (i--) {
>  		bdata = &ddata->data[i];
>  		if (bdata->button->wakeup)
> @@ -1038,6 +1061,9 @@ gpio_keys_disable_wakeup(struct gpio_keys_drvdata *ddata)
>  	struct gpio_button_data *bdata;
>  	int i;
>  
> +	if (!IS_ERR_OR_NULL(ddata->pinctrl_wakeup))
> +		pinctrl_pm_select_default_state(ddata->input->dev.parent);
> +
[Severity: Medium]
Can this ordering in gpio_keys_disable_wakeup() in
drivers/input/keyboard/gpio_keys.c inject a spurious key press on system
resume?

On resume, gpio_keys_disable_wakeup() restores the default pinctrl state
before disabling the wakeup IRQs and clearing bdata->suspended. If the
pinctrl state transition causes a line glitch, the IRQ fires while
bdata->suspended is still true.

The IRQ handler then sees bdata->suspended is true and explicitly
simulates a key press to handle transient wakeup events. If this key is
mapped to a power or sleep button, it might cause the system to immediately
re-suspend or power off.

Should the default pinctrl state be restored after disabling the IRQs and
clearing the suspended state to maintain a strict LIFO teardown order?

>  	for (i = 0; i < ddata->pdata->nbuttons; i++) {
>  		bdata = &ddata->data[i];
>  		bdata->suspended = false;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261005-upstream-gpio-wakeup-v4-1-96f092de4690@ti.com?part=2