lkml.org 
[lkml]   [2017]   [Aug]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH] acpi: apei: fix the wrongly parse generic error status block
From
Date
On 8/11/2017 5:41 PM, gengdongjiu wrote:
> 2017-08-11 21:19 GMT+08:00 Baicar, Tyler <tbaicar@codeaurora.org>:
>> 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

\
 
 \ /
  Last update: 2017-08-14 16:05    [W:0.288 / U:0.208 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site