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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 15 May 2017 15:52:52 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     Haim Daniel <haimdaniel@...il.com>
Cc:     lidza.louina@...il.com, markh@...pro.net,
        gregkh@...uxfoundation.org, driverdev-devel@...uxdriverproject.org,
        devel@...verdev.osuosl.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] drivers/staging: refactor dgnc tty registration.

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@...il.com>
> ---
>  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?

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

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.

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.

I still feel a bit bad about my review that I didn't spot bug Sparse
saw...

regards,
dan carpenter