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

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

On Tue, Aug 18, 2026 at 06:27:21PM +0200, Alvin Šipraga wrote:
> 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.

No, not at all. Especially something that was identified as a
pre-existing issue will not usually block the acceptance.

And please, take Sashiko's reviews with a grain of salt, not everything
that it identifies as an issue is actually correct finding.

Thanks.

-- 
Dmitry