Re: [PATCH 1/1] dmioem: Update HPE OEM Record 216

[Jerry Hoemann]

On Wed, Jan 25, 2023 at 05:10:05PM +0100, Jean Delvare wrote:
> Hi Jerry,
> 
> On Thu, 19 Jan 2023 18:32:46 -0700, Jerry Hoemann wrote:
> > Decode HPE OEM Record 216: Version Indicator Record
> > 
> > Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>
> > ---
> >  dmioem.c | 201 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 201 insertions(+)
> > 
> > diff --git a/dmioem.c b/dmioem.c
> > index b791118..e5cf63e 100644
> > --- a/dmioem.c
> > +++ b/dmioem.c
> > @@ -300,6 +300,162 @@ static void dmi_hp_203_devloc(const char *fname, 
> > unsigned int code)
> >     pr_attr(fname, "%s", str);
> >  }
> >  
> > +static void dmi_hp_216_fw_type(u16 code)
> > +{
> > +   const char *str = "Reserved";
> > +   static const char * const type[] = {
> > +           "Reserved",     /* 0x00 */
> > +           "System ROM",
> > +           "Redundant System ROM",
> > +           "System ROM Bootblock",
> > +           "Power Management Controller Firmware",
> > +           "Power Management Controller Firmware Bootloader",
> > +           "SL Chassis Firmware",
> > +           "SL Chassis Firmware Bootloader",
> > +           "Hardware PAL/CPLD",
> > +           "SPS Firmware (ME Firmware)",
> > +           "SL Chassis PAL/CPLD",
> > +           "Compatibility Support Module (CSM)",
> > +           "APML",
> > +           "Smart Storage Battery (Megacell) Frimware",
> 
> Typo: Frimware -> Firmware.

Done.

> 
> > +           "Trusted Module (TPM or TCM) Firmware Version",
> > +           "NVMe Backplane Firmware",
> > +           "Intelligent Provisioning",
> > +           "SPI Descriptor Version",
> > +           "Innovation Engine Firmware (IE Firmware)",
> > +           "UMB Backplane Firmware",
> > +           "Reserved", /* 0x14 */
> > +           "Reserved",
> > +           "Reserved",
> > +           "Reserved",
> > +           "Reserved",
> > +           "Reserved",
> > +           "Reserved",
> > +           "Reserved",
> > +           "Reserved",
> > +           "Reserved",
> > +           "Reserved",
> > +           "Reserved", /* 0x1f */
> > +           "EL Chassis Abstraction Revision",
> > +           "EL Chassis Firmware Revision",
> > +           "EL Chassis PAL/CPLD",
> > +           "EL Cartride Abstraction Revision",
> > +           "Reserved", /* 0x24 */
> > +           "Reserved",
> > +           "Reserved",
> > +           "Reserved",
> > +           "Reserved",
> > +           "Reserved",
> > +           "Reserved",
> > +           "Reserved",
> > +           "Reserved",
> > +           "Reserved",
> > +           "Reserved",
> > +           "Reserved", /*0x2f */
> 
> Missing space. More generally, please use consistent comments in this
> table (alignment, spacing, case, radix).

Fixed.

> 
> > +           "Embedded Video Controller",
> > +           "PCIe Riser n Programmable Logic Device",
> 
> This "n" looks odd.

There was an additional comment in the spec:  (n is the riser number)

In the few instances where this FW type  is reported, the string version
had n == 1.  e.g. the display is:

        Firmware Type Indicator: PCIe Riser n Programmable Logic Device
        Firmware Name String: PCIe Riser 1 Programmable Logic Device

I don't see a reasonable way dmidecode could make the determination of
which specific riser the record is for.  I've added the comment into
the generic message.  Not sure what else to do with this.

> 
> > +           "PCIe cards that contain a programmable CPLD",
> 
> "programmable" is redundant here, as that's already what the P in CPLD
> stands for.

Straigt from the spec.  :)

I removed "programmable."

> 
> > +           "Intel NVMe VROC",
> > +           "Intel STATA VROC",
> 
> Do you mean "SATA"?

Yes.  Fixed.

> 
> > +           "Intel SPS Firmware",
> > +           "Secondary System Programmable Logic Device",
> > +           "CPU MEZZ Programmable Logic Device",   /* 0x37 */
> > +           "Intel Artic Sound -M Accelerator Models Firmware",
> > +           "Ampere System Control Processor (SCP – PMPro+SMPro)",
> > +           "Intel CFR information", /* 0x3A, 58 */
> > +   };
> > +
> > +   if (code < ARRAY_SIZE(type))
> > +           str = type[code];
> > +
> > +   pr_attr("Firmware Type Indicator", "%s", str);
> 
> Not sure "Indicator" adds much value.

"Indicator" was straight from the spec.  I'll remove it.
> 
> > +}
> > +
> > +static void dmi_hp_216_version(u32 format, u8 *data)
> 
> "format" has type u8 according to the record description.

fixed.

> 
> > +{
> > +   char buf[80];
> > +   const char * const reserved = "Reserved";
> > +   const char *vers = buf;
> > +   int gen;
> > +
> > +   gen = dmi_hpegen(dmi_product);
> > +
> > +   bzero(buf, sizeof(buf));
> 
> Shouldn't be needed if all cases below are handled properly.

removed.

> 
> > +   switch (format) {
> > +   case 0:
> > +           sprintf(buf, "No Version Data");
> > +           break;
> > +   case 1:
> > +           sprintf(buf, "%c.%d.%d", data[0] & (1 << 7) ? 'B' : 'R',
> > +                                   data[0] & 0x7, data[1] & 0x7);
> > +           break;
> > +   case 2:
> > +           sprintf(buf, "%d.%d", data[0] >> 4, data[0] & 0x0f);
> > +           break;
> > +   case 3:
> > +           vers = reserved;
> > +           break;
> 
> This would be caught by the default case, so no need to duplicate.

I would like to keep something here.  It is an indicator that format 3
is reserved as opposed to coding error of forgetting to include case 3.
I'll add a comment to that effect.


> 
> > +   case 4:
> > +           sprintf(buf, "%d.%d.%d", data[0] >> 4, data[0] & 0x0f, data[1] 
> > & 0x7f);
> > +           break;
> > +   case 5:
> > +           if (gen == G9) {
> > +                   sprintf(buf, "%d.%d.%d", data[0] >> 4, data[0] & 0x0f, 
> > data[1] & 0x7f);
> > +           } else if (gen == G10 || gen == G10P) {
> > +                   sprintf(buf, "%d.%d.%d.%d", data[1] & 0x0f, data[3] & 
> > 0x0f,
> > +                                              data[5] & 0x0f, data[6] & 
> > 0x0f);
> > +           }
> 
> You need a fallback for other generations, or you may (in theory at
> least) print an empty string, which is confusing.
> 
> I also suggest that you check whether this format is used on G11 and
> update the code accordingly if it is.

Fixed.  Adding else clause which sets to reserved.

I have email out to FW architect.  Answer will likely have to wait until
after CNY.
> 
> > +           break;
> > +   case 6:
> > +           sprintf(buf, "%d.%d", data[1], data[0]);
> > +           break;
> > +   case 7:
> > +           sprintf(buf, "v%d.%.2d (%.2d/%.2d/%d)", data[0], data[1],
> > +                                                   data[2], data[3], 
> > WORD(data + 0x04));
> 
> Please avoid mixing decimal and hexadecimal offsets within a given case
> (same comment for several other cases below) as this is somewhat
> error-prone.

done.

> 
> > +           break;
> > +   case 8:
> > +           sprintf(buf, "%d.%d", WORD(data + 0x04), WORD(data));
> > +           break;
> > +   case 9:
> > +           sprintf(buf, "%d.%d.%d", data[0], data[1], WORD(data + 0x02));
> > +           break;
> > +   case 10:
> > +           sprintf(buf, "%d.%d.%d Build %d", data[0], data[1], data[2], 
> > data[3]);
> > +           break;
> > +   case 11:
> > +           sprintf(buf, "%d.%d %d ", WORD(data + 0x02), WORD(data), 
> > DWORD(data + 0x04));
> 
> Stray space at end of string.

Fixed.

> 
> > +           break;
> > +   case 12:
> > +           sprintf(buf, "%d.%d.%d.%d", WORD(data), WORD(data + 0x02),
> > +                                       WORD(data + 0x04), WORD(data + 
> > 0x06));
> > +           break;
> > +   case 13:
> > +           sprintf(buf, "%d", data[0]);
> > +           break;
> > +   case 14:
> > +           sprintf(buf, "%d.%d.%d.%d", data[0], data[1], data[2], data[3]);
> > +           break;
> > +   case 15:
> > +           sprintf(buf, "%d.%d.%d.%d (%d/%d/%d)",
> > +                           WORD(data), WORD(data + 0x02), WORD(data + 
> > 0x04), WORD(data + 0x06),
> > +                           data[0x08], data[0x09], WORD(data + 0x0a));
> 
> Last part looks like a date, so it would make sense to use the same
> precision strategy as in case 7.

Agree. done.

> 
> > +           break;
> > +   case 16:
> > +           sprintf(buf, "%1c%1c%1c%1c.%d%d",
> > +                           data[0x0], data[0x1], data[0x2], data[0x3], 
> > data[0x4], data[0x5]);
> 
> I don't get the difference between "%1c" and just "%c"?

blindly copied spec.  there doesn't seem to be a difference. changed.

> 
> 
> > +           break;
> > +   case 17:
> > +           sprintf(buf, "%08X", DWORD(data));
> > +           break;
> > +   case 18:
> > +           sprintf(buf, "%d.%2d", data[0x0], data[0x1]);
> > +           break;
> > +   default:
> > +           vers = reserved;
> > +   }
> > +   pr_attr("Version Data", "%s", vers);
> > +}
> > +
> >  static int dmi_hp_224_status(u8 code)
> >  {
> >     static const char * const present[] = {
> > @@ -743,6 +899,51 @@ static int dmi_decode_hp(const struct dmi_header *h)
> >                     }
> >                     break;
> >  
> > +           case 216:
> > +                   /*
> > +                    * Vendor Specific: Version Indicator Record
> > +                    *
> > +                    * This record is used to allow determining Firmware 
> > and CPLD revisions for
> > +                    * components in the system. The goal of this record is 
> > to provide a
> > +                    * flexible method to communicate to software and 
> > firmware the revisions
> > +                    * of these components. This record replaces much of 
> > the functionality of
> > +                    * Record Type 193. OEM SMBIOS Record Type 193 was not 
> > scaling well with
> > +                    * the large number of potential CPLD devices, power 
> > management controllers,
> > +                    * etc. This record is flexible such that each instance 
> > of Type 216
> > +                    * defines one firmware component. This record also 
> > includes the string
> > +                    * name for which software should refer to the 
> > component. The record
> > +                    * includes both data bytes to indicate the revision 
> > and a string value. A
> > +                    * firmware component can implement either or both. If 
> > both are supported,
> > +                    * it allows easy display of the revision, but prevents 
> > the need for
> > +                    * software/firmware to parse strings when doing 
> > comparisons on revisions.
> > +                    * As there is one Type 216 Record per firmware 
> > component, the Handle for
> > +                    * the Record can be used to tie firmware components 
> > with other OEM SMBIOS
> > +                    * Records in the future if needed (similar to how 
> > SMBIOS Type 17 is tied
> > +                    * to other Record Types related to DIMMs)
> > +                    *
> > +                    * Offset |  Name      | Width | Description
> > +                    * ---------------------------------------
> > +                    *  0x00  | Type       | BYTE  | 0xD8, SMBIOS Version 
> > Indicator Record
> > +                    *  0x01  | Length     | BYTE  | Length of structure
> > +                    *  0x02  | Handle     | WORD  | Unique handle
> > +                    *  0x04  | FW Type    | WORD  | Type of FW
> 
> Please spell out "Firmware" in the Description column (FW abbreviation
> is fine in the Name column as you lack space there). Same at 0x15 below.

done

> 
> > +                    *  0x06  | FW Name    |STRING | Name of Firmware
> > +                    *  0x07  | FW Version |STRING | Firmware Version
> > +                    *  0x08  | Data Format| BYTE  | Format of the Version 
> > Data
> > +                    *  0x09  |Version Data|12 BTYE| Version Data in Format 
> > from field 0x08
> 
> Typo: BTYE -> BYTE. Feel free to increase the column size by 1 if you
> want to fit the plural form.

fixed.

> 
> > +                    *  0x15  | Unique ID  | WORD  | Unique ID for FW flash
> > +                    */
> > +                   if (gen < G8) return 0;
> > +                   pr_handle_name("%s SMBIOS Version Indicator", company);
> 
> "SMBIOS" seems redundant here, all the records in the table as SMBIOS
> records by definition. Same in the description of field 0x00 in the
> table above, by the way.


Fixed.

> 
> > +                   if (h->length < 23) break;
> > +                   dmi_hp_216_fw_type(WORD(data + 0x04));
> > +                   pr_attr("Firmware Name String", "%s",  dmi_string(h, 
> > data[0x06]));
> 
> Doubled space before dmi_string.

Fixed.
> 
> > +                   pr_attr("Firmware Version String", "%s",  dmi_string(h, 
> > data[0x07]));
> 
> Same here.
Fixed.
> 
> > +                   dmi_hp_216_version(data[0x08], data + 0x09);
> > +                   if (WORD(data + 0x15))
> > +                           pr_attr("Unique ID", "%x", WORD(data + 0x15));
> 
> Unless you have a requirement to match a format used in technical
> documentation, I suggest using "0x%04x" as the format, as is done
> pretty much everywhere else in dmidecode, to avoid any ambiguity.

changed it.

> 
> > +                   break;
> > +
> >             case 219:
> >                     /*
> >                      * Vendor Specific: HPE ProLiant Information
> 
> Thanks,
> -- 
> Jean Delvare
> SUSE L3 Support

-- 

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