Re: [dmidecode] [PATCH] dmidecode: Extensions to Memory Device (Type 17)

[Jean Delvare]

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 <address@hidden>
> ---
>  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