Re: [PATCH] ns16550: delay resume until dom0 ACPI has a chance to run

[Malcolm Crossley <malcolm.crossley@citrix.com>]

On 16/01/13 21:31, Ben Guthro wrote:
>
> On 01/16/2013 04:24 PM, Pasi Kärkkäinen wrote:
>> On Wed, Jan 16, 2013 at 03:20:56PM -0500, Ben Guthro wrote:
>>> Check for ioport access, before fully resuming operation, to avoid
>>> spinning in __ns16550_poll when reading the LSR register returns 0xFF
>>> on failing ioport access.
>>>
>>> On some systems, there is a SuperIO card that provides this legacy ioport
>>> on the LPC bus.
>>>
>>> In this case, we need to wait for dom0's ACPI processing to run the proper
>>> AML to re-initialize the chip, before we can use the card again.
>>>
>>> This may cause a small amount of garbage to be written
>>> to the serial log while we wait patiently for that AML to
>>> be executed.
>>>
>>> It limits the number of retries, to avoid a situation where we keep trying
>>> over and over again, in the case of some other failure on the ioport.
>>>
>> So I assume this patch fixes the ACPI S3 suspend/resume on the Lenovo T430/T530 laptops?
>> It might be good to mention that aswell..
> Yes, it seems to resolve the issues seen on T430, T530, and Intel
> Ivybridge mobile SDP. It likely fixes others, as the Intel mobile SDP is
> the reference platform for all the OEMs.
>
> If I need to re-submit this patch for any other reason, I will be sure
> to mention this in the comment.
>
> That said - there seems to still be an issue on some older Sandy Bridge
> machines, like a Lenovo T520, and X220 - but I don't yet know if it is
> related.

Do these laptops (T430/T530) have built in serial?

Or is the hang fixed because you were using a serial Express card to debug?

BTW, nearly all PC's have external SUPERIO devices, it just seems we 
have managed to be lucky that most platforms seem to default to using 
the BIOS to enable the serial port after resume instead of the OS.

Malcolm
> Ben
>
>
>
>> -- Pasi
>>
>>
>>> Signed-Off-By: Ben Guthro <benjamin.guthro@citrix.com>
>>> ---
>>>    xen/drivers/char/ns16550.c |   56 +++++++++++++++++++++++++++++++++++++++++++-
>>>    1 file changed, 55 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
>>> index d77042e..27555c7 100644
>>> --- a/xen/drivers/char/ns16550.c
>>> +++ b/xen/drivers/char/ns16550.c
>>> @@ -42,6 +42,7 @@ static struct ns16550 {
>>>        struct irqaction irqaction;
>>>        /* UART with no IRQ line: periodically-polled I/O. */
>>>        struct timer timer;
>>> +    struct timer resume_timer;
>>>        unsigned int timeout_ms;
>>>        bool_t intr_works;
>>>        /* PCI card parameters. */
>>> @@ -120,6 +121,10 @@ static struct ns16550 {
>>>    /* Frequency of external clock source. This definition assumes PC platform. */
>>>    #define UART_CLOCK_HZ   1843200
>>>
>>> +/* Resume retry settings */
>>> +#define RESUME_DELAY    MILLISECS(10)
>>> +#define RESUME_RETRIES  100
>>> +
>>>    static char ns_read_reg(struct ns16550 *uart, int reg)
>>>    {
>>>        if ( uart->remapped_io_base == NULL )
>>> @@ -330,7 +335,7 @@ static void ns16550_suspend(struct serial_port *port)
>>>                                      uart->ps_bdf[2], PCI_COMMAND);
>>>    }
>>>
>>> -static void ns16550_resume(struct serial_port *port)
>>> +static void __ns16550_resume(struct serial_port *port)
>>>    {
>>>        struct ns16550 *uart = port->uart;
>>>
>>> @@ -346,6 +351,55 @@ 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, LSR)) == 0xff) &&
>>> +            (((unsigned char)ns_read_reg(uart, MCR)) == 0xff) &&
>>> +            (((unsigned char)ns_read_reg(uart, IER)) == 0xff) &&
>>> +            (((unsigned char)ns_read_reg(uart, IIR)) == 0xff) &&
>>> +            (((unsigned char)ns_read_reg(uart, LCR)) == 0xff));
>>> +}
>>> +
>>> +static int delayed_resume_tries;
>>> +static void ns16550_delayed_resume(void *data)
>>> +{
>>> +    struct serial_port *port = data;
>>> +    struct ns16550 *uart = port->uart;
>>> +
>>> +    if (ns16550_ioport_invalid(port->uart) && delayed_resume_tries--) {
>>> +	set_timer(&uart->resume_timer, NOW() + RESUME_DELAY);
>>> +	return;
>>> +    }
>>> +
>>> +    __ns16550_resume(port);
>>> +}
>>> +
>>> +static void ns16550_resume(struct serial_port *port)
>>> +{
>>> +    struct ns16550 *uart = port->uart;
>>> +
>>> +    /*
>>> +     * Check for ioport access, before fully resuming operation.
>>> +     * On some systems, there is a SuperIO card that provides
>>> +     * this legacy ioport on the LPC bus.
>>> +     *
>>> +     * We need to wait for dom0's ACPI processing to run the proper
>>> +     * AML to re-initialize the chip, before we can use the card again.
>>> +     *
>>> +     * This may cause a small amount of garbage to be written
>>> +     * to the serial log while we wait patiently for that AML to
>>> +     * be executed.
>>> +     */
>>> +    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);
>>> +	return;
>>> +    }
>>> +
>>> +    __ns16550_resume(port);
>>> +}
>>> +
>>>    #ifdef CONFIG_X86
>>>    static void __init ns16550_endboot(struct serial_port *port)
>>>    {
>>> --
>>> 1.7.9.5
>>>
>>>
>>> _______________________________________________
>>> Xen-devel mailing list
>>> Xen-devel@lists.xen.org
>>> http://lists.xen.org/xen-devel
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel