On Thu, Mar 13, 2025 at 11:03:38AM +0100, Jean Delvare wrote:
> 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.
Fixed.
if (h->length < 0x0C) return;
>
> > + pr_attr("Risder ID", "%x", data[0x05]);
>
> Typo: Riser.
Fixed.
>
> 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.
Fixed. print as decimal to make consistent with prior usage.
>
> > + 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.
Fixed: Printing Yes|No
> > + 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 would like to retain as I admit to not having memorized all 17 levels
of C operator precedence. In fact, I going to add another pair. :)
>
> 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.
Fixed.
>
> 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).
Converted to: 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
Fixed. Description changed to: See Below
>
> 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.
Removed.
>
> > */
> > 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).
Changed to switch statement.
>
> > + 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.
Done.
>
> > break;
> >
> > default:
>
> Thanks,
> --
> Jean Delvare
> SUSE L3 Support
--
-----------------------------------------------------------------------------
Jerry Hoemann Software Engineer Hewlett Packard Enterprise
-----------------------------------------------------------------------------
