Re: [dmidecode] OEM type 236 for HPe Gen10(+) servers

[Jean Delvare]

Hi Erwan,

On Mon, 9 Nov 2020 13:03:31 +0100, Erwan Velu wrote:
> I rebased my patch on top of yours and it works perfectly.
> Please find attached the v2 of my patch which is now into the hpe specific
> case.

Thanks for your contribution. Review:

> HPE servers encodes some information about the HDD backplane into type 236.
> 
> A typical output looks like:
> 
>       Handle 0x002F, DMI type 236, 21 bytes
>       HPE HDD Backplane FRU Information
>               FRU I2C Address: 0xAE
>               Box Number: 4
>               NVRAM ID: 0x109
>               Sas Expander WWID: 0x0
>               Total SAS Bays: 4
> 
> Signed-off-by: Erwan Velu <e.velu@criteo.com>
> ---
>  dmioem.c | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/dmioem.c b/dmioem.c
> index 60b667416563..7d8931500619 100644
> --- a/dmioem.c
> +++ b/dmioem.c
> @@ -282,6 +282,32 @@ static int dmi_decode_hp(const struct dmi_header *h)
>                       pr_subattr("UEFI", "%s", feat & 0x1400 ? "Yes" : "No");
>                       break;
>  
> +             case 236:
> +                     /*
> +                      * Vendor Specific: HPE ProLiant HDD Backplane
> +                      *
> +                      *
> +                      * Offset |  Name      | Width | Description
> +                      * ---------------------------------------
> +                      *  0x00  | I2C Address| BYTE  | Backplane FRU I2C 
> Address
> +                      *  0x01  | Box Number | WORD  | Backplane Box Number
> +                      *  0x03  | NVRAM ID   | WORD  | Backplane NVRAM Id

Id -> ID for consistency.

> +                      *  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".

> +                      *  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)?

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.

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.


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

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

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

> +                     break;
> +
>               default:
>                       return 0;
>       }

Thanks,
-- 
Jean Delvare
SUSE L3 Support