Re: [PATCH] smbios: Fix SMBIOS tables

[Ilias Apalodimas <ilias.apalodimas@linaro.org>]

Hi Simon, 

> > ---
[...]
> > This depends on https://lists.denx.de/pipermail/u-boot/2021-June/451761.html
> >  lib/smbios.c | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> 
> It is strange that all the boards that defined the old CONFIG set it
> to "", meaning that an empty string is used. Was that just wrong?

Yea I think so. In fact Heinrich fixed an identical error due to that on 
00a871d34e2f

> 
> In your patch you are using 'Unknown' and 'Unknown Product'. Should it
> use CONFIG_SYS_VENDOR and CONFIG_SYS_BOARD instead?

I don't have an issue with that, as long as they are defined for every board.
I think already Tom pulled this though, but I can send a follow up.

> 
> Or should we actually fail (and return an error code), and require the
> properties to be set by the board? It does not seem very useful to
> have a meaningless string. Are these SMBIOS values ignored in Linux?
> 

Well the problem is that those tables are marked as mandatory (section 6.2)
So failing one would mean disable the entire thing.  Why dont we do something
less intrusive?  I can send a follow up, popping a warning 'fix your dts
and/or CONFIG_SYS_VENDOR/CONFIG_SYS_BOARD'.  Then we can rid of the fallback
but keep the warning for future boards.

I am not aware of all the cases linux uses those.  I found the problem trying
to enable fwupd and specifically the EFI capsule updates for the firmware.  In
that case, fwupd is trying to find the 'EFI bit' in 'BIOS Characteristics
Extension Byte 2'.  There were two things wrong, the bit was wrong and the
tables were not installed at all.  With the two patches applied fwupd seems
happy.

Cheers
/Ilias

> >
> > diff --git a/lib/smbios.c b/lib/smbios.c
> > index abdd157a7084..e2c6b1a44ee3 100644
> > --- a/lib/smbios.c
> > +++ b/lib/smbios.c
> > @@ -259,7 +259,11 @@ static int smbios_write_type1(ulong *current, int handle,
> >         fill_smbios_header(t, SMBIOS_SYSTEM_INFORMATION, len, handle);
> >         smbios_set_eos(ctx, t->eos);
> >         t->manufacturer = smbios_add_prop(ctx, "manufacturer");
> > +       if (!t->manufacturer)
> > +               t->manufacturer = smbios_add_string(ctx, "Unknown");
> >         t->product_name = smbios_add_prop(ctx, "product");
> > +       if (!t->product_name)
> > +               t->product_name = smbios_add_string(ctx, "Unknown Product");
> >         t->version = smbios_add_prop_si(ctx, "version",
> >                                         SYSINFO_ID_SMBIOS_SYSTEM_VERSION);
> >         if (serial_str) {
> > @@ -289,7 +293,11 @@ static int smbios_write_type2(ulong *current, int handle,
> >         fill_smbios_header(t, SMBIOS_BOARD_INFORMATION, len, handle);
> >         smbios_set_eos(ctx, t->eos);
> >         t->manufacturer = smbios_add_prop(ctx, "manufacturer");
> > +       if (!t->manufacturer)
> > +               t->manufacturer = smbios_add_string(ctx, "Unknown");
> >         t->product_name = smbios_add_prop(ctx, "product");
> > +       if (!t->product_name)
> > +               t->product_name = smbios_add_string(ctx, "Unknown Product");
> >         t->version = smbios_add_prop_si(ctx, "version",
> >                                         SYSINFO_ID_SMBIOS_BASEBOARD_VERSION);
> >         t->asset_tag_number = smbios_add_prop(ctx, "asset-tag");
> > @@ -314,6 +322,8 @@ static int smbios_write_type3(ulong *current, int handle,
> >         fill_smbios_header(t, SMBIOS_SYSTEM_ENCLOSURE, len, handle);
> >         smbios_set_eos(ctx, t->eos);
> >         t->manufacturer = smbios_add_prop(ctx, "manufacturer");
> > +       if (!t->manufacturer)
> > +               t->manufacturer = smbios_add_string(ctx, "Unknown");
> >         t->chassis_type = SMBIOS_ENCLOSURE_DESKTOP;
> >         t->bootup_state = SMBIOS_STATE_SAFE;
> >         t->power_supply_state = SMBIOS_STATE_SAFE;
> > --
> > 2.32.0.rc0
> >
> 
> Regrads,
> Simon