[PATCH v1 1/3] gpiolib: sysfs: Move sysfs_emit() calls outside of the mutex lock

[PATCH v1 1/3] gpiolib: sysfs: Move sysfs_emit() calls outside of the mutex lock

[Andy Shevchenko]

In a few places we perform sysfs_emit() operations under mutex that
do not require any locking. Move them outside of the mutex locks.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpiolib-sysfs.c | 29 ++++++++++++++---------------
 1 file changed, 14 insertions(+), 15 deletions(-)

diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index 78ca7dee8b64..57d8ecab53b7 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -61,17 +61,16 @@ static ssize_t direction_show(struct device *dev,
 {
 	struct gpiod_data *data = dev_get_drvdata(dev);
 	struct gpio_desc *desc = data->desc;
-	ssize_t			status;
+	int value;
 
 	mutex_lock(&data->mutex);
 
 	gpiod_get_direction(desc);
-	status = sysfs_emit(buf, "%s\n",
-			    test_bit(FLAG_IS_OUT, &desc->flags) ? "out" : "in");
+	value = !!test_bit(FLAG_IS_OUT, &desc->flags);
 
 	mutex_unlock(&data->mutex);
 
-	return status;
+	return sysfs_emit(buf, "%s\n", value ? "out" : "in");
 }
 
 static ssize_t direction_store(struct device *dev,
@@ -108,12 +107,13 @@ static ssize_t value_show(struct device *dev,
 	mutex_lock(&data->mutex);
 
 	status = gpiod_get_value_cansleep(desc);
-	if (status >= 0)
-		status = sysfs_emit(buf, "%zd\n", status);
 
 	mutex_unlock(&data->mutex);
 
-	return status;
+	if (status < 0)
+		return status;
+
+	return sysfs_emit(buf, "%zd\n", status);
 }
 
 static ssize_t value_store(struct device *dev,
@@ -238,7 +238,6 @@ static ssize_t edge_show(struct device *dev,
 		struct device_attribute *attr, char *buf)
 {
 	struct gpiod_data *data = dev_get_drvdata(dev);
-	ssize_t	status = 0;
 	int i;
 
 	mutex_lock(&data->mutex);
@@ -247,12 +246,13 @@ static ssize_t edge_show(struct device *dev,
 		if (data->irq_flags == trigger_types[i].flags)
 			break;
 	}
-	if (i < ARRAY_SIZE(trigger_types))
-		status = sysfs_emit(buf, "%s\n", trigger_types[i].name);
 
 	mutex_unlock(&data->mutex);
 
-	return status;
+	if (i >= ARRAY_SIZE(trigger_types))
+		return 0;
+
+	return sysfs_emit(buf, "%s\n", trigger_types[i].name);
 }
 
 static ssize_t edge_store(struct device *dev,
@@ -324,16 +324,15 @@ static ssize_t active_low_show(struct device *dev,
 {
 	struct gpiod_data *data = dev_get_drvdata(dev);
 	struct gpio_desc *desc = data->desc;
-	ssize_t			status;
+	int value;
 
 	mutex_lock(&data->mutex);
 
-	status = sysfs_emit(buf, "%d\n",
-			    !!test_bit(FLAG_ACTIVE_LOW, &desc->flags));
+	value = !!test_bit(FLAG_ACTIVE_LOW, &desc->flags);
 
 	mutex_unlock(&data->mutex);
 
-	return status;
+	return sysfs_emit(buf, "%d\n", value);
 }
 
 static ssize_t active_low_store(struct device *dev,
-- 
2.34.1

[PATCH v1 2/3] gpiolib: sysfs: Move kstrtox() calls outside of the mutex lock

[Andy Shevchenko]

In a few places we perform kstrtox() operations under mutex that
do not require any locking. Move them outside of the mutex locks.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpiolib-sysfs.c | 28 ++++++++++++----------------
 1 file changed, 12 insertions(+), 16 deletions(-)

diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index 57d8ecab53b7..e7828d4dc7a8 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -121,24 +121,18 @@ static ssize_t value_store(struct device *dev,
 {
 	struct gpiod_data *data = dev_get_drvdata(dev);
 	struct gpio_desc *desc = data->desc;
-	ssize_t status = 0;
+	ssize_t status;
+	long value;
+
+	status = kstrtol(buf, 0, &value);
 
 	mutex_lock(&data->mutex);
 
 	if (!test_bit(FLAG_IS_OUT, &desc->flags)) {
 		status = -EPERM;
-	} else {
-		long		value;
-
-		if (size <= 2 && isdigit(buf[0]) &&
-		    (size == 1 || buf[1] == '\n'))
-			value = buf[0] - '0';
-		else
-			status = kstrtol(buf, 0, &value);
-		if (status == 0) {
-			gpiod_set_value_cansleep(desc, value);
-			status = size;
-		}
+	} else if (status == 0) {
+		gpiod_set_value_cansleep(desc, value);
+		status = size;
 	}
 
 	mutex_unlock(&data->mutex);
@@ -342,11 +336,13 @@ static ssize_t active_low_store(struct device *dev,
 	ssize_t			status;
 	long			value;
 
+	status = kstrtol(buf, 0, &value);
+	if (status)
+		return status;
+
 	mutex_lock(&data->mutex);
 
-	status = kstrtol(buf, 0, &value);
-	if (status == 0)
-		status = gpio_sysfs_set_active_low(dev, value);
+	status = gpio_sysfs_set_active_low(dev, value);
 
 	mutex_unlock(&data->mutex);
 
-- 
2.34.1

[PATCH v1 3/3] gpiolib: sysfs: Simplify edge handling in the code

[Andy Shevchenko]

Instead of keeping specific data structure for IRQ trigger types, switch
to array of trigger names and use index as a type.

The code is in maintenance mode and that array is not going to grow.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpiolib-sysfs.c | 40 +++++++++++++-----------------------
 1 file changed, 14 insertions(+), 26 deletions(-)

diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index e7828d4dc7a8..d44ffea038f5 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -13,6 +13,7 @@
 #include "gpiolib.h"
 #include "gpiolib-sysfs.h"
 
+#define GPIO_IRQF_TRIGGER_NONE		0
 #define GPIO_IRQF_TRIGGER_FALLING	BIT(0)
 #define GPIO_IRQF_TRIGGER_RISING	BIT(1)
 #define GPIO_IRQF_TRIGGER_BOTH		(GPIO_IRQF_TRIGGER_FALLING | \
@@ -218,54 +219,41 @@ static void gpio_sysfs_free_irq(struct device *dev)
 	sysfs_put(data->value_kn);
 }
 
-static const struct {
-	const char *name;
-	unsigned char flags;
-} trigger_types[] = {
-	{ "none",    0 },
-	{ "falling", GPIO_IRQF_TRIGGER_FALLING },
-	{ "rising",  GPIO_IRQF_TRIGGER_RISING },
-	{ "both",    GPIO_IRQF_TRIGGER_BOTH },
+static const char * const trigger_names[] = {
+	[GPIO_IRQF_TRIGGER_NONE]	= "none",
+	[GPIO_IRQF_TRIGGER_FALLING]	= "falling",
+	[GPIO_IRQF_TRIGGER_RISING]	= "rising",
+	[GPIO_IRQF_TRIGGER_BOTH]	= "both",
 };
 
 static ssize_t edge_show(struct device *dev,
 		struct device_attribute *attr, char *buf)
 {
 	struct gpiod_data *data = dev_get_drvdata(dev);
-	int i;
+	int flags;
 
 	mutex_lock(&data->mutex);
 
-	for (i = 0; i < ARRAY_SIZE(trigger_types); i++) {
-		if (data->irq_flags == trigger_types[i].flags)
-			break;
-	}
+	flags = data->irq_flags;
 
 	mutex_unlock(&data->mutex);
 
-	if (i >= ARRAY_SIZE(trigger_types))
+	if (flags >= ARRAY_SIZE(trigger_names))
 		return 0;
 
-	return sysfs_emit(buf, "%s\n", trigger_types[i].name);
+	return sysfs_emit(buf, "%s\n", trigger_names[flags]);
 }
 
 static ssize_t edge_store(struct device *dev,
 		struct device_attribute *attr, const char *buf, size_t size)
 {
 	struct gpiod_data *data = dev_get_drvdata(dev);
-	unsigned char flags;
 	ssize_t	status = size;
-	int i;
-
-	for (i = 0; i < ARRAY_SIZE(trigger_types); i++) {
-		if (sysfs_streq(trigger_types[i].name, buf))
-			break;
-	}
-
-	if (i == ARRAY_SIZE(trigger_types))
-		return -EINVAL;
+	int flags;
 
-	flags = trigger_types[i].flags;
+	flags = sysfs_match_string(trigger_names, buf);
+	if (flags < 0)
+		return flags;
 
 	mutex_lock(&data->mutex);
 
-- 
2.34.1

Re: [PATCH v1 1/3] gpiolib: sysfs: Move sysfs_emit() calls outside of the mutex lock

[Bartosz Golaszewski]

On Wed, Feb 9, 2022 at 12:31 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> In a few places we perform sysfs_emit() operations under mutex that
> do not require any locking. Move them outside of the mutex locks.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/gpio/gpiolib-sysfs.c | 29 ++++++++++++++---------------
>  1 file changed, 14 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
> index 78ca7dee8b64..57d8ecab53b7 100644
> --- a/drivers/gpio/gpiolib-sysfs.c
> +++ b/drivers/gpio/gpiolib-sysfs.c
> @@ -61,17 +61,16 @@ static ssize_t direction_show(struct device *dev,
>  {
>         struct gpiod_data *data = dev_get_drvdata(dev);
>         struct gpio_desc *desc = data->desc;
> -       ssize_t                 status;
> +       int value;
>
>         mutex_lock(&data->mutex);
>
>         gpiod_get_direction(desc);
> -       status = sysfs_emit(buf, "%s\n",
> -                           test_bit(FLAG_IS_OUT, &desc->flags) ? "out" : "in");
> +       value = !!test_bit(FLAG_IS_OUT, &desc->flags);
>
>         mutex_unlock(&data->mutex);
>
> -       return status;
> +       return sysfs_emit(buf, "%s\n", value ? "out" : "in");
>  }
>
>  static ssize_t direction_store(struct device *dev,
> @@ -108,12 +107,13 @@ static ssize_t value_show(struct device *dev,
>         mutex_lock(&data->mutex);
>
>         status = gpiod_get_value_cansleep(desc);
> -       if (status >= 0)
> -               status = sysfs_emit(buf, "%zd\n", status);
>
>         mutex_unlock(&data->mutex);
>
> -       return status;
> +       if (status < 0)
> +               return status;
> +
> +       return sysfs_emit(buf, "%zd\n", status);
>  }
>
>  static ssize_t value_store(struct device *dev,
> @@ -238,7 +238,6 @@ static ssize_t edge_show(struct device *dev,
>                 struct device_attribute *attr, char *buf)
>  {
>         struct gpiod_data *data = dev_get_drvdata(dev);
> -       ssize_t status = 0;
>         int i;
>
>         mutex_lock(&data->mutex);
> @@ -247,12 +246,13 @@ static ssize_t edge_show(struct device *dev,
>                 if (data->irq_flags == trigger_types[i].flags)
>                         break;
>         }
> -       if (i < ARRAY_SIZE(trigger_types))
> -               status = sysfs_emit(buf, "%s\n", trigger_types[i].name);
>
>         mutex_unlock(&data->mutex);
>
> -       return status;
> +       if (i >= ARRAY_SIZE(trigger_types))
> +               return 0;
> +
> +       return sysfs_emit(buf, "%s\n", trigger_types[i].name);
>  }
>
>  static ssize_t edge_store(struct device *dev,
> @@ -324,16 +324,15 @@ static ssize_t active_low_show(struct device *dev,
>  {
>         struct gpiod_data *data = dev_get_drvdata(dev);
>         struct gpio_desc *desc = data->desc;
> -       ssize_t                 status;
> +       int value;
>
>         mutex_lock(&data->mutex);
>
> -       status = sysfs_emit(buf, "%d\n",
> -                           !!test_bit(FLAG_ACTIVE_LOW, &desc->flags));
> +       value = !!test_bit(FLAG_ACTIVE_LOW, &desc->flags);
>
>         mutex_unlock(&data->mutex);
>
> -       return status;
> +       return sysfs_emit(buf, "%d\n", value);
>  }
>
>  static ssize_t active_low_store(struct device *dev,
> --
> 2.34.1
>

Applied all three, thanks Andy!

Bart