On Mon, May 12, 2025 at 07:05:28PM +0200, Jean Delvare wrote:
> Hi Jerry,
>
> On Tue, 6 May 2025 23:52:03 -0600, Jerry Hoemann wrote:
> > OEM Type 193: Other ROM Info.
> >
> > Signed-off-by: Jerry Hoemann <jerry.hoem...@hpe.com>
> > ---
> > dmioem.c | 28 ++++++++++++++++++++++++++++
> > 1 file changed, 28 insertions(+)
> >
> > diff --git a/dmioem.c b/dmioem.c
> > index e86f863..b3d5bbd 100644
> > --- a/dmioem.c
> > +++ b/dmioem.c
> > @@ -1001,6 +1001,34 @@ static int dmi_decode_hp(const struct dmi_header *h)
> >
> > switch (h->type)
> > {
> > + case 193:
> > + /*
> > + * Vendor Specific: Other ROM Info
> > + *
> > + * Offset | Name | Width | Description
> > + * -------------------------------------
> > + * 0x00 | Type | BYTE | 0xC1, ROM Structure
> > Indicator
> > + * 0x01 | Length | BYTE | Length of structure
> > + * 0x02 | Handle | WORD | Unique handle
> > + * 0x04 | ROM | BYTE | 01: Redundant ROM
> > installed
>
> You never test nor report this field. Does it make sense to report the
> version string if the "installed" bit isn't set? I have 3 samples here
> (Gen11 RL300) where the bit is not set and the version string is
> "0/0000", which looks invalid to me.
fixed. used in determining to print redundant rom version.
>
> > + * 0x05 | ROM vers | STRING| Version of the
> > Rendundant ROM
>
> Typo: redundant
fixed.
>
> > + * 0x06 | Reserved | BYTE | Reserved in Gen9
> > forward
> > + * 0x07 | OEM ROM | STRING| If not blank, OEM ROM
> > binary file name
> > + * 0x08 | OEM Date | STRING| If not blank, OEM ROM
> > binary build date
> > + */
> > + if (gen < G9) return 0;
> > + pr_handle_name("%s ProLiant Other ROM Info", company);
> > + if (h->length < 0x09) break;
> > + if (gen < G12)
> > + pr_attr("Redundant ROM Version", "%s",
> > dmi_string(h, data[0x05]));
> > + char const *str = dmi_string(h, data[0x07]);
> > + char blank[] = " "; // 9 spaces
>
> For maximum portability, variables should be declared at the beginning
> of a block (or at the beginning of the function). For the same reason,
> please use /* */ for comments.
moved to inside scope under new test of if (data[0x07])
>
> "const char *" is preferred over "char const *".
fixed.
>
> blank could be declared const too.
removed.
>
> Comparing with exactly a 9-space string seems fragile to me. I can
> imagine how a future firmware implementation would alter the length of
> the string and that would break the test. Wouldn't it be sufficient,
> and more robust, to check whether the string starts with a space? My
> assumption is that a valid string would never start with a space.
changed to check on string not starting w/ spaces.
>
> (As a side note for your firmware engineers: the preferred way to
> disable a DMI string is to set its index to 0.)
this was explicit in the spec that these would be spaces (as opposed to
a zero index which is used in other OEM records.)
Its possible that this field is patched in the oem rom as opposed to
creating new rom with a modified smbios with valid string.
But this is just my guess.
>
> > + if (str && strncmp(str, blank, strlen(blank)))
> > + {
> > + pr_attr("OEM ROM Version", "%s", dmi_string(h,
> > data[0x07]));
>
> You already know dmi_string(h, data[0x07]) as str, it would be more
> efficient to reuse str.
new code checks for not starting with 2 spaces.
so, will continue using dmi_string.
>
> The description above says this is the binary file name, not version.
fixed.
>
> > + pr_attr("OEM ROM Date", "%s", dmi_string(h,
> > data[0x08]));
>
> And that would be the binary build date.
fixed.
>
> > + }
> > + break;
>
> A blank line would be appreciated here for consistency.
done.
>
> > case 194:
> > /*
> > * Vendor Specific: Super IO Enable/Disable Features
>
> Thanks,
> --
> Jean Delvare
> SUSE L3 Support
--
-----------------------------------------------------------------------------
Jerry Hoemann Software Engineer Hewlett Packard Enterprise
-----------------------------------------------------------------------------
