[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [dmidecode] [PATCH] dmioem: Decode HPE OEM Record 238
|
From: |
Jerry Hoemann |
|
Subject: |
Re: [dmidecode] [PATCH] dmioem: Decode HPE OEM Record 238 |
|
Date: |
Mon, 30 May 2022 10:37:30 -0600 |
|
User-agent: |
Mutt/1.10.1 (2018-07-13) |
On Mon, May 30, 2022 at 03:00:45PM +0200, Jean Delvare wrote:
> 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.
Fixed. Comment should have been /* 0x02 */
>
> > + };
> > +
> > + 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?
Fixed. cut/paste error. Should have been: HP Device Correlation Record
>
> > + * 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.
Done.
>
> > + * 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).
Fixed.
>
> > + * 0x0C | Parent HUB | BYTE | Instance number of
> > internal HuB
>
> Should be spelled "Hub" both times.
Fixed.
>
> > + * 0x0D | Port Speed | BYTE | Enumerated value of
> > speed confiured by BIOS.
>
> Typo: configured
Fixed.
>
> > + * 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));
You are correct. Fixed.
Note: Also changed capitialization of "port" to "Port" to make conssistent.
>
> > + 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.
Changed to decimal.
>
> > + if ( data[0xC] != 0xFE )
>
> Coding style: no spaces inside the parentheses. You also have a
> duplicate space before "!=".
Done.
>
> > + 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?
Yes, this is better. Fixed.
Note, also changed "HUB" to "Hub" to make consistent.
>
> > + 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
--
-----------------------------------------------------------------------------
Jerry Hoemann Software Engineer Hewlett Packard Enterprise
-----------------------------------------------------------------------------