[BUG] Re: [PATCH v10 1/3] aerdrv: Trace Event for AER

Commit Message

Betty Dall Dec. 5, 2013, 6:21 p.m. UTC

On Wed, 2013-12-04 at 23:28 +0800, Ethan Zhao wrote:
> Rui,
>    Agree with that, there are really many such confusing error type definition
> need to be standardized or unified, some of them are
> ambiguous?inconsistent, some of them violates ACPI/PCI spec.
> 
>   According to the ACPI spec, the 'FATAL' in fact, is a sub-category
> of 'UNCORRECTABLE' , the another one is 'NON-FATAL', then how to
> understand the bebow 'UNCORRECTABLE's ?  mean  uncorrectable-non-fatal
> ? just guess, confusing.
> 
> acpi\actbl1.h
> 
> #define ACPI_EINJ_PROCESSOR_CORRECTABLE     (1)
> #define ACPI_EINJ_PROCESSOR_UNCORRECTABLE   (1<<1)
> #define ACPI_EINJ_PROCESSOR_FATAL           (1<<2)
> #define ACPI_EINJ_MEMORY_CORRECTABLE        (1<<3)
> #define ACPI_EINJ_MEMORY_UNCORRECTABLE      (1<<4)
> #define ACPI_EINJ_MEMORY_FATAL              (1<<5)
> #define ACPI_EINJ_PCIX_CORRECTABLE          (1<<6)
> #define ACPI_EINJ_PCIX_UNCORRECTABLE        (1<<7)
> #define ACPI_EINJ_PCIX_FATAL                (1<<8)
> #define ACPI_EINJ_PLATFORM_CORRECTABLE      (1<<9)
> #define ACPI_EINJ_PLATFORM_UNCORRECTABLE    (1<<10)
> #define ACPI_EINJ_PLATFORM_FATAL            (1<<11)
> 
> edac.h
> 
> enum hw_event_mc_err_type {
> HW_EVENT_ERR_CORRECTED,
> HW_EVENT_ERR_UNCORRECTED,
> HW_EVENT_ERR_FATAL,
> HW_EVENT_ERR_INFO,
> };
> 
> ghes.h
> enum {
> GHES_SEV_NO = 0x0,
> GHES_SEV_CORRECTED = 0x1,
> GHES_SEV_RECOVERABLE = 0x2,
> GHES_SEV_PANIC = 0x3,
> };
> 
> What's the meaning of GHES_SEV_PANIC ? Why not 'FATAL' , just as
> described in ACPI spec section 18.3.2.6.1,
> "
> Error Severity 4 16 Identifies the error severity of the reported error:
> 0 – Recoverable?
> 1 – Fatal?
> 2 – Corrected?
> 3 – None
> "
> If there is other intension, but could be seen translated into 'FATAL' later:
> 
> case GHES_SEV_PANIC:
>   type = HW_EVENT_ERR_FATAL;
> 
> And these looks reasonable,
> aer.h
> 
> #define AER_NONFATAL 0
> #define AER_FATAL 1
> #define AER_CORRECTABLE 2

The definition of the GHES_SEV* matches up with the error severity
definition of the CPER records as defined in the UEFI spec section
N.2.1:
"Indicates the severity of the error condition. The severity of
the error record corresponds to the most severe error
section.
0 - Recoverable (also called non-fatal uncorrected)
1 - Fatal
2 - Corrected
3 - Informational
All other values are reserved.
Note that severity of "Informational" indicates that the record
could be safely ignored by error handling software."

The ghes code uses the CPER record's severity and always calls the
function ghes_severity() to convert to the GHES_SEV value. Since the
ACPI spec defines the GHES severity, it makes sense to maintain an enum
for it and use the ghes_severity() to convert where necessary. This is
what I am thinking:

Author: Betty Dall <[e-mail]>
Date:   Thu Dec 5 11:05:43 2013 -0700



-Betty


--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Borislav Petkov Dec. 5, 2013, 9:12 p.m. UTC | #1

On Thu, Dec 05, 2013 at 11:21:10AM -0700, Betty Dall wrote:
> The definition of the GHES_SEV* matches up with the error severity
> definition of the CPER records as defined in the UEFI spec section
> N.2.1:
> "Indicates the severity of the error condition. The severity of
> the error record corresponds to the most severe error
> section.
> 0 - Recoverable (also called non-fatal uncorrected)
> 1 - Fatal
> 2 - Corrected
> 3 - Informational
> All other values are reserved.
> Note that severity of "Informational" indicates that the record
> could be safely ignored by error handling software."

Actually, we can go even one radical step further and drop
ghes_severity() completely because GHES severity in the ACPI spec 5.0 is
defined almost exactly the same:

"18.3.2.6.1 Generic Error Data

...

Identifies the error severity of the reported error:
0 – Recoverable?
1 – Fatal?
2 – Corrected?
3 – None
Note: This is the error severity of the entire event. Each Generic
Error Data Entry also includes its own Error Severity field."

I don't know which version of the spec dictated

enum {
        GHES_SEV_NO = 0x0,
        GHES_SEV_CORRECTED = 0x1,
        GHES_SEV_RECOVERABLE = 0x2,
        GHES_SEV_PANIC = 0x3,
};

though and whether we're going to have to differentiate between the old
and GHES numerical severity levels. Which, if we have to, would be very
nasty...

Patch

diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index a30bc31..c59144e 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -301,16 +301,18 @@  static inline int ghes_severity(int severity)
 {
        switch (severity) {
        case CPER_SEV_INFORMATIONAL:
-               return GHES_SEV_NO;
+               return GHES_SEV_NONE;
        case CPER_SEV_CORRECTED:
                return GHES_SEV_CORRECTED;
        case CPER_SEV_RECOVERABLE:
                return GHES_SEV_RECOVERABLE;
        case CPER_SEV_FATAL:
-               return GHES_SEV_PANIC;
+               return GHES_SEV_FATAL;
        default:
                /* Unknown, go panic */
-               return GHES_SEV_PANIC;
+               pr_warn(FW_WARN GHES_PFX
+                       "Invalid CPER severity: %d\n", severity);
+               return GHES_SEV_FATAL;
        }
 }
 
@@ -828,7 +830,7 @@  static int ghes_notify_nmi(unsigned int cmd, struct
pt_regs *regs)
        if (ret == NMI_DONE)
                goto out;
 
-       if (sev_global >= GHES_SEV_PANIC) {
+       if (sev_global >= GHES_SEV_FATAL) {
                oops_begin();
                ghes_print_queued_estatus();
                __ghes_print_estatus(KERN_EMERG, ghes_global->generic,
diff --git a/include/acpi/ghes.h b/include/acpi/ghes.h
index dfd60d0..7cefa89 100644
--- a/include/acpi/ghes.h
+++ b/include/acpi/ghes.h
@@ -39,10 +39,10 @@  struct ghes_estatus_cache {
 };
 
 enum {
-       GHES_SEV_NO = 0x0,
-       GHES_SEV_CORRECTED = 0x1,
-       GHES_SEV_RECOVERABLE = 0x2,
-       GHES_SEV_PANIC = 0x3,
+       GHES_SEV_RECOVERABLE = 0x0,
+       GHES_SEV_FATAL = 0x1,
+       GHES_SEV_CORRECTED = 0x2,
+       GHES_SEV_NONE = 0x3,
 };
 
 /* From drivers/edac/ghes_edac.c */