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

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

[Alvin Šipraga]

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 <alvin.sipraga@analog.com>
---
 drivers/input/keyboard/adp5588-keys.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/input/keyboard/adp5588-keys.c b/drivers/input/keyboard/adp5588-keys.c
index 40371f5bd9ba..4f0ddff5baba 100644
--- a/drivers/input/keyboard/adp5588-keys.c
+++ b/drivers/input/keyboard/adp5588-keys.c
@@ -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,
--  
Alvin Šipraga <alvin.sipraga@analog.com>

Re: [PATCH] Input: adp5588-keys - cache GPIO state before registering the gpiochip

[Nuno Sá]

On Tue, Aug 18, 2026 at 06:00:02PM +0200, Alvin Šipraga wrote:
> 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 <alvin.sipraga@analog.com>
> ---

Reviewed-by: Nuno Sá <nuno.sa@analog.com>

>  drivers/input/keyboard/adp5588-keys.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/input/keyboard/adp5588-keys.c b/drivers/input/keyboard/adp5588-keys.c
> index 40371f5bd9ba..4f0ddff5baba 100644
> --- a/drivers/input/keyboard/adp5588-keys.c
> +++ b/drivers/input/keyboard/adp5588-keys.c
> @@ -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,
> --  
> Alvin Šipraga <alvin.sipraga@analog.com>
> 

Re: [PATCH] Input: adp5588-keys - cache GPIO state before registering the gpiochip

[sashiko-bot]

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

Pre-existing issues:
- [High] Missing error handling for I2C reads causes out-of-bounds memory accesses and hardware state corruption.
--

--- Patch [1]: [PATCH] Input: adp5588-keys - cache GPIO state before registering the gpiochip ---
Note: The format of this review has been altered to a free-form summary
due to system recitation restrictions. No direct patch quoting is used.

commit d8a90d44c5e4df663ac4d3f942090902da1d6732
Author: Alvin Šipraga <alvin.sipraga@analog.com>
Subject: Input: adp5588-keys - cache GPIO state before registering the gpiochip

This commit reorders the GPIO state caching to occur before the
gpiochip is registered. This prevents gpiochip operations from clobbering
pre-programmed GPIO states set by the bootloader.

[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?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260818-adp5588-gpio-cache-v1-1-650a2674fc0d@analog.com?part=1

Re: [PATCH] Input: adp5588-keys - cache GPIO state before registering the gpiochip

[Alvin Šipraga]

Thanks for your review sashiko-bot,

On Tue, Aug 18, 2026 at 04:11:19PM +0000, sashiko-bot@kernel.org 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). 
> 
> 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.

> 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?

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.

Alvin