dmidecode-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Jean Delvare
Subject: Re: [dmidecode] [PATCH 1/2] dmioem: Decode HPE OEM Record 240
Date: Tue, 5 Jan 2021 13:56:09 +0100

Hi Jerry,

On Wed, 16 Dec 2020 14:18:58 -0700, Jerry Hoemann wrote:
> HP Firmware Inventory Record (Type 240)
> 
> Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>
> ---
>  dmioem.c | 41 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 41 insertions(+)
> 
> diff --git a/dmioem.c b/dmioem.c
> index 180a95d..d8cab2c 100644
> --- a/dmioem.c
> +++ b/dmioem.c
> @@ -361,6 +361,47 @@ 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

Capital A after dot.

> +                      * with other SMBIOS records, such as Type 203 (which 
> in turn is
> +                      * associated with Types 9, 41, and 228),

End with a dot instead of comma?

> +                      *
> +                      * 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 HPE Proliant Inventory Record", 
> company);

Remove the hard-coded "HPE" else you end up with "HPE HPE" in the
output.

To be on the safe side, please start with a length check as is done for
regular DMI types (although apparently not all OEM types - will look
into that).

> +                     pr_attr("Associated Handle ", "0x%04X",  WORD(data + 
> 0x4));

You may want to condition that one on !(opt.flags & FLAG_QUIET) as
we do for other handle cross-references.

> +                     pr_attr("Package Version   ", "0x%X",   DWORD(data + 
> 0x6));
> +                     pr_attr("Version String    ", "%s",     dmi_string(h, 
> data[0x0A]));

We don't do space-alignment of attribute names in dmidecode, so it
would look weird to do it for just that one record.

(If there is a general desire to present things that way for better
readability then that would be handled globally, through the recently
introduced "output drivers".)

We also don't do space-alignment in the source code.

> +
> +                     if (DWORD(data + 0x0B))
> +                             pr_attr("Image Size (bytes)", "0x%08x", 
> QWORD(data + 0xB));

Could we use dmi_print_memory_size() here so that the size is
automatically expressed in the most suitable unit?

> +                     else
> +                             pr_attr("Image Size        ", "%s",     "Not 
> Available");
> +
> +                     pr_attr("Attribute Defined ", "0x%08x", QWORD(data + 
> 0x13));
> +                     pr_attr("Attribute Set     ", "0x%08x", QWORD(data + 
> 0x1B));

I would use Attributes (plural) for both?

Would it make sense to go into greater details and list the individual
attributes? Or if that too low details?

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

I have one system where this prints -1. Should all FF's be treated as
"Not Available" too? Or does it have a special meaning?

> +
>               default:
>                       return 0;
>       }


-- 
Jean Delvare
SUSE L3 Support



reply via email to

[Prev in Thread] Current Thread [Next in Thread]