Re: [PATCH v3] drivers/staging: refactor dgnc tty registration.

[Haim Daniel]

me@haim-toshiba1 ~ $ dpkg -l sparse
Desired=Unknown/Install/Remove/Purge/Hold
| 
Status=Not/Inst/Conf-files/Unpacked/halF-conf/Half-inst/trig-aWait/Trig-pend
|/ Err?=(none)/Reinst-required (Status,Err: uppercase=bad)
||/ Name              Version       Architecture  Description
+++-=================-=============-=============-========================================
ii  sparse            0.4.5~rc1-1   amd64         semantic parser of 
source files

me@haim-toshiba1 linux (next-20170512) $ make clean M=drivers/staging 
/dgnc/; make C=1 M=drivers/staging/dgnc/

drivers/staging/dgnc/dgnc_tty.c:66:25: warning: too long 
initializer-string for array of char

On 05/15/2017 03:52 PM, Dan Carpenter wrote:
> On Mon, May 15, 2017 at 03:30:50PM +0300, Haim Daniel wrote:
> > -remove duplicate tty allocation code for serial and printer drivers.
> > -fix sparse warning: too long initializer-string for array of char.
>
> I think my version of Sparse is too old.  I don't get a warning.  Please
> cut and paste the exact warning.
>
> > Signed-off-by: Haim Daniel <haimdaniel@xxxxxxxxx>
> > ---
> >  drivers/staging/dgnc/dgnc_driver.h |  13 ----
> >  drivers/staging/dgnc/dgnc_tty.c    | 150 +++++++++++++++----------------------
> >  2 files changed, 59 insertions(+), 104 deletions(-)
> >
> > diff --git a/drivers/staging/dgnc/dgnc_driver.h b/drivers/staging/dgnc/dgnc_driver.h
> > index 980410f..764d6fe 100644
> > --- a/drivers/staging/dgnc/dgnc_driver.h
> > +++ b/drivers/staging/dgnc/dgnc_driver.h
> > @@ -52,19 +52,6 @@
> >
> >  #define dgnc_jiffies_from_ms(a) (((a) * HZ) / 1000)
> >
> > -/*
> > - * Define a local default termios struct. All ports will be created
> > - * with this termios initially.  This is the same structure that is defined
> > - * as the default in tty_io.c with the same settings overridden as in serial.c
> > - *
> > - * In short, this should match the internal serial ports' defaults.
> > - */
> > -#define	DEFAULT_IFLAGS	(ICRNL | IXON)
> > -#define	DEFAULT_OFLAGS	(OPOST | ONLCR)
> > -#define	DEFAULT_CFLAGS	(B9600 | CS8 | CREAD | HUPCL | CLOCAL)
> > -#define	DEFAULT_LFLAGS	(ISIG | ICANON | ECHO | ECHOE | ECHOK | \
> > -			ECHOCTL | ECHOKE | IEXTEN)
> > -
> >  #ifndef _POSIX_VDISABLE
> >  #define   _POSIX_VDISABLE '\0'
> >  #endif
> > diff --git a/drivers/staging/dgnc/dgnc_tty.c b/drivers/staging/dgnc/dgnc_tty.c
> > index 9e98781..d3736da 100644
> > --- a/drivers/staging/dgnc/dgnc_tty.c
> > +++ b/drivers/staging/dgnc/dgnc_tty.c
> > @@ -51,22 +51,6 @@
> >  	.digi_term =	"ansi"		/* default terminal type */
> >  };
> >
> > -/*
> > - * Define a local default termios struct. All ports will be created
> > - * with this termios initially.
> > - *
> > - * This defines a raw port at 9600 baud, 8 data bits, no parity,
> > - * 1 stop bit.
> > - */
> > -static const struct ktermios default_termios = {
> > -	.c_iflag =	(DEFAULT_IFLAGS),
> > -	.c_oflag =	(DEFAULT_OFLAGS),
> > -	.c_cflag =	(DEFAULT_CFLAGS),
> > -	.c_lflag =	(DEFAULT_LFLAGS),
> > -	.c_cc =		INIT_C_CC,
>
> We don't need INIT_C_CC?
No, the idea was reusing the tty_std_termios, and setting only the 
relevant params.

> > -	.c_line =	0,
> > -};
> > -
> >  static int dgnc_tty_open(struct tty_struct *tty, struct file *file);
> >  static void dgnc_tty_close(struct tty_struct *tty, struct file *file);
> >  static int dgnc_block_til_ready(struct tty_struct *tty, struct file *file,
> > @@ -129,6 +113,49 @@ static void dgnc_tty_set_termios(struct tty_struct *tty,
> >
> >  /* TTY Initialization/Cleanup Functions */
> >
> > +static struct tty_driver *dgnc_tty_create(char *serial_name, uint maxports,
> > +					  int major, int minor)
> > +{
> > +	int rc;
> > +	struct tty_driver *drv;
> > +
> > +	drv = tty_alloc_driver(maxports,
> > +			       TTY_DRIVER_REAL_RAW |
> > +			       TTY_DRIVER_DYNAMIC_DEV |
> > +			       TTY_DRIVER_HARDWARE_BREAK);
> > +	if (IS_ERR(drv))
> > +		return drv;
> > +
> > +	drv->name = serial_name;
> > +	drv->name_base = 0;
> > +	drv->major = major;
> > +	drv->minor_start = minor;
> > +	drv->type = TTY_DRIVER_TYPE_SERIAL;
> > +	drv->subtype = SERIAL_TYPE_NORMAL;
> > +	drv->init_termios = tty_std_termios;
> > +	drv->init_termios.c_cflag = (B9600 | CS8 | CREAD | HUPCL | CLOCAL);
> > +	drv->init_termios.c_ispeed = 9600;
> > +	drv->init_termios.c_ospeed = 9600;
>
> Setting c_ispeed and c_ospeed is almost certainly correct, but they're
> not mentioned in the changelog.
Ack, will do.
> Btw, the overwhelming vast majority of staging drivers are sent without
> testing.  But it looks like maybe you are testing yours?  Please mention
> that in the changelog because it makes us feel warm inside.
I only unit tested a fraction of the code, will mention it in the 
changelog, for the sake of that fuzzy feeling :)

> Otherwise it looks fine to me.  Sorry for making you redo it so many
> times instead of reviewing thouroughly all at once.  I shouldn't have
> been so lazy.
np.

> I still feel a bit bad about my review that I didn't spot bug Sparse
> saw...
>
> regards,
> dan carpenter