Skip to content

Commit

Permalink
gpio: remove gpio_lock
Browse files Browse the repository at this point in the history
The "multi-function" gpio_lock is pretty much useless with how it's used
in GPIOLIB currently. Because many GPIO API calls can be called from all
contexts but may also call into sleeping driver callbacks, there are
many places with utterly broken workarounds like yielding the lock to
call a possibly sleeping function and then re-acquiring it again without
taking into account that the protected state may have changed.

It was also used to protect several unrelated things: like individual
descriptors AND the GPIO device list. We now serialize access to these
two with SRCU and so can finally remove the spinlock.

There is of course the question of consistency of lockless access to
GPIO descriptors. Because we only support exclusive access to GPIOs
(officially anyway, I'm looking at you broken
GPIOD_FLAGS_BIT_NONEXCLUSIVE bit...) and the API contract with providers
does not guarantee serialization, it's enough to ensure we cannot
accidentally dereference an invalid pointer and that the state we present
to both users and providers remains consistent. To achieve that: read the
flags field atomically except for a few special cases. Read their current
value before executing callback code and use this value for any subsequent
logic. Modifying the flags depends on the particular use-case and can
differ. For instance: when requesting a GPIO, we need to set the
REQUESTED bit immediately so that the next user trying to request the
same line sees -EBUSY.

While at it: the allocations that used GFP_ATOMIC until this point can
now switch to GFP_KERNEL.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Acked-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
  • Loading branch information
Bartosz Golaszewski committed Feb 12, 2024
1 parent 2a9101e commit 35b5453
Show file tree
Hide file tree
Showing 4 changed files with 47 additions and 98 deletions.
20 changes: 9 additions & 11 deletions drivers/gpio/gpiolib-cdev.c
Original file line number Diff line number Diff line change
Expand Up @@ -2302,18 +2302,16 @@ static void gpio_desc_to_lineinfo(struct gpio_desc *desc,
memset(info, 0, sizeof(*info));
info->offset = gpio_chip_hwgpio(desc);

scoped_guard(spinlock_irqsave, &gpio_lock) {
if (desc->name)
strscpy(info->name, desc->name, sizeof(info->name));

scoped_guard(srcu, &desc->srcu) {
label = gpiod_get_label(desc);
if (label)
strscpy(info->consumer, label,
sizeof(info->consumer));
}
if (desc->name)
strscpy(info->name, desc->name, sizeof(info->name));

dflags = READ_ONCE(desc->flags);

dflags = READ_ONCE(desc->flags);
scoped_guard(srcu, &desc->srcu) {
label = gpiod_get_label(desc);
if (label && test_bit(FLAG_REQUESTED, &dflags))
strscpy(info->consumer, label,
sizeof(info->consumer));
}

/*
Expand Down
17 changes: 6 additions & 11 deletions drivers/gpio/gpiolib-sysfs.c
Original file line number Diff line number Diff line change
Expand Up @@ -563,7 +563,6 @@ int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
struct gpio_device *gdev;
struct gpiod_data *data;
struct gpio_chip *chip;
unsigned long flags;
struct device *dev;
int status, offset;

Expand All @@ -578,6 +577,9 @@ int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
return -EINVAL;
}

if (!test_and_set_bit(FLAG_EXPORT, &desc->flags))
return -EPERM;

gdev = desc->gdev;
chip = gdev->chip;

Expand All @@ -589,18 +591,11 @@ int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
goto err_unlock;
}

spin_lock_irqsave(&gpio_lock, flags);
if (!test_bit(FLAG_REQUESTED, &desc->flags) ||
test_bit(FLAG_EXPORT, &desc->flags)) {
spin_unlock_irqrestore(&gpio_lock, flags);
gpiod_dbg(desc, "%s: unavailable (requested=%d, exported=%d)\n",
__func__,
test_bit(FLAG_REQUESTED, &desc->flags),
test_bit(FLAG_EXPORT, &desc->flags));
if (!test_bit(FLAG_REQUESTED, &desc->flags)) {
gpiod_dbg(desc, "%s: unavailable (not requested)\n", __func__);
status = -EPERM;
goto err_unlock;
}
spin_unlock_irqrestore(&gpio_lock, flags);

data = kzalloc(sizeof(*data), GFP_KERNEL);
if (!data) {
Expand Down Expand Up @@ -628,14 +623,14 @@ int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
goto err_free_data;
}

set_bit(FLAG_EXPORT, &desc->flags);
mutex_unlock(&sysfs_lock);
return 0;

err_free_data:
kfree(data);
err_unlock:
mutex_unlock(&sysfs_lock);
clear_bit(FLAG_EXPORT, &desc->flags);
gpiod_dbg(desc, "%s: status %d\n", __func__, status);
return status;
}
Expand Down
106 changes: 32 additions & 74 deletions drivers/gpio/gpiolib.c
Original file line number Diff line number Diff line change
Expand Up @@ -76,12 +76,6 @@ static const struct bus_type gpio_bus_type = {
*/
#define FASTPATH_NGPIO CONFIG_GPIOLIB_FASTPATH_LIMIT

/* gpio_lock prevents conflicts during gpio_desc[] table updates.
* While any GPIO is requested, its gpio_chip is not removable;
* each GPIO's "requested" flag serves as a lock and refcount.
*/
DEFINE_SPINLOCK(gpio_lock);

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

Expand Down Expand Up @@ -123,8 +117,7 @@ static int desc_set_label(struct gpio_desc *desc, const char *label)
const char *new = NULL, *old;

if (label) {
/* FIXME: make this GFP_KERNEL once the spinlock is out. */
new = kstrdup_const(label, GFP_ATOMIC);
new = kstrdup_const(label, GFP_KERNEL);
if (!new)
return -ENOMEM;
}
Expand Down Expand Up @@ -1093,7 +1086,6 @@ EXPORT_SYMBOL_GPL(gpiochip_add_data_with_key);
void gpiochip_remove(struct gpio_chip *gc)
{
struct gpio_device *gdev = gc->gpiodev;
unsigned long flags;
unsigned int i;

down_write(&gdev->sem);
Expand All @@ -1119,12 +1111,10 @@ void gpiochip_remove(struct gpio_chip *gc)
*/
gpiochip_set_data(gc, NULL);

spin_lock_irqsave(&gpio_lock, flags);
for (i = 0; i < gdev->ngpio; i++) {
if (test_bit(FLAG_REQUESTED, &gdev->descs[i].flags))
break;
}
spin_unlock_irqrestore(&gpio_lock, flags);

if (i != gdev->ngpio)
dev_crit(&gdev->dev,
Expand Down Expand Up @@ -2227,62 +2217,43 @@ EXPORT_SYMBOL_GPL(gpiochip_remove_pin_ranges);
static int gpiod_request_commit(struct gpio_desc *desc, const char *label)
{
struct gpio_chip *gc = desc->gdev->chip;
unsigned long flags;
unsigned int offset;
int ret;

if (test_and_set_bit(FLAG_REQUESTED, &desc->flags))
return -EBUSY;

if (label) {
label = kstrdup_const(label, GFP_KERNEL);
if (!label)
return -ENOMEM;
}

spin_lock_irqsave(&gpio_lock, flags);

/* NOTE: gpio_request() can be called in early boot,
* before IRQs are enabled, for non-sleeping (SOC) GPIOs.
*/

if (test_and_set_bit(FLAG_REQUESTED, &desc->flags)) {
ret = -EBUSY;
goto out_free_unlock;
}

if (gc->request) {
/* gc->request may sleep */
spin_unlock_irqrestore(&gpio_lock, flags);
offset = gpio_chip_hwgpio(desc);
if (gpiochip_line_is_valid(gc, offset))
ret = gc->request(gc, offset);
else
ret = -EINVAL;
spin_lock_irqsave(&gpio_lock, flags);

if (ret) {
desc_set_label(desc, NULL);
clear_bit(FLAG_REQUESTED, &desc->flags);
goto out_free_unlock;
}
if (ret)
goto out_clear_bit;
}
if (gc->get_direction) {
/* gc->get_direction may sleep */
spin_unlock_irqrestore(&gpio_lock, flags);

if (gc->get_direction)
gpiod_get_direction(desc);
spin_lock_irqsave(&gpio_lock, flags);
}
spin_unlock_irqrestore(&gpio_lock, flags);

ret = desc_set_label(desc, label ? : "?");
if (ret) {
clear_bit(FLAG_REQUESTED, &desc->flags);
return ret;
}
if (ret)
goto out_clear_bit;

return 0;

out_free_unlock:
spin_unlock_irqrestore(&gpio_lock, flags);
kfree_const(label);
out_clear_bit:
clear_bit(FLAG_REQUESTED, &desc->flags);
return ret;
}

Expand Down Expand Up @@ -2352,35 +2323,32 @@ static bool gpiod_free_commit(struct gpio_desc *desc)

might_sleep();

spin_lock_irqsave(&gpio_lock, flags);

gc = desc->gdev->chip;
if (gc && test_bit(FLAG_REQUESTED, &desc->flags)) {
if (gc->free) {
spin_unlock_irqrestore(&gpio_lock, flags);
might_sleep_if(gc->can_sleep);
flags = READ_ONCE(desc->flags);

if (gc && test_bit(FLAG_REQUESTED, &flags)) {
if (gc->free)
gc->free(gc, gpio_chip_hwgpio(desc));
spin_lock_irqsave(&gpio_lock, flags);
}
clear_bit(FLAG_ACTIVE_LOW, &desc->flags);
clear_bit(FLAG_REQUESTED, &desc->flags);
clear_bit(FLAG_OPEN_DRAIN, &desc->flags);
clear_bit(FLAG_OPEN_SOURCE, &desc->flags);
clear_bit(FLAG_PULL_UP, &desc->flags);
clear_bit(FLAG_PULL_DOWN, &desc->flags);
clear_bit(FLAG_BIAS_DISABLE, &desc->flags);
clear_bit(FLAG_EDGE_RISING, &desc->flags);
clear_bit(FLAG_EDGE_FALLING, &desc->flags);
clear_bit(FLAG_IS_HOGGED, &desc->flags);

clear_bit(FLAG_ACTIVE_LOW, &flags);
clear_bit(FLAG_REQUESTED, &flags);
clear_bit(FLAG_OPEN_DRAIN, &flags);
clear_bit(FLAG_OPEN_SOURCE, &flags);
clear_bit(FLAG_PULL_UP, &flags);
clear_bit(FLAG_PULL_DOWN, &flags);
clear_bit(FLAG_BIAS_DISABLE, &flags);
clear_bit(FLAG_EDGE_RISING, &flags);
clear_bit(FLAG_EDGE_FALLING, &flags);
clear_bit(FLAG_IS_HOGGED, &flags);
#ifdef CONFIG_OF_DYNAMIC
WRITE_ONCE(desc->hog, NULL);
#endif
ret = true;
}
desc_set_label(desc, NULL);
WRITE_ONCE(desc->flags, flags);

spin_unlock_irqrestore(&gpio_lock, flags);
desc_set_label(desc, NULL);
gpiod_line_state_notify(desc, GPIOLINE_CHANGED_RELEASED);
gpiod_line_state_notify(desc, GPIOLINE_CHANGED_RELEASED);
}

return ret;
}
Expand Down Expand Up @@ -2422,22 +2390,12 @@ char *gpiochip_dup_line_label(struct gpio_chip *gc, unsigned int offset)
if (IS_ERR(desc))
return NULL;

guard(spinlock_irqsave)(&gpio_lock);

if (!test_bit(FLAG_REQUESTED, &desc->flags))
return NULL;

guard(srcu)(&desc->srcu);

/*
* FIXME: Once we mark gpiod_direction_input/output() and
* gpiod_get_direction() with might_sleep(), we'll be able to protect
* the GPIO descriptors with mutex (while value setting operations will
* become lockless).
*
* Until this happens, this allocation needs to be atomic.
*/
label = kstrdup(gpiod_get_label(desc), GFP_ATOMIC);
label = kstrdup(gpiod_get_label(desc), GFP_KERNEL);
if (!label)
return ERR_PTR(-ENOMEM);

Expand Down
2 changes: 0 additions & 2 deletions drivers/gpio/gpiolib.h
Original file line number Diff line number Diff line change
Expand Up @@ -135,8 +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;

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

/**
Expand Down

0 comments on commit 35b5453

Please sign in to comment.