[PATCH 0/5] gpio: sysfs: send character device notifications for sysfs class events

[PATCH 0/5] gpio: sysfs: send character device notifications for sysfs class events

[Bartosz Golaszewski]

This may be a total corner-case but for consistency and completeness I
think it makes sense to also send out line state change events on actions
triggered from the GPIO sysfs class.

The first two patches use cleanup helpers in sysfs code. The next three
change the code to emit notifications on line export (unexport is
already handled) and active_low & edge changes.

One last thing I considered was also notifying user-space whenever
gpiochip_un/lock_as_irq() is called but that doesn't make much sense as
it's largely independent from the GPIO core and can be called for both
requested and available lines whenever someone requests an interrupt
from a GPIO controller.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
Bartosz Golaszewski (5):
      gpio: sysfs: use cleanup guards for gpiod_data::mutex
      gpio: sysfs: use cleanup guards for the sysfs_lock mutex
      gpio: sysfs: emit chardev line-state events on GPIO export
      gpio: sysfs: emit chardev line-state events on active-low changes
      gpio: sysfs: emit chardev line-state events on edge store

 drivers/gpio/gpiolib-sysfs.c | 143 +++++++++++++++++++------------------------
 1 file changed, 62 insertions(+), 81 deletions(-)
---
base-commit: fd21fa4a912ebbf8a6a341c31d8456f61e7d4170
change-id: 20241022-gpio-notify-sysfs-3bddf9ecca9f

Best regards,
-- 
Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

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

[Bartosz Golaszewski]

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

[PATCH 2/5] gpio: sysfs: use cleanup guards for the sysfs_lock mutex

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Shrink the code and remove some goto labels by using guards around the
sysfs_lock mutex. While at it: use __free(kfree) when allocating sysfs
callback data.

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

diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index 3ccb41a93ea7..096f79bbfe42 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -550,7 +550,6 @@ static const struct class gpio_class = {
 int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
 {
 	struct gpio_device *gdev;
-	struct gpiod_data *data;
 	struct device *dev;
 	int status;
 
@@ -574,24 +573,25 @@ int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
 
 	gdev = desc->gdev;
 
-	mutex_lock(&sysfs_lock);
+	guard(mutex)(&sysfs_lock);
 
 	/* check if chip is being removed */
 	if (!gdev->mockdev) {
 		status = -ENODEV;
-		goto err_unlock;
+		goto err_clear_bit;
 	}
 
 	if (!test_bit(FLAG_REQUESTED, &desc->flags)) {
 		gpiod_dbg(desc, "%s: unavailable (not requested)\n", __func__);
 		status = -EPERM;
-		goto err_unlock;
+		goto err_clear_bit;
 	}
 
-	data = kzalloc(sizeof(*data), GFP_KERNEL);
+	struct gpiod_data *data __free(kfree) = kzalloc(sizeof(*data),
+							GFP_KERNEL);
 	if (!data) {
 		status = -ENOMEM;
-		goto err_unlock;
+		goto err_clear_bit;
 	}
 
 	data->desc = desc;
@@ -606,16 +606,13 @@ int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
 					"gpio%u", desc_to_gpio(desc));
 	if (IS_ERR(dev)) {
 		status = PTR_ERR(dev);
-		goto err_free_data;
+		goto err_clear_bit;
 	}
 
-	mutex_unlock(&sysfs_lock);
+	data = NULL;
 	return 0;
 
-err_free_data:
-	kfree(data);
-err_unlock:
-	mutex_unlock(&sysfs_lock);
+err_clear_bit:
 	clear_bit(FLAG_EXPORT, &desc->flags);
 	gpiod_dbg(desc, "%s: status %d\n", __func__, status);
 	return status;
@@ -679,36 +676,28 @@ void gpiod_unexport(struct gpio_desc *desc)
 		return;
 	}
 
-	mutex_lock(&sysfs_lock);
+	scoped_guard(mutex, &sysfs_lock) {
+		if (!test_bit(FLAG_EXPORT, &desc->flags))
+			return;
 
-	if (!test_bit(FLAG_EXPORT, &desc->flags))
-		goto err_unlock;
+		dev = class_find_device(&gpio_class, NULL, desc, match_export);
+		if (!dev)
+			return;
 
-	dev = class_find_device(&gpio_class, NULL, desc, match_export);
-	if (!dev)
-		goto err_unlock;
+		data = dev_get_drvdata(dev);
+		clear_bit(FLAG_EXPORT, &desc->flags);
+		device_unregister(dev);
 
-	data = dev_get_drvdata(dev);
-
-	clear_bit(FLAG_EXPORT, &desc->flags);
-
-	device_unregister(dev);
-
-	/*
-	 * Release irq after deregistration to prevent race with edge_store.
-	 */
-	if (data->irq_flags)
-		gpio_sysfs_free_irq(dev);
-
-	mutex_unlock(&sysfs_lock);
+		/*
+		 * Release irq after deregistration to prevent race with
+		 * edge_store.
+		 */
+		if (data->irq_flags)
+			gpio_sysfs_free_irq(dev);
+	}
 
 	put_device(dev);
 	kfree(data);
-
-	return;
-
-err_unlock:
-	mutex_unlock(&sysfs_lock);
 }
 EXPORT_SYMBOL_GPL(gpiod_unexport);
 
@@ -749,9 +738,8 @@ int gpiochip_sysfs_register(struct gpio_device *gdev)
 	if (IS_ERR(dev))
 		return PTR_ERR(dev);
 
-	mutex_lock(&sysfs_lock);
+	guard(mutex)(&sysfs_lock);
 	gdev->mockdev = dev;
-	mutex_unlock(&sysfs_lock);
 
 	return 0;
 }

-- 
2.45.2

[PATCH 3/5] gpio: sysfs: emit chardev line-state events on GPIO export

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

We already emit a CONFIG_RELEASED event when a line is unexported over
sysfs (this is handled by gpiod_free()) but we don't do the opposite
when it's exported. This adds the missing call to
gpiod_line_state_notify().

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

diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index 096f79bbfe42..3f24bf278b77 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -21,6 +21,8 @@
 #include <linux/gpio/consumer.h>
 #include <linux/gpio/driver.h>
 
+#include <uapi/linux/gpio.h>
+
 #include "gpiolib.h"
 #include "gpiolib-sysfs.h"
 
@@ -470,10 +472,12 @@ static ssize_t export_store(const struct class *class,
 	}
 
 	status = gpiod_export(desc, true);
-	if (status < 0)
+	if (status < 0) {
 		gpiod_free(desc);
-	else
+	} else {
 		set_bit(FLAG_SYSFS, &desc->flags);
+		gpiod_line_state_notify(desc, GPIO_V2_LINE_CHANGED_REQUESTED);
+	}
 
 done:
 	if (status)

-- 
2.45.2

[PATCH 4/5] gpio: sysfs: emit chardev line-state events on active-low changes

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

The sysfs active_low attribute doesn't go through the usual paths so it
doesn't emit the line-state event. Add the missing call to
gpiod_line_state_notify() to gpio_sysfs_set_active_low().

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

diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index 3f24bf278b77..b967b76ea046 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -305,6 +305,8 @@ static int gpio_sysfs_set_active_low(struct device *dev, int value)
 		status = gpio_sysfs_request_irq(dev, flags);
 	}
 
+	gpiod_line_state_notify(desc, GPIO_V2_LINE_CHANGED_CONFIG);
+
 	return status;
 }
 

-- 
2.45.2

[PATCH 5/5] gpio: sysfs: emit chardev line-state events on edge store

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

In order to emit line-state events on edge changes in sysfs, update the
EDGE flags in the descriptor in gpio_sysfs_request_irq() and emit the
event on a successful store.

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

diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index b967b76ea046..87f27a0288f9 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -178,12 +178,16 @@ static int gpio_sysfs_request_irq(struct device *dev, unsigned char flags)
 		return -ENODEV;
 
 	irq_flags = IRQF_SHARED;
-	if (flags & GPIO_IRQF_TRIGGER_FALLING)
+	if (flags & GPIO_IRQF_TRIGGER_FALLING) {
 		irq_flags |= test_bit(FLAG_ACTIVE_LOW, &desc->flags) ?
 			IRQF_TRIGGER_RISING : IRQF_TRIGGER_FALLING;
-	if (flags & GPIO_IRQF_TRIGGER_RISING)
+		set_bit(FLAG_EDGE_FALLING, &desc->flags);
+	}
+	if (flags & GPIO_IRQF_TRIGGER_RISING) {
 		irq_flags |= test_bit(FLAG_ACTIVE_LOW, &desc->flags) ?
 			IRQF_TRIGGER_FALLING : IRQF_TRIGGER_RISING;
+		set_bit(FLAG_EDGE_RISING, &desc->flags);
+	}
 
 	/*
 	 * FIXME: This should be done in the irq_request_resources callback
@@ -209,6 +213,8 @@ static int gpio_sysfs_request_irq(struct device *dev, unsigned char flags)
 err_unlock:
 	gpiochip_unlock_as_irq(guard.gc, gpio_chip_hwgpio(desc));
 err_put_kn:
+	clear_bit(FLAG_EDGE_RISING, &desc->flags);
+	clear_bit(FLAG_EDGE_FALLING, &desc->flags);
 	sysfs_put(data->value_kn);
 
 	return ret;
@@ -230,6 +236,8 @@ static void gpio_sysfs_free_irq(struct device *dev)
 	data->irq_flags = 0;
 	free_irq(data->irq, data);
 	gpiochip_unlock_as_irq(guard.gc, gpio_chip_hwgpio(desc));
+	clear_bit(FLAG_EDGE_RISING, &desc->flags);
+	clear_bit(FLAG_EDGE_FALLING, &desc->flags);
 	sysfs_put(data->value_kn);
 }
 
@@ -280,6 +288,8 @@ static ssize_t edge_store(struct device *dev,
 			status = size;
 	}
 
+	gpiod_line_state_notify(data->desc, GPIO_V2_LINE_CHANGED_CONFIG);
+
 	return status;
 }
 static DEVICE_ATTR_RW(edge);

-- 
2.45.2

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

[Kent Gibson]

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

[Bartosz Golaszewski]

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