gpio: shared: make locking more fine-grained

The global gpio_shared_lock has caused some issues when recursively
removing GPIO shared proxies. The thing is: we don't really need it.
Once created from an init-call, the shared GPIO data structures are
never removed, there's no need to protect the linked lists of entries
and references. All we need to protect is the shared GPIO descriptor
(which we already do with a per-entry mutex) and the auxiliary device
data in struct gpio_shared_ref.

Remove the global gpio_shared_lock and use a per-reference mutex to
protect shared references when adding/removing the auxiliary devices and
their GPIO lookup entries.

Link: https://lore.kernel.org/r/20251206-gpio-shared-teardown-fixes-v1-4-35ac458cfce1@oss.qualcomm.com
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>

diff --git a/drivers/gpio/gpiolib-shared.c b/drivers/gpio/gpiolib-shared.c
index 2d3b0c3460e56941af8415af6989914104060bf7..ba4b718d40a087608fc06f59d242932df1d117e9 100644 (file)
--- a/drivers/gpio/gpiolib-shared.c
+++ b/drivers/gpio/gpiolib-shared.c
@@ -36,6 +36,8 @@ struct gpio_shared_ref {
        enum gpiod_flags flags;
        char *con_id;
        int dev_id;
+       /* Protects the auxiliary device struct and the lookup table. */
+       struct mutex lock;
        struct auxiliary_device adev;
        struct gpiod_lookup_table *lookup;
 };
@@ -49,6 +51,7 @@ struct gpio_shared_entry {
        unsigned int offset;
        /* Index in the property value array. */
        size_t index;
+       /* Synchronizes the modification of shared_desc. */
        struct mutex lock;
        struct gpio_shared_desc *shared_desc;
        struct kref ref;
@@ -56,7 +59,6 @@ struct gpio_shared_entry {
 };
 
 static LIST_HEAD(gpio_shared_list);
-static DEFINE_MUTEX(gpio_shared_lock);
 static DEFINE_IDA(gpio_shared_ida);
 
 #if IS_ENABLED(CONFIG_OF)
@@ -187,6 +189,7 @@ static int gpio_shared_of_traverse(struct device_node *curr)
 
                        ref->fwnode = fwnode_handle_get(of_fwnode_handle(curr));
                        ref->flags = args.args[1];
+                       mutex_init(&ref->lock);
 
                        if (strends(prop->name, "gpios"))
                                suffix = "-gpios";
@@ -258,7 +261,7 @@ static int gpio_shared_make_adev(struct gpio_device *gdev,
        struct auxiliary_device *adev = &ref->adev;
        int ret;
 
-       lockdep_assert_held(&gpio_shared_lock);
+       guard(mutex)(&ref->lock);
 
        memset(adev, 0, sizeof(*adev));
 
@@ -373,14 +376,14 @@ int gpio_shared_add_proxy_lookup(struct device *consumer, unsigned long lflags)
        if (!lookup)
                return -ENOMEM;
 
-       guard(mutex)(&gpio_shared_lock);
-
        list_for_each_entry(entry, &gpio_shared_list, list) {
                list_for_each_entry(ref, &entry->refs, list) {
                        if (!device_match_fwnode(consumer, ref->fwnode) &&
                            !gpio_shared_dev_is_reset_gpio(consumer, entry, ref))
                                continue;
 
+                       guard(mutex)(&ref->lock);
+
                        /* We've already done that on a previous request. */
                        if (ref->lookup)
                                return 0;
@@ -413,8 +416,6 @@ int gpio_shared_add_proxy_lookup(struct device *consumer, unsigned long lflags)
 
 static void gpio_shared_remove_adev(struct auxiliary_device *adev)
 {
-       lockdep_assert_held(&gpio_shared_lock);
-
        auxiliary_device_delete(adev);
        auxiliary_device_uninit(adev);
 }
@@ -426,8 +427,6 @@ int gpio_device_setup_shared(struct gpio_device *gdev)
        unsigned long *flags;
        int ret;
 
-       guard(mutex)(&gpio_shared_lock);
-
        list_for_each_entry(entry, &gpio_shared_list, list) {
                list_for_each_entry(ref, &entry->refs, list) {
                        if (gdev->dev.parent == &ref->adev.dev) {
@@ -484,13 +483,22 @@ void gpio_device_teardown_shared(struct gpio_device *gdev)
        struct gpio_shared_entry *entry;
        struct gpio_shared_ref *ref;
 
-       guard(mutex)(&gpio_shared_lock);
-
        list_for_each_entry(entry, &gpio_shared_list, list) {
                if (!device_match_fwnode(&gdev->dev, entry->fwnode))
                        continue;
 
+               /*
+                * For some reason if we call synchronize_srcu() in GPIO core,
+                * descent here and take this mutex and then recursively call
+                * synchronize_srcu() again from gpiochip_remove() (which is
+                * totally fine) called after gpio_shared_remove_adev(),
+                * lockdep prints a false positive deadlock splat. Disable
+                * lockdep here.
+                */
+               lockdep_off();
                list_for_each_entry(ref, &entry->refs, list) {
+                       guard(mutex)(&ref->lock);
+
                        if (ref->lookup) {
                                gpiod_remove_lookup_table(ref->lookup);
                                kfree(ref->lookup->table[0].key);
@@ -500,6 +508,7 @@ void gpio_device_teardown_shared(struct gpio_device *gdev)
 
                        gpio_shared_remove_adev(&ref->adev);
                }
+               lockdep_on();
        }
 }
 
@@ -523,8 +532,6 @@ static void gpiod_shared_put(void *data)
 {
        struct gpio_shared_entry *entry = data;
 
-       lockdep_assert_not_held(&gpio_shared_lock);
-
        kref_put(&entry->ref, gpio_shared_release);
 }
 
@@ -562,8 +569,6 @@ struct gpio_shared_desc *devm_gpiod_shared_get(struct device *dev)
        struct gpio_shared_entry *entry;
        int ret;
 
-       lockdep_assert_not_held(&gpio_shared_lock);
-
        entry = dev_get_platdata(dev);
        if (WARN_ON(!entry))
                /* Programmer bug */
@@ -598,6 +603,7 @@ EXPORT_SYMBOL_GPL(devm_gpiod_shared_get);
 static void gpio_shared_drop_ref(struct gpio_shared_ref *ref)
 {
        list_del(&ref->list);
+       mutex_destroy(&ref->lock);
        kfree(ref->con_id);
        ida_free(&gpio_shared_ida, ref->dev_id);
        fwnode_handle_put(ref->fwnode);