Re: [PATCH v3 1/3] gpio: shared-proxy: Fix race between concurrent first users

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] gpio: shared-proxy: unreset vote state in gpio_shared_proxy_direction_input()
--

commit f02d19c15e193895e840a218407e46f54481d01c
Author: Shawn Guo <shengchao.guo@oss.qualcomm.com>

gpio: shared-proxy: Fix race between concurrent first users

This commit fixes a race between concurrent first users of a shared GPIO
by tracking the number of proxies that have configured the line (via
direction setting or voting) in cfgcnt rather than checking usecnt == 1.

> diff --git a/drivers/gpio/gpio-shared-proxy.c b/drivers/gpio/gpio-shared-proxy.c
> index bc69b8729d196..0d1d724a6a376 100644
> --- a/drivers/gpio/gpio-shared-proxy.c
> +++ b/drivers/gpio/gpio-shared-proxy.c

[ ... ]

> @@ -155,15 +184,21 @@ static int gpio_shared_proxy_direction_input(struct gpio_chip *gc,
>  	struct gpio_shared_proxy_data *proxy = gpiochip_get_data(gc);
>  	struct gpio_shared_desc *shared_desc = proxy->shared_desc;
>  	struct gpio_desc *desc = shared_desc->desc;
> -	int dir;
> +	int ret, dir;
>  
>  	guard(mutex)(&shared_desc->mutex);
>  
> -	if (shared_desc->usecnt == 1) {
> +	if (gpio_shared_proxy_may_init(proxy)) {
>  		dev_dbg(proxy->dev,
> -			"Only one user of this shared GPIO, allowing to set direction to input\n");
> +			"No other user configured this shared GPIO, allowing to set direction to input\n");
> +
> +		ret = gpiod_direction_input(desc);
> +		if (ret)
> +			return ret;
> +
> +		gpio_shared_proxy_mark_configured(proxy);
>  
> -		return gpiod_direction_input(desc);
> +		return 0;
>  	}

[Severity: Medium]
Should gpio_shared_proxy_direction_input() also reset shared_desc->votecnt
and proxy->voted_change here, just like gpio_shared_proxy_direction_output()
does when gpio_shared_proxy_may_init(proxy) is true?

If a proxy becomes the sole configured user (shared_desc->cfgcnt == 1 and
proxy->configured == true) while holding an active vote (proxy->voted_change
== true and shared_desc->votecnt == 1) -- for example, after calling
gpiod_direction_output() with 0 followed by gpiod_set_value_cansleep() with
1, after voting as a GPIOD_ASIS consumer, or after another consumer that
initially set the direction to output-low releases its descriptor -- and
then reconfigures the line as input via gpio_shared_proxy_direction_input(),
gpiod_direction_input(desc) clears GPIOD_FLAG_IS_OUT on shared_desc->desc
while leaving proxy->voted_change == true and shared_desc->votecnt == 1.

When the proxy is later released in gpio_shared_proxy_free():

drivers/gpio/gpio-shared-proxy.c:gpio_shared_proxy_free() {
	if (proxy->voted_change) {
		ret = gpio_shared_proxy_set_unlocked(proxy, shared_desc->def_val);
		if (ret)
			dev_err(proxy->dev,
				"Failed to unset the shared GPIO value on release: %d\n", ret);
	}
}

gpio_shared_proxy_set_unlocked() -> gpiod_set_value_cansleep() ->
gpiod_set_raw_value_commit() will see GPIOD_FLAG_IS_OUT cleared on the
input descriptor, fail with -EPERM, and log an error (and at this commit,
prior to commit "gpio: shared-proxy: Drop the vote of a released proxy on
write failure", leave shared_desc->votecnt and proxy->voted_change leaked).

[ ... ]

> @@ -189,18 +226,20 @@ static int gpio_shared_proxy_direction_output(struct gpio_chip *gc,
>  
>  	guard(mutex)(&shared_desc->mutex);
>  
> -	if (shared_desc->usecnt == 1) {
> +	if (gpio_shared_proxy_may_init(proxy)) {
>  		dev_dbg(proxy->dev,
> -			"Only one user of this shared GPIO, allowing to set direction to output with value '%s'\n",
> +			"No other user configured this shared GPIO, allowing to set direction to output with value '%s'\n",
>  			str_high_low(value));
>  
>  		ret = gpiod_direction_output(desc, value);
>  		if (ret)
>  			return ret;
>  
> +		/* Only this proxy can have voted, so the tally is ours to reset */
>  		shared_desc->def_val = value;
>  		shared_desc->votecnt = 0;
>  		proxy->voted_change = false;
> +		gpio_shared_proxy_mark_configured(proxy);
>  
>  		return 0;
>  	}

[ ... ]

>
>
> -- 
> 2.47.3
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261017024335.1432020-1-shengchao.guo@oss.qualcomm.com?part=1