Re: [PATCH v3 2/2] Input: gpio-keys - support wakeup pinctrl state on suspend

[Kendall Willis <k-willis@ti.com>]

On 16:02-20261001, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] input: gpio-keys: silent masking of pinctrl resource acquisition errors
> --
> 
> commit f85782ff9c9eb4bfa803e47eea6427b48cb93f09
> Author: Kendall Willis <k-willis@ti.com>
> 
> Input: gpio-keys - support wakeup pinctrl state on suspend
> 
> This commit adds support for selecting a wakeup pinctrl state during suspend
> to RAM on TI K3 AM62 devices. It retrieves the pinctrl state using
> devm_pinctrl_get() and looks up the wakeup state if the device is enabled.
> 
> > 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
> 
> [ ... ]
> 
> > @@ -884,6 +887,15 @@ static int gpio_keys_probe(struct platform_device *pdev)
> >  	platform_set_drvdata(pdev, ddata);
> >  	input_set_drvdata(input, ddata);
> >  
> > +	ddata->pinctrl = devm_pinctrl_get(dev);
> > +	if (IS_ERR(ddata->pinctrl)) {
> > +		error = PTR_ERR(ddata->pinctrl);
> > +		if (error == -EPROBE_DEFER)
> > +			return error;
> 
> [Severity: Medium]
> Does this code intentionally swallow all error codes from devm_pinctrl_get()
> other than -EPROBE_DEFER?
> 
> If devm_pinctrl_get() fails for other reasons, such as a memory allocation
> failure (-ENOMEM) or an invalid device tree configuration (-EINVAL), the
> error is silently ignored. This allows the driver to probe successfully but
> with silently broken wakeup functionality. 
> 
> Should the error be returned for all failure cases to avoid masking resource
> acquisition failures?

Pinctrl is optional to use for gpio-keys so we only care if
-EPROBE_DEFER is the error from pinctrl which would mean that it just
has not been probed yet. All other errors can fall through and the
pinctrl_wakeup variable would be left as NULL.

Best,
Kendall

> 
> > +	} else if (ddata->pinctrl) {
> > +		ddata->pinctrl_wakeup = pinctrl_lookup_state(ddata->pinctrl, "wakeup");
> > +	}
> > +
> >  	input->name = pdata->name ? : pdev->name;
> >  	input->phys = "gpio-keys/input0";
> >  	input->dev.parent = dev;
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20261001-upstream-gpio-wakeup-v3-0-6f3bcde367a3@ti.com?part=2