Re: [PATCH 10/21] EDAC, ghes: Remove pvt->detail_location string

[Robert Richter]

On 29.05.19 16:13:20, James Morse wrote:
> Hi Robert,
> 
> On 29/05/2019 09:44, Robert Richter wrote:
> > The detail_location[] string in struct ghes_edac_pvt is complete
> > useless and data is just copied around. Put everything into
> > e->other_detail from the beginning.
> 
> We still print all that complete-useless detail_location stuff... so this commit message
> had me confused about what you're doing here. I think you meant the space for the string,
> instead of the value!

No, this patch does not remove the driver details nor it changes the
data representation. It changes how that data is prepared internally.
The patch removes the (useless) intermediate buffer detail_location[]
and writes everything directly into other_detail[] buffer.

> 
> | detail_location[] is used to collect two location strings so they can be passed as one
> | to trace_mc_event(). Instead of having an extra copy step, assemble the location string
> | in other_detail[] from the beginning.
> 
> 
> > diff --git a/drivers/edac/ghes_edac.c b/drivers/edac/ghes_edac.c
> > index 39702bac5eaf..c18f16bc9e4d 100644
> > --- a/drivers/edac/ghes_edac.c
> > +++ b/drivers/edac/ghes_edac.c
> > @@ -23,8 +23,7 @@ struct ghes_edac_pvt {
> >  	struct mem_ctl_info *mci;
> >  
> >  	/* Buffers for the error handling routine */
> > -	char detail_location[240];
> > -	char other_detail[160];
> > +	char other_detail[400];
> >  	char msg[80];
> >  };
> >  
> > @@ -225,13 +224,14 @@ void ghes_edac_report_mem_error(int sev, struct cper_sec_mem_err *mem_err)
> >  	memset(e, 0, sizeof (*e));
> >  	e->error_count = 1;
> >  	strcpy(e->label, "unknown label");
> > -	e->msg = pvt->msg;
> > -	e->other_detail = pvt->other_detail;
> >  	e->top_layer = -1;
> >  	e->mid_layer = -1;
> >  	e->low_layer = -1;
> > -	*pvt->other_detail = '\0';
> > +	e->msg = pvt->msg;
> > +	e->other_detail = pvt->other_detail;
> > +
> >  	*pvt->msg = '\0';
> > +	*pvt->other_detail = '\0';
> 
> ... so no change? Could you drop this hunk?

No actual change here, but I found it useful to reorder things here
for comparization with the similar code block in
edac_mc_handle_error().


> 
> Regardless,
> Reviewed-by: James Morse <james.morse@xxxxxxx>

Thank you for review.

-Robert

> 
> 
> Thanks,
> 
> James