linux-kernel - Re: [PATCH 2/4] cpufreq/amd-pstate: Don't update CPPC request in amd_pstate_cpu_boost

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Zw+MUOYw+nMuQ9Nk@BLRRASHENOY1.amd.com>
Date: Wed, 16 Oct 2024 15:20:08 +0530
From: "Gautham R. Shenoy" <gautham.shenoy@....com>
To: "Yuan, Perry" <Perry.Yuan@....com>
Cc: "Limonciello, Mario" <Mario.Limonciello@....com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-pm@...r.kernel.org" <linux-pm@...r.kernel.org>,
	"Ugwekar, Dhananjay" <Dhananjay.Ugwekar@....com>
Subject: Re: [PATCH 2/4] cpufreq/amd-pstate: Don't update CPPC request in
 amd_pstate_cpu_boost_update()

Hello Mario,

[..snip..]

> >
> > When boost is changed the CPPC value is changed in
> > amd_pstate_cpu_boost_update() but then changed again when
> > refresh_frequency_limits() and all it's callbacks occur.  The first is a pointless write,
> > so instead just update the limits for the policy and let the policy refresh anchor
> > everything properly.
> >
> > Fixes: c8c68c38b56f ("cpufreq: amd-pstate: initialize core precision boost state")
> > Signed-off-by: Mario Limonciello <mario.limonciello@....com>
> > ---
> >  drivers/cpufreq/amd-pstate.c | 24 +-----------------------
> >  1 file changed, 1 insertion(+), 23 deletions(-)
> >
> > diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c index
> > dfa9a146769b..13dec8b1e7a8 100644
> > --- a/drivers/cpufreq/amd-pstate.c
> > +++ b/drivers/cpufreq/amd-pstate.c
> > @@ -665,34 +665,12 @@ static void amd_pstate_adjust_perf(unsigned int cpu,
> > static int amd_pstate_cpu_boost_update(struct cpufreq_policy *policy, bool on)  {
> >       struct amd_cpudata *cpudata = policy->driver_data;
> > -     struct cppc_perf_ctrls perf_ctrls;
> > -     u32 highest_perf, nominal_perf, nominal_freq, max_freq;
> > +     u32 nominal_freq, max_freq;
> >       int ret = 0;
> >
> > -     highest_perf = READ_ONCE(cpudata->highest_perf);
> > -     nominal_perf = READ_ONCE(cpudata->nominal_perf);
> >       nominal_freq = READ_ONCE(cpudata->nominal_freq);
> >       max_freq = READ_ONCE(cpudata->max_freq);
> >
> > -     if (boot_cpu_has(X86_FEATURE_CPPC)) {
> > -             u64 value = READ_ONCE(cpudata->cppc_req_cached);
> > -
> > -             value &= ~GENMASK_ULL(7, 0);
> > -             value |= on ? highest_perf : nominal_perf;
> > -             WRITE_ONCE(cpudata->cppc_req_cached, value);
> > -
> > -             wrmsrl_on_cpu(cpudata->cpu, MSR_AMD_CPPC_REQ, value);

So, for amd-pstate-epp driver

refresh_frequency_limits()
  --> cpufreq_set_policy()
       --> amd_pstate_epp_set_policy()
           --> amd_pstate_epp_update_limit()

will ensure that the MSR is updated with the is updated with the
value after Patch 1.


> > -     } else {
> > -             perf_ctrls.max_perf = on ? highest_perf : nominal_perf;
> > -             ret = cppc_set_perf(cpudata->cpu, &perf_ctrls);


refresh_frequency_limits()
 --> cpufreq_start_governor()
   --> governor->limits()
      --> cpufreq_policy_apply_limits()
        --> __cpufreq_driver_target()
	   --> amd_pstate_target()
	     --> amd_pstate_update_freq()
	       --> amd_pstate_update()

So these updates in amd_pstate_cpu_boost_update() seem redundant.

Reviewed-by: Gautham R. Shenoy <gautham.shenoy@....com>

--
Thanks and Regards
gautham.