[PATCH V2 0/2] MCE, AMD: MCE decoding support for AMD Family 16h

[PATCH V2 0/2] MCE, AMD: MCE decoding support for AMD Family 16h

[Jacob Shin]

The following patchset enables MCE decoding support for AMD Family 16h
processors.

Changes in V2:
* Changed if/else style and pr_cont usage per feedback from:
  https://lkml.org/lkml/2012/12/17/365

* Merged f14h and f16h mc0 and mc2 decoding into common function per
  feedback from: https://lkml.org/lkml/2012/12/18/269

* Fixed typo in INT_ERROR decoding.

Jacob Shin (2):
  MCE, AMD: Make MC2 decoding part of amd_decoder_ops as well
  MCE, AMD: MCE decoding support for AMD Family 16h

 drivers/edac/mce_amd.c |  139 +++++++++++++++++++++++++++++++++++-------------
 drivers/edac/mce_amd.h |    4 ++
 2 files changed, 105 insertions(+), 38 deletions(-)

-- 
1.7.9.5

[PATCH 1/2] MCE, AMD: Make MC2 decoding part of amd_decoder_ops as well

[Jacob Shin]

Currently only AMD Family 15h processors have special handling for MC2
errors, since upcoming Family 16h will also need unique handling,
let's make MC2 handling part of amd_decoder_ops.

Signed-off-by: Jacob Shin <jacob.shin@amd.com>
---
 drivers/edac/mce_amd.c |   56 +++++++++++++++++++++++++-----------------------
 drivers/edac/mce_amd.h |    1 +
 2 files changed, 30 insertions(+), 27 deletions(-)

diff --git a/drivers/edac/mce_amd.c b/drivers/edac/mce_amd.c
index ad63757..e4752be 100644
--- a/drivers/edac/mce_amd.c
+++ b/drivers/edac/mce_amd.c
@@ -399,12 +399,9 @@ static void decode_mc1_mce(struct mce *m)
 		pr_emerg(HW_ERR "Corrupted MC1 MCE info?\n");
 }
 
-static void decode_mc2_mce(struct mce *m)
+static bool k8_mc2_mce(u16 ec, u8 xec)
 {
-	u16 ec = EC(m->status);
-	u8 xec = XEC(m->status, xec_mask);
-
-	pr_emerg(HW_ERR "MC2 Error");
+	bool ret = true;
 
 	if (xec == 0x1)
 		pr_cont(" in the write data buffers.\n");
@@ -429,24 +426,18 @@ static void decode_mc2_mce(struct mce *m)
 				pr_cont(": %s parity/ECC error during data "
 					"access from L2.\n", R4_MSG(ec));
 			else
-				goto wrong_mc2_mce;
+				ret = false;
 		} else
-			goto wrong_mc2_mce;
+			ret = false;
 	} else
-		goto wrong_mc2_mce;
-
-	return;
+		ret = false;
 
- wrong_mc2_mce:
-	pr_emerg(HW_ERR "Corrupted MC2 MCE info?\n");
+	return ret;
 }
 
-static void decode_f15_mc2_mce(struct mce *m)
+static bool f15h_mc2_mce(u16 ec, u8 xec)
 {
-	u16 ec = EC(m->status);
-	u8 xec = XEC(m->status, xec_mask);
-
-	pr_emerg(HW_ERR "MC2 Error: ");
+	bool ret = true;
 
 	if (TLB_ERROR(ec)) {
 		if (xec == 0x0)
@@ -454,10 +445,10 @@ static void decode_f15_mc2_mce(struct mce *m)
 		else if (xec == 0x1)
 			pr_cont("Poison data provided for TLB fill.\n");
 		else
-			goto wrong_f15_mc2_mce;
+			ret = false;
 	} else if (BUS_ERROR(ec)) {
 		if (xec > 2)
-			goto wrong_f15_mc2_mce;
+			ret = false;
 
 		pr_cont("Error during attempted NB data read.\n");
 	} else if (MEM_ERROR(ec)) {
@@ -471,14 +462,22 @@ static void decode_f15_mc2_mce(struct mce *m)
 			break;
 
 		default:
-			goto wrong_f15_mc2_mce;
+			ret = false;
 		}
 	}
 
-	return;
+	return ret;
+}
+
+static void decode_mc2_mce(struct mce *m)
+{
+	u16 ec = EC(m->status);
+	u8 xec = XEC(m->status, xec_mask);
 
- wrong_f15_mc2_mce:
-	pr_emerg(HW_ERR "Corrupted MC2 MCE info?\n");
+	pr_emerg(HW_ERR "MC2 Error: ");
+
+	if (!fam_ops->mc2_mce(ec, xec))
+		pr_cont(HW_ERR "Corrupted MC2 MCE info?\n");
 }
 
 static void decode_mc3_mce(struct mce *m)
@@ -702,10 +701,7 @@ int amd_decode_mce(struct notifier_block *nb, unsigned long val, void *data)
 		break;
 
 	case 2:
-		if (c->x86 == 0x15)
-			decode_f15_mc2_mce(m);
-		else
-			decode_mc2_mce(m);
+		decode_mc2_mce(m);
 		break;
 
 	case 3:
@@ -783,33 +779,39 @@ static int __init mce_amd_init(void)
 	case 0xf:
 		fam_ops->mc0_mce = k8_mc0_mce;
 		fam_ops->mc1_mce = k8_mc1_mce;
+		fam_ops->mc2_mce = k8_mc2_mce;
 		break;
 
 	case 0x10:
 		fam_ops->mc0_mce = f10h_mc0_mce;
 		fam_ops->mc1_mce = k8_mc1_mce;
+		fam_ops->mc2_mce = k8_mc2_mce;
 		break;
 
 	case 0x11:
 		fam_ops->mc0_mce = k8_mc0_mce;
 		fam_ops->mc1_mce = k8_mc1_mce;
+		fam_ops->mc2_mce = k8_mc2_mce;
 		break;
 
 	case 0x12:
 		fam_ops->mc0_mce = f12h_mc0_mce;
 		fam_ops->mc1_mce = k8_mc1_mce;
+		fam_ops->mc2_mce = k8_mc2_mce;
 		break;
 
 	case 0x14:
 		nb_err_cpumask  = 0x3;
 		fam_ops->mc0_mce = f14h_mc0_mce;
 		fam_ops->mc1_mce = f14h_mc1_mce;
+		fam_ops->mc2_mce = k8_mc2_mce;
 		break;
 
 	case 0x15:
 		xec_mask = 0x1f;
 		fam_ops->mc0_mce = f15h_mc0_mce;
 		fam_ops->mc1_mce = f15h_mc1_mce;
+		fam_ops->mc2_mce = f15h_mc2_mce;
 		break;
 
 	default:
diff --git a/drivers/edac/mce_amd.h b/drivers/edac/mce_amd.h
index 6796799..000f6e2 100644
--- a/drivers/edac/mce_amd.h
+++ b/drivers/edac/mce_amd.h
@@ -78,6 +78,7 @@ extern const char * const ii_msgs[];
 struct amd_decoder_ops {
 	bool (*mc0_mce)(u16, u8);
 	bool (*mc1_mce)(u16, u8);
+	bool (*mc2_mce)(u16, u8);
 };
 
 void amd_report_gart_errors(bool);
-- 
1.7.9.5

[PATCH 2/2] MCE, AMD: MCE decoding support for AMD Family 16h

[Jacob Shin]

Add MCE decoding logic for AMD Family 16h processors.

Signed-off-by: Jacob Shin <jacob.shin@amd.com>
---
 drivers/edac/mce_amd.c |   83 +++++++++++++++++++++++++++++++++++++++++-------
 drivers/edac/mce_amd.h |    3 ++
 2 files changed, 75 insertions(+), 11 deletions(-)

diff --git a/drivers/edac/mce_amd.c b/drivers/edac/mce_amd.c
index e4752be..ce4c8b7 100644
--- a/drivers/edac/mce_amd.c
+++ b/drivers/edac/mce_amd.c
@@ -64,6 +64,10 @@ EXPORT_SYMBOL_GPL(to_msgs);
 const char * const ii_msgs[] = { "MEM", "RESV", "IO", "GEN" };
 EXPORT_SYMBOL_GPL(ii_msgs);
 
+/* internal error type */
+const char * const uu_msgs[] = { "RESV", "RESV", "HWA", "RESV" };
+EXPORT_SYMBOL_GPL(uu_msgs);
+
 static const char * const f15h_mc1_mce_desc[] = {
 	"UC during a demand linefill from L2",
 	"Parity error during data load from IC",
@@ -176,7 +180,7 @@ static bool k8_mc0_mce(u16 ec, u8 xec)
 	return f10h_mc0_mce(ec, xec);
 }
 
-static bool f14h_mc0_mce(u16 ec, u8 xec)
+static bool cat_mc0_mce(u16 ec, u8 xec)
 {
 	u8 r4	 = R4(ec);
 	bool ret = true;
@@ -330,22 +334,27 @@ static bool k8_mc1_mce(u16 ec, u8 xec)
 	return ret;
 }
 
-static bool f14h_mc1_mce(u16 ec, u8 xec)
+static bool cat_mc1_mce(u16 ec, u8 xec)
 {
 	u8 r4    = R4(ec);
 	bool ret = true;
 
 	if (MEM_ERROR(ec)) {
-		if (TT(ec) != 0 || LL(ec) != 1)
+		if (TT(ec) != TT_INSTR)
 			ret = false;
-
-		if (r4 == R4_IRD)
+		else if (r4 == R4_IRD)
 			pr_cont("Data/tag array parity error for a tag hit.\n");
 		else if (r4 == R4_SNOOP)
 			pr_cont("Tag error during snoop/victimization.\n");
+		else if (xec == 0x0)
+			pr_cont("Tag parity error from victim castout.\n");
+		else if (xec == 0x2)
+			pr_cont("Microcode patch RAM parity error.\n");
 		else
 			ret = false;
-	}
+	} else
+		ret = false;
+
 	return ret;
 }
 
@@ -469,6 +478,47 @@ static bool f15h_mc2_mce(u16 ec, u8 xec)
 	return ret;
 }
 
+static bool f16h_mc2_mce(u16 ec, u8 xec)
+{
+	u8 r4 = R4(ec);
+
+	if (!MEM_ERROR(ec))
+		return false;
+
+	switch (xec) {
+	case 0x04 ... 0x05:
+		pr_cont("%cBUFF parity error.\n", (r4 == R4_RD) ? 'I' : 'O');
+		break;
+
+	case 0x09 ... 0x0b:
+	case 0x0d ... 0x0f:
+		pr_cont("ECC error in L2 tag (%s).\n",
+			((r4 == R4_GEN)   ? "BankReq" :
+			((r4 == R4_SNOOP) ? "Prb"     : "Fill")));
+		break;
+
+	case 0x10 ... 0x19:
+	case 0x1b:
+		pr_cont("ECC error in L2 data array (%s).\n",
+			(((r4 == R4_RD) && !(xec & 0x3)) ? "Hit"  :
+			((r4 == R4_GEN)   ? "Attr" :
+			((r4 == R4_EVICT) ? "Vict" : "Fill"))));
+		break;
+
+	case 0x1c ... 0x1d:
+	case 0x1f:
+		pr_cont("Parity error in L2 attribute bits (%s).\n",
+			((r4 == R4_RD)  ? "Hit"  :
+			((r4 == R4_GEN) ? "Attr" : "Fill")));
+		break;
+
+	default:
+		return false;
+	}
+
+	return true;
+}
+
 static void decode_mc2_mce(struct mce *m)
 {
 	u16 ec = EC(m->status);
@@ -546,7 +596,7 @@ static void decode_mc4_mce(struct mce *m)
 		return;
 
 	case 0x19:
-		if (boot_cpu_data.x86 == 0x15)
+		if (boot_cpu_data.x86 == 0x15 || boot_cpu_data.x86 == 0x16)
 			pr_cont("Compute Unit Data Error.\n");
 		else
 			goto wrong_mc4_mce;
@@ -632,6 +682,10 @@ static void decode_mc6_mce(struct mce *m)
 
 static inline void amd_decode_err_code(u16 ec)
 {
+	if (INT_ERROR(ec)) {
+		pr_emerg(HW_ERR "internal: %s\n", UU_MSG(ec));
+		return;
+	}
 
 	pr_emerg(HW_ERR "cache level: %s", LL_MSG(ec));
 
@@ -736,7 +790,7 @@ int amd_decode_mce(struct notifier_block *nb, unsigned long val, void *data)
 		((m->status & MCI_STATUS_PCC)	? "PCC"	  : "-"),
 		((m->status & MCI_STATUS_ADDRV)	? "AddrV" : "-"));
 
-	if (c->x86 == 0x15)
+	if (c->x86 == 0x15 || c->x86 == 0x16)
 		pr_cont("|%s|%s",
 			((m->status & MCI_STATUS_DEFERRED) ? "Deferred" : "-"),
 			((m->status & MCI_STATUS_POISON)   ? "Poison"   : "-"));
@@ -768,7 +822,7 @@ static int __init mce_amd_init(void)
 	if (c->x86_vendor != X86_VENDOR_AMD)
 		return 0;
 
-	if (c->x86 < 0xf || c->x86 > 0x15)
+	if (c->x86 < 0xf || c->x86 > 0x16)
 		return 0;
 
 	fam_ops = kzalloc(sizeof(struct amd_decoder_ops), GFP_KERNEL);
@@ -802,8 +856,8 @@ static int __init mce_amd_init(void)
 
 	case 0x14:
 		nb_err_cpumask  = 0x3;
-		fam_ops->mc0_mce = f14h_mc0_mce;
-		fam_ops->mc1_mce = f14h_mc1_mce;
+		fam_ops->mc0_mce = cat_mc0_mce;
+		fam_ops->mc1_mce = cat_mc1_mce;
 		fam_ops->mc2_mce = k8_mc2_mce;
 		break;
 
@@ -814,6 +868,13 @@ static int __init mce_amd_init(void)
 		fam_ops->mc2_mce = f15h_mc2_mce;
 		break;
 
+	case 0x16:
+		xec_mask = 0x1f;
+		fam_ops->mc0_mce = cat_mc0_mce;
+		fam_ops->mc1_mce = cat_mc1_mce;
+		fam_ops->mc2_mce = f16h_mc2_mce;
+		break;
+
 	default:
 		printk(KERN_WARNING "Huh? What family is it: 0x%x?!\n", c->x86);
 		kfree(fam_ops);
diff --git a/drivers/edac/mce_amd.h b/drivers/edac/mce_amd.h
index 000f6e2..2828908 100644
--- a/drivers/edac/mce_amd.h
+++ b/drivers/edac/mce_amd.h
@@ -14,6 +14,7 @@
 #define TLB_ERROR(x)			(((x) & 0xFFF0) == 0x0010)
 #define MEM_ERROR(x)			(((x) & 0xFF00) == 0x0100)
 #define BUS_ERROR(x)			(((x) & 0xF800) == 0x0800)
+#define INT_ERROR(x)			(((x) & 0xF4FF) == 0x0400)
 
 #define TT(x)				(((x) >> 2) & 0x3)
 #define TT_MSG(x)			tt_msgs[TT(x)]
@@ -25,6 +26,8 @@
 #define TO_MSG(x)			to_msgs[TO(x)]
 #define PP(x)				(((x) >> 9) & 0x3)
 #define PP_MSG(x)			pp_msgs[PP(x)]
+#define UU(x)				(((x) >> 8) & 0x3)
+#define UU_MSG(x)			uu_msgs[UU(x)]
 
 #define R4(x)				(((x) >> 4) & 0xf)
 #define R4_MSG(x)			((R4(x) < 9) ?  rrrr_msgs[R4(x)] : "Wrong R4!")
-- 
1.7.9.5

Re: [PATCH 2/2] MCE, AMD: MCE decoding support for AMD Family 16h

[Joe Perches]

On Tue, 2012-12-18 at 15:06 -0600, Jacob Shin wrote:
> Add MCE decoding logic for AMD Family 16h processors.

More trivia:

> diff --git a/drivers/edac/mce_amd.c b/drivers/edac/mce_amd.c
[]
> @@ -64,6 +64,10 @@ EXPORT_SYMBOL_GPL(to_msgs);
>  const char * const ii_msgs[] = { "MEM", "RESV", "IO", "GEN" };
>  EXPORT_SYMBOL_GPL(ii_msgs);
>  
> +/* internal error type */
> +const char * const uu_msgs[] = { "RESV", "RESV", "HWA", "RESV" };
> +EXPORT_SYMBOL_GPL(uu_msgs);

Do these really need to be exported?
These don't seem to be used outside of mce_amd.

If these really need export, are these very good names?
Perhaps these should be prefixed with something?

Re: [PATCH 2/2] MCE, AMD: MCE decoding support for AMD Family 16h

[Borislav Petkov]

On Tue, Dec 18, 2012 at 02:10:59PM -0800, Joe Perches wrote:
> On Tue, 2012-12-18 at 15:06 -0600, Jacob Shin wrote:
> > Add MCE decoding logic for AMD Family 16h processors.
> 
> More trivia:
> 
> > diff --git a/drivers/edac/mce_amd.c b/drivers/edac/mce_amd.c
> []
> > @@ -64,6 +64,10 @@ EXPORT_SYMBOL_GPL(to_msgs);
> >  const char * const ii_msgs[] = { "MEM", "RESV", "IO", "GEN" };
> >  EXPORT_SYMBOL_GPL(ii_msgs);
> >  
> > +/* internal error type */
> > +const char * const uu_msgs[] = { "RESV", "RESV", "HWA", "RESV" };
> > +EXPORT_SYMBOL_GPL(uu_msgs);
> 
> Do these really need to be exported?
> These don't seem to be used outside of mce_amd.
> 
> If these really need export, are these very good names?
> Perhaps these should be prefixed with something?

Hmm, some of those were used in amd64_edac at some point but then the
whole decoding code got concentrated into mce_amd.c but the exports
remained.

PP() and thus pp_msgs is still used there, for example.

So, if you'd like, you could wait until I apply Jacob's patches and push
that git branch to kernel.org and then write a patch ontop removing all
the exports except pp_msgs (you could add them to the "amd_" namespace
by calling it amd_pp_msgs or something) test that patch and send it to
me.

Thanks.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH 2/2] MCE, AMD: MCE decoding support for AMD Family 16h

[Borislav Petkov]

On Tue, Dec 18, 2012 at 11:56:10PM +0100, Borislav Petkov wrote:
> So, if you'd like, you could wait until I apply Jacob's patches and
> push that git branch to kernel.org and then write a patch ontop
> removing all the exports except pp_msgs (you could add them to the
> "amd_" namespace by calling it amd_pp_msgs or something) test that
> patch and send it to me.

Ok, I've applied Jacob's stuff. You can do your patch ontop of

git://git.kernel.org/pub/scm/linux/kernel/git/bp/bp.git edac-for-3.9

and send it to me.

Thanks.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH V2 0/2] MCE, AMD: MCE decoding support for AMD Family 16h

[Borislav Petkov]

On Tue, Dec 18, 2012 at 03:06:09PM -0600, Jacob Shin wrote:
> The following patchset enables MCE decoding support for AMD Family 16h
> processors.
> 
> Changes in V2:
> * Changed if/else style and pr_cont usage per feedback from:
>   https://lkml.org/lkml/2012/12/17/365
> 
> * Merged f14h and f16h mc0 and mc2 decoding into common function per
>   feedback from: https://lkml.org/lkml/2012/12/18/269
> 
> * Fixed typo in INT_ERROR decoding.
> 
> Jacob Shin (2):
>   MCE, AMD: Make MC2 decoding part of amd_decoder_ops as well
>   MCE, AMD: MCE decoding support for AMD Family 16h

Ok, applied and pushed out to:

git://git.kernel.org/pub/scm/linux/kernel/git/bp/bp.git edac-for-3.9

Please take a look and give them a run.

Also, I have a python injection script with which you could test
your decoding code on F16h - no real MCEs are getting injected, but
software-only. Let me know if you want it and I'll dust it off and send
it to you along with instructions on how to use it.

Thanks.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

[PATCH 1/2] MCE, AMD: Make MC2 decoding part of amd_decoder_ops as well

[Jacob Shin]

Currently only AMD Family 15h processors have special handling for MC2
errors, since upcoming Family 16h will also need unique handling,
let's make MC2 handling part of amd_decoder_ops.

Signed-off-by: Jacob Shin <jacob.shin@amd.com>
---
 drivers/edac/mce_amd.c |   58 ++++++++++++++++++++++++++----------------------
 drivers/edac/mce_amd.h |    1 +
 2 files changed, 32 insertions(+), 27 deletions(-)

diff --git a/drivers/edac/mce_amd.c b/drivers/edac/mce_amd.c
index ad63757..84320f9 100644
--- a/drivers/edac/mce_amd.c
+++ b/drivers/edac/mce_amd.c
@@ -399,12 +399,9 @@ static void decode_mc1_mce(struct mce *m)
 		pr_emerg(HW_ERR "Corrupted MC1 MCE info?\n");
 }
 
-static void decode_mc2_mce(struct mce *m)
+static bool k8_mc2_mce(u16 ec, u8 xec)
 {
-	u16 ec = EC(m->status);
-	u8 xec = XEC(m->status, xec_mask);
-
-	pr_emerg(HW_ERR "MC2 Error");
+	bool ret = true;
 
 	if (xec == 0x1)
 		pr_cont(" in the write data buffers.\n");
@@ -429,24 +426,18 @@ static void decode_mc2_mce(struct mce *m)
 				pr_cont(": %s parity/ECC error during data "
 					"access from L2.\n", R4_MSG(ec));
 			else
-				goto wrong_mc2_mce;
+				ret = false;
 		} else
-			goto wrong_mc2_mce;
+			ret = false;
 	} else
-		goto wrong_mc2_mce;
-
-	return;
+		ret = false;
 
- wrong_mc2_mce:
-	pr_emerg(HW_ERR "Corrupted MC2 MCE info?\n");
+	return ret;
 }
 
-static void decode_f15_mc2_mce(struct mce *m)
+static bool f15h_mc2_mce(u16 ec, u8 xec)
 {
-	u16 ec = EC(m->status);
-	u8 xec = XEC(m->status, xec_mask);
-
-	pr_emerg(HW_ERR "MC2 Error: ");
+	bool ret = true;
 
 	if (TLB_ERROR(ec)) {
 		if (xec == 0x0)
@@ -454,10 +445,10 @@ static void decode_f15_mc2_mce(struct mce *m)
 		else if (xec == 0x1)
 			pr_cont("Poison data provided for TLB fill.\n");
 		else
-			goto wrong_f15_mc2_mce;
+			ret = false;
 	} else if (BUS_ERROR(ec)) {
 		if (xec > 2)
-			goto wrong_f15_mc2_mce;
+			ret = false;
 
 		pr_cont("Error during attempted NB data read.\n");
 	} else if (MEM_ERROR(ec)) {
@@ -471,14 +462,24 @@ static void decode_f15_mc2_mce(struct mce *m)
 			break;
 
 		default:
-			goto wrong_f15_mc2_mce;
+			ret = false;
 		}
 	}
 
-	return;
+	return ret;
+}
 
- wrong_f15_mc2_mce:
-	pr_emerg(HW_ERR "Corrupted MC2 MCE info?\n");
+static void decode_mc2_mce(struct mce *m)
+{
+	u16 ec = EC(m->status);
+	u8 xec = XEC(m->status, xec_mask);
+
+	pr_emerg(HW_ERR "MC2 Error: ");
+
+	if (fam_ops->mc2_mce(ec, xec))
+		;
+	else
+		pr_emerg(HW_ERR "Corrupted MC2 MCE info?\n");
 }
 
 static void decode_mc3_mce(struct mce *m)
@@ -702,10 +703,7 @@ int amd_decode_mce(struct notifier_block *nb, unsigned long val, void *data)
 		break;
 
 	case 2:
-		if (c->x86 == 0x15)
-			decode_f15_mc2_mce(m);
-		else
-			decode_mc2_mce(m);
+		decode_mc2_mce(m);
 		break;
 
 	case 3:
@@ -783,33 +781,39 @@ static int __init mce_amd_init(void)
 	case 0xf:
 		fam_ops->mc0_mce = k8_mc0_mce;
 		fam_ops->mc1_mce = k8_mc1_mce;
+		fam_ops->mc2_mce = k8_mc2_mce;
 		break;
 
 	case 0x10:
 		fam_ops->mc0_mce = f10h_mc0_mce;
 		fam_ops->mc1_mce = k8_mc1_mce;
+		fam_ops->mc2_mce = k8_mc2_mce;
 		break;
 
 	case 0x11:
 		fam_ops->mc0_mce = k8_mc0_mce;
 		fam_ops->mc1_mce = k8_mc1_mce;
+		fam_ops->mc2_mce = k8_mc2_mce;
 		break;
 
 	case 0x12:
 		fam_ops->mc0_mce = f12h_mc0_mce;
 		fam_ops->mc1_mce = k8_mc1_mce;
+		fam_ops->mc2_mce = k8_mc2_mce;
 		break;
 
 	case 0x14:
 		nb_err_cpumask  = 0x3;
 		fam_ops->mc0_mce = f14h_mc0_mce;
 		fam_ops->mc1_mce = f14h_mc1_mce;
+		fam_ops->mc2_mce = k8_mc2_mce;
 		break;
 
 	case 0x15:
 		xec_mask = 0x1f;
 		fam_ops->mc0_mce = f15h_mc0_mce;
 		fam_ops->mc1_mce = f15h_mc1_mce;
+		fam_ops->mc2_mce = f15h_mc2_mce;
 		break;
 
 	default:
diff --git a/drivers/edac/mce_amd.h b/drivers/edac/mce_amd.h
index 6796799..000f6e2 100644
--- a/drivers/edac/mce_amd.h
+++ b/drivers/edac/mce_amd.h
@@ -78,6 +78,7 @@ extern const char * const ii_msgs[];
 struct amd_decoder_ops {
 	bool (*mc0_mce)(u16, u8);
 	bool (*mc1_mce)(u16, u8);
+	bool (*mc2_mce)(u16, u8);
 };
 
 void amd_report_gart_errors(bool);
-- 
1.7.9.5

Re: [PATCH 1/2] MCE, AMD: Make MC2 decoding part of amd_decoder_ops as well

[Joe Perches]

On Mon, 2012-12-17 at 13:39 -0600, Jacob Shin wrote:
> Currently only AMD Family 15h processors have special handling for MC2
> errors, since upcoming Family 16h will also need unique handling,
> let's make MC2 handling part of amd_decoder_ops.

[]

> diff --git a/drivers/edac/mce_amd.c b/drivers/edac/mce_amd.c
[]
> @@ -399,12 +399,9 @@ static void decode_mc1_mce(struct mce *m)
>  		pr_emerg(HW_ERR "Corrupted MC1 MCE info?\n");
>  }
>  
> -static void decode_mc2_mce(struct mce *m)
> +static bool k8_mc2_mce(u16 ec, u8 xec)
>  {
> -	u16 ec = EC(m->status);
> -	u8 xec = XEC(m->status, xec_mask);
> -
> -	pr_emerg(HW_ERR "MC2 Error");
> +	bool ret = true;
>  
>  	if (xec == 0x1)
>  		pr_cont(" in the write data buffers.\n");

It'd be better to change the pr_cont uses to pr_emerg

[]

> +static void decode_mc2_mce(struct mce *m)
> +{
> +	u16 ec = EC(m->status);
> +	u8 xec = XEC(m->status, xec_mask);
> +
> +	pr_emerg(HW_ERR "MC2 Error: ");

Remove this and

> +	if (fam_ops->mc2_mce(ec, xec))
> +		;
> +	else
> +		pr_emerg(HW_ERR "Corrupted MC2 MCE info?\n");
>  }

And make this

	if (!fam_ops->mc2_mce(ec, xec))
		pr_emerg(etc...);

Re: [PATCH 1/2] MCE, AMD: Make MC2 decoding part of amd_decoder_ops as well

[Borislav Petkov]

On Mon, Dec 17, 2012 at 11:57:01AM -0800, Joe Perches wrote:
> It'd be better to change the pr_cont uses to pr_emerg
> 
> []
> 
> > +static void decode_mc2_mce(struct mce *m)
> > +{
> > +	u16 ec = EC(m->status);
> > +	u8 xec = XEC(m->status, xec_mask);
> > +
> > +	pr_emerg(HW_ERR "MC2 Error: ");
> 
> Remove this and
> 
> > +	if (fam_ops->mc2_mce(ec, xec))
> > +		;
> > +	else
> > +		pr_emerg(HW_ERR "Corrupted MC2 MCE info?\n");
> >  }
> 
> And make this
> 
> 	if (!fam_ops->mc2_mce(ec, xec))
> 		pr_emerg(etc...);

No, this is not how we do this here. We do pr_emerg in the main per-bank
function, i.e. mc0, mc1, mc2... and we finish the line in the respective
function with pr_cont.

If your fear is line interleaving, then this shouldn't happen in most
cases because we're in atomic #MC context and nothing else is executing
in that case. (And I haven't seen it interleave in all my testing so
far).

If it does interleave when we have a non-critical error detected and
reported in process context, then this whole decoding code needs a lot
more work than this.

Thanks.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH 1/2] MCE, AMD: Make MC2 decoding part of amd_decoder_ops as well

[Joe Perches]

On Mon, 2012-12-17 at 21:05 +0100, Borislav Petkov wrote:
> On Mon, Dec 17, 2012 at 11:57:01AM -0800, Joe Perches wrote:
> > It'd be better to change the pr_cont uses to pr_emerg
> > 
> > []
> > 
> > > +static void decode_mc2_mce(struct mce *m)
> > > +{
> > > +	u16 ec = EC(m->status);
> > > +	u8 xec = XEC(m->status, xec_mask);
> > > +
> > > +	pr_emerg(HW_ERR "MC2 Error: ");

> > 
> > Remove this and
> > 
> > > +	if (fam_ops->mc2_mce(ec, xec))
> > > +		;
> > > +	else
> > > +		pr_emerg(HW_ERR "Corrupted MC2 MCE info?\n");
> > >  }
> > 
> > And make this
> > 
> > 	if (!fam_ops->mc2_mce(ec, xec))
> > 		pr_emerg(etc...);
> 
> No, this is not how we do this here. We do pr_emerg in the main per-bank
> function, i.e. mc0, mc1, mc2... and we finish the line in the respective
> function with pr_cont.
> 
> If your fear is line interleaving, then this shouldn't happen in most
> cases because we're in atomic #MC context and nothing else is executing
> in that case. (And I haven't seen it interleave in all my testing so
> far).
> 
> If it does interleave when we have a non-critical error detected and
> reported in process context, then this whole decoding code needs a lot
> more work than this.
> 

You also have paths where you start a pr_emerg and do not terminate
it when there is no apparent error at all.

Re: [PATCH 1/2] MCE, AMD: Make MC2 decoding part of amd_decoder_ops as well

[Borislav Petkov]

On Mon, Dec 17, 2012 at 12:16:16PM -0800, Joe Perches wrote:
> You also have paths where you start a pr_emerg and do not terminate it
> when there is no apparent error at all.

For example?

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH 1/2] MCE, AMD: Make MC2 decoding part of amd_decoder_ops as well

[Joe Perches]

On Mon, 2012-12-17 at 21:22 +0100, Borislav Petkov wrote:
> On Mon, Dec 17, 2012 at 12:16:16PM -0800, Joe Perches wrote:
> > You also have paths where you start a pr_emerg and do not terminate it
> > when there is no apparent error at all.
> 
> For example?
> 

decode_mc2_mce does

	pr_emerg(HW_ERR "MC2 Error: ");
calls
	fam_ops->mc2_mce()
which may not emit anything
and if it doesn't, does
	pr_emerg(HW_ERR "Corrupted ...");
on another line leaving a trailing colon without
newline from the pr_emerg above.

I believe the second pr_emerg should be pr_cont

cheers, Joe

Re: [PATCH 1/2] MCE, AMD: Make MC2 decoding part of amd_decoder_ops as well

[Borislav Petkov]

On Mon, Dec 17, 2012 at 12:34:35PM -0800, Joe Perches wrote:
> decode_mc2_mce does
> 
> 	pr_emerg(HW_ERR "MC2 Error: ");
> calls
> 	fam_ops->mc2_mce()
> which may not emit anything
> and if it doesn't, does
> 	pr_emerg(HW_ERR "Corrupted ...");
> on another line leaving a trailing colon without
> newline from the pr_emerg above.

Ok, this is in Jacob's patch AFAICT, I'll review those tomorrow but
thanks for the heads-up.

> I believe the second pr_emerg should be pr_cont

Actually, it should be

pr_emerg(HW_ERR "\nCorrupted...\n");

to finish the initial pr_emerg.

Thanks.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH 1/2] MCE, AMD: Make MC2 decoding part of amd_decoder_ops as well

[Jacob Shin]

On Mon, Dec 17, 2012 at 09:40:11PM +0100, Borislav Petkov wrote:
> On Mon, Dec 17, 2012 at 12:34:35PM -0800, Joe Perches wrote:
> > decode_mc2_mce does
> > 
> > 	pr_emerg(HW_ERR "MC2 Error: ");
> > calls
> > 	fam_ops->mc2_mce()
> > which may not emit anything
> > and if it doesn't, does
> > 	pr_emerg(HW_ERR "Corrupted ...");
> > on another line leaving a trailing colon without
> > newline from the pr_emerg above.
> 
> Ok, this is in Jacob's patch AFAICT, I'll review those tomorrow but
> thanks for the heads-up.

Okay, I'll wait until your feedback before sending V2

> 
> > I believe the second pr_emerg should be pr_cont
> 
> Actually, it should be
> 
> pr_emerg(HW_ERR "\nCorrupted...\n");
> 
> to finish the initial pr_emerg.

Okay, all of the other decode_mc*_mce functions needs to change then.

Thanks!

> 
> Thanks.
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> Sent from a fat crate under my desk. Formatting is fine.
> --
> 

Re: [PATCH 1/2] MCE, AMD: Make MC2 decoding part of amd_decoder_ops as well

[Joe Perches]

On Mon, 2012-12-17 at 14:43 -0600, Jacob Shin wrote:
> On Mon, Dec 17, 2012 at 09:40:11PM +0100, Borislav Petkov wrote:
> > On Mon, Dec 17, 2012 at 12:34:35PM -0800, Joe Perches wrote:
> > > decode_mc2_mce does
> > > 
> > > 	pr_emerg(HW_ERR "MC2 Error: ");
> > > calls
> > > 	fam_ops->mc2_mce()
> > > which may not emit anything
> > > and if it doesn't, does
> > > 	pr_emerg(HW_ERR "Corrupted ...");
> > > on another line leaving a trailing colon without
> > > newline from the pr_emerg above.
> > 
> > Ok, this is in Jacob's patch AFAICT, I'll review those tomorrow but
> > thanks for the heads-up.
> 
> Okay, I'll wait until your feedback before sending V2
> 
> > 
> > > I believe the second pr_emerg should be pr_cont
> > 
> > Actually, it should be
> > 
> > pr_emerg(HW_ERR "\nCorrupted...\n");
> > 
> > to finish the initial pr_emerg.
> 
> Okay, all of the other decode_mc*_mce functions needs to change then.

Hi Jacob.

I don't think that's necessary.

Using any pr_<level>("\n...") is just not good form.
I think the pr_cont would be fine, it's the same
form as all the other decoded continuations.

The new pr_emerg is effectively a termination of the
previous one.  There's no need for 2 consecutive uses
of pr_emerg(HW_ERR ...) here.

cheers, Joe

Re: [PATCH 1/2] MCE, AMD: Make MC2 decoding part of amd_decoder_ops as well

[Borislav Petkov]

On Mon, Dec 17, 2012 at 12:59:00PM -0800, Joe Perches wrote:
> The new pr_emerg is effectively a termination of the
> previous one.  There's no need for 2 consecutive uses
> of pr_emerg(HW_ERR ...) here.

Actually, in decode_mc5_mce(), the second pr_emerg needs to stay like
what it is now.

So Jacob, please leave those changes out, I'll sort them out later.

Thanks.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH 1/2] MCE, AMD: Make MC2 decoding part of amd_decoder_ops as well

[Jacob Shin]

On Mon, Dec 17, 2012 at 10:08:02PM +0100, Borislav Petkov wrote:
> On Mon, Dec 17, 2012 at 12:59:00PM -0800, Joe Perches wrote:
> > The new pr_emerg is effectively a termination of the
> > previous one.  There's no need for 2 consecutive uses
> > of pr_emerg(HW_ERR ...) here.
> 
> Actually, in decode_mc5_mce(), the second pr_emerg needs to stay like
> what it is now.
> 
> So Jacob, please leave those changes out, I'll sort them out later.

Okay, will do, I'll change fix it in my changes then. But I'll wait
for your feedback tomorrow, before sending out V2.

Thanks!

-Jacob

> 
> Thanks.
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> Sent from a fat crate under my desk. Formatting is fine.
> --
> 

Re: [PATCH 1/2] MCE, AMD: Make MC2 decoding part of amd_decoder_ops as well

[Jacob Shin]

On Mon, Dec 17, 2012 at 12:59:00PM -0800, Joe Perches wrote:
> On Mon, 2012-12-17 at 14:43 -0600, Jacob Shin wrote:
> > On Mon, Dec 17, 2012 at 09:40:11PM +0100, Borislav Petkov wrote:
> > > On Mon, Dec 17, 2012 at 12:34:35PM -0800, Joe Perches wrote:
> > > > decode_mc2_mce does
> > > > 
> > > > 	pr_emerg(HW_ERR "MC2 Error: ");
> > > > calls
> > > > 	fam_ops->mc2_mce()
> > > > which may not emit anything
> > > > and if it doesn't, does
> > > > 	pr_emerg(HW_ERR "Corrupted ...");
> > > > on another line leaving a trailing colon without
> > > > newline from the pr_emerg above.
> > > 
> > > Ok, this is in Jacob's patch AFAICT, I'll review those tomorrow but
> > > thanks for the heads-up.
> > 
> > Okay, I'll wait until your feedback before sending V2
> > 
> > > 
> > > > I believe the second pr_emerg should be pr_cont
> > > 
> > > Actually, it should be
> > > 
> > > pr_emerg(HW_ERR "\nCorrupted...\n");
> > > 
> > > to finish the initial pr_emerg.
> > 
> > Okay, all of the other decode_mc*_mce functions needs to change then.
> 
> Hi Jacob.
> 
> I don't think that's necessary.
> 
> Using any pr_<level>("\n...") is just not good form.
> I think the pr_cont would be fine, it's the same
> form as all the other decoded continuations.
> 
> The new pr_emerg is effectively a termination of the
> previous one.  There's no need for 2 consecutive uses
> of pr_emerg(HW_ERR ...) here.

Ah, okay, I'll use pr_cont. Thanks!

-Jacob

> 
> cheers, Joe
> 
>