Re: [PATCH] acpi: apei: fix the wrongly parse generic error status block

[Baicar, Tyler]

On 8/11/2017 5:41 PM, gengdongjiu wrote:
> 2017-08-11 21:19 GMT+08:00 Baicar, Tyler <tbaicar@xxxxxxxxxxxxxx>:
> > 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