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 [off-list ref]
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/b3c94552-c104-42e3-be15-7e8362e8039e@gmail.com/ (local)
Link: https://lore.kernel.org/all/afJXG4_rtaj3l2Dk@google.com/ (local)
Link: https://lore.kernel.org/all/ajQ-CtU131FAJ9ES@google.com/ (local)
quoted hunk ↗ jump to hunk
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>
[ ... ]
quoted hunk ↗ jump to hunk
@@ -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.
[ ... ]
quoted hunk ↗ jump to hunk
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>
[ ... ]
quoted hunk ↗ jump to hunk
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.
quoted hunk ↗ jump to hunk
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/20260710211854.1371746-1-arnd@kernel.org?part=2