gpio: aggregator: Further improvements

PATCH Superseded Andy Shevchenko 4 participants 9 messages Jul 18, 2020 – Aug 10, 2020

Subsystems (2)

GPIO GPIO aggregator

lore

Share & tools

lore

Andy Shevchenko

author

2020-07-18 21:26 UTC #0 lore raw

cover gpio: aggregator: Further improvements

Patch 1 makes sparse happy about locking and, in my opinion, improves readability, though increases LOC count.
Patch 2 simplifies parser by using existing helpers.
Patch 3 refactors loop in parser for better readability in my opinion.

It might be you have different opinions about patches 1 and 3, we may discuss.

Compile tested only.

Andy Shevchenko (3):
gpio: aggregator: Refactor ->{get,set}_multiple() to make Sparse happy
gpio: aggregator: Simplify isrange() by using get_option()
gpio: aggregator: Assign name and offsets only once in a loop

 drivers/gpio/gpio-aggregator.c | 110 +++++++++++++++++----------------
 1 file changed, 57 insertions(+), 53 deletions(-)

Signature · 1 line

2.27.0

Andy Shevchenko

author

2020-07-18 21:26 UTC #1 lore raw

PATCH 1/3 gpio: aggregator: Refactor ->{get,set}_multiple() to make Sparse happy

Sparse can't see locking scheme used in ->get_multiple() and
->set_multiple() callbacks.
CHECK .../drivers/gpio/gpio-aggregator.c
.../spinlock.h:409:9: warning: context imbalance in 'gpio_fwd_get_multiple' - unexpected unlock .../spinlock.h:409:9: warning: context imbalance in 'gpio_fwd_set_multiple' - unexpected unlock

Refactor them to have better readability and make Sparse happy.

Signed-off-by Andy Shevchenko <[email protected]>

---
 drivers/gpio/gpio-aggregator.c | 70 +++++++++++++++++++++-------------
 1 file changed, 43 insertions(+), 27 deletions(-)

drivers/gpio/gpio-aggregator.c +43 ~~-27~~

index 424a3d25350b..5be166e73381 100644
@@ -333,20 +333,14 @@ static int gpio_fwd_get(struct gpio_chip *chip, unsigned int offset)
 	return gpiod_get_value(fwd->descs[offset]);
 }
 
-static int gpio_fwd_get_multiple(struct gpio_chip *chip, unsigned long *mask,
+static int gpio_fwd_get_multiple(struct gpiochip_fwd *fwd, unsigned long *mask,
 				 unsigned long *bits)
 {
-	struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
-	unsigned long *values, flags = 0;
 	struct gpio_desc **descs;
+	unsigned long *values;
 	unsigned int i, j = 0;
 	int error;
 
-	if (chip->can_sleep)
-		mutex_lock(&fwd->mlock);
-	else
-		spin_lock_irqsave(&fwd->slock, flags);
-
 	/* Both values bitmap and desc pointers are stored in tmp[] */
 	values = &fwd->tmp[0];
 	descs = (void *)&fwd->tmp[BITS_TO_LONGS(fwd->chip.ngpio)];

@@ -356,16 +350,32 @@ static int gpio_fwd_get_multiple(struct gpio_chip *chip, unsigned long *mask,
 		descs[j++] = fwd->descs[i];
 
 	error = gpiod_get_array_value(j, descs, NULL, values);
-	if (!error) {
-		j = 0;
-		for_each_set_bit(i, mask, fwd->chip.ngpio)
-			__assign_bit(i, bits, test_bit(j++, values));
-	}
+	if (error)
+		return error;
 
-	if (chip->can_sleep)
+	j = 0;
+	for_each_set_bit(i, mask, fwd->chip.ngpio)
+		__assign_bit(i, bits, test_bit(j++, values));
+
+	return 0;
+}
+
+static int gpio_fwd_get_multiple_locked(struct gpio_chip *chip,
+				        unsigned long *mask, unsigned long *bits)
+{
+	struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
+	unsigned long flags;
+	int error;
+
+	if (chip->can_sleep) {
+		mutex_lock(&fwd->mlock);
+		error = gpio_fwd_get_multiple(fwd, mask, bits);
 		mutex_unlock(&fwd->mlock);
-	else
+	} else {
+		spin_lock_irqsave(&fwd->slock, flags);
+		error = gpio_fwd_get_multiple(fwd, mask, bits);
 		spin_unlock_irqrestore(&fwd->slock, flags);
+	}
 
 	return error;
 }

@@ -377,19 +387,13 @@ static void gpio_fwd_set(struct gpio_chip *chip, unsigned int offset, int value)
 	gpiod_set_value(fwd->descs[offset], value);
 }
 
-static void gpio_fwd_set_multiple(struct gpio_chip *chip, unsigned long *mask,
+static void gpio_fwd_set_multiple(struct gpiochip_fwd *fwd, unsigned long *mask,
 				  unsigned long *bits)
 {
-	struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
-	unsigned long *values, flags = 0;
 	struct gpio_desc **descs;
+	unsigned long *values;
 	unsigned int i, j = 0;
 
-	if (chip->can_sleep)
-		mutex_lock(&fwd->mlock);
-	else
-		spin_lock_irqsave(&fwd->slock, flags);
-
 	/* Both values bitmap and desc pointers are stored in tmp[] */
 	values = &fwd->tmp[0];
 	descs = (void *)&fwd->tmp[BITS_TO_LONGS(fwd->chip.ngpio)];

@@ -400,11 +404,23 @@ static void gpio_fwd_set_multiple(struct gpio_chip *chip, unsigned long *mask,
 	}
 
 	gpiod_set_array_value(j, descs, NULL, values);
+}
 
-	if (chip->can_sleep)
+static void gpio_fwd_set_multiple_locked(struct gpio_chip *chip,
+				         unsigned long *mask, unsigned long *bits)
+{
+	struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
+	unsigned long flags;
+
+	if (chip->can_sleep) {
+		mutex_lock(&fwd->mlock);
+		gpio_fwd_set_multiple(fwd, mask, bits);
 		mutex_unlock(&fwd->mlock);
-	else
+	} else {
+		spin_lock_irqsave(&fwd->slock, flags);
+		gpio_fwd_set_multiple(fwd, mask, bits);
 		spin_unlock_irqrestore(&fwd->slock, flags);
+	}
 }
 
 static int gpio_fwd_set_config(struct gpio_chip *chip, unsigned int offset,

@@ -470,9 +486,9 @@ static struct gpiochip_fwd *gpiochip_fwd_create(struct device *dev,
 	chip->direction_input = gpio_fwd_direction_input;
 	chip->direction_output = gpio_fwd_direction_output;
 	chip->get = gpio_fwd_get;
-	chip->get_multiple = gpio_fwd_get_multiple;
+	chip->get_multiple = gpio_fwd_get_multiple_locked;
 	chip->set = gpio_fwd_set;
-	chip->set_multiple = gpio_fwd_set_multiple;
+	chip->set_multiple = gpio_fwd_set_multiple_locked;
 	chip->base = -1;
 	chip->ngpio = ngpios;
 	fwd->descs = descs;

Signature · 1 line

2.27.0

Geert Uytterhoeven

Reviewed-by on v1 1/3

2020-08-05 17:23 UTC #2 lore raw

Re: [PATCH v1 1/3] gpio: aggregator: Refactor ->{get,set}_multiple() to make Sparse happy

Hi Andy,

Quoted 9 lines · Andy Shevchenko

Sparse can't see locking scheme used in ->get_multiple() and
->set_multiple() callbacks.
CHECK .../drivers/gpio/gpio-aggregator.c
.../spinlock.h:409:9: warning: context imbalance in 'gpio_fwd_get_multiple' - unexpected unlock .../spinlock.h:409:9: warning: context imbalance in 'gpio_fwd_set_multiple' - unexpected unlock
Refactor them to have better readability and make Sparse happy.
Signed-off-by Andy Shevchenko <[email protected]>

Thanks for your patch!

drivers/gpio/gpio-aggregator.c static int gpio_fwd_get_multiple(struct gpio_chip *chip, unsigned long *mask, · line 364

Quoted 5 lines

drivers/gpio/gpio-aggregator.c +2 -0

 
+static int gpio_fwd_get_multiple_locked(struct gpio_chip *chip,
+                                       unsigned long *mask, unsigned long *bits)

Trading one static analysis tool error for a different one? ;-)

ERROR: code indent should use tabs where possible

drivers/gpio/gpio-aggregator.c static void gpio_fwd_set_multiple(struct gpio_chip *chip, unsigned long *mask, · line 410

Quoted 9 lines

drivers/gpio/gpio-aggregator.c +3 -1

@@ -400,11 +404,23 @@ static void gpio_fwd_set_multiple(struct gpio_chip *chip, unsigned long *mask,
        }
 
        gpiod_set_array_value(j, descs, NULL, values);
+}
 
-       if (chip->can_sleep)
+static void gpio_fwd_set_multiple_locked(struct gpio_chip *chip,
+                                        unsigned long *mask, unsigned long *bits)

ERROR: code indent should use tabs where possible

With the above checkpatch errors fixed:

Reviewed-by Geert Uytterhoeven <[email protected]>

Code size impact is +52 bytes with arm-linux-gnueabihf-gcc 7.5.0.

Gr{oetje,eeting}s,

Geert

Signature · 5 lines

Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Andy Shevchenko

author

2020-08-10 11:33 UTC #3 lore raw

Re: [PATCH v1 1/3] gpio: aggregator: Refactor ->{get,set}_multiple() to make Sparse happy

Quoted 2 lines · Geert Uytterhoeven

On Sat, Jul 18, 2020 at 11:26 PM Andy Shevchenko
<[email protected]> wrote:

...

Quoted 1 line

ERROR: code indent should use tabs where possible

...

Quoted 4 lines

ERROR: code indent should use tabs where possible
With the above checkpatch errors fixed:
Reviewed-by Geert Uytterhoeven <[email protected]>

Thanks, I'll send it separately, patch 2 needs to be thought thru a bit more.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Andy Shevchenko

author

2020-07-18 21:26 UTC #4 lore raw

PATCH 2/3 gpio: aggregator: Simplify isrange() by using get_option()

In reply to Andy Shevchenko

We already have a nice helper called get_option() which can be used to validate the input format. Simplify isrange() by using it.

Signed-off-by Andy Shevchenko <[email protected]>

---
 drivers/gpio/gpio-aggregator.c | 29 ++++++++---------------------
 1 file changed, 8 insertions(+), 21 deletions(-)

drivers/gpio/gpio-aggregator.c +8 ~~-21~~

index 5be166e73381..de9ae622ca23 100644
@@ -64,30 +64,17 @@ static char *get_arg(char **args)
 
 static bool isrange(const char *s)
 {
-	size_t n;
+	char **args = (char **)&s;
+	int dummy;
+	int res;
 
-	if (IS_ERR_OR_NULL(s))
+	if (IS_ERR_OR_NULL(s) || *s == '\0')
 		return false;
 
-	while (1) {
-		n = strspn(s, "0123456789");
-		if (!n)
-			return false;
-
-		s += n;
-
-		switch (*s++) {
-		case '\0':
-			return true;
-
-		case '-':
-		case ',':
-			break;
-
-		default:
-			return false;
-		}
-	}
+	do {
+		res = get_option(args, &dummy);
+	} while (res);
+	return **args == '\0';
 }
 
 static int aggr_add_gpio(struct gpio_aggregator *aggr, const char *key,

Signature · 1 line

2.27.0

Geert Uytterhoeven

2020-08-05 16:29 UTC #5 lore raw

Re: [PATCH v1 2/3] gpio: aggregator: Simplify isrange() by using get_option()

Hi Andy,

Quoted 4 lines · Andy Shevchenko

We already have a nice helper called get_option() which can be used to validate the input format. Simplify isrange() by using it.
Signed-off-by Andy Shevchenko <[email protected]>

Thanks for your patch!

Quoted 29 lines

drivers/gpio/gpio-aggregator.c +4 ~~-16~~

@@ -64,30 +64,17 @@ static char *get_arg(char **args)
 
 static bool isrange(const char *s)
 {
-       size_t n;
+       char **args = (char **)&s;
+       int dummy;
+       int res;
 
-       if (IS_ERR_OR_NULL(s))
+       if (IS_ERR_OR_NULL(s) || *s == '\0')
                return false;
 
-       while (1) {
-               n = strspn(s, "0123456789");
-               if (!n)
-                       return false;
-
-               s += n;
-
-               switch (*s++) {
-               case '\0':
-                       return true;
-
-               case '-':
-               case ',':
-                       break;

drivers/gpio/gpio-aggregator.c static char *get_arg(char **args) · line 78

Quoted 10 lines

drivers/gpio/gpio-aggregator.c +4 -5

-
-               default:
-                       return false;
-               }
-       }
+       do {
+               res = get_option(args, &dummy);
+       } while (res);
+       return **args == '\0';
 }

This change makes isrange() return true for all of the following, although they are not valid ranges:

Gr{oetje,eeting}s,

Geert

Signature · 5 lines

Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Andy Shevchenko

author

2020-07-18 21:26 UTC #6 lore raw

PATCH 3/3 gpio: aggregator: Assign name and offsets only once in a loop

In reply to Andy Shevchenko

The for-loop looks a bit hard to read when we extract two arguments per iteration. The 'do {} while (true)' makes it easier to read despite being infinite loop.

Signed-off-by Andy Shevchenko <[email protected]>

---
 drivers/gpio/gpio-aggregator.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

drivers/gpio/gpio-aggregator.c +6 -5

index de9ae622ca23..962ec9373d6f 100644
@@ -109,14 +109,17 @@ static int aggr_parse(struct gpio_aggregator *aggr)
 	if (!bitmap)
 		return -ENOMEM;
 
-	for (name = get_arg(&args), offsets = get_arg(&args); name;
-	     offsets = get_arg(&args)) {
+	do {
+		name = get_arg(&args);
+		if (!name)
+			break;
 		if (IS_ERR(name)) {
 			pr_err("Cannot get GPIO specifier: %pe\n", name);
 			error = PTR_ERR(name);
 			goto free_bitmap;
 		}
 
+		offsets = get_arg(&args);
 		if (!isrange(offsets)) {
 			/* Named GPIO line */
 			error = aggr_add_gpio(aggr, name, U16_MAX, &n);

@@ -139,9 +142,7 @@ static int aggr_parse(struct gpio_aggregator *aggr)
 			if (error)
 				goto free_bitmap;
 		}
-
-		name = get_arg(&args);
-	}
+	} while (true);
 
 	if (!n) {
 		pr_err("No GPIOs specified\n");

Signature · 1 line

2.27.0

Andy Shevchenko

2020-07-19 10:13 UTC #7 lore raw

Re: [PATCH v1 3/3] gpio: aggregator: Assign name and offsets only once in a loop

Quoted 4 lines · Andy Shevchenko

The for-loop looks a bit hard to read when we extract two arguments per iteration. The 'do {} while (true)' makes it easier to read despite being infinite loop.

This is not gonna work.
Please, discard this patch from the series.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Linus Walleij

2020-07-21 09:46 UTC #8 lore raw

In reply to Andy Shevchenko

Quoted 4 lines · Andy Shevchenko

Patch 1 makes sparse happy about locking and, in my opinion, improves readability, though increases LOC count.
Patch 2 simplifies parser by using existing helpers.
Patch 3 refactors loop in parser for better readability in my opinion.

I see you retracted patch 3, but patch 1 & 2 looks good to me, I just would like Geert to have a look at them first, Geert?

Yours,
Linus Walleij