Re: [PATCH v2 3/5] x86/mce: Use mca_msr_reg() in prepare_msrs()

["Koralahalli Channabasappa, Smita" <skoralah@amd.com>]

On 10/27/21 6:41 AM, Borislav Petkov wrote:
> On Tue, Oct 19, 2021 at 06:36:39PM -0500, Smita Koralahalli wrote:
>> Replace MCx_{STATUS, ADDR, MISC} macros with mca_msr_reg().
> And this is where your commit message and patch should end. It is a bad
> idea to do textual replacements *and* functional changes in a single
> patch: it is hard to review and debug if there are possible issues. So
> you do the textual replacements in the first one and then the functional
> changes in subsequent patches.

Okay I will break this down and send v3 as suggested.

>> Also, restructure the code to avoid multiple initializations for MCA
>> registers.
> What multiple initializations?

Multiple initialization here I mean: Initializing the MCA registers twice.
Prior to mca_msr_reg() replacement, the MCA registers were initialized
separately for SMCA and legacy processors. However, this is not required
after replacing with mca_msr_reg() as it does the job of returning the
proper MSR addresses.

Probably, my wording is more confusing here. Does this seem better?
"Do not initialize MCx_{STATUS, ADDR, MISC} separately for SMCA and
legacy processors as mca_msr_reg() returns the appropriate MSR addresses
for both."

I will split this into second patch.

>> SMCA machines define a different set of MSRs for MCA registers
>> and mca_msr_reg() returns the proper MSR address for SMCA and legacy
>> processors.
>>
>> Initialize MCA_MISC and MCA_SYND registers at the end after initializing
>> MCx_{STATUS, DESTAT} which is further explained in the next patch.
> And this should be *in* the next patch.

Okay, basically break it down into three. One for replacing, one for
cleaning up the multiple initialization of MCA registers and the last for
moving MCA_MISC and MCA_SYND to the end.

Will do it as suggested..

>
> Also, there's no concept of "next patch" when you do git log on the
> upstream tree and use different sorting etc. So a patch should be
> self-contained and do one change only.
>
> There's very good documentation in Documentation/process/, expecially
> Documentation/process/submitting-patches.rst, which explains how a patch
> should look like.
>
> Thx.

Ok will take a look at this again to correct my mistakes. Thanks for the
inputs.

Thanks,
Smita