[PATCH] EDAC, ghes: use CPER module handles to locate DIMMs

Hi James,

quoted

The current ghes_edac driver does not update per-dimm error counters
when reporting memory errors, because there is no platform-independent
way to find DIMMs based on the error information provided by firmware.

I'd argue there is: its in the CPER records, we just didn't do anything useful
with the information in the past!

Agreed. Will update the wording. 
 

quoted

+static int ghes_edac_dimm_index(u16 handle) {
+	struct mem_ctl_info *mci;
+	int i;
+
+	if (!ghes_pvt)
+		return -1;

ghes_edac_report_mem_error() already checked this, as its the only caller
there is no need to check it again.

Will remove.
 

quoted

+	mci = ghes_pvt->mci;
+
+	if (!mci)
+		return -1;

Can this happen? ghes_edac_report_mem_error() would have
dereferenced this already!

If you need the struct mem_ctl_info, you may as well pass it in as the only
caller has it to hand.

Will remove.

quoted

+
+	for (i = 0; i < mci->tot_dimms; i++) {
+		if (mci->dimms[i]->smbios_handle == handle)
+			return i;
+	}
+	return -1;
+}
+
 static void ghes_edac_dmidecode(const struct dmi_header *dh, void
*arg)  {
 	struct ghes_edac_dimm_fill *dimm_fill = arg; @@ -177,6 +197,8 @@
static void ghes_edac_dmidecode(const struct dmi_header *dh, void *arg)
 				entry->total_width, entry->data_width);
 		}

+		dimm->smbios_handle = entry->handle;

We aren't checking for duplicate handles, (e.g. they're all zero). I think this is
fine as chances are firmware on those systems won't set
CPER_MEM_VALID_MODULE_HANDLE. If it does, the handle it gives us is
ambiguous, and we pick a dimm, instead of whine-ing about broken
firmware tables.

(I'm just drawing attention to it in case someone disagrees)

SBMIOS tables are typically automatically generated so chances for duplicate
handles are small. 

quoted

 		dimm_fill->count++;
 	}
 }

@@ -327,12 +349,20 @@ void ghes_edac_report_mem_error(int sev,

struct cper_sec_mem_err *mem_err)

quoted

 		p += sprintf(p, "bit_pos:%d ", mem_err->bit_pos);
 	if (mem_err->validation_bits &

CPER_MEM_VALID_MODULE_HANDLE) {

quoted

 		const char *bank = NULL, *device = NULL;
+		int index = -1;
+
 		dmi_memdev_name(mem_err->mem_dev_handle, &bank,

&device);

quoted

+		p += sprintf(p, "DIMM DMI handle: 0x%.4x ",
+			     mem_err->mem_dev_handle);
 		if (bank != NULL && device != NULL)
 			p += sprintf(p, "DIMM location:%s %s ", bank, device);
-		else
-			p += sprintf(p, "DIMM DMI handle: 0x%.4x ",
-				     mem_err->mem_dev_handle);

Why do we now print the handle every time? The handle is pretty
meaningless, it can only be used to find the location-strings, if we get those
we print them instead.

For ghes_edac the bank/device is informational, and nothing would go wrong
if the bank/device numbers are the same as another entry. But the handle
is now critical for DIMM lookup, thus pull it out.

Thanks!
Fan