Re: [PATCH v2 1/2] cpufreq/amd-pstate: Fix EPP initialization for shared memory systems

[Mario Limonciello]

On 6/3/26 06:56, Marco Scardovi wrote:
> At CPU boot/initialization, the private cpudata structure is allocated
> via kzalloc, which means cpudata->cppc_req_cached is initialized to 0.
> This makes the default cached EPP value 0 (AMD_CPPC_EPP_PERFORMANCE).
>
> When initializing a system that defaults to performance EPP, the driver
> attempts to configure the EPP via shmem_set_epp(policy, 0). Because the
> requested EPP (0) matches the uninitialized cached value (0), the early
> return check (epp == epp_cached) triggers, and the driver skips writing
> to the hardware.
>
> The cppc_set_epp_perf() helper is responsible for writing both the EPP
> register and the CPPC autonomous mode enable register (auto_sel). Skipping
> the EPP write on initialization due to the false cache hit consequently
> skips setting auto_sel to 1, leaving the CPU in non-autonomous mode. This
> prevents the hardware from boosting and leaves the CPU frequency stuck at
> the lowest non-linear frequency (1.7GHz).
>
> Fix this by introducing an `epp_hw_programmed` flag in struct amd_cpudata
> to track whether EPP has been configured on the hardware. Bypass the early
> return check in shmem_set_epp() if EPP has not yet been programmed on the
> hardware. Once EPP has been successfully programmed once, the cached EPP
> value becomes authoritative (representing driver intent rather than dynamic
> firmware state), allowing subsequent runtime mode transitions to use the
> cache guard optimization safely.
>
> Fixes: ffa5096a7c33 ("cpufreq: amd-pstate: implement Pstate EPP support for the AMD processors")
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=221473
> Assisted-by: Antigravity:gemini-3.5-flash
> Signed-off-by: Marco Scardovi <scardracs@xxxxxxxxxxx>
> ---
>   drivers/cpufreq/amd-pstate.c | 9 ++++++++-
>   drivers/cpufreq/amd-pstate.h | 2 ++
>   2 files changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index 8d55e2be825b..e8057f3dfb1e 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -424,7 +424,13 @@ static int shmem_set_epp(struct cpufreq_policy *policy, u8 epp)
>   					  epp != epp_cached);
>   	}
>   
> -	if (epp == epp_cached)
> +	/*
> +	 * After the first successful programming, the cached EPP value
> +	 * becomes authoritative (representing driver intent rather than
> +	 * dynamic firmware state), allowing us to skip redundant hardware
> +	 * writes when the EPP value is unchanged.
> +	 */
> +	if (cpudata->epp_hw_programmed && epp == epp_cached)
>   		return 0;
>   
>   	perf_ctrls.energy_perf = epp;
> @@ -434,6 +440,7 @@ static int shmem_set_epp(struct cpufreq_policy *policy, u8 epp)
>   		return ret;
>   	}
>   
> +	cpudata->epp_hw_programmed = true;
>   	value = READ_ONCE(cpudata->cppc_req_cached);
>   	FIELD_MODIFY(AMD_CPPC_EPP_PERF_MASK, &value, epp);
>   	WRITE_ONCE(cpudata->cppc_req_cached, value);
> diff --git a/drivers/cpufreq/amd-pstate.h b/drivers/cpufreq/amd-pstate.h
> index e4722e54387b..fc6cd4b873f6 100644
> --- a/drivers/cpufreq/amd-pstate.h
> +++ b/drivers/cpufreq/amd-pstate.h
> @@ -128,6 +128,8 @@ struct amd_cpudata {
>   	u8	epp_default_dc;
>   	bool	dynamic_epp;
>   	bool	raw_epp;
> +	/* Indicates that EPP has been successfully programmed at least once since boot. */
> +	bool	epp_hw_programmed;
>   	struct notifier_block power_nb;
>   
>   	/* platform profile */

I'm having a hard time following why this is needed.  Let me explain my 
chain of thought.

We start at amd_pstate_epp_cpu_init().
* cpudata (and thus cppc_req_cached) is initalized to 0 via kzalloc()
   - On a server we initialize "epp_default" via a lookup to the HW 
(amd_pstate_get_epp).
   - On a non-server we initialize "epp_default" to 0x80.
* we call amd_pstate_set_epp() with epp_deafult as the argument.

Now in the shared mem backend (shmem_set_epp) we do a lookup of 
epp_cached and it should be 0.  We do a comparison of the argument sent 
to the function, and this should be 0x80.

So how do we get into a case that amd_pstate_set_epp() is actually 
called with 0?

There are some trace points specifically for this purpose - you could 
confirm there really is such a call by using them at bootup (or by 
starting in passive and with a mode change to active at runtime).

Then we call amd_pstate_set_epp() with that epp_default value.