[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 2/2] dmioem: Decode HPE OEM Record 239
|
From: |
Jerry Hoemann |
|
Subject: |
Re: [PATCH 2/2] dmioem: Decode HPE OEM Record 239 |
|
Date: |
Wed, 29 Mar 2023 12:52:37 -0600 |
|
User-agent: |
Mutt/1.10.1 (2018-07-13) |
On Wed, Mar 22, 2023 at 12:02:57PM +0100, Jean Delvare wrote:
> 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.
done.
>
> > + "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.
Done.
>
> > + } 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).
kept switched, assigned to str as above.
>
> > + } 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.
I believe you want:
if (foo)
{
....
}
else
{
....
}
done.
>
> > +}
> > +
> > 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".
done.
>
> > + * 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.
made all offsets for case 239 be two hex digits which is generally what
the remainder of the function and its helpers do, although that is
not universal.
let me know if you want me to clean up other usages in a subsequent
patch.
>
> The format should be "0x%04x" (lower-case x) to be consistent with all
> other IDs printed.
done.
>
> Also for consistency, I'd use "USB Vendor ID" as the attribute name.
done.
>
> > + 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.
done.
>
> > + 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
--
-----------------------------------------------------------------------------
Jerry Hoemann Software Engineer Hewlett Packard Enterprise
-----------------------------------------------------------------------------