Re: [dmidecode] [RFC] dmioem: Decode HPE OEM Record 242

[Jerry Hoemann]

On Fri, Sep 30, 2022 at 02:51:19PM +0200, Jean Delvare wrote:
> Hi Jerry,
> 
> On Mon, 26 Sep 2022 23:40:25 -0600, Jerry Hoemann wrote:
> > From: Jerry Hoemann <jerry.hoemann@hpe.com>
> > 
> > Decode HPE OEM Record 242: Hard Drive Inventory Record
> 
> You added "RFC" to the subject. Is there anything in this patch that
> you think needs do be discussed?

I wasn't quite ready to send this patch upstream, but wanted something
to test the mailing list with to narrow down problems we were seeing.
I mentioned it to you in off list email, but should have also made it
more clear in patch documentation.


> 
> > Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>
> > ---
> >  dmioem.c | 117 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 117 insertions(+)
> > 
> > diff --git a/dmioem.c b/dmioem.c
> > index 6000e1c..250c2c6 100644
> > --- a/dmioem.c
> > +++ b/dmioem.c
> > @@ -466,6 +466,44 @@ static void dmi_hp_238_speed(const char *fname, 
> > unsigned int code)
> >     pr_attr(fname, "%s", str);
> >  }
> >  
> > +static void dmi_hp_242_hdd_type(u8 code)
> > +{
> > +   const char *str = "Reserved";
> > +   static const char * const type[] = {
> > +           "Undetermined", /* 0x00 */
> > +           "NVMe SSD",
> > +           "SATA",
> > +           "SAS",
> > +           "SATA SSD",
> > +           "NVMe Manged by VROC/VMD", /* 0x05 */
> > +   };
> > +   if (code < ARRAY_SIZE(type))
> > +           str = type[code];
> > +
> > +   pr_attr("Hard Drive Type", "%s", str);
> > +}
> > +
> > +static void dmi_hp_242_ff(u8 code)
> 
> Please spell out form_factor for clarity.

done.

> 
> > +{
> > +   const char *str = "Reserved";
> > +   static const char * const form[] = {
> > +           "Reserved", /* 0x00 */
> > +           "Reserved",
> > +           "3.5\" form factor",
> > +           "2.5\" form factor",
> > +           "1.8\" form factor",
> > +           "less than 1.8\" form factor",
> 
> "Less" (uppercase L) for consistency.

done.
> 
> > +           "mSATA",
> > +           "M.2",
> > +           "MicroSSD",
> > +           "CFast", /* 0x09 */
> > +   };
> > +   if (code < ARRAY_SIZE(form))
> > +           str = form[code];
> > +
> > +   pr_attr("Hard Drive Form Factor", "%s", str);
> 
> Isn't "Hard Drive" already pretty much implied by the record type?

removed "Hard Drive".

> 
> > +}
> > +
> >  static int dmi_decode_hp(const struct dmi_header *h)
> >  {
> >     u8 *data = h->data;
> > @@ -944,6 +982,85 @@ static int dmi_decode_hp(const struct dmi_header *h)
> >                             pr_attr("Lowest Supported Version", "Not 
> > Available");
> >                     break;
> >  
> > +           case 242:
> > +                   /*
> > +                    * Vendor Specific: HPE Hard Drive Inventory Record
> > +                    *
> > +                    * This record provides a mechanism for software to 
> > gather information for
> > +                    * NVMe and SATA drives that are directly attached to 
> > the system. This record
> > +                    * does not contain drive information for drive 
> > attached to a HBA (i.e. a
> 
> "drives" (plural)?

cut/paste from doc.  Updated to:
                         * does not contain drive information for drives 
attached to a HBA (i.e. a

> 
> > +                    * SmartArray controller). This record will only 
> > contain information for a
> > +                    * hard drive detected by the BIOS during POST and does 
> > not comprehend a hot
> > +                    * plug event after the system has booted.
> > +                    *
> > +                    * Offset |  Name      | Width | Description
> > +                    * ---------------------------------------
> > +                    *  0x00  | Type       | BYTE  | 0xF2, HPE Hard Drive 
> > Inventory Record
> > +                    *  0x01  | Length     | BYTE  | Length of structure
> > +                    *  0x02  | Handle     | WORD  | Unique handle
> > +                    *  0x04  | Hndl Assoc | WORD  | Handle to map to Type 
> > 203
> > +                    *  0x06  | HDD Type   | BYTE  | Hard drive type
> > +                    *  0x07  | HDD Uniq ID| QWORD | SATA-> WWID.  NVMe -> 
> > IEEE Ext Uniq ID.
> > +                    *  0x0F  | Capacity   | DWORD | Drive Capacity in 
> > Mbytes
> > +                    *  0x13  | Hours      | 16BYTE| Number of poweron hours
> > +                    *  0x23  | Reserved   | BYTE  | Reserved
> > +                    *  0x24  | Power      | BTYE  | Wattage
> > +                    *  0x25  | Form Factor| BYTE  | HDD Form Factor
> > +                    *  0x26  | Health     | BYTE  | Hard Drive Health 
> > Status
> > +                    *  0x27  | Serial Num | STRING| NVMe/SATA Serial Number
> > +                    *  0x28  | Model Num  | STRING| NVMe/SATA Model Number
> > +                    *  0x29  | FW Rev     | STRING| Firmware revision
> > +                    *  0x2A  | Location   | STRING| Drive location
> > +                    *  0x2B  | Crypt Stat | BYTE  | Drive encryption 
> > status from BIOS
> > +                    *  0x2C  | Capacity   | QWORD | Hard Drive capacity in 
> > bytes
> > +                    *  0x34  | Block Size | DWORD | Logical Block Size in 
> > bytes
> > +                    *  0x38  | Rot Speed  | WORD  | Nominal Rotational 
> > Speed (rpm)
> 
> "RPM" (uppercase).

done.

> 
> > +                    *  0x3A  | Neg Speed  | WORD  | Current negotiated bus 
> > speed
> > +                    *  0x3C  | Cap Speed  | WORD  | Fastest Capable Bus 
> > Speed of drive
> > +                    */
> > +                   if (gen < G10) return 0;
> > +                   pr_handle_name("%s ProLiant Hard Drive Inventory 
> > Record", company);
> > +                   if (h->length < 0x2c) break;
> > +                   if (!(opt.flags & FLAG_QUIET))
> > +                           pr_attr("Associated Handle", "0x%04X", 
> > WORD(data + 0x4));
> > +                   dmi_hp_242_hdd_type(data[0x06]);
> > +                   pr_attr("ID", "%lx", QWORD(data + 0x07));
> > +                   pr_attr("Capacity", "%d MB", DWORD(data + 0x0f));
> 
> %u


done.

> 
> > +                   // NB: Poweron upper QWORD not needed for 
> > 2,104,351,365,926,255 years
> 
> Please stick to /* legacy C comment style */.

done.

> 
> I admit I was surprised to see 16 bytes allocated to store the power-on
> hours value ;-)

The field was clearly defined by an optimist. :)

> 
> > +                   pr_attr("Poweron", "%ld hours", QWORD(data + 0x13));
> 
> QWORD returns a struct. I doubt this can be reliably passed to printf?
> TBH I'm surprised gcc doesn't complain. Maybe it's OK...


Hmm,  Using the -E gcc flag, this gets expanded to:

pr_attr("Poweron", "%ld hours", (*(const u64 *)(data + 0x13)));

Note: field 0x007 of record 242 and field 0x09 of record 236 also do a
similar print statement.  So, if the above is incorrect, we may need to
fix up the other places as well.


> 
> > +                   if (data[0x24])
> > +                           pr_attr("Power Wattage", "%d", data[0x24]);
> 
> %hhu

done.


> 
> > +                   else
> > +                           pr_attr("Power Wattage", "%s", "Unknown");
> 
> In which unit is this value expressed? On my sample, the value is 25,

spec says Watts.

> for a NVMe SSD. I've never seen a mechanical drive consuming more than
> 13 W, and I'd expect a SSD to consume under 5 W. So 25 W seems a lot.
> But then again I'm not too much into enterprise server hardware, so I
> could be wrong.
> 
> > +                   dmi_hp_242_ff(data[0x25]);
> > +                   feat = data[0x26];
> > +                   pr_attr("Health Status", "%s", (feat == 0x00) ? "OK" :
> > +                                                   (feat == 0x01) ? 
> > "Warning" :
> > +                                                   (feat == 0x02) ? 
> > "Critical" :
> > +                                                   (feat == 0xFF) ? 
> > "Unknown" : "Reserved");
> 
> Any reason why you do not introduce a helper function for that one, as
> we typically do for such enumerated fields?

Jugdgment call.  There aren't very many entries and all the entries are
short. So, thought this was eaiser to read.  YMMV.

Do you want me to change to helper?

> 
> > +                   pr_attr("Serial Number", dmi_string(h, data[0x27]));
> > +                   pr_attr("Model Number", dmi_string(h, data[0x28]));
> > +                   pr_attr("Firmware Revsion", dmi_string(h, data[0x29]));
> > +                   pr_attr("Hard Drive LOCATION", dmi_string(h, 
> > data[0x2A]));
> 
> Case: "Location". I'd also strip "Hard Drive" as it is implied.

done.

> 
> > +                   feat = data[0x2B];
> > +                   pr_attr("Encryption Status", "%s", (feat == 0) ? "Not 
> > Encrypted" :
> > +                                                   (feat == 1) ? 
> > "Encrypted" :
> > +                                                   (feat == 2) ? "Uknown" 
> > :  
> > +                                                   (feat == 3) ? "Not 
> > Supported" : "Reserved");
> 
> Same question here.

Same.  I can switch to helper functions if you want.

> 
> > +                   if (h->length < 0x3e) break;
> > +                   pr_attr("Capacity", "%lu bytes", QWORD(data + 0x2C));
> 
> This field is redundant with the field at offset 0x0F. Does your
> specification clarify what to do when both fields are present? I
> don't think we want to print both values.

field 0f is 32 bits field representing MB -> effective 52 bits number.
field 2c is 64 bits field represent byte -> efiecitve 64 bit number.
  
So. new field gives more head room.  My examples report same size
(in different units.)  But at some point that need not be so.

I will display field 0x2C when available instead of field 0x0F.

> 
> There is a similar situation for memory sizes (DMI type 16, fields 0x07
> and 0x0F, as well as DMI type 17, fields 0x0C and 0x1C). I think we
> should handle the situation in the same way here.
> 
> It might also make sense to use dmi_print_memory_size() to display the
> value in a more human-friendly way. I'm not sure if disk sizes are
> usually a power of 2 (or a multiple of some large power of 2) as memory
> modules tend to be. I guess not, then we could print both the exact
> value, and a rounded value using the most suitable unit.
> 
> > +                   pr_attr("Black Size", "%u bytes", DWORD(data + 0x34));
> 
> It might make sense to print this in a human-friendly unit.

I should also spell block correctly.  :)

All my examples have block size == 512 bytes.

Is there a "size" print helper that takes DWORD?  I didn't see one.
[ dmi_print_memory_size takes a u64 which isn't compatible. ]


> 
> > +                   if (data[0x38] > 1)
> 
> Hmm, so 2 RPM is OK, but 1 RPM is not? Odd.

These are special cases:  0 -> Not reported.  1 -> SSD.

So, I just decided not to print anything in these two cases.

Do you want me to print something in one or both of these cases?
Or just comment the code better?


> 
> > +                           pr_attr("Rotational Speed", "%d rpm", 
> > data[0x38]);
> 
> Case: "RPM".

done.

> 
> I also think you want to use %u instead of %d. In case it spins
> realllly fast :-D

done. :)
> 
> > +                   if (WORD(data + 0x3A))
> > +                           pr_attr("Negotiated Speed", "%d Gbit/s", 
> > WORD(data + 0x3A));
> 
> %h is preferred for short-size variables. %hu in this case as it can't
> be negative.

done.
> 
> > +                   else
> > +                           pr_attr("Negotiated Speed", "%s", "Uknown");
> 
> Typo: "Unknown".

done.
> 
> > +                   if (WORD(data + 0x3C))
> > +                           pr_attr("Capable Speed", "%d Gbit/s", WORD(data 
> > + 0x3C));
> 
> %hu

done.

> 
> > +                   else
> > +                           pr_attr("Capable Speed", "%s", "Unknown");
> 
> It might make sense to introduce a helper function for Negotiated Speed
> and Capable Speed, as the code is pretty much the same for both.

done.

> 
> > +                   break;
> >             default:
> >                     return 0;
> >     }
> 
> You use a mix of lowercase and uppercase letters in your hexadecimal
> numbers. Please stick to uppercase everywhere for consistency.
> 

done.

> Also note that I don't have any sample implementing the second part of
> type 242. If you could share a dump with from a system where this is
> implemented, I'd be happy to add it to my test suite.


I'll see what I can give you.

> 
> Thanks,
> -- 
> Jean Delvare
> SUSE L3 Support

-- 

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