1. Patchew
  2. linux
  3. [PATCH 0/5] gpio: sysfs: send character device notifications for sysfs class events
  4. View patch

[PATCH 1/5] gpio: sysfs: use cleanup guards for gpiod_data::mutex

Bartosz Golaszewski posted 5 patches 1 month ago
There is a newer version of this series
[PATCH 1/5] gpio: sysfs: use cleanup guards for gpiod_data::mutex
Posted by Bartosz Golaszewski 1 month ago 
From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Shrink the code and drop some goto labels by using lock guards around
gpiod_data::mutex.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/gpio/gpiolib-sysfs.c | 57 +++++++++++++-------------------------------
 1 file changed, 17 insertions(+), 40 deletions(-)

diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index 0c713baa7784..3ccb41a93ea7 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -77,12 +77,10 @@ static ssize_t direction_show(struct device *dev,
 	struct gpio_desc *desc = data->desc;
 	int value;
 
-	mutex_lock(&data->mutex);
-
-	gpiod_get_direction(desc);
-	value = !!test_bit(FLAG_IS_OUT, &desc->flags);
-
-	mutex_unlock(&data->mutex);
+	scoped_guard(mutex, &data->mutex) {
+		gpiod_get_direction(desc);
+		value = !!test_bit(FLAG_IS_OUT, &desc->flags);
+	}
 
 	return sysfs_emit(buf, "%s\n", value ? "out" : "in");
 }
@@ -94,7 +92,7 @@ static ssize_t direction_store(struct device *dev,
 	struct gpio_desc *desc = data->desc;
 	ssize_t			status;
 
-	mutex_lock(&data->mutex);
+	guard(mutex)(&data->mutex);
 
 	if (sysfs_streq(buf, "high"))
 		status = gpiod_direction_output_raw(desc, 1);
@@ -105,8 +103,6 @@ static ssize_t direction_store(struct device *dev,
 	else
 		status = -EINVAL;
 
-	mutex_unlock(&data->mutex);
-
 	return status ? : size;
 }
 static DEVICE_ATTR_RW(direction);
@@ -118,11 +114,8 @@ static ssize_t value_show(struct device *dev,
 	struct gpio_desc *desc = data->desc;
 	ssize_t			status;
 
-	mutex_lock(&data->mutex);
-
-	status = gpiod_get_value_cansleep(desc);
-
-	mutex_unlock(&data->mutex);
+	scoped_guard(mutex, &data->mutex)
+		status = gpiod_get_value_cansleep(desc);
 
 	if (status < 0)
 		return status;
@@ -140,7 +133,7 @@ static ssize_t value_store(struct device *dev,
 
 	status = kstrtol(buf, 0, &value);
 
-	mutex_lock(&data->mutex);
+	guard(mutex)(&data->mutex);
 
 	if (!test_bit(FLAG_IS_OUT, &desc->flags)) {
 		status = -EPERM;
@@ -149,8 +142,6 @@ static ssize_t value_store(struct device *dev,
 		status = size;
 	}
 
-	mutex_unlock(&data->mutex);
-
 	return status;
 }
 static DEVICE_ATTR_PREALLOC(value, S_IWUSR | S_IRUGO, value_show, value_store);
@@ -253,11 +244,8 @@ static ssize_t edge_show(struct device *dev,
 	struct gpiod_data *data = dev_get_drvdata(dev);
 	int flags;
 
-	mutex_lock(&data->mutex);
-
-	flags = data->irq_flags;
-
-	mutex_unlock(&data->mutex);
+	scoped_guard(mutex, &data->mutex)
+		flags = data->irq_flags;
 
 	if (flags >= ARRAY_SIZE(trigger_names))
 		return 0;
@@ -276,12 +264,10 @@ static ssize_t edge_store(struct device *dev,
 	if (flags < 0)
 		return flags;
 
-	mutex_lock(&data->mutex);
+	guard(mutex)(&data->mutex);
 
-	if (flags == data->irq_flags) {
-		status = size;
-		goto out_unlock;
-	}
+	if (flags == data->irq_flags)
+		return size;
 
 	if (data->irq_flags)
 		gpio_sysfs_free_irq(dev);
@@ -292,9 +278,6 @@ static ssize_t edge_store(struct device *dev,
 			status = size;
 	}
 
-out_unlock:
-	mutex_unlock(&data->mutex);
-
 	return status;
 }
 static DEVICE_ATTR_RW(edge);
@@ -330,11 +313,8 @@ static ssize_t active_low_show(struct device *dev,
 	struct gpio_desc *desc = data->desc;
 	int value;
 
-	mutex_lock(&data->mutex);
-
-	value = !!test_bit(FLAG_ACTIVE_LOW, &desc->flags);
-
-	mutex_unlock(&data->mutex);
+	scoped_guard(mutex, &data->mutex)
+		value = !!test_bit(FLAG_ACTIVE_LOW, &desc->flags);
 
 	return sysfs_emit(buf, "%d\n", value);
 }
@@ -350,11 +330,8 @@ static ssize_t active_low_store(struct device *dev,
 	if (status)
 		return status;
 
-	mutex_lock(&data->mutex);
-
-	status = gpio_sysfs_set_active_low(dev, value);
-
-	mutex_unlock(&data->mutex);
+	scoped_guard(mutex, &data->mutex)
+		status = gpio_sysfs_set_active_low(dev, value);
 
 	return status ? : size;
 }

-- 
2.45.2
Re: [PATCH 1/5] gpio: sysfs: use cleanup guards for gpiod_data::mutex
Posted by Kent Gibson 1 month ago 
On Thu, Oct 24, 2024 at 01:32:44PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> Shrink the code and drop some goto labels by using lock guards around
> gpiod_data::mutex.
>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---
>  drivers/gpio/gpiolib-sysfs.c | 57 +++++++++++++-------------------------------
>  1 file changed, 17 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
> index 0c713baa7784..3ccb41a93ea7 100644
> --- a/drivers/gpio/gpiolib-sysfs.c
> +++ b/drivers/gpio/gpiolib-sysfs.c
> @@ -77,12 +77,10 @@ static ssize_t direction_show(struct device *dev,
>  	struct gpio_desc *desc = data->desc;
>  	int value;
>
> -	mutex_lock(&data->mutex);
> -
> -	gpiod_get_direction(desc);
> -	value = !!test_bit(FLAG_IS_OUT, &desc->flags);
> -
> -	mutex_unlock(&data->mutex);
> +	scoped_guard(mutex, &data->mutex) {
> +		gpiod_get_direction(desc);
> +		value = !!test_bit(FLAG_IS_OUT, &desc->flags);
> +	}
>
>  	return sysfs_emit(buf, "%s\n", value ? "out" : "in");
>  }
> @@ -94,7 +92,7 @@ static ssize_t direction_store(struct device *dev,
>  	struct gpio_desc *desc = data->desc;
>  	ssize_t			status;
>
> -	mutex_lock(&data->mutex);
> +	guard(mutex)(&data->mutex);
>
>  	if (sysfs_streq(buf, "high"))
>  		status = gpiod_direction_output_raw(desc, 1);
> @@ -105,8 +103,6 @@ static ssize_t direction_store(struct device *dev,
>  	else
>  		status = -EINVAL;
>
> -	mutex_unlock(&data->mutex);
> -
>  	return status ? : size;
>  }
>  static DEVICE_ATTR_RW(direction);
> @@ -118,11 +114,8 @@ static ssize_t value_show(struct device *dev,
>  	struct gpio_desc *desc = data->desc;
>  	ssize_t			status;
>
> -	mutex_lock(&data->mutex);
> -
> -	status = gpiod_get_value_cansleep(desc);
> -
> -	mutex_unlock(&data->mutex);
> +	scoped_guard(mutex, &data->mutex)
> +		status = gpiod_get_value_cansleep(desc);
>
>  	if (status < 0)
>  		return status;
> @@ -140,7 +133,7 @@ static ssize_t value_store(struct device *dev,
>
>  	status = kstrtol(buf, 0, &value);
>
> -	mutex_lock(&data->mutex);
> +	guard(mutex)(&data->mutex);
>
>  	if (!test_bit(FLAG_IS_OUT, &desc->flags)) {
>  		status = -EPERM;
> @@ -149,8 +142,6 @@ static ssize_t value_store(struct device *dev,
>  		status = size;
>  	}
>
> -	mutex_unlock(&data->mutex);
> -
>  	return status;
>  }


With the guard, this can be further simplified by returning immediately
and collapsing the if-else chain:

@@ -135,14 +135,14 @@ static ssize_t value_store(struct device *dev,

        guard(mutex)(&data->mutex);

-       if (!test_bit(FLAG_IS_OUT, &desc->flags)) {
-               status = -EPERM;
-       } else if (status == 0) {
-               gpiod_set_value_cansleep(desc, value);
-               status = size;
-       }
+       if (!test_bit(FLAG_IS_OUT, &desc->flags))
+               return -EPERM;

-       return status;
+       if (status)
+               return status;
+
+       gpiod_set_value_cansleep(desc, value);
+       return size;
 }

That also removes the overloading of status, previously containing a status
OR a size, which is no longer necessary.

>  static DEVICE_ATTR_PREALLOC(value, S_IWUSR | S_IRUGO, value_show, value_store);
> @@ -253,11 +244,8 @@ static ssize_t edge_show(struct device *dev,
>  	struct gpiod_data *data = dev_get_drvdata(dev);
>  	int flags;
>
> -	mutex_lock(&data->mutex);
> -
> -	flags = data->irq_flags;
> -
> -	mutex_unlock(&data->mutex);
> +	scoped_guard(mutex, &data->mutex)
> +		flags = data->irq_flags;
>
>  	if (flags >= ARRAY_SIZE(trigger_names))
>  		return 0;
> @@ -276,12 +264,10 @@ static ssize_t edge_store(struct device *dev,
>  	if (flags < 0)
>  		return flags;
>
> -	mutex_lock(&data->mutex);
> +	guard(mutex)(&data->mutex);
>
> -	if (flags == data->irq_flags) {
> -		status = size;
> -		goto out_unlock;
> -	}
> +	if (flags == data->irq_flags)
> +		return size;
>
>  	if (data->irq_flags)
>  		gpio_sysfs_free_irq(dev);
> @@ -292,9 +278,6 @@ static ssize_t edge_store(struct device *dev,
>  			status = size;
>  	}
>
> -out_unlock:
> -	mutex_unlock(&data->mutex);
> -
>  	return status;
>  }


Similarly drop the overloading of status:

@@ -257,7 +257,7 @@ 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);
-       ssize_t status = size;
+       ssize_t status;
        int flags;

        flags = sysfs_match_string(trigger_names, buf);
@@ -274,11 +274,11 @@ static ssize_t edge_store(struct device *dev,

        if (flags) {
                status = gpio_sysfs_request_irq(dev, flags);
-               if (!status)
-                       status = size;
+               if (status)
+                       return status;
        }

-       return status;
+       return size;
 }

>  static DEVICE_ATTR_RW(edge);
> @@ -330,11 +313,8 @@ static ssize_t active_low_show(struct device *dev,
>  	struct gpio_desc *desc = data->desc;
>  	int value;
>
> -	mutex_lock(&data->mutex);
> -
> -	value = !!test_bit(FLAG_ACTIVE_LOW, &desc->flags);
> -
> -	mutex_unlock(&data->mutex);
> +	scoped_guard(mutex, &data->mutex)
> +		value = !!test_bit(FLAG_ACTIVE_LOW, &desc->flags);
>
>  	return sysfs_emit(buf, "%d\n", value);
>  }
> @@ -350,11 +330,8 @@ static ssize_t active_low_store(struct device *dev,
>  	if (status)
>  		return status;
>
> -	mutex_lock(&data->mutex);
> -
> -	status = gpio_sysfs_set_active_low(dev, value);
> -
> -	mutex_unlock(&data->mutex);
> +	scoped_guard(mutex, &data->mutex)
> +		status = gpio_sysfs_set_active_low(dev, value);
>
>  	return status ? : size;
>  }
>

Doesn't need to be scoped:

-       scoped_guard(mutex, &data->mutex)
-               status = gpio_sysfs_set_active_low(dev, value);
+       guard(mutex, &data->mutex);
+
+       status = gpio_sysfs_set_active_low(dev, value);

        return status ? : size;
 }


No issues with the other patches.

Cheers,
Kent.
Re: [PATCH 1/5] gpio: sysfs: use cleanup guards for gpiod_data::mutex
Posted by Bartosz Golaszewski 1 month ago 
On Fri, Oct 25, 2024 at 4:54 AM Kent Gibson <warthog618@gmail.com> wrote:
>
> > @@ -140,7 +133,7 @@ static ssize_t value_store(struct device *dev,
> >
> >       status = kstrtol(buf, 0, &value);
> >
> > -     mutex_lock(&data->mutex);
> > +     guard(mutex)(&data->mutex);
> >
> >       if (!test_bit(FLAG_IS_OUT, &desc->flags)) {
> >               status = -EPERM;
> > @@ -149,8 +142,6 @@ static ssize_t value_store(struct device *dev,
> >               status = size;
> >       }
> >
> > -     mutex_unlock(&data->mutex);
> > -
> >       return status;
> >  }
>
>
> With the guard, this can be further simplified by returning immediately
> and collapsing the if-else chain:
>

Ah, right, it's the whole goal of this change after all.

Bart

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.