[PATCH] smbios: Try CONFIG_SYS_ options before using "Unknow" as a value

[Ilias Apalodimas]

Hi Heinrich, 

On Mon, Jul 05, 2021 at 12:25:47PM +0200, Heinrich Schuchardt wrote:
> On 7/5/21 10:49 AM, Ilias Apalodimas wrote:
> > SMBIOS entries for manufacturer and board can't be empty, as the spec
> > explicitly requires a value.
> > Instead of passing "Unknown" and "Unknown product" for the manufacturer and
> > product name if the .dts options are missing, try to use CONFIG_SYS_VENDOR
> > and CONFIG_SYS_BOARD respectively.  If those are not found either warn the
> > user at runtime and use "Unknown" for both entries.
> > 
> > Signed-off-by: Ilias Apalodimas <ilias.apalodimas at linaro.org>
> 
> These defaults don't make any sense to me:
> 
> Hardkernel Odroid C2
> CONFIG_SYS_VENDOR="amlogic"
> CONFIG_SYS_BOARD="p200"
> 
> Orange Pi PC
> CONFIG_SYS_BOARD="sunxi"
> CONFIG_SYS_CONFIG_NAME="sun8i"
> 
> Solidrun MacchiatoBIN
> CONFIG_SYS_VENDOR="Marvell"
> CONFIG_SYS_BOARD="mvebu_armada-8k"
> 
> None of these CONFIG_SYS_VENDOR and CONFIG_SYS_CONFIG_NAME values match
> the manufacturer nor the board name.

The point here is to provide a viable fallback in case the .dts options
are missing and define the semantics we expect from U-Boot.  
I do think Simon did the right thing by deleting the specific
SMBIOS Kconfig options for these values.  We should have a common place
keeping those instead of scattering around config options. 
If the .config values don't make sense then the SMBIOS wont either.
Perhaps we should just fix the .config options properly?

> 
> If you want a default for the board name, use the 'model' property of
> the devicetree.

That would be require a different patch. Right now the code *already* uses 
'product=' from the device tree.  If everyone is fine I can send a different 
patch, using 'model=' instead of that.  I don't think parsing multiple .dts
values makes sense here.

Cheers
/Ilias

> 
> Hardkernel Odroid C2:
> model = "Hardkernel ODROID-C2"
> 
> Orange Pi PC
> model = "Xunlong Orange Pi PC"
> 
> Solidrun MacchiatoBIN:
> model = "MACCHIATOBin-8040"
> 
> Best regards
> 
> Heinrich
> 
> > ---
> >   lib/smbios.c | 14 ++++++++------
> >   1 file changed, 8 insertions(+), 6 deletions(-)
> > 
> > diff --git a/lib/smbios.c b/lib/smbios.c
> > index b52e125eeb14..d1997ce70d19 100644
> > --- a/lib/smbios.c
> > +++ b/lib/smbios.c
> > @@ -79,8 +79,10 @@ static int smbios_add_string(struct smbios_ctx *ctx, const char *str)
> >   	int i = 1;
> >   	char *p = ctx->eos;
> > 
> > -	if (!*str)
> > +	if (!*str) {
> >   		str = "Unknown";
> > +		log_warning("Empty string found. Please fix your DTS and/or CONFIG_SYS_* options");
> > +	}
> > 
> >   	for (;;) {
> >   		if (!*p) {
> > @@ -259,10 +261,10 @@ static int smbios_write_type1(ulong *current, int 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->manufacturer = smbios_add_string(ctx, CONFIG_SYS_VENDOR);
> >   	t->product_name = smbios_add_prop(ctx, "product");
> >   	if (!t->product_name)
> > -		t->product_name = smbios_add_string(ctx, "Unknown Product");
> > +		t->product_name = smbios_add_string(ctx, CONFIG_SYS_BOARD);
> >   	t->version = smbios_add_prop_si(ctx, "version",
> >   					SYSINFO_ID_SMBIOS_SYSTEM_VERSION);
> >   	if (serial_str) {
> > @@ -293,10 +295,10 @@ static int smbios_write_type2(ulong *current, int 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->manufacturer = smbios_add_string(ctx, CONFIG_SYS_VENDOR);
> >   	t->product_name = smbios_add_prop(ctx, "product");
> >   	if (!t->product_name)
> > -		t->product_name = smbios_add_string(ctx, "Unknown Product");
> > +		t->product_name = smbios_add_string(ctx, CONFIG_SYS_BOARD);
> >   	t->version = smbios_add_prop_si(ctx, "version",
> >   					SYSINFO_ID_SMBIOS_BASEBOARD_VERSION);
> >   	t->asset_tag_number = smbios_add_prop(ctx, "asset-tag");
> > @@ -322,7 +324,7 @@ static int smbios_write_type3(ulong *current, int 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->manufacturer = smbios_add_string(ctx, CONFIG_SYS_VENDOR);
> >   	t->chassis_type = SMBIOS_ENCLOSURE_DESKTOP;
> >   	t->bootup_state = SMBIOS_STATE_SAFE;
> >   	t->power_supply_state = SMBIOS_STATE_SAFE;
> > 
>