[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [dmidecode] [PATCH] dmidecode: Extensions to Memory Device (Type 17)
|
From: |
Jerry Hoemann |
|
Subject: |
Re: [dmidecode] [PATCH] dmidecode: Extensions to Memory Device (Type 17) |
|
Date: |
Fri, 15 Jun 2018 15:08:39 -0600 |
|
User-agent: |
Mutt/1.9.1 (2017-09-22) |
On Fri, Jun 15, 2018 at 10:01:02AM +0200, Jean Delvare wrote:
> 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.
I'll look into what I can provide.
>
> >
> > 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.
Done.
>
> > + "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 */.
Done.
>
> > + };
> > + if (code && code <= 0x07)
>
> Also for consistency the first test would be code >= 0x01 (even if the
> result is obviously the same.)
Done.
>
> > + printf(" %s", detail[code - 0x01]);
> > + else
> > + printf(" %s", out_of_spec);
> > +}
> > +
> > +static void dmi_memory_operating_mode_capility(u16 code)
>
> "capility", really? :-D
Fixed.
>
> > +{
> > + /* 7.18.7 */
> > + static const char * const detail[] = {
>
> I'd name it "mode".
Done.
>
> > + "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.
Done.
>
> > +
> > +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.
Done.
Looking at spec again, fields 34h, 3Ch, and 44h, say if the value is 0,
there is no XXXX portion. This verbiage isn't present for field 4Ch.
I'm thinking that for 34h, 3Ch, 44h, we also special case when field is "0."
This would have then two functions, dmi_memory_size() for these three fields
and dmi_memory_logical_size() for 4Ch.
>
> > +
> > +
>
> Extra blank line.
Done.
>
> > 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.
Done
>
> 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 missed this. I was thinking it was all or nothing on the new fields.
Fixed.
>
> 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.
You're correct. Fixed.
Next version will have appropriate length checks before printing
*each* of the new fields.
>
> > + 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.
Done.
>
> > break;
> >
> > case 18: /* 7.19 32-bit Memory Error Information */
>
> Thanks,
> --
> Jean Delvare
> SUSE L3 Support
--
-----------------------------------------------------------------------------
Jerry Hoemann Software Engineer Hewlett Packard Enterprise
-----------------------------------------------------------------------------