>
> Id -> ID for consistency.
>
Fixed.
> + * 0x05 | WWID | 64B | SAS Expander WWWID
>
> Following the convention that is used in the other tables in this
> function, the "B" stands for bytes, not bits, so that should be "8B".
>
I fixed with QWORD which sounds more accurate right ?
>
> > + * 0x0D | Total Bays | WORD | Total SAS Bays
> > + */
> > + pr_handle_name("%s HDD Backplane FRU Information",
> company);
> > +
> > + /* If the record isn't 0x15, that's suspicious */
> > + if (h->length != 0x15) break;
>
> If I counted right, you actually only decode 4 + 15 = 19 bytes. Any
> idea what's left in the last 2 bytes that lead to length = 21 (0x15)?
>
I have to admit I reversed the output, with their agreement, of an HPe
tooling so I might got wrong here.
HPe didn't share the spec to me on this.
So the first byte is maybe at 0x3 and not 0x4, this could explain 1 missing
byte.
About the Total Bays, the size of the structure is unknown to me, I
suspected a WORD as it worked on my system.
That's possibly wrong.
Let's hope to see HPe clarifying this.
>
> FWIW, I do have several DMI table dumps from a HP ProLiant servers
> (BL460c G9, DL385 G10 Plus) where the structure length is 17 (0x11)
> bytes. Without your patch, it looks like:
>
> OEM-specific Type
> Header and Data:
> EC 11 B5 00 A0 01 00 AC 00 00 00 00 00 00 00 00
> 00
>
> The bytes which are present make sense when decoded with the table you
> provided above, just the "Total Bays" field is missing. So I think you
> should make your code tolerant to this case, otherwise we'll fail to
> decode type 236 on many HP/HPE systems.
>
Ok, that's a version of type 236 I didn't saw.
I'd love to see HPe sharing here what are the fields here...
>
> The usual way to deal with variable length in dmidecode is to check for
> a minimum length at start, and then add conditional breaks for all
> other length values which are known to be used in practice.
>
Yeah I see that but as long as I have no idea of how the fields are encoded
in your case, I could be wrong at decoding...
>
> > +
> > + u8 *backplane = data + 0x4;
> > + pr_attr("FRU I2C Address", "0x%X", backplane[0x0]);
>
> I2C addresses are 7-bit numbers. For all the type 236 examples I've seen
> so far, the values are even numbers in the 0xA0-0xAE range, which does
> NOT fit in 7 bits. I suspect the 7-bit value is left-aligned (which
> happens often in the I2C literature, because that's how the address is
> sent on the wire, with bit 0 representing the transfer direction).
>
> Therefore you should right-shift the value by 1 bit before you print
> it, so that it matches the actual I2C address.
>
I see what you mean but in the sample I have from HPe, 0xAE is read as
0xAE, I don't see any shift in their usage...
Maybe they code it in 10bits so I should consider the byte at position 3
while I starts at 4...
>
> > + pr_attr("Box Number", "%d", WORD(backplane + 0x1));
> > + pr_attr("NVRAM ID", "0x%X", WORD(backplane + 0x3));
>
> I'm not familiar with NVRAM technology, is it usual to represent the
> NVRAM ID as an hexadecimal number? I found examples of both on the web
> so I'm a bit confused as what is standard.
>
One more time, I've silly replicating what HPe showed me on this type. They
report it this way.
>
> > + pr_attr("Sas Expander WWID", "0x%X",
> QWORD(backplane + 0x5));
> > + pr_attr("Total SAS Bays", "%d", WORD(backplane +
> 0x0D));
>
> That one is suspicious. The values I see in my examples are all huge
> (1032, 514). I have a hard time imagining a back-end with that many
> bays. Considering that 1032 = 1024 + 8 and 514 = 512 + 2, I suspect
> that the field is actually composed of 2 values, with the higher bits
> meaning something else (or maybe these are actually 2 one-byte fields
> rather than one 2-byte field?) Can you please double check with your
> contact at HP?
>
> One more time, I have no clue about the real data structures, just guessed
from the sample.
I'll ask them to clarify
Thanks for the review,
Erwan,
_______________________________________________
https://lists.nongnu.org/mailman/listinfo/dmidecode-devel
