Re: [dmidecode] [PATCH v2 2/4] dmioem: Decode HPE OEM Record 240

[Jerry Hoemann]

On Tue, Jan 12, 2021 at 04:12:45PM +0100, Jean Delvare wrote:
> On Tue, 12 Jan 2021 01:05:03 -0700, Jerry Hoemann wrote:
> > HP Firmware Inventory Record (Type 240)

...


> > 
> >  static int dmi_decode_hp(const struct dmi_header *h)
> >  {
> >     u8 *data = h->data;
> > @@ -345,7 +363,6 @@ static int dmi_decode_hp(const struct dmi_header *h)
> >                      *  0x14  | Name       | STRING| (deprecated) Backplane 
> > Name
> >                      */
> >                     pr_handle_name("%s HDD Backplane FRU Information", 
> > company);
> > -
> >                     pr_attr("FRU I2C Address", "0x%X raw(0x%X)", data[0x4] 
> > >> 1, data[0x4]);
> >                     pr_attr("Box Number", "%d", WORD(data + 0x5));
> >                     pr_attr("NVRAM ID", "0x%X", WORD(data + 0x7));
> 
> That blank line change shouldn't be there, I guess it sneaked in while
> you were looking into fixing OEM type 236. I'll remove it before
> applying.

fixed.

> 
> > @@ -361,6 +378,49 @@ static int dmi_decode_hp(const struct dmi_header *h)
> >                     }
> >                     break;
> >  
> > +           case 240:
> > +                   /*
> > +                    * Vendor Specific: HPE Proliant Inventory Record
> > +                    *
> > +                    * Reports firmware version information for devices 
> > that report their
> > +                    * firmware using their UEFI drivers. Additionally 
> > provides association
> > +                    * with other SMBIOS records, such as Type 203 (which 
> > in turn is
> > +                    * associated with Types 9, 41, and 228).
> > +                    *
> > +                    * Offset |  Name      | Width | Description
> > +                    * ---------------------------------------
> > +                    *  0x00  | Type       | BYTE  | 0xF0, HP Firmware 
> > Inventory Record
> > +                    *  0x01  | Length     | BYTE  | Length of structure
> > +                    *  0x02  | Handle     | WORD  | Unique handle
> > +                    *  0x04  | Hndl Assoc | WORD  | Handle to map to Type 
> > 203
> > +                    *  0x06  | Pkg Vers   | DWORD | FW Vers Release of All 
> > FW in Device
> > +                    *  0x0A  | Ver String | STRING| FW Version String
> > +                    *  0x0B  | Image Size | QWORD | FW image size (bytes)
> > +                    *  0x13  | Attributes | QWORD | Bitfield: Is attribute 
> > defined?
> > +                    *  0x1B  | Attr Set   | QWORD | BitField: If defined, 
> > is attribute set?
> > +                    *  0x23  | Version    | DWORD | Lowest supported 
> > version.
> > +                    */
> > +                   pr_handle_name("%s Proliant Inventory Record", company);
> > +                   if (h->length < 0x24) break;
> 
> Sorry for telling you wrong in my original review. You have a DWORD at
> 0x23, meaning the last byte used is at offset 0x26, in turn meaning
> that you need to check for h->length < 0x27, not 0x24. I'll fix it
> before applying.


Fixed.

> 
> > +                   if (!(opt.flags & FLAG_QUIET))
> > +                           pr_attr("Associated Handle", "0x%04X", 
> > WORD(data + 0x4));
> > +                   pr_attr("Package Version", "0x%08X", DWORD(data + 0x6));
> > +                   pr_attr("Version String", "%s", dmi_string(h, 
> > data[0x0A]));
> > +
> > +                   if (DWORD(data + 0x0B))
> > +                           dmi_print_memory_size("Image Size", QWORD(data 
> > + 0xB), 0);
> > +                   else
> > +                           pr_attr("Image Size", "%s", "Not Available");
> 
> Note that format isn't mandatory. We can simply use:

Fixed.

> 
>                               pr_attr("Image Size", "Not Available");
> 
> Same below. I'll fix both.

Fixed.

> 
> > +
> > +                   dmi_hp_240_attr("Attributes Def", QWORD(data + 0x13));
> > +                   dmi_hp_240_attr("Attributes Set", QWORD(data + 0x1B));
> 
> I'm not completely satisfied with the way this presents the attributes.
> But we can commit this first and I'll see if I can come up with
> something better later.

I was thinking about puting in "|" to denote bit AND, but didn't
want to add the complication requied to avoid trailing "|" so I
have a trailing " " which isn't as noticable.

But, yes, we can defer.

> 
> > +
> > +                   if (DWORD(data + 0x23))
> > +                           pr_attr("Lowest Supported Version", "0x%08X", 
> > DWORD(data + 0x23));
> > +                   else
> > +                           pr_attr("Lowest Supported Version", "%s", "Not 
> > Available");
> 
> I'm curious what this "version" is about. On my samples, I have seen
> values 1, 2, 3, which do not look like hexadecimal values. But also
> 0x00000000 (properly handled) and 0x0000FFFF (displayed as is).
> 
> Are you sure this is a 32-bit field and not 2 16-bit fields?

The format for the Firmware Package Versions are vendor specific.
So, not sure how much order we can apply to the situation.

On the records that have Lowest Supported Version 0f: 0x0000FFFF.
I see Package Version: 0xFF000201.

So, the encodings for these are probably more involved than
a simple compact number range.



-- 

-----------------------------------------------------------------------------
Jerry Hoemann                  Software Engineer   Hewlett Packard Enterprise
-----------------------------------------------------------------------------