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;
From:   lizhe.67@bytedance.com
To:     hpa@zytor.com
Cc:     bp@alien8.de, dave.hansen@linux.intel.com,
        linux-edac@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-pm@vger.kernel.org, lizefan.x@bytedance.com,
        lizhe.67@bytedance.com, mingo@redhat.com, rafael@kernel.org,
        tglx@linutronix.de, tony.luck@intel.com, viresh.kumar@linaro.org,
        x86@kernel.org, yuanzhu@bytedance.com
Subject: Re: [RFC] msr: judge the return val of function rdmsrl_on_cpu() by WARN_ON
Date:   Fri, 30 Jun 2023 10:42:34 +0800
Message-Id: <20230630024234.76075-1-lizhe.67@bytedance.com>
In-Reply-To: <1D4C4027-C450-4346-A2EA-CEEC94CFDA64@zytor.com>
References: <1D4C4027-C450-4346-A2EA-CEEC94CFDA64@zytor.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 29 Jun 2023 08:13:27 -0700, hpa@zytor.com wrote:
>>There are ten places call rdmsrl_on_cpu() in the current code without
>>judging the return value. This may introduce a potential bug. For example,
>>inj_bank_set() may return -EINVAL, show_base_frequency() may show an error
>>freq value, intel_pstate_hwp_set() may write an error value to the related
>>msr register and so on. But rdmsrl_on_cpu() do rarely returns an error, so
>>it seems that add a WARN_ON is enough for debugging.
>>
>>Signed-off-by: Li Zhe <lizhe.67@bytedance.com>
>>---
>> arch/x86/kernel/cpu/mce/inject.c |  2 +-
>> drivers/cpufreq/intel_pstate.c   | 18 +++++++++---------
>> 2 files changed, 10 insertions(+), 10 deletions(-)
>>
>>diff --git a/arch/x86/kernel/cpu/mce/inject.c b/arch/x86/kernel/cpu/mce/inject.c
>>index 12cf2e7ca33c..0a34057f4fc6 100644
>>--- a/arch/x86/kernel/cpu/mce/inject.c
>>+++ b/arch/x86/kernel/cpu/mce/inject.c
>>@@ -587,7 +587,7 @@ static int inj_bank_set(void *data, u64 val)
>> 	u64 cap;
>> 
>> 	/* Get bank count on target CPU so we can handle non-uniform values. */
>>-	rdmsrl_on_cpu(m->extcpu, MSR_IA32_MCG_CAP, &cap);
>>+	WARN_ON(rdmsrl_on_cpu(m->extcpu, MSR_IA32_MCG_CAP, &cap));
>> 	n_banks = cap & MCG_BANKCNT_MASK;
>> 
>> 	if (val >= n_banks) {
>>diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
>>index 2548ec92faa2..fe2bdb38d6a0 100644
>>--- a/drivers/cpufreq/intel_pstate.c
>>+++ b/drivers/cpufreq/intel_pstate.c
>>@@ -859,7 +859,7 @@ static ssize_t show_base_frequency(struct cpufreq_policy *policy, char *buf)
>> 	if (ratio <= 0) {
>> 		u64 cap;
>> 
>>-		rdmsrl_on_cpu(policy->cpu, MSR_HWP_CAPABILITIES, &cap);
>>+		WARN_ON(rdmsrl_on_cpu(policy->cpu, MSR_HWP_CAPABILITIES, &cap));
>> 		ratio = HWP_GUARANTEED_PERF(cap);
>> 	}
>> 
>>@@ -883,7 +883,7 @@ static void __intel_pstate_get_hwp_cap(struct cpudata *cpu)
>> {
>> 	u64 cap;
>> 
>>-	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.max_pstate = HWP_GUARANTEED_PERF(cap);
>> 	cpu->pstate.turbo_pstate = HWP_HIGHEST_PERF(cap);
>>@@ -920,7 +920,7 @@ static void intel_pstate_hwp_set(unsigned int cpu)
>> 	if (cpu_data->policy == CPUFREQ_POLICY_PERFORMANCE)
>> 		min = max;
>> 
>>-	rdmsrl_on_cpu(cpu, MSR_HWP_REQUEST, &value);
>>+	WARN_ON(rdmsrl_on_cpu(cpu, MSR_HWP_REQUEST, &value));
>> 
>> 	value &= ~HWP_MIN_PERF(~0L);
>> 	value |= HWP_MIN_PERF(min);
>>@@ -1802,7 +1802,7 @@ static int core_get_min_pstate(int cpu)
>> {
>> 	u64 value;
>> 
>>-	rdmsrl_on_cpu(cpu, MSR_PLATFORM_INFO, &value);
>>+	WARN_ON(rdmsrl_on_cpu(cpu, MSR_PLATFORM_INFO, &value));
>> 	return (value >> 40) & 0xFF;
>> }
>> 
>>@@ -1810,7 +1810,7 @@ static int core_get_max_pstate_physical(int cpu)
>> {
>> 	u64 value;
>> 
>>-	rdmsrl_on_cpu(cpu, MSR_PLATFORM_INFO, &value);
>>+	WARN_ON(rdmsrl_on_cpu(cpu, MSR_PLATFORM_INFO, &value));
>> 	return (value >> 8) & 0xFF;
>> }
>> 
>>@@ -1855,7 +1855,7 @@ static int core_get_max_pstate(int cpu)
>> 	int tdp_ratio;
>> 	int err;
>> 
>>-	rdmsrl_on_cpu(cpu, MSR_PLATFORM_INFO, &plat_info);
>>+	WARN_ON(rdmsrl_on_cpu(cpu, MSR_PLATFORM_INFO, &plat_info));
>> 	max_pstate = (plat_info >> 8) & 0xFF;
>> 
>> 	tdp_ratio = core_get_tdp_ratio(cpu, plat_info);
>>@@ -1887,7 +1887,7 @@ static int core_get_turbo_pstate(int cpu)
>> 	u64 value;
>> 	int nont, ret;
>> 
>>-	rdmsrl_on_cpu(cpu, MSR_TURBO_RATIO_LIMIT, &value);
>>+	WARN_ON(rdmsrl_on_cpu(cpu, MSR_TURBO_RATIO_LIMIT, &value));
>> 	nont = core_get_max_pstate(cpu);
>> 	ret = (value) & 255;
>> 	if (ret <= nont)
>>@@ -1921,7 +1921,7 @@ static int knl_get_turbo_pstate(int cpu)
>> 	u64 value;
>> 	int nont, ret;
>> 
>>-	rdmsrl_on_cpu(cpu, MSR_TURBO_RATIO_LIMIT, &value);
>>+	WARN_ON(rdmsrl_on_cpu(cpu, MSR_TURBO_RATIO_LIMIT, &value));
>> 	nont = core_get_max_pstate(cpu);
>> 	ret = (((value) >> 8) & 0xFF);
>> 	if (ret <= nont)
>>@@ -2974,7 +2974,7 @@ static int intel_cpufreq_cpu_init(struct cpufreq_policy *policy)
>> 
>> 		intel_pstate_get_hwp_cap(cpu);
>> 
>>-		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);
>> 
>> 		cpu->epp_cached = 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 faults.

Thanks for your advice. I will take tglx@linutronix.de's advice to analysis each callsite.