diff mbox

[v2,1/4] x86/mcheck: Minor cleanup to amd_nonfatal

Message ID 1502882530-31700-2-git-send-email-andrew.cooper3@citrix.com (mailing list archive)
State New, archived
Headers show

Commit Message

Andrew Cooper Aug. 16, 2017, 11:22 a.m. UTC 
* Drop trailing whitespace.
  * Move amd_nonfatal_mcheck_init() into .init.text and drop a trailing return.
  * Drop unnecessary wmb()'s.  Because of Xen's implementation, they are only
    compiler barriers anyway, and each wrmsr() is already fully serialising.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>

New in v2
---
 xen/arch/x86/cpu/mcheck/amd_nonfatal.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

Comments

Jan Beulich Aug. 16, 2017, 3:11 p.m. UTC | #1 
>>> On 16.08.17 at 13:22, <andrew.cooper3@citrix.com> wrote:
> * Drop trailing whitespace.
>   * Move amd_nonfatal_mcheck_init() into .init.text and drop a trailing return.
>   * Drop unnecessary wmb()'s.  Because of Xen's implementation, they are only
>     compiler barriers anyway, and each wrmsr() is already fully serialising.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>
Tim Deegan Aug. 18, 2017, 1:19 p.m. UTC | #2 
At 12:22 +0100 on 16 Aug (1502886127), Andrew Cooper wrote:
>   * Drop trailing whitespace.
>   * Move amd_nonfatal_mcheck_init() into .init.text and drop a trailing return.
>   * Drop unnecessary wmb()'s.  Because of Xen's implementation, they are only
>     compiler barriers anyway, and each wrmsr() is already fully serialising.

But wrmsr() is not a compiler barrier!  So if the write-barriers are
needed (e.g. for the update to the global 'adjust') then you can't
remove them just because WRMSR is a CPU barrier.

If they're not needed (which is plausible) then the commit message
should explain that instead.

Nit: I think tinkering with memory barriers deserves its own commit,
not to be the third item in a list of 'minor cleanup's.

Cheers,

Tim.
diff mbox

Patch

diff --git a/xen/arch/x86/cpu/mcheck/amd_nonfatal.c b/xen/arch/x86/cpu/mcheck/amd_nonfatal.c
index c6a9c89..222f539 100644
--- a/xen/arch/x86/cpu/mcheck/amd_nonfatal.c
+++ b/xen/arch/x86/cpu/mcheck/amd_nonfatal.c
@@ -106,7 +106,7 @@  static void mce_amd_checkregs(void *info)
 		} else {
 			mctelem_dismiss(mctc);
 		}
-		
+
 	} else if (mctc != NULL) {
 		mctelem_dismiss(mctc);
 	}
@@ -151,7 +151,7 @@  static void mce_amd_work_fn(void *data)
 
 		/* HW does not count *all* kinds of correctable errors.
 		 * Thus it is possible, that the polling routine finds an
-		 * correctable error even if the HW reports nothing. */ 
+		 * correctable error even if the HW reports nothing. */
 		if (counter > 0) {
 			/* HW reported correctable errors,
 			 * the polling routine did not find...
@@ -164,8 +164,8 @@  static void mce_amd_work_fn(void *data)
 					(counter == 1 ? "" : "s"),
 					(counter == 1 ? "was" : "were"));
 			}
-			/* subtract 1 to not double count the error 
-			 * from the polling service routine */ 
+			/* subtract 1 to not double count the error
+			 * from the polling service routine */
 			adjust += (counter - 1);
 
 			/* Restart counter */
@@ -174,7 +174,6 @@  static void mce_amd_work_fn(void *data)
 			/* Counter enable */
 			value |= (1ULL << 51);
 			mca_wrmsr(MSR_IA32_MCx_MISC(4), value);
-			wmb();
 		}
 	}
 
@@ -202,7 +201,7 @@  static void mce_amd_work_fn(void *data)
 	adjust = 0;
 }
 
-void amd_nonfatal_mcheck_init(struct cpuinfo_x86 *c)
+void __init amd_nonfatal_mcheck_init(struct cpuinfo_x86 *c)
 {
 	if (c->x86_vendor != X86_VENDOR_AMD)
 		return;
@@ -238,14 +237,10 @@  void amd_nonfatal_mcheck_init(struct cpuinfo_x86 *c)
 			/* Counter enable */
 			value |= (1ULL << 51);
 			wrmsrl(MSR_IA32_MCx_MISC(4), value);
-			/* serialize */
-			wmb();
 			printk(XENLOG_INFO "MCA: Use hw thresholding to adjust polling frequency\n");
 		}
 	}
 
 	init_timer(&mce_timer, mce_amd_work_fn, NULL, 0);
 	set_timer(&mce_timer, NOW() + period);
-
-	return;
 }