cleanup: provide and use a locking guard for nested read semaphores
Subsystems (2)
cover cleanup: provide and use a locking guard for nested read semaphores
From: Bartosz Golaszewski <[email protected]>
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(-)
Signature · 1 line
2.40.1
PATCH 1/3 cleanup: provide DEFINE_LOCK_GUARD_ARGS()
From: Bartosz Golaszewski <[email protected]>
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.
--- include/linux/cleanup.h | 6 ++++++ 1 file changed, 6 insertions(+)
include/linux/cleanup.h +6 -0
@@ -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 */
Signature · 1 line
2.40.1
Re: [PATCH 1/3] cleanup: provide DEFINE_LOCK_GUARD_ARGS()
Quoted 16 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>
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 <[email protected]>--- include/linux/cleanup.h | 6 ++++++ 1 file changed, 6 insertions(+)include/linux/cleanup.h +0
-0
include/linux/cleanup.h __DEFINE_LOCK_GUARD_0(_name, _lock) · line 255
Quoted 10 lines
include/linux/cleanup.h +6
-0@@ -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:
include/linux/cleanup.h +5 -2
@@ -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
Quoted 4 lines
include/linux/cleanup.h +0
-0#endif /* __LINUX_GUARDS_H */ Signature · 1 line
2.40.1
PATCH 2/3 locking/rwsem: provide a lock guard for down_read_nested()
In reply to Bartosz GolaszewskiFrom: Bartosz Golaszewski <[email protected]>
This adds a lock guard for taking an RW semaphore for reading in nested context. It takes the nesting depth as a second argument.
--- include/linux/rwsem.h | 6 ++++++ 1 file changed, 6 insertions(+)
include/linux/rwsem.h +6 -0
@@ -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 */
Signature · 1 line
2.40.1
PATCH 3/3 gpiolib: pin GPIO devices in place during descriptor lookup
In reply to Bartosz GolaszewskiFrom: Bartosz Golaszewski <[email protected]>
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().
--- drivers/gpio/gpiolib.c | 42 ++++++++++++++++++++++++------------------ 1 file changed, 24 insertions(+), 18 deletions(-)
drivers/gpio/gpiolib.c +24 -18
@@ -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);
Signature · 1 line
2.40.1