| Message ID | 20190529084344.28562-11-rrichter@marvell.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | EDAC, mc, ghes: Fixes and updates to improve memory error reporting | expand |
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
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
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);
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