Re: [PATCH 2/5] CPER: Adjust code flow of some functions

[Borislav Petkov <bp@alien8.de>]

On Wed, Apr 16, 2014 at 02:23:23AM -0400, Chen, Gong wrote:
> We have different CPER error type. For example, for processor error type,
> it has following definition:
> 
> struct cper_sec_proc_generic {
>         __u64   validation_bits;
>         __u8    proc_type;
>         __u8    proc_isa;
>         __u8    proc_error_type;
>         __u8    operation;
>         __u8    flags;
>         __u8    level;
>         __u16   reserved;
>         __u64   cpu_version;
>         char    cpu_brand[128];
>         __u64   proc_id;
>         __u64   target_addr;
>         __u64   requestor_id;
>         __u64   responder_id;
>         __u64   ip;
> };
> 
> If we want to show it in string format, 256 bytes should be not enough. But
> by now we don't use this macro for processor error type so I will shrink it
> to 256 bytes and add a comment for it.

Yeah, we don't have to always adhere to the spec if we feel it doesn't
make any sense. A lot of those fields above are purely useless and we
shouldn't carry them blindly to the outside.

For example, we don't need to carry cpu_brand[128] to the outside for
*every* error. Who even came up with this crap, is beyond me??? It's
like the cpu changes brand on every other error or what? You harvest
this info only *once* from /proc/cpuinfo. cpu_version too. And so on and
so on...

Please sanity-check stuff like that before hardcoding it into the
tracepoint. If it is in the spec it doesn't always mean it makes sense.
We need to carry out only the minimum amount of information of each
error which is actually getting used in userspace, for additional RAS
actions. Carrying fat blobs just because the spec says so is simply
wrong.

Thanks.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--