Re: [PATCH 1/1] dmioem: Decode HPE OEM Record 245

[Jerry Hoemann]

On Sat, May 20, 2023 at 03:33:04PM +0200, Jean Delvare wrote:
> Hi Jerry,
> 
> On Mon, 2023-05-08 at 11:32 -0600, Jerry Hoemann wrote:
> > Decode HPE OEM Record 245: Extension Board Inventory Record.
> > 
> > Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>
> > ---
> >  dmioem.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 56 insertions(+)
> > 
> > diff --git a/dmioem.c b/dmioem.c
> > index 04a89ab..5ccd30e 100644
> > --- a/dmioem.c
> > +++ b/dmioem.c
> > @@ -771,6 +771,28 @@ static void dmi_hp_242_speed(const char *attr, u16 
> > speed)
> >                 pr_attr(attr, "%s", "Unknown");
> >  }
> >  
> > +static void dmi_hp_245_pcie_riser(const struct dmi_header *h)
> > +{
> > +       const char *str = "Reserved";
> > +       u8 *data = h->data;
> > +
> > +       pr_attr("Board Type", "PCIe Riser");
> > +       if (h->length < 0x09) return;
> > +       switch (data[0x05])
> > +       {
> > +               case 1: str = "Primary"; break;
> > +               case 2: str = "Secondary"; break;
> > +               case 3: str = "Tertiary"; break;
> > +               case 4: str = "Quaternary"; break;
> > +               case 10: str = "Front"; break;
> > +       }
> > +       pr_attr("Riser Position", "%s", str);
> > +       pr_attr("Riser ID", "%d", data[0x06]);
> 
> In the sample you provided, there are 2 riser records and both have
> this field set to 2. I'd expect this ID to be unique within a given
> system. The code looks right though, so I suppose the problem is with
> the DMI table data?

That does look a bit strange.  I'll follow up w/ the firmware team as
that might be a minor defect in how they fill out the smbios table.

> 
> > +       if (data[0x07])
> > +               pr_attr("CPLD Version", "%c:%x", (data[0x07] >> 7) ? 'B' : 
> > 'R', (data[0x07] & 0x7F));
> 
> Hexadecimal number printed without a prefix can be confusing. Is there
> any standard regarding how a CPLD version should be presented?

Oh, I should have put in the leading "0x".

Don't know about industry standard for displaying CPLD. ProLiant's iLO 
ineterface
displays these under "System Progammable Logic Device" and displays the
version with format 0x%02X.

This field also has msb denoting an internal release as 'B'.  Customers
should never see this and the 'R' is normally not displayed as it is
assumed.  If you want I can tweak the code to only display the 'B' and
not display the 'R'.

> 
> > +       pr_attr("Riser Name", dmi_string(h, data[0x08]));
> > +}
> > +
> >  static int dmi_decode_hp(const struct dmi_header *h)
> >  {
> >         u8 *data = h->data;
> > @@ -1490,6 +1512,40 @@ static int dmi_decode_hp(const struct dmi_header *h)
> >                         dmi_hp_242_speed("Negotiated Speed", WORD(data + 
> > 0x3A));
> >                         dmi_hp_242_speed("Capable Speed", WORD(data + 
> > 0x3C));
> >                         break;
> > +
> > +               case 245:
> > +                       /*
> > +                        * Vendor Specific: HPE Extension Board Inventory 
> > Record
> > +                        *
> > +                        * This record provides a mechanism for software to 
> > retrieve installed
> > +                        * Extension Boards in system, such as Riser Cards, 
> > etc. Each extension
> > +                        * board discovered in system boot time has a 
> > corresponding record
> 
> "at system boot time" would sound better, methinks.
> 

Done


> > +                        * produced in SMBIOS Type 245. This record is 
> > currently applicable
> > +                        * for ML, DL and Alletra series servers in Gen11 
> > and will be backward
> > +                        * compatible with next generations
> > +                        *
> > +                        * This is a variant record. Definition of fields 
> > 0x05 ... vary based
> > +                        * upon field 0x04 Board Type.
> > +                        *
> > +                        * Offset |  Name      | Width | Description
> > +                        * ---------------------------------------
> > +                        *  0x00  | Type       | BYTE  | 0xF5, Extension 
> > Board Inventory Record
> > +                        *  0x01  | Length     | BYTE  | Length of structure
> > +                        *  0x02  | Handle     | WORD  | Unique handle
> > +                        *  0x04  | Board Type | WORD  | 0: PCIe Riser, 
> > Other Reserved
> > +                        *
> > +                        *  If Board Type == 0
> > +                        *  0x05  | Riser Pos  | WORD  |
> > +                        *  0x06  | Riser ID   | BYTE  |
> > +                        *  0x07  | CPLD Vers  | BTYE  | 0-> No CPLD. Bits 
> > [7][6:0] Release:Vers
> > +                        *  0x08  | Riser Name | STRING|
> > +                        */
> > +                       pr_handle_name("%s ProLiant Extension Board 
> > Inventory Record", company);
> > +                       if (h->length < 0x05) break;
> > +                       if (data[0x04] == 0)
> > +                               dmi_hp_245_pcie_riser(h);
> > +                       break;
> > +
> >                 default:
> >                         return 0;
> >         }
> 
> Everything else looks good to me.
> 
> -- 
> Jean Delvare
> SUSE L3 Support

-- 

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