Re: [Xen-devel] [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  wrote:
 On 17.08.16 at 16:02,  wrote:
>> From: Chris Patterson 
>>
>> 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

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

[Jan Beulich]

>>> On 17.08.16 at 16:02,  wrote:
> From: Chris Patterson 
> 
> 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

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

[Chris Patterson]

From: Chris Patterson 

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