Re: [PATCH] cpufreq: amd-pstate: avoid uninitialized variable use

[Wyes Karny]

Hi Arnd, Ray, Perry,

On 07 Feb 17:12, Arnd Bergmann wrote:
> @@ -1076,9 +1075,9 @@ static void amd_pstate_epp_init(unsigned int cpu)
>  		value |= (u64)epp << 24;
>  	}
>  
> +	amd_pstate_set_epp(cpudata, epp);
>  skip_epp:
>  	WRITE_ONCE(cpudata->cppc_req_cached, value);
> -	amd_pstate_set_epp(cpudata, epp);
>  	cpufreq_cpu_put(policy);
>  }

I see an issue here with MSR-based systems.
Here EPP is being set before updating `cppc_req_cached` and for MSR
based systems while updating EPP value, MIN_PERF and MAX_PERF also
updated. So, here whatever value MSR_AMD_CPPC_REQ had for min and max
perf that will be rewritten instead of highest and lowest perf from
MSR_AMD_CPPC_CAP1.

Can we do something like this:

@@ -1053,32 +1052,34 @@ static void amd_pstate_epp_init(unsigned int
cpu)
        value &= ~AMD_CPPC_DES_PERF(~0L);
        value |= AMD_CPPC_DES_PERF(0);

+       /* No need to set epp again if previous and current policy is same */
        if (cpudata->epp_policy == cpudata->policy)
                goto skip_epp;

        cpudata->epp_policy = cpudata->policy;

-       if (cpudata->policy == CPUFREQ_POLICY_PERFORMANCE) {
-               epp = amd_pstate_get_epp(cpudata, value);
-               if (epp < 0)
-                       goto skip_epp;
-               /* force the epp value to be zero for performance policy */
-               epp = 0;
-       } else {
-               /* Get BIOS pre-defined epp value */
-               epp = amd_pstate_get_epp(cpudata, value);
-               if (epp)
-                       goto skip_epp;
+       /* Get BIOS pre-defined epp value */
+       epp = amd_pstate_get_epp(cpudata, value);
+       if (epp < 0) {
+               /**
+                * This return value can only be negative for shared_memory
+                * systems where EPP register read/write not supported.
+                */
+               goto skip_epp;
        }
+
+       if (cpudata->policy == CPUFREQ_POLICY_PERFORMANCE)
+               epp = 0;
+
        /* Set initial EPP value */
        if (boot_cpu_has(X86_FEATURE_CPPC)) {
                value &= ~GENMASK_ULL(31, 24);
                value |= (u64)epp << 24;
        }

-skip_epp:
        WRITE_ONCE(cpudata->cppc_req_cached, value);
        amd_pstate_set_epp(cpudata, epp);
+skip_epp:
        cpufreq_cpu_put(policy);
 }


>  
> -- 
> 2.39.1
>