[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [dmidecode] [PATCH] dmioem: Decode HPE OEM Record 230
|
From: |
Jerry Hoemann |
|
Subject: |
Re: [dmidecode] [PATCH] dmioem: Decode HPE OEM Record 230 |
|
Date: |
Thu, 4 Aug 2022 11:42:39 -0600 |
|
User-agent: |
Mutt/1.10.1 (2018-07-13) |
On Wed, Aug 03, 2022 at 02:59:40PM +0200, Jean Delvare wrote:
> Hi Jerry,
>
> On Tue, 19 Jul 2022 15:47:03 -0600, Jerry Hoemann wrote:
> > Decode HPE OEM Record 230: Power Supply Information
> >
> > Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>
> > ---
> > dmioem.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 52 insertions(+)
> >
> > diff --git a/dmioem.c b/dmioem.c
> > index 0c73771..24b2b9e 100644
> > --- a/dmioem.c
> > +++ b/dmioem.c
> > @@ -389,6 +389,20 @@ static void dmi_hp_224_chipid(u16 code)
> > pr_attr("Chip Identifier", "%s", str);
> > }
> >
> > +static void dmi_hp_230_method(u8 code)
> > +{
> > + const char *str = "Reserved";
> > + static const char * const method[] = {
> > + "Not Available", /* 0x00 */
>
> Space after the comma, instead of tab, for consistency.
done.
> "Not Available" is a bit confusing in this context. You used "Unknown",
context, spec says:
00h = Not Available (this value
would be used for a “dumb” power
supply which does not contain FRU
information.
> "Not Specified", "Not Present" or "None" for the other types, which
> expresses the situation more clearly IMHO.
I'm pretty literal when implementing these to try to follow what is in
the spec as closely as possible with the thought that FW engineers could
be using dmidecode to verify that how they implemented the SMBIOS entry
matches the spec.
A down side to this approach is that it shows that the extension specification
is a living document that has been updated over several years by several
different engineers who don't always use consistent verbiage.
>
> "Not Available" could mean that there is no access possible to the PSU
> information, or that it is not know how to access it, which are two
> different things. In the former case, "None" would seem appropriate,
> while "Unknown" would be more suitable in the latter case. It all
> depends how your documentation defines value 0x00 for this field.
>
> > + "IPMI I2C Mechanism",
> > + "iLO Mechanism",
> > + "Chassis Manager Mechanism", /* 0x4 */
>
> Should be spelled 0x04 for consistency. Numbering doesn't match though.
> If "Chassis Manager Mechanism" is really 0x04 then there's one element
> missing in your array.
>
fixed: /* 0x03 */
> Would it make sense to drop "Mechanism" from all strings? To me it
> sounds redundant with "Access Method".
I agree that it sounds ackward, but it is straight from the spec.
Since the proposed change keeps the beginning of the description, there
is little chance for confusion, so I dropped "Mechanism."
>
> > + };
> > + if (code < ARRAY_SIZE(method))
> > + str = method[code];
> > + pr_attr("Access Method", "%s", str);
> > +}
> > +
> > static void dmi_hp_238_loc(const char *fname, unsigned int code)
> > {
> > const char *str = "Reserved";
> > @@ -719,6 +733,44 @@ static int dmi_decode_hp(const struct dmi_header *h)
> > dmi_hp_224_chipid(WORD(data + 0x0a));
> > break;
> >
> > + case 230:
> > + /*
> > + * Vendor Specific: Power Supply Information OEM SMBIOS
> > Record
> > + *
> > + * This record is used to communicate additional Power
> > Supply Information
> > + * beyond the Industry Standard System Power Supply
> > (Type 39) Record.
> > + *
> > + * Offset| Name | Width | Description
> > + * ------------------------------------
> > + * 0x00 | Type | BYTE | 0xE6, Power Supply
> > Information Indicator
> > + * 0x01 | Length | BYTE | Length of structure
> > + * 0x02 | Handle | WORD | Unique handle
> > + * 0x04 | Assoc Handle| WORD | Associated Handle
> > (Type 39)
> > + * 0x06 | Manufacturer| STRING| Actual third party
> > manufacturer
> > + * 0x07 | Revision | STRING| Power Supply Revision
> > Level
> > + * 0x08 | FRU Access | BYTE | Power Supply FRU
> > Access Method.
>
> I'm confused by the use of "FRU" in this context. If it means
> "field-replaceable unit" as is common in technical documentation, what
> does it have to do with how the PSU information is being retrieved?
Not exactly sure. I believe it is merely reflecting that power supplies on
ProLiant systems
are field replaceable units. The spec has:
08h | Power Supply FRU Access Method | BYTE | Varies | Indicates the
general mechanism used to
access the
Power Supply FRU:
• 00h = Not
Available (this value
would be used
for a “dumb” power
supply which
does not contain FRU
information.
• 01h = IPMI
I2C Mechanism
• 02h = iLO
Mechanism
• 03h = Chassis
Manager
Mechanism
>
> > + * 0x09 | I2C Buss Num| BYTE | I2C Bus #. Value based
> > upon context
>
> "Bus" takes a single S.
done.
>
> > + * 0x0A | I2C Address | BYTE | I2C Address.
>
> Please drop the 2 trailing dots for consistency.
done.
>
> > + */
> > + pr_handle_name("%s Power Supply Information", company);
> > + if (h->length < 0x0B) break;
> > + if (!(opt.flags & FLAG_QUIET))
> > + pr_attr("Associated Handle", "0x%04X",
> > WORD(data + 0x4));
> > + pr_attr("Manufacturer", "%s", dmi_string(h,
> > data[0x06]));
> > + pr_attr("Revision", "%s", dmi_string(h, data[0x07]));
> > + dmi_hp_230_method(data[0x08]);
> > + feat = data[0x09];
> > + if (feat == 0xFF)
> > + pr_attr("I2C Bus Number", "%s", "Not
> > Available");
This is technically a Bus or Segment number based upon context.
(segment number when iLO method.) I need to rework this.
> > + else
> > + pr_attr("I2C Bus Number", "%d", feat);
>
> I suppose that I2C bus and address may not make sense for all access
> methods, and that would be the reason why the value is set to 0xFF. In
> such case, wouldn't it be better to simply omit these fields, rather
> than printing "Not Available"?
done.
>
> > + feat = data[0x0A];
> > + if (feat == 0xFF)
> > + pr_attr("I2C Address", "%s", "Not Available");
> > + else
> > + pr_attr("I2C Address", "%d", feat);
> > + break;
>
> I2C addresses are almost always expressed as hexadecimal values in the
> literature, so you should use 0x%02x as the format. Furthermore, the
> values I see in my samples are all even and above 127. This suggests
> they are encoded as left-aligned 7-bit values, while I2C addresses in
> the Linux world are always expressed as right-aligned 7-bit values. So
> you should shift the value by 1 bit to the right before printing it, so
> that it actually matches the values seen in the kernel log, i2c-tools
> etc.
done.
>
> > +
> > case 233:
> > /*
> > * Vendor Specific: HPE ProLiant NIC MAC Information
>
> Thanks,
> --
> Jean Delvare
> SUSE L3 Support
--
-----------------------------------------------------------------------------
Jerry Hoemann Software Engineer Hewlett Packard Enterprise
-----------------------------------------------------------------------------