Skip to contentSkip to conversation

staging: panel: fix sparse warnings in lcd_write

PATCH In review Bastien Armand 3 participants 5 messages Apr 18, 2014 – Apr 25, 2014 Staging

lore
Share & tools
lore

Bastien Armand

author
#0 lore raw

PATCH v2 2/2 staging: panel: fix sparse warnings in lcd_write

This patch fixes two sparse warnings related to lcd_write :
warning: incorrect type in argument 1 (different address spaces)
warning: incorrect type in initializer (incompatible argument 2 (different address spaces))

lcd_write can be used from kernel space (in panel_lcd_print) or from user space. So we introduce the lcd_write_char function that will write a char to the device. We modify lcd_write and panel_lcd_print to use it. Finally we add __user annotation missing in lcd_write.

Signed-off-by Bastien Armand <[email protected]>
---
 drivers/staging/panel/panel.c |  205 ++++++++++++++++++++++-------------------
 1 file changed, 109 insertions(+), 96 deletions(-)
drivers/staging/panel/panel.c +109 -96
index 1569e26..dc34254 100644
@@ -1217,111 +1217,113 @@ static inline int handle_lcd_special_code(void)
 	return processed;
 }
 
+static void lcd_write_char(char c)
+{
+	/* first, we'll test if we're in escape mode */
+	if ((c != '\n') && lcd_escape_len >= 0) {
+		/* yes, let's add this char to the buffer */
+		lcd_escape[lcd_escape_len++] = c;
+		lcd_escape[lcd_escape_len] = 0;
+	} else {
+		/* aborts any previous escape sequence */
+		lcd_escape_len = -1;
+
+		switch (c) {
+		case LCD_ESCAPE_CHAR:
+			/* start of an escape sequence */
+			lcd_escape_len = 0;
+			lcd_escape[lcd_escape_len] = 0;
+			break;
+		case '\b':
+			/* go back one char and clear it */
+			if (lcd_addr_x > 0) {
+				/* check if we're not at the
+				   end of the line */
+				if (lcd_addr_x < lcd_bwidth)
+					/* back one char */
+					lcd_write_cmd(0x10);
+				lcd_addr_x--;
+			}
+			/* replace with a space */
+			lcd_write_data(' ');
+			/* back one char again */
+			lcd_write_cmd(0x10);
+			break;
+		case '\014':
+			/* quickly clear the display */
+			lcd_clear_fast();
+			break;
+		case '\n':
+			/* flush the remainder of the current line and
+			   go to the beginning of the next line */
+			for (; lcd_addr_x < lcd_bwidth; lcd_addr_x++)
+				lcd_write_data(' ');
+			lcd_addr_x = 0;
+			lcd_addr_y = (lcd_addr_y + 1) % lcd_height;
+			lcd_gotoxy();
+			break;
+		case '\r':
+			/* go to the beginning of the same line */
+			lcd_addr_x = 0;
+			lcd_gotoxy();
+			break;
+		case '\t':
+			/* print a space instead of the tab */
+			lcd_print(' ');
+			break;
+		default:
+			/* simply print this char */
+			lcd_print(c);
+			break;
+		}
+	}
+
+	/* now we'll see if we're in an escape mode and if the current
+	   escape sequence can be understood. */
+	if (lcd_escape_len >= 2) {
+		int processed = 0;
+
+		if (!strcmp(lcd_escape, "[2J")) {
+			/* clear the display */
+			lcd_clear_fast();
+			processed = 1;
+		} else if (!strcmp(lcd_escape, "[H")) {
+			/* cursor to home */
+			lcd_addr_x = lcd_addr_y = 0;
+			lcd_gotoxy();
+			processed = 1;
+		}
+		/* codes starting with ^[[L */
+		else if ((lcd_escape_len >= 3) &&
+			 (lcd_escape[0] == '[') &&
+			 (lcd_escape[1] == 'L')) {
+			processed = handle_lcd_special_code();
+		}
+
+		/* LCD special escape codes */
+		/* flush the escape sequence if it's been processed
+		   or if it is getting too long. */
+		if (processed || (lcd_escape_len >= LCD_ESCAPE_LEN))
+			lcd_escape_len = -1;
+	} /* escape codes */
+}
+
 static ssize_t lcd_write(struct file *file,
-			 const char *buf, size_t count, loff_t *ppos)
+	const char __user *buf, size_t count, loff_t *ppos)
 {
-	const char *tmp = buf;
+	const char __user *tmp = buf;
 	char c;
 
-	for (; count-- > 0; (ppos ? (*ppos)++ : 0), ++tmp) {
+	for (; count-- > 0; (*ppos)++, tmp++) {
 		if (!in_interrupt() && (((count + 1) & 0x1f) == 0))
 			/* let's be a little nice with other processes
 			   that need some CPU */
 			schedule();
 
-		if (ppos == NULL && file == NULL)
-			/* let's not use get_user() from the kernel ! */
-			c = *tmp;
-		else if (get_user(c, tmp))
+		if (get_user(c, buf))
 			return -EFAULT;
 
-		/* first, we'll test if we're in escape mode */
-		if ((c != '\n') && lcd_escape_len >= 0) {
-			/* yes, let's add this char to the buffer */
-			lcd_escape[lcd_escape_len++] = c;
-			lcd_escape[lcd_escape_len] = 0;
-		} else {
-			/* aborts any previous escape sequence */
-			lcd_escape_len = -1;
-
-			switch (c) {
-			case LCD_ESCAPE_CHAR:
-				/* start of an escape sequence */
-				lcd_escape_len = 0;
-				lcd_escape[lcd_escape_len] = 0;
-				break;
-			case '\b':
-				/* go back one char and clear it */
-				if (lcd_addr_x > 0) {
-					/* check if we're not at the
-					   end of the line */
-					if (lcd_addr_x < lcd_bwidth)
-						/* back one char */
-						lcd_write_cmd(0x10);
-					lcd_addr_x--;
-				}
-				/* replace with a space */
-				lcd_write_data(' ');
-				/* back one char again */
-				lcd_write_cmd(0x10);
-				break;
-			case '\014':
-				/* quickly clear the display */
-				lcd_clear_fast();
-				break;
-			case '\n':
-				/* flush the remainder of the current line and
-				   go to the beginning of the next line */
-				for (; lcd_addr_x < lcd_bwidth; lcd_addr_x++)
-					lcd_write_data(' ');
-				lcd_addr_x = 0;
-				lcd_addr_y = (lcd_addr_y + 1) % lcd_height;
-				lcd_gotoxy();
-				break;
-			case '\r':
-				/* go to the beginning of the same line */
-				lcd_addr_x = 0;
-				lcd_gotoxy();
-				break;
-			case '\t':
-				/* print a space instead of the tab */
-				lcd_print(' ');
-				break;
-			default:
-				/* simply print this char */
-				lcd_print(c);
-				break;
-			}
-		}
-
-		/* now we'll see if we're in an escape mode and if the current
-		   escape sequence can be understood. */
-		if (lcd_escape_len >= 2) {
-			int processed = 0;
-
-			if (!strcmp(lcd_escape, "[2J")) {
-				/* clear the display */
-				lcd_clear_fast();
-				processed = 1;
-			} else if (!strcmp(lcd_escape, "[H")) {
-				/* cursor to home */
-				lcd_addr_x = lcd_addr_y = 0;
-				lcd_gotoxy();
-				processed = 1;
-			}
-			/* codes starting with ^[[L */
-			else if ((lcd_escape_len >= 3) &&
-				 (lcd_escape[0] == '[') &&
-				 (lcd_escape[1] == 'L')) {
-				processed = handle_lcd_special_code();
-			}
-
-			/* LCD special escape codes */
-			/* flush the escape sequence if it's been processed
-			   or if it is getting too long. */
-			if (processed || (lcd_escape_len >= LCD_ESCAPE_LEN))
-				lcd_escape_len = -1;
-		} /* escape codes */
+		lcd_write_char(c);
 	}
 
 	return tmp - buf;

@@ -1365,8 +1367,19 @@ static struct miscdevice lcd_dev = {
 /* public function usable from the kernel for any purpose */
 static void panel_lcd_print(const char *s)
 {
-	if (lcd_enabled && lcd_initialized)
-		lcd_write(NULL, s, strlen(s), NULL);
+	const char *tmp = s;
+	int count = strlen(s);
+
+	if (lcd_enabled && lcd_initialized) {
+		for (; count-- > 0; tmp++) {
+			if (!in_interrupt() && (((count + 1) & 0x1f) == 0))
+				/* let's be a little nice with other processes
+				   that need some CPU */
+				schedule();
+
+			lcd_write_char(*tmp);
+		}
+	}
 }
 
 /* initialize the LCD driver */

--
1.7.10.4

#1 lore raw

Out of curiosity, have you tested this patch?

Quoted 126 lines · Bastien Armand

This patch fixes two sparse warnings related to lcd_write :
warning: incorrect type in argument 1 (different address spaces)
warning: incorrect type in initializer (incompatible argument 2 (different address spaces))

lcd_write can be used from kernel space (in panel_lcd_print) or from user space. So we introduce the lcd_write_char function that will write a char to the device. We modify lcd_write and panel_lcd_print to use it. Finally we add __user annotation missing in lcd_write.

Signed-off-by Bastien Armand <[email protected]>
---
 drivers/staging/panel/panel.c |  205 ++++++++++++++++++++++-------------------
 1 file changed, 109 insertions(+), 96 deletions(-)
drivers/staging/panel/panel.c +94 -3
index 1569e26..dc34254 100644
@@ -1217,111 +1217,113 @@ static inline int handle_lcd_special_code(void)
 	return processed;
 }
 
+static void lcd_write_char(char c)
+{
+	/* first, we'll test if we're in escape mode */
+	if ((c != '\n') && lcd_escape_len >= 0) {
+		/* yes, let's add this char to the buffer */
+		lcd_escape[lcd_escape_len++] = c;
+		lcd_escape[lcd_escape_len] = 0;
+	} else {
+		/* aborts any previous escape sequence */
+		lcd_escape_len = -1;
+
+		switch (c) {
+		case LCD_ESCAPE_CHAR:
+			/* start of an escape sequence */
+			lcd_escape_len = 0;
+			lcd_escape[lcd_escape_len] = 0;
+			break;
+		case '\b':
+			/* go back one char and clear it */
+			if (lcd_addr_x > 0) {
+				/* check if we're not at the
+				   end of the line */
+				if (lcd_addr_x < lcd_bwidth)
+					/* back one char */
+					lcd_write_cmd(0x10);
+				lcd_addr_x--;
+			}
+			/* replace with a space */
+			lcd_write_data(' ');
+			/* back one char again */
+			lcd_write_cmd(0x10);
+			break;
+		case '\014':
+			/* quickly clear the display */
+			lcd_clear_fast();
+			break;
+		case '\n':
+			/* flush the remainder of the current line and
+			   go to the beginning of the next line */
+			for (; lcd_addr_x < lcd_bwidth; lcd_addr_x++)
+				lcd_write_data(' ');
+			lcd_addr_x = 0;
+			lcd_addr_y = (lcd_addr_y + 1) % lcd_height;
+			lcd_gotoxy();
+			break;
+		case '\r':
+			/* go to the beginning of the same line */
+			lcd_addr_x = 0;
+			lcd_gotoxy();
+			break;
+		case '\t':
+			/* print a space instead of the tab */
+			lcd_print(' ');
+			break;
+		default:
+			/* simply print this char */
+			lcd_print(c);
+			break;
+		}
+	}
+
+	/* now we'll see if we're in an escape mode and if the current
+	   escape sequence can be understood. */
+	if (lcd_escape_len >= 2) {
+		int processed = 0;
+
+		if (!strcmp(lcd_escape, "[2J")) {
+			/* clear the display */
+			lcd_clear_fast();
+			processed = 1;
+		} else if (!strcmp(lcd_escape, "[H")) {
+			/* cursor to home */
+			lcd_addr_x = lcd_addr_y = 0;
+			lcd_gotoxy();
+			processed = 1;
+		}
+		/* codes starting with ^[[L */
+		else if ((lcd_escape_len >= 3) &&
+			 (lcd_escape[0] == '[') &&
+			 (lcd_escape[1] == 'L')) {
+			processed = handle_lcd_special_code();
+		}
+
+		/* LCD special escape codes */
+		/* flush the escape sequence if it's been processed
+		   or if it is getting too long. */
+		if (processed || (lcd_escape_len >= LCD_ESCAPE_LEN))
+			lcd_escape_len = -1;
+	} /* escape codes */
+}
+
 static ssize_t lcd_write(struct file *file,
-			 const char *buf, size_t count, loff_t *ppos)
+	const char __user *buf, size_t count, loff_t *ppos)
 {
-	const char *tmp = buf;
+	const char __user *tmp = buf;
 	char c;
 
-	for (; count-- > 0; (ppos ? (*ppos)++ : 0), ++tmp) {
+	for (; count-- > 0; (*ppos)++, tmp++) {
 		if (!in_interrupt() && (((count + 1) & 0x1f) == 0))

drivers/staging/panel/panel.c static inline int handle_lcd_special_code(void) · line 1324

Quoted 10 lines
drivers/staging/panel/panel.c +1 -4
 			/* let's be a little nice with other processes
 			   that need some CPU */
 			schedule();
 
-		if (ppos == NULL && file == NULL)
-			/* let's not use get_user() from the kernel ! */
-			c = *tmp;
-		else if (get_user(c, tmp))
+		if (get_user(c, buf))
 			return -EFAULT;

This is buggy. You are getting the first character over and over. You should be doing:

		if (get_user(c, tmp))
			return -EFAULT;

Btw, this whole function is terrible. It should be reading larger chunks at once instead of get_user() for each character.

Quoted 97 lines
drivers/staging/panel/panel.c +1 -88
-		/* first, we'll test if we're in escape mode */
-		if ((c != '\n') && lcd_escape_len >= 0) {
-			/* yes, let's add this char to the buffer */
-			lcd_escape[lcd_escape_len++] = c;
-			lcd_escape[lcd_escape_len] = 0;
-		} else {
-			/* aborts any previous escape sequence */
-			lcd_escape_len = -1;
-
-			switch (c) {
-			case LCD_ESCAPE_CHAR:
-				/* start of an escape sequence */
-				lcd_escape_len = 0;
-				lcd_escape[lcd_escape_len] = 0;
-				break;
-			case '\b':
-				/* go back one char and clear it */
-				if (lcd_addr_x > 0) {
-					/* check if we're not at the
-					   end of the line */
-					if (lcd_addr_x < lcd_bwidth)
-						/* back one char */
-						lcd_write_cmd(0x10);
-					lcd_addr_x--;
-				}
-				/* replace with a space */
-				lcd_write_data(' ');
-				/* back one char again */
-				lcd_write_cmd(0x10);
-				break;
-			case '\014':
-				/* quickly clear the display */
-				lcd_clear_fast();
-				break;
-			case '\n':
-				/* flush the remainder of the current line and
-				   go to the beginning of the next line */
-				for (; lcd_addr_x < lcd_bwidth; lcd_addr_x++)
-					lcd_write_data(' ');
-				lcd_addr_x = 0;
-				lcd_addr_y = (lcd_addr_y + 1) % lcd_height;
-				lcd_gotoxy();
-				break;
-			case '\r':
-				/* go to the beginning of the same line */
-				lcd_addr_x = 0;
-				lcd_gotoxy();
-				break;
-			case '\t':
-				/* print a space instead of the tab */
-				lcd_print(' ');
-				break;
-			default:
-				/* simply print this char */
-				lcd_print(c);
-				break;
-			}
-		}
-
-		/* now we'll see if we're in an escape mode and if the current
-		   escape sequence can be understood. */
-		if (lcd_escape_len >= 2) {
-			int processed = 0;
-
-			if (!strcmp(lcd_escape, "[2J")) {
-				/* clear the display */
-				lcd_clear_fast();
-				processed = 1;
-			} else if (!strcmp(lcd_escape, "[H")) {
-				/* cursor to home */
-				lcd_addr_x = lcd_addr_y = 0;
-				lcd_gotoxy();
-				processed = 1;
-			}
-			/* codes starting with ^[[L */
-			else if ((lcd_escape_len >= 3) &&
-				 (lcd_escape[0] == '[') &&
-				 (lcd_escape[1] == 'L')) {
-				processed = handle_lcd_special_code();
-			}
-
-			/* LCD special escape codes */
-			/* flush the escape sequence if it's been processed
-			   or if it is getting too long. */
-			if (processed || (lcd_escape_len >= LCD_ESCAPE_LEN))
-				lcd_escape_len = -1;
-		} /* escape codes */
+		lcd_write_char(c);
 	}
 
 	return tmp - buf;

@@ -1365,8 +1367,19 @@ static struct miscdevice lcd_dev = {
 /* public function usable from the kernel for any purpose */
 static void panel_lcd_print(const char *s)
 {
-	if (lcd_enabled && lcd_initialized)

drivers/staging/panel/panel.c static struct miscdevice lcd_dev = { · line 1378

Quoted 10 lines
drivers/staging/panel/panel.c +9 -1
-		lcd_write(NULL, s, strlen(s), NULL);
+	const char *tmp = s;
+	int count = strlen(s);
+
+	if (lcd_enabled && lcd_initialized) {
+		for (; count-- > 0; tmp++) {
+			if (!in_interrupt() && (((count + 1) & 0x1f) == 0))
+				/* let's be a little nice with other processes
+				   that need some CPU */
+				schedule();

This schedule() isn't needed. It just prints a line or two at start up and some other debug output or something. Small small.

regards,
dan carpenter

Quoted 14 lines
drivers/staging/panel/panel.c +4 -1
+
+			lcd_write_char(*tmp);
+		}
+	}
 }
 
 /* initialize the LCD driver */
--

1.7.10.4

_______________________________________________
devel mailing list
[email protected]
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

author
#2 lore raw
Quoted 31 lines · Dan Carpenter

Out of curiosity, have you tested this patch?

Quoted 21 lines · Bastien Armand

This patch fixes two sparse warnings related to lcd_write :
warning: incorrect type in argument 1 (different address spaces)
warning: incorrect type in initializer (incompatible argument 2 (different address spaces))

lcd_write can be used from kernel space (in panel_lcd_print) or from user space. So we introduce the lcd_write_char function that will write a char to the device. We modify lcd_write and panel_lcd_print to use it. Finally we add __user annotation missing in lcd_write.

Signed-off-by Bastien Armand <[email protected]>
---
 drivers/staging/panel/panel.c |  205 ++++++++++++++++++++++-------------------
 1 file changed, 109 insertions(+), 96 deletions(-)
drivers/staging/panel/panel.c +0 -0
index 1569e26..dc34254 100644

drivers/staging/panel/panel.c static inline int handle_lcd_special_code(void) · line 1218

Quoted 3 lines
drivers/staging/panel/panel.c +0 -0
@@ -1217,111 +1217,113 @@ static inline int handle_lcd_special_code(void)
 	return processed;
 }
...

This is buggy. You are getting the first character over and over. You should be doing:

Quoted 3 lines
		if (get_user(c, tmp))
			return -EFAULT;

Hi,

You're totally right. I am sorry to have missed that... As the patch has been accepted in linux-next. I will send a correction right away.

Quoted 4 lines

Btw, this whole function is terrible. It should be reading larger chunks at once instead of get_user() for each character.

The aim of my patch was basically to add __user annotation. I tried to keep the change minimal to lessen the risk of regression (it seems I have failed here...). Perhaps, I can write an other patch to change that.

Quoted 9 lines
Quoted 9 lines

...
static void panel_lcd_print(const char *s)
{

drivers/staging/panel/panel.c +4 -2
-	if (lcd_enabled && lcd_initialized)
-		lcd_write(NULL, s, strlen(s), NULL);
+	const char *tmp = s;
+	int count = strlen(s);
+
+	if (lcd_enabled && lcd_initialized) {
Quoted 9 lines

drivers/staging/panel/panel.c static struct miscdevice lcd_dev = { · line 1378

Quoted 5 lines
drivers/staging/panel/panel.c +5 -0
+		for (; count-- > 0; tmp++) {
+			if (!in_interrupt() && (((count + 1) & 0x1f) == 0))
+				/* let's be a little nice with other processes
+				   that need some CPU */
+				schedule();

This schedule() isn't needed. It just prints a line or two at start up and some other debug output or something. Small small.

I hesitated to remove it. I leave it here as it was allready in lcd_write. Perhaps, I could send another patch to remove it.

Quoted 3 lines

regards,
dan carpenter

Thanks for your review. It was really appreciated.

Regards,
bastien armand

Quoted 14 lines
Quoted 14 lines
drivers/staging/panel/panel.c +4 -1
+
+			lcd_write_char(*tmp);
+		}
+	}
 }
 
 /* initialize the LCD driver */
--

1.7.10.4

_______________________________________________
devel mailing list
[email protected]
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

#3 lore raw
Quoted 3 lines · Bastien Armand

On Tue, Apr 22, 2014 at 01:01:45PM +0300, Dan Carpenter wrote:
The aim of my patch was basically to add __user annotation. I tried to keep the change minimal to lessen the risk of regression

Yes. That's the right approach. These comments are just a free bonus in case you want to do more work on this. Or you can ignore if you don't want to. It's not something you introduced so it's not your responsibility.

regards,
dan carpenter

#4 lore raw
In reply to Bastien Armand
Quoted 3 lines · Bastien Armand
Quoted 2 lines · Dan Carpenter

Btw, this whole function is terrible. It should be reading larger chunks at once instead of get_user() for each character.

Just for the record, very small character counts may be sent to an LCD panel, in general these are 2 lines of 16 characters, and at most 2x40, and changes are very rare. The worst case will be if someone displays the time of day, it will change every second.

Quoted 11 lines
Quoted 10 lines

drivers/staging/panel/panel.c static struct miscdevice lcd_dev = { · line 1378

Quoted 6 lines
drivers/staging/panel/panel.c +6 -0
+	if (lcd_enabled && lcd_initialized) {
+		for (; count-- > 0; tmp++) {
+			if (!in_interrupt() && (((count + 1) & 0x1f) == 0))
+				/* let's be a little nice with other processes
+				   that need some CPU */
+				schedule();

This schedule() isn't needed. It just prints a line or two at start up and some other debug output or something. Small small.

Quoted 2 lines

I hesitated to remove it. I leave it here as it was allready in lcd_write. Perhaps, I could send another patch to remove it.

I believe it can go. I have some memories of it when I was developing the driver because I didn't know if some LCDs would need long pauses between each character. Hmmm well, thinking about it after re-reading the code, we could wait up to 20+40+120 = 180 microseconds when sending one command (eg: position change), or 20+40+45 = 105 microseconds when sending one char. That's basically 180+40*105 = 4.185 milliseconds for one full line, or 8 milliseconds for two lines of 40 chars. So maybe we should keep the schedule() in the end...

Best regards,
Willy