Hi Jerry,
On Mon, 24 Feb 2025 17:32:02 -0700, Jerry Hoemann wrote:
> Add PCIe Riser.
This would deserve additional verbosity, considering that "PCI Riser"
was already a supported board type.
>
> Signed-off-by: Jerry Hoemann <jerry.hoem...@hpe.com>
> ---
> dmioem.c | 28 ++++++++++++++++++++++++++++
> 1 file changed, 28 insertions(+)
>
> diff --git a/dmioem.c b/dmioem.c
> index c7af5d6..f330cbe 100644
> --- a/dmioem.c
> +++ b/dmioem.c
> @@ -962,6 +962,23 @@ static void dmi_hp_245_pcie_riser(const struct
> dmi_header *h)
> pr_attr("Riser Name", dmi_string(h, data[0x08]));
> }
>
> +static void dmi_hp_245_pcie_mhs_riser(const struct dmi_header *h, int len)
> +{
> + u8 *data = h->data;
> + u8 i, count;
> +
> + pr_attr("Board Type", "PCIe Riser (MHS Platform)");
You are missing the usual safety length check here, to ensure that you
don't read beyond the end of the record.
> + pr_attr("Risder ID", "%x", data[0x05]);
Typo: Riser.
The Riser ID was printed as a decimal number in the non-MHS case. Is
the change to hexadecimal on purpose? This could cause confusion,
especially considering that there's no 0x prefix to make it clear it's
an hexadecimal value.
> + if (data[0x06])
> + pr_attr("Firmware Version", "%x.%x", data[0x06], data[0x07]);
> + pr_attr("Downgradable", "%s", data[0x08] & 0x01 ? "yes" : "no");
Please upper-case the first letter of each value string for consistency.
> + pr_attr("Riser Name", dmi_string(h, data[0x09]));
> + count = data[0x0A];
> + pr_attr("Slot Count", "%d", count);
> + for (i = 0; (i < count) && (i + 0x0B < len); i++)
Parentheses around the tests aren't needed, as && has lower precedence.
I'd prefer "0x0B + i" for consistency with the following line.
> + pr_attr("Slot ID", "0x%x", data[0X0B + i]);
0x0B (Lower-case x) for consistency.
Considering the current work to add support for JSON output to
dmidecode, printing several attributes using the same name ("Slot ID")
is going to be a problem. You should either customize the attribute
name (by including a counter) or (probably cleaner) convert this to a
list (printed with pr_list_start/pr_list_item/pr_list_end).
> +}
> +
> static int dmi_decode_hp(const struct dmi_header *h)
> {
> u8 *data = h->data;
> @@ -1717,11 +1734,22 @@ static int dmi_decode_hp(const struct dmi_header *h)
> * 0x06 | Riser ID | BYTE |
> * 0x07 | CPLD Vers | BTYE | 0-> No CPLD. Bits
> [7][6:0] Release:Vers
> * 0x08 | Riser Name | STRING|
> + *
> + * If Board Type == 1
With this addition, an earlier comment becomes obsolete:
* 0x04 | Board Type | WORD | 0: PCIe Riser, Other
Reserved
It should either be updated, or changed to an invitation to look for
the valid board type values further down.
> + * 0x05 | Riser ID | BYTE |
> + * 0x06 | Riser FW Major | BYTE |
> + * 0x07 | Riser FW Minor | BYTE |
> + * 0x08 | Misc Attr | BYTE |
> + * 0x09 | Riser Name | STRING|
> + * 0x0A | Slot Count | BYTE |
> + * 0x0B | Slot ID | Varies| One per slot.
No trailing dot for consistency.
> */
> pr_handle_name("%s ProLiant Extension Board Inventory
> Record", company);
> if (h->length < 0x05) break;
> if (data[0x04] == 0)
> dmi_hp_245_pcie_riser(h);
> + if (data[0x04] == 1)
An "else" would be a welcome optimization (or you may convert to a
switch/case construct if you prefer).
> + dmi_hp_245_pcie_mhs_riser(h, h->length);
Please only pass h, and get h->length inside the function. This is more
efficient, and consistent with the rest of the code.
> break;
>
> default:
Thanks,
--
Jean Delvare
SUSE L3 Support
