* [PATCH RFC v3] auxdisplay: charlcd: Fix and clean up handling of x/y commands
@ 2018-02-27 22:09 Miguel Ojeda
2018-02-27 22:23 ` Willy Tarreau
2018-02-27 23:23 ` Robert Abel
0 siblings, 2 replies; 4+ messages in thread
From: Miguel Ojeda @ 2018-02-27 22:09 UTC (permalink / raw)
To: w, geert, andy.shevchenko, rabel, linux-kernel
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 does two passes over
the string.
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>
---
Disregard v2, this is much more clear. I am too tired :)
This one and v2 are the loop versions, v1 is the kstrto*() version.
I also added some more comments on top of the v2.
drivers/auxdisplay/charlcd.c | 95 ++++++++++++++++++++++++++++++++++++--------
1 file changed, 78 insertions(+), 17 deletions(-)
diff --git a/drivers/auxdisplay/charlcd.c b/drivers/auxdisplay/charlcd.c
index 642afd88870b..36930bce5a1f 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,6 +293,79 @@ 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]+".
+ *
+ * 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).
+ * - "" fails.
+ * - "x" fails.
+ * - "x;" fails.
+ * - "x1" fails.
+ * - "xy12;" fails.
+ * - "x12yy12;" fails.
+ * - "xx" fails.
+ */
+static bool parse_xy(const char *s, unsigned long *x, unsigned long *y)
+{
+ unsigned long new_x = *x;
+ unsigned long new_y = *y;
+
+ for (;;) {
+ if (!*s)
+ return false;
+
+ if (*s == ';')
+ break;
+
+ if (*s == 'x') {
+ if (!parse_n(s + 1, &new_x, &s))
+ return false;
+ } else if (*s == 'y') {
+ if (!parse_n(s + 1, &new_y, &s))
+ return false;
+ } 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
@@ -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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH RFC v3] auxdisplay: charlcd: Fix and clean up handling of x/y commands
2018-02-27 22:09 [PATCH RFC v3] auxdisplay: charlcd: Fix and clean up handling of x/y commands Miguel Ojeda
@ 2018-02-27 22:23 ` Willy Tarreau
2018-02-27 23:23 ` Robert Abel
1 sibling, 0 replies; 4+ messages in thread
From: Willy Tarreau @ 2018-02-27 22:23 UTC (permalink / raw)
To: Miguel Ojeda; +Cc: geert, andy.shevchenko, rabel, linux-kernel
On Tue, Feb 27, 2018 at 11:09:52PM +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 does two passes over
> the string.
>
> Some explanations about the supported syntax are added as well.
Looks much better, thank you ;-)
Willy
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH RFC v3] auxdisplay: charlcd: Fix and clean up handling of x/y commands
2018-02-27 22:09 [PATCH RFC v3] auxdisplay: charlcd: Fix and clean up handling of x/y commands Miguel Ojeda
2018-02-27 22:23 ` Willy Tarreau
@ 2018-02-27 23:23 ` Robert Abel
2018-02-27 23:40 ` Miguel Ojeda
1 sibling, 1 reply; 4+ messages in thread
From: Robert Abel @ 2018-02-27 23:23 UTC (permalink / raw)
To: Miguel Ojeda, w, geert, andy.shevchenko, linux-kernel
On 27 Feb 2018 23:09, Miguel Ojeda wrote:> @@ -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;
Might want to keep this. It's in line with all other cases and prevents
calling parse_xy with input that has no chance of being correct due to
missing final ';'.
> + /* If the command is valid, move to the new address */
> + if (parse_xy(esc, &priv->addr.x, &priv->addr.y))
> + charlcd_gotoxy(lcd);
While not in the original code, the inputs are now not clamped to width
and height.
That means for a two-line display ^[[Ly02; will actually end up on line
y = 1, not y = 2 % 2 = 0, because the four-line display logic bumps the
address up.
The same goes for going over lcd->width/lcd->bwidth in the x coordinate.
For four-line displays that ends up going to the line y + 2, because the
buffer is split in the middle.
The distinction between lcd->width and lcd->bwidth depends on whether it
makes sense to put the cursor outside the visible area or not.
Regards,
Robert
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH RFC v3] auxdisplay: charlcd: Fix and clean up handling of x/y commands
2018-02-27 23:23 ` Robert Abel
@ 2018-02-27 23:40 ` Miguel Ojeda
0 siblings, 0 replies; 4+ messages in thread
From: Miguel Ojeda @ 2018-02-27 23:40 UTC (permalink / raw)
To: Robert Abel
Cc: Willy Tarreau, Geert Uytterhoeven, Andy Shevchenko, linux-kernel
On Wed, Feb 28, 2018 at 12:23 AM, Robert Abel <rabel@robertabel.eu> wrote:
> On 27 Feb 2018 23:09, Miguel Ojeda wrote:> @@ -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;
>
> Might want to keep this. It's in line with all other cases and prevents
> calling parse_xy with input that has no chance of being correct due to
> missing final ';'.
I agree performance is not that critical here and the string would
probably be in cache anyway, but I would say it is better to fix
parse_xy() if it is not doing what it specifies.
>
>> + /* If the command is valid, move to the new address */
>> + if (parse_xy(esc, &priv->addr.x, &priv->addr.y))
>> + charlcd_gotoxy(lcd);
>
> While not in the original code, the inputs are now not clamped to width
> and height.
> That means for a two-line display ^[[Ly02; will actually end up on line
> y = 1, not y = 2 % 2 = 0, because the four-line display logic bumps the
> address up.
> The same goes for going over lcd->width/lcd->bwidth in the x coordinate.
> For four-line displays that ends up going to the line y + 2, because the
> buffer is split in the middle.
> The distinction between lcd->width and lcd->bwidth depends on whether it
> makes sense to put the cursor outside the visible area or not.
Don't worry, the RFC patch is meant to agree on a fix for the multiple
x/y command code. The clamping and other stuff can still go in in
another patch of yours :)
Cheers,
Miguel
>
> Regards,
>
> Robert
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2018-02-27 23:40 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-02-27 22:09 [PATCH RFC v3] auxdisplay: charlcd: Fix and clean up handling of x/y commands Miguel Ojeda
2018-02-27 22:23 ` Willy Tarreau
2018-02-27 23:23 ` Robert Abel
2018-02-27 23:40 ` Miguel Ojeda
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).