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

[Jerry Hoemann]

On Wed, Sep 07, 2022 at 11:53:13AM +0200, Jean Delvare wrote:
> Hi Jerry,
> 
> On Mon,  8 Aug 2022 13:30:37 -0600, Jerry Hoemann wrote:
> > Decode HPE OEM Record 230: Power Supply Information
> > 
> > Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>
> > ---
> >  dmioem.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 51 insertions(+)
> > 
> > diff --git a/dmioem.c b/dmioem.c
> > index 0c73771..c4db435 100644
> > --- a/dmioem.c
> > +++ b/dmioem.c
> > @@ -389,6 +389,26 @@ static void dmi_hp_224_chipid(u16 code)
> >     pr_attr("Chip Identifier", "%s", str);
> >  }
> >  
> > +static void dmi_hp_230_method_bus_seg(u8 code, u8 bus_seg)
> > +{
> > +   const char *str = "Reserved";
> > +   static const char * const method[] = {
> > +           "Not Available", /* 0x00 */
> > +           "IPMI I2C",
> > +           "iLO",
> > +           "Chassis Manager", /* 0x03 */
> > +   };
> > +   if (code < ARRAY_SIZE(method))
> > +           str = method[code];
> > +   pr_attr("Access Method", "%s", str);
> > +   if (bus_seg != 0xff) {
> > +           if (code == 2)
> > +                   pr_attr("I2C Segment Number", "%d", bus_seg);
> > +           else
> > +                   pr_attr("I2C Bus Number", "%d", bus_seg);
> > +   }
> > +}
> > +
> >  static void dmi_hp_238_loc(const char *fname, unsigned int code)
> >  {
> >     const char *str = "Reserved";
> > @@ -719,6 +739,37 @@ 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
> > +                    *  0x09 | I2C Bus Num | BYTE  | I2C Bus #. Value based 
> > upon context
> > +                    *  0x0A | I2C Address | BYTE  | I2C Address
> > +                    */
> > +                   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_bus_seg(data[0x08], data[0x09]);
> > +                   feat = data[0x0A];
> > +                   if (feat != 0xFF)
> > +                           pr_attr("I2C Address", "0x%02x", feat >> 0);
> 
> I guess you meant "feat >> 1". No need to resend, I'll fix it up.

Yes.  It should have been "feat >> 1" to address your code review
comment from version 2 of the patch.

> 
> > +                   break;
> > +
> >             case 233:
> >                     /*
> >                      * Vendor Specific: HPE ProLiant NIC MAC Information
> 
> Looks good overall. I only have one occurrence which gets decoded
> incorrectly:
> 
> HP Power Supply Information
>        Associated Handle: 0x002E
>        Manufacturer:                 
>        Revision:         
>        Access Method: Not Available
>        I2C Bus Number: 7
>        I2C Address: 0x00
> 
> Raw data is:
> 
>       E6 0B 7B 00 2E 00 01 02 00 07 00
> 
> This is on a HP ProLiant DL180 Gen9 with BIOS version U20.
> 
> Maybe it would make sense to skip "I2C Bus Number/Segment" and "I2C
> Address" if "Access Method" is "Not Available"? Or are you happy with
> the current output per specification, and we leave it like that?

The "Not Available" is defined as:

                00h = Not Available (this value
                would be used for a “dumb” power
                supply which does not contain FRU
                information.

So, I think you're correct and we should not decode the record further
in these cases.  Bios doesn't know type of FRU so we really don't know
how to access it.  Similar logic woud hold if method is outside range.

I'll send out v4 when it is ready.


> 
> -- 
> Jean Delvare
> SUSE L3 Support

-- 

-----------------------------------------------------------------------------
Jerry Hoemann                  Software Engineer   Hewlett Packard Enterprise
-----------------------------------------------------------------------------