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

[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

Add new eMCA trace event interface V2

[Chen, Gong]

[PATCH 1/2 v2] x86, MCE: Fix a bug in CMCI handler
[PATCH 2/5 v2] CPER: Adjust code flow of some functions
[PATCH 3/5 v2] trace, RAS: Add eMCA trace event interface
[PATCH 4/5 v2] 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.

v2 -> v1: merge the comments from Tony Luck & Borislav Petkov

BTW, in patch 2 & 3 I don't put some strings as stack parameter to avoid
potential overflow. After all they are too big (512 bytes in total).

[PATCH 1/2 v2] x86, MCE: Fix a bug in CMCI handler

[Chen, Gong]

This bug is introduced by me in commit 27f6c573e0. I forget
to execute put_cpu_var operation after get_cpu_var. Fix it
via this_cpu_write instead of get_cpu_var.

v2 -> v1: Separate cleanup from bug fix.

Signed-off-by: Chen, Gong <gong.chen@linux.intel.com>
Suggested-by: H. Peter Anvin <hpa@zytor.com>
---
 arch/x86/kernel/cpu/mcheck/mce.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
index eeee23f..68317c8 100644
--- a/arch/x86/kernel/cpu/mcheck/mce.c
+++ b/arch/x86/kernel/cpu/mcheck/mce.c
@@ -598,7 +598,6 @@ void machine_check_poll(enum mcp_flags flags, mce_banks_t *b)
 {
 	struct mce m;
 	int i;
-	unsigned long *v;
 
 	this_cpu_inc(mce_poll_count);
 
@@ -618,8 +617,7 @@ void machine_check_poll(enum mcp_flags flags, mce_banks_t *b)
 		if (!(m.status & MCI_STATUS_VAL))
 			continue;
 
-		v = &get_cpu_var(mce_polled_error);
-		set_bit(0, v);
+		this_cpu_write(mce_polled_error, 1);
 		/*
 		 * Uncorrected or signalled events are handled by the exception
 		 * handler when it is enabled, so don't process those here.
-- 
1.9.0

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

[Chen, Gong]

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

v2 -> v1: Use scnprintf to simplify codes.

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

diff --git a/drivers/firmware/efi/cper.c b/drivers/firmware/efi/cper.c
index 1491dd4..951049b 100644
--- a/drivers/firmware/efi/cper.c
+++ b/drivers/firmware/efi/cper.c
@@ -34,6 +34,10 @@
 #include <linux/aer.h>
 
 #define INDENT_SP	" "
+
+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 +61,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 +201,100 @@ 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);
+
+void cper_mem_err_location(const struct cper_sec_mem_err *mem, char *msg)
+{
+	u32 n = 0, len;
+
+	if (!msg)
+		return;
+
+	len = strlen(msg);
+	memset(msg, 0, len);
+
 	if (mem->validation_bits & CPER_MEM_VALID_NODE)
-		pr_debug("node: %d\n", mem->node);
+		n += scnprintf(msg + n, len - n - 1, " node: %d", mem->node);
 	if (mem->validation_bits & CPER_MEM_VALID_CARD)
-		pr_debug("card: %d\n", mem->card);
+		n += scnprintf(msg + n, len - n - 1, " card: %d", mem->card);
 	if (mem->validation_bits & CPER_MEM_VALID_MODULE)
-		pr_debug("module: %d\n", mem->module);
+		n += scnprintf(msg + n, len - n - 1, " module: %d",
+			       mem->module);
 	if (mem->validation_bits & CPER_MEM_VALID_RANK_NUMBER)
-		pr_debug("rank: %d\n", mem->rank);
+		n += scnprintf(msg + n, len - n - 1, " rank: %d", mem->rank);
 	if (mem->validation_bits & CPER_MEM_VALID_BANK)
-		pr_debug("bank: %d\n", mem->bank);
+		n += scnprintf(msg + n, len - n - 1, " bank: %d", mem->bank);
 	if (mem->validation_bits & CPER_MEM_VALID_DEVICE)
-		pr_debug("device: %d\n", mem->device);
+		n += scnprintf(msg + n, len - n - 1, " device: %d",
+			       mem->device);
 	if (mem->validation_bits & CPER_MEM_VALID_ROW)
-		pr_debug("row: %d\n", mem->row);
+		n += scnprintf(msg + n, len - n - 1, " row: %d", mem->row);
 	if (mem->validation_bits & CPER_MEM_VALID_COLUMN)
-		pr_debug("column: %d\n", mem->column);
+		n += scnprintf(msg + n, len - n - 1, " column: %d",
+			       mem->column);
 	if (mem->validation_bits & CPER_MEM_VALID_BIT_POSITION)
-		pr_debug("bit_position: %d\n", mem->bit_pos);
+		n += scnprintf(msg + n, len - n - 1, " bit_position: %d",
+			       mem->bit_pos);
 	if (mem->validation_bits & CPER_MEM_VALID_REQUESTOR_ID)
-		pr_debug("requestor_id: 0x%016llx\n", mem->requestor_id);
+		n += scnprintf(msg + n, len - n - 1,
+			       " requestor_id: 0x%016llx", mem->requestor_id);
 	if (mem->validation_bits & CPER_MEM_VALID_RESPONDER_ID)
-		pr_debug("responder_id: 0x%016llx\n", mem->responder_id);
+		n += scnprintf(msg + n, len - n - 1,
+			       " responder_id: 0x%016llx", mem->responder_id);
 	if (mem->validation_bits & CPER_MEM_VALID_TARGET_ID)
-		pr_debug("target_id: 0x%016llx\n", mem->target_id);
+		scnprintf(msg + n, len - n - 1, " target_id: 0x%016llx",
+			  mem->target_id);
+}
+EXPORT_SYMBOL_GPL(cper_mem_err_location);
+
+void cper_dimm_err_location(const struct cper_sec_mem_err *mem, char *msg)
+{
+	const char *bank = NULL, *device = NULL;
+	int len;
+
+	if (!msg)
+		return;
+
+	len = strlen(msg);
+	memset(msg, 0, len);
+
+	if (!(mem->validation_bits & CPER_MEM_VALID_MODULE_HANDLE))
+		return;
+
+	dmi_memdev_name(mem->mem_dev_handle, &bank, &device);
+	if (bank && device)
+		snprintf(msg, len - 1, "DIMM location: %s %s", bank, device);
+	else
+		snprintf(msg, len - 1,
+			 "DIMM location: not present. DMI handle: 0x%.4x",
+			 mem->mem_dev_handle);
+}
+EXPORT_SYMBOL_GPL(cper_dimm_err_location);
+
+static void cper_print_mem(const char *pfx, const struct cper_sec_mem_err *mem)
+{
+	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);
+	cper_mem_err_location(mem, mem_location);
+	pr_debug("%s", mem_location);
 	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));
 	}
+	cper_dimm_err_location(mem, dimm_location);
+	printk("%s%s", pfx, dimm_location);
 }
 
 static const char *cper_pcie_port_type_strs[] = {
diff --git a/include/linux/cper.h b/include/linux/cper.h
index 2fc0ec3..038cc11 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
@@ -36,6 +38,12 @@
 #define CPER_RECORD_REV				0x0100
 
 /*
+ * CPER record length is variable depending on differnet error type.
+ * By now it is mainly used for memory error type. 256 bytes should
+ * be enough. Increase this value once it is not enough.
+ */
+#define CPER_REC_LEN				256
+/*
  * Severity difinition for error_severity in struct cper_record_header
  * and section_severity in struct cper_section_descriptor
  */
@@ -395,7 +403,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);
+void cper_mem_err_location(const struct cper_sec_mem_err *mem, char *msg);
+void cper_dimm_err_location(const struct cper_sec_mem_err *mem, char *msg);
 
 #endif
-- 
1.9.0

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

[Chen, Gong]

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

v2 -> v1: spinlock is not needed anymore.

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

diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
index ab686b3..0023668 100644
--- a/drivers/acpi/Kconfig
+++ b/drivers/acpi/Kconfig
@@ -367,6 +367,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..6192ec1 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 */
 
@@ -43,7 +44,11 @@ struct extlog_l1_head {
 
 static int old_edac_report_status;
 
+static char mem_location[CPER_REC_LEN];
+static char dimm_location[CPER_REC_LEN];
+
 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 +74,28 @@ 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;
+
+	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;
+	}
+
+	cper_mem_err_location(mem, mem_location);
+	cper_dimm_err_location(mem, dimm_location);
+	trace_extlog_mem_event(etype, dimm_location, fru_id, fru_text,
+			       err_count, severity, phy_addr, mem_location);
+}
+
 static struct acpi_generic_status *extlog_elog_entry_check(int cpu, int bank)
 {
 	int idx;
@@ -137,8 +164,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 +179,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 63e76a7..81cf0b2 100644
--- a/drivers/ras/Kconfig
+++ b/drivers/ras/Kconfig
@@ -32,6 +32,6 @@ menuconfig RAS
 if RAS
 config RAS_TRACE
 	def_bool y
-	depends on EDAC_MM_EDAC
+	depends on EDAC_MM_EDAC || ACPI_EXTLOG
 
 endif # RAS
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 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 6192ec1..860d5ce 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>
 
@@ -65,6 +66,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))
 
@@ -180,21 +184,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;
@@ -230,6 +237,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,
 };
@@ -240,6 +272,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;
 
@@ -303,6 +336,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.
@@ -315,6 +356,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);
@@ -340,6 +383,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 81cf0b2..1b7a914 100644
--- a/drivers/ras/Kconfig
+++ b/drivers/ras/Kconfig
@@ -32,6 +32,6 @@ menuconfig RAS
 if RAS
 config RAS_TRACE
 	def_bool y
-	depends on EDAC_MM_EDAC || ACPI_EXTLOG
+	depends on EDAC_MM_EDAC || ACPI_EXTLOG || PCIEAER
 
 endif # RAS
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 2/5 v2] CPER: Adjust code flow of some functions

[Borislav Petkov]

On Thu, Apr 17, 2014 at 02:28:36AM -0400, Chen, Gong wrote:
> Some codes can be reorganzied as a common function for
> other usages.
> 
> v2 -> v1: Use scnprintf to simplify codes.
> 
> Signed-off-by: Chen, Gong <gong.chen@linux.intel.com>
> ---
>  drivers/firmware/efi/cper.c | 116 +++++++++++++++++++++++++++++++-------------
>  include/linux/cper.h        |  12 +++++
>  2 files changed, 95 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/firmware/efi/cper.c b/drivers/firmware/efi/cper.c
> index 1491dd4..951049b 100644
> --- a/drivers/firmware/efi/cper.c
> +++ b/drivers/firmware/efi/cper.c
> @@ -34,6 +34,10 @@
>  #include <linux/aer.h>
>  
>  #define INDENT_SP	" "
> +
> +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 +61,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 +201,100 @@ 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";

Btw, while you're at it, please fix those string arrays defitions to be

static const char * const

as they should be. This is cper_severity_strs, cper_mem_err_type_strs
and cper_pcie_port_type_strs. You can also drop the "cper_" prefix as
they're static.

> +}
> +EXPORT_SYMBOL_GPL(cper_mem_err_type_str);
> +
> +void cper_mem_err_location(const struct cper_sec_mem_err *mem, char *msg)
> +{
> +	u32 n = 0, len;
> +
> +	if (!msg)
> +		return;
> +
> +	len = strlen(msg);
> +	memset(msg, 0, len);

Have you even tested this???

[    0.104006] ------------[ cut here ]------------
[    0.108012] WARNING: CPU: 0 PID: 1 at lib/vsprintf.c:1660 vsnprintf+0x551/0x560()
[    0.112006] Modules linked in:
[    0.114167] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 3.15.0-rc1+ #13
[    0.116006] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
[    0.120025]  0000000000000009 ffff88007bc4bd78 ffffffff815e3c68 0000000000000000
[    0.130251]  ffff88007bc4bdb0 ffffffff8104d10c 00000000ffffffff 0000000000000000
[    0.136006]  ffffffff82bd42c0 0000000000000000 0000000000000000 ffff88007bc4bdc0
[    0.140881] Call Trace:
[    0.142555]  [<ffffffff815e3c68>] dump_stack+0x4e/0x7a
[    0.144009]  [<ffffffff8104d10c>] warn_slowpath_common+0x8c/0xc0
[    0.148034]  [<ffffffff8104d1fa>] warn_slowpath_null+0x1a/0x20
[    0.152013]  [<ffffffff812be301>] vsnprintf+0x551/0x560
[    0.156013]  [<ffffffff812be31d>] vscnprintf+0xd/0x30
[    0.160012]  [<ffffffff812be379>] scnprintf+0x39/0x40
[    0.164009]  [<ffffffff815e4f92>] cper_mem_err_location.part.1+0x6a/0x253
[    0.168009]  [<ffffffff81496a11>] cper_print_mem+0x41/0x100
[    0.172018]  [<ffffffff815db7c0>] ? rest_init+0x140/0x140
[    0.176035]  [<ffffffff815db806>] kernel_init+0x46/0x120
[    0.180009]  [<ffffffff815ec8ec>] ret_from_fork+0x7c/0xb0
[    0.182780]  [<ffffffff815db7c0>] ? rest_init+0x140/0x140
[    0.184012] ---[ end trace d69126aad4cf21bf ]---

Think about what happens the first time you call this function and
strlen returns 0 because the string is empty.

I enforced the first call to that function in kvm just so that I can
test your stuff but I don't see why this won't happen on real hardware
either.

Gong, I would strongly urge you to spend more time
and care on your patches instead of rushing them out.
ea431643d6c38728195e2c456801c3ef66bb9991 shouldnt've happened if you
tested your stuff more thoroughly and it cost me a whole day to dig out
what exactly was happening and talking to bug reporters.

Please be more responsible when preparing your patches and think hard
about each change you're doing. I'm not in the mood to review your stuff
with a magnifying glass and look for landmines.

Please!

>  	if (mem->validation_bits & CPER_MEM_VALID_NODE)
> -		pr_debug("node: %d\n", mem->node);
> +		n += scnprintf(msg + n, len - n - 1, " node: %d", mem->node);
>  	if (mem->validation_bits & CPER_MEM_VALID_CARD)
> -		pr_debug("card: %d\n", mem->card);
> +		n += scnprintf(msg + n, len - n - 1, " card: %d", mem->card);
>  	if (mem->validation_bits & CPER_MEM_VALID_MODULE)
> -		pr_debug("module: %d\n", mem->module);
> +		n += scnprintf(msg + n, len - n - 1, " module: %d",
> +			       mem->module);
>  	if (mem->validation_bits & CPER_MEM_VALID_RANK_NUMBER)
> -		pr_debug("rank: %d\n", mem->rank);
> +		n += scnprintf(msg + n, len - n - 1, " rank: %d", mem->rank);
>  	if (mem->validation_bits & CPER_MEM_VALID_BANK)
> -		pr_debug("bank: %d\n", mem->bank);
> +		n += scnprintf(msg + n, len - n - 1, " bank: %d", mem->bank);
>  	if (mem->validation_bits & CPER_MEM_VALID_DEVICE)
> -		pr_debug("device: %d\n", mem->device);
> +		n += scnprintf(msg + n, len - n - 1, " device: %d",
> +			       mem->device);
>  	if (mem->validation_bits & CPER_MEM_VALID_ROW)
> -		pr_debug("row: %d\n", mem->row);
> +		n += scnprintf(msg + n, len - n - 1, " row: %d", mem->row);
>  	if (mem->validation_bits & CPER_MEM_VALID_COLUMN)
> -		pr_debug("column: %d\n", mem->column);
> +		n += scnprintf(msg + n, len - n - 1, " column: %d",
> +			       mem->column);
>  	if (mem->validation_bits & CPER_MEM_VALID_BIT_POSITION)
> -		pr_debug("bit_position: %d\n", mem->bit_pos);
> +		n += scnprintf(msg + n, len - n - 1, " bit_position: %d",
> +			       mem->bit_pos);
>  	if (mem->validation_bits & CPER_MEM_VALID_REQUESTOR_ID)
> -		pr_debug("requestor_id: 0x%016llx\n", mem->requestor_id);
> +		n += scnprintf(msg + n, len - n - 1,
> +			       " requestor_id: 0x%016llx", mem->requestor_id);
>  	if (mem->validation_bits & CPER_MEM_VALID_RESPONDER_ID)
> -		pr_debug("responder_id: 0x%016llx\n", mem->responder_id);
> +		n += scnprintf(msg + n, len - n - 1,
> +			       " responder_id: 0x%016llx", mem->responder_id);
>  	if (mem->validation_bits & CPER_MEM_VALID_TARGET_ID)
> -		pr_debug("target_id: 0x%016llx\n", mem->target_id);
> +		scnprintf(msg + n, len - n - 1, " target_id: 0x%016llx",
> +			  mem->target_id);
> +}
> +EXPORT_SYMBOL_GPL(cper_mem_err_location);
> +
> +void cper_dimm_err_location(const struct cper_sec_mem_err *mem, char *msg)
> +{
> +	const char *bank = NULL, *device = NULL;
> +	int len;
> +
> +	if (!msg)
> +		return;
> +
> +	len = strlen(msg);
> +	memset(msg, 0, len);
> +
> +	if (!(mem->validation_bits & CPER_MEM_VALID_MODULE_HANDLE))
> +		return;
> +
> +	dmi_memdev_name(mem->mem_dev_handle, &bank, &device);
> +	if (bank && device)
> +		snprintf(msg, len - 1, "DIMM location: %s %s", bank, device);
> +	else
> +		snprintf(msg, len - 1,
> +			 "DIMM location: not present. DMI handle: 0x%.4x",
> +			 mem->mem_dev_handle);
> +}
> +EXPORT_SYMBOL_GPL(cper_dimm_err_location);
> +
> +static void cper_print_mem(const char *pfx, const struct cper_sec_mem_err *mem)
> +{
> +	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);
> +	cper_mem_err_location(mem, mem_location);
> +	pr_debug("%s", mem_location);
>  	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));
>  	}
> +	cper_dimm_err_location(mem, dimm_location);
> +	printk("%s%s", pfx, dimm_location);

???

>  }
>  
>  static const char *cper_pcie_port_type_strs[] = {
> diff --git a/include/linux/cper.h b/include/linux/cper.h
> index 2fc0ec3..038cc11 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;
> +

Forgot to remove the spinlock.

See what I mean? Slow down for <deity>'s sake and *think* before sending
out!

>  /* CPER record signature and the size */
>  #define CPER_SIG_RECORD				"CPER"
>  #define CPER_SIG_SIZE				4
> @@ -36,6 +38,12 @@
>  #define CPER_RECORD_REV				0x0100
>  
>  /*
> + * CPER record length is variable depending on differnet error type.

s/differnet/different/

> + * By now it is mainly used for memory error type. 256 bytes should
> + * be enough. Increase this value once it is not enough.

How about this instead:

/*
 * CPER record length contains the CPER fields which are relevant for further
 * handling of a memory error in userspace (we don't carry all the fields
 * defined in the UEFI spec because some of them don't make any sense.
 * Currently, a length of 256 should be more than enough.
 */

> + */
> +#define CPER_REC_LEN				256
> +/*
>   * Severity difinition for error_severity in struct cper_record_header
>   * and section_severity in struct cper_section_descriptor
>   */
> @@ -395,7 +403,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);
> +void cper_mem_err_location(const struct cper_sec_mem_err *mem, char *msg);
> +void cper_dimm_err_location(const struct cper_sec_mem_err *mem, char *msg);


-- 
Regards/Gruss,
    Boris.

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

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

[Chen, Gong]

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

On Tue, Apr 22, 2014 at 03:54:22PM +0200, Borislav Petkov wrote:
> > +void cper_mem_err_location(const struct cper_sec_mem_err *mem, char *msg)
> > +{
> > +	u32 n = 0, len;
> > +
> > +	if (!msg)
> > +		return;
> > +
> > +	len = strlen(msg);
> > +	memset(msg, 0, len);
> 
> Have you even tested this???
> 
> [    0.104006] ------------[ cut here ]------------
> [    0.108012] WARNING: CPU: 0 PID: 1 at lib/vsprintf.c:1660 vsnprintf+0x551/0x560()
> [    0.112006] Modules linked in:
> [    0.114167] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 3.15.0-rc1+ #13
> [    0.116006] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
> [    0.120025]  0000000000000009 ffff88007bc4bd78 ffffffff815e3c68 0000000000000000
> [    0.130251]  ffff88007bc4bdb0 ffffffff8104d10c 00000000ffffffff 0000000000000000
> [    0.136006]  ffffffff82bd42c0 0000000000000000 0000000000000000 ffff88007bc4bdc0
> [    0.140881] Call Trace:
> [    0.142555]  [<ffffffff815e3c68>] dump_stack+0x4e/0x7a
> [    0.144009]  [<ffffffff8104d10c>] warn_slowpath_common+0x8c/0xc0
> [    0.148034]  [<ffffffff8104d1fa>] warn_slowpath_null+0x1a/0x20
> [    0.152013]  [<ffffffff812be301>] vsnprintf+0x551/0x560
> [    0.156013]  [<ffffffff812be31d>] vscnprintf+0xd/0x30
> [    0.160012]  [<ffffffff812be379>] scnprintf+0x39/0x40
> [    0.164009]  [<ffffffff815e4f92>] cper_mem_err_location.part.1+0x6a/0x253
> [    0.168009]  [<ffffffff81496a11>] cper_print_mem+0x41/0x100
> [    0.172018]  [<ffffffff815db7c0>] ? rest_init+0x140/0x140
> [    0.176035]  [<ffffffff815db806>] kernel_init+0x46/0x120
> [    0.180009]  [<ffffffff815ec8ec>] ret_from_fork+0x7c/0xb0
> [    0.182780]  [<ffffffff815db7c0>] ? rest_init+0x140/0x140
> [    0.184012] ---[ end trace d69126aad4cf21bf ]---
> 
> Think about what happens the first time you call this function and
> strlen returns 0 because the string is empty.
> 
> I enforced the first call to that function in kvm just so that I can
> test your stuff but I don't see why this won't happen on real hardware
> either.

When I have the first glance on the call trace, I feel strange because I
have tested it for many times on real machine, until I noticed that once
lenght is 0, the evaluation via scnprintf will be a disaster.

BTW, because I can't figure out what field looks more important than
others, I have to keep all of them until some guy tells me something new.

> 
> Gong, I would strongly urge you to spend more time
> and care on your patches instead of rushing them out.
> ea431643d6c38728195e2c456801c3ef66bb9991 shouldnt've happened if you
> tested your stuff more thoroughly and it cost me a whole day to dig out
> what exactly was happening and talking to bug reporters.
> 
Thanks very much for your help for it. I do need to be more careful to
avoid similiar stupid thing happened again.

> > +static void cper_print_mem(const char *pfx, const struct cper_sec_mem_err *mem)
> > +{
> > +	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);
> > +	cper_mem_err_location(mem, mem_location);
> > +	pr_debug("%s", mem_location);
> >  	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));
> >  	}
> > +	cper_dimm_err_location(mem, dimm_location);
> > +	printk("%s%s", pfx, dimm_location);
> 
> ???
Here you mean the last line?

printk("%s%s", pfx, dimm_location);

If so, I have to say I follow previous design style in APEI series drivers.
pfx is a prefix to tell printk what priority for this print. It can be
changed under different conditions.
> 
> >  }
> >  
> >  static const char *cper_pcie_port_type_strs[] = {
> > diff --git a/include/linux/cper.h b/include/linux/cper.h
> > index 2fc0ec3..038cc11 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;
> > +
> 
> Forgot to remove the spinlock.
> 
OK, gotta it.


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