From: "Kani, Toshimitsu" <toshi.kani@hpe.com>
To: "bp@alien8.de" <bp@alien8.de>
CC: "linux-edac@vger.kernel.org" <linux-edac@vger.kernel.org>,
        "lenb@kernel.org" <lenb@kernel.org>,
        "mchehab@kernel.org" <mchehab@kernel.org>,
        "tony.luck@intel.com" <tony.luck@intel.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "rjw@rjwysocki.net" <rjw@rjwysocki.net>,
        "linux-acpi@vger.kernel.org" <linux-acpi@vger.kernel.org>
Subject: Re: [PATCH v2 4/7] ghes_edac: avoid multiple calls to dmi_walk()
Thread-Topic: [PATCH v2 4/7] ghes_edac: avoid multiple calls to dmi_walk()
Thread-Index: AQHTDKTuWyxK/OMe00e0Ovda8CeGBqJzlIKAgAEZcQA=
Date: Fri, 4 Aug 2017 21:02:17 +0000
Message-ID: <1501879964.2042.117.camel@hpe.com>
References: <20170803215753.30553-1-toshi.kani@hpe.com>
         <20170803215753.30553-5-toshi.kani@hpe.com>
         <20170804040525.GE12082@nazgul.tnic>
In-Reply-To: <20170804040525.GE12082@nazgul.tnic>
Accept-Language: en-US
Content-Language: en-US
spamdiagnosticoutput: 1:99
spamdiagnosticmetadata: NSPM
Content-Type: text/plain; charset="utf-8"
Content-ID: <9C7235CF1EDB634C8FE73BD985DD43FC@NAMPRD84.PROD.OUTLOOK.COM>
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-originalarrivaltime: 04 Aug 2017 21:02:17.2590
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 105b2061-b669-4b31-92ac-24d304d195dc
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DF4PR84MB0185
X-OriginatorOrg: hpe.com
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Transfer-Encoding: 8bit
X-MIME-Autoconverted: from base64 to 8bit by nfs id v74L2iIC004435
Content-Length: 2180
Lines: 60

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@hpe.com>
> > Suggested-by: Borislav Petkov <bp@alien8.de>
> > Cc: Borislav Petkov <bp@alien8.de>
> > Cc: Mauro Carvalho Chehab <mchehab@kernel.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