Messages in this thread | | | Subject | Re: [PATCH v2 3/5] x86/mce: Use mca_msr_reg() in prepare_msrs() | From | "Koralahalli Channabasappa, Smita" <> | Date | Wed, 27 Oct 2021 15:19:51 -0500 |
| |
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
| |