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

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

[Ilias Apalodimas]

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@linaro.org>
---
 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;
-- 
2.32.0.rc0

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

[Bin Meng]

On Mon, Jul 5, 2021 at 4:49 PM Ilias Apalodimas
<ilias.apalodimas@linaro.org> 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@linaro.org>
> ---
>  lib/smbios.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
>

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

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

[Heinrich Schuchardt]

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@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.

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

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;
>

Re: [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@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;
> > 
> 

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

[Simon Glass]

On Mon, 5 Jul 2021 at 02:49, Ilias Apalodimas
<ilias.apalodimas@linaro.org> 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@linaro.org>
> ---
>  lib/smbios.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

I'm not quite sure what this is based on, but perhaps the code changed
without me noticing.

How about adding some tests for this code?

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

[Ilias Apalodimas]

Simon, Bin, 

As Heinrich points out some of the CONFIG_SYS_* values are not representing
the devices properly. Maybe it's a better idea to only check the .dts?
If the values are missing we can pop a runtime warning and keep using
'Unknown'.

On Mon, Jul 05, 2021 at 09:29:57AM -0600, Simon Glass wrote:
> On Mon, 5 Jul 2021 at 02:49, Ilias Apalodimas
> <ilias.apalodimas@linaro.org> 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@linaro.org>
> > ---
> >  lib/smbios.c | 14 ++++++++------
> >  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> Reviewed-by: Simon Glass <sjg@chromium.org>
> 
> I'm not quite sure what this is based on, but perhaps the code changed
> without me noticing.
> 
> How about adding some tests for this code?

How? The only valid test we can do is that the values we expect are not "". 
But as the code is right now it will always replace "" with "Unknown"


Thanks
/Ilias

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

[Simon Glass]

Hi Ilias,

On Mon, 5 Jul 2021 at 11:31, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> Simon, Bin,
>
> As Heinrich points out some of the CONFIG_SYS_* values are not representing
> the devices properly. Maybe it's a better idea to only check the .dts?
> If the values are missing we can pop a runtime warning and keep using
> 'Unknown'.

I think we should pick one of two options:

1. Fail if we don't have the info
2. Try to use CONFIG_SYS_... as the old code did

As mentioned earlier, I actually prefer 1, since then it means boards
must add the info in the DT (probably) which indicates that someone
has actually thought about it.

But if we go with 2, we may as well go all the way, and use all the
info at our disposal.

>
> On Mon, Jul 05, 2021 at 09:29:57AM -0600, Simon Glass wrote:
> > On Mon, 5 Jul 2021 at 02:49, Ilias Apalodimas
> > <ilias.apalodimas@linaro.org> 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@linaro.org>
> > > ---
> > >  lib/smbios.c | 14 ++++++++------
> > >  1 file changed, 8 insertions(+), 6 deletions(-)
> >
> > Reviewed-by: Simon Glass <sjg@chromium.org>
> >
> > I'm not quite sure what this is based on, but perhaps the code changed
> > without me noticing.
> >
> > How about adding some tests for this code?
>
> How? The only valid test we can do is that the values we expect are not "".
> But as the code is right now it will always replace "" with "Unknown"

I just mean we need unit tests for generation of smbios tables....it
is complicated enough.

Regards,
Simon