gpio: sysfs: use per-gpio locking
Add a per-gpio mutex to serialise attribute operations rather than use
one global mutex for all gpios and chips.
Having a single global lock for all gpios in a system adds unnecessary
latency to the sysfs interface, and especially when having gpio
controllers connected over slow buses.
Now that the global gpio-sysfs interrupt table is gone and with per-gpio
data in place, we can easily switch to using a more fine-grained locking
scheme.
Keep the global mutex to serialise the global (class) operations of gpio
export and unexport and chip removal.
Also document the locking assumptions made.
Note that this is also needed to fix a race between gpiod_export and
gpiod_unexport.
Signed-off-by: Johan Hovold <johan@kernel.org>
Reviewed-by: Alexandre Courbot <acourbot@nvidia.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index 682e4d3..1bb05aa 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -12,12 +12,15 @@
struct gpiod_data {
struct gpio_desc *desc;
+
+ struct mutex mutex;
struct kernfs_node *value_kn;
int irq;
};
-/* lock protects against unexport_gpio() being called while
- * sysfs files are active.
+/*
+ * Lock to serialise gpiod export and unexport, and prevent re-export of
+ * gpiod whose chip is being unregistered.
*/
static DEFINE_MUTEX(sysfs_lock);
@@ -49,14 +52,15 @@
struct gpio_desc *desc = data->desc;
ssize_t status;
- mutex_lock(&sysfs_lock);
+ mutex_lock(&data->mutex);
gpiod_get_direction(desc);
status = sprintf(buf, "%s\n",
test_bit(FLAG_IS_OUT, &desc->flags)
? "out" : "in");
- mutex_unlock(&sysfs_lock);
+ mutex_unlock(&data->mutex);
+
return status;
}
@@ -67,7 +71,7 @@
struct gpio_desc *desc = data->desc;
ssize_t status;
- mutex_lock(&sysfs_lock);
+ mutex_lock(&data->mutex);
if (sysfs_streq(buf, "high"))
status = gpiod_direction_output_raw(desc, 1);
@@ -78,7 +82,8 @@
else
status = -EINVAL;
- mutex_unlock(&sysfs_lock);
+ mutex_unlock(&data->mutex);
+
return status ? : size;
}
static DEVICE_ATTR_RW(direction);
@@ -90,11 +95,12 @@
struct gpio_desc *desc = data->desc;
ssize_t status;
- mutex_lock(&sysfs_lock);
+ mutex_lock(&data->mutex);
status = sprintf(buf, "%d\n", gpiod_get_value_cansleep(desc));
- mutex_unlock(&sysfs_lock);
+ mutex_unlock(&data->mutex);
+
return status;
}
@@ -105,7 +111,7 @@
struct gpio_desc *desc = data->desc;
ssize_t status;
- mutex_lock(&sysfs_lock);
+ mutex_lock(&data->mutex);
if (!test_bit(FLAG_IS_OUT, &desc->flags)) {
status = -EPERM;
@@ -119,7 +125,8 @@
}
}
- mutex_unlock(&sysfs_lock);
+ mutex_unlock(&data->mutex);
+
return status;
}
static DEVICE_ATTR_RW(value);
@@ -133,6 +140,7 @@
return IRQ_HANDLED;
}
+/* Caller holds gpiod-data mutex. */
static int gpio_sysfs_request_irq(struct device *dev, unsigned long gpio_flags)
{
struct gpiod_data *data = dev_get_drvdata(dev);
@@ -185,6 +193,10 @@
return ret;
}
+/*
+ * Caller holds gpiod-data mutex (unless called after class-device
+ * deregistration).
+ */
static void gpio_sysfs_free_irq(struct device *dev)
{
struct gpiod_data *data = dev_get_drvdata(dev);
@@ -215,7 +227,7 @@
ssize_t status = 0;
int i;
- mutex_lock(&sysfs_lock);
+ mutex_lock(&data->mutex);
for (i = 0; i < ARRAY_SIZE(trigger_types); i++) {
mask = desc->flags & GPIO_TRIGGER_MASK;
@@ -225,7 +237,8 @@
}
}
- mutex_unlock(&sysfs_lock);
+ mutex_unlock(&data->mutex);
+
return status;
}
@@ -248,7 +261,7 @@
flags = trigger_types[i].flags;
- mutex_lock(&sysfs_lock);
+ mutex_lock(&data->mutex);
if ((desc->flags & GPIO_TRIGGER_MASK) == flags) {
status = size;
@@ -265,12 +278,13 @@
}
out_unlock:
- mutex_unlock(&sysfs_lock);
+ mutex_unlock(&data->mutex);
return status;
}
static DEVICE_ATTR_RW(edge);
+/* Caller holds gpiod-data mutex. */
static int sysfs_set_active_low(struct device *dev, int value)
{
struct gpiod_data *data = dev_get_drvdata(dev);
@@ -304,12 +318,12 @@
struct gpio_desc *desc = data->desc;
ssize_t status;
- mutex_lock(&sysfs_lock);
+ mutex_lock(&data->mutex);
status = sprintf(buf, "%d\n",
!!test_bit(FLAG_ACTIVE_LOW, &desc->flags));
- mutex_unlock(&sysfs_lock);
+ mutex_unlock(&data->mutex);
return status;
}
@@ -317,16 +331,17 @@
static ssize_t active_low_store(struct device *dev,
struct device_attribute *attr, const char *buf, size_t size)
{
+ struct gpiod_data *data = dev_get_drvdata(dev);
ssize_t status;
long value;
- mutex_lock(&sysfs_lock);
+ mutex_lock(&data->mutex);
status = kstrtol(buf, 0, &value);
if (status == 0)
status = sysfs_set_active_low(dev, value != 0);
- mutex_unlock(&sysfs_lock);
+ mutex_unlock(&data->mutex);
return status ? : size;
}
@@ -583,6 +598,7 @@
}
data->desc = desc;
+ mutex_init(&data->mutex);
offset = gpio_chip_hwgpio(desc);
if (chip->names && chip->names[offset])