* [PATCH 1/2] smbios: Simplify reporting of unknown values
@ 2022-09-06 13:44 Ilias Apalodimas
2022-09-06 13:44 ` [PATCH 2/2] smbios: Fallback to the default DT if sysinfo nodes are missing Ilias Apalodimas
` (2 more replies)
0 siblings, 3 replies; 19+ messages in thread
From: Ilias Apalodimas @ 2022-09-06 13:44 UTC (permalink / raw)
To: u-boot; +Cc: trini, sjg, heinrich.schuchardt, pbrobinson, Ilias Apalodimas
If a value is not valid during the DT or SYSINFO parsing, we explicitly
set that to "Unknown Product" and "Unknown" for the product and
manufacturer respectively. It's cleaner if we move the checks insisde
smbios_add_string() and always report "Unknown" regardless of the missing
field.
pre-patch dmidecode
<snip>
Handle 0x0001, DMI type 1, 27 bytes
System Information
Manufacturer: Unknown
Product Name: Unknown Product
Version: Not Specified
Serial Number: Not Specified
UUID: Not Settable
Wake-up Type: Reserved
SKU Number: Not Specified
Family: Not Specified
Handle 0x0002, DMI type 2, 14 bytes
Base Board Information
Manufacturer: Unknown
Product Name: Unknown Product
Version: Not Specified
Serial Number: Not Specified
Asset Tag: Not Specified
Features:
Board is a hosting board
Location In Chassis: Not Specified
Chassis Handle: 0x0000
Type: Motherboard
<snip>
post-patch dmidecode:
Handle 0x0001, DMI type 1, 27 bytes
System Information
Manufacturer: Unknown
Product Name: Unknown
Version: Unknown
Serial Number: Unknown
UUID: Not Settable
Wake-up Type: Reserved
SKU Number: Unknown
Family: Unknown
Handle 0x0002, DMI type 2, 14 bytes
Base Board Information
Manufacturer: Unknown
Product Name: Unknown
Version: Unknown
Serial Number: Not Specified
Asset Tag: Unknown
Features:
Board is a hosting board
Location In Chassis: Not Specified
Chassis Handle: 0x0000
Type: Motherboard
Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
lib/smbios.c | 17 +++--------------
1 file changed, 3 insertions(+), 14 deletions(-)
diff --git a/lib/smbios.c b/lib/smbios.c
index d7f4999e8b2a..fcc8686993ef 100644
--- a/lib/smbios.c
+++ b/lib/smbios.c
@@ -102,7 +102,7 @@ static int smbios_add_string(struct smbios_ctx *ctx, const char *str)
int i = 1;
char *p = ctx->eos;
- if (!*str)
+ if (!str || !*str)
str = "Unknown";
for (;;) {
@@ -151,8 +151,7 @@ static int smbios_add_prop_si(struct smbios_ctx *ctx, const char *prop,
const char *str;
str = ofnode_read_string(ctx->node, prop);
- if (str)
- return smbios_add_string(ctx, str);
+ return smbios_add_string(ctx, str);
}
return 0;
@@ -231,7 +230,7 @@ static int smbios_write_type0(ulong *current, int handle,
t->vendor = smbios_add_string(ctx, "U-Boot");
t->bios_ver = smbios_add_prop(ctx, "version");
- if (!t->bios_ver)
+ if (!strcmp(ctx->last_str, "Unknown"))
t->bios_ver = smbios_add_string(ctx, PLAIN_VERSION);
if (t->bios_ver)
gd->smbios_version = ctx->last_str;
@@ -281,11 +280,7 @@ 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) {
@@ -315,11 +310,7 @@ 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");
@@ -344,8 +335,6 @@ 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.37.2
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 2/2] smbios: Fallback to the default DT if sysinfo nodes are missing
2022-09-06 13:44 [PATCH 1/2] smbios: Simplify reporting of unknown values Ilias Apalodimas
@ 2022-09-06 13:44 ` Ilias Apalodimas
2022-09-20 11:10 ` Peter Robinson
2022-09-12 18:31 ` [PATCH 1/2] smbios: Simplify reporting of unknown values Simon Glass
2022-09-20 11:09 ` Peter Robinson
2 siblings, 1 reply; 19+ messages in thread
From: Ilias Apalodimas @ 2022-09-06 13:44 UTC (permalink / raw)
To: u-boot; +Cc: trini, sjg, heinrich.schuchardt, pbrobinson, Ilias Apalodimas
In order to fill in the SMBIOS tables U-Boot currently relies on a
"u-boot,sysinfo-smbios" compatible node. This is fine for the boards
that already include such nodes. However with some recent EFI changes,
the majority of boards can boot up distros, which usually rely on
things like dmidecode etc for their reporting. For boards that
lack this special node the SMBIOS output looks like:
System Information
Manufacturer: Unknown
Product Name: Unknown
Version: Unknown
Serial Number: Unknown
UUID: Not Settable
Wake-up Type: Reserved
SKU Number: Unknown
Family: Unknown
This looks problematic since most of the info are "Unknown". The DT spec
specifies standard properties containing relevant information like
'model' and 'compatible' for which the suggested format is
<manufacturer,model>. So let's add a last resort to our current
smbios parsing. If none of the sysinfo properties are found, we can
scan the root node for 'model' and 'compatible'.
pre-patch dmidecode:
<snip>
Handle 0x0001, DMI type 1, 27 bytes
System Information
Manufacturer: Unknown
Product Name: Unknown
Version: Unknown
Serial Number: Unknown
UUID: Not Settable
Wake-up Type: Reserved
SKU Number: Unknown
Family: Unknown
Handle 0x0002, DMI type 2, 14 bytes
Base Board Information
Manufacturer: Unknown
Product Name: Unknown
Version: Unknown
Serial Number: Not Specified
Asset Tag: Unknown
Features:
Board is a hosting board
Location In Chassis: Not Specified
Chassis Handle: 0x0000
Type: Motherboard
Handle 0x0003, DMI type 3, 21 bytes
Chassis Information
Manufacturer: Unknown
Type: Desktop
Lock: Not Present
Version: Not Specified
Serial Number: Not Specified
Asset Tag: Not Specified
Boot-up State: Safe
Power Supply State: Safe
Thermal State: Safe
Security Status: None
OEM Information: 0x00000000
Height: Unspecified
Number Of Power Cords: Unspecified
Contained Elements: 0
<snip>
post-pastch dmidecode:
<snip>
Handle 0x0001, DMI type 1, 27 bytes
System Information
Manufacturer: socionext,developer-box
Product Name: Socionext Developer Box
Version: Unknown
Serial Number: Unknown
UUID: Not Settable
Wake-up Type: Reserved
SKU Number: Unknown
Family: Unknown
Handle 0x0002, DMI type 2, 14 bytes
Base Board Information
Manufacturer: socionext,developer-box
Product Name: Socionext Developer Box
Version: Unknown
Serial Number: Not Specified
Asset Tag: Unknown
Features:
Board is a hosting board
Location In Chassis: Not Specified
Chassis Handle: 0x0000
Type: Motherboard
Handle 0x0003, DMI type 3, 21 bytes
Chassis Information
Manufacturer: socionext,developer-box
Type: Desktop
Lock: Not Present
Version: Not Specified
Serial Number: Not Specified
Asset Tag: Not Specified
Boot-up State: Safe
Power Supply State: Safe
Thermal State: Safe
Security Status: None
OEM Information: 0x00000000
Height: Unspecified
Number Of Power Cords: Unspecified
Contained Elements: 0
<snip>
Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
lib/smbios.c | 41 +++++++++++++++++++++++++++++++++++++++--
1 file changed, 39 insertions(+), 2 deletions(-)
diff --git a/lib/smbios.c b/lib/smbios.c
index fcc8686993ef..f2eb961f514b 100644
--- a/lib/smbios.c
+++ b/lib/smbios.c
@@ -43,6 +43,20 @@
DECLARE_GLOBAL_DATA_PTR;
+/**
+ * struct sysifno_to_dt - Mapping of sysinfo strings to DT
+ *
+ * @sysinfo_str: sysinfo string
+ * @dt_str: DT string
+ */
+static const struct {
+ const char *sysinfo_str;
+ const char *dt_str;
+} sysifno_to_dt[] = {
+ { .sysinfo_str = "product", .dt_str = "model" },
+ { .sysinfo_str = "manufacturer", .dt_str = "compatible" },
+};
+
/**
* struct smbios_ctx - context for writing SMBIOS tables
*
@@ -87,6 +101,18 @@ struct smbios_write_method {
const char *subnode_name;
};
+static const char *convert_sysinfo_to_dt(const char *sysinfo_str)
+{
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(sysifno_to_dt); i++) {
+ if (!strcmp(sysinfo_str, sysifno_to_dt[i].sysinfo_str))
+ return sysifno_to_dt[i].dt_str;
+ }
+
+ return NULL;
+}
+
/**
* smbios_add_string() - add a string to the string area
*
@@ -148,9 +174,20 @@ static int smbios_add_prop_si(struct smbios_ctx *ctx, const char *prop,
return smbios_add_string(ctx, val);
}
if (IS_ENABLED(CONFIG_OF_CONTROL)) {
- const char *str;
+ const char *str = NULL;
- str = ofnode_read_string(ctx->node, prop);
+ /*
+ * If the node is not valid fallback and try the entire DT
+ * so we can at least fill in maufacturer and board type
+ */
+ if (!ofnode_valid(ctx->node)) {
+ const char *nprop = convert_sysinfo_to_dt(prop);
+
+ if (nprop)
+ str = ofnode_read_string(ofnode_root(), nprop);
+ } else {
+ str = ofnode_read_string(ctx->node, prop);
+ }
return smbios_add_string(ctx, str);
}
--
2.37.2
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 1/2] smbios: Simplify reporting of unknown values
2022-09-06 13:44 [PATCH 1/2] smbios: Simplify reporting of unknown values Ilias Apalodimas
2022-09-06 13:44 ` [PATCH 2/2] smbios: Fallback to the default DT if sysinfo nodes are missing Ilias Apalodimas
@ 2022-09-12 18:31 ` Simon Glass
2022-09-16 20:30 ` Ilias Apalodimas
2022-09-20 10:15 ` Peter Robinson
2022-09-20 11:09 ` Peter Robinson
2 siblings, 2 replies; 19+ messages in thread
From: Simon Glass @ 2022-09-12 18:31 UTC (permalink / raw)
To: Ilias Apalodimas
Cc: U-Boot Mailing List, Tom Rini, Heinrich Schuchardt, Peter Robinson
Hi Ilias,
On Tue, 6 Sept 2022 at 07:44, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> If a value is not valid during the DT or SYSINFO parsing, we explicitly
> set that to "Unknown Product" and "Unknown" for the product and
> manufacturer respectively. It's cleaner if we move the checks insisde
> smbios_add_string() and always report "Unknown" regardless of the missing
> field.
>
> pre-patch dmidecode
> <snip>
> Handle 0x0001, DMI type 1, 27 bytes
> System Information
> Manufacturer: Unknown
> Product Name: Unknown Product
> Version: Not Specified
> Serial Number: Not Specified
> UUID: Not Settable
> Wake-up Type: Reserved
> SKU Number: Not Specified
> Family: Not Specified
>
> Handle 0x0002, DMI type 2, 14 bytes
> Base Board Information
> Manufacturer: Unknown
> Product Name: Unknown Product
> Version: Not Specified
> Serial Number: Not Specified
> Asset Tag: Not Specified
> Features:
> Board is a hosting board
> Location In Chassis: Not Specified
> Chassis Handle: 0x0000
> Type: Motherboard
> <snip>
>
> post-patch dmidecode:
>
> Handle 0x0001, DMI type 1, 27 bytes
> System Information
> Manufacturer: Unknown
> Product Name: Unknown
> Version: Unknown
> Serial Number: Unknown
> UUID: Not Settable
> Wake-up Type: Reserved
> SKU Number: Unknown
> Family: Unknown
>
> Handle 0x0002, DMI type 2, 14 bytes
> Base Board Information
> Manufacturer: Unknown
> Product Name: Unknown
> Version: Unknown
> Serial Number: Not Specified
> Asset Tag: Unknown
> Features:
> Board is a hosting board
> Location In Chassis: Not Specified
> Chassis Handle: 0x0000
> Type: Motherboard
>
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> ---
> lib/smbios.c | 17 +++--------------
> 1 file changed, 3 insertions(+), 14 deletions(-)
Perhaps a better fix is to drop the smbios info?
What upstream projects use this information to show things to the
user? You showed a screenshot of some sort of system-info app. We
could teach it about falling back to the device tree. That way we are
not adding fake information to SMBIOS.
Also, SMBIOS is a legacy thing and a PITA to work with. How about we
use the device tree binding for the same info:
smbios {
compatible = "u-boot,sysinfo-smbios";
smbios {
system {
manufacturer = "pine64";
product = "rock64_rk3328";
};
baseboard {
manufacturer = "pine64";
product = "rock64_rk3328";
};
chassis {
manufacturer = "pine64";
product = "rock64_rk3328";
};
};
};
This is easy to parse and gets us away from all this legacy junk that
we don't need.
Regards,
Simon
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/2] smbios: Simplify reporting of unknown values
2022-09-12 18:31 ` [PATCH 1/2] smbios: Simplify reporting of unknown values Simon Glass
@ 2022-09-16 20:30 ` Ilias Apalodimas
2022-09-17 16:55 ` Sean Anderson
2022-09-20 10:15 ` Peter Robinson
1 sibling, 1 reply; 19+ messages in thread
From: Ilias Apalodimas @ 2022-09-16 20:30 UTC (permalink / raw)
To: Simon Glass
Cc: U-Boot Mailing List, Tom Rini, Heinrich Schuchardt, Peter Robinson
Hi Simon,
[...]
> > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > ---
> > lib/smbios.c | 17 +++--------------
> > 1 file changed, 3 insertions(+), 14 deletions(-)
>
> Perhaps a better fix is to drop the smbios info?
Unfortunately there's a ton of userspace tools still using it. So I think
we still need it
>
> What upstream projects use this information to show things to the
> user? You showed a screenshot of some sort of system-info app. We
> could teach it about falling back to the device tree. That way we are
> not adding fake information to SMBIOS.
>
What's fake here? The model and compatible are taken directly from the DT
and that should be accurate. I'd rather fix the DT if that's problematic.
What would make sense for me to change is take the first token of the
compatible node instead of the entire string as it's format is expected to
be <manufacturer, model> anyway.
> Also, SMBIOS is a legacy thing and a PITA to work with. How about we
> use the device tree binding for the same info:
>
> smbios {
> compatible = "u-boot,sysinfo-smbios";
>
> smbios {
> system {
> manufacturer = "pine64";
> product = "rock64_rk3328";
> };
>
> baseboard {
> manufacturer = "pine64";
> product = "rock64_rk3328";
> };
>
> chassis {
> manufacturer = "pine64";
> product = "rock64_rk3328";
> };
> };
> };
>
> This is easy to parse and gets us away from all this legacy junk that
> we don't need.
That's the exact opposite of the patch description. Most of these info are
already included in the DT in it's standard properties. So if U-Boot ends
up with a DT without these we get a usable smbios table. For example a DT
handed over by the previous stage bootloader would not include these nodes.
As far as sysinfo-smbios node is concerned, it's only present in 13
boards, so it's not like it's used by the majority of boards. Yes we
could fix them, but imho we are better off re-using what's already there
and defined on the DT spec at least for the simplistic values.
Thanks
/Ilias
>
> Regards,
> Simon
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/2] smbios: Simplify reporting of unknown values
2022-09-16 20:30 ` Ilias Apalodimas
@ 2022-09-17 16:55 ` Sean Anderson
2022-09-26 10:56 ` Ilias Apalodimas
0 siblings, 1 reply; 19+ messages in thread
From: Sean Anderson @ 2022-09-17 16:55 UTC (permalink / raw)
To: Ilias Apalodimas, Simon Glass
Cc: U-Boot Mailing List, Tom Rini, Heinrich Schuchardt, Peter Robinson
On 9/16/22 16:30, Ilias Apalodimas wrote:
> Hi Simon,
>
> [...]
>
>>> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
>>> ---
>>> lib/smbios.c | 17 +++--------------
>>> 1 file changed, 3 insertions(+), 14 deletions(-)
>>
>> Perhaps a better fix is to drop the smbios info?
>
> Unfortunately there's a ton of userspace tools still using it. So I think
> we still need it
>
>>
>> What upstream projects use this information to show things to the
>> user? You showed a screenshot of some sort of system-info app. We
>> could teach it about falling back to the device tree. That way we are
>> not adding fake information to SMBIOS.
>>
>
> What's fake here? The model and compatible are taken directly from the DT
> and that should be accurate. I'd rather fix the DT if that's problematic.
> What would make sense for me to change is take the first token of the
> compatible node instead of the entire string as it's format is expected to
> be <manufacturer, model> anyway.
> Manufacturer: socionext,developer-box
> Product Name: Socionext Developer Box
Well, firstly, the manufacturer is "Socionext", not
"socionext,developer-box". Compatibles are not suitable for
user-visible identifiers. The product name should also be something like
"Socionext Developerbox" or maybe "SynQuacer E-series", but this more of
a "bug" in the devicetree model property.
Second, these identifiers are not suitable for all structures you want
to use it for. For example, the chassis is really a "INWIN industrial PC
case: MicroATX mini-tower case IW-BK623/300-H E USB 3.0 Black with 300W
SFX power supply" [1]. I would describe this as something like
Handle 0x0003, DMI type 3, 21 bytes
Chassis Information
Manufacturer: INWIN
Type: Mini Tower
Lock: Not Present
Version: Unknown
Serial Number: Not Specified
Asset Tag: Not Specified
Boot-up State: Safe
Power Supply State: Safe
Thermal State: Safe
Security Status: None
OEM Information: 0x00000000
Height: Unspecified
Number Of Power Cords: 1
Contained Elements: 0
The exact values are not particularly important, but I would certainly
classify a manufacturer of "socionext,developer-box" as fake. We might
not even know what the chassis is; what's to stop a user from using a
different case?
[1] https://www.96boards.org/documentation/enterprise/developerbox/hardware-docs/MN04-00002-3E.pdf
>> Also, SMBIOS is a legacy thing and a PITA to work with. How about we
>> use the device tree binding for the same info:
>>
>> smbios {
>> compatible = "u-boot,sysinfo-smbios";
>>
>> smbios {
>> system {
>> manufacturer = "pine64";
>> product = "rock64_rk3328";
>> };
>>
>> baseboard {
>> manufacturer = "pine64";
>> product = "rock64_rk3328";
>> };
>>
>> chassis {
>> manufacturer = "pine64";
>> product = "rock64_rk3328";
>> };
>> };
>> };
>>
>> This is easy to parse and gets us away from all this legacy junk that
>> we don't need.
>
> That's the exact opposite of the patch description. Most of these info are
> already included in the DT in it's standard properties. So if U-Boot ends
> up with a DT without these we get a usable smbios table. For example a DT
> handed over by the previous stage bootloader would not include these nodes.
I agree. I think a better example would fill in these fields with
descriptive values.
> As far as sysinfo-smbios node is concerned, it's only present in 13
> boards, so it's not like it's used by the majority of boards. Yes we
> could fix them, but imho we are better off re-using what's already there
> and defined on the DT spec at least for the simplistic values.
IMO SYS_VENDOR and SYS_BOARD are more descriptive than the devicetree
values, but neither is good...
How many boards do we have which actually use the SMBIOS tables? There
are a lot of boards with EFI_LOADER enabled by default, but I suspect
most never boot anything EFI.
--Sean
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/2] smbios: Simplify reporting of unknown values
2022-09-12 18:31 ` [PATCH 1/2] smbios: Simplify reporting of unknown values Simon Glass
2022-09-16 20:30 ` Ilias Apalodimas
@ 2022-09-20 10:15 ` Peter Robinson
1 sibling, 0 replies; 19+ messages in thread
From: Peter Robinson @ 2022-09-20 10:15 UTC (permalink / raw)
To: Simon Glass
Cc: Ilias Apalodimas, U-Boot Mailing List, Tom Rini,
Heinrich Schuchardt, Rob Herring
Hi Simon,
Adding Rob for information around putting things in device tree.
> > If a value is not valid during the DT or SYSINFO parsing, we explicitly
> > set that to "Unknown Product" and "Unknown" for the product and
> > manufacturer respectively. It's cleaner if we move the checks insisde
> > smbios_add_string() and always report "Unknown" regardless of the missing
> > field.
> >
> > pre-patch dmidecode
> > <snip>
> > Handle 0x0001, DMI type 1, 27 bytes
> > System Information
> > Manufacturer: Unknown
> > Product Name: Unknown Product
> > Version: Not Specified
> > Serial Number: Not Specified
> > UUID: Not Settable
> > Wake-up Type: Reserved
> > SKU Number: Not Specified
> > Family: Not Specified
> >
> > Handle 0x0002, DMI type 2, 14 bytes
> > Base Board Information
> > Manufacturer: Unknown
> > Product Name: Unknown Product
> > Version: Not Specified
> > Serial Number: Not Specified
> > Asset Tag: Not Specified
> > Features:
> > Board is a hosting board
> > Location In Chassis: Not Specified
> > Type: Motherboard
> > <snip>
> >
> > post-patch dmidecode:
> >
> > Handle 0x0001, DMI type 1, 27 bytes
> > System Information
> > Manufacturer: Unknown
> > Product Name: Unknown
> > Version: Unknown
> > Serial Number: Unknown
> > UUID: Not Settable
> > Wake-up Type: Reserved
> > SKU Number: Unknown
> > Family: Unknown
> >
> > Handle 0x0002, DMI type 2, 14 bytes
> > Base Board Information
> > Manufacturer: Unknown
> > Product Name: Unknown
> > Version: Unknown
> > Serial Number: Not Specified
> > Asset Tag: Unknown
> > Features:
> > Board is a hosting board
> > Location In Chassis: Not Specified
> > Chassis Handle: 0x0000
> > Type: Motherboard
> >
> > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > ---
> > lib/smbios.c | 17 +++--------------
> > 1 file changed, 3 insertions(+), 14 deletions(-)
>
> Perhaps a better fix is to drop the smbios info?
>
> What upstream projects use this information to show things to the
> user? You showed a screenshot of some sort of system-info app. We
> could teach it about falling back to the device tree. That way we are
> not adding fake information to SMBIOS.
Lots of things use it, particularly for enterprise support platforms.
The system-info apps you mention above are the GNOME about dialog box
and a tool called neofetch (which I personally don't particularly care
for but people like it for some reason).
There's general tools like dmidecode that use the information, but
there's a LOT of open source tools that use the SMBIOS information,
most of the Desktop UX About or HW tools, a lot of support tools like
sosreport (https://github.com/sosreport/sos), and server management
tools like cockpit (https://cockpit-project.org/) and uncountable
proprietary tools.
The problem with putting these things into Device Tree is that they're
not really describing the hardware explicitly and it's U-Boot specific
and Rob has mentioned in the past we absolutely shouldn't be making
things up that don't belong in Device Tree.
> Also, SMBIOS is a legacy thing and a PITA to work with. How about we
> use the device tree binding for the same info:
You say it's legacy, yet it released a new 3.6.0 version in June this
year and is used extensively across x86 and is required for some of
the Arm SystemReady standards. Doesn't appear particularly legacy to
me.
While I don't particularly like SMBIOS either it's the standard and
widely used, I don't see it going anywhere soon. The advantage of it
from someone who had to deal with end to end I feel it's better to
support this because patching all the various projects to support
other things and getting them deployed and enterprises and third
parties to upgrade and integrate into their platforms and processes.
Which from experience takes an extremely long time!
> smbios {
> compatible = "u-boot,sysinfo-smbios";
>
> smbios {
> system {
> manufacturer = "pine64";
> product = "rock64_rk3328";
> };
>
> baseboard {
> manufacturer = "pine64";
> product = "rock64_rk3328";
> };
>
> chassis {
> manufacturer = "pine64";
> product = "rock64_rk3328";
> };
> };
> };
>
> This is easy to parse and gets us away from all this legacy junk that
> we don't need.
I don't see this is any more pretty or any better than getting the
information from Device Tree, moving forward I think it would also be
useful to populate things like serial numbers from the fuse/otp blocks
which we often already display either on the console during boot for
via various commands.
Overall I think we need to be practical, this is a standard that is
widely used now, it's much more straight forward for the overall
ecosystem to continue to use a widely adopted standard rather than
trying to update all the open and proprietary tools out there to some
new "standard" that is only useful for device tree platforms. I'll
reference XKCD here for that: https://xkcd.com/927/
Peter
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/2] smbios: Simplify reporting of unknown values
2022-09-06 13:44 [PATCH 1/2] smbios: Simplify reporting of unknown values Ilias Apalodimas
2022-09-06 13:44 ` [PATCH 2/2] smbios: Fallback to the default DT if sysinfo nodes are missing Ilias Apalodimas
2022-09-12 18:31 ` [PATCH 1/2] smbios: Simplify reporting of unknown values Simon Glass
@ 2022-09-20 11:09 ` Peter Robinson
2 siblings, 0 replies; 19+ messages in thread
From: Peter Robinson @ 2022-09-20 11:09 UTC (permalink / raw)
To: Ilias Apalodimas; +Cc: u-boot, trini, sjg, heinrich.schuchardt
On Tue, Sep 6, 2022 at 2:44 PM Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> If a value is not valid during the DT or SYSINFO parsing, we explicitly
> set that to "Unknown Product" and "Unknown" for the product and
> manufacturer respectively. It's cleaner if we move the checks insisde
> smbios_add_string() and always report "Unknown" regardless of the missing
> field.
The data below probably can go below the --- as I'm not sure it's
useful in the final commit.
> pre-patch dmidecode
> <snip>
> Handle 0x0001, DMI type 1, 27 bytes
> System Information
> Manufacturer: Unknown
> Product Name: Unknown Product
> Version: Not Specified
> Serial Number: Not Specified
> UUID: Not Settable
> Wake-up Type: Reserved
> SKU Number: Not Specified
> Family: Not Specified
>
> Handle 0x0002, DMI type 2, 14 bytes
> Base Board Information
> Manufacturer: Unknown
> Product Name: Unknown Product
> Version: Not Specified
> Serial Number: Not Specified
> Asset Tag: Not Specified
> Features:
> Board is a hosting board
> Location In Chassis: Not Specified
> Chassis Handle: 0x0000
> Type: Motherboard
> <snip>
>
> post-patch dmidecode:
>
> Handle 0x0001, DMI type 1, 27 bytes
> System Information
> Manufacturer: Unknown
> Product Name: Unknown
> Version: Unknown
> Serial Number: Unknown
> UUID: Not Settable
> Wake-up Type: Reserved
> SKU Number: Unknown
> Family: Unknown
>
> Handle 0x0002, DMI type 2, 14 bytes
> Base Board Information
> Manufacturer: Unknown
> Product Name: Unknown
> Version: Unknown
> Serial Number: Not Specified
> Asset Tag: Unknown
> Features:
> Board is a hosting board
> Location In Chassis: Not Specified
> Chassis Handle: 0x0000
> Type: Motherboard
>
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
Reviewed-by: Peter Robinson <pbrobinson@gmail.com>
Tested-by: Peter Robinson <pbrobinson@gmail.com>
> ---
> lib/smbios.c | 17 +++--------------
> 1 file changed, 3 insertions(+), 14 deletions(-)
>
> diff --git a/lib/smbios.c b/lib/smbios.c
> index d7f4999e8b2a..fcc8686993ef 100644
> --- a/lib/smbios.c
> +++ b/lib/smbios.c
> @@ -102,7 +102,7 @@ static int smbios_add_string(struct smbios_ctx *ctx, const char *str)
> int i = 1;
> char *p = ctx->eos;
>
> - if (!*str)
> + if (!str || !*str)
> str = "Unknown";
>
> for (;;) {
> @@ -151,8 +151,7 @@ static int smbios_add_prop_si(struct smbios_ctx *ctx, const char *prop,
> const char *str;
>
> str = ofnode_read_string(ctx->node, prop);
> - if (str)
> - return smbios_add_string(ctx, str);
> + return smbios_add_string(ctx, str);
> }
>
> return 0;
> @@ -231,7 +230,7 @@ static int smbios_write_type0(ulong *current, int handle,
> t->vendor = smbios_add_string(ctx, "U-Boot");
>
> t->bios_ver = smbios_add_prop(ctx, "version");
> - if (!t->bios_ver)
> + if (!strcmp(ctx->last_str, "Unknown"))
> t->bios_ver = smbios_add_string(ctx, PLAIN_VERSION);
> if (t->bios_ver)
> gd->smbios_version = ctx->last_str;
> @@ -281,11 +280,7 @@ 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) {
> @@ -315,11 +310,7 @@ 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");
> @@ -344,8 +335,6 @@ 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.37.2
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] smbios: Fallback to the default DT if sysinfo nodes are missing
2022-09-06 13:44 ` [PATCH 2/2] smbios: Fallback to the default DT if sysinfo nodes are missing Ilias Apalodimas
@ 2022-09-20 11:10 ` Peter Robinson
2022-09-29 9:59 ` Simon Glass
0 siblings, 1 reply; 19+ messages in thread
From: Peter Robinson @ 2022-09-20 11:10 UTC (permalink / raw)
To: Ilias Apalodimas; +Cc: u-boot, trini, sjg, heinrich.schuchardt
On Tue, Sep 6, 2022 at 2:44 PM Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> In order to fill in the SMBIOS tables U-Boot currently relies on a
> "u-boot,sysinfo-smbios" compatible node. This is fine for the boards
> that already include such nodes. However with some recent EFI changes,
> the majority of boards can boot up distros, which usually rely on
> things like dmidecode etc for their reporting. For boards that
> lack this special node the SMBIOS output looks like:
>
> System Information
> Manufacturer: Unknown
> Product Name: Unknown
> Version: Unknown
> Serial Number: Unknown
> UUID: Not Settable
> Wake-up Type: Reserved
> SKU Number: Unknown
> Family: Unknown
>
> This looks problematic since most of the info are "Unknown". The DT spec
> specifies standard properties containing relevant information like
> 'model' and 'compatible' for which the suggested format is
> <manufacturer,model>. So let's add a last resort to our current
> smbios parsing. If none of the sysinfo properties are found, we can
> scan the root node for 'model' and 'compatible'.
I don't think the information below all needs to go in the commit,
maybe in the cover letter?
> pre-patch dmidecode:
> <snip>
> Handle 0x0001, DMI type 1, 27 bytes
> System Information
> Manufacturer: Unknown
> Product Name: Unknown
> Version: Unknown
> Serial Number: Unknown
> UUID: Not Settable
> Wake-up Type: Reserved
> SKU Number: Unknown
> Family: Unknown
>
> Handle 0x0002, DMI type 2, 14 bytes
> Base Board Information
> Manufacturer: Unknown
> Product Name: Unknown
> Version: Unknown
> Serial Number: Not Specified
> Asset Tag: Unknown
> Features:
> Board is a hosting board
> Location In Chassis: Not Specified
> Chassis Handle: 0x0000
> Type: Motherboard
>
> Handle 0x0003, DMI type 3, 21 bytes
> Chassis Information
> Manufacturer: Unknown
> Type: Desktop
> Lock: Not Present
> Version: Not Specified
> Serial Number: Not Specified
> Asset Tag: Not Specified
> Boot-up State: Safe
> Power Supply State: Safe
> Thermal State: Safe
> Security Status: None
> OEM Information: 0x00000000
> Height: Unspecified
> Number Of Power Cords: Unspecified
> Contained Elements: 0
> <snip>
>
> post-pastch dmidecode:
> <snip>
> Handle 0x0001, DMI type 1, 27 bytes
> System Information
> Manufacturer: socionext,developer-box
> Product Name: Socionext Developer Box
> Version: Unknown
> Serial Number: Unknown
> UUID: Not Settable
> Wake-up Type: Reserved
> SKU Number: Unknown
> Family: Unknown
>
> Handle 0x0002, DMI type 2, 14 bytes
> Base Board Information
> Manufacturer: socionext,developer-box
> Product Name: Socionext Developer Box
> Version: Unknown
> Serial Number: Not Specified
> Asset Tag: Unknown
> Features:
> Board is a hosting board
> Location In Chassis: Not Specified
> Chassis Handle: 0x0000
> Type: Motherboard
>
> Handle 0x0003, DMI type 3, 21 bytes
> Chassis Information
> Manufacturer: socionext,developer-box
> Type: Desktop
> Lock: Not Present
> Version: Not Specified
> Serial Number: Not Specified
> Asset Tag: Not Specified
> Boot-up State: Safe
> Power Supply State: Safe
> Thermal State: Safe
> Security Status: None
> OEM Information: 0x00000000
> Height: Unspecified
> Number Of Power Cords: Unspecified
> Contained Elements: 0
> <snip>
>
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
Reviewed-by: Peter Robinson <pbrobinson@gmail.com>
Tested-by: Peter Robinson <pbrobinson@gmail.com>
> ---
> lib/smbios.c | 41 +++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 39 insertions(+), 2 deletions(-)
>
> diff --git a/lib/smbios.c b/lib/smbios.c
> index fcc8686993ef..f2eb961f514b 100644
> --- a/lib/smbios.c
> +++ b/lib/smbios.c
> @@ -43,6 +43,20 @@
>
> DECLARE_GLOBAL_DATA_PTR;
>
> +/**
> + * struct sysifno_to_dt - Mapping of sysinfo strings to DT
> + *
> + * @sysinfo_str: sysinfo string
> + * @dt_str: DT string
> + */
> +static const struct {
> + const char *sysinfo_str;
> + const char *dt_str;
> +} sysifno_to_dt[] = {
> + { .sysinfo_str = "product", .dt_str = "model" },
> + { .sysinfo_str = "manufacturer", .dt_str = "compatible" },
> +};
> +
> /**
> * struct smbios_ctx - context for writing SMBIOS tables
> *
> @@ -87,6 +101,18 @@ struct smbios_write_method {
> const char *subnode_name;
> };
>
> +static const char *convert_sysinfo_to_dt(const char *sysinfo_str)
> +{
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(sysifno_to_dt); i++) {
> + if (!strcmp(sysinfo_str, sysifno_to_dt[i].sysinfo_str))
> + return sysifno_to_dt[i].dt_str;
> + }
> +
> + return NULL;
> +}
> +
> /**
> * smbios_add_string() - add a string to the string area
> *
> @@ -148,9 +174,20 @@ static int smbios_add_prop_si(struct smbios_ctx *ctx, const char *prop,
> return smbios_add_string(ctx, val);
> }
> if (IS_ENABLED(CONFIG_OF_CONTROL)) {
> - const char *str;
> + const char *str = NULL;
>
> - str = ofnode_read_string(ctx->node, prop);
> + /*
> + * If the node is not valid fallback and try the entire DT
> + * so we can at least fill in maufacturer and board type
> + */
> + if (!ofnode_valid(ctx->node)) {
> + const char *nprop = convert_sysinfo_to_dt(prop);
> +
> + if (nprop)
> + str = ofnode_read_string(ofnode_root(), nprop);
> + } else {
> + str = ofnode_read_string(ctx->node, prop);
> + }
> return smbios_add_string(ctx, str);
> }
>
> --
> 2.37.2
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/2] smbios: Simplify reporting of unknown values
2022-09-17 16:55 ` Sean Anderson
@ 2022-09-26 10:56 ` Ilias Apalodimas
2022-09-29 4:34 ` Sean Anderson
0 siblings, 1 reply; 19+ messages in thread
From: Ilias Apalodimas @ 2022-09-26 10:56 UTC (permalink / raw)
To: Sean Anderson
Cc: Simon Glass, U-Boot Mailing List, Tom Rini, Heinrich Schuchardt,
Peter Robinson
Hi Sean
On Sat, 17 Sept 2022 at 19:55, Sean Anderson <seanga2@gmail.com> wrote:
>
> On 9/16/22 16:30, Ilias Apalodimas wrote:
> > Hi Simon,
> >
> > [...]
> >
> >>> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> >>> ---
> >>> lib/smbios.c | 17 +++--------------
> >>> 1 file changed, 3 insertions(+), 14 deletions(-)
> >>
> >> Perhaps a better fix is to drop the smbios info?
> >
> > Unfortunately there's a ton of userspace tools still using it. So I think
> > we still need it
> >
> >>
> >> What upstream projects use this information to show things to the
> >> user? You showed a screenshot of some sort of system-info app. We
> >> could teach it about falling back to the device tree. That way we are
> >> not adding fake information to SMBIOS.
> >>
> >
> > What's fake here? The model and compatible are taken directly from the DT
> > and that should be accurate. I'd rather fix the DT if that's problematic.
> > What would make sense for me to change is take the first token of the
> > compatible node instead of the entire string as it's format is expected to
> > be <manufacturer, model> anyway.
>
> > Manufacturer: socionext,developer-box
> > Product Name: Socionext Developer Box
>
> Well, firstly, the manufacturer is "Socionext", not
> "socionext,developer-box". Compatibles are not suitable for
> user-visible identifiers. The product name should also be something like
> "Socionext Developerbox" or maybe "SynQuacer E-series", but this more of
> a "bug" in the devicetree model property.
Yea as I said we can get rid of the everything after the ',' on the
compatible node. Ideally if vendors followed the DT spec, we could
also just use manufacturer node, the reality is that we can't though.
The whole point of the patchset is provide something reasonable
without having to add a .dtsi smbios node for all our devices. We can
then go back to fixing the DT with proper values if it's a DT "bug".
>
> Second, these identifiers are not suitable for all structures you want
> to use it for. For example, the chassis is really a "INWIN industrial PC
> case: MicroATX mini-tower case IW-BK623/300-H E USB 3.0 Black with 300W
> SFX power supply" [1]. I would describe this as something like
The chassis isn't even addressed in the series. IIRC it's currently
hardcoded in smbios.c.
>
> Handle 0x0003, DMI type 3, 21 bytes
> Chassis Information
> Manufacturer: INWIN
> Type: Mini Tower
> Lock: Not Present
> Version: Unknown
> Serial Number: Not Specified
> Asset Tag: Not Specified
> Boot-up State: Safe
> Power Supply State: Safe
> Thermal State: Safe
> Security Status: None
> OEM Information: 0x00000000
> Height: Unspecified
> Number Of Power Cords: 1
> Contained Elements: 0
>
> The exact values are not particularly important, but I would certainly
> classify a manufacturer of "socionext,developer-box" as fake. We might
> not even know what the chassis is; what's to stop a user from using a
> different case?
But the chassis isn't even addressed in the series? Again I am mostly
interested in a sane fallback for device and manufacturer.
>
> [1] https://www.96boards.org/documentation/enterprise/developerbox/hardware-docs/MN04-00002-3E.pdf
>
> >> Also, SMBIOS is a legacy thing and a PITA to work with. How about we
> >> use the device tree binding for the same info:
> >>
> >> smbios {
> >> compatible = "u-boot,sysinfo-smbios";
> >>
> >> smbios {
> >> system {
> >> manufacturer = "pine64";
> >> product = "rock64_rk3328";
> >> };
> >>
> >> baseboard {
> >> manufacturer = "pine64";
> >> product = "rock64_rk3328";
> >> };
> >>
> >> chassis {
> >> manufacturer = "pine64";
> >> product = "rock64_rk3328";
> >> };
> >> };
> >> };
> >>
> >> This is easy to parse and gets us away from all this legacy junk that
> >> we don't need.
> >
> > That's the exact opposite of the patch description. Most of these info are
> > already included in the DT in it's standard properties. So if U-Boot ends
> > up with a DT without these we get a usable smbios table. For example a DT
> > handed over by the previous stage bootloader would not include these nodes.
>
> I agree. I think a better example would fill in these fields with
> descriptive values.
We are off to a chicken and egg problem now. Can you provide U-Boot
with a 'configuration' DT, which would be disjoint from the DT that
describes hardware?
>
> > As far as sysinfo-smbios node is concerned, it's only present in 13
> > boards, so it's not like it's used by the majority of boards. Yes we
> > could fix them, but imho we are better off re-using what's already there
> > and defined on the DT spec at least for the simplistic values.
>
> IMO SYS_VENDOR and SYS_BOARD are more descriptive than the devicetree
> values, but neither is good...
Didn't we use to do that? IOW fill in smbios nodes based on Kconfig
values. But then we moved away from that in favor of the
sysinfo-smbios node, but a very small amount of boards got converted.
>
> How many boards do we have which actually use the SMBIOS tables? There
> are a lot of boards with EFI_LOADER enabled by default, but I suspect
> most never boot anything EFI.
I don't see how that's relevant? If someone for any reason enables
smbios it shouldn't report always "Unknown".
Thanks
/Ilias
>
> --Sean
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/2] smbios: Simplify reporting of unknown values
2022-09-26 10:56 ` Ilias Apalodimas
@ 2022-09-29 4:34 ` Sean Anderson
2022-09-29 9:59 ` Simon Glass
2022-09-29 10:09 ` Ilias Apalodimas
0 siblings, 2 replies; 19+ messages in thread
From: Sean Anderson @ 2022-09-29 4:34 UTC (permalink / raw)
To: Ilias Apalodimas
Cc: Simon Glass, U-Boot Mailing List, Tom Rini, Heinrich Schuchardt,
Peter Robinson
On 9/26/22 06:56, Ilias Apalodimas wrote:
> Hi Sean
>
> On Sat, 17 Sept 2022 at 19:55, Sean Anderson <seanga2@gmail.com> wrote:
>>
>> On 9/16/22 16:30, Ilias Apalodimas wrote:
>>> Hi Simon,
>>>
>>> [...]
>>>
>>>>> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
>>>>> ---
>>>>> lib/smbios.c | 17 +++--------------
>>>>> 1 file changed, 3 insertions(+), 14 deletions(-)
>>>>
>>>> Perhaps a better fix is to drop the smbios info?
>>>
>>> Unfortunately there's a ton of userspace tools still using it. So I think
>>> we still need it
>>>
>>>>
>>>> What upstream projects use this information to show things to the
>>>> user? You showed a screenshot of some sort of system-info app. We
>>>> could teach it about falling back to the device tree. That way we are
>>>> not adding fake information to SMBIOS.
>>>>
>>>
>>> What's fake here? The model and compatible are taken directly from the DT
>>> and that should be accurate. I'd rather fix the DT if that's problematic.
>>> What would make sense for me to change is take the first token of the
>>> compatible node instead of the entire string as it's format is expected to
>>> be <manufacturer, model> anyway.
>>
>>> Manufacturer: socionext,developer-box
>>> Product Name: Socionext Developer Box
>>
>> Well, firstly, the manufacturer is "Socionext", not
>> "socionext,developer-box". Compatibles are not suitable for
>> user-visible identifiers. The product name should also be something like
>> "Socionext Developerbox" or maybe "SynQuacer E-series", but this more of
>> a "bug" in the devicetree model property.
>
> Yea as I said we can get rid of the everything after the ',' on the
> compatible node. Ideally if vendors followed the DT spec, we could
> also just use manufacturer node, the reality is that we can't though.
This is another one of the problems with this approach. There's no
consistency in existing device trees, because at most this info is
printed in the boot log.
> The whole point of the patchset is provide something reasonable
> without having to add a .dtsi smbios node for all our devices. We can
> then go back to fixing the DT with proper values if it's a DT "bug".
>>
>> Second, these identifiers are not suitable for all structures you want
>> to use it for. For example, the chassis is really a "INWIN industrial PC
>> case: MicroATX mini-tower case IW-BK623/300-H E USB 3.0 Black with 300W
>> SFX power supply" [1]. I would describe this as something like
>
> The chassis isn't even addressed in the series. IIRC it's currently
> hardcoded in smbios.c.
You showed it as different in the commit message.
>>
>> Handle 0x0003, DMI type 3, 21 bytes
>> Chassis Information
>> Manufacturer: INWIN
>> Type: Mini Tower
>> Lock: Not Present
>> Version: Unknown
>> Serial Number: Not Specified
>> Asset Tag: Not Specified
>> Boot-up State: Safe
>> Power Supply State: Safe
>> Thermal State: Safe
>> Security Status: None
>> OEM Information: 0x00000000
>> Height: Unspecified
>> Number Of Power Cords: 1
>> Contained Elements: 0
>>
>> The exact values are not particularly important, but I would certainly
>> classify a manufacturer of "socionext,developer-box" as fake. We might
>> not even know what the chassis is; what's to stop a user from using a
>> different case?
>
> But the chassis isn't even addressed in the series? Again I am mostly
> interested in a sane fallback for device and manufacturer.
ditto
>>
>> [1] https://www.96boards.org/documentation/enterprise/developerbox/hardware-docs/MN04-00002-3E.pdf
>>
>>>> Also, SMBIOS is a legacy thing and a PITA to work with. How about we
>>>> use the device tree binding for the same info:
>>>>
>>>> smbios {
>>>> compatible = "u-boot,sysinfo-smbios";
>>>>
>>>> smbios {
>>>> system {
>>>> manufacturer = "pine64";
>>>> product = "rock64_rk3328";
>>>> };
>>>>
>>>> baseboard {
>>>> manufacturer = "pine64";
>>>> product = "rock64_rk3328";
>>>> };
>>>>
>>>> chassis {
>>>> manufacturer = "pine64";
>>>> product = "rock64_rk3328";
>>>> };
>>>> };
>>>> };
>>>>
>>>> This is easy to parse and gets us away from all this legacy junk that
>>>> we don't need.
>>>
>>> That's the exact opposite of the patch description. Most of these info are
>>> already included in the DT in it's standard properties. So if U-Boot ends
>>> up with a DT without these we get a usable smbios table. For example a DT
>>> handed over by the previous stage bootloader would not include these nodes.
>>
>> I agree. I think a better example would fill in these fields with
>> descriptive values.
>
> We are off to a chicken and egg problem now. Can you provide U-Boot
> with a 'configuration' DT, which would be disjoint from the DT that
> describes hardware?
Sorry, I misread the context there.
I still don't think this is the right approach for this... better to fix
the prior stage's devicetree.
>>
>>> As far as sysinfo-smbios node is concerned, it's only present in 13
>>> boards, so it's not like it's used by the majority of boards. Yes we
>>> could fix them, but imho we are better off re-using what's already there
>>> and defined on the DT spec at least for the simplistic values.
>>
>> IMO SYS_VENDOR and SYS_BOARD are more descriptive than the devicetree
>> values, but neither is good...
>
> Didn't we use to do that? IOW fill in smbios nodes based on Kconfig
> values. But then we moved away from that in favor of the
> sysinfo-smbios node, but a very small amount of boards got converted.
I mean that SYS_VENDOR and SYS_BOARD have content which more closely
matches the content of the SMBios tables, not that we should use them
("neither is good...").
>>
>> How many boards do we have which actually use the SMBIOS tables? There
>> are a lot of boards with EFI_LOADER enabled by default, but I suspect
>> most never boot anything EFI.
>
> I don't see how that's relevant? If someone for any reason enables
> smbios it shouldn't report always "Unknown".
I'm mostly trying to figure out how much effort it would be to just add
nodes for all devices which boot with SMBios. I know that most boards
which have it enabled don't actually use it, since it's enabled by
default.
--Sean
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] smbios: Fallback to the default DT if sysinfo nodes are missing
2022-09-20 11:10 ` Peter Robinson
@ 2022-09-29 9:59 ` Simon Glass
2022-09-29 10:23 ` Ilias Apalodimas
0 siblings, 1 reply; 19+ messages in thread
From: Simon Glass @ 2022-09-29 9:59 UTC (permalink / raw)
To: Peter Robinson
Cc: Ilias Apalodimas, U-Boot Mailing List, Tom Rini, Heinrich Schuchardt
Hi,
On Tue, 20 Sept 2022 at 05:10, Peter Robinson <pbrobinson@gmail.com> wrote:
>
> On Tue, Sep 6, 2022 at 2:44 PM Ilias Apalodimas
> <ilias.apalodimas@linaro.org> wrote:
> >
> > In order to fill in the SMBIOS tables U-Boot currently relies on a
> > "u-boot,sysinfo-smbios" compatible node. This is fine for the boards
> > that already include such nodes. However with some recent EFI changes,
> > the majority of boards can boot up distros, which usually rely on
> > things like dmidecode etc for their reporting. For boards that
> > lack this special node the SMBIOS output looks like:
> >
> > System Information
> > Manufacturer: Unknown
> > Product Name: Unknown
> > Version: Unknown
> > Serial Number: Unknown
> > UUID: Not Settable
> > Wake-up Type: Reserved
> > SKU Number: Unknown
> > Family: Unknown
> >
> > This looks problematic since most of the info are "Unknown". The DT spec
> > specifies standard properties containing relevant information like
> > 'model' and 'compatible' for which the suggested format is
> > <manufacturer,model>. So let's add a last resort to our current
> > smbios parsing. If none of the sysinfo properties are found, we can
> > scan the root node for 'model' and 'compatible'.
>
> I don't think the information below all needs to go in the commit,
> maybe in the cover letter?
>
> > pre-patch dmidecode:
> > <snip>
> > Handle 0x0001, DMI type 1, 27 bytes
> > System Information
> > Manufacturer: Unknown
> > Product Name: Unknown
> > Version: Unknown
> > Serial Number: Unknown
> > UUID: Not Settable
> > Wake-up Type: Reserved
> > SKU Number: Unknown
> > Family: Unknown
> >
> > Handle 0x0002, DMI type 2, 14 bytes
> > Base Board Information
> > Manufacturer: Unknown
> > Product Name: Unknown
> > Version: Unknown
> > Serial Number: Not Specified
> > Asset Tag: Unknown
> > Features:
> > Board is a hosting board
> > Location In Chassis: Not Specified
> > Chassis Handle: 0x0000
> > Type: Motherboard
> >
> > Handle 0x0003, DMI type 3, 21 bytes
> > Chassis Information
> > Manufacturer: Unknown
> > Type: Desktop
> > Lock: Not Present
> > Version: Not Specified
> > Serial Number: Not Specified
> > Asset Tag: Not Specified
> > Boot-up State: Safe
> > Power Supply State: Safe
> > Thermal State: Safe
> > Security Status: None
> > OEM Information: 0x00000000
> > Height: Unspecified
> > Number Of Power Cords: Unspecified
> > Contained Elements: 0
> > <snip>
> >
> > post-pastch dmidecode:
> > <snip>
> > Handle 0x0001, DMI type 1, 27 bytes
> > System Information
> > Manufacturer: socionext,developer-box
> > Product Name: Socionext Developer Box
> > Version: Unknown
> > Serial Number: Unknown
> > UUID: Not Settable
> > Wake-up Type: Reserved
> > SKU Number: Unknown
> > Family: Unknown
> >
> > Handle 0x0002, DMI type 2, 14 bytes
> > Base Board Information
> > Manufacturer: socionext,developer-box
> > Product Name: Socionext Developer Box
> > Version: Unknown
> > Serial Number: Not Specified
> > Asset Tag: Unknown
> > Features:
> > Board is a hosting board
> > Location In Chassis: Not Specified
> > Chassis Handle: 0x0000
> > Type: Motherboard
> >
> > Handle 0x0003, DMI type 3, 21 bytes
> > Chassis Information
> > Manufacturer: socionext,developer-box
> > Type: Desktop
> > Lock: Not Present
> > Version: Not Specified
> > Serial Number: Not Specified
> > Asset Tag: Not Specified
> > Boot-up State: Safe
> > Power Supply State: Safe
> > Thermal State: Safe
> > Security Status: None
> > OEM Information: 0x00000000
> > Height: Unspecified
> > Number Of Power Cords: Unspecified
> > Contained Elements: 0
> > <snip>
> >
> > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
>
> Reviewed-by: Peter Robinson <pbrobinson@gmail.com>
> Tested-by: Peter Robinson <pbrobinson@gmail.com>
>
> > ---
> > lib/smbios.c | 41 +++++++++++++++++++++++++++++++++++++++--
> > 1 file changed, 39 insertions(+), 2 deletions(-)
I've thought about this a lot.
As I mentioned earlier, we should require boards to add this
information when they enable GENERATE_SMBIOS_TABLE
It is a simple patch for each board vendor and it solves the problem.
What we have here just masks it.
Regards,
Simon
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/2] smbios: Simplify reporting of unknown values
2022-09-29 4:34 ` Sean Anderson
@ 2022-09-29 9:59 ` Simon Glass
2022-09-29 14:02 ` Sean Anderson
2022-09-29 10:09 ` Ilias Apalodimas
1 sibling, 1 reply; 19+ messages in thread
From: Simon Glass @ 2022-09-29 9:59 UTC (permalink / raw)
To: Sean Anderson
Cc: Ilias Apalodimas, U-Boot Mailing List, Tom Rini,
Heinrich Schuchardt, Peter Robinson
Hi,
On Wed, 28 Sept 2022 at 22:34, Sean Anderson <seanga2@gmail.com> wrote:
>
> On 9/26/22 06:56, Ilias Apalodimas wrote:
> > Hi Sean
> >
> > On Sat, 17 Sept 2022 at 19:55, Sean Anderson <seanga2@gmail.com> wrote:
> >>
> >> On 9/16/22 16:30, Ilias Apalodimas wrote:
> >>> Hi Simon,
> >>>
> >>> [...]
> >>>
> >>>>> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> >>>>> ---
> >>>>> lib/smbios.c | 17 +++--------------
> >>>>> 1 file changed, 3 insertions(+), 14 deletions(-)
> >>>>
> >>>> Perhaps a better fix is to drop the smbios info?
> >>>
> >>> Unfortunately there's a ton of userspace tools still using it. So I think
> >>> we still need it
> >>>
> >>>>
> >>>> What upstream projects use this information to show things to the
> >>>> user? You showed a screenshot of some sort of system-info app. We
> >>>> could teach it about falling back to the device tree. That way we are
> >>>> not adding fake information to SMBIOS.
> >>>>
> >>>
> >>> What's fake here? The model and compatible are taken directly from the DT
> >>> and that should be accurate. I'd rather fix the DT if that's problematic.
> >>> What would make sense for me to change is take the first token of the
> >>> compatible node instead of the entire string as it's format is expected to
> >>> be <manufacturer, model> anyway.
> >>
> >>> Manufacturer: socionext,developer-box
> >>> Product Name: Socionext Developer Box
> >>
> >> Well, firstly, the manufacturer is "Socionext", not
> >> "socionext,developer-box". Compatibles are not suitable for
> >> user-visible identifiers. The product name should also be something like
> >> "Socionext Developerbox" or maybe "SynQuacer E-series", but this more of
> >> a "bug" in the devicetree model property.
> >
> > Yea as I said we can get rid of the everything after the ',' on the
> > compatible node. Ideally if vendors followed the DT spec, we could
> > also just use manufacturer node, the reality is that we can't though.
>
> This is another one of the problems with this approach. There's no
> consistency in existing device trees, because at most this info is
> printed in the boot log.
>
> > The whole point of the patchset is provide something reasonable
> > without having to add a .dtsi smbios node for all our devices. We can
> > then go back to fixing the DT with proper values if it's a DT "bug".
> >>
> >> Second, these identifiers are not suitable for all structures you want
> >> to use it for. For example, the chassis is really a "INWIN industrial PC
> >> case: MicroATX mini-tower case IW-BK623/300-H E USB 3.0 Black with 300W
> >> SFX power supply" [1]. I would describe this as something like
> >
> > The chassis isn't even addressed in the series. IIRC it's currently
> > hardcoded in smbios.c.
>
> You showed it as different in the commit message.
>
> >>
> >> Handle 0x0003, DMI type 3, 21 bytes
> >> Chassis Information
> >> Manufacturer: INWIN
> >> Type: Mini Tower
> >> Lock: Not Present
> >> Version: Unknown
> >> Serial Number: Not Specified
> >> Asset Tag: Not Specified
> >> Boot-up State: Safe
> >> Power Supply State: Safe
> >> Thermal State: Safe
> >> Security Status: None
> >> OEM Information: 0x00000000
> >> Height: Unspecified
> >> Number Of Power Cords: 1
> >> Contained Elements: 0
> >>
> >> The exact values are not particularly important, but I would certainly
> >> classify a manufacturer of "socionext,developer-box" as fake. We might
> >> not even know what the chassis is; what's to stop a user from using a
> >> different case?
> >
> > But the chassis isn't even addressed in the series? Again I am mostly
> > interested in a sane fallback for device and manufacturer.
>
> ditto
>
> >>
> >> [1] https://www.96boards.org/documentation/enterprise/developerbox/hardware-docs/MN04-00002-3E.pdf
> >>
> >>>> Also, SMBIOS is a legacy thing and a PITA to work with. How about we
> >>>> use the device tree binding for the same info:
> >>>>
> >>>> smbios {
> >>>> compatible = "u-boot,sysinfo-smbios";
> >>>>
> >>>> smbios {
> >>>> system {
> >>>> manufacturer = "pine64";
> >>>> product = "rock64_rk3328";
> >>>> };
> >>>>
> >>>> baseboard {
> >>>> manufacturer = "pine64";
> >>>> product = "rock64_rk3328";
> >>>> };
> >>>>
> >>>> chassis {
> >>>> manufacturer = "pine64";
> >>>> product = "rock64_rk3328";
> >>>> };
> >>>> };
> >>>> };
> >>>>
> >>>> This is easy to parse and gets us away from all this legacy junk that
> >>>> we don't need.
> >>>
> >>> That's the exact opposite of the patch description. Most of these info are
> >>> already included in the DT in it's standard properties. So if U-Boot ends
> >>> up with a DT without these we get a usable smbios table. For example a DT
> >>> handed over by the previous stage bootloader would not include these nodes.
> >>
> >> I agree. I think a better example would fill in these fields with
> >> descriptive values.
> >
> > We are off to a chicken and egg problem now. Can you provide U-Boot
> > with a 'configuration' DT, which would be disjoint from the DT that
> > describes hardware?
>
> Sorry, I misread the context there.
>
> I still don't think this is the right approach for this... better to fix
> the prior stage's devicetree.
>
> >>
> >>> As far as sysinfo-smbios node is concerned, it's only present in 13
> >>> boards, so it's not like it's used by the majority of boards. Yes we
> >>> could fix them, but imho we are better off re-using what's already there
> >>> and defined on the DT spec at least for the simplistic values.
> >>
> >> IMO SYS_VENDOR and SYS_BOARD are more descriptive than the devicetree
> >> values, but neither is good...
> >
> > Didn't we use to do that? IOW fill in smbios nodes based on Kconfig
> > values. But then we moved away from that in favor of the
> > sysinfo-smbios node, but a very small amount of boards got converted.
>
> I mean that SYS_VENDOR and SYS_BOARD have content which more closely
> matches the content of the SMBios tables, not that we should use them
> ("neither is good...").
>
> >>
> >> How many boards do we have which actually use the SMBIOS tables? There
> >> are a lot of boards with EFI_LOADER enabled by default, but I suspect
> >> most never boot anything EFI.
> >
> > I don't see how that's relevant? If someone for any reason enables
> > smbios it shouldn't report always "Unknown".
>
> I'm mostly trying to figure out how much effort it would be to just add
> nodes for all devices which boot with SMBios. I know that most boards
> which have it enabled don't actually use it, since it's enabled by
> default.
It is a patch like this:
https://patchwork.ozlabs.org/project/uboot/patch/20220929001520.9095-1-christian@kohlschutter.com/
I just found out that this option is enabled for hundreds of boards.
Perhaps the solution is to turn it off unless the board enables it?
Regards,
Simon
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/2] smbios: Simplify reporting of unknown values
2022-09-29 4:34 ` Sean Anderson
2022-09-29 9:59 ` Simon Glass
@ 2022-09-29 10:09 ` Ilias Apalodimas
1 sibling, 0 replies; 19+ messages in thread
From: Ilias Apalodimas @ 2022-09-29 10:09 UTC (permalink / raw)
To: Sean Anderson
Cc: Simon Glass, U-Boot Mailing List, Tom Rini, Heinrich Schuchardt,
Peter Robinson
Hi Sean,
On Thu, Sep 29, 2022 at 12:34:37AM -0400, Sean Anderson wrote:
> On 9/26/22 06:56, Ilias Apalodimas wrote:
> > Hi Sean
> >
> > On Sat, 17 Sept 2022 at 19:55, Sean Anderson <seanga2@gmail.com> wrote:
> > >
> > > On 9/16/22 16:30, Ilias Apalodimas wrote:
> > > > Hi Simon,
> > > >
> > > > [...]
> > > >
> > > > > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > > > > > ---
> > > > > > lib/smbios.c | 17 +++--------------
> > > > > > 1 file changed, 3 insertions(+), 14 deletions(-)
> > > > >
> > > > > Perhaps a better fix is to drop the smbios info?
> > > >
> > > > Unfortunately there's a ton of userspace tools still using it. So I think
> > > > we still need it
> > > >
> > > > >
> > > > > What upstream projects use this information to show things to the
> > > > > user? You showed a screenshot of some sort of system-info app. We
> > > > > could teach it about falling back to the device tree. That way we are
> > > > > not adding fake information to SMBIOS.
> > > > >
> > > >
> > > > What's fake here? The model and compatible are taken directly from the DT
> > > > and that should be accurate. I'd rather fix the DT if that's problematic.
> > > > What would make sense for me to change is take the first token of the
> > > > compatible node instead of the entire string as it's format is expected to
> > > > be <manufacturer, model> anyway.
> > >
> > > > Manufacturer: socionext,developer-box
> > > > Product Name: Socionext Developer Box
> > >
> > > Well, firstly, the manufacturer is "Socionext", not
> > > "socionext,developer-box". Compatibles are not suitable for
> > > user-visible identifiers. The product name should also be something like
> > > "Socionext Developerbox" or maybe "SynQuacer E-series", but this more of
> > > a "bug" in the devicetree model property.
> >
> > Yea as I said we can get rid of the everything after the ',' on the
> > compatible node. Ideally if vendors followed the DT spec, we could
> > also just use manufacturer node, the reality is that we can't though.
>
> This is another one of the problems with this approach. There's no
> consistency in existing device trees, because at most this info is
> printed in the boot log.
I see these 2 as completely disjoint problems tbh. The approach says
"Let's use what the DT spec suggests to derive values that make sense as a
last resort". The fact that some DTs decide to do differently is a side
effect. But greping into DT's a bit all of the 'model' seems sane and all
the values before the first ',' as well.
>
> > The whole point of the patchset is provide something reasonable
> > without having to add a .dtsi smbios node for all our devices. We can
> > then go back to fixing the DT with proper values if it's a DT "bug".
> > >
> > > Second, these identifiers are not suitable for all structures you want
> > > to use it for. For example, the chassis is really a "INWIN industrial PC
> > > case: MicroATX mini-tower case IW-BK623/300-H E USB 3.0 Black with 300W
> > > SFX power supply" [1]. I would describe this as something like
> >
> > The chassis isn't even addressed in the series. IIRC it's currently
> > hardcoded in smbios.c.
>
> You showed it as different in the commit message.
Not on the commit message. Maybe you remember a discussion over IRC?
In any case I think this is a moot argument. Whether we parse the chassis
from the DT or the sysinfo-smbios in the DT someone still has to change the
*text* if he changes the enclosure. So I don't really see any big
difference here.
>
> > >
> > > Handle 0x0003, DMI type 3, 21 bytes
> > > Chassis Information
> > > Manufacturer: INWIN
> > > Type: Mini Tower
> > > Lock: Not Present
> > > Version: Unknown
> > > Serial Number: Not Specified
> > > Asset Tag: Not Specified
> > > Boot-up State: Safe
> > > Power Supply State: Safe
> > > Thermal State: Safe
> > > Security Status: None
> > > OEM Information: 0x00000000
> > > Height: Unspecified
> > > Number Of Power Cords: 1
> > > Contained Elements: 0
> > >
> > > The exact values are not particularly important, but I would certainly
> > > classify a manufacturer of "socionext,developer-box" as fake. We might
> > > not even know what the chassis is; what's to stop a user from using a
> > > different case?
> >
> > But the chassis isn't even addressed in the series? Again I am mostly
> > interested in a sane fallback for device and manufacturer.
>
> ditto
>
> > >
> > > [1] https://www.96boards.org/documentation/enterprise/developerbox/hardware-docs/MN04-00002-3E.pdf
> > >
> > > > > Also, SMBIOS is a legacy thing and a PITA to work with. How about we
> > > > > use the device tree binding for the same info:
> > > > >
> > > > > smbios {
> > > > > compatible = "u-boot,sysinfo-smbios";
> > > > >
> > > > > smbios {
> > > > > system {
> > > > > manufacturer = "pine64";
> > > > > product = "rock64_rk3328";
> > > > > };
> > > > >
> > > > > baseboard {
> > > > > manufacturer = "pine64";
> > > > > product = "rock64_rk3328";
> > > > > };
> > > > >
> > > > > chassis {
> > > > > manufacturer = "pine64";
> > > > > product = "rock64_rk3328";
> > > > > };
> > > > > };
> > > > > };
> > > > >
> > > > > This is easy to parse and gets us away from all this legacy junk that
> > > > > we don't need.
> > > >
> > > > That's the exact opposite of the patch description. Most of these info are
> > > > already included in the DT in it's standard properties. So if U-Boot ends
> > > > up with a DT without these we get a usable smbios table. For example a DT
> > > > handed over by the previous stage bootloader would not include these nodes.
> > >
> > > I agree. I think a better example would fill in these fields with
> > > descriptive values.
> >
> > We are off to a chicken and egg problem now. Can you provide U-Boot
> > with a 'configuration' DT, which would be disjoint from the DT that
> > describes hardware?
>
> Sorry, I misread the context there.
>
> I still don't think this is the right approach for this... better to fix
> the prior stage's devicetree.
You are asking bootloaders that run *before* U-Boot to add nodes that are not
in any spec and demand to respect an internal U-Boot API.
That sounds like a layering violation to me.
>
> > >
> > > > As far as sysinfo-smbios node is concerned, it's only present in 13
> > > > boards, so it's not like it's used by the majority of boards. Yes we
> > > > could fix them, but imho we are better off re-using what's already there
> > > > and defined on the DT spec at least for the simplistic values.
> > >
> > > IMO SYS_VENDOR and SYS_BOARD are more descriptive than the devicetree
> > > values, but neither is good...
> >
> > Didn't we use to do that? IOW fill in smbios nodes based on Kconfig
> > values. But then we moved away from that in favor of the
> > sysinfo-smbios node, but a very small amount of boards got converted.
>
> I mean that SYS_VENDOR and SYS_BOARD have content which more closely
> matches the content of the SMBios tables, not that we should use them
> ("neither is good...").
>
> > >
> > > How many boards do we have which actually use the SMBIOS tables? There
> > > are a lot of boards with EFI_LOADER enabled by default, but I suspect
> > > most never boot anything EFI.
> >
> > I don't see how that's relevant? If someone for any reason enables
> > smbios it shouldn't report always "Unknown".
>
> I'm mostly trying to figure out how much effort it would be to just add
> nodes for all devices which boot with SMBios. I know that most boards
> which have it enabled don't actually use it, since it's enabled by
> default.
There are 1079 .dts atm and only 13 have smbios nodes (for arm only). It's
not too much but it's not trivial. While at it, who's going to make sure
that every new board has an smbios node even if we fix it ?
FWIW, these comments should have been on patch 2/2. The first patch is a
straight cleanup we should pick up
Thanks
/Ilias
>
> --Sean
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] smbios: Fallback to the default DT if sysinfo nodes are missing
2022-09-29 9:59 ` Simon Glass
@ 2022-09-29 10:23 ` Ilias Apalodimas
2022-09-29 23:55 ` Simon Glass
0 siblings, 1 reply; 19+ messages in thread
From: Ilias Apalodimas @ 2022-09-29 10:23 UTC (permalink / raw)
To: Simon Glass
Cc: Peter Robinson, U-Boot Mailing List, Tom Rini, Heinrich Schuchardt
Hi Simon,
On Thu, Sep 29, 2022 at 03:59:51AM -0600, Simon Glass wrote:
> Hi,
>
> On Tue, 20 Sept 2022 at 05:10, Peter Robinson <pbrobinson@gmail.com> wrote:
> >
> > On Tue, Sep 6, 2022 at 2:44 PM Ilias Apalodimas
> > <ilias.apalodimas@linaro.org> wrote:
> > >
> > > In order to fill in the SMBIOS tables U-Boot currently relies on a
> > > "u-boot,sysinfo-smbios" compatible node. This is fine for the boards
> > > that already include such nodes. However with some recent EFI changes,
> > > the majority of boards can boot up distros, which usually rely on
> > > things like dmidecode etc for their reporting. For boards that
> > > lack this special node the SMBIOS output looks like:
> > >
> > > System Information
> > > Manufacturer: Unknown
> > > Product Name: Unknown
> > > Version: Unknown
> > > Serial Number: Unknown
> > > UUID: Not Settable
> > > Wake-up Type: Reserved
> > > SKU Number: Unknown
> > > Family: Unknown
> > >
> > > This looks problematic since most of the info are "Unknown". The DT spec
> > > specifies standard properties containing relevant information like
> > > 'model' and 'compatible' for which the suggested format is
> > > <manufacturer,model>. So let's add a last resort to our current
> > > smbios parsing. If none of the sysinfo properties are found, we can
> > > scan the root node for 'model' and 'compatible'.
> >
> > I don't think the information below all needs to go in the commit,
> > maybe in the cover letter?
> >
> > > pre-patch dmidecode:
> > > <snip>
> > > Handle 0x0001, DMI type 1, 27 bytes
> > > System Information
> > > Manufacturer: Unknown
> > > Product Name: Unknown
> > > Version: Unknown
> > > Serial Number: Unknown
> > > UUID: Not Settable
> > > Wake-up Type: Reserved
> > > SKU Number: Unknown
> > > Family: Unknown
> > >
> > > Handle 0x0002, DMI type 2, 14 bytes
> > > Base Board Information
> > > Manufacturer: Unknown
> > > Product Name: Unknown
> > > Version: Unknown
> > > Serial Number: Not Specified
> > > Asset Tag: Unknown
> > > Features:
> > > Board is a hosting board
> > > Location In Chassis: Not Specified
> > > Chassis Handle: 0x0000
> > > Type: Motherboard
> > >
> > > Handle 0x0003, DMI type 3, 21 bytes
> > > Chassis Information
> > > Manufacturer: Unknown
> > > Type: Desktop
> > > Lock: Not Present
> > > Version: Not Specified
> > > Serial Number: Not Specified
> > > Asset Tag: Not Specified
> > > Boot-up State: Safe
> > > Power Supply State: Safe
> > > Thermal State: Safe
> > > Security Status: None
> > > OEM Information: 0x00000000
> > > Height: Unspecified
> > > Number Of Power Cords: Unspecified
> > > Contained Elements: 0
> > > <snip>
> > >
> > > post-pastch dmidecode:
> > > <snip>
> > > Handle 0x0001, DMI type 1, 27 bytes
> > > System Information
> > > Manufacturer: socionext,developer-box
> > > Product Name: Socionext Developer Box
> > > Version: Unknown
> > > Serial Number: Unknown
> > > UUID: Not Settable
> > > Wake-up Type: Reserved
> > > SKU Number: Unknown
> > > Family: Unknown
> > >
> > > Handle 0x0002, DMI type 2, 14 bytes
> > > Base Board Information
> > > Manufacturer: socionext,developer-box
> > > Product Name: Socionext Developer Box
> > > Version: Unknown
> > > Serial Number: Not Specified
> > > Asset Tag: Unknown
> > > Features:
> > > Board is a hosting board
> > > Location In Chassis: Not Specified
> > > Chassis Handle: 0x0000
> > > Type: Motherboard
> > >
> > > Handle 0x0003, DMI type 3, 21 bytes
> > > Chassis Information
> > > Manufacturer: socionext,developer-box
> > > Type: Desktop
> > > Lock: Not Present
> > > Version: Not Specified
> > > Serial Number: Not Specified
> > > Asset Tag: Not Specified
> > > Boot-up State: Safe
> > > Power Supply State: Safe
> > > Thermal State: Safe
> > > Security Status: None
> > > OEM Information: 0x00000000
> > > Height: Unspecified
> > > Number Of Power Cords: Unspecified
> > > Contained Elements: 0
> > > <snip>
> > >
> > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> >
> > Reviewed-by: Peter Robinson <pbrobinson@gmail.com>
> > Tested-by: Peter Robinson <pbrobinson@gmail.com>
> >
> > > ---
> > > lib/smbios.c | 41 +++++++++++++++++++++++++++++++++++++++--
> > > 1 file changed, 39 insertions(+), 2 deletions(-)
>
> I've thought about this a lot.
>
> As I mentioned earlier, we should require boards to add this
> information when they enable GENERATE_SMBIOS_TABLE
>
> It is a simple patch for each board vendor and it solves the problem.
> What we have here just masks it.
Not entirely. I think we just see the problem differently here. I agree
that the code here masks a problem (but only for *some* boards) and ideally
we should go and add smbios nodes on the boards that miss it. However we
conveniently keep ignoring OF_BOARD here. Until those things are documented
in a spec and you can *demand* a previous bootloader to include it, we'll have
boards that display "Unknown" all over the place. Personally I don't
think that's acceptable, hence the last resort solution.
I'd be much happier if we popped a warning on boards that miss the smbios
node and are not compiling with OF_BOARD, explaining smbios will be
disabled for them in the future, while having the flexibility to not
display values that make no sense.
Thanks
/Ilias
>
> Regards,
> Simon
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/2] smbios: Simplify reporting of unknown values
2022-09-29 9:59 ` Simon Glass
@ 2022-09-29 14:02 ` Sean Anderson
2022-09-30 14:25 ` Tom Rini
0 siblings, 1 reply; 19+ messages in thread
From: Sean Anderson @ 2022-09-29 14:02 UTC (permalink / raw)
To: Simon Glass
Cc: Ilias Apalodimas, U-Boot Mailing List, Tom Rini,
Heinrich Schuchardt, Peter Robinson
On 9/29/22 05:59, Simon Glass wrote:
> Hi,
>
> On Wed, 28 Sept 2022 at 22:34, Sean Anderson <seanga2@gmail.com> wrote:
>>
>> On 9/26/22 06:56, Ilias Apalodimas wrote:
>>> Hi Sean
>>>
>>> On Sat, 17 Sept 2022 at 19:55, Sean Anderson <seanga2@gmail.com> wrote:
>>>>
>>>> On 9/16/22 16:30, Ilias Apalodimas wrote:
>>>>> Hi Simon,
>>>>>
>>>>> [...]
>>>>>
>>>>>>> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
>>>>>>> ---
>>>>>>> lib/smbios.c | 17 +++--------------
>>>>>>> 1 file changed, 3 insertions(+), 14 deletions(-)
>>>>>>
>>>>>> Perhaps a better fix is to drop the smbios info?
>>>>>
>>>>> Unfortunately there's a ton of userspace tools still using it. So I think
>>>>> we still need it
>>>>>
>>>>>>
>>>>>> What upstream projects use this information to show things to the
>>>>>> user? You showed a screenshot of some sort of system-info app. We
>>>>>> could teach it about falling back to the device tree. That way we are
>>>>>> not adding fake information to SMBIOS.
>>>>>>
>>>>>
>>>>> What's fake here? The model and compatible are taken directly from the DT
>>>>> and that should be accurate. I'd rather fix the DT if that's problematic.
>>>>> What would make sense for me to change is take the first token of the
>>>>> compatible node instead of the entire string as it's format is expected to
>>>>> be <manufacturer, model> anyway.
>>>>
>>>>> Manufacturer: socionext,developer-box
>>>>> Product Name: Socionext Developer Box
>>>>
>>>> Well, firstly, the manufacturer is "Socionext", not
>>>> "socionext,developer-box". Compatibles are not suitable for
>>>> user-visible identifiers. The product name should also be something like
>>>> "Socionext Developerbox" or maybe "SynQuacer E-series", but this more of
>>>> a "bug" in the devicetree model property.
>>>
>>> Yea as I said we can get rid of the everything after the ',' on the
>>> compatible node. Ideally if vendors followed the DT spec, we could
>>> also just use manufacturer node, the reality is that we can't though.
>>
>> This is another one of the problems with this approach. There's no
>> consistency in existing device trees, because at most this info is
>> printed in the boot log.
>>
>>> The whole point of the patchset is provide something reasonable
>>> without having to add a .dtsi smbios node for all our devices. We can
>>> then go back to fixing the DT with proper values if it's a DT "bug".
>>>>
>>>> Second, these identifiers are not suitable for all structures you want
>>>> to use it for. For example, the chassis is really a "INWIN industrial PC
>>>> case: MicroATX mini-tower case IW-BK623/300-H E USB 3.0 Black with 300W
>>>> SFX power supply" [1]. I would describe this as something like
>>>
>>> The chassis isn't even addressed in the series. IIRC it's currently
>>> hardcoded in smbios.c.
>>
>> You showed it as different in the commit message.
>>
>>>>
>>>> Handle 0x0003, DMI type 3, 21 bytes
>>>> Chassis Information
>>>> Manufacturer: INWIN
>>>> Type: Mini Tower
>>>> Lock: Not Present
>>>> Version: Unknown
>>>> Serial Number: Not Specified
>>>> Asset Tag: Not Specified
>>>> Boot-up State: Safe
>>>> Power Supply State: Safe
>>>> Thermal State: Safe
>>>> Security Status: None
>>>> OEM Information: 0x00000000
>>>> Height: Unspecified
>>>> Number Of Power Cords: 1
>>>> Contained Elements: 0
>>>>
>>>> The exact values are not particularly important, but I would certainly
>>>> classify a manufacturer of "socionext,developer-box" as fake. We might
>>>> not even know what the chassis is; what's to stop a user from using a
>>>> different case?
>>>
>>> But the chassis isn't even addressed in the series? Again I am mostly
>>> interested in a sane fallback for device and manufacturer.
>>
>> ditto
>>
>>>>
>>>> [1] https://www.96boards.org/documentation/enterprise/developerbox/hardware-docs/MN04-00002-3E.pdf
>>>>
>>>>>> Also, SMBIOS is a legacy thing and a PITA to work with. How about we
>>>>>> use the device tree binding for the same info:
>>>>>>
>>>>>> smbios {
>>>>>> compatible = "u-boot,sysinfo-smbios";
>>>>>>
>>>>>> smbios {
>>>>>> system {
>>>>>> manufacturer = "pine64";
>>>>>> product = "rock64_rk3328";
>>>>>> };
>>>>>>
>>>>>> baseboard {
>>>>>> manufacturer = "pine64";
>>>>>> product = "rock64_rk3328";
>>>>>> };
>>>>>>
>>>>>> chassis {
>>>>>> manufacturer = "pine64";
>>>>>> product = "rock64_rk3328";
>>>>>> };
>>>>>> };
>>>>>> };
>>>>>>
>>>>>> This is easy to parse and gets us away from all this legacy junk that
>>>>>> we don't need.
>>>>>
>>>>> That's the exact opposite of the patch description. Most of these info are
>>>>> already included in the DT in it's standard properties. So if U-Boot ends
>>>>> up with a DT without these we get a usable smbios table. For example a DT
>>>>> handed over by the previous stage bootloader would not include these nodes.
>>>>
>>>> I agree. I think a better example would fill in these fields with
>>>> descriptive values.
>>>
>>> We are off to a chicken and egg problem now. Can you provide U-Boot
>>> with a 'configuration' DT, which would be disjoint from the DT that
>>> describes hardware?
>>
>> Sorry, I misread the context there.
>>
>> I still don't think this is the right approach for this... better to fix
>> the prior stage's devicetree.
>>
>>>>
>>>>> As far as sysinfo-smbios node is concerned, it's only present in 13
>>>>> boards, so it's not like it's used by the majority of boards. Yes we
>>>>> could fix them, but imho we are better off re-using what's already there
>>>>> and defined on the DT spec at least for the simplistic values.
>>>>
>>>> IMO SYS_VENDOR and SYS_BOARD are more descriptive than the devicetree
>>>> values, but neither is good...
>>>
>>> Didn't we use to do that? IOW fill in smbios nodes based on Kconfig
>>> values. But then we moved away from that in favor of the
>>> sysinfo-smbios node, but a very small amount of boards got converted.
>>
>> I mean that SYS_VENDOR and SYS_BOARD have content which more closely
>> matches the content of the SMBios tables, not that we should use them
>> ("neither is good...").
>>
>>>>
>>>> How many boards do we have which actually use the SMBIOS tables? There
>>>> are a lot of boards with EFI_LOADER enabled by default, but I suspect
>>>> most never boot anything EFI.
>>>
>>> I don't see how that's relevant? If someone for any reason enables
>>> smbios it shouldn't report always "Unknown".
>>
>> I'm mostly trying to figure out how much effort it would be to just add
>> nodes for all devices which boot with SMBios. I know that most boards
>> which have it enabled don't actually use it, since it's enabled by
>> default.
>
> It is a patch like this:
>
> https://patchwork.ozlabs.org/project/uboot/patch/20220929001520.9095-1-christian@kohlschutter.com/
>
> I just found out that this option is enabled for hundreds of boards.
I first noticed it when doing the K210 and wondering why I had EFI
enabled.
> Perhaps the solution is to turn it off unless the board enables it?
But how do we determine if the board enables it? Since it was on by
default, it's not so easy. One way would be to look at the boards which
use bootefi, but from what I can tell, that's enabled by distroboot.
Which has a similar problem where include/configs/mycpu_common.h might
enable it, but (most of) the boards for that cpu might not care.
--Sean
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] smbios: Fallback to the default DT if sysinfo nodes are missing
2022-09-29 10:23 ` Ilias Apalodimas
@ 2022-09-29 23:55 ` Simon Glass
2022-09-30 9:56 ` Mark Kettenis
0 siblings, 1 reply; 19+ messages in thread
From: Simon Glass @ 2022-09-29 23:55 UTC (permalink / raw)
To: Ilias Apalodimas
Cc: Peter Robinson, U-Boot Mailing List, Tom Rini, Heinrich Schuchardt
Hi Ilias,
On Thu, 29 Sept 2022 at 04:23, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> Hi Simon,
>
> On Thu, Sep 29, 2022 at 03:59:51AM -0600, Simon Glass wrote:
> > Hi,
> >
> > On Tue, 20 Sept 2022 at 05:10, Peter Robinson <pbrobinson@gmail.com> wrote:
> > >
> > > On Tue, Sep 6, 2022 at 2:44 PM Ilias Apalodimas
> > > <ilias.apalodimas@linaro.org> wrote:
> > > >
> > > > In order to fill in the SMBIOS tables U-Boot currently relies on a
> > > > "u-boot,sysinfo-smbios" compatible node. This is fine for the boards
> > > > that already include such nodes. However with some recent EFI changes,
> > > > the majority of boards can boot up distros, which usually rely on
> > > > things like dmidecode etc for their reporting. For boards that
> > > > lack this special node the SMBIOS output looks like:
> > > >
> > > > System Information
> > > > Manufacturer: Unknown
> > > > Product Name: Unknown
> > > > Version: Unknown
> > > > Serial Number: Unknown
> > > > UUID: Not Settable
> > > > Wake-up Type: Reserved
> > > > SKU Number: Unknown
> > > > Family: Unknown
> > > >
> > > > This looks problematic since most of the info are "Unknown". The DT spec
> > > > specifies standard properties containing relevant information like
> > > > 'model' and 'compatible' for which the suggested format is
> > > > <manufacturer,model>. So let's add a last resort to our current
> > > > smbios parsing. If none of the sysinfo properties are found, we can
> > > > scan the root node for 'model' and 'compatible'.
> > >
> > > I don't think the information below all needs to go in the commit,
> > > maybe in the cover letter?
> > >
> > > > pre-patch dmidecode:
> > > > <snip>
> > > > Handle 0x0001, DMI type 1, 27 bytes
> > > > System Information
> > > > Manufacturer: Unknown
> > > > Product Name: Unknown
> > > > Version: Unknown
> > > > Serial Number: Unknown
> > > > UUID: Not Settable
> > > > Wake-up Type: Reserved
> > > > SKU Number: Unknown
> > > > Family: Unknown
> > > >
> > > > Handle 0x0002, DMI type 2, 14 bytes
> > > > Base Board Information
> > > > Manufacturer: Unknown
> > > > Product Name: Unknown
> > > > Version: Unknown
> > > > Serial Number: Not Specified
> > > > Asset Tag: Unknown
> > > > Features:
> > > > Board is a hosting board
> > > > Location In Chassis: Not Specified
> > > > Chassis Handle: 0x0000
> > > > Type: Motherboard
> > > >
> > > > Handle 0x0003, DMI type 3, 21 bytes
> > > > Chassis Information
> > > > Manufacturer: Unknown
> > > > Type: Desktop
> > > > Lock: Not Present
> > > > Version: Not Specified
> > > > Serial Number: Not Specified
> > > > Asset Tag: Not Specified
> > > > Boot-up State: Safe
> > > > Power Supply State: Safe
> > > > Thermal State: Safe
> > > > Security Status: None
> > > > OEM Information: 0x00000000
> > > > Height: Unspecified
> > > > Number Of Power Cords: Unspecified
> > > > Contained Elements: 0
> > > > <snip>
> > > >
> > > > post-pastch dmidecode:
> > > > <snip>
> > > > Handle 0x0001, DMI type 1, 27 bytes
> > > > System Information
> > > > Manufacturer: socionext,developer-box
> > > > Product Name: Socionext Developer Box
> > > > Version: Unknown
> > > > Serial Number: Unknown
> > > > UUID: Not Settable
> > > > Wake-up Type: Reserved
> > > > SKU Number: Unknown
> > > > Family: Unknown
> > > >
> > > > Handle 0x0002, DMI type 2, 14 bytes
> > > > Base Board Information
> > > > Manufacturer: socionext,developer-box
> > > > Product Name: Socionext Developer Box
> > > > Version: Unknown
> > > > Serial Number: Not Specified
> > > > Asset Tag: Unknown
> > > > Features:
> > > > Board is a hosting board
> > > > Location In Chassis: Not Specified
> > > > Chassis Handle: 0x0000
> > > > Type: Motherboard
> > > >
> > > > Handle 0x0003, DMI type 3, 21 bytes
> > > > Chassis Information
> > > > Manufacturer: socionext,developer-box
> > > > Type: Desktop
> > > > Lock: Not Present
> > > > Version: Not Specified
> > > > Serial Number: Not Specified
> > > > Asset Tag: Not Specified
> > > > Boot-up State: Safe
> > > > Power Supply State: Safe
> > > > Thermal State: Safe
> > > > Security Status: None
> > > > OEM Information: 0x00000000
> > > > Height: Unspecified
> > > > Number Of Power Cords: Unspecified
> > > > Contained Elements: 0
> > > > <snip>
> > > >
> > > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > >
> > > Reviewed-by: Peter Robinson <pbrobinson@gmail.com>
> > > Tested-by: Peter Robinson <pbrobinson@gmail.com>
> > >
> > > > ---
> > > > lib/smbios.c | 41 +++++++++++++++++++++++++++++++++++++++--
> > > > 1 file changed, 39 insertions(+), 2 deletions(-)
> >
> > I've thought about this a lot.
> >
> > As I mentioned earlier, we should require boards to add this
> > information when they enable GENERATE_SMBIOS_TABLE
> >
> > It is a simple patch for each board vendor and it solves the problem.
> > What we have here just masks it.
>
>
> Not entirely. I think we just see the problem differently here. I agree
> that the code here masks a problem (but only for *some* boards) and ideally
> we should go and add smbios nodes on the boards that miss it. However we
> conveniently keep ignoring OF_BOARD here. Until those things are documented
> in a spec and you can *demand* a previous bootloader to include it, we'll have
> boards that display "Unknown" all over the place. Personally I don't
> think that's acceptable, hence the last resort solution.
I think you mean OF_HAS_PRIOR_STAGE - we have an explicit Kconfig now.
We can easily make U-Boot halt if the info is not there but it is
needed. That will cause people to fix it for their board.
>
> I'd be much happier if we popped a warning on boards that miss the smbios
> node and are not compiling with OF_BOARD, explaining smbios will be
> disabled for them in the future, while having the flexibility to not
> display values that make no sense.
How about just failing the build and producing an error, if people
enable the SMBIOS tables without the data? We could run with a warning
for a while if you like, then change it to an error.
Regards,
Simon
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] smbios: Fallback to the default DT if sysinfo nodes are missing
2022-09-29 23:55 ` Simon Glass
@ 2022-09-30 9:56 ` Mark Kettenis
2022-09-30 14:51 ` Tom Rini
0 siblings, 1 reply; 19+ messages in thread
From: Mark Kettenis @ 2022-09-30 9:56 UTC (permalink / raw)
To: Simon Glass
Cc: ilias.apalodimas, pbrobinson, u-boot, trini, heinrich.schuchardt
> From: Simon Glass <sjg@chromium.org>
> Date: Thu, 29 Sep 2022 17:55:43 -0600
>
> Hi Ilias,
>
> On Thu, 29 Sept 2022 at 04:23, Ilias Apalodimas
> <ilias.apalodimas@linaro.org> wrote:
> >
> > Hi Simon,
> >
> > On Thu, Sep 29, 2022 at 03:59:51AM -0600, Simon Glass wrote:
> > > Hi,
> > >
> > > On Tue, 20 Sept 2022 at 05:10, Peter Robinson <pbrobinson@gmail.com> wrote:
> > > >
> > > > On Tue, Sep 6, 2022 at 2:44 PM Ilias Apalodimas
> > > > <ilias.apalodimas@linaro.org> wrote:
> > > > >
> > > > > In order to fill in the SMBIOS tables U-Boot currently relies on a
> > > > > "u-boot,sysinfo-smbios" compatible node. This is fine for the boards
> > > > > that already include such nodes. However with some recent EFI changes,
> > > > > the majority of boards can boot up distros, which usually rely on
> > > > > things like dmidecode etc for their reporting. For boards that
> > > > > lack this special node the SMBIOS output looks like:
> > > > >
> > > > > System Information
> > > > > Manufacturer: Unknown
> > > > > Product Name: Unknown
> > > > > Version: Unknown
> > > > > Serial Number: Unknown
> > > > > UUID: Not Settable
> > > > > Wake-up Type: Reserved
> > > > > SKU Number: Unknown
> > > > > Family: Unknown
> > > > >
> > > > > This looks problematic since most of the info are "Unknown". The DT spec
> > > > > specifies standard properties containing relevant information like
> > > > > 'model' and 'compatible' for which the suggested format is
> > > > > <manufacturer,model>. So let's add a last resort to our current
> > > > > smbios parsing. If none of the sysinfo properties are found, we can
> > > > > scan the root node for 'model' and 'compatible'.
> > > >
> > > > I don't think the information below all needs to go in the commit,
> > > > maybe in the cover letter?
> > > >
> > > > > pre-patch dmidecode:
> > > > > <snip>
> > > > > Handle 0x0001, DMI type 1, 27 bytes
> > > > > System Information
> > > > > Manufacturer: Unknown
> > > > > Product Name: Unknown
> > > > > Version: Unknown
> > > > > Serial Number: Unknown
> > > > > UUID: Not Settable
> > > > > Wake-up Type: Reserved
> > > > > SKU Number: Unknown
> > > > > Family: Unknown
> > > > >
> > > > > Handle 0x0002, DMI type 2, 14 bytes
> > > > > Base Board Information
> > > > > Manufacturer: Unknown
> > > > > Product Name: Unknown
> > > > > Version: Unknown
> > > > > Serial Number: Not Specified
> > > > > Asset Tag: Unknown
> > > > > Features:
> > > > > Board is a hosting board
> > > > > Location In Chassis: Not Specified
> > > > > Chassis Handle: 0x0000
> > > > > Type: Motherboard
> > > > >
> > > > > Handle 0x0003, DMI type 3, 21 bytes
> > > > > Chassis Information
> > > > > Manufacturer: Unknown
> > > > > Type: Desktop
> > > > > Lock: Not Present
> > > > > Version: Not Specified
> > > > > Serial Number: Not Specified
> > > > > Asset Tag: Not Specified
> > > > > Boot-up State: Safe
> > > > > Power Supply State: Safe
> > > > > Thermal State: Safe
> > > > > Security Status: None
> > > > > OEM Information: 0x00000000
> > > > > Height: Unspecified
> > > > > Number Of Power Cords: Unspecified
> > > > > Contained Elements: 0
> > > > > <snip>
> > > > >
> > > > > post-pastch dmidecode:
> > > > > <snip>
> > > > > Handle 0x0001, DMI type 1, 27 bytes
> > > > > System Information
> > > > > Manufacturer: socionext,developer-box
> > > > > Product Name: Socionext Developer Box
> > > > > Version: Unknown
> > > > > Serial Number: Unknown
> > > > > UUID: Not Settable
> > > > > Wake-up Type: Reserved
> > > > > SKU Number: Unknown
> > > > > Family: Unknown
> > > > >
> > > > > Handle 0x0002, DMI type 2, 14 bytes
> > > > > Base Board Information
> > > > > Manufacturer: socionext,developer-box
> > > > > Product Name: Socionext Developer Box
> > > > > Version: Unknown
> > > > > Serial Number: Not Specified
> > > > > Asset Tag: Unknown
> > > > > Features:
> > > > > Board is a hosting board
> > > > > Location In Chassis: Not Specified
> > > > > Chassis Handle: 0x0000
> > > > > Type: Motherboard
> > > > >
> > > > > Handle 0x0003, DMI type 3, 21 bytes
> > > > > Chassis Information
> > > > > Manufacturer: socionext,developer-box
> > > > > Type: Desktop
> > > > > Lock: Not Present
> > > > > Version: Not Specified
> > > > > Serial Number: Not Specified
> > > > > Asset Tag: Not Specified
> > > > > Boot-up State: Safe
> > > > > Power Supply State: Safe
> > > > > Thermal State: Safe
> > > > > Security Status: None
> > > > > OEM Information: 0x00000000
> > > > > Height: Unspecified
> > > > > Number Of Power Cords: Unspecified
> > > > > Contained Elements: 0
> > > > > <snip>
> > > > >
> > > > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > > >
> > > > Reviewed-by: Peter Robinson <pbrobinson@gmail.com>
> > > > Tested-by: Peter Robinson <pbrobinson@gmail.com>
> > > >
> > > > > ---
> > > > > lib/smbios.c | 41 +++++++++++++++++++++++++++++++++++++++--
> > > > > 1 file changed, 39 insertions(+), 2 deletions(-)
> > >
> > > I've thought about this a lot.
> > >
> > > As I mentioned earlier, we should require boards to add this
> > > information when they enable GENERATE_SMBIOS_TABLE
> > >
> > > It is a simple patch for each board vendor and it solves the problem.
> > > What we have here just masks it.
> >
> >
> > Not entirely. I think we just see the problem differently here. I agree
> > that the code here masks a problem (but only for *some* boards) and ideally
> > we should go and add smbios nodes on the boards that miss it. However we
> > conveniently keep ignoring OF_BOARD here. Until those things are documented
> > in a spec and you can *demand* a previous bootloader to include it, we'll have
> > boards that display "Unknown" all over the place. Personally I don't
> > think that's acceptable, hence the last resort solution.
>
> I think you mean OF_HAS_PRIOR_STAGE - we have an explicit Kconfig now.
>
> We can easily make U-Boot halt if the info is not there but it is
> needed. That will cause people to fix it for their board.
That seems unecessarily harsh...
The smbios stuff is by no means essential to run an OS on a board. On
many low-end (or user assembled) x86 machines it is full of lies as
well (gotta love all those machines with serial number 123456789) and
a lot of the information in the tables doesn't make sense for
"embedded" boards anyway. At best the smbios tables are a "nice to
have" feature. But it seems to be mostly a box ticking excercise to
me.
> > I'd be much happier if we popped a warning on boards that miss the smbios
> > node and are not compiling with OF_BOARD, explaining smbios will be
> > disabled for them in the future, while having the flexibility to not
> > display values that make no sense.
>
> How about just failing the build and producing an error, if people
> enable the SMBIOS tables without the data? We could run with a warning
> for a while if you like, then change it to an error.
Again, that seems unecessarily harsh. If foks are really bothered
about the correctness of the information we supply, we should either
just not offer the tables if essential information is missing from the
device tree, or maybe require boards to explicitly request the smbios
feature by dropping the "|| EFI_LOADER" from its Kconfig entry.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/2] smbios: Simplify reporting of unknown values
2022-09-29 14:02 ` Sean Anderson
@ 2022-09-30 14:25 ` Tom Rini
0 siblings, 0 replies; 19+ messages in thread
From: Tom Rini @ 2022-09-30 14:25 UTC (permalink / raw)
To: Sean Anderson
Cc: Simon Glass, Ilias Apalodimas, U-Boot Mailing List,
Heinrich Schuchardt, Peter Robinson
[-- Attachment #1: Type: text/plain, Size: 9217 bytes --]
On Thu, Sep 29, 2022 at 10:02:48AM -0400, Sean Anderson wrote:
> On 9/29/22 05:59, Simon Glass wrote:
> > Hi,
> >
> > On Wed, 28 Sept 2022 at 22:34, Sean Anderson <seanga2@gmail.com> wrote:
> > >
> > > On 9/26/22 06:56, Ilias Apalodimas wrote:
> > > > Hi Sean
> > > >
> > > > On Sat, 17 Sept 2022 at 19:55, Sean Anderson <seanga2@gmail.com> wrote:
> > > > >
> > > > > On 9/16/22 16:30, Ilias Apalodimas wrote:
> > > > > > Hi Simon,
> > > > > >
> > > > > > [...]
> > > > > >
> > > > > > > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > > > > > > > ---
> > > > > > > > lib/smbios.c | 17 +++--------------
> > > > > > > > 1 file changed, 3 insertions(+), 14 deletions(-)
> > > > > > >
> > > > > > > Perhaps a better fix is to drop the smbios info?
> > > > > >
> > > > > > Unfortunately there's a ton of userspace tools still using it. So I think
> > > > > > we still need it
> > > > > >
> > > > > > >
> > > > > > > What upstream projects use this information to show things to the
> > > > > > > user? You showed a screenshot of some sort of system-info app. We
> > > > > > > could teach it about falling back to the device tree. That way we are
> > > > > > > not adding fake information to SMBIOS.
> > > > > > >
> > > > > >
> > > > > > What's fake here? The model and compatible are taken directly from the DT
> > > > > > and that should be accurate. I'd rather fix the DT if that's problematic.
> > > > > > What would make sense for me to change is take the first token of the
> > > > > > compatible node instead of the entire string as it's format is expected to
> > > > > > be <manufacturer, model> anyway.
> > > > >
> > > > > > Manufacturer: socionext,developer-box
> > > > > > Product Name: Socionext Developer Box
> > > > >
> > > > > Well, firstly, the manufacturer is "Socionext", not
> > > > > "socionext,developer-box". Compatibles are not suitable for
> > > > > user-visible identifiers. The product name should also be something like
> > > > > "Socionext Developerbox" or maybe "SynQuacer E-series", but this more of
> > > > > a "bug" in the devicetree model property.
> > > >
> > > > Yea as I said we can get rid of the everything after the ',' on the
> > > > compatible node. Ideally if vendors followed the DT spec, we could
> > > > also just use manufacturer node, the reality is that we can't though.
> > >
> > > This is another one of the problems with this approach. There's no
> > > consistency in existing device trees, because at most this info is
> > > printed in the boot log.
> > >
> > > > The whole point of the patchset is provide something reasonable
> > > > without having to add a .dtsi smbios node for all our devices. We can
> > > > then go back to fixing the DT with proper values if it's a DT "bug".
> > > > >
> > > > > Second, these identifiers are not suitable for all structures you want
> > > > > to use it for. For example, the chassis is really a "INWIN industrial PC
> > > > > case: MicroATX mini-tower case IW-BK623/300-H E USB 3.0 Black with 300W
> > > > > SFX power supply" [1]. I would describe this as something like
> > > >
> > > > The chassis isn't even addressed in the series. IIRC it's currently
> > > > hardcoded in smbios.c.
> > >
> > > You showed it as different in the commit message.
> > >
> > > > >
> > > > > Handle 0x0003, DMI type 3, 21 bytes
> > > > > Chassis Information
> > > > > Manufacturer: INWIN
> > > > > Type: Mini Tower
> > > > > Lock: Not Present
> > > > > Version: Unknown
> > > > > Serial Number: Not Specified
> > > > > Asset Tag: Not Specified
> > > > > Boot-up State: Safe
> > > > > Power Supply State: Safe
> > > > > Thermal State: Safe
> > > > > Security Status: None
> > > > > OEM Information: 0x00000000
> > > > > Height: Unspecified
> > > > > Number Of Power Cords: 1
> > > > > Contained Elements: 0
> > > > >
> > > > > The exact values are not particularly important, but I would certainly
> > > > > classify a manufacturer of "socionext,developer-box" as fake. We might
> > > > > not even know what the chassis is; what's to stop a user from using a
> > > > > different case?
> > > >
> > > > But the chassis isn't even addressed in the series? Again I am mostly
> > > > interested in a sane fallback for device and manufacturer.
> > >
> > > ditto
> > >
> > > > >
> > > > > [1] https://www.96boards.org/documentation/enterprise/developerbox/hardware-docs/MN04-00002-3E.pdf
> > > > >
> > > > > > > Also, SMBIOS is a legacy thing and a PITA to work with. How about we
> > > > > > > use the device tree binding for the same info:
> > > > > > >
> > > > > > > smbios {
> > > > > > > compatible = "u-boot,sysinfo-smbios";
> > > > > > >
> > > > > > > smbios {
> > > > > > > system {
> > > > > > > manufacturer = "pine64";
> > > > > > > product = "rock64_rk3328";
> > > > > > > };
> > > > > > >
> > > > > > > baseboard {
> > > > > > > manufacturer = "pine64";
> > > > > > > product = "rock64_rk3328";
> > > > > > > };
> > > > > > >
> > > > > > > chassis {
> > > > > > > manufacturer = "pine64";
> > > > > > > product = "rock64_rk3328";
> > > > > > > };
> > > > > > > };
> > > > > > > };
> > > > > > >
> > > > > > > This is easy to parse and gets us away from all this legacy junk that
> > > > > > > we don't need.
> > > > > >
> > > > > > That's the exact opposite of the patch description. Most of these info are
> > > > > > already included in the DT in it's standard properties. So if U-Boot ends
> > > > > > up with a DT without these we get a usable smbios table. For example a DT
> > > > > > handed over by the previous stage bootloader would not include these nodes.
> > > > >
> > > > > I agree. I think a better example would fill in these fields with
> > > > > descriptive values.
> > > >
> > > > We are off to a chicken and egg problem now. Can you provide U-Boot
> > > > with a 'configuration' DT, which would be disjoint from the DT that
> > > > describes hardware?
> > >
> > > Sorry, I misread the context there.
> > >
> > > I still don't think this is the right approach for this... better to fix
> > > the prior stage's devicetree.
> > >
> > > > >
> > > > > > As far as sysinfo-smbios node is concerned, it's only present in 13
> > > > > > boards, so it's not like it's used by the majority of boards. Yes we
> > > > > > could fix them, but imho we are better off re-using what's already there
> > > > > > and defined on the DT spec at least for the simplistic values.
> > > > >
> > > > > IMO SYS_VENDOR and SYS_BOARD are more descriptive than the devicetree
> > > > > values, but neither is good...
> > > >
> > > > Didn't we use to do that? IOW fill in smbios nodes based on Kconfig
> > > > values. But then we moved away from that in favor of the
> > > > sysinfo-smbios node, but a very small amount of boards got converted.
> > >
> > > I mean that SYS_VENDOR and SYS_BOARD have content which more closely
> > > matches the content of the SMBios tables, not that we should use them
> > > ("neither is good...").
> > >
> > > > >
> > > > > How many boards do we have which actually use the SMBIOS tables? There
> > > > > are a lot of boards with EFI_LOADER enabled by default, but I suspect
> > > > > most never boot anything EFI.
> > > >
> > > > I don't see how that's relevant? If someone for any reason enables
> > > > smbios it shouldn't report always "Unknown".
> > >
> > > I'm mostly trying to figure out how much effort it would be to just add
> > > nodes for all devices which boot with SMBios. I know that most boards
> > > which have it enabled don't actually use it, since it's enabled by
> > > default.
> >
> > It is a patch like this:
> >
> > https://patchwork.ozlabs.org/project/uboot/patch/20220929001520.9095-1-christian@kohlschutter.com/
> >
> > I just found out that this option is enabled for hundreds of boards.
>
> I first noticed it when doing the K210 and wondering why I had EFI
> enabled.
>
> > Perhaps the solution is to turn it off unless the board enables it?
>
> But how do we determine if the board enables it? Since it was on by
> default, it's not so easy. One way would be to look at the boards which
> use bootefi, but from what I can tell, that's enabled by distroboot.
> Which has a similar problem where include/configs/mycpu_common.h might
> enable it, but (most of) the boards for that cpu might not care.
I think the point that's trying to be made in the thread is that this
bit of code is common and widely used / visible as it's part of the
regular commodity Linux distro "tell the user useful things" tools. So
it should default to be as correct as can be.
--
Tom
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] smbios: Fallback to the default DT if sysinfo nodes are missing
2022-09-30 9:56 ` Mark Kettenis
@ 2022-09-30 14:51 ` Tom Rini
0 siblings, 0 replies; 19+ messages in thread
From: Tom Rini @ 2022-09-30 14:51 UTC (permalink / raw)
To: Mark Kettenis
Cc: Simon Glass, ilias.apalodimas, pbrobinson, u-boot, heinrich.schuchardt
[-- Attachment #1: Type: text/plain, Size: 7958 bytes --]
On Fri, Sep 30, 2022 at 11:56:53AM +0200, Mark Kettenis wrote:
> > From: Simon Glass <sjg@chromium.org>
> > Date: Thu, 29 Sep 2022 17:55:43 -0600
> >
> > Hi Ilias,
> >
> > On Thu, 29 Sept 2022 at 04:23, Ilias Apalodimas
> > <ilias.apalodimas@linaro.org> wrote:
> > >
> > > Hi Simon,
> > >
> > > On Thu, Sep 29, 2022 at 03:59:51AM -0600, Simon Glass wrote:
> > > > Hi,
> > > >
> > > > On Tue, 20 Sept 2022 at 05:10, Peter Robinson <pbrobinson@gmail.com> wrote:
> > > > >
> > > > > On Tue, Sep 6, 2022 at 2:44 PM Ilias Apalodimas
> > > > > <ilias.apalodimas@linaro.org> wrote:
> > > > > >
> > > > > > In order to fill in the SMBIOS tables U-Boot currently relies on a
> > > > > > "u-boot,sysinfo-smbios" compatible node. This is fine for the boards
> > > > > > that already include such nodes. However with some recent EFI changes,
> > > > > > the majority of boards can boot up distros, which usually rely on
> > > > > > things like dmidecode etc for their reporting. For boards that
> > > > > > lack this special node the SMBIOS output looks like:
> > > > > >
> > > > > > System Information
> > > > > > Manufacturer: Unknown
> > > > > > Product Name: Unknown
> > > > > > Version: Unknown
> > > > > > Serial Number: Unknown
> > > > > > UUID: Not Settable
> > > > > > Wake-up Type: Reserved
> > > > > > SKU Number: Unknown
> > > > > > Family: Unknown
> > > > > >
> > > > > > This looks problematic since most of the info are "Unknown". The DT spec
> > > > > > specifies standard properties containing relevant information like
> > > > > > 'model' and 'compatible' for which the suggested format is
> > > > > > <manufacturer,model>. So let's add a last resort to our current
> > > > > > smbios parsing. If none of the sysinfo properties are found, we can
> > > > > > scan the root node for 'model' and 'compatible'.
> > > > >
> > > > > I don't think the information below all needs to go in the commit,
> > > > > maybe in the cover letter?
> > > > >
> > > > > > pre-patch dmidecode:
> > > > > > <snip>
> > > > > > Handle 0x0001, DMI type 1, 27 bytes
> > > > > > System Information
> > > > > > Manufacturer: Unknown
> > > > > > Product Name: Unknown
> > > > > > Version: Unknown
> > > > > > Serial Number: Unknown
> > > > > > UUID: Not Settable
> > > > > > Wake-up Type: Reserved
> > > > > > SKU Number: Unknown
> > > > > > Family: Unknown
> > > > > >
> > > > > > Handle 0x0002, DMI type 2, 14 bytes
> > > > > > Base Board Information
> > > > > > Manufacturer: Unknown
> > > > > > Product Name: Unknown
> > > > > > Version: Unknown
> > > > > > Serial Number: Not Specified
> > > > > > Asset Tag: Unknown
> > > > > > Features:
> > > > > > Board is a hosting board
> > > > > > Location In Chassis: Not Specified
> > > > > > Chassis Handle: 0x0000
> > > > > > Type: Motherboard
> > > > > >
> > > > > > Handle 0x0003, DMI type 3, 21 bytes
> > > > > > Chassis Information
> > > > > > Manufacturer: Unknown
> > > > > > Type: Desktop
> > > > > > Lock: Not Present
> > > > > > Version: Not Specified
> > > > > > Serial Number: Not Specified
> > > > > > Asset Tag: Not Specified
> > > > > > Boot-up State: Safe
> > > > > > Power Supply State: Safe
> > > > > > Thermal State: Safe
> > > > > > Security Status: None
> > > > > > OEM Information: 0x00000000
> > > > > > Height: Unspecified
> > > > > > Number Of Power Cords: Unspecified
> > > > > > Contained Elements: 0
> > > > > > <snip>
> > > > > >
> > > > > > post-pastch dmidecode:
> > > > > > <snip>
> > > > > > Handle 0x0001, DMI type 1, 27 bytes
> > > > > > System Information
> > > > > > Manufacturer: socionext,developer-box
> > > > > > Product Name: Socionext Developer Box
> > > > > > Version: Unknown
> > > > > > Serial Number: Unknown
> > > > > > UUID: Not Settable
> > > > > > Wake-up Type: Reserved
> > > > > > SKU Number: Unknown
> > > > > > Family: Unknown
> > > > > >
> > > > > > Handle 0x0002, DMI type 2, 14 bytes
> > > > > > Base Board Information
> > > > > > Manufacturer: socionext,developer-box
> > > > > > Product Name: Socionext Developer Box
> > > > > > Version: Unknown
> > > > > > Serial Number: Not Specified
> > > > > > Asset Tag: Unknown
> > > > > > Features:
> > > > > > Board is a hosting board
> > > > > > Location In Chassis: Not Specified
> > > > > > Chassis Handle: 0x0000
> > > > > > Type: Motherboard
> > > > > >
> > > > > > Handle 0x0003, DMI type 3, 21 bytes
> > > > > > Chassis Information
> > > > > > Manufacturer: socionext,developer-box
> > > > > > Type: Desktop
> > > > > > Lock: Not Present
> > > > > > Version: Not Specified
> > > > > > Serial Number: Not Specified
> > > > > > Asset Tag: Not Specified
> > > > > > Boot-up State: Safe
> > > > > > Power Supply State: Safe
> > > > > > Thermal State: Safe
> > > > > > Security Status: None
> > > > > > OEM Information: 0x00000000
> > > > > > Height: Unspecified
> > > > > > Number Of Power Cords: Unspecified
> > > > > > Contained Elements: 0
> > > > > > <snip>
> > > > > >
> > > > > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > > > >
> > > > > Reviewed-by: Peter Robinson <pbrobinson@gmail.com>
> > > > > Tested-by: Peter Robinson <pbrobinson@gmail.com>
> > > > >
> > > > > > ---
> > > > > > lib/smbios.c | 41 +++++++++++++++++++++++++++++++++++++++--
> > > > > > 1 file changed, 39 insertions(+), 2 deletions(-)
> > > >
> > > > I've thought about this a lot.
> > > >
> > > > As I mentioned earlier, we should require boards to add this
> > > > information when they enable GENERATE_SMBIOS_TABLE
> > > >
> > > > It is a simple patch for each board vendor and it solves the problem.
> > > > What we have here just masks it.
> > >
> > >
> > > Not entirely. I think we just see the problem differently here. I agree
> > > that the code here masks a problem (but only for *some* boards) and ideally
> > > we should go and add smbios nodes on the boards that miss it. However we
> > > conveniently keep ignoring OF_BOARD here. Until those things are documented
> > > in a spec and you can *demand* a previous bootloader to include it, we'll have
> > > boards that display "Unknown" all over the place. Personally I don't
> > > think that's acceptable, hence the last resort solution.
> >
> > I think you mean OF_HAS_PRIOR_STAGE - we have an explicit Kconfig now.
> >
> > We can easily make U-Boot halt if the info is not there but it is
> > needed. That will cause people to fix it for their board.
>
> That seems unecessarily harsh...
>
> The smbios stuff is by no means essential to run an OS on a board. On
> many low-end (or user assembled) x86 machines it is full of lies as
> well (gotta love all those machines with serial number 123456789) and
> a lot of the information in the tables doesn't make sense for
> "embedded" boards anyway. At best the smbios tables are a "nice to
> have" feature. But it seems to be mostly a box ticking excercise to
> me.
This is another point I'd been trying to think how to best bring up. The
majority of x86 HW I've ever used is full of useless values. Even my
laptop has some to be filled in values. So it's entirely reasonable I
think to populate some default values and document how and where to put
something more correct, when it's possible / useful.
--
Tom
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]
^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2022-09-30 14:52 UTC | newest]
Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-06 13:44 [PATCH 1/2] smbios: Simplify reporting of unknown values Ilias Apalodimas
2022-09-06 13:44 ` [PATCH 2/2] smbios: Fallback to the default DT if sysinfo nodes are missing Ilias Apalodimas
2022-09-20 11:10 ` Peter Robinson
2022-09-29 9:59 ` Simon Glass
2022-09-29 10:23 ` Ilias Apalodimas
2022-09-29 23:55 ` Simon Glass
2022-09-30 9:56 ` Mark Kettenis
2022-09-30 14:51 ` Tom Rini
2022-09-12 18:31 ` [PATCH 1/2] smbios: Simplify reporting of unknown values Simon Glass
2022-09-16 20:30 ` Ilias Apalodimas
2022-09-17 16:55 ` Sean Anderson
2022-09-26 10:56 ` Ilias Apalodimas
2022-09-29 4:34 ` Sean Anderson
2022-09-29 9:59 ` Simon Glass
2022-09-29 14:02 ` Sean Anderson
2022-09-30 14:25 ` Tom Rini
2022-09-29 10:09 ` Ilias Apalodimas
2022-09-20 10:15 ` Peter Robinson
2022-09-20 11:09 ` Peter Robinson
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.