Input: adp5588-keys - cache GPIO state before registering the gpiochip

So as not to clobber any pre-programmed GPIO state in the execution of its gpiochip ops, the driver caches things during probe time. However, since those ops can be called both during and immediately after the call to devm_gpiochip_add_data(), it is imperative that things are cached before that. That's not the case right now, so reorder the two steps to prevent any clobbering.

In a conrete example which motivated this change, a bootloader was preconfiguring an important GPIO output to HIGH before booting the kernel. Linux would then inadvertently set that output to LOW while configuring a GPIO hog on a discrete GPIO line within the same 8-bit bank (because the cached value was 0=LOW).

Fixes ba9f507a1bea ("Input: adp5588-keys - export unused GPIO pins")

Signed-off-by Alvin Šipraga <[email protected]>

---

 drivers/input/keyboard/adp5588-keys.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

drivers/input/keyboard/adp5588-keys.c +6 -7

index 40371f5bd9ba..4f0ddff5baba 100644
@@ -446,12 +446,6 @@ static int adp5588_gpio_add(struct adp5588_kpad *kpad)
 
 	mutex_init(&kpad->gpio_lock);
 
-	error = devm_gpiochip_add_data(dev, &kpad->gc, kpad);
-	if (error) {
-		dev_err(dev, "gpiochip_add failed: %d\n", error);
-		return error;
-	}
-
 	for (i = 0; i <= ADP5588_BANK(ADP5588_MAXGPIO); i++) {
 		kpad->dat_out[i] = adp5588_read(kpad->client,
 						GPIO_DAT_OUT1 + i);

@@ -459,6 +453,12 @@ static int adp5588_gpio_add(struct adp5588_kpad *kpad)
 		kpad->pull_dis[i] = adp5588_read(kpad->client, GPIO_PULL1 + i);
 	}
 
+	error = devm_gpiochip_add_data(dev, &kpad->gc, kpad);
+	if (error) {
+		dev_err(dev, "gpiochip_add failed: %d\n", error);
+		return error;
+	}
+
 	return 0;
 }
 
 
---

base-commit: 44f3468a0aef1aabdad551898ab7cfa2a9d20e99
change-id: 20260818-adp5588-gpio-cache-23fb8d0a3190

Best regards,

Signature · 1 line

Alvin Šipraga <[email protected]>

[Severity: High]
This isn't a bug introduced by this patch, but this patch actively exacerbates the consequences of a pre-existing issue with missing error handling on I2C reads.

If adp5588_read() fails due to a bus fault, it returns a negative error code (e.g., -EIO). In adp5588_gpio_add(), these negative values are silently truncated to 8-bit unsigned values and stored in the driver's cached GPIO state (dat_out, dir, pull_dis).

Because this patch moves this caching to occur before devm_gpiochip_add_data(), when a GPIO hog is subsequently applied, the driver inadvertently clobbers the real hardware state by writing back this error-corrupted cache. Can this actively undermine the patch's stated goal of preserving bootloader GPIO configurations if an I2C read fails during probe?

Additionally, this same missing error check causes severe issues elsewhere. In adp5588_report_events(), the error is bitwise-ANDed with KEY_EV_MASK, resulting in an invalid, large key value. This yields a calculated code index that exceeds the bounds of the kpad->keycode array, causing an out-of-bounds heap read and leaking phantom key presses to userspace.

Finally, in adp5588_gpio_get_value(), a negative error code evaluates to true, incorrectly reporting the pin state as HIGH.

Would it be appropriate to add proper error checking to adp5588_read() or its callers to prevent state corruption and out-of-bounds memory access during I2C faults?

Thanks for your review sashiko-bot,

Quoted 10 lines · On Tue, Aug 18, 2026 at 04:11:19PM +0000, [email protected] wrote:

[Severity: High]
This isn't a bug introduced by this patch, but this patch actively exacerbates the consequences of a pre-existing issue with missing error handling on I2C reads.

If adp5588_read() fails due to a bus fault, it returns a negative error code (e.g., -EIO). In adp5588_gpio_add(), these negative values are silently truncated to 8-bit unsigned values and stored in the driver's cached GPIO state (dat_out, dir, pull_dis).

Quoted 6 lines

Because this patch moves this caching to occur before devm_gpiochip_add_data(), when a GPIO hog is subsequently applied, the driver inadvertently clobbers the real hardware state by writing back this error-corrupted cache. Can this actively undermine the patch's stated goal of preserving bootloader GPIO configurations if an I2C read fails during probe?

If the reads fail, I would not expect the driver to behave properly at all - neither before nor after this patch. It's true that this patch might trade some broken behavior for another type of broken behavior though. But only on systems where I2C reads are failing.

If the argument is that such a trade can be considered a regression, I would point out that the suggestion here - to add error checking and to fail probe on bus errors - would invariably cause probe failure on such systems with buggy I2C reads. That's also a regression.

Quoted 9 lines

Additionally, this same missing error check causes severe issues elsewhere. In adp5588_report_events(), the error is bitwise-ANDed with KEY_EV_MASK, resulting in an invalid, large key value. This yields a calculated code index that exceeds the bounds of the kpad->keycode array, causing an out-of-bounds heap read and leaking phantom key presses to userspace.

Finally, in adp5588_gpio_get_value(), a negative error code evaluates to true, incorrectly reporting the pin state as HIGH.

Quoted 3 lines

Would it be appropriate to add proper error checking to adp5588_read() or its callers to prevent state corruption and out-of-bounds memory access during I2C faults?

Sure the driver can be improved by adding error checking, but that would be a much larger change. I hope that is not an impediment to getting this particular issue fixed.

So as not to clobber any pre-programmed GPIO state in the execution of its gpiochip ops, the driver caches things during probe time. However, since those ops can be called both during and immediately after the call to devm_gpiochip_add_data(), it is imperative that things are cached before that. That's not the case right now, so reorder the two steps to prevent any clobbering.

In a conrete example which motivated this change, a bootloader was preconfiguring an important GPIO output to HIGH before booting the kernel. Linux would then inadvertently set that output to LOW while configuring a GPIO hog on a discrete GPIO line within the same 8-bit bank (because the cached value was 0=LOW).

Fixes ba9f507a1bea ("Input: adp5588-keys - export unused GPIO pins")

Signed-off-by Alvin Šipraga <[email protected]>