Re: [dmidecode] [PATCH v2 2/4] dmioem: Decode HPE OEM Record 240

[Jean Delvare]

On Tue, 12 Jan 2021 01:05:03 -0700, Jerry Hoemann wrote:
> HP Firmware Inventory Record (Type 240)
> 
> Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>
> ---
>  dmioem.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 61 insertions(+), 1 deletion(-)
> 
> diff --git a/dmioem.c b/dmioem.c
> index 180a95d..4a7ff2b 100644
> --- a/dmioem.c
> +++ b/dmioem.c
> @@ -27,6 +27,7 @@
>  #include "dmidecode.h"
>  #include "dmioem.h"
>  #include "dmioutput.h"
> +#include "dmiopt.h"
>  
>  /*
>   * Globals for vendor-specific decodes
> @@ -186,6 +187,23 @@ static int dmi_hpegen(const char *s)
>       return (dmi_vendor == VENDOR_HPE) ? G10P : G6;
>  }
>  
> +static void dmi_hp_240_attr(const char *fname, u64 code)
> +{
> +     char attr[80] = "";
> +
> +     if (code.l & (1ULL << 0))
> +             strcat(attr, "Updatable ");
> +     if (code.l & (1ULL << 1))
> +             strcat(attr, "ResetRequired ");
> +     if (code.l & (1ULL << 2))
> +             strcat(attr, "AuthenticationRequired ");
> +     if (code.l & (1ULL << 3))
> +             strcat(attr, "InUse ");
> +     if (code.l & (1ULL << 4))
> +             strcat(attr, "UefiImage");
> +     pr_attr(fname, "%s", attr);
> +}
> +
>  static int dmi_decode_hp(const struct dmi_header *h)
>  {
>       u8 *data = h->data;
> @@ -345,7 +363,6 @@ static int dmi_decode_hp(const struct dmi_header *h)
>                        *  0x14  | Name       | STRING| (deprecated) Backplane 
> Name
>                        */
>                       pr_handle_name("%s HDD Backplane FRU Information", 
> company);
> -
>                       pr_attr("FRU I2C Address", "0x%X raw(0x%X)", data[0x4] 
> >> 1, data[0x4]);
>                       pr_attr("Box Number", "%d", WORD(data + 0x5));
>                       pr_attr("NVRAM ID", "0x%X", WORD(data + 0x7));

That blank line change shouldn't be there, I guess it sneaked in while
you were looking into fixing OEM type 236. I'll remove it before
applying.

> @@ -361,6 +378,49 @@ static int dmi_decode_hp(const struct dmi_header *h)
>                       }
>                       break;
>  
> +             case 240:
> +                     /*
> +                      * Vendor Specific: HPE Proliant Inventory Record
> +                      *
> +                      * Reports firmware version information for devices 
> that report their
> +                      * firmware using their UEFI drivers. Additionally 
> provides association
> +                      * with other SMBIOS records, such as Type 203 (which 
> in turn is
> +                      * associated with Types 9, 41, and 228).
> +                      *
> +                      * Offset |  Name      | Width | Description
> +                      * ---------------------------------------
> +                      *  0x00  | Type       | BYTE  | 0xF0, HP Firmware 
> Inventory Record
> +                      *  0x01  | Length     | BYTE  | Length of structure
> +                      *  0x02  | Handle     | WORD  | Unique handle
> +                      *  0x04  | Hndl Assoc | WORD  | Handle to map to Type 
> 203
> +                      *  0x06  | Pkg Vers   | DWORD | FW Vers Release of All 
> FW in Device
> +                      *  0x0A  | Ver String | STRING| FW Version String
> +                      *  0x0B  | Image Size | QWORD | FW image size (bytes)
> +                      *  0x13  | Attributes | QWORD | Bitfield: Is attribute 
> defined?
> +                      *  0x1B  | Attr Set   | QWORD | BitField: If defined, 
> is attribute set?
> +                      *  0x23  | Version    | DWORD | Lowest supported 
> version.
> +                      */
> +                     pr_handle_name("%s Proliant Inventory Record", company);
> +                     if (h->length < 0x24) break;

Sorry for telling you wrong in my original review. You have a DWORD at
0x23, meaning the last byte used is at offset 0x26, in turn meaning
that you need to check for h->length < 0x27, not 0x24. I'll fix it
before applying.

> +                     if (!(opt.flags & FLAG_QUIET))
> +                             pr_attr("Associated Handle", "0x%04X", 
> WORD(data + 0x4));
> +                     pr_attr("Package Version", "0x%08X", DWORD(data + 0x6));
> +                     pr_attr("Version String", "%s", dmi_string(h, 
> data[0x0A]));
> +
> +                     if (DWORD(data + 0x0B))
> +                             dmi_print_memory_size("Image Size", QWORD(data 
> + 0xB), 0);
> +                     else
> +                             pr_attr("Image Size", "%s", "Not Available");

Note that format isn't mandatory. We can simply use:

                                pr_attr("Image Size", "Not Available");

Same below. I'll fix both.

> +
> +                     dmi_hp_240_attr("Attributes Def", QWORD(data + 0x13));
> +                     dmi_hp_240_attr("Attributes Set", QWORD(data + 0x1B));

I'm not completely satisfied with the way this presents the attributes.
But we can commit this first and I'll see if I can come up with
something better later.

> +
> +                     if (DWORD(data + 0x23))
> +                             pr_attr("Lowest Supported Version", "0x%08X", 
> DWORD(data + 0x23));
> +                     else
> +                             pr_attr("Lowest Supported Version", "%s", "Not 
> Available");

I'm curious what this "version" is about. On my samples, I have seen
values 1, 2, 3, which do not look like hexadecimal values. But also
0x00000000 (properly handled) and 0x0000FFFF (displayed as is).

Are you sure this is a 32-bit field and not 2 16-bit fields?

> +                     break;
> +
>               default:
>                       return 0;
>       }


-- 
Jean Delvare
SUSE L3 Support