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

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

[Tomasz Wroblewski]

[-- Attachment #1: Type: text/plain, Size: 1 bytes --]



[-- Attachment #2: pci-uart-fixes --]
[-- Type: text/plain, Size: 3485 bytes --]

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 <tomasz.wroblewski@citrix.com>

diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index 6082c85..3ef096e 100644
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -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

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH] PCI uart: fix boot hang, and second S3 resume inactive timer list corruption

[Jan Beulich]

>>> On 26.08.13 at 11:17, Tomasz Wroblewski <tomasz.wroblewski@citrix.com> wrote:
>- 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().

>+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

>-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

Re: [PATCH] PCI uart: fix boot hang, and second S3 resume inactive timer list corruption

[Tomasz Wroblewski]

On 08/26/2013 01:17 PM, Jan Beulich wrote:
>>>> On 26.08.13 at 11:17, Tomasz Wroblewski<tomasz.wroblewski@citrix.com>  wrote:
>> - 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).

>> +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
>
>> -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.
> Jan
>

Re: [PATCH] PCI uart: fix boot hang, and second S3 resume inactive timer list corruption

[Jan Beulich]

>>> On 26.08.13 at 13:39, Tomasz Wroblewski <tomasz.wroblewski@citrix.com> wrote:
> On 08/26/2013 01:17 PM, Jan Beulich wrote:
>>>>> On 26.08.13 at 11:17, Tomasz Wroblewski<tomasz.wroblewski@citrix.com>  wrote:
>>> - 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).

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.

>>> +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
>>
>>> -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.

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

Jan

Re: [PATCH] PCI uart: fix boot hang, and second S3 resume inactive timer list corruption

[Tomasz Wroblewski]

>>> 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.
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

>>>> +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
>>>
>>>> -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.
> 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

> Jan
>

Re: [PATCH] PCI uart: fix boot hang, and second S3 resume inactive timer list corruption

[Jan Beulich]

>>> On 26.08.13 at 15:25, Tomasz Wroblewski <tomasz.wroblewski@citrix.com> wrote:
>> 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

Re: [PATCH] PCI uart: fix boot hang, and second S3 resume inactive timer list corruption

[Tomasz Wroblewski]

On 08/26/2013 03:52 PM, Jan Beulich wrote:
>>>> On 26.08.13 at 15:25, Tomasz Wroblewski<tomasz.wroblewski@citrix.com>  wrote:
>>> 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?
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


> Jan
>

Re: [PATCH] PCI uart: fix boot hang, and second S3 resume inactive timer list corruption

[Jan Beulich]

>>> On 26.08.13 at 17:09, Tomasz Wroblewski <tomasz.wroblewski@citrix.com> wrote:
> On 08/26/2013 03:52 PM, Jan Beulich wrote:
>>>>> On 26.08.13 at 15:25, Tomasz Wroblewski<tomasz.wroblewski@citrix.com>  wrote:
>>>> 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?
> 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

Re: [PATCH] PCI uart: fix boot hang, and second S3 resume inactive timer list corruption

[Tomasz Wroblewski]

On 08/26/2013 05:26 PM, Jan Beulich wrote:
>>>> On 26.08.13 at 17:09, Tomasz Wroblewski<tomasz.wroblewski@citrix.com>  wrote:
>> On 08/26/2013 03:52 PM, Jan Beulich wrote:
>>>>>> On 26.08.13 at 15:25, Tomasz Wroblewski<tomasz.wroblewski@citrix.com>   wrote:
>>>>> 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?
>> 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.
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
> Jan
>

Re: [PATCH] PCI uart: fix boot hang, and second S3 resume inactive timer list corruption

[Jan Beulich]

>>> On 26.08.13 at 18:12, Tomasz Wroblewski <tomasz.wroblewski@citrix.com> wrote:
> On 08/26/2013 05:26 PM, Jan Beulich wrote:
>> 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

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

> * 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.

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

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

> * 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

Re: [PATCH] PCI uart: fix boot hang, and second S3 resume inactive timer list corruption

[Tomasz Wroblewski]

On 08/27/2013 08:55 AM, Jan Beulich wrote:
>>>> On 26.08.13 at 18:12, Tomasz Wroblewski<tomasz.wroblewski@citrix.com>  wrote:
>> On 08/26/2013 05:26 PM, Jan Beulich wrote:
>>> 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
> Ah, right, the flag being tested is a "no-interrupt-pending" one.
> Good.
>
>> * 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.
>
>> * ns16550_putc should be fine since write to ioport will just be dropped
> Ideally it would of course postpone the writes, see below.
>
>> * 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
> Jan
>

Re: [PATCH] PCI uart: fix boot hang, and second S3 resume inactive timer list corruption

[Jan Beulich]

>>> On 27.08.13 at 10:52, Tomasz Wroblewski <tomasz.wroblewski@citrix.com> wrote:
> 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