Re: [PATCH 3/4] dmioem: Update HPE OEM Type 238

[Jean Delvare]

Hi Jerry,

On Wed, 17 Apr 2024 10:22:56 -0600, Jerry Hoemann wrote:
> New field: PCI Segment Number.
> 
> Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>
> ---
>  dmioem.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/dmioem.c b/dmioem.c
> index 71bd019..e03d1eb 100644
> --- a/dmioem.c
> +++ b/dmioem.c
> @@ -1345,6 +1345,7 @@ static int dmi_decode_hp(const struct dmi_header *h)
>                        *  0x0C  | Parent Hub | BYTE  | Instance number of 
> internal Hub
>                        *  0x0D  | Port Speed | BYTE  | Enumerated value of 
> speed configured by BIOS
>                        *  0x0E  | Device Path| STRING| UEFI Device Path of 
> USB endpoint
> +                      *  0x0F  | PCI Seg    | BYTE  | PCI Segment number of 
> the USB controller

Are you sure this is a byte? Traditionally the PCI segment number (or
domain number) is a 16-bit value.

>                        */
>                       if (gen < G9) return 0;
>                       pr_handle_name("%s Proliant USB Port Connector 
> Correlation Record", company);
> @@ -1362,6 +1363,8 @@ static int dmi_decode_hp(const struct dmi_header *h)
>                               pr_attr("Parent Hub Port Instance", "N/A");
>                       dmi_hp_238_speed("Port Speed Capability", data[0xD]);
>                       pr_attr("Device Path", "%s", dmi_string(h, data[0xE]));
> +                     if (h->length < 0x10) break;
> +                     pr_attr("PCI Segment", "%b", data[0xF]);

How portable is %b? Your code builds without a warning, but the
printf() man page does not mention this format at all.

One suggestion I would have is to add the PCI Segment as a prefix to
the "PCI Device" field printed before. This would display the PCI
device the same way "lspci -D" presents them. Something like:

        if (h->length < 0x10)
                pr_attr("PCI Device", "%02x:%02x.%x", data[0x6],
                        data[0x7] >> 3, data[0x7] & 0x7);
        else
                pr_attr("PCI Device", "%04x:%02x:%02x.%x",
                        data[0xF], data[0x6], data[0x7] >> 3,
                        data[0x7] & 0x7);

What do you think?


>                       break;
>  
>               case 239:

Thanks,
-- 
Jean Delvare
SUSE L3 Support