From: Li Zhe <[email protected]>
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 <[email protected]>
---
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);
--
2.20.1
On June 29, 2023 12:27:54 AM PDT, [email protected] wrote:
>From: Li Zhe <[email protected]>
>
>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 <[email protected]>
>---
> 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.
Li!
On Thu, Jun 29 2023 at 15:27, [email protected] 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.
Can you please structure your changelogs as documented in:
https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#changelog
instead of providing a big lump of words?
> There are ten places call rdmsrl_on_cpu() in the current code without
> judging the return value.
Return values are not judged. They are either ignored or checked/evaluated.
> This may introduce a potential bug.
Sure. Anything which does not check a return value from a function might
be a bug, but you have to look at each instance whether its a bug or
not.
> 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.
This is hillarious at best.
1) It does not matter at all whether that function returns an error rarely
or not.
2) Adding WARN_ON() without justification at each call site is not
enough. Neither for debugging nor for real world usage.
You have to come up with individual patches for each callsite to add the
WARN_ON() and in each patch you have to explain why it is justified and
why there is no other solution, e.g. taking an error exit path.
Just slapping WARN_ON()'s into the code without any deeper analysis is
worse than the current state of the code.
If you have identified a real world problem at any of these call sites
then adding a WARN_ON() does not solve it at all.
I'm looking forward to your profound anlysis of each of these "problems".
Thanks,
tglx
On 29 Jun 2023 08:13:27 -0700, [email protected] 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 <[email protected]>
>>---
>> 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 [email protected]'s advice to analysis each callsite.
On Fri, 30 Jun 2023 at 01:03:22 +0200, [email protected] wrote:
>Li!
>
>On Thu, Jun 29 2023 at 15:27, [email protected] 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.
>
>Can you please structure your changelogs as documented in:
>
> https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#changelog
>
>instead of providing a big lump of words?
>
>> There are ten places call rdmsrl_on_cpu() in the current code without
>> judging the return value.
>
>Return values are not judged. They are either ignored or checked/evaluated.
>
>> This may introduce a potential bug.
>
>Sure. Anything which does not check a return value from a function might
>be a bug, but you have to look at each instance whether its a bug or
>not.
>
>> 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.
>
>This is hillarious at best.
>
> 1) It does not matter at all whether that function returns an error rarely
> or not.
>
> 2) Adding WARN_ON() without justification at each call site is not
> enough. Neither for debugging nor for real world usage.
>
>You have to come up with individual patches for each callsite to add the
>WARN_ON() and in each patch you have to explain why it is justified and
>why there is no other solution, e.g. taking an error exit path.
>
>Just slapping WARN_ON()'s into the code without any deeper analysis is
>worse than the current state of the code.
>
>If you have identified a real world problem at any of these call sites
>then adding a WARN_ON() does not solve it at all.
>
>I'm looking forward to your profound anlysis of each of these "problems".
>
>Thanks,
>
> tglx
Thanks for all your advice. I will analysis each of these "problems".