Re: [RFC/PATCH] PM / Sleep: Timer quiesce in freeze state

[Peter Zijlstra]

On Wed, Oct 29, 2014 at 06:46:03AM +0800, Li, Aubrey wrote:
> On 2014/10/28 16:29, Peter Zijlstra wrote:
> > On Tue, Oct 28, 2014 at 12:32:16PM +0800, Li, Aubrey wrote:
> >> On 2014/10/27 15:28, Peter Zijlstra wrote:
> >>> On Mon, Oct 27, 2014 at 02:27:27PM +0800, Li, Aubrey wrote:
> >>>>> Now I suppose the problem is with cpu_pause() which needs IPIs to
> >>>>> complete? Do we really need cpuidle_pause() there?
> >>>>> cpuidle_uninstall_handlers() seems like a daft thing to call just about
> >>>>> there.
> >>>>
> >>>> Please check the log of 8651f97bd951d0bb1c10fa24e3fa3455193f3548.
> >>>> Rafael should know more this question than me.
> >>>
> >>> That changelog explains its complete bollocks to do it here. We _want_
> >>> to enter and/or remain in deep idle states.
> >>
> >> cpuidle_resume() will be called at the end of dpm_resume_noirq(). So we
> >> still are able to enter deep idle states after resume.
> > 
> > cpuidle_resume is absolute crap, as is cpuidle_suspend for that matter
> > -- in this case.
> > 
> > The only reason we needed cpuidle_suspend is because some BIOS shat its
> > pants when some CPUs were in higher C states while trying to do the S3
> > thing. We're not going to use S states or BIOS calls _at_all_, so no
> > point in kicking CPUs out of their deep C states.
> 
> We already kicked CPUs out of their deep C states in dpm_suspend_noirq().
> 
> We pause cpuidle in dpm_suspend_noirq() and resume cpuidle in
> dpm_resume_noirq(), so currently we can't enter deep c-state during S
> states. That's an intention for some buggy BIOS.

And work arounds for buggy crap hardware should not be in generic code.
They should be in the platform drivers associated with said crap bugs.

But I think I see what you're saying, we're going through this dpm_ crap
even for suspend to idle, which is wrong too.

> However, for freeze state, there is another intention that we want
> always to enter the *deepest* c-state every time we enter freeze.
> So we need cpuidle_resume() to make sure we have deep cstate  in freeze.
> 
> So back to your question in another email,
> 
> > I think you can simply remove them altogether, they're nonsense.
> 
> We need them to resume cpuidle in freeze.

So you can do cpuidle_resume() before we do the stop machine dance, but
ideally it'd all be ripped out from generic code and stuffed into
the platform drivers where it belongs. But at the very least it should
be isolated to the S3 path, I bet suspend to disk doesn't care either.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/