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

[Mario Limonciello]

On 6/7/26 23:21, K Prateek Nayak wrote:
> Hello Marco,
>
> I don't mind the changes, except that it can be broken down into
> two patches as Mario suggested for easier backporting :-)

Totally agree. All the technical stuff I'm happy with too, just split it up.

> Some notes below ...
>
> On 6/6/2026 12:27 PM, Marco Scardovi wrote:
> > diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> > index 8d55e2be825b..a35cf126e335 100644
> > --- a/drivers/cpufreq/amd-pstate.c
> > +++ b/drivers/cpufreq/amd-pstate.c
> > @@ -199,7 +199,7 @@ static inline int get_mode_idx_from_str(const char *str, size_t size)
> >   
> >   static DEFINE_MUTEX(amd_pstate_driver_lock);
> >   
> > -static u8 msr_get_epp(struct amd_cpudata *cpudata)
> > +static int msr_get_epp(struct amd_cpudata *cpudata)
> >   {
> >   	u64 value;
> >   	int ret;
> > @@ -215,12 +215,12 @@ static u8 msr_get_epp(struct amd_cpudata *cpudata)
> >   
> >   DEFINE_STATIC_CALL(amd_pstate_get_epp, msr_get_epp);
> >   
> > -static inline s16 amd_pstate_get_epp(struct amd_cpudata *cpudata)
> > +static inline int amd_pstate_get_epp(struct amd_cpudata *cpudata)
>
> The change to return type and the amd_pstate_epp_cpu_init() bits
> should be Patch 1/2 with:
>
> Fixes: 555bbe67a622 ("cpufreq/amd-pstate: Convert all perf values to u8")
>
> and commit message stating the necessary error handling that is
> required if amd_pstate_get_epp() fails.
>
> >   {
> >   	return static_call(amd_pstate_get_epp)(cpudata);
> >   }
> >   
> > -static u8 shmem_get_epp(struct amd_cpudata *cpudata)
> > +static int shmem_get_epp(struct amd_cpudata *cpudata)
> >   {
> >   	u64 epp;
> >   	int ret;
> > @@ -525,10 +525,6 @@ static int shmem_init_perf(struct amd_cpudata *cpudata)
> >   	perf.lowest_perf = cppc_perf.lowest_perf;
> >   	WRITE_ONCE(cpudata->perf, perf);
> >   	WRITE_ONCE(cpudata->prefcore_ranking, cppc_perf.highest_perf);
> > -
> > -	if (cppc_state == AMD_PSTATE_ACTIVE)
> > -		return 0;
> > -
>
> And this should be Patch 2/2 with:
>
> Fixes: 2dd6d0ebf740 ("cpufreq: amd-pstate: Add guided autonomous mode")
>
> and a commit message that reads it is necessary for AMD_PSTATE_ACTIVE mode
> on shared memory system to toggle on AUTO_SEL_ENABLE based on the
> ACPI spec for CPPC v2 and below.
>
> >   	ret = cppc_get_auto_sel(cpudata->cpu, &auto_sel);
> >   	if (ret) {
> >   		pr_warn("failed to get auto_sel, ret: %d\n", ret);
> > @@ -1877,6 +1873,7 @@ static int amd_pstate_epp_cpu_init(struct cpufreq_policy *policy)
> >   	struct amd_cpudata *cpudata;
> >   	union perf_cached perf;
> >   	struct device *dev;
> > +	int default_epp;
> >   	int ret;
> >   
> >   	/*
> > @@ -1926,6 +1923,14 @@ static int amd_pstate_epp_cpu_init(struct cpufreq_policy *policy)
> >   
> >   	policy->boost_supported = READ_ONCE(cpudata->boost_supported);
> >   
> > +	/* Cache the firmware programmed EPP */
> > +	default_epp = amd_pstate_get_epp(cpudata);
> > +	if (default_epp < 0) {
> > +		ret = default_epp;
> > +		goto free_cpudata1;
> > +	}
> > +	FIELD_MODIFY(AMD_CPPC_EPP_PERF_MASK, &cpudata->cppc_req_cached, default_epp);
>
> I would actually prefer this "cppc_req_cached" change to be a third
> patch that says caching the initial EPP saves on an unnecessary
> reprogramming later when the EPP is first set but I don't mind it
> being part of Patch 1 with a small note in the commit message.
>
> > +
> >   	/*
> >   	 * Set the policy to provide a valid fallback value in case
> >   	 * the default cpufreq governor is neither powersave nor performance.
> > @@ -1933,7 +1938,7 @@ static int amd_pstate_epp_cpu_init(struct cpufreq_policy *policy)
> >   	if (amd_pstate_acpi_pm_profile_server() ||
> >   	    amd_pstate_acpi_pm_profile_undefined()) {
> >   		policy->policy = CPUFREQ_POLICY_PERFORMANCE;
> > -		cpudata->epp_default_ac = cpudata->epp_default_dc = amd_pstate_get_epp(cpudata);
> > +		cpudata->epp_default_ac = cpudata->epp_default_dc = default_epp;
> >   		cpudata->current_profile = PLATFORM_PROFILE_PERFORMANCE;
> >   	} else {
> >   		policy->policy = CPUFREQ_POLICY_POWERSAVE;