Re: [PATCH RFC 3/4] Input: gpio-keys - add regulator to gpio_keys

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Tying the regulator's power state to the input device's open()/close() lifecycle leaves interrupts active while the hardware is unpowered, causing an interrupt storm.
- [Medium] System wakeup functionality is broken because regulators are disabled if the input device is closed.
- [Medium] Unsafe hardware access during system resume causes spurious key events.
- [Medium] Missing pdata->disable() in the gpio_keys_open() error path leads to a resource leak.
- [Low] Incorrect teardown order in gpio_keys_close() violates LIFO symmetry.
--

commit d47e6012a488ac21d0005ba6dd4aa95a285831d3
Author: Griffin Kroah-Hartman <griffin.kroah@fairphone.com>

Input: gpio-keys - add regulator to gpio_keys

This commit allows gpio-keys to have a vdd power supply managed through
regulators.

> diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
> index e196174856796..2199ce7455cfc 100644
> --- a/drivers/input/keyboard/gpio_keys.c
> +++ b/drivers/input/keyboard/gpio_keys.c

[ ... ]

> @@ -729,6 +730,7 @@ static int gpio_keys_open(struct input_dev *input)
>  	struct gpio_keys_drvdata *ddata = input_get_drvdata(input);
>  	const struct gpio_keys_platform_data *pdata = ddata->pdata;
>  	int error;
> +	int i;
>  
>  	if (pdata->enable) {
>  		error = pdata->enable(input->dev.parent);
> @@ -736,19 +738,48 @@ static int gpio_keys_open(struct input_dev *input)
>  			return error;
>  	}
>  
> +	for (i = 0; i < pdata->nbuttons; i++) {
> +		const struct gpio_keys_button *button = &pdata->buttons[i];
> +
> +		if (!button->regulator)
> +			continue;
> +		error = regulator_enable(button->regulator);
> +		if (error)
> +			goto reg_err;
> +	}

Because interrupts are requested unconditionally in gpio_keys_probe() and
remain unmasked, tying the regulator power state to the input device's
open()/close() lifecycle leaves the physical button unpowered when the device
is closed. Could this allow the GPIO pin to float and trigger an interrupt
storm?

Also, if the input device is closed before suspend, the regulator remains
disabled. Does this prevent an unpowered physical button from generating the
electrical signal required to trigger a wakeup interrupt?

Additionally, gpio_keys_resume() unconditionally calls
gpio_keys_report_state(), which reads the current state of all GPIOs. If the
device is closed, the regulators are powered off. Can reading unpowered GPIOs
return floating states that inject spurious phantom key press events on every
resume?

> +
>  	/* Report current state of buttons that are connected to GPIOs */
>  	gpio_keys_report_state(ddata);
>  
>  	return 0;
> +
> +reg_err:
> +	for (--i; i >= 0; i--) {
> +		const struct gpio_keys_button *button = &pdata->buttons[i];
> +
> +		if (!button->regulator)
> +			continue;
> +		regulator_disable(button->regulator);
> +	}
> +	return error;

If regulator_enable() fails and we jump to reg_err, we return the error but
fail to call pdata->disable(). Does this leave the platform state permanently
active and leak resources since pdata->enable() was called earlier?

>  }
>  
>  static void gpio_keys_close(struct input_dev *input)
>  {
>  	struct gpio_keys_drvdata *ddata = input_get_drvdata(input);
>  	const struct gpio_keys_platform_data *pdata = ddata->pdata;
> +	int i;
>  
>  	if (pdata->disable)
>  		pdata->disable(input->dev.parent);
> +
> +	for (i = 0; i < pdata->nbuttons; i++) {
> +		const struct gpio_keys_button *button = &pdata->buttons[i];
> +
> +		if (!button->regulator)
> +			continue;
> +		regulator_disable(button->regulator);
> +	}
>  }

Is the teardown order correct here? In gpio_keys_open(), pdata->enable() is
called before the regulators are enabled. Should gpio_keys_close() reverse
this sequence by disabling the regulators before calling pdata->disable() to
maintain LIFO symmetry?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260508-gpiokeys-vdd-supply-v1-0-0bb32e8e6428@fairphone.com?part=3