[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [dmidecode] [PATCH 2/2] dmioem: Decode HPE OEM Record 203
|
From: |
Jerry Hoemann |
|
Subject: |
Re: [dmidecode] [PATCH 2/2] dmioem: Decode HPE OEM Record 203 |
|
Date: |
Tue, 12 Jan 2021 00:51:44 -0700 |
On Tue, Jan 05, 2021 at 04:31:54PM +0100, Jean Delvare wrote:
> Hi Jerry,
>
> On Wed, 16 Dec 2020 14:18:59 -0700, Jerry Hoemann wrote:
> > HP Device Correlation Record (Type 203)
> >
> > Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>
> > ---
> > dmioem.c | 153 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 153 insertions(+)
> >
> > diff --git a/dmioem.c b/dmioem.c
> > index d8cab2c..7983664 100644
> > --- a/dmioem.c
> > +++ b/dmioem.c
> > @@ -186,6 +186,87 @@ static int dmi_hpegen(const char *s)
> > return (dmi_vendor == VENDOR_HPE) ? G10P : G6;
> > }
> >
> > +static inline const char * dmi_hp_203_assochd(u16 num)
>
> No "inline" please. They cause portability issues, and the compiler
> will generally know better anyway.
>
done.
> I see why you would want to force inlining (local static buffer) but
> that should not be a problem in practice given the call pattern.
>
> That being said, a different solution to the problem would be to do the
> same as is done in various places in dmidecode.c: have the function
> print the attribute, instead of return a string. See
> dmi_chassis_height() for a simple example of that.
converted all the helper functions to actually do the printing.
>
> Coding style: no space between * and the function name.
done
>
> > +{
> > + static char val[20];
> > + if (num == 0xFFFE)
> > + return "N/A";
> > + sprintf (val, "0x%04x", num);
> > + return val;
> > +}
> > +
> > +static inline const char * dmi_hp_203_pciinfo(u16 num)
> > +{
> > + static char val[20];
> > + if (num == 0xFFFF)
> > + return "Device Not Present";
> > + sprintf (val, "0x%04x", num);
> > + return val;
> > +}
> > +
> > +static inline const char * dmi_hp_203_bayenc(u8 num)
> > +{
> > + static char val[20];
> > + switch (num) {
> > + case 0x00: return "Unknown";
> > + case 0xff: return "Do Not Display";
> > + default: sprintf (val, "0x%04x", num);
>
> Coding style mandates indentation inside the switch.
scripts/checkpatch.pl flags having "case" indented relative to "switch".
checkpatch.pl doesn't like statments on same line as case label.
I updated the function to be clean relative to checkpatch.pl.
>
> > + }
> > + return val;
> > +}
> > +
> > +static inline const char * dmi_hp_203_devtyp(unsigned int type)
> > +{
> > + static char reserved[20];
> > + switch(type) {
> > + case 0x00: return "Unknown";
> > + case 0x03: return "Flexible LOM";
> > + case 0x04: return "Embedded LOM";
> > + case 0x05: return "NIC in a Slot";
> > + case 0x06: return "Storage Controller";
> > + case 0x07: return "Smart Array Storage Controller";
> > + case 0x08: return "USB Hard Disk";
> > + case 0x09: return "Other PCI Device";
> > + case 0x0A: return "RAM Disk";
> > + case 0x0B: return "Firmware Volume";
> > + case 0x0C: return "UEFI Shell";
> > + case 0x0D: return "Generic UEFI USB Boot Entry";
> > + case 0x0E: return "Dynamic Smart Array Controller";
> > + case 0x0F: return "File";
> > + case 0x10: return "NVME Hard Drive";
> > + case 0x11: return "NVDIMM";
>
> This is really calling for a lookup on a static data array. This is how
> we deal with such cases in dmidecode.c. See dmi_base_board_type() for a
> simple example of that.
>
> > + default:
> > + sprintf (reserved, "Reserved - 0x%04x", type);
>
> If it's reserved, it's reserved, don't bother printing the hexadecimal
> value. If anyone really cares, the information is always available with
> dmidecode -u.
Converted to array of char *.
We shouldn't see any reserved values, it would indicate an error.
Two causes that come to mind are firmware filling in garbage in the
table, or dmidecode being out of date.
>
> > + return reserved;
> > + }
> > +}
> > +
> > +static inline const char * dmi_hp_203_devloc(unsigned int location)
> > +{
> > + static char reserved[20];
> > + switch(location) {
> > + case 0x00: return "Unknown";
> > + case 0x01: return "Embedded";
> > + case 0x02: return "iLO Virtual Media";
> > + case 0x03: return "Front USB Port";
> > + case 0x04: return "Rear USB Port";
> > + case 0x05: return "Internal USB";
> > + case 0x06: return "Internal SD Card";
> > + case 0x07: return "Internal Virutal USB (Embedded NAND)";
> > + case 0x08: return "Embedded SATA Port";
> > + case 0x09: return "Embedded Smart Array";
> > + case 0x0A: return "PCI Slot";
> > + case 0x0B: return "RAM Memory";
> > + case 0x0C: return "USB";
> > + case 0x0D: return "Dynamic Smart Array Controller";
> > + case 0x0E: return "URL";
> > + case 0x0F: return "NVMe Drive Bay";
> > + default:
> > + sprintf (reserved, "Reserved - 0x%04x", location);
> > + return reserved;
> > + }
> > +}
converted to array of char *.
> > +
> > static int dmi_decode_hp(const struct dmi_header *h)
> > {
> > u8 *data = h->data;
> > @@ -200,6 +281,78 @@ 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
>
> Left-align "Type" like all other names.
done
>
> > + * 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 |
>
> No trailing white-space - else both quilt and git will complain.
>
done
> > + * 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 of
> > UEFI Device Path
> > + * 0x1D | Struct Name | STRING| String numer for
> > UERI Device Structured Name
>
> Is UERI a new standard I'm not aware of, or a typo?
typo followed by multiple cut/paste.
fixed.
>
> "numer" has to be a typo ;-)
>
> > + * 0x1E | Device Name | STRING| String numer for
> > UERI Device Name
> > + * 0x1F | UEFI Location| STRING| String numer for
> > UERI Location
> > + * 0x20 | Assoc Handle | WORD | Type 9 Handle.
> > Defined if Flags[0] == 1.
>
> Confused, how is it different from "Assoc Device" defined at offset 0x04?
It's complicated involving bifurcated virtual functions. The explanation is
about a page long in the document and none of the systems I have access to
actually implements this feature.
Unfortunately, the firmware team doesn't want to publish the documentation.
So, I'm a bit stuck. I've updated the output a bit, but not going
be able to do much better.
>
> > + * 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);
> > + pr_attr("Associated Device Rec ", "%s",
> > dmi_hp_203_assochd(WORD(data + 0x04)));
>
> Same comment as previous patch, don't align attribute names nor source
> code.
done.
>
> > + pr_attr("Associated SMBus Rec ", "%s",
> > dmi_hp_203_assochd(WORD(data + 0x06)));
> > + pr_attr("PCI Vendor ID ", "%s",
> > dmi_hp_203_pciinfo(WORD(data + 0x08)));
> > + pr_attr("PCI Device ID ", "%s",
> > dmi_hp_203_pciinfo(WORD(data + 0x0A)));
> > + pr_attr("PCI Sub Vendor ID ", "%s",
> > dmi_hp_203_pciinfo(WORD(data + 0x0C)));
> > + pr_attr("PCI Sub Device ID ", "%s",
> > dmi_hp_203_pciinfo(WORD(data + 0x0E)));
> > + pr_attr("PCI Class Code ", "%s",
> > dmi_hp_203_pciinfo((char)data[0x10]));
> > + pr_attr("PCI Sub Class Code ", "%s",
> > dmi_hp_203_pciinfo((char)data[0x11]));
>
> It is weird to print an 8-bit value with a "0x%04x" format.
>
> Also I would assume that either all PCI fields are set, or none is set.
One would hope.
> I'm not sure it makes a lot of sense to repeat "Device Not Present" 6
> times for the same device. Maybe we could check for PCI Vendor ID ==
> 0xFFFF and simply skip all lines if no device is present?
I'll check if all are set to all Fs and if so, do one print accordingly.
Else I'll print out each individually.
This will cover the wonky case where some are set and some aren't.
Don't think we should see that case, but if we do, the decoding
will handle it.
>
> > + pr_attr("Parent Handle ", "%s",
> > dmi_hp_203_assochd(WORD(data + 0x12)));
> > + pr_attr("Flags ", "0x%04X",
> > WORD(data + 0x14));
> > + pr_attr("Device Type ", "%s",
> > dmi_hp_203_devtyp(data[0x16]));
> > + pr_attr("Device Location ", "%s",
> > dmi_hp_203_devloc(data[0x17]));
> > + pr_attr("Device Instance ", "0x%02X",
> > data[0x18]);
> > + pr_attr("Device Sub-Instance ", "0x%02X",
> > data[0x19]);
> > + pr_attr("Bay ", "%s",
> > dmi_hp_203_bayenc(data[0x1A]));
> > + pr_attr("Enclosure ", "%s",
> > dmi_hp_203_bayenc(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;
>
> If the last byte you need to be present is at offset 0x22 then the test
> should be h->length < 0x23? Or if this length check is correct, then
> it's the next ont which is misplaced.
Lengths for this record that I have found are:
type 203, 31 bytes == 0x1f bytes
type 203, 34 bytes == 0x22 bytes
type 203, 36 bytes == 0x24 bytes
type 203, 40 bytes == 0x28 bytes
So the length check for 0x24 i believe is correct. The mistake is where
I put the length check of 0x28. It should be after the printing of Serial
Number.
Fixed.
>
> > + pr_attr("UEFI Location ", "%s",
> > dmi_string(h, data[0x1F]));
> > + if (WORD(data + 0x14) & 1)
> > + pr_attr("Associate Handle ", "0x%04X",
> > WORD(data + 0x20));
> > + else
> > + pr_attr("Associate Handle ", "N/A");
>
> "Associated Handle"?
done.
>
> > + pr_attr("PCI Part Number ", "%s",
> > dmi_string(h, data[0x22]));
> > + if (h->length < 0x28) break;
> > + pr_attr("Serial Number ", "%s",
> > dmi_string(h, data[0x23]));
> > + pr_attr("Segment Group Number ", "0x%04X",
> > WORD(data + 0x24));
> > + pr_attr("PCI BUS Number ", "0x%02X",
> > data[0x26]);
> > + pr_attr("PCI Function Number ", "0x%02X",
> > data[0x27]);
>
> The last field is the combination of device and function numbers, not
> just function number. But wouldn't it be more convenient for the user
> to present these 3 fields in the traditional PCI group:bus:dev.fn
> format? That's what we do for HP OEM types 221 and 233:
>
> pr_attr(attr, "PCI device %02x:%02x.%x, ",
> (...)
> bus, dev >> 3, dev & 7,
> (...)
I think I updated to what you want here, but not sure. please double check.
>
> > + break;
> > +
> > case 204:
> > /*
> > * Vendor Specific: HPE ProLiant System/Rack Locator
>
> Thanks,
> --
> Jean Delvare
> SUSE L3 Support
>
> _______________________________________________
> https://lists.nongnu.org/mailman/listinfo/dmidecode-devel
--
-----------------------------------------------------------------------------
Jerry Hoemann Software Engineer Hewlett Packard Enterprise
-----------------------------------------------------------------------------