Hi Jerry, On Mon, 11 Jun 2018 11:40:07 -0600, Jerry Hoemann wrote: > The DSP0134 v3.2.0 extended the Memory Device (Type 17) structure > starting at offset 28h continuing to 4Ch to reflect persistent memory.
Do you have access to any system implementing SMBIOS 3.2.0 already? If so, I would appreciate if you could send a dump of the DMI table to me (using "dmidecode --dump-bin") so that I can add it to my non-regression test suite. > > Signed-off-by: Jerry Hoemann <[email protected]> > --- > dmidecode.c | 113 > ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 113 insertions(+) > > diff --git a/dmidecode.c b/dmidecode.c > index d18a258..77feee2 100644 > --- a/dmidecode.c > +++ b/dmidecode.c > @@ -2499,6 +2499,84 @@ static void dmi_memory_device_type_detail(u16 code) > } > } > > +static void dmi_memory_technology(u8 code) > +{ > + /* 7.18.6 */ > + static const char * const detail[] = { I suppose you copied the name "detail" from dmi_memory_device_type_detail(). The array was named "detail" there because of what it was representing, but that doesn't really make sense in your new function. "technology" would be a better name. > + "Other", /* 1 */ For consistency with the rest of the code, comments should be /* 0x01 */... > + "Unknown", > + "DRAM", > + "NVDIMM-N", > + "NVDIMM-F", > + "NVDIMM-P", > + "Intel persistent memory" /* 7 */ ... and /* 0x07 */. > + }; > + if (code && code <= 0x07) Also for consistency the first test would be code >= 0x01 (even if the result is obviously the same.) > + printf(" %s", detail[code - 0x01]); > + else > + printf(" %s", out_of_spec); > +} > + > +static void dmi_memory_operating_mode_capility(u16 code) "capility", really? :-D > +{ > + /* 7.18.7 */ > + static const char * const detail[] = { I'd name it "mode". > + "Other", /* 1 */ > + "Unknown", > + "Volatile memory", > + "Byte-accessible persistent memory", > + "Block-accessible persistent memory" /* 5 */ > + }; > + > + if ((code & 0xFFFE) == 0) > + printf(" None"); > + else { > + int i; > + > + for (i = 1; i <= 5; i++) > + if (code & (1 << i)) > + printf(" %s", detail[i - 1]); > + } > +} > + > +static void dmi_memory_manufacturer_id(u16 code) > +{ > + /* 7.18.8 */ > + /* 7.18.10 */ > + /* LSB is 7-bit Odd Parity number of continuation codes. */ > + if (code == 0) > + printf(" Unknown"); > + else > + printf(" Bank %d, Hex 0x%2X", (code & 0x7f) + 1, code >> 8); > +} > + > +static void dmi_memory_module_product_id(u16 code) > +{ > + /* 7.18.9 */ > + if (code == 0) > + printf(" Unknown"); > + else > + printf(" 0x%04X", code); > +} > + > +static void dmi_memory_subsystem_controller_product_id(u16 code) > +{ > + /* 7.18.11 */ > + if (code == 0) > + printf(" Unknown"); > + else > + printf(" 0x%04X", code); > +} The two functions above are exactly the same, so there should be only one of them defined, and called twice. You could call it "dmi_memory_product_id" to show that it is somewhat generic. > + > +static void dmi_memory_size(u64 code) > +{ > + if (code.h == 0xFFFFFFFF && code.l == 0xFFFFFFFF) > + printf(" Unknown"); > + else > + printf(" 0x%08X%08X", code.h, code.l); > +} Please re-use dmi_print_memory_size() instead. > + > + Extra blank line. > static void dmi_memory_device_speed(u16 code) > { > if (code == 0) > @@ -3907,6 +3985,41 @@ static void dmi_decode(const struct dmi_header *h, u16 > ver) > printf("\tConfigured Voltage:"); > dmi_memory_voltage_value(WORD(data + 0x26)); > printf("\n"); > + > + if (h->length < 0x4c) break; For consistency, there should be no blank line here, and 0x4C should be with uppercase C. Also, the specification says that length must be at least 34h for SMBIOS version 3.2.0 and later. This suggests that the fields after that are optional, and may not be present in all implementations. So it would be safer to check for h->length < 0x34 first, and add another length check later in the code flow for the remaining fields. I think your length check is wrong too... Last field *starts* at 0x4C and is 8 byte long, so the length check for a full record should be < 0x54. > + printf("\tMemory Technology:"); > + dmi_memory_technology(data[0x28]); > + printf("\n"); > + printf("\tMemory Operating Mode Capability:"); > + dmi_memory_operating_mode_capility(WORD(data + 0x29)); > + printf("\n"); > + printf("\tFirmware Version: %s\n", > + dmi_string(h, data[0x2B])); > + printf("\tModule Manufacturer ID:"); > + dmi_memory_manufacturer_id(WORD(data + 0x2C)); > + printf("\n"); > + printf("\tModule Product ID:"); > + dmi_memory_module_product_id(WORD(data + 0x2E)); > + printf("\n"); > + printf("\tMemory Subsystem Controller Manufacturer > ID:"); > + dmi_memory_manufacturer_id(WORD(data + 0x30)); > + printf("\n"); > + printf("\tMemory Subsystem Controller Product ID:"); > + dmi_memory_subsystem_controller_product_id(WORD(data + > 0x32)); > + printf("\n"); > + printf("\tNon-Volatile Size:"); > + dmi_memory_size(QWORD(data + 0x34)); > + printf("\n"); > + printf("\tVolatile Size:"); > + dmi_memory_size(QWORD(data + 0x3C)); > + printf("\n"); > + printf("\tCache Size:"); > + dmi_memory_size(QWORD(data + 0x44)); > + printf("\n"); > + printf("\tLogical Size:"); > + dmi_memory_size(QWORD(data + 0x4C)); > + printf("\n"); > + No blank line here either. > break; > > case 18: /* 7.19 32-bit Memory Error Information */ Thanks, -- Jean Delvare SUSE L3 Support _______________________________________________ https://lists.nongnu.org/mailman/listinfo/dmidecode-devel
