Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756713AbcJXUei (ORCPT ); Mon, 24 Oct 2016 16:34:38 -0400 Received: from smtp.codeaurora.org ([198.145.29.96]:52533 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752607AbcJXUee (ORCPT ); Mon, 24 Oct 2016 16:34:34 -0400 DMARC-Filter: OpenDMARC Filter v1.3.1 smtp.codeaurora.org 822CD61A83 Authentication-Results: pdx-caf-mail.web.codeaurora.org; dmarc=none header.from=codeaurora.org Authentication-Results: pdx-caf-mail.web.codeaurora.org; spf=pass smtp.mailfrom=tbaicar@codeaurora.org Subject: Re: [PATCH V4 02/10] ras: acpi/apei: cper: generic error data entry v3 per ACPI 6.1 To: Suzuki K Poulose , marc.zyngier@arm.com, pbonzini@redhat.com, rkrcmar@redhat.com, linux@armlinux.org.uk, catalin.marinas@arm.com, will.deacon@arm.com, rjw@rjwysocki.net, lenb@kernel.org, matt@codeblueprint.co.uk, robert.moore@intel.com, lv.zheng@intel.com, nkaje@codeaurora.org, zjzhang@codeaurora.org, mark.rutland@arm.com, james.morse@arm.com, akpm@linux-foundation.org, eun.taik.lee@samsung.com, sandeepa.s.prabhu@gmail.com, shijie.huang@arm.com, rruigrok@codeaurora.org, paul.gortmaker@windriver.com, tomasz.nowicki@linaro.org, fu.wei@linaro.org, rostedt@goodmis.org, bristot@redhat.com, linux-arm-kernel@lists.infradead.org, kvmarm@lists.cs.columbia.edu, kvm@vger.kernel.org, linux-kernel@vger.kernel.org, linux-acpi@vger.kernel.org, linux-efi@vger.kernel.org, punit.agrawal@arm.com, astone@redhat.com, harba@codeaurora.org, hanjun.guo@linaro.org References: <1477071013-29563-1-git-send-email-tbaicar@codeaurora.org> <1477071013-29563-3-git-send-email-tbaicar@codeaurora.org> From: "Baicar, Tyler" Message-ID: <2422a0c6-ebe4-7f51-2b8d-c42b9bb468b1@codeaurora.org> Date: Mon, 24 Oct 2016 14:33:54 -0600 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 10489 Lines: 260 On 10/24/2016 3:50 AM, Suzuki K Poulose wrote: > On 21/10/16 18:30, Tyler Baicar wrote: >> Currently when a RAS error is reported it is not timestamped. >> The ACPI 6.1 spec adds the timestamp field to the generic error >> data entry v3 structure. The timestamp of when the firmware >> generated the error is now being reported. >> >> Signed-off-by: Jonathan (Zhixiong) Zhang >> Signed-off-by: Richard Ruigrok >> Signed-off-by: Tyler Baicar >> Signed-off-by: Naveen Kaje >> --- >> drivers/acpi/apei/ghes.c | 14 +++++++--- >> drivers/firmware/efi/cper.c | 67 >> +++++++++++++++++++++++++++++++++++++-------- >> include/acpi/ghes.h | 10 +++++++ >> include/linux/cper.h | 12 ++++++++ >> 4 files changed, 88 insertions(+), 15 deletions(-) >> >> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c >> index 7d020b0..7610f08 100644 >> --- a/drivers/acpi/apei/ghes.c >> +++ b/drivers/acpi/apei/ghes.c >> @@ -419,7 +419,8 @@ static void ghes_handle_memory_failure(struct >> acpi_hest_generic_data *gdata, int >> int flags = -1; >> int sec_sev = ghes_severity(gdata->error_severity); >> struct cper_sec_mem_err *mem_err; >> - mem_err = (struct cper_sec_mem_err *)(gdata + 1); >> + >> + mem_err = acpi_hest_generic_data_payload(gdata); >> >> if (!(mem_err->validation_bits & CPER_MEM_VALID_PA)) >> return; >> @@ -449,14 +450,18 @@ static void ghes_do_proc(struct ghes *ghes, >> { >> int sev, sec_sev; >> struct acpi_hest_generic_data *gdata; >> + uuid_le sec_type; >> >> sev = ghes_severity(estatus->error_severity); >> apei_estatus_for_each_section(estatus, gdata) { >> sec_sev = ghes_severity(gdata->error_severity); >> - if (!uuid_le_cmp(*(uuid_le *)gdata->section_type, >> + sec_type = *(uuid_le *)gdata->section_type; >> + >> + if (!uuid_le_cmp(sec_type, >> CPER_SEC_PLATFORM_MEM)) { >> struct cper_sec_mem_err *mem_err; >> - mem_err = (struct cper_sec_mem_err *)(gdata+1); >> + >> + mem_err = acpi_hest_generic_data_payload(gdata); >> ghes_edac_report_mem_error(ghes, sev, mem_err); >> >> arch_apei_report_mem_error(sev, mem_err); >> @@ -466,7 +471,8 @@ static void ghes_do_proc(struct ghes *ghes, >> else if (!uuid_le_cmp(*(uuid_le *)gdata->section_type, >> CPER_SEC_PCIE)) { >> struct cper_sec_pcie *pcie_err; >> - pcie_err = (struct cper_sec_pcie *)(gdata+1); >> + >> + pcie_err = acpi_hest_generic_data_payload(gdata); >> if (sev == GHES_SEV_RECOVERABLE && >> sec_sev == GHES_SEV_RECOVERABLE && >> pcie_err->validation_bits & >> CPER_PCIE_VALID_DEVICE_ID && >> diff --git a/drivers/firmware/efi/cper.c b/drivers/firmware/efi/cper.c >> index d425374..af7e1e9 100644 >> --- a/drivers/firmware/efi/cper.c >> +++ b/drivers/firmware/efi/cper.c >> @@ -32,6 +32,9 @@ >> #include >> #include >> #include >> +#include >> +#include >> +#include >> >> #define INDENT_SP " " >> >> @@ -386,13 +389,37 @@ static void cper_print_pcie(const char *pfx, >> const struct cper_sec_pcie *pcie, >> pfx, pcie->bridge.secondary_status, pcie->bridge.control); >> } >> >> +static void cper_estatus_print_section_v300(const char *pfx, >> + const struct acpi_hest_generic_data_v300 *gdata) >> +{ >> + __u8 hour, min, sec, day, mon, year, century, *timestamp; >> + >> + if (gdata->validation_bits & ACPI_HEST_GEN_VALID_TIMESTAMP) { >> + timestamp = (__u8 *)&(gdata->time_stamp); >> + sec = bcd2bin(timestamp[0]); >> + min = bcd2bin(timestamp[1]); >> + hour = bcd2bin(timestamp[2]); >> + day = bcd2bin(timestamp[4]); >> + mon = bcd2bin(timestamp[5]); >> + year = bcd2bin(timestamp[6]); >> + century = bcd2bin(timestamp[7]); >> + printk("%stime: %7s %02d%02d-%02d-%02d %02d:%02d:%02d\n", pfx, >> + 0x01 & *(timestamp + 3) ? "precise" : "", century, >> + year, mon, day, hour, min, sec); >> + } >> +} >> + >> static void cper_estatus_print_section( >> - const char *pfx, const struct acpi_hest_generic_data *gdata, int >> sec_no) >> + const char *pfx, struct acpi_hest_generic_data *gdata, int sec_no) >> { >> uuid_le *sec_type = (uuid_le *)gdata->section_type; >> __u16 severity; >> char newpfx[64]; >> >> + if (acpi_hest_generic_data_version(gdata)) >> + cper_estatus_print_section_v300(pfx, >> + (const struct acpi_hest_generic_data_v300 *)gdata); >> + >> severity = gdata->error_severity; >> printk("%s""Error %d, type: %s\n", pfx, sec_no, >> cper_severity_str(severity)); >> @@ -403,14 +430,18 @@ static void cper_estatus_print_section( >> >> snprintf(newpfx, sizeof(newpfx), "%s%s", pfx, INDENT_SP); >> if (!uuid_le_cmp(*sec_type, CPER_SEC_PROC_GENERIC)) { >> - struct cper_sec_proc_generic *proc_err = (void *)(gdata + 1); >> + struct cper_sec_proc_generic *proc_err; >> + >> + proc_err = acpi_hest_generic_data_payload(gdata); >> printk("%s""section_type: general processor error\n", newpfx); >> if (gdata->error_data_length >= sizeof(*proc_err)) >> cper_print_proc_generic(newpfx, 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); >> + struct cper_sec_mem_err *mem_err; >> + >> + mem_err = acpi_hest_generic_data_payload(gdata); >> printk("%s""section_type: memory error\n", newpfx); >> if (gdata->error_data_length >= >> sizeof(struct cper_sec_mem_err_old)) >> @@ -419,7 +450,9 @@ static void cper_estatus_print_section( >> else >> goto err_section_too_small; >> } else if (!uuid_le_cmp(*sec_type, CPER_SEC_PCIE)) { >> - struct cper_sec_pcie *pcie = (void *)(gdata + 1); >> + struct cper_sec_pcie *pcie; >> + >> + pcie = acpi_hest_generic_data_payload(gdata); >> printk("%s""section_type: PCIe error\n", newpfx); >> if (gdata->error_data_length >= sizeof(*pcie)) >> cper_print_pcie(newpfx, pcie, gdata); >> @@ -438,6 +471,7 @@ void cper_estatus_print(const char *pfx, >> const struct acpi_hest_generic_status *estatus) >> { >> struct acpi_hest_generic_data *gdata; >> + struct acpi_hest_generic_data_v300 *gdata_v3 = NULL; >> unsigned int data_len, gedata_len; >> int sec_no = 0; >> char newpfx[64]; >> @@ -451,12 +485,22 @@ void cper_estatus_print(const char *pfx, >> printk("%s""event severity: %s\n", pfx, >> cper_severity_str(severity)); >> data_len = estatus->data_length; >> gdata = (struct acpi_hest_generic_data *)(estatus + 1); >> + if (acpi_hest_generic_data_version(gdata)) >> + gdata_v3 = (struct acpi_hest_generic_data_v300 *)gdata; > > I think the acpi_hest_generic_data_version() doesn't check if the > version is > V3 or higher ? Oops :) I need to make sure that returns 3. > >> + >> snprintf(newpfx, sizeof(newpfx), "%s%s", pfx, INDENT_SP); >> + >> while (data_len >= sizeof(*gdata)) { >> gedata_len = gdata->error_data_length; >> cper_estatus_print_section(newpfx, gdata, sec_no); >> - data_len -= gedata_len + sizeof(*gdata); >> - gdata = (void *)(gdata + 1) + gedata_len; >> + if(gdata_v3) { >> + data_len -= gedata_len + sizeof(*gdata_v3); >> + gdata_v3 = (void *)(gdata_v3 + 1) + gedata_len; >> + gdata = (struct acpi_hest_generic_data *)gdata_v3; >> + } else { >> + data_len -= gedata_len + sizeof(*gdata); >> + gdata = (void *)(gdata + 1) + gedata_len; >> + } > > Could we not use the helpers we define below to unify the code here > and avoid the > switch ? > Yes, those helpers should be able to reduce the code here, I'll update this. > >> sec_no++; >> } >> } >> @@ -486,12 +530,13 @@ int cper_estatus_check(const struct >> acpi_hest_generic_status *estatus) >> return rc; >> data_len = estatus->data_length; >> gdata = (struct acpi_hest_generic_data *)(estatus + 1); >> - while (data_len >= sizeof(*gdata)) { >> - gedata_len = gdata->error_data_length; >> - if (gedata_len > data_len - sizeof(*gdata)) >> + >> + while (data_len >= acpi_hest_generic_data_size(gdata)) { >> + gedata_len = acpi_hest_generic_data_error_length(gdata); >> + if (gedata_len > data_len - acpi_hest_generic_data_size(gdata)) >> return -EINVAL; >> - data_len -= gedata_len + sizeof(*gdata); >> - gdata = (void *)(gdata + 1) + gedata_len; >> + data_len -= gedata_len + acpi_hest_generic_data_size(gdata); >> + gdata = acpi_hest_generic_data_next(gdata); >> } >> if (data_len) >> return -EINVAL; >> diff --git a/include/acpi/ghes.h b/include/acpi/ghes.h >> index 68f088a..56b9679 100644 >> --- a/include/acpi/ghes.h >> +++ b/include/acpi/ghes.h >> @@ -73,3 +73,13 @@ static inline void ghes_edac_unregister(struct >> ghes *ghes) >> { >> } >> #endif >> + >> +#define acpi_hest_generic_data_version(gdata) \ >> + (gdata->revision >> 8) >> + > > >> >> +#define acpi_hest_generic_data_error_length(gdata) \ >> + (((struct acpi_hest_generic_data *)(gdata))->error_data_length) >> +#define acpi_hest_generic_data_size(gdata) \ >> + ((acpi_hest_generic_data_version(gdata) >= 3) ? \ >> + sizeof(struct acpi_hest_generic_data_v300) : \ >> + sizeof(struct acpi_hest_generic_data)) >> +#define acpi_hest_generic_data_record_size(gdata) \ >> + (acpi_hest_generic_data_size(gdata) + \ >> + acpi_hest_generic_data_error_length(gdata)) >> +#define acpi_hest_generic_data_next(gdata) \ >> + ((void *)(gdata) + acpi_hest_generic_data_record_size(gdata)) >> + > > Rest looks good > > Cheers > Suzuki Thanks, Tyler -- Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.