Re: [PATCH 1/1] dmioem: HPE OEM Record 237 Firmware change

[Jean Delvare]

Hi Jerry,

On Fri, 17 Mar 2023 16:19:56 -0600, Jerry Hoemann wrote:
> HPE OEM record type 237 offset 0x09 field was changed from a single
> byte STRING to a two byte WORD representing date.
> 
> Decode both forms of the record based upon record size.
> 
> Fixes: cdab638dabb7 ("dmioem: Decode HPE OEM Record 237")
> Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>
> ---
>  dmioem.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/dmioem.c b/dmioem.c
> index dc4b857..fd6c191 100644
> --- a/dmioem.c
> +++ b/dmioem.c
> @@ -1094,7 +1094,8 @@ static int dmi_decode_hp(const struct dmi_header *h)
>                        *  0x06  | Manufacture|STRING | DIMM Manufacturer
>                        *  0x07  | Part Number|STRING | DIMM Manufacturer's 
> Part Number
>                        *  0x08  | Serial Num |STRING | DIMM Vendor Serial 
> Number
> -                      *  0x09  | Spare Part |STRING | DIMM Spare Part Number
> +                      *  0x09  | Spare Part |STRING | DIMM Spare Part Number 
>  --OR--
> +                      *  0x09  | Man Date   | WORD  | DIM Manufacture Date 
> YYWW in BCD

Typo: DIM -> DIMM.

It's so sad to still see a year stored on only 2 digits. Not to mention
BCD which barely makes sense here. But I know this is not HPE's fault
as the data comes straight from SPD EEPROMs which must follow this
unfortunate JEDEC standard :(

I don't think the definition is completely correct. Integers stored in
DMI tables follow the little-endian ordering convention. So if that
field was actually a WORD (16-bit integer) in YYWW format, the week
number would be at offset 0x09 and the year would be at offset 0x0A.
However the code below assumes the year is at offset 0x09 and the week
is at offset 0x0A.

The samples I have access to suggest that the code is correct, so the
definition above should IMHO be clarified. The most straightforward way
is probably to define 2 BYTE fields at offsets 0x09 and 0x0A so that
the ordering is unambiguous. What do you think?

>                        */
>                       if (gen < G9) return 0;
>                       pr_handle_name("%s DIMM Vendor Information", company);
> @@ -1106,7 +1107,11 @@ static int dmi_decode_hp(const struct dmi_header *h)
>                       if (h->length < 0x09) break;
>                       pr_attr("DIMM Vendor Serial Number", "%s", 
> dmi_string(h, data[0x08]));
>                       if (h->length < 0x0A) break;
> -                     pr_attr("DIMM Spare Part Number", "%s", dmi_string(h, 
> data[0x09]));
> +                     if (h->length == 0x0A)
> +                             pr_attr("DIMM Spare Part Number", "%s", 
> dmi_string(h, data[0x09]));
> +                     if (h->length < 0x0B) break;

Not relevant if we go for the alternative, simpler fix, but note that
you were testing the same condition twice here.

> +                     if (WORD(data + 0x09))
> +                             pr_attr("DIMM Manufacture Date", "20%02x Week 
> %2x", data[0x09], data[0x0a]);

Please use upper-case for 0x0A for consistency.

The heuristic used in decode-dimms is that the year is within range
1980-2079, but I suppose we can safely assume that no type 237 record
of size 11 is present on a system that would accept pre-2000 memory
modules? If so then I'm fine with hard-coding the leading "20".

Note that the ISO 8601-compliant way to display a week would be
"20%02x-W%02x". This is what I used in decode-dimms, and it would
probably make sense to do the same here for consistency with HPE record
type 199 where we do already use the ISO 8601 notation to display the
date.

Thanks,
-- 
Jean Delvare
SUSE L3 Support