Teach EDAC about non-volatile DIMMs and add partial support to skx_edac
Subsystems (6)
cover Teach EDAC about non-volatile DIMMs and add partial support to skx_edac
This series was posted as RFC and got some review back in December:
https://marc.info/?l=linux-edac&m=151257213825419&w=2
It couldn't go upstream back then because I was waiting for some updates to the ACPICA headers for NFIT support. Those patches are complete now, so here is the series again.
The "partial support" part is that the changes here only do the detection of the non-volatile DIMMs. Coming later will be another patch/series to teach skx_edac how to decode system addresses to NVDIMM addresses. But this part stands on its own and is a useful first step.
Tony Luck (5):
EDAC: Drop duplicated array of strings for memory type names
edac: Add new memory type for non-volatile DIMMs
acpi, nfit: Add function to look up nvdimm device and provide SMBIOS
handle
firmware: dmi: Add function to look up a handle and return DIMM size EDAC, skx_edac: Detect non-volatile DIMMs
drivers/acpi/nfit/core.c | 27 ++++++++++++++++++ drivers/edac/Kconfig | 5 +++- drivers/edac/edac_mc.c | 41 +++++++++++++------------- drivers/edac/edac_mc_sysfs.c | 26 ++--------------- drivers/edac/skx_edac.c | 68 ++++++++++++++++++++++++++++++++++++++++---- drivers/firmware/dmi_scan.c | 31 ++++++++++++++++++++ include/acpi/nfit.h | 26 +++++++++++++++++ include/linux/dmi.h | 2 ++ include/linux/edac.h | 3 ++ 9 files changed, 178 insertions(+), 51 deletions(-) create mode 100644 include/acpi/nfit.h
Signature · 6 lines
2.14.1 _______________________________________________ Linux-nvdimm mailing list [email protected] https://lists.01.org/mailman/listinfo/linux-nvdimm
A message that is not in the archive (sent off-list or not yet received): 20180222195811.27237-1-tony.luck-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org
PATCH 1/5 EDAC: Drop duplicated array of strings for memory type names
Somehow we ended up with two separate arrays of strings to describe the "enum mem_type" values.
In edac_mc.c we have an exported list edac_mem_types[] that is used by a couple of drivers in debug messaged.
In edac_mc_sysfs.c we have a private list that is used to display values in:
/sys/devices/system/edac/mc/mc*/dimm*/dimm_mem_type /sys/devices/system/edac/mc/mc*/csrow*/mem_type
this list was missing a value for MEM_LRDDR3
The string values in the two lists were different :-(
Combining the lists, I kept the values so that the sysfs output will be unchanged as some scripts may depend on that.
--- drivers/edac/edac_mc.c | 40 ++++++++++++++++++++-------------------- drivers/edac/edac_mc_sysfs.c | 26 ++------------------------ 2 files changed, 22 insertions(+), 44 deletions(-)
drivers/edac/edac_mc.c +20 -20
@@ -195,26 +195,26 @@ static void edac_mc_dump_mci(struct mem_ctl_info *mci)#endif /* CONFIG_EDAC_DEBUG */ const char *const edac_mem_types[] = { - [MEM_EMPTY ] ="Empty csrow" , - [MEM_RESERVED ] ="Reserved csrow type" , - [MEM_UNKNOWN ] ="Unknown csrow type" , - [MEM_FPM ] ="Fast page mode RAM" , - [MEM_EDO ] ="Extended data out RAM" , - [MEM_BEDO ] ="Burst Extended data out RAM" , - [MEM_SDR ] ="Single data rate SDRAM" , - [MEM_RDR ] ="Registered single data rate SDRAM" , - [MEM_DDR ] ="Double data rate SDRAM" , - [MEM_RDDR ] ="Registered Double data rate SDRAM" , - [MEM_RMBS ] ="Rambus DRAM" , - [MEM_DDR2 ] ="Unbuffered DDR2 RAM" , - [MEM_FB_DDR2 ] ="Fully buffered DDR2" , - [MEM_RDDR2 ] ="Registered DDR2 RAM" , - [MEM_XDR ] ="Rambus XDR" , - [MEM_DDR3 ] ="Unbuffered DDR3 RAM" , - [MEM_RDDR3 ] ="Registered DDR3 RAM" , - [MEM_LRDDR3 ] ="Load-Reduced DDR3 RAM" , - [MEM_DDR4 ] ="Unbuffered DDR4 RAM" , - [MEM_RDDR4 ] ="Registered DDR4 RAM" , + [MEM_EMPTY ] ="Empty" , + [MEM_RESERVED ] ="Reserved" , + [MEM_UNKNOWN ] ="Unknown" , + [MEM_FPM ] ="FPM" , + [MEM_EDO ] ="EDO" , + [MEM_BEDO ] ="BEDO" , + [MEM_SDR ] ="Unbuffered-SDR" , + [MEM_RDR ] ="Registered-SDR" , + [MEM_DDR ] ="Unbuffered-DDR" , + [MEM_RDDR ] ="Registered-DDR" , + [MEM_RMBS ] ="RMBS" , + [MEM_DDR2 ] ="Unbuffered-DDR2" , + [MEM_FB_DDR2 ] ="FullyBuffered-DDR2" , + [MEM_RDDR2 ] ="Registered-DDR2" , + [MEM_XDR ] ="XDR" , + [MEM_DDR3 ] ="Unbuffered-DDR3" , + [MEM_RDDR3 ] ="Registered-DDR3" , + [MEM_LRDDR3 ] ="Load-Reduced-DDR3-RAM" , + [MEM_DDR4 ] ="Unbuffered-DDR4" , + [MEM_RDDR4 ] ="Registered-DDR4" };EXPORT_SYMBOL_GPL (edac_mem_types);
drivers/edac/edac_mc_sysfs.c +2 -24
@@ -91,28 +91,6 @@ static struct device *mci_pdev;/* * various constants for Memory Controllers */ -static const char *const mem_types[] = { - [MEM_EMPTY ] ="Empty" , - [MEM_RESERVED ] ="Reserved" , - [MEM_UNKNOWN ] ="Unknown" , - [MEM_FPM ] ="FPM" , - [MEM_EDO ] ="EDO" , - [MEM_BEDO ] ="BEDO" , - [MEM_SDR ] ="Unbuffered-SDR" , - [MEM_RDR ] ="Registered-SDR" , - [MEM_DDR ] ="Unbuffered-DDR" , - [MEM_RDDR ] ="Registered-DDR" , - [MEM_RMBS ] ="RMBS" , - [MEM_DDR2 ] ="Unbuffered-DDR2" , - [MEM_FB_DDR2 ] ="FullyBuffered-DDR2" , - [MEM_RDDR2 ] ="Registered-DDR2" , - [MEM_XDR ] ="XDR" , - [MEM_DDR3 ] ="Unbuffered-DDR3" , - [MEM_RDDR3 ] ="Registered-DDR3" , - [MEM_DDR4 ] ="Unbuffered-DDR4" , - [MEM_RDDR4 ] ="Registered-DDR4" -}; -static const char *const dev_types[] = { [DEV_UNKNOWN ] ="Unknown" , [DEV_X1 ] ="x1" ,
@@ -196,7 +174,7 @@ static ssize_t csrow_mem_type_show(struct device *dev, {struct csrow_info *csrow =to_csrow (dev); -return sprintf (data,"%s\n" , mem_types[csrow->channels[0 ]->dimm->mtype]); +return sprintf (data,"%s\n" , edac_mem_types[csrow->channels[0 ]->dimm->mtype]); } static ssize_t csrow_dev_type_show(struct device *dev,
@@ -549,7 +527,7 @@ static ssize_t dimmdev_mem_type_show(struct device *dev, {struct dimm_info *dimm =to_dimm (dev); -return sprintf (data,"%s\n" , mem_types[dimm->mtype]); +return sprintf (data,"%s\n" , edac_mem_types[dimm->mtype]); } static ssize_t dimmdev_dev_type_show(struct device *dev,
Signature · 6 lines
2.14.1 _______________________________________________ Linux-nvdimm mailing list [email protected] https://lists.01.org/mailman/listinfo/linux-nvdimm
PATCH 2/5 edac: Add new memory type for non-volatile DIMMs
In reply to a message not in the archiveThere are now non-volatile versions of DIMMs. Add a new entry to "enum mem_type" and a new string in edac_mem_types[].
--- drivers/edac/edac_mc.c | 3 ++- include/linux/edac.h | 3 +++ 2 files changed, 5 insertions(+), 1 deletion(-)
drivers/edac/edac_mc.c +2 -1
@@ -214,7 +214,8 @@ const char * const edac_mem_types[] = { [MEM_RDDR3] = "Registered-DDR3", [MEM_LRDDR3] = "Load-Reduced-DDR3-RAM", [MEM_DDR4] = "Unbuffered-DDR4", - [MEM_RDDR4] = "Registered-DDR4" + [MEM_RDDR4] = "Registered-DDR4", + [MEM_NVDIMM ] ="Non-volatile-RAM" , };EXPORT_SYMBOL_GPL (edac_mem_types);
include/linux/edac.h +3 -0
@@ -186,6 +186,7 @@ static inline char *mc_event_error_type(const unsigned int err_type)* @MEM_RDDR4: Registered DDR4 RAM * This is a variant of the DDR4 memories. * @MEM_LRDDR4: Load-Reduced DDR4 memory. +* @MEM_NVDIMM: Non-volatile RAM */ enum mem_type {MEM_EMPTY =0 ,
@@ -209,6 +210,7 @@ enum mem_type {MEM_DDR4 ,MEM_RDDR4 ,MEM_LRDDR4 , + MEM_NVDIMM, };#define MEM_FLAG_EMPTY BIT(MEM_EMPTY)
@@ -231,6 +233,7 @@ enum mem_type {#define MEM_FLAG_DDR4 BIT(MEM_DDR4)#define MEM_FLAG_RDDR4 BIT(MEM_RDDR4)#define MEM_FLAG_LRDDR4 BIT(MEM_LRDDR4) +#define MEM_FLAG_NVDIMM BIT(MEM_NVDIMM)/** * enum edac-type - Error Detection and Correction capabilities and mode
Signature · 6 lines
2.14.1 _______________________________________________ Linux-nvdimm mailing list [email protected] https://lists.01.org/mailman/listinfo/linux-nvdimm
PATCH 3/5 acpi, nfit: Add function to look up nvdimm device and provide SMBIOS handle
In reply to a message not in the archiveEDAC driver needs to look up attributes of NVDIMMs provided in SMBIOS.
Provide a function that looks up an acpi_nfit_memory_map from a device handle (node/socket/mc/channel/dimm) and returns the SMBIOS handle. Also pass back the "flags" so we can see if the NVDIMM is OK.
--- drivers/acpi/nfit/core.c | 27 +++++++++++++++++++++++++++ include/acpi/nfit.h | 26 ++++++++++++++++++++++++++ 2 files changed, 53 insertions(+) create mode 100644 include/acpi/nfit.h
drivers/acpi/nfit/core.c +27 -0
@@ -23,6 +23,7 @@#include <linux/io.h> #include <linux/nd.h> #include <asm/cacheflush.h> +#include <acpi/nfit.h> #include "nfit.h" /*
@@ -690,6 +691,32 @@ static bool add_memdev(struct acpi_nfit_desc *acpi_desc,return true; } +int nfit_get_smbios_id (u32 device_handle,u16 *flags) +{ +struct acpi_nfit_memory_map *memdev; +struct acpi_nfit_desc *acpi_desc; +struct nfit_mem *nfit_mem; + +mutex_lock (&acpi_desc_lock); +list_for_each_entry (acpi_desc, &acpi_descs, list) { +mutex_lock (&acpi_desc->init_mutex); +list_for_each_entry (nfit_mem, &acpi_desc->dimms, list) { + memdev =__to_nfit_memdev (nfit_mem); +if (memdev->device_handle == device_handle) { +mutex_unlock (&acpi_desc->init_mutex); +mutex_unlock (&acpi_desc_lock); + *flags = memdev->flags; +return memdev->physical_id; + } + } +mutex_unlock (&acpi_desc->init_mutex); + } +mutex_unlock (&acpi_desc_lock); + +return -ENODEV ; +} +EXPORT_SYMBOL_GPL (nfit_get_smbios_id); +/* * An implementation may provide a truncated control region if no block windows * are defined.
include/acpi/nfit.h new file +26 -0
@@ -0,0 +1,26 @@ +/* +* Copyright(c) 2017 Intel Corporation. All rights reserved. +* +* This program is free software; you can redistribute it and/or modify +* it under the terms of version 2 of the GNU General Public License as +* published by the Free Software Foundation. +* +* This program is distributed in the hope that it will be useful, but +* WITHOUT ANY WARRANTY; without even the implied warranty of +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU +* General Public License for more details. +*/ + +#ifndef __ACPI_NFIT_H +#define __ACPI_NFIT_H + +#if IS_ENABLED (CONFIG_ACPI_NFIT ) +int nfit_get_smbios_id (u32 device_handle,u16 *flags); +#else +static inline int nfit_get_smbios_id (u32 device_handle,u16 *flags) +{ +return -EOPNOTSUPP ; +} +#endif + +#endif /* __ACPI_NFIT_H */
Signature · 6 lines
2.14.1 _______________________________________________ Linux-nvdimm mailing list [email protected] https://lists.01.org/mailman/listinfo/linux-nvdimm
A message that is not in the archive (sent off-list or not yet received): 20180222195811.27237-4-tony.luck-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org
Re: [PATCH 3/5] acpi, nfit: Add function to look up nvdimm device and provide SMBIOS handle
Quoted 8 lines · Tony Luck
EDAC driver needs to look up attributes of NVDIMMs provided in SMBIOS.
Provide a function that looks up an
acpi_nfit_memory_mapfrom a device handle (node/socket/mc/channel/dimm) and returns the SMBIOS handle. Also pass back the "flags" so we can see if the NVDIMM is OK.Signed-off-by Tony Luck <[email protected]>---
<>
Quoted 8 lines
include/acpi/nfit.h new file +2
-0@@ -0,0 +1,26 @@ +/* +* Copyright(c) 2017 Intel Corporation. All rights reserved.
include/acpi/nfit.h line 12
Quoted 10 lines
include/acpi/nfit.h new file +10
-0+* +* This program is free software; you can redistribute it and/or modify +* it under the terms of version 2 of the GNU General Public License as +* published by the Free Software Foundation. +* +* This program is distributed in the hope that it will be useful, but +* WITHOUT ANY WARRANTY; without even the implied warranty of +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU +* General Public License for more details. +*/
Just a quick nit - I think we're supposed to use SPDX license identifiers for new files?
_______________________________________________
Linux-nvdimm mailing list
[email protected]
https://lists.01.org/mailman/listinfo/linux-nvdimm
A message that is not in the archive (sent off-list or not yet received): 20180228173621.GA12883-VuQAYsv1563Yd54FQh9/[email protected]
Re: [PATCH 3/5] acpi, nfit: Add function to look up nvdimm device and provide SMBIOS handle
Quoted 2 lines · Ross Zwisler
Just a quick nit - I think we're supposed to use SPDX license identifiers for new files?
One would think checkpatch would warn about that but nope.
Signature · 8 lines
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
_______________________________________________
Linux-nvdimm mailing list
[email protected]
https://lists.01.org/mailman/listinfo/linux-nvdimmPATCH 4/5 firmware: dmi: Add function to look up a handle and return DIMM size
In reply to a message not in the archiveWhen 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.
--- drivers/firmware/dmi_scan.c | 31 +++++++++++++++++++++++++++++++ include/linux/dmi.h | 2 ++ 2 files changed, 33 insertions(+)
drivers/firmware/dmi_scan.c +31 -0
@@ -32,6 +32,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;
@@ -386,6 +387,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 ;
@@ -396,6 +399,20 @@ static void __init save_mem_devices(const struct dmi_header *dm, void *v) dmi_memdev[nr].handle =get_unaligned (&dm->handle); dmi_memdev[nr].device =dmi_string (dm, d[0x10 ]); dmi_memdev[nr].bank =dmi_string (dm, d[0x11 ]); + + 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 ; + + dmi_memdev[nr].size = bytes; nr++; }
@@ -1065,3 +1082,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) +{ +int n; + +if (dmi_memdev) { +for (n =0 ; n < dmi_memdev_nr; n++) { +if (handle == dmi_memdev[n].handle) +return dmi_memdev[n].size; + } + } +return ~0ul ; +} +EXPORT_SYMBOL_GPL (dmi_memdev_size);
include/linux/dmi.h +2 -0
@@ -113,6 +113,7 @@ extern int dmi_walk(void (*decode)(const struct dmi_header *, void *), void *private_data);extern bool dmi_match (enum dmi_field f,const char *str);extern void dmi_memdev_name (u16 handle,const char **bank,const char **device); +extern u64 dmi_memdev_size (u16 handle);#else
@@ -142,6 +143,7 @@ static inline bool dmi_match(enum dmi_field f, const char *str) {return false; }static inline void dmi_memdev_name (u16 handle,const char **bank,const char **device) { } +static inline u64 dmi_memdev_size (u16 handle) {return ~0ul ; }static inline const struct dmi_system_id *dmi_first_match (const struct dmi_system_id *list) {return NULL ; }
Signature · 6 lines
2.14.1 _______________________________________________ Linux-nvdimm mailing list [email protected] https://lists.01.org/mailman/listinfo/linux-nvdimm
A message that is not in the archive (sent off-list or not yet received): 20180222195811.27237-5-tony.luck-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org
Re: [PATCH 4/5] firmware: dmi: Add function to look up a handle and return DIMM size
Hi Tony,
Quoted 4 lines · Tony Luck
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.
On the principle I'm OK with the idea. My comments on the implementation details are inline below.
Quoted 11 lines
Signed-off-by Tony Luck <[email protected]>--- drivers/firmware/dmi_scan.c | 31 +++++++++++++++++++++++++++++++ include/linux/dmi.h | 2 ++ 2 files changed, 33 insertions(+)drivers/firmware/dmi_scan.c +0
-0
drivers/firmware/dmi_scan.c static char dmi_ids_string[128] __initdata; · line 35
Quoted 4 lines
drivers/firmware/dmi_scan.c +1
-0@@ -32,6 +32,7 @@ static char dmi_ids_string[128] __initdata; static struct dmi_memdev_info { const char *device; const char *bank; + u64 size;
A comment stating in which unit the size is stored would be appreciated.
Quoted 13 lines
drivers/firmware/dmi_scan.c +2
-0u16 handle; } *dmi_memdev;static int dmi_memdev_nr;@@ -386,6 +387,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 static void __init save_mem_devices(const struct dmi_header *dm, void *v) · line 407
Quoted 9 lines
drivers/firmware/dmi_scan.c +6
-0@@ -396,6 +399,20 @@ static void __init save_mem_devices(const struct dmi_header *dm, void *v) dmi_memdev[nr].handle =get_unaligned (&dm->handle); dmi_memdev[nr].device =dmi_string (dm, d[0x10 ]); dmi_memdev[nr].bank =dmi_string (dm, d[0x11 ]); + + size =get_unaligned ((u16 *)&d[0xC ]); +if (size ==0 ) + bytes =0 ; +else if (size ==0xffff ) + bytes = ~0ul ;
Should this not be ~0ull? On 32-bit systems ~0ul would be 0xffffffff not 0xffffffffffffffff.
Also it would be nice to document somewhere that 0 means memory module not installed and all fs means size is unknown.
drivers/firmware/dmi_scan.c static void __init save_mem_devices(const struct dmi_header *dm, void *v) · line 415
Quoted 8 lines
drivers/firmware/dmi_scan.c +8
-0+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 ; + + dmi_memdev[nr].size = bytes;
I'm curious, do you really need to know the amount of memory to the byte? The SMBIOS specification itself does not support granularity under 1 kB. I would be very surprised if any machine running Linux today has memory modules under 1 MB. If storing in MB you wouldn't need a u64...
Quoted 9 lines
drivers/firmware/dmi_scan.c +2
-0nr++; }@@ -1065,3 +1082,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)
drivers/firmware/dmi_scan.c void dmi_memdev_name(u16 handle, const char **bank, const char **device) · line 1096
Quoted 10 lines
drivers/firmware/dmi_scan.c +10
-0+{ +int n; + +if (dmi_memdev) { +for (n =0 ; n < dmi_memdev_nr; n++) { +if (handle == dmi_memdev[n].handle) +return dmi_memdev[n].size; + } + } +return ~0ul ;
~0ull?
Quoted 15 lines
drivers/firmware/dmi_scan.c +2
-0+} +EXPORT_SYMBOL_GPL (dmi_memdev_size);include/linux/dmi.h +1
-0@@ -113,6 +113,7 @@ extern int dmi_walk(void (*decode)(const struct dmi_header *, void *), void *private_data);extern bool dmi_match (enum dmi_field f,const char *str);extern void dmi_memdev_name (u16 handle,const char **bank,const char **device); +extern u64 dmi_memdev_size (u16 handle);#else
include/linux/dmi.h static inline bool dmi_match(enum dmi_field f, const char *str) · line 146
Quoted 4 lines
include/linux/dmi.h +1
-0@@ -142,6 +143,7 @@ static inline bool dmi_match(enum dmi_field f, const char *str) {return false; }static inline void dmi_memdev_name (u16 handle,const char **bank,const char **device) { } +static inline u64 dmi_memdev_size (u16 handle) {return ~0ul ; }
~0ull?
Quoted 3 lines
include/linux/dmi.h +0
-0static inline const struct dmi_system_id *dmi_first_match (const struct dmi_system_id *list) {return NULL ; }
Signature · 6 lines
Jean Delvare SUSE L3 Support _______________________________________________ Linux-nvdimm mailing list [email protected] https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH 4/5] firmware: dmi: Add function to look up a handle and return DIMM size
Quoted 6 lines · Jean Delvare
Quoted 6 lines
drivers/firmware/dmi_scan.c +6
-0+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 ;
Quoted 8 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+ + dmi_memdev[nr].size = bytes;I'm curious, do you really need to know the amount of memory to the byte? The SMBIOS specification itself does not support granularity under 1 kB. I would be very surprised if any machine running Linux today has memory modules under 1 MB. If storing in MB you wouldn't need a u64...
I got side-tracked reading the standard with the ancient ways used to report size back in the day when kilobytes was a plausible unit. So I wrote code that covers all the crazy cases. Persistant DIMMs have sizes measured in gigabytes. I should stop worrying about "0" vs. "fffffff" and just treat the old cases as errors and simplify the code to be:
u32 mbytes; if (size == 0 || (size & 0x8000)) mbytes = 0; if (size != 0x7fff) mbytes = size; else mbytes = get_unaligned((u32 *)&d[0x1C]);
Then I can use 32-bit throughout this and patch 5.
Thanks for the review.
-Tony
_______________________________________________
Linux-nvdimm mailing list
[email protected]
https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH 4/5] firmware: dmi: Add function to look up a handle and return DIMM size
Hi Tony,
Quoted 7 lines · Tony
I got side-tracked reading the standard with the ancient ways used to report size back in the day when kilobytes was a plausible unit. So I wrote code that covers all the crazy cases. Persistant DIMMs have sizes measured in gigabytes. I should stop worrying about "0" vs. "fffffff" and just treat the old cases as errors and simplify the code to be:
Quoted 5 lines
u32 mbytes; if (size == 0 || (size & 0x8000)) mbytes = 0; if (size != 0x7fff)
"else" missing.
Quoted 5 lines
mbytes = size; else mbytes = get_unaligned((u32 *)&d[0x1C]);Then I can use 32-bit throughout this and patch 5.
Note that it is possible to store MB values (up to 16 MB) using kB as the unit. The specification allows for it, and a few systems use that option. For example [1], the DMI data of a Supermicro X8STi board looks like:
Handle 0x0038, DMI type 17, 28 bytes
Memory Device
(...) Size: 4096 kB
and it is encoded as 0x9000.
I understand you don't care about such "small" memory modules for persistent DIMMs, however the API is not specific to these, and it could be confusing for callers that modules between 1 MB and 16 MB are sometimes reported as 0 and sometimes not. So I believe that your code should handle this case.
Quoted 1 line
Thanks for the review.
You're welcome.
[1] Let's be honest, that was the only instance out of the 180 DMI dumps I collected. So it's fairly rare. But it did happen.
Signature · 6 lines
Jean Delvare SUSE L3 Support _______________________________________________ Linux-nvdimm mailing list [email protected] https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH 4/5] firmware: dmi: Add function to look up a handle and return DIMM size
Quoted 10 lines · Jean Delvare
Note that it is possible to store MB values (up to 16 MB) using kB as the unit. The specification allows for it, and a few systems use that option. For example [1], the DMI data of a Supermicro X8STi board looks like:
Handle
0x0038, DMI type 17, 28 bytes
Memory Device(...) Size: 4096 kB
Quoted 7 lines
and it is encoded as
0x9000.I understand you don't care about such "small" memory modules for persistent DIMMs, however the API is not specific to these, and it could be confusing for callers that modules between 1 MB and 16 MB are sometimes reported as 0 and sometimes not. So I believe that your code should handle this case.
Then I might as well go with a fully standards compliant version (just in case somebody in the future has a 512KB module encoded as 0x8200, they would also be confused by a return of 0 MB).
That means I should stick with u64 as the type of the "size" field (using KB in a u32 would max out with 4TB modules, which may happen before I retire).
-Tony
_______________________________________________
Linux-nvdimm mailing list
[email protected]
https://lists.01.org/mailman/listinfo/linux-nvdimm
PATCH 5/5 EDAC, skx_edac: Detect non-volatile DIMMs
In reply to a message not in the archiveThis 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.
--- drivers/edac/Kconfig | 5 +++- drivers/edac/skx_edac.c | 68 ++++++++++++++++++++++++++++++++++++++++++++----- 2 files changed, 66 insertions(+), 7 deletions(-)
drivers/edac/Kconfig +4 -1
@@ -232,9 +232,12 @@ config EDAC_SBRIDGE config EDAC_SKX tristate "Intel Skylake server Integrated MC" depends on PCI && X86_64 && X86_MCE_INTEL && PCI_MMCONFIG + select DMI help Support for error detection and correction the Intel - Skylake server Integrated Memory Controllers. + Skylake server Integrated Memory Controllers. If your + system has non-volatile DIMMs you should also manually + select CONFIG_ACPI_NFIT config EDAC_PND2 tristate "Intel Pondicherry2"
drivers/edac/skx_edac.c +62 -6
127 lines not on this page · raw message
Signature · 6 lines
2.14.1 _______________________________________________ Linux-nvdimm mailing list [email protected] https://lists.01.org/mailman/listinfo/linux-nvdimm
A message that is not in the archive (sent off-list or not yet received): 20180222195811.27237-6-tony.luck-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org
Re: [PATCH 5/5] EDAC, skx_edac: Detect non-volatile DIMMs
Hi Tony,
Quoted 15 lines · Tony Luck
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 | 5 +++- drivers/edac/skx_edac.c | 68 ++++++++++++++++++++++++++++++++++++++++++++----- 2 files changed, 66 insertions(+), 7 deletions(-)(...)
drivers/edac/skx_edac.c +5
-0+static int get_nvdimm_info (struct dimm_info *dimm,struct skx_imc *imc, +int chan,int dimmno) +{ +int smbios_handle; +u32 dev_handle;
drivers/edac/skx_edac.c static int get_dimm_info(u32 mtr, u32 amap, struct dimm_info *dimm, · line 402
Quoted 10 lines
drivers/edac/skx_edac.c +10
-0+u16 flags; +u64 size =0 ; + + dev_handle =ACPI_NFIT_BUILD_DEVICE_HANDLE (dimmno, chan, imc->lmc, + imc->src_id,0 ); + + smbios_handle =nfit_get_smbios_id (dev_handle, &flags); +if (smbios_handle == -EOPNOTSUPP ) { +pr_warn_once ("skx_edac: can't find size of NVDIMM. Try enabling CONFIG_ACPI_NFIT\n" ); + goto unknown_size;
I'm curious why you continue in this (worse) error case, but stop on all other (some presumably less critical) error cases? Specifically I can't see how an unknown size returned by the dmi subsystem can be worse than not being able to query the size at all.
drivers/edac/skx_edac.c static int get_dimm_info(u32 mtr, u32 amap, struct dimm_info *dimm, · line 415
Quoted 13 lines
drivers/edac/skx_edac.c +13
-0+ } +if (smbios_handle <0 ) { +skx_printk (KERN_ERR ,"Can't find handle for NVDIMM ADR=%x\n" , dev_handle); +return 0 ; + } + +if (flags &ACPI_NFIT_MEM_MAP_FAILED ) { +skx_printk (KERN_ERR ,"NVDIMM ADR=%x is not mapped\n" , dev_handle); +return 0 ; + } + + size =dmi_memdev_size (smbios_handle); +if (size == ~0ul ) {
If you agree with my comment on previous patch then this becomes ~0ull.
drivers/edac/skx_edac.c static int get_dimm_info(u32 mtr, u32 amap, struct dimm_info *dimm, · line 422
Quoted 7 lines
drivers/edac/skx_edac.c +7
-0+skx_printk (KERN_ERR ,"Can't find size for NVDIMM ADR=%x/SMBIOS=%x\n" , + dev_handle, smbios_handle); +return 0 ; + } + +unknown_size: + edac_dbg(0, "mc#%d: channel %d, dimm %d, %lld Mb (%lld pages)\n",
%llu instead of %lld (twice)?
drivers/edac/skx_edac.c static int get_dimm_info(u32 mtr, u32 amap, struct dimm_info *dimm, · line 425
Quoted 3 lines
drivers/edac/skx_edac.c +3
-0+ imc->mc, chan, dimmno, size >> 20, size >> PAGE_SHIFT); + + dimm->nr_pages = size >>PAGE_SHIFT ;
If you moved the debug print after, you wouldn't have to do the shift twice.
Quoted 10 lines
drivers/edac/skx_edac.c +10
-0+ dimm->grain =32 ; + dimm->dtype =DEV_UNKNOWN ; + dimm->mtype =MEM_NVDIMM ; + dimm->edac_mode =EDAC_SECDED ;/* likely better than this */ + +snprintf (dimm->label,sizeof (dimm->label),"CPU_SrcID#%u_MC#%u_Chan#%u_DIMM#%u" , + imc->src_id, imc->lmc, chan, dimmno); + +return 1 ; +}
Signature · 6 lines
Jean Delvare SUSE L3 Support _______________________________________________ Linux-nvdimm mailing list [email protected] https://lists.01.org/mailman/listinfo/linux-nvdimm
Quoted 21 lines · Tony Luck
This series was posted as RFC and got some review back in December:
https://marc.info/?l=linux-edac&m=151257213825419&w=2It couldn't go upstream back then because I was waiting for some updates to the ACPICA headers for NFIT support. Those patches are complete now, so here is the series again.
The "partial support" part is that the changes here only do the detection of the non-volatile DIMMs. Coming later will be another patch/series to teach
skx_edachow to decode system addresses to NVDIMM addresses. But this part stands on its own and is a useful first step.Tony Luck (5):
EDAC: Drop duplicated array of strings for memory type names
edac: Add new memory type for non-volatile DIMMs
acpi, nfit: Add function to look up nvdimm device and provide SMBIOS
handle
firmware: dmi: Add function to look up a handle and return DIMM size EDAC,skx_edac: Detect non-volatile DIMMs
Quoted 11 lines
drivers/acpi/nfit/core.c | 27 ++++++++++++++++++ drivers/edac/Kconfig | 5 +++- drivers/edac/edac_mc.c | 41 +++++++++++++------------- drivers/edac/edac_mc_sysfs.c | 26 ++--------------- drivers/edac/skx_edac.c | 68 ++++++++++++++++++++++++++++++++++++++++---- drivers/firmware/dmi_scan.c | 31 ++++++++++++++++++++ include/acpi/nfit.h | 26 +++++++++++++++++ include/linux/dmi.h | 2 ++ include/linux/edac.h | 3 ++ 9 files changed, 178 insertions(+), 51 deletions(-) create mode 100644 include/acpi/nfit.h
Series looks ok to me.
Dan, I could route patch 3 through the EDAC tree if you'd prefer.
Jean, ditto for patch 4.
Thanks.
Signature · 8 lines
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
_______________________________________________
Linux-nvdimm mailing list
[email protected]
https://lists.01.org/mailman/listinfo/linux-nvdimmA message that is not in the archive (sent off-list or not yet received): [email protected]
Dan Williams
Acked-byQuoted 34 lines · Borislav Petkov
Quoted 21 lines · Tony Luck
This series was posted as RFC and got some review back in December:
https://marc.info/?l=linux-edac&m=151257213825419&w=2It couldn't go upstream back then because I was waiting for some updates to the ACPICA headers for NFIT support. Those patches are complete now, so here is the series again.
The "partial support" part is that the changes here only do the detection of the non-volatile DIMMs. Coming later will be another patch/series to teach
skx_edachow to decode system addresses to NVDIMM addresses. But this part stands on its own and is a useful first step.Tony Luck (5):
EDAC: Drop duplicated array of strings for memory type names
edac: Add new memory type for non-volatile DIMMs
acpi, nfit: Add function to look up nvdimm device and provide SMBIOS
handle
firmware: dmi: Add function to look up a handle and return DIMM size EDAC,skx_edac: Detect non-volatile DIMMsQuoted 11 lines
drivers/acpi/nfit/core.c | 27 ++++++++++++++++++ drivers/edac/Kconfig | 5 +++- drivers/edac/edac_mc.c | 41 +++++++++++++------------- drivers/edac/edac_mc_sysfs.c | 26 ++--------------- drivers/edac/skx_edac.c | 68 ++++++++++++++++++++++++++++++++++++++++---- drivers/firmware/dmi_scan.c | 31 ++++++++++++++++++++ include/acpi/nfit.h | 26 +++++++++++++++++ include/linux/dmi.h | 2 ++ include/linux/edac.h | 3 ++ 9 files changed, 178 insertions(+), 51 deletions(-) create mode 100644 include/acpi/nfit.h
Quoted 3 lines
Series looks ok to me.
Dan, I could route patch 3 through the EDAC tree if you'd prefer.
Yes, please, and you can add:
_______________________________________________
Linux-nvdimm mailing list
[email protected]
https://lists.01.org/mailman/listinfo/linux-nvdimm
A message that is not in the archive (sent off-list or not yet received): CAPcyv4iHr9nUAS0SiFeqqDkU8NXZ_x=rkD3YS010Dy-nbHJXmA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org
v3 cover Teach EDAC about non-volatile DIMMs and add partial support to skx_edac
Add support for non-volatile DIMMS
[Repost with fixes from Ross Zwisler and Jean Delvare's comments]
Changes since previous version:
Parts 1-2 unchanged since last post
Part 3 Use new SPDX license header for new file include/acpi/nfit.h Part 4 Fix some ~0ul that should be ~0ull Part 5 Rationalize error handling for various different reasons
that we didn't find the size of a non-volatile DIMM Fix some printk formats.
Fix test for ~0ul that should be ~0ull Re-order code to avoid computing number of pages twice
Dan gave the thumb's up to run part 3 through the EDAC tree.
Tony Luck (5):
EDAC: Drop duplicated array of strings for memory type names
edac: Add new memory type for non-volatile DIMMs
acpi, nfit: Add function to look up nvdimm device and provide SMBIOS
handle
firmware: dmi: Add function to look up a handle and return DIMM size EDAC, skx_edac: Detect non-volatile DIMMs
drivers/acpi/nfit/core.c | 27 ++++++++++++++++++ drivers/edac/Kconfig | 5 +++- drivers/edac/edac_mc.c | 41 +++++++++++++-------------- drivers/edac/edac_mc_sysfs.c | 26 ++--------------- drivers/edac/skx_edac.c | 66 ++++++++++++++++++++++++++++++++++++++++---- drivers/firmware/dmi_scan.c | 31 +++++++++++++++++++++ include/acpi/nfit.h | 18 ++++++++++++ include/linux/dmi.h | 2 ++ include/linux/edac.h | 3 ++ 9 files changed, 168 insertions(+), 51 deletions(-) create mode 100644 include/acpi/nfit.h
Signature · 6 lines
2.14.1 _______________________________________________ Linux-nvdimm mailing list [email protected] https://lists.01.org/mailman/listinfo/linux-nvdimm
A message that is not in the archive (sent off-list or not yet received): 20180312182430.10335-1-tony.luck-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org
PATCH 1/5 EDAC: Drop duplicated array of strings for memory type names
Somehow we ended up with two separate arrays of strings to describe the "enum mem_type" values.
In edac_mc.c we have an exported list edac_mem_types[] that is used by a couple of drivers in debug messaged.
In edac_mc_sysfs.c we have a private list that is used to display values in:
/sys/devices/system/edac/mc/mc*/dimm*/dimm_mem_type /sys/devices/system/edac/mc/mc*/csrow*/mem_type
this list was missing a value for MEM_LRDDR3
The string values in the two lists were different :-(
Combining the lists, I kept the values so that the sysfs output will be unchanged as some scripts may depend on that.
--- drivers/edac/edac_mc.c | 40 ++++++++++++++++++++-------------------- drivers/edac/edac_mc_sysfs.c | 26 ++------------------------ 2 files changed, 22 insertions(+), 44 deletions(-)
drivers/edac/edac_mc.c +20 -20
@@ -195,26 +195,26 @@ static void edac_mc_dump_mci(struct mem_ctl_info *mci)#endif /* CONFIG_EDAC_DEBUG */ const char *const edac_mem_types[] = { - [MEM_EMPTY ] ="Empty csrow" , - [MEM_RESERVED ] ="Reserved csrow type" , - [MEM_UNKNOWN ] ="Unknown csrow type" , - [MEM_FPM ] ="Fast page mode RAM" , - [MEM_EDO ] ="Extended data out RAM" , - [MEM_BEDO ] ="Burst Extended data out RAM" , - [MEM_SDR ] ="Single data rate SDRAM" , - [MEM_RDR ] ="Registered single data rate SDRAM" , - [MEM_DDR ] ="Double data rate SDRAM" , - [MEM_RDDR ] ="Registered Double data rate SDRAM" , - [MEM_RMBS ] ="Rambus DRAM" , - [MEM_DDR2 ] ="Unbuffered DDR2 RAM" , - [MEM_FB_DDR2 ] ="Fully buffered DDR2" , - [MEM_RDDR2 ] ="Registered DDR2 RAM" , - [MEM_XDR ] ="Rambus XDR" , - [MEM_DDR3 ] ="Unbuffered DDR3 RAM" , - [MEM_RDDR3 ] ="Registered DDR3 RAM" , - [MEM_LRDDR3 ] ="Load-Reduced DDR3 RAM" , - [MEM_DDR4 ] ="Unbuffered DDR4 RAM" , - [MEM_RDDR4 ] ="Registered DDR4 RAM" , + [MEM_EMPTY ] ="Empty" , + [MEM_RESERVED ] ="Reserved" , + [MEM_UNKNOWN ] ="Unknown" , + [MEM_FPM ] ="FPM" , + [MEM_EDO ] ="EDO" , + [MEM_BEDO ] ="BEDO" , + [MEM_SDR ] ="Unbuffered-SDR" , + [MEM_RDR ] ="Registered-SDR" , + [MEM_DDR ] ="Unbuffered-DDR" , + [MEM_RDDR ] ="Registered-DDR" , + [MEM_RMBS ] ="RMBS" , + [MEM_DDR2 ] ="Unbuffered-DDR2" , + [MEM_FB_DDR2 ] ="FullyBuffered-DDR2" , + [MEM_RDDR2 ] ="Registered-DDR2" , + [MEM_XDR ] ="XDR" , + [MEM_DDR3 ] ="Unbuffered-DDR3" , + [MEM_RDDR3 ] ="Registered-DDR3" , + [MEM_LRDDR3 ] ="Load-Reduced-DDR3-RAM" , + [MEM_DDR4 ] ="Unbuffered-DDR4" , + [MEM_RDDR4 ] ="Registered-DDR4" };EXPORT_SYMBOL_GPL (edac_mem_types);
drivers/edac/edac_mc_sysfs.c +2 -24
@@ -91,28 +91,6 @@ static struct device *mci_pdev;/* * various constants for Memory Controllers */ -static const char *const mem_types[] = { - [MEM_EMPTY ] ="Empty" , - [MEM_RESERVED ] ="Reserved" , - [MEM_UNKNOWN ] ="Unknown" , - [MEM_FPM ] ="FPM" , - [MEM_EDO ] ="EDO" , - [MEM_BEDO ] ="BEDO" , - [MEM_SDR ] ="Unbuffered-SDR" , - [MEM_RDR ] ="Registered-SDR" , - [MEM_DDR ] ="Unbuffered-DDR" , - [MEM_RDDR ] ="Registered-DDR" , - [MEM_RMBS ] ="RMBS" , - [MEM_DDR2 ] ="Unbuffered-DDR2" , - [MEM_FB_DDR2 ] ="FullyBuffered-DDR2" , - [MEM_RDDR2 ] ="Registered-DDR2" , - [MEM_XDR ] ="XDR" , - [MEM_DDR3 ] ="Unbuffered-DDR3" , - [MEM_RDDR3 ] ="Registered-DDR3" , - [MEM_DDR4 ] ="Unbuffered-DDR4" , - [MEM_RDDR4 ] ="Registered-DDR4" -}; -static const char *const dev_types[] = { [DEV_UNKNOWN ] ="Unknown" , [DEV_X1 ] ="x1" ,
@@ -196,7 +174,7 @@ static ssize_t csrow_mem_type_show(struct device *dev, {struct csrow_info *csrow =to_csrow (dev); -return sprintf (data,"%s\n" , mem_types[csrow->channels[0 ]->dimm->mtype]); +return sprintf (data,"%s\n" , edac_mem_types[csrow->channels[0 ]->dimm->mtype]); } static ssize_t csrow_dev_type_show(struct device *dev,
@@ -549,7 +527,7 @@ static ssize_t dimmdev_mem_type_show(struct device *dev, {struct dimm_info *dimm =to_dimm (dev); -return sprintf (data,"%s\n" , mem_types[dimm->mtype]); +return sprintf (data,"%s\n" , edac_mem_types[dimm->mtype]); } static ssize_t dimmdev_dev_type_show(struct device *dev,
Signature · 6 lines
2.14.1 _______________________________________________ Linux-nvdimm mailing list [email protected] https://lists.01.org/mailman/listinfo/linux-nvdimm
PATCH 2/5 edac: Add new memory type for non-volatile DIMMs
In reply to a message not in the archiveThere are now non-volatile versions of DIMMs. Add a new entry to "enum mem_type" and a new string in edac_mem_types[].
--- drivers/edac/edac_mc.c | 3 ++- include/linux/edac.h | 3 +++ 2 files changed, 5 insertions(+), 1 deletion(-)
drivers/edac/edac_mc.c +2 -1
@@ -214,7 +214,8 @@ const char * const edac_mem_types[] = { [MEM_RDDR3] = "Registered-DDR3", [MEM_LRDDR3] = "Load-Reduced-DDR3-RAM", [MEM_DDR4] = "Unbuffered-DDR4", - [MEM_RDDR4] = "Registered-DDR4" + [MEM_RDDR4] = "Registered-DDR4", + [MEM_NVDIMM ] ="Non-volatile-RAM" , };EXPORT_SYMBOL_GPL (edac_mem_types);
include/linux/edac.h +3 -0
@@ -186,6 +186,7 @@ static inline char *mc_event_error_type(const unsigned int err_type)* @MEM_RDDR4: Registered DDR4 RAM * This is a variant of the DDR4 memories. * @MEM_LRDDR4: Load-Reduced DDR4 memory. +* @MEM_NVDIMM: Non-volatile RAM */ enum mem_type {MEM_EMPTY =0 ,
@@ -209,6 +210,7 @@ enum mem_type {MEM_DDR4 ,MEM_RDDR4 ,MEM_LRDDR4 , + MEM_NVDIMM, };#define MEM_FLAG_EMPTY BIT(MEM_EMPTY)
@@ -231,6 +233,7 @@ enum mem_type {#define MEM_FLAG_DDR4 BIT(MEM_DDR4)#define MEM_FLAG_RDDR4 BIT(MEM_RDDR4)#define MEM_FLAG_LRDDR4 BIT(MEM_LRDDR4) +#define MEM_FLAG_NVDIMM BIT(MEM_NVDIMM)/** * enum edac-type - Error Detection and Correction capabilities and mode
Signature · 6 lines
2.14.1 _______________________________________________ Linux-nvdimm mailing list [email protected] https://lists.01.org/mailman/listinfo/linux-nvdimm
PATCH 3/5 acpi, nfit: Add function to look up nvdimm device and provide SMBIOS handle
In reply to a message not in the archiveEDAC driver needs to look up attributes of NVDIMMs provided in SMBIOS.
Provide a function that looks up an acpi_nfit_memory_map from a device handle (node/socket/mc/channel/dimm) and returns the SMBIOS handle. Also pass back the "flags" so we can see if the NVDIMM is OK.
--- drivers/acpi/nfit/core.c | 27 +++++++++++++++++++++++++++ include/acpi/nfit.h | 18 ++++++++++++++++++ 2 files changed, 45 insertions(+) create mode 100644 include/acpi/nfit.h
drivers/acpi/nfit/core.c +27 -0
@@ -23,6 +23,7 @@#include <linux/io.h> #include <linux/nd.h> #include <asm/cacheflush.h> +#include <acpi/nfit.h> #include "nfit.h" /*
@@ -690,6 +691,32 @@ static bool add_memdev(struct acpi_nfit_desc *acpi_desc,return true; } +int nfit_get_smbios_id (u32 device_handle,u16 *flags) +{ +struct acpi_nfit_memory_map *memdev; +struct acpi_nfit_desc *acpi_desc; +struct nfit_mem *nfit_mem; + +mutex_lock (&acpi_desc_lock); +list_for_each_entry (acpi_desc, &acpi_descs, list) { +mutex_lock (&acpi_desc->init_mutex); +list_for_each_entry (nfit_mem, &acpi_desc->dimms, list) { + memdev =__to_nfit_memdev (nfit_mem); +if (memdev->device_handle == device_handle) { +mutex_unlock (&acpi_desc->init_mutex); +mutex_unlock (&acpi_desc_lock); + *flags = memdev->flags; +return memdev->physical_id; + } + } +mutex_unlock (&acpi_desc->init_mutex); + } +mutex_unlock (&acpi_desc_lock); + +return -ENODEV ; +} +EXPORT_SYMBOL_GPL (nfit_get_smbios_id); +/* * An implementation may provide a truncated control region if no block windows * are defined.
include/acpi/nfit.h new file +18 -0
@@ -0,0 +1,18 @@ +/* +* SPDX-License-Identifier: GPL-2.0 +* Copyright (C) 2018 Intel Corporation +*/ + +#ifndef __ACPI_NFIT_H +#define __ACPI_NFIT_H + +#if IS_ENABLED (CONFIG_ACPI_NFIT ) +int nfit_get_smbios_id (u32 device_handle,u16 *flags); +#else +static inline int nfit_get_smbios_id (u32 device_handle,u16 *flags) +{ +return -EOPNOTSUPP ; +} +#endif + +#endif /* __ACPI_NFIT_H */
Signature · 6 lines
2.14.1 _______________________________________________ Linux-nvdimm mailing list [email protected] https://lists.01.org/mailman/listinfo/linux-nvdimm
PATCH 4/5 firmware: dmi: Add function to look up a handle and return DIMM size
In reply to a message not in the archiveWhen 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.
--- drivers/firmware/dmi_scan.c | 31 +++++++++++++++++++++++++++++++ include/linux/dmi.h | 2 ++ 2 files changed, 33 insertions(+)
drivers/firmware/dmi_scan.c +31 -0
@@ -32,6 +32,7 @@ static char dmi_ids_string[128] __initdata;static struct dmi_memdev_info {const char *device;const char *bank; +u64 size;/* bytes */ u16 handle; } *dmi_memdev;static int dmi_memdev_nr;
@@ -386,6 +387,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 ;
@@ -396,6 +399,20 @@ static void __init save_mem_devices(const struct dmi_header *dm, void *v) dmi_memdev[nr].handle =get_unaligned (&dm->handle); dmi_memdev[nr].device =dmi_string (dm, d[0x10 ]); dmi_memdev[nr].bank =dmi_string (dm, d[0x11 ]); + + size =get_unaligned ((u16 *)&d[0xC ]); +if (size ==0 ) + bytes =0 ; +else if (size ==0xffff ) + bytes = ~0ull ; +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 ; + + dmi_memdev[nr].size = bytes; nr++; }
@@ -1065,3 +1082,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) +{ +int n; + +if (dmi_memdev) { +for (n =0 ; n < dmi_memdev_nr; n++) { +if (handle == dmi_memdev[n].handle) +return dmi_memdev[n].size; + } + } +return ~0ull ; +} +EXPORT_SYMBOL_GPL (dmi_memdev_size);
include/linux/dmi.h +2 -0
@@ -113,6 +113,7 @@ extern int dmi_walk(void (*decode)(const struct dmi_header *, void *), void *private_data);extern bool dmi_match (enum dmi_field f,const char *str);extern void dmi_memdev_name (u16 handle,const char **bank,const char **device); +extern u64 dmi_memdev_size (u16 handle);#else
@@ -142,6 +143,7 @@ static inline bool dmi_match(enum dmi_field f, const char *str) {return false; }static inline void dmi_memdev_name (u16 handle,const char **bank,const char **device) { } +static inline u64 dmi_memdev_size (u16 handle) {return ~0ul ; }static inline const struct dmi_system_id *dmi_first_match (const struct dmi_system_id *list) {return NULL ; }
Signature · 6 lines
2.14.1 _______________________________________________ Linux-nvdimm mailing list [email protected] https://lists.01.org/mailman/listinfo/linux-nvdimm
A message that is not in the archive (sent off-list or not yet received): 20180312182430.10335-5-tony.luck-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org
Jean Delvare
Reviewed-byRe: [PATCH 4/5] firmware: dmi: Add function to look up a handle and return DIMM size
Quoted 84 lines · Tony Luck
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 | 31 +++++++++++++++++++++++++++++++ include/linux/dmi.h | 2 ++ 2 files changed, 33 insertions(+)drivers/firmware/dmi_scan.c +31
-0@@ -32,6 +32,7 @@ static char dmi_ids_string[128] __initdata;static struct dmi_memdev_info {const char *device;const char *bank; +u64 size;/* bytes */ u16 handle; } *dmi_memdev;static int dmi_memdev_nr;@@ -386,6 +387,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 ;@@ -396,6 +399,20 @@ static void __init save_mem_devices(const struct dmi_header *dm, void *v) dmi_memdev[nr].handle =get_unaligned (&dm->handle); dmi_memdev[nr].device =dmi_string (dm, d[0x10 ]); dmi_memdev[nr].bank =dmi_string (dm, d[0x11 ]); + + size =get_unaligned ((u16 *)&d[0xC ]); +if (size ==0 ) + bytes =0 ; +else if (size ==0xffff ) + bytes = ~0ull ; +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 ; + + dmi_memdev[nr].size = bytes; nr++; }@@ -1065,3 +1082,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) +{ +int n; + +if (dmi_memdev) { +for (n =0 ; n < dmi_memdev_nr; n++) { +if (handle == dmi_memdev[n].handle) +return dmi_memdev[n].size; + } + } +return ~0ull ; +} +EXPORT_SYMBOL_GPL (dmi_memdev_size);include/linux/dmi.h +1
-0@@ -113,6 +113,7 @@ extern int dmi_walk(void (*decode)(const struct dmi_header *, void *), void *private_data);extern bool dmi_match (enum dmi_field f,const char *str);extern void dmi_memdev_name (u16 handle,const char **bank,const char **device); +extern u64 dmi_memdev_size (u16 handle);#else
include/linux/dmi.h static inline bool dmi_match(enum dmi_field f, const char *str) · line 149
Quoted 7 lines
include/linux/dmi.h +1
-0@@ -142,6 +143,7 @@ static inline bool dmi_match(enum dmi_field f, const char *str) {return false; }static inline void dmi_memdev_name (u16 handle,const char **bank,const char **device) { } +static inline u64 dmi_memdev_size (u16 handle) {return ~0ul ; }static inline const struct dmi_system_id *dmi_first_match (const struct dmi_system_id *list) {return NULL ; }
Signature · 6 lines
Jean Delvare SUSE L3 Support _______________________________________________ Linux-nvdimm mailing list [email protected] https://lists.01.org/mailman/listinfo/linux-nvdimm
PATCH 5/5 EDAC, skx_edac: Detect non-volatile DIMMs
In reply to a message not in the archiveThis 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.
--- drivers/edac/Kconfig | 5 +++- drivers/edac/skx_edac.c | 66 ++++++++++++++++++++++++++++++++++++++++++++----- 2 files changed, 64 insertions(+), 7 deletions(-)
drivers/edac/Kconfig +4 -1
@@ -232,9 +232,12 @@ config EDAC_SBRIDGE config EDAC_SKX tristate "Intel Skylake server Integrated MC" depends on PCI && X86_64 && X86_MCE_INTEL && PCI_MMCONFIG + select DMI help Support for error detection and correction the Intel - Skylake server Integrated Memory Controllers. + Skylake server Integrated Memory Controllers. If your + system has non-volatile DIMMs you should also manually + select CONFIG_ACPI_NFIT config EDAC_PND2 tristate "Intel Pondicherry2"
drivers/edac/skx_edac.c +60 -6
125 lines not on this page · raw message
Signature · 6 lines
2.14.1 _______________________________________________ Linux-nvdimm mailing list [email protected] https://lists.01.org/mailman/listinfo/linux-nvdimm
A message that is not in the archive (sent off-list or not yet received): 20180312182430.10335-6-tony.luck-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org
Jean Delvare
Reviewed-byRe: [PATCH 5/5] EDAC, skx_edac: Detect non-volatile DIMMs
Hi Tony,
Quoted 15 lines · Tony Luck
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 | 5 +++- drivers/edac/skx_edac.c | 66 ++++++++++++++++++++++++++++++++++++++++++++----- 2 files changed, 64 insertions(+), 7 deletions(-)drivers/edac/Kconfig +0
-0
drivers/edac/Kconfig config EDAC_SBRIDGE · line 240
Quoted 10 lines
drivers/edac/Kconfig +4
-1@@ -232,9 +232,12 @@ config EDAC_SBRIDGE config EDAC_SKX tristate "Intel Skylake server Integrated MC" depends on PCI && X86_64 && X86_MCE_INTEL && PCI_MMCONFIG + select DMI help Support for error detection and correction the Intel - Skylake server Integrated Memory Controllers. + Skylake server Integrated Memory Controllers. If your + system has non-volatile DIMMs you should also manually + select CONFIG_ACPI_NFIT
Nitpicking: trailing dot is missing.
Quoted 37 lines
drivers/edac/Kconfig +0
-0config EDAC_PND2 tristate "Intel Pondicherry2"drivers/edac/skx_edac.c +29
-0+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; +u64 size =0 ; + + dev_handle =ACPI_NFIT_BUILD_DEVICE_HANDLE (dimmno, chan, imc->lmc, + imc->src_id,0 ); + + smbios_handle =nfit_get_smbios_id (dev_handle, &flags); +if (smbios_handle == -EOPNOTSUPP ) { +pr_warn_once ("skx_edac: can't find size of NVDIMM. Try enabling CONFIG_ACPI_NFIT\n" ); + goto unknown_size; + } +if (smbios_handle <0 ) { +skx_printk (KERN_ERR ,"Can't find handle for NVDIMM ADR=%x\n" , dev_handle); + goto unknown_size; + } + +if (flags &ACPI_NFIT_MEM_MAP_FAILED ) { +skx_printk (KERN_ERR ,"NVDIMM ADR=%x is not mapped\n" , dev_handle); + goto unknown_size; + } + + size =dmi_memdev_size (smbios_handle); +if (size == ~0ull ) + skx_printk(KERN_ERR, "Can't find size for NVDIMM ADR=%x/SMBIOS=%x\n",
drivers/edac/skx_edac.c static int get_dimm_info(u32 mtr, u32 amap, struct dimm_info *dimm, · line 426
Quoted 10 lines
drivers/edac/skx_edac.c +10
-0+ dev_handle, smbios_handle); + +unknown_size: + 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 */ + + edac_dbg(0, "mc#%d: channel %d, dimm %d, %llu Mb (%u pages)\n",
I did not notice on previous review, but I think "b" in general means bit not byte, so "MB" would be better.
drivers/edac/skx_edac.c static int get_dimm_info(u32 mtr, u32 amap, struct dimm_info *dimm, · line 433
Quoted 7 lines
drivers/edac/skx_edac.c +7
-0+ imc->mc, chan, dimmno, size >> 20, dimm->nr_pages); + +snprintf (dimm->label,sizeof (dimm->label),"CPU_SrcID#%u_MC#%u_Chan#%u_DIMM#%u" , + imc->src_id, imc->lmc, chan, dimmno); + +return 1 ; +}
Now this function always return 1, that doesn't make a lot of sense?
Other than these details, the dmi-related code looks good to me now.
Signature · 6 lines
Jean Delvare SUSE L3 Support _______________________________________________ Linux-nvdimm mailing list [email protected] https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH 5/5] EDAC, skx_edac: Detect non-volatile DIMMs
Quoted 4 lines · Jean Delvare
Review
Quoted 1 line
(quoted diff) +1
-0+ edac_dbg(0, "mc#%d: channel %d, dimm %d, %llu Mb (%u pages)\n",I did not notice on previous review, but I think "b" in general means bit not byte, so "MB" would be better.
Heh! We've been cut & pasting that line from EDAC drivers since the i7core_edac.c I think the origin is in 2.6.35. So you are far from the only person to not notice it in a review :-)
Quoted 10 lines
Review
Quoted 8 lines
(quoted diff) +7
-0+ imc->mc, chan, dimmno, size >> 20, dimm->nr_pages); + + snprintf(dimm->label, sizeof(dimm->label), "CPU_SrcID#%u_MC#%u_Chan#%u_DIMM#%u", + imc->src_id, imc->lmc, chan, dimmno); + + return 1; +}Now this function always return 1, that doesn't make a lot of sense?
I could fix the return to be:
drivers/edac/skx_edac.c +1 -1
@@ -429,7 +429,7 @@ static int get_nvdimm_info(struct dimm_info *dimm, struct skx_imc *imc,snprintf (dimm->label,sizeof (dimm->label),"CPU_SrcID#%u_MC#%u_Chan#%u_DIMM#%u" , imc->src_id, imc->lmc, chan, dimmno); -return 1 ; +return (size ==0 || size == ~0ull ) ?0 :1 ; }#define SKX_GET_MTMTR (dev, reg) \
Quoted 3 lines
Other than these details, the dmi-related code looks good to me now.
Reviewed-by Jean Delvare <[email protected]> # for DMI
Thanks. Boris ... ready to go now? Or do you have some other comments?
-Tony
_______________________________________________
Linux-nvdimm mailing list
[email protected]
https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH 5/5] EDAC, skx_edac: Detect non-volatile DIMMs
Quoted 9 lines · Tony
Quoted 4 lines · Jean Delvare
Review
Quoted 1 line
(quoted diff) +1
-0+ edac_dbg(0, "mc#%d: channel %d, dimm %d, %llu Mb (%u pages)\n",I did not notice on previous review, but I think "b" in general means bit not byte, so "MB" would be better.
Heh! We've been cut & pasting that line from EDAC drivers since the
i7core_edac.cI think the origin is in 2.6.35. So you are far from the only person to not notice it in a review :-)
Hehe :-D Then I guess a patch fixing all drivers are once is in order, after your patch set it accepted.
Quoted 18 lines
Quoted 9 lines
Review
Quoted 7 lines
(quoted diff) +7
-0+ imc->mc, chan, dimmno, size >> 20, dimm->nr_pages); + + snprintf(dimm->label, sizeof(dimm->label), "CPU_SrcID#%u_MC#%u_Chan#%u_DIMM#%u", + imc->src_id, imc->lmc, chan, dimmno); + + return 1; +}Now this function always return 1, that doesn't make a lot of sense?
I could fix the return to be:
drivers/edac/skx_edac.c +0
-0
drivers/edac/skx_edac.c static int get_nvdimm_info(struct dimm_info *dimm, struct skx_imc *imc, · line 433
Quoted 6 lines
drivers/edac/skx_edac.c +1
-1@@ -429,7 +429,7 @@ static int get_nvdimm_info(struct dimm_info *dimm, struct skx_imc *imc,snprintf (dimm->label,sizeof (dimm->label),"CPU_SrcID#%u_MC#%u_Chan#%u_DIMM#%u" , imc->src_id, imc->lmc, chan, dimmno); -return 1 ; +return (size ==0 || size == ~0ull ) ?0 :1 ; }
Yes, I think it makes sense, thanks.
Signature · 6 lines
Jean Delvare SUSE L3 Support _______________________________________________ Linux-nvdimm mailing list [email protected] https://lists.01.org/mailman/listinfo/linux-nvdimm
Borislav Petkov
Reviewed-by Jean DelvareRe: [PATCH 5/5] EDAC, skx_edac: Detect non-volatile DIMMs
In reply to Luck, TonyQuoted 1 line · Tony
Thanks. Boris ... ready to go now? Or do you have some other comments?
I've committed this. Running build smoke tests now.
Thx.
---
From: Tony Luck <[email protected]>
Date: Mon, 12 Mar 2018 11:24:30 -0700
Subject: [PATCH] EDAC, skx_edac: Detect non-volatile DIMMs
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.
--- drivers/edac/Kconfig | 5 +++- drivers/edac/skx_edac.c | 67 ++++++++++++++++++++++++++++++++++++++++++++----- 2 files changed, 65 insertions(+), 7 deletions(-)
drivers/edac/Kconfig +4 -1
@@ -232,9 +232,12 @@ config EDAC_SBRIDGE config EDAC_SKX tristate "Intel Skylake server Integrated MC" depends on PCI && X86_64 && X86_MCE_INTEL && PCI_MMCONFIG + select DMI help Support for error detection and correction the Intel - Skylake server Integrated Memory Controllers. + Skylake server Integrated Memory Controllers. If your + system has non-volatile DIMMs you should also manually + select CONFIG_ACPI_NFIT. config EDAC_PND2 tristate "Intel Pondicherry2"
drivers/edac/skx_edac.c +61 -6
126 lines not on this page · raw message
Signature · 8 lines
2.13.0 SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg) -- _______________________________________________ Linux-nvdimm mailing list [email protected] https://lists.01.org/mailman/listinfo/linux-nvdimm