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

["Rafael J. Wysocki" <rjw@rjwysocki.net>]

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