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

[Jean Delvare]

Hi Jerry,

On Mon, 13 Feb 2023 18:25:26 -0700, Jerry Hoemann wrote:
> Decode HPE OEM Record 239: HP USB Device Correlation Record
> 
> Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>
> ---
>  dmioem.c | 117 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 117 insertions(+)
> 
> diff --git a/dmioem.c b/dmioem.c
> index f3243da..25b3ea6 100644
> --- a/dmioem.c
> +++ b/dmioem.c
> @@ -620,6 +620,75 @@ static void dmi_hp_238_speed(const char *fname, unsigned 
> int code)
>       pr_attr(fname, "%s", str);
>  }
>  
> +static void dmi_hp_239_usb_device(u8 class, u8 subclass, u8 protocol)
> +{
> +     /* https://www.usb.org/defined-class-codes */
> +     /* 
> https://www.usb.org/sites/default/files/Mass_Storage_Specification_Overview_v1.4_2-19-2010.pdf
>  */
> +     if (class == 0x08) {
> +             const char *str = "Reserved";
> +             static const char * const SubClassName[] = {

Please use only lower-case letters for variable names as is done
everywhere else. Use underscores as separators if needed.

> +                     "SCSI command set not reported", /* 0x00 */
> +                     "RBC",
> +                     "ATAPI",
> +                     "Obsolete",
> +                     "UFI",
> +                     "Obsolete",
> +                     "SCSI",
> +                     "LSD FS",
> +                     "IEEE 1667" /* 0x08 */
> +             };
> +             pr_attr("USB Class", "%s", "Mass Storage");
> +             if (subclass == 0xFF) {
> +                     str = "Vendor Specific";
> +             } else if (subclass < ARRAY_SIZE(SubClassName)) {
> +                     str = SubClassName[subclass];
> +             }
> +             pr_attr("USB SubClass", "%s", str);
> +
> +             switch (protocol) {
> +             case 0x00:
> +                     pr_attr("USB Protocol", "%s", "CBI w/ completion 
> interrupt");
> +                     break;
> +             case 0x01:
> +                     pr_attr("USB Protocol", "%s", "CBI w/o completion 
> interrupt");
> +                     break;
> +             case 0x02:
> +                     pr_attr("USB Protocol", "%s", "Obsolete");
> +                     break;
> +             case 0x50:
> +                     pr_attr("USB Protocol", "%s", "Bulk-Only");
> +                     break;
> +             case 0x62:
> +                     pr_attr("USB Protocol", "%s", "UAS");
> +                     break;
> +             case 0xFF:
> +                     pr_attr("USB Protocol", "%s", "Vendor Specific");
> +                     break;
> +             default:
> +                     pr_attr("USB Protocol", "%s", "Reserved");
> +             }

That's a lot of redundant code. What about setting str to the name
string in the switch, and calling pr_attr(..., str) only once after the
switch? This would also be similar to how you handle "USB SubClass"
above.

> +     } else if (class == 0x09 && subclass == 0) {
> +             pr_attr("USB Class", "%s", "HUB");
> +             switch (protocol) {
> +             case 0:
> +                     pr_attr("USB Protocol", "%s", "Full Speed");
> +                     break;
> +             case 1:
> +                     pr_attr("USB Protocol", "%s", "Hi-Speed w/ single TT");
> +                     break;
> +             case 2:
> +                     pr_attr("USB Protocol", "%s", "Hi-Speed w/ multiple 
> TT");
> +                     break;
> +             default:
> +                     pr_attr("USB Protocol", "%s", "Reserved");
> +             }

Same could be done here (or even use an array instead of the switch).

> +     } else {
> +             pr_attr("USB Class", "0x%02x", class);
> +             pr_attr("USB SubClass", "0x%02x", subclass);
> +             pr_attr("USB Protocol", "0x%02x", protocol);
> +     }

Also please use the same curly brace placement in this new function as
is done in the rest of the code. I know it's not the most popular style
these days but consistency is important to me.

> +}
> +
>  static void dmi_hp_242_hdd_type(u8 code)
>  {
>       const char *str = "Reserved";
> @@ -1147,6 +1216,54 @@ static int dmi_decode_hp(const struct dmi_header *h)
>                       pr_attr("Device Path", "%s", dmi_string(h, data[0xE]));
>                       break;
>  
> +             case 239:
> +                     /*
> +                      * Vendor Specific: HPE USB Device Correlation Record
> +                      *
> +                      * This record provides a mechanism for software to 
> correlate USB device
> +                      * information provided in SMBIOS record Type 8 and 
> Type 238. It
> +                      * additionally provides device specific data that is 
> typically not
> +                      * available in SMBIOS to allow HP tools to understand 
> how these device
> +                      * entries correlate to both UEFI and Legacy USB Boot 
> entries. This record
> +                      * will only contain information for a device detected 
> by the BIOS during
> +                      * POST and does not comprehend a hot plug event after 
> the system has
> +                      * booted. This record will only be supported on UEFI 
> Based systems.
> +                      *
> +                      * Offset |  Name      | Width | Description
> +                      * -------+------------+-------+------------
> +                      *  0x00  | Type       | BYTE  | 0xEF, HP Device 
> Correlation Record
> +                      *  0x01  | Length     | BYTE  | Length of structure
> +                      *  0x02  | Handle     | WORD  | Unique handle
> +                      *  0x04  | Hand Assoc | WORD  | Handle to map to Type 
> 238
> +                      *  0x06  | Vendor ID  | WORD  | Vendor ID of detected 
> USB Device
> +                      *  0x08  | FALGS      | WORD  | Bit[0] - Indicates 
> presence of SD card

You most certainly meant "Flags".

> +                      *  0x0A  | Class      | BYTE  | USB Device Class per 
> USB HID Dev Spec
> +                      *  0x0B  | Sub Class  | BYTE  | USB Device SubClass 
> per USB HID Dev Spec
> +                      *  0x0C  | Protocol   | BYTE  | Device Protocol per 
> USB HID Dev Spec
> +                      *  0x0D  | Product ID | WORD  | USB Product ID
> +                      *  0x0F  | Capacity   | DWORD | USB Device Capacity 
> (if apropos) in Mbytes
> +                      *  0x13  | Device Path| STRING| UEFI Device Path
> +                      *  0x14  | Device Name| STRING| UEFI Device Structured 
> Name
> +                      *  0x15  | UEFI Name  | STRING| Device Name
> +                      *  0x16  | Location   | STRING| USB Device Location
> +                      */
> +                     if (gen < G9) return 0;
> +                     pr_handle_name("%s USB Device Correlation Record", 
> company);
> +                     if (h->length < 0x17) break;
> +                     if (!(opt.flags & FLAG_QUIET))
> +                             pr_attr("Associated Handle", "0x%04X", 
> WORD(data + 0x4));
> +                     pr_attr("Vendor ID", "0x%04X", WORD(data + 0x6));

Please be consistent with hexadecimal offsets. Up to this point, you
don't pad them to 2 hex digits, but starting with the next line, you do.

The format should be "0x%04x" (lower-case x) to be consistent with all
other IDs printed.

Also for consistency, I'd use "USB Vendor ID" as the attribute name.

> +                     pr_attr("Embedded SD Card", "%s", data[0x08] & 0x01 ? 
> "Present" : "Empty");
> +                     dmi_hp_239_usb_device(data[0x0A], data[0x0B], 
> data[0x0C]);
> +                     pr_attr("USB Product ID", "0x%04x", WORD(data + 0x0D));
> +                     if (DWORD(data + 0x0F))
> +                             pr_attr("USB Capacity", "%d MB", DWORD(data + 
> 0x0F));

%u would be more appropriate.

> +                     pr_attr("UEFI Device Path", "%s", dmi_string(h, 
> data[0x13]));
> +                     pr_attr("UEFI Device Name", "%s", dmi_string(h, 
> data[0x14]));
> +                     pr_attr("Device Name", "%s", dmi_string(h, data[0x15]));
> +                     pr_attr("Device Location", "%s", dmi_string(h, 
> data[0x16]));
> +                     break;
> +
>               case 240:
>                       /*
>                        * Vendor Specific: HPE Proliant Inventory Record

Thanks,
-- 
Jean Delvare
SUSE L3 Support