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

[Keir Fraser <keir.xen@gmail.com>]

On 23/04/2013 20:34, "Ben Guthro" <benjamin.guthro@citrix.com> 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 (like Lenovo T410, and some HP machines of similar vintage)
> 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.
> 
> This implementation 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.
> 
> v2: Formatting, comment adjustment
> 
> Signed-Off-By: Ben Guthro <benjamin.guthro@citrix.com>

Acked-by: Keir Fraser <keir@xen.org>

> ---
>  xen/drivers/char/ns16550.c |   59
> +++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 58 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
> index a91743e..7390874 100644
> --- a/xen/drivers/char/ns16550.c
> +++ b/xen/drivers/char/ns16550.c
> @@ -45,6 +45,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. */
> @@ -123,6 +124,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 )
> @@ -351,7 +356,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;
>  
> @@ -367,6 +372,58 @@ 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. However, this is preferable to spinning in an
> +     * infinite loop, as seen on a Lenovo T430, when serial was enabled.
> +     */
> +    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)
>  {