Add new eMCA trace event interface

Add new eMCA trace event interface

[Chen, Gong]

[PATCH 1/5] trace, RAS: Add basic RAS trace event
[PATCH 2/5] CPER: Adjust code flow of some functions
[PATCH 3/5] trace, RAS: Add eMCA trace event interface
[PATCH 4/5] trace, eMCA: Add a knob to adjust where to save event log
[PATCH 5/5] trace, AER: Move trace into unified interface

This patch series add new eMCA trace event interface. To avoid conflict with
existed interface, a new unified trace event stub in the kernel is used.
New trace interface is mutually exclusive with console message via
a knob under debugfs. This knob is a reference counter. When it is opened,
the counter will be increased, whereas the counter will be decreased
if it is closed. Once this counter is greater than 0, the trace will be
used, otherwise, message will be routed to the console.

In the last patch, I merge older AER trace interface into new unified
trace interface.

[PATCH 1/5] trace, RAS: Add basic RAS trace event

[Chen, Gong]

To avoid the confuision of usage for RAS related trace event, add
an unified RAS trace event stub.

Signed-off-by: Chen, Gong <gong.chen@linux.intel.com>
---
 drivers/Kconfig          |  2 ++
 drivers/Makefile         |  1 +
 drivers/edac/edac_mc.c   |  3 ---
 drivers/ras/Kconfig      |  4 ++++
 drivers/ras/Makefile     |  1 +
 drivers/ras/ras-traces.c | 12 ++++++++++++
 6 files changed, 20 insertions(+), 3 deletions(-)
 create mode 100644 drivers/ras/Kconfig
 create mode 100644 drivers/ras/Makefile
 create mode 100644 drivers/ras/ras-traces.c

diff --git a/drivers/Kconfig b/drivers/Kconfig
index b3138fb..d70f7ba 100644
--- a/drivers/Kconfig
+++ b/drivers/Kconfig
@@ -170,4 +170,6 @@ source "drivers/phy/Kconfig"
 
 source "drivers/powercap/Kconfig"
 
+source "drivers/ras/Kconfig"
+
 endmenu
diff --git a/drivers/Makefile b/drivers/Makefile
index 8e3b8b0..10aaab0 100644
--- a/drivers/Makefile
+++ b/drivers/Makefile
@@ -155,3 +155,4 @@ obj-$(CONFIG_IPACK_BUS)		+= ipack/
 obj-$(CONFIG_NTB)		+= ntb/
 obj-$(CONFIG_FMC)		+= fmc/
 obj-$(CONFIG_POWERCAP)		+= powercap/
+obj-$(CONFIG_RAS_TRACE)		+= ras/
diff --git a/drivers/edac/edac_mc.c b/drivers/edac/edac_mc.c
index 33edd67..28c1695 100644
--- a/drivers/edac/edac_mc.c
+++ b/drivers/edac/edac_mc.c
@@ -33,9 +33,6 @@
 #include <asm/edac.h>
 #include "edac_core.h"
 #include "edac_module.h"
-
-#define CREATE_TRACE_POINTS
-#define TRACE_INCLUDE_PATH ../../include/ras
 #include <ras/ras_event.h>
 
 /* lock to memory controller's control array */
diff --git a/drivers/ras/Kconfig b/drivers/ras/Kconfig
new file mode 100644
index 0000000..6e4aec5
--- /dev/null
+++ b/drivers/ras/Kconfig
@@ -0,0 +1,4 @@
+# RAS_TRACE always gets selected by whoever wants it.
+config RAS_TRACE
+	def_bool y
+	depends on EDAC_MM_EDAC
diff --git a/drivers/ras/Makefile b/drivers/ras/Makefile
new file mode 100644
index 0000000..826afc6
--- /dev/null
+++ b/drivers/ras/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_RAS_TRACE) += ras-traces.o
diff --git a/drivers/ras/ras-traces.c b/drivers/ras/ras-traces.c
new file mode 100644
index 0000000..b0c6ed1
--- /dev/null
+++ b/drivers/ras/ras-traces.c
@@ -0,0 +1,12 @@
+/*
+ * Copyright (C) 2014 Intel Corporation
+ *
+ * Authors:
+ *	Chen, Gong <gong.chen@linux.intel.com>
+ */
+
+#define CREATE_TRACE_POINTS
+#define TRACE_INCLUDE_PATH ../../include/ras
+#include <ras/ras_event.h>
+
+EXPORT_TRACEPOINT_SYMBOL_GPL(mc_event);
-- 
1.9.0

[PATCH 2/5] CPER: Adjust code flow of some functions

[Chen, Gong]

Some codes can be reorganzied as a common function for
other usages. No functional changes.

Signed-off-by: Chen, Gong <gong.chen@linux.intel.com>
---
 drivers/firmware/efi/cper.c | 134 +++++++++++++++++++++++++++++++++-----------
 include/linux/cper.h        |   7 +++
 2 files changed, 108 insertions(+), 33 deletions(-)

diff --git a/drivers/firmware/efi/cper.c b/drivers/firmware/efi/cper.c
index 1491dd4..4e88885 100644
--- a/drivers/firmware/efi/cper.c
+++ b/drivers/firmware/efi/cper.c
@@ -34,6 +34,12 @@
 #include <linux/aer.h>
 
 #define INDENT_SP	" "
+DEFINE_RAW_SPINLOCK(cper_loc_lock);
+EXPORT_SYMBOL_GPL(cper_loc_lock);
+
+static char mem_location[CPER_REC_LEN];
+static char dimm_location[CPER_REC_LEN];
+
 /*
  * CPER record ID need to be unique even after reboot, because record
  * ID is used as index for ERST storage, while CPER records from
@@ -57,11 +63,12 @@ static const char *cper_severity_strs[] = {
 	"info",
 };
 
-static const char *cper_severity_str(unsigned int severity)
+const char *cper_severity_str(unsigned int severity)
 {
 	return severity < ARRAY_SIZE(cper_severity_strs) ?
 		cper_severity_strs[severity] : "unknown";
 }
+EXPORT_SYMBOL_GPL(cper_severity_str);
 
 /*
  * cper_print_bits - print strings for set bits
@@ -196,55 +203,116 @@ static const char *cper_mem_err_type_strs[] = {
 	"physical memory map-out event",
 };
 
-static void cper_print_mem(const char *pfx, const struct cper_sec_mem_err *mem)
+const char *cper_mem_err_type_str(unsigned int etype)
 {
-	if (mem->validation_bits & CPER_MEM_VALID_ERROR_STATUS)
-		printk("%s""error_status: 0x%016llx\n", pfx, mem->error_status);
-	if (mem->validation_bits & CPER_MEM_VALID_PA)
-		printk("%s""physical_address: 0x%016llx\n",
-		       pfx, mem->physical_addr);
-	if (mem->validation_bits & CPER_MEM_VALID_PA_MASK)
-		printk("%s""physical_address_mask: 0x%016llx\n",
-		       pfx, mem->physical_addr_mask);
+	return etype < ARRAY_SIZE(cper_mem_err_type_strs) ?
+		cper_mem_err_type_strs[etype] : "unknown";
+}
+EXPORT_SYMBOL_GPL(cper_mem_err_type_str);
+
+char *cper_mem_err_location(const struct cper_sec_mem_err *mem)
+{
+	char *p;
+	u32 n = 0;
+
+	memset(mem_location, 0, CPER_REC_LEN);
+	p = mem_location;
 	if (mem->validation_bits & CPER_MEM_VALID_NODE)
-		pr_debug("node: %d\n", mem->node);
+		n += sprintf(p + n, " node: %d", mem->node);
+	if (n >= CPER_REC_LEN)
+		goto end;
 	if (mem->validation_bits & CPER_MEM_VALID_CARD)
-		pr_debug("card: %d\n", mem->card);
+		n += sprintf(p + n, " card: %d", mem->card);
+	if (n >= CPER_REC_LEN)
+		goto end;
 	if (mem->validation_bits & CPER_MEM_VALID_MODULE)
-		pr_debug("module: %d\n", mem->module);
+		n += sprintf(p + n, " module: %d", mem->module);
+	if (n >= CPER_REC_LEN)
+		goto end;
 	if (mem->validation_bits & CPER_MEM_VALID_RANK_NUMBER)
-		pr_debug("rank: %d\n", mem->rank);
+		n += sprintf(p + n, " rank: %d", mem->rank);
+	if (n >= CPER_REC_LEN)
+		goto end;
 	if (mem->validation_bits & CPER_MEM_VALID_BANK)
-		pr_debug("bank: %d\n", mem->bank);
+		n += sprintf(p + n, " bank: %d", mem->bank);
+	if (n >= CPER_REC_LEN)
+		goto end;
 	if (mem->validation_bits & CPER_MEM_VALID_DEVICE)
-		pr_debug("device: %d\n", mem->device);
+		n += sprintf(p + n, " device: %d", mem->device);
+	if (n >= CPER_REC_LEN)
+		goto end;
 	if (mem->validation_bits & CPER_MEM_VALID_ROW)
-		pr_debug("row: %d\n", mem->row);
+		n += sprintf(p + n, " row: %d", mem->row);
+	if (n >= CPER_REC_LEN)
+		goto end;
 	if (mem->validation_bits & CPER_MEM_VALID_COLUMN)
-		pr_debug("column: %d\n", mem->column);
+		n += sprintf(p + n, " column: %d", mem->column);
+	if (n >= CPER_REC_LEN)
+		goto end;
 	if (mem->validation_bits & CPER_MEM_VALID_BIT_POSITION)
-		pr_debug("bit_position: %d\n", mem->bit_pos);
+		n += sprintf(p + n, " bit_position: %d", mem->bit_pos);
+	if (n >= CPER_REC_LEN)
+		goto end;
 	if (mem->validation_bits & CPER_MEM_VALID_REQUESTOR_ID)
-		pr_debug("requestor_id: 0x%016llx\n", mem->requestor_id);
+		n += sprintf(p + n, " requestor_id: 0x%016llx",
+				mem->requestor_id);
+	if (n >= CPER_REC_LEN)
+		goto end;
 	if (mem->validation_bits & CPER_MEM_VALID_RESPONDER_ID)
-		pr_debug("responder_id: 0x%016llx\n", mem->responder_id);
+		n += sprintf(p + n, " responder_id: 0x%016llx",
+				mem->responder_id);
+	if (n >= CPER_REC_LEN)
+		goto end;
 	if (mem->validation_bits & CPER_MEM_VALID_TARGET_ID)
-		pr_debug("target_id: 0x%016llx\n", mem->target_id);
+		n += sprintf(p + n, " target_id: 0x%016llx", mem->target_id);
+end:
+	return mem_location;
+}
+EXPORT_SYMBOL_GPL(cper_mem_err_location);
+
+char *cper_dimm_err_location(const struct cper_sec_mem_err *mem)
+{
+	const char *bank = NULL, *device = NULL;
+
+	memset(dimm_location, 0, CPER_REC_LEN);
+	if (!(mem->validation_bits & CPER_MEM_VALID_MODULE_HANDLE))
+		goto end;
+
+	dmi_memdev_name(mem->mem_dev_handle, &bank, &device);
+	if (bank != NULL && device != NULL)
+		snprintf(dimm_location, CPER_REC_LEN - 1,
+			 "DIMM location: %s %s", bank, device);
+	else
+		snprintf(dimm_location, CPER_REC_LEN - 1, "DMI handle: 0x%.4x",
+			 mem->mem_dev_handle);
+end:
+	return dimm_location;
+}
+EXPORT_SYMBOL_GPL(cper_dimm_err_location);
+
+static void cper_print_mem(const char *pfx, const struct cper_sec_mem_err *mem)
+{
+	unsigned long flags;
+
+	if (mem->validation_bits & CPER_MEM_VALID_ERROR_STATUS)
+		printk("%s""error_status: 0x%016llx\n", pfx, mem->error_status);
+	if (mem->validation_bits & CPER_MEM_VALID_PA)
+		printk("%s""physical_address: 0x%016llx\n",
+		       pfx, mem->physical_addr);
+	if (mem->validation_bits & CPER_MEM_VALID_PA_MASK)
+		printk("%s""physical_address_mask: 0x%016llx\n",
+		       pfx, mem->physical_addr_mask);
+	raw_spin_lock_irqsave(&cper_loc_lock, flags);
+	pr_debug("%s", cper_mem_err_location(mem));
+	raw_spin_unlock_irqrestore(&cper_loc_lock, flags);
 	if (mem->validation_bits & CPER_MEM_VALID_ERROR_TYPE) {
 		u8 etype = mem->error_type;
 		printk("%s""error_type: %d, %s\n", pfx, etype,
-		       etype < ARRAY_SIZE(cper_mem_err_type_strs) ?
-		       cper_mem_err_type_strs[etype] : "unknown");
-	}
-	if (mem->validation_bits & CPER_MEM_VALID_MODULE_HANDLE) {
-		const char *bank = NULL, *device = NULL;
-		dmi_memdev_name(mem->mem_dev_handle, &bank, &device);
-		if (bank != NULL && device != NULL)
-			printk("%s""DIMM location: %s %s", pfx, bank, device);
-		else
-			printk("%s""DIMM DMI handle: 0x%.4x",
-			       pfx, mem->mem_dev_handle);
+		       cper_mem_err_type_str(etype));
 	}
+	raw_spin_lock_irqsave(&cper_loc_lock, flags);
+	printk("%s%s", pfx, cper_dimm_err_location(mem));
+	raw_spin_unlock_irqrestore(&cper_loc_lock, flags);
 }
 
 static const char *cper_pcie_port_type_strs[] = {
diff --git a/include/linux/cper.h b/include/linux/cper.h
index 2fc0ec3..55c10db 100644
--- a/include/linux/cper.h
+++ b/include/linux/cper.h
@@ -23,6 +23,8 @@
 
 #include <linux/uuid.h>
 
+extern struct raw_spinlock cper_loc_lock;
+
 /* CPER record signature and the size */
 #define CPER_SIG_RECORD				"CPER"
 #define CPER_SIG_SIZE				4
@@ -35,6 +37,7 @@
  */
 #define CPER_RECORD_REV				0x0100
 
+#define CPER_REC_LEN				512
 /*
  * Severity difinition for error_severity in struct cper_record_header
  * and section_severity in struct cper_section_descriptor
@@ -395,7 +398,11 @@ struct cper_sec_pcie {
 #pragma pack()
 
 u64 cper_next_record_id(void);
+const char *cper_severity_str(unsigned int);
+const char *cper_mem_err_type_str(unsigned int);
 void cper_print_bits(const char *prefix, unsigned int bits,
 		     const char * const strs[], unsigned int strs_size);
+char *cper_mem_err_location(const struct cper_sec_mem_err *mem);
+char *cper_dimm_err_location(const struct cper_sec_mem_err *mem);
 
 #endif
-- 
1.9.0

[PATCH 3/5] trace, RAS: Add eMCA trace event interface

[Chen, Gong]

Add trace interface to elaborate all H/W error related information.

Signed-off-by: Chen, Gong <gong.chen@linux.intel.com>
---
 drivers/acpi/Kconfig       |  3 ++-
 drivers/acpi/Makefile      |  1 +
 drivers/acpi/acpi_extlog.c | 56 ++++++++++++++++++++++++++++++++++++++++---
 drivers/ras/Kconfig        |  2 +-
 drivers/ras/ras-traces.c   |  1 +
 include/ras/ras_event.h    | 60 ++++++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 118 insertions(+), 5 deletions(-)

diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
index 4770de5..3e569d4 100644
--- a/drivers/acpi/Kconfig
+++ b/drivers/acpi/Kconfig
@@ -363,6 +363,7 @@ config ACPI_EXTLOG
 
 	  Enhanced MCA Logging allows firmware to provide additional error
 	  information to system software, synchronous with MCE or CMCI. This
-	  driver adds support for that functionality.
+	  driver adds support for that functionality with corresponding
+	  tracepoint which carries that information to userspace.
 
 endif	# ACPI
diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
index 0331f91..f6abc4a 100644
--- a/drivers/acpi/Makefile
+++ b/drivers/acpi/Makefile
@@ -82,4 +82,5 @@ obj-$(CONFIG_ACPI_PROCESSOR_AGGREGATOR) += acpi_pad.o
 
 obj-$(CONFIG_ACPI_APEI)		+= apei/
 
+CFLAGS_acpi_extlog.o := -I$(src)
 obj-$(CONFIG_ACPI_EXTLOG)	+= acpi_extlog.o
diff --git a/drivers/acpi/acpi_extlog.c b/drivers/acpi/acpi_extlog.c
index c4a5d87..0ee2c38 100644
--- a/drivers/acpi/acpi_extlog.c
+++ b/drivers/acpi/acpi_extlog.c
@@ -16,6 +16,7 @@
 #include <asm/mce.h>
 
 #include "apei/apei-internal.h"
+#include <ras/ras_event.h>
 
 #define EXT_ELOG_ENTRY_MASK	GENMASK_ULL(51, 0) /* elog entry address mask */
 
@@ -44,6 +45,7 @@ struct extlog_l1_head {
 static int old_edac_report_status;
 
 static u8 extlog_dsm_uuid[] __initdata = "663E35AF-CC10-41A4-88EA-5470AF055295";
+static const uuid_le invalid_uuid = NULL_UUID_LE;
 
 /* L1 table related physical address */
 static u64 elog_base;
@@ -69,6 +71,34 @@ static u32 l1_percpu_entry;
 #define ELOG_ENTRY_ADDR(phyaddr) \
 	(phyaddr - elog_base + (u8 *)elog_addr)
 
+static void __trace_mem_error(const uuid_le *fru_id, char *fru_text,
+			       u64 err_count, u32 severity,
+			       struct cper_sec_mem_err *mem)
+{
+	u32 etype = ~0U;
+	u64 phy_addr = ~0ull;
+	unsigned long flags;
+	char *mem_location;
+	char *dimm_location;
+
+	if (mem->validation_bits & CPER_MEM_VALID_ERROR_TYPE)
+		etype = mem->error_type;
+
+	if (mem->validation_bits & CPER_MEM_VALID_PA) {
+		phy_addr = mem->physical_addr;
+		if (mem->validation_bits & CPER_MEM_VALID_PA_MASK)
+			phy_addr &= mem->physical_addr_mask;
+	}
+
+	raw_spin_lock_irqsave(&cper_loc_lock, flags);
+	mem_location = cper_mem_err_location(mem);
+	dimm_location = cper_dimm_err_location(mem);
+
+	trace_extlog_mem_event(etype, dimm_location, fru_id, fru_text,
+			       err_count, severity, phy_addr, mem_location);
+	raw_spin_unlock_irqrestore(&cper_loc_lock, flags);
+}
+
 static struct acpi_generic_status *extlog_elog_entry_check(int cpu, int bank)
 {
 	int idx;
@@ -137,8 +167,12 @@ static int extlog_print(struct notifier_block *nb, unsigned long val,
 	struct mce *mce = (struct mce *)data;
 	int	bank = mce->bank;
 	int	cpu = mce->extcpu;
-	struct acpi_generic_status *estatus;
-	int rc;
+	struct acpi_generic_status *estatus, *tmp;
+	struct acpi_generic_data *gdata;
+	const uuid_le *fru_id = &invalid_uuid;
+	char *fru_text = "";
+	uuid_le *sec_type;
+	static u64 err_count;
 
 	estatus = extlog_elog_entry_check(cpu, bank);
 	if (estatus == NULL)
@@ -148,7 +182,23 @@ static int extlog_print(struct notifier_block *nb, unsigned long val,
 	/* clear record status to enable BIOS to update it again */
 	estatus->block_status = 0;
 
-	rc = print_extlog_rcd(NULL, (struct acpi_generic_status *)elog_buf, cpu);
+	tmp = (struct acpi_generic_status *)elog_buf;
+	print_extlog_rcd(NULL, tmp, cpu);
+
+	/* log event via trace */
+	err_count++;
+	gdata = (struct acpi_generic_data *)(tmp + 1);
+	if (gdata->validation_bits & CPER_SEC_VALID_FRU_ID)
+		fru_id = (uuid_le *)gdata->fru_id;
+	if (gdata->validation_bits & CPER_SEC_VALID_FRU_TEXT)
+		fru_text = gdata->fru_text;
+	sec_type = (uuid_le *)gdata->section_type;
+	if (!uuid_le_cmp(*sec_type, CPER_SEC_PLATFORM_MEM)) {
+		struct cper_sec_mem_err *mem_err = (void *)(gdata + 1);
+		if (gdata->error_data_length >= sizeof(*mem_err))
+			__trace_mem_error(fru_id, fru_text, err_count,
+					  gdata->error_severity, mem_err);
+	}
 
 	return NOTIFY_STOP;
 }
diff --git a/drivers/ras/Kconfig b/drivers/ras/Kconfig
index 6e4aec5..64f09641 100644
--- a/drivers/ras/Kconfig
+++ b/drivers/ras/Kconfig
@@ -1,4 +1,4 @@
 # RAS_TRACE always gets selected by whoever wants it.
 config RAS_TRACE
 	def_bool y
-	depends on EDAC_MM_EDAC
+	depends on EDAC_MM_EDAC || ACPI_EXTLOG
diff --git a/drivers/ras/ras-traces.c b/drivers/ras/ras-traces.c
index b0c6ed1..197b1ea 100644
--- a/drivers/ras/ras-traces.c
+++ b/drivers/ras/ras-traces.c
@@ -9,4 +9,5 @@
 #define TRACE_INCLUDE_PATH ../../include/ras
 #include <ras/ras_event.h>
 
+EXPORT_TRACEPOINT_SYMBOL_GPL(extlog_mem_event);
 EXPORT_TRACEPOINT_SYMBOL_GPL(mc_event);
diff --git a/include/ras/ras_event.h b/include/ras/ras_event.h
index 21cdb0b..dfda854 100644
--- a/include/ras/ras_event.h
+++ b/include/ras/ras_event.h
@@ -8,6 +8,66 @@
 #include <linux/tracepoint.h>
 #include <linux/edac.h>
 #include <linux/ktime.h>
+#include <linux/cper.h>
+
+/*
+ * MCE Extended Error Log trace event
+ *
+ * These events are generated when hardware detects a corrected or
+ * uncorrected event.
+ *
+ */
+
+/* memory trace event */
+
+TRACE_EVENT(extlog_mem_event,
+	TP_PROTO(u32 etype,
+		 char *dimm_info,
+		 const uuid_le *fru_id,
+		 char *fru_text,
+		 u64 error_count,
+		 u32 severity,
+		 u64 phy_addr,
+		 char *mem_loc),
+
+	TP_ARGS(etype, dimm_info, fru_id, fru_text, error_count, severity,
+		phy_addr, mem_loc),
+
+	TP_STRUCT__entry(
+		__field(u32, etype)
+		__dynamic_array(char, dimm_info, CPER_REC_LEN)
+		__field(u64, error_count)
+		__field(u32, severity)
+		__field(u64, paddr)
+		__string(mem_loc, mem_loc)
+		__dynamic_array(char, fru, CPER_REC_LEN)
+	),
+
+	TP_fast_assign(
+		__entry->error_count = error_count;
+		__entry->severity = severity;
+		__entry->etype = etype;
+		if (dimm_info[0] != '\0')
+			snprintf(__get_dynamic_array(dimm_info),
+				 CPER_REC_LEN - 1, "%s", dimm_info);
+		else
+			__assign_str(dimm_info, "");
+		__entry->paddr = phy_addr;
+		__assign_str(mem_loc, mem_loc);
+		snprintf(__get_dynamic_array(fru), CPER_REC_LEN - 1,
+			 "FRU: %pUl %.20s", fru_id, fru_text);
+	),
+
+	TP_printk("%llu %s error%s: %s %s physical addr: 0x%016llx%s %s",
+		  __entry->error_count,
+		  cper_severity_str(__entry->severity),
+		  __entry->error_count > 1 ? "s" : "",
+		  cper_mem_err_type_str(__entry->etype),
+		  __get_str(dimm_info),
+		  __entry->paddr,
+		  __get_str(mem_loc),
+		  __get_str(fru))
+);
 
 /*
  * Hardware Events Report
-- 
1.9.0

[PATCH 4/5] trace, eMCA: Add a knob to adjust where to save event log

[Chen, Gong]

To avoid saving two copies for one H/W event, add a new
file under debugfs to control how to save event log.
Once this file is opened, the perf/trace will be used,
in the meanwhile, kernel will stop to print event log
to the console. On the other hand, if this file is closed,
kernel will print event log to the console again.

Signed-off-by: Chen, Gong <gong.chen@linux.intel.com>
---
 drivers/acpi/acpi_extlog.c | 74 ++++++++++++++++++++++++++++++++++++----------
 1 file changed, 59 insertions(+), 15 deletions(-)

diff --git a/drivers/acpi/acpi_extlog.c b/drivers/acpi/acpi_extlog.c
index 0ee2c38..f9a63dd 100644
--- a/drivers/acpi/acpi_extlog.c
+++ b/drivers/acpi/acpi_extlog.c
@@ -12,6 +12,7 @@
 #include <linux/cper.h>
 #include <linux/ratelimit.h>
 #include <linux/edac.h>
+#include <linux/debugfs.h>
 #include <asm/cpu.h>
 #include <asm/mce.h>
 
@@ -62,6 +63,9 @@ static void *elog_buf;
 static u64 *l1_entry_base;
 static u32 l1_percpu_entry;
 
+static struct dentry *extlog_debug_dir;
+static atomic_t trace_on;
+
 #define ELOG_IDX(cpu, bank) \
 	(cpu_physical_id(cpu) * l1_percpu_entry + (bank))
 
@@ -183,21 +187,24 @@ static int extlog_print(struct notifier_block *nb, unsigned long val,
 	estatus->block_status = 0;
 
 	tmp = (struct acpi_generic_status *)elog_buf;
-	print_extlog_rcd(NULL, tmp, cpu);
-
-	/* log event via trace */
-	err_count++;
-	gdata = (struct acpi_generic_data *)(tmp + 1);
-	if (gdata->validation_bits & CPER_SEC_VALID_FRU_ID)
-		fru_id = (uuid_le *)gdata->fru_id;
-	if (gdata->validation_bits & CPER_SEC_VALID_FRU_TEXT)
-		fru_text = gdata->fru_text;
-	sec_type = (uuid_le *)gdata->section_type;
-	if (!uuid_le_cmp(*sec_type, CPER_SEC_PLATFORM_MEM)) {
-		struct cper_sec_mem_err *mem_err = (void *)(gdata + 1);
-		if (gdata->error_data_length >= sizeof(*mem_err))
-			__trace_mem_error(fru_id, fru_text, err_count,
-					  gdata->error_severity, mem_err);
+	if (!atomic_read(&trace_on))
+		print_extlog_rcd(NULL, tmp, cpu);
+	else {
+		/* log event via trace */
+		err_count++;
+		gdata = (struct acpi_generic_data *)(tmp + 1);
+		if (gdata->validation_bits & CPER_SEC_VALID_FRU_ID)
+			fru_id = (uuid_le *)gdata->fru_id;
+		if (gdata->validation_bits & CPER_SEC_VALID_FRU_TEXT)
+			fru_text = gdata->fru_text;
+		sec_type = (uuid_le *)gdata->section_type;
+		if (!uuid_le_cmp(*sec_type, CPER_SEC_PLATFORM_MEM)) {
+			struct cper_sec_mem_err *mem_err = (void *)(gdata + 1);
+			if (gdata->error_data_length >= sizeof(*mem_err))
+				__trace_mem_error(fru_id, fru_text, err_count,
+						  gdata->error_severity,
+						  mem_err);
+		}
 	}
 
 	return NOTIFY_STOP;
@@ -233,6 +240,31 @@ static bool __init extlog_get_l1addr(void)
 
 	return true;
 }
+
+static int extlog_trace_show(struct seq_file *m, void *v)
+{
+	atomic_inc(&trace_on);
+	return 0;
+}
+
+static int trace_open(struct inode *inode, struct file *file)
+{
+	return single_open(file, extlog_trace_show, NULL);
+}
+
+static int trace_release(struct inode *inode, struct file *file)
+{
+	atomic_dec(&trace_on);
+	return single_release(inode, file);
+}
+
+static const struct file_operations trace_fops = {
+	.open    = trace_open,
+	.read    = seq_read,
+	.llseek  = seq_lseek,
+	.release = trace_release,
+};
+
 static struct notifier_block extlog_mce_dec = {
 	.notifier_call	= extlog_print,
 };
@@ -243,6 +275,7 @@ static int __init extlog_init(void)
 	void __iomem *extlog_l1_hdr;
 	size_t l1_hdr_size;
 	struct resource *r;
+	struct dentry *fentry;
 	u64 cap;
 	int rc;
 
@@ -306,6 +339,14 @@ static int __init extlog_init(void)
 	if (elog_buf == NULL)
 		goto err_release_elog;
 
+	extlog_debug_dir = debugfs_create_dir("extlog", NULL);
+	if (!extlog_debug_dir)
+		goto err_cleanup;
+	fentry = debugfs_create_file("suppress_console", S_IRUSR,
+				     extlog_debug_dir, NULL, &trace_fops);
+	if (!fentry)
+		goto err_cleanup;
+
 	/*
 	 * eMCA event report method has higher priority than EDAC method,
 	 * unless EDAC event report method is mandatory.
@@ -318,6 +359,8 @@ static int __init extlog_init(void)
 
 	return 0;
 
+err_cleanup:
+	debugfs_remove_recursive(extlog_debug_dir);
 err_release_elog:
 	if (elog_addr)
 		acpi_os_unmap_memory(elog_addr, elog_size);
@@ -343,6 +386,7 @@ static void __exit extlog_exit(void)
 	release_mem_region(elog_base, elog_size);
 	release_mem_region(l1_dirbase, l1_size);
 	kfree(elog_buf);
+	debugfs_remove_recursive(extlog_debug_dir);
 }
 
 module_init(extlog_init);
-- 
1.9.0

[PATCH 5/5] trace, AER: Move trace into unified interface

[Chen, Gong]

AER uses a separate trace interface by now. To make it
consistent, move it into unified RAS trace interface.

Signed-off-by: Chen, Gong <gong.chen@linux.intel.com>
---
 drivers/pci/pcie/aer/aerdrv_errprint.c |  4 +-
 drivers/ras/Kconfig                    |  2 +-
 include/ras/ras_event.h                | 64 ++++++++++++++++++++++++++++
 include/trace/events/ras.h             | 77 ----------------------------------
 4 files changed, 66 insertions(+), 81 deletions(-)
 delete mode 100644 include/trace/events/ras.h

diff --git a/drivers/pci/pcie/aer/aerdrv_errprint.c b/drivers/pci/pcie/aer/aerdrv_errprint.c
index 34ff702..73e73b7 100644
--- a/drivers/pci/pcie/aer/aerdrv_errprint.c
+++ b/drivers/pci/pcie/aer/aerdrv_errprint.c
@@ -22,9 +22,7 @@
 #include <linux/cper.h>
 
 #include "aerdrv.h"
-
-#define CREATE_TRACE_POINTS
-#include <trace/events/ras.h>
+#include <ras/ras_event.h>
 
 #define AER_AGENT_RECEIVER		0
 #define AER_AGENT_REQUESTER		1
diff --git a/drivers/ras/Kconfig b/drivers/ras/Kconfig
index 64f09641..75845b9 100644
--- a/drivers/ras/Kconfig
+++ b/drivers/ras/Kconfig
@@ -1,4 +1,4 @@
 # RAS_TRACE always gets selected by whoever wants it.
 config RAS_TRACE
 	def_bool y
-	depends on EDAC_MM_EDAC || ACPI_EXTLOG
+	depends on EDAC_MM_EDAC || ACPI_EXTLOG || PCIEAER
diff --git a/include/ras/ras_event.h b/include/ras/ras_event.h
index dfda854..375d318 100644
--- a/include/ras/ras_event.h
+++ b/include/ras/ras_event.h
@@ -9,6 +9,7 @@
 #include <linux/edac.h>
 #include <linux/ktime.h>
 #include <linux/cper.h>
+#include <linux/aer.h>
 
 /*
  * MCE Extended Error Log trace event
@@ -154,6 +155,69 @@ TRACE_EVENT(mc_event,
 		  __get_str(driver_detail))
 );
 
+/*
+ * PCIe AER Trace event
+ *
+ * These events are generated when hardware detects a corrected or
+ * uncorrected event on a PCIe device. The event report has
+ * the following structure:
+ *
+ * char * dev_name -	The name of the slot where the device resides
+ *			([domain:]bus:device.function).
+ * u32 status -		Either the correctable or uncorrectable register
+ *			indicating what error or errors have been seen
+ * u8 severity -	error severity 0:NONFATAL 1:FATAL 2:CORRECTED
+ */
+
+#define aer_correctable_errors		\
+	{BIT(0),	"Receiver Error"},		\
+	{BIT(6),	"Bad TLP"},			\
+	{BIT(7),	"Bad DLLP"},			\
+	{BIT(8),	"RELAY_NUM Rollover"},		\
+	{BIT(12),	"Replay Timer Timeout"},	\
+	{BIT(13),	"Advisory Non-Fatal"}
+
+#define aer_uncorrectable_errors		\
+	{BIT(4),	"Data Link Protocol"},		\
+	{BIT(12),	"Poisoned TLP"},		\
+	{BIT(13),	"Flow Control Protocol"},	\
+	{BIT(14),	"Completion Timeout"},		\
+	{BIT(15),	"Completer Abort"},		\
+	{BIT(16),	"Unexpected Completion"},	\
+	{BIT(17),	"Receiver Overflow"},		\
+	{BIT(18),	"Malformed TLP"},		\
+	{BIT(19),	"ECRC"},			\
+	{BIT(20),	"Unsupported Request"}
+
+TRACE_EVENT(aer_event,
+	TP_PROTO(const char *dev_name,
+		 const u32 status,
+		 const u8 severity),
+
+	TP_ARGS(dev_name, status, severity),
+
+	TP_STRUCT__entry(
+		__string(	dev_name,	dev_name	)
+		__field(	u32,		status		)
+		__field(	u8,		severity	)
+	),
+
+	TP_fast_assign(
+		__assign_str(dev_name, dev_name);
+		__entry->status		= status;
+		__entry->severity	= severity;
+	),
+
+	TP_printk("%s PCIe Bus Error: severity=%s, %s\n",
+		__get_str(dev_name),
+		__entry->severity == AER_CORRECTABLE ? "Corrected" :
+			__entry->severity == AER_FATAL ?
+			"Fatal" : "Uncorrected, non-fatal",
+		__entry->severity == AER_CORRECTABLE ?
+		__print_flags(__entry->status, "|", aer_correctable_errors) :
+		__print_flags(__entry->status, "|", aer_uncorrectable_errors))
+);
+
 #endif /* _TRACE_HW_EVENT_MC_H */
 
 /* This part must be outside protection */
diff --git a/include/trace/events/ras.h b/include/trace/events/ras.h
deleted file mode 100644
index 1c875ad..0000000
--- a/include/trace/events/ras.h
+++ /dev/null
@@ -1,77 +0,0 @@
-#undef TRACE_SYSTEM
-#define TRACE_SYSTEM ras
-
-#if !defined(_TRACE_AER_H) || defined(TRACE_HEADER_MULTI_READ)
-#define _TRACE_AER_H
-
-#include <linux/tracepoint.h>
-#include <linux/aer.h>
-
-
-/*
- * PCIe AER Trace event
- *
- * These events are generated when hardware detects a corrected or
- * uncorrected event on a PCIe device. The event report has
- * the following structure:
- *
- * char * dev_name -	The name of the slot where the device resides
- *			([domain:]bus:device.function).
- * u32 status -		Either the correctable or uncorrectable register
- *			indicating what error or errors have been seen
- * u8 severity -	error severity 0:NONFATAL 1:FATAL 2:CORRECTED
- */
-
-#define aer_correctable_errors		\
-	{BIT(0),	"Receiver Error"},		\
-	{BIT(6),	"Bad TLP"},			\
-	{BIT(7),	"Bad DLLP"},			\
-	{BIT(8),	"RELAY_NUM Rollover"},		\
-	{BIT(12),	"Replay Timer Timeout"},	\
-	{BIT(13),	"Advisory Non-Fatal"}
-
-#define aer_uncorrectable_errors		\
-	{BIT(4),	"Data Link Protocol"},		\
-	{BIT(12),	"Poisoned TLP"},		\
-	{BIT(13),	"Flow Control Protocol"},	\
-	{BIT(14),	"Completion Timeout"},		\
-	{BIT(15),	"Completer Abort"},		\
-	{BIT(16),	"Unexpected Completion"},	\
-	{BIT(17),	"Receiver Overflow"},		\
-	{BIT(18),	"Malformed TLP"},		\
-	{BIT(19),	"ECRC"},			\
-	{BIT(20),	"Unsupported Request"}
-
-TRACE_EVENT(aer_event,
-	TP_PROTO(const char *dev_name,
-		 const u32 status,
-		 const u8 severity),
-
-	TP_ARGS(dev_name, status, severity),
-
-	TP_STRUCT__entry(
-		__string(	dev_name,	dev_name	)
-		__field(	u32,		status		)
-		__field(	u8,		severity	)
-	),
-
-	TP_fast_assign(
-		__assign_str(dev_name, dev_name);
-		__entry->status		= status;
-		__entry->severity	= severity;
-	),
-
-	TP_printk("%s PCIe Bus Error: severity=%s, %s\n",
-		__get_str(dev_name),
-		__entry->severity == AER_CORRECTABLE ? "Corrected" :
-			__entry->severity == AER_FATAL ?
-			"Fatal" : "Uncorrected, non-fatal",
-		__entry->severity == AER_CORRECTABLE ?
-		__print_flags(__entry->status, "|", aer_correctable_errors) :
-		__print_flags(__entry->status, "|", aer_uncorrectable_errors))
-);
-
-#endif /* _TRACE_AER_H */
-
-/* This part must be outside protection */
-#include <trace/define_trace.h>
-- 
1.9.0

Re: [PATCH 4/5] trace, eMCA: Add a knob to adjust where to save event log

[Tony Luck]

> +static int extlog_trace_show(struct seq_file *m, void *v)
> +{
> +       atomic_inc(&trace_on);
> +       return 0;
> +}
> +
> +static int trace_open(struct inode *inode, struct file *file)
> +{
> +       return single_open(file, extlog_trace_show, NULL);
> +}

Shouldn't this atomic_inc() be inside the trace_open() function
... not the extlog_trace_show()?

-Tony

Re: [PATCH 4/5] trace, eMCA: Add a knob to adjust where to save event log

[Chen, Gong]

[-- Attachment #1: Type: text/plain, Size: 648 bytes --]

On Thu, Apr 03, 2014 at 04:46:53PM -0700, Tony Luck wrote:
> > +static int extlog_trace_show(struct seq_file *m, void *v)
> > +{
> > +       atomic_inc(&trace_on);
> > +       return 0;
> > +}
> > +
> > +static int trace_open(struct inode *inode, struct file *file)
> > +{
> > +       return single_open(file, extlog_trace_show, NULL);
> > +}
> 
> Shouldn't this atomic_inc() be inside the trace_open() function
> ... not the extlog_trace_show()?
> 
Gee. It's my fault. I thought extlog_trace_show is only called
by *open* but after I look through related codes, I find it is called
be *read*, too. I will fix it in next version.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH 4/5 v2] trace, eMCA: Add a knob to adjust where to save event log

[Chen, Gong]

To avoid saving two copies for one H/W event, add a new
file under debugfs to control how to save event log.
Once this file is opened, the perf/trace will be used,
in the meanwhile, kernel will stop to print event log
to the console. On the other hand, if this file is closed,
kernel will print event log to the console again.

v2 -> v1: move counter operation from *read* function to *open* function

Signed-off-by: Chen, Gong <gong.chen@linux.intel.com>
---
 drivers/acpi/acpi_extlog.c | 74 ++++++++++++++++++++++++++++++++++++----------
 1 file changed, 59 insertions(+), 15 deletions(-)

diff --git a/drivers/acpi/acpi_extlog.c b/drivers/acpi/acpi_extlog.c
index 0ee2c38..7e8f9ed 100644
--- a/drivers/acpi/acpi_extlog.c
+++ b/drivers/acpi/acpi_extlog.c
@@ -12,6 +12,7 @@
 #include <linux/cper.h>
 #include <linux/ratelimit.h>
 #include <linux/edac.h>
+#include <linux/debugfs.h>
 #include <asm/cpu.h>
 #include <asm/mce.h>
 
@@ -62,6 +63,9 @@ static void *elog_buf;
 static u64 *l1_entry_base;
 static u32 l1_percpu_entry;
 
+static struct dentry *extlog_debug_dir;
+static atomic_t trace_on;
+
 #define ELOG_IDX(cpu, bank) \
 	(cpu_physical_id(cpu) * l1_percpu_entry + (bank))
 
@@ -183,21 +187,24 @@ static int extlog_print(struct notifier_block *nb, unsigned long val,
 	estatus->block_status = 0;
 
 	tmp = (struct acpi_generic_status *)elog_buf;
-	print_extlog_rcd(NULL, tmp, cpu);
-
-	/* log event via trace */
-	err_count++;
-	gdata = (struct acpi_generic_data *)(tmp + 1);
-	if (gdata->validation_bits & CPER_SEC_VALID_FRU_ID)
-		fru_id = (uuid_le *)gdata->fru_id;
-	if (gdata->validation_bits & CPER_SEC_VALID_FRU_TEXT)
-		fru_text = gdata->fru_text;
-	sec_type = (uuid_le *)gdata->section_type;
-	if (!uuid_le_cmp(*sec_type, CPER_SEC_PLATFORM_MEM)) {
-		struct cper_sec_mem_err *mem_err = (void *)(gdata + 1);
-		if (gdata->error_data_length >= sizeof(*mem_err))
-			__trace_mem_error(fru_id, fru_text, err_count,
-					  gdata->error_severity, mem_err);
+	if (!atomic_read(&trace_on))
+		print_extlog_rcd(NULL, tmp, cpu);
+	else {
+		/* log event via trace */
+		err_count++;
+		gdata = (struct acpi_generic_data *)(tmp + 1);
+		if (gdata->validation_bits & CPER_SEC_VALID_FRU_ID)
+			fru_id = (uuid_le *)gdata->fru_id;
+		if (gdata->validation_bits & CPER_SEC_VALID_FRU_TEXT)
+			fru_text = gdata->fru_text;
+		sec_type = (uuid_le *)gdata->section_type;
+		if (!uuid_le_cmp(*sec_type, CPER_SEC_PLATFORM_MEM)) {
+			struct cper_sec_mem_err *mem_err = (void *)(gdata + 1);
+			if (gdata->error_data_length >= sizeof(*mem_err))
+				__trace_mem_error(fru_id, fru_text, err_count,
+						  gdata->error_severity,
+						  mem_err);
+		}
 	}
 
 	return NOTIFY_STOP;
@@ -233,6 +240,31 @@ static bool __init extlog_get_l1addr(void)
 
 	return true;
 }
+
+static int extlog_trace_show(struct seq_file *m, void *v)
+{
+	return 0;
+}
+
+static int trace_open(struct inode *inode, struct file *file)
+{
+	atomic_inc(&trace_on);
+	return single_open(file, extlog_trace_show, NULL);
+}
+
+static int trace_release(struct inode *inode, struct file *file)
+{
+	atomic_dec(&trace_on);
+	return single_release(inode, file);
+}
+
+static const struct file_operations trace_fops = {
+	.open    = trace_open,
+	.read    = seq_read,
+	.llseek  = seq_lseek,
+	.release = trace_release,
+};
+
 static struct notifier_block extlog_mce_dec = {
 	.notifier_call	= extlog_print,
 };
@@ -243,6 +275,7 @@ static int __init extlog_init(void)
 	void __iomem *extlog_l1_hdr;
 	size_t l1_hdr_size;
 	struct resource *r;
+	struct dentry *fentry;
 	u64 cap;
 	int rc;
 
@@ -306,6 +339,14 @@ static int __init extlog_init(void)
 	if (elog_buf == NULL)
 		goto err_release_elog;
 
+	extlog_debug_dir = debugfs_create_dir("extlog", NULL);
+	if (!extlog_debug_dir)
+		goto err_cleanup;
+	fentry = debugfs_create_file("suppress_console", S_IRUSR,
+				     extlog_debug_dir, NULL, &trace_fops);
+	if (!fentry)
+		goto err_cleanup;
+
 	/*
 	 * eMCA event report method has higher priority than EDAC method,
 	 * unless EDAC event report method is mandatory.
@@ -318,6 +359,8 @@ static int __init extlog_init(void)
 
 	return 0;
 
+err_cleanup:
+	debugfs_remove_recursive(extlog_debug_dir);
 err_release_elog:
 	if (elog_addr)
 		acpi_os_unmap_memory(elog_addr, elog_size);
@@ -343,6 +386,7 @@ static void __exit extlog_exit(void)
 	release_mem_region(elog_base, elog_size);
 	release_mem_region(l1_dirbase, l1_size);
 	kfree(elog_buf);
+	debugfs_remove_recursive(extlog_debug_dir);
 }
 
 module_init(extlog_init);
-- 
1.9.0

Re: [PATCH 1/5] trace, RAS: Add basic RAS trace event

[Borislav Petkov]

On Fri, Mar 28, 2014 at 01:52:57AM -0400, Chen, Gong wrote:
> To avoid the confuision of usage for RAS related trace event, add
> an unified RAS trace event stub.
> 
> Signed-off-by: Chen, Gong <gong.chen@linux.intel.com>
> ---
>  drivers/Kconfig          |  2 ++
>  drivers/Makefile         |  1 +
>  drivers/edac/edac_mc.c   |  3 ---
>  drivers/ras/Kconfig      |  4 ++++
>  drivers/ras/Makefile     |  1 +
>  drivers/ras/ras-traces.c | 12 ++++++++++++
>  6 files changed, 20 insertions(+), 3 deletions(-)
>  create mode 100644 drivers/ras/Kconfig
>  create mode 100644 drivers/ras/Makefile
>  create mode 100644 drivers/ras/ras-traces.c
> 
> diff --git a/drivers/Kconfig b/drivers/Kconfig
> index b3138fb..d70f7ba 100644
> --- a/drivers/Kconfig
> +++ b/drivers/Kconfig
> @@ -170,4 +170,6 @@ source "drivers/phy/Kconfig"
>  
>  source "drivers/powercap/Kconfig"
>  
> +source "drivers/ras/Kconfig"
> +
>  endmenu
> diff --git a/drivers/Makefile b/drivers/Makefile
> index 8e3b8b0..10aaab0 100644
> --- a/drivers/Makefile
> +++ b/drivers/Makefile
> @@ -155,3 +155,4 @@ obj-$(CONFIG_IPACK_BUS)		+= ipack/
>  obj-$(CONFIG_NTB)		+= ntb/
>  obj-$(CONFIG_FMC)		+= fmc/
>  obj-$(CONFIG_POWERCAP)		+= powercap/
> +obj-$(CONFIG_RAS_TRACE)		+= ras/
> diff --git a/drivers/edac/edac_mc.c b/drivers/edac/edac_mc.c
> index 33edd67..28c1695 100644
> --- a/drivers/edac/edac_mc.c
> +++ b/drivers/edac/edac_mc.c
> @@ -33,9 +33,6 @@
>  #include <asm/edac.h>
>  #include "edac_core.h"
>  #include "edac_module.h"
> -
> -#define CREATE_TRACE_POINTS
> -#define TRACE_INCLUDE_PATH ../../include/ras
>  #include <ras/ras_event.h>
>  
>  /* lock to memory controller's control array */
> diff --git a/drivers/ras/Kconfig b/drivers/ras/Kconfig
> new file mode 100644
> index 0000000..6e4aec5
> --- /dev/null
> +++ b/drivers/ras/Kconfig
> @@ -0,0 +1,4 @@
> +# RAS_TRACE always gets selected by whoever wants it.
> +config RAS_TRACE
> +	def_bool y
> +	depends on EDAC_MM_EDAC

This should actually be

menuconfig RAS
	bool "Reliability, Availability, Serviceability features"
	help
	  <A nice text about what this is going to contain, i.e. RAS stuff

	  ...

if RAS
config RAS_TRACE
	def_bool y
	depends on ...

See drivers/edac/Kconfig for an example.

RAS_TRACE should actually depend on all the code that uses it, or, it
should be selected by them. EDAC_MM_EDAC is not accurate enough and will
enable RAS_TRACE even for drivers which don't use the tracepoint(s).

Thanks.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH 1/5] trace, RAS: Add basic RAS trace event

[Chen, Gong]

[-- Attachment #1: Type: text/plain, Size: 1086 bytes --]

On Wed, Apr 09, 2014 at 09:46:54PM +0200, Borislav Petkov wrote:
[...]
> 
> This should actually be
> 
> menuconfig RAS
> 	bool "Reliability, Availability, Serviceability features"
> 	help
> 	  <A nice text about what this is going to contain, i.e. RAS stuff
> 
> 	  ...
> 
> if RAS
> config RAS_TRACE
> 	def_bool y
> 	depends on ...
> 
> See drivers/edac/Kconfig for an example.
I don't use a explicit menu for RAS because I'm not sure if it is worth
to add such a *heavy hammer* in the kernel tree. Since you suggest it,
it is fine to me.

> 
> RAS_TRACE should actually depend on all the code that uses it, or, it
> should be selected by them. EDAC_MM_EDAC is not accurate enough and will
> enable RAS_TRACE even for drivers which don't use the tracepoint(s).
Maybe some drivers don't call trace_mc_event directly/indirectly, but
edac_mc.c is the core of EDAC and must exist before any other drivers
are loaded, which means whether the drivers call trace_mc_event or not,
the trace interface in edac_mc should be there in advance. Do I miss
something?

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/5] trace, RAS: Add basic RAS trace event

[Borislav Petkov]

On Sun, Apr 13, 2014 at 11:20:58PM -0400, Chen, Gong wrote:
> I don't use a explicit menu for RAS because I'm not sure if it is
> worth to add such a *heavy hammer* in the kernel tree.

We're going to be adding more stuff to it so a full menu will come
sooner rather than later.

> Maybe some drivers don't call trace_mc_event directly/indirectly, but
> edac_mc.c is the core of EDAC and must exist before any other drivers
> are loaded, which means whether the drivers call trace_mc_event or
> not, the trace interface in edac_mc should be there in advance. Do I
> miss something?

No, you're fine. I missed the fact that you've moved the mc_event
tracepoint, sorry.

Thanks.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH 2/5] CPER: Adjust code flow of some functions

[Borislav Petkov]

On Fri, Mar 28, 2014 at 01:52:58AM -0400, Chen, Gong wrote:
> Some codes can be reorganzied as a common function for
> other usages. No functional changes.
> 
> Signed-off-by: Chen, Gong <gong.chen@linux.intel.com>
> ---
>  drivers/firmware/efi/cper.c | 134 +++++++++++++++++++++++++++++++++-----------
>  include/linux/cper.h        |   7 +++
>  2 files changed, 108 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/firmware/efi/cper.c b/drivers/firmware/efi/cper.c
> index 1491dd4..4e88885 100644
> --- a/drivers/firmware/efi/cper.c
> +++ b/drivers/firmware/efi/cper.c
> @@ -34,6 +34,12 @@
>  #include <linux/aer.h>
>  
>  #define INDENT_SP	" "
> +DEFINE_RAW_SPINLOCK(cper_loc_lock);
> +EXPORT_SYMBOL_GPL(cper_loc_lock);

Definitely a bad idea to export a spinlock. If all you need is to sync
against multiple callers of cper_mem_err_location(), simply grab that
spinlock in the function itself, without exporting it.

> +
> +static char mem_location[CPER_REC_LEN];
> +static char dimm_location[CPER_REC_LEN];
> +
>  /*
>   * CPER record ID need to be unique even after reboot, because record
>   * ID is used as index for ERST storage, while CPER records from
> @@ -57,11 +63,12 @@ static const char *cper_severity_strs[] = {
>  	"info",
>  };
>  
> -static const char *cper_severity_str(unsigned int severity)
> +const char *cper_severity_str(unsigned int severity)
>  {
>  	return severity < ARRAY_SIZE(cper_severity_strs) ?
>  		cper_severity_strs[severity] : "unknown";
>  }
> +EXPORT_SYMBOL_GPL(cper_severity_str);
>
>   * cper_print_bits - print strings for set bits
> @@ -196,55 +203,116 @@ static const char *cper_mem_err_type_strs[] = {
>  	"physical memory map-out event",
>  };
>  
> -static void cper_print_mem(const char *pfx, const struct cper_sec_mem_err *mem)
> +const char *cper_mem_err_type_str(unsigned int etype)
>  {
> -	if (mem->validation_bits & CPER_MEM_VALID_ERROR_STATUS)
> -		printk("%s""error_status: 0x%016llx\n", pfx, mem->error_status);
> -	if (mem->validation_bits & CPER_MEM_VALID_PA)
> -		printk("%s""physical_address: 0x%016llx\n",
> -		       pfx, mem->physical_addr);
> -	if (mem->validation_bits & CPER_MEM_VALID_PA_MASK)
> -		printk("%s""physical_address_mask: 0x%016llx\n",
> -		       pfx, mem->physical_addr_mask);
> +	return etype < ARRAY_SIZE(cper_mem_err_type_strs) ?
> +		cper_mem_err_type_strs[etype] : "unknown";
> +}
> +EXPORT_SYMBOL_GPL(cper_mem_err_type_str);
> +
> +char *cper_mem_err_location(const struct cper_sec_mem_err *mem)
> +{
> +	char *p;
> +	u32 n = 0;
> +
> +	memset(mem_location, 0, CPER_REC_LEN);
> +	p = mem_location;
>  	if (mem->validation_bits & CPER_MEM_VALID_NODE)
> -		pr_debug("node: %d\n", mem->node);
> +		n += sprintf(p + n, " node: %d", mem->node);
> +	if (n >= CPER_REC_LEN)
> +		goto end;

	n += vscnprintf(p + n, CPER_REC_LEN - n - 1, "... ", ...);

and you can drop those

	if (n >= CPER_REC_LEN)

tests.

vscnprintf() because it returns the actual number of characters written
and not what snprintf does and return the chars count it would've
written. (Thx Richard for the hint!).

>  	if (mem->validation_bits & CPER_MEM_VALID_CARD)
> -		pr_debug("card: %d\n", mem->card);
> +		n += sprintf(p + n, " card: %d", mem->card);
> +	if (n >= CPER_REC_LEN)
> +		goto end;
>  	if (mem->validation_bits & CPER_MEM_VALID_MODULE)
> -		pr_debug("module: %d\n", mem->module);
> +		n += sprintf(p + n, " module: %d", mem->module);
> +	if (n >= CPER_REC_LEN)
> +		goto end;
>  	if (mem->validation_bits & CPER_MEM_VALID_RANK_NUMBER)
> -		pr_debug("rank: %d\n", mem->rank);
> +		n += sprintf(p + n, " rank: %d", mem->rank);
> +	if (n >= CPER_REC_LEN)
> +		goto end;
>  	if (mem->validation_bits & CPER_MEM_VALID_BANK)
> -		pr_debug("bank: %d\n", mem->bank);
> +		n += sprintf(p + n, " bank: %d", mem->bank);
> +	if (n >= CPER_REC_LEN)
> +		goto end;
>  	if (mem->validation_bits & CPER_MEM_VALID_DEVICE)
> -		pr_debug("device: %d\n", mem->device);
> +		n += sprintf(p + n, " device: %d", mem->device);
> +	if (n >= CPER_REC_LEN)
> +		goto end;
>  	if (mem->validation_bits & CPER_MEM_VALID_ROW)
> -		pr_debug("row: %d\n", mem->row);
> +		n += sprintf(p + n, " row: %d", mem->row);
> +	if (n >= CPER_REC_LEN)
> +		goto end;
>  	if (mem->validation_bits & CPER_MEM_VALID_COLUMN)
> -		pr_debug("column: %d\n", mem->column);
> +		n += sprintf(p + n, " column: %d", mem->column);
> +	if (n >= CPER_REC_LEN)
> +		goto end;
>  	if (mem->validation_bits & CPER_MEM_VALID_BIT_POSITION)
> -		pr_debug("bit_position: %d\n", mem->bit_pos);
> +		n += sprintf(p + n, " bit_position: %d", mem->bit_pos);
> +	if (n >= CPER_REC_LEN)
> +		goto end;
>  	if (mem->validation_bits & CPER_MEM_VALID_REQUESTOR_ID)
> -		pr_debug("requestor_id: 0x%016llx\n", mem->requestor_id);
> +		n += sprintf(p + n, " requestor_id: 0x%016llx",
> +				mem->requestor_id);
> +	if (n >= CPER_REC_LEN)
> +		goto end;
>  	if (mem->validation_bits & CPER_MEM_VALID_RESPONDER_ID)
> -		pr_debug("responder_id: 0x%016llx\n", mem->responder_id);
> +		n += sprintf(p + n, " responder_id: 0x%016llx",
> +				mem->responder_id);
> +	if (n >= CPER_REC_LEN)
> +		goto end;
>  	if (mem->validation_bits & CPER_MEM_VALID_TARGET_ID)
> -		pr_debug("target_id: 0x%016llx\n", mem->target_id);
> +		n += sprintf(p + n, " target_id: 0x%016llx", mem->target_id);
> +end:
> +	return mem_location;
> +}
> +EXPORT_SYMBOL_GPL(cper_mem_err_location);
> +
> +char *cper_dimm_err_location(const struct cper_sec_mem_err *mem)
> +{
> +	const char *bank = NULL, *device = NULL;
> +
> +	memset(dimm_location, 0, CPER_REC_LEN);
> +	if (!(mem->validation_bits & CPER_MEM_VALID_MODULE_HANDLE))
> +		goto end;

a goto label which is used only once?? What's wrong with

		return dimm_location;

?

> +
> +	dmi_memdev_name(mem->mem_dev_handle, &bank, &device);
> +	if (bank != NULL && device != NULL)

	if (bank && device)

> +		snprintf(dimm_location, CPER_REC_LEN - 1,
> +			 "DIMM location: %s %s", bank, device);
> +	else
> +		snprintf(dimm_location, CPER_REC_LEN - 1, "DMI handle: 0x%.4x",

Why the DMI handle? Why not say: "unable to find location" or "location
not present in DMI" or something more user-friendly.

> +			 mem->mem_dev_handle);
> +end:
> +	return dimm_location;
> +}
> +EXPORT_SYMBOL_GPL(cper_dimm_err_location);
> +
> +static void cper_print_mem(const char *pfx, const struct cper_sec_mem_err *mem)
> +{
> +	unsigned long flags;
> +
> +	if (mem->validation_bits & CPER_MEM_VALID_ERROR_STATUS)
> +		printk("%s""error_status: 0x%016llx\n", pfx, mem->error_status);
> +	if (mem->validation_bits & CPER_MEM_VALID_PA)
> +		printk("%s""physical_address: 0x%016llx\n",
> +		       pfx, mem->physical_addr);
> +	if (mem->validation_bits & CPER_MEM_VALID_PA_MASK)
> +		printk("%s""physical_address_mask: 0x%016llx\n",
> +		       pfx, mem->physical_addr_mask);
> +	raw_spin_lock_irqsave(&cper_loc_lock, flags);
> +	pr_debug("%s", cper_mem_err_location(mem));
> +	raw_spin_unlock_irqrestore(&cper_loc_lock, flags);
>  	if (mem->validation_bits & CPER_MEM_VALID_ERROR_TYPE) {
>  		u8 etype = mem->error_type;
>  		printk("%s""error_type: %d, %s\n", pfx, etype,
> -		       etype < ARRAY_SIZE(cper_mem_err_type_strs) ?
> -		       cper_mem_err_type_strs[etype] : "unknown");
> -	}
> -	if (mem->validation_bits & CPER_MEM_VALID_MODULE_HANDLE) {
> -		const char *bank = NULL, *device = NULL;
> -		dmi_memdev_name(mem->mem_dev_handle, &bank, &device);
> -		if (bank != NULL && device != NULL)
> -			printk("%s""DIMM location: %s %s", pfx, bank, device);
> -		else
> -			printk("%s""DIMM DMI handle: 0x%.4x",
> -			       pfx, mem->mem_dev_handle);
> +		       cper_mem_err_type_str(etype));
>  	}
> +	raw_spin_lock_irqsave(&cper_loc_lock, flags);
> +	printk("%s%s", pfx, cper_dimm_err_location(mem));
> +	raw_spin_unlock_irqrestore(&cper_loc_lock, flags);
>  }
>  
>  static const char *cper_pcie_port_type_strs[] = {
> diff --git a/include/linux/cper.h b/include/linux/cper.h
> index 2fc0ec3..55c10db 100644
> --- a/include/linux/cper.h
> +++ b/include/linux/cper.h
> @@ -23,6 +23,8 @@
>  
>  #include <linux/uuid.h>
>  
> +extern struct raw_spinlock cper_loc_lock;
> +
>  /* CPER record signature and the size */
>  #define CPER_SIG_RECORD				"CPER"
>  #define CPER_SIG_SIZE				4
> @@ -35,6 +37,7 @@
>   */
>  #define CPER_RECORD_REV				0x0100
>  
> +#define CPER_REC_LEN				512

How did we come up with this? Some spec? 512 chars for an error record?
That's a bit too much in my book.

Thanks.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH 2/5] CPER: Adjust code flow of some functions

[Borislav Petkov]

On Mon, Apr 14, 2014 at 03:39:24PM +0200, Borislav Petkov wrote:
> Definitely a bad idea to export a spinlock. If all you need is to sync
> against multiple callers of cper_mem_err_location(), simply grab that
> spinlock in the function itself, without exporting it.
> 
> > +
> > +static char mem_location[CPER_REC_LEN];
> > +static char dimm_location[CPER_REC_LEN];

In thinking about this more, even with the proper synchronization,
cper_dimm_err_location() returns a pointer to this dimm_location string.
Now, imagine what happens if another caller grabs the lock and enters
cper_dimm_err_location() while dimm_location is still being accessed by
the tracepoint or the previous caller of cper_dimm_err_location...

IOW, you either need a synchronization at a higher level so that dumping
of dimm locations can be serialized or the higher callers (interrupt
handlers, etc) already give you that synchronization.

So you have to think about all possible call paths ending here and
*then* introduce proper sync.

Oh, and saying "No functional changes." in the commit message is a bit
misleading, don't you think?

:-)

Thanks.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH 2/5] CPER: Adjust code flow of some functions

[Chen, Gong]

[-- Attachment #1: Type: text/plain, Size: 2268 bytes --]

On Mon, Apr 14, 2014 at 03:39:24PM +0200, Borislav Petkov wrote:
> > +char *cper_mem_err_location(const struct cper_sec_mem_err *mem)
> > +{
> > +	char *p;
> > +	u32 n = 0;
> > +
> > +	memset(mem_location, 0, CPER_REC_LEN);
> > +	p = mem_location;
> >  	if (mem->validation_bits & CPER_MEM_VALID_NODE)
> > -		pr_debug("node: %d\n", mem->node);
> > +		n += sprintf(p + n, " node: %d", mem->node);
> > +	if (n >= CPER_REC_LEN)
> > +		goto end;
> 
> 	n += vscnprintf(p + n, CPER_REC_LEN - n - 1, "... ", ...);
> 
> and you can drop those
> 
> 	if (n >= CPER_REC_LEN)
> 
> tests.
> 
> vscnprintf() because it returns the actual number of characters written
> and not what snprintf does and return the chars count it would've
> written. (Thx Richard for the hint!).
You are absolutely right. This function is perfect for this usage.

> > +char *cper_dimm_err_location(const struct cper_sec_mem_err *mem)
> > +{
> > +	const char *bank = NULL, *device = NULL;
> > +
> > +	memset(dimm_location, 0, CPER_REC_LEN);
> > +	if (!(mem->validation_bits & CPER_MEM_VALID_MODULE_HANDLE))
> > +		goto end;
> 
> a goto label which is used only once?? What's wrong with
> 
> 		return dimm_location;
> 
I just feel repeating the same words looks ugly. OK, it is fine to me
to adopt your suggestion.

> > +	dmi_memdev_name(mem->mem_dev_handle, &bank, &device);
> > +	if (bank != NULL && device != NULL)
> 
> 	if (bank && device)
> 
> > +		snprintf(dimm_location, CPER_REC_LEN - 1,
> > +			 "DIMM location: %s %s", bank, device);
> > +	else
> > +		snprintf(dimm_location, CPER_REC_LEN - 1, "DMI handle: 0x%.4x",
> 
> Why the DMI handle? Why not say: "unable to find location" or "location
> not present in DMI" or something more user-friendly.
In essential, I use the suggestion from Tony Luck. DMI handle can't be found
so I just list the original info for it. Maybe I can merge them into one.

> > +#define CPER_REC_LEN				512
> 
> How did we come up with this? Some spec? 512 chars for an error record?
> That's a bit too much in my book.
> 
Because 128 is not enough once all fields in error record exist, and 256 looks
a little bit tough so I choose a bigger value. No spec for it. I just hope
it can contain everything.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 2/5] CPER: Adjust code flow of some functions

[Chen, Gong]

[-- Attachment #1: Type: text/plain, Size: 2082 bytes --]

On Mon, Apr 14, 2014 at 04:05:14PM +0200, Borislav Petkov wrote:
> Date: Mon, 14 Apr 2014 16:05:14 +0200
> From: Borislav Petkov <bp@alien8.de>
> To: "Chen, Gong" <gong.chen@linux.intel.com>
> Cc: tony.luck@intel.com, m.chehab@samsung.com, rostedt@goodmis.org,
>  linux-acpi@vger.kernel.org, arozansk@redhat.com
> Subject: Re: [PATCH 2/5] CPER: Adjust code flow of some functions
> User-Agent: Mutt/1.5.21 (2010-09-15)
> 
> On Mon, Apr 14, 2014 at 03:39:24PM +0200, Borislav Petkov wrote:
> > Definitely a bad idea to export a spinlock. If all you need is to sync
> > against multiple callers of cper_mem_err_location(), simply grab that
> > spinlock in the function itself, without exporting it.
> > 
> > > +
> > > +static char mem_location[CPER_REC_LEN];
> > > +static char dimm_location[CPER_REC_LEN];
> 
> In thinking about this more, even with the proper synchronization,
> cper_dimm_err_location() returns a pointer to this dimm_location string.
> Now, imagine what happens if another caller grabs the lock and enters
> cper_dimm_err_location() while dimm_location is still being accessed by
> the tracepoint or the previous caller of cper_dimm_err_location...
> 
> IOW, you either need a synchronization at a higher level so that dumping
> of dimm locations can be serialized or the higher callers (interrupt
> handlers, etc) already give you that synchronization.
That's why I export this spinlock because in another patch(3/5) I use
this spinlock to surround whole handling procedure of tracepoint.

If exporting this spinlock directly is too ugly, I can use an inline
function to get the same purpose.

> 
> So you have to think about all possible call paths ending here and
> *then* introduce proper sync.
> 
> Oh, and saying "No functional changes." in the commit message is a bit
> misleading, don't you think?
OK. Looks like this is not the 1st time you remind me not using so-called
"No functional change" :-). I have a different definition for functional
change ;-). It seems that I need to align my standard with you guys.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 2/5] CPER: Adjust code flow of some functions

[Borislav Petkov]

On Tue, Apr 15, 2014 at 05:24:42AM -0400, Chen, Gong wrote:
> That's why I export this spinlock because in another patch(3/5) I use
> this spinlock to surround whole handling procedure of tracepoint.
>
> If exporting this spinlock directly is too ugly, I can use an inline
> function to get the same purpose.

You still haven't answered...

> > So you have to think about all possible call paths ending here and
> > *then* introduce proper sync.
^^^^^^^^^^^^^

this question. Do we even *need* the locking? If so, why? What paths are
going to end there?

The answers to those questions will give you the correct synchronization.

Thanks.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH 2/5] CPER: Adjust code flow of some functions

[Borislav Petkov]

On Tue, Apr 15, 2014 at 05:19:44AM -0400, Chen, Gong wrote:
> > Why the DMI handle? Why not say: "unable to find location" or "location
> > not present in DMI" or something more user-friendly.
> In essential, I use the suggestion from Tony Luck. DMI handle can't be found
> so I just list the original info for it. Maybe I can merge them into one.

Yep. Make it user-friendly while showing the original info pls.

> > > +#define CPER_REC_LEN				512
> > 
> > How did we come up with this? Some spec? 512 chars for an error record?
> > That's a bit too much in my book.
> > 
> Because 128 is not enough once all fields in error record exist, and 256 looks
> a little bit tough so I choose a bigger value. No spec for it. I just hope

Tough? How? Not enough?

> it can contain everything.

Pls add a comment over this definition to state why we've chosen this
number exactly.

Thanks.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH 2/5] CPER: Adjust code flow of some functions

[Chen, Gong]

[-- Attachment #1: Type: text/plain, Size: 268 bytes --]

On Tue, Apr 15, 2014 at 08:02:59PM +0200, Borislav Petkov wrote:
> this question. Do we even *need* the locking? If so, why? What paths are
> going to end there?
> 
Yes, The sync can be avoided via separating buffers. I will update my patch
in the next version.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 2/5] CPER: Adjust code flow of some functions

[Chen, Gong]

[-- Attachment #1: Type: text/plain, Size: 1032 bytes --]

On Tue, Apr 15, 2014 at 08:05:54PM +0200, Borislav Petkov wrote:
> > Because 128 is not enough once all fields in error record exist, and 256 looks
> > a little bit tough so I choose a bigger value. No spec for it. I just hope
> 
> Tough? How? Not enough?
> 
We have different CPER error type. For example, for processor error type,
it has following definition:

struct cper_sec_proc_generic {
        __u64   validation_bits;
        __u8    proc_type;
        __u8    proc_isa;
        __u8    proc_error_type;
        __u8    operation;
        __u8    flags;
        __u8    level;
        __u16   reserved;
        __u64   cpu_version;
        char    cpu_brand[128];
        __u64   proc_id;
        __u64   target_addr;
        __u64   requestor_id;
        __u64   responder_id;
        __u64   ip;
};

If we want to show it in string format, 256 bytes should be not enough. But
by now we don't use this macro for processor error type so I will shrink it
to 256 bytes and add a comment for it.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/5] trace, RAS: Add basic RAS trace event

[Chen, Gong]

[-- Attachment #1: Type: text/plain, Size: 1726 bytes --]

On Wed, Apr 09, 2014 at 09:46:54PM +0200, Borislav Petkov wrote:
> 
> menuconfig RAS
> 	bool "Reliability, Availability, Serviceability features"
> 	help
> 	  <A nice text about what this is going to contain, i.e. RAS stuff
> 
How about this:

         Reliability, availability, and serviceability (RAS) is a computer
         hardware engineering term. Computers designed with higher levels
         of RAS have a multitude of features that protect data integrity
         and help them stay available for long periods of time without
         failure.

         Reliability can be defined as the probability that it will produce
         correct outputs up to some given time. Reliability is enhanced by
         features that help to avoid, detect and repair hardware faults.

         Availability is the probability a system is operational at a given
         time, i.e. the amount of time a device is actually operating as the
         percentage of total time it should be operating.

         Serviceability or maintainability is the simplicity and speed with
         which a system can be repaired or maintained; if the time to repair
         a failed system increases, then availability will decrease.

         Note that reliability and availability are distinct concepts:
         Reliability is a measure of the ability of a system to function
         correctly, including avoiding data corruption, whereas availability
         measures how often it is available for use, even though it may not
         be functioning correctly. For example, a server may run forever and
         so have ideal availability, but may be unreliable, with frequent
         data corruption.


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/5] trace, RAS: Add basic RAS trace event

[Borislav Petkov]

On Wed, Apr 16, 2014 at 02:33:01AM -0400, Chen, Gong wrote:
> On Wed, Apr 09, 2014 at 09:46:54PM +0200, Borislav Petkov wrote:
> > 
> > menuconfig RAS
> > 	bool "Reliability, Availability, Serviceability features"
> > 	help
> > 	  <A nice text about what this is going to contain, i.e. RAS stuff
> > 
> How about this:

Good. Just nitpicks below:

>          Reliability, availability, and serviceability (RAS) is a computer
>          hardware engineering term. Computers designed with higher levels
>          of RAS have a multitude of features that protect data integrity
>          and help them stay available for long periods of time without
>          failure.
> 
>          Reliability can be defined as the probability that it will produce

s/it/the system/. "it" is kinda misleading as to what we refer to.

>          correct outputs up to some given time. Reliability is enhanced by
>          features that help to avoid, detect and repair hardware faults.
> 
>          Availability is the probability a system is operational at a given
>          time, i.e. the amount of time a device is actually operating as the
>          percentage of total time it should be operating.
> 
>          Serviceability or maintainability is the simplicity and speed with
>          which a system can be repaired or maintained; if the time to repair
>          a failed system increases, then availability will decrease.

Nice!

>          Note that reliability and availability are distinct concepts:

capitalized: ... that Reliability and Availability are ...

>          Reliability is a measure of the ability of a system to function
>          correctly, including avoiding data corruption, whereas availability

ditto: Availability

>          measures how often it is available for use, even though it may not
>          be functioning correctly. For example, a server may run forever and
>          so have ideal availability, but may be unreliable, with frequent
>          data corruption.

Very good description! :-)

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH 2/5] CPER: Adjust code flow of some functions

[Borislav Petkov]

On Wed, Apr 16, 2014 at 01:01:58AM -0400, Chen, Gong wrote:
> Yes, The sync can be avoided via separating buffers.

But why do we even need to sync? IOW, we can land here from GHES and
EXTLOG, right? So we have two different paths, which means, we need
synchronization.

And yes, separating the buffers by having GHES and EXTLOG hand down
their own char buffers would make the whole thing very clean IMHO and
lockless. So definitely worth a try.

:-)

Thanks.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH 2/5] CPER: Adjust code flow of some functions

[Borislav Petkov]

On Wed, Apr 16, 2014 at 02:23:23AM -0400, Chen, Gong wrote:
> We have different CPER error type. For example, for processor error type,
> it has following definition:
> 
> struct cper_sec_proc_generic {
>         __u64   validation_bits;
>         __u8    proc_type;
>         __u8    proc_isa;
>         __u8    proc_error_type;
>         __u8    operation;
>         __u8    flags;
>         __u8    level;
>         __u16   reserved;
>         __u64   cpu_version;
>         char    cpu_brand[128];
>         __u64   proc_id;
>         __u64   target_addr;
>         __u64   requestor_id;
>         __u64   responder_id;
>         __u64   ip;
> };
> 
> If we want to show it in string format, 256 bytes should be not enough. But
> by now we don't use this macro for processor error type so I will shrink it
> to 256 bytes and add a comment for it.

Yeah, we don't have to always adhere to the spec if we feel it doesn't
make any sense. A lot of those fields above are purely useless and we
shouldn't carry them blindly to the outside.

For example, we don't need to carry cpu_brand[128] to the outside for
*every* error. Who even came up with this crap, is beyond me??? It's
like the cpu changes brand on every other error or what? You harvest
this info only *once* from /proc/cpuinfo. cpu_version too. And so on and
so on...

Please sanity-check stuff like that before hardcoding it into the
tracepoint. If it is in the spec it doesn't always mean it makes sense.
We need to carry out only the minimum amount of information of each
error which is actually getting used in userspace, for additional RAS
actions. Carrying fat blobs just because the spec says so is simply
wrong.

Thanks.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH 2/5] CPER: Adjust code flow of some functions

[Chen, Gong]

[-- Attachment #1: Type: text/plain, Size: 1147 bytes --]

On Wed, Apr 16, 2014 at 03:28:18PM +0200, Borislav Petkov wrote:
> Yeah, we don't have to always adhere to the spec if we feel it doesn't
> make any sense. A lot of those fields above are purely useless and we
> shouldn't carry them blindly to the outside.
> 
> For example, we don't need to carry cpu_brand[128] to the outside for
> *every* error. Who even came up with this crap, is beyond me??? It's
> like the cpu changes brand on every other error or what? You harvest
> this info only *once* from /proc/cpuinfo. cpu_version too. And so on and
> so on...
> 
> Please sanity-check stuff like that before hardcoding it into the
> tracepoint. If it is in the spec it doesn't always mean it makes sense.
> We need to carry out only the minimum amount of information of each
> error which is actually getting used in userspace, for additional RAS
> actions. Carrying fat blobs just because the spec says so is simply
> wrong.
> 
I agree. But at least for memory, I want to show them all. Maybe
requestor_id/responder_id/target_id are not so important. But in
reality, I've never seen they are valid. So just leave them there.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]