Re: [PATCH v2 6/9] Input: cap11xx - add reset gpio support

[Jun Yan <jerrysteve1101@gmail.com>]

> Thank you for your contribution! Sashiko AI review found 2 potential issue(=
> s) to consider:
> 
> New issues:
> - [Medium] Unintended glitch on the reset line during probe due to GPIOD_OU=
> T_LOW initialization.
> 
> Pre-existing issues:
> - [High] Unvalidated keycodes from Device Tree cause an out-of-bounds heap =
> write.
> --
> 
> commit 5befbfbb7019f474fe2d2d2e0e3da23976b75ada
> Author: Jun Yan <jerrysteve1101@gmail.com>
> 
> Input: cap11xx - add reset gpio support
> 
> Some CAP11xx devices (CAP1126/CAP1188) have a dedicated RESET pin. Add
> hardware reset operation to improve device reliability and ensure proper
> initialization on probe.
> 
> > diff --git a/drivers/input/keyboard/cap11xx.c b/drivers/input/keyboard/ca=
> p11xx.c
> > index 686174722204c..75746a8a22332 100644
> > --- a/drivers/input/keyboard/cap11xx.c
> > +++ b/drivers/input/keyboard/cap11xx.c
> 
> [ ... ]
> 
> > @@ -452,6 +457,17 @@ static int cap11xx_i2c_probe(struct i2c_client *i2c_=
> client)
> >  	if (IS_ERR(priv->regmap))
> >  		return PTR_ERR(priv->regmap);
> > =20
> > +	priv->reset_gpio =3D devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LO=
> W);
> > +	if (IS_ERR(priv->reset_gpio))
> > +		return dev_err_probe(dev, PTR_ERR(priv->reset_gpio),
> > +				     "Failed to get 'reset' GPIO\n");
> > +	if (priv->reset_gpio) {
> > +		gpiod_set_value_cansleep(priv->reset_gpio, 1);
> 
> [Severity: Medium]
> Does initializing the reset GPIO with GPIOD_OUT_LOW cause an unintended
> glitch on the reset line?
> 
> When devm_gpiod_get_optional() is called with GPIOD_OUT_LOW, the GPIO line
> is immediately driven to its logical low (inactive) state. If the hardware
> or bootloader previously left the device in a reset state, this prematurely
> pulls it out of reset.
> 
> The driver then immediately calls gpiod_set_value_cansleep() to assert
> the reset. This sequence creates an extremely short, unintended
> de-assert/assert glitch on the reset pin, which could violate hardware
> timing requirements.
> 
> Would it be safer to request the reset GPIO with GPIOD_OUT_HIGH (or
> GPIOD_ASIS) to guarantee it starts in the logically asserted state before
> we hold it for the required minimum duration?

Agreed, this also makes the code cleaner.

> 
> [Severity: High]
> This is a pre-existing issue, but does this code overflow the keybit array
> if a device tree provides unvalidated keycodes?
> 
> Later in cap11xx_i2c_probe(), the driver loops over the keycodes read from
> the device tree:
> 
> for (i =3D 0; i < cap->num_channels; i++)
>         __set_bit(priv->keycodes[i], priv->idev->keybit);
> 
> Since __set_bit() does not perform bounds checking, if a device tree
> specifies a keycode value greater than or equal to KEY_MAX, this will
> write outside the bounds of the keybit array in the input_dev structure,
> clobbering adjacent memory in the kernel heap.
> 
> Should we add a check against KEY_MAX before setting the bit?
> 
> > +		usleep_range(CAP11XX_T_RST_FILT_MIN_US, CAP11XX_T_RST_FILT_MIN_US * 2);
> > +		gpiod_set_value_cansleep(priv->reset_gpio, 0);
> > +		msleep(CAP11XX_T_RST_ON_MIN_MS);
> > +	}
> > +
> >  	error =3D regmap_read(priv->regmap, CAP11XX_REG_PRODUCT_ID, &val);
> 
> --=20
> Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20260612072237.1177=
> 304-1-jerrysteve1101@gmail.com?part=3D6
>