[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: |
Jerry Hoemann |
|
Subject: |
Re: [dmidecode] [PATCH v2 3/4] dmioem: Decode HPE OEM Record 203 |
|
Date: |
Tue, 12 Jan 2021 11:35:44 -0700 |
On Tue, Jan 12, 2021 at 06:01:43PM +0100, Jean Delvare wrote:
> 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.
Yes. This is better. I will fix.
>
> > +}
> > +
> > +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.
Ditto.
>
> > +}
> > +
> > +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?
Good point. Prior change + decimal for this one.
>
> > +}
> > +
> > +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?
Good catch. Will fix.
>
> > + 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.
Will fix.
>
> > + } 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().
The documentation for the OEM 203 record states:
PCI Class Code of Endpoint. 0xFF if device not present.
Similar statement for SubClass.
So while what you say above may be true in general for PCI [Sub] Class,
it doesn't appear to be how HPE firmware is handling this field.
>
> > + }
> > + 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?
I can change to decimal.
>
> > + 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?
Yes, I think above is overly conservative and will not decode some fields
on some system. I'll look into more.
>
> > + 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).
Do you in general have a preference on the capitialization of hex numbers?
My prior submit was inconsistent, so I made version 2 of the patches to
use capital X for hex formatting which seems to be more prevalent in
other parts of dmidecode.
Or is PCI device like above an exception?
>
> > + break;
> > +
> > case 204:
> > /*
> > * Vendor Specific: HPE ProLiant System/Rack Locator
>
> Thanks,
> --
> Jean Delvare
> SUSE L3 Support
--
-----------------------------------------------------------------------------
Jerry Hoemann Software Engineer Hewlett Packard Enterprise
-----------------------------------------------------------------------------
[dmidecode] [PATCH v2 4/4] dmioem: Fix HPE OEM 236, Jerry Hoemann, 2021/01/12