Re: [PATCH] EDAC/mce_amd: Do not print a stray newline

[Yazen Ghannam <yazen.ghannam@amd.com>]

On Mon, Jul 20, 2026 at 10:43:10AM +0530, Srivatsa S. Bhat wrote:

[...]

> > > > 
> > > > Yeah, Sashiko has a point:
> > > > 
> > > > https://sashiko.dev/#/patchset/20260712031648.486110-1-bp%40kernel.org
> > > > 
> > > > So we want the error records to be compact and fit in a block so that you can
> > > > recognize them easier and thus not print each thing with a newline.
> > > > 
> > > > But those things are not unconditional so you cannot always know.
> > > > 
> > > > I need to think of a slick scheme what to do here.
> > > >
> > > 
> > > Do you mean something other than moving the newline print into an else
> > > block like this?
> > > 
> > > ---
> > >  drivers/edac/mce_amd.c | 10 +++++++---
> > >  1 file changed, 7 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/edac/mce_amd.c b/drivers/edac/mce_amd.c
> > > index bd252cb3c38e..60d1a62e1a95 100644
> > > --- a/drivers/edac/mce_amd.c
> > > +++ b/drivers/edac/mce_amd.c
> > > @@ -870,12 +870,16 @@ amd_decode_mce(struct notifier_block *nb, unsigned long val, void *data)
> > >  				memcpy(&frutext[0], &err->vendor.amd.synd1, 8);
> > >  				memcpy(&frutext[8], &err->vendor.amd.synd2, 8);
> > >  
> > > -				pr_emerg(HW_ERR "FRU Text: %s", frutext);
> > > +				pr_emerg(HW_ERR "FRU Text: %s\n", frutext);
> > >  			}
> > > +		} else {
> > > +			/*
> > > +			 * Explicit line termination to handle nested
> > > +			 * conditional prints correctly.
> > > +			 */
> > > +			pr_cont("\n");
> > >  		}
> > >  
> > > -		pr_cont("\n");
> > > -
> > >  		decode_smca_error(m);
> > >  		goto err_code;
> > >  	}
> > > -- 
> > 
> > Another option could be to build up each newline string and print it
> > once. The gist is to get rid of pr_cont() usage.
> >
> 
> I don't think getting rid of pr_cont() was the intention behind the
> original patch by Boris; he was trying to address a user-visible
> stray newline in the logs, if I understood correctly.

Correct. Though the issue may be resolved with other rework.

> 
> > I've attached an old patch for that. Would this work?
> > 
> 
> The approach to build up each string fully before printing it that you
> demonstrated below does look like a nice improvement in general, but
> I'm afraid it may not be a good fit for this particular subsystem
> though, for the following reasons:
> 
> 1. Given the EDAC subsystem's role in handling/reporting hardware
>    errors, I think the logging preference for this subsystem would be
>    to get the error message out as soon as possible, as the machine is
>    already likely to be in trouble if we are printing out kernel logs at
>    the criticality level of "emergency". So, delaying these prints to
>    construct the full string may not be desirable, as doing so can
>    potentially cripple the ability to get even the error indicators
>    known upto that point printed out (in case of further occurrence of
>    hardware errors).

These EDAC messages are a convenience to the user and are not indicative
of the severity.

In fact, this is another low priority item I think should be addressed.

The "printk" log level could be linked to the error severity. For
example, "info" for corrected errors and "warning" for uncorrected
errors. It is not rational to treat all decoded errors with "emergency"
log level.

The x86 EDAC modules listen on the MCE notifier chain. If there is time
for EDAC to decode, then the event is not an emergency.

> 
> 2. The static buffer sizing for the character string (256) is
>    potentially prone to buffer overflows in the future as and when these
>    print statements evolve (without taking the buffer size into
>    careful consideration for every change to the string).

Yes, that's the goal of patch review.

> 
> 3. This is a much larger (cleanup) change than what Boris had posted
>    earlier (which was ~2 lines).
> 

Yes, sometimes simple issues lead to larger cleanup.

Thanks,
Yazen