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

["Rafael J. Wysocki" <rjw@sisk.pl>]

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