Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752384AbdHNOFC (ORCPT ); Mon, 14 Aug 2017 10:05:02 -0400 Received: from smtp.codeaurora.org ([198.145.29.96]:35392 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750876AbdHNOFA (ORCPT ); Mon, 14 Aug 2017 10:05:00 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 smtp.codeaurora.org 1C03A606B7 Authentication-Results: pdx-caf-mail.web.codeaurora.org; dmarc=none (p=none dis=none) header.from=codeaurora.org Authentication-Results: pdx-caf-mail.web.codeaurora.org; spf=none smtp.mailfrom=tbaicar@codeaurora.org Subject: Re: [PATCH] acpi: apei: fix the wrongly parse generic error status block To: gengdongjiu Cc: gengdongjiu , rjw@rjwysocki.net, Len Brown , linux-acpi@vger.kernel.org, linux-kernel@vger.kernel.org References: <1502388412-20273-1-git-send-email-gengdongjiu@huawei.com> <9ea70eed-f743-0c8b-2a70-27ce4c0d8705@codeaurora.org> <1be489f7-c1c3-63dc-dc0d-af67e5fd03a1@huawei.com> <70ebfc99-0666-fa2b-8ed4-19e1f93194bd@codeaurora.org> From: "Baicar, Tyler" Message-ID: <571aed7f-39aa-b5f0-1ea2-2e57a588d22f@codeaurora.org> Date: Mon, 14 Aug 2017 08:04:52 -0600 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Content-Language: en-US Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3564 Lines: 84 On 8/11/2017 5:41 PM, gengdongjiu wrote: > 2017-08-11 21:19 GMT+08:00 Baicar, Tyler : >> I removed the apei_estatus_for_each_section because it was only being used >> in this one spot even though several other parts of the code do the same >> iteration (it is done several times in the CPER code). I made this iteration >> match the CPER iterations because the CPER iterations are verifying that the >> structures are all valid and lengths are correct. If those checks are being >> done this way, it makes the most sense to mimic that iteration here when >> calling into EDAC and triggering trace events. > I think the macro includes the verification for the structures and > lengths correction, it it does not correct, it will breadk the loop. > I do not see your modifcation does some special validation, it almost > smilar with the macro does. > in all the code there are three functions to do the iteration. > ghes_do_proc/cper_estatus_print/cper_estatus_check > the cper_estatus_check function is especial, because its purpose is > to validate CPER, so it will check every length. but not all > function's purpose is to check, for example cper_estatus_print, as > shown below, so we can use this macro to clear the code. Now we can > see there are two function use it, but in the future, if want to > iterate CPER, we can use the macro if it does not want to do special > thing. > > #define apei_estatus_for_each_section(estatus, section) \ > for (section = (struct acpi_hest_generic_data *)(estatus + 1); \ > (void *)section - (void *)(estatus + 1) < estatus->data_length; \ > ------> here it will check whether length is valid, if not, it will > break the loop > section = acpi_hest_get_next(section)) > > > Original code: > void cper_estatus_print(const char *pfx, > const struct acpi_hest_generic_status *estatus) > { > struct acpi_hest_generic_data *gdata; > unsigned int data_len; > int sec_no = 0; > char newpfx[64]; > __u16 severity; > severity = estatus->error_severity; > if (severity == CPER_SEV_CORRECTED) > printk("%s%s\n", pfx, > "It has been corrected by h/w " > "and requires no further action"); > printk("%s""event severity: %s\n", pfx, cper_severity_str(severity)); > data_len = estatus->data_length; > gdata = (struct acpi_hest_generic_data *)(estatus + 1); > snprintf(newpfx, sizeof(newpfx), "%s%s", pfx, INDENT_SP); > while (data_len >= acpi_hest_get_size(gdata)) { > cper_estatus_print_section(newpfx, gdata, sec_no); > data_len -= acpi_hest_get_record_size(gdata); > gdata = acpi_hest_get_next(gdata); > sec_no++; > } > } > > Can change to: > void cper_estatus_print(const char *pfx, > const struct acpi_hest_generic_status *estatus) > { > struct acpi_hest_generic_data *gdata; > int sec_no = 0; > char newpfx[64]; > __u16 severity; > severity = estatus->error_severity; > if (severity == CPER_SEV_CORRECTED) > printk("%s%s\n", pfx, > "It has been corrected by h/w " > "and requires no further action"); > printk("%s""event severity: %s\n", pfx, cper_severity_str(severity)); > snprintf(newpfx, sizeof(newpfx), "%s%s", pfx, INDENT_SP); > apei_estatus_for_each_section { > cper_estatus_print_section(newpfx, gdata, sec_no); > sec_no++; > } > } This change works too, I think it just makes sense to have the iterations in the CPER and GHES code match. Do you want to add a patch to your patch here to change the CPER code as well? If so, I'll wait for that and test it out. Thanks, Tyler