[PATCH RFC] auxdisplay: charlcd: Fix and clean up handling of x/y commands

[PATCH RFC] auxdisplay: charlcd: Fix and clean up handling of x/y commands

[Miguel Ojeda]

The current version is not parsing multiple x/y commands as the code
originally intended. On top of that, kstrtoul() expects
NULL-terminated strings. Finally, the code had to do two passes over
the string, while now only one is done.

Some explanations about the supported syntax are added as well.

Cc: Willy Tarreau <w@1wt.eu>
Cc: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: Robert Abel <rabel@robertabel.eu>
Signed-off-by: Miguel Ojeda <miguel.ojeda.sandonis@gmail.com>
---
Only the parsing functions are tested, please try on real HW.

 drivers/auxdisplay/charlcd.c | 97 ++++++++++++++++++++++++++++++++++++--------
 1 file changed, 79 insertions(+), 18 deletions(-)

diff --git a/drivers/auxdisplay/charlcd.c b/drivers/auxdisplay/charlcd.c
index 642afd88870b..aa9a6abc2ada 100644
--- a/drivers/auxdisplay/charlcd.c
+++ b/drivers/auxdisplay/charlcd.c
@@ -11,6 +11,7 @@
  */
 
 #include <linux/atomic.h>
+#include <linux/ctype.h>
 #include <linux/delay.h>
 #include <linux/fs.h>
 #include <linux/miscdevice.h>
@@ -292,13 +293,86 @@ static int charlcd_init_display(struct charlcd *lcd)
 	return 0;
 }
 
+/*
+ * Parses a base 10 number from a string, until a non-digit number is found or
+ * until PARSE_N_MAX_SIZE digits.
+ *
+ * See kstrtoul() for the meaning of the return value.
+ * It also returns the next character in the string, i.e. the first non-digit.
+ */
+#define PARSE_N_MAX_SIZE (3)
+static unsigned long parse_n(const char *s, unsigned long *res,
+	const char **next_s)
+{
+	char tmp[PARSE_N_MAX_SIZE + 1];
+	int i;
+
+	for (i = 0; ; ++i) {
+		if (!isdigit(s[i]))
+			break;
+		if (i >= PARSE_N_MAX_SIZE)
+			return -EINVAL;
+		tmp[i] = s[i];
+	}
+
+	tmp[i] = 0;
+	*next_s = &s[i];
+	return kstrtoul(tmp, 10, res);
+}
+
+/*
+ * Parses a movement command of the form "(.*);", where the group can be
+ * any number of subcommands of the form "(x|y)[0-9]+".
+ *
+ * Returns whether the command is valid. The position arguments are
+ * only written if the parsing was successful.
+ *
+ * For instance:
+ *   - ";"          returns (<original x>, <original y>).
+ *   - "x1;"        returns (1, <original y>).
+ *   - "y2x1;"      returns (1, 2).
+ *   - "x12y34x56;" returns (56, 34).
+ *   - "x1"         fails.
+ *   - "xy12;"      fails.
+ *   - "x12yy12;"   fails.
+ */
+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 *next_s;
+
+	for (;;) {
+		if (!*s)
+			return false;
+
+		if (*s == ';')
+			break;
+
+		if (*s == 'x') {
+			if (parse_n(s + 1, &new_x, &next_s) != 0)
+				return false;
+			s = next_s;
+		} else if (*s == 'y') {
+			if (parse_n(s + 1, &new_y, &next_s) != 0)
+				return false;
+			s = next_s;
+		} else {
+			return false;
+		}
+	}
+
+	*x = new_x;
+	*y = new_y;
+	return true;
+}
+
 /*
  * These are the file operation function for user access to /dev/lcd
  * This function can also be called from inside the kernel, by
  * setting file and ppos to NULL.
  *
  */
-
 static inline int handle_lcd_special_code(struct charlcd *lcd)
 {
 	struct charlcd_priv *priv = to_priv(lcd);
@@ -469,24 +543,11 @@ static inline int handle_lcd_special_code(struct charlcd *lcd)
 	}
 	case 'x':	/* gotoxy : LxXXX[yYYY]; */
 	case 'y':	/* gotoxy : LyYYY[xXXX]; */
-		if (!strchr(esc, ';'))
-			break;
-
-		while (*esc) {
-			if (*esc == 'x') {
-				esc++;
-				if (kstrtoul(esc, 10, &priv->addr.x) < 0)
-					break;
-			} else if (*esc == 'y') {
-				esc++;
-				if (kstrtoul(esc, 10, &priv->addr.y) < 0)
-					break;
-			} else {
-				break;
-			}
-		}
+		/* If the command is valid, move to the new address */
+		if (parse_xy(esc, &priv->addr.x, &priv->addr.y))
+			charlcd_gotoxy(lcd);
 
-		charlcd_gotoxy(lcd);
+		/* Regardless of its validity, mark as processed */
 		processed = 1;
 		break;
 	}
-- 
2.14.1

Re: [PATCH RFC] auxdisplay: charlcd: Fix and clean up handling of x/y commands

[Willy Tarreau]

On Tue, Feb 27, 2018 at 08:32:21PM +0100, Miguel Ojeda wrote:
> The current version is not parsing multiple x/y commands as the code
> originally intended. On top of that, kstrtoul() expects
> NULL-terminated strings. Finally, the code had to do two passes over
> the string, while now only one is done.
> 
> Some explanations about the supported syntax are added as well.

Thanks Miguel for doing this. To be fair, I personally think that the code
was significantly complexified compared to the original version, just for
the purpose of using one library function designed to multiply by ten and
add a number. Not to mention that it now requires a copy before parsing.
For me it simply means that the initial code change was a wrong idea in
the end. But at least with your version, the bugs introduced by the
previous "fix" should now be gone, which is a good thing.

Thanks,
Willy

Re: [PATCH RFC] auxdisplay: charlcd: Fix and clean up handling of x/y commands

[Andy Shevchenko]

On Tue, Feb 27, 2018 at 9:32 PM, Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
> The current version is not parsing multiple x/y commands as the code
> originally intended. On top of that, kstrtoul() expects
> NULL-terminated strings. Finally, the code had to do two passes over
> the string, while now only one is done.
>
> Some explanations about the supported syntax are added as well.

Something like this would work, but see my comments below.

> +/*
> + * Parses a base 10 number from a string, until a non-digit number is found or
> + * until PARSE_N_MAX_SIZE digits.
> + *
> + * See kstrtoul() for the meaning of the return value.
> + * It also returns the next character in the string, i.e. the first non-digit.
> + */
> +#define PARSE_N_MAX_SIZE (3)
> +static unsigned long parse_n(const char *s, unsigned long *res,
> +       const char **next_s)

static int parse_n(const char *s, unsigned long *res, const char **next_s)

> +{
> +       char tmp[PARSE_N_MAX_SIZE + 1];
> +       int i;
> +

First of all you need

unsigned int i = 0;

while (s[i] == '0')
 i++;

> +       for (i = 0; ; ++i) {
> +               if (!isdigit(s[i]))
> +                       break;
> +               if (i >= PARSE_N_MAX_SIZE)
> +                       return -EINVAL;
> +               tmp[i] = s[i];
> +       }

And here you partially open coded what simple_strto*() does.

> +
> +       tmp[i] = 0;
> +       *next_s = &s[i];
> +       return kstrtoul(tmp, 10, res);
> +}

It's over engineered. Just simple use simple_strto*() and that's all.
Do you really care about overflows? Is those numbers somehow critical?
What will go wrong?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH RFC] auxdisplay: charlcd: Fix and clean up handling of x/y commands

[Miguel Ojeda]

On Tue, Feb 27, 2018 at 9:13 PM, Willy Tarreau <w@1wt.eu> wrote:
> On Tue, Feb 27, 2018 at 08:32:21PM +0100, Miguel Ojeda wrote:
>> The current version is not parsing multiple x/y commands as the code
>> originally intended. On top of that, kstrtoul() expects
>> NULL-terminated strings. Finally, the code had to do two passes over
>> the string, while now only one is done.
>>
>> Some explanations about the supported syntax are added as well.
>
> Thanks Miguel for doing this. To be fair, I personally think that the code

You're welcome!

> was significantly complexified compared to the original version, just for
> the purpose of using one library function designed to multiply by ten and
> add a number. Not to mention that it now requires a copy before parsing.

I agree. I wrote this version like this so that parse_n() can be
removed in the future if we have access to an in-place alternative to
the kstrtoul*() functions (cf. the discussion in the other thread with
Andy); which would take the copy away as well.

> For me it simply means that the initial code change was a wrong idea in
> the end. But at least with your version, the bugs introduced by the
> previous "fix" should now be gone, which is a good thing.

Thanks! :-) This one has the "advantage" that it only goes by the
string once (i.e. no initial strchr()) and that gotoxy() is not called
unless the full command is valid (but I am not sure whether this is OK
or not for the HW, though).

Cheers,
Miguel

>
> Thanks,
> Willy

Re: [PATCH RFC] auxdisplay: charlcd: Fix and clean up handling of x/y commands

[Miguel Ojeda]

On Tue, Feb 27, 2018 at 9:52 PM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Tue, Feb 27, 2018 at 9:32 PM, Miguel Ojeda
> <miguel.ojeda.sandonis@gmail.com> wrote:
>> The current version is not parsing multiple x/y commands as the code
>> originally intended. On top of that, kstrtoul() expects
>> NULL-terminated strings. Finally, the code had to do two passes over
>> the string, while now only one is done.
>>
>> Some explanations about the supported syntax are added as well.
>
> Something like this would work, but see my comments below.
>
>> +/*
>> + * Parses a base 10 number from a string, until a non-digit number is found or
>> + * until PARSE_N_MAX_SIZE digits.
>> + *
>> + * See kstrtoul() for the meaning of the return value.
>> + * It also returns the next character in the string, i.e. the first non-digit.
>> + */
>> +#define PARSE_N_MAX_SIZE (3)
>> +static unsigned long parse_n(const char *s, unsigned long *res,
>> +       const char **next_s)
>
> static int parse_n(const char *s, unsigned long *res, const char **next_s)

Oops, thanks!

>
>> +{
>> +       char tmp[PARSE_N_MAX_SIZE + 1];
>> +       int i;
>> +
>
> First of all you need
>
> unsigned int i = 0;
>
> while (s[i] == '0')
>  i++;

Why?

>
>> +       for (i = 0; ; ++i) {
>> +               if (!isdigit(s[i]))
>> +                       break;
>> +               if (i >= PARSE_N_MAX_SIZE)
>> +                       return -EINVAL;
>> +               tmp[i] = s[i];
>> +       }
>
> And here you partially open coded what simple_strto*() does.
>
>> +
>> +       tmp[i] = 0;
>> +       *next_s = &s[i];
>> +       return kstrtoul(tmp, 10, res);
>> +}
>
> It's over engineered. Just simple use simple_strto*() and that's all.
> Do you really care about overflows? Is those numbers somehow critical?
> What will go wrong?

We shouldn't use simple_strto*() since it is deprecated. So parse_n()
can be either this wrapper to call kstrto*() (which indeed is
overengineered, but easier to understand than Robert's patch that
modified the original sequence, and can be replaced by an inplace
version whenever we have one) or we just put a simple loop for the
moment (like the one Willy coded in 2008). Both are fine with me.

Cheers,
Miguel

>
> --
> With Best Regards,
> Andy Shevchenko