Re: [PATCH v4 2/3] gpiolib: add get_config() and gpiochip_generic_get_config()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] gpiochip_generic_get_config() returns an unpacked argument instead of the documented packed configuration format.
--

commit 76e13bb42cd8827a4ba44cd0214563286c510f1f
Author: Mehmet Fide <mehmet.fide@screeningeagle.com>

gpiolib: add get_config() and gpiochip_generic_get_config()

A chip with a pin control backend sets a line's configuration through
set_config() and gpiochip_generic_config(), but has no way to read one
back. gpio-mmio needs that to learn the direction of a line whose
direction lives in the pin controller.

>     Add the optional get_config() callback, taking the packed parameter to
>     query and returning the packed answer the way pinctrl_gpio_get_config()
>     does, and gpiochip_generic_get_config() as the pin control backed

[Severity: Medium]
Does pinctrl_gpio_get_config() actually return a packed answer?

Looking at drivers/pinctrl/core.c, pinctrl_gpio_get_config() intentionally
strips the parameter and returns only the unpacked argument:

int pinctrl_gpio_get_config(...) {
    ...
    *config = pinconf_to_config_argument(*config);
    return 0;
}

[ ... ]

> diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
> index 17511434ed077..5c81cce691738 100644
> --- a/include/linux/gpio/driver.h
> +++ b/include/linux/gpio/driver.h
> @@ -359,6 +359,10 @@ struct gpio_irq_chip {
>   * @set_config: optional hook for all kinds of settings. Uses the same
>   *	packed config format as generic pinconf. Must return 0 on success and
>   *	a negative error number on failure.
> + * @get_config: optional hook to read back a setting. Uses the same packed
> + *	config format as generic pinconf: the parameter to query goes in, the
> + *	packed parameter and argument come out. Must return 0 on success and a
> + *	negative error number on failure.

[Severity: Medium]
Because pinctrl_gpio_get_config() unpacks the argument, does this documented
API contract contradict the actual implementation?

If a driver follows this documentation and parses the returned value using
pinconf_to_config_argument(), it will shift the already-unpacked value right
by 8 bits, yielding 0.

[ ... ]

> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index ef8ccaf17c9ce..44328aed16465 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -2440,6 +2440,29 @@ int gpiochip_generic_config(struct gpio_chip *gc, unsigned int offset,
>  }
>  EXPORT_SYMBOL_GPL(gpiochip_generic_config);
>  
> +/**
> + * gpiochip_generic_get_config() - read back the configuration of a pin
> + * @gc: the gpiochip owning the GPIO
> + * @offset: the offset of the GPIO to query
> + * @config: the packed parameter to query, replaced by the packed answer
> + *
> + * Returns:
> + * 0 on success, or negative errno on failure.
> + */
> +int gpiochip_generic_get_config(struct gpio_chip *gc, unsigned int offset,
> +				unsigned long *config)
> +{
> +#ifdef CONFIG_PINCTRL
> +	if (list_empty(&gc->gpiodev->pin_ranges))
> +		return -ENOTSUPP;
> +
> +	return pinctrl_gpio_get_config(gc, offset, config);

[Severity: Medium]
Will returning this unpacked value directly cause logic errors for drivers
expecting a packed configuration?

Returning the unpacked output of pinctrl_gpio_get_config() here finalizes
the API contract violation.

Any callers, such as the gpio-mmio driver in patch 3/3, that rely on the
actual returned unpacked value will bypass the documented API contract. If
the value was truly packed as documented, config would always evaluate to
true since the parameter is non-zero.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260903073143.2068494-1-mehmet.fide@gmail.com?part=2