Re: [RFC] dmioem: Decode HPE OEM Record 242

[Jean Delvare]

Hi Jerry,

On Mon, 2022-10-03 at 11:22 -0600, Jerry Hoemann wrote:
> On Fri, Sep 30, 2022 at 02:51:19PM +0200, Jean Delvare wrote:
> > QWORD returns a struct. I doubt this can be reliably passed to printf?
> > TBH I'm surprised gcc doesn't complain. Maybe it's OK...
> 
> Hmm,  Using the -E gcc flag, this gets expanded to:
> 
> pr_attr("Poweron", "%ld hours", (*(const u64 *)(data + 0x13)));
> 
> Note: field 0x007 of record 242 and field 0x09 of record 236 also do a
> similar print statement.  So, if the above is incorrect, we may need to
> fix up the other places as well.

If it works, let's keep it as is for simplicity. We can fix it up later
if anyone reports an issue with this construct.

> > (...)
> > In which unit is this value expressed? On my sample, the value is 25,
> 
> spec says Watts.

Please add " W" to the format string then, for clarity.

> > (...)
> > Any reason why you do not introduce a helper function for that one, as
> > we typically do for such enumerated fields?
> 
> Jugdgment call.  There aren't very many entries and all the entries are
> short. So, thought this was eaiser to read.  YMMV.
> 
> Do you want me to change to helper?

I would for consistency, but no strong opinion actually, you can keep
it as is if you like it better that way.

> > (...)
> > This field is redundant with the field at offset 0x0F. Does your
> > specification clarify what to do when both fields are present? I
> > don't think we want to print both values.
> 
> field 0f is 32 bits field representing MB -> effective 52 bits number.
> field 2c is 64 bits field represent byte -> efiecitve 64 bit number.
>   
> So. new field gives more head room.  My examples report same size
> (in different units.)  But at some point that need not be so.
> 
> I will display field 0x2C when available instead of field 0x0F.

Yes, thanks.

> > There is a similar situation for memory sizes (DMI type 16, fields 0x07
> > and 0x0F, as well as DMI type 17, fields 0x0C and 0x1C). I think we
> > should handle the situation in the same way here.
> > 
> > It might also make sense to use dmi_print_memory_size() to display the
> > value in a more human-friendly way. I'm not sure if disk sizes are
> > usually a power of 2 (or a multiple of some large power of 2) as memory
> > modules tend to be. I guess not, then we could print both the exact
> > value, and a rounded value using the most suitable unit.
> > 
> > > +                 pr_attr("Black Size", "%u bytes", DWORD(data + 0x34));
> > 
> > It might make sense to print this in a human-friendly unit.
> 
> I should also spell block correctly.  :)

Ah ah, didn't notice.

> All my examples have block size == 512 bytes.
> 
> Is there a "size" print helper that takes DWORD?  I didn't see one.
> [ dmi_print_memory_size takes a u64 which isn't compatible. ]

Doesn't seem so. We have dmi_memory_device_extended_size() but it's for
larger values (1 MB minimum).

Thinking about it some more, I suppose there are only 2 realistic
values, 512 and 4096. Both are OK when expressed in bytes, so let's
keep it simple for now. If larger block sizes are introduced in the
future, we'll change it then.

> > (...)
> > Hmm, so 2 RPM is OK, but 1 RPM is not? Odd.
> 
> These are special cases:  0 -> Not reported.  1 -> SSD.
> 
> So, I just decided not to print anything in these two cases.
> 
> Do you want me to print something in one or both of these cases?

No need.

> Or just comment the code better?

Would be nice, yes.

-- 
Jean Delvare
SUSE L3 Support