[TEST PATCH] cpufreq: intel_pstate: Workaround to for wrong ACPI perf table entry

[TEST PATCH] cpufreq: intel_pstate: Workaround to for wrong ACPI perf table entry

[Srinivas Pandruvada]

With the implementation of ACPI _PSS and _PPC processing in the Intel P
state driver, a bad ACPI configuration can impact max/min states. For
example as reported by Borislav Petkov <bp@alien8.de>, the log shows:

[    0.826119] intel_pstate: default limits 0xc 0x1d 0x24
[    0.827000] intel_pstate: CPU0 - ACPI _PSS perf data
[    0.827020]      *P0: 2901 MHz, 35000 mW, 0xff00
The above control value of 0xff00, is invalid. The first entry sets the
max control value for turbo with a max non turbo frequency + 1 MHz. Here
the control values should be 0x1d00. intel_pstate_set_policy() depends on
this control value in setting correct max pstate.
Two fixes have been done here:
- If the control value is invalid then use the physical max turbo as the
max. Here 0xff00 will be changed to 0x1d00
- Always reset the limits->min_perf_ctl and limits->max_perf_ctl, so that
we will not use last value. In this case even if entry is not found the
_PSS it will fallback to limits->max_perf and limits-> limits->max_perf.

Reported-by: Borislav Petkov <bp@alien8.de>
Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 drivers/cpufreq/intel_pstate.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
index d3159f0..fc99e97 100644
--- a/drivers/cpufreq/intel_pstate.c
+++ b/drivers/cpufreq/intel_pstate.c
@@ -311,6 +311,13 @@ static int intel_pstate_init_perf_limits(struct cpufreq_policy *policy)
 		 * correct max turbo frequency based on the turbo ratio.
 		 * Also need to convert to MHz as _PSS freq is in MHz.
 		 */
+		if (turbo_pss_ctl > cpu->pstate.turbo_pstate) {
+			/* We hava an invalid control value here */
+			turbo_pss_ctl = cpu->pstate.turbo_pstate;
+			cpu->acpi_perf_data.states[0].control =
+							turbo_pss_ctl << 8;
+		}
+
 		cpu->acpi_perf_data.states[0].core_frequency =
 				turbo_pss_ctl * cpu->pstate.scaling / 1000;
 	}
@@ -1272,6 +1279,8 @@ static int intel_pstate_set_policy(struct cpufreq_policy *policy)
 
 #if IS_ENABLED(CONFIG_ACPI)
 	cpu = all_cpu_data[policy->cpu];
+	limits->min_perf_ctl = 0;
+	limits->max_perf_ctl = 0;
 	for (i = 0; i < cpu->acpi_perf_data.state_count; i++) {
 		int control;
 
-- 
2.4.3

Re: [TEST PATCH] cpufreq: intel_pstate: Workaround to for wrong ACPI perf table entry

[Srinivas Pandruvada]

Hi Borislav,

Sorry for the trouble. Please test the patch and let me know if it
fixes your issue.
In my description the value 0x1d00 should be 0x2400, I was simulating
on another system with max as 0x1d00.

Thanks,
Srinivas

On Tue, 2015-11-17 at 15:00 -0800, Srinivas Pandruvada wrote:
> With the implementation of ACPI _PSS and _PPC processing in the Intel
> P
> state driver, a bad ACPI configuration can impact max/min states. For
> example as reported by Borislav Petkov <bp@alien8.de>, the log shows:
> 
> [    0.826119] intel_pstate: default limits 0xc 0x1d 0x24
> [    0.827000] intel_pstate: CPU0 - ACPI _PSS perf data
> [    0.827020]      *P0: 2901 MHz, 35000 mW, 0xff00
> The above control value of 0xff00, is invalid. The first entry sets
> the
> max control value for turbo with a max non turbo frequency + 1 MHz.
> Here
> the control values should be 0x1d00. intel_pstate_set_policy()
> depends on
> this control value in setting correct max pstate.
> Two fixes have been done here:
> - If the control value is invalid then use the physical max turbo as
> the
> max. Here 0xff00 will be changed to 0x1d00
> - Always reset the limits->min_perf_ctl and limits->max_perf_ctl, so
> that
> we will not use last value. In this case even if entry is not found
> the
> _PSS it will fallback to limits->max_perf and limits-> limits
> ->max_perf.
> 
> Reported-by: Borislav Petkov <bp@alien8.de>
> Signed-off-by: Srinivas Pandruvada <
> srinivas.pandruvada@linux.intel.com>
> ---
>  drivers/cpufreq/intel_pstate.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/cpufreq/intel_pstate.c
> b/drivers/cpufreq/intel_pstate.c
> index d3159f0..fc99e97 100644
> --- a/drivers/cpufreq/intel_pstate.c
> +++ b/drivers/cpufreq/intel_pstate.c
> @@ -311,6 +311,13 @@ static int intel_pstate_init_perf_limits(struct
> cpufreq_policy *policy)
>  		 * correct max turbo frequency based on the turbo
> ratio.
>  		 * Also need to convert to MHz as _PSS freq is in
> MHz.
>  		 */
> +		if (turbo_pss_ctl > cpu->pstate.turbo_pstate) {
> +			/* We hava an invalid control value here */
> +			turbo_pss_ctl = cpu->pstate.turbo_pstate;
> +			cpu->acpi_perf_data.states[0].control =
> +							turbo_pss_ct
> l << 8;
> +		}
> +
>  		cpu->acpi_perf_data.states[0].core_frequency =
>  				turbo_pss_ctl * cpu->pstate.scaling
> / 1000;
>  	}
> @@ -1272,6 +1279,8 @@ static int intel_pstate_set_policy(struct
> cpufreq_policy *policy)
>  
>  #if IS_ENABLED(CONFIG_ACPI)
>  	cpu = all_cpu_data[policy->cpu];
> +	limits->min_perf_ctl = 0;
> +	limits->max_perf_ctl = 0;
>  	for (i = 0; i < cpu->acpi_perf_data.state_count; i++) {
>  		int control;
>  

Re: [TEST PATCH] cpufreq: intel_pstate: Workaround to for wrong ACPI perf table entry

[Rafael J. Wysocki]

On Tuesday, November 17, 2015 03:00:52 PM Srinivas Pandruvada wrote:
> With the implementation of ACPI _PSS and _PPC processing in the Intel P
> state driver, a bad ACPI configuration can impact max/min states. For
> example as reported by Borislav Petkov <bp@alien8.de>, the log shows:
> 
> [    0.826119] intel_pstate: default limits 0xc 0x1d 0x24
> [    0.827000] intel_pstate: CPU0 - ACPI _PSS perf data
> [    0.827020]      *P0: 2901 MHz, 35000 mW, 0xff00
> The above control value of 0xff00, is invalid. The first entry sets the
> max control value for turbo with a max non turbo frequency + 1 MHz. Here
> the control values should be 0x1d00.

I guess "the control value should not be greater than the one reported by the
CPU itself".

> intel_pstate_set_policy() depends on this control value in setting correct
> max pstate.

This looks sort of fragile to me.

> Two fixes have been done here:
> - If the control value is invalid then use the physical max turbo as the
> max. Here 0xff00 will be changed to 0x1d00

It will be changed to whatever is reported by the CPU I suppose.

> - Always reset the limits->min_perf_ctl and limits->max_perf_ctl, so that
> we will not use last value. In this case even if entry is not found the
> _PSS it will fallback to limits->max_perf and limits-> limits->max_perf.
> 
> Reported-by: Borislav Petkov <bp@alien8.de>
> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> ---
>  drivers/cpufreq/intel_pstate.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
> index d3159f0..fc99e97 100644
> --- a/drivers/cpufreq/intel_pstate.c
> +++ b/drivers/cpufreq/intel_pstate.c
> @@ -311,6 +311,13 @@ static int intel_pstate_init_perf_limits(struct cpufreq_policy *policy)
>  		 * correct max turbo frequency based on the turbo ratio.
>  		 * Also need to convert to MHz as _PSS freq is in MHz.
>  		 */
> +		if (turbo_pss_ctl > cpu->pstate.turbo_pstate) {
> +			/* We hava an invalid control value here */
> +			turbo_pss_ctl = cpu->pstate.turbo_pstate;
> +			cpu->acpi_perf_data.states[0].control =
> +							turbo_pss_ctl << 8;
> +		}

Should we update pstate.turbo_pstate otherwise?

> +
>  		cpu->acpi_perf_data.states[0].core_frequency =
>  				turbo_pss_ctl * cpu->pstate.scaling / 1000;
>  	}

We seem to have one more bug in this function, but it doesn't affect the case
at hand.  Namely, if the turbo range is not present in the _PSS, we should
set pstate.turbo_pstate to pstate.max_pstate after we've updated the latter
and not before updating it.

I'm also wondering if turbo should be enabled when turbo_pass_ctl is less than
pstate.min_state.  Perhaps not?

> @@ -1272,6 +1279,8 @@ static int intel_pstate_set_policy(struct cpufreq_policy *policy)
>  
>  #if IS_ENABLED(CONFIG_ACPI)
>  	cpu = all_cpu_data[policy->cpu];
> +	limits->min_perf_ctl = 0;
> +	limits->max_perf_ctl = 0;

Wouldn't this re-introduce the problem fixed by commit 4ef451487019
(cpufreq: intel_pstate: Avoid calculation for max/min) in corner cases?

>  	for (i = 0; i < cpu->acpi_perf_data.state_count; i++) {
>  		int control;

That whole loop is fragile.

To me, it should just pick the first state that is not greater than policy->max
and the last one that is not less than policy->min.

Thanks,
Rafael

Re: [TEST PATCH] cpufreq: intel_pstate: Workaround to for wrong ACPI perf table entry

[Srinivas Pandruvada]

On Wed, 2015-11-18 at 02:33 +0100, Rafael J. Wysocki wrote:
> On Tuesday, November 17, 2015 03:00:52 PM Srinivas Pandruvada wrote:
> > With the implementation of ACPI _PSS and _PPC processing in the Intel P
> > state driver, a bad ACPI configuration can impact max/min states. For
> > example as reported by Borislav Petkov <bp@alien8.de>, the log shows:
> > 
> > [    0.826119] intel_pstate: default limits 0xc 0x1d 0x24
> > [    0.827000] intel_pstate: CPU0 - ACPI _PSS perf data
> > [    0.827020]      *P0: 2901 MHz, 35000 mW, 0xff00
> > The above control value of 0xff00, is invalid. The first entry sets the
> > max control value for turbo with a max non turbo frequency + 1 MHz. Here
> > the control values should be 0x1d00.
> 
> I guess "the control value should not be greater than the one reported by the
> CPU itself".
Yes.
> 
> > intel_pstate_set_policy() depends on this control value in setting correct
> > max pstate.
> 
> This looks sort of fragile to me.
> 
> > Two fixes have been done here:
> > - If the control value is invalid then use the physical max turbo as the
> > max. Here 0xff00 will be changed to 0x1d00
> 
> It will be changed to whatever is reported by the CPU I suppose.
> 
> > - Always reset the limits->min_perf_ctl and limits->max_perf_ctl, so that
> > we will not use last value. In this case even if entry is not found the
> > _PSS it will fallback to limits->max_perf and limits-> limits->max_perf.
> > 
> > Reported-by: Borislav Petkov <bp@alien8.de>
> > Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> > ---
> >  drivers/cpufreq/intel_pstate.c | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
> > index d3159f0..fc99e97 100644
> > --- a/drivers/cpufreq/intel_pstate.c
> > +++ b/drivers/cpufreq/intel_pstate.c
> > @@ -311,6 +311,13 @@ static int intel_pstate_init_perf_limits(struct cpufreq_policy *policy)
> >  		 * correct max turbo frequency based on the turbo ratio.
> >  		 * Also need to convert to MHz as _PSS freq is in MHz.
> >  		 */
> > +		if (turbo_pss_ctl > cpu->pstate.turbo_pstate) {
> > +			/* We hava an invalid control value here */
> > +			turbo_pss_ctl = cpu->pstate.turbo_pstate;
> > +			cpu->acpi_perf_data.states[0].control =
> > +							turbo_pss_ctl << 8;
> > +		}
> 
> Should we update pstate.turbo_pstate otherwise?
It will happen as the policy will reduce the frequency based on the
_PSS[PPC_INDEX] frequency. So if turbo_pstate is less then the
intel_pstate_set_policy will be called with reduced max->policy.
> 
> > +
> >  		cpu->acpi_perf_data.states[0].core_frequency =
> >  				turbo_pss_ctl * cpu->pstate.scaling / 1000;
> >  	}
> 
> We seem to have one more bug in this function, but it doesn't affect the case
> at hand.  Namely, if the turbo range is not present in the _PSS, we should
> set pstate.turbo_pstate to pstate.max_pstate after we've updated the latter
> and not before updating it.
Doing it before we have good known reference for max_sysf_pct. which is
either physical max turbo or physical max non turbo as 100%. The policy
will limit to actual pss max pstate frequency. which will be reflected
in reduced max_perf_pct.

> 
> I'm also wondering if turbo should be enabled when turbo_pass_ctl is less than
> pstate.min_state.  Perhaps not?
> 
This is too bad entry in that case. But we can disable the turbo.
> > @@ -1272,6 +1279,8 @@ static int intel_pstate_set_policy(struct cpufreq_policy *policy)
> >  
> >  #if IS_ENABLED(CONFIG_ACPI)
> >  	cpu = all_cpu_data[policy->cpu];
> > +	limits->min_perf_ctl = 0;
> > +	limits->max_perf_ctl = 0;
> 
> Wouldn't this re-introduce the problem fixed by commit 4ef451487019
> (cpufreq: intel_pstate: Avoid calculation for max/min) in corner cases?
If there is a match in _PSS, it will be correctly set in loop below. In
the current problem case we failed to match the _PSS for policy->max so
we were relying on the max_perf calculation. But later some thermal
issue happened, which reduced the policy to minimum. We stuck in that
because limits->max_perf_ctl was not reset even after when policy
changed to turbo max. With reset we should have used max pstate
calculated value, which will not be as bad as last value.
> 
> >  	for (i = 0; i < cpu->acpi_perf_data.state_count; i++) {
> >  		int control;
> 
> That whole loop is fragile.
Could you suggest or submit a change for this?  Trying to do get max and
min in one for-loop assuming not sorted.
With the reset of limits->[max|min]_perf_ctl, we can do 
	for (i = 0; i < cpu->acpi_perf_data.state_count; i++) {
		int control;

		control = convert_to_native_pstate_format(cpu, i);
		if (!limits->max_perf_ctl && control * cpu->pstate.scaling ==
policy->max)
			limits->max_perf_ctl = control;
		if (!limits->min_perf_ctl && control * cpu->pstate.scaling ==
policy->min)
			limits->min_perf_ctl = control;
	}


Thanks,
Srinivas
> To me, it should just pick the first state that is not greater than policy->max
> and the last one that is not less than policy->min.
> 
> Thanks,
> Rafael
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [TEST PATCH] cpufreq: intel_pstate: Workaround to for wrong ACPI perf table entry

[Rafael J. Wysocki]

On Wed, Nov 18, 2015 at 3:10 AM, Srinivas Pandruvada
<srinivas.pandruvada@linux.intel.com> wrote:
> On Wed, 2015-11-18 at 02:33 +0100, Rafael J. Wysocki wrote:
>> On Tuesday, November 17, 2015 03:00:52 PM Srinivas Pandruvada wrote:
>> > With the implementation of ACPI _PSS and _PPC processing in the Intel P
>> > state driver, a bad ACPI configuration can impact max/min states. For
>> > example as reported by Borislav Petkov <bp@alien8.de>, the log shows:
>> >
>> > [    0.826119] intel_pstate: default limits 0xc 0x1d 0x24
>> > [    0.827000] intel_pstate: CPU0 - ACPI _PSS perf data
>> > [    0.827020]      *P0: 2901 MHz, 35000 mW, 0xff00
>> > The above control value of 0xff00, is invalid. The first entry sets the
>> > max control value for turbo with a max non turbo frequency + 1 MHz. Here
>> > the control values should be 0x1d00.

[cut]

>> > diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
>> > index d3159f0..fc99e97 100644
>> > --- a/drivers/cpufreq/intel_pstate.c
>> > +++ b/drivers/cpufreq/intel_pstate.c
>> > @@ -311,6 +311,13 @@ static int intel_pstate_init_perf_limits(struct cpufreq_policy *policy)
>> >              * correct max turbo frequency based on the turbo ratio.
>> >              * Also need to convert to MHz as _PSS freq is in MHz.
>> >              */
>> > +           if (turbo_pss_ctl > cpu->pstate.turbo_pstate) {
>> > +                   /* We hava an invalid control value here */
>> > +                   turbo_pss_ctl = cpu->pstate.turbo_pstate;
>> > +                   cpu->acpi_perf_data.states[0].control =
>> > +                                                   turbo_pss_ctl << 8;
>> > +           }
>>
>> Should we update pstate.turbo_pstate otherwise?
>
> It will happen as the policy will reduce the frequency based on the
> _PSS[PPC_INDEX] frequency. So if turbo_pstate is less then the
> intel_pstate_set_policy will be called with reduced max->policy.
>

Well, OK, but here we're leaving policy->cpuinfo.max_freq and
acpi_perf_data.states[0].core_frequency somewhat inconsistent which at
least looks confusing.

>>
>> > +
>> >             cpu->acpi_perf_data.states[0].core_frequency =
>> >                             turbo_pss_ctl * cpu->pstate.scaling / 1000;
>> >     }
>>
>> We seem to have one more bug in this function, but it doesn't affect the case
>> at hand.  Namely, if the turbo range is not present in the _PSS, we should
>> set pstate.turbo_pstate to pstate.max_pstate after we've updated the latter
>> and not before updating it.
>
> Doing it before we have good known reference for max_sysf_pct. which is
> either physical max turbo or physical max non turbo as 100%. The policy
> will limit to actual pss max pstate frequency. which will be reflected
> in reduced max_perf_pct.
>

So say we have the "non-turbo" situation, so we set the
pstate.turbo_pstate to pstate.max_pstate and then we update
pstate.max_pstate.  policy->cpuinfo.max_freq is updated to reflect the
new max_pstate and turbo_pstate points to physical max non turbo which
is above max_pstate.  Why is this correct?  What about
update_turbo_state(), for example?

And now we're claiming "no turbo", but we're presenting the "non-PSS
max_pstate" as an "effective turbo" and the max_pstate from _PSS is
the new limit.  How is this not confusing?

>>
>> I'm also wondering if turbo should be enabled when turbo_pass_ctl is less than
>> pstate.min_state.  Perhaps not?
>>
> This is too bad entry in that case. But we can disable the turbo.
>> > @@ -1272,6 +1279,8 @@ static int intel_pstate_set_policy(struct cpufreq_policy *policy)
>> >
>> >  #if IS_ENABLED(CONFIG_ACPI)
>> >     cpu = all_cpu_data[policy->cpu];
>> > +   limits->min_perf_ctl = 0;
>> > +   limits->max_perf_ctl = 0;
>>
>> Wouldn't this re-introduce the problem fixed by commit 4ef451487019
>> (cpufreq: intel_pstate: Avoid calculation for max/min) in corner cases?
>
> If there is a match in _PSS, it will be correctly set in loop below.

Sure, but what if there isn't?  We'll end up with 0 and that may not
work (or the commit mentioned above was not necessary).

> In the current problem case we failed to match the _PSS for policy->max so
> we were relying on the max_perf calculation. But later some thermal
> issue happened, which reduced the policy to minimum. We stuck in that
> because limits->max_perf_ctl was not reset even after when policy
> changed to turbo max. With reset we should have used max pstate
> calculated value, which will not be as bad as last value.
>>
>> >     for (i = 0; i < cpu->acpi_perf_data.state_count; i++) {
>> >             int control;
>>
>> That whole loop is fragile.
> Could you suggest or submit a change for this?  Trying to do get max and
> min in one for-loop assuming not sorted.

I'll try to cut a patch for that tomorrow.

Thanks,
Rafael

Re: [TEST PATCH] cpufreq: intel_pstate: Workaround to for wrong ACPI perf table entry

[Borislav Petkov]

On Wed, Nov 18, 2015 at 04:19:40AM +0100, Rafael J. Wysocki wrote:
> I'll try to cut a patch for that tomorrow.

And I'm going to wait out a bit before testing, after you guys have
agreed on the final version.

Thanks.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

Re: [TEST PATCH] cpufreq: intel_pstate: Workaround to for wrong ACPI perf table entry

[Srinivas Pandruvada]

On 11/17/2015 07:19 PM, Rafael J. Wysocki wrote:
> On Wed, Nov 18, 2015 at 3:10 AM, Srinivas Pandruvada
> <srinivas.pandruvada@linux.intel.com> wrote:
>> On Wed, 2015-11-18 at 02:33 +0100, Rafael J. Wysocki wrote:
>>> On Tuesday, November 17, 2015 03:00:52 PM Srinivas Pandruvada wrote:
>>>> With the implementation of ACPI _PSS and _PPC processing in the Intel P
>>>> state driver, a bad ACPI configuration can impact max/min states. For
>>>> example as reported by Borislav Petkov <bp@alien8.de>, the log shows:
>>>>
>>>> [    0.826119] intel_pstate: default limits 0xc 0x1d 0x24
>>>> [    0.827000] intel_pstate: CPU0 - ACPI _PSS perf data
>>>> [    0.827020]      *P0: 2901 MHz, 35000 mW, 0xff00
>>>> The above control value of 0xff00, is invalid. The first entry sets the
>>>> max control value for turbo with a max non turbo frequency + 1 MHz. Here
>>>> the control values should be 0x1d00.
> [cut]
>
>>>> diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
>>>> index d3159f0..fc99e97 100644
>>>> --- a/drivers/cpufreq/intel_pstate.c
>>>> +++ b/drivers/cpufreq/intel_pstate.c
>>>> @@ -311,6 +311,13 @@ static int intel_pstate_init_perf_limits(struct cpufreq_policy *policy)
>>>>               * correct max turbo frequency based on the turbo ratio.
>>>>               * Also need to convert to MHz as _PSS freq is in MHz.
>>>>               */
>>>> +           if (turbo_pss_ctl > cpu->pstate.turbo_pstate) {
>>>> +                   /* We hava an invalid control value here */
>>>> +                   turbo_pss_ctl = cpu->pstate.turbo_pstate;
>>>> +                   cpu->acpi_perf_data.states[0].control =
>>>> +                                                   turbo_pss_ctl << 8;
>>>> +           }
>>> Should we update pstate.turbo_pstate otherwise?
>> It will happen as the policy will reduce the frequency based on the
>> _PSS[PPC_INDEX] frequency. So if turbo_pstate is less then the
>> intel_pstate_set_policy will be called with reduced max->policy.
>>
> Well, OK, but here we're leaving policy->cpuinfo.max_freq and
> acpi_perf_data.states[0].core_frequency somewhat inconsistent which at
> least looks confusing.
OK I will do what you want here.
>>>> +
>>>>              cpu->acpi_perf_data.states[0].core_frequency =
>>>>                              turbo_pss_ctl * cpu->pstate.scaling / 1000;
>>>>      }
>>> We seem to have one more bug in this function, but it doesn't affect the case
>>> at hand.  Namely, if the turbo range is not present in the _PSS, we should
>>> set pstate.turbo_pstate to pstate.max_pstate after we've updated the latter
>>> and not before updating it.
>> Doing it before we have good known reference for max_sysf_pct. which is
>> either physical max turbo or physical max non turbo as 100%. The policy
>> will limit to actual pss max pstate frequency. which will be reflected
>> in reduced max_perf_pct.
>>
> So say we have the "non-turbo" situation, so we set the
> pstate.turbo_pstate to pstate.max_pstate and then we update
> pstate.max_pstate.  policy->cpuinfo.max_freq is updated to reflect the
> new max_pstate and turbo_pstate points to physical max non turbo which
> is above max_pstate.  Why is this correct?  What about
> update_turbo_state(), for example?
>
> And now we're claiming "no turbo", but we're presenting the "non-PSS
> max_pstate" as an "effective turbo" and the max_pstate from _PSS is
> the new limit.  How is this not confusing?
OK I will do what you wanted here.
>>> I'm also wondering if turbo should be enabled when turbo_pass_ctl is less than
>>> pstate.min_state.  Perhaps not?
>>>
>> This is too bad entry in that case. But we can disable the turbo.
>>>> @@ -1272,6 +1279,8 @@ static int intel_pstate_set_policy(struct cpufreq_policy *policy)
>>>>
>>>>   #if IS_ENABLED(CONFIG_ACPI)
>>>>      cpu = all_cpu_data[policy->cpu];
>>>> +   limits->min_perf_ctl = 0;
>>>> +   limits->max_perf_ctl = 0;
>>> Wouldn't this re-introduce the problem fixed by commit 4ef451487019
>>> (cpufreq: intel_pstate: Avoid calculation for max/min) in corner cases?
>> If there is a match in _PSS, it will be correctly set in loop below.
> Sure, but what if there isn't?  We'll end up with 0 and that may not
> work (or the commit mentioned above was not necessary).
No, 0 is fine. If 0 then the old limit based on the max_perf calculation 
will work.
Since there will be no match in _PSS we have no other way. Setting not 
to 0, causes the old
limits->max_perf_ctl to get used.
Let's see what happened in the case now:

Here we tried to match 0xff in the _PSS and failed. From Borslov's log

[   28.587413] intel_pstate: intel_pstate_set_policy max 3600000 policy->max 3600000
[   28.587417] intel_pstate: set powersave
[   28.587419] intel_pstate: max 3600000 policy_max 3600000 perf_ctl match [min 0xc- max 0x0] [EDITED to add min/max tag]

Looks like now some thermal event happened, setting to Pn

[   28.589573] intel_pstate: intel_pstate_set_policy max 3600000 policy->max 1200000
[   28.589574] intel_pstate: set powersave
[   28.589575] intel_pstate: max 3600000 policy_max 1200000 perf_ctl match  [min 0xc- max 0xc] [EDITED to add min/max tag]
We matched 1.2GHz and we correctly identified min and max ctl value from _PSS.

Now we back to normal.
[   28.589599] intel_pstate: intel_pstate_set_policy max 3600000 policy->max 3600000
[   28.589600] intel_pstate: set powersave
[   28.589601] intel_pstate: max 3600000 policy_max 3600000 perf_ctl [0xc-0xc]
We still carried the old value, so we were stuck in 1.2GHz.


So resetting 0 would have caused no match again
[   28.587419] intel_pstate: max 3600000 policy_max 3600000 perf_ctl match [min 0xc- max 0x0] [EDITED]
and would have worked with logic.

The commit only helps when there is match in the _PSS table to get max control value. If there is no match we have to live with rounding error in some corner cases. Prareet from Redhat  is submitted a patch which reduces this rounding error.
With Intel P state cpufreq, we allow any frequencies to set (we don't have available_scaling.. attribute). So it is possible not to match _PSS in all the cases. For Borislov's system there is no 2100 entry in _PSS, but user can still request 2100.

>> In the current problem case we failed to match the _PSS for policy->max so
>> we were relying on the max_perf calculation. But later some thermal
>> issue happened, which reduced the policy to minimum. We stuck in that
>> because limits->max_perf_ctl was not reset even after when policy
>> changed to turbo max. With reset we should have used max pstate
>> calculated value, which will not be as bad as last value.
>>>>      for (i = 0; i < cpu->acpi_perf_data.state_count; i++) {
>>>>              int control;
>>> That whole loop is fragile.
>> Could you suggest or submit a change for this?  Trying to do get max and
>> min in one for-loop assuming not sorted.
> I'll try to cut a patch for that tomorrow.
Sure.

Thanks,
Srinivas
> Thanks,
> Rafael
>

Re: [TEST PATCH] cpufreq: intel_pstate: Workaround to for wrong ACPI perf table entry

[Borislav Petkov]

On Wed, Nov 18, 2015 at 08:32:58AM -0800, Srinivas Pandruvada wrote:
> Here we tried to match 0xff in the _PSS and failed. From Borslov's log

That is most likely me calling

powertop --auto-tune

...

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.