[PATCH 0 of 3] fix S3 suspend and resume error

[PATCH 0 of 3] fix S3 suspend and resume error

[Guanqun Lu]

This series fix S3 suspend and resume error,
which is mainly caused by inconsistent spinlock usage.

Signed-off-by: Guanqun Lu <guanqun.lu@intel.com>

[PATCH 1 of 3] fix S3 suspend error

[Guanqun Lu]

# HG changeset patch
# User Guanqun Lu <guanqun.lu@intel.com>
# Date 1235401592 -28800
# Node ID b713518a241b6f6e07edb10c3fc38143138f82ea
# Parent  84af3ded5b02ba0b3f8647e3bfa993725428633b
fix S3 suspend error.

invoke ioapic_pm_state_alloc() eariler,
thus avoiding check_lock() BUG_ON() in spin_lock().

Signed-off-by: Guanqun Lu <guanqun.lu@intel.com>

diff -r 84af3ded5b02 -r b713518a241b xen/arch/x86/io_apic.c
--- a/xen/arch/x86/io_apic.c	Thu Feb 19 11:07:33 2009 +0000
+++ b/xen/arch/x86/io_apic.c	Mon Feb 23 23:06:32 2009 +0800
@@ -1779,6 +1779,22 @@ static inline void check_timer(void)
  */
 #define PIC_IRQS	(1 << PIC_CASCADE_IR)
 
+struct IO_APIC_route_entry *ioapic_pm_state=NULL;
+
+void ioapic_pm_state_alloc(void)
+{
+    int i, nr_entry = 0;
+
+    if (ioapic_pm_state != NULL)
+        return;
+
+    for (i = 0; i < nr_ioapics; i++)
+        nr_entry += nr_ioapic_registers[i];
+
+    ioapic_pm_state = _xmalloc(sizeof(struct IO_APIC_route_entry)*nr_entry,
+                               sizeof(struct IO_APIC_route_entry));
+}
+
 void __init setup_IO_APIC(void)
 {
     enable_IO_APIC();
@@ -1801,24 +1817,9 @@ void __init setup_IO_APIC(void)
     init_IO_APIC_traps();
     check_timer();
     print_IO_APIC();
+    ioapic_pm_state_alloc();
 
     register_keyhandler('z', print_IO_APIC_keyhandler, "print ioapic info");
-}
-
-struct IO_APIC_route_entry *ioapic_pm_state=NULL;
-
-void ioapic_pm_state_alloc(void)
-{
-    int i, nr_entry = 0;
-
-    if (ioapic_pm_state != NULL)
-        return;
-
-    for (i = 0; i < nr_ioapics; i++)
-        nr_entry += nr_ioapic_registers[i];
-
-    ioapic_pm_state = _xmalloc(sizeof(struct IO_APIC_route_entry)*nr_entry,
-                               sizeof(struct IO_APIC_route_entry));
 }
 
 int ioapic_suspend(void)
@@ -1826,8 +1827,6 @@ int ioapic_suspend(void)
     struct IO_APIC_route_entry *entry;
     unsigned long flags;
     int apic,i;
-
-    ioapic_pm_state_alloc();
 
     if (ioapic_pm_state == NULL) {
         printk("Cannot suspend ioapic due to lack of memory\n");

Re: [PATCH 1 of 3] fix S3 suspend error

[Keir Fraser]

On 23/02/2009 07:27, "Guanqun Lu" <guanqun.lu@intel.com> wrote:

> fix S3 suspend error.
> 
> invoke ioapic_pm_state_alloc() eariler,
> thus avoiding check_lock() BUG_ON() in spin_lock().
> 
> Signed-off-by: Guanqun Lu <guanqun.lu@intel.com>

This one I think is a good idea anyway. The other two -- I wonder why we do
device suspend/resume with IRQs disabled? As far as I can tell Linux does
not do that. So would it be cleaner to move the device suspend/resume hooks
outside the IRQs-disabled region?

 -- Keir

RE: [PATCH 1 of 3] fix S3 suspend error

[Tian, Kevin]

>From: Keir Fraser
>Sent: Monday, February 23, 2009 8:39 PM
>
>On 23/02/2009 07:27, "Guanqun Lu" <guanqun.lu@intel.com> wrote:
>
>> fix S3 suspend error.
>> 
>> invoke ioapic_pm_state_alloc() eariler,
>> thus avoiding check_lock() BUG_ON() in spin_lock().
>> 
>> Signed-off-by: Guanqun Lu <guanqun.lu@intel.com>
>
>This one I think is a good idea anyway. The other two -- I 
>wonder why we do
>device suspend/resume with IRQs disabled? As far as I can tell 
>Linux does
>not do that. So would it be cleaner to move the device 
>suspend/resume hooks
>outside the IRQs-disabled region?
>

I can't ensure my understanding exactly matching latest Linux
S3 implementation which evolves quickly. But generally device
drivers typically are allowed with two callbacks, one being invoked
with irq enabled and the other with irq disabled, since some actions
have such requirement. On the other side, system device class,
such as ioapic, lapic, etc. are saved with irq disabled after other
external devices.

Xen owns a few devices with most as system class, so for 
simplicity interrupt is disabled from start. It's not worthy of touching
this flow for this simple spinlock issue, imo. :-)

Thanks,
Kevin

Re: [PATCH 1 of 3] fix S3 suspend error

[Keir Fraser]

On 23/02/2009 04:46, "Tian, Kevin" <kevin.tian@intel.com> wrote:

> I can't ensure my understanding exactly matching latest Linux
> S3 implementation which evolves quickly. But generally device
> drivers typically are allowed with two callbacks, one being invoked
> with irq enabled and the other with irq disabled, since some actions
> have such requirement. On the other side, system device class,
> such as ioapic, lapic, etc. are saved with irq disabled after other
> external devices.
> 
> Xen owns a few devices with most as system class, so for
> simplicity interrupt is disabled from start. It's not worthy of touching
> this flow for this simple spinlock issue, imo. :-)

Can you point out any core devices in modern Linux 2.6 which implement the
suspend_late() or *_noirq() callbacks (those appear to be the ones handled
in device_power_down() with IRQs disabled)? They seem very obsolete/legacy,
and looks to me like we could unconditionally *not* disable IRQs rather than
unconditionally disabling them.

 -- Keir

Re: [PATCH 1 of 3] fix S3 suspend error

[Keir Fraser]

On 23/02/2009 05:15, "Keir Fraser" <keir.fraser@eu.citrix.com> wrote:

>> Xen owns a few devices with most as system class, so for
>> simplicity interrupt is disabled from start. It's not worthy of touching
>> this flow for this simple spinlock issue, imo. :-)
> 
> Can you point out any core devices in modern Linux 2.6 which implement the
> suspend_late() or *_noirq() callbacks (those appear to be the ones handled
> in device_power_down() with IRQs disabled)? They seem very obsolete/legacy,
> and looks to me like we could unconditionally *not* disable IRQs rather than
> unconditionally disabling them.

Oh, yes, now I see the sysdev_suspend() stuff. Okay, I agree with the
patches then.

 -- Keir

[PATCH 2 of 3] fix S3 suspend error

[Guanqun Lu]

# HG changeset patch
# User Guanqun Lu <guanqun.lu@intel.com>
# Date 1235401621 -28800
# Node ID 0e17f070db2aa92bc74f29245cdb8528aa72bbe3
# Parent  b713518a241b6f6e07edb10c3fc38143138f82ea
fix S3 suspend error

platform_time_calibration() is invoked in S3 when irq is disabled,
which causes ASSERT() error in spin_lock_irq(). spin_lock_irqsave()
saves us.

Signed-off-by: Guanqun Lu <guanqun.lu@intel.com>

diff -r b713518a241b -r 0e17f070db2a xen/arch/x86/time.c
--- a/xen/arch/x86/time.c	Mon Feb 23 23:06:32 2009 +0800
+++ b/xen/arch/x86/time.c	Mon Feb 23 23:07:01 2009 +0800
@@ -607,13 +607,14 @@ static void platform_time_calibration(vo
 {
     u64 count;
     s_time_t stamp;
-
-    spin_lock_irq(&platform_timer_lock);
+    unsigned long flags;
+
+    spin_lock_irqsave(&platform_timer_lock, flags);
     count = plt_stamp64 + ((plt_src.read_counter() - plt_stamp) & plt_mask);
     stamp = __read_platform_stime(count);
     stime_platform_stamp = stamp;
     platform_timer_stamp = count;
-    spin_unlock_irq(&platform_timer_lock);
+    spin_unlock_irqrestore(&platform_timer_lock, flags);
 }
 
 static void resume_platform_timer(void)

[PATCH 3 of 3] fix S3 resume error

[Guanqun Lu]

# HG changeset patch
# User Guanqun Lu <guanqun.lu@intel.com>
# Date 1235401627 -28800
# Node ID 04e7e4e8519fbbd2ec141875d466ea8ca038d553
# Parent  0e17f070db2aa92bc74f29245cdb8528aa72bbe3
fix S3 resume error

spin_lock() usage in do_settime() in this situation is invoked
when irq is disabled, which causes check_lock() BUG_ON().
We bypass the check by using spin_debug_disable().  It's safe
since no other CPUs are online yet at this moment.

Signed-off-by: Guanqun Lu <guanqun.lu@intel.com>

diff -r 0e17f070db2a -r 04e7e4e8519f xen/arch/x86/time.c
--- a/xen/arch/x86/time.c	Mon Feb 23 23:07:01 2009 +0800
+++ b/xen/arch/x86/time.c	Mon Feb 23 23:07:07 2009 +0800
@@ -1300,7 +1300,9 @@ int time_resume(void)
 
     init_percpu_time();
 
+    spin_debug_disable();
     do_settime(get_cmos_time() + cmos_utc_offset, 0, NOW());
+    spin_debug_enable();
 
     update_vcpu_system_time(current);