Re: [PATCH] cpufreq: intel_pstate: enable HWP before manipulating on corresponding registers

[Srinivas Pandruvada]

On Fri, 2018-01-26 at 14:35 +0800, Yu Chen wrote:
> On Thu, Jan 25, 2018 at 02:44:59PM -0800, Srinivas Pandruvada wrote:
> > 
> > On Thu, 2018-01-25 at 19:08 +0800, Yu Chen wrote:
> > 
> > Thanks for debugging.
> > 
> > > 
> > > The following warning was triggered after resumed from S3 -
> > > if all the nonboot CPUs were put offline before suspend:
> > > 
> > > [ 1840.329515] unchecked MSR access error: RDMSR from 0x771 at
> > > rIP:
> > > 0xffffffff86061e3a (native_read_msr+0xa/0x30)
> > [...]
> > 
> > [ 1840.329556]ÂÂacpi_processor_ppc_has_changed+0x65/0x80
> > 
> > This is the problem. You are getting a _PPC during resume which
> > needs
> > _PSS table to really do anything.
> > 
> OK.
> > 
> > So the correct fix should not in intel_pstate IMO but
> > 
> > diff --git a/drivers/acpi/processor_perflib.c
> > b/drivers/acpi/processor_perflib.c
> > index 18b72ee..c7cf48a 100644
> > --- a/drivers/acpi/processor_perflib.c
> > +++ b/drivers/acpi/processor_perflib.c
> > @@ -159,7 +159,7 @@ void acpi_processor_ppc_has_changed(struct
> > acpi_processor *pr, int event_flag)
> > Â{
> > ÂÂÂÂÂÂÂÂint ret;
> > Â
> > -ÂÂÂÂÂÂÂif (ignore_ppc) {
> > +ÂÂÂÂÂÂÂif (ignore_ppc || !pr->performance) {
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ/*
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ* Only when it is notification event, the _OST
> > object
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ* will be evaluated. Otherwise it is skipped.
> > 
> > 
> > ...
> > Since we don't callÂacpi_processor_register_performance(), the pr-
> > > 
> > > performance will be NULL. When this is NULL we don't need to do
> > > PPC
> > change notification.
> > Even if we register performance, processing a PPC notification is
> > complex as we have to wait for PPC=0 before enabling HWP otherwise
> > we
> > will be stuck with low performance (The event may not come once in
> > HWP
> > is in control).
> > 
> OK.
> > 
> > The important bug which you identified need a fix in resume when
> > maxcpus=1.
> > 
> > diff --git a/drivers/cpufreq/intel_pstate.c
> > b/drivers/cpufreq/intel_pstate.c
> > index 93a0e88..10e5efc 100644
> > --- a/drivers/cpufreq/intel_pstate.c
> > +++ b/drivers/cpufreq/intel_pstate.c
> > @@ -779,13 +779,16 @@ static int intel_pstate_hwp_save_state(struct
> > cpufreq_policy *policy)
> > ÂÂÂÂÂÂÂÂreturn 0;
> > Â}
> > Â
> > +static void intel_pstate_hwp_enable(struct cpudata *cpudata);
> > +
> > Âstatic int intel_pstate_resume(struct cpufreq_policy *policy)
> > Â{
> > ÂÂÂÂÂÂÂÂif (!hwp_active)
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn 0;
> > Â
> > ÂÂÂÂÂÂÂÂmutex_lock(&intel_pstate_limits_lock);
> > -
> > +ÂÂÂÂÂÂÂif (!policy->cpu)
> The 'if' statement might not be needed, as intel_pstate_resume()
> is always invoked on boot cpu IMO.

It will be invoked on all CPUs. Since we already do this for other CPUs
during cpu-online, this will avoid double calls to HWP enable.

Do these changes address your issues? If yes, you can submit two
patches.

Thanks,
Srinivas

> Thanks,
> Yu
> > 
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂintel_pstate_hwp_enable(all_cpu_data[policy->cpu]);
> > ÂÂÂÂÂÂÂÂall_cpu_data[policy->cpu]->epp_policy = 0;
> > ÂÂÂÂÂÂÂÂintel_pstate_hwp_set(policy->cpu);
> >