From: Bartosz Golaszewski <[email protected]>
The forward declaration for struct gpio_device should be provided for
both branches of the #ifdef.
Fixes: 08a149c40bdb ("gpiolib: Clean up headers")
Signed-off-by: Bartosz Golaszewski <[email protected]>
---
drivers/gpio/gpiolib-sysfs.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpio/gpiolib-sysfs.h b/drivers/gpio/gpiolib-sysfs.h
index 0f213bdb4732..b794b396d6a5 100644
--- a/drivers/gpio/gpiolib-sysfs.h
+++ b/drivers/gpio/gpiolib-sysfs.h
@@ -3,10 +3,10 @@
#ifndef GPIOLIB_SYSFS_H
#define GPIOLIB_SYSFS_H
-#ifdef CONFIG_GPIO_SYSFS
-
struct gpio_device;
+#ifdef CONFIG_GPIO_SYSFS
+
int gpiochip_sysfs_register(struct gpio_device *gdev);
void gpiochip_sysfs_unregister(struct gpio_device *gdev);
--
2.40.1
From: Bartosz Golaszewski <[email protected]>
The global list of GPIO devices is never modified or accessed from
atomic context so it's fine to protect it using a mutex. Add a new
global lock dedicated to the gpio_devices list and use it whenever
accessing or modifying it.
While at it: fold the sysfs registering of existing devices into
gpiolib.c and make gpio_devices static within its compilation unit.
Signed-off-by: Bartosz Golaszewski <[email protected]>
---
drivers/gpio/gpiolib-sysfs.c | 26 +-----
drivers/gpio/gpiolib-sysfs.h | 6 ++
drivers/gpio/gpiolib.c | 158 ++++++++++++++++++-----------------
drivers/gpio/gpiolib.h | 1 -
4 files changed, 89 insertions(+), 102 deletions(-)
diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index 6f309a3b2d9a..c538568604e8 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -790,9 +790,7 @@ void gpiochip_sysfs_unregister(struct gpio_device *gdev)
static int __init gpiolib_sysfs_init(void)
{
- int status;
- unsigned long flags;
- struct gpio_device *gdev;
+ int status;
status = class_register(&gpio_class);
if (status < 0)
@@ -804,26 +802,6 @@ 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().
*/
- 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;
+ return gpiochip_sysfs_register_all();
}
postcore_initcall(gpiolib_sysfs_init);
diff --git a/drivers/gpio/gpiolib-sysfs.h b/drivers/gpio/gpiolib-sysfs.h
index b794b396d6a5..ab157cec0b4b 100644
--- a/drivers/gpio/gpiolib-sysfs.h
+++ b/drivers/gpio/gpiolib-sysfs.h
@@ -8,6 +8,7 @@ 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
@@ -17,6 +18,11 @@ 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)
{
}
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index a5faaea6915d..f0a51d465df9 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2,6 +2,7 @@
#include <linux/acpi.h>
#include <linux/bitmap.h>
+#include <linux/cleanup.h>
#include <linux/compat.h>
#include <linux/debugfs.h>
#include <linux/device.h>
@@ -15,6 +16,7 @@
#include <linux/kernel.h>
#include <linux/list.h>
#include <linux/module.h>
+#include <linux/mutex.h>
#include <linux/of.h>
#include <linux/pinctrl/consumer.h>
#include <linux/seq_file.h>
@@ -94,7 +96,9 @@ DEFINE_SPINLOCK(gpio_lock);
static DEFINE_MUTEX(gpio_lookup_lock);
static LIST_HEAD(gpio_lookup_list);
-LIST_HEAD(gpio_devices);
+
+static LIST_HEAD(gpio_devices);
+static DEFINE_MUTEX(gpio_devices_lock);
static DEFINE_MUTEX(gpio_machine_hogs_mutex);
static LIST_HEAD(gpio_machine_hogs);
@@ -126,20 +130,15 @@ static inline void desc_set_label(struct gpio_desc *d, const char *label)
struct gpio_desc *gpio_to_desc(unsigned gpio)
{
struct gpio_device *gdev;
- unsigned long flags;
- spin_lock_irqsave(&gpio_lock, flags);
-
- list_for_each_entry(gdev, &gpio_devices, list) {
- if (gdev->base <= gpio &&
- gdev->base + gdev->ngpio > gpio) {
- spin_unlock_irqrestore(&gpio_lock, flags);
- return &gdev->descs[gpio - gdev->base];
+ scoped_guard(mutex, &gpio_devices_lock) {
+ list_for_each_entry(gdev, &gpio_devices, list) {
+ if (gdev->base <= gpio &&
+ gdev->base + gdev->ngpio > gpio)
+ return &gdev->descs[gpio - gdev->base];
}
}
- spin_unlock_irqrestore(&gpio_lock, flags);
-
if (!gpio_is_valid(gpio))
pr_warn("invalid GPIO %d\n", gpio);
@@ -412,26 +411,21 @@ static int gpiodev_add_to_list(struct gpio_device *gdev)
static struct gpio_desc *gpio_name_to_desc(const char * const name)
{
struct gpio_device *gdev;
- unsigned long flags;
if (!name)
return NULL;
- spin_lock_irqsave(&gpio_lock, flags);
+ guard(mutex)(&gpio_devices_lock);
list_for_each_entry(gdev, &gpio_devices, list) {
struct gpio_desc *desc;
for_each_gpio_desc(gdev->chip, desc) {
- if (desc->name && !strcmp(desc->name, name)) {
- spin_unlock_irqrestore(&gpio_lock, flags);
+ if (desc->name && !strcmp(desc->name, name))
return desc;
- }
}
}
- spin_unlock_irqrestore(&gpio_lock, flags);
-
return NULL;
}
@@ -669,11 +663,9 @@ EXPORT_SYMBOL_GPL(gpiochip_line_is_valid);
static void gpiodev_release(struct device *dev)
{
struct gpio_device *gdev = to_gpio_device(dev);
- unsigned long flags;
- spin_lock_irqsave(&gpio_lock, flags);
- list_del(&gdev->list);
- spin_unlock_irqrestore(&gpio_lock, flags);
+ scoped_guard(mutex, &gpio_devices_lock)
+ list_del(&gdev->list);
ida_free(&gpio_ida, gdev->id);
kfree_const(gdev->label);
@@ -726,6 +718,27 @@ static int gpiochip_setup_dev(struct gpio_device *gdev)
return ret;
}
+#if IS_ENABLED(CONFIG_GPIO_SYSFS)
+int gpiochip_sysfs_register_all(void)
+{
+ struct gpio_device *gdev;
+ int ret;
+
+ guard(mutex)(&gpio_devices_lock);
+
+ list_for_each_entry(gdev, &gpio_devices, list) {
+ if (gdev->mockdev)
+ continue;
+
+ ret = gpiochip_sysfs_register(gdev);
+ if (ret)
+ return ret;
+ }
+
+ return 0;
+}
+#endif /* CONFIG_GPIO_SYSFS */
+
static void gpiochip_machine_hog(struct gpio_chip *gc, struct gpiod_hog *hog)
{
struct gpio_desc *desc;
@@ -831,7 +844,6 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
struct lock_class_key *request_key)
{
struct gpio_device *gdev;
- unsigned long flags;
unsigned int i;
int base = 0;
int ret = 0;
@@ -896,48 +908,44 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
gdev->ngpio = gc->ngpio;
- spin_lock_irqsave(&gpio_lock, flags);
-
- /*
- * TODO: this allocates a Linux GPIO number base in the global
- * GPIO numberspace for this chip. In the long run we want to
- * get *rid* of this numberspace and use only descriptors, but
- * it may be a pipe dream. It will not happen before we get rid
- * of the sysfs interface anyways.
- */
- base = gc->base;
- if (base < 0) {
- base = gpiochip_find_base(gc->ngpio);
+ scoped_guard(mutex, &gpio_devices_lock) {
+ /*
+ * TODO: this allocates a Linux GPIO number base in the global
+ * GPIO numberspace for this chip. In the long run we want to
+ * get *rid* of this numberspace and use only descriptors, but
+ * it may be a pipe dream. It will not happen before we get rid
+ * of the sysfs interface anyways.
+ */
+ base = gc->base;
if (base < 0) {
- spin_unlock_irqrestore(&gpio_lock, flags);
- ret = base;
- base = 0;
+ base = gpiochip_find_base(gc->ngpio);
+ if (base < 0) {
+ ret = base;
+ base = 0;
+ goto err_free_label;
+ }
+ /*
+ * TODO: it should not be necessary to reflect the assigned
+ * base outside of the GPIO subsystem. Go over drivers and
+ * see if anyone makes use of this, else drop this and assign
+ * a poison instead.
+ */
+ gc->base = base;
+ } else {
+ dev_warn(&gdev->dev,
+ "Static allocation of GPIO base is deprecated, use dynamic allocation.\n");
+ }
+ gdev->base = base;
+
+ ret = gpiodev_add_to_list(gdev);
+ if (ret) {
+ chip_err(gc, "GPIO integer space overlap, cannot add chip\n");
goto err_free_label;
}
- /*
- * TODO: it should not be necessary to reflect the assigned
- * base outside of the GPIO subsystem. Go over drivers and
- * see if anyone makes use of this, else drop this and assign
- * a poison instead.
- */
- gc->base = base;
- } else {
- dev_warn(&gdev->dev,
- "Static allocation of GPIO base is deprecated, use dynamic allocation.\n");
+
+ for (i = 0; i < gc->ngpio; i++)
+ gdev->descs[i].gdev = gdev;
}
- gdev->base = base;
-
- ret = gpiodev_add_to_list(gdev);
- if (ret) {
- spin_unlock_irqrestore(&gpio_lock, flags);
- chip_err(gc, "GPIO integer space overlap, cannot add chip\n");
- goto err_free_label;
- }
-
- for (i = 0; i < gc->ngpio; i++)
- gdev->descs[i].gdev = gdev;
-
- spin_unlock_irqrestore(&gpio_lock, flags);
BLOCKING_INIT_NOTIFIER_HEAD(&gdev->line_state_notifier);
BLOCKING_INIT_NOTIFIER_HEAD(&gdev->device_notifier);
@@ -1029,9 +1037,8 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
goto err_print_message;
}
err_remove_from_list:
- spin_lock_irqsave(&gpio_lock, flags);
- list_del(&gdev->list);
- spin_unlock_irqrestore(&gpio_lock, flags);
+ scoped_guard(mutex, &gpio_devices_lock)
+ list_del(&gdev->list);
err_free_label:
kfree_const(gdev->label);
err_free_descs:
@@ -4741,35 +4748,32 @@ static void gpiolib_dbg_show(struct seq_file *s, struct gpio_device *gdev)
static void *gpiolib_seq_start(struct seq_file *s, loff_t *pos)
{
- unsigned long flags;
struct gpio_device *gdev = NULL;
loff_t index = *pos;
s->private = "";
- spin_lock_irqsave(&gpio_lock, flags);
+ guard(mutex)(&gpio_devices_lock);
+
list_for_each_entry(gdev, &gpio_devices, list)
- if (index-- == 0) {
- spin_unlock_irqrestore(&gpio_lock, flags);
+ if (index-- == 0)
return gdev;
- }
- spin_unlock_irqrestore(&gpio_lock, flags);
return NULL;
}
static void *gpiolib_seq_next(struct seq_file *s, void *v, loff_t *pos)
{
- unsigned long flags;
struct gpio_device *gdev = v;
void *ret = NULL;
- spin_lock_irqsave(&gpio_lock, flags);
- if (list_is_last(&gdev->list, &gpio_devices))
- ret = NULL;
- else
- ret = list_first_entry(&gdev->list, struct gpio_device, list);
- spin_unlock_irqrestore(&gpio_lock, flags);
+ scoped_guard(mutex, &gpio_devices_lock) {
+ if (list_is_last(&gdev->list, &gpio_devices))
+ ret = NULL;
+ else
+ ret = list_first_entry(&gdev->list, struct gpio_device,
+ list);
+ }
s->private = "\n";
++*pos;
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index 3ccacf3c1288..9278796db079 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -135,7 +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;
-extern struct list_head gpio_devices;
void gpiod_line_state_notify(struct gpio_desc *desc, unsigned long action);
--
2.40.1
On Mon, Nov 27, 2023 at 8:37 PM Bartosz Golaszewski <[email protected]> wrote:
> From: Bartosz Golaszewski <[email protected]>
>
> The forward declaration for struct gpio_device should be provided for
> both branches of the #ifdef.
>
> Fixes: 08a149c40bdb ("gpiolib: Clean up headers")
> Signed-off-by: Bartosz Golaszewski <[email protected]>
Reviewed-by: Linus Walleij <[email protected]>
Yours,
Linus Walleij
On Mon, Nov 27, 2023 at 8:37 PM Bartosz Golaszewski <[email protected]> wrote:
> From: Bartosz Golaszewski <[email protected]>
>
> The global list of GPIO devices is never modified or accessed from
> atomic context so it's fine to protect it using a mutex. Add a new
> global lock dedicated to the gpio_devices list and use it whenever
> accessing or modifying it.
>
> While at it: fold the sysfs registering of existing devices into
> gpiolib.c and make gpio_devices static within its compilation unit.
>
> Signed-off-by: Bartosz Golaszewski <[email protected]>
Nice! I might have found some snag:
gpio_device_find() still does guard(spinlock_irqsave)(&gpio_lock);
shouldn't that be switched to the mutex?
On top of this I can update my patch to the delete the comment
for gpio_lock to just rename that thing to gpio_descriptor_lock
and document it as such.
But when I think about it: gpio[_decriptor]_lock can now (after this
patch) be moved into struct gpio_chip as it is really just protecting
the descriptors on the same chip from simultaneous modification,
especially desc->flags. This is a BIG WIN because it makes it a local
lock not a global one, do you wanna try it or should I? (On top of
these two patches, then.)
Yours,
Linus Walleij
On Tue, Nov 28, 2023 at 3:21 PM Linus Walleij <[email protected]> wrote:
>
> On Mon, Nov 27, 2023 at 8:37 PM Bartosz Golaszewski <[email protected]> wrote:
>
> > From: Bartosz Golaszewski <[email protected]>
> >
> > The global list of GPIO devices is never modified or accessed from
> > atomic context so it's fine to protect it using a mutex. Add a new
> > global lock dedicated to the gpio_devices list and use it whenever
> > accessing or modifying it.
> >
> > While at it: fold the sysfs registering of existing devices into
> > gpiolib.c and make gpio_devices static within its compilation unit.
> >
> > Signed-off-by: Bartosz Golaszewski <[email protected]>
>
> Nice! I might have found some snag:
>
> gpio_device_find() still does guard(spinlock_irqsave)(&gpio_lock);
> shouldn't that be switched to the mutex?
>
Good catch!
> On top of this I can update my patch to the delete the comment
> for gpio_lock to just rename that thing to gpio_descriptor_lock
> and document it as such.
>
No need, this will soon go away anyway. See below.
> But when I think about it: gpio[_decriptor]_lock can now (after this
> patch) be moved into struct gpio_chip as it is really just protecting
> the descriptors on the same chip from simultaneous modification,
> especially desc->flags. This is a BIG WIN because it makes it a local
> lock not a global one, do you wanna try it or should I? (On top of
> these two patches, then.)
>
I will have the series making locking in GPIOLIB more fine-grained
ready tomorrow or on Thursday. It will have separate locks for each
descriptor. We will use spinlock or mutex per descriptor depending on
the value of gc->can_sleep. I think it should work fine as a sleeping
chip can always use a mutex and a non-sleeping one cannot have
sleeping callbacks (correct me if I'm wrong).
We don't need to lock the GPIO device or chip separately - the
descriptor structs will stay alive as long as there's a live reference
to the GPIO device. GPIO device will have an SRCU cookie for
protecting API calls against removal of the chip.
To summarize: one mutex for the GPIO device list, one lock per GPIO
descriptor and SRCU protection of the GPIO device's chip.
Does it make sense?
Bart
> Yours,
> Linus Walleij
On Tue, Nov 28, 2023 at 3:52 PM Bartosz Golaszewski <[email protected]> wrote:
> We don't need to lock the GPIO device or chip separately - the
> descriptor structs will stay alive as long as there's a live reference
> to the GPIO device. GPIO device will have an SRCU cookie for
> protecting API calls against removal of the chip.
>
> To summarize: one mutex for the GPIO device list, one lock per GPIO
> descriptor and SRCU protection of the GPIO device's chip.
>
> Does it make sense?
Absolutely, standing by to review, it's gonna look awesome!
Linus
On Mon, Nov 27, 2023 at 08:37:15PM +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <[email protected]>
>
> The forward declaration for struct gpio_device should be provided for
> both branches of the #ifdef.
Oh, right.
Reviewed-by: Andy Shevchenko <[email protected]>
--
With Best Regards,
Andy Shevchenko
On Mon, Nov 27, 2023 at 08:37:16PM +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <[email protected]>
>
> The global list of GPIO devices is never modified or accessed from
> atomic context so it's fine to protect it using a mutex. Add a new
> global lock dedicated to the gpio_devices list and use it whenever
> accessing or modifying it.
>
> While at it: fold the sysfs registering of existing devices into
> gpiolib.c and make gpio_devices static within its compilation unit.
...
> + scoped_guard(mutex, &gpio_devices_lock) {
This is a lot of churn with this because of switching to RAII.
Can the body be firstly moved to a helper?
> + /*
> + * TODO: this allocates a Linux GPIO number base in the global
> + * GPIO numberspace for this chip. In the long run we want to
> + * get *rid* of this numberspace and use only descriptors, but
> + * it may be a pipe dream. It will not happen before we get rid
> + * of the sysfs interface anyways.
> + */
...
> list_for_each_entry(gdev, &gpio_devices, list)
> + if (index-- == 0)
> return gdev;
I believe this is better with outer {}.
--
With Best Regards,
Andy Shevchenko
On Tue, Nov 28, 2023 at 5:09 PM Andy Shevchenko
<[email protected]> wrote:
>
> On Mon, Nov 27, 2023 at 08:37:16PM +0100, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <[email protected]>
> >
> > The global list of GPIO devices is never modified or accessed from
> > atomic context so it's fine to protect it using a mutex. Add a new
> > global lock dedicated to the gpio_devices list and use it whenever
> > accessing or modifying it.
> >
> > While at it: fold the sysfs registering of existing devices into
> > gpiolib.c and make gpio_devices static within its compilation unit.
>
> ...
>
> > + scoped_guard(mutex, &gpio_devices_lock) {
>
> This is a lot of churn with this because of switching to RAII.
> Can the body be firstly moved to a helper?
>
But that would mean more churn. I don't get why you insist on
splitting these everytime. We're going from spinlock to a mutex so we
may as well use guards right away.
> > + /*
> > + * TODO: this allocates a Linux GPIO number base in the global
> > + * GPIO numberspace for this chip. In the long run we want to
> > + * get *rid* of this numberspace and use only descriptors, but
> > + * it may be a pipe dream. It will not happen before we get rid
> > + * of the sysfs interface anyways.
> > + */
>
> ...
>
> > list_for_each_entry(gdev, &gpio_devices, list)
> > + if (index-- == 0)
> > return gdev;
>
> I believe this is better with outer {}.
>
Right.
Bart
> --
> With Best Regards,
> Andy Shevchenko
>
>
On Mon, Nov 27, 2023 at 8:37 PM Bartosz Golaszewski <[email protected]> wrote:
>
> From: Bartosz Golaszewski <[email protected]>
>
> The forward declaration for struct gpio_device should be provided for
> both branches of the #ifdef.
>
> Fixes: 08a149c40bdb ("gpiolib: Clean up headers")
> Signed-off-by: Bartosz Golaszewski <[email protected]>
> ---
> drivers/gpio/gpiolib-sysfs.h | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib-sysfs.h b/drivers/gpio/gpiolib-sysfs.h
> index 0f213bdb4732..b794b396d6a5 100644
> --- a/drivers/gpio/gpiolib-sysfs.h
> +++ b/drivers/gpio/gpiolib-sysfs.h
> @@ -3,10 +3,10 @@
> #ifndef GPIOLIB_SYSFS_H
> #define GPIOLIB_SYSFS_H
>
> -#ifdef CONFIG_GPIO_SYSFS
> -
> struct gpio_device;
>
> +#ifdef CONFIG_GPIO_SYSFS
> +
> int gpiochip_sysfs_register(struct gpio_device *gdev);
> void gpiochip_sysfs_unregister(struct gpio_device *gdev);
>
> --
> 2.40.1
>
I applied this. I'll make patch 2/2 part of the upcoming locking rework.
Bart