Re: [PATCH V2 2/9] ras: acpi/apei: cper: generic error data entry v3 per ACPI 6.1

[Baicar, Tyler]

Hello Suzuki,

On 4/14/2016 4:22 AM, Suzuki K Poulose wrote:
> On 06/04/16 16:12, 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 <zjzhang@xxxxxxxxxxxxxx>
> > Signed-off-by: Richard Ruigrok <rruigrok@xxxxxxxxxxxxxx>
> > Signed-off-by: Tyler Baicar <tbaicar@xxxxxxxxxxxxxx>
> > Signed-off-by: Naveen Kaje <nkaje@xxxxxxxxxxxxxx>
> > ---
> >   drivers/acpi/apei/ghes.c    |  35 ++++++++++++--
> >   drivers/firmware/efi/cper.c | 111 
> > +++++++++++++++++++++++++++++++++++++-------
> >   2 files changed, 126 insertions(+), 20 deletions(-)
> >
> > diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> > index 9b0543e..a6848706 100644
> > --- a/drivers/acpi/apei/ghes.c
> > +++ b/drivers/acpi/apei/ghes.c
> > @@ -419,7 +419,15 @@ 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);
> > +    struct acpi_hest_generic_data_v300 *gdata_v3 = NULL;
> > +
> > +    if ((gdata->revision >> 8) >= 0x03)
>
> Could we please make that a macro ? We seem to be using the check 
> everywhere.
Yes, I can make this a macro since we do this several times.
> > +        gdata_v3 = (struct acpi_hest_generic_data_v300 *)gdata;
> > +
> > +    if (gdata_v3)
> > +        mem_err = (struct cper_sec_mem_err *)(gdata_v3 + 1);
> > +    else
> > +        mem_err = (struct cper_sec_mem_err *)(gdata + 1);
> >
> >       if (!(mem_err->validation_bits & CPER_MEM_VALID_PA))
> >           return;
> > @@ -449,14 +457,27 @@ static void ghes_do_proc(struct ghes *ghes,
> >   {
> >       int sev, sec_sev;
> >       struct acpi_hest_generic_data *gdata;
> > +    struct acpi_hest_generic_data_v300 *gdata_v3 = NULL;
> > +    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 ((gdata->revision >> 8) >= 0x03)
> > +            gdata_v3 = (struct acpi_hest_generic_data_v300 *)gdata;
> > +
> > +        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);
> > +
> > +            if (gdata_v3)
> > +                mem_err = (struct cper_sec_mem_err *)
> > +                      (gdata_v3 + 1);
> > +            else
> > +                mem_err = (struct cper_sec_mem_err *)
> > +                      (gdata + 1);
> >               ghes_edac_report_mem_error(ghes, sev, mem_err);
> >
> >               arch_apei_report_mem_error(sev, mem_err);
> > @@ -466,7 +487,13 @@ 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);
> > +
> > +            if (gdata_v3)
> > +                pcie_err = (struct cper_sec_pcie *)
> > +                       (gdata_v3 + 1);
> > +            else
> > +                pcie_err = (struct cper_sec_pcie *)
> > +                       (gdata + 1);
> >               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..23f62962 100644
> > --- a/drivers/firmware/efi/cper.c
> > +++ b/drivers/firmware/efi/cper.c
> > @@ -32,6 +32,8 @@
> >   #include <linux/acpi.h>
> >   #include <linux/pci.h>
> >   #include <linux/aer.h>
> > +#include <linux/printk.h>
> > +#include <linux/bcd.h>
> >
> >   #define INDENT_SP    " "
> >
> > @@ -392,6 +394,10 @@ static void cper_estatus_print_section(
> >       uuid_le *sec_type = (uuid_le *)gdata->section_type;
> >       __u16 severity;
> >       char newpfx[64];
> > +    struct acpi_hest_generic_data_v300 *gdata_v3 = NULL;
> > +
> > +    if ((gdata->revision >> 8) >= 0x03)
> > +        gdata_v3 = (struct acpi_hest_generic_data_v300 *)gdata;
> >
> >       severity = gdata->error_severity;
> >       printk("%s""Error %d, type: %s\n", pfx, sec_no,
> > @@ -403,14 +409,24 @@ 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;
> > +
> > +        if (gdata_v3)
> > +            proc_err = (void *)(gdata_v3 + 1);
> > +        else
> > +            proc_err = (void *)(gdata + 1);
> >           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;
> > +
> > +        if (gdata_v3)
> > +            mem_err = (void *)(gdata_v3 + 1);
> > +        else
> > +            mem_err = (void *)(gdata + 1);
> >           printk("%s""section_type: memory error\n", newpfx);
> >           if (gdata->error_data_length >=
> >               sizeof(struct cper_sec_mem_err_old))
> > @@ -419,7 +435,12 @@ 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;
> > +
> > +        if (gdata_v3)
> > +            pcie = (void *)(gdata_v3 + 1);
> > +        else
> > +            pcie = (void *)(gdata + 1);
>
>
> The only use of the gdata_v3 above cases is to get the payload(or 
> error_record).
> So instead of spilling these checks all over could we use something 
> like :
>
> #define acpi_hest_generic_data_version(gdata) \
>     (gdata->revision >> 8)
>
> static inline void *
> acpi_hest_generic_data_payload(struct acpi_hest_generic_data *gdata)
> {
>     return acpi_hest_generic_data_version(gdata) >= 3 ?
>         ((struct acpi_hest_generic_data_v300 *)(gdata)) + 1 :
>         gdata + 1;
> }
>
> And then do :
>
>     void *payload = acpi_hest_generic_data_payload(gdata);
Yes, this should simplify the the code to get the payload. This can also 
be done in the ghes.c code above as well.
> >           printk("%s""section_type: PCIe error\n", newpfx);
> >           if (gdata->error_data_length >= sizeof(*pcie))
> >               cper_print_pcie(newpfx, pcie, gdata);
> > @@ -434,10 +455,38 @@ err_section_too_small:
> >       pr_err(FW_WARN "error section length is too small\n");
> >   }
> >
> > +static void cper_estatus_print_section_v300(const char *pfx,
> > +    const struct acpi_hest_generic_data_v300 *gdata, int sec_no)
> > +{
> > +    __u8 hour, min, sec, day, mon, year, century, *timestamp;
> > +
> > +    if (gdata->validation_bits & ACPI_HEST_GEN_VALID_TIMESTAMP) {
> > +        timestamp = (__u8 *)&(gdata->time_stamp);
> > +        memcpy(&sec, timestamp, 1);
> > +        memcpy(&min, timestamp + 1, 1);
> > +        memcpy(&hour, timestamp + 2, 1);
> > +        memcpy(&day, timestamp + 4, 1);
> > +        memcpy(&mon, timestamp + 5, 1);
> > +        memcpy(&year, timestamp + 6, 1);
> > +        memcpy(&century, timestamp + 7, 1);
> > +        printk("%stime: ", pfx);
> > +        printk("%7s", 0x01 & *(timestamp + 3) ? "precise" : "");
> > +        printk(" %02d:%02d:%02d %02d%02d-%02d-%02d\n",
> > +            bcd2bin(hour), bcd2bin(min), bcd2bin(sec),
> > +            bcd2bin(century), bcd2bin(year), bcd2bin(mon),
> > +            bcd2bin(day));
> > +    }
> > +
> > +    cper_estatus_print_section(pfx,
> > +                   (const struct acpi_hest_generic_data *)gdata,
> > +                   sec_no);
> > +}
>
>
> Wouldn't it be better do the v3 header check from 
> cper_erstatus_print_section() and call out
> to cper_erstatus_print_section_v300() ? That way, we can leave the 
> callers unaffected,
> even for future changes.
>
>
> > +    if (gdata_v3) {
> > +        while (data_len >= sizeof(*gdata_v3)) {
> > +            gedata_len = gdata_v3->error_data_length;
> > +            cper_estatus_print_section_v300(newpfx, gdata_v3,
> > +                            sec_no);
> > +            data_len -= gedata_len + sizeof(*gdata_v3);
> > +            gdata_v3 = (void *)(gdata_v3 + 1) + gedata_len;
> > +            sec_no++;
> > +        }
> > +    } else {
> > +        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;
> > +            sec_no++;
> > +        }
>
> With the change mentioned above and storing the sizeof(), we could 
> make this
> hunk a bit more cleaner.
This should certainly clean up this code and I agree it is better to 
leave the callers unaffected.

I'll add all these suggested changes in my next patch-set.

Thanks,
Tyler
> Suzuki

--
Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project