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

[Jean Delvare]

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.

"Not Available" is a bit confusing in this context. You used "Unknown",
"Not Specified", "Not Present" or "None" for the other types, which
expresses the situation more clearly IMHO.

"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.

Would it make sense to drop "Mechanism" from all strings? To me it
sounds redundant with "Access Method".

> +     };
> +     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?

> +                      *  0x09 | I2C Buss Num| BYTE  | I2C Bus #. Value based 
> upon context

"Bus" takes a single S.

> +                      *  0x0A | I2C Address | BYTE  | I2C Address.

Please drop the 2 trailing dots for consistency.

> +                      */
> +                     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");
> +                     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"?

> +                     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.

> +
>               case 233:
>                       /*
>                        * Vendor Specific: HPE ProLiant NIC MAC Information

Thanks,
-- 
Jean Delvare
SUSE L3 Support