Re: [PATCH v2 2/2] pps: clients: gpio: release pins to an inactive state on remove and shutdown

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] pps-gpio: unconditional probe failure for boards without pinctrl bindings
- [Medium] pps-gpio: pins left in 'default' state on probe failure
--

commit 8a9e09ee4e79423b318933ea7fb883c1d285a638
Author: Eliav Farber <farbere@amazon.com>

pps: clients: gpio: release pins to an inactive state on remove and shutdown

This patch looks up an optional "inactive" pinctrl state in probe and
selects it on remove and shutdown to release the pins. The goal is to
prevent pins from staying stuck in the GPIO function for subsequent
executions like kexec or after the driver is unbound.

> diff --git a/drivers/pps/clients/pps-gpio.c b/drivers/pps/clients/pps-gpio.c
> index 73ec2c7335e51..e619c7bb2f78a 100644
> --- a/drivers/pps/clients/pps-gpio.c
> +++ b/drivers/pps/clients/pps-gpio.c

[ ... ]

> @@ -96,6 +99,51 @@ static void pps_gpio_echo_timer_callback(struct timer_list *t)
>  	gpiod_set_value(info->echo_pin, 0);
>  }
>  
> +/*
> + * Look up the optional "inactive" pinctrl state: the mux to restore when
> + * pps-gpio is unbound or the system is shut down. It is only meaningful
> + * paired with a "default" state, which the driver core applies before probe
> + * to mux the pins for PPS use. A board that describes neither is unaffected;
> + * one that describes "inactive" without "default" is rejected, since
> + * releasing pins that were never put into a defined PPS state is incoherent.
> + */
> +static int pps_gpio_get_pins(struct device *dev)
> +{
> +	struct pps_gpio_device_data *data = dev_get_drvdata(dev);
> +	struct pinctrl_state *pins_default;
> +
> +	data->pinctrl = devm_pinctrl_get(dev);
> +	if (IS_ERR(data->pinctrl))
> +		return dev_err_probe(dev, PTR_ERR(data->pinctrl),
> +				     "failed to get pinctrl\n");

[Severity: High]
If a board doesn't have explicit pinctrl bindings, won't devm_pinctrl_get()
return -ENODEV?

The driver core typically ignores -ENODEV to support devices without explicit
pinmuxes. Would treating it as a fatal error here cause the probe to fail
unconditionally on those boards and break existing setups?

> +
> +	/* The "inactive" state is optional; without it there is nothing to do. */
> +	data->pins_inactive = pinctrl_lookup_state(data->pinctrl, "inactive");
> +	if (IS_ERR(data->pins_inactive)) {
> +		data->pins_inactive = NULL;
> +		return 0;
> +	}

[ ... ]

> @@ -161,6 +209,11 @@ static int pps_gpio_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> +	/* pinctrl setup (optional states) */
> +	ret = pps_gpio_get_pins(dev);
> +	if (ret)
> +		return ret;
> +
>  	/* IRQ setup */
>  	ret = gpiod_to_irq(data->gpio_pin);
>  	if (ret < 0) {

[Severity: Medium]
If an error occurs later in the probe function, for example in gpiod_to_irq()
or pps_register_source(), the function returns early:

pps_gpio_probe() {
	...
	ret = gpiod_to_irq(data->gpio_pin);
	if (ret < 0) {
		dev_err(dev, "failed to map GPIO to IRQ: %d\n", ret);
		return -EINVAL;
	}
	...
}

Since the driver core applies the 'default' pinctrl state before probe begins,
and these error paths exit without calling pps_gpio_release_pins(), would
the pins be left stuck in the 'default' state if probe fails?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260916182641.9768-1-farbere@amazon.com?part=2