On Tue, Jan 05, 2021 at 01:56:09PM +0100, Jean Delvare wrote:
> 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.hoem...@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.
>
done.
> > + * 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?
done.
>
> > + *
> > + * 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.
done.
>
> 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).
I think you mean a check like:
pr_handle_name("%s Proliant Inventory Record", company);
if (h->length < 0x24) break;
before decoding any fields?
Looks like record 236 (commit d9b84ea) should have done something similar.
I'll fix record 236 as a separate patch.
>
> > + 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.
hadn't noticed that paridigm.
There are multiple Associated handles, will apply to all.
>
> > + 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.
done.
>
> > +
> > + 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?
dmi_print_memory_size() is defined statically in dmidecode.c.
Will make global and put prototype in dmidecode.h
>
> > + 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?
done
>
> Would it make sense to go into greater details and list the individual
> attributes? Or if that too low details?
I found details and will add.
>
> > +
> > + 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?
I fixed the printf format.
>
> > +
> > default:
> > return 0;
> > }
>
>
> --
> Jean Delvare
> SUSE L3 Support
--
-----------------------------------------------------------------------------
Jerry Hoemann Software Engineer Hewlett Packard Enterprise
-----------------------------------------------------------------------------
_______________________________________________
https://lists.nongnu.org/mailman/listinfo/dmidecode-devel
