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

[Pasi Kärkkäinen <pasik@xxxxxx>]

On Wed, Jan 16, 2013 at 09:40:40PM +0000, Malcolm Crossley wrote:
> 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?
>

At least my T430 laptop is "Intel Core i7 vPro" model, so it has the 
Intel AMT (Active Management Technology), which provides Serial Over LAN (SOL) 
port.

-- Pasi

 
> 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@xxxxxxxxxx>
> >>>---
> >>>   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@xxxxxxxxxxxxx
> >>>http://lists.xen.org/xen-devel
> >_______________________________________________
> >Xen-devel mailing list
> >Xen-devel@xxxxxxxxxxxxx
> >http://lists.xen.org/xen-devel
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxx
> http://lists.xen.org/xen-devel

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