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

[Jean Delvare]

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?

> 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.

> +{
> +     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.

> +             "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?

> +}
> +
>  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)?

> +                      * 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).

> +                      *  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

> +                     // NB: Poweron upper QWORD not needed for 
> 2,104,351,365,926,255 years

Please stick to /* legacy C comment style */.

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

> +                     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...

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

%hhu

> +                     else
> +                             pr_attr("Power Wattage", "%s", "Unknown");

In which unit is this value expressed? On my sample, the value is 25,
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?

> +                     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.

> +                     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.

> +                     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.

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.

> +                     if (data[0x38] > 1)

Hmm, so 2 RPM is OK, but 1 RPM is not? Odd.

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

Case: "RPM".

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

> +                     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.

> +                     else
> +                             pr_attr("Negotiated Speed", "%s", "Uknown");

Typo: "Unknown".

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

%hu

> +                     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.

> +                     break;
>               default:
>                       return 0;
>       }

You use a mix of lowercase and uppercase letters in your hexadecimal
numbers. Please stick to uppercase everywhere for consistency.

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.

Thanks,
-- 
Jean Delvare
SUSE L3 Support