[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [dmidecode] [PATCH 2/2] dmioem: Decode HPE OEM Record 203
From: |
Jean Delvare |
Subject: |
Re: [dmidecode] [PATCH 2/2] dmioem: Decode HPE OEM Record 203 |
Date: |
Tue, 5 Jan 2021 16:31:54 +0100 |
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.
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.
Coding style: no space between * and the function name.
> +{
> + 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.
> + }
> + 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.
> + 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;
> + }
> +}
> +
> 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.
> + * 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.
> + * 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?
"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?
> + * 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.
> + 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.
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?
> + 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.
> + 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"?
> + 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,
(...)
> + break;
> +
> case 204:
> /*
> * Vendor Specific: HPE ProLiant System/Rack Locator
Thanks,
--
Jean Delvare
SUSE L3 Support
- Re: [dmidecode] [PATCH 2/2] dmioem: Decode HPE OEM Record 203,
Jean Delvare <=