linux-kernel - RE: [PATCH v3] Bluetooth: hci_uart: Support firmware download for Marvell

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] [day] [month] [year] [list]

Date:	Wed, 2 Mar 2016 06:47:50 +0000
From:	Amitkumar Karwar <akarwar@...vell.com>
To:	One Thousand Gnomes <gnomes@...rguk.ukuu.org.uk>
CC:	"linux-bluetooth@...r.kernel.org" <linux-bluetooth@...r.kernel.org>,
	"Cathy Luo" <cluo@...vell.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Nishant Sarmukadam <nishants@...vell.com>,
	Ganapathi Bhat <gbhat@...vell.com>
Subject: RE: [PATCH v3] Bluetooth: hci_uart: Support firmware download for
 Marvell

Hi Alan,

> From: One Thousand Gnomes [mailto:gnomes@...rguk.ukuu.org.uk]
> Sent: Wednesday, March 02, 2016 3:29 AM
> To: Amitkumar Karwar
> Cc: linux-bluetooth@...r.kernel.org; Cathy Luo; linux-
> kernel@...r.kernel.org; Nishant Sarmukadam; Ganapathi Bhat
> Subject: Re: [PATCH v3] Bluetooth: hci_uart: Support firmware download
> for Marvell
> 
> O
> > +/* Get standard baud rate, given the speed */ static unsigned int
> > +get_baud_rate(unsigned int speed) {
> > +	switch (speed) {
> > +	case 9600:
> > +		return B9600;
> > +	case 19200:
> > +		return B19200;
> > +	case 38400:
> > +		return B38400;
> > +	case 57600:
> > +		return B57600;
> > +	case 115200:
> > +		return B115200;
> > +	case 230400:
> > +		return B230400;
> > +	case 460800:
> > +		return B460800;
> > +	case 921600:
> > +		return B921600;
> > +	case 2000000:
> > +		return B2000000;
> > +	case 3000000:
> > +		return B3000000;
> > +	default:
> > +		return -1;
> > +	}
> > +}
> 
> 
> NAK. Please use the existing kernel helpers for this
> 
> 
>  +
> > +/* Set terminal properties */
> > +static int mrvl_set_term_baud(struct tty_struct *tty, unsigned int
> speed,
> > +			      unsigned char flow_ctl)
> > +{
> > +	struct ktermios old_termios = tty->termios;
> > +	int baud;
> > +
> > +	tty->termios.c_cflag &= ~CBAUD;
> > +	baud = get_baud_rate(speed);
> > +
> > +	if (baud == -1) {
> > +		BT_ERR("Baud rate not supported");
> > +		return -1;
> > +	}
> > +
> > +	tty->termios.c_cflag |= baud;
> > +
> 
> This isn't the correct way to do any of this, just do
> 
> 	tty_termios_encode_baud_rate(&tty->termios, speed, speed)
> 
> 
> > +	if (flow_ctl)
> > +		tty->termios.c_cflag |= CRTSCTS;
> > +	else
> > +		tty->termios.c_cflag &= ~CRTSCTS;
> > +
> > +	tty->ops->set_termios(tty, &old_termios);
> 
> Call the provided kernel helpers that get the locking right.
> 
> tty_set_termios(tty, &new_termios);
> 
> You should also do your error checking here and see what baud rate was
> actually provided by the hardware (tty_get_baud_rate(tty)) by checking
> the value actually selected by the tty.
> 
> > +	/* restore uart settings */
> > +	new_termios = tty->termios;
> > +	tty->termios.c_cflag = old_termios.c_cflag;
> > +	tty->ops->set_termios(tty, &new_termios);
> > +	clear_bit(HCI_UART_DNLD_FW, &hu->flags);
> 
> Again use the proper helpers
> 
> > +
> > +set_baud:
> > +	ret = mrvl_set_baud(hu);
> > +	if (ret)
> > +		goto fail;
> > +
> > +	mdelay(MRVL_DNLD_DELAY);
> 
> Why not msleep() ?
> 
> > +
> > +	return ret;
> > +fail:
> > +	/* restore uart settings */
> > +	new_termios = tty->termios;
> > +	tty->termios.c_cflag = old_termios.c_cflag;
> > +	tty->ops->set_termios(tty, &new_termios);
> > +	clear_bit(HCI_UART_DNLD_FW, &hu->flags);
> > +
> 

Thanks for review. We will work on these comments.

Regards,
Amitkumar