gpiolib: use a mutex to protect the list of GPIO devices

The global list of GPIO devices is never modified or accessed from
atomic context so it's fine to protect it using a mutex. Add a new
global lock dedicated to the gpio_devices list and use it whenever
accessing or modifying it.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index 6f309a3b2d9ade8a30e14e76a3b861c573302705..ae4fc013b675ce23e92569a25eb3d4bf6152deea 100644 (file)
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -766,6 +766,25 @@ int gpiochip_sysfs_register(struct gpio_device *gdev)
        return 0;
 }
 
+int gpiochip_sysfs_register_all(void)
+{
+       struct gpio_device *gdev;
+       int ret;
+
+       guard(mutex)(&gpio_devices_lock);
+
+       list_for_each_entry(gdev, &gpio_devices, list) {
+               if (gdev->mockdev)
+                       continue;
+
+               ret = gpiochip_sysfs_register(gdev);
+               if (ret)
+                       return ret;
+       }
+
+       return 0;
+}
+
 void gpiochip_sysfs_unregister(struct gpio_device *gdev)
 {
        struct gpio_desc *desc;
@@ -790,9 +809,7 @@ void gpiochip_sysfs_unregister(struct gpio_device *gdev)
 
 static int __init gpiolib_sysfs_init(void)
 {
-       int             status;
-       unsigned long   flags;
-       struct gpio_device *gdev;
+       int status;
 
        status = class_register(&gpio_class);
        if (status < 0)
@@ -804,26 +821,6 @@ static int __init gpiolib_sysfs_init(void)
         * We run before arch_initcall() so chip->dev nodes can have
         * registered, and so arch_initcall() can always gpiod_export().
         */
-       spin_lock_irqsave(&gpio_lock, flags);
-       list_for_each_entry(gdev, &gpio_devices, list) {
-               if (gdev->mockdev)
-                       continue;
-
-               /*
-                * TODO we yield gpio_lock here because
-                * gpiochip_sysfs_register() acquires a mutex. This is unsafe
-                * and needs to be fixed.
-                *
-                * Also it would be nice to use gpio_device_find() here so we
-                * can keep gpio_chips local to gpiolib.c, but the yield of
-                * gpio_lock prevents us from doing this.
-                */
-               spin_unlock_irqrestore(&gpio_lock, flags);
-               status = gpiochip_sysfs_register(gdev);
-               spin_lock_irqsave(&gpio_lock, flags);
-       }
-       spin_unlock_irqrestore(&gpio_lock, flags);
-
-       return status;
+       return gpiochip_sysfs_register_all();
 }
 postcore_initcall(gpiolib_sysfs_init);

diff --git a/drivers/gpio/gpiolib-sysfs.h b/drivers/gpio/gpiolib-sysfs.h
index b794b396d6a52588c93839fbba062eb160236ca4..ab157cec0b4bec5ae89249fedbd8374e1cd0d91e 100644 (file)
--- a/drivers/gpio/gpiolib-sysfs.h
+++ b/drivers/gpio/gpiolib-sysfs.h
@@ -8,6 +8,7 @@ struct gpio_device;
 #ifdef CONFIG_GPIO_SYSFS
 
 int gpiochip_sysfs_register(struct gpio_device *gdev);
+int gpiochip_sysfs_register_all(void);
 void gpiochip_sysfs_unregister(struct gpio_device *gdev);
 
 #else
@@ -17,6 +18,11 @@ static inline int gpiochip_sysfs_register(struct gpio_device *gdev)
        return 0;
 }
 
+static inline int gpiochip_sysfs_register_all(void)
+{
+       return 0;
+}
+
 static inline void gpiochip_sysfs_unregister(struct gpio_device *gdev)
 {
 }

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 5b744d1f31f62972c4733420fc048e86b9cdbf31..9508396c967ef17dd22216dd94060d18173d3927 100644 (file)
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2,6 +2,7 @@
 
 #include <linux/acpi.h>
 #include <linux/bitmap.h>
+#include <linux/cleanup.h>
 #include <linux/compat.h>
 #include <linux/debugfs.h>
 #include <linux/device.h>
@@ -15,6 +16,7 @@
 #include <linux/kernel.h>
 #include <linux/list.h>
 #include <linux/module.h>
+#include <linux/mutex.h>
 #include <linux/of.h>
 #include <linux/pinctrl/consumer.h>
 #include <linux/seq_file.h>
@@ -94,7 +96,9 @@ DEFINE_SPINLOCK(gpio_lock);
 
 static DEFINE_MUTEX(gpio_lookup_lock);
 static LIST_HEAD(gpio_lookup_list);
+
 LIST_HEAD(gpio_devices);
+DEFINE_MUTEX(gpio_devices_lock);
 
 static DEFINE_MUTEX(gpio_machine_hogs_mutex);
 static LIST_HEAD(gpio_machine_hogs);
@@ -126,20 +130,15 @@ static inline void desc_set_label(struct gpio_desc *d, const char *label)
 struct gpio_desc *gpio_to_desc(unsigned gpio)
 {
        struct gpio_device *gdev;
-       unsigned long flags;
-
-       spin_lock_irqsave(&gpio_lock, flags);
 
-       list_for_each_entry(gdev, &gpio_devices, list) {
-               if (gdev->base <= gpio &&
-                   gdev->base + gdev->ngpio > gpio) {
-                       spin_unlock_irqrestore(&gpio_lock, flags);
-                       return &gdev->descs[gpio - gdev->base];
+       scoped_guard(mutex, &gpio_devices_lock) {
+               list_for_each_entry(gdev, &gpio_devices, list) {
+                       if (gdev->base <= gpio &&
+                           gdev->base + gdev->ngpio > gpio)
+                               return &gdev->descs[gpio - gdev->base];
                }
        }
 
-       spin_unlock_irqrestore(&gpio_lock, flags);
-
        if (!gpio_is_valid(gpio))
                pr_warn("invalid GPIO %d\n", gpio);
 
@@ -412,26 +411,21 @@ static int gpiodev_add_to_list_unlocked(struct gpio_device *gdev)
 static struct gpio_desc *gpio_name_to_desc(const char * const name)
 {
        struct gpio_device *gdev;
-       unsigned long flags;
 
        if (!name)
                return NULL;
 
-       spin_lock_irqsave(&gpio_lock, flags);
+       guard(mutex)(&gpio_devices_lock);
 
        list_for_each_entry(gdev, &gpio_devices, list) {
                struct gpio_desc *desc;
 
                for_each_gpio_desc(gdev->chip, desc) {
-                       if (desc->name && !strcmp(desc->name, name)) {
-                               spin_unlock_irqrestore(&gpio_lock, flags);
+                       if (desc->name && !strcmp(desc->name, name))
                                return desc;
-                       }
                }
        }
 
-       spin_unlock_irqrestore(&gpio_lock, flags);
-
        return NULL;
 }
 
@@ -669,11 +663,9 @@ EXPORT_SYMBOL_GPL(gpiochip_line_is_valid);
 static void gpiodev_release(struct device *dev)
 {
        struct gpio_device *gdev = to_gpio_device(dev);
-       unsigned long flags;
 
-       spin_lock_irqsave(&gpio_lock, flags);
-       list_del(&gdev->list);
-       spin_unlock_irqrestore(&gpio_lock, flags);
+       scoped_guard(mutex, &gpio_devices_lock)
+               list_del(&gdev->list);
 
        ida_free(&gpio_ida, gdev->id);
        kfree_const(gdev->label);
@@ -831,7 +823,6 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
                               struct lock_class_key *request_key)
 {
        struct gpio_device *gdev;
-       unsigned long flags;
        unsigned int i;
        int base = 0;
        int ret = 0;
@@ -896,49 +887,46 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
 
        gdev->ngpio = gc->ngpio;
 
-       spin_lock_irqsave(&gpio_lock, flags);
+       scoped_guard(mutex, &gpio_devices_lock) {
+               /*
+                * TODO: this allocates a Linux GPIO number base in the global
+                * GPIO numberspace for this chip. In the long run we want to
+                * get *rid* of this numberspace and use only descriptors, but
+                * it may be a pipe dream. It will not happen before we get rid
+                * of the sysfs interface anyways.
+                */
+               base = gc->base;
 
-       /*
-        * TODO: this allocates a Linux GPIO number base in the global
-        * GPIO numberspace for this chip. In the long run we want to
-        * get *rid* of this numberspace and use only descriptors, but
-        * it may be a pipe dream. It will not happen before we get rid
-        * of the sysfs interface anyways.
-        */
-       base = gc->base;
-       if (base < 0) {
-               base = gpiochip_find_base_unlocked(gc->ngpio);
                if (base < 0) {
-                       spin_unlock_irqrestore(&gpio_lock, flags);
-                       ret = base;
-                       base = 0;
+                       base = gpiochip_find_base_unlocked(gc->ngpio);
+                       if (base < 0) {
+                               ret = base;
+                               base = 0;
+                               goto err_free_label;
+                       }
+                       /*
+                        * TODO: it should not be necessary to reflect the assigned
+                        * base outside of the GPIO subsystem. Go over drivers and
+                        * see if anyone makes use of this, else drop this and assign
+                        * a poison instead.
+                        */
+                       gc->base = base;
+               } else {
+                       dev_warn(&gdev->dev,
+                                "Static allocation of GPIO base is deprecated, use dynamic allocation.\n");
+               }
+               gdev->base = base;
+
+               ret = gpiodev_add_to_list_unlocked(gdev);
+               if (ret) {
+                       chip_err(gc, "GPIO integer space overlap, cannot add chip\n");
                        goto err_free_label;
                }
-               /*
-                * TODO: it should not be necessary to reflect the assigned
-                * base outside of the GPIO subsystem. Go over drivers and
-                * see if anyone makes use of this, else drop this and assign
-                * a poison instead.
-                */
-               gc->base = base;
-       } else {
-               dev_warn(&gdev->dev,
-                        "Static allocation of GPIO base is deprecated, use dynamic allocation.\n");
-       }
-       gdev->base = base;
 
-       ret = gpiodev_add_to_list_unlocked(gdev);
-       if (ret) {
-               spin_unlock_irqrestore(&gpio_lock, flags);
-               chip_err(gc, "GPIO integer space overlap, cannot add chip\n");
-               goto err_free_label;
+               for (i = 0; i < gc->ngpio; i++)
+                       gdev->descs[i].gdev = gdev;
        }
 
-       for (i = 0; i < gc->ngpio; i++)
-               gdev->descs[i].gdev = gdev;
-
-       spin_unlock_irqrestore(&gpio_lock, flags);
-
        BLOCKING_INIT_NOTIFIER_HEAD(&gdev->line_state_notifier);
        BLOCKING_INIT_NOTIFIER_HEAD(&gdev->device_notifier);
        init_rwsem(&gdev->sem);
@@ -1029,9 +1017,8 @@ err_free_gpiochip_mask:
                goto err_print_message;
        }
 err_remove_from_list:
-       spin_lock_irqsave(&gpio_lock, flags);
-       list_del(&gdev->list);
-       spin_unlock_irqrestore(&gpio_lock, flags);
+       scoped_guard(mutex, &gpio_devices_lock)
+               list_del(&gdev->list);
 err_free_label:
        kfree_const(gdev->label);
 err_free_descs:
@@ -1140,7 +1127,7 @@ struct gpio_device *gpio_device_find(void *data,
         */
        might_sleep();
 
-       guard(spinlock_irqsave)(&gpio_lock);
+       guard(mutex)(&gpio_devices_lock);
 
        list_for_each_entry(gdev, &gpio_devices, list) {
                if (gdev->chip && match(gdev->chip, data))
@@ -4756,35 +4743,33 @@ static void gpiolib_dbg_show(struct seq_file *s, struct gpio_device *gdev)
 
 static void *gpiolib_seq_start(struct seq_file *s, loff_t *pos)
 {
-       unsigned long flags;
        struct gpio_device *gdev = NULL;
        loff_t index = *pos;
 
        s->private = "";
 
-       spin_lock_irqsave(&gpio_lock, flags);
-       list_for_each_entry(gdev, &gpio_devices, list)
-               if (index-- == 0) {
-                       spin_unlock_irqrestore(&gpio_lock, flags);
+       guard(mutex)(&gpio_devices_lock);
+
+       list_for_each_entry(gdev, &gpio_devices, list) {
+               if (index-- == 0)
                        return gdev;
-               }
-       spin_unlock_irqrestore(&gpio_lock, flags);
+       }
 
        return NULL;
 }
 
 static void *gpiolib_seq_next(struct seq_file *s, void *v, loff_t *pos)
 {
-       unsigned long flags;
        struct gpio_device *gdev = v;
        void *ret = NULL;
 
-       spin_lock_irqsave(&gpio_lock, flags);
-       if (list_is_last(&gdev->list, &gpio_devices))
-               ret = NULL;
-       else
-               ret = list_first_entry(&gdev->list, struct gpio_device, list);
-       spin_unlock_irqrestore(&gpio_lock, flags);
+       scoped_guard(mutex, &gpio_devices_lock) {
+               if (list_is_last(&gdev->list, &gpio_devices))
+                       ret = NULL;
+               else
+                       ret = list_first_entry(&gdev->list, struct gpio_device,
+                                              list);
+       }
 
        s->private = "\n";
        ++*pos;

diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index 3ccacf3c1288e75341016dfe30d40167c8171785..8142b5cbc39537a6a019cd526fa3469b7890640e 100644 (file)
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -15,6 +15,7 @@
 #include <linux/gpio/consumer.h> /* for enum gpiod_flags */
 #include <linux/gpio/driver.h>
 #include <linux/module.h>
+#include <linux/mutex.h>
 #include <linux/notifier.h>
 #include <linux/rwsem.h>
 
@@ -136,6 +137,7 @@ int gpiod_set_transitory(struct gpio_desc *desc, bool transitory);
 
 extern spinlock_t gpio_lock;
 extern struct list_head gpio_devices;
+extern struct mutex gpio_devices_lock;
 
 void gpiod_line_state_notify(struct gpio_desc *desc, unsigned long action);