[PATCH -v2 0/3] Report APEI GHES error information via printk

[PATCH -v2 0/3] Report APEI GHES error information via printk

[Huang Ying]

Hi, Len,

Although we have no consensus on whether we need a hardware error specific
reporting interface. We have the consensus that we need to report hardware
error information via printk. This patchset adds that to APEI GHES.

v2:

- Some minor adjustment of PCIe error section definition and printk format

[PATCH -v2 1/3] Add CPER PCIe error section structure and constants definition
[PATCH -v2 2/3] ACPI, APEI, Add APEI generic error status print support
[PATCH -v2 3/3] ACPI, APEI, report GHES error information via printk

Best Regards,
Huang Ying

[PATCH -v2 1/3] Add CPER PCIe error section structure and constants definition

[Huang Ying]

On some machine, PCIe error is reported via APEI (ACPI Platform Error
Interface).  The error data is passed from firmware to Linux via CPER
PCIe error section structure.

This patch adds CPER PCIe error section structure and constants
definition.

Signed-off-by: Huang Ying <ying.huang@intel.com>
---
 include/linux/cper.h |   76 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 76 insertions(+)

--- a/include/linux/cper.h
+++ b/include/linux/cper.h
@@ -201,6 +201,47 @@
 	UUID_LE(0x036F84E1, 0x7F37, 0x428c, 0xA7, 0x9E, 0x57, 0x5F,	\
 		0xDF, 0xAA, 0x84, 0xEC)
 
+#define CPER_PROC_VALID_TYPE			0x0001
+#define CPER_PROC_VALID_ISA			0x0002
+#define CPER_PROC_VALID_ERROR_TYPE		0x0004
+#define CPER_PROC_VALID_OPERATION		0x0008
+#define CPER_PROC_VALID_FLAGS			0x0010
+#define CPER_PROC_VALID_LEVEL			0x0020
+#define CPER_PROC_VALID_VERSION			0x0040
+#define CPER_PROC_VALID_BRAND_INFO		0x0080
+#define CPER_PROC_VALID_ID			0x0100
+#define CPER_PROC_VALID_TARGET_ADDRESS		0x0200
+#define CPER_PROC_VALID_REQUESTOR_ID		0x0400
+#define CPER_PROC_VALID_RESPONDER_ID		0x0800
+#define CPER_PROC_VALID_IP			0x1000
+
+#define CPER_MEM_VALID_ERROR_STATUS		0x0001
+#define CPER_MEM_VALID_PHYSICAL_ADDRESS		0x0002
+#define CPER_MEM_VALID_PHYSICAL_ADDRESS_MASK	0x0004
+#define CPER_MEM_VALID_NODE			0x0008
+#define CPER_MEM_VALID_CARD			0x0010
+#define CPER_MEM_VALID_MODULE			0x0020
+#define CPER_MEM_VALID_BANK			0x0040
+#define CPER_MEM_VALID_DEVICE			0x0080
+#define CPER_MEM_VALID_ROW			0x0100
+#define CPER_MEM_VALID_COLUMN			0x0200
+#define CPER_MEM_VALID_BIT_POSITION		0x0400
+#define CPER_MEM_VALID_REQUESTOR_ID		0x0800
+#define CPER_MEM_VALID_RESPONDER_ID		0x1000
+#define CPER_MEM_VALID_TARGET_ID		0x2000
+#define CPER_MEM_VALID_ERROR_TYPE		0x4000
+
+#define CPER_PCIE_VALID_PORT_TYPE		0x0001
+#define CPER_PCIE_VALID_VERSION			0x0002
+#define CPER_PCIE_VALID_COMMAND_STATUS		0x0004
+#define CPER_PCIE_VALID_DEVICE_ID		0x0008
+#define CPER_PCIE_VALID_SERIAL_NUMBER		0x0010
+#define CPER_PCIE_VALID_BRIDGE_CONTROL_STATUS	0x0020
+#define CPER_PCIE_VALID_CAPABILITY		0x0040
+#define CPER_PCIE_VALID_AER_INFO		0x0080
+
+#define CPER_PCIE_SLOT_SHIFT			3
+
 /*
  * All tables and structs must be byte-packed to match CPER
  * specification, since the tables are provided by the system BIOS
@@ -306,6 +347,41 @@ struct cper_sec_mem_err {
 	__u8	error_type;
 };
 
+struct cper_sec_pcie {
+	__u64		validation_bits;
+	__u32		port_type;
+	struct {
+		__u8	minor;
+		__u8	major;
+		__u8	reserved[2];
+	}		version;
+	__u16		command;
+	__u16		status;
+	__u32		reserved;
+	struct {
+		__u16	vendor_id;
+		__u16	device_id;
+		__u8	class_code[3];
+		__u8	function;
+		__u8	device;
+		__u16	segment;
+		__u8	bus;
+		__u8	secondary_bus;
+		__u16	slot;
+		__u8	reserved;
+	}		device_id;
+	struct {
+		__u32	lower;
+		__u32	upper;
+	}		serial_number;
+	struct {
+		__u16	secondary_status;
+		__u16	control;
+	}		bridge;
+	__u8	capability[60];
+	__u8	aer_info[96];
+};
+
 /* Reset to default packing */
 #pragma pack()
 

[PATCH -v2 2/3] ACPI, APEI, Add APEI generic error status print support

[Huang Ying]

printk is one of the methods to report hardware errors to user space.
Hardware error information reported by firmware to Linux kernel is in
the format of APEI generic error status (struct
acpi_hes_generic_status).  This patch adds print support for the
format, so that the corresponding hardware error information can be
reported to user space via printk.

PCIe AER information print is not implemented yet.  Will refactor the
original PCIe AER information printing code to avoid code duplicating.

Signed-off-by: Huang Ying <ying.huang@intel.com>
---
 drivers/acpi/apei/apei-internal.h |    2 
 drivers/acpi/apei/cper.c          |  299 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 301 insertions(+)

--- a/drivers/acpi/apei/apei-internal.h
+++ b/drivers/acpi/apei/apei-internal.h
@@ -109,6 +109,8 @@ static inline u32 apei_estatus_len(struc
 		return sizeof(*estatus) + estatus->data_length;
 }
 
+void apei_estatus_print(const char *pfx,
+			const struct acpi_hest_generic_status *estatus);
 int apei_estatus_check_header(const struct acpi_hest_generic_status *estatus);
 int apei_estatus_check(const struct acpi_hest_generic_status *estatus);
 #endif
--- a/drivers/acpi/apei/cper.c
+++ b/drivers/acpi/apei/cper.c
@@ -30,6 +30,9 @@
 #include <linux/cper.h>
 #include <linux/acpi.h>
 
+#define pr_pfx(pfx, fmt, ...)			\
+	printk("%s" fmt, pfx, ##__VA_ARGS__)
+
 /*
  * CPER record ID need to be unique even after reboot, because record
  * ID is used as index for ERST storage, while CPER records from
@@ -46,6 +49,302 @@ u64 cper_next_record_id(void)
 }
 EXPORT_SYMBOL_GPL(cper_next_record_id);
 
+static const char *cper_severity_strs[] = {
+	[CPER_SEV_RECOVERABLE]		= "recoverable",
+	[CPER_SEV_FATAL]		=  "fatal",
+	[CPER_SEV_CORRECTED]		= "corrected",
+	[CPER_SEV_INFORMATIONAL]	= "info",
+};
+
+static const char *cper_severity_str(unsigned int severity)
+{
+	return severity < ARRAY_SIZE(cper_severity_strs) ?
+		cper_severity_strs[severity] : "unknown";
+}
+
+static void cper_print_bits(const char *pfx, unsigned int bits,
+			    const char *strs[], unsigned int strs_size)
+{
+	int i, len = 0;
+	const char *str;
+
+	for (i = 0; i < strs_size; i++) {
+		if (!(bits & (1U << i)))
+			continue;
+		str = strs[i];
+		if (len && len + strlen(str) + 2 > 80) {
+			printk("\n");
+			len = 0;
+		}
+		if (!len)
+			len = pr_pfx(pfx, "%s", str);
+		else
+			len += printk(", %s", str);
+	}
+	if (len)
+		printk("\n");
+}
+
+static const char *cper_proc_type_strs[] = {
+	"IA32/X64",
+	"IA64",
+};
+
+static const char *cper_proc_isa_strs[] = {
+	"IA32",
+	"IA64",
+	"X64",
+};
+
+static const char *cper_proc_error_type_strs[] = {
+	"cache error",
+	"TLB error",
+	"bus error",
+	"micro-architectural error",
+};
+
+static const char *cper_proc_op_strs[] = {
+	"unknown or generic",
+	"data read",
+	"data write",
+	"instruction execution",
+};
+
+static const char *cper_proc_flag_strs[] = {
+	"restartable",
+	"precise IP",
+	"overflow",
+	"corrected",
+};
+
+static void cper_print_proc_generic(const char *pfx,
+				    const struct cper_sec_proc_generic *proc)
+{
+	if (proc->validation_bits & CPER_PROC_VALID_TYPE)
+		pr_pfx(pfx, "processor_type: %d, %s\n", proc->proc_type,
+			proc->proc_type < ARRAY_SIZE(cper_proc_type_strs) ?
+			cper_proc_type_strs[proc->proc_type] : "unknown");
+	if (proc->validation_bits & CPER_PROC_VALID_ISA)
+		pr_pfx(pfx, "processor_isa: %d, %s\n", proc->proc_isa,
+			proc->proc_isa < ARRAY_SIZE(cper_proc_isa_strs) ?
+			cper_proc_isa_strs[proc->proc_isa] : "unknown");
+	if (proc->validation_bits & CPER_PROC_VALID_ERROR_TYPE) {
+		pr_pfx(pfx, "error_type: 0x%02x\n", proc->proc_error_type);
+		cper_print_bits(pfx, proc->proc_error_type,
+				cper_proc_error_type_strs,
+				ARRAY_SIZE(cper_proc_error_type_strs));
+	}
+	if (proc->validation_bits & CPER_PROC_VALID_OPERATION)
+		pr_pfx(pfx, "operation: %d, %s\n", proc->operation,
+		       proc->operation < ARRAY_SIZE(cper_proc_op_strs) ?
+		       cper_proc_op_strs[proc->operation] : "unknown");
+	if (proc->validation_bits & CPER_PROC_VALID_FLAGS) {
+		pr_pfx(pfx, "flags: 0x%02x\n", proc->flags);
+		cper_print_bits(pfx, proc->flags, cper_proc_flag_strs,
+				ARRAY_SIZE(cper_proc_flag_strs));
+	}
+	if (proc->validation_bits & CPER_PROC_VALID_LEVEL)
+		pr_pfx(pfx, "level: %d\n", proc->level);
+	if (proc->validation_bits & CPER_PROC_VALID_VERSION)
+		pr_pfx(pfx, "version_info: 0x%016llx\n", proc->cpu_version);
+	if (proc->validation_bits & CPER_PROC_VALID_ID)
+		pr_pfx(pfx, "processor_id: 0x%016llx\n", proc->proc_id);
+	if (proc->validation_bits & CPER_PROC_VALID_TARGET_ADDRESS)
+		pr_pfx(pfx, "target_address: 0x%016llx\n",
+		       proc->target_addr);
+	if (proc->validation_bits & CPER_PROC_VALID_REQUESTOR_ID)
+		pr_pfx(pfx, "requestor_id: 0x%016llx\n", proc->requestor_id);
+	if (proc->validation_bits & CPER_PROC_VALID_RESPONDER_ID)
+		pr_pfx(pfx, "responder_id: 0x%016llx\n", proc->responder_id);
+	if (proc->validation_bits & CPER_PROC_VALID_IP)
+		pr_pfx(pfx, "IP: 0x%016llx\n", proc->ip);
+}
+
+static const char *cper_mem_err_type_strs[] = {
+	"Unknown",
+	"No error",
+	"Single-bit ECC",
+	"Multi-bit ECC",
+	"Single-symbol chipkill ECC",
+	"Multi-symbol chipkill ECC",
+	"Master abort",
+	"Target abort",
+	"Parity error",
+	"Watchdog timeout",
+	"Invalid address",
+	"Mirror Broken",
+	"Memory sparing",
+	"Scrub corrected error",
+	"Scrub uncorrected error",
+};
+
+static void cper_print_mem(const char *pfx, const struct cper_sec_mem_err *mem)
+{
+	if (mem->validation_bits & CPER_MEM_VALID_ERROR_STATUS)
+		pr_pfx(pfx, "error_status: 0x%016llx\n", mem->error_status);
+	if (mem->validation_bits & CPER_MEM_VALID_PHYSICAL_ADDRESS)
+		pr_pfx(pfx, "physical_address: 0x%016llx\n",
+		       mem->physical_addr);
+	if (mem->validation_bits & CPER_MEM_VALID_PHYSICAL_ADDRESS_MASK)
+		pr_pfx(pfx, "physical_address_mask: 0x%016llx\n",
+		       mem->physical_addr_mask);
+	if (mem->validation_bits & CPER_MEM_VALID_NODE)
+		pr_pfx(pfx, "node: %d\n", mem->node);
+	if (mem->validation_bits & CPER_MEM_VALID_CARD)
+		pr_pfx(pfx, "card: %d\n", mem->card);
+	if (mem->validation_bits & CPER_MEM_VALID_MODULE)
+		pr_pfx(pfx, "module: %d\n", mem->module);
+	if (mem->validation_bits & CPER_MEM_VALID_BANK)
+		pr_pfx(pfx, "bank: %d\n", mem->bank);
+	if (mem->validation_bits & CPER_MEM_VALID_DEVICE)
+		pr_pfx(pfx, "device: %d\n", mem->device);
+	if (mem->validation_bits & CPER_MEM_VALID_ROW)
+		pr_pfx(pfx, "row: %d\n", mem->row);
+	if (mem->validation_bits & CPER_MEM_VALID_COLUMN)
+		pr_pfx(pfx, "column: %d\n", mem->column);
+	if (mem->validation_bits & CPER_MEM_VALID_BIT_POSITION)
+		pr_pfx(pfx, "bit_position: %d\n", mem->bit_pos);
+	if (mem->validation_bits & CPER_MEM_VALID_REQUESTOR_ID)
+		pr_pfx(pfx, "requestor_id: 0x%016llx\n", mem->requestor_id);
+	if (mem->validation_bits & CPER_MEM_VALID_RESPONDER_ID)
+		pr_pfx(pfx, "responder_id: 0x%016llx\n", mem->responder_id);
+	if (mem->validation_bits & CPER_MEM_VALID_TARGET_ID)
+		pr_pfx(pfx, "target_id: 0x%016llx\n", mem->target_id);
+	if (mem->validation_bits & CPER_MEM_VALID_ERROR_TYPE) {
+		u8 etype = mem->error_type;
+		pr_pfx(pfx, "error_type: %d, %s\n", etype,
+		       etype < ARRAY_SIZE(cper_mem_err_type_strs) ?
+		       cper_mem_err_type_strs[etype] : "unknown");
+	}
+}
+
+static const char *cper_pcie_port_type_strs[] = {
+	"PCIe end point",
+	"legacy PCI end point",
+	"unknown",
+	"unknown",
+	"root port",
+	"upstream switch port",
+	"downstream switch port",
+	"PCIe to PCI/PCI-X bridge",
+	"PCI/PCI-X to PCIe bridge",
+	"root complex integrated endpoint device",
+	"root complex event collector",
+};
+
+static void cper_print_pcie(const char *pfx, const struct cper_sec_pcie *pcie)
+{
+	if (pcie->validation_bits & CPER_PCIE_VALID_PORT_TYPE)
+		pr_pfx(pfx, "port_type: %d, %s\n", pcie->port_type,
+		       pcie->port_type < ARRAY_SIZE(cper_pcie_port_type_strs) ?
+		       cper_pcie_port_type_strs[pcie->port_type] : "unknown");
+	if (pcie->validation_bits & CPER_PCIE_VALID_VERSION)
+		pr_pfx(pfx, "version: %d.%d\n",
+		       pcie->version.major, pcie->version.minor);
+	if (pcie->validation_bits & CPER_PCIE_VALID_COMMAND_STATUS)
+		pr_pfx(pfx, "command: 0x%04x, status: 0x%04x\n",
+		       pcie->command, pcie->status);
+	if (pcie->validation_bits & CPER_PCIE_VALID_DEVICE_ID) {
+		const __u8 *p;
+		pr_pfx(pfx, "device_id: %04x:%02x:%02x.%x\n",
+		       pcie->device_id.segment, pcie->device_id.bus,
+		       pcie->device_id.device, pcie->device_id.function);
+		pr_pfx(pfx, "slot: %d\n",
+		       pcie->device_id.slot >> CPER_PCIE_SLOT_SHIFT);
+		pr_pfx(pfx, "secondary_bus: 0x%02x\n",
+		       pcie->device_id.secondary_bus);
+		pr_pfx(pfx, "vendor_id: 0x%04x, device_id: 0x%04x\n",
+		       pcie->device_id.vendor_id, pcie->device_id.device_id);
+		p = pcie->device_id.class_code;
+		pr_pfx(pfx, "class_code: %02x%02x%02x\n", p[0], p[1], p[2]);
+	}
+	if (pcie->validation_bits & CPER_PCIE_VALID_SERIAL_NUMBER)
+		pr_pfx(pfx, "serial number: 0x%04x, 0x%04x\n",
+		       pcie->serial_number.lower, pcie->serial_number.upper);
+	if (pcie->validation_bits & CPER_PCIE_VALID_BRIDGE_CONTROL_STATUS)
+		pr_pfx(pfx,
+	"bridge: secondary_status: 0x%04x, control: 0x%04x\n",
+	pcie->bridge.secondary_status, pcie->bridge.control);
+}
+
+static const char *apei_estatus_section_flag_strs[] = {
+	"primary",
+	"containment warning",
+	"reset",
+	"threshold exceeded",
+	"resource not accessible",
+	"latent error",
+};
+
+static void apei_estatus_print_section(
+	const char *pfx, const struct acpi_hest_generic_data *gdata, int sec_no)
+{
+	uuid_le *sec_type = (uuid_le *)gdata->section_type;
+	__u16 severity;
+
+	severity = gdata->error_severity;
+	pr_pfx(pfx, "section: %d, severity: %d, %s\n", sec_no, severity,
+	       cper_severity_str(severity));
+	pr_pfx(pfx, "flags: 0x%02x\n", gdata->flags);
+	cper_print_bits(pfx, gdata->flags, apei_estatus_section_flag_strs,
+			ARRAY_SIZE(apei_estatus_section_flag_strs));
+	if (gdata->validation_bits & CPER_SEC_VALID_FRU_ID)
+		pr_pfx(pfx, "fru_id: %pUl\n", (uuid_le *)gdata->fru_id);
+	if (gdata->validation_bits & CPER_SEC_VALID_FRU_TEXT)
+		pr_pfx(pfx, "fru_text: %20s\n", gdata->fru_text);
+
+	if (!uuid_le_cmp(*sec_type, CPER_SEC_PROC_GENERIC)) {
+		struct cper_sec_proc_generic *proc_err = (void *)(gdata + 1);
+		pr_pfx(pfx, "section_type: general processor error\n");
+		if (gdata->error_data_length >= sizeof(*proc_err))
+			cper_print_proc_generic(pfx, proc_err);
+		else
+			goto err_section_too_small;
+	} else if (!uuid_le_cmp(*sec_type, CPER_SEC_PLATFORM_MEM)) {
+		struct cper_sec_mem_err *mem_err = (void *)(gdata + 1);
+		pr_pfx(pfx, "section_type: memory error\n");
+		if (gdata->error_data_length >= sizeof(*mem_err))
+			cper_print_mem(pfx, mem_err);
+		else
+			goto err_section_too_small;
+	} else if (!uuid_le_cmp(*sec_type, CPER_SEC_PCIE)) {
+		struct cper_sec_pcie *pcie = (void *)(gdata + 1);
+		pr_pfx(pfx, "section_type: PCIe error\n");
+		if (gdata->error_data_length >= sizeof(*pcie))
+			cper_print_pcie(pfx, pcie);
+		else
+			goto err_section_too_small;
+	} else
+		pr_pfx(pfx, "Unknown section type: %pUl\n", sec_type);
+
+	return;
+
+err_section_too_small:
+	pr_err(FW_WARN "error section length is too small\n");
+}
+
+void apei_estatus_print(const char *pfx,
+			const struct acpi_hest_generic_status *estatus)
+{
+	struct acpi_hest_generic_data *gdata;
+	unsigned int data_len, gedata_len;
+	int sec_no = 0;
+	__u16 severity;
+
+	severity = estatus->error_severity;
+	pr_pfx(pfx, "severity: %d, %s\n", severity,
+	       cper_severity_str(severity));
+	data_len = estatus->data_length;
+	gdata = (struct acpi_hest_generic_data *)(estatus + 1);
+	while (data_len > sizeof(*gdata)) {
+		gedata_len = gdata->error_data_length;
+		apei_estatus_print_section(pfx, gdata, sec_no);
+		data_len -= gedata_len + sizeof(*gdata);
+		sec_no++;
+	}
+}
+EXPORT_SYMBOL_GPL(apei_estatus_print);
+
 int apei_estatus_check_header(const struct acpi_hest_generic_status *estatus)
 {
 	if (estatus->data_length &&

Re: [PATCH -v2 2/3] ACPI, APEI, Add APEI generic error status print support

[Andrew Morton]

On Tue, 30 Nov 2010 10:51:40 +0800 Huang Ying <ying.huang@intel.com> wrote:

> printk is one of the methods to report hardware errors to user space.
> Hardware error information reported by firmware to Linux kernel is in
> the format of APEI generic error status (struct
> acpi_hes_generic_status).  This patch adds print support for the
> format, so that the corresponding hardware error information can be
> reported to user space via printk.
> 
> PCIe AER information print is not implemented yet.  Will refactor the
> original PCIe AER information printing code to avoid code duplicating.
> 
> ...
>  
> +#define pr_pfx(pfx, fmt, ...)			\
> +	printk("%s" fmt, pfx, ##__VA_ARGS__)

hm, why does so much code create little printk helper macros.  Isn't
there something generic somewhere?

>  /*
>   * CPER record ID need to be unique even after reboot, because record
>   * ID is used as index for ERST storage, while CPER records from
> @@ -46,6 +49,302 @@ u64 cper_next_record_id(void)
>  }
>  EXPORT_SYMBOL_GPL(cper_next_record_id);
>  
> +static const char *cper_severity_strs[] = {
> +	[CPER_SEV_RECOVERABLE]		= "recoverable",
> +	[CPER_SEV_FATAL]		=  "fatal",
> +	[CPER_SEV_CORRECTED]		= "corrected",
> +	[CPER_SEV_INFORMATIONAL]	= "info",
> +};
> +
> +static const char *cper_severity_str(unsigned int severity)
> +{
> +	return severity < ARRAY_SIZE(cper_severity_strs) ?
> +		cper_severity_strs[severity] : "unknown";
> +}

This code will explode nastily if CPER_SEV_RECOVERABLE .. 
CPER_SEV_INFORMATIONAL do not exactly have the values 0, 1, 2 and 3. 
They do have those values, but it would be a bit safer if they were
enumerated types and not #defines..

> +static void cper_print_bits(const char *pfx, unsigned int bits,
> +			    const char *strs[], unsigned int strs_size)
> +{
> +	int i, len = 0;
> +	const char *str;
> +
> +	for (i = 0; i < strs_size; i++) {
> +		if (!(bits & (1U << i)))
> +			continue;
> +		str = strs[i];
> +		if (len && len + strlen(str) + 2 > 80) {
> +			printk("\n");
> +			len = 0;
> +		}
> +		if (!len)
> +			len = pr_pfx(pfx, "%s", str);
> +		else
> +			len += printk(", %s", str);
> +	}
> +	if (len)
> +		printk("\n");
> +}

geeze, that's the sort of code you have to execute to find out what it
does.  Or ask the author to document it.



This patchset appears to implement a new kernel->userspace interface. 
But that interface isn't actually described anywhere, so reviewers must
reverse-engineer the interface from the implementation to be able to
review the interface.  Nobody bothers doing that so we end up with an
unreviewed interface, which we must maintain for eternity.

Please fully document all proposed interfaces?

Re: [PATCH -v2 2/3] ACPI, APEI, Add APEI generic error status print support

[Huang Ying]

On Tue, 2010-11-30 at 11:03 +0800, Andrew Morton wrote:
> On Tue, 30 Nov 2010 10:51:40 +0800 Huang Ying <ying.huang@intel.com> wrote:
> 
> > printk is one of the methods to report hardware errors to user space.
> > Hardware error information reported by firmware to Linux kernel is in
> > the format of APEI generic error status (struct
> > acpi_hes_generic_status).  This patch adds print support for the
> > format, so that the corresponding hardware error information can be
> > reported to user space via printk.
> > 
> > PCIe AER information print is not implemented yet.  Will refactor the
> > original PCIe AER information printing code to avoid code duplicating.
> > 
> > ...
> >  
> > +#define pr_pfx(pfx, fmt, ...)			\
> > +	printk("%s" fmt, pfx, ##__VA_ARGS__)
> 
> hm, why does so much code create little printk helper macros.  Isn't
> there something generic somewhere?

Sorry, I do not find the generic code for this helper. But I think this
macro may be helpful for others too, who need to determine the log level
only at runtime. Here corrected errors should have log level:
KERN_WARNING, while uncorrected errors should have log level: KERN_ERR.

Do you think it is a good idea to make this macro generic?

> >  /*
> >   * CPER record ID need to be unique even after reboot, because record
> >   * ID is used as index for ERST storage, while CPER records from
> > @@ -46,6 +49,302 @@ u64 cper_next_record_id(void)
> >  }
> >  EXPORT_SYMBOL_GPL(cper_next_record_id);
> >  
> > +static const char *cper_severity_strs[] = {
> > +	[CPER_SEV_RECOVERABLE]		= "recoverable",
> > +	[CPER_SEV_FATAL]		=  "fatal",
> > +	[CPER_SEV_CORRECTED]		= "corrected",
> > +	[CPER_SEV_INFORMATIONAL]	= "info",
> > +};
> > +
> > +static const char *cper_severity_str(unsigned int severity)
> > +{
> > +	return severity < ARRAY_SIZE(cper_severity_strs) ?
> > +		cper_severity_strs[severity] : "unknown";
> > +}
> 
> This code will explode nastily if CPER_SEV_RECOVERABLE .. 
> CPER_SEV_INFORMATIONAL do not exactly have the values 0, 1, 2 and 3. 
> They do have those values, but it would be a bit safer if they were
> enumerated types and not #defines..

OK. I will change this.

> > +static void cper_print_bits(const char *pfx, unsigned int bits,
> > +			    const char *strs[], unsigned int strs_size)
> > +{
> > +	int i, len = 0;
> > +	const char *str;
> > +
> > +	for (i = 0; i < strs_size; i++) {
> > +		if (!(bits & (1U << i)))
> > +			continue;
> > +		str = strs[i];
> > +		if (len && len + strlen(str) + 2 > 80) {
> > +			printk("\n");
> > +			len = 0;
> > +		}
> > +		if (!len)
> > +			len = pr_pfx(pfx, "%s", str);
> > +		else
> > +			len += printk(", %s", str);
> > +	}
> > +	if (len)
> > +		printk("\n");
> > +}
> 
> geeze, that's the sort of code you have to execute to find out what it
> does.  Or ask the author to document it.

OK. I will add comments for all necessary functions in the patch.

> This patchset appears to implement a new kernel->userspace interface. 
> But that interface isn't actually described anywhere, so reviewers must
> reverse-engineer the interface from the implementation to be able to
> review the interface.  Nobody bothers doing that so we end up with an
> unreviewed interface, which we must maintain for eternity.
> 
> Please fully document all proposed interfaces?

Sorry. I don't realize that printk-ing something means implementing a
new kernel->userspace interface. I think the messages resulted are
self-explaining for human. Is it sufficient just to add example messages
in patch description?

Best Regards,
Huang Ying

Re: [PATCH -v2 2/3] ACPI, APEI, Add APEI generic error status print support

[Andrew Morton]

On Tue, 30 Nov 2010 11:29:12 +0800 Huang Ying <ying.huang@intel.com> wrote:

> On Tue, 2010-11-30 at 11:03 +0800, Andrew Morton wrote:
> > On Tue, 30 Nov 2010 10:51:40 +0800 Huang Ying <ying.huang@intel.com> wrote:
> > 
> > > printk is one of the methods to report hardware errors to user space.
> > > Hardware error information reported by firmware to Linux kernel is in
> > > the format of APEI generic error status (struct
> > > acpi_hes_generic_status).  This patch adds print support for the
> > > format, so that the corresponding hardware error information can be
> > > reported to user space via printk.
> > > 
> > > PCIe AER information print is not implemented yet.  Will refactor the
> > > original PCIe AER information printing code to avoid code duplicating.
> > > 
> > > ...
> > >  
> > > +#define pr_pfx(pfx, fmt, ...)			\
> > > +	printk("%s" fmt, pfx, ##__VA_ARGS__)
> > 
> > hm, why does so much code create little printk helper macros.  Isn't
> > there something generic somewhere?
> 
> Sorry, I do not find the generic code for this helper. But I think this
> macro may be helpful for others too, who need to determine the log level
> only at runtime. Here corrected errors should have log level:
> KERN_WARNING, while uncorrected errors should have log level: KERN_ERR.

Oh, is that what it does.  Replacing "pfx" everywhere with "loglevel"
(or similar) would have been much clearer?

> Do you think it is a good idea to make this macro generic?

hm, maybe.  It's the sort of thing which gives rise to much
chin-scratching, which is why people usually avoid doing it ;) If the
macro is well-named and its intended use is quite clear then yes, it's
probably worth pursuing.

> > This patchset appears to implement a new kernel->userspace interface. 
> > But that interface isn't actually described anywhere, so reviewers must
> > reverse-engineer the interface from the implementation to be able to
> > review the interface.  Nobody bothers doing that so we end up with an
> > unreviewed interface, which we must maintain for eternity.
> > 
> > Please fully document all proposed interfaces?
> 
> Sorry. I don't realize that printk-ing something means implementing a
> new kernel->userspace interface. I think the messages resulted are
> self-explaining for human. Is it sufficient just to add example messages
> in patch description?

Well normally a printk() isn't really considered a "userspace
interface".  This allows us to change them even though there surely
_are_ existing tools which treat particular messages as a userspace
interface.  But I don't recall hearing of much breakage from changed
kernel printks.

However in this case you are avowedly treating the printks as a
userspace interface, with the intention that software be written to
parse them, yes?  So once they're in place, we cannot change them?  That
makes it more important.

Re: [PATCH -v2 2/3] ACPI, APEI, Add APEI generic error status print support

[Huang Ying]

On Tue, 2010-11-30 at 11:40 +0800, Andrew Morton wrote:
> On Tue, 30 Nov 2010 11:29:12 +0800 Huang Ying <ying.huang@intel.com> wrote:
> 
> > On Tue, 2010-11-30 at 11:03 +0800, Andrew Morton wrote:
> > > On Tue, 30 Nov 2010 10:51:40 +0800 Huang Ying <ying.huang@intel.com> wrote:
> > > 
> > > > printk is one of the methods to report hardware errors to user space.
> > > > Hardware error information reported by firmware to Linux kernel is in
> > > > the format of APEI generic error status (struct
> > > > acpi_hes_generic_status).  This patch adds print support for the
> > > > format, so that the corresponding hardware error information can be
> > > > reported to user space via printk.
> > > > 
> > > > PCIe AER information print is not implemented yet.  Will refactor the
> > > > original PCIe AER information printing code to avoid code duplicating.
> > > > 
> > > > ...
> > > >  
> > > > +#define pr_pfx(pfx, fmt, ...)			\
> > > > +	printk("%s" fmt, pfx, ##__VA_ARGS__)
> > > 
> > > hm, why does so much code create little printk helper macros.  Isn't
> > > there something generic somewhere?
> > 
> > Sorry, I do not find the generic code for this helper. But I think this
> > macro may be helpful for others too, who need to determine the log level
> > only at runtime. Here corrected errors should have log level:
> > KERN_WARNING, while uncorrected errors should have log level: KERN_ERR.
> 
> Oh, is that what it does.  Replacing "pfx" everywhere with "loglevel"
> (or similar) would have been much clearer?

The pfx (prefix) here is more than "loglevel", I prefix each line with
"[Hardware Error:]" to make it clear that this is a hardware error
reporting. I think that can be useful for some shared functions doing
printk, the prefix parameter can provide sufficient flexibility for
caller to use prefix like <module name> or <device ID>.

> > Do you think it is a good idea to make this macro generic?
> 
> hm, maybe.  It's the sort of thing which gives rise to much
> chin-scratching, which is why people usually avoid doing it ;) If the
> macro is well-named and its intended use is quite clear then yes, it's
> probably worth pursuing.
> 
> > > This patchset appears to implement a new kernel->userspace interface. 
> > > But that interface isn't actually described anywhere, so reviewers must
> > > reverse-engineer the interface from the implementation to be able to
> > > review the interface.  Nobody bothers doing that so we end up with an
> > > unreviewed interface, which we must maintain for eternity.
> > > 
> > > Please fully document all proposed interfaces?
> > 
> > Sorry. I don't realize that printk-ing something means implementing a
> > new kernel->userspace interface. I think the messages resulted are
> > self-explaining for human. Is it sufficient just to add example messages
> > in patch description?
> 
> Well normally a printk() isn't really considered a "userspace
> interface".  This allows us to change them even though there surely
> _are_ existing tools which treat particular messages as a userspace
> interface.  But I don't recall hearing of much breakage from changed
> kernel printks.
> 
> However in this case you are avowedly treating the printks as a
> userspace interface, with the intention that software be written to
> parse them, yes?  So once they're in place, we cannot change them?  That
> makes it more important.

If my understanding is correct, Linus still don't like the idea of user
space hardware error tool.  On the other hand, if we need this tool, I
think printk is not a good tool-oriented hardware error reporting
interface for it, because:

- There is no overall format or record boundaries for printk, because
printk is traditionally for 1-2 lines.  This makes that printk is hard
to parse in general.

- Messages from different CPUs may be interleaved.

- Good error reporting is too verbose for kernel log

- printk has no internal priority support, so that high severity errors
has no more priority than low severity ones.


So my opinion is:

- Use printk as human oriented hardware error reporting.
- Use another tool oriented interface for user space hardware error tool
if necessary.

Do you agree?  Do you think printk can be used as a good tool-oriented
hardware error reporting interface too?


Best Regards,
Huang Ying

Re: [PATCH -v2 2/3] ACPI, APEI, Add APEI generic error status print support

[Andrew Morton]

On Tue, 30 Nov 2010 15:00:31 +0800
Huang Ying <ying.huang@intel.com> wrote:

> > However in this case you are avowedly treating the printks as a
> > userspace interface, with the intention that software be written to
> > parse them, yes?  So once they're in place, we cannot change them?  That
> > makes it more important.
> 
> If my understanding is correct, Linus still don't like the idea of user
> space hardware error tool.

I'm sure he has no problem with a userspace tool ;) Surely what he doesn't
like is the proposed kernel interface.

>  On the other hand, if we need this tool, I
> think printk is not a good tool-oriented hardware error reporting
> interface for it, because:
> 
> - There is no overall format or record boundaries for printk, because
> printk is traditionally for 1-2 lines.  This makes that printk is hard
> to parse in general.

Well.  These things can be addressed by careful choice of output
format.

> - Messages from different CPUs may be interleaved.

A single printk() should be atomic.

> - Good error reporting is too verbose for kernel log
> 
> - printk has no internal priority support, so that high severity errors
> has no more priority than low severity ones.
> 
> 
> So my opinion is:
> 
> - Use printk as human oriented hardware error reporting.
> - Use another tool oriented interface for user space hardware error tool
> if necessary.
> 
> Do you agree?  Do you think printk can be used as a good tool-oriented
> hardware error reporting interface too?

I agree that using printk() is pretty sucky.

However your proposals are waaaaaaaaay too narrow and specific IMO. 
There are several reasons why people want more regular and structured
kerenl->userspace messaging features.  One such requirement is for
internationalisation: people want messages to come out in some
non-language-specific manner so that userspace tools can perform
catalogue lookups and display the messages in the appropriate language.
 Others (eg google) want to feed the messages into large-scale
capturing systems for offline analysis.  And there are other
requirements which I forget.  Such a messaging/logging system would
also incorporate the requirement to log to a persistent store.

So I think that quite a lot of people would be interested in proposals
for a new and improved kernel->userspace messaging/logging facility. 
But talking about "hardware error reporting" (especially when it covers
only a teeny subset of possible hardware errors!) is very myopic.

And implementing the broad facility would be a pretty big project.  Simply
chasing down all the stakeholders and understanding their needs would
turn one's hair grey.

So we're a bit stuck, really.  We would benefit from a quite broad and
expensive-to-implement messaging/logging system, but we don't even know
what that will look like yet.  You have a small and highly-specific
subset of that.  If we merge the subset then it probably will live
forever even if the broader feature gets written one day, because the
subset is userspace-visible and adds interfaces which the larger system
probably won't even implement.

So...  for your pretty narrow and specific problem, perhaps using
printk as a stopgap until somethine better to come along is the correct
choice.

Re: [PATCH -v2 2/3] ACPI, APEI, Add APEI generic error status print support

[huang ying]

On Wed, Dec 1, 2010 at 7:49 AM, Andrew Morton <akpm@linux-foundation.org> wrote:
> On Tue, 30 Nov 2010 15:00:31 +0800
> Huang Ying <ying.huang@intel.com> wrote:
>
>> > However in this case you are avowedly treating the printks as a
>> > userspace interface, with the intention that software be written to
>> > parse them, yes?  So once they're in place, we cannot change them?  That
>> > makes it more important.
>>
>> If my understanding is correct, Linus still don't like the idea of user
>> space hardware error tool.
>
> I'm sure he has no problem with a userspace tool ;) Surely what he doesn't
> like is the proposed kernel interface.
>
>>  On the other hand, if we need this tool, I
>> think printk is not a good tool-oriented hardware error reporting
>> interface for it, because:
>>
>> - There is no overall format or record boundaries for printk, because
>> printk is traditionally for 1-2 lines.  This makes that printk is hard
>> to parse in general.
>
> Well.  These things can be addressed by careful choice of output
> format.
>
>> - Messages from different CPUs may be interleaved.
>
> A single printk() should be atomic.
>
>> - Good error reporting is too verbose for kernel log
>>
>> - printk has no internal priority support, so that high severity errors
>> has no more priority than low severity ones.
>>
>>
>> So my opinion is:
>>
>> - Use printk as human oriented hardware error reporting.
>> - Use another tool oriented interface for user space hardware error tool
>> if necessary.
>>
>> Do you agree?  Do you think printk can be used as a good tool-oriented
>> hardware error reporting interface too?
>
> I agree that using printk() is pretty sucky.
>
> However your proposals are waaaaaaaaay too narrow and specific IMO.
> There are several reasons why people want more regular and structured
> kerenl->userspace messaging features.  One such requirement is for
> internationalisation: people want messages to come out in some
> non-language-specific manner so that userspace tools can perform
> catalogue lookups and display the messages in the appropriate language.
>  Others (eg google) want to feed the messages into large-scale
> capturing systems for offline analysis.  And there are other
> requirements which I forget.  Such a messaging/logging system would
> also incorporate the requirement to log to a persistent store.
>
> So I think that quite a lot of people would be interested in proposals
> for a new and improved kernel->userspace messaging/logging facility.
> But talking about "hardware error reporting" (especially when it covers
> only a teeny subset of possible hardware errors!) is very myopic.
>
> And implementing the broad facility would be a pretty big project.  Simply
> chasing down all the stakeholders and understanding their needs would
> turn one's hair grey.
>
> So we're a bit stuck, really.  We would benefit from a quite broad and
> expensive-to-implement messaging/logging system, but we don't even know
> what that will look like yet.  You have a small and highly-specific
> subset of that.  If we merge the subset then it probably will live
> forever even if the broader feature gets written one day, because the
> subset is userspace-visible and adds interfaces which the larger system
> probably won't even implement.
>
> So...  for your pretty narrow and specific problem, perhaps using
> printk as a stopgap until somethine better to come along is the correct
> choice.

OK. I will work on the printk based solution, at least as the first step.

Best Regards,
Huang Ying
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH -v2 2/3] ACPI, APEI, Add APEI generic error status print support

[Luck, Tony]

> +	for (i = 0; i < strs_size; i++) {
> +		if (!(bits & (1U << i)))
> +			continue;
> +		str = strs[i];
> +		if (len && len + strlen(str) + 2 > 80) {
> +			printk("\n");
> +			len = 0;
> +		}
> +		if (!len)
> +			len = pr_pfx(pfx, "%s", str);
> +		else
> +			len += printk(", %s", str);
> +	}
> +	if (len)
> +		printk("\n");

Does printk() offer any guarantees about getting all the characters
from a single printk() call out to the console without interleaving
with messages from printk() calls on other cpus?  If it does, then
it would be a good idea to sprintf() the parts of this message to
a buffer and then use one printk() call.  I think I read that netconsole
ends up with one packet on the wire for each call to printk().

Trying to parse output jumbled together from multiple cpus
doesn't sound like fun.

-Tony

Re: [PATCH -v2 2/3] ACPI, APEI, Add APEI generic error status print support

[Andrew Morton]

On Tue, 30 Nov 2010 10:00:47 -0800 "Luck, Tony" <tony.luck@intel.com> wrote:

> > +	for (i = 0; i < strs_size; i++) {
> > +		if (!(bits & (1U << i)))
> > +			continue;
> > +		str = strs[i];
> > +		if (len && len + strlen(str) + 2 > 80) {
> > +			printk("\n");
> > +			len = 0;
> > +		}
> > +		if (!len)
> > +			len = pr_pfx(pfx, "%s", str);
> > +		else
> > +			len += printk(", %s", str);
> > +	}
> > +	if (len)
> > +		printk("\n");
> 
> Does printk() offer any guarantees about getting all the characters
> from a single printk() call out to the console without interleaving
> with messages from printk() calls on other cpus?

Yes, it uses logbuf_lock to atomically append all the output from a
printk into log_buf[].  Then it calls the console drivers against
log_buf[].  It's hard to see how a console driver could then screw that
up.

>  If it does, then
> it would be a good idea to sprintf() the parts of this message to
> a buffer and then use one printk() call.

yup.

>  I think I read that netconsole
> ends up with one packet on the wire for each call to printk().
> 
> Trying to parse output jumbled together from multiple cpus
> doesn't sound like fun.
> 
> -Tony

Re: [PATCH -v2 2/3] ACPI, APEI, Add APEI generic error status print support

[huang ying]

On Wed, Dec 1, 2010 at 2:00 AM, Luck, Tony <tony.luck@intel.com> wrote:
>> +     for (i = 0; i < strs_size; i++) {
>> +             if (!(bits & (1U << i)))
>> +                     continue;
>> +             str = strs[i];
>> +             if (len && len + strlen(str) + 2 > 80) {
>> +                     printk("\n");
>> +                     len = 0;
>> +             }
>> +             if (!len)
>> +                     len = pr_pfx(pfx, "%s", str);
>> +             else
>> +                     len += printk(", %s", str);
>> +     }
>> +     if (len)
>> +             printk("\n");
>
> Does printk() offer any guarantees about getting all the characters
> from a single printk() call out to the console without interleaving
> with messages from printk() calls on other cpus?  If it does, then
> it would be a good idea to sprintf() the parts of this message to
> a buffer and then use one printk() call.  I think I read that netconsole
> ends up with one packet on the wire for each call to printk().
>
> Trying to parse output jumbled together from multiple cpus
> doesn't sound like fun.

Yes. Will do that.

Best Regards,
Huang Ying
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH -v2 3/3] ACPI, APEI, report GHES error information via printk

[Huang Ying]

printk is one of the methods to report hardware errors to user space.
This patch implements hardware error reporting for GHES via printk.

Signed-off-by: Huang Ying <ying.huang@intel.com>
---
 drivers/acpi/apei/ghes.c |   21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -255,11 +255,23 @@ static void ghes_do_proc(struct ghes *gh
 		}
 #endif
 	}
+}
 
-	if (!processed && printk_ratelimit())
-		pr_warning(GHES_PFX
-		"Unknown error record from generic hardware error source: %d\n",
-			   ghes->generic->header.source_id);
+static void ghes_print_estatus(const char *pfx, struct ghes *ghes)
+{
+	if (pfx == NULL) {
+		if (ghes_severity(ghes->estatus->error_severity) <=
+		    GHES_SEV_CORRECTED)
+			pfx = KERN_WARNING HW_ERR;
+		else
+			pfx = KERN_ERR HW_ERR;
+	}
+	if (printk_ratelimit()) {
+		printk(
+	"%s""Hardware error from APEI Generic Hardware Error Source: %d\n",
+	pfx, ghes->generic->header.source_id);
+		apei_estatus_print(pfx, ghes->estatus);
+	}
 }
 
 static int ghes_proc(struct ghes *ghes)
@@ -269,6 +281,7 @@ static int ghes_proc(struct ghes *ghes)
 	rc = ghes_read_estatus(ghes, 0);
 	if (rc)
 		goto out;
+	ghes_print_estatus(NULL, ghes);
 	ghes_do_proc(ghes);
 
 out:

Re: [PATCH -v2 3/3] ACPI, APEI, report GHES error information via printk

[Andrew Morton]

On Tue, 30 Nov 2010 10:51:41 +0800 Huang Ying <ying.huang@intel.com> wrote:

> printk is one of the methods to report hardware errors to user space.
> This patch implements hardware error reporting for GHES via printk.
> 
> Signed-off-by: Huang Ying <ying.huang@intel.com>
> ---
>  drivers/acpi/apei/ghes.c |   21 +++++++++++++++++----
>  1 file changed, 17 insertions(+), 4 deletions(-)
> 
> --- a/drivers/acpi/apei/ghes.c
> +++ b/drivers/acpi/apei/ghes.c
> @@ -255,11 +255,23 @@ static void ghes_do_proc(struct ghes *gh
>  		}
>  #endif
>  	}
> +}
>  
> -	if (!processed && printk_ratelimit())
> -		pr_warning(GHES_PFX
> -		"Unknown error record from generic hardware error source: %d\n",
> -			   ghes->generic->header.source_id);
> +static void ghes_print_estatus(const char *pfx, struct ghes *ghes)
> +{
> +	if (pfx == NULL) {
> +		if (ghes_severity(ghes->estatus->error_severity) <=
> +		    GHES_SEV_CORRECTED)
> +			pfx = KERN_WARNING HW_ERR;
> +		else
> +			pfx = KERN_ERR HW_ERR;
> +	}
> +	if (printk_ratelimit()) {
> +		printk(
> +	"%s""Hardware error from APEI Generic Hardware Error Source: %d\n",
> +	pfx, ghes->generic->header.source_id);
> +		apei_estatus_print(pfx, ghes->estatus);

That code layout is just ghastly.  Please, if it can't be done nicely
in 80-cols then simply exceed the 80 cols.

And please don't use (or retain) printk_ratelimit().  It was a mistake.
A printk_ratelimt() site shares state with all other
printk_ratelimit() states, so if a random firewire driver is doing a lot of
printk_ratelimit() calls, your messages get suppressed!  Use
printk_ratelimited() or __ratelimit().

>
> ...
>

Re: [PATCH -v2 3/3] ACPI, APEI, report GHES error information via printk

[Huang Ying]

On Tue, 2010-11-30 at 11:07 +0800, Andrew Morton wrote:
> On Tue, 30 Nov 2010 10:51:41 +0800 Huang Ying <ying.huang@intel.com> wrote:
> 
> > printk is one of the methods to report hardware errors to user space.
> > This patch implements hardware error reporting for GHES via printk.
> > 
> > Signed-off-by: Huang Ying <ying.huang@intel.com>
> > ---
> >  drivers/acpi/apei/ghes.c |   21 +++++++++++++++++----
> >  1 file changed, 17 insertions(+), 4 deletions(-)
> > 
> > --- a/drivers/acpi/apei/ghes.c
> > +++ b/drivers/acpi/apei/ghes.c
> > @@ -255,11 +255,23 @@ static void ghes_do_proc(struct ghes *gh
> >  		}
> >  #endif
> >  	}
> > +}
> >  
> > -	if (!processed && printk_ratelimit())
> > -		pr_warning(GHES_PFX
> > -		"Unknown error record from generic hardware error source: %d\n",
> > -			   ghes->generic->header.source_id);
> > +static void ghes_print_estatus(const char *pfx, struct ghes *ghes)
> > +{
> > +	if (pfx == NULL) {
> > +		if (ghes_severity(ghes->estatus->error_severity) <=
> > +		    GHES_SEV_CORRECTED)
> > +			pfx = KERN_WARNING HW_ERR;
> > +		else
> > +			pfx = KERN_ERR HW_ERR;
> > +	}
> > +	if (printk_ratelimit()) {
> > +		printk(
> > +	"%s""Hardware error from APEI Generic Hardware Error Source: %d\n",
> > +	pfx, ghes->generic->header.source_id);
> > +		apei_estatus_print(pfx, ghes->estatus);
> 
> That code layout is just ghastly.  Please, if it can't be done nicely
> in 80-cols then simply exceed the 80 cols.

Just for printk, I think sometimes it may be helpful to put the "format"
string at a new line in source code. Because it may be helpful to check
whether the resulting string from printk fits 80 cols.

> And please don't use (or retain) printk_ratelimit().  It was a mistake.
> A printk_ratelimt() site shares state with all other
> printk_ratelimit() states, so if a random firewire driver is doing a lot of
> printk_ratelimit() calls, your messages get suppressed!  Use
> printk_ratelimited() or __ratelimit().

Yes. Will change this.

Best Regards,
Huang Ying

Re: [PATCH -v2 3/3] ACPI, APEI, report GHES error information via printk

[KOSAKI Motohiro]

> On Tue, 2010-11-30 at 11:07 +0800, Andrew Morton wrote:
> > On Tue, 30 Nov 2010 10:51:41 +0800 Huang Ying <ying.huang@intel.com> wrote:
> > 
> > > printk is one of the methods to report hardware errors to user space.
> > > This patch implements hardware error reporting for GHES via printk.
> > > 
> > > Signed-off-by: Huang Ying <ying.huang@intel.com>
> > > ---
> > >  drivers/acpi/apei/ghes.c |   21 +++++++++++++++++----
> > >  1 file changed, 17 insertions(+), 4 deletions(-)
> > > 
> > > --- a/drivers/acpi/apei/ghes.c
> > > +++ b/drivers/acpi/apei/ghes.c
> > > @@ -255,11 +255,23 @@ static void ghes_do_proc(struct ghes *gh
> > >  		}
> > >  #endif
> > >  	}
> > > +}
> > >  
> > > -	if (!processed && printk_ratelimit())
> > > -		pr_warning(GHES_PFX
> > > -		"Unknown error record from generic hardware error source: %d\n",
> > > -			   ghes->generic->header.source_id);
> > > +static void ghes_print_estatus(const char *pfx, struct ghes *ghes)
> > > +{
> > > +	if (pfx == NULL) {
> > > +		if (ghes_severity(ghes->estatus->error_severity) <=
> > > +		    GHES_SEV_CORRECTED)
> > > +			pfx = KERN_WARNING HW_ERR;
> > > +		else
> > > +			pfx = KERN_ERR HW_ERR;
> > > +	}
> > > +	if (printk_ratelimit()) {
> > > +		printk(
> > > +	"%s""Hardware error from APEI Generic Hardware Error Source: %d\n",
> > > +	pfx, ghes->generic->header.source_id);
> > > +		apei_estatus_print(pfx, ghes->estatus);
> > 
> > That code layout is just ghastly.  Please, if it can't be done nicely
> > in 80-cols then simply exceed the 80 cols.
> 
> Just for printk, I think sometimes it may be helpful to put the "format"
> string at a new line in source code. Because it may be helpful to check
> whether the resulting string from printk fits 80 cols.

No. please reconsider why all other persons don't do that.
It's beyond ugly.

Re: [PATCH -v2 3/3] ACPI, APEI, report GHES error information via printk

[Huang Ying]

On Tue, 2010-11-30 at 13:47 +0800, KOSAKI Motohiro wrote:
> > On Tue, 2010-11-30 at 11:07 +0800, Andrew Morton wrote:
> > > On Tue, 30 Nov 2010 10:51:41 +0800 Huang Ying <ying.huang@intel.com> wrote:
[...]
> > > > +static void ghes_print_estatus(const char *pfx, struct ghes *ghes)
> > > > +{
> > > > +	if (pfx == NULL) {
> > > > +		if (ghes_severity(ghes->estatus->error_severity) <=
> > > > +		    GHES_SEV_CORRECTED)
> > > > +			pfx = KERN_WARNING HW_ERR;
> > > > +		else
> > > > +			pfx = KERN_ERR HW_ERR;
> > > > +	}
> > > > +	if (printk_ratelimit()) {
> > > > +		printk(
> > > > +	"%s""Hardware error from APEI Generic Hardware Error Source: %d\n",
> > > > +	pfx, ghes->generic->header.source_id);
> > > > +		apei_estatus_print(pfx, ghes->estatus);
> > > 
> > > That code layout is just ghastly.  Please, if it can't be done nicely
> > > in 80-cols then simply exceed the 80 cols.
> > 
> > Just for printk, I think sometimes it may be helpful to put the "format"
> > string at a new line in source code. Because it may be helpful to check
> > whether the resulting string from printk fits 80 cols.
> 
> No. please reconsider why all other persons don't do that.
> It's beyond ugly.

I think that "why other persons don't/do do that" is not good reasoning.

Best Regards,
Huang Ying