[PATCH] ns16550: mask transmit holding register empty interrupt when tx is stopped

[PATCH] ns16550: mask transmit holding register empty interrupt when tx is stopped

[Chris Patterson]

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.

This has been tested on various x86 PCs for any obvious signs of regressions.

[1] http://lxr.free-electrons.com/source/drivers/tty/serial/8250/8250_port.c?v=4.7#L1518

Signed-off-by: Chris Patterson <pattersonc@ainfosec.com>
---
 xen/drivers/char/ns16550.c | 26 ++++++++++++++++++++++++--
 1 file changed, 24 insertions(+), 2 deletions(-)

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


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] ns16550: mask transmit holding register empty interrupt when tx is stopped

[Jan Beulich]

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


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] ns16550: mask transmit holding register empty interrupt when tx is stopped

[Chris Patterson]

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.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel