From 0857c39bfd09183792c680858b4416498ee1d1ca Mon Sep 17 00:00:00 2001 From: Bartosz Golaszewski Date: Fri, 5 Jan 2024 19:51:19 +0100 Subject: gpio: remove unused logging helpers The general rule of the kernel is to not provide symbols that have no users upstream. Let's remove logging helpers that are not used anywhere. This will save us work later when we'll be modifying them to use the upcoming SRCU infrastructure. Signed-off-by: Bartosz Golaszewski Reviewed-by: Linus Walleij Acked-by: Andy Shevchenko --- drivers/gpio/gpiolib.h | 13 ------------- 1 file changed, 13 deletions(-) (limited to 'drivers/gpio/gpiolib.h') diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h index a4a2520b5f31..c3ae5bfa3f2e 100644 --- a/drivers/gpio/gpiolib.h +++ b/drivers/gpio/gpiolib.h @@ -219,31 +219,18 @@ static inline int gpio_chip_hwgpio(const struct gpio_desc *desc) /* With descriptor prefix */ -#define gpiod_emerg(desc, fmt, ...) \ - pr_emerg("gpio-%d (%s): " fmt, desc_to_gpio(desc), desc->label ? : "?",\ - ##__VA_ARGS__) -#define gpiod_crit(desc, fmt, ...) \ - pr_crit("gpio-%d (%s): " fmt, desc_to_gpio(desc), desc->label ? : "?", \ - ##__VA_ARGS__) #define gpiod_err(desc, fmt, ...) \ pr_err("gpio-%d (%s): " fmt, desc_to_gpio(desc), desc->label ? : "?", \ ##__VA_ARGS__) #define gpiod_warn(desc, fmt, ...) \ pr_warn("gpio-%d (%s): " fmt, desc_to_gpio(desc), desc->label ? : "?", \ ##__VA_ARGS__) -#define gpiod_info(desc, fmt, ...) \ - pr_info("gpio-%d (%s): " fmt, desc_to_gpio(desc), desc->label ? : "?", \ - ##__VA_ARGS__) #define gpiod_dbg(desc, fmt, ...) \ pr_debug("gpio-%d (%s): " fmt, desc_to_gpio(desc), desc->label ? : "?",\ ##__VA_ARGS__) /* With chip prefix */ -#define chip_emerg(gc, fmt, ...) \ - dev_emerg(&gc->gpiodev->dev, "(%s): " fmt, gc->label, ##__VA_ARGS__) -#define chip_crit(gc, fmt, ...) \ - dev_crit(&gc->gpiodev->dev, "(%s): " fmt, gc->label, ##__VA_ARGS__) #define chip_err(gc, fmt, ...) \ dev_err(&gc->gpiodev->dev, "(%s): " fmt, gc->label, ##__VA_ARGS__) #define chip_warn(gc, fmt, ...) \ -- cgit From d23dc4a9a88f16d0327e7b4bb03e6f43c1a1b166 Mon Sep 17 00:00:00 2001 From: Bartosz Golaszewski Date: Mon, 29 Jan 2024 10:11:41 +0100 Subject: gpio: provide and use gpiod_get_label() We will soon serialize access to the descriptor label using SRCU. The write-side of the protection will require calling synchronize_srcu() which must not be called from atomic context. We have two irq helpers: gpiochip_lock_as_irq() and gpiochip_unlock_as_irq() that set the label if the GPIO is not requested but is being used as interrupt. They are called with a spinlock held from the interrupt subsystem. They must not do it if we are to use SRCU so instead let's move the special corner case to a dedicated getter. First: let's implement and use the getter where it's applicable. Signed-off-by: Bartosz Golaszewski Reviewed-by: Linus Walleij Acked-by: Andy Shevchenko --- drivers/gpio/gpiolib.h | 1 + 1 file changed, 1 insertion(+) (limited to 'drivers/gpio/gpiolib.h') diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h index c3ae5bfa3f2e..1058f326fe2b 100644 --- a/drivers/gpio/gpiolib.h +++ b/drivers/gpio/gpiolib.h @@ -208,6 +208,7 @@ int gpio_set_debounce_timeout(struct gpio_desc *desc, unsigned int debounce); int gpiod_hog(struct gpio_desc *desc, const char *name, unsigned long lflags, enum gpiod_flags dflags); int gpiochip_get_ngpios(struct gpio_chip *gc, struct device *dev); +const char *gpiod_get_label(struct gpio_desc *desc); /* * Return the GPIO number of the passed descriptor relative to its chip -- cgit From be711caa87c5c81d5dc00b244cac3a0b775adb18 Mon Sep 17 00:00:00 2001 From: Bartosz Golaszewski Date: Fri, 5 Jan 2024 15:46:27 +0100 Subject: gpio: add SRCU infrastructure to struct gpio_desc Extend the GPIO descriptor with an SRCU structure in order to serialize the access to the label. Initialize and clean it up where applicable. Signed-off-by: Bartosz Golaszewski Reviewed-by: Linus Walleij Acked-by: Andy Shevchenko --- drivers/gpio/gpiolib.h | 3 +++ 1 file changed, 3 insertions(+) (limited to 'drivers/gpio/gpiolib.h') diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h index 1058f326fe2b..6e14b629c48b 100644 --- a/drivers/gpio/gpiolib.h +++ b/drivers/gpio/gpiolib.h @@ -17,6 +17,7 @@ #include #include #include +#include #define GPIOCHIP_NAME "gpiochip" @@ -147,6 +148,7 @@ void gpiod_line_state_notify(struct gpio_desc *desc, unsigned long action); * @label: Name of the consumer * @name: Line name * @hog: Pointer to the device node that hogs this line (if any) + * @srcu: SRCU struct protecting the label pointer. * * These are obtained using gpiod_get() and are preferable to the old * integer-based handles. @@ -184,6 +186,7 @@ struct gpio_desc { #ifdef CONFIG_OF_DYNAMIC struct device_node *hog; #endif + struct srcu_struct srcu; }; #define gpiod_not_found(desc) (IS_ERR(desc) && PTR_ERR(desc) == -ENOENT) -- cgit From 1f2bcb8c8ccdf9dc2e46f7986e1e22408506a6d6 Mon Sep 17 00:00:00 2001 From: Bartosz Golaszewski Date: Fri, 5 Jan 2024 17:08:36 +0100 Subject: gpio: protect the descriptor label with SRCU In order to ensure that the label is not freed while it's being accessed, let's protect it with SRCU and synchronize it everytime it's changed. Let's modify desc_set_label() to manage the memory used for the label as it can only be freed once synchronize_srcu() returns. Signed-off-by: Bartosz Golaszewski Reviewed-by: Linus Walleij Acked-by: Andy Shevchenko --- drivers/gpio/gpiolib.h | 34 ++++++++++++++++++++++++---------- 1 file changed, 24 insertions(+), 10 deletions(-) (limited to 'drivers/gpio/gpiolib.h') diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h index 6e14b629c48b..d2e73eea9e92 100644 --- a/drivers/gpio/gpiolib.h +++ b/drivers/gpio/gpiolib.h @@ -180,7 +180,7 @@ struct gpio_desc { #define FLAG_EVENT_CLOCK_HTE 19 /* GPIO CDEV reports hardware timestamps in events */ /* Connection label */ - const char *label; + const char __rcu *label; /* Name of the GPIO */ const char *name; #ifdef CONFIG_OF_DYNAMIC @@ -223,15 +223,29 @@ static inline int gpio_chip_hwgpio(const struct gpio_desc *desc) /* With descriptor prefix */ -#define gpiod_err(desc, fmt, ...) \ - pr_err("gpio-%d (%s): " fmt, desc_to_gpio(desc), desc->label ? : "?", \ - ##__VA_ARGS__) -#define gpiod_warn(desc, fmt, ...) \ - pr_warn("gpio-%d (%s): " fmt, desc_to_gpio(desc), desc->label ? : "?", \ - ##__VA_ARGS__) -#define gpiod_dbg(desc, fmt, ...) \ - pr_debug("gpio-%d (%s): " fmt, desc_to_gpio(desc), desc->label ? : "?",\ - ##__VA_ARGS__) +#define gpiod_err(desc, fmt, ...) \ +do { \ + scoped_guard(srcu, &desc->srcu) { \ + pr_err("gpio-%d (%s): " fmt, desc_to_gpio(desc), \ + gpiod_get_label(desc) ? : "?", ##__VA_ARGS__); \ + } \ +} while (0) + +#define gpiod_warn(desc, fmt, ...) \ +do { \ + scoped_guard(srcu, &desc->srcu) { \ + pr_warn("gpio-%d (%s): " fmt, desc_to_gpio(desc), \ + gpiod_get_label(desc) ? : "?", ##__VA_ARGS__); \ + } \ +} while (0) + +#define gpiod_dbg(desc, fmt, ...) \ +do { \ + scoped_guard(srcu, &desc->srcu) { \ + pr_debug("gpio-%d (%s): " fmt, desc_to_gpio(desc), \ + gpiod_get_label(desc) ? : "?", ##__VA_ARGS__); \ + } \ +} while (0) /* With chip prefix */ -- cgit From 2a9101e875bc3aa6423b559e0ea43b2077f3be87 Mon Sep 17 00:00:00 2001 From: Bartosz Golaszewski Date: Wed, 24 Jan 2024 16:29:39 +0100 Subject: gpio: sysfs: use gpio_device_find() to iterate over existing devices With the list of GPIO devices now protected with SRCU we can use gpio_device_find() to traverse it from sysfs. Signed-off-by: Bartosz Golaszewski Reviewed-by: Linus Walleij Acked-by: Andy Shevchenko --- drivers/gpio/gpiolib.h | 1 - 1 file changed, 1 deletion(-) (limited to 'drivers/gpio/gpiolib.h') diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h index d2e73eea9e92..2bf3f9e13ae4 100644 --- a/drivers/gpio/gpiolib.h +++ b/drivers/gpio/gpiolib.h @@ -136,7 +136,6 @@ int gpiod_set_array_value_complex(bool raw, bool can_sleep, int gpiod_set_transitory(struct gpio_desc *desc, bool transitory); extern spinlock_t gpio_lock; -extern struct list_head gpio_devices; void gpiod_line_state_notify(struct gpio_desc *desc, unsigned long action); -- cgit From 35b545332b809a439aa1bf3fde5305e6eb243cf0 Mon Sep 17 00:00:00 2001 From: Bartosz Golaszewski Date: Fri, 12 Jan 2024 14:49:04 +0100 Subject: gpio: remove gpio_lock The "multi-function" gpio_lock is pretty much useless with how it's used in GPIOLIB currently. Because many GPIO API calls can be called from all contexts but may also call into sleeping driver callbacks, there are many places with utterly broken workarounds like yielding the lock to call a possibly sleeping function and then re-acquiring it again without taking into account that the protected state may have changed. It was also used to protect several unrelated things: like individual descriptors AND the GPIO device list. We now serialize access to these two with SRCU and so can finally remove the spinlock. There is of course the question of consistency of lockless access to GPIO descriptors. Because we only support exclusive access to GPIOs (officially anyway, I'm looking at you broken GPIOD_FLAGS_BIT_NONEXCLUSIVE bit...) and the API contract with providers does not guarantee serialization, it's enough to ensure we cannot accidentally dereference an invalid pointer and that the state we present to both users and providers remains consistent. To achieve that: read the flags field atomically except for a few special cases. Read their current value before executing callback code and use this value for any subsequent logic. Modifying the flags depends on the particular use-case and can differ. For instance: when requesting a GPIO, we need to set the REQUESTED bit immediately so that the next user trying to request the same line sees -EBUSY. While at it: the allocations that used GFP_ATOMIC until this point can now switch to GFP_KERNEL. Signed-off-by: Bartosz Golaszewski Reviewed-by: Linus Walleij Acked-by: Andy Shevchenko --- drivers/gpio/gpiolib.h | 2 -- 1 file changed, 2 deletions(-) (limited to 'drivers/gpio/gpiolib.h') diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h index 2bf3f9e13ae4..9b7afe87f1bd 100644 --- a/drivers/gpio/gpiolib.h +++ b/drivers/gpio/gpiolib.h @@ -135,8 +135,6 @@ int gpiod_set_array_value_complex(bool raw, bool can_sleep, int gpiod_set_transitory(struct gpio_desc *desc, bool transitory); -extern spinlock_t gpio_lock; - void gpiod_line_state_notify(struct gpio_desc *desc, unsigned long action); /** -- cgit From 8a5b477bb3e9c891eb064accabb3162ccf2c590e Mon Sep 17 00:00:00 2001 From: Bartosz Golaszewski Date: Sat, 27 Jan 2024 22:14:15 +0100 Subject: gpio: add the can_sleep flag to struct gpio_device Duplicating the can_sleep value in GPIO device will allow us to not needlessly dereference the chip pointer in several places and reduce the number of SRCU read-only critical sections. Signed-off-by: Bartosz Golaszewski Reviewed-by: Linus Walleij Acked-by: Andy Shevchenko --- drivers/gpio/gpiolib.h | 3 +++ 1 file changed, 3 insertions(+) (limited to 'drivers/gpio/gpiolib.h') diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h index 9b7afe87f1bd..43ff4931e2c3 100644 --- a/drivers/gpio/gpiolib.h +++ b/drivers/gpio/gpiolib.h @@ -34,6 +34,8 @@ * @descs: array of ngpio descriptors. * @ngpio: the number of GPIO lines on this GPIO device, equal to the size * of the @descs array. + * @can_sleep: indicate whether the GPIO chip driver's callbacks can sleep + * implying that they cannot be used from atomic context * @base: GPIO base in the DEPRECATED global Linux GPIO numberspace, assigned * at device creation time. * @label: a descriptive name for the GPIO device, such as the part number @@ -64,6 +66,7 @@ struct gpio_device { struct gpio_desc *descs; int base; u16 ngpio; + bool can_sleep; const char *label; void *data; struct list_head list; -- cgit From 47d8b4c1d868148c8fb51b785a89e58ca2d02c4d Mon Sep 17 00:00:00 2001 From: Bartosz Golaszewski Date: Fri, 5 Jan 2024 20:42:44 +0100 Subject: gpio: add SRCU infrastructure to struct gpio_device Add the SRCU struct to GPIO device. It will be used to serialize access to the GPIO chip pointer. Initialize and clean it up where applicable. Signed-off-by: Bartosz Golaszewski Reviewed-by: Linus Walleij Acked-by: Andy Shevchenko --- drivers/gpio/gpiolib.h | 2 ++ 1 file changed, 2 insertions(+) (limited to 'drivers/gpio/gpiolib.h') diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h index 43ff4931e2c3..35d71e30c546 100644 --- a/drivers/gpio/gpiolib.h +++ b/drivers/gpio/gpiolib.h @@ -49,6 +49,7 @@ * @sem: protects the structure from a NULL-pointer dereference of @chip by * user-space operations when the device gets unregistered during * a hot-unplug event + * @srcu: protects the pointer to the underlying GPIO chip * @pin_ranges: range of pins served by the GPIO driver * * This state container holds most of the runtime variable data @@ -73,6 +74,7 @@ struct gpio_device { struct blocking_notifier_head line_state_notifier; struct blocking_notifier_head device_notifier; struct rw_semaphore sem; + struct srcu_struct srcu; #ifdef CONFIG_PINCTRL /* -- cgit From d83cee3d2bb131c7fca7e19f1e33dc7530fd7083 Mon Sep 17 00:00:00 2001 From: Bartosz Golaszewski Date: Tue, 23 Jan 2024 12:01:10 +0100 Subject: gpio: protect the pointer to gpio_chip in gpio_device with SRCU Ensure we cannot crash if the GPIO device gets unregistered (and the chip pointer set to NULL) during any of the API calls. To that end: wait for all users of gdev->chip to exit their read-only SRCU critical sections in gpiochip_remove(). For brevity: add a guard class which can be instantiated at the top of every function requiring read-only access to the chip pointer and use it in all API calls taking a GPIO descriptor as argument. In places where we only deal with the GPIO device - use regular guard() helpers and rcu_dereference() for chip access. Do the same in API calls taking a const pointer to gpio_desc. Signed-off-by: Bartosz Golaszewski Reviewed-by: Linus Walleij Acked-by: Andy Shevchenko --- drivers/gpio/gpiolib.h | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) (limited to 'drivers/gpio/gpiolib.h') diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h index 35d71e30c546..b3810f7d286a 100644 --- a/drivers/gpio/gpiolib.h +++ b/drivers/gpio/gpiolib.h @@ -63,7 +63,7 @@ struct gpio_device { int id; struct device *mockdev; struct module *owner; - struct gpio_chip *chip; + struct gpio_chip __rcu *chip; struct gpio_desc *descs; int base; u16 ngpio; @@ -193,6 +193,26 @@ struct gpio_desc { #define gpiod_not_found(desc) (IS_ERR(desc) && PTR_ERR(desc) == -ENOENT) +struct gpio_chip_guard { + struct gpio_device *gdev; + struct gpio_chip *gc; + int idx; +}; + +DEFINE_CLASS(gpio_chip_guard, + struct gpio_chip_guard, + srcu_read_unlock(&_T.gdev->srcu, _T.idx), + ({ + struct gpio_chip_guard _guard; + + _guard.gdev = desc->gdev; + _guard.idx = srcu_read_lock(&_guard.gdev->srcu); + _guard.gc = rcu_dereference(_guard.gdev->chip); + + _guard; + }), + struct gpio_desc *desc) + int gpiod_request(struct gpio_desc *desc, const char *label); void gpiod_free(struct gpio_desc *desc); -- cgit From f067372c6a3c3b0c5cc775157408970b8fa6c010 Mon Sep 17 00:00:00 2001 From: Bartosz Golaszewski Date: Wed, 24 Jan 2024 17:02:03 +0100 Subject: gpio: remove the RW semaphore from the GPIO device With all accesses to gdev->chip being protected with SRCU, we can now remove the RW-semaphore specific to the character device which fulfilled the same role up to this point. Signed-off-by: Bartosz Golaszewski Reviewed-by: Linus Walleij Acked-by: Andy Shevchenko --- drivers/gpio/gpiolib.h | 5 ----- 1 file changed, 5 deletions(-) (limited to 'drivers/gpio/gpiolib.h') diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h index b3810f7d286a..07443d26cbca 100644 --- a/drivers/gpio/gpiolib.h +++ b/drivers/gpio/gpiolib.h @@ -16,7 +16,6 @@ #include #include #include -#include #include #define GPIOCHIP_NAME "gpiochip" @@ -46,9 +45,6 @@ * requested, released or reconfigured * @device_notifier: used to notify character device wait queues about the GPIO * device being unregistered - * @sem: protects the structure from a NULL-pointer dereference of @chip by - * user-space operations when the device gets unregistered during - * a hot-unplug event * @srcu: protects the pointer to the underlying GPIO chip * @pin_ranges: range of pins served by the GPIO driver * @@ -73,7 +69,6 @@ struct gpio_device { struct list_head list; struct blocking_notifier_head line_state_notifier; struct blocking_notifier_head device_notifier; - struct rw_semaphore sem; struct srcu_struct srcu; #ifdef CONFIG_PINCTRL -- cgit From d82b9e0887e69d9060c854b079a3a5024788f7cb Mon Sep 17 00:00:00 2001 From: Bartosz Golaszewski Date: Wed, 14 Feb 2024 09:44:18 +0100 Subject: gpio: use srcu_dereference() with SRCU-protected pointers Lockdep with CONFIG_PROVE_RCU enabled reports false positives about suspicious rcu_dereference() usage. Let's silence it by using srcu_dereference() which is the correct helper with SRCU. Fixes: d83cee3d2bb1 ("gpio: protect the pointer to gpio_chip in gpio_device with SRCU") Reported-by: kernel test robot Closes: https://lore.kernel.org/oe-lkp/202402122234.d85cca9b-lkp@intel.com Signed-off-by: Bartosz Golaszewski Acked-by: Paul E. McKenney --- drivers/gpio/gpiolib.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) (limited to 'drivers/gpio/gpiolib.h') diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h index 07443d26cbca..ada36aa0f81a 100644 --- a/drivers/gpio/gpiolib.h +++ b/drivers/gpio/gpiolib.h @@ -202,7 +202,8 @@ DEFINE_CLASS(gpio_chip_guard, _guard.gdev = desc->gdev; _guard.idx = srcu_read_lock(&_guard.gdev->srcu); - _guard.gc = rcu_dereference(_guard.gdev->chip); + _guard.gc = srcu_dereference(_guard.gdev->chip, + &_guard.gdev->srcu); _guard; }), -- cgit From 0d776cfd5e5b559fdf2e38285c2aea4b7048acbd Mon Sep 17 00:00:00 2001 From: Stephen Boyd Date: Thu, 22 Feb 2024 22:52:53 -0800 Subject: gpiolib: Pass consumer device through to core in devm_fwnode_gpiod_get_index() This devm API takes a consumer device as an argument to setup the devm action, but throws it away when calling further into gpiolib. This leads to odd debug messages like this: (NULL device *): using DT '/gpio-keys/switch-pen-insert' for '(null)' GPIO lookup Let's pass the consumer device down, by directly calling what fwnode_gpiod_get_index() calls but pass the device used for devm. This changes the message to look like this instead: gpio-keys gpio-keys: using DT '/gpio-keys/switch-pen-insert' for '(null)' GPIO lookup Note that callers of fwnode_gpiod_get_index() will still see the NULL device pointer debug message, but there's not much we can do about that because the API doesn't take a struct device. Cc: Dmitry Torokhov Fixes: 8eb1f71e7acc ("gpiolib: consolidate GPIO lookups") Signed-off-by: Stephen Boyd Signed-off-by: Bartosz Golaszewski --- drivers/gpio/gpiolib.h | 8 ++++++++ 1 file changed, 8 insertions(+) (limited to 'drivers/gpio/gpiolib.h') diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h index ada36aa0f81a..f67d5991ab1c 100644 --- a/drivers/gpio/gpiolib.h +++ b/drivers/gpio/gpiolib.h @@ -223,6 +223,14 @@ static inline int gpiod_request_user(struct gpio_desc *desc, const char *label) return ret; } +struct gpio_desc *gpiod_find_and_request(struct device *consumer, + struct fwnode_handle *fwnode, + const char *con_id, + unsigned int idx, + enum gpiod_flags flags, + const char *label, + bool platform_lookup_allowed); + int gpiod_configure_flags(struct gpio_desc *desc, const char *con_id, unsigned long lflags, enum gpiod_flags dflags); int gpio_set_debounce_timeout(struct gpio_desc *desc, unsigned int debounce); -- cgit