[PATCH v2 1/2] kernel.h: Update comment about simple_strto<foo>() functions

[PATCH v2 1/2] kernel.h: Update comment about simple_strto<foo>() functions

[Andy Shevchenko]

There were discussions in the past about use cases for
simple_strto<foo>() functions and in some rare cases they have a benefit
on kstrto<foo>() ones.

Update a comment to reduce confusing about special use cases.

Suggested-by: Miguel Ojeda <miguel.ojeda.sandonis@gmail.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
- Cc to Andrew
 include/linux/kernel.h | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 74b1ee9027f5..e156b8b41d05 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -331,8 +331,7 @@ int __must_check kstrtoll(const char *s, unsigned int base, long long *res);
  * @res: Where to write the result of the conversion on success.
  *
  * Returns 0 on success, -ERANGE on overflow and -EINVAL on parsing error.
- * Used as a replacement for the obsolete simple_strtoull. Return code must
- * be checked.
+ * Used as a replacement for the simple_strtoull. Return code must be checked.
 */
 static inline int __must_check kstrtoul(const char *s, unsigned int base, unsigned long *res)
 {
@@ -360,8 +359,7 @@ static inline int __must_check kstrtoul(const char *s, unsigned int base, unsign
  * @res: Where to write the result of the conversion on success.
  *
  * Returns 0 on success, -ERANGE on overflow and -EINVAL on parsing error.
- * Used as a replacement for the obsolete simple_strtoull. Return code must
- * be checked.
+ * Used as a replacement for the simple_strtoull. Return code must be checked.
  */
 static inline int __must_check kstrtol(const char *s, unsigned int base, long *res)
 {
@@ -437,7 +435,15 @@ static inline int __must_check kstrtos32_from_user(const char __user *s, size_t
 	return kstrtoint_from_user(s, count, base, res);
 }
 
-/* Obsolete, do not use.  Use kstrto<foo> instead */
+/*
+ * Use kstrto<foo> instead.
+ *
+ * NOTE: The simple_strto<foo> does not check for overflow and,
+ *	 depending on the input, may give interesting results.
+ *
+ * Use these functions if and only if the code will need in place
+ * conversion and otherwise looks very ugly. Keep in mind above caveat.
+ */
 
 extern unsigned long simple_strtoul(const char *,char **,unsigned int);
 extern long simple_strtol(const char *,char **,unsigned int);
-- 
2.20.1

[PATCH v2 2/2] auxdisplay: charlcd: Deduplicate simple_strtoul()

[Andy Shevchenko]

Like in the commit
  8b2303de399f ("serial: core: Fix handling of options after MMIO address")
we may use simple_strtoul() which in comparison to kstrtoul() can do conversion
in-place without additional and unnecessary code to be written.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
- Cc to Andrew
- fix compilation issue
 drivers/auxdisplay/charlcd.c | 34 +++++++---------------------------
 1 file changed, 7 insertions(+), 27 deletions(-)

diff --git a/drivers/auxdisplay/charlcd.c b/drivers/auxdisplay/charlcd.c
index 92745efefb54..3858dc7a4154 100644
--- a/drivers/auxdisplay/charlcd.c
+++ b/drivers/auxdisplay/charlcd.c
@@ -287,31 +287,6 @@ static int charlcd_init_display(struct charlcd *lcd)
 	return 0;
 }
 
-/*
- * Parses an unsigned integer from a string, until a non-digit character
- * is found. The empty string is not accepted. No overflow checks are done.
- *
- * Returns whether the parsing was successful. Only in that case
- * the output parameters are written to.
- *
- * TODO: If the kernel adds an inplace version of kstrtoul(), this function
- * could be easily replaced by that.
- */
-static bool parse_n(const char *s, unsigned long *res, const char **next_s)
-{
-	if (!isdigit(*s))
-		return false;
-
-	*res = 0;
-	while (isdigit(*s)) {
-		*res = *res * 10 + (*s - '0');
-		++s;
-	}
-
-	*next_s = s;
-	return true;
-}
-
 /*
  * Parses a movement command of the form "(.*);", where the group can be
  * any number of subcommands of the form "(x|y)[0-9]+".
@@ -336,6 +311,7 @@ static bool parse_xy(const char *s, unsigned long *x, unsigned long *y)
 {
 	unsigned long new_x = *x;
 	unsigned long new_y = *y;
+	char *p;
 
 	for (;;) {
 		if (!*s)
@@ -345,11 +321,15 @@ static bool parse_xy(const char *s, unsigned long *x, unsigned long *y)
 			break;
 
 		if (*s == 'x') {
-			if (!parse_n(s + 1, &new_x, &s))
+			new_x = simple_strtoul(s + 1, &p, 10);
+			if (p == s + 1)
 				return false;
+			s = p;
 		} else if (*s == 'y') {
-			if (!parse_n(s + 1, &new_y, &s))
+			new_y = simple_strtoul(s + 1, &p, 10);
+			if (p == s + 1)
 				return false;
+			s = p;
 		} else {
 			return false;
 		}
-- 
2.20.1

Re: [PATCH v2 1/2] kernel.h: Update comment about simple_strto<foo>() functions

[Geert Uytterhoeven]

Hi Andy,

On Wed, Jun 26, 2019 at 11:39 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> There were discussions in the past about use cases for
> simple_strto<foo>() functions and in some rare cases they have a benefit
> on kstrto<foo>() ones.

over

> Update a comment to reduce confusing about special use cases.

confusion

> Suggested-by: Miguel Ojeda <miguel.ojeda.sandonis@gmail.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h

> @@ -437,7 +435,15 @@ static inline int __must_check kstrtos32_from_user(const char __user *s, size_t
>         return kstrtoint_from_user(s, count, base, res);
>  }
>
> -/* Obsolete, do not use.  Use kstrto<foo> instead */
> +/*
> + * Use kstrto<foo> instead.
> + *
> + * NOTE: The simple_strto<foo> does not check for overflow and,
> + *      depending on the input, may give interesting results.
> + *
> + * Use these functions if and only if the code will need in place
> + * conversion and otherwise looks very ugly. Keep in mind above caveat.

What do you mean by "in place conversion"?
The input buffer is const, and not modified by the callee.
Do you mean that these functions do not require NUL termination (just
after the number), and the characters making up the number don't have to
be copied to a separate buffer to make them NUL-terminated?

> + */
>
>  extern unsigned long simple_strtoul(const char *,char **,unsigned int);
>  extern long simple_strtol(const char *,char **,unsigned int);

Yeah, they're still very useful.

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 v2 1/2] kernel.h: Update comment about simple_strto<foo>() functions

[Andy Shevchenko]

On Wed, Jun 26, 2019 at 01:00:45PM +0200, Geert Uytterhoeven wrote:
> Hi Andy,
> 
> On Wed, Jun 26, 2019 at 11:39 AM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > There were discussions in the past about use cases for
> > simple_strto<foo>() functions and in some rare cases they have a benefit
> > on kstrto<foo>() ones.
> 
> over

Will fix.

> > Update a comment to reduce confusing about special use cases.
> 
> confusion

Will fix.

> > Suggested-by: Miguel Ojeda <miguel.ojeda.sandonis@gmail.com>
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> > --- a/include/linux/kernel.h
> > +++ b/include/linux/kernel.h
> 
> > @@ -437,7 +435,15 @@ static inline int __must_check kstrtos32_from_user(const char __user *s, size_t
> >         return kstrtoint_from_user(s, count, base, res);
> >  }
> >
> > -/* Obsolete, do not use.  Use kstrto<foo> instead */
> > +/*
> > + * Use kstrto<foo> instead.
> > + *
> > + * NOTE: The simple_strto<foo> does not check for overflow and,
> > + *      depending on the input, may give interesting results.
> > + *
> > + * Use these functions if and only if the code will need in place
> > + * conversion and otherwise looks very ugly. Keep in mind above caveat.
> 
> What do you mean by "in place conversion"?
> The input buffer is const, and not modified by the callee.
> Do you mean that these functions do not require NUL termination (just
> after the number), and the characters making up the number don't have to
> be copied to a separate buffer to make them NUL-terminated?

The second one, could you propose better wording for that?

> > + */
> >
> >  extern unsigned long simple_strtoul(const char *,char **,unsigned int);
> >  extern long simple_strtol(const char *,char **,unsigned int);
> 
> Yeah, they're still very useful.

Thanks for review.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/2] kernel.h: Update comment about simple_strto<foo>() functions

[Geert Uytterhoeven]

Hi Andy,

On Wed, Jul 3, 2019 at 4:37 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Wed, Jun 26, 2019 at 01:00:45PM +0200, Geert Uytterhoeven wrote:
> > On Wed, Jun 26, 2019 at 11:39 AM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > > --- a/include/linux/kernel.h
> > > +++ b/include/linux/kernel.h
> >
> > > @@ -437,7 +435,15 @@ static inline int __must_check kstrtos32_from_user(const char __user *s, size_t
> > >         return kstrtoint_from_user(s, count, base, res);
> > >  }
> > >
> > > -/* Obsolete, do not use.  Use kstrto<foo> instead */
> > > +/*
> > > + * Use kstrto<foo> instead.
> > > + *
> > > + * NOTE: The simple_strto<foo> does not check for overflow and,
> > > + *      depending on the input, may give interesting results.
> > > + *
> > > + * Use these functions if and only if the code will need in place
> > > + * conversion and otherwise looks very ugly. Keep in mind above caveat.
> >
> > What do you mean by "in place conversion"?
> > The input buffer is const, and not modified by the callee.
> > Do you mean that these functions do not require NUL termination (just
> > after the number), and the characters making up the number don't have to
> > be copied to a separate buffer to make them NUL-terminated?
>
> The second one, could you propose better wording for that?

What about:

"Use these functions if and only if you cannot use kstrto<foo>, because
 the number is not immediately followed by a NUL-character in the buffer.
 Keep in mind above caveat."

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