| 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
| ||
|
Date: Tue, 6 Feb 2007 09:37:00 +0000
From: Russell King <rmk+lkml@....linux.org.uk>
To: "Wu, Bryan" <bryan.wu@...log.com>
Cc: alan@...rguk.ukuu.org.uk, aubreylee@...il.com, akpm@...l.org,
rdunlap@...otime.net, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/3, try #2] Blackfin: serial driver for Blackfin architecture against Linux kernel 2.6.20
I think you have a bit of work to do on this driver; it seems to be
missing all termios handling apart from setting the baud rate.
Moreover, it's re-using the 8250 driver port range despite claiming
to have a proper allocation.
(I don't maintain serial anymore but I do reserve the right to
occasionally review serial code.)
On Tue, Feb 06, 2007 at 11:20:11AM +0800, Wu, Bryan wrote:
> +/* We've been assigned a range on the "Low-density serial ports" major */
> +#define SERIAL_BFIN_MAJOR TTY_MAJOR
> +#define MINOR_START 64
If you've been assigned a range in the low density serial ports major,
why aren't you using it instead of taking the 8250 drivers range (thereby
preventing PCMCIA cards from ever being used) ?
> +#if defined(CONFIG_BAUD_9600)
> +#define CONSOLE_BAUD_RATE 9600
> +#elif defined(CONFIG_BAUD_19200)
> +#define CONSOLE_BAUD_RATE 19200
> +#elif defined(CONFIG_BAUD_38400)
> +#define CONSOLE_BAUD_RATE 38400
> +#elif defined(CONFIG_BAUD_57600)
> +#define CONSOLE_BAUD_RATE 57600
> +#elif defined(CONFIG_BAUD_115200)
> +#define CONSOLE_BAUD_RATE 115200
> +#endif
What's wrong with passing console=ttyblackfin0,115200 to the kernel or
via some default command line?
> +/*
> + * interrupts are disabled on entry
> + */
> +static void bfin_serial_stop_tx(struct uart_port *port)
> +{
> + struct bfin_serial_port *uart = (struct bfin_serial_port *)port;
> +
> +#ifdef CONFIG_SERIAL_BFIN_DMA
> + disable_dma(uart->tx_dma_channel);
It's far better to stop transmission as soon as possible to prevent
overruns at the remote end. I doubt disabling DMA achieves that.
> +#else
> + unsigned short ier;
> +
> + ier = UART_GET_IER(uart);
> + ier &= ~ETBEI;
> + UART_PUT_IER(uart, ier);
> +#endif
> +}
...
> +static void bfin_serial_rx_chars(struct bfin_serial_port *uart)
> +{
> + struct tty_struct *tty = uart->port.info?uart->port.info->tty:0;
> + unsigned int status, ch, flg;
> +#if defined(CONFIG_BF531) || defined(CONFIG_BF532) || defined(CONFIG_BF533)
> + static int in_break = 0;
> +#endif
> +
> + status = UART_GET_LSR(uart);
> + ch = UART_GET_CHAR(uart);
> + uart->port.icount.rx++;
> +
> +#if defined(CONFIG_BF531) || defined(CONFIG_BF532) || defined(CONFIG_BF533)
> + if (in_break) {
> + if (ch != 0) {
> + in_break = 0;
> + ch = UART_GET_CHAR(uart);
> + }
> + return;
> + }
> +#endif
> +
> + if (status & BI) {
> +#if defined(CONFIG_BF531) || defined(CONFIG_BF532) || defined(CONFIG_BF533)
> + in_break = 1;
> +#endif
> + uart->port.icount.brk++;
> + if (uart_handle_break(&uart->port))
> + goto ignore_char;
> + flg = TTY_BREAK;
> + } else if (status & PE) {
> + flg = TTY_PARITY;
> + uart->port.icount.parity++;
> + } else if (status & OE) {
> + flg = TTY_OVERRUN;
> + uart->port.icount.overrun++;
> + } else if (status & FE) {
> + flg = TTY_FRAME;
> + uart->port.icount.frame++;
> + } else
> + flg = TTY_NORMAL;
Why do many serial drivers when they're first written utterly ignore
termios modes in their reception path by not using port.read_status_mask?
It's broken as stands.
> +
> + if (uart_handle_sysrq_char(&uart->port, ch))
> + goto ignore_char;
> + if (tty)
> + uart_insert_char(&uart->port, status, 2, ch, flg);
> +
> +ignore_char:
> + if (tty)
> + tty_flip_buffer_push(tty);
> +}
...
> +static irqreturn_t bfin_serial_int(int irq, void *dev_id)
> +{
> + struct bfin_serial_port *uart = dev_id;
> + unsigned short status;
> +
> + spin_lock(&uart->port.lock);
> + status = UART_GET_IIR(uart);
> + do {
> + if ((status & IIR_STATUS) == IIR_TX_READY)
> + bfin_serial_tx_chars(uart);
> + if ((status & IIR_STATUS) == IIR_RX_READY)
> + bfin_serial_rx_chars(uart);
> + status = UART_GET_IIR(uart);
> + } while (status &(IIR_TX_READY | IIR_RX_READY));
Space between '&' and '(' please.
> + spin_unlock(&uart->port.lock);
> + return IRQ_HANDLED;
> +}
...
> +static void bfin_serial_dma_rx_chars(struct bfin_serial_port * uart)
> +{
> + struct tty_struct *tty = uart->port.info->tty;
> + int i, flg, status;
> +
> + status = UART_GET_LSR(uart);
> + uart->port.icount.rx += CIRC_CNT(uart->rx_dma_buf.head, uart->rx_dma_buf.tail, UART_XMIT_SIZE);;
> +
> + if (status & BI) {
> + uart->port.icount.brk++;
> + if (uart_handle_break(&uart->port))
> + goto dma_ignore_char;
> + flg = TTY_BREAK;
> + } else if (status & PE) {
> + flg = TTY_PARITY;
> + uart->port.icount.parity++;
> + } else if (status & OE) {
> + flg = TTY_OVERRUN;
> + uart->port.icount.overrun++;
> + } else if (status & FE) {
> + flg = TTY_FRAME;
> + uart->port.icount.frame++;
> + } else
> + flg = TTY_NORMAL;
No termios handling.
> +
> + for (i = uart->rx_dma_buf.head; i < uart->rx_dma_buf.tail; i++) {
> + if (uart_handle_sysrq_char(&uart->port, uart->rx_dma_buf.buf[i]))
> + goto dma_ignore_char;
> + uart_insert_char(&uart->port, status, 2, uart->rx_dma_buf.buf[i], flg);
> + }
> +dma_ignore_char:
> + tty_flip_buffer_push(tty);
> +}
...
> +/*
> + * Handle any change of modem status signal since we were last called.
> + */
> +static void bfin_serial_mctrl_check(struct bfin_serial_port *uart)
> +{
> +#ifdef CONFIG_SERIAL_BFIN_CTSRTS
> + unsigned int status;
> +# ifdef CONFIG_SERIAL_BFIN_DMA
> + struct uart_info *info = uart->port.info;
> + struct tty_struct *tty = info->tty;
> +
> + status = bfin_serial_get_mctrl(&uart->port);
> + if (!(status & TIOCM_CTS)) {
> + tty->hw_stopped = 1;
> + } else {
> + tty->hw_stopped = 0;
> + }
A change of CTS doesn't always mean transmission has to stop.
uart_handle_cts_change() knows when this has to happen. Use it.
> +# else
> + status = bfin_serial_get_mctrl(&uart->port);
> + uart_handle_cts_change(&uart->port, status & TIOCM_CTS);
> + if (!(status & TIOCM_CTS))
> + schedule_work(&uart->cts_workqueue);
> +# endif
> +#endif
> +}
...
> +static void
> +bfin_serial_set_termios(struct uart_port *port, struct ktermios *termios,
> + struct ktermios *old)
> +{
> + struct bfin_serial_port *uart = (struct bfin_serial_port *)port;
> + unsigned long flags;
> + unsigned int baud, quot;
> + unsigned short val, ier;
> +
> + baud = uart_get_baud_rate(port, termios, old, 0, port->uartclk/16);
> + quot = bfin_serial_calc_baud(port->uartclk, baud);
> + spin_lock_irqsave(&uart->port.lock, flags);
Doesn't set port.ignore_status_mask nor port.read_status_mask, so *NONE*
of the termios modes to do with error handling will work with this driver.
In fact, I doubt it's possible to even change the parity, length or stop
bit settings as this code stands.
> +
> + /* Disable UART */
> + ier = UART_GET_IER(uart);
> + UART_PUT_IER(uart, 0);
> +
> + /* Set DLAB in LCR to Access DLL and DLH */
> + val = UART_GET_LCR(uart);
> + val |= DLAB;
> + UART_PUT_LCR(uart, val);
> + __builtin_bfin_ssync();
> +
> + UART_PUT_DLL(uart, quot & 0xFF);
> + __builtin_bfin_ssync();
> + UART_PUT_DLH(uart, (quot >> 8) & 0xFF);
> + __builtin_bfin_ssync();
> +
> + /* Clear DLAB in LCR to Access THR RBR IER */
> + val = UART_GET_LCR(uart);
> + val &= ~DLAB;
> + UART_PUT_LCR(uart, val);
> + __builtin_bfin_ssync();
> +
> + /* Enable UART */
> + UART_PUT_IER(uart, ier);
> +
> + val = UART_GET_GCTL(uart);
> + val |= UCEN;
> + UART_PUT_GCTL(uart, val);
> +
> + spin_unlock_irqrestore(&uart->port.lock, flags);
> +}
...
> +static int bfin_serial_calc_baud(unsigned int uartclk, unsigned int baud)
> +{
> + int quot;
> +
> + quot = uartclk / (baud * 8);
> + if ((quot & 0x1) == 1) {
> + quot++;
> + }
> + return quot/2;
Something wrong with the standard function (uart_get_divisor)? If so, what?
> +}
...
> +#ifdef CONFIG_SERIAL_BFIN_CONSOLE
> +/*
> + * Interrupts are disabled on entering
> + */
> +static void
> +bfin_serial_console_write(struct console *co, const char *s, unsigned int count)
> +{
> + struct bfin_serial_port *uart = &bfin_serial_ports[co->index];
> + int flags = 0;
> + unsigned short status, tmp;
> + int i;
> +
> + spin_lock_irqsave(&uart->port.lock, flags);
> + for (i = 0; i < count; i++) {
> + do {
> + status = UART_GET_LSR(uart);
> + } while (!(status & THRE));
> +
> + tmp = UART_GET_LCR(uart);
> + tmp &= ~DLAB;
> + UART_PUT_LCR(uart, tmp);
> +
> + UART_PUT_CHAR(uart, s[i]);
> + if (s[i] == '\n') {
> + do {
> + status = UART_GET_LSR(uart);
> + } while(!(status & THRE));
> + UART_PUT_CHAR(uart, '\r');
> + }
Use uart_console_write()
> + }
> + spin_unlock_irqrestore(&uart->port.lock, flags);
> +
> +}
> +
> +/*
> + * If the port was already initialised (eg, by a boot loader),
> + * try to determine the current setup.
> + */
> +static void __init
> +bfin_serial_console_get_options(struct bfin_serial_port *uart, int *baud,
> + int *parity, int *bits)
> +{
> + unsigned short status;
> +
> + status = UART_GET_IER(uart) & (ERBFI | ETBEI);
> + if (status == (ERBFI | ETBEI)) {
> + /* ok, the port was enabled */
> + unsigned short lcr, val;
> + unsigned short dlh, dll;
> +
> + lcr = UART_GET_LCR(uart);
> +
> + *parity = 'n';
> + if (lcr & PEN) {
> + if (lcr & EPS)
> + *parity = 'e';
> + else
> + *parity = 'o';
> + }
> + switch (lcr & 0x03) {
> + case 0: *bits = 5; break;
> + case 1: *bits = 6; break;
> + case 2: *bits = 7; break;
> + case 3: *bits = 8; break;
> + }
You do have length and parity settings on this port, yet you don't
have code in your set_termios method to set them from the termios...
> + /* Set DLAB in LCR to Access DLL and DLH */
> + val = UART_GET_LCR(uart);
> + val |= DLAB;
> + UART_PUT_LCR(uart, val);
> +
> + dll = UART_GET_DLL(uart);
> + dlh = UART_GET_DLH(uart);
> +
> + /* Clear DLAB in LCR to Access THR RBR IER */
> + val = UART_GET_LCR(uart);
> + val &= ~DLAB;
> + UART_PUT_LCR(uart, val);
> +
> + *baud = get_sclk() / (16*(dll | dlh << 8));
> + }
> + pr_debug("%s:baud = %d, parity = %c, bits= %d\n", __FUNCTION__, *baud, *parity, *bits);
> +}
--
Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of:
-
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/
Powered by blists - more mailing lists