[PATCH] V4 pci uart - better cope with UART being temporarily unavailable

[PATCH] V4 pci uart - better cope with UART being temporarily unavailable

[Tomasz Wroblewski]

This happens for example when dom0 disables ioport responses during PCI subsystem initialisation. If a __ns16550_poll() happens to be scheduled during that time, Xen hangs. Detect and exit that condition.

Amended ns16550_ioport_invalid function to only check IER register, which contins 3 reserved (always 0) bits, therefore it's sufficient for that test.

Changes since V3:
* readded invalid port test in the loop in __ns16550, moved it before serial_rx_interrupt call

Changes since V2:
* pulled out invalid port test before the loop in __ns16550_poll
* coding style/patch size fixes

Changes since V1:
* added invalid port test in getc()
* changed ns16550_tx_ready to return -EIO and code in serial.[ch] to cope with that error condition. If port is temporarily unavailable tx_ready will return -EIO and serial driver code will attempt to buffer the character instead of dropping it.

Signed-off-by: Tomasz Wroblewski <tomasz.wroblewski@citrix.com>
---
 xen/drivers/char/ehci-dbgp.c |    2 +-
 xen/drivers/char/ns16550.c   |   27 ++++++++++++++++-----------
 xen/drivers/char/serial.c    |   38 +++++++++++++++++++++++++-------------
 xen/include/xen/serial.h     |    5 +++--
 4 files changed, 45 insertions(+), 27 deletions(-)

diff --git a/xen/drivers/char/ehci-dbgp.c b/xen/drivers/char/ehci-dbgp.c
index 6b70660..6aa502e 100644
--- a/xen/drivers/char/ehci-dbgp.c
+++ b/xen/drivers/char/ehci-dbgp.c
@@ -1204,7 +1204,7 @@ static void ehci_dbgp_putc(struct serial_port *port, char c)
         ehci_dbgp_flush(port);
 }
 
-static unsigned int ehci_dbgp_tx_ready(struct serial_port *port)
+static int ehci_dbgp_tx_ready(struct serial_port *port)
 {
     struct ehci_dbgp *dbgp = port->uart;
 
diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index 6082c85..3245886 100644
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -73,6 +73,11 @@ static void ns_write_reg(struct ns16550 *uart, int reg, char c)
     writeb(c, uart->remapped_io_base + reg);
 }
 
+static int ns16550_ioport_invalid(struct ns16550 *uart)
+{
+    return (unsigned char)ns_read_reg(uart, UART_IER) == 0xff;
+}
+
 static void ns16550_interrupt(
     int irq, void *dev_id, struct cpu_user_regs *regs)
 {
@@ -103,11 +108,17 @@ static void __ns16550_poll(struct cpu_user_regs *regs)
         return; /* Interrupts work - no more polling */
 
     while ( ns_read_reg(uart, UART_LSR) & UART_LSR_DR )
+    {
+        if ( ns16550_ioport_invalid(uart) )
+            goto out;
+
         serial_rx_interrupt(port, regs);
+    }
 
     if ( ns_read_reg(uart, UART_LSR) & UART_LSR_THRE )
         serial_tx_interrupt(port, regs);
 
+out:
     set_timer(&uart->timer, NOW() + MILLISECS(uart->timeout_ms));
 }
 
@@ -121,10 +132,12 @@ static void ns16550_poll(void *data)
 #endif
 }
 
-static unsigned int ns16550_tx_ready(struct serial_port *port)
+static int ns16550_tx_ready(struct serial_port *port)
 {
     struct ns16550 *uart = port->uart;
 
+    if ( ns16550_ioport_invalid(uart) )
+        return -EIO;
     return ns_read_reg(uart, UART_LSR) & UART_LSR_THRE ? uart->fifo_size : 0;
 }
 
@@ -138,7 +151,8 @@ static int ns16550_getc(struct serial_port *port, char *pc)
 {
     struct ns16550 *uart = port->uart;
 
-    if ( !(ns_read_reg(uart, UART_LSR) & UART_LSR_DR) )
+    if ( ns16550_ioport_invalid(uart) ||
+        !(ns_read_reg(uart, UART_LSR) & UART_LSR_DR) )
         return 0;
 
     *pc = ns_read_reg(uart, UART_RBR);
@@ -305,15 +319,6 @@ static void _ns16550_resume(struct serial_port *port)
     ns16550_setup_postirq(port->uart);
 }
 
-static int ns16550_ioport_invalid(struct ns16550 *uart)
-{
-    return ((((unsigned char)ns_read_reg(uart, UART_LSR)) == 0xff) &&
-            (((unsigned char)ns_read_reg(uart, UART_MCR)) == 0xff) &&
-            (((unsigned char)ns_read_reg(uart, UART_IER)) == 0xff) &&
-            (((unsigned char)ns_read_reg(uart, UART_IIR)) == 0xff) &&
-            (((unsigned char)ns_read_reg(uart, UART_LCR)) == 0xff));
-}
-
 static int delayed_resume_tries;
 static void ns16550_delayed_resume(void *data)
 {
diff --git a/xen/drivers/char/serial.c b/xen/drivers/char/serial.c
index cd0b864..9b006f2 100644
--- a/xen/drivers/char/serial.c
+++ b/xen/drivers/char/serial.c
@@ -59,7 +59,7 @@ void serial_rx_interrupt(struct serial_port *port, struct cpu_user_regs *regs)
 
 void serial_tx_interrupt(struct serial_port *port, struct cpu_user_regs *regs)
 {
-    unsigned int i, n;
+    int i, n;
     unsigned long flags;
 
     local_irq_save(flags);
@@ -71,7 +71,7 @@ void serial_tx_interrupt(struct serial_port *port, struct cpu_user_regs *regs)
      */
     while ( !spin_trylock(&port->tx_lock) )
     {
-        if ( !port->driver->tx_ready(port) )
+        if ( port->driver->tx_ready(port) <= 0 )
             goto out;
         cpu_relax();
     }
@@ -111,15 +111,18 @@ static void __serial_putc(struct serial_port *port, char c)
             if ( port->tx_log_everything )
             {
                 /* Buffer is full: we spin waiting for space to appear. */
-                unsigned int n;
+                int n;
 
                 while ( (n = port->driver->tx_ready(port)) == 0 )
                     cpu_relax();
-                while ( n-- )
-                    port->driver->putc(
-                        port,
-                        port->txbuf[mask_serial_txbuf_idx(port->txbufc++)]);
-                port->txbuf[mask_serial_txbuf_idx(port->txbufp++)] = c;
+                if ( n > 0 )
+                {
+                    while ( n-- )
+                        port->driver->putc(
+                            port,
+                            port->txbuf[mask_serial_txbuf_idx(port->txbufc++)]);
+                    port->txbuf[mask_serial_txbuf_idx(port->txbufp++)] = c;
+                }
             }
             else
             {
@@ -130,9 +133,9 @@ static void __serial_putc(struct serial_port *port, char c)
         }
 
         if ( ((port->txbufp - port->txbufc) == 0) &&
-             port->driver->tx_ready(port) )
+             port->driver->tx_ready(port) > 0 )
         {
-            /* Buffer and UART FIFO are both empty. */
+            /* Buffer and UART FIFO are both empty, and port is available. */
             port->driver->putc(port, c);
         }
         else
@@ -143,10 +146,13 @@ static void __serial_putc(struct serial_port *port, char c)
     }
     else if ( port->driver->tx_ready )
     {
+        int n;
+
         /* Synchronous finite-capacity transmitter. */
-        while ( !port->driver->tx_ready(port) )
+        while ( !(n = port->driver->tx_ready(port)) )
             cpu_relax();
-        port->driver->putc(port, c);
+        if ( n > 0 )
+            port->driver->putc(port, c);
     }
     else
     {
@@ -390,8 +396,14 @@ void serial_start_sync(int handle)
     {
         while ( (port->txbufp - port->txbufc) != 0 )
         {
-            while ( !port->driver->tx_ready(port) )
+            int n;
+
+            while ( !(n = port->driver->tx_ready(port)) )
                 cpu_relax();
+            if ( n < 0 )
+                /* port is unavailable and might not come up until reenabled by
+                   dom0, we can't really do proper sync */
+                break;
             port->driver->putc(
                 port, port->txbuf[mask_serial_txbuf_idx(port->txbufc++)]);
         }
diff --git a/xen/include/xen/serial.h b/xen/include/xen/serial.h
index 403e193..f38c9b7 100644
--- a/xen/include/xen/serial.h
+++ b/xen/include/xen/serial.h
@@ -70,8 +70,9 @@ struct uart_driver {
     /* Driver suspend/resume. */
     void (*suspend)(struct serial_port *);
     void (*resume)(struct serial_port *);
-    /* Return number of characters the port can hold for transmit. */
-    unsigned int (*tx_ready)(struct serial_port *);
+    /* Return number of characters the port can hold for transmit,
+     * or -EIO if port is inaccesible */
+    int (*tx_ready)(struct serial_port *);
     /* Put a character onto the serial line. */
     void (*putc)(struct serial_port *, char);
     /* Flush accumulated characters. */
-- 
1.7.10.4

Re: [PATCH] V4 pci uart - better cope with UART being temporarily unavailable

[Tomasz Wroblewski]

On 08/27/2013 04:05 PM, Tomasz Wroblewski wrote:
> This happens for example when dom0 disables ioport responses during PCI subsystem initialisation. If a __ns16550_poll() happens to be scheduled during that time, Xen hangs. Detect and exit that condition.
>
> Amended ns16550_ioport_invalid function to only check IER register, which contins 3 reserved (always 0) bits, therefore it's sufficient for that test.
>
> Changes since V3:
> * readded invalid port test in the loop in __ns16550, moved it before serial_rx_interrupt call
>
> Changes since V2:
> * pulled out invalid port test before the loop in __ns16550_poll
> * coding style/patch size fixes
>
> Changes since V1:
> * added invalid port test in getc()
> * changed ns16550_tx_ready to return -EIO and code in serial.[ch] to cope with that error condition. If port is temporarily unavailable tx_ready will return -EIO and serial driver code will attempt to buffer the character instead of dropping it.
>
> Signed-off-by: Tomasz Wroblewski<tomasz.wroblewski@citrix.com>
readding

Reviewed-by: Jan Beulich <jbeulich@suse.com>

since he said:
"(and you may retain the tag if you decide to undo the first of the
two mentioned changes since V2 - I'm fine with it either way)"

V4 undoes that mentioned change, it was wrong.

Re: [PATCH] V4 pci uart - better cope with UART being temporarily unavailable

[Keir Fraser]

On 27/08/2013 15:05, "Tomasz Wroblewski" <tomasz.wroblewski@citrix.com>
wrote:

> This happens for example when dom0 disables ioport responses during PCI
> subsystem initialisation. If a __ns16550_poll() happens to be scheduled during
> that time, Xen hangs. Detect and exit that condition.
> 
> Amended ns16550_ioport_invalid function to only check IER register, which
> contins 3 reserved (always 0) bits, therefore it's sufficient for that test.
> 
> Changes since V3:
> * readded invalid port test in the loop in __ns16550, moved it before
> serial_rx_interrupt call
> 
> Changes since V2:
> * pulled out invalid port test before the loop in __ns16550_poll
> * coding style/patch size fixes
> 
> Changes since V1:
> * added invalid port test in getc()
> * changed ns16550_tx_ready to return -EIO and code in serial.[ch] to cope with
> that error condition. If port is temporarily unavailable tx_ready will return
> -EIO and serial driver code will attempt to buffer the character instead of
> dropping it.
> 
> Signed-off-by: Tomasz Wroblewski <tomasz.wroblewski@citrix.com>
> ---

Acked-by: Keir Fraser <keir@xen.org>

Re: [PATCH] V4 pci uart - better cope with UART being temporarily unavailable

[Julien Grall]

On 08/27/2013 03:05 PM, Tomasz Wroblewski wrote:
> diff --git a/xen/include/xen/serial.h b/xen/include/xen/serial.h
> index 403e193..f38c9b7 100644
> --- a/xen/include/xen/serial.h
> +++ b/xen/include/xen/serial.h
> @@ -70,8 +70,9 @@ struct uart_driver {
>      /* Driver suspend/resume. */
>      void (*suspend)(struct serial_port *);
>      void (*resume)(struct serial_port *);
> -    /* Return number of characters the port can hold for transmit. */
> -    unsigned int (*tx_ready)(struct serial_port *);
> +    /* Return number of characters the port can hold for transmit,
> +     * or -EIO if port is inaccesible */
> +    int (*tx_ready)(struct serial_port *);

Hi,

This callback is shared between ARM and X86. You forgot to modify ARM UART driver
(omap, pl011, exynos4210,...), so it breaks Xen unstable compilation on ARM.

pl011.c:209:5: error: initialization from incompatible pointer type [-Werror]
     .tx_ready     = pl011_tx_ready,
     ^
pl011.c:209:5: error: (near initialization for ‘pl011_driver.tx_ready’) [-Werror]
exynos4210-uart.c:298:5: error: initialization from incompatible pointer type [-Werror]
     .tx_ready     = exynos4210_uart_tx_ready,
     ^
exynos4210-uart.c:298:5: error: (near initialization for ‘exynos4210_uart_driver.tx_ready’) [-Werror]
omap-uart.c:285:5: error: initialization from incompatible pointer type [-Werror]
     .tx_ready = omap_uart_tx_ready,
     ^

Please, send a patch to fix the compilation on ARM.

Thanks


-- 
Julien Grall

Re: [PATCH] V4 pci uart - better cope with UART being temporarily unavailable

[Tomasz Wroblewski]

On 08/28/2013 01:15 PM, Julien Grall wrote:
> On 08/27/2013 03:05 PM, Tomasz Wroblewski wrote:
>> diff --git a/xen/include/xen/serial.h b/xen/include/xen/serial.h
>> index 403e193..f38c9b7 100644
>> --- a/xen/include/xen/serial.h
>> +++ b/xen/include/xen/serial.h
>> @@ -70,8 +70,9 @@ struct uart_driver {
>>       /* Driver suspend/resume. */
>>       void (*suspend)(struct serial_port *);
>>       void (*resume)(struct serial_port *);
>> -    /* Return number of characters the port can hold for transmit. */
>> -    unsigned int (*tx_ready)(struct serial_port *);
>> +    /* Return number of characters the port can hold for transmit,
>> +     * or -EIO if port is inaccesible */
>> +    int (*tx_ready)(struct serial_port *);
> Hi,
>
> This callback is shared between ARM and X86. You forgot to modify ARM UART driver
> (omap, pl011, exynos4210,...), so it breaks Xen unstable compilation on ARM.
>
> pl011.c:209:5: error: initialization from incompatible pointer type [-Werror]
>       .tx_ready     = pl011_tx_ready,
>       ^
> pl011.c:209:5: error: (near initialization for ‘pl011_driver.tx_ready’) [-Werror]
> exynos4210-uart.c:298:5: error: initialization from incompatible pointer type [-Werror]
>       .tx_ready     = exynos4210_uart_tx_ready,
>       ^
> exynos4210-uart.c:298:5: error: (near initialization for ‘exynos4210_uart_driver.tx_ready’) [-Werror]
> omap-uart.c:285:5: error: initialization from incompatible pointer type [-Werror]
>       .tx_ready = omap_uart_tx_ready,
>       ^
>
> Please, send a patch to fix the compilation on ARM.
sorry for that, will send soon
> Thanks
>
>