Re: [dmidecode] [PATCH 4/4] dmioem: Decode HPE OEM record 236

[Jean Delvare]

On Thu, 19 Nov 2020 12:55:16 -0700, Jerry Hoemann wrote:
> HPE OEM record 236 describes the HPE ProLiant HDD Backplane.  A non-backward
> compatible change was introduced with Gen10 Plus which retracted three
> fields and made them "Reserved."
> 
> Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>
> ---
>  dmioem.c | 41 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 41 insertions(+)
> 
> diff --git a/dmioem.c b/dmioem.c
> index 16f4488..8ad24f1 100644
> --- a/dmioem.c
> +++ b/dmioem.c
> @@ -314,6 +314,47 @@ static int dmi_decode_hp(const struct dmi_header *h, 
> const char *product)
>                                                  &data[0x08]);
>                       break;
>  
> +             case 236:
> +                     /*
> +                      * Vendor Specific: HPE ProLiant HDD Backplane
> +                      *
> +                      *

Please remove one "blank" line.

> +                      * Offset |  Name      | Width | Description
> +                      * ---------------------------------------
> +                      *  0x00  | Type       | BYTE  | 0xEC, HDD Backplane
> +                      *  0x01  | Length     | BYTE  | Length of structure
> +                      *  0x02  | Handle     | WORD  | Unique handle
> +                      *  0x04  | I2C Address| BYTE  | Backplane FRU I2C 
> Address
> +                      *  0x05  | Box Number | WORD  | Backplane Box Number
> +                      *  0x07  | NVRAM ID   | WORD  | Backplane NVRAM Id

Id -> ID for consistency.

> +                      *  0x09  | WWID       | QWORD | SAS Expander WWWID

That's one too many "W", isn't it?

> +                      *  0x11  | Total Bays | BYTE  | Total SAS Bays
> +                      *  0x12  | A0 Bays    | BYTE  | (depricated) Number of 
> SAS drive bays behind port 0xA0
> +                      *  0x13  | A2 Bays    | BYTE  | (depricated) Number of 
> SAS drive bays behind port 0xA2
> +                      *  0x14  | Name       | STRING| (depricated) Backplane 
> Name

Spelling: "deprecated".

> +                      */
> +                     pr_handle_name("%s HDD Backplane FRU Information", 
> company);
> +
> +

Please avoid double blank lines.

> +                     pr_attr("FRU I2C Address", "0x%X raw(0x%X)", data[0x4] 
> >> 1, data[0x4]);
> +                     pr_attr("Box Number", "%d", WORD(data + 0x5));
> +                     pr_attr("NVRAM ID", "0x%X", WORD(data + 0x7));
> +                     if (h->length < 0x11) break;
> +                     pr_attr("Sas Expander WWID", "0x%X", QWORD(data + 0x9));

"SAS" for consistency.

> +                     if (h->length < 0x12) break;
> +                     pr_attr("Total SAS Bays", "%d", data[0x11]);
> +                     if (h->length < 0x15) break;
> +                     if (gen < G10P) {
> +                             pr_attr("A0 Bay Count", "%d", data[0x12]);
> +                             pr_attr("A2 Bay Count", "%d", data[0x13]);
> +                             pr_attr("Backplane Name", "%s", dmi_string(h, 
> data[0x14]));
> +                     } else {
> +                             pr_attr("Reserved", "%d", data[0x12]);
> +                             pr_attr("Reserved", "%d", data[0x13]);
> +                             pr_attr("Reserved", "%d", data[0x14]);

Please just skip the fields if they are not meaningful.

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

Thanks for your contribution, this is very appreciated.

-- 
Jean Delvare
SUSE L3 Support