Xen devel - Aug 2013 - [PATCH] V2 pci uart - better cope with UART being temporarily unavailable

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 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   |   33 +++++++++++++++++++--------------
 xen/drivers/char/serial.c    |   41 ++++++++++++++++++++++++++---------------
 xen/include/xen/serial.h     |    5 +++--
 4 files changed, 49 insertions(+), 32 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..8be0849 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)
 {
@@ -102,12 +107,17 @@ static void __ns16550_poll(struct cpu_user_regs *regs)
     if ( uart->intr_works )
         return; /* Interrupts work - no more polling */
 
-    while ( ns_read_reg(uart, UART_LSR) & UART_LSR_DR )
-        serial_rx_interrupt(port, regs);
+     while ( ns_read_reg(uart, UART_LSR) & UART_LSR_DR )
+     {
+         serial_rx_interrupt(port, regs);
+         if ( ns16550_ioport_invalid(uart) )
+             goto out;
+     }
 
     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,11 +131,14 @@ 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;
 
-    return ns_read_reg(uart, UART_LSR) & UART_LSR_THRE ? uart->fifo_size
: 0;
+    if ( ns16550_ioport_invalid(uart) )
+        return -EIO;
+    else
+        return ns_read_reg(uart, UART_LSR) & UART_LSR_THRE ?
uart->fifo_size : 0;
 }
 
 static void ns16550_putc(struct serial_port *port, char c)
@@ -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..dfed4d9 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,12 @@ 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,10 +395,16 @@ 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();
-            port->driver->putc(
-                port,
port->txbuf[mask_serial_txbuf_idx(port->txbufc++)]);
+            if (n > 0)
+                port->driver->putc(
+                    port,
port->txbuf[mask_serial_txbuf_idx(port->txbufc++)]);
+            else
+                /* port is unavailable and might not come up until reenabled by
+                   dom0, we can''t really do proper sync */
+                break;
         }
         if ( port->driver->flush )
             port->driver->flush(port);
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

>>> On 27.08.13 at 12:15, Tomasz Wroblewski
<tomasz.wroblewski@citrix.com> wrote:
> @@ -102,12 +107,17 @@ static void __ns16550_poll(struct cpu_user_regs
*regs)
>      if ( uart->intr_works )
>          return; /* Interrupts work - no more polling */
>  
> -    while ( ns_read_reg(uart, UART_LSR) & UART_LSR_DR )
> -        serial_rx_interrupt(port, regs);
> +     while ( ns_read_reg(uart, UART_LSR) & UART_LSR_DR )
> +     {
> +         serial_rx_interrupt(port, regs);
> +         if ( ns16550_ioport_invalid(uart) )
> +             goto out;
> +     }
>  
>      if ( ns_read_reg(uart, UART_LSR) & UART_LSR_THRE )
>          serial_tx_interrupt(port, regs);
>  
> +out:
So serial_rx_interrupt() gets run once in that case, but
serial_tx_interrupt() not at all? That not only inconsistent, I also
can''t see why anything would need to be done here at all in this
case. Plus doing the check before the loop would shrink patch
size.
> +static int ns16550_tx_ready(struct serial_port *port)
>  {
>      struct ns16550 *uart = port->uart;
>  
> -    return ns_read_reg(uart, UART_LSR) & UART_LSR_THRE ?
uart->fifo_size : 0;
> +    if ( ns16550_ioport_invalid(uart) )
> +        return -EIO;
> +    else
> +        return ns_read_reg(uart, UART_LSR) & UART_LSR_THRE ?
uart->fifo_size : 0;
Dropping the unnecessary "else" would again shrink patch size.
> --- a/xen/drivers/char/serial.c
> +++ b/xen/drivers/char/serial.c
> @@ -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-- )
"while ( n-- > 0 )" can achieve the same without the extra if()
and with just a one line change.
> +                        port->driver->putc(
> +                            port,
> +                           
port->txbuf[mask_serial_txbuf_idx(port->txbufc++)]);
> +                   
port->txbuf[mask_serial_txbuf_idx(port->txbufp++)] = c;
> +                }
>              }
>              else
>              {
> @@ -143,10 +146,12 @@ static void __serial_putc(struct serial_port *port,
char c)
>      }
>      else if ( port->driver->tx_ready )
>      {
> +        int n;
Missing blank line.
>          /* 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)
Coding style.
> +            port->driver->putc(port, c);
>      }
>      else
>      {
> @@ -390,10 +395,16 @@ void serial_start_sync(int handle)
>      {
>          while ( (port->txbufp - port->txbufc) != 0 )
>          {
> -            while ( !port->driver->tx_ready(port) )
> +            int n;
Again missing blank line.
> +            while ( !(n = port->driver->tx_ready(port)) )
>                  cpu_relax();
> -            port->driver->putc(
> -                port,
port->txbuf[mask_serial_txbuf_idx(port->txbufc++)]);
> +            if (n > 0)
Again coding style.
> +                port->driver->putc(
> +                    port,
port->txbuf[mask_serial_txbuf_idx(port->txbufc++)]);
> +            else
> +                /* port is unavailable and might not come up until
reenabled by
> +                   dom0, we can''t really do proper sync */
> +                break;
Once again, patch size would be smaller if you handled the n < 0
case first and without "else".

Jan

>> --- a/xen/drivers/char/serial.c
>> +++ b/xen/drivers/char/serial.c
>> @@ -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-- )
> "while ( n-->  0 )" can achieve the same without the extra
if()
> and with just a one line change.
Thanks for review - agree with most comments except this one, we need to 
avoid executing "port->txbuf[mask_serial_txbuf_idx(port->txbufp++)] =
c"
if port is offline, since in this case the while loop will not consume 
pull any character out of the buffer, and it will stay full, and we 
should just drop the char. The "if" I added pulled the buffer insert 
into its body.

On 08/27/2013 02:26 PM, Jan Beulich wrote:
>>>> On 27.08.13 at 12:15, Tomasz
Wroblewski<tomasz.wroblewski@citrix.com>  wrote:
>> @@ -102,12 +107,17 @@ static void __ns16550_poll(struct cpu_user_regs
*regs)
>>       if ( uart->intr_works )
>>           return; /* Interrupts work - no more polling */
>>
>> -    while ( ns_read_reg(uart, UART_LSR)&  UART_LSR_DR )
>> -        serial_rx_interrupt(port, regs);
>> +     while ( ns_read_reg(uart, UART_LSR)&  UART_LSR_DR )
>> +     {
>> +         serial_rx_interrupt(port, regs);
>> +         if ( ns16550_ioport_invalid(uart) )
>> +             goto out;
>> +     }
>>
>>       if ( ns_read_reg(uart, UART_LSR)&  UART_LSR_THRE )
>>           serial_tx_interrupt(port, regs);
>>
>> +out:
> So serial_rx_interrupt() gets run once in that case, but
> serial_tx_interrupt() not at all? That not only inconsistent, I also
> can''t see why anything would need to be done here at all in this
> case. Plus doing the check before the loop would shrink patch
> size.
So I presume it is impossible for dom0 code to run on another cpu whilst 
xen is executing the poll routine, I was not sure at all about this? 
Would like to avoid the possibility of dom0 disabling device whilst this 
loop is running.

>>> On 27.08.13 at 15:36, Tomasz Wroblewski
<tomasz.wroblewski@citrix.com> wrote:
>>> --- a/xen/drivers/char/serial.c
>>> +++ b/xen/drivers/char/serial.c
>>> @@ -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-- )
>> "while ( n-->  0 )" can achieve the same without the extra
if()
>> and with just a one line change.
> Thanks for review - agree with most comments except this one, we need to 
> avoid executing
"port->txbuf[mask_serial_txbuf_idx(port->txbufp++)] = c"
> if port is offline, since in this case the while loop will not consume 
> pull any character out of the buffer, and it will stay full, and we 
> should just drop the char. The "if" I added pulled the buffer
insert
> into its body.
Indeed. However, we''re in a log-everything section, so dropping
characters here is not really right. Not sure what to do about this.

Jan

>>> On 27.08.13 at 15:44, Tomasz Wroblewski
<tomasz.wroblewski@citrix.com> wrote:
> On 08/27/2013 02:26 PM, Jan Beulich wrote:
>>>>> On 27.08.13 at 12:15, Tomasz
Wroblewski<tomasz.wroblewski@citrix.com>  wrote:
>>> @@ -102,12 +107,17 @@ static void __ns16550_poll(struct
cpu_user_regs *regs)
>>>       if ( uart->intr_works )
>>>           return; /* Interrupts work - no more polling */
>>>
>>> -    while ( ns_read_reg(uart, UART_LSR)&  UART_LSR_DR )
>>> -        serial_rx_interrupt(port, regs);
>>> +     while ( ns_read_reg(uart, UART_LSR)&  UART_LSR_DR )
>>> +     {
>>> +         serial_rx_interrupt(port, regs);
>>> +         if ( ns16550_ioport_invalid(uart) )
>>> +             goto out;
>>> +     }
>>>
>>>       if ( ns_read_reg(uart, UART_LSR)&  UART_LSR_THRE )
>>>           serial_tx_interrupt(port, regs);
>>>
>>> +out:
>> So serial_rx_interrupt() gets run once in that case, but
>> serial_tx_interrupt() not at all? That not only inconsistent, I also
>> can''t see why anything would need to be done here at all in
this
>> case. Plus doing the check before the loop would shrink patch
>> size.
> So I presume it is impossible for dom0 code to run on another cpu whilst 
> xen is executing the poll routine, I was not sure at all about this? 
> Would like to avoid the possibility of dom0 disabling device whilst this 
> loop is running.
Of course can a Dom0 vCPU run on another pCPU.

And yes, I hence appreciate reducing the risk window. But then
please do the check before calling serial_rx_interrupt().

Jan

On 08/27/2013 03:59 PM, Jan Beulich wrote:
>>>> On 27.08.13 at 15:44, Tomasz
Wroblewski<tomasz.wroblewski@citrix.com>  wrote:
>> On 08/27/2013 02:26 PM, Jan Beulich wrote:
>>>>>> On 27.08.13 at 12:15, Tomasz
Wroblewski<tomasz.wroblewski@citrix.com>   wrote:
>>>> @@ -102,12 +107,17 @@ static void __ns16550_poll(struct
cpu_user_regs *regs)
>>>>        if ( uart->intr_works )
>>>>            return; /* Interrupts work - no more polling */
>>>>
>>>> -    while ( ns_read_reg(uart, UART_LSR)&   UART_LSR_DR )
>>>> -        serial_rx_interrupt(port, regs);
>>>> +     while ( ns_read_reg(uart, UART_LSR)&   UART_LSR_DR )
>>>> +     {
>>>> +         serial_rx_interrupt(port, regs);
>>>> +         if ( ns16550_ioport_invalid(uart) )
>>>> +             goto out;
>>>> +     }
>>>>
>>>>        if ( ns_read_reg(uart, UART_LSR)&   UART_LSR_THRE )
>>>>            serial_tx_interrupt(port, regs);
>>>>
>>>> +out:
>>> So serial_rx_interrupt() gets run once in that case, but
>>> serial_tx_interrupt() not at all? That not only inconsistent, I
also
>>> can''t see why anything would need to be done here at all
in this
>>> case. Plus doing the check before the loop would shrink patch
>>> size.
>> So I presume it is impossible for dom0 code to run on another cpu
whilst
>> xen is executing the poll routine, I was not sure at all about this?
>> Would like to avoid the possibility of dom0 disabling device whilst
this
>> loop is running.
> Of course can a Dom0 vCPU run on another pCPU.
>
> And yes, I hence appreciate reducing the risk window. But then
> please do the check before calling serial_rx_interrupt().
>
Ah sure, will repost v4 soon then