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

[Jerry Hoemann]

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.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.

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
-----------------------------------------------------------------------------