Cpuidle drivers,Suspend : Fix suspend/resume hang with intel_idle driver

Cpuidle drivers,Suspend : Fix suspend/resume hang with intel_idle driver

[preeti]

From: Preeti U Murthy <preeti@linux.vnet.ibm.com>

Dave Hansen reported problems with suspend/resume when used
with intel_idle driver.More such problems were noticed.
https://bugs.launchpad.net/ubuntu/+source/linux/+bug/674075.

The reason for this could not be suspected till he reported
resume hang issue when used with acpi idle driver on his Lenovo-S10-3
Atom netbook.

The patch-[PM / ACPI: Fix suspend/resume regression caused by cpuidle
cleanup],fixed this issue by ensuring that acpi idle drivers prevent
cpus from going into deeper sleep states during suspend to prevent
resume hang on certain bios.
http://marc.info/?l=linux-pm&m=133958534231884&w=2

Commit b04e7bdb984e3b7f62fb7f44146a529f88cc7639
(ACPI: disable lower idle C-states across suspend/resume) throws
light on the resume hang issue on certain specific bios.

Also the following lines in drivers/idle/intel_idle.c suggest intel_idle
drivers should also ensure cpus are prevented from entering idle states
during suspend to avoid a resume hang.

/*
 * Known limitations
 * [..]
 *
 * ACPI has a .suspend hack to turn off deep c-states during suspend
 * to avoid complications with the lapic timer workaround.
 * Have not seen issues with suspend, but may need same workaround here.
 *
 */
This patch aims at having this fix in a place common to both the idle
drivers.

Suspend is enabled only if ACPI is active on x86.Thus the setting of
acpi_idle_suspend during suspend is moved up to ACPI specific code with
both acpi and intel idle drivers checking if it is valid to enter deeper
idle states.The setting of acpi_idle_suspend is done via PM_SUSPEND_PREPARE
notifiers to avoid race conditions between processors entering idle states
and the ongoing process of suspend.

The check on acpi_idle_suspend is included in the most appropriate header
so as to be visible to both the idle drivers irrespective of the
different configurations.Even if ACPI is disabled intel idle drivers can
still carry out the acpi_idle_suspend check.

Reported-by: Dave Hansen <dave@linux.vnet.ibm.com>
Reviewed-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
Reviewed-by: Deepthi Dharwar <deepthi@linux.vnet.ibm.com>
Signed-off-by: Preeti U Murthy <preeti@linux.vnet.ibm.com>
---
Dave, does this patch ensure suspend/resume works
fine on your netbook if intel_idle driver is enabled?

 drivers/acpi/processor_idle.c |   46 +++++++++++++++++++----------------------
 drivers/acpi/sleep.c          |   38 ++++++++++++++++++++++++++++++++++
 drivers/idle/intel_idle.c     |   10 +++++++++
 include/linux/suspend.h       |   24 +++++++++++++++++++++
 4 files changed, 93 insertions(+), 25 deletions(-)


diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
index c2ffd84..3ab0963 100644
--- a/drivers/acpi/processor_idle.c
+++ b/drivers/acpi/processor_idle.c
@@ -41,7 +41,7 @@
 #include <linux/clockchips.h>
 #include <linux/cpuidle.h>
 #include <linux/irqflags.h>
-
+#include <linux/suspend.h>
 /*
  * Include the apic definitions for x86 to have the APIC timer related defines
  * available also for UP (on SMP it gets magically included via linux/smp.h).
@@ -221,10 +221,6 @@ static void lapic_timer_state_broadcast(struct acpi_processor *pr,
 
 #endif
 
-/*
- * Suspend / resume control
- */
-static int acpi_idle_suspend;
 static u32 saved_bm_rld;
 
 static void acpi_idle_bm_rld_save(void)
@@ -243,21 +239,13 @@ static void acpi_idle_bm_rld_restore(void)
 
 int acpi_processor_suspend(struct acpi_device * device, pm_message_t state)
 {
-	if (acpi_idle_suspend == 1)
-		return 0;
-
 	acpi_idle_bm_rld_save();
-	acpi_idle_suspend = 1;
 	return 0;
 }
 
 int acpi_processor_resume(struct acpi_device * device)
 {
-	if (acpi_idle_suspend == 0)
-		return 0;
-
 	acpi_idle_bm_rld_restore();
-	acpi_idle_suspend = 0;
 	return 0;
 }
 
@@ -762,11 +750,13 @@ static int acpi_idle_enter_c1(struct cpuidle_device *dev,
 		return -EINVAL;
 
 	local_irq_disable();
-
-	if (acpi_idle_suspend) {
+	/*
+	 * Do not enter idle states if you are in suspend path
+	 */
+	if (in_suspend_path()) {
 		local_irq_enable();
 		cpu_relax();
-		return -EINVAL;
+		return -EBUSY;
 	}
 
 	lapic_timer_state_broadcast(pr, cx, 1);
@@ -838,10 +828,13 @@ static int acpi_idle_enter_simple(struct cpuidle_device *dev,
 
 	local_irq_disable();
 
-	if (acpi_idle_suspend) {
+	/*
+	 * Do not enter idle states if you are in suspend path
+	 */
+	if (in_suspend_path()) {
 		local_irq_enable();
 		cpu_relax();
-		return -EINVAL;
+		return -EBUSY;
 	}
 
 	if (cx->entry_method != ACPI_CSTATE_FFH) {
@@ -922,12 +915,6 @@ static int acpi_idle_enter_bm(struct cpuidle_device *dev,
 	if (unlikely(!pr))
 		return -EINVAL;
 
-	if (acpi_idle_suspend) {
-		if (irqs_disabled())
-			local_irq_enable();
-		cpu_relax();
-		return -EINVAL;
-	}
 
 	if (!cx->bm_sts_skip && acpi_idle_bm_check()) {
 		if (drv->safe_state_index >= 0) {
@@ -935,13 +922,22 @@ static int acpi_idle_enter_bm(struct cpuidle_device *dev,
 						drv, drv->safe_state_index);
 		} else {
 			local_irq_disable();
-			acpi_safe_halt();
+			if (!(in_suspend_path()))
+				acpi_safe_halt();
 			local_irq_enable();
 			return -EINVAL;
 		}
 	}
 
 	local_irq_disable();
+	/*
+	 * Do not enter idle states if you are in suspend path
+	 */
+	if (in_suspend_path()) {
+		local_irq_enable();
+		cpu_relax();
+		return -EBUSY;
+	}
 
 	if (cx->entry_method != ACPI_CSTATE_FFH) {
 		current_thread_info()->status &= ~TS_POLLING;
diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
index 8856102..4ec77db 100644
--- a/drivers/acpi/sleep.c
+++ b/drivers/acpi/sleep.c
@@ -28,6 +28,11 @@
 #include "internal.h"
 #include "sleep.h"
 
+/*
+ * Suspend/Resume control
+ */
+int acpi_idle_suspend;
+
 u8 wake_sleep_flags = ACPI_NO_OPTIONAL_METHODS;
 static unsigned int gts, bfs;
 static int set_param_wake_flag(const char *val, struct kernel_param *kp)
@@ -905,6 +910,36 @@ static void __init acpi_gts_bfs_check(void)
 	}
 }
 
+/**
+ * cpuidle_pm_callback - On some bios, resume hangs
+ * if idle states are entered during suspend.
+ *
+ * acpi_idle_suspend is used by the x86 idle drivers
+ * to decide whether to go into idle states or not.
+ */
+static int
+cpuidle_pm_callback(struct notifier_block *nb,
+			unsigned long action, void *ptr)
+{
+	switch (action) {
+
+	case PM_SUSPEND_PREPARE:
+		if (acpi_idle_suspend == 0)
+			acpi_idle_suspend = 1;
+		break;
+
+	case PM_POST_SUSPEND:
+		if (acpi_idle_suspend == 1)
+			acpi_idle_suspend = 0;
+		break;
+
+	default:
+		return NOTIFY_DONE;
+	}
+
+	return NOTIFY_OK;
+}
+
 int __init acpi_sleep_init(void)
 {
 	acpi_status status;
@@ -932,6 +967,9 @@ int __init acpi_sleep_init(void)
 
 	suspend_set_ops(old_suspend_ordering ?
 		&acpi_suspend_ops_old : &acpi_suspend_ops);
+
+	pm_notifier(cpuidle_pm_callback, 0);
+
 #endif
 
 #ifdef CONFIG_HIBERNATION
diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
index d0f59c3..a595207 100644
--- a/drivers/idle/intel_idle.c
+++ b/drivers/idle/intel_idle.c
@@ -65,6 +65,7 @@
 #include <asm/cpu_device_id.h>
 #include <asm/mwait.h>
 #include <asm/msr.h>
+#include <linux/suspend.h>
 
 #define INTEL_IDLE_VERSION "0.4"
 #define PREFIX "intel_idle: "
@@ -255,6 +256,15 @@ static int intel_idle(struct cpuidle_device *dev,
 	cstate = (((eax) >> MWAIT_SUBSTATE_SIZE) & MWAIT_CSTATE_MASK) + 1;
 
 	/*
+	 * Do not enter idle states if you are in suspend path
+	 */
+	if (in_suspend_path()) {
+		local_irq_enable();
+		cpu_relax();
+		return -EBUSY;
+	}
+
+	/*
 	 * leave_mm() to avoid costly and often unnecessary wakeups
 	 * for flushing the user TLB's associated with the active mm.
 	 */
diff --git a/include/linux/suspend.h b/include/linux/suspend.h
index 0c808d7..eb96670 100644
--- a/include/linux/suspend.h
+++ b/include/linux/suspend.h
@@ -38,6 +38,30 @@ typedef int __bitwise suspend_state_t;
 #define PM_SUSPEND_MEM		((__force suspend_state_t) 3)
 #define PM_SUSPEND_MAX		((__force suspend_state_t) 4)
 
+extern int acpi_idle_suspend;
+
+/**
+ * in_suspend_path - X86 idle drivers make a call
+ * to this function before entering idle states.
+ *
+ * Entering idle states is prevented if it is in suspend
+ * path.
+ */
+#ifdef CONFIG_ACPI
+static inline int in_suspend_path(void)
+{
+	if (acpi_idle_suspend == 1)
+		return 1;
+	else
+		return 0;
+}
+#else
+static inline int in_suspend_path(void)
+{
+	return 0;
+}
+#endif
+
 enum suspend_stat_step {
 	SUSPEND_FREEZE = 1,
 	SUSPEND_PREPARE,

Regards,
Preeti U Murthy

Re: Cpuidle drivers,Suspend : Fix suspend/resume hang with intel_idle driver

[Rafael J. Wysocki]

On Thursday, June 28, 2012, preeti wrote:
> 
> From: Preeti U Murthy <preeti@linux.vnet.ibm.com>
> 
> Dave Hansen reported problems with suspend/resume when used
> with intel_idle driver.More such problems were noticed.
> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/674075.
> 
> The reason for this could not be suspected till he reported
> resume hang issue when used with acpi idle driver on his Lenovo-S10-3
> Atom netbook.
> 
> The patch-[PM / ACPI: Fix suspend/resume regression caused by cpuidle
> cleanup],fixed this issue by ensuring that acpi idle drivers prevent
> cpus from going into deeper sleep states during suspend to prevent
> resume hang on certain bios.
> http://marc.info/?l=linux-pm&m=133958534231884&w=2
> 
> Commit b04e7bdb984e3b7f62fb7f44146a529f88cc7639
> (ACPI: disable lower idle C-states across suspend/resume) throws
> light on the resume hang issue on certain specific bios.
> 
> Also the following lines in drivers/idle/intel_idle.c suggest intel_idle
> drivers should also ensure cpus are prevented from entering idle states
> during suspend to avoid a resume hang.
> 
> /*
>  * Known limitations
>  * [..]
>  *
>  * ACPI has a .suspend hack to turn off deep c-states during suspend
>  * to avoid complications with the lapic timer workaround.
>  * Have not seen issues with suspend, but may need same workaround here.
>  *
>  */
> This patch aims at having this fix in a place common to both the idle
> drivers.
> 
> Suspend is enabled only if ACPI is active on x86.Thus the setting of
> acpi_idle_suspend during suspend is moved up to ACPI specific code with
> both acpi and intel idle drivers checking if it is valid to enter deeper
> idle states.The setting of acpi_idle_suspend is done via PM_SUSPEND_PREPARE
> notifiers to avoid race conditions between processors entering idle states
> and the ongoing process of suspend.
> 
> The check on acpi_idle_suspend is included in the most appropriate header
> so as to be visible to both the idle drivers irrespective of the
> different configurations.Even if ACPI is disabled intel idle drivers can
> still carry out the acpi_idle_suspend check.
> 
> Reported-by: Dave Hansen <dave@linux.vnet.ibm.com>
> Reviewed-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
> Reviewed-by: Deepthi Dharwar <deepthi@linux.vnet.ibm.com>
> Signed-off-by: Preeti U Murthy <preeti@linux.vnet.ibm.com>
> ---
> Dave, does this patch ensure suspend/resume works
> fine on your netbook if intel_idle driver is enabled?
> 
>  drivers/acpi/processor_idle.c |   46 +++++++++++++++++++----------------------
>  drivers/acpi/sleep.c          |   38 ++++++++++++++++++++++++++++++++++
>  drivers/idle/intel_idle.c     |   10 +++++++++
>  include/linux/suspend.h       |   24 +++++++++++++++++++++
>  4 files changed, 93 insertions(+), 25 deletions(-)
> 
> 
> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
> index c2ffd84..3ab0963 100644
> --- a/drivers/acpi/processor_idle.c
> +++ b/drivers/acpi/processor_idle.c
> @@ -41,7 +41,7 @@
>  #include <linux/clockchips.h>
>  #include <linux/cpuidle.h>
>  #include <linux/irqflags.h>
> -
> +#include <linux/suspend.h>
>  /*
>   * Include the apic definitions for x86 to have the APIC timer related defines
>   * available also for UP (on SMP it gets magically included via linux/smp.h).
> @@ -221,10 +221,6 @@ static void lapic_timer_state_broadcast(struct acpi_processor *pr,
>  
>  #endif
>  
> -/*
> - * Suspend / resume control
> - */
> -static int acpi_idle_suspend;
>  static u32 saved_bm_rld;
>  
>  static void acpi_idle_bm_rld_save(void)
> @@ -243,21 +239,13 @@ static void acpi_idle_bm_rld_restore(void)
>  
>  int acpi_processor_suspend(struct acpi_device * device, pm_message_t state)
>  {
> -	if (acpi_idle_suspend == 1)
> -		return 0;
> -
>  	acpi_idle_bm_rld_save();
> -	acpi_idle_suspend = 1;
>  	return 0;
>  }

So, this seems to be on top of the "PM / ACPI: Fix suspend/resume regression
caused by cpuidle" patch, right?

>  int acpi_processor_resume(struct acpi_device * device)
>  {
> -	if (acpi_idle_suspend == 0)
> -		return 0;
> -
>  	acpi_idle_bm_rld_restore();
> -	acpi_idle_suspend = 0;
>  	return 0;
>  }
>  
> @@ -762,11 +750,13 @@ static int acpi_idle_enter_c1(struct cpuidle_device *dev,
>  		return -EINVAL;
>  
>  	local_irq_disable();
> -
> -	if (acpi_idle_suspend) {
> +	/*
> +	 * Do not enter idle states if you are in suspend path
> +	 */
> +	if (in_suspend_path()) {

This is more than ugly.  Can't you simply use the acpi_idle_suspend _variable_
here?

Besides, why the name of the variable is acpi_idle_suspend, if it is used
by the Intel driver too?

>  		local_irq_enable();
>  		cpu_relax();
> -		return -EINVAL;
> +		return -EBUSY;

That change is made by the "PM / ACPI: Fix suspend/resume regression
caused by cpuidle" patch already.

>  	}
>  
>  	lapic_timer_state_broadcast(pr, cx, 1);
> @@ -838,10 +828,13 @@ static int acpi_idle_enter_simple(struct cpuidle_device *dev,
>  
>  	local_irq_disable();
>  
> -	if (acpi_idle_suspend) {
> +	/*
> +	 * Do not enter idle states if you are in suspend path
> +	 */
> +	if (in_suspend_path()) {
>  		local_irq_enable();
>  		cpu_relax();
> -		return -EINVAL;
> +		return -EBUSY;
>  	}
>  
>  	if (cx->entry_method != ACPI_CSTATE_FFH) {
> @@ -922,12 +915,6 @@ static int acpi_idle_enter_bm(struct cpuidle_device *dev,
>  	if (unlikely(!pr))
>  		return -EINVAL;
>  
> -	if (acpi_idle_suspend) {
> -		if (irqs_disabled())
> -			local_irq_enable();
> -		cpu_relax();
> -		return -EINVAL;
> -	}

The last version of the "PM / ACPI: Fix suspend/resume regression
caused by cpuidle" patch didn't contain this hunk.

>  
>  	if (!cx->bm_sts_skip && acpi_idle_bm_check()) {
>  		if (drv->safe_state_index >= 0) {
> @@ -935,13 +922,22 @@ static int acpi_idle_enter_bm(struct cpuidle_device *dev,
>  						drv, drv->safe_state_index);
>  		} else {
>  			local_irq_disable();
> -			acpi_safe_halt();
> +			if (!(in_suspend_path()))
> +				acpi_safe_halt();
>  			local_irq_enable();
>  			return -EINVAL;
>  		}
>  	}
>  
>  	local_irq_disable();
> +	/*
> +	 * Do not enter idle states if you are in suspend path
> +	 */
> +	if (in_suspend_path()) {
> +		local_irq_enable();
> +		cpu_relax();
> +		return -EBUSY;
> +	}
>  
>  	if (cx->entry_method != ACPI_CSTATE_FFH) {
>  		current_thread_info()->status &= ~TS_POLLING;
> diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
> index 8856102..4ec77db 100644
> --- a/drivers/acpi/sleep.c
> +++ b/drivers/acpi/sleep.c
> @@ -28,6 +28,11 @@
>  #include "internal.h"
>  #include "sleep.h"
>  
> +/*
> + * Suspend/Resume control
> + */
> +int acpi_idle_suspend;
> +
>  u8 wake_sleep_flags = ACPI_NO_OPTIONAL_METHODS;
>  static unsigned int gts, bfs;
>  static int set_param_wake_flag(const char *val, struct kernel_param *kp)
> @@ -905,6 +910,36 @@ static void __init acpi_gts_bfs_check(void)
>  	}
>  }
>  
> +/**
> + * cpuidle_pm_callback - On some bios, resume hangs
> + * if idle states are entered during suspend.
> + *
> + * acpi_idle_suspend is used by the x86 idle drivers
> + * to decide whether to go into idle states or not.
> + */
> +static int
> +cpuidle_pm_callback(struct notifier_block *nb,
> +			unsigned long action, void *ptr)
> +{
> +	switch (action) {
> +
> +	case PM_SUSPEND_PREPARE:
> +		if (acpi_idle_suspend == 0)
> +			acpi_idle_suspend = 1;
> +		break;
> +
> +	case PM_POST_SUSPEND:
> +		if (acpi_idle_suspend == 1)
> +			acpi_idle_suspend = 0;
> +		break;
> +
> +	default:
> +		return NOTIFY_DONE;
> +	}
> +
> +	return NOTIFY_OK;
> +}

Why don't you put this notifier into cpuidle instead?

> +
>  int __init acpi_sleep_init(void)
>  {
>  	acpi_status status;
> @@ -932,6 +967,9 @@ int __init acpi_sleep_init(void)
>  
>  	suspend_set_ops(old_suspend_ordering ?
>  		&acpi_suspend_ops_old : &acpi_suspend_ops);
> +
> +	pm_notifier(cpuidle_pm_callback, 0);
> +
>  #endif
>  
>  #ifdef CONFIG_HIBERNATION
> diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
> index d0f59c3..a595207 100644
> --- a/drivers/idle/intel_idle.c
> +++ b/drivers/idle/intel_idle.c
> @@ -65,6 +65,7 @@
>  #include <asm/cpu_device_id.h>
>  #include <asm/mwait.h>
>  #include <asm/msr.h>
> +#include <linux/suspend.h>
>  
>  #define INTEL_IDLE_VERSION "0.4"
>  #define PREFIX "intel_idle: "
> @@ -255,6 +256,15 @@ static int intel_idle(struct cpuidle_device *dev,
>  	cstate = (((eax) >> MWAIT_SUBSTATE_SIZE) & MWAIT_CSTATE_MASK) + 1;
>  
>  	/*
> +	 * Do not enter idle states if you are in suspend path
> +	 */
> +	if (in_suspend_path()) {
> +		local_irq_enable();
> +		cpu_relax();
> +		return -EBUSY;
> +	}
> +
> +	/*
>  	 * leave_mm() to avoid costly and often unnecessary wakeups
>  	 * for flushing the user TLB's associated with the active mm.
>  	 */
> diff --git a/include/linux/suspend.h b/include/linux/suspend.h
> index 0c808d7..eb96670 100644
> --- a/include/linux/suspend.h
> +++ b/include/linux/suspend.h
> @@ -38,6 +38,30 @@ typedef int __bitwise suspend_state_t;
>  #define PM_SUSPEND_MEM		((__force suspend_state_t) 3)
>  #define PM_SUSPEND_MAX		((__force suspend_state_t) 4)
>  
> +extern int acpi_idle_suspend;
> +
> +/**
> + * in_suspend_path - X86 idle drivers make a call
> + * to this function before entering idle states.
> + *
> + * Entering idle states is prevented if it is in suspend
> + * path.
> + */
> +#ifdef CONFIG_ACPI

Why #ifdef CONFIG_ACPI?

> +static inline int in_suspend_path(void)
> +{
> +	if (acpi_idle_suspend == 1)
> +		return 1;
> +	else
> +		return 0;
> +}
> +#else
> +static inline int in_suspend_path(void)
> +{
> +	return 0;
> +}
> +#endif
> +
>  enum suspend_stat_step {
>  	SUSPEND_FREEZE = 1,
>  	SUSPEND_PREPARE,

Thanks,
Rafael

Re: Cpuidle drivers,Suspend : Fix suspend/resume hang with intel_idle driver

[preeti]

On 06/28/2012 03:23 PM, Rafael J. Wysocki wrote:
> On Thursday, June 28, 2012, preeti wrote:
>>
>> From: Preeti U Murthy <preeti@linux.vnet.ibm.com>
>>
>> Dave Hansen reported problems with suspend/resume when used
>> with intel_idle driver.More such problems were noticed.
>> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/674075.
>>
>> The reason for this could not be suspected till he reported
>> resume hang issue when used with acpi idle driver on his Lenovo-S10-3
>> Atom netbook.
>>
>> The patch-[PM / ACPI: Fix suspend/resume regression caused by cpuidle
>> cleanup],fixed this issue by ensuring that acpi idle drivers prevent
>> cpus from going into deeper sleep states during suspend to prevent
>> resume hang on certain bios.
>> http://marc.info/?l=linux-pm&m=133958534231884&w=2
>>
>> Commit b04e7bdb984e3b7f62fb7f44146a529f88cc7639
>> (ACPI: disable lower idle C-states across suspend/resume) throws
>> light on the resume hang issue on certain specific bios.
>>
>> Also the following lines in drivers/idle/intel_idle.c suggest intel_idle
>> drivers should also ensure cpus are prevented from entering idle states
>> during suspend to avoid a resume hang.
>>
>> /*
>>  * Known limitations
>>  * [..]
>>  *
>>  * ACPI has a .suspend hack to turn off deep c-states during suspend
>>  * to avoid complications with the lapic timer workaround.
>>  * Have not seen issues with suspend, but may need same workaround here.
>>  *
>>  */
>> This patch aims at having this fix in a place common to both the idle
>> drivers.
>>
>> Suspend is enabled only if ACPI is active on x86.Thus the setting of
>> acpi_idle_suspend during suspend is moved up to ACPI specific code with
>> both acpi and intel idle drivers checking if it is valid to enter deeper
>> idle states.The setting of acpi_idle_suspend is done via PM_SUSPEND_PREPARE
>> notifiers to avoid race conditions between processors entering idle states
>> and the ongoing process of suspend.
>>
>> The check on acpi_idle_suspend is included in the most appropriate header
>> so as to be visible to both the idle drivers irrespective of the
>> different configurations.Even if ACPI is disabled intel idle drivers can
>> still carry out the acpi_idle_suspend check.
>>
>> Reported-by: Dave Hansen <dave@linux.vnet.ibm.com>
>> Reviewed-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
>> Reviewed-by: Deepthi Dharwar <deepthi@linux.vnet.ibm.com>
>> Signed-off-by: Preeti U Murthy <preeti@linux.vnet.ibm.com>
>> ---
>> Dave, does this patch ensure suspend/resume works
>> fine on your netbook if intel_idle driver is enabled?
>>
>>  drivers/acpi/processor_idle.c |   46 +++++++++++++++++++----------------------
>>  drivers/acpi/sleep.c          |   38 ++++++++++++++++++++++++++++++++++
>>  drivers/idle/intel_idle.c     |   10 +++++++++
>>  include/linux/suspend.h       |   24 +++++++++++++++++++++
>>  4 files changed, 93 insertions(+), 25 deletions(-)
>>
>>
>> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
>> index c2ffd84..3ab0963 100644
>> --- a/drivers/acpi/processor_idle.c
>> +++ b/drivers/acpi/processor_idle.c
>> @@ -41,7 +41,7 @@
>>  #include <linux/clockchips.h>
>>  #include <linux/cpuidle.h>
>>  #include <linux/irqflags.h>
>> -
>> +#include <linux/suspend.h>
>>  /*
>>   * Include the apic definitions for x86 to have the APIC timer related defines
>>   * available also for UP (on SMP it gets magically included via linux/smp.h).
>> @@ -221,10 +221,6 @@ static void lapic_timer_state_broadcast(struct acpi_processor *pr,
>>  
>>  #endif
>>  
>> -/*
>> - * Suspend / resume control
>> - */
>> -static int acpi_idle_suspend;
>>  static u32 saved_bm_rld;
>>  
>>  static void acpi_idle_bm_rld_save(void)
>> @@ -243,21 +239,13 @@ static void acpi_idle_bm_rld_restore(void)
>>  
>>  int acpi_processor_suspend(struct acpi_device * device, pm_message_t state)
>>  {
>> -	if (acpi_idle_suspend == 1)
>> -		return 0;
>> -
>>  	acpi_idle_bm_rld_save();
>> -	acpi_idle_suspend = 1;
>>  	return 0;
>>  }
> 
> So, this seems to be on top of the "PM / ACPI: Fix suspend/resume regression
> caused by cpuidle" patch, right?
> 
This patch is on top of the PM/ACPI fix which is posted in the below link
 http://marc.info/?l=linux-pm&m=133958534231884&w=2 and adhering to the
suggestions posted in reply to it.

Very sorry about the patch not being applied on the latest
linux-pm/linux-next tree.If this code structure is agreed upon,
I will post the patch as relevant to the latest linux-pm/linux-next tree.

>>  int acpi_processor_resume(struct acpi_device * device)
>>  {
>> -	if (acpi_idle_suspend == 0)
>> -		return 0;
>> -
>>  	acpi_idle_bm_rld_restore();
>> -	acpi_idle_suspend = 0;
>>  	return 0;
>>  }
>>  
>> @@ -762,11 +750,13 @@ static int acpi_idle_enter_c1(struct cpuidle_device *dev,
>>  		return -EINVAL;
>>  
>>  	local_irq_disable();
>> -
>> -	if (acpi_idle_suspend) {
>> +	/*
>> +	 * Do not enter idle states if you are in suspend path
>> +	 */
>> +	if (in_suspend_path()) {
> 
> This is more than ugly.  Can't you simply use the acpi_idle_suspend _variable_
> here?
> 
> Besides, why the name of the variable is acpi_idle_suspend, if it is used
> by the Intel driver too?
> 
The acpi_idle_suspend is defined in ACPI specific code.So if ACPI
is commented out in the configs ,then intel_idle driver cannot reference
acpi_idle_suspend.

Yes the variable can be named better.I made an attempt to retain the variable
name from the PM/ACPI patch.This will be corrected.

>>  		local_irq_enable();
>>  		cpu_relax();
>> -		return -EINVAL;
>> +		return -EBUSY;
> 
> That change is made by the "PM / ACPI: Fix suspend/resume regression
> caused by cpuidle" patch already.

This confusion,which i regret, has been addressed above.
> 
>>  	}
>>  
>>  	lapic_timer_state_broadcast(pr, cx, 1);
>> @@ -838,10 +828,13 @@ static int acpi_idle_enter_simple(struct cpuidle_device *dev,
>>  
>>  	local_irq_disable();
>>  
>> -	if (acpi_idle_suspend) {
>> +	/*
>> +	 * Do not enter idle states if you are in suspend path
>> +	 */
>> +	if (in_suspend_path()) {
>>  		local_irq_enable();
>>  		cpu_relax();
>> -		return -EINVAL;
>> +		return -EBUSY;
>>  	}
>>  
>>  	if (cx->entry_method != ACPI_CSTATE_FFH) {
>> @@ -922,12 +915,6 @@ static int acpi_idle_enter_bm(struct cpuidle_device *dev,
>>  	if (unlikely(!pr))
>>  		return -EINVAL;
>>  
>> -	if (acpi_idle_suspend) {
>> -		if (irqs_disabled())
>> -			local_irq_enable();
>> -		cpu_relax();
>> -		return -EINVAL;
>> -	}
> 
> The last version of the "PM / ACPI: Fix suspend/resume regression
> caused by cpuidle" patch didn't contain this hunk.
> 
Addressed above.
>>  
>>  	if (!cx->bm_sts_skip && acpi_idle_bm_check()) {
>>  		if (drv->safe_state_index >= 0) {
>> @@ -935,13 +922,22 @@ static int acpi_idle_enter_bm(struct cpuidle_device *dev,
>>  						drv, drv->safe_state_index);
>>  		} else {
>>  			local_irq_disable();
>> -			acpi_safe_halt();
>> +			if (!(in_suspend_path()))
>> +				acpi_safe_halt();
>>  			local_irq_enable();
>>  			return -EINVAL;
>>  		}
>>  	}
>>  
>>  	local_irq_disable();
>> +	/*
>> +	 * Do not enter idle states if you are in suspend path
>> +	 */
>> +	if (in_suspend_path()) {
>> +		local_irq_enable();
>> +		cpu_relax();
>> +		return -EBUSY;
>> +	}
>>  
>>  	if (cx->entry_method != ACPI_CSTATE_FFH) {
>>  		current_thread_info()->status &= ~TS_POLLING;
>> diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
>> index 8856102..4ec77db 100644
>> --- a/drivers/acpi/sleep.c
>> +++ b/drivers/acpi/sleep.c
>> @@ -28,6 +28,11 @@
>>  #include "internal.h"
>>  #include "sleep.h"
>>  
>> +/*
>> + * Suspend/Resume control
>> + */
>> +int acpi_idle_suspend;
>> +
>>  u8 wake_sleep_flags = ACPI_NO_OPTIONAL_METHODS;
>>  static unsigned int gts, bfs;
>>  static int set_param_wake_flag(const char *val, struct kernel_param *kp)
>> @@ -905,6 +910,36 @@ static void __init acpi_gts_bfs_check(void)
>>  	}
>>  }
>>  
>> +/**
>> + * cpuidle_pm_callback - On some bios, resume hangs
>> + * if idle states are entered during suspend.
>> + *
>> + * acpi_idle_suspend is used by the x86 idle drivers
>> + * to decide whether to go into idle states or not.
>> + */
>> +static int
>> +cpuidle_pm_callback(struct notifier_block *nb,
>> +			unsigned long action, void *ptr)
>> +{
>> +	switch (action) {
>> +
>> +	case PM_SUSPEND_PREPARE:
>> +		if (acpi_idle_suspend == 0)
>> +			acpi_idle_suspend = 1;
>> +		break;
>> +
>> +	case PM_POST_SUSPEND:
>> +		if (acpi_idle_suspend == 1)
>> +			acpi_idle_suspend = 0;
>> +		break;
>> +
>> +	default:
>> +		return NOTIFY_DONE;
>> +	}
>> +
>> +	return NOTIFY_OK;
>> +}
> 
> Why don't you put this notifier into cpuidle instead?

cpuidle is an architecture independent part of the kernel  code.Since 
this patch aims at x86 architecture in specific,I considered it
inappropriate.

In addition to this,suspend happens on x86 only if ACPI is configured.
Therefore it seemed right to put the callback in ACPI specific code
which deals with ACPI sleep support.
> 
>> +
>>  int __init acpi_sleep_init(void)
>>  {
>>  	acpi_status status;
>> @@ -932,6 +967,9 @@ int __init acpi_sleep_init(void)
>>  
>>  	suspend_set_ops(old_suspend_ordering ?
>>  		&acpi_suspend_ops_old : &acpi_suspend_ops);
>> +
>> +	pm_notifier(cpuidle_pm_callback, 0);
>> +
>>  #endif
>>  
>>  #ifdef CONFIG_HIBERNATION
>> diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
>> index d0f59c3..a595207 100644
>> --- a/drivers/idle/intel_idle.c
>> +++ b/drivers/idle/intel_idle.c
>> @@ -65,6 +65,7 @@
>>  #include <asm/cpu_device_id.h>
>>  #include <asm/mwait.h>
>>  #include <asm/msr.h>
>> +#include <linux/suspend.h>
>>  
>>  #define INTEL_IDLE_VERSION "0.4"
>>  #define PREFIX "intel_idle: "
>> @@ -255,6 +256,15 @@ static int intel_idle(struct cpuidle_device *dev,
>>  	cstate = (((eax) >> MWAIT_SUBSTATE_SIZE) & MWAIT_CSTATE_MASK) + 1;
>>  
>>  	/*
>> +	 * Do not enter idle states if you are in suspend path
>> +	 */
>> +	if (in_suspend_path()) {
>> +		local_irq_enable();
>> +		cpu_relax();
>> +		return -EBUSY;
>> +	}
>> +
>> +	/*
>>  	 * leave_mm() to avoid costly and often unnecessary wakeups
>>  	 * for flushing the user TLB's associated with the active mm.
>>  	 */
>> diff --git a/include/linux/suspend.h b/include/linux/suspend.h
>> index 0c808d7..eb96670 100644
>> --- a/include/linux/suspend.h
>> +++ b/include/linux/suspend.h
>> @@ -38,6 +38,30 @@ typedef int __bitwise suspend_state_t;
>>  #define PM_SUSPEND_MEM		((__force suspend_state_t) 3)
>>  #define PM_SUSPEND_MAX		((__force suspend_state_t) 4)
>>  
>> +extern int acpi_idle_suspend;
>> +
>> +/**
>> + * in_suspend_path - X86 idle drivers make a call
>> + * to this function before entering idle states.
>> + *
>> + * Entering idle states is prevented if it is in suspend
>> + * path.
>> + */
>> +#ifdef CONFIG_ACPI
> 
> Why #ifdef CONFIG_ACPI?

As mentioned above suspend happens on x86 only if ACPI is configured.
The setting of acpi_idle_suspend occurs in ACPI specific part of the code,
which will not be present if ACPI is configured out. Hence this check.
> 
>> +static inline int in_suspend_path(void)
>> +{
>> +	if (acpi_idle_suspend == 1)
>> +		return 1;
>> +	else
>> +		return 0;
>> +}
>> +#else
>> +static inline int in_suspend_path(void)
>> +{
>> +	return 0;
>> +}
>> +#endif
>> +
>>  enum suspend_stat_step {
>>  	SUSPEND_FREEZE = 1,
>>  	SUSPEND_PREPARE,
> 
> Thanks,
> Rafael
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Regards
Preeti

Re: Cpuidle drivers, Suspend : Fix suspend/resume hang with intel_idle driver

[Rafael J. Wysocki]

On Thursday, June 28, 2012, preeti wrote:
> On 06/28/2012 03:23 PM, Rafael J. Wysocki wrote:
> > On Thursday, June 28, 2012, preeti wrote:
> >>
> >> From: Preeti U Murthy <preeti@linux.vnet.ibm.com>
[...]
> cpuidle is an architecture independent part of the kernel  code.Since 
> this patch aims at x86 architecture in specific,I considered it
> inappropriate.
> 
> In addition to this,suspend happens on x86 only if ACPI is configured.

But that is not required for intel_idle, so if it hangs with intel_idle,
then it is not dependent on ACPI after all.

> Therefore it seemed right to put the callback in ACPI specific code
> which deals with ACPI sleep support.

I wonder if we can address this issue correctly.  That is, in a non-racy
way and in a better place.

First, I really don't think it is necessary to "suspend" cpuidle (be it
ACPI or any other) when device drivers' suspend routines are being
executed (which also is racy, because the cpuidle "suspend" may be running
concurrently with cpuidle on another CPU) or earlier.  We really may want
to disable the deeper C-states when we're about to execute
suspend_ops->prepare_late(), or hibernation_ops->prepare(), i.e. after
we've run dpm_suspend_end() successfully.

So, it seems, it might be a good idea to place a cpuidle driver suspend
(and/or hibernation) hook at the end of dpm_suspend_end() and make ACPI
and intel_idle drivers implement that hook.

That would be much more deterministic than your patch I think.

Thanks,
Rafael

Re: Cpuidle drivers,Suspend : Fix suspend/resume hang with intel_idle driver

[preeti]

On 06/29/2012 12:41 AM, Rafael J. Wysocki wrote:
> On Thursday, June 28, 2012, preeti wrote:
>> On 06/28/2012 03:23 PM, Rafael J. Wysocki wrote:
>>> On Thursday, June 28, 2012, preeti wrote:
>>>>
>>>> From: Preeti U Murthy <preeti@linux.vnet.ibm.com>
> [...]
>> cpuidle is an architecture independent part of the kernel  code.Since 
>> this patch aims at x86 architecture in specific,I considered it
>> inappropriate.
>>
>> In addition to this,suspend happens on x86 only if ACPI is configured.
> 
> But that is not required for intel_idle, so if it hangs with intel_idle,
> then it is not dependent on ACPI after all.
True intel_idle does not need ACPI to be configured,but that also means
that the kernel will not provide you the means to suspend.There is no
question of resume hang here at all as you cannot suspend in the first
place.

The issue is when ACPI is configured,and intel_idle is chosen to be the
cpuidle driver.In this situation when the user suspends,cpus can enter
deep sleep states as intel_idle driver does not prevent then from doing so.
This is when resume hangs.
> 
>> Therefore it seemed right to put the callback in ACPI specific code
>> which deals with ACPI sleep support.
> 
> I wonder if we can address this issue correctly.  That is, in a non-racy
> way and in a better place.
> 
> First, I really don't think it is necessary to "suspend" cpuidle (be it
> ACPI or any other) when device drivers' suspend routines are being
> executed (which also is racy, because the cpuidle "suspend" may be running
> concurrently with cpuidle on another CPU) or earlier.  We really may want
> to disable the deeper C-states when we're about to execute
> suspend_ops->prepare_late(), or hibernation_ops->prepare(), i.e. after
> we've run dpm_suspend_end() successfully.

The commit "ACPI:disable lower idle C-states across suspend/resume"
states that device_suspend() calls ACPI suspend functions which cause
side effects on the lower idle C-states.This means we need to disable
entry into deeper C-states even before dpm_suspend_start(),but how much
before?

The commit answers this too.It says removing the functionality of
entering deep C-states before suspend removed the side effects.Besides,
the commit title says'across suspend/resume'.So I think to address the
resume hang effectively,it is desirable to disable entry into deeper
C-states during suspend_prepare operations.
> 
> So, it seems, it might be a good idea to place a cpuidle driver suspend
> (and/or hibernation) hook at the end of dpm_suspend_end() and make ACPI
> and intel_idle drivers implement that hook.
> 
Dont you think this patch is meant to fix a very ACPI specific problem?
Which is why I think the call backs or any hook meant to fix this should
go into ACPI specific code.
Else it will seem irrelevant to all other architectures that implement
suspend.
> That would be much more deterministic than your patch I think.
> 
> Thanks,
> Rafael
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Regards
Preeti

Re: Cpuidle drivers,Suspend : Fix suspend/resume hang with intel_idle driver

[preeti]

On 06/29/2012 09:41 AM, preeti wrote:
> On 06/29/2012 12:41 AM, Rafael J. Wysocki wrote:
>> On Thursday, June 28, 2012, preeti wrote:
>>> On 06/28/2012 03:23 PM, Rafael J. Wysocki wrote:
>>>> On Thursday, June 28, 2012, preeti wrote:
>>>>>
>>>>> From: Preeti U Murthy <preeti@linux.vnet.ibm.com>
>> [...]
>>> cpuidle is an architecture independent part of the kernel  code.Since 
>>> this patch aims at x86 architecture in specific,I considered it
>>> inappropriate.
>>>
>>> In addition to this,suspend happens on x86 only if ACPI is configured.
>>
>> But that is not required for intel_idle, so if it hangs with intel_idle,
>> then it is not dependent on ACPI after all.
> True intel_idle does not need ACPI to be configured,but that also means
> that the kernel will not provide you the means to suspend.There is no
> question of resume hang here at all as you cannot suspend in the first
> place.
> 
> The issue is when ACPI is configured,and intel_idle is chosen to be the
> cpuidle driver.In this situation when the user suspends,cpus can enter
> deep sleep states as intel_idle driver does not prevent then from doing so.
> This is when resume hangs.
>>
>>> Therefore it seemed right to put the callback in ACPI specific code
>>> which deals with ACPI sleep support.
>>
>> I wonder if we can address this issue correctly.  That is, in a non-racy
>> way and in a better place.
>>
>> First, I really don't think it is necessary to "suspend" cpuidle (be it
>> ACPI or any other) when device drivers' suspend routines are being
>> executed (which also is racy, because the cpuidle "suspend" may be running
>> concurrently with cpuidle on another CPU) or earlier.  We really may want
>> to disable the deeper C-states when we're about to execute
>> suspend_ops->prepare_late(), or hibernation_ops->prepare(), i.e. after
>> we've run dpm_suspend_end() successfully.
> 
> The commit "ACPI:disable lower idle C-states across suspend/resume"
> states that device_suspend() calls ACPI suspend functions which cause
> side effects on the lower idle C-states.This means we need to disable
> entry into deeper C-states even before dpm_suspend_start(),but how much
> before?
> 
> The commit answers this too.It says removing the functionality of
> entering deep C-states before suspend removed the side effects.Besides,
> the commit title says'across suspend/resume'.So I think to address the
> resume hang effectively,it is desirable to disable entry into deeper
> C-states during suspend_prepare operations.

To clarify this further,since we take action upon PM_SUSPEND_PREPARE
notification,which is called before suspend begins,we avoid race
condition between suspend operations and disabling entry into deeper
c-states altogether.

>>
>> So, it seems, it might be a good idea to place a cpuidle driver suspend
>> (and/or hibernation) hook at the end of dpm_suspend_end() and make ACPI
>> and intel_idle drivers implement that hook.
>>
> Dont you think this patch is meant to fix a very ACPI specific problem?
> Which is why I think the call backs or any hook meant to fix this should
> go into ACPI specific code.
> Else it will seem irrelevant to all other architectures that implement
> suspend.
>> That would be much more deterministic than your patch I think.
>>
>> Thanks,
>> Rafael
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
> 
> Regards
> Preeti
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

[PATCH] Cpuidle drivers,Suspend : Fix suspend/resume hang with intel_idle driver

[preeti]

From: Preeti U Murthy <preeti@linux.vnet.ibm.com>

Dave Hansen reported problems with suspend/resume when used
with intel_idle driver.More such problems were noticed.
https://bugs.launchpad.net/ubuntu/+source/linux/+bug/674075.

The reason for this could not be suspected till he reported
resume hang issue when used with acpi idle driver on his Lenovo-S10-3
Atom netbook.

The patch-[PM / ACPI: Fix suspend/resume regression caused by cpuidle
cleanup],fixed this issue by ensuring that acpi idle drivers prevent
cpus from going into deeper sleep states during suspend to prevent
resume hang on certain bios.
http://marc.info/?l=linux-pm&m=133958534231884&w=2

Commit b04e7bdb984e3b7f62fb7f44146a529f88cc7639
(ACPI: disable lower idle C-states across suspend/resume) throws
light on the resume hang issue on certain specific bios.

Also the following lines in drivers/idle/intel_idle.c suggest intel_idle
drivers should also ensure cpus are prevented from entering idle states
during suspend to avoid a resume hang.

/*
 * Known limitations
 * [..]
 *
 * ACPI has a .suspend hack to turn off deep c-states during suspend
 * to avoid complications with the lapic timer workaround.
 * Have not seen issues with suspend, but may need same workaround here.
 *
 */
This patch aims at having this fix in a place common to both the idle
drivers.

Suspend is enabled only if ACPI is active on x86.Thus the setting of
acpi_idle_suspend during suspend is moved up to ACPI specific code with
both acpi and intel idle drivers checking if it is valid to enter deeper
idle states.The setting of acpi_idle_suspend is done via PM_SUSPEND_PREPARE
notifiers to avoid race conditions between processors entering idle states
and the ongoing process of suspend.

The check on acpi_idle_suspend is included in the most appropriate header
so as to be visible to both the idle drivers irrespective of the
different configurations.Even if ACPI is disabled intel idle drivers can
still carry out the acpi_idle_suspend check.

Reported-by: Dave Hansen <dave@linux.vnet.ibm.com>
Reviewed-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
Reviewed-by: Deepthi Dharwar <deepthi@linux.vnet.ibm.com>
Signed-off-by: Preeti U Murthy <preeti@linux.vnet.ibm.com>
---

The patch has been applied on the latest linux-pm/linux-next
tree due to the confusion that was present earlier.Also
acpi_idle_suspend has been changed to idle_suspend.Rest of the
code structure is the same.

 drivers/acpi/processor_idle.c |   22 +++++-----------------
 drivers/acpi/sleep.c          |   34 ++++++++++++++++++++++++++++++++++
 drivers/idle/intel_idle.c     |    7 +++++++
 include/linux/suspend.h       |   24 ++++++++++++++++++++++++
 4 files changed, 70 insertions(+), 17 deletions(-)

diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
index 47a8caa..d6704a7 100644
--- a/drivers/acpi/processor_idle.c
+++ b/drivers/acpi/processor_idle.c
@@ -41,7 +41,7 @@
 #include <linux/clockchips.h>
 #include <linux/cpuidle.h>
 #include <linux/irqflags.h>
-
+#include <linux/suspend.h>
 /*
  * Include the apic definitions for x86 to have the APIC timer related
defines
  * available also for UP (on SMP it gets magically included via
linux/smp.h).
@@ -221,10 +221,6 @@ static void lapic_timer_state_broadcast(struct
acpi_processor *pr,

 #endif

-/*
- * Suspend / resume control
- */
-static int acpi_idle_suspend;
 static u32 saved_bm_rld;

 static void acpi_idle_bm_rld_save(void)
@@ -243,21 +239,13 @@ static void acpi_idle_bm_rld_restore(void)

 int acpi_processor_suspend(struct acpi_device * device, pm_message_t state)
 {
-	if (acpi_idle_suspend == 1)
-		return 0;
-
 	acpi_idle_bm_rld_save();
-	acpi_idle_suspend = 1;
 	return 0;
 }

 int acpi_processor_resume(struct acpi_device * device)
 {
-	if (acpi_idle_suspend == 0)
-		return 0;
-
 	acpi_idle_bm_rld_restore();
-	acpi_idle_suspend = 0;
 	return 0;
 }

@@ -763,7 +751,7 @@ static int acpi_idle_enter_c1(struct cpuidle_device
*dev,

 	local_irq_disable();

-	if (acpi_idle_suspend) {
+	if (in_suspend_path()) {
 		local_irq_enable();
 		cpu_relax();
 		return -EBUSY;
@@ -838,7 +826,7 @@ static int acpi_idle_enter_simple(struct
cpuidle_device *dev,

 	local_irq_disable();

-	if (acpi_idle_suspend) {
+	if (in_suspend_path()) {
 		local_irq_enable();
 		cpu_relax();
 		return -EBUSY;
@@ -928,7 +916,7 @@ static int acpi_idle_enter_bm(struct cpuidle_device
*dev,
 						drv, drv->safe_state_index);
 		} else {
 			local_irq_disable();
-			if (!acpi_idle_suspend)
+			if (!(in_suspend_path()))
 				acpi_safe_halt();
 			local_irq_enable();
 			return -EBUSY;
@@ -937,7 +925,7 @@ static int acpi_idle_enter_bm(struct cpuidle_device
*dev,

 	local_irq_disable();

-	if (acpi_idle_suspend) {
+	if (in_suspend_path()) {
 		local_irq_enable();
 		cpu_relax();
 		return -EBUSY;
diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
index 8856102..c052e7e 100644
--- a/drivers/acpi/sleep.c
+++ b/drivers/acpi/sleep.c
@@ -28,6 +28,9 @@
 #include "internal.h"
 #include "sleep.h"

+/* Suspend/Resume control */
+int idle_suspend;
+
 u8 wake_sleep_flags = ACPI_NO_OPTIONAL_METHODS;
 static unsigned int gts, bfs;
 static int set_param_wake_flag(const char *val, struct kernel_param *kp)
@@ -904,6 +907,36 @@ static void __init acpi_gts_bfs_check(void)
 			"please notify linux-acpi@vger.kernel.org\n");
 	}
 }
+/**
+ * cpuidle_pm_callback - On some bios, resume hangs
+ * if idle states are entered during suspend.
+ *
+ * acpi_idle_suspend is used by the x86 idle drivers
+ * to decide whether to go into idle states or not.
+ */
+static int
+cpuidle_pm_callback(struct notifier_block *nb,
+			unsigned long action, void *ptr)
+{
+	switch (action) {
+
+	case PM_SUSPEND_PREPARE:
+		if (idle_suspend == 0)
+			idle_suspend = 1;
+		break;
+
+	case PM_POST_SUSPEND:
+		if (idle_suspend == 1)
+			idle_suspend = 0;
+		break;
+
+	default:
+		return NOTIFY_DONE;
+	}
+
+	return NOTIFY_OK;
+}
+

 int __init acpi_sleep_init(void)
 {
@@ -932,6 +965,7 @@ int __init acpi_sleep_init(void)

 	suspend_set_ops(old_suspend_ordering ?
 		&acpi_suspend_ops_old : &acpi_suspend_ops);
+	pm_notifier(cpuidle_pm_callback, 0);
 #endif

 #ifdef CONFIG_HIBERNATION
diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
index d0f59c3..10555f8 100644
--- a/drivers/idle/intel_idle.c
+++ b/drivers/idle/intel_idle.c
@@ -62,6 +62,7 @@
 #include <linux/notifier.h>
 #include <linux/cpu.h>
 #include <linux/module.h>
+#include <linux/suspend.h>
 #include <asm/cpu_device_id.h>
 #include <asm/mwait.h>
 #include <asm/msr.h>
@@ -254,6 +255,12 @@ static int intel_idle(struct cpuidle_device *dev,

 	cstate = (((eax) >> MWAIT_SUBSTATE_SIZE) & MWAIT_CSTATE_MASK) + 1;

+	if (in_suspend_path()) {
+		local_irq_enable();
+		cpu_relax();
+		return -EBUSY;
+	}
+
 	/*
 	 * leave_mm() to avoid costly and often unnecessary wakeups
 	 * for flushing the user TLB's associated with the active mm.
diff --git a/include/linux/suspend.h b/include/linux/suspend.h
index 0c808d7..8d39a1a 100644
--- a/include/linux/suspend.h
+++ b/include/linux/suspend.h
@@ -38,6 +38,30 @@ typedef int __bitwise suspend_state_t;
 #define PM_SUSPEND_MEM		((__force suspend_state_t) 3)
 #define PM_SUSPEND_MAX		((__force suspend_state_t) 4)

+extern int idle_suspend;
+
+/**
+ * in_suspend_path - X86 idle drivers make a call
+ * to this function before entering idle states.
+ *
+ * Entering idle states is prevented if it is in suspend
+ * path.
+ */
+#ifdef CONFIG_ACPI
+static inline int in_suspend_path(void)
+{
+	if (idle_suspend == 1)
+		return 1;
+	else
+		return 0;
+}
+#else
+static inline int in_suspend_path(void)
+{
+	return 0;
+}
+#endif
+
 enum suspend_stat_step {
 	SUSPEND_FREEZE = 1,
 	SUSPEND_PREPARE,

Re: Cpuidle drivers,Suspend : Fix suspend/resume hang with intel_idle driver

[Rafael J. Wysocki]

On Friday, June 29, 2012, preeti wrote:
> On 06/29/2012 12:41 AM, Rafael J. Wysocki wrote:
> > On Thursday, June 28, 2012, preeti wrote:
> >> On 06/28/2012 03:23 PM, Rafael J. Wysocki wrote:
> >>> On Thursday, June 28, 2012, preeti wrote:
> >>>>
> >>>> From: Preeti U Murthy <preeti@linux.vnet.ibm.com>
> > [...]
> >> cpuidle is an architecture independent part of the kernel  code.Since 
> >> this patch aims at x86 architecture in specific,I considered it
> >> inappropriate.
> >>
> >> In addition to this,suspend happens on x86 only if ACPI is configured.
> > 
> > But that is not required for intel_idle, so if it hangs with intel_idle,
> > then it is not dependent on ACPI after all.
> True intel_idle does not need ACPI to be configured,but that also means
> that the kernel will not provide you the means to suspend.

Yes, it will.  You can hibernate without ACPI in theory.

> There is no question of resume hang here at all as you cannot suspend in
> the first place.
> 
> The issue is when ACPI is configured,and intel_idle is chosen to be the
> cpuidle driver.In this situation when the user suspends,cpus can enter
> deep sleep states as intel_idle driver does not prevent then from doing so.
> This is when resume hangs.

I know that.

> >> Therefore it seemed right to put the callback in ACPI specific code
> >> which deals with ACPI sleep support.
> > 
> > I wonder if we can address this issue correctly.  That is, in a non-racy
> > way and in a better place.
> > 
> > First, I really don't think it is necessary to "suspend" cpuidle (be it
> > ACPI or any other) when device drivers' suspend routines are being
> > executed (which also is racy, because the cpuidle "suspend" may be running
> > concurrently with cpuidle on another CPU) or earlier.  We really may want
> > to disable the deeper C-states when we're about to execute
> > suspend_ops->prepare_late(), or hibernation_ops->prepare(), i.e. after
> > we've run dpm_suspend_end() successfully.
> 
> The commit "ACPI:disable lower idle C-states across suspend/resume"
> states that device_suspend() calls ACPI suspend functions which cause
> side effects on the lower idle C-states.

I'd like to know the details, then.  Which ACPI suspend functions those are,
in particular, because I'm not aware of any in device_suspend().

Also, please note that this commit is 5 years old and some things have changed
in the suspend code paths since that time.

> This means we need to disable entry into deeper C-states even before
> dpm_suspend_start(),

This most likely is wrong.

We may need to "suspend" cpuidle before calling suspend_device_irqs(),
but then again that shouldn't be made via a notifier I think.  Why don't
we simply make suspend_device_irqs() disable lower C-states to start with?

> but how much before?
> 
> The commit answers this too.It says removing the functionality of
> entering deep C-states before suspend removed the side effects.Besides,
> the commit title says'across suspend/resume'.So I think to address the
> resume hang effectively,it is desirable to disable entry into deeper
> C-states during suspend_prepare operations.
> > 
> > So, it seems, it might be a good idea to place a cpuidle driver suspend
> > (and/or hibernation) hook at the end of dpm_suspend_end() and make ACPI
> > and intel_idle drivers implement that hook.
> > 
> Dont you think this patch is meant to fix a very ACPI specific problem?

No, I don't.

> Which is why I think the call backs or any hook meant to fix this should
> go into ACPI specific code.
> Else it will seem irrelevant to all other architectures that implement
> suspend.

I don't honestly think it is irrelevant.  The fact is that similar problems
have not been reported on other architectures _yet_, which by no means can
be taken as a proof that those architectures are not affected.

Anyway, I think that the right way to address this is through a cpuidle driver
callback executed from suspend_device_irqs() (and analogously for the resume
code path) and not through some hacky-ugly ACPI changes.

Thanks,
Rafael

Re: Cpuidle drivers,Suspend : Fix suspend/resume hang with intel_idle driver

[Rafael J. Wysocki]

On Friday, June 29, 2012, preeti wrote:
> On 06/29/2012 09:41 AM, preeti wrote:
> > On 06/29/2012 12:41 AM, Rafael J. Wysocki wrote:
> >> On Thursday, June 28, 2012, preeti wrote:
> >>> On 06/28/2012 03:23 PM, Rafael J. Wysocki wrote:
> >>>> On Thursday, June 28, 2012, preeti wrote:
> >>>>>
> >>>>> From: Preeti U Murthy <preeti@linux.vnet.ibm.com>
> >> [...]
> >>> cpuidle is an architecture independent part of the kernel  code.Since 
> >>> this patch aims at x86 architecture in specific,I considered it
> >>> inappropriate.
> >>>
> >>> In addition to this,suspend happens on x86 only if ACPI is configured.
> >>
> >> But that is not required for intel_idle, so if it hangs with intel_idle,
> >> then it is not dependent on ACPI after all.
> > True intel_idle does not need ACPI to be configured,but that also means
> > that the kernel will not provide you the means to suspend.There is no
> > question of resume hang here at all as you cannot suspend in the first
> > place.
> > 
> > The issue is when ACPI is configured,and intel_idle is chosen to be the
> > cpuidle driver.In this situation when the user suspends,cpus can enter
> > deep sleep states as intel_idle driver does not prevent then from doing so.
> > This is when resume hangs.
> >>
> >>> Therefore it seemed right to put the callback in ACPI specific code
> >>> which deals with ACPI sleep support.
> >>
> >> I wonder if we can address this issue correctly.  That is, in a non-racy
> >> way and in a better place.
> >>
> >> First, I really don't think it is necessary to "suspend" cpuidle (be it
> >> ACPI or any other) when device drivers' suspend routines are being
> >> executed (which also is racy, because the cpuidle "suspend" may be running
> >> concurrently with cpuidle on another CPU) or earlier.  We really may want
> >> to disable the deeper C-states when we're about to execute
> >> suspend_ops->prepare_late(), or hibernation_ops->prepare(), i.e. after
> >> we've run dpm_suspend_end() successfully.
> > 
> > The commit "ACPI:disable lower idle C-states across suspend/resume"
> > states that device_suspend() calls ACPI suspend functions which cause
> > side effects on the lower idle C-states.This means we need to disable
> > entry into deeper C-states even before dpm_suspend_start(),but how much
> > before?
> > 
> > The commit answers this too.It says removing the functionality of
> > entering deep C-states before suspend removed the side effects.Besides,
> > the commit title says'across suspend/resume'.So I think to address the
> > resume hang effectively,it is desirable to disable entry into deeper
> > C-states during suspend_prepare operations.
> 
> To clarify this further,since we take action upon PM_SUSPEND_PREPARE
> notification,which is called before suspend begins,we avoid race
> condition between suspend operations and disabling entry into deeper
> c-states altogether.

Well, what about races between disabling deeper C-states and cpuidle?

Rafael

Re: [PATCH] Cpuidle drivers, Suspend : Fix suspend/resume hang with intel_idle driver

[Rafael J. Wysocki]

On Friday, June 29, 2012, preeti wrote:
> 
> From: Preeti U Murthy <preeti@linux.vnet.ibm.com>
> 
> Dave Hansen reported problems with suspend/resume when used
> with intel_idle driver.More such problems were noticed.
> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/674075.
> 
> The reason for this could not be suspected till he reported
> resume hang issue when used with acpi idle driver on his Lenovo-S10-3
> Atom netbook.
> 
> The patch-[PM / ACPI: Fix suspend/resume regression caused by cpuidle
> cleanup],fixed this issue by ensuring that acpi idle drivers prevent
> cpus from going into deeper sleep states during suspend to prevent
> resume hang on certain bios.
> http://marc.info/?l=linux-pm&m=133958534231884&w=2
> 
> Commit b04e7bdb984e3b7f62fb7f44146a529f88cc7639
> (ACPI: disable lower idle C-states across suspend/resume) throws
> light on the resume hang issue on certain specific bios.
> 
> Also the following lines in drivers/idle/intel_idle.c suggest intel_idle
> drivers should also ensure cpus are prevented from entering idle states
> during suspend to avoid a resume hang.
> 
> /*
>  * Known limitations
>  * [..]
>  *
>  * ACPI has a .suspend hack to turn off deep c-states during suspend
>  * to avoid complications with the lapic timer workaround.
>  * Have not seen issues with suspend, but may need same workaround here.
>  *
>  */
> This patch aims at having this fix in a place common to both the idle
> drivers.
> 
> Suspend is enabled only if ACPI is active on x86.Thus the setting of
> acpi_idle_suspend during suspend is moved up to ACPI specific code with
> both acpi and intel idle drivers checking if it is valid to enter deeper
> idle states.The setting of acpi_idle_suspend is done via PM_SUSPEND_PREPARE
> notifiers to avoid race conditions between processors entering idle states
> and the ongoing process of suspend.
> 
> The check on acpi_idle_suspend is included in the most appropriate header
> so as to be visible to both the idle drivers irrespective of the
> different configurations.Even if ACPI is disabled intel idle drivers can
> still carry out the acpi_idle_suspend check.
> 
> Reported-by: Dave Hansen <dave@linux.vnet.ibm.com>
> Reviewed-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
> Reviewed-by: Deepthi Dharwar <deepthi@linux.vnet.ibm.com>
> Signed-off-by: Preeti U Murthy <preeti@linux.vnet.ibm.com>

The patch didn't get any better as a result of modifying the changelog.

Please consider it as rejected.

Thanks,
Rafael

Re: Cpuidle drivers,Suspend : Fix suspend/resume hang with intel_idle driver

[Rafael J. Wysocki]

On Saturday, June 30, 2012, Rafael J. Wysocki wrote:
> On Friday, June 29, 2012, preeti wrote:
> > On 06/29/2012 12:41 AM, Rafael J. Wysocki wrote:
> > > On Thursday, June 28, 2012, preeti wrote:
> > >> On 06/28/2012 03:23 PM, Rafael J. Wysocki wrote:
> > >>> On Thursday, June 28, 2012, preeti wrote:
> > >>>>
> > >>>> From: Preeti U Murthy <preeti@linux.vnet.ibm.com>
> > > [...]
> > >> cpuidle is an architecture independent part of the kernel  code.Since 
> > >> this patch aims at x86 architecture in specific,I considered it
> > >> inappropriate.
> > >>
> > >> In addition to this,suspend happens on x86 only if ACPI is configured.
> > > 
> > > But that is not required for intel_idle, so if it hangs with intel_idle,
> > > then it is not dependent on ACPI after all.
> > True intel_idle does not need ACPI to be configured,but that also means
> > that the kernel will not provide you the means to suspend.
> 
> Yes, it will.  You can hibernate without ACPI in theory.
> 
> > There is no question of resume hang here at all as you cannot suspend in
> > the first place.
> > 
> > The issue is when ACPI is configured,and intel_idle is chosen to be the
> > cpuidle driver.In this situation when the user suspends,cpus can enter
> > deep sleep states as intel_idle driver does not prevent then from doing so.
> > This is when resume hangs.
> 
> I know that.
> 
> > >> Therefore it seemed right to put the callback in ACPI specific code
> > >> which deals with ACPI sleep support.
> > > 
> > > I wonder if we can address this issue correctly.  That is, in a non-racy
> > > way and in a better place.
> > > 
> > > First, I really don't think it is necessary to "suspend" cpuidle (be it
> > > ACPI or any other) when device drivers' suspend routines are being
> > > executed (which also is racy, because the cpuidle "suspend" may be running
> > > concurrently with cpuidle on another CPU) or earlier.  We really may want
> > > to disable the deeper C-states when we're about to execute
> > > suspend_ops->prepare_late(), or hibernation_ops->prepare(), i.e. after
> > > we've run dpm_suspend_end() successfully.
> > 
> > The commit "ACPI:disable lower idle C-states across suspend/resume"
> > states that device_suspend() calls ACPI suspend functions which cause
> > side effects on the lower idle C-states.
> 
> I'd like to know the details, then.  Which ACPI suspend functions those are,
> in particular, because I'm not aware of any in device_suspend().
> 
> Also, please note that this commit is 5 years old and some things have changed
> in the suspend code paths since that time.
> 
> > This means we need to disable entry into deeper C-states even before
> > dpm_suspend_start(),
> 
> This most likely is wrong.
> 
> We may need to "suspend" cpuidle before calling suspend_device_irqs(),
> but then again that shouldn't be made via a notifier I think.  Why don't
> we simply make suspend_device_irqs() disable lower C-states to start with?
> 
> > but how much before?
> > 
> > The commit answers this too.It says removing the functionality of
> > entering deep C-states before suspend removed the side effects.Besides,
> > the commit title says'across suspend/resume'.So I think to address the
> > resume hang effectively,it is desirable to disable entry into deeper
> > C-states during suspend_prepare operations.
> > > 
> > > So, it seems, it might be a good idea to place a cpuidle driver suspend
> > > (and/or hibernation) hook at the end of dpm_suspend_end() and make ACPI
> > > and intel_idle drivers implement that hook.
> > > 
> > Dont you think this patch is meant to fix a very ACPI specific problem?
> 
> No, I don't.
> 
> > Which is why I think the call backs or any hook meant to fix this should
> > go into ACPI specific code.
> > Else it will seem irrelevant to all other architectures that implement
> > suspend.
> 
> I don't honestly think it is irrelevant.  The fact is that similar problems
> have not been reported on other architectures _yet_, which by no means can
> be taken as a proof that those architectures are not affected.
> 
> Anyway, I think that the right way to address this is through a cpuidle driver
> callback executed from suspend_device_irqs() (and analogously for the resume
> code path) and not through some hacky-ugly ACPI changes.

On a second thought, that may be confusing, so I'd create a cpuidle_suspend()
routine that would be executed right before suspend_device_irqs() by
dpm_suspend_noirq().  I'd make that routine run a suspend callback from the
cpuidle driver (the drivers that don't need to "suspend" would not provide that
callback).  And analogously for resume.

Thanks,
Rafael

Re: Cpuidle drivers,Suspend : Fix suspend/resume hang with intel_idle driver

[preeti]

On 06/30/2012 03:41 AM, Rafael J. Wysocki wrote:
> On Friday, June 29, 2012, preeti wrote:
>> On 06/29/2012 09:41 AM, preeti wrote:
>>> On 06/29/2012 12:41 AM, Rafael J. Wysocki wrote:
>>>> On Thursday, June 28, 2012, preeti wrote:
>>>>> On 06/28/2012 03:23 PM, Rafael J. Wysocki wrote:
>>>>>> On Thursday, June 28, 2012, preeti wrote:
>>>>>>>
>>>>>>> From: Preeti U Murthy <preeti@linux.vnet.ibm.com>
>>>> [...]
>>>>> cpuidle is an architecture independent part of the kernel  code.Since 
>>>>> this patch aims at x86 architecture in specific,I considered it
>>>>> inappropriate.
>>>>>
>>>>> In addition to this,suspend happens on x86 only if ACPI is configured.
>>>>
>>>> But that is not required for intel_idle, so if it hangs with intel_idle,
>>>> then it is not dependent on ACPI after all.
>>> True intel_idle does not need ACPI to be configured,but that also means
>>> that the kernel will not provide you the means to suspend.There is no
>>> question of resume hang here at all as you cannot suspend in the first
>>> place.
>>>
>>> The issue is when ACPI is configured,and intel_idle is chosen to be the
>>> cpuidle driver.In this situation when the user suspends,cpus can enter
>>> deep sleep states as intel_idle driver does not prevent then from doing so.
>>> This is when resume hangs.
>>>>
>>>>> Therefore it seemed right to put the callback in ACPI specific code
>>>>> which deals with ACPI sleep support.
>>>>
>>>> I wonder if we can address this issue correctly.  That is, in a non-racy
>>>> way and in a better place.
>>>>
>>>> First, I really don't think it is necessary to "suspend" cpuidle (be it
>>>> ACPI or any other) when device drivers' suspend routines are being
>>>> executed (which also is racy, because the cpuidle "suspend" may be running
>>>> concurrently with cpuidle on another CPU) or earlier.  We really may want
>>>> to disable the deeper C-states when we're about to execute
>>>> suspend_ops->prepare_late(), or hibernation_ops->prepare(), i.e. after
>>>> we've run dpm_suspend_end() successfully.
>>>
>>> The commit "ACPI:disable lower idle C-states across suspend/resume"
>>> states that device_suspend() calls ACPI suspend functions which cause
>>> side effects on the lower idle C-states.This means we need to disable
>>> entry into deeper C-states even before dpm_suspend_start(),but how much
>>> before?
>>>
>>> The commit answers this too.It says removing the functionality of
>>> entering deep C-states before suspend removed the side effects.Besides,
>>> the commit title says'across suspend/resume'.So I think to address the
>>> resume hang effectively,it is desirable to disable entry into deeper
>>> C-states during suspend_prepare operations.
>>
>> To clarify this further,since we take action upon PM_SUSPEND_PREPARE
>> notification,which is called before suspend begins,we avoid race
>> condition between suspend operations and disabling entry into deeper
>> c-states altogether.
> 
> Well, what about races between disabling deeper C-states and cpuidle?

Yes.The question still remains about the cpus that have already entered
deep C-states even before suspend routines have begun.We are not taking
precautions to prevent them from going into idle.

If the resume hang does depend on the cpus being in deep C-state,even
after the fix with acpi_idle_suspend, there should have been a hang
in scenarios where the cpus have already entered deep C-states before
suspend has begun.

> 
> Rafael
> 

Regards
Preeti

Re: Cpuidle drivers, Suspend : Fix suspend/resume hang with intel_idle driver

[Srivatsa S. Bhat]

On 07/02/2012 10:07 AM, preeti wrote:
> On 06/30/2012 03:41 AM, Rafael J. Wysocki wrote:
>> On Friday, June 29, 2012, preeti wrote:
>>> On 06/29/2012 09:41 AM, preeti wrote:
>>>> On 06/29/2012 12:41 AM, Rafael J. Wysocki wrote:
>>>>> On Thursday, June 28, 2012, preeti wrote:
>>>>>> On 06/28/2012 03:23 PM, Rafael J. Wysocki wrote:
>>>>>>> On Thursday, June 28, 2012, preeti wrote:
>>>>>>>>
>>>>>>>> From: Preeti U Murthy <preeti@linux.vnet.ibm.com>
>>>>> [...]
>>>>>> cpuidle is an architecture independent part of the kernel  code.Since 
>>>>>> this patch aims at x86 architecture in specific,I considered it
>>>>>> inappropriate.
>>>>>>
>>>>>> In addition to this,suspend happens on x86 only if ACPI is configured.
>>>>>
>>>>> But that is not required for intel_idle, so if it hangs with intel_idle,
>>>>> then it is not dependent on ACPI after all.
>>>> True intel_idle does not need ACPI to be configured,but that also means
>>>> that the kernel will not provide you the means to suspend.There is no
>>>> question of resume hang here at all as you cannot suspend in the first
>>>> place.
>>>>
>>>> The issue is when ACPI is configured,and intel_idle is chosen to be the
>>>> cpuidle driver.In this situation when the user suspends,cpus can enter
>>>> deep sleep states as intel_idle driver does not prevent then from doing so.
>>>> This is when resume hangs.
>>>>>
>>>>>> Therefore it seemed right to put the callback in ACPI specific code
>>>>>> which deals with ACPI sleep support.
>>>>>
>>>>> I wonder if we can address this issue correctly.  That is, in a non-racy
>>>>> way and in a better place.
>>>>>
>>>>> First, I really don't think it is necessary to "suspend" cpuidle (be it
>>>>> ACPI or any other) when device drivers' suspend routines are being
>>>>> executed (which also is racy, because the cpuidle "suspend" may be running
>>>>> concurrently with cpuidle on another CPU) or earlier.  We really may want
>>>>> to disable the deeper C-states when we're about to execute
>>>>> suspend_ops->prepare_late(), or hibernation_ops->prepare(), i.e. after
>>>>> we've run dpm_suspend_end() successfully.
>>>>
>>>> The commit "ACPI:disable lower idle C-states across suspend/resume"
>>>> states that device_suspend() calls ACPI suspend functions which cause
>>>> side effects on the lower idle C-states.This means we need to disable
>>>> entry into deeper C-states even before dpm_suspend_start(),but how much
>>>> before?
>>>>
>>>> The commit answers this too.It says removing the functionality of
>>>> entering deep C-states before suspend removed the side effects.Besides,
>>>> the commit title says'across suspend/resume'.So I think to address the
>>>> resume hang effectively,it is desirable to disable entry into deeper
>>>> C-states during suspend_prepare operations.
>>>
>>> To clarify this further,since we take action upon PM_SUSPEND_PREPARE
>>> notification,which is called before suspend begins,we avoid race
>>> condition between suspend operations and disabling entry into deeper
>>> c-states altogether.
>>
>> Well, what about races between disabling deeper C-states and cpuidle?
> 
> Yes.The question still remains about the cpus that have already entered
> deep C-states even before suspend routines have begun.We are not taking
> precautions to prevent them from going into idle.
> 

Actually we need *not* take such precautions. See below.

> If the resume hang does depend on the cpus being in deep C-state,even
> after the fix with acpi_idle_suspend, there should have been a hang
> in scenarios where the cpus have already entered deep C-states before
> suspend has begun.
> 

Nope, that won't happen because we have CPU hotplug in between. The suspend
path goes through CPU hotplug (cpu offline), and one of the phases of the
cpu offline operation requires that the cpu that is going down runs the
CPU_DYING_FROZEN callbacks. No other cpu can execute that. So even if a cpu
was in a deep C-state, it would be kicked out of cpu idle and will run
these callbacks during cpu hotplug. Its enough if we ensure that it doesn't
enter deep C-states again, *after* the cpu hotplug operation. And the flag you
are using or the callback method that Rafael suggested looks sufficient for
ensuring that.

So, we need not break our heads on too many race conditions here :-)

Regards,
Srivatsa S. Bhat

Re: Cpuidle drivers,Suspend : Fix suspend/resume hang with intel_idle driver

[preeti]

On 07/02/2012 10:34 AM, Srivatsa S. Bhat wrote:
> On 07/02/2012 10:07 AM, preeti wrote:
>> On 06/30/2012 03:41 AM, Rafael J. Wysocki wrote:
>>> On Friday, June 29, 2012, preeti wrote:
>>>> On 06/29/2012 09:41 AM, preeti wrote:
>>>>> On 06/29/2012 12:41 AM, Rafael J. Wysocki wrote:
>>>>>> On Thursday, June 28, 2012, preeti wrote:
>>>>>>> On 06/28/2012 03:23 PM, Rafael J. Wysocki wrote:
>>>>>>>> On Thursday, June 28, 2012, preeti wrote:
>>>>>>>>>
>>>>>>>>> From: Preeti U Murthy <preeti@linux.vnet.ibm.com>
>>>>>> [...]
>>>>>>> cpuidle is an architecture independent part of the kernel  code.Since 
>>>>>>> this patch aims at x86 architecture in specific,I considered it
>>>>>>> inappropriate.
>>>>>>>
>>>>>>> In addition to this,suspend happens on x86 only if ACPI is configured.
>>>>>>
>>>>>> But that is not required for intel_idle, so if it hangs with intel_idle,
>>>>>> then it is not dependent on ACPI after all.
>>>>> True intel_idle does not need ACPI to be configured,but that also means
>>>>> that the kernel will not provide you the means to suspend.There is no
>>>>> question of resume hang here at all as you cannot suspend in the first
>>>>> place.
>>>>>
>>>>> The issue is when ACPI is configured,and intel_idle is chosen to be the
>>>>> cpuidle driver.In this situation when the user suspends,cpus can enter
>>>>> deep sleep states as intel_idle driver does not prevent then from doing so.
>>>>> This is when resume hangs.
>>>>>>
>>>>>>> Therefore it seemed right to put the callback in ACPI specific code
>>>>>>> which deals with ACPI sleep support.
>>>>>>
>>>>>> I wonder if we can address this issue correctly.  That is, in a non-racy
>>>>>> way and in a better place.
>>>>>>
>>>>>> First, I really don't think it is necessary to "suspend" cpuidle (be it
>>>>>> ACPI or any other) when device drivers' suspend routines are being
>>>>>> executed (which also is racy, because the cpuidle "suspend" may be running
>>>>>> concurrently with cpuidle on another CPU) or earlier.  We really may want
>>>>>> to disable the deeper C-states when we're about to execute
>>>>>> suspend_ops->prepare_late(), or hibernation_ops->prepare(), i.e. after
>>>>>> we've run dpm_suspend_end() successfully.
>>>>>
>>>>> The commit "ACPI:disable lower idle C-states across suspend/resume"
>>>>> states that device_suspend() calls ACPI suspend functions which cause
>>>>> side effects on the lower idle C-states.This means we need to disable
>>>>> entry into deeper C-states even before dpm_suspend_start(),but how much
>>>>> before?
>>>>>
>>>>> The commit answers this too.It says removing the functionality of
>>>>> entering deep C-states before suspend removed the side effects.Besides,
>>>>> the commit title says'across suspend/resume'.So I think to address the
>>>>> resume hang effectively,it is desirable to disable entry into deeper
>>>>> C-states during suspend_prepare operations.
>>>>
>>>> To clarify this further,since we take action upon PM_SUSPEND_PREPARE
>>>> notification,which is called before suspend begins,we avoid race
>>>> condition between suspend operations and disabling entry into deeper
>>>> c-states altogether.
>>>
>>> Well, what about races between disabling deeper C-states and cpuidle?
>>
>> Yes.The question still remains about the cpus that have already entered
>> deep C-states even before suspend routines have begun.We are not taking
>> precautions to prevent them from going into idle.
>>
> 
> Actually we need *not* take such precautions. See below.
> 
>> If the resume hang does depend on the cpus being in deep C-state,even
>> after the fix with acpi_idle_suspend, there should have been a hang
>> in scenarios where the cpus have already entered deep C-states before
>> suspend has begun.
>>
> 
> Nope, that won't happen because we have CPU hotplug in between. The suspend
> path goes through CPU hotplug (cpu offline), and one of the phases of the
> cpu offline operation requires that the cpu that is going down runs the
> CPU_DYING_FROZEN callbacks. No other cpu can execute that. So even if a cpu
> was in a deep C-state, it would be kicked out of cpu idle and will run
> these callbacks during cpu hotplug. Its enough if we ensure that it doesn't
> enter deep C-states again, *after* the cpu hotplug operation. And the flag you
> are using or the callback method that Rafael suggested looks sufficient for
> ensuring that.
> 
> So, we need not break our heads on too many race conditions here :-)

But let us note that without the acpi_idle_suspend check,it was at the
device layer,that the hang was happening.Before cpu hotplug even begins.

This means that with the acpi_idle_suspend fix, we ensure cpus do not
enter deeper C-states but we do not check if there are already idle
cpus.But with the fix,device layer suspend happens fine.Cpu hotplug does
not come to the rescue of the cpus already in deep C-states.

This is where the question about 'are the idle cpus actually causing
problems' arises.
> 
> Regards,
> Srivatsa S. Bhat
> 
Regards
Preeti

Re: Cpuidle drivers,Suspend : Fix suspend/resume hang with intel_idle driver

[Srivatsa S. Bhat]

On 07/02/2012 10:57 AM, preeti wrote:
> On 07/02/2012 10:34 AM, Srivatsa S. Bhat wrote:
>> On 07/02/2012 10:07 AM, preeti wrote:
>>> On 06/30/2012 03:41 AM, Rafael J. Wysocki wrote:
>>>> On Friday, June 29, 2012, preeti wrote:
>>>>> On 06/29/2012 09:41 AM, preeti wrote:
>>>>>> On 06/29/2012 12:41 AM, Rafael J. Wysocki wrote:
>>>>>>> On Thursday, June 28, 2012, preeti wrote:
>>>>>>>> On 06/28/2012 03:23 PM, Rafael J. Wysocki wrote:
>>>>>>>>> On Thursday, June 28, 2012, preeti wrote:
>>>>>>>>>>
>>>>>>>>>> From: Preeti U Murthy <preeti@linux.vnet.ibm.com>
>>>>>>> [...]
>>>>>>>> cpuidle is an architecture independent part of the kernel  code.Since 
>>>>>>>> this patch aims at x86 architecture in specific,I considered it
>>>>>>>> inappropriate.
>>>>>>>>
>>>>>>>> In addition to this,suspend happens on x86 only if ACPI is configured.
>>>>>>>
>>>>>>> But that is not required for intel_idle, so if it hangs with intel_idle,
>>>>>>> then it is not dependent on ACPI after all.
>>>>>> True intel_idle does not need ACPI to be configured,but that also means
>>>>>> that the kernel will not provide you the means to suspend.There is no
>>>>>> question of resume hang here at all as you cannot suspend in the first
>>>>>> place.
>>>>>>
>>>>>> The issue is when ACPI is configured,and intel_idle is chosen to be the
>>>>>> cpuidle driver.In this situation when the user suspends,cpus can enter
>>>>>> deep sleep states as intel_idle driver does not prevent then from doing so.
>>>>>> This is when resume hangs.
>>>>>>>
>>>>>>>> Therefore it seemed right to put the callback in ACPI specific code
>>>>>>>> which deals with ACPI sleep support.
>>>>>>>
>>>>>>> I wonder if we can address this issue correctly.  That is, in a non-racy
>>>>>>> way and in a better place.
>>>>>>>
>>>>>>> First, I really don't think it is necessary to "suspend" cpuidle (be it
>>>>>>> ACPI or any other) when device drivers' suspend routines are being
>>>>>>> executed (which also is racy, because the cpuidle "suspend" may be running
>>>>>>> concurrently with cpuidle on another CPU) or earlier.  We really may want
>>>>>>> to disable the deeper C-states when we're about to execute
>>>>>>> suspend_ops->prepare_late(), or hibernation_ops->prepare(), i.e. after
>>>>>>> we've run dpm_suspend_end() successfully.
>>>>>>
>>>>>> The commit "ACPI:disable lower idle C-states across suspend/resume"
>>>>>> states that device_suspend() calls ACPI suspend functions which cause
>>>>>> side effects on the lower idle C-states.This means we need to disable
>>>>>> entry into deeper C-states even before dpm_suspend_start(),but how much
>>>>>> before?
>>>>>>
>>>>>> The commit answers this too.It says removing the functionality of
>>>>>> entering deep C-states before suspend removed the side effects.Besides,
>>>>>> the commit title says'across suspend/resume'.So I think to address the
>>>>>> resume hang effectively,it is desirable to disable entry into deeper
>>>>>> C-states during suspend_prepare operations.
>>>>>
>>>>> To clarify this further,since we take action upon PM_SUSPEND_PREPARE
>>>>> notification,which is called before suspend begins,we avoid race
>>>>> condition between suspend operations and disabling entry into deeper
>>>>> c-states altogether.
>>>>
>>>> Well, what about races between disabling deeper C-states and cpuidle?
>>>
>>> Yes.The question still remains about the cpus that have already entered
>>> deep C-states even before suspend routines have begun.We are not taking
>>> precautions to prevent them from going into idle.
>>>
>>
>> Actually we need *not* take such precautions. See below.
>>
>>> If the resume hang does depend on the cpus being in deep C-state,even
>>> after the fix with acpi_idle_suspend, there should have been a hang
>>> in scenarios where the cpus have already entered deep C-states before
>>> suspend has begun.
>>>
>>
>> Nope, that won't happen because we have CPU hotplug in between. The suspend
>> path goes through CPU hotplug (cpu offline), and one of the phases of the
>> cpu offline operation requires that the cpu that is going down runs the
>> CPU_DYING_FROZEN callbacks. No other cpu can execute that. So even if a cpu
>> was in a deep C-state, it would be kicked out of cpu idle and will run
>> these callbacks during cpu hotplug. Its enough if we ensure that it doesn't
>> enter deep C-states again, *after* the cpu hotplug operation. And the flag you
>> are using or the callback method that Rafael suggested looks sufficient for
>> ensuring that.
>>
>> So, we need not break our heads on too many race conditions here :-)
> 
> But let us note that without the acpi_idle_suspend check,it was at the
> device layer,that the hang was happening.Before cpu hotplug even begins.
> 

acpi_idle_suspend check has nothing to do with hang at the device layer,
IIRC. The hang at device layer was because we were coming out of cpu idle
without enabling interrupts. And Deepthi already fixed that issue
(commit 75cc523 upstream).

So the problem that still remains is the _resume_ hang when using intel idle
as the idle driver. And not allowing cpus to be in deep C-states while
doing suspend should fix that.

Regards,
Srivatsa S. Bhat

Re: Cpuidle drivers,Suspend : Fix suspend/resume hang with intel_idle driver

[Srivatsa S. Bhat]

On 07/02/2012 11:06 AM, preeti wrote:
> On 06/30/2012 03:37 AM, Rafael J. Wysocki wrote:
>> On Friday, June 29, 2012, preeti wrote:
>>> On 06/29/2012 12:41 AM, Rafael J. Wysocki wrote:
>>>> On Thursday, June 28, 2012, preeti wrote:
>>>>> On 06/28/2012 03:23 PM, Rafael J. Wysocki wrote:
>>>>>> On Thursday, June 28, 2012, preeti wrote:
>>>>>>>
>>>>>>> From: Preeti U Murthy <preeti@linux.vnet.ibm.com>
>>>> [...]
>>>>> cpuidle is an architecture independent part of the kernel  code.Since 
>>>>> this patch aims at x86 architecture in specific,I considered it
>>>>> inappropriate.
>>>>>
>>>>> In addition to this,suspend happens on x86 only if ACPI is configured.
>>>>
>>>> But that is not required for intel_idle, so if it hangs with intel_idle,
>>>> then it is not dependent on ACPI after all.
>>> True intel_idle does not need ACPI to be configured,but that also means
>>> that the kernel will not provide you the means to suspend.
>>
>> Yes, it will.  You can hibernate without ACPI in theory.
> 
> True.You can suspend to disk without ACPI,but can suspend to RAM only
> with ACPI.
> This also highlights the fact that my patch has not taken care of
> hibernate notifiers,which I should have done.This goes to say that the
> callbacks during suspend better not be in ACPI specific code.I will look
> into correcting the placement of the callbacks.
> 
>>
>>> There is no question of resume hang here at all as you cannot suspend in
>>> the first place.
>>>
>>> The issue is when ACPI is configured,and intel_idle is chosen to be the
>>> cpuidle driver.In this situation when the user suspends,cpus can enter
>>> deep sleep states as intel_idle driver does not prevent then from doing so.
>>> This is when resume hangs.
>>
>> I know that.
>>
>>>>> Therefore it seemed right to put the callback in ACPI specific code
>>>>> which deals with ACPI sleep support.
>>>>
>>>> I wonder if we can address this issue correctly.  That is, in a non-racy
>>>> way and in a better place.
>>>>
>>>> First, I really don't think it is necessary to "suspend" cpuidle (be it
>>>> ACPI or any other) when device drivers' suspend routines are being
>>>> executed (which also is racy, because the cpuidle "suspend" may be running
>>>> concurrently with cpuidle on another CPU) or earlier.  We really may want
>>>> to disable the deeper C-states when we're about to execute
>>>> suspend_ops->prepare_late(), or hibernation_ops->prepare(), i.e. after
>>>> we've run dpm_suspend_end() successfully.
>>>
>>> The commit "ACPI:disable lower idle C-states across suspend/resume"
>>> states that device_suspend() calls ACPI suspend functions which cause
>>> side effects on the lower idle C-states.
>>
>> I'd like to know the details, then.  Which ACPI suspend functions those are,
>> in particular, because I'm not aware of any in device_suspend().
>>
>> Also, please note that this commit is 5 years old and some things have changed
>> in the suspend code paths since that time.
> 
> I agree.My view on this,as I have mentioned in one of my previous mails
> is, in the patch with the acpi_idle_suspend workaround,we have not taken
> precautions to check if there are cpus that have already entered deep
> C-states before the suspend routine has begun.
> 
> We take care of disabling entry into deep C-states only during suspend.
> so if deep C-states are posing problems during suspend,then why dont
> these cpus that have entered idle states before suspend cause a resume hang?
>

Because cpu hotplug kicks the cpu out of any idle state it was in, in order
to execute the CPU_DYING_FROZEN callbacks. (See my other mail about this).
 
>>
>>> This means we need to disable entry into deeper C-states even before
>>> dpm_suspend_start(),
>>
>> This most likely is wrong.
>>
>> We may need to "suspend" cpuidle before calling suspend_device_irqs(),
>> but then again that shouldn't be made via a notifier I think.  Why don't
>> we simply make suspend_device_irqs() disable lower C-states to start with?
>>
>>> but how much before?
>>>
>>> The commit answers this too.It says removing the functionality of
>>> entering deep C-states before suspend removed the side effects.Besides,
>>> the commit title says'across suspend/resume'.So I think to address the
>>> resume hang effectively,it is desirable to disable entry into deeper
>>> C-states during suspend_prepare operations.
>>>>
>>>> So, it seems, it might be a good idea to place a cpuidle driver suspend
>>>> (and/or hibernation) hook at the end of dpm_suspend_end() and make ACPI
>>>> and intel_idle drivers implement that hook.
>>>>
>>> Dont you think this patch is meant to fix a very ACPI specific problem?
>>
>> No, I don't.
>>
>>> Which is why I think the call backs or any hook meant to fix this should
>>> go into ACPI specific code.
>>> Else it will seem irrelevant to all other architectures that implement
>>> suspend.
>>
>> I don't honestly think it is irrelevant.  The fact is that similar problems
>> have not been reported on other architectures _yet_, which by no means can
>> be taken as a proof that those architectures are not affected.
>>
>> Anyway, I think that the right way to address this is through a cpuidle driver
>> callback executed from suspend_device_irqs() (and analogously for the resume
>> code path) and not through some hacky-ugly ACPI changes.
> 
> I agree with having a cpuidle driver callback as even hibernate
> callbacks need to be taken care of which have nothing to do with ACPI.
> 
> But on what basis is the placement of the call back suggested at
> suspend_device_irqs()? What is the assurance that this placement  will
> not cause a resume hang considering we dont know what precisely is
> causing this problem?
> 

Any place before CPU hotplug should do, I think.

Regards,
Srivatsa S. Bhat

Re: Cpuidle drivers,Suspend : Fix suspend/resume hang with intel_idle driver

[preeti]

On 06/30/2012 03:37 AM, Rafael J. Wysocki wrote:
> On Friday, June 29, 2012, preeti wrote:
>> On 06/29/2012 12:41 AM, Rafael J. Wysocki wrote:
>>> On Thursday, June 28, 2012, preeti wrote:
>>>> On 06/28/2012 03:23 PM, Rafael J. Wysocki wrote:
>>>>> On Thursday, June 28, 2012, preeti wrote:
>>>>>>
>>>>>> From: Preeti U Murthy <preeti@linux.vnet.ibm.com>
>>> [...]
>>>> cpuidle is an architecture independent part of the kernel  code.Since 
>>>> this patch aims at x86 architecture in specific,I considered it
>>>> inappropriate.
>>>>
>>>> In addition to this,suspend happens on x86 only if ACPI is configured.
>>>
>>> But that is not required for intel_idle, so if it hangs with intel_idle,
>>> then it is not dependent on ACPI after all.
>> True intel_idle does not need ACPI to be configured,but that also means
>> that the kernel will not provide you the means to suspend.
> 
> Yes, it will.  You can hibernate without ACPI in theory.

True.You can suspend to disk without ACPI,but can suspend to RAM only
with ACPI.
This also highlights the fact that my patch has not taken care of
hibernate notifiers,which I should have done.This goes to say that the
callbacks during suspend better not be in ACPI specific code.I will look
into correcting the placement of the callbacks.

> 
>> There is no question of resume hang here at all as you cannot suspend in
>> the first place.
>>
>> The issue is when ACPI is configured,and intel_idle is chosen to be the
>> cpuidle driver.In this situation when the user suspends,cpus can enter
>> deep sleep states as intel_idle driver does not prevent then from doing so.
>> This is when resume hangs.
> 
> I know that.
> 
>>>> Therefore it seemed right to put the callback in ACPI specific code
>>>> which deals with ACPI sleep support.
>>>
>>> I wonder if we can address this issue correctly.  That is, in a non-racy
>>> way and in a better place.
>>>
>>> First, I really don't think it is necessary to "suspend" cpuidle (be it
>>> ACPI or any other) when device drivers' suspend routines are being
>>> executed (which also is racy, because the cpuidle "suspend" may be running
>>> concurrently with cpuidle on another CPU) or earlier.  We really may want
>>> to disable the deeper C-states when we're about to execute
>>> suspend_ops->prepare_late(), or hibernation_ops->prepare(), i.e. after
>>> we've run dpm_suspend_end() successfully.
>>
>> The commit "ACPI:disable lower idle C-states across suspend/resume"
>> states that device_suspend() calls ACPI suspend functions which cause
>> side effects on the lower idle C-states.
> 
> I'd like to know the details, then.  Which ACPI suspend functions those are,
> in particular, because I'm not aware of any in device_suspend().
> 
> Also, please note that this commit is 5 years old and some things have changed
> in the suspend code paths since that time.

I agree.My view on this,as I have mentioned in one of my previous mails
is, in the patch with the acpi_idle_suspend workaround,we have not taken
precautions to check if there are cpus that have already entered deep
C-states before the suspend routine has begun.

We take care of disabling entry into deep C-states only during suspend.
so if deep C-states are posing problems during suspend,then why dont
these cpus that have entered idle states before suspend cause a resume hang?

> 
>> This means we need to disable entry into deeper C-states even before
>> dpm_suspend_start(),
> 
> This most likely is wrong.
> 
> We may need to "suspend" cpuidle before calling suspend_device_irqs(),
> but then again that shouldn't be made via a notifier I think.  Why don't
> we simply make suspend_device_irqs() disable lower C-states to start with?
> 
>> but how much before?
>>
>> The commit answers this too.It says removing the functionality of
>> entering deep C-states before suspend removed the side effects.Besides,
>> the commit title says'across suspend/resume'.So I think to address the
>> resume hang effectively,it is desirable to disable entry into deeper
>> C-states during suspend_prepare operations.
>>>
>>> So, it seems, it might be a good idea to place a cpuidle driver suspend
>>> (and/or hibernation) hook at the end of dpm_suspend_end() and make ACPI
>>> and intel_idle drivers implement that hook.
>>>
>> Dont you think this patch is meant to fix a very ACPI specific problem?
> 
> No, I don't.
> 
>> Which is why I think the call backs or any hook meant to fix this should
>> go into ACPI specific code.
>> Else it will seem irrelevant to all other architectures that implement
>> suspend.
> 
> I don't honestly think it is irrelevant.  The fact is that similar problems
> have not been reported on other architectures _yet_, which by no means can
> be taken as a proof that those architectures are not affected.
> 
> Anyway, I think that the right way to address this is through a cpuidle driver
> callback executed from suspend_device_irqs() (and analogously for the resume
> code path) and not through some hacky-ugly ACPI changes.

I agree with having a cpuidle driver callback as even hibernate
callbacks need to be taken care of which have nothing to do with ACPI.

But on what basis is the placement of the call back suggested at
suspend_device_irqs()? What is the assurance that this placement  will
not cause a resume hang considering we dont know what precisely is
causing this problem?

> 
> Thanks,
> Rafael
> 

Regards
Preeti

Re: Cpuidle drivers,Suspend : Fix suspend/resume hang with intel_idle driver

[preeti]

On 06/30/2012 06:41 PM, Rafael J. Wysocki wrote:
> On Saturday, June 30, 2012, Rafael J. Wysocki wrote:
>> On Friday, June 29, 2012, preeti wrote:
>>> On 06/29/2012 12:41 AM, Rafael J. Wysocki wrote:
>>>> On Thursday, June 28, 2012, preeti wrote:
>>>>> On 06/28/2012 03:23 PM, Rafael J. Wysocki wrote:
>>>>>> On Thursday, June 28, 2012, preeti wrote:
>>>>>>>
>>>>>>> From: Preeti U Murthy <preeti@linux.vnet.ibm.com>
>>>> [...]
>>>>> cpuidle is an architecture independent part of the kernel  code.Since 
>>>>> this patch aims at x86 architecture in specific,I considered it
>>>>> inappropriate.
>>>>>
>>>>> In addition to this,suspend happens on x86 only if ACPI is configured.
>>>>
>>>> But that is not required for intel_idle, so if it hangs with intel_idle,
>>>> then it is not dependent on ACPI after all.
>>> True intel_idle does not need ACPI to be configured,but that also means
>>> that the kernel will not provide you the means to suspend.
>>
>> Yes, it will.  You can hibernate without ACPI in theory.
>>
>>> There is no question of resume hang here at all as you cannot suspend in
>>> the first place.
>>>
>>> The issue is when ACPI is configured,and intel_idle is chosen to be the
>>> cpuidle driver.In this situation when the user suspends,cpus can enter
>>> deep sleep states as intel_idle driver does not prevent then from doing so.
>>> This is when resume hangs.
>>
>> I know that.
>>
>>>>> Therefore it seemed right to put the callback in ACPI specific code
>>>>> which deals with ACPI sleep support.
>>>>
>>>> I wonder if we can address this issue correctly.  That is, in a non-racy
>>>> way and in a better place.
>>>>
>>>> First, I really don't think it is necessary to "suspend" cpuidle (be it
>>>> ACPI or any other) when device drivers' suspend routines are being
>>>> executed (which also is racy, because the cpuidle "suspend" may be running
>>>> concurrently with cpuidle on another CPU) or earlier.  We really may want
>>>> to disable the deeper C-states when we're about to execute
>>>> suspend_ops->prepare_late(), or hibernation_ops->prepare(), i.e. after
>>>> we've run dpm_suspend_end() successfully.
>>>
>>> The commit "ACPI:disable lower idle C-states across suspend/resume"
>>> states that device_suspend() calls ACPI suspend functions which cause
>>> side effects on the lower idle C-states.
>>
>> I'd like to know the details, then.  Which ACPI suspend functions those are,
>> in particular, because I'm not aware of any in device_suspend().
>>
>> Also, please note that this commit is 5 years old and some things have changed
>> in the suspend code paths since that time.
>>
>>> This means we need to disable entry into deeper C-states even before
>>> dpm_suspend_start(),
>>
>> This most likely is wrong.
>>
>> We may need to "suspend" cpuidle before calling suspend_device_irqs(),
>> but then again that shouldn't be made via a notifier I think.  Why don't
>> we simply make suspend_device_irqs() disable lower C-states to start with?
>>
>>> but how much before?
>>>
>>> The commit answers this too.It says removing the functionality of
>>> entering deep C-states before suspend removed the side effects.Besides,
>>> the commit title says'across suspend/resume'.So I think to address the
>>> resume hang effectively,it is desirable to disable entry into deeper
>>> C-states during suspend_prepare operations.
>>>>
>>>> So, it seems, it might be a good idea to place a cpuidle driver suspend
>>>> (and/or hibernation) hook at the end of dpm_suspend_end() and make ACPI
>>>> and intel_idle drivers implement that hook.
>>>>
>>> Dont you think this patch is meant to fix a very ACPI specific problem?
>>
>> No, I don't.
>>
>>> Which is why I think the call backs or any hook meant to fix this should
>>> go into ACPI specific code.
>>> Else it will seem irrelevant to all other architectures that implement
>>> suspend.
>>
>> I don't honestly think it is irrelevant.  The fact is that similar problems
>> have not been reported on other architectures _yet_, which by no means can
>> be taken as a proof that those architectures are not affected.
>>
>> Anyway, I think that the right way to address this is through a cpuidle driver
>> callback executed from suspend_device_irqs() (and analogously for the resume
>> code path) and not through some hacky-ugly ACPI changes.
> 
> On a second thought, that may be confusing, so I'd create a cpuidle_suspend()
> routine that would be executed right before suspend_device_irqs() by
> dpm_suspend_noirq().  I'd make that routine run a suspend callback from the
> cpuidle driver (the drivers that don't need to "suspend" would not provide that
> callback).  And analogously for resume.

Why is the cpuidle_suspend() necessary? There are already operations
invoking device callbacks (i.e .suspend,.suspend_late and
.suspend_noirq) at different levels of suspend. We need to now add a few
call backs to the idle drivers depending on when during suspend we wish
to invoke them.

As per the above suggestion,we need to add a .suspend_noirq callback to
both the idle drivers which individually set the local acpi_idle_suspend
variable. Again it depends on how significant a difference it makes in
deciding which level of suspend
(i.e dpm_suspend(),dpm_suspend_late() or dpm_suspend_noirq()) executes
this callback.

If it does not make much of a difference we might as well have the
set/unset of acpi_idle_suspend in .suspend callback, as it is now for
acpi idle driver, in the intel_idle driver as well.

The same holds for resume.
> 
> Thanks,
> Rafael

Regards
Preeti
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: Cpuidle drivers,Suspend : Fix suspend/resume hang with intel_idle driver

[Rafael J. Wysocki]

On Monday, July 02, 2012, Srivatsa S. Bhat wrote:
> On 07/02/2012 11:06 AM, preeti wrote:
> > On 06/30/2012 03:37 AM, Rafael J. Wysocki wrote:
> >> On Friday, June 29, 2012, preeti wrote:
> >>> On 06/29/2012 12:41 AM, Rafael J. Wysocki wrote:
> >>>> On Thursday, June 28, 2012, preeti wrote:
> >>>>> On 06/28/2012 03:23 PM, Rafael J. Wysocki wrote:
> >>>>>> On Thursday, June 28, 2012, preeti wrote:
> >>>>>>>
> >>>>>>> From: Preeti U Murthy <preeti@linux.vnet.ibm.com>
> >>>> [...]
> >>>>> cpuidle is an architecture independent part of the kernel  code.Since 
> >>>>> this patch aims at x86 architecture in specific,I considered it
> >>>>> inappropriate.
> >>>>>
> >>>>> In addition to this,suspend happens on x86 only if ACPI is configured.
> >>>>
> >>>> But that is not required for intel_idle, so if it hangs with intel_idle,
> >>>> then it is not dependent on ACPI after all.
> >>> True intel_idle does not need ACPI to be configured,but that also means
> >>> that the kernel will not provide you the means to suspend.
> >>
> >> Yes, it will.  You can hibernate without ACPI in theory.
> > 
> > True.You can suspend to disk without ACPI,but can suspend to RAM only
> > with ACPI.
> > This also highlights the fact that my patch has not taken care of
> > hibernate notifiers,which I should have done.This goes to say that the
> > callbacks during suspend better not be in ACPI specific code.I will look
> > into correcting the placement of the callbacks.
> > 
> >>
> >>> There is no question of resume hang here at all as you cannot suspend in
> >>> the first place.
> >>>
> >>> The issue is when ACPI is configured,and intel_idle is chosen to be the
> >>> cpuidle driver.In this situation when the user suspends,cpus can enter
> >>> deep sleep states as intel_idle driver does not prevent then from doing so.
> >>> This is when resume hangs.
> >>
> >> I know that.
> >>
> >>>>> Therefore it seemed right to put the callback in ACPI specific code
> >>>>> which deals with ACPI sleep support.
> >>>>
> >>>> I wonder if we can address this issue correctly.  That is, in a non-racy
> >>>> way and in a better place.
> >>>>
> >>>> First, I really don't think it is necessary to "suspend" cpuidle (be it
> >>>> ACPI or any other) when device drivers' suspend routines are being
> >>>> executed (which also is racy, because the cpuidle "suspend" may be running
> >>>> concurrently with cpuidle on another CPU) or earlier.  We really may want
> >>>> to disable the deeper C-states when we're about to execute
> >>>> suspend_ops->prepare_late(), or hibernation_ops->prepare(), i.e. after
> >>>> we've run dpm_suspend_end() successfully.
> >>>
> >>> The commit "ACPI:disable lower idle C-states across suspend/resume"
> >>> states that device_suspend() calls ACPI suspend functions which cause
> >>> side effects on the lower idle C-states.
> >>
> >> I'd like to know the details, then.  Which ACPI suspend functions those are,
> >> in particular, because I'm not aware of any in device_suspend().
> >>
> >> Also, please note that this commit is 5 years old and some things have changed
> >> in the suspend code paths since that time.
> > 
> > I agree.My view on this,as I have mentioned in one of my previous mails
> > is, in the patch with the acpi_idle_suspend workaround,we have not taken
> > precautions to check if there are cpus that have already entered deep
> > C-states before the suspend routine has begun.
> > 
> > We take care of disabling entry into deep C-states only during suspend.
> > so if deep C-states are posing problems during suspend,then why dont
> > these cpus that have entered idle states before suspend cause a resume hang?
> >
> 
> Because cpu hotplug kicks the cpu out of any idle state it was in, in order
> to execute the CPU_DYING_FROZEN callbacks. (See my other mail about this).
>  
> >>
> >>> This means we need to disable entry into deeper C-states even before
> >>> dpm_suspend_start(),
> >>
> >> This most likely is wrong.
> >>
> >> We may need to "suspend" cpuidle before calling suspend_device_irqs(),
> >> but then again that shouldn't be made via a notifier I think.  Why don't
> >> we simply make suspend_device_irqs() disable lower C-states to start with?
> >>
> >>> but how much before?
> >>>
> >>> The commit answers this too.It says removing the functionality of
> >>> entering deep C-states before suspend removed the side effects.Besides,
> >>> the commit title says'across suspend/resume'.So I think to address the
> >>> resume hang effectively,it is desirable to disable entry into deeper
> >>> C-states during suspend_prepare operations.
> >>>>
> >>>> So, it seems, it might be a good idea to place a cpuidle driver suspend
> >>>> (and/or hibernation) hook at the end of dpm_suspend_end() and make ACPI
> >>>> and intel_idle drivers implement that hook.
> >>>>
> >>> Dont you think this patch is meant to fix a very ACPI specific problem?
> >>
> >> No, I don't.
> >>
> >>> Which is why I think the call backs or any hook meant to fix this should
> >>> go into ACPI specific code.
> >>> Else it will seem irrelevant to all other architectures that implement
> >>> suspend.
> >>
> >> I don't honestly think it is irrelevant.  The fact is that similar problems
> >> have not been reported on other architectures _yet_, which by no means can
> >> be taken as a proof that those architectures are not affected.
> >>
> >> Anyway, I think that the right way to address this is through a cpuidle driver
> >> callback executed from suspend_device_irqs() (and analogously for the resume
> >> code path) and not through some hacky-ugly ACPI changes.
> > 
> > I agree with having a cpuidle driver callback as even hibernate
> > callbacks need to be taken care of which have nothing to do with ACPI.
> > 
> > But on what basis is the placement of the call back suggested at
> > suspend_device_irqs()? What is the assurance that this placement  will
> > not cause a resume hang considering we dont know what precisely is
> > causing this problem?
> > 
> 
> Any place before CPU hotplug should do, I think.

There may be ugly interactions with suspend_device_irqs(), though, that
I'd prefer to avoid.  Hence my suggestion to place the cpuidle "suspend"
before suspend_device_irqs().

Thanks,
Rafael

Re: [PATCH V2] Suspend,cpuidle:resume_hang fix with cpuidle

[Dave Hansen]

On 07/05/2012 08:11 PM, preeti wrote:
> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
> index 6d9ec3e..b894627 100644
> --- a/drivers/acpi/processor_idle.c
> +++ b/drivers/acpi/processor_idle.c
> @@ -221,10 +221,6 @@ static void lapic_timer_state_broadcast(struct
> acpi_processor *pr,
> 
>  #endif
> 
> -/*
> - * Suspend / resume control
> - */

This looks line-wrapped to me.

[PATCH V2] Suspend,cpuidle:resume_hang fix with cpuidle

[preeti]

From: Preeti U Murthy <preeti@linux.vnet.ibm.com>

On certain bios,resume hangs if cpus are allowed to enter idle states
during suspend[1]

This was fixed in apci idle driver[2].But intel_idle driver does not
have this fix.Thus instead of replicating the fix in both the idle
drivers,or in more platform specific idle drivers if needed,the
more general cpuidle infrastructure could handle this.

A suspend callback in cpuidle_driver could handle this fix.But
a cpuidle_driver provides only basic functionalities like platform idle
state detection capability and mechanisms to support entry and exit
into CPU idle states.All other cpuidle functions are found in the
cpuidle generic infrastructure for good reason that all cpuidle
drivers,irrespective of their platforms will support these functions.

One option therefore would be to register a suspend callback in cpuidle
which handles this fix.This could be called through a PM_SUSPEND_PREPARE
notifier.But this is too generic a notfier for a driver to handle.

Also,ideally the job of cpuidle is not to handle side effects of suspend.
It should expose the interfaces which "handle cpuidle 'during' suspend"
or any other operation,which the subsystems call during that respective
operation.

The fix demands that during suspend,no cpus should be allowed to enter
deep C-states.The interface cpuidle_uninstall_idle_handler() in cpuidle
ensures that.Not just that it also kicks all the cpus which are already
in idle out of their idle states which was being done during cpu hotplug
through a CPU_DYING_FROZEN callbacks.

Now the question arises about when during suspend should
cpuidle_uninstall_idle_handler() be called.Since we are dealing with
drivers it seems best to call this function during dpm_suspend().
Delaying the call till dpm_suspend_noirq() does no harm,as long as it is
before cpu_hotplug_begin() to avoid race conditions with cpu hotpulg
operations.In dpm_suspend_noirq(),it would be wise to place this call
before suspend_device_irqs() to avoid ugly interactions with the same.

Analogously,during resume.

References:
1.https://bugs.launchpad.net/ubuntu/+source/linux/+bug/674075.
2.http://marc.info/?l=linux-pm&m=133958534231884&w=2

Reported-by: Dave Hansen <dave@linux.vnet.ibm.com>
Signed-off-by: Preeti U Murthy <preeti@linux.vnet.ibm.com>
---

 drivers/acpi/processor_idle.c |   30 +-----------------------------
 drivers/base/power/main.c     |    4 +++-
 drivers/cpuidle/cpuidle.c     |   16 ++++++++++++++++
 include/linux/cpuidle.h       |    2 ++
 4 files changed, 22 insertions(+), 30 deletions(-)

diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
index 6d9ec3e..b894627 100644
--- a/drivers/acpi/processor_idle.c
+++ b/drivers/acpi/processor_idle.c
@@ -221,10 +221,6 @@ static void lapic_timer_state_broadcast(struct
acpi_processor *pr,

 #endif

-/*
- * Suspend / resume control
- */
-static int acpi_idle_suspend;
 static u32 saved_bm_rld;

 static void acpi_idle_bm_rld_save(void)
@@ -243,21 +239,13 @@ static void acpi_idle_bm_rld_restore(void)

 int acpi_processor_suspend(struct device *dev)
 {
-	if (acpi_idle_suspend == 1)
-		return 0;
-
 	acpi_idle_bm_rld_save();
-	acpi_idle_suspend = 1;
 	return 0;
 }

 int acpi_processor_resume(struct device *dev)
 {
-	if (acpi_idle_suspend == 0)
-		return 0;
-
 	acpi_idle_bm_rld_restore();
-	acpi_idle_suspend = 0;
 	return 0;
 }

@@ -763,11 +751,6 @@ static int acpi_idle_enter_c1(struct cpuidle_device
*dev,

 	local_irq_disable();

-	if (acpi_idle_suspend) {
-		local_irq_enable();
-		cpu_relax();
-		return -EBUSY;
-	}

 	lapic_timer_state_broadcast(pr, cx, 1);
 	kt1 = ktime_get_real();
@@ -838,11 +821,6 @@ static int acpi_idle_enter_simple(struct
cpuidle_device *dev,

 	local_irq_disable();

-	if (acpi_idle_suspend) {
-		local_irq_enable();
-		cpu_relax();
-		return -EBUSY;
-	}

 	if (cx->entry_method != ACPI_CSTATE_FFH) {
 		current_thread_info()->status &= ~TS_POLLING;
@@ -928,8 +906,7 @@ static int acpi_idle_enter_bm(struct cpuidle_device
*dev,
 						drv, drv->safe_state_index);
 		} else {
 			local_irq_disable();
-			if (!acpi_idle_suspend)
-				acpi_safe_halt();
+			acpi_safe_halt();
 			local_irq_enable();
 			return -EBUSY;
 		}
@@ -937,11 +914,6 @@ static int acpi_idle_enter_bm(struct cpuidle_device
*dev,

 	local_irq_disable();

-	if (acpi_idle_suspend) {
-		local_irq_enable();
-		cpu_relax();
-		return -EBUSY;
-	}

 	if (cx->entry_method != ACPI_CSTATE_FFH) {
 		current_thread_info()->status &= ~TS_POLLING;
diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
index df5f41d..d791950 100644
--- a/drivers/base/power/main.c
+++ b/drivers/base/power/main.c
@@ -28,7 +28,7 @@
 #include <linux/sched.h>
 #include <linux/async.h>
 #include <linux/suspend.h>
-
+#include <linux/cpuidle.h>
 #include "../base.h"
 #include "power.h"

@@ -467,6 +467,7 @@ static void dpm_resume_noirq(pm_message_t state)
 	mutex_unlock(&dpm_list_mtx);
 	dpm_show_time(starttime, state, "noirq");
 	resume_device_irqs();
+	cpuidle_resume();
 }

 /**
@@ -867,6 +868,7 @@ static int dpm_suspend_noirq(pm_message_t state)
 	ktime_t starttime = ktime_get();
 	int error = 0;

+	cpuidle_pause();
 	suspend_device_irqs();
 	mutex_lock(&dpm_list_mtx);
 	while (!list_empty(&dpm_late_early_list)) {
diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
index 0132706..d6a533e 100644
--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -201,6 +201,22 @@ void cpuidle_resume_and_unlock(void)

 EXPORT_SYMBOL_GPL(cpuidle_resume_and_unlock);

+/* Currently used in suspend/resume path to suspend cpuidle */
+void cpuidle_pause(void)
+{
+	mutex_lock(&cpuidle_lock);
+	cpuidle_uninstall_idle_handler();
+	mutex_unlock(&cpuidle_lock);
+}
+
+/* Currently used in suspend/resume path to resume cpuidle */
+void cpuidle_resume(void)
+{
+	mutex_lock(&cpuidle_lock);
+	cpuidle_install_idle_handler();
+	mutex_unlock(&cpuidle_lock);
+}
+
 /**
  * cpuidle_wrap_enter - performs timekeeping and irqen around enter
function
  * @dev: pointer to a valid cpuidle_device object
diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
index 8684a0d..d83b27e 100644
--- a/include/linux/cpuidle.h
+++ b/include/linux/cpuidle.h
@@ -146,6 +146,8 @@ extern void cpuidle_unregister_device(struct
cpuidle_device *dev);

 extern void cpuidle_pause_and_lock(void);
 extern void cpuidle_resume_and_unlock(void);
+extern void cpuidle_pause(void);
+extern void cpuidle_resume(void);
 extern int cpuidle_enable_device(struct cpuidle_device *dev);
 extern void cpuidle_disable_device(struct cpuidle_device *dev);
 extern int cpuidle_wrap_enter(struct cpuidle_device *dev,

Re: [PATCH V2] Suspend,cpuidle:resume_hang fix with cpuidle

[preeti]

On 07/06/2012 08:37 AM, Dave Hansen wrote:
> On 07/05/2012 08:11 PM, preeti wrote:
>> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
>> index 6d9ec3e..b894627 100644
>> --- a/drivers/acpi/processor_idle.c
>> +++ b/drivers/acpi/processor_idle.c
>> @@ -221,10 +221,6 @@ static void lapic_timer_state_broadcast(struct
>> acpi_processor *pr,
>>
>>  #endif
>>
>> -/*
>> - * Suspend / resume control
>> - */
> 
> This looks line-wrapped to me.
> 
Sorry about this!! Will mail the patch again.

Regards
Preeti

[PATCH V2] Suspend,cpuidle:resume_hang fix with cpuidle

[preeti]

From: Preeti U Murthy <preeti@linux.vnet.ibm.com>

On certain bios,resume hangs if cpus are allowed to enter idle states
during suspend[1]

This was fixed in apci idle driver[2].But intel_idle driver does not
have this fix.Thus instead of replicating the fix in both the idle
drivers,or in more platform specific idle drivers if needed,the
more general cpuidle infrastructure could handle this.

A suspend callback in cpuidle_driver could handle this fix.But
a cpuidle_driver provides only basic functionalities like platform idle
state detection capability and mechanisms to support entry and exit
into CPU idle states.All other cpuidle functions are found in the
cpuidle generic infrastructure for good reason that all cpuidle
drivers,irrepective of their platforms will support these functions.

One option therefore would be to register a suspend callback in cpuidle
which handles this fix.This could be called through a PM_SUSPEND_PREPARE
notifier.But this is too generic a notfier for a driver to handle.

Also,ideally the job of cpuidle is not to handle side effects of suspend.
It should expose the interfaces which "handle cpuidle 'during' suspend"
or any other operation,which the subsystems call during that respective
operation.

The fix demands that during suspend,no cpus should be allowed to enter
deep C-states.The interface cpuidle_uninstall_idle_handler() in cpuidle
ensures that.Not just that it also kicks all the cpus which are already
in idle out of their idle states which was being done during cpu hotplug
through a CPU_DYING_FROZEN callbacks.

Now the question arises about when during suspend should
cpuidle_uninstall_idle_handler() be called.Since we are dealing with
drivers it seems best to call this function during dpm_suspend().
Delaying the call till dpm_suspend_noirq() does no harm,as long as it is
before cpu_hotplug_begin() to avoid race conditions with cpu hotpulg
operations.In dpm_suspend_noirq(),it would be wise to place this call
before suspend_device_irqs() to avoid ugly interactions with the same.

Ananlogously,during resume.

References:
1.https://bugs.launchpad.net/ubuntu/+source/linux/+bug/674075.
2.http://marc.info/?l=linux-pm&m=133958534231884&w=2

Reported-by: Dave Hansen <dave@linux.vnet.ibm.com>
Signed-off-by: Preeti U Murthy <preeti@linux.vnet.ibm.com>
---

 drivers/acpi/processor_idle.c |   30 +-----------------------------
 drivers/base/power/main.c     |    4 +++-
 drivers/cpuidle/cpuidle.c     |   16 ++++++++++++++++
 include/linux/cpuidle.h       |    2 ++
 4 files changed, 22 insertions(+), 30 deletions(-)

diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
index 6d9ec3e..b894627 100644
--- a/drivers/acpi/processor_idle.c
+++ b/drivers/acpi/processor_idle.c
@@ -221,10 +221,6 @@ static void lapic_timer_state_broadcast(struct acpi_processor *pr,
 
 #endif
 
-/*
- * Suspend / resume control
- */
-static int acpi_idle_suspend;
 static u32 saved_bm_rld;
 
 static void acpi_idle_bm_rld_save(void)
@@ -243,21 +239,13 @@ static void acpi_idle_bm_rld_restore(void)
 
 int acpi_processor_suspend(struct device *dev)
 {
-	if (acpi_idle_suspend == 1)
-		return 0;
-
 	acpi_idle_bm_rld_save();
-	acpi_idle_suspend = 1;
 	return 0;
 }
 
 int acpi_processor_resume(struct device *dev)
 {
-	if (acpi_idle_suspend == 0)
-		return 0;
-
 	acpi_idle_bm_rld_restore();
-	acpi_idle_suspend = 0;
 	return 0;
 }
 
@@ -763,11 +751,6 @@ static int acpi_idle_enter_c1(struct cpuidle_device *dev,
 
 	local_irq_disable();
 
-	if (acpi_idle_suspend) {
-		local_irq_enable();
-		cpu_relax();
-		return -EBUSY;
-	}
 
 	lapic_timer_state_broadcast(pr, cx, 1);
 	kt1 = ktime_get_real();
@@ -838,11 +821,6 @@ static int acpi_idle_enter_simple(struct cpuidle_device *dev,
 
 	local_irq_disable();
 
-	if (acpi_idle_suspend) {
-		local_irq_enable();
-		cpu_relax();
-		return -EBUSY;
-	}
 
 	if (cx->entry_method != ACPI_CSTATE_FFH) {
 		current_thread_info()->status &= ~TS_POLLING;
@@ -928,8 +906,7 @@ static int acpi_idle_enter_bm(struct cpuidle_device *dev,
 						drv, drv->safe_state_index);
 		} else {
 			local_irq_disable();
-			if (!acpi_idle_suspend)
-				acpi_safe_halt();
+			acpi_safe_halt();
 			local_irq_enable();
 			return -EBUSY;
 		}
@@ -937,11 +914,6 @@ static int acpi_idle_enter_bm(struct cpuidle_device *dev,
 
 	local_irq_disable();
 
-	if (acpi_idle_suspend) {
-		local_irq_enable();
-		cpu_relax();
-		return -EBUSY;
-	}
 
 	if (cx->entry_method != ACPI_CSTATE_FFH) {
 		current_thread_info()->status &= ~TS_POLLING;
diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
index df5f41d..d791950 100644
--- a/drivers/base/power/main.c
+++ b/drivers/base/power/main.c
@@ -28,7 +28,7 @@
 #include <linux/sched.h>
 #include <linux/async.h>
 #include <linux/suspend.h>
-
+#include <linux/cpuidle.h>
 #include "../base.h"
 #include "power.h"
 
@@ -467,6 +467,7 @@ static void dpm_resume_noirq(pm_message_t state)
 	mutex_unlock(&dpm_list_mtx);
 	dpm_show_time(starttime, state, "noirq");
 	resume_device_irqs();
+	cpuidle_resume();
 }
 
 /**
@@ -867,6 +868,7 @@ static int dpm_suspend_noirq(pm_message_t state)
 	ktime_t starttime = ktime_get();
 	int error = 0;
 
+	cpuidle_pause();
 	suspend_device_irqs();
 	mutex_lock(&dpm_list_mtx);
 	while (!list_empty(&dpm_late_early_list)) {
diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
index 0132706..d6a533e 100644
--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -201,6 +201,22 @@ void cpuidle_resume_and_unlock(void)
 
 EXPORT_SYMBOL_GPL(cpuidle_resume_and_unlock);
 
+/* Currently used in suspend/resume path to suspend cpuidle */
+void cpuidle_pause(void)
+{
+	mutex_lock(&cpuidle_lock);
+	cpuidle_uninstall_idle_handler();
+	mutex_unlock(&cpuidle_lock);
+}
+
+/* Currently used in suspend/resume path to resume cpuidle */
+void cpuidle_resume(void)
+{
+	mutex_lock(&cpuidle_lock);
+	cpuidle_install_idle_handler();
+	mutex_unlock(&cpuidle_lock);
+}
+
 /**
  * cpuidle_wrap_enter - performs timekeeping and irqen around enter function
  * @dev: pointer to a valid cpuidle_device object
diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
index 8684a0d..d83b27e 100644
--- a/include/linux/cpuidle.h
+++ b/include/linux/cpuidle.h
@@ -146,6 +146,8 @@ extern void cpuidle_unregister_device(struct cpuidle_device *dev);
 
 extern void cpuidle_pause_and_lock(void);
 extern void cpuidle_resume_and_unlock(void);
+extern void cpuidle_pause(void);
+extern void cpuidle_resume(void);
 extern int cpuidle_enable_device(struct cpuidle_device *dev);
 extern void cpuidle_disable_device(struct cpuidle_device *dev);
 extern int cpuidle_wrap_enter(struct cpuidle_device *dev,

Re: [PATCH V2] Suspend,cpuidle:resume_hang fix with cpuidle

[Srivatsa S. Bhat]

On 07/06/2012 01:12 PM, preeti wrote:
> 
> From: Preeti U Murthy <preeti@linux.vnet.ibm.com>
> 
> On certain bios,resume hangs if cpus are allowed to enter idle states
> during suspend[1]
> 
> This was fixed in apci idle driver[2].But intel_idle driver does not
> have this fix.Thus instead of replicating the fix in both the idle
> drivers,or in more platform specific idle drivers if needed,the
> more general cpuidle infrastructure could handle this.
> 
> A suspend callback in cpuidle_driver could handle this fix.But
> a cpuidle_driver provides only basic functionalities like platform idle
> state detection capability and mechanisms to support entry and exit
> into CPU idle states.All other cpuidle functions are found in the
> cpuidle generic infrastructure for good reason that all cpuidle
> drivers,irrepective of their platforms will support these functions.
> 
> One option therefore would be to register a suspend callback in cpuidle
> which handles this fix.This could be called through a PM_SUSPEND_PREPARE
> notifier.But this is too generic a notfier for a driver to handle.
> 
> Also,ideally the job of cpuidle is not to handle side effects of suspend.
> It should expose the interfaces which "handle cpuidle 'during' suspend"
> or any other operation,which the subsystems call during that respective
> operation.
> 
> The fix demands that during suspend,no cpus should be allowed to enter
> deep C-states.The interface cpuidle_uninstall_idle_handler() in cpuidle
> ensures that.Not just that it also kicks all the cpus which are already
> in idle out of their idle states which was being done during cpu hotplug
> through a CPU_DYING_FROZEN callbacks.
> 
> Now the question arises about when during suspend should
> cpuidle_uninstall_idle_handler() be called.Since we are dealing with
> drivers it seems best to call this function during dpm_suspend().
> Delaying the call till dpm_suspend_noirq() does no harm,as long as it is
> before cpu_hotplug_begin() to avoid race conditions with cpu hotpulg
> operations.In dpm_suspend_noirq(),it would be wise to place this call
> before suspend_device_irqs() to avoid ugly interactions with the same.
> 
> Ananlogously,during resume.
> 
> References:
> 1.https://bugs.launchpad.net/ubuntu/+source/linux/+bug/674075.
> 2.http://marc.info/?l=linux-pm&m=133958534231884&w=2
> 
> Reported-by: Dave Hansen <dave@linux.vnet.ibm.com>
> Signed-off-by: Preeti U Murthy <preeti@linux.vnet.ibm.com>

Much better design. Good to avoid all those ugly flags and checks :-)

Reviewed-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>

Regards,
Srivatsa S. Bhat

> ---
> 
>  drivers/acpi/processor_idle.c |   30 +-----------------------------
>  drivers/base/power/main.c     |    4 +++-
>  drivers/cpuidle/cpuidle.c     |   16 ++++++++++++++++
>  include/linux/cpuidle.h       |    2 ++
>  4 files changed, 22 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
> index 6d9ec3e..b894627 100644
> --- a/drivers/acpi/processor_idle.c
> +++ b/drivers/acpi/processor_idle.c
> @@ -221,10 +221,6 @@ static void lapic_timer_state_broadcast(struct acpi_processor *pr,
> 
>  #endif
> 
> -/*
> - * Suspend / resume control
> - */
> -static int acpi_idle_suspend;
>  static u32 saved_bm_rld;
> 
>  static void acpi_idle_bm_rld_save(void)
> @@ -243,21 +239,13 @@ static void acpi_idle_bm_rld_restore(void)
> 
>  int acpi_processor_suspend(struct device *dev)
>  {
> -	if (acpi_idle_suspend == 1)
> -		return 0;
> -
>  	acpi_idle_bm_rld_save();
> -	acpi_idle_suspend = 1;
>  	return 0;
>  }
> 
>  int acpi_processor_resume(struct device *dev)
>  {
> -	if (acpi_idle_suspend == 0)
> -		return 0;
> -
>  	acpi_idle_bm_rld_restore();
> -	acpi_idle_suspend = 0;
>  	return 0;
>  }
> 
> @@ -763,11 +751,6 @@ static int acpi_idle_enter_c1(struct cpuidle_device *dev,
> 
>  	local_irq_disable();
> 
> -	if (acpi_idle_suspend) {
> -		local_irq_enable();
> -		cpu_relax();
> -		return -EBUSY;
> -	}
> 
>  	lapic_timer_state_broadcast(pr, cx, 1);
>  	kt1 = ktime_get_real();
> @@ -838,11 +821,6 @@ static int acpi_idle_enter_simple(struct cpuidle_device *dev,
> 
>  	local_irq_disable();
> 
> -	if (acpi_idle_suspend) {
> -		local_irq_enable();
> -		cpu_relax();
> -		return -EBUSY;
> -	}
> 
>  	if (cx->entry_method != ACPI_CSTATE_FFH) {
>  		current_thread_info()->status &= ~TS_POLLING;
> @@ -928,8 +906,7 @@ static int acpi_idle_enter_bm(struct cpuidle_device *dev,
>  						drv, drv->safe_state_index);
>  		} else {
>  			local_irq_disable();
> -			if (!acpi_idle_suspend)
> -				acpi_safe_halt();
> +			acpi_safe_halt();
>  			local_irq_enable();
>  			return -EBUSY;
>  		}
> @@ -937,11 +914,6 @@ static int acpi_idle_enter_bm(struct cpuidle_device *dev,
> 
>  	local_irq_disable();
> 
> -	if (acpi_idle_suspend) {
> -		local_irq_enable();
> -		cpu_relax();
> -		return -EBUSY;
> -	}
> 
>  	if (cx->entry_method != ACPI_CSTATE_FFH) {
>  		current_thread_info()->status &= ~TS_POLLING;
> diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
> index df5f41d..d791950 100644
> --- a/drivers/base/power/main.c
> +++ b/drivers/base/power/main.c
> @@ -28,7 +28,7 @@
>  #include <linux/sched.h>
>  #include <linux/async.h>
>  #include <linux/suspend.h>
> -
> +#include <linux/cpuidle.h>
>  #include "../base.h"
>  #include "power.h"
> 
> @@ -467,6 +467,7 @@ static void dpm_resume_noirq(pm_message_t state)
>  	mutex_unlock(&dpm_list_mtx);
>  	dpm_show_time(starttime, state, "noirq");
>  	resume_device_irqs();
> +	cpuidle_resume();
>  }
> 
>  /**
> @@ -867,6 +868,7 @@ static int dpm_suspend_noirq(pm_message_t state)
>  	ktime_t starttime = ktime_get();
>  	int error = 0;
> 
> +	cpuidle_pause();
>  	suspend_device_irqs();
>  	mutex_lock(&dpm_list_mtx);
>  	while (!list_empty(&dpm_late_early_list)) {
> diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
> index 0132706..d6a533e 100644
> --- a/drivers/cpuidle/cpuidle.c
> +++ b/drivers/cpuidle/cpuidle.c
> @@ -201,6 +201,22 @@ void cpuidle_resume_and_unlock(void)
> 
>  EXPORT_SYMBOL_GPL(cpuidle_resume_and_unlock);
> 
> +/* Currently used in suspend/resume path to suspend cpuidle */
> +void cpuidle_pause(void)
> +{
> +	mutex_lock(&cpuidle_lock);
> +	cpuidle_uninstall_idle_handler();
> +	mutex_unlock(&cpuidle_lock);
> +}
> +
> +/* Currently used in suspend/resume path to resume cpuidle */
> +void cpuidle_resume(void)
> +{
> +	mutex_lock(&cpuidle_lock);
> +	cpuidle_install_idle_handler();
> +	mutex_unlock(&cpuidle_lock);
> +}
> +
>  /**
>   * cpuidle_wrap_enter - performs timekeeping and irqen around enter function
>   * @dev: pointer to a valid cpuidle_device object
> diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
> index 8684a0d..d83b27e 100644
> --- a/include/linux/cpuidle.h
> +++ b/include/linux/cpuidle.h
> @@ -146,6 +146,8 @@ extern void cpuidle_unregister_device(struct cpuidle_device *dev);
> 
>  extern void cpuidle_pause_and_lock(void);
>  extern void cpuidle_resume_and_unlock(void);
> +extern void cpuidle_pause(void);
> +extern void cpuidle_resume(void);
>  extern int cpuidle_enable_device(struct cpuidle_device *dev);
>  extern void cpuidle_disable_device(struct cpuidle_device *dev);
>  extern int cpuidle_wrap_enter(struct cpuidle_device *dev,
> 
> 

Re: [PATCH V2] Suspend,cpuidle:resume_hang fix with cpuidle

[Dave Hansen]

On 07/06/2012 10:23 AM, Rafael J. Wysocki wrote:
> OK, this looks good to me.  Queuing up in the linux-next branch of the
> linux-pm.git tree.  If no problems with it are reported, I'll move it to the
> pm-cpuidle branch in a couple of days.

I've got this running on the problem hardware.  It seems to be OK, at
least initially, so feel free to add my Tested-by.

Re: [PATCH V2] Suspend,cpuidle:resume_hang fix with cpuidle

[Rafael J. Wysocki]

On Friday, July 06, 2012, preeti wrote:
> 
> From: Preeti U Murthy <preeti@linux.vnet.ibm.com>
> 
> On certain bios,resume hangs if cpus are allowed to enter idle states
> during suspend[1]
> 
> This was fixed in apci idle driver[2].But intel_idle driver does not
> have this fix.Thus instead of replicating the fix in both the idle
> drivers,or in more platform specific idle drivers if needed,the
> more general cpuidle infrastructure could handle this.
> 
> A suspend callback in cpuidle_driver could handle this fix.But
> a cpuidle_driver provides only basic functionalities like platform idle
> state detection capability and mechanisms to support entry and exit
> into CPU idle states.All other cpuidle functions are found in the
> cpuidle generic infrastructure for good reason that all cpuidle
> drivers,irrepective of their platforms will support these functions.
> 
> One option therefore would be to register a suspend callback in cpuidle
> which handles this fix.This could be called through a PM_SUSPEND_PREPARE
> notifier.But this is too generic a notfier for a driver to handle.
> 
> Also,ideally the job of cpuidle is not to handle side effects of suspend.
> It should expose the interfaces which "handle cpuidle 'during' suspend"
> or any other operation,which the subsystems call during that respective
> operation.
> 
> The fix demands that during suspend,no cpus should be allowed to enter
> deep C-states.The interface cpuidle_uninstall_idle_handler() in cpuidle
> ensures that.Not just that it also kicks all the cpus which are already
> in idle out of their idle states which was being done during cpu hotplug
> through a CPU_DYING_FROZEN callbacks.
> 
> Now the question arises about when during suspend should
> cpuidle_uninstall_idle_handler() be called.Since we are dealing with
> drivers it seems best to call this function during dpm_suspend().
> Delaying the call till dpm_suspend_noirq() does no harm,as long as it is
> before cpu_hotplug_begin() to avoid race conditions with cpu hotpulg
> operations.In dpm_suspend_noirq(),it would be wise to place this call
> before suspend_device_irqs() to avoid ugly interactions with the same.
> 
> Ananlogously,during resume.
> 
> References:
> 1.https://bugs.launchpad.net/ubuntu/+source/linux/+bug/674075.
> 2.http://marc.info/?l=linux-pm&m=133958534231884&w=2
> 
> Reported-by: Dave Hansen <dave@linux.vnet.ibm.com>
> Signed-off-by: Preeti U Murthy <preeti@linux.vnet.ibm.com>

OK, this looks good to me.  Queuing up in the linux-next branch of the
linux-pm.git tree.  If no problems with it are reported, I'll move it to the
pm-cpuidle branch in a couple of days.

Thanks,
Rafael


> ---
> 
>  drivers/acpi/processor_idle.c |   30 +-----------------------------
>  drivers/base/power/main.c     |    4 +++-
>  drivers/cpuidle/cpuidle.c     |   16 ++++++++++++++++
>  include/linux/cpuidle.h       |    2 ++
>  4 files changed, 22 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
> index 6d9ec3e..b894627 100644
> --- a/drivers/acpi/processor_idle.c
> +++ b/drivers/acpi/processor_idle.c
> @@ -221,10 +221,6 @@ static void lapic_timer_state_broadcast(struct acpi_processor *pr,
>  
>  #endif
>  
> -/*
> - * Suspend / resume control
> - */
> -static int acpi_idle_suspend;
>  static u32 saved_bm_rld;
>  
>  static void acpi_idle_bm_rld_save(void)
> @@ -243,21 +239,13 @@ static void acpi_idle_bm_rld_restore(void)
>  
>  int acpi_processor_suspend(struct device *dev)
>  {
> -	if (acpi_idle_suspend == 1)
> -		return 0;
> -
>  	acpi_idle_bm_rld_save();
> -	acpi_idle_suspend = 1;
>  	return 0;
>  }
>  
>  int acpi_processor_resume(struct device *dev)
>  {
> -	if (acpi_idle_suspend == 0)
> -		return 0;
> -
>  	acpi_idle_bm_rld_restore();
> -	acpi_idle_suspend = 0;
>  	return 0;
>  }
>  
> @@ -763,11 +751,6 @@ static int acpi_idle_enter_c1(struct cpuidle_device *dev,
>  
>  	local_irq_disable();
>  
> -	if (acpi_idle_suspend) {
> -		local_irq_enable();
> -		cpu_relax();
> -		return -EBUSY;
> -	}
>  
>  	lapic_timer_state_broadcast(pr, cx, 1);
>  	kt1 = ktime_get_real();
> @@ -838,11 +821,6 @@ static int acpi_idle_enter_simple(struct cpuidle_device *dev,
>  
>  	local_irq_disable();
>  
> -	if (acpi_idle_suspend) {
> -		local_irq_enable();
> -		cpu_relax();
> -		return -EBUSY;
> -	}
>  
>  	if (cx->entry_method != ACPI_CSTATE_FFH) {
>  		current_thread_info()->status &= ~TS_POLLING;
> @@ -928,8 +906,7 @@ static int acpi_idle_enter_bm(struct cpuidle_device *dev,
>  						drv, drv->safe_state_index);
>  		} else {
>  			local_irq_disable();
> -			if (!acpi_idle_suspend)
> -				acpi_safe_halt();
> +			acpi_safe_halt();
>  			local_irq_enable();
>  			return -EBUSY;
>  		}
> @@ -937,11 +914,6 @@ static int acpi_idle_enter_bm(struct cpuidle_device *dev,
>  
>  	local_irq_disable();
>  
> -	if (acpi_idle_suspend) {
> -		local_irq_enable();
> -		cpu_relax();
> -		return -EBUSY;
> -	}
>  
>  	if (cx->entry_method != ACPI_CSTATE_FFH) {
>  		current_thread_info()->status &= ~TS_POLLING;
> diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
> index df5f41d..d791950 100644
> --- a/drivers/base/power/main.c
> +++ b/drivers/base/power/main.c
> @@ -28,7 +28,7 @@
>  #include <linux/sched.h>
>  #include <linux/async.h>
>  #include <linux/suspend.h>
> -
> +#include <linux/cpuidle.h>
>  #include "../base.h"
>  #include "power.h"
>  
> @@ -467,6 +467,7 @@ static void dpm_resume_noirq(pm_message_t state)
>  	mutex_unlock(&dpm_list_mtx);
>  	dpm_show_time(starttime, state, "noirq");
>  	resume_device_irqs();
> +	cpuidle_resume();
>  }
>  
>  /**
> @@ -867,6 +868,7 @@ static int dpm_suspend_noirq(pm_message_t state)
>  	ktime_t starttime = ktime_get();
>  	int error = 0;
>  
> +	cpuidle_pause();
>  	suspend_device_irqs();
>  	mutex_lock(&dpm_list_mtx);
>  	while (!list_empty(&dpm_late_early_list)) {
> diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
> index 0132706..d6a533e 100644
> --- a/drivers/cpuidle/cpuidle.c
> +++ b/drivers/cpuidle/cpuidle.c
> @@ -201,6 +201,22 @@ void cpuidle_resume_and_unlock(void)
>  
>  EXPORT_SYMBOL_GPL(cpuidle_resume_and_unlock);
>  
> +/* Currently used in suspend/resume path to suspend cpuidle */
> +void cpuidle_pause(void)
> +{
> +	mutex_lock(&cpuidle_lock);
> +	cpuidle_uninstall_idle_handler();
> +	mutex_unlock(&cpuidle_lock);
> +}
> +
> +/* Currently used in suspend/resume path to resume cpuidle */
> +void cpuidle_resume(void)
> +{
> +	mutex_lock(&cpuidle_lock);
> +	cpuidle_install_idle_handler();
> +	mutex_unlock(&cpuidle_lock);
> +}
> +
>  /**
>   * cpuidle_wrap_enter - performs timekeeping and irqen around enter function
>   * @dev: pointer to a valid cpuidle_device object
> diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
> index 8684a0d..d83b27e 100644
> --- a/include/linux/cpuidle.h
> +++ b/include/linux/cpuidle.h
> @@ -146,6 +146,8 @@ extern void cpuidle_unregister_device(struct cpuidle_device *dev);
>  
>  extern void cpuidle_pause_and_lock(void);
>  extern void cpuidle_resume_and_unlock(void);
> +extern void cpuidle_pause(void);
> +extern void cpuidle_resume(void);
>  extern int cpuidle_enable_device(struct cpuidle_device *dev);
>  extern void cpuidle_disable_device(struct cpuidle_device *dev);
>  extern int cpuidle_wrap_enter(struct cpuidle_device *dev,
> 
> 
> 
> 

Re: [PATCH V2] Suspend,cpuidle:resume_hang fix with cpuidle

[Rafael J. Wysocki]

On Friday, July 06, 2012, Dave Hansen wrote:
> On 07/06/2012 10:23 AM, Rafael J. Wysocki wrote:
> > OK, this looks good to me.  Queuing up in the linux-next branch of the
> > linux-pm.git tree.  If no problems with it are reported, I'll move it to the
> > pm-cpuidle branch in a couple of days.
> 
> I've got this running on the problem hardware.  It seems to be OK, at
> least initially, so feel free to add my Tested-by.

OK, thanks!