On 06/30/2012 03:41 AM, Rafael J. Wysocki wrote:

On Friday, June 29, 2012, preeti wrote:

quoted

On 06/29/2012 09:41 AM, preeti wrote:

quoted

On 06/29/2012 12:41 AM, Rafael J. Wysocki wrote:

quoted

On Thursday, June 28, 2012, preeti wrote:

quoted

On 06/28/2012 03:23 PM, Rafael J. Wysocki wrote:

quoted

On Thursday, June 28, 2012, preeti wrote:

quoted

From: Preeti U Murthy <redacted>

[...]

quoted

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.

quoted

quoted

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