[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 1/1] dmioem: Update HPE OEM Record 216
|
From: |
Jean Delvare |
|
Subject: |
Re: [PATCH 1/1] dmioem: Update HPE OEM Record 216 |
|
Date: |
Wed, 25 Jan 2023 17:10:05 +0100 |
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.
> + "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).
> + "Embedded Video Controller",
> + "PCIe Riser n Programmable Logic Device",
This "n" looks odd.
> + "PCIe cards that contain a programmable CPLD",
"programmable" is redundant here, as that's already what the P in CPLD
stands for.
> + "Intel NVMe VROC",
> + "Intel STATA VROC",
Do you mean "SATA"?
> + "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.
> +}
> +
> +static void dmi_hp_216_version(u32 format, u8 *data)
"format" has type u8 according to the record description.
> +{
> + 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.
> + 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.
> + 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.
> + 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.
> + 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.
> + 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.
> + 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"?
> + 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.
> + * 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.
> + * 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.
> + 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.
> + pr_attr("Firmware Version String", "%s", dmi_string(h,
> data[0x07]));
Same here.
> + 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.
> + break;
> +
> case 219:
> /*
> * Vendor Specific: HPE ProLiant Information
Thanks,
--
Jean Delvare
SUSE L3 Support