| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20241025025358.GA47379@rigel>
Date: Fri, 25 Oct 2024 10:53:58 +0800
From: Kent Gibson <warthog618@...il.com>
To: Bartosz Golaszewski <brgl@...ev.pl>
Cc: Linus Walleij <linus.walleij@...aro.org>, linux-gpio@...r.kernel.org,
linux-kernel@...r.kernel.org,
Bartosz Golaszewski <bartosz.golaszewski@...aro.org>
Subject: Re: [PATCH 1/5] gpio: sysfs: use cleanup guards for gpiod_data::mutex
On Thu, Oct 24, 2024 at 01:32:44PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@...aro.org>
>
> Shrink the code and drop some goto labels by using lock guards around
> gpiod_data::mutex.
>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@...aro.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.
Powered by blists - more mailing lists