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

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

[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 <armand.bastien@laposte.net>
---
 drivers/staging/panel/panel.c |  205 ++++++++++++++++++++++-------------------
 1 file changed, 109 insertions(+), 96 deletions(-)

diff --git a/drivers/staging/panel/panel.c b/drivers/staging/panel/panel.c
index 1569e26..dc34254 100644
--- a/drivers/staging/panel/panel.c
+++ b/drivers/staging/panel/panel.c
@@ -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

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

[Dan Carpenter]

Out of curiosity, have you tested this patch?

On Fri, Apr 18, 2014 at 06:10:57PM +0200, Bastien Armand wrote:
> 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 <armand.bastien@laposte.net>
> ---
>  drivers/staging/panel/panel.c |  205 ++++++++++++++++++++++-------------------
>  1 file changed, 109 insertions(+), 96 deletions(-)
> 
> diff --git a/drivers/staging/panel/panel.c b/drivers/staging/panel/panel.c
> index 1569e26..dc34254 100644
> --- a/drivers/staging/panel/panel.c
> +++ b/drivers/staging/panel/panel.c
> @@ -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;

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.


> 
> -		/* 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();

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

> +
> +			lcd_write_char(*tmp);
> +		}
> +	}
>  }
> 
>  /* initialize the LCD driver */
> --
> 1.7.10.4
> 
> _______________________________________________
> devel mailing list
> devel@linuxdriverproject.org
> http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Bastien Armand]

On Tue, Apr 22, 2014 at 01:01:45PM +0300, Dan Carpenter wrote:
> Out of curiosity, have you tested this patch?
> 
> On Fri, Apr 18, 2014 at 06:10:57PM +0200, Bastien Armand wrote:
> > 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 <armand.bastien@laposte.net>
> > ---
> >  drivers/staging/panel/panel.c |  205 ++++++++++++++++++++++-------------------
> >  1 file changed, 109 insertions(+), 96 deletions(-)
> > 
> > diff --git a/drivers/staging/panel/panel.c b/drivers/staging/panel/panel.c
> > index 1569e26..dc34254 100644
> > --- a/drivers/staging/panel/panel.c
> > +++ b/drivers/staging/panel/panel.c
> > @@ -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:
> 
> 		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.

> 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.

> > ...
> >  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();
> 
> 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.

> regards,
> dan carpenter
> 

Thanks for your review. It was really appreciated.

Regards,
bastien armand

> > +
> > +			lcd_write_char(*tmp);
> > +		}
> > +	}
> >  }
> > 
> >  /* initialize the LCD driver */
> > --
> > 1.7.10.4
> > 
> > _______________________________________________
> > devel mailing list
> > devel@linuxdriverproject.org
> > http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Dan Carpenter]

On Wed, Apr 23, 2014 at 07:35:02PM +0200, Bastien Armand wrote:
> 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

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

[Willy Tarreau]

On Wed, Apr 23, 2014 at 07:35:02PM +0200, Bastien Armand wrote:
> On Tue, Apr 22, 2014 at 01:01:45PM +0300, Dan Carpenter wrote:
> > 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.

> > > +	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.
> > 
> 
> 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