On Wed, Mar 22, 2023 at 03:29:05PM +0100, Jean Delvare wrote:
> 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.hoem...@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.
Fixed.
>
> 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?
I agree the one line documentation is open to interpretation. The spec
has similar lack of certainty. I had a similar interpretation when I
first read the field description. It wasn't until I implemented the
code that I changed.
I've reached out to my firmware team to clarify the hpe specification in
hopes of avoiding future confusion.
I'll update the code comment based upon what I hear back from the fw
team.
>
> > */
> > 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.
Done.
>
> 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".
I think this is a safe assumption. Gen9 systems required DDR4 memory
which was released in 2014 according to Wikipedia.
>
> 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.
Fixed. using format: 20%02x-W%02x
>
> Thanks,
> --
> Jean Delvare
> SUSE L3 Support
--
-----------------------------------------------------------------------------
Jerry Hoemann Software Engineer Hewlett Packard Enterprise
-----------------------------------------------------------------------------
