[PATCH v3 1/6] lib/cmdline_kunit: add a new test case for get_options()

[PATCH v3 1/6] lib/cmdline_kunit: add a new test case for get_options()

[Andy Shevchenko]

Add a test case for get_options() which is provided by cmdline.c.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
v3: drop string macros (Bart)
 lib/cmdline_kunit.c | 47 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 47 insertions(+)

diff --git a/lib/cmdline_kunit.c b/lib/cmdline_kunit.c
index 550e7a47fd24..91737f17ce51 100644
--- a/lib/cmdline_kunit.c
+++ b/lib/cmdline_kunit.c
@@ -18,6 +18,26 @@ static const int cmdline_test_values[] = {
 	1, 3, 2, 1, 1, 1, 3, 1,
 };
 
+static_assert(ARRAY_SIZE(cmdline_test_strings) == ARRAY_SIZE(cmdline_test_values));
+
+static const char *cmdline_test_range_strings[] = {
+	"-7" , "--7"  , "-1-2"    , "7--9",
+	"7-" , "-7--9", "7-9,"    , "9-7" ,
+	"5-a", "a-5"  , "5-8"     , ",8-5",
+	"+,1", "-,4"  , "-3,0-1,6", "4,-" ,
+	" +2", " -9"  , "0-1,-3,6", "- 9" ,
+};
+
+static const int cmdline_test_range_values[][16] = {
+	{ 1, -7, }, { 0, -0, }, { 4, -1, 0, +1, 2, }, { 0, 7, },
+	{ 0, +7, }, { 0, -7, }, { 3, +7, 8, +9, 0, }, { 0, 9, },
+	{ 0, +5, }, { 0, -0, }, { 4, +5, 6, +7, 8, }, { 0, 0, },
+	{ 0, +0, }, { 0, -0, }, { 4, -3, 0, +1, 6, }, { 1, 4, },
+	{ 0, +0, }, { 0, -0, }, { 4, +0, 1, -3, 6, }, { 0, 0, },
+};
+
+static_assert(ARRAY_SIZE(cmdline_test_range_strings) == ARRAY_SIZE(cmdline_test_range_values));
+
 static void cmdline_do_one_test(struct kunit *test, const char *in, int rc, int offset)
 {
 	const char *fmt = "Pattern: %s";
@@ -84,10 +104,37 @@ static void cmdline_test_tail_int(struct kunit *test)
 	} while (++i < ARRAY_SIZE(cmdline_test_strings));
 }
 
+static void cmdline_do_one_range_test(struct kunit *test, const char *in,
+				      unsigned int n, const int *e)
+{
+	unsigned int i;
+	int r[16];
+
+	memset(r, 0, sizeof(r));
+	get_options(in, ARRAY_SIZE(r), r);
+	KUNIT_EXPECT_EQ_MSG(test, r[0], e[0], "in test %u expected %d numbers, got %d",
+			    n, e[0], r[0]);
+	for (i = 1; i < ARRAY_SIZE(r); i++)
+		KUNIT_EXPECT_EQ_MSG(test, r[i], e[i], "in test %u at %d", n, i);
+}
+
+static void cmdline_test_range(struct kunit *test)
+{
+	unsigned int i = 0;
+
+	do {
+		const char *str = cmdline_test_range_strings[i];
+		const int *e = cmdline_test_range_values[i];
+
+		cmdline_do_one_range_test(test, str, i, e);
+	} while (++i < ARRAY_SIZE(cmdline_test_range_strings));
+}
+
 static struct kunit_case cmdline_test_cases[] = {
 	KUNIT_CASE(cmdline_test_noint),
 	KUNIT_CASE(cmdline_test_lead_int),
 	KUNIT_CASE(cmdline_test_tail_int),
+	KUNIT_CASE(cmdline_test_range),
 	{}
 };
 
-- 
2.29.2

[PATCH v3 2/6] lib/cmdline: Update documentation to reflect behaviour

[Andy Shevchenko]

get_options() API has some tricks to optimize that may be not so obvious
to the caller. Update documentation to reflect current behaviour.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
v3: added Rb tag (Bart)
 lib/cmdline.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/lib/cmdline.c b/lib/cmdline.c
index b390dd03363b..2a9ae2143e42 100644
--- a/lib/cmdline.c
+++ b/lib/cmdline.c
@@ -83,7 +83,7 @@ EXPORT_SYMBOL(get_option);
  *	get_options - Parse a string into a list of integers
  *	@str: String to be parsed
  *	@nints: size of integer array
- *	@ints: integer array
+ *	@ints: integer array (must have a room for at least one element)
  *
  *	This function parses a string containing a comma-separated
  *	list of integers, a hyphen-separated range of _positive_ integers,
@@ -91,6 +91,11 @@ EXPORT_SYMBOL(get_option);
  *	full, or when no more numbers can be retrieved from the
  *	string.
  *
+ *	Returns:
+ *
+ *	The first element is filled by the amount of the collected numbers
+ *	in the range. The rest is what was parsed from the @str.
+ *
  *	Return value is the character in the string which caused
  *	the parse to end (typically a null terminator, if @str is
  *	completely parseable).
-- 
2.29.2

Re: [PATCH v3 2/6] lib/cmdline: Update documentation to reflect behaviour

[Geert Uytterhoeven]

Hi Andriy,

On Fri, Jan 22, 2021 at 1:39 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> get_options() API has some tricks to optimize that may be not so obvious
> to the caller. Update documentation to reflect current behaviour.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Reviewed-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Thanks for your patch!

> --- a/lib/cmdline.c
> +++ b/lib/cmdline.c
> @@ -83,7 +83,7 @@ EXPORT_SYMBOL(get_option);
>   *     get_options - Parse a string into a list of integers
>   *     @str: String to be parsed
>   *     @nints: size of integer array
> - *     @ints: integer array
> + *     @ints: integer array (must have a room for at least one element)

must have room

>   *
>   *     This function parses a string containing a comma-separated
>   *     list of integers, a hyphen-separated range of _positive_ integers,
> @@ -91,6 +91,11 @@ EXPORT_SYMBOL(get_option);
>   *     full, or when no more numbers can be retrieved from the
>   *     string.
>   *
> + *     Returns:
> + *
> + *     The first element is filled by the amount of the collected numbers

the number of collected integers?

Yes, the lack of articles in RU can be a disadvantage...

> + *     in the range. The rest is what was parsed from the @str.
> + *
>   *     Return value is the character in the string which caused
>   *     the parse to end (typically a null terminator, if @str is
>   *     completely parseable).

With the above fixed:
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

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

Re: [PATCH v3 2/6] lib/cmdline: Update documentation to reflect behaviour

[Andy Shevchenko]

On Fri, Jan 22, 2021 at 02:10:17PM +0100, Geert Uytterhoeven wrote:
> On Fri, Jan 22, 2021 at 1:39 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > get_options() API has some tricks to optimize that may be not so obvious
> > to the caller. Update documentation to reflect current behaviour.
> >
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Reviewed-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> Thanks for your patch!

Thanks for review.
I'll fix this in my repo w/o sending a v4.

> > --- a/lib/cmdline.c
> > +++ b/lib/cmdline.c
> > @@ -83,7 +83,7 @@ EXPORT_SYMBOL(get_option);
> >   *     get_options - Parse a string into a list of integers
> >   *     @str: String to be parsed
> >   *     @nints: size of integer array
> > - *     @ints: integer array
> > + *     @ints: integer array (must have a room for at least one element)
> 
> must have room
> 
> >   *
> >   *     This function parses a string containing a comma-separated
> >   *     list of integers, a hyphen-separated range of _positive_ integers,
> > @@ -91,6 +91,11 @@ EXPORT_SYMBOL(get_option);
> >   *     full, or when no more numbers can be retrieved from the
> >   *     string.
> >   *
> > + *     Returns:
> > + *
> > + *     The first element is filled by the amount of the collected numbers
> 
> the number of collected integers?
> 
> Yes, the lack of articles in RU can be a disadvantage...

:-)

> 
> > + *     in the range. The rest is what was parsed from the @str.
> > + *
> >   *     Return value is the character in the string which caused
> >   *     the parse to end (typically a null terminator, if @str is
> >   *     completely parseable).
> 
> With the above fixed:
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> 
> 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

-- 
With Best Regards,
Andy Shevchenko

[PATCH v3 3/6] lib/cmdline: Allow get_options() to take 0 to validate the input

[Andy Shevchenko]

Allow get_options() to take 0 as a number of integers parameter to validate
the input.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
v3: dropped string macros, added "the" in the comment (Bart)
 lib/cmdline.c       | 14 +++++++++++---
 lib/cmdline_kunit.c | 11 ++++++++++-
 2 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/lib/cmdline.c b/lib/cmdline.c
index 2a9ae2143e42..16b9eaa39538 100644
--- a/lib/cmdline.c
+++ b/lib/cmdline.c
@@ -91,6 +91,9 @@ EXPORT_SYMBOL(get_option);
  *	full, or when no more numbers can be retrieved from the
  *	string.
  *
+ *	When @nints is 0, the function just validates the given @str and
+ *	returns the amount of parseable integers as described below.
+ *
  *	Returns:
  *
  *	The first element is filled by the amount of the collected numbers
@@ -103,15 +106,20 @@ EXPORT_SYMBOL(get_option);
 
 char *get_options(const char *str, int nints, int *ints)
 {
+	bool validate = (nints == 0);
 	int res, i = 1;
 
-	while (i < nints) {
-		res = get_option((char **)&str, ints + i);
+	while (i < nints || validate) {
+		int *pint = validate ? ints : ints + i;
+
+		res = get_option((char **)&str, pint);
 		if (res == 0)
 			break;
 		if (res == 3) {
+			int n = validate ? 0 : nints - i;
 			int range_nums;
-			range_nums = get_range((char **)&str, ints + i, nints - i);
+
+			range_nums = get_range((char **)&str, pint, n);
 			if (range_nums < 0)
 				break;
 			/*
diff --git a/lib/cmdline_kunit.c b/lib/cmdline_kunit.c
index 91737f17ce51..3622c62bcf08 100644
--- a/lib/cmdline_kunit.c
+++ b/lib/cmdline_kunit.c
@@ -109,13 +109,22 @@ static void cmdline_do_one_range_test(struct kunit *test, const char *in,
 {
 	unsigned int i;
 	int r[16];
+	int *p;
 
 	memset(r, 0, sizeof(r));
 	get_options(in, ARRAY_SIZE(r), r);
-	KUNIT_EXPECT_EQ_MSG(test, r[0], e[0], "in test %u expected %d numbers, got %d",
+	KUNIT_EXPECT_EQ_MSG(test, r[0], e[0], "in test %u (parsed) expected %d numbers, got %d",
 			    n, e[0], r[0]);
 	for (i = 1; i < ARRAY_SIZE(r); i++)
 		KUNIT_EXPECT_EQ_MSG(test, r[i], e[i], "in test %u at %d", n, i);
+
+	memset(r, 0, sizeof(r));
+	get_options(in, 0, r);
+	KUNIT_EXPECT_EQ_MSG(test, r[0], e[0], "in test %u (validated) expected %d numbers, got %d",
+			    n, e[0], r[0]);
+
+	p = memchr_inv(&r[1], 0, sizeof(r) - sizeof(r[0]));
+	KUNIT_EXPECT_PTR_EQ_MSG(test, p, (int *)0, "in test %u out of bound at %d", n, p - r);
 }
 
 static void cmdline_test_range(struct kunit *test)
-- 
2.29.2

Re: [PATCH v3 3/6] lib/cmdline: Allow get_options() to take 0 to validate the input

[Geert Uytterhoeven]

Hi Andy,

On Fri, Jan 22, 2021 at 1:39 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> Allow get_options() to take 0 as a number of integers parameter to validate
> the input.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Thanks for your patch!

> --- a/lib/cmdline.c
> +++ b/lib/cmdline.c

> @@ -103,15 +106,20 @@ EXPORT_SYMBOL(get_option);
>
>  char *get_options(const char *str, int nints, int *ints)
>  {
> +       bool validate = (nints == 0);
>         int res, i = 1;
>
> -       while (i < nints) {
> -               res = get_option((char **)&str, ints + i);
> +       while (i < nints || validate) {
> +               int *pint = validate ? ints : ints + i;

I think you can use NULL for validation, as per the documentation for
get_option().

> +
> +               res = get_option((char **)&str, pint);
>                 if (res == 0)
>                         break;
>                 if (res == 3) {
> +                       int n = validate ? 0 : nints - i;
>                         int range_nums;
> -                       range_nums = get_range((char **)&str, ints + i, nints - i);
> +
> +                       range_nums = get_range((char **)&str, pint, n);
>                         if (range_nums < 0)
>                                 break;
>                         /*

Regardless:
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

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

Re: [PATCH v3 3/6] lib/cmdline: Allow get_options() to take 0 to validate the input

[Andy Shevchenko]

On Fri, Jan 22, 2021 at 02:13:10PM +0100, Geert Uytterhoeven wrote:
> On Fri, Jan 22, 2021 at 1:39 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:

...

> > +       while (i < nints || validate) {
> > +               int *pint = validate ? ints : ints + i;
> 
> I think you can use NULL for validation, as per the documentation for
> get_option().

That's what takes me a long time to realize how this machinery works and no,
unfortunately we may not use NULL, we have to keep the parsed number
for the further operations.

Thanks for review!

-- 
With Best Regards,
Andy Shevchenko

[PATCH v3 4/6] gpio: aggregator: Replace isrange() by using get_options()

[Andy Shevchenko]

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

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Reviewed-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
v3: added Rb tags (Linus, Bart)
 drivers/gpio/gpio-aggregator.c | 33 +++------------------------------
 1 file changed, 3 insertions(+), 30 deletions(-)

diff --git a/drivers/gpio/gpio-aggregator.c b/drivers/gpio/gpio-aggregator.c
index dfd8a4876a27..40a081b095fb 100644
--- a/drivers/gpio/gpio-aggregator.c
+++ b/drivers/gpio/gpio-aggregator.c
@@ -62,34 +62,6 @@ static char *get_arg(char **args)
 	return start;
 }
 
-static bool isrange(const char *s)
-{
-	size_t n;
-
-	if (IS_ERR_OR_NULL(s))
-		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;
-		}
-	}
-}
-
 static int aggr_add_gpio(struct gpio_aggregator *aggr, const char *key,
 			 int hwnum, unsigned int *n)
 {
@@ -112,10 +84,10 @@ static int aggr_add_gpio(struct gpio_aggregator *aggr, const char *key,
 
 static int aggr_parse(struct gpio_aggregator *aggr)
 {
+	char *name, *offsets, *p;
 	char *args = aggr->args;
 	unsigned long *bitmap;
 	unsigned int i, n = 0;
-	char *name, *offsets;
 	int error = 0;
 
 	bitmap = bitmap_alloc(ARCH_NR_GPIOS, GFP_KERNEL);
@@ -130,7 +102,8 @@ static int aggr_parse(struct gpio_aggregator *aggr)
 			goto free_bitmap;
 		}
 
-		if (!isrange(offsets)) {
+		p = get_options(offsets, 0, &error);
+		if (error == 0 || *p) {
 			/* Named GPIO line */
 			error = aggr_add_gpio(aggr, name, U16_MAX, &n);
 			if (error)
-- 
2.29.2

Re: [PATCH v3 4/6] gpio: aggregator: Replace isrange() by using get_options()

[Geert Uytterhoeven]

Hi Andy,

On Fri, Jan 22, 2021 at 1:39 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:

> We already have a nice helper called get_options() which can be used
> to validate the input format. Replace isrange() by using it.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> Reviewed-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Let's hope this is never backported to stable before [PATCH 3/6]....

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

[PATCH v3 5/6] gpio: aggregator: Use compound literal from the header

[Andy Shevchenko]

Instead of doing it in place, convert GPIO_LOOKUP_IDX() and GPIO_HOG()
to be compund literals that's allow to use them as rvalue in assignments.

Due to above conversion, use compound literal from the header
in the gpio-aggregator.c.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
v3: new patch
 drivers/gpio/gpio-aggregator.c | 3 +--
 include/linux/gpio/machine.h   | 4 ++--
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/gpio/gpio-aggregator.c b/drivers/gpio/gpio-aggregator.c
index 40a081b095fb..13e473c97ce4 100644
--- a/drivers/gpio/gpio-aggregator.c
+++ b/drivers/gpio/gpio-aggregator.c
@@ -72,8 +72,7 @@ static int aggr_add_gpio(struct gpio_aggregator *aggr, const char *key,
 	if (!lookups)
 		return -ENOMEM;
 
-	lookups->table[*n] =
-		(struct gpiod_lookup)GPIO_LOOKUP_IDX(key, hwnum, NULL, *n, 0);
+	lookups->table[*n] = GPIO_LOOKUP_IDX(key, hwnum, NULL, *n, 0);
 
 	(*n)++;
 	memset(&lookups->table[*n], 0, sizeof(lookups->table[*n]));
diff --git a/include/linux/gpio/machine.h b/include/linux/gpio/machine.h
index 781a053abbb9..d755e529c1e3 100644
--- a/include/linux/gpio/machine.h
+++ b/include/linux/gpio/machine.h
@@ -75,7 +75,7 @@ struct gpiod_hog {
  * gpiod_get_index()
  */
 #define GPIO_LOOKUP_IDX(_key, _chip_hwnum, _con_id, _idx, _flags)         \
-{                                                                         \
+(struct gpiod_lookup) {                                                   \
 	.key = _key,                                                      \
 	.chip_hwnum = _chip_hwnum,                                        \
 	.con_id = _con_id,                                                \
@@ -87,7 +87,7 @@ struct gpiod_hog {
  * Simple definition of a single GPIO hog in an array.
  */
 #define GPIO_HOG(_chip_label, _chip_hwnum, _line_name, _lflags, _dflags)  \
-{                                                                         \
+(struct gpiod_hog) {                                                      \
 	.chip_label = _chip_label,                                        \
 	.chip_hwnum = _chip_hwnum,                                        \
 	.line_name = _line_name,                                          \
-- 
2.29.2

Re: [PATCH v3 5/6] gpio: aggregator: Use compound literal from the header

[Geert Uytterhoeven]

Hi Andy,

Thanks for your patch!

On Fri, Jan 22, 2021 at 1:39 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> Instead of doing it in place, convert GPIO_LOOKUP_IDX() and GPIO_HOG()
> to be compund literals that's allow to use them as rvalue in assignments.

... to compound literals, which can be used as rvalues ...

> Due to above conversion, use compound literal from the header
> in the gpio-aggregator.c.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

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

[PATCH v3 6/6] gpio: aggregator: Remove trailing comma in terminator entries

[Andy Shevchenko]

Remove trailing comma in terminator entries to avoid potential
expanding an array behind it.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Reviewed-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
v3: added tags (Linus, Bart, Geert)
 drivers/gpio/gpio-aggregator.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpio-aggregator.c b/drivers/gpio/gpio-aggregator.c
index 13e473c97ce4..08171431bb8f 100644
--- a/drivers/gpio/gpio-aggregator.c
+++ b/drivers/gpio/gpio-aggregator.c
@@ -243,7 +243,7 @@ static DRIVER_ATTR_WO(delete_device);
 static struct attribute *gpio_aggregator_attrs[] = {
 	&driver_attr_new_device.attr,
 	&driver_attr_delete_device.attr,
-	NULL,
+	NULL
 };
 ATTRIBUTE_GROUPS(gpio_aggregator);
 
@@ -517,7 +517,7 @@ static const struct of_device_id gpio_aggregator_dt_ids[] = {
 	 * Add GPIO-operated devices controlled from userspace below,
 	 * or use "driver_override" in sysfs
 	 */
-	{},
+	{}
 };
 MODULE_DEVICE_TABLE(of, gpio_aggregator_dt_ids);
 #endif
-- 
2.29.2