Re: [PATCH 2/6] [v6] Input: gpio-keys: make legacy gpiolib optional

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

New issues:
- [High] Legacy platforms that specify both a valid GPIO and an IRQ for a button are forced into purely interrupt-driven mode, degrading them to simulated pulse buttons.
- [Low] The patch includes a non-existent header file `linux/gpio/legacy.h`, unconditionally breaking the build.

Pre-existing issues:
- [High] The Rohm MFD drivers dynamically mutate a globally shared static struct during probe, breaking reentrancy and violating MFD guidelines.
--

--- Patch [2]: [PATCH 2/6] [v6] Input: gpio-keys: make legacy gpiolib optional ---
commit 1e2acc08348477940313314a2e3563828ed3ae8b
Author: Arnd Bergmann <[email protected]>

Input: gpio-keys: make legacy gpiolib optional

Most users of gpio-keys and gpio-keys-polled use modern gpiolib interfaces,
but there are still number of ancient sh, arm32 and x86 machines that have
never been converted. Add an #ifdef block for the parts of the driver that
are only used on those legacy machines.

Link: https://lore.kernel.org/all/[email protected]/
Link: https://lore.kernel.org/all/[email protected]/
Link: https://lore.kernel.org/all/[email protected]/

> diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
> --- a/drivers/input/keyboard/gpio_keys.c
> +++ b/drivers/input/keyboard/gpio_keys.c
> @@ -23,8 +23,8 @@
>  #include <linux/input.h>
>  #include <linux/gpio_keys.h>
>  #include <linux/workqueue.h>
> -#include <linux/gpio.h>
>  #include <linux/gpio/consumer.h>
> +#include <linux/gpio/legacy.h>

[Severity: Low]
Does this header file exist in the source tree? Because this include is
outside of any #ifdef blocks, it looks like it will unconditionally break
the build for anyone compiling with CONFIG_KEYBOARD_GPIO enabled.

>  #include <linux/of.h>
>  #include <linux/of_irq.h>
>  #include <linux/spinlock.h>

[ ... ]

> @@ -528,7 +528,8 @@ static int gpio_keys_setup_key(struct platform_device *pdev,
>  			 */
>  			bdata->gpiod = NULL;
>  		}
> -	} else if (gpio_is_valid(button->gpio)) {
> +#ifdef CONFIG_GPIOLIB_LEGACY
> +	} else if (!button->irq && gpio_is_valid(button->gpio)) {

[Severity: High]
Does this new !button->irq condition inadvertently break legacy boards that
legitimately supply both a valid .gpio and an .irq?

For example, in arch/arm/mach-sa1100/assabet.c, assabet_keys_buttons defines
buttons with both .gpio = 0 and .irq = IRQ_GPIO0. Since the GPIO is no
longer requested when an IRQ is present, wouldn't the driver fall back to
purely interrupt-driven mode?

If so, this would lack a GPIO descriptor to read the physical button level,
turning holdable physical buttons into simulated momentary pulses via the
auto-release timer.

>  		/*
>  		 * Legacy GPIO number, so request the GPIO here and
>  		 * convert it to descriptor.

[ ... ]

> diff --git a/drivers/input/keyboard/gpio_keys_polled.c b/drivers/input/keyboard/gpio_keys_polled.c
> --- a/drivers/input/keyboard/gpio_keys_polled.c
> +++ b/drivers/input/keyboard/gpio_keys_polled.c
> @@ -18,8 +18,8 @@
>  #include <linux/input.h>
>  #include <linux/ioport.h>
>  #include <linux/platform_device.h>
> -#include <linux/gpio.h>
>  #include <linux/gpio/consumer.h>
> +#include <linux/gpio/legacy.h>

[Severity: Low]
Similar to gpio_keys.c, does this unconditionally break the build since
linux/gpio/legacy.h is missing from the tree?

>  #include <linux/gpio_keys.h>
>  #include <linux/property.h>

[ ... ]

> diff --git a/drivers/mfd/rohm-bd71828.c b/drivers/mfd/rohm-bd71828.c
> --- a/drivers/mfd/rohm-bd71828.c
> +++ b/drivers/mfd/rohm-bd71828.c
> @@ -39,7 +39,6 @@
>  
>  static struct gpio_keys_button button = {
>  	.code = KEY_POWER,
> -	.gpio = -1,
>  	.type = EV_KEY,
>  	.wakeup = 1,
>  };

[Severity: High]
This isn't a bug introduced by this patch, but this static struct is modified
dynamically during bd71828_i2c_probe() when assigning button.irq = ret.

Since mfd_add_devices() only shallow-copies the platform data, doesn't
mutating a globally shared static struct during probe break reentrancy?
Multiple instances of the PMIC or repeated probes due to unbind/deferral
could clobber each other's IRQ number here.

> diff --git a/drivers/mfd/rohm-bd718x7.c b/drivers/mfd/rohm-bd718x7.c
> --- a/drivers/mfd/rohm-bd718x7.c
> +++ b/drivers/mfd/rohm-bd718x7.c
> @@ -20,7 +20,6 @@
>  
>  static struct gpio_keys_button button = {
>  	.code = KEY_POWER,
> -	.gpio = -1,
>  	.type = EV_KEY,
>  };

[Severity: High]
This is a pre-existing issue, but just like in rohm-bd71828.c, mutating this
shared static struct during bd718xx_i2c_probe() violates MFD guidelines and
could cause race conditions with concurrent or repeated probes.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2