[PATCH 0/3] cleanup: provide and use a locking guard for nested read semaphores

[PATCH 0/3] cleanup: provide and use a locking guard for nested read semaphores

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

This series adds cleanup infrastructure for defining locking guards with
additional arguments, then uses it to provide a guard for
down_read_nested() with RW semaphores and finally uses the new guard in
GPIOLIB.

Patch 3/3 of this series is a second attempt, this time with
lockdep-correct nesting.

The first two patches can either be picked up into the relevant
maintainer trees and I can pull an immutable tag or can be acked and go
directly through the GPIO tree.

Bartosz Golaszewski (3):
  cleanup: provide DEFINE_LOCK_GUARD_ARGS()
  locking/rwsem: provide a lock guard for down_read_nested()
  gpiolib: pin GPIO devices in place during descriptor lookup

 drivers/gpio/gpiolib.c  | 42 +++++++++++++++++++++++------------------
 include/linux/cleanup.h |  6 ++++++
 include/linux/rwsem.h   |  6 ++++++
 3 files changed, 36 insertions(+), 18 deletions(-)

-- 
2.40.1

[PATCH 1/3] cleanup: provide DEFINE_LOCK_GUARD_ARGS()

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

This macro allows defining lock guard with additional arguments that
can be passed to the locking function. This is useful for implementing
guards for nested locking.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 include/linux/cleanup.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/include/linux/cleanup.h b/include/linux/cleanup.h
index c2d09bc4f976..921db45023bb 100644
--- a/include/linux/cleanup.h
+++ b/include/linux/cleanup.h
@@ -246,5 +246,11 @@ __DEFINE_LOCK_GUARD_0(_name, _lock)
 	static inline void * class_##_name##_ext##_lock_ptr(class_##_name##_t *_T) \
 	{ return class_##_name##_lock_ptr(_T); }
 
+/*
+ * Helper for implementing guard locks with additional arguments passed to
+ * the locking function.
+ */
+#define DEFINE_LOCK_GUARD_ARGS(_name, _type, _lock, _unlock, _args...)	\
+DEFINE_CLASS(_name, _type, _unlock, ({ _lock; _T; }), _type _T, _args)
 
 #endif /* __LINUX_GUARDS_H */
-- 
2.40.1

Re: [PATCH 1/3] cleanup: provide DEFINE_LOCK_GUARD_ARGS()

[Boqun Feng]

On Wed, Jan 10, 2024 at 09:32:13PM +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> This macro allows defining lock guard with additional arguments that
> can be passed to the locking function. This is useful for implementing
> guards for nested locking.
> 
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---
>  include/linux/cleanup.h | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/include/linux/cleanup.h b/include/linux/cleanup.h
> index c2d09bc4f976..921db45023bb 100644
> --- a/include/linux/cleanup.h
> +++ b/include/linux/cleanup.h
> @@ -246,5 +246,11 @@ __DEFINE_LOCK_GUARD_0(_name, _lock)
>  	static inline void * class_##_name##_ext##_lock_ptr(class_##_name##_t *_T) \
>  	{ return class_##_name##_lock_ptr(_T); }
>  
> +/*
> + * Helper for implementing guard locks with additional arguments passed to
> + * the locking function.
> + */
> +#define DEFINE_LOCK_GUARD_ARGS(_name, _type, _lock, _unlock, _args...)	\
> +DEFINE_CLASS(_name, _type, _unlock, ({ _lock; _T; }), _type _T, _args)
>  

First I think the name should really be DEFINE_GUARD_ARGS(), these
DEFINE_LOCK_GUARD_*() functions have different meaning. Also this should
really be a more generic case than DEFINE_GUARD(), so how about the
following:

diff --git a/include/linux/cleanup.h b/include/linux/cleanup.h
index c2d09bc4f976..4fcdcb478fd1 100644
--- a/include/linux/cleanup.h
+++ b/include/linux/cleanup.h
@@ -148,11 +148,14 @@ static inline class_##_name##_t class_##_name##ext##_constructor(_init_args) \
  *
  */
 
-#define DEFINE_GUARD(_name, _type, _lock, _unlock) \
-	DEFINE_CLASS(_name, _type, if (_T) { _unlock; }, ({ _lock; _T; }), _type _T); \
+#define DEFINE_GUARD_ARGS(_name, _type, _lock, _unlock, _args...) \
+	DEFINE_CLASS(_name, _type, if (_T) { _unlock; }, ({ _lock; _T; }), _type _T, ##_args) \
 	static inline void * class_##_name##_lock_ptr(class_##_name##_t *_T) \
 	{ return *_T; }
 
+#define DEFINE_GUARD(_name, _type, _lock, _unlock) \
+	DEFINE_GUARD_ARGS(_name, _type, _lock, _unlock)
+
 #define DEFINE_GUARD_COND(_name, _ext, _condlock) \
 	EXTEND_CLASS(_name, _ext, \
 		     ({ void *_t = _T; if (_T && !(_condlock)) _t = NULL; _t; }), \

Thoughts?

Regards,
Boqun


>  #endif /* __LINUX_GUARDS_H */
> -- 
> 2.40.1
> 

[PATCH 2/3] locking/rwsem: provide a lock guard for down_read_nested()

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

This adds a lock guard for taking an RW semaphore for reading in nested
context. It takes the nesting depth as a second argument.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 include/linux/rwsem.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/include/linux/rwsem.h b/include/linux/rwsem.h
index 9c29689ff505..298f5e60d30c 100644
--- a/include/linux/rwsem.h
+++ b/include/linux/rwsem.h
@@ -258,4 +258,10 @@ extern void up_read_non_owner(struct rw_semaphore *sem);
 # define up_read_non_owner(sem)			up_read(sem)
 #endif
 
+DEFINE_LOCK_GUARD_ARGS(rwsem_read_nested,
+		       struct rw_semaphore *,
+		       down_read_nested(_T, subclass),
+		       up_read(_T),
+		       int subclass);
+
 #endif /* _LINUX_RWSEM_H */
-- 
2.40.1

[PATCH 3/3] gpiolib: pin GPIO devices in place during descriptor lookup

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

There's time between when we locate the relevant descriptor during
lookup and when we actually take the reference to its parent GPIO
device where - if the GPIO device in question is removed - we'll end up
with a dangling pointer to freed memory. Make sure devices cannot be
removed until we hold a new reference to the device.

In order to not overcomplicate the code with locking and unlocking
variants of the same functions, just use the nesting feature of RW
semaphores in gpio_device_find().

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/gpio/gpiolib.c | 42 ++++++++++++++++++++++++------------------
 1 file changed, 24 insertions(+), 18 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 4c93cf73a826..972f33bc1a72 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1114,7 +1114,7 @@ struct gpio_device *gpio_device_find(void *data,
 	 */
 	might_sleep();
 
-	guard(rwsem_read)(&gpio_devices_sem);
+	guard(rwsem_read_nested)(&gpio_devices_sem, SINGLE_DEPTH_NESTING);
 
 	list_for_each_entry(gdev, &gpio_devices, list) {
 		if (gdev->chip && match(gdev->chip, data))
@@ -4134,27 +4134,33 @@ static struct gpio_desc *gpiod_find_and_request(struct device *consumer,
 	struct gpio_desc *desc;
 	int ret;
 
-	desc = gpiod_find_by_fwnode(fwnode, consumer, con_id, idx, &flags, &lookupflags);
-	if (gpiod_not_found(desc) && platform_lookup_allowed) {
+	scoped_guard(rwsem_read, &gpio_devices_sem) {
+		desc = gpiod_find_by_fwnode(fwnode, consumer, con_id, idx,
+					    &flags, &lookupflags);
+		if (gpiod_not_found(desc) && platform_lookup_allowed) {
+			/*
+			 * Either we are not using DT or ACPI, or their lookup
+			 * did not return a result. In that case, use platform
+			 * lookup as a fallback.
+			 */
+			dev_dbg(consumer,
+				"using lookup tables for GPIO lookup\n");
+			desc = gpiod_find(consumer, con_id, idx, &lookupflags);
+		}
+
+		if (IS_ERR(desc)) {
+			dev_dbg(consumer, "No GPIO consumer %s found\n",
+				con_id);
+			return desc;
+		}
+
 		/*
-		 * Either we are not using DT or ACPI, or their lookup did not
-		 * return a result. In that case, use platform lookup as a
-		 * fallback.
+		 * If a connection label was passed use that, else attempt to
+		 * use the device name as label
 		 */
-		dev_dbg(consumer, "using lookup tables for GPIO lookup\n");
-		desc = gpiod_find(consumer, con_id, idx, &lookupflags);
+		ret = gpiod_request(desc, label);
 	}
 
-	if (IS_ERR(desc)) {
-		dev_dbg(consumer, "No GPIO consumer %s found\n", con_id);
-		return desc;
-	}
-
-	/*
-	 * If a connection label was passed use that, else attempt to use
-	 * the device name as label
-	 */
-	ret = gpiod_request(desc, label);
 	if (ret) {
 		if (!(ret == -EBUSY && flags & GPIOD_FLAGS_BIT_NONEXCLUSIVE))
 			return ERR_PTR(ret);
-- 
2.40.1