ACPI, cpuidle: Fix suspend/resume regression caused by cpuidle cleanup.

PATCH Applied Deepthi Dharwar 3 participants 8 messages Jun 13, 2012 – Jun 19, 2012

Subsystems (3)

Stable branch x86 ACPI

lore

Share & tools

lore

Deepthi Dharwar

author

2012-06-13 10:58 UTC #0 lore raw

PATCH 1/1 ACPI, cpuidle: Fix suspend/resume regression caused by cpuidle cleanup.

From: Deepthi Dharwar <[email protected]>

Fix suspend/resume regression caused by cpuidle cleanup.

Commit e978aa7d7d57d04eb5f88a7507c4fb98577def77 ( cpuidle: Move dev->last_residency update to driver enter routine; remove dev->last_state) was breaking suspend on laptops, as reported in the below link
- https://lkml.org/lkml/2011/11/11/164

This was fixed in commit 3439a8da16bcad6b0982ece938c9f8299bb53584 (ACPI / cpuidle: Remove acpi_idle_suspend (to fix suspend regression) by removing acpi_idle_suspend flag.
- https://lkml.org/lkml/2011/11/14/74

But this fix did not work on all systems
as Suspend/resume regression was reported on Lenovo S10-3 recently by Dave.
- https://lkml.org/lkml/2012/5/27/115
It looked like with commit e978aa7d broke suspend and with commit 3439a8da resume was not working with acpi_idle driver.

This patch fixes the regression that caused this issue in the first place. acpi_idle_suspend flag is essential on some x86 systems to prevent the cpus from going to deeper C-states when suspend is triggered ( commit b04e7bdb984 )
So reverting the commit 3439a8da is essential.

By default, irqs are disabled in cpu_idle arch specific call and re-enabled in idle state return path . As the acpi_idle_suspend flag was being set during suspend, which prevented the cpus going to deeper idle states, it is essential to
enabling the irqs in its return path too.

To address the suspend issue,
we were not re-enabling the interrupts while returning from acpi_idle_enter_bm() routine if acpi_idle_suspend flag is set. and this was causing suspend failure.

In addition to the above fix, a sanity check has also been added in x86 arch specific cpu_idle call to ensure that the idle call always returns with IRQs enabled.

This patch applies on 3.5-rc2
---

Reported-and-Tested-by Dav Hansen <[email protected]>

Tested-by Preeti Murthy <[email protected]>

Signed-off-by Deepthi Dharwar <[email protected]>

Reviewed-by Srivatsa S Bhat <[email protected]>

---
 arch/x86/kernel/process.c     |    6 ++++++
 drivers/acpi/processor_idle.c |   28 ++++++++++++++++++++++++++++
 2 files changed, 34 insertions(+), 0 deletions(-)

arch/x86/kernel/process.c +6 -0

index 735279e..8ab76ad 100644
@@ -460,6 +460,12 @@ void cpu_idle(void)
 				pm_idle();
 
 			rcu_idle_exit();
+
+			/*
+			 * Sanity check to ensure that idle call returns
+			 * with IRQs enabled
+			 */
+			WARN_ON(irqs_disabled());
 			start_critical_timings();
 
 			/* In many cases the interrupt that ended idle

drivers/acpi/processor_idle.c +28 -0

index f3decb3..c2ffd84 100644
@@ -224,6 +224,7 @@ static void lapic_timer_state_broadcast(struct acpi_processor *pr,
 /*
  * Suspend / resume control
  */
+static int acpi_idle_suspend;
 static u32 saved_bm_rld;
 
 static void acpi_idle_bm_rld_save(void)

@@ -242,13 +243,21 @@ 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;
 }

@@ -754,6 +763,12 @@ static int acpi_idle_enter_c1(struct cpuidle_device *dev,
 
 	local_irq_disable();
 
+	if (acpi_idle_suspend) {
+		local_irq_enable();
+		cpu_relax();
+		return -EINVAL;
+	}
+
 	lapic_timer_state_broadcast(pr, cx, 1);
 	kt1 = ktime_get_real();
 	acpi_idle_do_entry(cx);

@@ -823,6 +838,12 @@ static int acpi_idle_enter_simple(struct cpuidle_device *dev,
 
 	local_irq_disable();
 
+	if (acpi_idle_suspend) {
+		local_irq_enable();
+		cpu_relax();
+		return -EINVAL;
+	}
+
 	if (cx->entry_method != ACPI_CSTATE_FFH) {
 		current_thread_info()->status &= ~TS_POLLING;
 		/*

@@ -901,6 +922,13 @@ 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) {
 			return drv->states[drv->safe_state_index].enter(dev,

Regards,
Deepthi

Dave Hansen

Tested-by on v1 1/1

2012-06-13 17:25 UTC #1 lore raw

Quoted 5 lines · Deepthi Dharwar

In addition to the above fix, a sanity check has also been added in x86 arch specific cpu_idle call to ensure that the idle call always returns with IRQs enabled.
This patch applies on 3.5-rc2

Just applied this on top of a post-3.5-rc2 kernel (7794f70). Suspend/resume seems to work just fine on the hardware where it was broken. Thanks, Deepthi!

My tested-by was for the original hackish patch, but this is just to spell out that it worked for me with this patch too.

Tested-by Dave Hansen <[email protected]>

Rafael J. Wysocki

Acked-by on v1 1/1

2012-06-13 19:56 UTC #2 lore raw

In reply to Deepthi Dharwar

Quoted 43 lines · Deepthi Dharwar

From: Deepthi Dharwar <[email protected]>
Fix suspend/resume regression caused by cpuidle cleanup.
Commit e978aa7d7d57d04eb5f88a7507c4fb98577def77 ( cpuidle: Move dev->last_residency update to driver enter routine; remove dev->last_state) was breaking suspend on laptops, as reported in the below link
- https://lkml.org/lkml/2011/11/11/164
This was fixed in commit 3439a8da16bcad6b0982ece938c9f8299bb53584 (ACPI / cpuidle: Remove acpi_idle_suspend (to fix suspend regression) by removing acpi_idle_suspend flag.
- https://lkml.org/lkml/2011/11/14/74
But this fix did not work on all systems
as Suspend/resume regression was reported on Lenovo S10-3 recently by Dave.
- https://lkml.org/lkml/2012/5/27/115
It looked like with commit e978aa7d broke suspend and with commit 3439a8da resume was not working with acpi_idle driver.
This patch fixes the regression that caused this issue in the first place. acpi_idle_suspend flag is essential on some x86 systems to prevent the cpus from going to deeper C-states when suspend is triggered ( commit b04e7bdb984 )
So reverting the commit 3439a8da is essential.
By default, irqs are disabled in cpu_idle arch specific call and re-enabled in idle state return path . As the acpi_idle_suspend flag was being set during suspend, which prevented the cpus going to deeper idle states, it is essential to
enabling the irqs in its return path too.
To address the suspend issue,
we were not re-enabling the interrupts while returning from acpi_idle_enter_bm() routine if acpi_idle_suspend flag is set. and this was causing suspend failure.
In addition to the above fix, a sanity check has also been added in x86 arch specific cpu_idle call to ensure that the idle call always returns with IRQs enabled.

Quoted 7 lines

This patch applies on 3.5-rc2
---
Reported-and-Tested-by Dav Hansen <[email protected]>
Tested-by Preeti Murthy <[email protected]>
Signed-off-by Deepthi Dharwar <[email protected]>
Reviewed-by Srivatsa S Bhat <[email protected]>

Acked-by Rafael J. Wysocki <[email protected]>

Quoted 102 lines

---
 arch/x86/kernel/process.c     |    6 ++++++
 drivers/acpi/processor_idle.c |   28 ++++++++++++++++++++++++++++
 2 files changed, 34 insertions(+), 0 deletions(-)

arch/x86/kernel/process.c +6 -0

index 735279e..8ab76ad 100644
@@ -460,6 +460,12 @@ void cpu_idle(void)
 				pm_idle();
 
 			rcu_idle_exit();
+
+			/*
+			 * Sanity check to ensure that idle call returns
+			 * with IRQs enabled
+			 */
+			WARN_ON(irqs_disabled());
 			start_critical_timings();
 
 			/* In many cases the interrupt that ended idle

drivers/acpi/processor_idle.c +28 -0

index f3decb3..c2ffd84 100644
@@ -224,6 +224,7 @@ static void lapic_timer_state_broadcast(struct acpi_processor *pr,
 /*
  * Suspend / resume control
  */
+static int acpi_idle_suspend;
 static u32 saved_bm_rld;
 
 static void acpi_idle_bm_rld_save(void)

@@ -242,13 +243,21 @@ 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;
 }

@@ -754,6 +763,12 @@ static int acpi_idle_enter_c1(struct cpuidle_device *dev,
 
 	local_irq_disable();
 
+	if (acpi_idle_suspend) {
+		local_irq_enable();
+		cpu_relax();
+		return -EINVAL;
+	}
+
 	lapic_timer_state_broadcast(pr, cx, 1);
 	kt1 = ktime_get_real();
 	acpi_idle_do_entry(cx);

@@ -823,6 +838,12 @@ static int acpi_idle_enter_simple(struct cpuidle_device *dev,
 
 	local_irq_disable();
 
+	if (acpi_idle_suspend) {
+		local_irq_enable();
+		cpu_relax();
+		return -EINVAL;
+	}
+
 	if (cx->entry_method != ACPI_CSTATE_FFH) {
 		current_thread_info()->status &= ~TS_POLLING;
 		/*

@@ -901,6 +922,13 @@ 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) {
 			return drv->states[drv->safe_state_index].enter(dev,

Regards,
Deepthi

Deepthi Dharwar

author

2012-06-15 12:16 UTC #3 lore raw

Please include this in -stable tree, as this is a regression fix which affects older kernels ( 3.1 onwards )
Link to the patch :
http://permalink.gmane.org/gmane.linux.power-management.general/27614

Thanks !
Deepthi

Quoted 162 lines · Rafael J. Wysocki

Quoted 43 lines · Deepthi Dharwar
From: Deepthi Dharwar <[email protected]>
Fix suspend/resume regression caused by cpuidle cleanup.
Commit e978aa7d7d57d04eb5f88a7507c4fb98577def77 ( cpuidle: Move dev->last_residency update to driver enter routine; remove dev->last_state) was breaking suspend on laptops, as reported in the below link
- https://lkml.org/lkml/2011/11/11/164
This was fixed in commit 3439a8da16bcad6b0982ece938c9f8299bb53584 (ACPI / cpuidle: Remove acpi_idle_suspend (to fix suspend regression) by removing acpi_idle_suspend flag.
- https://lkml.org/lkml/2011/11/14/74
But this fix did not work on all systems
as Suspend/resume regression was reported on Lenovo S10-3 recently by Dave.
- https://lkml.org/lkml/2012/5/27/115
It looked like with commit e978aa7d broke suspend and with commit 3439a8da resume was not working with acpi_idle driver.
This patch fixes the regression that caused this issue in the first place. acpi_idle_suspend flag is essential on some x86 systems to prevent the cpus from going to deeper C-states when suspend is triggered ( commit b04e7bdb984 )
So reverting the commit 3439a8da is essential.
By default, irqs are disabled in cpu_idle arch specific call and re-enabled in idle state return path . As the acpi_idle_suspend flag was being set during suspend, which prevented the cpus going to deeper idle states, it is essential to
enabling the irqs in its return path too.
To address the suspend issue,
we were not re-enabling the interrupts while returning from acpi_idle_enter_bm() routine if acpi_idle_suspend flag is set. and this was causing suspend failure.
In addition to the above fix, a sanity check has also been added in x86 arch specific cpu_idle call to ensure that the idle call always returns with IRQs enabled.
Quoted 7 lines
This patch applies on 3.5-rc2
---
Reported-and-Tested-by Dav Hansen <[email protected]>
Tested-by Preeti Murthy <[email protected]>
Signed-off-by Deepthi Dharwar <[email protected]>
Reviewed-by Srivatsa S Bhat <[email protected]>
Acked-by Rafael J. Wysocki <[email protected]>
Quoted 97 lines
---
 arch/x86/kernel/process.c     |    6 ++++++
 drivers/acpi/processor_idle.c |   28 ++++++++++++++++++++++++++++
 2 files changed, 34 insertions(+), 0 deletions(-)
arch/x86/kernel/process.c +6 -0
index 735279e..8ab76ad 100644
@@ -460,6 +460,12 @@ void cpu_idle(void)
 				pm_idle();
 
 			rcu_idle_exit();
+
+			/*
+			 * Sanity check to ensure that idle call returns
+			 * with IRQs enabled
+			 */
+			WARN_ON(irqs_disabled());
 			start_critical_timings();
 
 			/* In many cases the interrupt that ended idle
drivers/acpi/processor_idle.c +28 -0
index f3decb3..c2ffd84 100644
@@ -224,6 +224,7 @@ static void lapic_timer_state_broadcast(struct acpi_processor *pr,
 /*
  * Suspend / resume control
  */
+static int acpi_idle_suspend;
 static u32 saved_bm_rld;
 
 static void acpi_idle_bm_rld_save(void)
@@ -242,13 +243,21 @@ 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;
 }
 
@@ -754,6 +763,12 @@ static int acpi_idle_enter_c1(struct cpuidle_device *dev,
 
 	local_irq_disable();
 
+	if (acpi_idle_suspend) {
+		local_irq_enable();
+		cpu_relax();
+		return -EINVAL;
+	}
+
 	lapic_timer_state_broadcast(pr, cx, 1);
 	kt1 = ktime_get_real();
 	acpi_idle_do_entry(cx);
@@ -823,6 +838,12 @@ static int acpi_idle_enter_simple(struct cpuidle_device *dev,
 
 	local_irq_disable();
 
+	if (acpi_idle_suspend) {
+		local_irq_enable();
+		cpu_relax();
+		return -EINVAL;
+	}
+
 	if (cx->entry_method != ACPI_CSTATE_FFH) {
 		current_thread_info()->status &= ~TS_POLLING;
 		/*
@@ -901,6 +922,13 @@ 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) {
 			return drv->states[drv->safe_state_index].enter(dev,
Quoted 5 lines
Regards,
Deepthi
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Deepthi Dharwar

author

2012-06-15 12:25 UTC #4 lore raw

Apologies for the confusion.
Please add the Cc: stable tag for this patch as this regression affects older kernels too.
Link to the patch :
http://permalink.gmane.org/gmane.linux.power-management.general/27614

Thanks !
Deepthi

Quoted 2 lines · Deepthi Dharwar

Please include this in -stable tree, as this is a regression fix which affects older kernels ( 3.1 onwards )

Sorry for the typo. :(

Quoted 172 lines

Link to the patch :
http://permalink.gmane.org/gmane.linux.power-management.general/27614
Thanks !
Deepthi
Quoted 162 lines · Rafael J. Wysocki
Quoted 43 lines · Deepthi Dharwar
From: Deepthi Dharwar <[email protected]>
Fix suspend/resume regression caused by cpuidle cleanup.
Commit e978aa7d7d57d04eb5f88a7507c4fb98577def77 ( cpuidle: Move dev->last_residency update to driver enter routine; remove dev->last_state) was breaking suspend on laptops, as reported in the below link
- https://lkml.org/lkml/2011/11/11/164
This was fixed in commit 3439a8da16bcad6b0982ece938c9f8299bb53584 (ACPI / cpuidle: Remove acpi_idle_suspend (to fix suspend regression) by removing acpi_idle_suspend flag.
- https://lkml.org/lkml/2011/11/14/74
But this fix did not work on all systems
as Suspend/resume regression was reported on Lenovo S10-3 recently by Dave.
- https://lkml.org/lkml/2012/5/27/115
It looked like with commit e978aa7d broke suspend and with commit 3439a8da resume was not working with acpi_idle driver.
This patch fixes the regression that caused this issue in the first place. acpi_idle_suspend flag is essential on some x86 systems to prevent the cpus from going to deeper C-states when suspend is triggered ( commit b04e7bdb984 )
So reverting the commit 3439a8da is essential.
By default, irqs are disabled in cpu_idle arch specific call and re-enabled in idle state return path . As the acpi_idle_suspend flag was being set during suspend, which prevented the cpus going to deeper idle states, it is essential to
enabling the irqs in its return path too.
To address the suspend issue,
we were not re-enabling the interrupts while returning from acpi_idle_enter_bm() routine if acpi_idle_suspend flag is set. and this was causing suspend failure.
In addition to the above fix, a sanity check has also been added in x86 arch specific cpu_idle call to ensure that the idle call always returns with IRQs enabled.
Quoted 7 lines
This patch applies on 3.5-rc2
---
Reported-and-Tested-by Dav Hansen <[email protected]>
Tested-by Preeti Murthy <[email protected]>
Signed-off-by Deepthi Dharwar <[email protected]>
Reviewed-by Srivatsa S Bhat <[email protected]>
Acked-by Rafael J. Wysocki <[email protected]>
Quoted 97 lines
---
 arch/x86/kernel/process.c     |    6 ++++++
 drivers/acpi/processor_idle.c |   28 ++++++++++++++++++++++++++++
 2 files changed, 34 insertions(+), 0 deletions(-)
arch/x86/kernel/process.c +6 -0
index 735279e..8ab76ad 100644
@@ -460,6 +460,12 @@ void cpu_idle(void)
 				pm_idle();
 
 			rcu_idle_exit();
+
+			/*
+			 * Sanity check to ensure that idle call returns
+			 * with IRQs enabled
+			 */
+			WARN_ON(irqs_disabled());
 			start_critical_timings();
 
 			/* In many cases the interrupt that ended idle
drivers/acpi/processor_idle.c +28 -0
index f3decb3..c2ffd84 100644
@@ -224,6 +224,7 @@ static void lapic_timer_state_broadcast(struct acpi_processor *pr,
 /*
  * Suspend / resume control
  */
+static int acpi_idle_suspend;
 static u32 saved_bm_rld;
 
 static void acpi_idle_bm_rld_save(void)
@@ -242,13 +243,21 @@ 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;
 }
 
@@ -754,6 +763,12 @@ static int acpi_idle_enter_c1(struct cpuidle_device *dev,
 
 	local_irq_disable();
 
+	if (acpi_idle_suspend) {
+		local_irq_enable();
+		cpu_relax();
+		return -EINVAL;
+	}
+
 	lapic_timer_state_broadcast(pr, cx, 1);
 	kt1 = ktime_get_real();
 	acpi_idle_do_entry(cx);
@@ -823,6 +838,12 @@ static int acpi_idle_enter_simple(struct cpuidle_device *dev,
 
 	local_irq_disable();
 
+	if (acpi_idle_suspend) {
+		local_irq_enable();
+		cpu_relax();
+		return -EINVAL;
+	}
+
 	if (cx->entry_method != ACPI_CSTATE_FFH) {
 		current_thread_info()->status &= ~TS_POLLING;
 		/*
@@ -901,6 +922,13 @@ 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) {
 			return drv->states[drv->safe_state_index].enter(dev,
Quoted 5 lines
Regards,
Deepthi
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Deepthi Dharwar

author

2012-06-15 12:27 UTC #5 lore raw

In reply to Deepthi Dharwar

Thanks !
Deepthi

Quoted 174 lines · Deepthi Dharwar

Please include this in -stable tree, as this is a regression fix which affects older kernels ( 3.1 onwards )
Link to the patch :
http://permalink.gmane.org/gmane.linux.power-management.general/27614
Thanks !
Deepthi
Quoted 162 lines · Rafael J. Wysocki
Quoted 43 lines · Deepthi Dharwar
From: Deepthi Dharwar <[email protected]>
Fix suspend/resume regression caused by cpuidle cleanup.
Commit e978aa7d7d57d04eb5f88a7507c4fb98577def77 ( cpuidle: Move dev->last_residency update to driver enter routine; remove dev->last_state) was breaking suspend on laptops, as reported in the below link
- https://lkml.org/lkml/2011/11/11/164
This was fixed in commit 3439a8da16bcad6b0982ece938c9f8299bb53584 (ACPI / cpuidle: Remove acpi_idle_suspend (to fix suspend regression) by removing acpi_idle_suspend flag.
- https://lkml.org/lkml/2011/11/14/74
But this fix did not work on all systems
as Suspend/resume regression was reported on Lenovo S10-3 recently by Dave.
- https://lkml.org/lkml/2012/5/27/115
It looked like with commit e978aa7d broke suspend and with commit 3439a8da resume was not working with acpi_idle driver.
This patch fixes the regression that caused this issue in the first place. acpi_idle_suspend flag is essential on some x86 systems to prevent the cpus from going to deeper C-states when suspend is triggered ( commit b04e7bdb984 )
So reverting the commit 3439a8da is essential.
By default, irqs are disabled in cpu_idle arch specific call and re-enabled in idle state return path . As the acpi_idle_suspend flag was being set during suspend, which prevented the cpus going to deeper idle states, it is essential to
enabling the irqs in its return path too.
To address the suspend issue,
we were not re-enabling the interrupts while returning from acpi_idle_enter_bm() routine if acpi_idle_suspend flag is set. and this was causing suspend failure.
In addition to the above fix, a sanity check has also been added in x86 arch specific cpu_idle call to ensure that the idle call always returns with IRQs enabled.
Quoted 7 lines
This patch applies on 3.5-rc2
---
Reported-and-Tested-by Dav Hansen <[email protected]>
Tested-by Preeti Murthy <[email protected]>
Signed-off-by Deepthi Dharwar <[email protected]>
Reviewed-by Srivatsa S Bhat <[email protected]>
Acked-by Rafael J. Wysocki <[email protected]>
Quoted 97 lines
---
 arch/x86/kernel/process.c     |    6 ++++++
 drivers/acpi/processor_idle.c |   28 ++++++++++++++++++++++++++++
 2 files changed, 34 insertions(+), 0 deletions(-)
arch/x86/kernel/process.c +6 -0
index 735279e..8ab76ad 100644
@@ -460,6 +460,12 @@ void cpu_idle(void)
 				pm_idle();
 
 			rcu_idle_exit();
+
+			/*
+			 * Sanity check to ensure that idle call returns
+			 * with IRQs enabled
+			 */
+			WARN_ON(irqs_disabled());
 			start_critical_timings();
 
 			/* In many cases the interrupt that ended idle
drivers/acpi/processor_idle.c +28 -0
index f3decb3..c2ffd84 100644
@@ -224,6 +224,7 @@ static void lapic_timer_state_broadcast(struct acpi_processor *pr,
 /*
  * Suspend / resume control
  */
+static int acpi_idle_suspend;
 static u32 saved_bm_rld;
 
 static void acpi_idle_bm_rld_save(void)
@@ -242,13 +243,21 @@ 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;
 }
 
@@ -754,6 +763,12 @@ static int acpi_idle_enter_c1(struct cpuidle_device *dev,
 
 	local_irq_disable();
 
+	if (acpi_idle_suspend) {
+		local_irq_enable();
+		cpu_relax();
+		return -EINVAL;
+	}
+
 	lapic_timer_state_broadcast(pr, cx, 1);
 	kt1 = ktime_get_real();
 	acpi_idle_do_entry(cx);
@@ -823,6 +838,12 @@ static int acpi_idle_enter_simple(struct cpuidle_device *dev,
 
 	local_irq_disable();
 
+	if (acpi_idle_suspend) {
+		local_irq_enable();
+		cpu_relax();
+		return -EINVAL;
+	}
+
 	if (cx->entry_method != ACPI_CSTATE_FFH) {
 		current_thread_info()->status &= ~TS_POLLING;
 		/*
@@ -901,6 +922,13 @@ 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) {
 			return drv->states[drv->safe_state_index].enter(dev,
Quoted 5 lines
Regards,
Deepthi
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Rafael J. Wysocki

2012-06-16 13:31 UTC #6 lore raw

In reply to Deepthi Dharwar

Quoted 43 lines · Deepthi Dharwar

From: Deepthi Dharwar <[email protected]>
Fix suspend/resume regression caused by cpuidle cleanup.
Commit e978aa7d7d57d04eb5f88a7507c4fb98577def77 ( cpuidle: Move dev->last_residency update to driver enter routine; remove dev->last_state) was breaking suspend on laptops, as reported in the below link
- https://lkml.org/lkml/2011/11/11/164
This was fixed in commit 3439a8da16bcad6b0982ece938c9f8299bb53584 (ACPI / cpuidle: Remove acpi_idle_suspend (to fix suspend regression) by removing acpi_idle_suspend flag.
- https://lkml.org/lkml/2011/11/14/74
But this fix did not work on all systems
as Suspend/resume regression was reported on Lenovo S10-3 recently by Dave.
- https://lkml.org/lkml/2012/5/27/115
It looked like with commit e978aa7d broke suspend and with commit 3439a8da resume was not working with acpi_idle driver.
This patch fixes the regression that caused this issue in the first place. acpi_idle_suspend flag is essential on some x86 systems to prevent the cpus from going to deeper C-states when suspend is triggered ( commit b04e7bdb984 )
So reverting the commit 3439a8da is essential.
By default, irqs are disabled in cpu_idle arch specific call and re-enabled in idle state return path . As the acpi_idle_suspend flag was being set during suspend, which prevented the cpus going to deeper idle states, it is essential to
enabling the irqs in its return path too.
To address the suspend issue,
we were not re-enabling the interrupts while returning from acpi_idle_enter_bm() routine if acpi_idle_suspend flag is set. and this was causing suspend failure.
In addition to the above fix, a sanity check has also been added in x86 arch specific cpu_idle call to ensure that the idle call always returns with IRQs enabled.

Quoted 7 lines

This patch applies on 3.5-rc2
---
Reported-and-Tested-by Dav Hansen <[email protected]>
Tested-by Preeti Murthy <[email protected]>
Signed-off-by Deepthi Dharwar <[email protected]>
Reviewed-by Srivatsa S Bhat <[email protected]>

This appears to be an urgent regression fix.

Is anyone going to take it, or should I do that?

Rafael

Quoted 102 lines

---
 arch/x86/kernel/process.c     |    6 ++++++
 drivers/acpi/processor_idle.c |   28 ++++++++++++++++++++++++++++
 2 files changed, 34 insertions(+), 0 deletions(-)

arch/x86/kernel/process.c +6 -0

index 735279e..8ab76ad 100644
@@ -460,6 +460,12 @@ void cpu_idle(void)
 				pm_idle();
 
 			rcu_idle_exit();
+
+			/*
+			 * Sanity check to ensure that idle call returns
+			 * with IRQs enabled
+			 */
+			WARN_ON(irqs_disabled());
 			start_critical_timings();
 
 			/* In many cases the interrupt that ended idle

drivers/acpi/processor_idle.c +28 -0

index f3decb3..c2ffd84 100644
@@ -224,6 +224,7 @@ static void lapic_timer_state_broadcast(struct acpi_processor *pr,
 /*
  * Suspend / resume control
  */
+static int acpi_idle_suspend;
 static u32 saved_bm_rld;
 
 static void acpi_idle_bm_rld_save(void)

@@ -242,13 +243,21 @@ 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;
 }

@@ -754,6 +763,12 @@ static int acpi_idle_enter_c1(struct cpuidle_device *dev,
 
 	local_irq_disable();
 
+	if (acpi_idle_suspend) {
+		local_irq_enable();
+		cpu_relax();
+		return -EINVAL;
+	}
+
 	lapic_timer_state_broadcast(pr, cx, 1);
 	kt1 = ktime_get_real();
 	acpi_idle_do_entry(cx);

@@ -823,6 +838,12 @@ static int acpi_idle_enter_simple(struct cpuidle_device *dev,
 
 	local_irq_disable();
 
+	if (acpi_idle_suspend) {
+		local_irq_enable();
+		cpu_relax();
+		return -EINVAL;
+	}
+
 	if (cx->entry_method != ACPI_CSTATE_FFH) {
 		current_thread_info()->status &= ~TS_POLLING;
 		/*

@@ -901,6 +922,13 @@ 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) {
 			return drv->states[drv->safe_state_index].enter(dev,

Regards,
Deepthi

Rafael J. Wysocki

Applied

2012-06-19 20:29 UTC #7 lore raw

In reply to Deepthi Dharwar

Quoted 43 lines · Deepthi Dharwar

From: Deepthi Dharwar <[email protected]>
Fix suspend/resume regression caused by cpuidle cleanup.
Commit e978aa7d7d57d04eb5f88a7507c4fb98577def77 ( cpuidle: Move dev->last_residency update to driver enter routine; remove dev->last_state) was breaking suspend on laptops, as reported in the below link
- https://lkml.org/lkml/2011/11/11/164
This was fixed in commit 3439a8da16bcad6b0982ece938c9f8299bb53584 (ACPI / cpuidle: Remove acpi_idle_suspend (to fix suspend regression) by removing acpi_idle_suspend flag.
- https://lkml.org/lkml/2011/11/14/74
But this fix did not work on all systems
as Suspend/resume regression was reported on Lenovo S10-3 recently by Dave.
- https://lkml.org/lkml/2012/5/27/115
It looked like with commit e978aa7d broke suspend and with commit 3439a8da resume was not working with acpi_idle driver.
This patch fixes the regression that caused this issue in the first place. acpi_idle_suspend flag is essential on some x86 systems to prevent the cpus from going to deeper C-states when suspend is triggered ( commit b04e7bdb984 )
So reverting the commit 3439a8da is essential.
By default, irqs are disabled in cpu_idle arch specific call and re-enabled in idle state return path . As the acpi_idle_suspend flag was being set during suspend, which prevented the cpus going to deeper idle states, it is essential to
enabling the irqs in its return path too.
To address the suspend issue,
we were not re-enabling the interrupts while returning from acpi_idle_enter_bm() routine if acpi_idle_suspend flag is set. and this was causing suspend failure.
In addition to the above fix, a sanity check has also been added in x86 arch specific cpu_idle call to ensure that the idle call always returns with IRQs enabled.

Quoted 7 lines

This patch applies on 3.5-rc2
---
Reported-and-Tested-by Dav Hansen <[email protected]>
Tested-by Preeti Murthy <[email protected]>
Signed-off-by Deepthi Dharwar <[email protected]>
Reviewed-by Srivatsa S Bhat <[email protected]>

Applied to linux-pm/linux-next, will be pushed to Linus later this week.

Thanks,
Rafael

Quoted 102 lines

---
 arch/x86/kernel/process.c     |    6 ++++++
 drivers/acpi/processor_idle.c |   28 ++++++++++++++++++++++++++++
 2 files changed, 34 insertions(+), 0 deletions(-)

arch/x86/kernel/process.c +6 -0

index 735279e..8ab76ad 100644
@@ -460,6 +460,12 @@ void cpu_idle(void)
 				pm_idle();
 
 			rcu_idle_exit();
+
+			/*
+			 * Sanity check to ensure that idle call returns
+			 * with IRQs enabled
+			 */
+			WARN_ON(irqs_disabled());
 			start_critical_timings();
 
 			/* In many cases the interrupt that ended idle

drivers/acpi/processor_idle.c +28 -0

index f3decb3..c2ffd84 100644
@@ -224,6 +224,7 @@ static void lapic_timer_state_broadcast(struct acpi_processor *pr,
 /*
  * Suspend / resume control
  */
+static int acpi_idle_suspend;
 static u32 saved_bm_rld;
 
 static void acpi_idle_bm_rld_save(void)

@@ -242,13 +243,21 @@ 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;
 }

@@ -754,6 +763,12 @@ static int acpi_idle_enter_c1(struct cpuidle_device *dev,
 
 	local_irq_disable();
 
+	if (acpi_idle_suspend) {
+		local_irq_enable();
+		cpu_relax();
+		return -EINVAL;
+	}
+
 	lapic_timer_state_broadcast(pr, cx, 1);
 	kt1 = ktime_get_real();
 	acpi_idle_do_entry(cx);

@@ -823,6 +838,12 @@ static int acpi_idle_enter_simple(struct cpuidle_device *dev,
 
 	local_irq_disable();
 
+	if (acpi_idle_suspend) {
+		local_irq_enable();
+		cpu_relax();
+		return -EINVAL;
+	}
+
 	if (cx->entry_method != ACPI_CSTATE_FFH) {
 		current_thread_info()->status &= ~TS_POLLING;
 		/*

@@ -901,6 +922,13 @@ 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) {
 			return drv->states[drv->safe_state_index].enter(dev,

Regards,
Deepthi