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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 27.08.13 at 12:15, Tomasz Wroblewski <tomasz.wroblewski@xxxxxxxxxx> 
>>> 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

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