Re: [PATCH v10 4/5] gpio: shared: detach the proxy device from struct gpio_shared_ref

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] gpio: shared: unlocked read of ref->proxy_dev in gpiochip_setup_shared()
--

commit 4ce42ba5e6d6a1657f4afe872398baaf6f83b930
Author: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>

gpio: shared: detach the proxy device from struct gpio_shared_ref

This commit moves the auxiliary device structure out of struct
gpio_shared_ref and allocates it dynamically during proxy device setup so
that its lifetime is managed by its embedded struct device reference count
rather than being zeroed on re-initialization.

> diff --git a/drivers/gpio/gpiolib-shared.c b/drivers/gpio/gpiolib-shared.c
> index 6b18386493387..9d73a02e99890 100644
> --- a/drivers/gpio/gpiolib-shared.c
> +++ b/drivers/gpio/gpiolib-shared.c
> @@ -35,11 +35,10 @@ struct gpio_shared_ref {
>  	/* GPIO flags this consumer uses for the request. */
>  	enum gpiod_flags flags;
>  	char *con_id;
> -	int dev_id;
>  	/* Protects the auxiliary device struct and the lookup table. */
>  	struct mutex lock;
>  	struct lock_class_key lock_key;
> -	struct auxiliary_device adev;
> +	struct auxiliary_device *proxy_dev;
>  	struct gpiod_lookup_table *lookup;
>  	bool is_reset_gpio;
>  };

[ ... ]

> @@ -341,6 +348,10 @@ static int gpio_shared_make_adev(struct gpio_device *gdev,
>  	pr_debug("Created an auxiliary GPIO proxy %s for GPIO device %s\n",
>  		 dev_name(&adev->dev), gpio_device_get_label(gdev));
>  
> +	scoped_guard(mutex, &ref->lock) {
> +		ref->proxy_dev = adev;
> +	}
> +
>  	return 0;
>  }

[ ... ]

> @@ -511,10 +522,17 @@ int gpio_shared_add_proxy_lookup(struct device *consumer, struct fwnode_handle *
>  	return -ENOENT;
>  }
>  
> -static void gpio_shared_remove_adev(struct auxiliary_device *adev)
> +static void gpio_shared_remove_proxy_dev(struct gpio_shared_ref *ref)
>  {
> -	auxiliary_device_delete(adev);
> -	auxiliary_device_uninit(adev);
> +	struct auxiliary_device *proxy;
> +
> +	scoped_guard(mutex, &ref->lock) {
> +		proxy = ref->proxy_dev;
> +		ref->proxy_dev = NULL;
> +	}
> +
> +	auxiliary_device_delete(proxy);
> +	auxiliary_device_uninit(proxy);
>  }

[ ... ]

> @@ -617,7 +635,7 @@ int gpiochip_setup_shared(struct gpio_chip *gc)
>  
>  	list_for_each_entry(entry, &gpio_shared_list, list) {
>  		list_for_each_entry(ref, &entry->refs, list) {
> -			if (gdev->dev.parent == &ref->adev.dev) {
> +			if (ref->proxy_dev && gdev->dev.parent == &ref->proxy_dev->dev) {

[Severity: Medium]
Should this check in gpiochip_setup_shared() hold ref->lock while reading
ref->proxy_dev?

In drivers/gpio/gpiolib.c, gpiochip_add_data_with_key() and
gpiochip_remove() drop gpio_devices_lock before calling
gpiochip_setup_shared() and gpio_device_teardown_shared(), so
gpiochip_setup_shared() on one CPU can run concurrently with
gpio_shared_make_proxy_dev() or gpio_shared_remove_proxy_dev() on another
CPU.

While gpio_shared_make_proxy_dev() and gpio_shared_remove_proxy_dev() update
ref->proxy_dev under ref->lock, gpiochip_setup_shared() reads ref->proxy_dev
without holding ref->lock. Because struct device dev is at offset 0 of
struct auxiliary_device, &ref->proxy_dev->dev evaluates to the pointer value
of ref->proxy_dev itself. If ref->proxy_dev is non-NULL on the first load in
the condition and gpio_shared_remove_proxy_dev() concurrently clears
ref->proxy_dev to NULL before the second load, &ref->proxy_dev->dev can
evaluate to NULL. For a GPIO chip being registered with gdev->dev.parent ==
NULL, gdev->dev.parent == &ref->proxy_dev->dev would then evaluate to true,
falsely setting GPIOD_FLAG_SHARED_PROXY on an unrelated GPIO chip.

Also, in gpio_shared_make_proxy_dev(), auxiliary_device_add(adev) is called
before ref->proxy_dev is assigned under ref->lock, and in
gpio_shared_remove_proxy_dev(), ref->proxy_dev is cleared to NULL before
auxiliary_device_delete(proxy) completes. Could the proxy driver's probe run
gpiochip_setup_shared() while ref->proxy_dev is still NULL (or already
cleared to NULL) and miss setting GPIOD_FLAG_SHARED_PROXY?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103-gpio-shared-fwnode-lookup-v10-0-c0c592c8ab0a@oss.qualcomm.com?part=4