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

[Jerry Hoemann]

On Tue, Jan 05, 2021 at 01:56:09PM +0100, Jean Delvare wrote:
> Hi Jerry,
> 
> On Wed, 16 Dec 2020 14:18:58 -0700, Jerry Hoemann wrote:
> > HP Firmware Inventory Record (Type 240)
> > 
> > Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>
> > ---
> >  dmioem.c | 41 +++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 41 insertions(+)
> > 
> > diff --git a/dmioem.c b/dmioem.c
> > index 180a95d..d8cab2c 100644
> > --- a/dmioem.c
> > +++ b/dmioem.c
> > @@ -361,6 +361,47 @@ 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
> 
> Capital A after dot.
> 

done.

> > +                    * with other SMBIOS records, such as Type 203 (which 
> > in turn is
> > +                    * associated with Types 9, 41, and 228),
> 
> End with a dot instead of comma?

done.

> 
> > +                    *
> > +                    * 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 HPE Proliant Inventory Record", 
> > company);
> 
> Remove the hard-coded "HPE" else you end up with "HPE HPE" in the
> output.

done.

> 
> To be on the safe side, please start with a length check as is done for
> regular DMI types (although apparently not all OEM types - will look
> into that).

I think you mean a check like:
                        pr_handle_name("%s Proliant Inventory Record", company);
                        if (h->length < 0x24) break;

before decoding any fields?

Looks like record 236 (commit d9b84ea) should have done something similar.
I'll fix record 236 as a separate patch.

> 
> > +                   pr_attr("Associated Handle ", "0x%04X",  WORD(data + 
> > 0x4));
> 
> You may want to condition that one on !(opt.flags & FLAG_QUIET) as
> we do for other handle cross-references.

hadn't noticed that paridigm.

There are multiple Associated handles, will apply to all.

> 
> > +                   pr_attr("Package Version   ", "0x%X",   DWORD(data + 
> > 0x6));
> > +                   pr_attr("Version String    ", "%s",     dmi_string(h, 
> > data[0x0A]));
> 
> We don't do space-alignment of attribute names in dmidecode, so it
> would look weird to do it for just that one record.
> 
> (If there is a general desire to present things that way for better
> readability then that would be handled globally, through the recently
> introduced "output drivers".)
> 
> We also don't do space-alignment in the source code.

done.

> 
> > +
> > +                   if (DWORD(data + 0x0B))
> > +                           pr_attr("Image Size (bytes)", "0x%08x", 
> > QWORD(data + 0xB));
> 
> Could we use dmi_print_memory_size() here so that the size is
> automatically expressed in the most suitable unit?

dmi_print_memory_size() is defined statically in dmidecode.c.

Will make global and put prototype in dmidecode.h


> 
> > +                   else
> > +                           pr_attr("Image Size        ", "%s",     "Not 
> > Available");
> > +
> > +                   pr_attr("Attribute Defined ", "0x%08x", QWORD(data + 
> > 0x13));
> > +                   pr_attr("Attribute Set     ", "0x%08x", QWORD(data + 
> > 0x1B));
> 
> I would use Attributes (plural) for both?

done

> 
> Would it make sense to go into greater details and list the individual
> attributes? Or if that too low details?

I found details and will add.

> 
> > +
> > +                   if (DWORD(data + 0x23))
> > +                           pr_attr("Lowest Supported Version ", "%hi",     
> > DWORD(data + 0x23));
> > +                   else
> > +                           pr_attr("Lowest Supported Version ", "%s",      
> > "Not Available");
> > +                   break;
> 
> I have one system where this prints -1. Should all FF's be treated as
> "Not Available" too? Or does it have a special meaning?


I fixed the printf format.


> 
> > +
> >             default:
> >                     return 0;
> >     }
> 
> 
> -- 
> Jean Delvare
> SUSE L3 Support

-- 

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