[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [dmidecode] [PATCH v2 3/4] dmioem: Decode HPE OEM Record 203
|
From: |
Jean Delvare |
|
Subject: |
Re: [dmidecode] [PATCH v2 3/4] dmioem: Decode HPE OEM Record 203 |
|
Date: |
Tue, 12 Jan 2021 18:01:43 +0100 |
Hi Jerry,
Thanks for the updated patches. Here's my review:
On Tue, 12 Jan 2021 01:05:04 -0700, Jerry Hoemann wrote:
> HP Device Correlation Record (Type 203)
>
> Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>
> ---
> dmioem.c | 180 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 180 insertions(+)
>
> diff --git a/dmioem.c b/dmioem.c
> index 4a7ff2b..2f38229 100644
> --- a/dmioem.c
> +++ b/dmioem.c
> @@ -204,6 +204,106 @@ static void dmi_hp_240_attr(const char *fname, u64 code)
> pr_attr(fname, "%s", attr);
> }
>
> +static void dmi_hp_203_assoc_hndl(const char *fname, u16 num)
> +{
> + char val[20];
> +
> + if (opt.flags & FLAG_QUIET)
> + return;
> +
> + if (num == 0xFFFE)
> + strcpy(val, "N/A");
> + else
> + sprintf(val, "0x%04X", num);
> + pr_attr(fname, "%s", val);
My preference (which would align this code with what is done elsewhere
in dmidecode) would be:
if (num == 0xFFFE)
pr_attr(fname, "N/A");
else
pr_attr(fname, "0x%04X", num);
This avoids the intermediate buffer and double printf.
> +}
> +
> +static void dmi_hp_203_pciinfo(const char *fname, u16 num)
> +{
> + char val[20];
> +
> + if (num == 0xFFFF)
> + strcpy(val, "Device Not Present");
> + else
> + sprintf(val, "0x%04X", num);
> + pr_attr(fname, "%s", val);
Same idea here.
> +}
> +
> +static void dmi_hp_203_bayenc(const char *fname, u8 num)
> +{
> + static char val[20];
> +
> + switch (num) {
> + case 0x00:
> + strcpy(val, "Unknown");
> + break;
> + case 0xff:
> + strcpy(val, "Do Not Display");
> + break;
> + default:
> + sprintf(val, "0x%02X", num);
> + }
> + pr_attr(fname, "%s", val);
And here. BTW is it the common practice to use hexadecimal values to
refer to enclosure numbers?
> +}
> +
> +static void dmi_hp_203_devtyp(const char *fname, unsigned int code)
> +{
> + const char *str = "Reserved";
> + static const char * const type[] = {
> + /* 0x00 */ "Unknown",
> + /* 0x01 */ "Reserved",
> + /* 0x02 */ "Reserved",
> + /* 0x03 */ "Flexible LOM",
> + /* 0x04 */ "Embedded LOM",
> + /* 0x05 */ "NIC in a Slot",
> + /* 0x06 */ "Storage Controller",
> + /* 0x07 */ "Smart Array Storage Controller",
> + /* 0x08 */ "USB Hard Disk",
> + /* 0x09 */ "Other PCI Device",
> + /* 0x0A */ "RAM Disk",
> + /* 0x0B */ "Firmware Volume",
> + /* 0x0C */ "UEFI Shell",
> + /* 0x0D */ "Generic UEFI USB Boot Entry",
> + /* 0x0E */ "Dynamic Smart Array Controller",
> + /* 0x0F */ "File",
> + /* 0x10 */ "NVME Hard Drive",
> + /* 0x11 */ "NVDIMM"
> + };
> +
> + if (code < ARRAY_SIZE(type))
> + str = type[code];
> +
> + pr_attr(fname, "%s", str);
> +}
> +
> +static void dmi_hp_203_devloc(const char *fname, unsigned int code)
> +{
> + static const char *str = "Reserved";
> + static const char * const location[] = {
> + /* 0x00 */ "Unknown",
> + /* 0x01 */ "Embedded",
> + /* 0x02 */ "iLO Virtual Media",
> + /* 0x03 */ "Front USB Port",
> + /* 0x04 */ "Rear USB Port",
> + /* 0x05 */ "Internal USB",
> + /* 0x06 */ "Internal SD Card",
> + /* 0x07 */ "Internal Virutal USB (Embedded NAND)",
> + /* 0x08 */ "Embedded SATA Port",
> + /* 0x09 */ "Embedded Smart Array",
> + /* 0x0A */ "PCI Slot",
> + /* 0x0B */ "RAM Memory",
> + /* 0x0C */ "USB",
> + /* 0x0D */ "Dynamic Smart Array Controller",
> + /* 0x0E */ "URL",
> + /* 0x0F */ "NVMe Drive Bay",
> + };
> +
> + if (code < ARRAY_SIZE(location))
> + str = location[code];
> +
> + pr_attr(fname, "%s", str);
> +}
> +
> static int dmi_decode_hp(const struct dmi_header *h)
> {
> u8 *data = h->data;
> @@ -218,6 +318,86 @@ static int dmi_decode_hp(const struct dmi_header *h)
>
> switch (h->type)
> {
> + case 203:
> + /*
> + * Vendor Specific: HP Device Correlation Record
> + *
> + * Offset | Name | Width | Description
> + * -------------------------------------
> + * 0x00 | Type | BYTE | 0xCB, Correlation
> Record
> + * 0x01 | Length | BYTE | Length of structure
> + * 0x02 | Handle | WORD | Unique handle
> + * 0x04 | Assoc Device | WORD | Handle of Associated
> Type 9 or Type 41 Record
> + * 0x06 | Assoc SMBus | WORD | Handle of Associated
> Type 228 SMBus Segment Record
> + * 0x08 | PCI Vendor ID| WORD | PCI Vendor ID of
> device 0xFFFF -> not present.
> + * 0x0A | PCI Device ID| WORD | PCI Device ID of
> device 0xFFFF -> not present.
> + * 0x0C | PCI SubVendor| WORD | PCI Sub Vendor ID of
> device 0xFFFF -> not present.
> + * 0x0E | PCI SubDevice| WORD | PCI Sub Device ID of
> device 0xFFFF -> not present.
> + * 0x10 | Class Code | BYTE | PCI Class Code of
> Endpoint. 0xFF if device not present.
> + * 0x11 | Class SubCode| BYTE | PCI Sub Class Code
> of Endpoint. 0xFF if device not present.
> + * 0x12 | Parent Handle| WORD |
> + * 0x14 | Flags | WORD |
> + * 0x16 | Device Type | BYTE | UEFI only.
> + * 0x17 | Device Loc | BYTE | Device Location.
> + * 0x18 | Dev Instance | BYTE | Device Instance
> + * 0x19 | Sub Instance | BYTE | NIC Port # or NVMe
> Drive Bay
> + * 0x1A | Bay | BYTE |
> + * 0x1B | Enclosure | BYTE |
> + * 0x1C | UEFI Dev Path| STRING| String number for
> UEFI Device Path
> + * 0x1D | Struct Name | STRING| String number for
> UEFI Device Structured Name
> + * 0x1E | Device Name | STRING| String number for
> UEFI Device Name
> + * 0x1F | UEFI Location| STRING| String number for
> UEFI Location
> + * 0x20 | Assoc Handle | WORD | Type 9 Handle.
> Defined if Flags[0] == 1.
> + * 0x22 | Part Number | STRING| PCI Device Part
> Number
> + * 0x23 | Serial Number| STRING| PCI Device Serial
> Number
> + * 0x24 | Seg Number | WORD | Segment Group
> number. 0 -> Single group topology
> + * 0x26 | Bus Number | BYTE | PCI Device Bus Number
> + * 0x27 | Func Number | BTYE | PCI Device and
> Function Number
> + */
> + if (gen < G9) break;
> + if (h->length < 0x1F) break;
> + pr_handle_name("%s HP Device Correlation Record",
> company);
> + dmi_hp_203_assoc_hndl("Associated Device Record",
> WORD(data + 0x04));
> + dmi_hp_203_assoc_hndl("Associated SMBus Record",
> WORD(data + 0x08));
I think you mean data + 0x06 here?
> + if (WORD(data + 0x08) == 0xffff && WORD(data + 0x0A) ==
> 0xffff &&
> + WORD(data + 0x0C) == 0xffff && WORD(data + 0x0E) ==
> 0xffff &&
> + data[0x10] == 0xFF && data[0x11] == 0xFF) {
> + pr_attr("PCI Device Info", "%s", "Device Not
> Present");
You can skip the %s.
> + } else {
> + dmi_hp_203_pciinfo("PCI Vendor ID", WORD(data +
> 0x08));
> + dmi_hp_203_pciinfo("PCI Device ID", WORD(data +
> 0x0A));
> + dmi_hp_203_pciinfo("PCI Sub Vendor ID",
> WORD(data + 0x0C));
> + dmi_hp_203_pciinfo("PCI Sub Device ID",
> WORD(data + 0x0E));
> + dmi_hp_203_pciinfo("PCI Class Code",
> (char)data[0x10]);
> + dmi_hp_203_pciinfo("PCI Sub Class Code",
> (char)data[0x11]);
I took a quick look at the PCI spec out of curiosity. It turns our that
PCI Class Code 0xFF is legal, it means "Device does not fit in any
defined classes". Therefore you shouldn't special-case it.
I suppose the same holds for the PCI Sub Class Code, because its
meaning depends on the Class Code, and while there is no class using
0xFF as a sub class today, there's also nothing preventing it from
happening in the future.
So I think both should be printed with a straight pr_attr().
> + }
> + dmi_hp_203_assoc_hndl("Parent Handle", WORD(data +
> 0x12));
> + pr_attr("Flags", "0x%04X", WORD(data + 0x14));
> + dmi_hp_203_devtyp("Device Type", data[0x16]);
> + dmi_hp_203_devloc("Device Location", data[0x17]);
> + pr_attr("Device Instance", "0x%02X", data[0x18]);
> + pr_attr("Device Sub-Instance", "0x%02X", data[0x19]);
Is hexadecimal appropriate/common to designate instance numbers?
> + dmi_hp_203_bayenc("Bay", data[0x1A]);
> + dmi_hp_203_bayenc("Enclosure", data[0x1B]);
> + pr_attr("Device Path", "%s", dmi_string(h, data[0x1C]));
> + pr_attr("Structured Name", "%s", dmi_string(h,
> data[0x1D]));
> + pr_attr("Device Name", "%s", dmi_string(h, data[0x1E]));
> + if (h->length < 0x24) break;
I think you said you would test for length < 0x22 too?
> + pr_attr("UEFI Location", "%s", dmi_string(h,
> data[0x1F]));
> + if (!(opt.flags & FLAG_QUIET)) {
> + if (WORD(data + 0x14) & 1)
> + pr_attr("Associated Real/Phys Handle",
> "0x%04X", WORD(data + 0x20));
> + else
> + pr_attr("Associated Real/Phys Handle",
> "N/A");
> + }
> + pr_attr("PCI Part Number", "%s", dmi_string(h,
> data[0x22]));
> + pr_attr("Serial Number", "%s", dmi_string(h,
> data[0x23]));
> + if (h->length < 0x28) break;
> + pr_attr("Segment Group Number", "0x%04X", WORD(data +
> 0x24));
> + pr_attr("PCI Device", "%02X:%02X.%X",
> + data[0x26], data[0x27] >> 3, data[0x27]
> & 7);
Yes, that's what I had in mind. Except that I would use %x instead of %X
so that lower-case letters are used, same as lspci is using (and also
what we do in function dmi_print_hp_net_iface_rec).
> + break;
> +
> case 204:
> /*
> * Vendor Specific: HPE ProLiant System/Rack Locator
Thanks,
--
Jean Delvare
SUSE L3 Support
[dmidecode] [PATCH v2 4/4] dmioem: Fix HPE OEM 236, Jerry Hoemann, 2021/01/12