Message ID | 1471442564-26775-1-git-send-email-cjp256@gmail.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
>>> On 17.08.16 at 16:02, <cjp256@gmail.com> wrote: > From: Chris Patterson <pattersonc@ainfosec.com> > > The uart generates an interrupt whenever the transmit holding register is > empty and UART_IER_ETHREI is set in UART_IER. Currently, Xen's ns16550 > driver does not currently mask this interrupt when transmit is stopped, > unlike other platforms such as Linux [1]. > > Toggle UART_IER_ETHREI flag in the UART_IER according to the state dictated > by stop_tx and start_tx hooks. > > On the Tegra platform (forthcoming series), the reset via reading IIR does not > prevent re-assertion of THRE. This causes Xen to hang in the interrupt > handler's while loop whenever there is no data to transmit. This behavior (bug?) > is addressed by utilizing the start & stop tx hooks. Looks mostly okay from a technical pov, but there are a few minor (mostly style) issues. > [1] http://lxr.free-electrons.com/source/drivers/tty/serial/8250/8250_port.c?v=4.7#L1518 I'd appreciate for such references to be to the canonical (i.e. Linus'es) tree. > @@ -813,6 +813,26 @@ static int __init ns16550_irq(struct serial_port *port) > return ((uart->irq > 0) ? uart->irq : -1); > } > > +static void ns16550_start_tx(struct serial_port *port) > +{ > + struct ns16550 *uart = port->uart; > + unsigned int ier = ns_read_reg(uart, UART_IER); Please use u8 or unsigned char here, as is done elsewhere in the file. > + /* unmask transmit holding register empty interrupt if currently masked */ Comment style: Upper case at the beginning; the full stop at the end has become optional recently. > + if (!(ier & UART_IER_ETHREI)) Blanks missing inside the parentheses. Jan
On Wed, Aug 17, 2016 at 10:29 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>> On 17.08.16 at 16:02, <cjp256@gmail.com> wrote: >> From: Chris Patterson <pattersonc@ainfosec.com> >> >> The uart generates an interrupt whenever the transmit holding register is >> empty and UART_IER_ETHREI is set in UART_IER. Currently, Xen's ns16550 >> driver does not currently mask this interrupt when transmit is stopped, >> unlike other platforms such as Linux [1]. >> >> Toggle UART_IER_ETHREI flag in the UART_IER according to the state dictated >> by stop_tx and start_tx hooks. >> >> On the Tegra platform (forthcoming series), the reset via reading IIR does not >> prevent re-assertion of THRE. This causes Xen to hang in the interrupt >> handler's while loop whenever there is no data to transmit. This behavior (bug?) >> is addressed by utilizing the start & stop tx hooks. > > Looks mostly okay from a technical pov, but there are a few minor > (mostly style) issues. > >> [1] http://lxr.free-electrons.com/source/drivers/tty/serial/8250/8250_port.c?v=4.7#L1518 > > I'd appreciate for such references to be to the canonical (i.e. Linus'es) > tree. > >> @@ -813,6 +813,26 @@ static int __init ns16550_irq(struct serial_port *port) >> return ((uart->irq > 0) ? uart->irq : -1); >> } >> >> +static void ns16550_start_tx(struct serial_port *port) >> +{ >> + struct ns16550 *uart = port->uart; >> + unsigned int ier = ns_read_reg(uart, UART_IER); > > Please use u8 or unsigned char here, as is done elsewhere in the file. > >> + /* unmask transmit holding register empty interrupt if currently masked */ > > Comment style: Upper case at the beginning; the full stop at the > end has become optional recently. > >> + if (!(ier & UART_IER_ETHREI)) > > Blanks missing inside the parentheses. > > Jan > ACK, will fix these in v2. Thank you for the review.
diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c index b2b5f56..cae7f1b 100644 --- a/xen/drivers/char/ns16550.c +++ b/xen/drivers/char/ns16550.c @@ -656,8 +656,8 @@ static void ns16550_setup_postirq(struct ns16550 *uart) ns_write_reg(uart, UART_MCR, UART_MCR_OUT2 | UART_MCR_DTR | UART_MCR_RTS); - /* Enable receive and transmit interrupts. */ - ns_write_reg(uart, UART_IER, UART_IER_ERDAI | UART_IER_ETHREI); + /* Enable receive interrupts. */ + ns_write_reg(uart, UART_IER, UART_IER_ERDAI); } if ( uart->irq >= 0 ) @@ -813,6 +813,26 @@ static int __init ns16550_irq(struct serial_port *port) return ((uart->irq > 0) ? uart->irq : -1); } +static void ns16550_start_tx(struct serial_port *port) +{ + struct ns16550 *uart = port->uart; + unsigned int ier = ns_read_reg(uart, UART_IER); + + /* unmask transmit holding register empty interrupt if currently masked */ + if (!(ier & UART_IER_ETHREI)) + ns_write_reg(uart, UART_IER, ier | UART_IER_ETHREI); +} + +static void ns16550_stop_tx(struct serial_port *port) +{ + struct ns16550 *uart = port->uart; + unsigned int ier = ns_read_reg(uart, UART_IER); + + /* mask off transmit holding register empty interrupt if currently unmasked */ + if (ier & UART_IER_ETHREI) + ns_write_reg(uart, UART_IER, ier & ~UART_IER_ETHREI); +} + #ifdef CONFIG_ARM static const struct vuart_info *ns16550_vuart_info(struct serial_port *port) { @@ -832,6 +852,8 @@ static struct uart_driver __read_mostly ns16550_driver = { .putc = ns16550_putc, .getc = ns16550_getc, .irq = ns16550_irq, + .start_tx = ns16550_start_tx, + .stop_tx = ns16550_stop_tx, #ifdef CONFIG_ARM .vuart_info = ns16550_vuart_info, #endif