Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 29 Jun 2023 08:13:27 -0700
From:   "H. Peter Anvin" <hpa@zytor.com>
To:     lizhe.67@bytedance.com, tony.luck@intel.com, bp@alien8.de,
        tglx@linutronix.de, mingo@redhat.com, dave.hansen@linux.intel.com,
        x86@kernel.org, rafael@kernel.org, viresh.kumar@linaro.org
CC:     linux-edac@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-pm@vger.kernel.org, lizefan.x@bytedance.com,
        yuanzhu@bytedance.com
Subject: =?US-ASCII?Q?Re=3A_=5BRFC=5D_msr=3A_judge_the_return_val_o?= =?US-ASCII?Q?f_function_rdmsrl=5Fon=5Fcpu=28=29_by_WARN=5FON?=
User-Agent: K-9 Mail for Android
In-Reply-To: <20230629072754.39844-1-lizhe.67@bytedance.com>
References: <20230629072754.39844-1-lizhe.67@bytedance.com>
Message-ID: <1D4C4027-C450-4346-A2EA-CEEC94CFDA64@zytor.com>
MIME-Version: 1.0
Content-Type: text/plain;
 charset=utf-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On June 29, 2023 12:27:54 AM PDT, lizhe=2E67@bytedance=2Ecom wrote:
>From: Li Zhe <lizhe=2E67@bytedance=2Ecom>
>
>There are ten places call rdmsrl_on_cpu() in the current code without
>judging the return value=2E This may introduce a potential bug=2E For exa=
mple,
>inj_bank_set() may return -EINVAL, show_base_frequency() may show an erro=
r
>freq value, intel_pstate_hwp_set() may write an error value to the relate=
d
>msr register and so on=2E But rdmsrl_on_cpu() do rarely returns an error,=
 so
>it seems that add a WARN_ON is enough for debugging=2E
>
>Signed-off-by: Li Zhe <lizhe=2E67@bytedance=2Ecom>
>---
> arch/x86/kernel/cpu/mce/inject=2Ec |  2 +-
> drivers/cpufreq/intel_pstate=2Ec   | 18 +++++++++---------
> 2 files changed, 10 insertions(+), 10 deletions(-)
>
>diff --git a/arch/x86/kernel/cpu/mce/inject=2Ec b/arch/x86/kernel/cpu/mce=
/inject=2Ec
>index 12cf2e7ca33c=2E=2E0a34057f4fc6 100644
>--- a/arch/x86/kernel/cpu/mce/inject=2Ec
>+++ b/arch/x86/kernel/cpu/mce/inject=2Ec
>@@ -587,7 +587,7 @@ static int inj_bank_set(void *data, u64 val)
> 	u64 cap;
>=20
> 	/* Get bank count on target CPU so we can handle non-uniform values=2E =
*/
>-	rdmsrl_on_cpu(m->extcpu, MSR_IA32_MCG_CAP, &cap);
>+	WARN_ON(rdmsrl_on_cpu(m->extcpu, MSR_IA32_MCG_CAP, &cap));
> 	n_banks =3D cap & MCG_BANKCNT_MASK;
>=20
> 	if (val >=3D n_banks) {
>diff --git a/drivers/cpufreq/intel_pstate=2Ec b/drivers/cpufreq/intel_pst=
ate=2Ec
>index 2548ec92faa2=2E=2Efe2bdb38d6a0 100644
>--- a/drivers/cpufreq/intel_pstate=2Ec
>+++ b/drivers/cpufreq/intel_pstate=2Ec
>@@ -859,7 +859,7 @@ static ssize_t show_base_frequency(struct cpufreq_pol=
icy *policy, char *buf)
> 	if (ratio <=3D 0) {
> 		u64 cap;
>=20
>-		rdmsrl_on_cpu(policy->cpu, MSR_HWP_CAPABILITIES, &cap);
>+		WARN_ON(rdmsrl_on_cpu(policy->cpu, MSR_HWP_CAPABILITIES, &cap));
> 		ratio =3D HWP_GUARANTEED_PERF(cap);
> 	}
>=20
>@@ -883,7 +883,7 @@ static void __intel_pstate_get_hwp_cap(struct cpudata=
 *cpu)
> {
> 	u64 cap;
>=20
>-	rdmsrl_on_cpu(cpu->cpu, MSR_HWP_CAPABILITIES, &cap);
>+	WARN_ON(rdmsrl_on_cpu(cpu->cpu, MSR_HWP_CAPABILITIES, &cap));
> 	WRITE_ONCE(cpu->hwp_cap_cached, cap);
> 	cpu->pstate=2Emax_pstate =3D HWP_GUARANTEED_PERF(cap);
> 	cpu->pstate=2Eturbo_pstate =3D HWP_HIGHEST_PERF(cap);
>@@ -920,7 +920,7 @@ static void intel_pstate_hwp_set(unsigned int cpu)
> 	if (cpu_data->policy =3D=3D CPUFREQ_POLICY_PERFORMANCE)
> 		min =3D max;
>=20
>-	rdmsrl_on_cpu(cpu, MSR_HWP_REQUEST, &value);
>+	WARN_ON(rdmsrl_on_cpu(cpu, MSR_HWP_REQUEST, &value));
>=20
> 	value &=3D ~HWP_MIN_PERF(~0L);
> 	value |=3D HWP_MIN_PERF(min);
>@@ -1802,7 +1802,7 @@ static int core_get_min_pstate(int cpu)
> {
> 	u64 value;
>=20
>-	rdmsrl_on_cpu(cpu, MSR_PLATFORM_INFO, &value);
>+	WARN_ON(rdmsrl_on_cpu(cpu, MSR_PLATFORM_INFO, &value));
> 	return (value >> 40) & 0xFF;
> }
>=20
>@@ -1810,7 +1810,7 @@ static int core_get_max_pstate_physical(int cpu)
> {
> 	u64 value;
>=20
>-	rdmsrl_on_cpu(cpu, MSR_PLATFORM_INFO, &value);
>+	WARN_ON(rdmsrl_on_cpu(cpu, MSR_PLATFORM_INFO, &value));
> 	return (value >> 8) & 0xFF;
> }
>=20
>@@ -1855,7 +1855,7 @@ static int core_get_max_pstate(int cpu)
> 	int tdp_ratio;
> 	int err;
>=20
>-	rdmsrl_on_cpu(cpu, MSR_PLATFORM_INFO, &plat_info);
>+	WARN_ON(rdmsrl_on_cpu(cpu, MSR_PLATFORM_INFO, &plat_info));
> 	max_pstate =3D (plat_info >> 8) & 0xFF;
>=20
> 	tdp_ratio =3D core_get_tdp_ratio(cpu, plat_info);
>@@ -1887,7 +1887,7 @@ static int core_get_turbo_pstate(int cpu)
> 	u64 value;
> 	int nont, ret;
>=20
>-	rdmsrl_on_cpu(cpu, MSR_TURBO_RATIO_LIMIT, &value);
>+	WARN_ON(rdmsrl_on_cpu(cpu, MSR_TURBO_RATIO_LIMIT, &value));
> 	nont =3D core_get_max_pstate(cpu);
> 	ret =3D (value) & 255;
> 	if (ret <=3D nont)
>@@ -1921,7 +1921,7 @@ static int knl_get_turbo_pstate(int cpu)
> 	u64 value;
> 	int nont, ret;
>=20
>-	rdmsrl_on_cpu(cpu, MSR_TURBO_RATIO_LIMIT, &value);
>+	WARN_ON(rdmsrl_on_cpu(cpu, MSR_TURBO_RATIO_LIMIT, &value));
> 	nont =3D core_get_max_pstate(cpu);
> 	ret =3D (((value) >> 8) & 0xFF);
> 	if (ret <=3D nont)
>@@ -2974,7 +2974,7 @@ static int intel_cpufreq_cpu_init(struct cpufreq_po=
licy *policy)
>=20
> 		intel_pstate_get_hwp_cap(cpu);
>=20
>-		rdmsrl_on_cpu(cpu->cpu, MSR_HWP_REQUEST, &value);
>+		WARN_ON(rdmsrl_on_cpu(cpu->cpu, MSR_HWP_REQUEST, &value));
> 		WRITE_ONCE(cpu->hwp_req_cached, value);
>=20
> 		cpu->epp_cached =3D intel_pstate_get_epp(cpu, value);

Be careful here: if a return value of zero is acceptable as an equivalent =
of no return, the code is correct, as we always return zero if the MSR faul=
ts=2E