Hi Jerry,
On Mon, 26 Sep 2022 23:40:25 -0600, Jerry Hoemann wrote:
> From: Jerry Hoemann <jerry.hoem...@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.hoem...@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
_______________________________________________
https://lists.nongnu.org/mailman/listinfo/dmidecode-devel
