| 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
| ||
|
Message-ID: <Z1Kc+UNjW3QNJPMm@BLRRASHENOY1.amd.com>
Date: Fri, 6 Dec 2024 12:13:05 +0530
From: "Gautham R. Shenoy" <gautham.shenoy@....com>
To: Mario Limonciello <mario.limonciello@....com>
Cc: Perry Yuan <perry.yuan@....com>, linux-kernel@...r.kernel.org,
linux-pm@...r.kernel.org,
Dhananjay Ugwekar <Dhananjay.Ugwekar@....com>
Subject: Re: [PATCH 08/15] cpufreq/amd-pstate: store all values in cpudata
struct in khz
On Thu, Dec 05, 2024 at 04:28:40PM -0600, Mario Limonciello wrote:
> Storing values in the cpudata structure in different units leads
> to confusion and hardcoded conversions elsewhere. After ratios are
> calculated store everything in khz for any future use. Adjust all
> relevant consumers for this change as well.
>
> Suggested-by: Dhananjay Ugwekar <Dhananjay.Ugwekar@....com>
> Signed-off-by: Mario Limonciello <mario.limonciello@....com>
Thanks for cleaning this up.
This also makes the code in-line with the comment for cpudata->nominal_freq which says
* @nominal_freq: the frequency (in khz) that mapped to nominal_perf
Couple of minor nits. See below.
> ---
> drivers/cpufreq/amd-pstate-ut.c | 12 +++++-------
> drivers/cpufreq/amd-pstate.c | 30 +++++++++++++++---------------
> 2 files changed, 20 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/cpufreq/amd-pstate-ut.c b/drivers/cpufreq/amd-pstate-ut.c
> index a261d7300951e..3a0a380c3590c 100644
> --- a/drivers/cpufreq/amd-pstate-ut.c
> +++ b/drivers/cpufreq/amd-pstate-ut.c
> @@ -207,7 +207,6 @@ static void amd_pstate_ut_check_freq(u32 index)
> int cpu = 0;
> struct cpufreq_policy *policy = NULL;
> struct amd_cpudata *cpudata = NULL;
> - u32 nominal_freq_khz;
>
> for_each_possible_cpu(cpu) {
> policy = cpufreq_cpu_get(cpu);
> @@ -215,14 +214,13 @@ static void amd_pstate_ut_check_freq(u32 index)
> break;
> cpudata = policy->driver_data;
>
> - nominal_freq_khz = cpudata->nominal_freq*1000;
> - if (!((cpudata->max_freq >= nominal_freq_khz) &&
> - (nominal_freq_khz > cpudata->lowest_nonlinear_freq) &&
> + if (!((cpudata->max_freq >= cpudata->nominal_freq) &&
> + (cpudata->nominal_freq > cpudata->lowest_nonlinear_freq) &&
> (cpudata->lowest_nonlinear_freq > cpudata->min_freq) &&
> (cpudata->min_freq > 0))) {
> amd_pstate_ut_cases[index].result = AMD_PSTATE_UT_RESULT_FAIL;
> pr_err("%s cpu%d max=%d >= nominal=%d > lowest_nonlinear=%d > min=%d > 0, the formula is incorrect!\n",
> - __func__, cpu, cpudata->max_freq, nominal_freq_khz,
> + __func__, cpu, cpudata->max_freq, cpudata->nominal_freq,
> cpudata->lowest_nonlinear_freq, cpudata->min_freq);
> goto skip_test;
> }
> @@ -236,13 +234,13 @@ static void amd_pstate_ut_check_freq(u32 index)
>
> if (cpudata->boost_supported) {
> if ((policy->max == cpudata->max_freq) ||
> - (policy->max == nominal_freq_khz))
> + (policy->max == cpudata->nominal_freq))
> amd_pstate_ut_cases[index].result = AMD_PSTATE_UT_RESULT_PASS;
> else {
> amd_pstate_ut_cases[index].result = AMD_PSTATE_UT_RESULT_FAIL;
> pr_err("%s cpu%d policy_max=%d should be equal cpu_max=%d or cpu_nominal=%d !\n",
> __func__, cpu, policy->max, cpudata->max_freq,
> - nominal_freq_khz);
> + cpudata->nominal_freq);
> goto skip_test;
> }
> } else {
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index ce70d1bfa55d0..464db6745d84e 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -739,8 +739,8 @@ static int amd_pstate_cpu_boost_update(struct cpufreq_policy *policy, bool on)
>
> if (on)
> policy->cpuinfo.max_freq = max_freq;
> - else if (policy->cpuinfo.max_freq > nominal_freq * 1000)
> - policy->cpuinfo.max_freq = nominal_freq * 1000;
> + else if (policy->cpuinfo.max_freq > nominal_freq)
> + policy->cpuinfo.max_freq = nominal_freq;
>
> policy->max = policy->cpuinfo.max_freq;
>
> @@ -940,29 +940,29 @@ static int amd_pstate_init_freq(struct amd_cpudata *cpudata)
> return ret;
>
> if (quirks && quirks->lowest_freq)
> - min_freq = quirks->lowest_freq * 1000;
> + min_freq = quirks->lowest_freq;
> else
> - min_freq = cppc_perf.lowest_freq * 1000;
> + min_freq = cppc_perf.lowest_freq;
>
> if (quirks && quirks->nominal_freq)
> - nominal_freq = quirks->nominal_freq ;
> + nominal_freq = quirks->nominal_freq;
> else
> nominal_freq = cppc_perf.nominal_freq;
>
> nominal_perf = READ_ONCE(cpudata->nominal_perf);
>
> boost_ratio = div_u64(cpudata->highest_perf << SCHED_CAPACITY_SHIFT, nominal_perf);
> - max_freq = (nominal_freq * boost_ratio >> SCHED_CAPACITY_SHIFT) * 1000;
> + max_freq = (nominal_freq * boost_ratio >> SCHED_CAPACITY_SHIFT);
>
> lowest_nonlinear_perf = READ_ONCE(cpudata->lowest_nonlinear_perf);
> lowest_nonlinear_ratio = div_u64(lowest_nonlinear_perf << SCHED_CAPACITY_SHIFT,
> nominal_perf);
> - lowest_nonlinear_freq = (nominal_freq * lowest_nonlinear_ratio >> SCHED_CAPACITY_SHIFT) * 1000;
> + lowest_nonlinear_freq = (nominal_freq * lowest_nonlinear_ratio >> SCHED_CAPACITY_SHIFT);
>
> - WRITE_ONCE(cpudata->min_freq, min_freq);
> - WRITE_ONCE(cpudata->lowest_nonlinear_freq, lowest_nonlinear_freq);
> - WRITE_ONCE(cpudata->nominal_freq, nominal_freq);
> - WRITE_ONCE(cpudata->max_freq, max_freq);
> + WRITE_ONCE(cpudata->min_freq, min_freq * 1000);
> + WRITE_ONCE(cpudata->lowest_nonlinear_freq, lowest_nonlinear_freq * 1000);
> + WRITE_ONCE(cpudata->nominal_freq, nominal_freq * 1000);
> + WRITE_ONCE(cpudata->max_freq, max_freq * 1000);
>
> /**
> * Below values need to be initialized correctly, otherwise driver will fail to load
> @@ -970,15 +970,15 @@ static int amd_pstate_init_freq(struct amd_cpudata *cpudata)
> * lowest_nonlinear_freq is a value between [min_freq, nominal_freq]
> * Check _CPC in ACPI table objects if any values are incorrect
> */
> - if (min_freq <= 0 || max_freq <= 0 || nominal_freq <= 0 || min_freq > max_freq) {
> + if (min_freq <= 0 || max_freq <= 0 || cpudata->nominal_freq <= 0 || min_freq > max_freq) {
> pr_err("min_freq(%d) or max_freq(%d) or nominal_freq(%d) value is incorrect\n",
> - min_freq, max_freq, nominal_freq * 1000);
> + min_freq, max_freq, cpudata->nominal_freq);
> return -EINVAL;
We don't need this change. This looks odd since we are using min_freq
and max_freq from local variables, but nominal_freq from cpudata.
We may just as well use the local variables in all the cases since
"cpudata->nominal_freq = nominal_freq * 1000", and thus won't make any
difference to the checks above.
> }
>
> - if (lowest_nonlinear_freq <= min_freq || lowest_nonlinear_freq > nominal_freq * 1000) {
^^^^^^^^^^^^^^^^^^^^
Simply replace this with nominal_freq ?
Because both lowest_nonlinear_freq and nominal_freq are in MHz at this
point.
> + if (lowest_nonlinear_freq <= min_freq || lowest_nonlinear_freq > cpudata->nominal_freq) {
> pr_err("lowest_nonlinear_freq(%d) value is out of range [min_freq(%d), nominal_freq(%d)]\n",
> - lowest_nonlinear_freq, min_freq, nominal_freq * 1000);
> + lowest_nonlinear_freq, min_freq, cpudata->nominal_freq);
^^^^^^^^^^^^^^^^^^^^^^
Likewise, this can just be nominal_freq
> return -EINVAL;
> }
>
> --
> 2.43.0
>
Other than that,
Reviewed-by: Gautham R. Shenoy <gautham.shenoy@....com>
--
Thanks and Regards
gautham.
Powered by blists - more mailing lists