Skip to content

Commit

Permalink
gpiolib: use a mutex to protect the list of GPIO devices
Browse files Browse the repository at this point in the history 
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.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
  • Loading branch information
Bartosz Golaszewski committed Dec 18, 2023
1 parent f95fd4a commit 65a828b
Show file tree
Hide file tree
Showing 4 changed files with 90 additions and 100 deletions.
45 changes: 21 additions & 24 deletions drivers/gpio/gpiolib-sysfs.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -766,6 +766,25 @@ int gpiochip_sysfs_register(struct gpio_device *gdev)
return 0;
}

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;
}

void gpiochip_sysfs_unregister(struct gpio_device *gdev)
{
struct gpio_desc *desc;
Expand All @@ -790,9 +809,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)
Expand All @@ -804,26 +821,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);
6 changes: 6 additions & 0 deletions drivers/gpio/gpiolib-sysfs.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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)
{
}
Expand Down
137 changes: 61 additions & 76 deletions drivers/gpio/gpiolib.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand All @@ -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>
Expand Down Expand Up @@ -94,7 +96,9 @@ DEFINE_SPINLOCK(gpio_lock);

static DEFINE_MUTEX(gpio_lookup_lock);
static LIST_HEAD(gpio_lookup_list);

LIST_HEAD(gpio_devices);
DEFINE_MUTEX(gpio_devices_lock);

static DEFINE_MUTEX(gpio_machine_hogs_mutex);
static LIST_HEAD(gpio_machine_hogs);
Expand Down Expand Up @@ -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);

Expand Down Expand Up @@ -412,26 +411,21 @@ static int gpiodev_add_to_list_unlocked(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;
}

Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -831,7 +823,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;
Expand Down Expand Up @@ -896,49 +887,46 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,

gdev->ngpio = gc->ngpio;

spin_lock_irqsave(&gpio_lock, flags);
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;

/*
* 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_unlocked(gc->ngpio);
if (base < 0) {
spin_unlock_irqrestore(&gpio_lock, flags);
ret = base;
base = 0;
base = gpiochip_find_base_unlocked(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_unlocked(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");
}
gdev->base = base;

ret = gpiodev_add_to_list_unlocked(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;
}

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);
init_rwsem(&gdev->sem);
Expand Down Expand Up @@ -1029,9 +1017,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:
Expand Down Expand Up @@ -1140,7 +1127,7 @@ struct gpio_device *gpio_device_find(void *data,
*/
might_sleep();

guard(spinlock_irqsave)(&gpio_lock);
guard(mutex)(&gpio_devices_lock);

list_for_each_entry(gdev, &gpio_devices, list) {
if (gdev->chip && match(gdev->chip, data))
Expand Down Expand Up @@ -4756,35 +4743,33 @@ 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);
list_for_each_entry(gdev, &gpio_devices, list)
if (index-- == 0) {
spin_unlock_irqrestore(&gpio_lock, flags);
guard(mutex)(&gpio_devices_lock);

list_for_each_entry(gdev, &gpio_devices, list) {
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;
Expand Down
2 changes: 2 additions & 0 deletions drivers/gpio/gpiolib.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
#include <linux/gpio/consumer.h> /* for enum gpiod_flags */
#include <linux/gpio/driver.h>
#include <linux/module.h>
#include <linux/mutex.h>
#include <linux/notifier.h>
#include <linux/rwsem.h>

Expand Down Expand Up @@ -136,6 +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;

void gpiod_line_state_notify(struct gpio_desc *desc, unsigned long action);

Expand Down

0 comments on commit 65a828b

Please sign in to comment.