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

[Bastien Armand <armand.bastien@laposte.net>]

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