Fix EDAC userspace ABI

From what it was planned originally for this patch series, a few

From 50e9a89aad7045909780d635d73ab2893f8c1f90 Mon Sep 17 00:00:00 2001
From: Mauro Carvalho Chehab <[email protected]>
Date: Thu, 9 Feb 2012 11:05:20 -0300
Subject: [PATCH] edac: Initialize the dimm label with the known information

While userspace doesn't fill the dimm labels, add there the dimm location, as described by the used memory model. This could eventually match what is described at the dmidecode, making easier for people to identify the

memory.

For example, on an Intel motherboard, the memory is described as:

Memory Device
Array Handle: 0x0029
Error Information Handle: Not Provided
Total Width: 64 bits
Data Width: 64 bits
Size: 2048 MB
Form Factor: DIMM
Set: 1
Locator: A1_DIMM0
Bank Locator: A1_Node0_Channel0_Dimm0
Type: <OUT OF SPEC>
Type Detail: Synchronous
Speed: 800 MHz
Manufacturer: A1_Manufacturer0
Serial Number: A1_SerNum0
Asset Tag: A1_AssetTagNum0
Part Number: A1_PartNum0

After this patch, the memory label will be filled with:

	/sys/devices/system/edac/mc/mc0/dimm0/dimm_label:mc#0channel#0slot#0

With somewhat matches what it is at the Bank Locator DMI information.

So, it is easier to associate the dimm labels, of course assuming that the DMI has the Bank Locator filled, and the BIOS doesn't have any bugs.

Yet, even without it, several motherboards are provided with enough info to map from channel/slot (or branch/channel/slot) into the DIMM label. So, letting the EDAC core fill it, by default is a good thing.

It should noticed that, as the label filling happens at the edac_mc_alloc(), drivers can override it to better describe the memories (and some actually do it).

Cc Aristeu Rozanski <[email protected]>

Cc Doug Thompson <[email protected]>

Signed-off-by Mauro Carvalho Chehab <[email protected]>

---
 drivers/edac/edac_mc.c       |   25 +++++++++++++++++++------
 drivers/edac/edac_mc_sysfs.c |    8 ++++----
 include/linux/edac.h         |    2 +-
 3 files changed, 24 insertions(+), 11 deletions(-)

drivers/edac/edac_mc.c +2 -2

index 4c44cd298c0b..77263b33b7f0 100644
@@ -210,10 +210,10 @@ struct mem_ctl_info *edac_mc_alloc(unsigned mc_num,
 	struct dimm_info *dimm;
 	u32 *ce_per_layer[EDAC_MAX_LAYERS], *ue_per_layer[EDAC_MAX_LAYERS];
 	unsigned pos[EDAC_MAX_LAYERS];
-	void *pvt, *ptr = NULL;
 	unsigned size, tot_dimms = 1, count = 1;
 	unsigned tot_csrows = 1, tot_channels = 1, tot_errcount = 0;
-	int i, j, err, row, chn;
+	void *pvt, *p, *ptr = NULL;
+	int i, j, err, row, chn, n, len;
 	bool per_rank = false;
 
 	BUG_ON(n_layers > EDAC_MAX_LAYERS || n_layers == 0);

Adding latest version here:

Quoted 8 lines

From 50e9a89aad7045909780d635d73ab2893f8c1f90 Mon Sep 17 00:00:00 2001
From: Mauro Carvalho Chehab <[email protected]>
Date: Thu, 9 Feb 2012 11:05:20 -0300
Subject: [PATCH] edac: Initialize the dimm label with the known information

While userspace doesn't fill the dimm labels, add there the dimm location, as described by the used memory model. This could eventually match what is described at the dmidecode, making easier for people to identify the

in making it easier

Quoted 1 line

memory.

stick in error.

Quoted 20 lines

For example, on an Intel motherboard, the memory is described as:

Memory Device
Array Handle: 0x0029
Error Information Handle: Not Provided
Total Width: 64 bits
Data Width: 64 bits
Size: 2048 MB
Form Factor: DIMM
Set: 1
Locator: A1_DIMM0
Bank Locator: A1_Node0_Channel0_Dimm0
Type: <OUT OF SPEC>
Type Detail: Synchronous
Speed: 800 MHz
Manufacturer: A1_Manufacturer0
Serial Number: A1_SerNum0
Asset Tag: A1_AssetTagNum0
Part Number: A1_PartNum0

Quoted 2 lines

After this patch, the memory label will be filled with:

	/sys/devices/system/edac/mc/mc0/dimm0/dimm_label:mc#0channel#0slot#0

This is only with the Intel-MCs, right, I still have the csrows here:

tree /sys/devices/system/edac/mc/
/sys/devices/system/edac/mc/

|-- mc0
|   |-- ce_count
|   |-- ce_noinfo_count
|   |-- csrow0
|   |   |-- ce_count
|   |   |-- ch0_ce_count
|   |   |-- ch0_dimm_label
|   |   |-- ch1_ce_count
|   |   |-- ch1_dimm_label
|   |   |-- dev_type
|   |   |-- edac_mode
|   |   |-- mem_type
|   |   |-- size_mb
|   |   `-- ue_count
|   |-- csrow1
|   |   |-- ce_count
|   |   |-- ch0_ce_count
|   |   |-- ch0_dimm_label
|   |   |-- ch1_ce_count
|   |   |-- ch1_dimm_label
|   |   |-- dev_type
|   |   |-- edac_mode
|   |   |-- mem_type
|   |   |-- size_mb
|   |   `-- ue_count
|   |-- csrow2
|   |   |-- ce_count
|   |   |-- ch0_ce_count
|   |   |-- ch0_dimm_label
|   |   |-- ch1_ce_count
|   |   |-- ch1_dimm_label
|   |   |-- dev_type
|   |   |-- edac_mode
|   |   |-- mem_type
|   |   |-- size_mb
|   |   `-- ue_count

…

Quoted 1 line

With somewhat matches what it is at the Bank Locator DMI information.

I wouldn't say that - DMI is notoriously unreliable, let's look at some boxes:

1st box:

Handle 0x0038, DMI type 17, 28 bytes
Memory Device
Array Handle: 0x0036
Error Information Handle: Not Provided
Total Width: 72 bits
Data Width: 64 bits
Size: 2048 MB
Form Factor: DIMM
Set: None
Locator: DIMM0

2nd box:

Memory Device
Array Handle: 0x0014
Error Information Handle: Not Provided
Total Width: 64 bits
Data Width: 4096 bits
Size: 9 MB
Form Factor: <OUT OF SPEC>
Set: 73
Locator: P0_DIMM_A1
Bank Locator: CHANNEL A

3rd box:

Handle 0x0033, DMI type 17, 28 bytes
Memory Device
Array Handle: 0x0031
Error Information Handle: Not Provided
Total Width: 64 bits
Data Width: 64 bits
Size: 4096 MB
Form Factor: SODIMM
Set: 2
Locator: J401
Bank Locator: Channel B

and so on.

IOW, DMI fields are almost random permutations of [a-zA-Z0-9].

Quoted 10 lines

So, it is easier to associate the dimm labels, of course assuming that the DMI has the Bank Locator filled, and the BIOS doesn't have any bugs.

Yet, even without it, several motherboards are provided with enough info to map from channel/slot (or branch/channel/slot) into the DIMM label. So, letting the EDAC core fill it, by default is a good thing.

It should noticed that, as the label filling happens at the edac_mc_alloc(), drivers can override it to better describe the memories (and some actually do it).

But I guess having the info is still fine, simply remove the DMI references in the commit message pls.

Quoted 27 lines

Cc Aristeu Rozanski <[email protected]>

Cc Doug Thompson <[email protected]>

Signed-off-by Mauro Carvalho Chehab <[email protected]>

---
 drivers/edac/edac_mc.c       |   25 +++++++++++++++++++------
 drivers/edac/edac_mc_sysfs.c |    8 ++++----
 include/linux/edac.h         |    2 +-
 3 files changed, 24 insertions(+), 11 deletions(-)

drivers/edac/edac_mc.c +2 -2

index 4c44cd298c0b..77263b33b7f0 100644
@@ -210,10 +210,10 @@ struct mem_ctl_info *edac_mc_alloc(unsigned mc_num,
 	struct dimm_info *dimm;
 	u32 *ce_per_layer[EDAC_MAX_LAYERS], *ue_per_layer[EDAC_MAX_LAYERS];
 	unsigned pos[EDAC_MAX_LAYERS];
-	void *pvt, *ptr = NULL;
 	unsigned size, tot_dimms = 1, count = 1;
 	unsigned tot_csrows = 1, tot_channels = 1, tot_errcount = 0;
-	int i, j, err, row, chn;
+	void *pvt, *p, *ptr = NULL;
+	int i, j, err, row, chn, n, len;
 	bool per_rank = false;
 
 	BUG_ON(n_layers > EDAC_MAX_LAYERS || n_layers == 0);

drivers/edac/edac_mc.c struct mem_ctl_info *edac_mc_alloc(unsigned mc_num, · line 329

Quoted 7 lines

drivers/edac/edac_mc.c +2 -2

@@ -325,9 +325,22 @@ struct mem_ctl_info *edac_mc_alloc(unsigned mc_num,
 			i, per_rank ? "rank" : "dimm", (dimm - mci->dimms),
 			pos[0], pos[1], pos[2], row, chn);
 
-		/* Copy DIMM location */
-		for (j = 0; j < n_layers; j++)
+		/*
+		 * Copy DIMM location and initialize the memory location

initialize it.

or do you mean two different locations?

drivers/edac/edac_mc.c struct mem_ctl_info *edac_mc_alloc(unsigned edac_index, · line 327

Quoted 12 lines

drivers/edac/edac_mc.c +12 -0

+		 */
+		len = sizeof(dimm->label);
+		p = dimm->label;
+		n = snprintf(p, len, "mc#%u", mc_num);
+		p += n;
+		len -= n;
+		for (j = 0; j < n_layers; j++) {
+			n = snprintf(p, len, "%s#%u",
+				     edac_layer_name[layers[j].type],
+				     pos[j]);
+			p += n;
+			len -= n;

Err, you're not checking how much len is left here, i.e. EDAC_MC_LABEL_LEN. Or even better, each time before you do snprintf.

drivers/edac/edac_mc.c static void edac_inc_ce_error(struct mem_ctl_info *mci, · line 853

Quoted 11 lines

drivers/edac/edac_mc.c +2 -1

 			dimm->location[j] = pos[j];
+		}
 
 		/* Link it to the csrows old API data */
 		chan->dimm = dimm;

@@ -837,7 +850,7 @@ static void edac_inc_ce_error(struct mem_ctl_info *mci,
 {
 	int i, index = 0;
 
-	mci->ce_count++;
+	mci->ce_mc++;

Oh, renaming them back, ok.

<rest snipped>

Review comments to the one below are still not addressed, maybe slipped through the cracks?

Quoted 11 lines

Adding latest version here:

Quoted 8 lines

From 50e9a89aad7045909780d635d73ab2893f8c1f90 Mon Sep 17 00:00:00 2001
From: Mauro Carvalho Chehab <[email protected]>
Date: Thu, 9 Feb 2012 11:05:20 -0300
Subject: [PATCH] edac: Initialize the dimm label with the known information

While userspace doesn't fill the dimm labels, add there the dimm location, as described by the used memory model. This could eventually match what is described at the dmidecode, making easier for people to identify the

Quoted 5 lines

in making it easier

Quoted 1 line

memory.

stick in error.

Quoted 21 lines

Quoted 21 lines

For example, on an Intel motherboard, the memory is described as:

Memory Device
Array Handle: 0x0029
Error Information Handle: Not Provided
Total Width: 64 bits
Data Width: 64 bits
Size: 2048 MB
Form Factor: DIMM
Set: 1
Locator: A1_DIMM0
Bank Locator: A1_Node0_Channel0_Dimm0
Type: <OUT OF SPEC>
Type Detail: Synchronous
Speed: 800 MHz
Manufacturer: A1_Manufacturer0
Serial Number: A1_SerNum0
Asset Tag: A1_AssetTagNum0
Part Number: A1_PartNum0

Quoted 4 lines

Quoted 2 lines

After this patch, the memory label will be filled with:

	/sys/devices/system/edac/mc/mc0/dimm0/dimm_label:mc#0channel#0slot#0

This is only with the Intel-MCs, right, I still have the csrows here:

Quoted 87 lines

tree /sys/devices/system/edac/mc/
/sys/devices/system/edac/mc/

|-- mc0
|   |-- ce_count
|   |-- ce_noinfo_count
|   |-- csrow0
|   |   |-- ce_count
|   |   |-- ch0_ce_count
|   |   |-- ch0_dimm_label
|   |   |-- ch1_ce_count
|   |   |-- ch1_dimm_label
|   |   |-- dev_type
|   |   |-- edac_mode
|   |   |-- mem_type
|   |   |-- size_mb
|   |   `-- ue_count
|   |-- csrow1
|   |   |-- ce_count
|   |   |-- ch0_ce_count
|   |   |-- ch0_dimm_label
|   |   |-- ch1_ce_count
|   |   |-- ch1_dimm_label
|   |   |-- dev_type
|   |   |-- edac_mode
|   |   |-- mem_type
|   |   |-- size_mb
|   |   `-- ue_count
|   |-- csrow2
|   |   |-- ce_count
|   |   |-- ch0_ce_count
|   |   |-- ch0_dimm_label
|   |   |-- ch1_ce_count
|   |   |-- ch1_dimm_label
|   |   |-- dev_type
|   |   |-- edac_mode
|   |   |-- mem_type
|   |   |-- size_mb
|   |   `-- ue_count

…

Quoted 1 line

With somewhat matches what it is at the Bank Locator DMI information.

I wouldn't say that - DMI is notoriously unreliable, let's look at some boxes:

1st box:

Handle 0x0038, DMI type 17, 28 bytes
Memory Device
Array Handle: 0x0036
Error Information Handle: Not Provided
Total Width: 72 bits
Data Width: 64 bits
Size: 2048 MB
Form Factor: DIMM
Set: None
Locator: DIMM0

2nd box:

Memory Device
Array Handle: 0x0014
Error Information Handle: Not Provided
Total Width: 64 bits
Data Width: 4096 bits
Size: 9 MB
Form Factor: <OUT OF SPEC>
Set: 73
Locator: P0_DIMM_A1
Bank Locator: CHANNEL A

3rd box:

Handle 0x0033, DMI type 17, 28 bytes
Memory Device
Array Handle: 0x0031
Error Information Handle: Not Provided
Total Width: 64 bits
Data Width: 64 bits
Size: 4096 MB
Form Factor: SODIMM
Set: 2
Locator: J401
Bank Locator: Channel B

Quoted 1 line

and so on.

Quoted 2 lines

IOW, DMI fields are almost random permutations of [a-zA-Z0-9].

Quoted 8 lines

Quoted 8 lines

So, it is easier to associate the dimm labels, of course assuming that the DMI has the Bank Locator filled, and the BIOS doesn't have any bugs.

Yet, even without it, several motherboards are provided with enough info to map from channel/slot (or branch/channel/slot) into the DIMM label. So, letting the EDAC core fill it, by default is a good thing.

Quoted 6 lines

Quoted 3 lines

It should noticed that, as the label filling happens at the edac_mc_alloc(), drivers can override it to better describe the memories (and some actually do it).

But I guess having the info is still fine, simply remove the DMI references in the commit message pls.

Quoted 31 lines

Quoted 31 lines

Cc Aristeu Rozanski <[email protected]>

Cc Doug Thompson <[email protected]>

Signed-off-by Mauro Carvalho Chehab <[email protected]>

---
 drivers/edac/edac_mc.c       |   25 +++++++++++++++++++------
 drivers/edac/edac_mc_sysfs.c |    8 ++++----
 include/linux/edac.h         |    2 +-
 3 files changed, 24 insertions(+), 11 deletions(-)

drivers/edac/edac_mc.c +2 -2

index 4c44cd298c0b..77263b33b7f0 100644
@@ -210,10 +210,10 @@ struct mem_ctl_info *edac_mc_alloc(unsigned mc_num,
 	struct dimm_info *dimm;
 	u32 *ce_per_layer[EDAC_MAX_LAYERS], *ue_per_layer[EDAC_MAX_LAYERS];
 	unsigned pos[EDAC_MAX_LAYERS];
-	void *pvt, *ptr = NULL;
 	unsigned size, tot_dimms = 1, count = 1;
 	unsigned tot_csrows = 1, tot_channels = 1, tot_errcount = 0;
-	int i, j, err, row, chn;
+	void *pvt, *p, *ptr = NULL;
+	int i, j, err, row, chn, n, len;
 	bool per_rank = false;
 
 	BUG_ON(n_layers > EDAC_MAX_LAYERS || n_layers == 0);

@@ -325,9 +325,22 @@ struct mem_ctl_info *edac_mc_alloc(unsigned mc_num,
 			i, per_rank ? "rank" : "dimm", (dimm - mci->dimms),
 			pos[0], pos[1], pos[2], row, chn);
 

Quoted 8 lines

drivers/edac/edac_mc.c struct mem_ctl_info *edac_mc_alloc(unsigned mc_num, · line 329

Quoted 4 lines

drivers/edac/edac_mc.c +2 -2

-		/* Copy DIMM location */
-		for (j = 0; j < n_layers; j++)
+		/*
+		 * Copy DIMM location and initialize the memory location

initialize it.

or do you mean two different locations?

Quoted 16 lines

drivers/edac/edac_mc.c struct mem_ctl_info *edac_mc_alloc(unsigned mc_num, · line 341

Quoted 13 lines

drivers/edac/edac_mc.c +12 -0

+		 */
+		len = sizeof(dimm->label);
+		p = dimm->label;
+		n = snprintf(p, len, "mc#%u", mc_num);
+		p += n;
+		len -= n;
+		for (j = 0; j < n_layers; j++) {
+			n = snprintf(p, len, "%s#%u",
+				     edac_layer_name[layers[j].type],
+				     pos[j]);
+			p += n;
+			len -= n;

Err, you're not checking how much len is left here, i.e. EDAC_MC_LABEL_LEN. Or even better, each time before you do snprintf.

Quoted 10 lines

Quoted 10 lines

drivers/edac/edac_mc.c +1 -0

 			dimm->location[j] = pos[j];
+		}
 
 		/* Link it to the csrows old API data */
 		chan->dimm = dimm;

@@ -837,7 +850,7 @@ static void edac_inc_ce_error(struct mem_ctl_info *mci,
 {
 	int i, index = 0;
 

Quoted 4 lines

drivers/edac/edac_mc.c static void edac_inc_ce_error(struct mem_ctl_info *mci, · line 853

Quoted 2 lines

drivers/edac/edac_mc.c +1 -1

-	mci->ce_count++;
+	mci->ce_mc++;

Oh, renaming them back, ok.

Quoted 2 lines

<rest snipped>

From 4afb0250415e87b983f5937d456c83407fe96264 Mon Sep 17 00:00:00 2001
From: Mauro Carvalho Chehab <[email protected]>
Date: Thu, 23 Feb 2012 08:10:34 -0300
Subject: [PATCH] events/hw_event: Create a Hardware Events Report Mecanism
(HERM)

Adds a trace class for handle hardware events

Part of the description bellow is shamelessly copied from Tony Luck's notes about the Hardware Error BoF during LPC 2010 [1]. Tony, thanks for your notes and discussions to generate the h/w error reporting requirements.

[1] http://lwn.net/Articles/416669/

We have several subsystems & methods for reporting hardware errors:

1) EDAC ("Error Detection and Correction"). In its original form this consisted of a platform specific driver that read topology information and error counts from chipset registers and reported the results via a sysfs interface.

2) mcelog - x86 specific decoding of machine check bank registers reporting in binary form via /dev/mcelog. Recent additions make use of the APEI extensions that were documented in version 4.0a of the ACPI specification to acquire more information about errors without having to rely reading chipset registers directly. A user level programs decodes into somewhat human readable format.

3) drivers/edac/mce_amd.c - this driver hooks into the mcelog path and decodes errors reported via machine check bank registers in AMD processors to the console log using printk();

Each of these mechanisms has a band of followers ... and none of them appear to meet all the needs of all users.

In order to provide a proper hardware event subsystem, let's

encapsulate the memory error hardware events into a trace facility.

As no agreement was reached so far for the MCA-based trace events, for now, let's add events only for memory errors. A latter patch can change the tracepoint, for events originated via MCA.

Reviewed-by Aristeu Rozanski <[email protected]>

Cc Doug Thompson <[email protected]>

Cc Steven Rostedt <[email protected]>

Cc Frederic Weisbecker <[email protected]>

Cc Ingo Molnar <[email protected]>

Signed-off-by Mauro Carvalho Chehab <[email protected]>

---
 drivers/edac/edac_core.h        |    2 +-
 drivers/edac/edac_mc.c          |   26 +++++++---
 include/trace/events/hw_event.h |  107 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 127 insertions(+), 8 deletions(-)
 create mode 100644 include/trace/events/hw_event.h

drivers/edac/edac_core.h +1 -1

index f06ce9ab692c..eee73605c5a0 100644
@@ -468,7 +468,7 @@ void edac_mc_handle_error(const enum hw_event_mc_err_type type,
 			  const int layer2,
 			  const char *msg,
 			  const char *other_detail,
-			  const void *mcelog);
+			  const void *arch_log);
 
 /*
  * edac_device APIs

drivers/edac/edac_mc.c +3 -0

index e5b55632359f..c75774dcf434 100644
@@ -33,6 +33,9 @@
 #include "edac_core.h"
 #include "edac_module.h"
 
+#define CREATE_TRACE_POINTS
+#include <trace/events/hw_event.h>
+
 /* lock to memory controller's control array */
 static DEFINE_MUTEX(mem_ctls_mutex);
 static LIST_HEAD(mc_devices);

drivers/edac/edac_mc.c +15 -7

+	trace_hw_event_init("edac", (unsigned)mc_num);
+
 	return mci;
 }
 EXPORT_SYMBOL_GPL(edac_mc_alloc);

@@ -982,7 +988,7 @@ void edac_mc_handle_error(const enum hw_event_mc_err_type type,
 			  const int layer2,
 			  const char *msg,
 			  const char *other_detail,
-			  const void *mcelog)
+			  const void *arch_log)
 {
 	/* FIXME: too much for stack: move it to some pre-alocated area */
 	char detail[80], location[80];

@@ -1119,21 +1125,27 @@ void edac_mc_handle_error(const enum hw_event_mc_err_type type,
 	}
 
 	/* Memory type dependent details about the error */
-	if (type == HW_EVENT_ERR_CORRECTED) {
+	if (type == HW_EVENT_ERR_CORRECTED)
 		snprintf(detail, sizeof(detail),
 			"page:0x%lx offset:0x%lx grain:%d syndrome:0x%lx",
 			page_frame_number, offset_in_page,
 			grain, syndrome);
-		edac_ce_error(mci, pos, msg, location, label, detail,
-			      other_detail, enable_per_layer_report,
-			      page_frame_number, offset_in_page, grain);
-	} else {
+	else
 		snprintf(detail, sizeof(detail),
 			"page:0x%lx offset:0x%lx grain:%d",
 			page_frame_number, offset_in_page, grain);
 
+	/* Report the error via the trace interface */
+	trace_mc_error(type, mci->mc_idx, msg, label, location,
+		       detail, other_detail);
+
+	/* Report the error via the edac_mc_printk() interface */
+	if (type == HW_EVENT_ERR_CORRECTED)
+		edac_ce_error(mci, pos, msg, location, label, detail,
+			      other_detail, enable_per_layer_report,
+			      page_frame_number, offset_in_page, grain);
+	else
 		edac_ue_error(mci, pos, msg, location, label, detail,
 			      other_detail, enable_per_layer_report);
-	}
 }
 EXPORT_SYMBOL_GPL(edac_mc_handle_error);

include/trace/events/hw_event.h new file +12 -0

new file mode 100644
index 000000000000..1fabfe21e29a
@@ -0,0 +1,107 @@
+#undef TRACE_SYSTEM
+#define TRACE_SYSTEM hw_event
+
+#if !defined(_TRACE_HW_EVENT_MC_H) || defined(TRACE_HEADER_MULTI_READ)
+#define _TRACE_HW_EVENT_MC_H
+
+#include <linux/tracepoint.h>
+#include <linux/edac.h>
+#include <linux/ktime.h>
+
+/*
+ * Hardware Anomaly Report Mecanism (HARM) events

include/trace/events/hw_event.h new file +25 -0

+	TP_PROTO(const char *type, unsigned int instance),
+	TP_ARGS(type, instance),
+
+	TP_STRUCT__entry(
+		__string(	type,		type			)
+		__field(	unsigned int,	instance		)
+	),
+
+	TP_fast_assign(
+		__assign_str(type, type);
+		__entry->instance = instance;
+	),
+
+	TP_printk("Initialized %s#%d\n",
+		__get_str(type),
+		__entry->instance)
+);
+
+/*
+ * This event indicates that a hardware collection mechanism is started
+ */
+DEFINE_EVENT(hw_event_class, hw_event_init,
+
+	TP_PROTO(const char *type, unsigned int instance),
+

include/trace/events/hw_event.h new file +25 -0

+ */
+TRACE_EVENT(mc_error,
+
+	TP_PROTO(const unsigned int err_type,
+		 const unsigned int mc_index,
+		 const char *msg,
+		 const char *label,
+		 const char *location,
+		 const char *detail,
+		 const char *driver_detail),
+
+	TP_ARGS(err_type, mc_index, msg, label, location,
+		detail, driver_detail),
+
+	TP_STRUCT__entry(
+		__field(	unsigned int,	err_type		)
+		__field(	unsigned int,	mc_index		)
+		__string(	msg,		msg			)
+		__string(	label,		label			)
+		__string(	detail,		detail			)
+		__string(	location,	location		)
+		__string(	driver_detail,	driver_detail		)
+	),
+
+	TP_fast_assign(

include/trace/events/hw_event.h new file +15 -0

+		  __entry->mc_index,
+		  (__entry->err_type == HW_EVENT_ERR_CORRECTED) ? "Corrected" :
+			((__entry->err_type == HW_EVENT_ERR_FATAL) ?
+			"Fatal" : "Uncorrected"),
+		  __get_str(msg),
+		  __get_str(label),
+		  __get_str(location),
+		  __get_str(detail),
+		  __get_str(driver_detail))
+);
+
+#endif /* _TRACE_HW_EVENT_MC_H */
+
+/* This part must be outside protection */
+#include <trace/define_trace.h>

Signature · 1 line

1.7.9.3.362.g71319

Inserting the latest version:

Quoted 5 lines

From 4afb0250415e87b983f5937d456c83407fe96264 Mon Sep 17 00:00:00 2001
From: Mauro Carvalho Chehab <[email protected]>
Date: Thu, 23 Feb 2012 08:10:34 -0300
Subject: [PATCH] events/hw_event: Create a Hardware Events Report Mecanism
(HERM)

Ok, let's face it: this is just a single trace_mc_error tracepoint, nothing else. Let's drop the HERM bullshit bingo and call the thing by it's name: "Add yet another tracepoint to report DRAM ECC errors".

Quoted 27 lines

Adds a trace class for handle hardware events

Part of the description bellow is shamelessly copied from Tony Luck's notes about the Hardware Error BoF during LPC 2010 [1]. Tony, thanks for your notes and discussions to generate the h/w error reporting requirements.

[1] http://lwn.net/Articles/416669/

We have several subsystems & methods for reporting hardware errors:

1) EDAC ("Error Detection and Correction"). In its original form this consisted of a platform specific driver that read topology information and error counts from chipset registers and reported the results via a sysfs interface.

2) mcelog - x86 specific decoding of machine check bank registers reporting in binary form via /dev/mcelog. Recent additions make use of the APEI extensions that were documented in version 4.0a of the ACPI specification to acquire more information about errors without having to rely reading chipset registers directly. A user level programs decodes into somewhat human readable format.

3) drivers/edac/mce_amd.c - this driver hooks into the mcelog path and decodes errors reported via machine check bank registers in AMD processors to the console log using printk();

Quoted 4 lines

Each of these mechanisms has a band of followers ... and none of them appear to meet all the needs of all users.

In order to provide a proper hardware event subsystem, let's

err no, this is not a proper hw event subsystem.

Quoted 6 lines

encapsulate the memory error hardware events into a trace facility.

As no agreement was reached so far for the MCA-based trace events, for now, let's add events only for memory errors. A latter patch can change the tracepoint, for events originated via MCA.

This last paragraph has nothing to do with the patch so it can go.

Quoted 35 lines

Reviewed-by Aristeu Rozanski <[email protected]>

Cc Doug Thompson <[email protected]>

Cc Steven Rostedt <[email protected]>

Cc Frederic Weisbecker <[email protected]>

Cc Ingo Molnar <[email protected]>

Signed-off-by Mauro Carvalho Chehab <[email protected]>

---
 drivers/edac/edac_core.h        |    2 +-
 drivers/edac/edac_mc.c          |   26 +++++++---
 include/trace/events/hw_event.h |  107 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 127 insertions(+), 8 deletions(-)
 create mode 100644 include/trace/events/hw_event.h

drivers/edac/edac_core.h +1 -1

index f06ce9ab692c..eee73605c5a0 100644
@@ -468,7 +468,7 @@ void edac_mc_handle_error(const enum hw_event_mc_err_type type,
 			  const int layer2,
 			  const char *msg,
 			  const char *other_detail,
-			  const void *mcelog);
+			  const void *arch_log);
 
 /*
  * edac_device APIs

drivers/edac/edac_mc.c +0 -0

index e5b55632359f..c75774dcf434 100644
@@ -33,6 +33,9 @@
 #include "edac_core.h"
 #include "edac_module.h"
 

drivers/edac/edac_mc.c struct mem_ctl_info *edac_mc_alloc(unsigned mc_num, · line 387

Quoted 11 lines

drivers/edac/edac_mc.c +4 -0

+#define CREATE_TRACE_POINTS
+#include <trace/events/hw_event.h>
+
 /* lock to memory controller's control array */
 static DEFINE_MUTEX(mem_ctls_mutex);
 static LIST_HEAD(mc_devices);

@@ -381,6 +384,9 @@ struct mem_ctl_info *edac_mc_alloc(unsigned mc_num,
 	 * which will perform kobj unregistration and the actual free
 	 * will occur during the kobject callback operation
 	 */
+

superfluous newline.

Quoted 73 lines

drivers/edac/edac_mc.c +15 -7

+	trace_hw_event_init("edac", (unsigned)mc_num);
+
 	return mci;
 }
 EXPORT_SYMBOL_GPL(edac_mc_alloc);

@@ -982,7 +988,7 @@ void edac_mc_handle_error(const enum hw_event_mc_err_type type,
 			  const int layer2,
 			  const char *msg,
 			  const char *other_detail,
-			  const void *mcelog)
+			  const void *arch_log)
 {
 	/* FIXME: too much for stack: move it to some pre-alocated area */
 	char detail[80], location[80];

@@ -1119,21 +1125,27 @@ void edac_mc_handle_error(const enum hw_event_mc_err_type type,
 	}
 
 	/* Memory type dependent details about the error */
-	if (type == HW_EVENT_ERR_CORRECTED) {
+	if (type == HW_EVENT_ERR_CORRECTED)
 		snprintf(detail, sizeof(detail),
 			"page:0x%lx offset:0x%lx grain:%d syndrome:0x%lx",
 			page_frame_number, offset_in_page,
 			grain, syndrome);
-		edac_ce_error(mci, pos, msg, location, label, detail,
-			      other_detail, enable_per_layer_report,
-			      page_frame_number, offset_in_page, grain);
-	} else {
+	else
 		snprintf(detail, sizeof(detail),
 			"page:0x%lx offset:0x%lx grain:%d",
 			page_frame_number, offset_in_page, grain);
 
+	/* Report the error via the trace interface */
+	trace_mc_error(type, mci->mc_idx, msg, label, location,
+		       detail, other_detail);
+
+	/* Report the error via the edac_mc_printk() interface */
+	if (type == HW_EVENT_ERR_CORRECTED)
+		edac_ce_error(mci, pos, msg, location, label, detail,
+			      other_detail, enable_per_layer_report,
+			      page_frame_number, offset_in_page, grain);
+	else
 		edac_ue_error(mci, pos, msg, location, label, detail,
 			      other_detail, enable_per_layer_report);
-	}
 }
 EXPORT_SYMBOL_GPL(edac_mc_handle_error);

include/trace/events/hw_event.h new file +18 -0

new file mode 100644
index 000000000000..1fabfe21e29a
@@ -0,0 +1,107 @@
+#undef TRACE_SYSTEM
+#define TRACE_SYSTEM hw_event
+
+#if !defined(_TRACE_HW_EVENT_MC_H) || defined(TRACE_HEADER_MULTI_READ)
+#define _TRACE_HW_EVENT_MC_H
+
+#include <linux/tracepoint.h>
+#include <linux/edac.h>
+#include <linux/ktime.h>
+
+/*
+ * Hardware Anomaly Report Mecanism (HARM) events
+ *
+ * Those events are generated when hardware detected a corrected or
+ * uncorrected event, and are meant to replace the current API to report
+ * errors defined on both EDAC and MCE subsystems.
+ *
+ * FIXME: Add events for handling memory errors originated from the

include/trace/events/hw_event.h line 22

Quoted 4 lines

include/trace/events/hw_event.h new file +4 -0

+ *        MCE subsystem.
+ */
+
+DECLARE_EVENT_CLASS(hw_event_class,

Ok, event classes are for sharing tracepoints which have the same TP_PROTO, TP_ARGS.. etc arguments as Steven's (CCed) article on lwn points out.

I don't see this here and besides, why in the hell would you need a trace event which only announces that the mechanism starts?? A common, run-of-the-mill printk is more than enough here.

Quoted 30 lines

include/trace/events/hw_event.h new file +29 -0

+	TP_PROTO(const char *type, unsigned int instance),
+	TP_ARGS(type, instance),
+
+	TP_STRUCT__entry(
+		__string(	type,		type			)
+		__field(	unsigned int,	instance		)
+	),
+
+	TP_fast_assign(
+		__assign_str(type, type);
+		__entry->instance = instance;
+	),
+
+	TP_printk("Initialized %s#%d\n",
+		__get_str(type),
+		__entry->instance)
+);
+
+/*
+ * This event indicates that a hardware collection mechanism is started
+ */
+DEFINE_EVENT(hw_event_class, hw_event_init,
+
+	TP_PROTO(const char *type, unsigned int instance),
+
+	TP_ARGS(type, instance)
+);
+
+

include/trace/events/hw_event.h line 57

Quoted 6 lines

include/trace/events/hw_event.h new file +6 -0

+/*
+ * Hardware-independent Memory Controller specific events
+ */
+
+/*
+ * Default error mechanisms for Memory Controller errors (CE and UE)

DRAM ECC errors.

Quoted 30 lines

include/trace/events/hw_event.h new file +29 -0

+ */
+TRACE_EVENT(mc_error,
+
+	TP_PROTO(const unsigned int err_type,
+		 const unsigned int mc_index,
+		 const char *msg,
+		 const char *label,
+		 const char *location,
+		 const char *detail,
+		 const char *driver_detail),
+
+	TP_ARGS(err_type, mc_index, msg, label, location,
+		detail, driver_detail),
+
+	TP_STRUCT__entry(
+		__field(	unsigned int,	err_type		)
+		__field(	unsigned int,	mc_index		)
+		__string(	msg,		msg			)
+		__string(	label,		label			)
+		__string(	detail,		detail			)
+		__string(	location,	location		)
+		__string(	driver_detail,	driver_detail		)
+	),
+
+	TP_fast_assign(
+		__entry->err_type		= err_type;
+		__entry->mc_index		= mc_index;
+		__assign_str(msg, msg);
+		__assign_str(label, label);

include/trace/events/hw_event.h line 92

Quoted 6 lines

include/trace/events/hw_event.h new file +6 -0

+		__assign_str(location, location);
+		__assign_str(detail, detail);
+		__assign_str(driver_detail, driver_detail);
+	),
+
+	TP_printk(HW_ERR "mce#%d: %s error %s on label \"%s\" (%s %s %s)",

memory stick/DIMM

Quoted 10 lines

include/trace/events/hw_event.h new file +9 -0

+		  __entry->mc_index,
+		  (__entry->err_type == HW_EVENT_ERR_CORRECTED) ? "Corrected" :
+			((__entry->err_type == HW_EVENT_ERR_FATAL) ?
+			"Fatal" : "Uncorrected"),
+		  __get_str(msg),
+		  __get_str(label),
+		  __get_str(location),
+		  __get_str(detail),
+		  __get_str(driver_detail))

Quoted 8 lines

include/trace/events/hw_event.h new file +6 -0

+);
+
+#endif /* _TRACE_HW_EVENT_MC_H */
+
+/* This part must be outside protection */
+#include <trace/define_trace.h>

Signature · 1 line

1.7.9.3.362.g71319

Em 09-05-2012 09:13, Borislav Petkov escreveu:

Quoted 8 lines

Inserting the latest version:

Quoted 5 lines

From 4afb0250415e87b983f5937d456c83407fe96264 Mon Sep 17 00:00:00 2001
From: Mauro Carvalho Chehab <[email protected]>
Date: Thu, 23 Feb 2012 08:10:34 -0300
Subject: [PATCH] events/hw_event: Create a Hardware Events Report Mecanism
(HERM)

Quoted 3 lines

Ok, let's face it: this is just a single trace_mc_error tracepoint, nothing else. Let's drop the HERM bullshit bingo and call the thing by it's name: "Add yet another tracepoint to report DRAM ECC errors".

This name is nice and helps to distinguish this mechanism among others, and will help to distinguish between HARM-aware userspace tools from the existing ones.

Quoted 24 lines

Quoted 24 lines

include/trace/events/hw_event.h new file +18 -0

new file mode 100644
index 000000000000..1fabfe21e29a
@@ -0,0 +1,107 @@
+#undef TRACE_SYSTEM
+#define TRACE_SYSTEM hw_event
+
+#if !defined(_TRACE_HW_EVENT_MC_H) || defined(TRACE_HEADER_MULTI_READ)
+#define _TRACE_HW_EVENT_MC_H
+
+#include <linux/tracepoint.h>
+#include <linux/edac.h>
+#include <linux/ktime.h>
+
+/*
+ * Hardware Anomaly Report Mecanism (HARM) events
+ *
+ * Those events are generated when hardware detected a corrected or
+ * uncorrected event, and are meant to replace the current API to report
+ * errors defined on both EDAC and MCE subsystems.
+ *
+ * FIXME: Add events for handling memory errors originated from the

Quoted 8 lines

include/trace/events/hw_event.h line 22

Quoted 4 lines

include/trace/events/hw_event.h new file +4 -0

+ *        MCE subsystem.
+ */
+
+DECLARE_EVENT_CLASS(hw_event_class,

Ok, event classes are for sharing tracepoints which have the same TP_PROTO, TP_ARGS.. etc arguments as Steven's (CCed) article on lwn points out.

Other trace mechanisms will be added. One of them is the MCA-based tracepoint, that got removed while no consensus is reached on that.

Quoted 3 lines

I don't see this here and besides, why in the hell would you need a trace event which only announces that the mechanism starts?? A common, run-of-the-mill printk is more than enough here.

A daemon monitoring this trace may need to know when the trace mechanism started, in order to what might be lost before the event init.

+ Tony.

Quoted 13 lines

Em 09-05-2012 09:13, Borislav Petkov escreveu:

Quoted 11 lines

Inserting the latest version:

Quoted 5 lines

From 4afb0250415e87b983f5937d456c83407fe96264 Mon Sep 17 00:00:00 2001
From: Mauro Carvalho Chehab <[email protected]>
Date: Thu, 23 Feb 2012 08:10:34 -0300
Subject: [PATCH] events/hw_event: Create a Hardware Events Report Mecanism
(HERM)

Ok, let's face it: this is just a single trace_mc_error tracepoint, nothing else. Let's drop the HERM bullshit bingo and call the thing by it's name: "Add yet another tracepoint to report DRAM ECC errors".

Quoted 3 lines

This name is nice and helps to distinguish this mechanism among others, and will help to distinguish between HARM-aware userspace tools from the existing ones.

This may be so but it is not a mechanism - it is simply using a kernel facility - a tracepoint - and that's it.

Now, if you really want to have a generic mechanism for RAS, used by all the kernel, then I don't have a problem with you adding it to include/ras/hw_event.h or somewhere in that vicinity along with making it generic enough for other users and then taking care of it and developing it to address users' needs.

But simply growing stuff here and there for a specific use case is not the way to go.

[ … ]

Quoted 33 lines

Quoted 32 lines

Quoted 18 lines

include/trace/events/hw_event.h new file +12 -0

new file mode 100644
index 000000000000..1fabfe21e29a
@@ -0,0 +1,107 @@
+#undef TRACE_SYSTEM
+#define TRACE_SYSTEM hw_event
+
+#if !defined(_TRACE_HW_EVENT_MC_H) || defined(TRACE_HEADER_MULTI_READ)
+#define _TRACE_HW_EVENT_MC_H
+
+#include <linux/tracepoint.h>
+#include <linux/edac.h>
+#include <linux/ktime.h>
+
+/*
+ * Hardware Anomaly Report Mecanism (HARM) events

include/trace/events/hw_event.h line 22

Quoted 10 lines

include/trace/events/hw_event.h new file +10 -0

+ *
+ * Those events are generated when hardware detected a corrected or
+ * uncorrected event, and are meant to replace the current API to report
+ * errors defined on both EDAC and MCE subsystems.
+ *
+ * FIXME: Add events for handling memory errors originated from the
+ *        MCE subsystem.
+ */
+
+DECLARE_EVENT_CLASS(hw_event_class,

Ok, event classes are for sharing tracepoints which have the same TP_PROTO, TP_ARGS.. etc arguments as Steven's (CCed) article on lwn points out.

Quoted 2 lines

Other trace mechanisms will be added. One of them is the MCA-based tracepoint, that got removed while no consensus is reached on that.

You're missing the point: are the other tracepoints using "const char *type" and "unsigned int instance" as arguments? No.

IOW, go look at http://lwn.net/Articles/381064/ for an example what the trace event class is. Hint: sched_wakeup and sched_wakeup_new.

Quoted 7 lines

Quoted 4 lines

I don't see this here and besides, why in the hell would you need a trace event which only announces that the mechanism starts?? A common, run-of-the-mill printk is more than enough here.

A daemon monitoring this trace may need to know when the trace mechanism started, in order to what might be lost before the event init.

Yes, the trace has started when the daemon reads the first sample from the buffer, no need for explicitly letting us know it has.

Quoted 5 lines

Now, if you really want to have a generic mechanism for RAS, used by all the kernel, then I don't have a problem with you adding it to include/ras/hw_event.h or somewhere in that vicinity along with making it generic enough for other users and then taking care of it and developing it to address users' needs.

I'm ok to move it to include/ras, but, if I remember well from my initial tests when I wrote this, several kernel versions ago, there is/was a limitation that required that all trace events should be under include/trace/events/.

One other option would be to call it as "include/trace/events/ras.h".

Steven,

Would it be possible/recommendable to move this trace header to include/linux/ras/hw_event.h?

In a matter of fact, my plan is to move the EDAC core to drivers/ras, after making it generic enough.

events/hw_event: use a tracepoint-based Hardware Events Report Method (HERM)

From: Mauro Carvalho Chehab <[email protected]>

Adds a trace class for handle hardware events

Part of the description bellow is shamelessly copied from Tony Luck's notes about the Hardware Error BoF during LPC 2010 [1]. Tony, thanks for your notes and discussions to generate the h/w error reporting requirements.

[1] http://lwn.net/Articles/416669/

We have several subsystems & methods for reporting hardware errors:

1) EDAC ("Error Detection and Correction"). In its original form this consisted of a platform specific driver that read topology information and error counts from chipset registers and reported the results via a sysfs interface.

2) mcelog - x86 specific decoding of machine check bank registers reporting in binary form via /dev/mcelog. Recent additions make use of the APEI extensions that were documented in version 4.0a of the ACPI specification to acquire more information about errors without having to rely reading chipset registers directly. A user level programs decodes into somewhat human readable format.

3) drivers/edac/mce_amd.c - this driver hooks into the mcelog path and decodes errors reported via machine check bank registers in AMD processors to the console log using printk();

Each of these mechanisms has a band of followers ... and none of them appear to meet all the needs of all users.

As part of a hardware event subsystem, let's encapsulate the memory error hardware events into a trace facility.

NOTE: The original patch was providing an additional mechanism for MCA-based trace events that also contained MCA error register data. Hoever, as no agreement was reached so far for the MCA-based trace events, for now, let's add events only for memory errors. A latter patch is planned to change the tracepoint, for those types of event.

Reviewed-by Aristeu Rozanski <[email protected]>

Cc Doug Thompson <[email protected]>

Cc Steven Rostedt <[email protected]>

Cc Frederic Weisbecker <[email protected]>

Cc Ingo Molnar <[email protected]>

Signed-off-by Mauro Carvalho Chehab <[email protected]>

drivers/edac/edac_core.h +1 -1

index f06ce9a..eee7360 100644
@@ -468,7 +468,7 @@ void edac_mc_handle_error(const enum hw_event_mc_err_type type,
 			  const int layer2,
 			  const char *msg,
 			  const char *other_detail,
-			  const void *mcelog);
+			  const void *arch_log);
 
 /*
  * edac_device APIs

drivers/edac/edac_mc.c +17 -7

index e5b5563..eff8266 100644
@@ -33,6 +33,9 @@
 #include "edac_core.h"
 #include "edac_module.h"
 
+#define CREATE_TRACE_POINTS
+#include <trace/events/hw_event.h>
+
 /* lock to memory controller's control array */
 static DEFINE_MUTEX(mem_ctls_mutex);
 static LIST_HEAD(mc_devices);

@@ -381,6 +384,7 @@ struct mem_ctl_info *edac_mc_alloc(unsigned mc_num,
 	 * which will perform kobj unregistration and the actual free
 	 * will occur during the kobject callback operation
 	 */
+
 	return mci;
 }
 EXPORT_SYMBOL_GPL(edac_mc_alloc);

@@ -982,7 +986,7 @@ void edac_mc_handle_error(const enum hw_event_mc_err_type type,
 			  const int layer2,
 			  const char *msg,
 			  const char *other_detail,
-			  const void *mcelog)
+			  const void *arch_log)
 {
 	/* FIXME: too much for stack: move it to some pre-alocated area */
 	char detail[80], location[80];

@@ -1119,21 +1123,27 @@ void edac_mc_handle_error(const enum hw_event_mc_err_type type,
 	}
 
 	/* Memory type dependent details about the error */
-	if (type == HW_EVENT_ERR_CORRECTED) {
+	if (type == HW_EVENT_ERR_CORRECTED)
 		snprintf(detail, sizeof(detail),
 			"page:0x%lx offset:0x%lx grain:%d syndrome:0x%lx",
 			page_frame_number, offset_in_page,
 			grain, syndrome);
-		edac_ce_error(mci, pos, msg, location, label, detail,
-			      other_detail, enable_per_layer_report,
-			      page_frame_number, offset_in_page, grain);
-	} else {
+	else
 		snprintf(detail, sizeof(detail),
 			"page:0x%lx offset:0x%lx grain:%d",
 			page_frame_number, offset_in_page, grain);
 
+	/* Report the error via the trace interface */
+	trace_mc_error(type, mci->mc_idx, msg, label, location,
+		       detail, other_detail);
+
+	/* Report the error via the edac_mc_printk() interface */
+	if (type == HW_EVENT_ERR_CORRECTED)
+		edac_ce_error(mci, pos, msg, location, label, detail,
+			      other_detail, enable_per_layer_report,
+			      page_frame_number, offset_in_page, grain);
+	else
 		edac_ue_error(mci, pos, msg, location, label, detail,
 			      other_detail, enable_per_layer_report);
-	}
 }
 EXPORT_SYMBOL_GPL(edac_mc_handle_error);

include/trace/events/hw_event.h new file +2 -0

new file mode 100644
index 0000000..234220a
@@ -0,0 +1,77 @@
+#undef TRACE_SYSTEM
+#define TRACE_SYSTEM hw_event

include/trace/events/hw_event.h new file +65 -0

+ *
+ * Those events are generated when hardware detected a corrected or
+ * uncorrected event, and are meant to replace the current API to report
+ * errors defined on both EDAC and MCE subsystems.
+ *
+ * FIXME: Add events for handling memory errors originated from the
+ *        MCE subsystem.
+ */
+
+/*
+ * Hardware-independent Memory Controller specific events
+ */
+
+/*
+ * Default error mechanisms for Memory Controller errors (CE and UE)
+ */
+TRACE_EVENT(mc_error,
+
+	TP_PROTO(const unsigned int err_type,
+		 const unsigned int mc_index,
+		 const char *msg,
+		 const char *label,
+		 const char *location,
+		 const char *detail,
+		 const char *driver_detail),
+
+	TP_ARGS(err_type, mc_index, msg, label, location,
+		detail, driver_detail),
+
+	TP_STRUCT__entry(
+		__field(	unsigned int,	err_type		)
+		__field(	unsigned int,	mc_index		)
+		__string(	msg,		msg			)
+		__string(	label,		label			)
+		__string(	detail,		detail			)
+		__string(	location,	location		)
+		__string(	driver_detail,	driver_detail		)
+	),
+
+	TP_fast_assign(
+		__entry->err_type		= err_type;
+		__entry->mc_index		= mc_index;
+		__assign_str(msg, msg);
+		__assign_str(label, label);
+		__assign_str(location, location);
+		__assign_str(detail, detail);
+		__assign_str(driver_detail, driver_detail);
+	),
+
+	TP_printk(HW_ERR "mce#%d: %s error %s on memory stick \"%s\" (%s %s %s)",
+		  __entry->mc_index,
+		  (__entry->err_type == HW_EVENT_ERR_CORRECTED) ? "Corrected" :
+			((__entry->err_type == HW_EVENT_ERR_FATAL) ?
+			"Fatal" : "Uncorrected"),
+		  __get_str(msg),
+		  __get_str(label),
+		  __get_str(location),
+		  __get_str(detail),
+		  __get_str(driver_detail))
+);
+
+#endif /* _TRACE_HW_EVENT_MC_H */
+
+/* This part must be outside protection */
+#include <trace/define_trace.h>

Quoted 13 lines

Quoted 5 lines

Now, if you really want to have a generic mechanism for RAS, used by all the kernel, then I don't have a problem with you adding it to include/ras/hw_event.h or somewhere in that vicinity along with making it generic enough for other users and then taking care of it and developing it to address users' needs.

I'm ok to move it to include/ras, but, if I remember well from my initial tests when I wrote this, several kernel versions ago, there is/was a limitation that required that all trace events should be under include/trace/events/.

One other option would be to call it as "include/trace/events/ras.h".

Quoted 4 lines

Steven,

Would it be possible/recommendable to move this trace header to include/linux/ras/hw_event.h?

Oh, yeah, I remember vaguely something like that. Lets wait what Steven has to say.

Quoted 2 lines

In a matter of fact, my plan is to move the EDAC core to drivers/ras, after making it generic enough.

Now that sounds like a very bad idea to me. Why would you do that, they're just fine under drivers/edac/ the way they are now. If there's no real reason for doing that, then unnecessary code churn is not what we want.

[ … ]

Quoted 5 lines

events/hw_event: use a tracepoint-based Hardware Events Report Method (HERM)

From: Mauro Carvalho Chehab <[email protected]>

Adds a trace class for handle hardware events

You just removed the class - what class now?

Review

Quoted 3 lines

(quoted diff) +3 -0

+
+/*
+ * Hardware Anomaly Report Mecanism (HARM) events

and above says "Hardware Events Report Method (HERM)" The fact that you yourself are not exactly sure about what this abbreviation is supposed to be/mean/etc, simply should tell you that it is causing only confusion with no benefit what-so-ever, even to you!

Em 09-05-2012 11:06, Borislav Petkov escreveu:

Quoted 21 lines · Mauro Carvalho Chehab

Quoted 11 lines

Quoted 5 lines

Now, if you really want to have a generic mechanism for RAS, used by all the kernel, then I don't have a problem with you adding it to include/ras/hw_event.h or somewhere in that vicinity along with making it generic enough for other users and then taking care of it and developing it to address users' needs.

I'm ok to move it to include/ras, but, if I remember well from my initial tests when I wrote this, several kernel versions ago, there is/was a limitation that required that all trace events should be under include/trace/events/.

Quoted 6 lines

One other option would be to call it as "include/trace/events/ras.h".

Steven,

Would it be possible/recommendable to move this trace header to include/linux/ras/hw_event.h?

Oh, yeah, I remember vaguely something like that. Lets wait what Steven has to say.

Quoted 4 lines

and above says "Hardware Events Report Method (HERM)" The fact that you yourself are not exactly sure about what this abbreviation is supposed to be/mean/etc, simply should tell you that it is causing only confusion with no benefit what-so-ever, even to you!

Fixed.

events/hw_event: use a tracepoint-based Hardware Events Report Method (HERM)

From: Mauro Carvalho Chehab <[email protected]>

Adds a trace event for handle hardware issues

Part of the description bellow is shamelessly copied from Tony Luck's notes about the Hardware Error BoF during LPC 2010 [1]. Tony, thanks for your notes and discussions to generate the h/w error reporting requirements.

[1] http://lwn.net/Articles/416669/

We have several subsystems & methods for reporting hardware errors:

1) EDAC ("Error Detection and Correction"). In its original form this consisted of a platform specific driver that read topology information and error counts from chipset registers and reported the results via a sysfs interface.

2) mcelog - x86 specific decoding of machine check bank registers reporting in binary form via /dev/mcelog. Recent additions make use of the APEI extensions that were documented in version 4.0a of the ACPI specification to acquire more information about errors without having to rely reading chipset registers directly. A user level programs decodes into somewhat human readable format.

3) drivers/edac/mce_amd.c - this driver hooks into the mcelog path and decodes errors reported via machine check bank registers in AMD processors to the console log using printk();

Each of these mechanisms has a band of followers ... and none of them appear to meet all the needs of all users.

As part of a hardware event subsystem, let's encapsulate the memory error hardware events into a trace facility.

NOTE: The original patch was providing an additional mechanism for MCA-based trace events that also contained MCA error register data. Hoever, as no agreement was reached so far for the MCA-based trace events, for now, let's add events only for memory errors. A latter patch is planned to change the tracepoint, for those types of event.

Reviewed-by Aristeu Rozanski <[email protected]>

Cc Doug Thompson <[email protected]>

Cc Steven Rostedt <[email protected]>

Cc Frederic Weisbecker <[email protected]>

Cc Ingo Molnar <[email protected]>

Signed-off-by Mauro Carvalho Chehab <[email protected]>

drivers/edac/edac_core.h +1 -1

index f06ce9a..eee7360 100644
@@ -468,7 +468,7 @@ void edac_mc_handle_error(const enum hw_event_mc_err_type type,
 			  const int layer2,
 			  const char *msg,
 			  const char *other_detail,
-			  const void *mcelog);
+			  const void *arch_log);
 
 /*
  * edac_device APIs

drivers/edac/edac_mc.c +17 -7

index e5b5563..eff8266 100644
@@ -33,6 +33,9 @@
 #include "edac_core.h"
 #include "edac_module.h"
 
+#define CREATE_TRACE_POINTS
+#include <trace/events/hw_event.h>
+
 /* lock to memory controller's control array */
 static DEFINE_MUTEX(mem_ctls_mutex);
 static LIST_HEAD(mc_devices);

@@ -381,6 +384,7 @@ struct mem_ctl_info *edac_mc_alloc(unsigned mc_num,
 	 * which will perform kobj unregistration and the actual free
 	 * will occur during the kobject callback operation
 	 */
+
 	return mci;
 }
 EXPORT_SYMBOL_GPL(edac_mc_alloc);

@@ -982,7 +986,7 @@ void edac_mc_handle_error(const enum hw_event_mc_err_type type,
 			  const int layer2,
 			  const char *msg,
 			  const char *other_detail,
-			  const void *mcelog)
+			  const void *arch_log)
 {
 	/* FIXME: too much for stack: move it to some pre-alocated area */
 	char detail[80], location[80];

@@ -1119,21 +1123,27 @@ void edac_mc_handle_error(const enum hw_event_mc_err_type type,
 	}
 
 	/* Memory type dependent details about the error */
-	if (type == HW_EVENT_ERR_CORRECTED) {
+	if (type == HW_EVENT_ERR_CORRECTED)
 		snprintf(detail, sizeof(detail),
 			"page:0x%lx offset:0x%lx grain:%d syndrome:0x%lx",
 			page_frame_number, offset_in_page,
 			grain, syndrome);
-		edac_ce_error(mci, pos, msg, location, label, detail,
-			      other_detail, enable_per_layer_report,
-			      page_frame_number, offset_in_page, grain);
-	} else {
+	else
 		snprintf(detail, sizeof(detail),
 			"page:0x%lx offset:0x%lx grain:%d",
 			page_frame_number, offset_in_page, grain);
 
+	/* Report the error via the trace interface */
+	trace_mc_error(type, mci->mc_idx, msg, label, location,
+		       detail, other_detail);
+
+	/* Report the error via the edac_mc_printk() interface */
+	if (type == HW_EVENT_ERR_CORRECTED)
+		edac_ce_error(mci, pos, msg, location, label, detail,
+			      other_detail, enable_per_layer_report,
+			      page_frame_number, offset_in_page, grain);
+	else
 		edac_ue_error(mci, pos, msg, location, label, detail,
 			      other_detail, enable_per_layer_report);
-	}
 }
 EXPORT_SYMBOL_GPL(edac_mc_handle_error);

include/trace/events/hw_event.h new file +77 -0

new file mode 100644
index 0000000..fe2ec3b
@@ -0,0 +1,77 @@
+#undef TRACE_SYSTEM
+#define TRACE_SYSTEM hw_event
+
+#if !defined(_TRACE_HW_EVENT_MC_H) || defined(TRACE_HEADER_MULTI_READ)
+#define _TRACE_HW_EVENT_MC_H
+
+#include <linux/tracepoint.h>
+#include <linux/edac.h>
+#include <linux/ktime.h>
+
+/*
+ * Hardware Events Report Method (HERM)
+ *
+ * Those events are generated when hardware detected a corrected or
+ * uncorrected event, and are meant to replace the current API to report
+ * errors defined on both EDAC and MCE subsystems.
+ *
+ * FIXME: Add events for handling memory errors originated from the
+ *        MCE subsystem.
+ */
+
+/*
+ * Hardware-independent Memory Controller specific events
+ */
+
+/*
+ * Default error mechanisms for Memory Controller errors (CE and UE)
+ */
+TRACE_EVENT(mc_error,
+
+	TP_PROTO(const unsigned int err_type,
+		 const unsigned int mc_index,
+		 const char *msg,
+		 const char *label,
+		 const char *location,
+		 const char *detail,
+		 const char *driver_detail),
+
+	TP_ARGS(err_type, mc_index, msg, label, location,
+		detail, driver_detail),
+
+	TP_STRUCT__entry(
+		__field(	unsigned int,	err_type		)
+		__field(	unsigned int,	mc_index		)
+		__string(	msg,		msg			)
+		__string(	label,		label			)
+		__string(	detail,		detail			)
+		__string(	location,	location		)
+		__string(	driver_detail,	driver_detail		)
+	),
+
+	TP_fast_assign(
+		__entry->err_type		= err_type;
+		__entry->mc_index		= mc_index;
+		__assign_str(msg, msg);
+		__assign_str(label, label);
+		__assign_str(location, location);
+		__assign_str(detail, detail);
+		__assign_str(driver_detail, driver_detail);
+	),
+
+	TP_printk(HW_ERR "mce#%d: %s error %s on memory stick \"%s\" (%s %s %s)",
+		  __entry->mc_index,
+		  (__entry->err_type == HW_EVENT_ERR_CORRECTED) ? "Corrected" :
+			((__entry->err_type == HW_EVENT_ERR_FATAL) ?
+			"Fatal" : "Uncorrected"),
+		  __get_str(msg),
+		  __get_str(label),
+		  __get_str(location),
+		  __get_str(detail),
+		  __get_str(driver_detail))
+);
+
+#endif /* _TRACE_HW_EVENT_MC_H */
+
+/* This part must be outside protection */
+#include <trace/define_trace.h>

Quoted 22 lines

Em 09-05-2012 11:06, Borislav Petkov escreveu:

Quoted 21 lines · Mauro Carvalho Chehab

Quoted 11 lines

Quoted 5 lines

Now, if you really want to have a generic mechanism for RAS, used by all the kernel, then I don't have a problem with you adding it to include/ras/hw_event.h or somewhere in that vicinity along with making it generic enough for other users and then taking care of it and developing it to address users' needs.

I'm ok to move it to include/ras, but, if I remember well from my initial tests when I wrote this, several kernel versions ago, there is/was a limitation that required that all trace events should be under include/trace/events/.

Quoted 6 lines

One other option would be to call it as "include/trace/events/ras.h".

Steven,

Would it be possible/recommendable to move this trace header to include/linux/ras/hw_event.h?

Oh, yeah, I remember vaguely something like that. Lets wait what Steven has to say.

btw, http://lwn.net/Articles/383362/ explains how to define tracepoints outside of include/trace/events.

[ … ]

Quoted 6 lines

Quoted 4 lines

and above says "Hardware Events Report Method (HERM)" The fact that you yourself are not exactly sure about what this abbreviation is supposed to be/mean/etc, simply should tell you that it is causing only confusion with no benefit what-so-ever, even to you!

Fixed.

No, you haven't. It still says HERM below. Please drop the marketing speak!

Nah, this is not marketing speak. HERM is not a trademark. amd64_edac, on the other hand, is using a trademark on his name. If we use your logic, this would need to be renamed to something else, to avoid using a "marketing speak".

We need some name to differentiate between the broken EDAC core where modern memory controllers are not properly represented and reports errors on fake csrows/channels from the EDAC+HERM core that will properly provide the error information to where it really belongs.

In other words, HERM is just an acronym[1] to the version to point to where EDAC starts to work fine with modern memory controllers.

Quoted 4 lines

Nah, this is not marketing speak. HERM is not a trademark. amd64_edac, on the other hand, is using a trademark on his name. If we use your logic, this would need to be renamed to something else, to avoid using a "marketing speak".

I'm not even going to dignify that with an answer because it is a bunch of bullshit and you know it.

Quoted 4 lines

We need some name to differentiate between the broken EDAC core where modern memory controllers are not properly represented and reports errors on fake csrows/channels from the EDAC+HERM core that will properly provide the error information to where it really belongs.

A kernel version or a commit id is not enough here I guess.

Quoted 2 lines

In other words, HERM is just an acronym[1] to the version to point to where EDAC starts to work fine with modern memory controllers.

Seems to me you're desperately trying to prove your point for having this HERM thing by coming up with a bunch of bogus arguments.

And I'm still waiting for a real, technical reason which legitimizes calling a tracepoint something special.

Technical reasons were provided already: before this patch series, EDAC is *broken*.

Em 10-05-2012 10:41, Borislav Petkov escreveu:

Quoted 19 lines · Mauro Carvalho Chehab

Quoted 4 lines

Nah, this is not marketing speak. HERM is not a trademark. amd64_edac, on the other hand, is using a trademark on his name. If we use your logic, this would need to be renamed to something else, to avoid using a "marketing speak".

I'm not even going to dignify that with an answer because it is a bunch of bullshit and you know it.

Quoted 4 lines

We need some name to differentiate between the broken EDAC core where modern memory controllers are not properly represented and reports errors on fake csrows/channels from the EDAC+HERM core that will properly provide the error information to where it really belongs.

A kernel version or a commit id is not enough here I guess.

Quoted 2 lines

In other words, HERM is just an acronym[1] to the version to point to where EDAC starts to work fine with modern memory controllers.

Quoted 5 lines

Seems to me you're desperately trying to prove your point for having this HERM thing by coming up with a bunch of bogus arguments.

And I'm still waiting for a real, technical reason which legitimizes calling a tracepoint something special.

Technical reasons were provided already: before this patch series, EDAC is *broken*.

Mauro.
--
To unsubscribe from this list: send the line "unsubscribe linux-edac" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

There's also another technical reason to give an acronym to the EDAC version that actually works: changeset numbers are not consistent within distributions (or other trees, like -stable - although this 60+ patch series probably won't fit on -stable merging criteria).

Also, this EDAC changeset 60+ patch series can't be represented by a single changeset, and requires userspace changes in order to get a proper representation model for memories.

Tagging the EDAC core version with a name helps a lot when dealing with all the unsolved bugzillas that will be closed by backporting this patch series in order to fix the serious EDAC core bug that were providing fake information to the end user for all Intel memory controllers manufactured after 2005.

Quoted 9 lines

There's also another technical reason to give an acronym to the EDAC version that actually works: changeset numbers are not consistent within distributions (or other trees, like -stable - although this 60+ patch series probably won't fit on -stable merging criteria).

Also, this EDAC changeset 60+ patch series can't be represented by a single changeset, and requires userspace changes in order to get a proper representation model for memories.

Quoted 5 lines

Tagging the EDAC core version with a name helps a lot when dealing with all the unsolved bugzillas that will be closed by backporting this patch series in order to fix the serious EDAC core bug that were providing fake information to the end user for all Intel memory controllers manufactured after 2005.

edac_module.c:18:#define EDAC_VERSION "Ver: 2.1.0"

Increment that in the last patch.

There were lots of changes introduced to justify renaming it to 3.0.0 (HERM - Hardware Events Report Method):

- EDAC core were redesigned to represent all types of

    memory controllers;

- EDAC API were redesigned to properly represent the memory

    controller hierarchy;

- a tracepoint-based API were added to report memory errors.

Signed-off-by Mauro Carvalho Chehab <[email protected]>

Quoted 8 lines · Mauro Carvalho Chehab

There's also another technical reason to give an acronym to the EDAC version that actually works: changeset numbers are not consistent within distributions (or other trees, like -stable - although this 60+ patch series probably won't fit on -stable merging criteria).

Also, this EDAC changeset 60+ patch series can't be represented by a single changeset, and requires userspace changes in order to get a proper representation model for memories.

Quoted 6 lines

Tagging the EDAC core version with a name helps a lot when dealing with all the unsolved bugzillas that will be closed by backporting this patch series in order to fix the serious EDAC core bug that were providing fake information to the end user for all Intel memory controllers manufactured after 2005.

edac_module.c:18:#define EDAC_VERSION "Ver: 2.1.0"

Increment that in the last patch.

Quoted 9 lines · Borislav Petkov

Quoted 8 lines · Mauro Carvalho Chehab

There's also another technical reason to give an acronym to the EDAC version that actually works: changeset numbers are not consistent within distributions (or other trees, like -stable - although this 60+ patch series probably won't fit on -stable merging criteria).

Also, this EDAC changeset 60+ patch series can't be represented by a single changeset, and requires userspace changes in order to get a proper representation model for memories.

Is this a redesign of EDAC or just a fix of it? Does this require userspace to use a new ABI?

Quoted 7 lines

Quoted 6 lines

Tagging the EDAC core version with a name helps a lot when dealing with all the unsolved bugzillas that will be closed by backporting this patch series in order to fix the serious EDAC core bug that were providing fake information to the end user for all Intel memory controllers manufactured after 2005.

Quoted 3 lines

edac_module.c:18:#define EDAC_VERSION "Ver: 2.1.0"

Increment that in the last patch.

If this is redesigning a subsystem and changing the ABI for userspace than a new name is appropriate. Much like ipchains turning into iptables.

Quoted 6 lines · Mauro Carvalho Chehab

Quoted 5 lines

There's also another technical reason to give an acronym to the EDAC version that actually works: changeset numbers are not consistent within distributions (or other trees, like -stable - although this 60+ patch series probably won't fit on -stable merging criteria).

Quoted 3 lines

Quoted 3 lines

Also, this EDAC changeset 60+ patch series can't be represented by a single changeset, and requires userspace changes in order to get a proper representation model for memories.

Is this a redesign of EDAC or just a fix of it?

Does this require
userspace to use a new ABI?

Quoted 7 lines

Quoted 6 lines

Tagging the EDAC core version with a name helps a lot when dealing with all the unsolved bugzillas that will be closed by backporting this patch series in order to fix the serious EDAC core bug that were providing fake information to the end user for all Intel memory controllers manufactured after 2005.

Quoted 3 lines

edac_module.c:18:#define EDAC_VERSION "Ver: 2.1.0"

Increment that in the last patch.

If this is redesigning a subsystem and changing the ABI for userspace than a new name is appropriate. Much like ipchains turning into iptables.

But if this is just fixing the subsystem where userspace sees no difference, than the same name fits.

-- Steve

Em 09-05-2012 10:22, Borislav Petkov escreveu:

Quoted 1 line

+ Tony.

C/C Steven.

One other option would be to call it as "include/trace/events/ras.h".

Steven,

Would it be possible/recommendable to move this trace header to include/linux/ras/hw_event.h?

Quoted 4 lines

Em 09-05-2012 10:22, Borislav Petkov escreveu:

Quoted 1 line

+ Tony.

C/C Steven.

Hehe, I was already Cc'd ;-)

Quoted 6 lines

One other option would be to call it as "include/trace/events/ras.h".

Steven,

Would it be possible/recommendable to move this trace header to include/linux/ras/hw_event.h?

Yeah, it's not hard. In fact, it's documented in
samples/trace_events/trace-events-sample.h:

/*
* There are several ways I could have done this. If I left out the
* TRACE_INCLUDE_PATH, then it would default to the kernel source
* include/trace/events directory.
*
* I could specify a path from the define_trace.h file back to this
* file.
*
* #define TRACE_INCLUDE_PATH ../../samples/trace_events
*
* But the safest and easiest way to simply make it use the directory
* that the file is in is to add in the Makefile:
*
* CFLAGS_trace-events-sample.o := -I$(src)
*
* This will make sure the current path is part of the include
* structure for our file so that define_trace.h can find it. *
* I could have made only the top level directory the include:
*
* CFLAGS_trace-events-sample.o := -I$(PWD)
*
* And then let the path to this directory be the TRACE_INCLUDE_PATH:
*
* #define TRACE_INCLUDE_PATH samples/trace_events
*
* But then if something defines "samples" or "trace_events" as a macro
* then we could risk that being converted too, and give us an unexpected
* result.
*/

But usually the header would go where the code is:

drivers/edac/hw_event.h

Or rename it. The XFS tracepoints do this:

fs/xfs/xfs_trace.h