PCI uart: fix boot hang, and second S3 resume inactive timer list corruption

PATCH In review Tomasz Wroblewski 2 participants 12 messages Aug 26, 2013 – Aug 27, 2013

lore

Share & tools

lore

Tomasz Wroblewski

author

2013-08-26 09:17 UTC #0 lore raw

PATCH 1/1 PCI uart: fix boot hang, and second S3 resume inactive timer list corruption

Couple PCI uart resume/boot fixes

- fix occasional xen boot hang whilst using PCI uart. Dom0 kernel disables ioport responses during PCI system initialisation, causing xen hang if __ns16550_poll() routine happens to be scheduled during that time. Detect and exit. Amended ns16550_ioport_invalid function to only check IER register, which contains three reservered (always 0) bits, therefore it's sufficient for this test.

- fix second s3 resume failures due to inactive timer list corruption. init_timer cannot be safely called multiple times on same timer since it does memset(0) on the structure, erasing the auxiliary member used by linked list code, breaking the inactive timer list in common/timer.c. Moved resume_timer initialisation to ns16550_init_postirq, so it's only done once.

Signed-off-by Tomasz Wroblewski <[email protected]>

xen/drivers/char/ns16550.c +13 ~~-10~~

index 6082c85..3ef096e 100644
@@ -59,6 +59,8 @@ static struct ns16550 {
     u8 bar_idx;
 } ns16550_com[2] = { { 0 } };
 
+static void ns16550_delayed_resume(void *data);
+
 static char ns_read_reg(struct ns16550 *uart, int reg)
 {
     if ( uart->remapped_io_base == NULL )

@@ -73,6 +75,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 +110,16 @@ 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 )
+    {
         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));
 }

@@ -256,6 +268,7 @@ static void __init ns16550_init_postirq(struct serial_port *port)
     serial_async_transmit(port);
 
     init_timer(&uart->timer, ns16550_poll, port, 0);
+    init_timer(&uart->resume_timer, ns16550_delayed_resume, port, 0);
 
     /* Calculate time to fill RX FIFO and/or empty TX FIFO for polling. */
     bits = uart->data_bits + uart->stop_bits + !!uart->parity;

@@ -305,15 +318,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)
 {

@@ -346,7 +350,6 @@ static void ns16550_resume(struct serial_port *port)
     if ( ns16550_ioport_invalid(uart) )
     {
         delayed_resume_tries = RESUME_RETRIES;
-        init_timer(&uart->resume_timer, ns16550_delayed_resume, port, 0);
         set_timer(&uart->resume_timer, NOW() + RESUME_DELAY);
     }
     else

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

Jan Beulich

2013-08-26 11:17 UTC #1 lore raw

Quoted 6 lines

Quoted 1 line
Quoted 1 line · Tomasz Wroblewski
- fix occasional xen boot hang whilst using PCI uart. Dom0 kernel disables ioport responses during PCI system initialisation, causing xen hang if __ns16550_poll() routine happens to be scheduled during that time. Detect and exit. Amended ns16550_ioport_invalid function to only check IER register, which contains three reservered (always 0) bits, therefore it's sufficient for this test.

And this was observed with 4.4-unstable? I'm asking because I would at a first glance have thought that taking care of this ought to be a desirable side effect of calling pci_hide_device().

xen/drivers/char/ns16550.c static void ns_write_reg(struct ns16550 *uart, int reg, char c) · line 81

Quoted 4 lines

xen/drivers/char/ns16550.c +4 -0

+static int ns16550_ioport_invalid(struct ns16550 *uart)
+{
+    return (((unsigned char)ns_read_reg(uart, UART_IER)) == 0xff);
+}

Why checking just one register is sufficient when originally

xen/drivers/char/ns16550.c static void _ns16550_resume(struct serial_port *port) · line 315

Quoted 8 lines

xen/drivers/char/ns16550.c +0 -8

-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));
-}

we checked five also needs some better explanation.

Jan

Tomasz Wroblewski

author

2013-08-26 11:39 UTC #2 lore raw

Quoted 9 lines · Jan Beulich

Quoted 6 lines
Quoted 1 line
Quoted 1 line · Tomasz Wroblewski
- fix occasional xen boot hang whilst using PCI uart. Dom0 kernel disables ioport responses during PCI system initialisation, causing xen hang if __ns16550_poll() routine happens to be scheduled during that time. Detect and exit. Amended ns16550_ioport_invalid function to only check IER register, which contains three reservered (always 0) bits, therefore it's sufficient for this test.
And this was observed with 4.4-unstable? I'm asking because I would at a first glance have thought that taking care of this ought to be a desirable side effect of calling pci_hide_device().

This was observed with stable 4.3 - it seems to be doing the pci_hide_device as well, so I don't think this affects, or was it bugfixed later? I'm not entirely sure how is pci_hide_device supposed to work though - in my dom0, on 4.3, I am seeing the pci serial card used by xen console, so maybe it is bugged? (or i misunderstand it).

Quoted 6 lines

xen/drivers/char/ns16550.c static void ns_write_reg(struct ns16550 *uart, int reg, char c) · line 81
Quoted 4 lines
xen/drivers/char/ns16550.c +4 -0
+static int ns16550_ioport_invalid(struct ns16550 *uart)
+{
+    return (((unsigned char)ns_read_reg(uart, UART_IER)) == 0xff);
+}
Why checking just one register is sufficient when originally

Quoted 9 lines

xen/drivers/char/ns16550.c static void _ns16550_resume(struct serial_port *port) · line 315

Quoted 8 lines

xen/drivers/char/ns16550.c +0 -8

-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));
-}

we checked five also needs some better explanation.

I believe it's enough to test IER register since it contains 3 reserved bits which are always 0 during normal operation, therefore the condition will never hit then. Made this as a mini optimisation since this function would now be called more frequently.

Quoted 2 lines

Jan

Jan Beulich

2013-08-26 12:54 UTC #3 lore raw

Quoted 12 lines

Quoted 1 line
Quoted 1 line · Tomasz Wroblewski
Quoted 2 lines · Jan Beulich
Quoted 2 lines
Quoted 1 line
Quoted 1 line · Tomasz Wroblewski
- fix occasional xen boot hang whilst using PCI uart. Dom0 kernel disables
ioport responses
Quoted 1 line
Quoted 1 line
during PCI system initialisation, causing xen hang if __ns16550_poll()
routine happens to
Quoted 1 line
Quoted 1 line
be scheduled during that time. Detect and exit. Amended
ns16550_ioport_invalid function
Quoted 1 line
Quoted 1 line
to only check IER register, which contains three reservered (always 0)
bits, therefore
Quoted 1 line
Quoted 1 line
it's sufficient for this test.

Quoted 8 lines

Quoted 3 lines
And this was observed with 4.4-unstable? I'm asking because I would at a first glance have thought that taking care of this ought to be a desirable side effect of calling pci_hide_device().
This was observed with stable 4.3 - it seems to be doing the pci_hide_device as well, so I don't think this affects, or was it bugfixed later? I'm not entirely sure how is pci_hide_device supposed to work though - in my dom0, on 4.3, I am seeing the pci serial card used by xen console, so maybe it is bugged? (or i misunderstand it).

Wait, yes, pci_ro_device() is what would be needed to drop Dom0 writes to the device's config space. But we don't want this if at all possible, as there may be other devices (more serial ports and/or one or more parallel ports) on the same card, and we want to allow Dom0 to drive those.

Nevertheless, the approach of your patch in simply giving up the device (even if only termporarily) looks questionable to me We'd rather need to restore full access to it I would think. But yes, this hypervisor and Dom0 playing with the same device is sort of a gray area.

Quoted 15 lines

Quoted 6 lines
xen/drivers/char/ns16550.c static void ns_write_reg(struct ns16550 *uart, int reg, char c) · line 81
Quoted 4 lines
xen/drivers/char/ns16550.c +4 -0
+static int ns16550_ioport_invalid(struct ns16550 *uart)
+{
+    return (((unsigned char)ns_read_reg(uart, UART_IER)) == 0xff);
+}
Why checking just one register is sufficient when originally
Quoted 9 lines
xen/drivers/char/ns16550.c static void _ns16550_resume(struct serial_port *port) · line 315
Quoted 8 lines
xen/drivers/char/ns16550.c +0 -8
-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));
-}
we checked five also needs some better explanation.

Quoted 4 lines

I believe it's enough to test IER register since it contains 3 reserved bits which are always 0 during normal operation, therefore the condition will never hit then. Made this as a mini optimisation since this function would now be called more frequently.

I assumed it was something like this. But that needs to be said in the patch description.

Jan

Tomasz Wroblewski

author

2013-08-26 13:26 UTC #4 lore raw

Quoted 15 lines

Quoted 9 lines
Quoted 4 lines
And this was observed with 4.4-unstable? I'm asking because I would at a first glance have thought that taking care of this ought to be a desirable side effect of calling pci_hide_device().
This was observed with stable 4.3 - it seems to be doing the pci_hide_device as well, so I don't think this affects, or was it bugfixed later? I'm not entirely sure how is pci_hide_device supposed to work though - in my dom0, on 4.3, I am seeing the pci serial card used by xen console, so maybe it is bugged? (or i misunderstand it).
Wait, yes, pci_ro_device() is what would be needed to drop Dom0 writes to the device's config space. But we don't want this if at all possible, as there may be other devices (more serial ports and/or one or more parallel ports) on the same card, and we want to allow Dom0 to drive those.

Quoted 5 lines

Nevertheless, the approach of your patch in simply giving up the device (even if only termporarily) looks questionable to me We'd rather need to restore full access to it I would think. But yes, this hypervisor and Dom0 playing with the same device is sort of a gray area.

Restore ioport access at the start of poll routine (if not on) and disable it again at the end (if was not on)? I might do that (if you really prefer), but intuitively that seems more likely to produce side effects in dom0 kernel than skipping a poll in xen

Quoted 15 lines

Quoted 15 lines
Quoted 15 lines
xen/drivers/char/ns16550.c static void ns_write_reg(struct ns16550 *uart, int reg, char c) · line 81
Quoted 4 lines
xen/drivers/char/ns16550.c +4 -0
+static int ns16550_ioport_invalid(struct ns16550 *uart)
+{
+    return (((unsigned char)ns_read_reg(uart, UART_IER)) == 0xff);
+}
Why checking just one register is sufficient when originally
xen/drivers/char/ns16550.c static void _ns16550_resume(struct serial_port *port) · line 315
Quoted 8 lines
xen/drivers/char/ns16550.c +0 -8
-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));
-}
we checked five also needs some better explanation.

Quoted 6 lines

Quoted 4 lines
I believe it's enough to test IER register since it contains 3 reserved bits which are always 0 during normal operation, therefore the condition will never hit then. Made this as a mini optimisation since this function would now be called more frequently.
I assumed it was something like this. But that needs to be said in the patch description.

Yeah, I did mention it in the desc though, to quote: "Amended ns16550_ioport_invalid function to only check IER register, which contains three reservered (always 0) bits, therefore it's sufficient for this test", thought that was enough

Quoted 2 lines

Jan

Jan Beulich

2013-08-26 13:52 UTC #5 lore raw

Quoted 10 lines

Quoted 6 lines
Quoted 1 line · Tomasz Wroblewski
Nevertheless, the approach of your patch in simply giving up the device (even if only termporarily) looks questionable to me We'd rather need to restore full access to it I would think. But yes, this hypervisor and Dom0 playing with the same device is sort of a gray area.
Restore ioport access at the start of poll routine (if not on) and disable it again at the end (if was not on)? I might do that (if you really prefer), but intuitively that seems more likely to produce side effects in dom0 kernel than skipping a poll in xen

As long as it's guaranteed to only be a poll (or a few of them) being affected, this is fine. But what if an interrupt is being used?

Jan

Tomasz Wroblewski

author

2013-08-26 15:10 UTC #6 lore raw

Quoted 6 lines · Jan Beulich

Quoted 6 lines
Quoted 6 lines
Quoted 1 line · Tomasz Wroblewski
Nevertheless, the approach of your patch in simply giving up the device (even if only termporarily) looks questionable to me We'd rather need to restore full access to it I would think. But yes, this hypervisor and Dom0 playing with the same device is sort of a gray area.

Quoted 6 lines

Quoted 4 lines
Restore ioport access at the start of poll routine (if not on) and disable it again at the end (if was not on)? I might do that (if you really prefer), but intuitively that seems more likely to produce side effects in dom0 kernel than skipping a poll in xen
As long as it's guaranteed to only be a poll (or a few of them) being affected, this is fine. But what if an interrupt is being used?

I'm probably missing something so can you elaborate on this? Probably not what you are asking, but ns16550_interrupt function currently doesn't hang when ioports are disabled as a byproduct of the "while ( !(ns_read_reg(uart, IIR) & IIR_NOINT) )" test in there, which already causes it to break out on 0xFF regs

Quoted 2 lines

Jan

Jan Beulich

2013-08-26 15:26 UTC #7 lore raw

Quoted 12 lines

Quoted 1 line
Quoted 1 line · Tomasz Wroblewski
Quoted 10 lines · Jan Beulich
Quoted 10 lines
Quoted 6 lines
Quoted 1 line · Tomasz Wroblewski
Nevertheless, the approach of your patch in simply giving up the device (even if only termporarily) looks questionable to me We'd rather need to restore full access to it I would think. But yes, this hypervisor and Dom0 playing with the same device is sort of a gray area.
Restore ioport access at the start of poll routine (if not on) and disable it again at the end (if was not on)? I might do that (if you really prefer), but intuitively that seems more likely to produce side effects in dom0 kernel than skipping a poll in xen

Quoted 7 lines

Quoted 2 lines
As long as it's guaranteed to only be a poll (or a few of them) being affected, this is fine. But what if an interrupt is being used?
I'm probably missing something so can you elaborate on this? Probably not what you are asking, but ns16550_interrupt function currently
doesn't hang when ioports are disabled as a byproduct of the "while
( !(ns_read_reg(uart, IIR) & IIR_NOINT) )" test in there, which already causes it to break out on 0xFF regs

My question was along the lines of "If I/O port access is disabled, isn't the whole driver screwed (even if only temporarily)?" And if the answer to this is "yes" (I can't see it to be "no"), dealing with this likely requires more than the change you proposed.

Jan

Tomasz Wroblewski

author

2013-08-26 16:13 UTC #8 lore raw

Quoted 19 lines · Jan Beulich

Quoted 12 lines
Quoted 1 line
Quoted 1 line · Tomasz Wroblewski
Quoted 10 lines · Jan Beulich
Quoted 10 lines
Quoted 6 lines
Quoted 1 line · Tomasz Wroblewski
Nevertheless, the approach of your patch in simply giving up the device (even if only termporarily) looks questionable to me We'd rather need to restore full access to it I would think. But yes, this hypervisor and Dom0 playing with the same device is sort of a gray area.
Restore ioport access at the start of poll routine (if not on) and disable it again at the end (if was not on)? I might do that (if you really prefer), but intuitively that seems more likely to produce side effects in dom0 kernel than skipping a poll in xen
Quoted 7 lines
Quoted 2 lines
As long as it's guaranteed to only be a poll (or a few of them) being affected, this is fine. But what if an interrupt is being used?
I'm probably missing something so can you elaborate on this? Probably not what you are asking, but ns16550_interrupt function currently
doesn't hang when ioports are disabled as a byproduct of the "while
( !(ns_read_reg(uart, IIR)& IIR_NOINT) )" test in there, which already causes it to break out on 0xFF regs

Quoted 4 lines

My question was along the lines of "If I/O port access is disabled, isn't the whole driver screwed (even if only temporarily)?" And if the answer to this is "yes" (I can't see it to be "no"), dealing with this likely requires more than the change you proposed.

It could be, I only have empirical evidence of not noticing any serial out hiccups during dom0 kernel init. Since this is is small driver and it seems to primarily interact with the I/O only in ns16550_interrupt, ns16550_poll, ns16550_tx_ready, putc, getc (and in some init functions but these will only be called before dom0 boot), I thought that:

* ns16550_interrupt will be fine with IO ports disabled, it'll just exit
* ns16550_poll will be fine with the posted patch, it'll exit
* ns16550_getc looks like it has a potential of producing 0xFF characters incorrectly, so maybe would need a port test as well
* ns16550_putc should be fine since write to ioport will just be dropped
* ns16550_tx_ready should be fine, it will return 1 if ioports are disabled which is what it needs to be returning to avoid spinning in serial.c

So besides possibly the extra check in getc, not really sure what else can be done better here

Quoted 2 lines

Jan

Jan Beulich

2013-08-27 06:55 UTC #9 lore raw

Quoted 6 lines

Quoted 1 line
Quoted 1 line · Tomasz Wroblewski
Quoted 4 lines · Jan Beulich
My question was along the lines of "If I/O port access is disabled, isn't the whole driver screwed (even if only temporarily)?" And if the answer to this is "yes" (I can't see it to be "no"), dealing with this likely requires more than the change you proposed.

Quoted 7 lines

It could be, I only have empirical evidence of not noticing any serial out hiccups during dom0 kernel init. Since this is is small driver and it seems to primarily interact with the I/O only in ns16550_interrupt, ns16550_poll, ns16550_tx_ready, putc, getc (and in some init functions but these will only be called before dom0 boot), I thought that:
* ns16550_interrupt will be fine with IO ports disabled, it'll just exit

Ah, right, the flag being tested is a "no-interrupt-pending" one. Good.

Quoted 3 lines

* ns16550_poll will be fine with the posted patch, it'll exit
* ns16550_getc looks like it has a potential of producing 0xFF characters incorrectly, so maybe would need a port test as well

Right.

Quoted 1 line

* ns16550_putc should be fine since write to ioport will just be dropped

Ideally it would of course postpone the writes, see below.

Quoted 3 lines

* ns16550_tx_ready should be fine, it will return 1 if ioports are disabled which is what it needs to be returning to avoid spinning in serial.c

Actually, one could probably tweak this so that at least in the non- sync, non-log-everything case it serial.c would prefer buffering over calling ->putc() (and hence dropping), by allowing ->tx_ready() to indicate this "disconnected" state via a special return value.

Jan

Tomasz Wroblewski

author

2013-08-27 08:52 UTC #10 lore raw

Quoted 24 lines · Jan Beulich

Quoted 6 lines
Quoted 1 line
Quoted 1 line · Tomasz Wroblewski
Quoted 4 lines · Jan Beulich
My question was along the lines of "If I/O port access is disabled, isn't the whole driver screwed (even if only temporarily)?" And if the answer to this is "yes" (I can't see it to be "no"), dealing with this likely requires more than the change you proposed.
Quoted 7 lines
It could be, I only have empirical evidence of not noticing any serial out hiccups during dom0 kernel init. Since this is is small driver and it seems to primarily interact with the I/O only in ns16550_interrupt, ns16550_poll, ns16550_tx_ready, putc, getc (and in some init functions but these will only be called before dom0 boot), I thought that:
* ns16550_interrupt will be fine with IO ports disabled, it'll just exit
Ah, right, the flag being tested is a "no-interrupt-pending" one. Good.
Quoted 3 lines
* ns16550_poll will be fine with the posted patch, it'll exit
* ns16550_getc looks like it has a potential of producing 0xFF characters incorrectly, so maybe would need a port test as well
Right.
Quoted 1 line
* ns16550_putc should be fine since write to ioport will just be dropped
Ideally it would of course postpone the writes, see below.

Quoted 8 lines

Quoted 3 lines
* ns16550_tx_ready should be fine, it will return 1 if ioports are disabled which is what it needs to be returning to avoid spinning in serial.c
Actually, one could probably tweak this so that at least in the non- sync, non-log-everything case it serial.c would prefer buffering over calling ->putc() (and hence dropping), by allowing ->tx_ready() to indicate this "disconnected" state via a special return value.

Aha okay. I'm trying to come up with something along the above lines then. I think I'll split this patch into two, one for the timer list fix (which I think is not controversial) and other for the hang/port unavailable case

Quoted 2 lines

Jan

Jan Beulich

2013-08-27 09:02 UTC #11 lore raw

Quoted 4 lines

Quoted 1 line
Quoted 1 line · Tomasz Wroblewski
I think I'll split this patch into two, one for the timer list fix (which I think is not controversial) and other for the hang/port unavailable case

That'd be nice, yes.

Jan