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

[PATCH v1 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>
---
 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 v1 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>
---
 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..45e2e451253a 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;
+	const 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 v1 1/2] kernel.h: Update comment about simple_strto<foo>() functions

[Miguel Ojeda]

On Wed, Jun 19, 2019 at 6:38 PM 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.
>
> Update a comment to reduce confusing about special use cases.

I don't recall the discussions anymore... :-) But are we sure
simple_strtoul() etc. are not obsolete anymore and want to use them
again?

Cheers,
Miguel

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

[Andy Shevchenko]

On Thu, Jun 20, 2019 at 03:14:20PM +0200, Miguel Ojeda wrote:
> On Wed, Jun 19, 2019 at 6:38 PM 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.
> >
> > Update a comment to reduce confusing about special use cases.
> 
> I don't recall the discussions anymore... :-) But are we sure
> simple_strtoul() etc. are not obsolete anymore and want to use them
> again?

As I'm explaining there, making them obsolete without providing an alternative
was a not the best move. So, until we have no alternative and, as I pointed
out, we see the patches moving back to simple_strto*() from kstrto*(),
simple_strto*() may be used in some corner cases.

The code in charlcd.c shows a down side of people taking that "obsolete" word
too seriously. Instead of one old good function we have to replicate it many
times.

P.S. Despite the whatever decision will be made on this patch, the second one
makes sense on its own.

-- 
With Best Regards,
Andy Shevchenko