linux-kernel - Re: [PATCH] serial: Add driver for LPC32xx High Speed UARTs

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, 30 May 2012 16:31:52 +0100
From:	Alan Cox <alan@...rguk.ukuu.org.uk>
To:	Roland Stigge <stigge@...com.de>
Cc:	alan@...ux.intel.com, gregkh@...uxfoundation.org,
	linux-serial@...r.kernel.org, linux-kernel@...r.kernel.org,
	kevin.wells@....com, srinivas.bakki@....com,
	linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH] serial: Add driver for LPC32xx High Speed UARTs

> +#define LPC32XX_TTY_MINOR_START	196
> +#define LPC32XX_TTY_MAJOR 204

NAK - please use dynamic allocations.


> +	/* Read data from FIFO and push into terminal */
> +	tmp = readl(LPC32XX_HSUART_FIFO(port->membase));
> +	while (!(tmp & LPC32XX_HSU_RX_EMPTY)) {
> +		flag = TTY_NORMAL;
> +		port->icount.rx++;
> +
> +		if (tmp & LPC32XX_HSU_ERROR_DATA) {
> +			/* Framing error */
> +			writel(LPC32XX_HSU_FE_INT,
> +			       LPC32XX_HSUART_IIR(port->membase));
> +			port->icount.frame++;
> +			flag = TTY_FRAME;
> +			tty_insert_flip_char(port->state->port.tty, 0,
> +					     TTY_FRAME);
> +			tty_schedule_flip(port->state->port.tty);
> +		}
> +
> +		tty_insert_flip_char(port->state->port.tty, (tmp & 0xFF), flag);
> +
> +		tmp = readl(LPC32XX_HSUART_FIFO(port->membase));

This seems odd - you don't need to schedule the flip of the framing error
separately. It all just ends up in the queue and can be kicked in one go.

Also if the last byte is a normal char - who ensures the buffer is pushed
and not wedged until a future I/O ?

Second problem is port->state->port.tty can be or go to NULL. Take a look
how drivers use tty_port_tty_get(), and tty_kref_put.


> +	/* Data received? */
> +	if (status & (LPC32XX_HSU_RX_TIMEOUT_INT | LPC32XX_HSU_RX_TRIG_INT)) {
> +		__serial_lpc32xx_rx(port);
> +		spin_unlock(&port->lock);
> +		tty_flip_buffer_push(port->state->port.tty);
> +		spin_lock(&port->lock);
> +	}

Not sure what the locking is about here - you shouldn't need to fiddle
with locks unless you are using low latency and you can't use low latency
safely from an IRQ handler...

(Also again tty and NULL - you may want to grab it once at the start of
the handler)


> +static void serial_lpc32xx_set_termios(struct uart_port *port,
> +				       struct ktermios *termios,
> +				       struct ktermios *old)
> +{
> +	unsigned long flags;
> +	unsigned int baud, quot;
> +	u32 tmp;
> +
> +	/* Always 8-bit, no parity, 1 stop bit */
> +	termios->c_cflag &= ~(CSIZE | CSTOPB | PARENB | PARODD);
> +	termios->c_cflag |= CS8;
> +
> +	termios->c_cflag &= ~(HUPCL | CMSPAR | CLOCAL | CRTSCTS);
> +
> +	baud = uart_get_baud_rate(port, termios, old, 0,
> +				  port->uartclk / 14);

You want to set the resulting baud rate back into the struct - see 8250.c
for an example.

Alan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/