[PATCH v5 0/2] gpio: shared: replace platform lookup with dummy software nodes

[PATCH v5 0/2] gpio: shared: replace platform lookup with dummy software nodes

[Bartosz Golaszewski]

Abel reported to me a crash on Eliza linked to fwnode GPIO lookup for
shared GPIOs. Turns out we don't support no-device, fwnode-based lookup
for shared GPIOs. With the NULL-pointer dereference now fixed, this is
a rework that implements support for this use-case.

Sashiko pointed out several issues under v4. Some of them were bogus and
I fixed the rest.

Tested on Eliza and made sure the fwnode-only lookup works correctly.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
---
Changes in v5:
- Drop fixes already applied upstream
- Check ref->proxy_dev for NULL before dereferencing it
- Assign ref->proxy_fwnode with the shared reference lock taken
- Don't return uninitialized ret from gpio_shared_make_proxy_dev()
- Simplify error handling in gpio_shared_make_proxy_dev()
- Link to v4: https://patch.msgid.link/20261014-gpio-shared-fwnode-lookup-v4-0-2253ad645fd6@oss.qualcomm.com

Changes in v4:
- Fix arguments passed to ida_free() in error path
- Fix memory leak in error path
- Link to v3: https://patch.msgid.link/20261013-gpio-shared-fwnode-lookup-v3-0-4553c1a7d80f@oss.qualcomm.com

Changes in v3:
- Move struct auxiliary_device out of struct gpio_shared_ref
- Link to v2: https://patch.msgid.link/20261001-gpio-shared-fwnode-lookup-v2-0-9a40681713cc@oss.qualcomm.com

Changes in v2:
- Rework the shared GPIO management to replace platform lookup with
  software nodes associated with proxy GPIO controllers
- Link to v1: https://patch.msgid.link/20260929-gpio-shared-fwnode-lookup-v1-0-57d54811de1a@oss.qualcomm.com

---
Bartosz Golaszewski (2):
      gpio: shared: detach the proxy device from struct gpio_shared_ref
      gpio: shared: convert to using software nodes for GPIO shared proxy lookup

 drivers/gpio/gpiolib-shared.c | 158 +++++++++++++++++++++---------------------
 drivers/gpio/gpiolib-shared.h |  15 ++--
 drivers/gpio/gpiolib.c        |  20 +-----
 3 files changed, 89 insertions(+), 104 deletions(-)
---
base-commit: 3d229ce89862b8e82d71a9e11d6f840ca272a6d6
change-id: 20260929-gpio-shared-fwnode-lookup-e866faeb5bde

Best regards,
--  
Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>

[PATCH v5 1/2] gpio: shared: detach the proxy device from struct gpio_shared_ref

[Bartosz Golaszewski]

Sashiko reported a pre-existing issue with the reference-counted
auxiliary device being embedded in struct gpio_shared_ref. We
unconditionally memset() the struct when setting up the GPIO shared
proxy parent while there may still be references to it held by users.

While in practice this would never happen and sashiko seems to have
confused the parent auxiliary device and the separately allocated struct
device embedded in struct gpio_device, it's still a bad pattern and
could lead to memory corruption if a user deliberately took reference to
the auxiliary device via one of the lookup functions and kept it over
the GPIO parent's teardown.

Move the auxiliary device struct out of the shared reference structure
and allocate its memory separately on proxy setup.

Since we're already touching that field: change the name to something
more meaningful: it's more important to point out this is the proxy
device instead of indicating its type in the name.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
---
 drivers/gpio/gpiolib-shared.c | 61 +++++++++++++++++++++++++------------------
 1 file changed, 36 insertions(+), 25 deletions(-)

diff --git a/drivers/gpio/gpiolib-shared.c b/drivers/gpio/gpiolib-shared.c
index 8c9ac9d933049..c7893a5ac80a6 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;
 };
@@ -94,10 +93,6 @@ static struct gpio_shared_ref *gpio_shared_make_ref(struct fwnode_handle *fwnode
 			return NULL;
 	}
 
-	ref->dev_id = ida_alloc(&gpio_shared_ida, GFP_KERNEL);
-	if (ref->dev_id < 0)
-		return NULL;
-
 	ref->flags = flags;
 	ref->con_id = no_free_ptr(con_id_cpy);
 	ref->fwnode = fwnode;
@@ -306,31 +301,43 @@ static int gpio_shared_of_scan(void)
 }
 #endif /* CONFIG_OF */
 
-static void gpio_shared_adev_release(struct device *dev)
+static void gpio_shared_proxy_dev_release(struct device *dev)
 {
+	struct auxiliary_device *adev = to_auxiliary_dev(dev);
 
+	ida_free(&gpio_shared_ida, adev->id);
+	kfree(adev);
 }
 
-static int gpio_shared_make_adev(struct gpio_device *gdev,
-				 struct gpio_shared_entry *entry,
-				 struct gpio_shared_ref *ref)
+static int gpio_shared_make_proxy_dev(struct gpio_device *gdev,
+				      struct gpio_shared_entry *entry,
+				      struct gpio_shared_ref *ref)
 {
-	struct auxiliary_device *adev = &ref->adev;
-	int ret;
+	struct auxiliary_device *adev;
+	int ret, id;
 
-	guard(mutex)(&ref->lock);
+	adev = kzalloc_obj(*adev);
+	if (!adev)
+		return -ENOMEM;
 
-	memset(adev, 0, sizeof(*adev));
+	id = ida_alloc(&gpio_shared_ida, GFP_KERNEL);
+	if (id < 0) {
+		kfree(adev);
+		return id;
+	}
 
-	adev->id = ref->dev_id;
+	adev->id = id;
 	adev->name = "proxy";
 	adev->dev.parent = gdev->dev.parent;
 	adev->dev.platform_data = entry;
-	adev->dev.release = gpio_shared_adev_release;
+	adev->dev.release = gpio_shared_proxy_dev_release;
 
 	ret = auxiliary_device_init(adev);
-	if (ret)
+	if (ret) {
+		ida_free(&gpio_shared_ida, id);
+		kfree(adev);
 		return ret;
+	}
 
 	ret = auxiliary_device_add(adev);
 	if (ret) {
@@ -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;
 }
 
@@ -480,7 +491,7 @@ int gpio_shared_add_proxy_lookup(struct device *consumer, struct fwnode_handle *
 			char *key __free(kfree) =
 				kasprintf(GFP_KERNEL,
 					  KBUILD_MODNAME ".proxy.%u",
-					  ref->adev.id);
+					  ref->proxy_dev->id);
 			if (!key)
 				return -ENOMEM;
 
@@ -511,10 +522,11 @@ 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);
+	auxiliary_device_delete(ref->proxy_dev);
+	auxiliary_device_uninit(ref->proxy_dev);
+	ref->proxy_dev = NULL;
 }
 
 int gpiochip_setup_shared(struct gpio_chip *gc)
@@ -527,7 +539,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) {
 				/*
 				 * This is a shared GPIO proxy. Mark its
 				 * descriptor as such and return here.
@@ -592,7 +604,7 @@ int gpiochip_setup_shared(struct gpio_chip *gc)
 				 fwnode_get_name(ref->fwnode) ?: "(no fwnode)",
 				 ref->con_id ?: "(none)");
 
-			ret = gpio_shared_make_adev(gdev, entry, ref);
+			ret = gpio_shared_make_proxy_dev(gdev, entry, ref);
 			if (ret) {
 				gpiod_free_commit(desc);
 				return ret;
@@ -624,7 +636,7 @@ void gpio_device_teardown_shared(struct gpio_device *gdev)
 				ref->lookup = NULL;
 			}
 
-			gpio_shared_remove_adev(&ref->adev);
+			gpio_shared_remove_proxy_dev(ref);
 		}
 	}
 }
@@ -718,7 +730,6 @@ static void gpio_shared_drop_ref(struct gpio_shared_ref *ref)
 	mutex_destroy(&ref->lock);
 	lockdep_unregister_key(&ref->lock_key);
 	kfree(ref->con_id);
-	ida_free(&gpio_shared_ida, ref->dev_id);
 	fwnode_handle_put(ref->fwnode);
 	kfree(ref);
 }

-- 
2.47.3

Re: [PATCH v5 1/2] gpio: shared: detach the proxy device from struct gpio_shared_ref

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] gpio: shared: leak of proxy_dev and IDA ID on partial setup failure in gpiochip_setup_shared()
--

commit 31669b11716fa33851710078614d4d0ed83430d1
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 separately when setting up the GPIO shared
proxy device, while also moving the IDA allocation and release to match the
proxy device lifecycle.

> diff --git a/drivers/gpio/gpiolib-shared.c b/drivers/gpio/gpiolib-shared.c
> index 8c9ac9d933049..c7893a5ac80a6 100644
> --- a/drivers/gpio/gpiolib-shared.c
> +++ b/drivers/gpio/gpiolib-shared.c

[ ... ]

> @@ -306,31 +301,43 @@ static int gpio_shared_of_scan(void)
>  }
>  #endif /* CONFIG_OF */
>  
> -static void gpio_shared_adev_release(struct device *dev)
> +static void gpio_shared_proxy_dev_release(struct device *dev)
>  {
> +	struct auxiliary_device *adev = to_auxiliary_dev(dev);
>  
> +	ida_free(&gpio_shared_ida, adev->id);
> +	kfree(adev);
>  }
>  
> -static int gpio_shared_make_adev(struct gpio_device *gdev,
> -				 struct gpio_shared_entry *entry,
> -				 struct gpio_shared_ref *ref)
> +static int gpio_shared_make_proxy_dev(struct gpio_device *gdev,
> +				      struct gpio_shared_entry *entry,
> +				      struct gpio_shared_ref *ref)
>  {
> -	struct auxiliary_device *adev = &ref->adev;
> -	int ret;
> +	struct auxiliary_device *adev;
> +	int ret, id;
>  
> -	guard(mutex)(&ref->lock);
> +	adev = kzalloc_obj(*adev);
> +	if (!adev)
> +		return -ENOMEM;
>  
> -	memset(adev, 0, sizeof(*adev));
> +	id = ida_alloc(&gpio_shared_ida, GFP_KERNEL);
> +	if (id < 0) {
> +		kfree(adev);
> +		return id;
> +	}
>  
> -	adev->id = ref->dev_id;
> +	adev->id = id;
>  	adev->name = "proxy";
>  	adev->dev.parent = gdev->dev.parent;
>  	adev->dev.platform_data = entry;
> -	adev->dev.release = gpio_shared_adev_release;
> +	adev->dev.release = gpio_shared_proxy_dev_release;
>  
>  	ret = auxiliary_device_init(adev);
> -	if (ret)
> +	if (ret) {
> +		ida_free(&gpio_shared_ida, id);
> +		kfree(adev);
>  		return ret;
> +	}
>  
>  	ret = auxiliary_device_add(adev);
>  	if (ret) {
> @@ -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;
>  }

[ ... ]

> @@ -592,7 +604,7 @@ int gpiochip_setup_shared(struct gpio_chip *gc)
>  				 fwnode_get_name(ref->fwnode) ?: "(no fwnode)",
>  				 ref->con_id ?: "(none)");
>  
> -			ret = gpio_shared_make_adev(gdev, entry, ref);
> +			ret = gpio_shared_make_proxy_dev(gdev, entry, ref);
>  			if (ret) {
>  				gpiod_free_commit(desc);
>  				return ret;

[Severity: Medium]
Can this error path leak ref->proxy_dev and its gpio_shared_ida ID when a
subsequent iteration fails?

In gpiochip_setup_shared(), each shared entry has at least two references in
entry->refs:

drivers/gpio/gpiolib-shared.c:gpiochip_setup_shared() {
    ...
		if (list_count_nodes(&entry->refs) <= 1)
			continue;
    ...
		list_for_each_entry(ref, &entry->refs, list) {
			...
			ret = gpio_shared_make_proxy_dev(gdev, entry, ref);
			if (ret) {
				gpiod_free_commit(desc);
				return ret;
			}
		}
}

If gpio_shared_make_proxy_dev() succeeds for the first ref in entry->refs (or
for a previous shared entry on the same chip) and then fails on a later ref,
gpiochip_setup_shared() only calls gpiod_free_commit(desc) and returns the
error.

In the caller gpiochip_add_data_with_key() in drivers/gpio/gpiolib.c:

	ret = gpiochip_setup_shared(gc);
	if (ret)
		goto err_remove_irqchip;

this bypasses gpio_device_teardown_shared(), leaving the already-created
ref->proxy_dev auxiliary devices registered on the auxiliary bus and leaking
their dynamically allocatedadev structures and gpio_shared_ida IDs (as well as
subsequently overwriting ref->proxy_dev if the GPIO controller re-probes).

[ ... ]

>  			}
>  		}
>  	}

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261020-gpio-shared-fwnode-lookup-v5-0-f4a2dfc059d7@oss.qualcomm.com?part=1

[PATCH v5 2/2] gpio: shared: convert to using software nodes for GPIO shared proxy lookup

[Bartosz Golaszewski]

Automated management of shared GPIOs only allows to request shared pins
when they're assigned to a real struct device. Firmware node, no-device
lookup currently isn't supported and can't be because we're using
platform lookup tables to point the callers to the shared proxy GPIO
controllers.

Rework the GPIO sharing by using dummy software nodes assigned to the
proxy devices and stored in the reference structures which allow us -
once the correct struct gpio_shared_ref is found - to find the correct
target GPIO device by its firmware node.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
---
 drivers/gpio/gpiolib-shared.c | 125 ++++++++++++++++++++----------------------
 drivers/gpio/gpiolib-shared.h |  15 +++--
 drivers/gpio/gpiolib.c        |  20 +------
 3 files changed, 67 insertions(+), 93 deletions(-)

diff --git a/drivers/gpio/gpiolib-shared.c b/drivers/gpio/gpiolib-shared.c
index c7893a5ac80a6..2a6889a291bb9 100644
--- a/drivers/gpio/gpiolib-shared.c
+++ b/drivers/gpio/gpiolib-shared.c
@@ -35,11 +35,12 @@ struct gpio_shared_ref {
 	/* GPIO flags this consumer uses for the request. */
 	enum gpiod_flags flags;
 	char *con_id;
-	/* Protects the auxiliary device struct and the lookup table. */
+	/* Protects the auxiliary device struct and fwnode. */
 	struct mutex lock;
 	struct lock_class_key lock_key;
 	struct auxiliary_device *proxy_dev;
-	struct gpiod_lookup_table *lookup;
+	struct software_node proxy_swnode;
+	struct fwnode_handle *proxy_fwnode;
 	bool is_reset_gpio;
 };
 
@@ -313,35 +314,50 @@ static int gpio_shared_make_proxy_dev(struct gpio_device *gdev,
 				      struct gpio_shared_entry *entry,
 				      struct gpio_shared_ref *ref)
 {
+	struct software_node *swnode = &ref->proxy_swnode;
 	struct auxiliary_device *adev;
+	struct fwnode_handle *fwnode;
+	struct device *dev;
 	int ret, id;
 
 	adev = kzalloc_obj(*adev);
 	if (!adev)
 		return -ENOMEM;
 
+
 	id = ida_alloc(&gpio_shared_ida, GFP_KERNEL);
 	if (id < 0) {
-		kfree(adev);
-		return id;
+		ret = id;
+		goto err_free_adev;
 	}
 
+	ret = software_node_register(swnode);
+	if (ret)
+		goto err_free_ida;
+
+	fwnode = software_node_fwnode(swnode);
+	if (!fwnode) {
+		ret = -ENODEV;
+		goto err_swnode_unreg;
+	}
+
+	dev = &adev->dev;
 	adev->id = id;
 	adev->name = "proxy";
-	adev->dev.parent = gdev->dev.parent;
-	adev->dev.platform_data = entry;
-	adev->dev.release = gpio_shared_proxy_dev_release;
+	dev->parent = gdev->dev.parent;
+	dev->platform_data = entry;
+	dev->release = gpio_shared_proxy_dev_release;
+
+	device_set_node(dev, software_node_fwnode(swnode));
 
 	ret = auxiliary_device_init(adev);
-	if (ret) {
-		ida_free(&gpio_shared_ida, id);
-		kfree(adev);
-		return ret;
-	}
+	if (ret)
+		goto err_swnode_unreg;
 
 	ret = auxiliary_device_add(adev);
 	if (ret) {
 		auxiliary_device_uninit(adev);
+		software_node_unregister(swnode);
 		return ret;
 	}
 
@@ -350,9 +366,19 @@ static int gpio_shared_make_proxy_dev(struct gpio_device *gdev,
 
 	scoped_guard(mutex, &ref->lock) {
 		ref->proxy_dev = adev;
+		ref->proxy_fwnode = fwnode;
 	}
 
 	return 0;
+
+err_swnode_unreg:
+	software_node_unregister(swnode);
+err_free_ida:
+	ida_free(&gpio_shared_ida, id);
+err_free_adev:
+	kfree(adev);
+
+	return ret;
 }
 
 #if IS_ENABLED(CONFIG_RESET_GPIO)
@@ -457,23 +483,19 @@ static bool gpio_shared_dev_is_reset_gpio(struct device *consumer,
 }
 #endif /* CONFIG_RESET_GPIO */
 
-int gpio_shared_add_proxy_lookup(struct device *consumer, struct fwnode_handle *fwnode,
-				 const char *con_id, unsigned long lflags)
+struct gpio_desc *
+gpio_shared_get_proxy(struct device *consumer, struct fwnode_handle *fwnode,
+		      const char *con_id)
 {
 	struct gpio_shared_entry *entry;
 	struct gpio_shared_ref *ref;
-	const char *dev_id;
-
-	if (!consumer)
-		return -EOPNOTSUPP;
-
-	dev_id = dev_name(consumer);
 
 	list_for_each_entry(entry, &gpio_shared_list, list) {
 		list_for_each_entry(ref, &entry->refs, list) {
 			guard(mutex)(&ref->lock);
 
-			if (!ref->fwnode && device_is_compatible(consumer, "reset-gpio")) {
+			if (!ref->fwnode && consumer &&
+			    device_is_compatible(consumer, "reset-gpio")) {
 				if (!gpio_shared_dev_is_reset_gpio(consumer, entry, ref))
 					continue;
 			} else if (fwnode != ref->fwnode) {
@@ -484,49 +506,22 @@ int gpio_shared_add_proxy_lookup(struct device *consumer, struct fwnode_handle *
 			    (con_id && ref->con_id && strcmp(con_id, ref->con_id) != 0))
 				continue;
 
-			/* We've already done that on a previous request. */
-			if (ref->lookup)
-				return 0;
-
-			char *key __free(kfree) =
-				kasprintf(GFP_KERNEL,
-					  KBUILD_MODNAME ".proxy.%u",
-					  ref->proxy_dev->id);
-			if (!key)
-				return -ENOMEM;
-
-			struct gpiod_lookup_table *lookup __free(kfree) =
-				kzalloc_flex(*lookup, table, 2);
-			if (!lookup)
-				return -ENOMEM;
-
-			pr_debug("Adding machine lookup entry for a shared GPIO for consumer %s, with key '%s' and con_id '%s'\n",
-				 dev_id, key, ref->con_id ?: "none");
-
-			lookup->dev_id = kstrdup(dev_id, GFP_KERNEL);
-			if (!lookup->dev_id)
-				return -ENOMEM;
-
-			lookup->table[0] = GPIO_LOOKUP(no_free_ptr(key), 0,
-						       ref->con_id, lflags);
-
-			ref->lookup = no_free_ptr(lookup);
-			gpiod_add_lookup_table(ref->lookup);
+			struct gpio_device *gdev __free(gpio_device_put) =
+					gpio_device_find_by_fwnode(ref->proxy_fwnode);
+			if (!gdev)
+				/*
+				 * A matching entry is there but the associated
+				 * controller has not been registered yet.
+				 */
+				return ERR_PTR(-EPROBE_DEFER);
 
-			return 0;
+			return gpio_device_get_desc(gdev, 0);
 		}
 	}
 
 	/* We warn here because this can only happen if the programmer borked. */
 	WARN_ON(1);
-	return -ENOENT;
-}
-
-static void gpio_shared_remove_proxy_dev(struct gpio_shared_ref *ref)
-{
-	auxiliary_device_delete(ref->proxy_dev);
-	auxiliary_device_uninit(ref->proxy_dev);
-	ref->proxy_dev = NULL;
+	return ERR_PTR(-ENOENT);
 }
 
 int gpiochip_setup_shared(struct gpio_chip *gc)
@@ -628,15 +623,11 @@ void gpio_device_teardown_shared(struct gpio_device *gdev)
 			gpiod_free_commit(&gdev->descs[entry->offset]);
 
 		list_for_each_entry(ref, &entry->refs, list) {
-			if (ref->lookup) {
-				gpiod_remove_lookup_table(ref->lookup);
-				kfree(ref->lookup->table[0].key);
-				kfree(ref->lookup->dev_id);
-				kfree(ref->lookup);
-				ref->lookup = NULL;
-			}
-
-			gpio_shared_remove_proxy_dev(ref);
+			auxiliary_device_delete(ref->proxy_dev);
+			auxiliary_device_uninit(ref->proxy_dev);
+			software_node_unregister(&ref->proxy_swnode);
+			ref->proxy_dev = NULL;
+			ref->proxy_fwnode = NULL;
 		}
 	}
 }
diff --git a/drivers/gpio/gpiolib-shared.h b/drivers/gpio/gpiolib-shared.h
index 618756f6c6aaf..27fd2a40c5cb7 100644
--- a/drivers/gpio/gpiolib-shared.h
+++ b/drivers/gpio/gpiolib-shared.h
@@ -14,9 +14,9 @@ struct fwnode_handle;
 
 int gpiochip_setup_shared(struct gpio_chip *gc);
 void gpio_device_teardown_shared(struct gpio_device *gdev);
-int gpio_shared_add_proxy_lookup(struct device *consumer,
-				 struct fwnode_handle *fwnode,
-				 const char *con_id, unsigned long lflags);
+struct gpio_desc *
+gpio_shared_get_proxy(struct device *consumer, struct fwnode_handle *fwnode,
+		      const char *con_id);
 
 #else
 
@@ -27,12 +27,11 @@ static inline int gpiochip_setup_shared(struct gpio_chip *gc)
 
 static inline void gpio_device_teardown_shared(struct gpio_device *gdev) { }
 
-static inline int gpio_shared_add_proxy_lookup(struct device *consumer,
-					       struct fwnode_handle *fwnode,
-					       const char *con_id,
-					       unsigned long lflags)
+static inline struct gpio_desc *
+gpio_shared_get_proxy(struct device *consumer, struct fwnode_handle *fwnode,
+		      const char *con_id)
 {
-	return 0;
+	return NULL;
 }
 
 #endif /* CONFIG_GPIO_SHARED */
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 05f61092dd0d3..9f87c9ef42b5d 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -4829,24 +4829,8 @@ struct gpio_desc *gpiod_find_and_request(struct device *consumer,
 	scoped_guard(srcu, &gpio_devices_srcu) {
 		desc = gpiod_fwnode_lookup(fwnode, consumer, con_id, idx,
 					   &flags, &lookupflags);
-		if (!IS_ERR_OR_NULL(desc) &&
-		    test_bit(GPIOD_FLAG_SHARED, &desc->flags)) {
-			/*
-			 * We're dealing with a GPIO shared by multiple
-			 * consumers. This is the moment to add the machine
-			 * lookup table for the proxy device as previously
-			 * we only knew the consumer's fwnode.
-			 */
-			ret = gpio_shared_add_proxy_lookup(consumer, fwnode,
-							   con_id, lookupflags);
-			if (ret)
-				return ERR_PTR(ret);
-
-			/* Trigger platform lookup for shared GPIO proxy. */
-			desc = ERR_PTR(-ENOENT);
-			/* Trigger it even for fwnode-only gpiod_get(). */
-			platform_lookup_allowed = true;
-		}
+		if (!IS_ERR_OR_NULL(desc) && test_bit(GPIOD_FLAG_SHARED, &desc->flags))
+			desc = gpio_shared_get_proxy(consumer, fwnode, con_id);
 
 		if (gpiod_not_found(desc) && platform_lookup_allowed) {
 			/*

-- 
2.47.3

Re: [PATCH v5 2/2] gpio: shared: convert to using software nodes for GPIO shared proxy lookup

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] gpio: shared: NULL proxy_fwnode match in gpio_shared_get_proxy()
- [High] gpio: shared: proxy device and swnode leak in gpiochip_setup_shared()
- [High] gpio: shared: missing lock and NULL check in gpio_device_teardown_shared()
- [Low] gpio: shared: goto mixed with scoped_guard() in gpio_shared_make_proxy_dev()
--

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

gpio: shared: convert to using software nodes for GPIO shared proxy lookup

This commit replaces platform lookup tables for shared GPIO proxies with
dummy software nodes assigned to the proxy auxiliary devices, allowing
shared proxy GPIO devices to be looked up by their firmware node.

> diff --git a/drivers/gpio/gpiolib-shared.c b/drivers/gpio/gpiolib-shared.c
> index c7893a5ac80a6..2a6889a291bb9 100644
> --- a/drivers/gpio/gpiolib-shared.c
> +++ b/drivers/gpio/gpiolib-shared.c

[ ... ]

> @@ -313,35 +314,50 @@ static int gpio_shared_make_proxy_dev(struct gpio_device *gdev,
>  				      struct gpio_shared_entry *entry,
>  				      struct gpio_shared_ref *ref)
>  {
> +	struct software_node *swnode = &ref->proxy_swnode;
>  	struct auxiliary_device *adev;
> +	struct fwnode_handle *fwnode;
> +	struct device *dev;
>  	int ret, id;
>  
>  	adev = kzalloc_obj(*adev);
>  	if (!adev)
>  		return -ENOMEM;
>  
> +
>  	id = ida_alloc(&gpio_shared_ida, GFP_KERNEL);
>  	if (id < 0) {
> -		kfree(adev);
> -		return id;
> +		ret = id;
> +		goto err_free_adev;
>  	}
>  
> +	ret = software_node_register(swnode);
> +	if (ret)
> +		goto err_free_ida;

[Severity: High]
What happens if gpio_shared_make_proxy_dev() succeeds for the first
ref in gpiochip_setup_shared(), but fails on a subsequent ref or entry?

In drivers/gpio/gpiolib-shared.c:gpiochip_setup_shared():

	list_for_each_entry(ref, &entry->refs, list) {
		...
		ret = gpio_shared_make_proxy_dev(gdev, entry, ref);
		if (ret) {
			gpiod_free_commit(desc);
			return ret;
		}
	}

And in drivers/gpio/gpiolib.c:gpiochip_add_data_with_key():

	ret = gpiochip_setup_shared(gc);
	if (ret)
		goto err_remove_irqchip;

When gpiochip_setup_shared() fails partway through, it only calls
gpiod_free_commit(desc) for the current entry and jumps past
err_teardown_shared in gpiochip_add_data_with_key(). Does this leak the
auxiliary_device, gpio_shared_ida ID, and registered ref->proxy_swnode
from earlier iterations (as well as requested descriptors from earlier
entries)?

Because ref->proxy_swnode is embedded in the persistent struct
gpio_shared_ref and remains registered in swnode_kset, wouldn't any
subsequent attempt to re-probe the GPIO controller permanently fail in
software_node_register(&ref->proxy_swnode) with -EEXIST?

> +
> +	fwnode = software_node_fwnode(swnode);
> +	if (!fwnode) {
> +		ret = -ENODEV;
> +		goto err_swnode_unreg;
> +	}
> +
> +	dev = &adev->dev;
>  	adev->id = id;
>  	adev->name = "proxy";
> -	adev->dev.parent = gdev->dev.parent;
> -	adev->dev.platform_data = entry;
> -	adev->dev.release = gpio_shared_proxy_dev_release;
> +	dev->parent = gdev->dev.parent;
> +	dev->platform_data = entry;
> +	dev->release = gpio_shared_proxy_dev_release;
> +
> +	device_set_node(dev, software_node_fwnode(swnode));
>  
>  	ret = auxiliary_device_init(adev);
> -	if (ret) {
> -		ida_free(&gpio_shared_ida, id);
> -		kfree(adev);
> -		return ret;
> -	}
> +	if (ret)
> +		goto err_swnode_unreg;
>  
>  	ret = auxiliary_device_add(adev);
>  	if (ret) {
>  		auxiliary_device_uninit(adev);
> +		software_node_unregister(swnode);
>  		return ret;
>  	}
>  
> @@ -350,9 +366,19 @@ static int gpio_shared_make_proxy_dev(struct gpio_device *gdev,
>  
>  	scoped_guard(mutex, &ref->lock) {
>  		ref->proxy_dev = adev;
> +		ref->proxy_fwnode = fwnode;
>  	}
>  
>  	return 0;
> +
> +err_swnode_unreg:
> +	software_node_unregister(swnode);
> +err_free_ida:
> +	ida_free(&gpio_shared_ida, id);
> +err_free_adev:
> +	kfree(adev);
> +
> +	return ret;
>  }

[Severity: Low]
This isn't a bug, but include/linux/cleanup.h recommends not mixing
goto-based error handling labels with scope-based cleanup helpers such
as scoped_guard() in the same function. Could gpio_shared_make_proxy_dev()
avoid mixing goto labels with scoped_guard(mutex, &ref->lock)?

>  #if IS_ENABLED(CONFIG_RESET_GPIO)
> @@ -457,23 +483,19 @@ static bool gpio_shared_dev_is_reset_gpio(struct device *consumer,
>  }
>  #endif /* CONFIG_RESET_GPIO */
>  
> -int gpio_shared_add_proxy_lookup(struct device *consumer, struct fwnode_handle *fwnode,
> -				 const char *con_id, unsigned long lflags)
> +struct gpio_desc *
> +gpio_shared_get_proxy(struct device *consumer, struct fwnode_handle *fwnode,
> +		      const char *con_id)
>  {
>  	struct gpio_shared_entry *entry;
>  	struct gpio_shared_ref *ref;
> -	const char *dev_id;
> -
> -	if (!consumer)
> -		return -EOPNOTSUPP;
> -
> -	dev_id = dev_name(consumer);
>  
>  	list_for_each_entry(entry, &gpio_shared_list, list) {
>  		list_for_each_entry(ref, &entry->refs, list) {
>  			guard(mutex)(&ref->lock);
>  
> -			if (!ref->fwnode && device_is_compatible(consumer, "reset-gpio")) {
> +			if (!ref->fwnode && consumer &&
> +			    device_is_compatible(consumer, "reset-gpio")) {
>  				if (!gpio_shared_dev_is_reset_gpio(consumer, entry, ref))
>  					continue;
>  			} else if (fwnode != ref->fwnode) {
> @@ -484,49 +506,22 @@ int gpio_shared_add_proxy_lookup(struct device *consumer, struct fwnode_handle *
>  			    (con_id && ref->con_id && strcmp(con_id, ref->con_id) != 0))
>  				continue;
>  

[ ... ]

> +			struct gpio_device *gdev __free(gpio_device_put) =
> +					gpio_device_find_by_fwnode(ref->proxy_fwnode);
> +			if (!gdev)
> +				/*
> +				 * A matching entry is there but the associated
> +				 * controller has not been registered yet.
> +				 */
> +				return ERR_PTR(-EPROBE_DEFER);
>  
> -			return 0;
> +			return gpio_device_get_desc(gdev, 0);
>  		}
>  	}

[Severity: High]
Can gpio_device_find_by_fwnode(ref->proxy_fwnode) match an unrelated
GPIO controller when ref->proxy_fwnode is NULL?

ref->proxy_fwnode is NULL before gpio_shared_make_proxy_dev() finishes
initializing that ref (for example, while gpiochip_setup_shared() is
iterating through refs after setting GPIOD_FLAG_SHARED, or if a consumer
has multiple shared GPIOs with the same con_id on different controllers
and an earlier entry in gpio_shared_list belongs to a controller that
has not registered yet) and after gpio_device_teardown_shared() clears
it.

In drivers/gpio/gpiolib.c:gpio_chip_match_by_fwnode():

static int gpio_chip_match_by_fwnode(struct gpio_chip *gc, const void *fwnode)
{
	struct device *dev = &gc->gpiodev->dev;
	struct fwnode_handle *node = dev_fwnode(dev);

	if (IS_ERR(fwnode))
		return 0;

	if (device_match_fwnode(dev, fwnode))
		return 1;

	return node && node->secondary == fwnode;
}

When fwnode is NULL, node && node->secondary == NULL evaluates to true
for any registered GPIO device that has a primary fwnode and no
secondary fwnode (or device_match_fwnode() matches if dev has no
fwnode). Instead of returning NULL and deferring probe with
-EPROBE_DEFER, wouldn't gpio_shared_get_proxy() return descriptor 0 of
the first such unrelated GPIO chip?

> @@ -628,15 +623,11 @@ void gpio_device_teardown_shared(struct gpio_device *gdev)
>  			gpiod_free_commit(&gdev->descs[entry->offset]);
>  
>  		list_for_each_entry(ref, &entry->refs, list) {
> -			if (ref->lookup) {
> -				gpiod_remove_lookup_table(ref->lookup);
> -				kfree(ref->lookup->table[0].key);
> -				kfree(ref->lookup->dev_id);
> -				kfree(ref->lookup);
> -				ref->lookup = NULL;
> -			}
> -
> -			gpio_shared_remove_proxy_dev(ref);
> +			auxiliary_device_delete(ref->proxy_dev);
> +			auxiliary_device_uninit(ref->proxy_dev);
> +			software_node_unregister(&ref->proxy_swnode);
> +			ref->proxy_dev = NULL;
> +			ref->proxy_fwnode = NULL;
>  		}
>  	}
>  }

[Severity: High]
Should this loop in gpio_device_teardown_shared() acquire ref->lock and
check whether ref->proxy_dev is NULL?

In struct gpio_shared_ref, ref->lock protects proxy_dev and proxy_fwnode,
and both gpio_shared_make_proxy_dev() and gpio_shared_get_proxy() take
ref->lock when accessing them. Here, software_node_unregister() frees the
struct swnode backing ref->proxy_fwnode and auxiliary_device_uninit()
drops the last reference to ref->proxy_dev before the pointers are set to
NULL, without holding ref->lock.

Since gpio_shared_list is global and gpio_device_teardown_shared() is
also called at err_teardown_shared in gpiochip_add_data_with_key() before
gdev is removed from gpio_devices, could a concurrent
gpio_shared_get_proxy() or gpiochip_setup_shared() observe a freed
ref->proxy_fwnode or ref->proxy_dev?

Also, if ref->proxy_dev is NULL when gpio_device_teardown_shared() runs
(for instance, if a controller registered before gpio_shared_init() is
later removed, or if multiple gpio_chips share the same controller
fwnode), wouldn't auxiliary_device_delete(ref->proxy_dev) pass NULL to
device_del() and dereference a NULL pointer?

ینی

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261020-gpio-shared-fwnode-lookup-v5-0-f4a2dfc059d7@oss.qualcomm.com?part=2