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 | 81 ++++++++++++++++----------------------------
1 file changed, 29 insertions(+), 52 deletions(-)
diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index 0c713baa7784..e11b322d8e72 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;
@@ -139,19 +132,17 @@ static ssize_t value_store(struct device *dev,
long value;
status = kstrtol(buf, 0, &value);
+ if (status)
+ return status;
- mutex_lock(&data->mutex);
+ 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;
- mutex_unlock(&data->mutex);
+ gpiod_set_value_cansleep(desc, value);
- return status;
+ return size;
}
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,26 +264,22 @@ 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);
- if (flags) {
- status = gpio_sysfs_request_irq(dev, flags);
- if (!status)
- status = size;
- }
+ if (!flags)
+ return size;
-out_unlock:
- mutex_unlock(&data->mutex);
+ status = gpio_sysfs_request_irq(dev, flags);
+ if (status)
+ return status;
- return status;
+ return size;
}
static DEVICE_ATTR_RW(edge);
@@ -330,11 +314,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,13 +331,9 @@ static ssize_t active_low_store(struct device *dev,
if (status)
return status;
- mutex_lock(&data->mutex);
+ guard(mutex)(&data->mutex);
- status = gpio_sysfs_set_active_low(dev, value);
-
- mutex_unlock(&data->mutex);
-
- return status ? : size;
+ return gpio_sysfs_set_active_low(dev, value) ?: size;
}
static DEVICE_ATTR_RW(active_low);
--
2.45.2On Fri, Oct 25, 2024 at 02:18:51PM +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 | 81 ++++++++++++++++----------------------------
> 1 file changed, 29 insertions(+), 52 deletions(-)
>
> @@ -139,19 +132,17 @@ static ssize_t value_store(struct device *dev,
> long value;
>
> status = kstrtol(buf, 0, &value);
> + if (status)
> + return status;
>
> - mutex_lock(&data->mutex);
> + 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;
>
> - mutex_unlock(&data->mutex);
> + gpiod_set_value_cansleep(desc, value);
>
> - return status;
> + return size;
> }
This is a behavioural change as you've moved the decode check before the
permission check. Not sure if that is significant or not, so in my
suggestion I retained the old order.
Cheers,
Kent.
On Fri, Oct 25, 2024 at 3:24 PM Kent Gibson <warthog618@gmail.com> wrote:
>
> On Fri, Oct 25, 2024 at 02:18:51PM +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 | 81 ++++++++++++++++----------------------------
> > 1 file changed, 29 insertions(+), 52 deletions(-)
> >
> > @@ -139,19 +132,17 @@ static ssize_t value_store(struct device *dev,
> > long value;
> >
> > status = kstrtol(buf, 0, &value);
> > + if (status)
> > + return status;
> >
> > - mutex_lock(&data->mutex);
> > + 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;
> >
> > - mutex_unlock(&data->mutex);
> > + gpiod_set_value_cansleep(desc, value);
> >
> > - return status;
> > + return size;
> > }
>
> This is a behavioural change as you've moved the decode check before the
> permission check. Not sure if that is significant or not, so in my
> suggestion I retained the old order.
>
> Cheers,
> Kent.
Yeah, I don't know why it was done. Typically you want to sanitize the
input before anything else and this is what's done almost everywhere
else. I'd keep it like that.
Bart
On Fri, Oct 25, 2024 at 04:08:00PM +0200, Bartosz Golaszewski wrote:
> On Fri, Oct 25, 2024 at 3:24 PM Kent Gibson <warthog618@gmail.com> wrote:
> >
> > On Fri, Oct 25, 2024 at 02:18:51PM +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 | 81 ++++++++++++++++----------------------------
> > > 1 file changed, 29 insertions(+), 52 deletions(-)
> > >
> > > @@ -139,19 +132,17 @@ static ssize_t value_store(struct device *dev,
> > > long value;
> > >
> > > status = kstrtol(buf, 0, &value);
> > > + if (status)
> > > + return status;
> > >
> > > - mutex_lock(&data->mutex);
> > > + 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;
> > >
> > > - mutex_unlock(&data->mutex);
> > > + gpiod_set_value_cansleep(desc, value);
> > >
> > > - return status;
> > > + return size;
> > > }
> >
> > This is a behavioural change as you've moved the decode check before the
> > permission check. Not sure if that is significant or not, so in my
> > suggestion I retained the old order.
> >
> > Cheers,
> > Kent.
>
> Yeah, I don't know why it was done. Typically you want to sanitize the
> input before anything else and this is what's done almost everywhere
> else. I'd keep it like that.
Not knowing why it was done was precisely the reason I thought it
should be left as is. The fact that the checks are performed in the
other order elsewhere makes me think this one was done intentionally.
Conceivably it could be used by userspace to test if a line is output when
the direction is fixed (so /sys/class/gpio/gpioN/direction does not exist).
So write a non-integer to the value and see if it returns -EPERM rather
than -EINVAL.
Admittedly I'm speculating, but I can't rule it out, so I wouldn't
change the behaviour just because it is more aesthetically pleasing.
And if you insist on tidying the behaviour then it should be in a separate
patch rather than piggy-backing onto the guard change.
Anyway, that is my 2c.
Cheers,
Kent.
On Fri, Oct 25, 2024 at 5:34 PM Kent Gibson <warthog618@gmail.com> wrote: > > > > > Yeah, I don't know why it was done. Typically you want to sanitize the > > input before anything else and this is what's done almost everywhere > > else. I'd keep it like that. > > Not knowing why it was done was precisely the reason I thought it > should be left as is. The fact that the checks are performed in the > other order elsewhere makes me think this one was done intentionally. > Conceivably it could be used by userspace to test if a line is output when > the direction is fixed (so /sys/class/gpio/gpioN/direction does not exist). > So write a non-integer to the value and see if it returns -EPERM rather > than -EINVAL. > > Admittedly I'm speculating, but I can't rule it out, so I wouldn't > change the behaviour just because it is more aesthetically pleasing. > And if you insist on tidying the behaviour then it should be in a separate > patch rather than piggy-backing onto the guard change. > > Anyway, that is my 2c. > Ok, I'll restore the order in v3. Bartosz
© 2016 - 2024 Red Hat, Inc.