Re: [dmidecode] [PATCH] dmioem: Decode HPE OEM Record 238

[Jean Delvare]

Hi Jerry,

On Wed, 25 May 2022 16:35:42 -0600, Jerry Hoemann wrote:
> Decode HPE OEM Record 238: USB Port Connector Correlation Record.
> 
> Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>
> ---
>  dmioem.c | 89 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 89 insertions(+)
> 
> diff --git a/dmioem.c b/dmioem.c
> index 61569a6..e42a35f 100644
> --- a/dmioem.c
> +++ b/dmioem.c
> @@ -299,6 +299,57 @@ static void dmi_hp_203_devloc(const char *fname, 
> unsigned int code)
>       pr_attr(fname, "%s", str);
>  }
>  
> +static void dmi_hp_238_loc(const char *fname, unsigned int code)
> +{
> +     static const char *str = "Reserved";
> +     static const char *location[] = {
> +             "Internal", /* 0x00 */
> +             "Front of Server",
> +             "Rear of Server",
> +             "Embedded internal SD Card",
> +             "iLO USB",
> +             "HP NAND Controller (USX 2065 or other)",
> +             "Reserved",
> +             "Debug Port", /* 0x07 */
> +     };
> +
> +     if (code < ARRAY_SIZE(location))
> +             str = location[code];
> +
> +     pr_attr(fname, "%s", str);
> +}
> +
> +static void dmi_hp_238_flags(const char *fname, unsigned int code)
> +{
> +     static const char *str = "Reserved";
> +     static const char *flags[] = {
> +             "Not Shared", /* 0x00 */
> +             "Shared with physical switch",
> +             "Shared with automatic control", /* 0x03 */

Count is not correct. Either "Not Shared" is actually 0x01, or "Shared
with automatic control" is actually °0x02, or you missed an enumerated
value.

> +     };
> +
> +     if (code < ARRAY_SIZE(flags))
> +             str = flags[code];
> +
> +     pr_attr(fname, "%s", str);
> +}
> +
> +static void dmi_hp_238_speed(const char *fname, unsigned int code)
> +{
> +     static const char *str = "Reserved";
> +     static const char *speed[] = {
> +             "Reserved", /* 0x00 */
> +             "USB 1.1 Full Speed",
> +             "USB 2.0 High Speed",
> +             "USB 3.0 Super Speed" /* 0x03 */
> +     };
> +
> +     if (code < ARRAY_SIZE(speed))
> +             str = speed[code];
> +
> +     pr_attr(fname, "%s", str);
> +}
> +
>  static int dmi_decode_hp(const struct dmi_header *h)
>  {
>       u8 *data = h->data;
> @@ -591,6 +642,44 @@ static int dmi_decode_hp(const struct dmi_header *h)
>                       }
>                       break;
>  
> +             case 238:
> +                     /*
> +                      * Vendor Specific: HPE USB Port Connector Correlation 
> Record
> +                      *
> +                      * Offset |  Name      | Width | Description
> +                      * ---------------------------------------
> +                      *  0x00  | Type       | BYTE  | 0xEE, HDD Backplane

"HDD Backplane" seems wrong, bad copy-and-paste?

> +                      *  0x01  | Length     | BYTE  | Length of structure
> +                      *  0x02  | Handle     | WORD  | Unique handle
> +                      *  0x04  | Hand Assoc | WORD  | Handle to map to Type 8
> +                      *  0x06  | Parent Bus | BYTE  | PCI Bus number of USB 
> controller of this port.

For consistency, please omit the trailing dot here and below.

> +                      *  0x07  | Par Dev/Fun| BYTE  | PCI Dev/Fun of USB 
> Controller of this port.
> +                      *  0x08  | Location   | BYTE  | Enumerated value of 
> location of USB port.
> +                      *  0x09  | Flags      | WORD  | USB Shared Management 
> Port
> +                      *  0x0B  | Port Inst  | BYTE  | Instance number of for 
> this type of USB port.

Stray word ("of" or "for", you decide which).

> +                      *  0x0C  | Parent HUB | BYTE  | Instance number of 
> internal HuB

Should be spelled "Hub" both times.

> +                      *  0x0D  | Port Speed | BYTE  | Enumerated value of 
> speed confiured by BIOS.

Typo: configured

> +                      *  0x0E  | Device Path| STRING| UEFI Device Path of 
> USB endpoint.
> +                      */
> +                     if (gen < G9) return 0;
> +                     pr_handle_name("%s Proliant USB Port Connector 
> Correlation Record", company);
> +                     if (h->length < 0x0F) break;
> +                     if (!(opt.flags & FLAG_QUIET))
> +                             pr_attr("Associated Handle", "0x%04X", 
> WORD(data + 0x4));
> +                     pr_attr("PCI Bus of Parent USB", "0x%04X", data[0x6]);
> +                     pr_attr("PCI Device of Parent USB", "0x%04X", data[0x7] 
> >> 3);
> +                     pr_attr("PCI Function of Parent USB", "0x%04X", 
> data[0x7] & 0x7);
> +                     dmi_hp_238_loc("Location", data[0x8]);
> +                     dmi_hp_238_flags("Management port", data[0x8]);

According to the documentation above, I believe this should actually be:

                        dmi_hp_238_flags("Management port", WORD(data + 0x9));

> +                     pr_attr("Port Instance", "0x%04X", data[0xB]);

Is it really customary to use hexadecimal for an instance number? I
would have expected decimal instead. Decimal would be consistent with
what we did for HP(E) type 203's Device Instance field.

> +                     if ( data[0xC]  != 0xFE )

Coding style: no spaces inside the parentheses. You also have a
duplicate space before "!=".

> +                             pr_attr("Parent HUB Port Instance", "0x%04X", 
> data[0xC]);
> +                     else
> +                             pr_attr("No Parent HUB", "");

I don't like the idea of changing the attribute name depending on some
condition. Also, using an empty string as the attribute value is not
something we have done before, and it won't look good (the trailing ":"
will be preserved).

So I'd rather go for something like:

                        if (data[0xC] != 0xFE)
                                pr_attr("Parent HUB Port Instance", "%d", 
data[0xC]);
                        else
                                pr_attr("Parent HUB Port Instance", "N/A");

This would be consistent with how we handled the "Associated Real/Phys
Handle" field in type 203. Would that be OK for you?

> +                     dmi_hp_238_speed("Port Speed Capability", data[0xD]);
> +                     pr_attr("Device Path", "%s", dmi_string(h, data[0xE]));
> +                     break;
> +
>               case 240:
>                       /*
>                        * Vendor Specific: HPE Proliant Inventory Record

Rest looks good, thanks.

-- 
Jean Delvare
SUSE L3 Support