diff mbox

[7/8] ACPI, APEI, CPER: Cleanup CPER memory error output format

Message ID 1381473166-29303-8-git-send-email-gong.chen@linux.intel.com (mailing list archive)
State Not Applicable, archived
Headers show

Commit Message

Chen Gong Oct. 11, 2013, 6:32 a.m. UTC
Keep up only the most important fields for memory error
reporting. The detail information will be moved to perf/trace
interface.

Suggested-by: Tony Luck <tony.luck@intel.com>
Signed-off-by: Chen, Gong <gong.chen@linux.intel.com>
---
 drivers/acpi/apei/cper.c | 42 ++++++++++++++----------------------------
 1 file changed, 14 insertions(+), 28 deletions(-)

Comments

Borislav Petkov Oct. 11, 2013, 4:02 p.m. UTC | #1
On Fri, Oct 11, 2013 at 02:32:45AM -0400, Chen, Gong wrote:
> Keep up only the most important fields for memory error
> reporting. The detail information will be moved to perf/trace
> interface.
> 
> Suggested-by: Tony Luck <tony.luck@intel.com>
> Signed-off-by: Chen, Gong <gong.chen@linux.intel.com>
> ---
>  drivers/acpi/apei/cper.c | 42 ++++++++++++++----------------------------
>  1 file changed, 14 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/acpi/apei/cper.c b/drivers/acpi/apei/cper.c
> index 2a4389f..567410e 100644
> --- a/drivers/acpi/apei/cper.c
> +++ b/drivers/acpi/apei/cper.c
> @@ -206,29 +206,29 @@ static void cper_print_mem(const char *pfx, const struct cper_sec_mem_err *mem)
>  		printk("%s""physical_address_mask: 0x%016llx\n",
>  		       pfx, mem->physical_addr_mask);
>  	if (mem->validation_bits & CPER_MEM_VALID_NODE)
> -		printk("%s""node: %d\n", pfx, mem->node);
> +		pr_debug("node: %d\n", mem->node);
>  	if (mem->validation_bits & CPER_MEM_VALID_CARD)
> -		printk("%s""card: %d\n", pfx, mem->card);
> +		pr_debug("card: %d\n", mem->card);
>  	if (mem->validation_bits & CPER_MEM_VALID_MODULE)
> -		printk("%s""module: %d\n", pfx, mem->module);
> +		pr_debug("module: %d\n", mem->module);
>  	if (mem->validation_bits & CPER_MEM_VALID_RANK_NUMBER)
> -		printk("%s""rank: %d\n", pfx, mem->rank);
> +		pr_debug("rank: %d\n", mem->rank);
>  	if (mem->validation_bits & CPER_MEM_VALID_BANK)
> -		printk("%s""bank: %d\n", pfx, mem->bank);
> +		pr_debug("bank: %d\n", mem->bank);
>  	if (mem->validation_bits & CPER_MEM_VALID_DEVICE)
> -		printk("%s""device: %d\n", pfx, mem->device);
> +		pr_debug("device: %d\n", mem->device);
>  	if (mem->validation_bits & CPER_MEM_VALID_ROW)
> -		printk("%s""row: %d\n", pfx, mem->row);
> +		pr_debug("row: %d\n", mem->row);
>  	if (mem->validation_bits & CPER_MEM_VALID_COLUMN)
> -		printk("%s""column: %d\n", pfx, mem->column);
> +		pr_debug("column: %d\n", mem->column);
>  	if (mem->validation_bits & CPER_MEM_VALID_BIT_POSITION)
> -		printk("%s""bit_position: %d\n", pfx, mem->bit_pos);
> +		pr_debug("bit_position: %d\n", mem->bit_pos);
>  	if (mem->validation_bits & CPER_MEM_VALID_REQUESTOR_ID)
> -		printk("%s""requestor_id: 0x%016llx\n", pfx, mem->requestor_id);
> +		pr_debug("requestor_id: 0x%016llx\n", mem->requestor_id);
>  	if (mem->validation_bits & CPER_MEM_VALID_RESPONDER_ID)
> -		printk("%s""responder_id: 0x%016llx\n", pfx, mem->responder_id);
> +		pr_debug("responder_id: 0x%016llx\n", mem->responder_id);
>  	if (mem->validation_bits & CPER_MEM_VALID_TARGET_ID)
> -		printk("%s""target_id: 0x%016llx\n", pfx, mem->target_id);
> +		pr_debug("target_id: 0x%016llx\n", mem->target_id);
>  	if (mem->validation_bits & CPER_MEM_VALID_ERROR_TYPE) {
>  		u8 etype = mem->error_type;
>  		printk("%s""error_type: %d, %s\n", pfx, etype,

Hmm, so this cper_print_mem is called for CPER_SEC_PLATFORM_MEM section
type.

With the change above, the other caller __ghes_print_estatus() won't see
the error messages if they're debug. Do we want that?
Chen Gong Oct. 14, 2013, 4:55 a.m. UTC | #2
On Fri, Oct 11, 2013 at 06:02:08PM +0200, Borislav Petkov wrote:
> Date: Fri, 11 Oct 2013 18:02:08 +0200
> From: Borislav Petkov <bp@alien8.de>
> To: "Chen, Gong" <gong.chen@linux.intel.com>
> Cc: tony.luck@intel.com, linux-kernel@vger.kernel.org,
>  linux-acpi@vger.kernel.org
> Subject: Re: [PATCH 7/8] ACPI, APEI, CPER: Cleanup CPER memory error output
>  format
> User-Agent: Mutt/1.5.21 (2010-09-15)
> 
> On Fri, Oct 11, 2013 at 02:32:45AM -0400, Chen, Gong wrote:
> > Keep up only the most important fields for memory error
> > reporting. The detail information will be moved to perf/trace
> > interface.
> > 
> > Suggested-by: Tony Luck <tony.luck@intel.com>
> > Signed-off-by: Chen, Gong <gong.chen@linux.intel.com>
> > ---
> >  drivers/acpi/apei/cper.c | 42 ++++++++++++++----------------------------
> >  1 file changed, 14 insertions(+), 28 deletions(-)
> > 
> > diff --git a/drivers/acpi/apei/cper.c b/drivers/acpi/apei/cper.c
> > index 2a4389f..567410e 100644
> > --- a/drivers/acpi/apei/cper.c
> > +++ b/drivers/acpi/apei/cper.c
> > @@ -206,29 +206,29 @@ static void cper_print_mem(const char *pfx, const struct cper_sec_mem_err *mem)
> >  		printk("%s""physical_address_mask: 0x%016llx\n",
> >  		       pfx, mem->physical_addr_mask);
> >  	if (mem->validation_bits & CPER_MEM_VALID_NODE)
> > -		printk("%s""node: %d\n", pfx, mem->node);
> > +		pr_debug("node: %d\n", mem->node);
> >  	if (mem->validation_bits & CPER_MEM_VALID_CARD)
> > -		printk("%s""card: %d\n", pfx, mem->card);
> > +		pr_debug("card: %d\n", mem->card);
> >  	if (mem->validation_bits & CPER_MEM_VALID_MODULE)
> > -		printk("%s""module: %d\n", pfx, mem->module);
> > +		pr_debug("module: %d\n", mem->module);
> >  	if (mem->validation_bits & CPER_MEM_VALID_RANK_NUMBER)
> > -		printk("%s""rank: %d\n", pfx, mem->rank);
> > +		pr_debug("rank: %d\n", mem->rank);
> >  	if (mem->validation_bits & CPER_MEM_VALID_BANK)
> > -		printk("%s""bank: %d\n", pfx, mem->bank);
> > +		pr_debug("bank: %d\n", mem->bank);
> >  	if (mem->validation_bits & CPER_MEM_VALID_DEVICE)
> > -		printk("%s""device: %d\n", pfx, mem->device);
> > +		pr_debug("device: %d\n", mem->device);
> >  	if (mem->validation_bits & CPER_MEM_VALID_ROW)
> > -		printk("%s""row: %d\n", pfx, mem->row);
> > +		pr_debug("row: %d\n", mem->row);
> >  	if (mem->validation_bits & CPER_MEM_VALID_COLUMN)
> > -		printk("%s""column: %d\n", pfx, mem->column);
> > +		pr_debug("column: %d\n", mem->column);
> >  	if (mem->validation_bits & CPER_MEM_VALID_BIT_POSITION)
> > -		printk("%s""bit_position: %d\n", pfx, mem->bit_pos);
> > +		pr_debug("bit_position: %d\n", mem->bit_pos);
> >  	if (mem->validation_bits & CPER_MEM_VALID_REQUESTOR_ID)
> > -		printk("%s""requestor_id: 0x%016llx\n", pfx, mem->requestor_id);
> > +		pr_debug("requestor_id: 0x%016llx\n", mem->requestor_id);
> >  	if (mem->validation_bits & CPER_MEM_VALID_RESPONDER_ID)
> > -		printk("%s""responder_id: 0x%016llx\n", pfx, mem->responder_id);
> > +		pr_debug("responder_id: 0x%016llx\n", mem->responder_id);
> >  	if (mem->validation_bits & CPER_MEM_VALID_TARGET_ID)
> > -		printk("%s""target_id: 0x%016llx\n", pfx, mem->target_id);
> > +		pr_debug("target_id: 0x%016llx\n", mem->target_id);
> >  	if (mem->validation_bits & CPER_MEM_VALID_ERROR_TYPE) {
> >  		u8 etype = mem->error_type;
> >  		printk("%s""error_type: %d, %s\n", pfx, etype,
> 
> Hmm, so this cper_print_mem is called for CPER_SEC_PLATFORM_MEM section
> type.
> 
> With the change above, the other caller __ghes_print_estatus() won't see
> the error messages if they're debug. Do we want that?
> 

Because most of data in CPER are empty or unimportant. To avoid too much
disturbance to end users, it is worthy to do that. Moreover, I reserve
another way via trace to show these data.

> -- 
> Regards/Gruss,
>     Boris.
> 
> Sent from a fat crate under my desk. Formatting is fine.
> --
Borislav Petkov Oct. 14, 2013, 10:36 a.m. UTC | #3
On Mon, Oct 14, 2013 at 12:55:00AM -0400, Chen Gong wrote:
> Because most of data in CPER are empty or unimportant.

It is not about whether it is important or not - the question is whether
changing existing functionality which someone might rely upon is a
problem here? Someone might be expecting exactly those messages to
appear in dmesg.

Btw, what is the real reason for this patch, to save yourself the output
in dmesg? If so, you can disable this output when eMCA module has been
loaded but leave it intact otherwise.
Tony Luck Oct. 14, 2013, 5:12 p.m. UTC | #4
On Mon, Oct 14, 2013 at 3:36 AM, Borislav Petkov <bp@alien8.de> wrote:
> On Mon, Oct 14, 2013 at 12:55:00AM -0400, Chen Gong wrote:
>> Because most of data in CPER are empty or unimportant.
>
> It is not about whether it is important or not - the question is whether
> changing existing functionality which someone might rely upon is a
> problem here? Someone might be expecting exactly those messages to
> appear in dmesg.

Pulling in a couple more people who have been touching error reporting
code in the last year or so (Hi Lance, Naveen ... feel free to drag more
people to look at this thread).

I prodded Chen Gong in to make this change because our console messages
are way to verbose (and scary) for simple corrected errors.  There are 18
fields in the memory error section (as of UEFI 2.4 ... more are likely to be
added because there are issues that some of the 16-bit wide fields are too
small to handle increased internal values in modern DIMMs).  Whether you
print that one item per line, or a few very long lines - it is way
more information
than the average user will ever want or need to see.

> Btw, what is the real reason for this patch, to save yourself the output
> in dmesg? If so, you can disable this output when eMCA module has been
> loaded but leave it intact otherwise.

This is an excellent idea - if the full data is being logged via TRACE, then
we can drop to virtually nothing on the console (just have something similar
to the "Machine check events logged" message so that people will get a
tip that they might want to go dig into other logs).  Maybe something like:
    %d corrected memory errors\n", count
[rate limited]

But we'd have to make sure that the existing user(s) of this code also
have a TRACE path.

-Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Borislav Petkov Oct. 14, 2013, 6:47 p.m. UTC | #5
On Mon, Oct 14, 2013 at 10:12:35AM -0700, Tony Luck wrote:
> This is an excellent idea - if the full data is being logged via
> TRACE, then we can drop to virtually nothing on the console (just have
> something similar to the "Machine check events logged" message so that
> people will get a tip that they might want to go dig into other logs).
> Maybe something like: %d corrected memory errors\n", count [rate
> limited]
>
> But we'd have to make sure that the existing user(s) of this code also
> have a TRACE path.

It is basically the same idea as with the ras daemon - if we have a
userspace consumer of ras trace events, we disable dmesg output. So the
decision will be left to the userspace tool to disable dmesg output as a
last step of its initialization.
Tony Luck Oct. 14, 2013, 9:03 p.m. UTC | #6
On Mon, Oct 14, 2013 at 11:47 AM, Borislav Petkov <bp@alien8.de> wrote:
> It is basically the same idea as with the ras daemon - if we have a
> userspace consumer of ras trace events, we disable dmesg output. So the
> decision will be left to the userspace tool to disable dmesg output as a
> last step of its initialization.

Do you have a suggested mechanism for this disabling of dmesg?

-Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Borislav Petkov Oct. 14, 2013, 9:50 p.m. UTC | #7
On Mon, Oct 14, 2013 at 02:03:16PM -0700, Tony Luck wrote:
> Do you have a suggested mechanism for this disabling of dmesg?

Hmm, how about a 64-bit flag variable (we can use the remaining bits
for other stuff later) called x86_ras_flags which is private to
arch/x86/ras/core.c (a new file)...

 [ btw, I'm thinking of something similar to efi's x86_efi_facility which we
 nicely query with test_bit and set with set_bit and clear_bit, etc, etc ]

Also, look at arch/x86/platform/efi/efi.c::efi_enabled() how it hides
the actual variable and we can do something similar so that eMCA and
other users like cper.c can do

apei_estatus_print_section:

	if (!ras_tracepoint_enabled())
		cper_print_mem(...)

We set the bit in x86_ras_flags from, say, debugfs, i.e., a userspace
tool sets it and from that moment on all RAS output is rerouted to the
trace event. I.e., the output for which there is a trace event...

How does that look like?

Purely hypothetical, of course, I might me missing something but it
looks ok from here. As always, the devil is in the detail, of course.

HTH.
Chen Gong Oct. 15, 2013, 9:18 a.m. UTC | #8
On Mon, Oct 14, 2013 at 11:50:47PM +0200, Borislav Petkov wrote:
> Date: Mon, 14 Oct 2013 23:50:47 +0200
> From: Borislav Petkov <bp@alien8.de>
> To: Tony Luck <tony.luck@gmail.com>
> Cc: Chen Gong <gong.chen@linux.intel.com>, Linux Kernel Mailing List
>  <linux-kernel@vger.kernel.org>, linux-acpi <linux-acpi@vger.kernel.org>,
>  Lance Ortiz <lance.ortiz@hp.com>, "Naveen N. Rao"
>  <naveen.n.rao@linux.vnet.ibm.com>
> Subject: Re: [PATCH 7/8] ACPI, APEI, CPER: Cleanup CPER memory error output
>  format
> User-Agent: Mutt/1.5.21 (2010-09-15)
> 
> On Mon, Oct 14, 2013 at 02:03:16PM -0700, Tony Luck wrote:
> > Do you have a suggested mechanism for this disabling of dmesg?
> 
> Hmm, how about a 64-bit flag variable (we can use the remaining bits
> for other stuff later) called x86_ras_flags which is private to
> arch/x86/ras/core.c (a new file)...
> 
>  [ btw, I'm thinking of something similar to efi's x86_efi_facility which we
>  nicely query with test_bit and set with set_bit and clear_bit, etc, etc ]
> 
> Also, look at arch/x86/platform/efi/efi.c::efi_enabled() how it hides
> the actual variable and we can do something similar so that eMCA and
> other users like cper.c can do
> 
> apei_estatus_print_section:
> 
> 	if (!ras_tracepoint_enabled())
> 		cper_print_mem(...)
> 
> We set the bit in x86_ras_flags from, say, debugfs, i.e., a userspace
> tool sets it and from that moment on all RAS output is rerouted to the
> trace event. I.e., the output for which there is a trace event...
> 
> How does that look like?
> 

Looks fine to me. But a little bit out of current patch series. How
about adding such interfaces after this patch series is merged?
And from your words, it looks like you prefer to reserve all current
fields to avoid breaking user space things. IOW, drop patch [7/8]
and use another patch with above idea to get the same purpose. This
is what you want, Boris?
Borislav Petkov Oct. 15, 2013, 10:13 a.m. UTC | #9
On Tue, Oct 15, 2013 at 05:18:35AM -0400, Chen Gong wrote:
> Looks fine to me. But a little bit out of current patch series. How
> about adding such interfaces after this patch series is merged?

Ok.

> And from your words, it looks like you prefer to reserve all current
> fields to avoid breaking user space things. IOW, drop patch [7/8]

Well, I was simply wondering whether something is using those. And since
we don't know, apparently, we probably should keep them for now.

> and use another patch with above idea to get the same purpose. This
> is what you want, Boris?

Yeah, let's not break userspace. :-)

Thanks.
Naveen N. Rao Oct. 15, 2013, 11:28 a.m. UTC | #10
On 10/14/2013 10:42 PM, Tony Luck wrote:
> On Mon, Oct 14, 2013 at 3:36 AM, Borislav Petkov <bp@alien8.de> wrote:
>> On Mon, Oct 14, 2013 at 12:55:00AM -0400, Chen Gong wrote:
>>> Because most of data in CPER are empty or unimportant.
>>
>> It is not about whether it is important or not - the question is whether
>> changing existing functionality which someone might rely upon is a
>> problem here? Someone might be expecting exactly those messages to
>> appear in dmesg.
>
> Pulling in a couple more people who have been touching error reporting
> code in the last year or so (Hi Lance, Naveen ... feel free to drag more
> people to look at this thread).
>
> I prodded Chen Gong in to make this change because our console messages
> are way to verbose (and scary) for simple corrected errors.  There are 18
> fields in the memory error section (as of UEFI 2.4 ... more are likely to be
> added because there are issues that some of the 16-bit wide fields are too
> small to handle increased internal values in modern DIMMs).  Whether you
> print that one item per line, or a few very long lines - it is way
> more information
> than the average user will ever want or need to see.

I completely agree and I am all for bringing down the verbosity of GHES 
logs. In my testing, a corrected error event reported through GHES takes 
upwards of 10 lines, which is far too much. Perhaps a single line per 
GHES event with only a few important fields would be better?


Thanks,
Naveen

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Naveen N. Rao Oct. 15, 2013, 11:41 a.m. UTC | #11
On 10/14/2013 10:42 PM, Tony Luck wrote:
> On Mon, Oct 14, 2013 at 3:36 AM, Borislav Petkov <bp@alien8.de> wrote:
>> On Mon, Oct 14, 2013 at 12:55:00AM -0400, Chen Gong wrote:
>>> Because most of data in CPER are empty or unimportant.
>>
>> It is not about whether it is important or not - the question is whether
>> changing existing functionality which someone might rely upon is a
>> problem here? Someone might be expecting exactly those messages to
>> appear in dmesg.

If so, how will disabling dmesg output and switching to a trace event 
help? The user-space program will still break unless they add support 
for that trace event, right?

I'm not sure if we actually provide guarantees w.r.t kernel log 
messages. The issue in this specific scenario is the sheer verbosity of 
the log messages. I personally feel that it will be good if we can 
simplify the log output.

Thanks,
Naveen

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Borislav Petkov Oct. 15, 2013, 12:29 p.m. UTC | #12
On Tue, Oct 15, 2013 at 05:11:59PM +0530, Naveen N. Rao wrote:
> If so, how will disabling dmesg output and switching to a trace event
> help? The user-space program will still break unless they add support
> for that trace event, right?

Well, as yourself this: what happens to a userspace program which relies
on this *exact* error message format and greps dmesg?

> I'm not sure if we actually provide guarantees w.r.t kernel log
> messages. The issue in this specific scenario is the sheer verbosity
> of the log messages.

No, the issue is what I said above: userspace programs expecting exactly
this output. Once we exposed it to userspace, we cannot simply assume
that nothing is using it.
Joe Perches Oct. 15, 2013, 4:42 p.m. UTC | #13
On Tue, 2013-10-15 at 14:29 +0200, Borislav Petkov wrote:
> Once we exposed it to userspace, we cannot simply assume
> that nothing is using it.

Yes, we can.

No guarantees are made about dmesg output.

If guarantees were made, no typo could ever be fixed
nor could any conversion from printk to dev_<level>
be done.

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tony Luck Oct. 15, 2013, 4:49 p.m. UTC | #14
On Tue, Oct 15, 2013 at 9:42 AM, Joe Perches <joe@perches.com> wrote:
> No guarantees are made about dmesg output.

I'm with Joe here.  Current users that parse dmesg have grumbled a bit
that format changes - but they know that's the way life is.  Perhaps they'll
be too busy cheering about the TRACE formats that we are going to give
them that they won't whine too much about this.

-Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Borislav Petkov Oct. 15, 2013, 4:56 p.m. UTC | #15
On Tue, Oct 15, 2013 at 09:49:06AM -0700, Tony Luck wrote:
> On Tue, Oct 15, 2013 at 9:42 AM, Joe Perches <joe@perches.com> wrote:
> > No guarantees are made about dmesg output.
> 
> I'm with Joe here.  Current users that parse dmesg have grumbled a bit
> that format changes - but they know that's the way life is.  Perhaps they'll
> be too busy cheering about the TRACE formats that we are going to give
> them that they won't whine too much about this.

Ok, I'll make sure to forward all complaints to you and Joe then. :)

In any case, we talked about it, you think it is not worth preserving
it, we can drop the printks and see who cries out.
diff mbox

Patch

diff --git a/drivers/acpi/apei/cper.c b/drivers/acpi/apei/cper.c
index 2a4389f..567410e 100644
--- a/drivers/acpi/apei/cper.c
+++ b/drivers/acpi/apei/cper.c
@@ -206,29 +206,29 @@  static void cper_print_mem(const char *pfx, const struct cper_sec_mem_err *mem)
 		printk("%s""physical_address_mask: 0x%016llx\n",
 		       pfx, mem->physical_addr_mask);
 	if (mem->validation_bits & CPER_MEM_VALID_NODE)
-		printk("%s""node: %d\n", pfx, mem->node);
+		pr_debug("node: %d\n", mem->node);
 	if (mem->validation_bits & CPER_MEM_VALID_CARD)
-		printk("%s""card: %d\n", pfx, mem->card);
+		pr_debug("card: %d\n", mem->card);
 	if (mem->validation_bits & CPER_MEM_VALID_MODULE)
-		printk("%s""module: %d\n", pfx, mem->module);
+		pr_debug("module: %d\n", mem->module);
 	if (mem->validation_bits & CPER_MEM_VALID_RANK_NUMBER)
-		printk("%s""rank: %d\n", pfx, mem->rank);
+		pr_debug("rank: %d\n", mem->rank);
 	if (mem->validation_bits & CPER_MEM_VALID_BANK)
-		printk("%s""bank: %d\n", pfx, mem->bank);
+		pr_debug("bank: %d\n", mem->bank);
 	if (mem->validation_bits & CPER_MEM_VALID_DEVICE)
-		printk("%s""device: %d\n", pfx, mem->device);
+		pr_debug("device: %d\n", mem->device);
 	if (mem->validation_bits & CPER_MEM_VALID_ROW)
-		printk("%s""row: %d\n", pfx, mem->row);
+		pr_debug("row: %d\n", mem->row);
 	if (mem->validation_bits & CPER_MEM_VALID_COLUMN)
-		printk("%s""column: %d\n", pfx, mem->column);
+		pr_debug("column: %d\n", mem->column);
 	if (mem->validation_bits & CPER_MEM_VALID_BIT_POSITION)
-		printk("%s""bit_position: %d\n", pfx, mem->bit_pos);
+		pr_debug("bit_position: %d\n", mem->bit_pos);
 	if (mem->validation_bits & CPER_MEM_VALID_REQUESTOR_ID)
-		printk("%s""requestor_id: 0x%016llx\n", pfx, mem->requestor_id);
+		pr_debug("requestor_id: 0x%016llx\n", mem->requestor_id);
 	if (mem->validation_bits & CPER_MEM_VALID_RESPONDER_ID)
-		printk("%s""responder_id: 0x%016llx\n", pfx, mem->responder_id);
+		pr_debug("responder_id: 0x%016llx\n", mem->responder_id);
 	if (mem->validation_bits & CPER_MEM_VALID_TARGET_ID)
-		printk("%s""target_id: 0x%016llx\n", pfx, mem->target_id);
+		pr_debug("target_id: 0x%016llx\n", mem->target_id);
 	if (mem->validation_bits & CPER_MEM_VALID_ERROR_TYPE) {
 		u8 etype = mem->error_type;
 		printk("%s""error_type: %d, %s\n", pfx, etype,
@@ -296,15 +296,6 @@  static void cper_print_pcie(const char *pfx, const struct cper_sec_pcie *pcie,
 	pfx, pcie->bridge.secondary_status, pcie->bridge.control);
 }
 
-static const char *cper_estatus_section_flag_strs[] = {
-	"primary",
-	"containment warning",
-	"reset",
-	"error threshold exceeded",
-	"resource not accessible",
-	"latent error",
-};
-
 static void cper_estatus_print_section(
 	const char *pfx, const struct acpi_generic_data *gdata, int sec_no)
 {
@@ -312,11 +303,8 @@  static void cper_estatus_print_section(
 	__u16 severity;
 
 	severity = gdata->error_severity;
-	printk("%s""section: %d, severity: %d, %s\n", pfx, sec_no, severity,
+	printk("%s""sub_event[%d], severity: %s\n", pfx, sec_no,
 	       cper_severity_str(severity));
-	printk("%s""flags: 0x%02x\n", pfx, gdata->flags);
-	cper_print_bits(pfx, gdata->flags, cper_estatus_section_flag_strs,
-			ARRAY_SIZE(cper_estatus_section_flag_strs));
 	if (gdata->validation_bits & CPER_SEC_VALID_FRU_ID)
 		printk("%s""fru_id: %pUl\n", pfx, (uuid_le *)gdata->fru_id);
 	if (gdata->validation_bits & CPER_SEC_VALID_FRU_TEXT)
@@ -360,10 +348,8 @@  void cper_estatus_print(const char *pfx,
 	int sec_no = 0;
 	__u16 severity;
 
-	printk("%s""Generic Hardware Error Status\n", pfx);
 	severity = estatus->error_severity;
-	printk("%s""severity: %d, %s\n", pfx, severity,
-	       cper_severity_str(severity));
+	printk("%s""event severity: %s\n", pfx, cper_severity_str(severity));
 	data_len = estatus->data_length;
 	gdata = (struct acpi_generic_data *)(estatus + 1);
 	while (data_len >= sizeof(*gdata)) {