diff mbox

[v2,4/7] ghes_edac: avoid multiple calls to dmi_walk()

Message ID 20170816082931.p6rpvtlxwt5nccxr@pd.tnic (mailing list archive)
State Not Applicable, archived
Headers show

Commit Message

Borislav Petkov Aug. 16, 2017, 8:29 a.m. UTC
On Tue, Aug 15, 2017 at 08:48:16AM -0700, Luck, Tony wrote:
> Won't the user see all their DIMMs reported for each memory controller
> under /sys/devices/system/edac/mc/mc*/dimm* ?
> 
> That sounds confusing.

Right, and adding the locking was really easy. If only people would
debate less and actually try to do what they're being advised to.
But not really: if you wanna have something done, you have to do it
yourself.

Anyway, I think I have a box to run it to, lemme go find it.

Steve, pls check my locking. It looks straightforward to me but I might
be missing some corner case.

Thx.

---

Comments

Borislav Petkov Aug. 16, 2017, 11:29 a.m. UTC | #1
On Wed, Aug 16, 2017 at 10:29:31AM +0200, Borislav Petkov wrote:
> Anyway, I think I have a box to run it to, lemme go find it.

Seems to boot.

It's a whole another story whether it actually works. :-)

Need some EINJ capabilities urgently but finding a box where it works
reliably is like finding gold.

[    7.784960] ERST: Failed to get Error Log Address Range.
[    7.795905] EDAC DEBUG: edac_mc_alloc: allocating 2444 bytes for mci data (16 dimms, 16 csrows/ch
annels)
[    7.815266] ghes_edac: This EDAC driver relies on BIOS to enumerate memory and get error reports.
[    7.833372] ghes_edac: Unfortunately, not all BIOSes reflect the memory layout correctly.
[    7.850093] ghes_edac: So, the end result of using this driver varies from vendor to vendor.
[    7.867322] ghes_edac: If you find incorrect reports, please contact your hardware vendor
[    7.884034] ghes_edac: to correct its BIOS.
[    7.892599] ghes_edac: This system has 16 DIMM sockets.
[    7.903274] EDAC DEBUG: ghes_edac_dmidecode: DIMM8: Unbuffered DDR3 RAM size = 2048 MB(ECC)
[    7.920337] EDAC DEBUG: ghes_edac_dmidecode:         type 24, detail 0x80, width 72(total 64)
[    7.936532] EDAC DEBUG: ghes_edac_dmidecode: DIMM9: Unbuffered DDR3 RAM size = 2048 MB(ECC)
[    7.953591] EDAC DEBUG: ghes_edac_dmidecode:         type 24, detail 0x80, width 72(total 64)
[    7.969778] EDAC DEBUG: ghes_edac_dmidecode: DIMM10: Unbuffered DDR3 RAM size = 2048 MB(ECC)
[    7.987010] EDAC DEBUG: ghes_edac_dmidecode:         type 24, detail 0x80, width 72(total 64)
[    8.003199] EDAC DEBUG: ghes_edac_dmidecode: DIMM11: Unbuffered DDR3 RAM size = 2048 MB(ECC)
[    8.020432] EDAC DEBUG: ghes_edac_dmidecode:         type 24, detail 0x80, width 72(total 64)
[    8.036618] EDAC DEBUG: ghes_edac_dmidecode: DIMM12: Unbuffered DDR3 RAM size = 2048 MB(ECC)
[    8.053848] EDAC DEBUG: ghes_edac_dmidecode:         type 24, detail 0x80, width 72(total 64)
[    8.070038] EDAC DEBUG: ghes_edac_dmidecode: DIMM13: Unbuffered DDR3 RAM size = 2048 MB(ECC)
[    8.087268] EDAC DEBUG: ghes_edac_dmidecode:         type 24, detail 0x80, width 72(total 64)
[    8.103456] EDAC DEBUG: ghes_edac_dmidecode: DIMM14: Unbuffered DDR3 RAM size = 2048 MB(ECC)
[    8.120687] EDAC DEBUG: ghes_edac_dmidecode:         type 24, detail 0x80, width 72(total 64)
[    8.128053] tsc: Refined TSC clocksource calibration: 2099.999 MHz
[    8.128173] clocksource: tsc: mask: 0xffffffffffffffff max_cycles: 0x1e452fc488e, max_idle_ns: 44
0795307124 ns
[    8.169800] EDAC DEBUG: ghes_edac_dmidecode: DIMM15: Unbuffered DDR3 RAM size = 2048 MB(ECC)
[    8.187043] EDAC DEBUG: ghes_edac_dmidecode:         type 24, detail 0x80, width 72(total 64)
[    8.203230] EDAC DEBUG: edac_mc_add_mc_with_groups: 
[    8.213363] EDAC DEBUG: edac_create_sysfs_mci_device: creating bus mc1
[    8.226643] EDAC DEBUG: edac_create_sysfs_mci_device: creating device mc1
[    8.240450] EDAC DEBUG: edac_create_sysfs_mci_device: creating dimm8, located at memory 8 
[    8.257362] EDAC DEBUG: edac_create_dimm_object: creating rank/dimm device dimm8
[    8.272506] EDAC DEBUG: edac_create_sysfs_mci_device: creating dimm9, located at memory 9 
[    8.289409] EDAC DEBUG: edac_create_dimm_object: creating rank/dimm device dimm9
[    8.304556] EDAC DEBUG: edac_create_sysfs_mci_device: creating dimm10, located at memory 10 
[    8.321808] EDAC DEBUG: edac_create_dimm_object: creating rank/dimm device dimm10
[    8.337126] EDAC DEBUG: edac_create_sysfs_mci_device: creating dimm11, located at memory 11 
[    8.354377] EDAC DEBUG: edac_create_dimm_object: creating rank/dimm device dimm11
[    8.369697] EDAC DEBUG: edac_create_sysfs_mci_device: creating dimm12, located at memory 12 
[    8.386946] EDAC DEBUG: edac_create_dimm_object: creating rank/dimm device dimm12
[    8.402266] EDAC DEBUG: edac_create_sysfs_mci_device: creating dimm13, located at memory 13 
[    8.419516] EDAC DEBUG: edac_create_dimm_object: creating rank/dimm device dimm13
[    8.434835] EDAC DEBUG: edac_create_sysfs_mci_device: creating dimm14, located at memory 14 
[    8.452094] EDAC DEBUG: edac_create_dimm_object: creating rank/dimm device dimm14
[    8.467423] EDAC DEBUG: edac_create_sysfs_mci_device: creating dimm15, located at memory 15 
[    8.484674] EDAC DEBUG: edac_create_dimm_object: creating rank/dimm device dimm15
[    8.499994] EDAC DEBUG: edac_create_csrow_object: creating (virtual) csrow node csrow8
[    8.516215] EDAC DEBUG: edac_create_csrow_object: creating (virtual) csrow node csrow9
[    8.532433] EDAC DEBUG: edac_create_csrow_object: creating (virtual) csrow node csrow10
[    8.548824] EDAC DEBUG: edac_create_csrow_object: creating (virtual) csrow node csrow11
[    8.565203] EDAC DEBUG: edac_create_csrow_object: creating (virtual) csrow node csrow12
[    8.581585] EDAC DEBUG: edac_create_csrow_object: creating (virtual) csrow node csrow13
[    8.597964] EDAC DEBUG: edac_create_csrow_object: creating (virtual) csrow node csrow14
[    8.614347] EDAC DEBUG: edac_create_csrow_object: creating (virtual) csrow node csrow15
[    8.630744] EDAC MC1: Giving out device to module ghes_edac.c controller ghes_edac: DEV ghes (INTERRUPT)
[    8.650073] [Firmware Warn]: GHES: Poll interval is 0 for generic hardware error source: 1, disabled.
[    8.669012] GHES: APEI firmware first mode is enabled by WHEA _OSC.
[    8.681940] Serial: 8250/16550 driver, 32 ports, IRQ sharing disabled
Steven Rostedt Aug. 16, 2017, 1:59 p.m. UTC | #2
On Wed, 16 Aug 2017 10:29:31 +0200
Borislav Petkov <bp@alien8.de> wrote:


> ---
> diff --git a/drivers/edac/ghes_edac.c b/drivers/edac/ghes_edac.c
> index 6f80eb65c26c..a22fabef4791 100644
> --- a/drivers/edac/ghes_edac.c
> +++ b/drivers/edac/ghes_edac.c
> @@ -28,10 +28,14 @@ struct ghes_edac_pvt {
>  	char msg[80];
>  };
>  
> -static LIST_HEAD(ghes_reglist);
> -static DEFINE_MUTEX(ghes_edac_lock);
> -static int ghes_edac_mc_num;
> +static struct ghes_edac_pvt *ghes_pvt;
>  
> +/*
> + * Sync with other, potentially concurrent callers of
> + * ghes_edac_report_mem_error(). We don't know what the
> + * "inventive" firmware would do.
> + */
> +static DEFINE_SPINLOCK(ghes_lock);
>  
>  /* Memory Device - Type 17 of SMBIOS spec */
>  struct memdev_dmi_entry {
> @@ -169,14 +173,11 @@ void ghes_edac_report_mem_error(struct ghes *ghes, int sev,
>  	enum hw_event_mc_err_type type;
>  	struct edac_raw_error_desc *e;
>  	struct mem_ctl_info *mci;
> -	struct ghes_edac_pvt *pvt = NULL;
> +	struct ghes_edac_pvt *pvt = ghes_pvt;
> +	unsigned long flags;
>  	char *p;
>  	u8 grain_bits;
>  
> -	list_for_each_entry(pvt, &ghes_reglist, list) {
> -		if (ghes == pvt->ghes)
> -			break;
> -	}
>  	if (!pvt) {
>  		pr_err("Internal error: Can't find EDAC structure\n");
>  		return;
> @@ -398,8 +399,16 @@ void ghes_edac_report_mem_error(struct ghes *ghes, int sev,
>  		       (e->page_frame_number << PAGE_SHIFT) | e->offset_in_page,
>  		       grain_bits, e->syndrome, pvt->detail_location);
>  
> -	/* Report the error via EDAC API */
> +	/*
> +	 * We can do the locking below because GHES defers error processing
> +	 * from NMI to IRQ context. Whenever that changes, we'd at least
> +	 * know.
> +	 */
> +	WARN_ON_ONCE(in_nmi());

Should the above be:

	if (WARN_ON_ONCE(in_nmi()))
		return;

To prevent a deadlock? Or do we not care?

> +
> +	spin_lock_irqsave(&ghes_lock, flags);
>  	edac_raw_mc_handle_error(type, mci, e);
> +	spin_unlock_irqrestore(&ghes_lock, flags);

The above looks fine, as long as there's nothing before it that needs
synchronization.

>  }
>  EXPORT_SYMBOL_GPL(ghes_edac_report_mem_error);
>  
> @@ -409,9 +418,14 @@ int ghes_edac_register(struct ghes *ghes, struct device *dev)
>  	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;
>  
> +	/*
> +	 * We have only one logical memory controller to which all DIMMs belong.
> +	 */
> +	if (ghes_pvt)
> +		return 0;

What's the likelihood of two calls to ghes_edac_register being done
simultaneously?  Because two calls at the same time will get past this.

-- Steve


> +
>  	/* Get the number of DIMMs */
>  	dmi_walk(ghes_edac_count_dimms, &num_dimm);
>  
> @@ -425,26 +439,17 @@ int ghes_edac_register(struct ghes *ghes, struct device *dev)
>  	layers[0].size = num_dimm;
>  	layers[0].is_virt_csrow = true;
>  
> -	/*
> -	 * We need to serialize edac_mc_alloc() and edac_mc_add_mc(),
> -	 * to avoid duplicated memory controller numbers
> -	 */
> -	mutex_lock(&ghes_edac_lock);
> -	mci = edac_mc_alloc(ghes_edac_mc_num, ARRAY_SIZE(layers), layers,
> -			    sizeof(*pvt));
> +	mci = edac_mc_alloc(1, ARRAY_SIZE(layers), layers, sizeof(struct ghes_edac_pvt));
>  	if (!mci) {
>  		pr_info("Can't allocate memory for EDAC data\n");
> -		mutex_unlock(&ghes_edac_lock);
>  		return -ENOMEM;
>  	}
>  
> -	pvt = mci->pvt_info;
> -	memset(pvt, 0, sizeof(*pvt));
> -	list_add_tail(&pvt->list, &ghes_reglist);
> -	pvt->ghes = ghes;
> -	pvt->mci  = mci;
> -	mci->pdev = dev;
> +	ghes_pvt	= mci->pvt_info;
> +	ghes_pvt->ghes	= ghes;
> +	ghes_pvt->mci	= mci;
>  
> +	mci->pdev = dev;
>  	mci->mtype_cap = MEM_FLAG_EMPTY;
>  	mci->edac_ctl_cap = EDAC_FLAG_NONE;
>  	mci->edac_cap = EDAC_FLAG_NONE;
> @@ -452,36 +457,23 @@ int ghes_edac_register(struct ghes *ghes, struct device *dev)
>  	mci->ctl_name = "ghes_edac";
>  	mci->dev_name = "ghes";
>  
> -	if (!ghes_edac_mc_num) {
> -		if (!fake) {
> -			pr_info("This EDAC driver relies on BIOS to enumerate memory and get error reports.\n");
> -			pr_info("Unfortunately, not all BIOSes reflect the memory layout correctly.\n");
> -			pr_info("So, the end result of using this driver varies from vendor to vendor.\n");
> -			pr_info("If you find incorrect reports, please contact your hardware vendor\n");
> -			pr_info("to correct its BIOS.\n");
> -			pr_info("This system has %d DIMM sockets.\n",
> -				num_dimm);
> -		} else {
> -			pr_info("This system has a very crappy BIOS: It doesn't even list the DIMMS.\n");
> -			pr_info("Its SMBIOS info is wrong. It is doubtful that the error report would\n");
> -			pr_info("work on such system. Use this driver with caution\n");
> -		}
> +	if (!fake) {
> +		pr_info("This EDAC driver relies on BIOS to enumerate memory and get error reports.\n");
> +		pr_info("Unfortunately, not all BIOSes reflect the memory layout correctly.\n");
> +		pr_info("So, the end result of using this driver varies from vendor to vendor.\n");
> +		pr_info("If you find incorrect reports, please contact your hardware vendor\n");
> +		pr_info("to correct its BIOS.\n");
> +		pr_info("This system has %d DIMM sockets.\n", num_dimm);
> +	} else {
> +		pr_info("This system has a very crappy BIOS: It doesn't even list the DIMMS.\n");
> +		pr_info("Its SMBIOS info is wrong. It is doubtful that the error report would\n");
> +		pr_info("work on such system. Use this driver with caution\n");
>  	}
>  
>  	if (!fake) {
> -		/*
> -		 * Fill DIMM info from DMI for the memory controller #0
> -		 *
> -		 * Keep it in blank for the other memory controllers, as
> -		 * there's no reliable way to properly credit each DIMM to
> -		 * the memory controller, as different BIOSes fill the
> -		 * DMI bank location fields on different ways
> -		 */
> -		if (!ghes_edac_mc_num) {
> -			dimm_fill.count = 0;
> -			dimm_fill.mci = mci;
> -			dmi_walk(ghes_edac_dmidecode, &dimm_fill);
> -		}
> +		dimm_fill.count = 0;
> +		dimm_fill.mci = mci;
> +		dmi_walk(ghes_edac_dmidecode, &dimm_fill);
>  	} else {
>  		struct dimm_info *dimm = EDAC_DIMM_PTR(mci->layers, mci->dimms,
>  						       mci->n_layers, 0, 0, 0);
> @@ -497,12 +489,8 @@ int ghes_edac_register(struct ghes *ghes, struct device *dev)
>  	if (rc < 0) {
>  		pr_info("Can't register at EDAC core\n");
>  		edac_mc_free(mci);
> -		mutex_unlock(&ghes_edac_lock);
>  		return -ENODEV;
>  	}
> -
> -	ghes_edac_mc_num++;
> -	mutex_unlock(&ghes_edac_lock);
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(ghes_edac_register);
> @@ -510,15 +498,9 @@ EXPORT_SYMBOL_GPL(ghes_edac_register);
>  void ghes_edac_unregister(struct ghes *ghes)
>  {
>  	struct mem_ctl_info *mci;
> -	struct ghes_edac_pvt *pvt, *tmp;
> -
> -	list_for_each_entry_safe(pvt, tmp, &ghes_reglist, list) {
> -		if (ghes == pvt->ghes) {
> -			mci = pvt->mci;
> -			edac_mc_del_mc(mci->pdev);
> -			edac_mc_free(mci);
> -			list_del(&pvt->list);
> -		}
> -	}
> +
> +	mci = ghes_pvt->mci;
> +	edac_mc_del_mc(mci->pdev);
> +	edac_mc_free(mci);
>  }
>  EXPORT_SYMBOL_GPL(ghes_edac_unregister);
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Borislav Petkov Aug. 16, 2017, 2:03 p.m. UTC | #3
On Wed, Aug 16, 2017 at 09:59:01AM -0400, Steven Rostedt wrote:
> Should the above be:
> 
> 	if (WARN_ON_ONCE(in_nmi()))
> 		return;
> 
> To prevent a deadlock? Or do we not care?

Yeah, better this way.

> What's the likelihood of two calls to ghes_edac_register being done
> simultaneously?  Because two calls at the same time will get past this.

Well, that thing gets called per GHES platform device and last time I
checked they do get probed back-to-back but I'll check that again.

Thanks.
Steven Rostedt Aug. 16, 2017, 2:22 p.m. UTC | #4
On Wed, 16 Aug 2017 16:03:50 +0200
Borislav Petkov <bp@alien8.de> wrote:

> > What's the likelihood of two calls to ghes_edac_register being done
> > simultaneously?  Because two calls at the same time will get past this.  
> 
> Well, that thing gets called per GHES platform device and last time I
> checked they do get probed back-to-back but I'll check that again.

Maybe keep that original mutex just in case.

-- Steve

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Kani, Toshi Aug. 16, 2017, 3:26 p.m. UTC | #5
On Wed, 2017-08-16 at 10:29 +0200, Borislav Petkov wrote:
> On Tue, Aug 15, 2017 at 08:48:16AM -0700, Luck, Tony wrote:

> > Won't the user see all their DIMMs reported for each memory

> > controller

> > under /sys/devices/system/edac/mc/mc*/dimm* ?

> > 

> > That sounds confusing.

> 

> Right, and adding the locking was really easy. If only people would

> debate less and actually try to do what they're being advised to.

> But not really: if you wanna have something done, you have to do it

> yourself.


Sorry, but I did not agree on allowing concurrent accesses to mci...

 /* Memory Device - Type 17 of SMBIOS spec */
>  struct memdev_dmi_entry {

> @@ -169,14 +173,11 @@ void ghes_edac_report_mem_error(struct ghes

> *ghes, int sev,

>  	enum hw_event_mc_err_type type;

>  	struct edac_raw_error_desc *e;

>  	struct mem_ctl_info *mci;

> -	struct ghes_edac_pvt *pvt = NULL;

> +	struct ghes_edac_pvt *pvt = ghes_pvt;

> +	unsigned long flags;

>  	char *p;

>  	u8 grain_bits;


I believe you now need to protect from a race condition that a single
mci and pvt can be initialized / consumed from multiple threads.  This
protection is missing in your patch.

Thanks,
-Toshi
diff mbox

Patch

diff --git a/drivers/edac/ghes_edac.c b/drivers/edac/ghes_edac.c
index 6f80eb65c26c..a22fabef4791 100644
--- a/drivers/edac/ghes_edac.c
+++ b/drivers/edac/ghes_edac.c
@@ -28,10 +28,14 @@  struct ghes_edac_pvt {
 	char msg[80];
 };
 
-static LIST_HEAD(ghes_reglist);
-static DEFINE_MUTEX(ghes_edac_lock);
-static int ghes_edac_mc_num;
+static struct ghes_edac_pvt *ghes_pvt;
 
+/*
+ * Sync with other, potentially concurrent callers of
+ * ghes_edac_report_mem_error(). We don't know what the
+ * "inventive" firmware would do.
+ */
+static DEFINE_SPINLOCK(ghes_lock);
 
 /* Memory Device - Type 17 of SMBIOS spec */
 struct memdev_dmi_entry {
@@ -169,14 +173,11 @@  void ghes_edac_report_mem_error(struct ghes *ghes, int sev,
 	enum hw_event_mc_err_type type;
 	struct edac_raw_error_desc *e;
 	struct mem_ctl_info *mci;
-	struct ghes_edac_pvt *pvt = NULL;
+	struct ghes_edac_pvt *pvt = ghes_pvt;
+	unsigned long flags;
 	char *p;
 	u8 grain_bits;
 
-	list_for_each_entry(pvt, &ghes_reglist, list) {
-		if (ghes == pvt->ghes)
-			break;
-	}
 	if (!pvt) {
 		pr_err("Internal error: Can't find EDAC structure\n");
 		return;
@@ -398,8 +399,16 @@  void ghes_edac_report_mem_error(struct ghes *ghes, int sev,
 		       (e->page_frame_number << PAGE_SHIFT) | e->offset_in_page,
 		       grain_bits, e->syndrome, pvt->detail_location);
 
-	/* Report the error via EDAC API */
+	/*
+	 * We can do the locking below because GHES defers error processing
+	 * from NMI to IRQ context. Whenever that changes, we'd at least
+	 * know.
+	 */
+	WARN_ON_ONCE(in_nmi());
+
+	spin_lock_irqsave(&ghes_lock, flags);
 	edac_raw_mc_handle_error(type, mci, e);
+	spin_unlock_irqrestore(&ghes_lock, flags);
 }
 EXPORT_SYMBOL_GPL(ghes_edac_report_mem_error);
 
@@ -409,9 +418,14 @@  int ghes_edac_register(struct ghes *ghes, struct device *dev)
 	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;
 
+	/*
+	 * We have only one logical memory controller to which all DIMMs belong.
+	 */
+	if (ghes_pvt)
+		return 0;
+
 	/* Get the number of DIMMs */
 	dmi_walk(ghes_edac_count_dimms, &num_dimm);
 
@@ -425,26 +439,17 @@  int ghes_edac_register(struct ghes *ghes, struct device *dev)
 	layers[0].size = num_dimm;
 	layers[0].is_virt_csrow = true;
 
-	/*
-	 * We need to serialize edac_mc_alloc() and edac_mc_add_mc(),
-	 * to avoid duplicated memory controller numbers
-	 */
-	mutex_lock(&ghes_edac_lock);
-	mci = edac_mc_alloc(ghes_edac_mc_num, ARRAY_SIZE(layers), layers,
-			    sizeof(*pvt));
+	mci = edac_mc_alloc(1, ARRAY_SIZE(layers), layers, sizeof(struct ghes_edac_pvt));
 	if (!mci) {
 		pr_info("Can't allocate memory for EDAC data\n");
-		mutex_unlock(&ghes_edac_lock);
 		return -ENOMEM;
 	}
 
-	pvt = mci->pvt_info;
-	memset(pvt, 0, sizeof(*pvt));
-	list_add_tail(&pvt->list, &ghes_reglist);
-	pvt->ghes = ghes;
-	pvt->mci  = mci;
-	mci->pdev = dev;
+	ghes_pvt	= mci->pvt_info;
+	ghes_pvt->ghes	= ghes;
+	ghes_pvt->mci	= mci;
 
+	mci->pdev = dev;
 	mci->mtype_cap = MEM_FLAG_EMPTY;
 	mci->edac_ctl_cap = EDAC_FLAG_NONE;
 	mci->edac_cap = EDAC_FLAG_NONE;
@@ -452,36 +457,23 @@  int ghes_edac_register(struct ghes *ghes, struct device *dev)
 	mci->ctl_name = "ghes_edac";
 	mci->dev_name = "ghes";
 
-	if (!ghes_edac_mc_num) {
-		if (!fake) {
-			pr_info("This EDAC driver relies on BIOS to enumerate memory and get error reports.\n");
-			pr_info("Unfortunately, not all BIOSes reflect the memory layout correctly.\n");
-			pr_info("So, the end result of using this driver varies from vendor to vendor.\n");
-			pr_info("If you find incorrect reports, please contact your hardware vendor\n");
-			pr_info("to correct its BIOS.\n");
-			pr_info("This system has %d DIMM sockets.\n",
-				num_dimm);
-		} else {
-			pr_info("This system has a very crappy BIOS: It doesn't even list the DIMMS.\n");
-			pr_info("Its SMBIOS info is wrong. It is doubtful that the error report would\n");
-			pr_info("work on such system. Use this driver with caution\n");
-		}
+	if (!fake) {
+		pr_info("This EDAC driver relies on BIOS to enumerate memory and get error reports.\n");
+		pr_info("Unfortunately, not all BIOSes reflect the memory layout correctly.\n");
+		pr_info("So, the end result of using this driver varies from vendor to vendor.\n");
+		pr_info("If you find incorrect reports, please contact your hardware vendor\n");
+		pr_info("to correct its BIOS.\n");
+		pr_info("This system has %d DIMM sockets.\n", num_dimm);
+	} else {
+		pr_info("This system has a very crappy BIOS: It doesn't even list the DIMMS.\n");
+		pr_info("Its SMBIOS info is wrong. It is doubtful that the error report would\n");
+		pr_info("work on such system. Use this driver with caution\n");
 	}
 
 	if (!fake) {
-		/*
-		 * Fill DIMM info from DMI for the memory controller #0
-		 *
-		 * Keep it in blank for the other memory controllers, as
-		 * there's no reliable way to properly credit each DIMM to
-		 * the memory controller, as different BIOSes fill the
-		 * DMI bank location fields on different ways
-		 */
-		if (!ghes_edac_mc_num) {
-			dimm_fill.count = 0;
-			dimm_fill.mci = mci;
-			dmi_walk(ghes_edac_dmidecode, &dimm_fill);
-		}
+		dimm_fill.count = 0;
+		dimm_fill.mci = mci;
+		dmi_walk(ghes_edac_dmidecode, &dimm_fill);
 	} else {
 		struct dimm_info *dimm = EDAC_DIMM_PTR(mci->layers, mci->dimms,
 						       mci->n_layers, 0, 0, 0);
@@ -497,12 +489,8 @@  int ghes_edac_register(struct ghes *ghes, struct device *dev)
 	if (rc < 0) {
 		pr_info("Can't register at EDAC core\n");
 		edac_mc_free(mci);
-		mutex_unlock(&ghes_edac_lock);
 		return -ENODEV;
 	}
-
-	ghes_edac_mc_num++;
-	mutex_unlock(&ghes_edac_lock);
 	return 0;
 }
 EXPORT_SYMBOL_GPL(ghes_edac_register);
@@ -510,15 +498,9 @@  EXPORT_SYMBOL_GPL(ghes_edac_register);
 void ghes_edac_unregister(struct ghes *ghes)
 {
 	struct mem_ctl_info *mci;
-	struct ghes_edac_pvt *pvt, *tmp;
-
-	list_for_each_entry_safe(pvt, tmp, &ghes_reglist, list) {
-		if (ghes == pvt->ghes) {
-			mci = pvt->mci;
-			edac_mc_del_mc(mci->pdev);
-			edac_mc_free(mci);
-			list_del(&pvt->list);
-		}
-	}
+
+	mci = ghes_pvt->mci;
+	edac_mc_del_mc(mci->pdev);
+	edac_mc_free(mci);
 }
 EXPORT_SYMBOL_GPL(ghes_edac_unregister);