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

PATCH In review Bartosz Golaszewski 1 participant 2 messages Jan 15, 2024 – Jan 17, 2024 GPIO

lore

Share & tools

lore

Bartosz Golaszewski

author

2024-01-15 11:17 UTC #0 lore raw

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

From: Bartosz Golaszewski <[email protected]>

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 <[email protected]>

Closes https://lore.kernel.org/linux-wireless/[email protected]/ open here

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 <[email protected]>

---
 drivers/gpio/gpiolib-sysfs.c |  45 ++++++------
 drivers/gpio/gpiolib-sysfs.h |   6 --
 drivers/gpio/gpiolib.c       | 135 +++++++++++++++++++----------------
 drivers/gpio/gpiolib.h       |   2 -
 4 files changed, 98 insertions(+), 90 deletions(-)

drivers/gpio/gpiolib-sysfs.c +24 ~~-21~~

index 4dbf298bb5dd..6bf5332136e5 100644
@@ -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);

drivers/gpio/gpiolib-sysfs.h +0 -6

index ab157cec0b4b..b794b396d6a5 100644
@@ -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)
 {
 }

drivers/gpio/gpiolib.c +74 ~~-61~~

239 lines not on this page · raw message

drivers/gpio/gpiolib.h +0 -2

index 97df54abf57a..a4a2520b5f31 100644
@@ -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);

Signature · 1 line

2.40.1

Bartosz Golaszewski

author

2024-01-17 08:55 UTC #1 lore raw

Quoted 14 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>
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.

Quoted 8 lines

We'll probably have to protect this list with SRCU.
Reported-by Dan Carpenter <[email protected]>
Closes https://lore.kernel.org/linux-wireless/[email protected]/ open here
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 <[email protected]>
---

Patch queued for fixes for this merge window.

Bart