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.
> + * 0x05 | ROM vers | STRING| Version of the
> Rendundant ROM
Typo: redundant
> + * 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.
"const char *" is preferred over "char const *".
blank could be declared const too.
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.
(As a side note for your firmware engineers: the preferred way to
disable a DMI string is to set its index to 0.)
> + 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.
The description above says this is the binary file name, not version.
> + pr_attr("OEM ROM Date", "%s", dmi_string(h,
> data[0x08]));
And that would be the binary build date.
> + }
> + break;
A blank line would be appreciated here for consistency.
> case 194:
> /*
> * Vendor Specific: Super IO Enable/Disable Features
Thanks,
--
Jean Delvare
SUSE L3 Support
