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

Message ID 20190529084344.28562-11-rrichter@marvell.com
State Superseded
Headers show
Series
  • EDAC, mc, ghes: Fixes and updates to improve memory error reporting
Related show

Commit Message

Robert Richter May 29, 2019, 8:44 a.m.
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.

Signed-off-by: Robert Richter <rrichter@marvell.com>

---
 drivers/edac/ghes_edac.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

-- 
2.20.1

Comments

James Morse May 29, 2019, 3:13 p.m. | #1
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!

| 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?

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



Thanks,

James
Robert Richter June 12, 2019, 6:13 p.m. | #2
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@arm.com>


Thank you for review.

-Robert

> 

> 

> Thanks,

> 

> James

Patch

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';
 
 	switch (sev) {
 	case GHES_SEV_CORRECTED:
@@ -359,6 +359,8 @@  void ghes_edac_report_mem_error(int sev, struct cper_sec_mem_err *mem_err)
 
 	/* All other fields are mapped on e->other_detail */
 	p = pvt->other_detail;
+	p += snprintf(p, sizeof(pvt->other_detail),
+		"APEI location: %s ", e->location);
 	if (mem_err->validation_bits & CPER_MEM_VALID_ERROR_STATUS) {
 		u64 status = mem_err->error_status;
 
@@ -434,12 +436,11 @@  void ghes_edac_report_mem_error(int sev, struct cper_sec_mem_err *mem_err)
 
 	/* Generate the trace event */
 	grain_bits = fls_long(e->grain);
-	snprintf(pvt->detail_location, sizeof(pvt->detail_location),
-		 "APEI location: %s %s", e->location, e->other_detail);
+
 	trace_mc_event(type, e->msg, e->label, e->error_count,
 		       mci->mc_idx, e->top_layer, e->mid_layer, e->low_layer,
 		       (e->page_frame_number << PAGE_SHIFT) | e->offset_in_page,
-		       grain_bits, e->syndrome, pvt->detail_location);
+		       grain_bits, e->syndrome, e->other_detail);
 
 	dimm_info = edac_get_dimm_by_index(mci, e->top_layer);