Re: [dmidecode] [PATCH] dmioem: Reflect HPE's new company name.

[Jean Delvare]

Hi Jerry,

Sorry for the delay, I was on vacation. Thanks Erwan for the
preliminary review - overall I agree, the patch looks essentially good.
To the details now...

On Fri, 25 Aug 2017 17:54:43 -0600, Jerry Hoemann wrote:
> After Hewlett Packard Enterprise split from Hewlett-Packard, DMI OEM
> tables reflect the new company name.  Gen10 and subsequent systems will
> use HPE.  Gen9 and prior systems continue to use the old "HP" name.
> 
> Signed-off-by: Jerry Hoemann <address@hidden>
> Reviewed-by: Randy Wright <address@hidden>
> ---
>  dmioem.c | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/dmioem.c b/dmioem.c
> index 034ad9f..a09b6e5 100644
> --- a/dmioem.c
> +++ b/dmioem.c
> @@ -35,6 +35,7 @@ enum DMI_VENDORS
>       VENDOR_UNKNOWN,
>       VENDOR_HP,
>       VENDOR_ACER,
> +     VENDOR_HPE,
>  };
>  
>  static enum DMI_VENDORS dmi_vendor = VENDOR_UNKNOWN;
> @@ -58,6 +59,8 @@ void dmi_set_vendor(const char *s)
>  
>       if (strncmp(s, "HP", len) == 0 || strncmp(s, "Hewlett-Packard", len) == 
> 0)
>               dmi_vendor = VENDOR_HP;
> +     else if (strncmp(s, "HPE",len) == 0 || strncmp(s, "Hewlett Packard 
> Enterprise", len) == 0)

Coding style: missing space after comma.

> +             dmi_vendor = VENDOR_HPE;
>       else if (strncmp(s, "Acer", len) == 0)
>               dmi_vendor = VENDOR_ACER;
>  }
> @@ -98,14 +101,15 @@ static int dmi_decode_hp(const struct dmi_header *h)
>       u8 *data = h->data;
>       int nic, ptr;
>       u32 feat;
> +     const char *company = (dmi_vendor == VENDOR_HP) ? "HP": "HPE";

Coding style: missing space before colon.

>  
>       switch (h->type)
>       {
>               case 204:
>                       /*
> -                      * Vendor Specific: HP ProLiant System/Rack Locator
> +                      * Vendor Specific: HPE ProLiant System/Rack Locator
>                        */
> -                     printf("HP ProLiant System/Rack Locator\n");
> +                     printf("%s ProLiant System/Rack Locator\n", company);
>                       if (h->length < 0x0B) break;
>                       printf("\tRack Name: %s\n", dmi_string(h, data[0x04]));
>                       printf("\tEnclosure Name: %s\n", dmi_string(h, 
> data[0x05]));
> @@ -155,7 +159,7 @@ static int dmi_decode_hp(const struct dmi_header *h)
>  

Did you leave cases 209 and 221 out on purpose?

>               case 233:
>                       /*
> -                      * Vendor Specific: HP ProLiant NIC MAC Information
> +                      * Vendor Specific: HPE ProLiant NIC MAC Information
>                        *
>                        * This prints the BIOS NIC number,
>                        * PCI bus/device/function, and MAC address
> @@ -171,7 +175,7 @@ static int dmi_decode_hp(const struct dmi_header *h)
>                        *  0x08  |   MAC  | 32B   | MAC addr padded w/ 0s
>                        *  0x28  | Port No| BYTE  | Each NIC maps to a Port
>                        */
> -                     printf("HP BIOS PXE NIC PCI and MAC Information\n");
> +                     printf("%s BIOS PXE NIC PCI and MAC Information\n", 
> company);
>                       if (h->length < 0x0E) break;
>                       /* If the record isn't long enough, we don't have an ID
>                        * use 0xFF to use the internal counter.
> @@ -187,7 +191,7 @@ static int dmi_decode_hp(const struct dmi_header *h)
>                        *
>                        * Source: hpwdt kernel driver
>                        */
> -                     printf("HP 64-bit CRU Information\n");
> +                     printf("%s 64-bit CRU Information\n", company);
>                       if (h->length < 0x18) break;
>                       printf("\tSignature: 0x%08x", DWORD(data + 0x04));
>                       if (is_printable(data + 0x04, 4))
> @@ -212,7 +216,7 @@ static int dmi_decode_hp(const struct dmi_header *h)
>                        *
>                        * Source: hpwdt kernel driver
>                        */
> -                     printf("HP ProLiant Information\n");
> +                     printf("%s ProLiant Information\n", company);
>                       if (h->length < 0x08) break;
>                       printf("\tPower Features: 0x%08x\n", DWORD(data + 
> 0x04));
>                       if (h->length < 0x0C) break;
> @@ -281,6 +285,7 @@ int dmi_decode_oem(const struct dmi_header *h)
>       switch (dmi_vendor)
>       {
>               case VENDOR_HP:
> +             case VENDOR_HPE:
>                       return dmi_decode_hp(h);
>               case VENDOR_ACER:
>                       return dmi_decode_acer(h);

For the first 2, you updated "HP" to "HPE" in the heading comment. For
the last 2 you did not. Is it on purpose, or an overlook?

(I don't really care if we update these comments or not, but I like
consistency.)

-- 
Jean Delvare
SUSE L3 Support