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

[preeti <preeti@xxxxxxxxxxxxxxxxxx>]

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@xxxxxxxxxxxxxxxxxx>
>>>> [...]
>>>>> 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