linux-kernel - Re: [PATCH v2 4/7] ghes_edac: avoid multiple calls to dmi

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: <1501879964.2042.117.camel@hpe.com>
Date:   Fri, 4 Aug 2017 21:02:17 +0000
From:   "Kani, Toshimitsu" <toshi.kani@....com>
To:     "bp@...en8.de" <bp@...en8.de>
CC:     "linux-edac@...r.kernel.org" <linux-edac@...r.kernel.org>,
        "lenb@...nel.org" <lenb@...nel.org>,
        "mchehab@...nel.org" <mchehab@...nel.org>,
        "tony.luck@...el.com" <tony.luck@...el.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "rjw@...ysocki.net" <rjw@...ysocki.net>,
        "linux-acpi@...r.kernel.org" <linux-acpi@...r.kernel.org>
Subject: Re: [PATCH v2 4/7] ghes_edac: avoid multiple calls to dmi_walk()

On Fri, 2017-08-04 at 06:05 +0200, Borislav Petkov wrote:
> On Thu, Aug 03, 2017 at 03:57:50PM -0600, Toshi Kani wrote:
> > ghes_edac_register() is called for each GHES platform device
> > instantiated per a GHES entry in ACPI HEST table.  dmi_walk()
> > counts the number of DIMMs on the system, and there is no need
> > to call it multiple times.
> > 
> > Change ghes_edac_register() to call dmi_walk() only when
> > 'num_dimm' is uninitialized.
> > 
> > Signed-off-by: Toshi Kani <toshi.kani@....com>
> > Suggested-by: Borislav Petkov <bp@...en8.de>
> > Cc: Borislav Petkov <bp@...en8.de>
> > Cc: Mauro Carvalho Chehab <mchehab@...nel.org>
> > ---
> >  drivers/edac/ghes_edac.c |    9 ++++++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/edac/ghes_edac.c b/drivers/edac/ghes_edac.c
> > index 4e61a62..2e9ce9c 100644
> > --- a/drivers/edac/ghes_edac.c
> > +++ b/drivers/edac/ghes_edac.c
> > @@ -407,15 +407,18 @@
> > EXPORT_SYMBOL_GPL(ghes_edac_report_mem_error);
> >  
> >  int ghes_edac_register(struct ghes *ghes, struct device *dev)
> >  {
> > -	bool fake = false;
> > -	int rc, num_dimm = 0;
> >  	struct mem_ctl_info *mci;
> >  	struct edac_mc_layer layers[1];
> >  	struct ghes_edac_pvt *pvt;
> >  	struct ghes_edac_dimm_fill dimm_fill;
> > +	int rc;
> > +
> > +	static int num_dimm;
> > +	static bool fake;
> >  
> >  	/* Get the number of DIMMs */
> > -	dmi_walk(ghes_edac_count_dimms, &num_dimm);
> > +	if (num_dimm == 0)
> > +		dmi_walk(ghes_edac_count_dimms, &num_dimm);
> 
> So the problem is that ghes_edac_register() gets called multiple
> times depending on how many GHES platform devices are on the system.
> But yet they all scan *all* DIMMs. So instead you should return if
> the DIMMs have been counted already and not register a second time.
> 
> Which makes that whole mc counting kinda useless. So you could rip
> that out too.
> 
> Unless I'm missing something...

GHES platform devices correspond to GHES entries, which define firmware
interfaces to report generic memory errors to the OS, such as NMI and
SCI.  These devices are associated with all DIMMs, not a particular
DIMM.

Thanks,
-Toshi