* [PATCH 0/2] gpiolib: use a read-write semaphore to protect the GPIO device list
@ 2023-12-21 18:43 Bartosz Golaszewski
2023-12-21 18:43 ` [PATCH 1/2] gpiolib: replace the GPIO device mutex with a read-write semaphore Bartosz Golaszewski
2023-12-21 18:43 ` [PATCH 2/2] gpiolib: pin GPIO devices in place during descriptor lookup Bartosz Golaszewski
0 siblings, 2 replies; 4+ messages in thread
From: Bartosz Golaszewski @ 2023-12-21 18:43 UTC (permalink / raw)
To: Linus Walleij, Andy Shevchenko, Kent Gibson
Cc: linux-gpio, linux-kernel, Bartosz Golaszewski
From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
I'm still figuring out how to keep GPIO descriptors coherent while
(mostly) lockless. In the meantime, I found a potential race-condition
during GPIO descriptor lookup and also figured that the correct way to
protect the GPIO device list is actually a read-write semaphore as we're
not modifying the list very often and readers should be able to iterate
over it concurrently.
Bartosz Golaszewski (2):
gpiolib: replace the GPIO device mutex with a read-write semaphore
gpiolib: pin GPIO devices in place during descriptor lookup
drivers/gpio/gpiolib-sysfs.c | 2 +-
drivers/gpio/gpiolib.c | 58 ++++++++++++++++++++----------------
drivers/gpio/gpiolib.h | 2 +-
3 files changed, 34 insertions(+), 28 deletions(-)
--
2.40.1
^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH 1/2] gpiolib: replace the GPIO device mutex with a read-write semaphore
2023-12-21 18:43 [PATCH 0/2] gpiolib: use a read-write semaphore to protect the GPIO device list Bartosz Golaszewski
@ 2023-12-21 18:43 ` Bartosz Golaszewski
2023-12-21 18:43 ` [PATCH 2/2] gpiolib: pin GPIO devices in place during descriptor lookup Bartosz Golaszewski
1 sibling, 0 replies; 4+ messages in thread
From: Bartosz Golaszewski @ 2023-12-21 18:43 UTC (permalink / raw)
To: Linus Walleij, Andy Shevchenko, Kent Gibson
Cc: linux-gpio, linux-kernel, Bartosz Golaszewski
From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
There are only two spots where we modify (add to or remove objects from)
the GPIO device list. Readers should be able to access it concurrently.
Replace the mutex with a read-write semaphore and adjust the locking
operations accordingly.
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
drivers/gpio/gpiolib-sysfs.c | 2 +-
drivers/gpio/gpiolib.c | 18 +++++++++---------
drivers/gpio/gpiolib.h | 2 +-
3 files changed, 11 insertions(+), 11 deletions(-)
diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index ae4fc013b675..e4159344a0ea 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -771,7 +771,7 @@ int gpiochip_sysfs_register_all(void)
struct gpio_device *gdev;
int ret;
- guard(mutex)(&gpio_devices_lock);
+ guard(rwsem_read)(&gpio_devices_sem);
list_for_each_entry(gdev, &gpio_devices, list) {
if (gdev->mockdev)
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index c9ca809b55de..1baeb6778ec6 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -98,7 +98,7 @@ static DEFINE_MUTEX(gpio_lookup_lock);
static LIST_HEAD(gpio_lookup_list);
LIST_HEAD(gpio_devices);
-DEFINE_MUTEX(gpio_devices_lock);
+DECLARE_RWSEM(gpio_devices_sem);
static DEFINE_MUTEX(gpio_machine_hogs_mutex);
static LIST_HEAD(gpio_machine_hogs);
@@ -131,7 +131,7 @@ struct gpio_desc *gpio_to_desc(unsigned gpio)
{
struct gpio_device *gdev;
- scoped_guard(mutex, &gpio_devices_lock) {
+ scoped_guard(rwsem_read, &gpio_devices_sem) {
list_for_each_entry(gdev, &gpio_devices, list) {
if (gdev->base <= gpio &&
gdev->base + gdev->ngpio > gpio)
@@ -415,7 +415,7 @@ static struct gpio_desc *gpio_name_to_desc(const char * const name)
if (!name)
return NULL;
- guard(mutex)(&gpio_devices_lock);
+ guard(rwsem_read)(&gpio_devices_sem);
list_for_each_entry(gdev, &gpio_devices, list) {
struct gpio_desc *desc;
@@ -664,7 +664,7 @@ static void gpiodev_release(struct device *dev)
{
struct gpio_device *gdev = to_gpio_device(dev);
- scoped_guard(mutex, &gpio_devices_lock)
+ scoped_guard(rwsem_write, &gpio_devices_sem)
list_del(&gdev->list);
ida_free(&gpio_ida, gdev->id);
@@ -887,7 +887,7 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
gdev->ngpio = gc->ngpio;
- scoped_guard(mutex, &gpio_devices_lock) {
+ 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
@@ -1017,7 +1017,7 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
goto err_print_message;
}
err_remove_from_list:
- scoped_guard(mutex, &gpio_devices_lock)
+ scoped_guard(rwsem_write, &gpio_devices_sem)
list_del(&gdev->list);
err_free_label:
kfree_const(gdev->label);
@@ -1127,7 +1127,7 @@ struct gpio_device *gpio_device_find(void *data,
*/
might_sleep();
- guard(mutex)(&gpio_devices_lock);
+ guard(rwsem_read)(&gpio_devices_sem);
list_for_each_entry(gdev, &gpio_devices, list) {
if (gdev->chip && match(gdev->chip, data))
@@ -4745,7 +4745,7 @@ static void *gpiolib_seq_start(struct seq_file *s, loff_t *pos)
s->private = "";
- guard(mutex)(&gpio_devices_lock);
+ guard(rwsem_read)(&gpio_devices_sem);
list_for_each_entry(gdev, &gpio_devices, list) {
if (index-- == 0)
@@ -4760,7 +4760,7 @@ static void *gpiolib_seq_next(struct seq_file *s, void *v, loff_t *pos)
struct gpio_device *gdev = v;
void *ret = NULL;
- scoped_guard(mutex, &gpio_devices_lock) {
+ scoped_guard(rwsem_read, &gpio_devices_sem) {
if (list_is_last(&gdev->list, &gpio_devices))
ret = NULL;
else
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index 0ce7451a6b24..97df54abf57a 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -137,7 +137,7 @@ int gpiod_set_transitory(struct gpio_desc *desc, bool transitory);
extern spinlock_t gpio_lock;
extern struct list_head gpio_devices;
-extern struct mutex gpio_devices_lock;
+extern struct rw_semaphore gpio_devices_sem;
void gpiod_line_state_notify(struct gpio_desc *desc, unsigned long action);
--
2.40.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH 2/2] gpiolib: pin GPIO devices in place during descriptor lookup
2023-12-21 18:43 [PATCH 0/2] gpiolib: use a read-write semaphore to protect the GPIO device list Bartosz Golaszewski
2023-12-21 18:43 ` [PATCH 1/2] gpiolib: replace the GPIO device mutex with a read-write semaphore Bartosz Golaszewski
@ 2023-12-21 18:43 ` Bartosz Golaszewski
2023-12-21 19:03 ` Bartosz Golaszewski
1 sibling, 1 reply; 4+ messages in thread
From: Bartosz Golaszewski @ 2023-12-21 18:43 UTC (permalink / raw)
To: Linus Walleij, Andy Shevchenko, Kent Gibson
Cc: linux-gpio, linux-kernel, 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.
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
drivers/gpio/gpiolib.c | 40 +++++++++++++++++++++++-----------------
1 file changed, 23 insertions(+), 17 deletions(-)
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 1baeb6778ec6..8a15b8f6b50e 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -4147,27 +4147,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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH 2/2] gpiolib: pin GPIO devices in place during descriptor lookup
2023-12-21 18:43 ` [PATCH 2/2] gpiolib: pin GPIO devices in place during descriptor lookup Bartosz Golaszewski
@ 2023-12-21 19:03 ` Bartosz Golaszewski
0 siblings, 0 replies; 4+ messages in thread
From: Bartosz Golaszewski @ 2023-12-21 19:03 UTC (permalink / raw)
To: Linus Walleij, Andy Shevchenko, Kent Gibson
Cc: linux-gpio, linux-kernel, Bartosz Golaszewski
On Thu, Dec 21, 2023 at 7:43 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> 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.
>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---
> drivers/gpio/gpiolib.c | 40 +++++++++++++++++++++++-----------------
> 1 file changed, 23 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 1baeb6778ec6..8a15b8f6b50e 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -4147,27 +4147,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) {
I am too sleep deprived to be coding. This doesn't make sense because
if we are held at the write semaphore in gpiodev_release() then it's
already too late as the device is being removed already and we'll
still end up with a dangling pointer.
Should we hold off any reference putting for all GPIO devices when a
descriptor lookup is in progress?
Bart
> + 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
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-12-21 19:03 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-12-21 18:43 [PATCH 0/2] gpiolib: use a read-write semaphore to protect the GPIO device list Bartosz Golaszewski
2023-12-21 18:43 ` [PATCH 1/2] gpiolib: replace the GPIO device mutex with a read-write semaphore Bartosz Golaszewski
2023-12-21 18:43 ` [PATCH 2/2] gpiolib: pin GPIO devices in place during descriptor lookup Bartosz Golaszewski
2023-12-21 19:03 ` Bartosz Golaszewski
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).