[PATCH 1/2] smbios: Simplify reporting of unknown values
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;
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); }
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
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
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
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
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.
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.
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
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
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...
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
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 BoxWell, 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...
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
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 BoxWell, 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...
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
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 BoxWell, 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...
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-chris...
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
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 BoxWell, 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...
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-chris...
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
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 BoxWell, 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...
> 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-chris...
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.
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 BoxWell, 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...
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
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
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
participants (6)
-
Ilias Apalodimas -
Mark Kettenis -
Peter Robinson -
Sean Anderson -
Simon Glass -
Tom Rini