Teach EDAC driver about NVDIMMs

When we first scan the SMBIOS table, save the size of the DIMM.

Provide a function for other code (EDAC driver) to look up the size of a DIMM from its SMBIOS handle.

Signed-off-by Tony Luck <[email protected]>

---
 drivers/firmware/dmi_scan.c | 29 +++++++++++++++++++++++++++++
 include/linux/dmi.h         |  2 ++
 2 files changed, 31 insertions(+)

drivers/firmware/dmi_scan.c +3 -0

index 783041964439..946e86fb1ec6 100644
@@ -37,6 +37,7 @@ static char dmi_ids_string[128] __initdata;
 static struct dmi_memdev_info {
 	const char *device;
 	const char *bank;
+	u64 size;
 	u16 handle;
 } *dmi_memdev;
 static int dmi_memdev_nr;

@@ -395,6 +396,8 @@ static void __init save_mem_devices(const struct dmi_header *dm, void *v)
 {
 	const char *d = (const char *)dm;
 	static int nr;
+	u64 bytes;
+	u16 size;
 
 	if (dm->type != DMI_ENTRY_MEM_DEVICE || dm->length < 0x12)
 		return;

drivers/firmware/dmi_scan.c +4 -0

+	dmi_memdev[nr].size = bytes;
 	nr++;
 }
 

@@ -1073,3 +1088,17 @@ void dmi_memdev_name(u16 handle, const char **bank, const char **device)
 	}
 }
 EXPORT_SYMBOL_GPL(dmi_memdev_name);
+
+u64 dmi_memdev_size(u16 handle)
+{

Quoted 1 line · Tony Luck

dmi_memdev[nr].bank = dmi_string(dm, d[0x11]);

<---- newline here.

drivers/firmware/dmi_scan.c static void __init save_mem_devices(const struct dmi_header *dm, void *v) · line 421

Quoted 11 lines

drivers/firmware/dmi_scan.c +11 -0

+	size = get_unaligned((u16 *)&d[0xC]);
+	if (size == 0)
+		bytes = 0;
+	else if (size == 0xffff)
+		bytes = ~0ul;
+	else if (size & 0x8000)
+		bytes = (u64)(size & 0x7fff) << 10;
+	else if (size != 0x7fff)
+		bytes = (u64)size << 20;
+	else
+		bytes = (u64)get_unaligned((u32 *)&d[0x1C]) << 20;

<---- newline here.

So dmi_memdev_name() also loops over dmi_memdev and returns bank and device. This new function returns size.

If code is going to be calling those one after the other, you could do a

dmi_memdev_desc(u16 handle, struct memdev_desc *desc)

which fills up a descriptor with all fields a caller would need in one go so that you don't have to iterate multiple times. Looking at struct dmi_memdev_info, there are no more fields so maybe this is probably silly though or you can simply return struct dmi_memdev_info directly...

Meh.

I take it that you talked yourself out of asking for the cleanup now?

There are now non-volatile versions of DIMMs. Add a new entry to "enum mem_type" and update places that use it with new strings.

Signed-off-by Tony Luck <[email protected]>

---
 drivers/edac/edac_mc.c       | 1 +
 drivers/edac/edac_mc_sysfs.c | 3 ++-
 include/linux/edac.h         | 3 +++
 3 files changed, 6 insertions(+), 1 deletion(-)

drivers/edac/edac_mc.c +1 -0

index 480072139b7a..8178e74decbf 100644
@@ -215,6 +215,7 @@ const char * const edac_mem_types[] = {
 	[MEM_LRDDR3]	= "Load-Reduced DDR3 RAM",
 	[MEM_DDR4]	= "Unbuffered DDR4 RAM",
 	[MEM_RDDR4]	= "Registered DDR4 RAM",
+	[MEM_NVDIMM]	= "Non-volatile RAM",
 };
 EXPORT_SYMBOL_GPL(edac_mem_types);
 

drivers/edac/edac_mc_sysfs.c +0 -0

index e4fcfa84fbd3..53cbb3518efc 100644

On Thu, Nov 30, 2017 at 12:40:41PM -0800, Tony Luck wrote:
WTF, there are *two* string arrays with memory types?! How did that happen?

Can you please remove that above in a prepatch and switch to edac_mem_types? Or should we switch to this mem_types array instead and kill edac_mem_types since former is visible in sysfs?

:-( Grrr.

This just covers the topology function of the EDAC driver. We locate which DIMM slots are populated with NVDIMMs and query the NFIT and SMBIOS tables to get the size.

Signed-off-by Tony Luck <[email protected]>

---
 drivers/edac/Kconfig    |  2 ++
 drivers/edac/skx_edac.c | 56 ++++++++++++++++++++++++++++++++++++++++++++-----
 2 files changed, 53 insertions(+), 5 deletions(-)

drivers/edac/Kconfig +0 -0

index 96afb2aeed18..5c0c4a358f67 100644

drivers/edac/Kconfig +0 -0

 	help
 	  Support for error detection and correction the Intel
 	  Skylake server Integrated Memory Controllers.

drivers/edac/skx_edac.c +10 -2

index 16dea97568a1..814a5245029c 100644
@@ -14,6 +14,8 @@
 
 #include <linux/module.h>
 #include <linux/init.h>
+#include <linux/acpi.h>
+#include <linux/dmi.h>
 #include <linux/pci.h>
 #include <linux/pci_ids.h>
 #include <linux/slab.h>

@@ -24,6 +26,7 @@
 #include <linux/bitmap.h>
 #include <linux/math64.h>
 #include <linux/mod_devicetable.h>
+#include <acpi/nfit.h>
 #include <asm/cpu_device_id.h>
 #include <asm/intel-family.h>
 #include <asm/processor.h>

@@ -298,6 +301,7 @@ static int get_dimm_attr(u32 reg, int lobit, int hibit, int add, int minval,
 }
 
 #define IS_DIMM_PRESENT(mtr)		GET_BITFIELD((mtr), 15, 15)
+#define IS_NVDIMM_PRESENT(mcddrtcfg, i)	GET_BITFIELD((mcddrtcfg), (i), (i))
 
 #define numrank(reg) get_dimm_attr((reg), 12, 13, 0, 1, 2, "ranks")
 #define numrow(reg) get_dimm_attr((reg), 2, 4, 12, 1, 6, "rows")

@@ -346,8 +350,6 @@ static int get_dimm_info(u32 mtr, u32 amap, struct dimm_info *dimm,
 	int  banks = 16, ranks, rows, cols, npages;
 	u64 size;
 
-	if (!IS_DIMM_PRESENT(mtr))
-		return 0;
 	ranks = numrank(mtr);
 	rows = numrow(mtr);
 	cols = numcol(mtr);

@@ -379,6 +381,46 @@ static int get_dimm_info(u32 mtr, u32 amap, struct dimm_info *dimm,
 	return 1;
 }
 
+static int get_nvdimm_info(struct dimm_info *dimm, struct skx_imc *imc,
+			   int chan, int dimmno)
+{
+	int smbios_handle;
+	u32 dev_handle;
+	u16 flags;

drivers/edac/skx_edac.c +6 -0

+	snprintf(dimm->label, sizeof(dimm->label), "CPU_SrcID#%u_MC#%u_Chan#%u_DIMM#%u",
+		 imc->src_id, imc->lmc, chan, dimmno);
+
+	return 1;
+}
+
 #define SKX_GET_MTMTR(dev, reg) \
 	pci_read_config_dword((dev), 0x87c, &reg)

drivers/edac/Kconfig config EDAC_SBRIDGE · line 236

Quoted 5 lines · Tony Luck

config EDAC_SKX

drivers/edac/Kconfig +2 -0

 	tristate "Intel Skylake server Integrated MC"
 	depends on PCI && X86_64 && X86_MCE_INTEL && PCI_MMCONFIG
+	select DMI
+	select ACPI_NFIT

Hmm, that ACPI_NFIT depends on a bunch of stuff and enabling EDAC_SKX would pull in all that. Should we make this a user choice instead?

I mean, there could be boxes which don't have nvdimms so all that code would be dead weight there...

<---- newline here.

drivers/edac/skx_edac.c static int get_dimm_info(u32 mtr, u32 amap, struct dimm_info *dimm, · line 409

Quoted 6 lines

drivers/edac/skx_edac.c +6 -0

+	size = dmi_memdev_size(smbios_handle);
+	if (size == ~0ul) {
+		skx_printk(KERN_ERR, "Can't find size for NVDIMM ADR=%x/SMBIOS=%x\n",
+			   dev_handle, smbios_handle);
+		return 0;
+	}

Ditto.

drivers/edac/skx_edac.c static int get_dimm_info(u32 mtr, u32 amap, struct dimm_info *dimm, · line 417

Quoted 8 lines

drivers/edac/skx_edac.c +8 -0

+	edac_dbg(0, "mc#%d: channel %d, dimm %d, %lld Mb (%lld pages)\n",
+		 imc->mc, chan, dimmno, size >> 20, size >> PAGE_SHIFT);
+
+	dimm->nr_pages = size >> PAGE_SHIFT;
+	dimm->grain = 32;
+	dimm->dtype = DEV_UNKNOWN;
+	dimm->mtype = MEM_NVDIMM;
+	dimm->edac_mode = EDAC_SECDED; /* likely better than this */

Ditto.

/me hands aegl a bunch of newlines - seems he's all out of \n's

I could. But what happens when someone ends up on a system with an edac driver configured without ACPI_NFIT that does have NVDIMMs?

I can make a stub version of nfit_get_smbios_id() that returns some error code ... and have the EDAC driver report size==0.

Would that be OK?

/me finds that the "Enter" key on his keyboard will put in a newline. Hurrah! now I don't have to copy/paste them from Boris.

Quoted 2 lines · Tony

I could. But what happens when someone ends up on a system with an edac driver configured without ACPI_NFIT that does have NVDIMMs?

Same thing when you land on a system with a kernel where the driver for a piece of hw is not enabled. I mean, this won't be an issue on distros as there *everything* is enabled but for tailored configs, where people want skx_edac but don't need the nvdimm part.

Quoted 4 lines

I can make a stub version of nfit_get_smbios_id() that returns some error code ... and have the EDAC driver report size==0.

Would that be OK?

Sure, thanks!

So this is what that would look like (on top of existing patches, but would be folded into them for next version):

drivers/edac/Kconfig +3 -2

index 5c0c4a358f67..7f0bc4cd5086 100644
@@ -233,10 +233,11 @@ config EDAC_SKX
 	tristate "Intel Skylake server Integrated MC"
 	depends on PCI && X86_64 && X86_MCE_INTEL && PCI_MMCONFIG
 	select DMI
-	select ACPI_NFIT
 	help
 	  Support for error detection and correction the Intel
-	  Skylake server Integrated Memory Controllers.
+	  Skylake server Integrated Memory Controllers. If your
+	  has non-volatile DIMMs you should also manually select
+	  CONFIG_ACPI_NFIT
 
 config EDAC_PND2
 	tristate "Intel Pondicherry2"

drivers/edac/skx_edac.c +0 -0

index f42e382f82b1..8374deb83246 100644
@@ -387,12 +387,16 @@ static int get_nvdimm_info(struct dimm_info *dimm, struct skx_imc *imc,
 	int smbios_handle;
 	u32 dev_handle;