| Message ID | 20180222195811.27237-5-tony.luck@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Tony, On Thu, 22 Feb 2018 11:58:10 -0800, Tony Luck wrote: > 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. > Signed-off-by: Tony Luck <tony.luck@intel.com> > --- > drivers/firmware/dmi_scan.c | 31 +++++++++++++++++++++++++++++++ > include/linux/dmi.h | 2 ++ > 2 files changed, 33 insertions(+) > > diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c > index e763e1484331..9947f1f6ef7b 100644 > --- a/drivers/firmware/dmi_scan.c > +++ b/drivers/firmware/dmi_scan.c > @@ -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. > 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; 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. > + 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... > 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; ~0ull? > +} > +EXPORT_SYMBOL_GPL(dmi_memdev_size); > diff --git a/include/linux/dmi.h b/include/linux/dmi.h > index 46e151172d95..7f5929123b69 100644 > --- a/include/linux/dmi.h > +++ b/include/linux/dmi.h > @@ -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; } ~0ull? > static inline const struct dmi_system_id * > dmi_first_match(const struct dmi_system_id *list) { return NULL; } >
On Fri, Mar 09, 2018 at 11:20:53AM +0100, Jean Delvare wrote: > > + 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... 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
Hi Tony, On Fri, 9 Mar 2018 15:03:58 -0800, Luck, Tony wrote: > 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) "else" missing. > 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. > 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.
On Sat, Mar 10, 2018 at 02:22:17PM +0100, Jean Delvare wrote: > 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. 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
diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c index e763e1484331..9947f1f6ef7b 100644 --- a/drivers/firmware/dmi_scan.c +++ b/drivers/firmware/dmi_scan.c @@ -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); diff --git a/include/linux/dmi.h b/include/linux/dmi.h index 46e151172d95..7f5929123b69 100644 --- a/include/linux/dmi.h +++ b/include/linux/dmi.h @@ -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; }
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 <tony.luck@intel.com> --- drivers/firmware/dmi_scan.c | 31 +++++++++++++++++++++++++++++++ include/linux/dmi.h | 2 ++ 2 files changed, 33 insertions(+)