gpiolib: revert the attempt to protect the GPIO device list with an rwsem

This reverts commits 1979a2807547 ("gpiolib: replace the GPIO device
mutex with a read-write semaphore") and 65a828bab158 ("gpiolib: use
a mutex to protect the list of GPIO devices").

Unfortunately the legacy GPIO API that's still used in older code has to
translate numbers from the global GPIO numberspace to descriptors. This
results in a GPIO device lookup in every call to legacy functions. Some
of those functions - like gpio_set/get_value() - can be called from
atomic context so taking a sleeping lock that is an RW semaphore results
in an error.

We'll probably have to protect this list with SRCU.

Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Closes: https://lore.kernel.org/linux-wireless/f7b5ff1e-8f34-4d98-a7be-b826cb897dc8@moroto.mountain/
Fixes: 1979a2807547 ("gpiolib: replace the GPIO device mutex with a read-write semaphore")
Fixes: 65a828bab158 ("gpiolib: use a mutex to protect the list of GPIO devices")
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index 4dbf298bb5dda0f3fadc58843707d593a1adbc5c..6bf5332136e5a9d4bd0b4c52618fa05241ccaa00 100644 (file)
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -768,25 +768,6 @@ int gpiochip_sysfs_register(struct gpio_device *gdev)
        return 0;
 }
 
-int gpiochip_sysfs_register_all(void)
-{
-       struct gpio_device *gdev;
-       int ret;
-
-       guard(rwsem_read)(&gpio_devices_sem);
-
-       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;
@@ -811,7 +792,9 @@ void gpiochip_sysfs_unregister(struct gpio_device *gdev)
 
 static int __init gpiolib_sysfs_init(void)
 {
-       int status;
+       int             status;
+       unsigned long   flags;
+       struct gpio_device *gdev;
 
        status = class_register(&gpio_class);
        if (status < 0)
@@ -823,6 +806,26 @@ 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().
         */
-       return gpiochip_sysfs_register_all();
+       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;
 }
 postcore_initcall(gpiolib_sysfs_init);

diff --git a/drivers/gpio/gpiolib-sysfs.h b/drivers/gpio/gpiolib-sysfs.h
index ab157cec0b4bec5ae89249fedbd8374e1cd0d91e..b794b396d6a52588c93839fbba062eb160236ca4 100644 (file)
--- a/drivers/gpio/gpiolib-sysfs.h
+++ b/drivers/gpio/gpiolib-sysfs.h
@@ -8,7 +8,6 @@ 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
@@ -18,11 +17,6 @@ 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 4c93cf73a8260569de5287a4b2ae231dc54dbab6..44c8f5743a2416087b523e973967e993e8a192a1 100644 (file)
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2,7 +2,6 @@
 
 #include <linux/acpi.h>
 #include <linux/bitmap.h>
-#include <linux/cleanup.h>
 #include <linux/compat.h>
 #include <linux/debugfs.h>
 #include <linux/device.h>
@@ -16,7 +15,6 @@
 #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>
@@ -83,9 +81,7 @@ DEFINE_SPINLOCK(gpio_lock);
 
 static DEFINE_MUTEX(gpio_lookup_lock);
 static LIST_HEAD(gpio_lookup_list);
-
 LIST_HEAD(gpio_devices);
-DECLARE_RWSEM(gpio_devices_sem);
 
 static DEFINE_MUTEX(gpio_machine_hogs_mutex);
 static LIST_HEAD(gpio_machine_hogs);
@@ -117,15 +113,20 @@ 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);
 
-       scoped_guard(rwsem_read, &gpio_devices_sem) {
-               list_for_each_entry(gdev, &gpio_devices, list) {
-                       if (gdev->base <= gpio &&
-                           gdev->base + gdev->ngpio > gpio)
-                               return &gdev->descs[gpio - gdev->base];
+       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];
                }
        }
 
+       spin_unlock_irqrestore(&gpio_lock, flags);
+
        if (!gpio_is_valid(gpio))
                pr_warn("invalid GPIO %d\n", gpio);
 
@@ -398,21 +399,26 @@ 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;
 
-       guard(rwsem_read)(&gpio_devices_sem);
+       spin_lock_irqsave(&gpio_lock, flags);
 
        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))
+                       if (desc->name && !strcmp(desc->name, name)) {
+                               spin_unlock_irqrestore(&gpio_lock, flags);
                                return desc;
+                       }
                }
        }
 
+       spin_unlock_irqrestore(&gpio_lock, flags);
+
        return NULL;
 }
 
@@ -807,6 +813,7 @@ 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;
@@ -871,46 +878,49 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
 
        gdev->ngpio = gc->ngpio;
 
-       scoped_guard(rwsem_write, &gpio_devices_sem) {
-               /*
-                * 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;
+       spin_lock_irqsave(&gpio_lock, flags);
 
+       /*
+        * 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) {
-                       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");
+                       spin_unlock_irqrestore(&gpio_lock, flags);
+                       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;
 
-               for (i = 0; i < gc->ngpio; i++)
-                       gdev->descs[i].gdev = gdev;
+       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;
+
+       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);
@@ -1001,8 +1011,9 @@ err_free_gpiochip_mask:
                goto err_print_message;
        }
 err_remove_from_list:
-       scoped_guard(rwsem_write, &gpio_devices_sem)
-               list_del(&gdev->list);
+       spin_lock_irqsave(&gpio_lock, flags);
+       list_del(&gdev->list);
+       spin_unlock_irqrestore(&gpio_lock, flags);
 err_free_label:
        kfree_const(gdev->label);
 err_free_descs:
@@ -1065,7 +1076,7 @@ void gpiochip_remove(struct gpio_chip *gc)
                dev_crit(&gdev->dev,
                         "REMOVING GPIOCHIP WITH GPIOS STILL REQUESTED\n");
 
-       scoped_guard(rwsem_write, &gpio_devices_sem)
+       scoped_guard(spinlock_irqsave, &gpio_lock)
                list_del(&gdev->list);
 
        /*
@@ -1114,7 +1125,7 @@ struct gpio_device *gpio_device_find(void *data,
         */
        might_sleep();
 
-       guard(rwsem_read)(&gpio_devices_sem);
+       guard(spinlock_irqsave)(&gpio_lock);
 
        list_for_each_entry(gdev, &gpio_devices, list) {
                if (gdev->chip && match(gdev->chip, data))
@@ -4725,33 +4736,35 @@ 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 = "";
 
-       guard(rwsem_read)(&gpio_devices_sem);
-
-       list_for_each_entry(gdev, &gpio_devices, list) {
-               if (index-- == 0)
+       spin_lock_irqsave(&gpio_lock, flags);
+       list_for_each_entry(gdev, &gpio_devices, list)
+               if (index-- == 0) {
+                       spin_unlock_irqrestore(&gpio_lock, flags);
                        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;
 
-       scoped_guard(rwsem_read, &gpio_devices_sem) {
-               if (list_is_last(&gdev->list, &gpio_devices))
-                       ret = NULL;
-               else
-                       ret = list_first_entry(&gdev->list, struct gpio_device,
-                                              list);
-       }
+       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);
 
        s->private = "\n";
        ++*pos;

diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index 97df54abf57ac0f2a740d72bf37965b299d87927..a4a2520b5f31cced763696970a452ab1fff0e4bf 100644 (file)
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -15,7 +15,6 @@
 #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>
 
@@ -137,7 +136,6 @@ int gpiod_set_transitory(struct gpio_desc *desc, bool transitory);
 
 extern spinlock_t gpio_lock;
 extern struct list_head gpio_devices;
-extern struct rw_semaphore gpio_devices_sem;
 
 void gpiod_line_state_notify(struct gpio_desc *desc, unsigned long action);