Re: [dmidecode] [PATCH 2/2] dmioem: Decode HPE OEM Record 203

[Jean Delvare]

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