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:   Thomas Gleixner <tglx@linutronix.de>
To:     lizhe.67@bytedance.com, tony.luck@intel.com, bp@alien8.de,
        mingo@redhat.com, dave.hansen@linux.intel.com, x86@kernel.org,
        hpa@zytor.com, 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, lizhe.67@bytedance.com
Subject: Re: [RFC] msr: judge the return val of function rdmsrl_on_cpu() by
 WARN_ON
In-Reply-To: <20230629072754.39844-1-lizhe.67@bytedance.com>
References: <20230629072754.39844-1-lizhe.67@bytedance.com>
Date:   Fri, 30 Jun 2023 01:03:22 +0200
Message-ID: <87r0ptzxad.ffs@tglx>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

Li!

On Thu, Jun 29 2023 at 15:27, lizhe.67@bytedance.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.

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