Tomasz Wroblewski
2013-Aug-27 13:54 UTC
[PATCH] V3 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 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 | 24 +++++++++++++-----------
xen/drivers/char/serial.c | 38 +++++++++++++++++++++++++-------------
xen/include/xen/serial.h | 5 +++--
4 files changed, 42 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..e8d3232 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,6 +107,9 @@ static void __ns16550_poll(struct cpu_user_regs *regs)
if ( uart->intr_works )
return; /* Interrupts work - no more polling */
+ if ( ns16550_ioport_invalid(uart) )
+ return;
+
while ( ns_read_reg(uart, UART_LSR) & UART_LSR_DR )
serial_rx_interrupt(port, regs);
@@ -121,10 +129,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 +148,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 +316,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
Jan Beulich
2013-Aug-27 14:06 UTC
Re: [PATCH] V3 pci uart - better cope with UART being temporarily unavailable
>>> On 27.08.13 at 15:54, 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 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>Reviewed-by: Jan Beulich <jbeulich@suse.com> (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)> --- > xen/drivers/char/ehci-dbgp.c | 2 +- > xen/drivers/char/ns16550.c | 24 +++++++++++++----------- > xen/drivers/char/serial.c | 38 +++++++++++++++++++++++++------------- > xen/include/xen/serial.h | 5 +++-- > 4 files changed, 42 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..e8d3232 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,6 +107,9 @@ static void __ns16550_poll(struct cpu_user_regs *regs) > if ( uart->intr_works ) > return; /* Interrupts work - no more polling */ > > + if ( ns16550_ioport_invalid(uart) ) > + return; > + > while ( ns_read_reg(uart, UART_LSR) & UART_LSR_DR ) > serial_rx_interrupt(port, regs); > > @@ -121,10 +129,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 +148,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 +316,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