* [PATCH v1 0/3] gpio: aggregator: Further improvements
@ 2020-07-18 21:26 Andy Shevchenko
2020-07-18 21:26 ` [PATCH v1 1/3] gpio: aggregator: Refactor ->{get,set}_multiple() to make Sparse happy Andy Shevchenko
` (3 more replies)
0 siblings, 4 replies; 9+ messages in thread
From: Andy Shevchenko @ 2020-07-18 21:26 UTC (permalink / raw)
To: Linus Walleij, Bartosz Golaszewski, linux-gpio, Geert Uytterhoeven
Cc: 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.
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(-)
--
2.27.0
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH v1 1/3] gpio: aggregator: Refactor ->{get,set}_multiple() to make Sparse happy 2020-07-18 21:26 [PATCH v1 0/3] gpio: aggregator: Further improvements Andy Shevchenko @ 2020-07-18 21:26 ` Andy Shevchenko 2020-08-05 14:56 ` Geert Uytterhoeven 2020-07-18 21:26 ` [PATCH v1 2/3] gpio: aggregator: Simplify isrange() by using get_option() Andy Shevchenko ` (2 subsequent siblings) 3 siblings, 1 reply; 9+ messages in thread From: Andy Shevchenko @ 2020-07-18 21:26 UTC (permalink / raw) To: Linus Walleij, Bartosz Golaszewski, linux-gpio, Geert Uytterhoeven Cc: 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 <andriy.shevchenko@linux.intel.com> --- drivers/gpio/gpio-aggregator.c | 70 +++++++++++++++++++++------------- 1 file changed, 43 insertions(+), 27 deletions(-) diff --git a/drivers/gpio/gpio-aggregator.c b/drivers/gpio/gpio-aggregator.c index 424a3d25350b..5be166e73381 100644 --- a/drivers/gpio/gpio-aggregator.c +++ b/drivers/gpio/gpio-aggregator.c @@ -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; -- 2.27.0 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v1 1/3] gpio: aggregator: Refactor ->{get,set}_multiple() to make Sparse happy 2020-07-18 21:26 ` [PATCH v1 1/3] gpio: aggregator: Refactor ->{get,set}_multiple() to make Sparse happy Andy Shevchenko @ 2020-08-05 14:56 ` Geert Uytterhoeven 2020-08-10 11:09 ` Andy Shevchenko 0 siblings, 1 reply; 9+ messages in thread From: Geert Uytterhoeven @ 2020-08-05 14:56 UTC (permalink / raw) To: Andy Shevchenko Cc: Linus Walleij, Bartosz Golaszewski, open list:GPIO SUBSYSTEM Hi Andy, On Sat, Jul 18, 2020 at 11:26 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > 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 <andriy.shevchenko@linux.intel.com> Thanks for your patch! > --- a/drivers/gpio/gpio-aggregator.c > +++ b/drivers/gpio/gpio-aggregator.c > +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 > @@ -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 <geert+renesas@glider.be> Code size impact is +52 bytes with arm-linux-gnueabihf-gcc 7.5.0. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v1 1/3] gpio: aggregator: Refactor ->{get,set}_multiple() to make Sparse happy 2020-08-05 14:56 ` Geert Uytterhoeven @ 2020-08-10 11:09 ` Andy Shevchenko 0 siblings, 0 replies; 9+ messages in thread From: Andy Shevchenko @ 2020-08-10 11:09 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Linus Walleij, Bartosz Golaszewski, open list:GPIO SUBSYSTEM On Wed, Aug 05, 2020 at 04:56:06PM +0200, Geert Uytterhoeven wrote: > On Sat, Jul 18, 2020 at 11:26 PM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: ... > ERROR: code indent should use tabs where possible ... > ERROR: code indent should use tabs where possible > With the above checkpatch errors fixed: > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> Thanks, I'll send it separately, patch 2 needs to be thought thru a bit more. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v1 2/3] gpio: aggregator: Simplify isrange() by using get_option() 2020-07-18 21:26 [PATCH v1 0/3] gpio: aggregator: Further improvements Andy Shevchenko 2020-07-18 21:26 ` [PATCH v1 1/3] gpio: aggregator: Refactor ->{get,set}_multiple() to make Sparse happy Andy Shevchenko @ 2020-07-18 21:26 ` Andy Shevchenko 2020-08-05 14:37 ` Geert Uytterhoeven 2020-07-18 21:26 ` [PATCH v1 3/3] gpio: aggregator: Assign name and offsets only once in a loop Andy Shevchenko 2020-07-21 9:46 ` [PATCH v1 0/3] gpio: aggregator: Further improvements Linus Walleij 3 siblings, 1 reply; 9+ messages in thread From: Andy Shevchenko @ 2020-07-18 21:26 UTC (permalink / raw) To: Linus Walleij, Bartosz Golaszewski, linux-gpio, Geert Uytterhoeven Cc: 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 <andriy.shevchenko@linux.intel.com> --- drivers/gpio/gpio-aggregator.c | 29 ++++++++--------------------- 1 file changed, 8 insertions(+), 21 deletions(-) diff --git a/drivers/gpio/gpio-aggregator.c b/drivers/gpio/gpio-aggregator.c index 5be166e73381..de9ae622ca23 100644 --- a/drivers/gpio/gpio-aggregator.c +++ b/drivers/gpio/gpio-aggregator.c @@ -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, -- 2.27.0 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v1 2/3] gpio: aggregator: Simplify isrange() by using get_option() 2020-07-18 21:26 ` [PATCH v1 2/3] gpio: aggregator: Simplify isrange() by using get_option() Andy Shevchenko @ 2020-08-05 14:37 ` Geert Uytterhoeven 0 siblings, 0 replies; 9+ messages in thread From: Geert Uytterhoeven @ 2020-08-05 14:37 UTC (permalink / raw) To: Andy Shevchenko Cc: Linus Walleij, Bartosz Golaszewski, open list:GPIO SUBSYSTEM Hi Andy, On Sat, Jul 18, 2020 at 11:26 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > 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 <andriy.shevchenko@linux.intel.com> Thanks for your patch! > --- a/drivers/gpio/gpio-aggregator.c > +++ b/drivers/gpio/gpio-aggregator.c > @@ -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'; > } This change makes isrange() return true for all of the following, although they are not valid ranges: -7 --7 7- 7--9 Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v1 3/3] gpio: aggregator: Assign name and offsets only once in a loop 2020-07-18 21:26 [PATCH v1 0/3] gpio: aggregator: Further improvements Andy Shevchenko 2020-07-18 21:26 ` [PATCH v1 1/3] gpio: aggregator: Refactor ->{get,set}_multiple() to make Sparse happy Andy Shevchenko 2020-07-18 21:26 ` [PATCH v1 2/3] gpio: aggregator: Simplify isrange() by using get_option() Andy Shevchenko @ 2020-07-18 21:26 ` Andy Shevchenko 2020-07-19 10:12 ` Andy Shevchenko 2020-07-21 9:46 ` [PATCH v1 0/3] gpio: aggregator: Further improvements Linus Walleij 3 siblings, 1 reply; 9+ messages in thread From: Andy Shevchenko @ 2020-07-18 21:26 UTC (permalink / raw) To: Linus Walleij, Bartosz Golaszewski, linux-gpio, Geert Uytterhoeven Cc: 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 <andriy.shevchenko@linux.intel.com> --- drivers/gpio/gpio-aggregator.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/drivers/gpio/gpio-aggregator.c b/drivers/gpio/gpio-aggregator.c index de9ae622ca23..962ec9373d6f 100644 --- a/drivers/gpio/gpio-aggregator.c +++ b/drivers/gpio/gpio-aggregator.c @@ -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"); -- 2.27.0 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v1 3/3] gpio: aggregator: Assign name and offsets only once in a loop 2020-07-18 21:26 ` [PATCH v1 3/3] gpio: aggregator: Assign name and offsets only once in a loop Andy Shevchenko @ 2020-07-19 10:12 ` Andy Shevchenko 0 siblings, 0 replies; 9+ messages in thread From: Andy Shevchenko @ 2020-07-19 10:12 UTC (permalink / raw) To: Andy Shevchenko Cc: Linus Walleij, Bartosz Golaszewski, open list:GPIO SUBSYSTEM, Geert Uytterhoeven On Sun, Jul 19, 2020 at 12:26 AM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > 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. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v1 0/3] gpio: aggregator: Further improvements 2020-07-18 21:26 [PATCH v1 0/3] gpio: aggregator: Further improvements Andy Shevchenko ` (2 preceding siblings ...) 2020-07-18 21:26 ` [PATCH v1 3/3] gpio: aggregator: Assign name and offsets only once in a loop Andy Shevchenko @ 2020-07-21 9:46 ` Linus Walleij 3 siblings, 0 replies; 9+ messages in thread From: Linus Walleij @ 2020-07-21 9:46 UTC (permalink / raw) To: Andy Shevchenko, Geert Uytterhoeven Cc: Bartosz Golaszewski, open list:GPIO SUBSYSTEM On Sat, Jul 18, 2020 at 11:26 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-08-10 11:33 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-18 21:26 [PATCH v1 0/3] gpio: aggregator: Further improvements Andy Shevchenko
2020-07-18 21:26 ` [PATCH v1 1/3] gpio: aggregator: Refactor ->{get,set}_multiple() to make Sparse happy Andy Shevchenko
2020-08-05 14:56 ` Geert Uytterhoeven
2020-08-10 11:09 ` Andy Shevchenko
2020-07-18 21:26 ` [PATCH v1 2/3] gpio: aggregator: Simplify isrange() by using get_option() Andy Shevchenko
2020-08-05 14:37 ` Geert Uytterhoeven
2020-07-18 21:26 ` [PATCH v1 3/3] gpio: aggregator: Assign name and offsets only once in a loop Andy Shevchenko
2020-07-19 10:12 ` Andy Shevchenko
2020-07-21 9:46 ` [PATCH v1 0/3] gpio: aggregator: Further improvements Linus Walleij
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).