linux-kernel - Re: [PATCH 10/21] EDAC, ghes: Remove pvt->detail

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20190612181309.l36jlynvqe7xaqmi@rric.localdomain>
Date:   Wed, 12 Jun 2019 18:13:16 +0000
From:   Robert Richter <rrichter@...vell.com>
To:     James Morse <james.morse@....com>
CC:     Borislav Petkov <bp@...en8.de>, Tony Luck <tony.luck@...el.com>,
        "Mauro Carvalho Chehab" <mchehab@...nel.org>,
        "linux-edac@...r.kernel.org" <linux-edac@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 10/21] EDAC, ghes: Remove pvt->detail_location string

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@....com>

Thank you for review.

-Robert

> 
> 
> Thanks,
> 
> James