[PATCH 0/2 v3] Provide a fallback to smbios tables

[PATCH 0/2 v3] Provide a fallback to smbios tables

[Ilias Apalodimas]

Hi,

This is v3 of the smbios series [0].
v3 has a bigger cleanup in the existing smbios code, folding in
smbios_add_string() that were sprinkled around  to smbios_add_prop().

The latter is now the only caller of smbios_add_string().

Simon asked for a selftest which makes sense, but we plan to refactor some
of the smbios creation code anyway. I am happy to add tests once we do that

I've also removed the r-b tags from Peter and Tom, since some of the code
they reviewed changed.

[0] https://lore.kernel.org/u-boot/20231127171058.165777-1-ilias.apalodimas@linaro.org/
Ilias Apalodimas (2):
  smbios: Simplify reporting of unknown values
  smbios: Fallback to the default DT if sysinfo nodes are missing

 lib/smbios.c | 163 ++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 122 insertions(+), 41 deletions(-)

--
2.40.1

[PATCH 1/2 v3] smbios: Simplify reporting of unknown values

[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_prop_si() and provide an alternative string in case the
primary is NULL or empty

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

[...]

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
[...]

While at it make smbios_add_prop_si() add a string directly if the prop
node is NULL and replace smbios_add_string() calls with
smbios_add_prop_si(ctx, NULL, ....)

Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
Changes since v2:
- refactor even more code and remove the smbios_add_string calls from the
  main code. Instead use smbios_add_prop() foir everything and add a
  default value, in case the parsed one is NULL or emtpy
Changes since v1:
- None

 lib/smbios.c | 73 +++++++++++++++++++++++++---------------------------
 1 file changed, 35 insertions(+), 38 deletions(-)

diff --git a/lib/smbios.c b/lib/smbios.c
index d7f4999e8b2a..444aa245a273 100644
--- a/lib/smbios.c
+++ b/lib/smbios.c
@@ -102,9 +102,6 @@ static int smbios_add_string(struct smbios_ctx *ctx, const char *str)
 	int i = 1;
 	char *p = ctx->eos;

-	if (!*str)
-		str = "Unknown";
-
 	for (;;) {
 		if (!*p) {
 			ctx->last_str = p;
@@ -134,11 +131,18 @@ static int smbios_add_string(struct smbios_ctx *ctx, const char *str)
  *
  * @ctx:	context for writing the tables
  * @prop:	property to write
+ * @dval:	Default value to use if the string is not found or is empty
  * Return:	0 if not found, else SMBIOS string number (1 or more)
  */
 static int smbios_add_prop_si(struct smbios_ctx *ctx, const char *prop,
-			      int sysinfo_id)
+			      int sysinfo_id, const char *dval)
 {
+	if (!dval || !*dval)
+		dval = "Unknown";
+
+	if (!prop)
+		return smbios_add_string(ctx, dval);
+
 	if (sysinfo_id && ctx->dev) {
 		char val[SMBIOS_STR_MAX];
 		int ret;
@@ -151,8 +155,8 @@ 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 ? str : dval);
 	}

 	return 0;
@@ -161,12 +165,15 @@ static int smbios_add_prop_si(struct smbios_ctx *ctx, const char *prop,
 /**
  * smbios_add_prop() - Add a property from the devicetree
  *
- * @prop:	property to write
+ * @prop:	property to write. The default string will be written if
+ *		prop is NULL
+ * @dval:	Default value to use if the string is not found or is empty
  * Return:	0 if not found, else SMBIOS string number (1 or more)
  */
-static int smbios_add_prop(struct smbios_ctx *ctx, const char *prop)
+static int smbios_add_prop(struct smbios_ctx *ctx, const char *prop,
+			   const char *dval)
 {
-	return smbios_add_prop_si(ctx, prop, SYSINFO_ID_NONE);
+	return smbios_add_prop_si(ctx, prop, SYSINFO_ID_NONE, dval);
 }

 static void smbios_set_eos(struct smbios_ctx *ctx, char *eos)
@@ -228,11 +235,9 @@ static int smbios_write_type0(ulong *current, int handle,
 	memset(t, 0, sizeof(struct smbios_type0));
 	fill_smbios_header(t, SMBIOS_BIOS_INFORMATION, len, handle);
 	smbios_set_eos(ctx, t->eos);
-	t->vendor = smbios_add_string(ctx, "U-Boot");
+	t->vendor = smbios_add_prop(ctx, NULL, "U-Boot");

-	t->bios_ver = smbios_add_prop(ctx, "version");
-	if (!t->bios_ver)
-		t->bios_ver = smbios_add_string(ctx, PLAIN_VERSION);
+	t->bios_ver = smbios_add_prop(ctx, "version", PLAIN_VERSION);
 	if (t->bios_ver)
 		gd->smbios_version = ctx->last_str;
 	log_debug("smbios_version = %p: '%s'\n", gd->smbios_version,
@@ -241,7 +246,7 @@ static int smbios_write_type0(ulong *current, int handle,
 	print_buffer((ulong)gd->smbios_version, gd->smbios_version,
 		     1, strlen(gd->smbios_version) + 1, 0);
 #endif
-	t->bios_release_date = smbios_add_string(ctx, U_BOOT_DMI_DATE);
+	t->bios_release_date = smbios_add_prop(ctx, NULL, U_BOOT_DMI_DATE);
 #ifdef CONFIG_ROM_SIZE
 	t->bios_rom_size = (CONFIG_ROM_SIZE / 65536) - 1;
 #endif
@@ -280,22 +285,19 @@ static int smbios_write_type1(ulong *current, int handle,
 	memset(t, 0, sizeof(struct smbios_type1));
 	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->manufacturer = smbios_add_prop(ctx, "manufacturer", "Unknown");
+	t->product_name = smbios_add_prop(ctx, "product", "Unknown");
 	t->version = smbios_add_prop_si(ctx, "version",
-					SYSINFO_ID_SMBIOS_SYSTEM_VERSION);
+					SYSINFO_ID_SMBIOS_SYSTEM_VERSION,
+					"Unknown");
 	if (serial_str) {
-		t->serial_number = smbios_add_string(ctx, serial_str);
+		t->serial_number = smbios_add_prop(ctx, NULL, serial_str);
 		strncpy((char *)t->uuid, serial_str, sizeof(t->uuid));
 	} else {
-		t->serial_number = smbios_add_prop(ctx, "serial");
+		t->serial_number = smbios_add_prop(ctx, "serial", "Unknown");
 	}
-	t->sku_number = smbios_add_prop(ctx, "sku");
-	t->family = smbios_add_prop(ctx, "family");
+	t->sku_number = smbios_add_prop(ctx, "sku", "Unknown");
+	t->family = smbios_add_prop(ctx, "family", "Unknown");

 	len = t->length + smbios_string_table_len(ctx);
 	*current += len;
@@ -314,15 +316,12 @@ static int smbios_write_type2(ulong *current, int handle,
 	memset(t, 0, sizeof(struct smbios_type2));
 	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->manufacturer = smbios_add_prop(ctx, "manufacturer", "Unknown");
+	t->product_name = smbios_add_prop(ctx, "product", "Unknown");
 	t->version = smbios_add_prop_si(ctx, "version",
-					SYSINFO_ID_SMBIOS_BASEBOARD_VERSION);
-	t->asset_tag_number = smbios_add_prop(ctx, "asset-tag");
+					SYSINFO_ID_SMBIOS_BASEBOARD_VERSION,
+					"Unknown");
+	t->asset_tag_number = smbios_add_prop(ctx, "asset-tag", "Unknown");
 	t->feature_flags = SMBIOS_BOARD_FEATURE_HOSTING;
 	t->board_type = SMBIOS_BOARD_MOTHERBOARD;

@@ -343,9 +342,7 @@ static int smbios_write_type3(ulong *current, int handle,
 	memset(t, 0, sizeof(struct smbios_type3));
 	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->manufacturer = smbios_add_prop(ctx, "manufacturer", "Unknown");
 	t->chassis_type = SMBIOS_ENCLOSURE_DESKTOP;
 	t->bootup_state = SMBIOS_STATE_SAFE;
 	t->power_supply_state = SMBIOS_STATE_SAFE;
@@ -388,8 +385,8 @@ static void smbios_write_type4_dm(struct smbios_type4 *t,
 #endif

 	t->processor_family = processor_family;
-	t->processor_manufacturer = smbios_add_string(ctx, vendor);
-	t->processor_version = smbios_add_string(ctx, name);
+	t->processor_manufacturer = smbios_add_prop(ctx, NULL, vendor);
+	t->processor_version = smbios_add_prop(ctx, NULL, name);
 }

 static int smbios_write_type4(ulong *current, int handle,
--
2.40.1

[PATCH 2/2 v3] smbios: Fallback to the default DT if sysinfo nodes are missing

[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>. Unfortunately the 'model' string found in DTs is
usually lacking the manufacturer so we can't use it for both
'Manufacturer' and 'Product Name' SMBIOS entries reliably.

So let's add a last resort to our current smbios parsing.  If none of
the sysinfo properties are found, scan for those information in the
root node of the device tree. Use the 'model' to fill the 'Product
Name' and the first value of 'compatible' for the 'Manufacturer', since
that always contains one.

pre-patch:
Handle 0x0001, DMI type 1, 27 bytes
System Information
        Manufacturer: Unknown
        Product Name: Unknown
        Version: Unknown
        Serial Number: 100000000bb24ceb
        UUID: 30303031-3030-3030-3061-613234636435
        Wake-up Type: Reserved
        SKU Number: Unknown
        Family: Unknown
[...]

and post patch:
Handle 0x0001, DMI type 1, 27 bytes
System Information
        Manufacturer: raspberrypi
        Product Name: Raspberry Pi 4 Model B Rev 1.1
        Version: Unknown
        Serial Number: 100000000bb24ceb
        UUID: 30303031-3030-3030-3061-613234636435
        Wake-up Type: Reserved
        SKU Number: Unknown
        Family: Unknown
[...]

Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
Simon, I'll work with tou on the refactoring you wanted and
remove the EFI requirement of SMBIOS in !x86 platforms.
Since this code has no tests, I'll add some once the refactoring
is done

Changes since v2:
- Spelling mistakes
- rebase on top of patch #1 changes
Changes since v1:
- Tokenize the DT node entry and use the appropriate value instead of
  the entire string
- Removed Peters tested/reviewed-by tags due to the above
 lib/smbios.c | 94 +++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 89 insertions(+), 5 deletions(-)

diff --git a/lib/smbios.c b/lib/smbios.c
index 444aa245a273..3f0e1d529537 100644
--- a/lib/smbios.c
+++ b/lib/smbios.c
@@ -9,11 +9,14 @@
 #include <dm.h>
 #include <env.h>
 #include <linux/stringify.h>
+#include <linux/string.h>
 #include <mapmem.h>
 #include <smbios.h>
 #include <sysinfo.h>
 #include <tables_csum.h>
 #include <version.h>
+#include <malloc.h>
+#include <dm/ofnode.h>
 #ifdef CONFIG_CPU
 #include <cpu.h>
 #include <dm/uclass-internal.h>
@@ -43,6 +46,25 @@

 DECLARE_GLOBAL_DATA_PTR;

+/**
+ * struct map_sysinfo - Mapping of sysinfo strings to DT
+ *
+ * @sysinfo_str: sysinfo string
+ * @dt_str: DT string
+ * @max: Max index of the tokenized string to pick. Counting starts from 0
+ *
+ */
+struct map_sysinfo {
+	const char *sysinfo_str;
+	const char *dt_str;
+	int max;
+};
+
+static const struct map_sysinfo sysinfo_to_dt[] = {
+	{ .sysinfo_str = "product", .dt_str = "model", 2 },
+	{ .sysinfo_str = "manufacturer", .dt_str = "compatible", 1 },
+};
+
 /**
  * struct smbios_ctx - context for writing SMBIOS tables
  *
@@ -87,6 +109,18 @@ struct smbios_write_method {
 	const char *subnode_name;
 };

+static const struct map_sysinfo *convert_sysinfo_to_dt(const char *sysinfo_str)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(sysinfo_to_dt); i++) {
+		if (!strcmp(sysinfo_str, sysinfo_to_dt[i].sysinfo_str))
+			return &sysinfo_to_dt[i];
+	}
+
+	return NULL;
+}
+
 /**
  * smbios_add_string() - add a string to the string area
  *
@@ -124,6 +158,42 @@ static int smbios_add_string(struct smbios_ctx *ctx, const char *str)
 	}
 }

+/**
+ * get_str_from_dt - Get a substring from a DT property.
+ *                   After finding the property in the DT, the function
+ *                   will parse comma-separated values and return the value.
+ *                   If nprop->max exceeds the number of comma-separated
+ *                   elements, the last non NULL value will be returned.
+ *                   Counting starts from zero.
+ *
+ * @nprop: sysinfo property to use
+ * @str: pointer to fill with data
+ * @size: str buffer length
+ */
+static
+void get_str_from_dt(const struct map_sysinfo *nprop, char *str, size_t size)
+{
+	const char *dt_str;
+	int cnt = 0;
+	char *token;
+
+	memset(str, 0, size);
+	if (!nprop || !nprop->max)
+		return;
+
+	dt_str = ofnode_read_string(ofnode_root(), nprop->dt_str);
+	if (!dt_str)
+		return;
+
+	memcpy(str, dt_str, size);
+	token = strtok(str, ",");
+	while (token && cnt < nprop->max) {
+		strlcpy(str, token, strlen(token) + 1);
+		token = strtok(NULL, ",");
+		cnt++;
+	}
+}
+
 /**
  * smbios_add_prop_si() - Add a property from the devicetree or sysinfo
  *
@@ -137,6 +207,8 @@ static int smbios_add_string(struct smbios_ctx *ctx, const char *str)
 static int smbios_add_prop_si(struct smbios_ctx *ctx, const char *prop,
 			      int sysinfo_id, const char *dval)
 {
+	int ret;
+
 	if (!dval || !*dval)
 		dval = "Unknown";

@@ -145,18 +217,30 @@ static int smbios_add_prop_si(struct smbios_ctx *ctx, const char *prop,

 	if (sysinfo_id && ctx->dev) {
 		char val[SMBIOS_STR_MAX];
-		int ret;

 		ret = sysinfo_get_str(ctx->dev, sysinfo_id, sizeof(val), val);
 		if (!ret)
 			return smbios_add_string(ctx, val);
 	}
 	if (IS_ENABLED(CONFIG_OF_CONTROL)) {
-		const char *str;
-
-		str = ofnode_read_string(ctx->node, prop);
+		const char *str = NULL;
+		char str_dt[128] = { 0 };
+		/*
+		 * If the node is not valid fallback and try the entire DT
+		 * so we can at least fill in manufacturer and board type
+		 */
+		if (ofnode_valid(ctx->node)) {
+			str = ofnode_read_string(ctx->node, prop);
+		} else {
+			const struct map_sysinfo *nprop;
+
+			nprop = convert_sysinfo_to_dt(prop);
+			get_str_from_dt(nprop, str_dt, sizeof(str_dt));
+			str = (const char *)str_dt;
+		}

-		return smbios_add_string(ctx, str ? str : dval);
+		ret = smbios_add_string(ctx, str && *str ? str : dval);
+		return ret;
 	}

 	return 0;
--
2.40.1

Re: [PATCH 1/2 v3] smbios: Simplify reporting of unknown values

[Peter Robinson]

On Thu, Dec 7, 2023 at 10:17 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_prop_si() and provide an alternative string in case the
> primary is NULL or empty
>
> 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
>
> [...]
>
> 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
> [...]
>
> While at it make smbios_add_prop_si() add a string directly if the prop
> node is NULL and replace smbios_add_string() calls with
> smbios_add_prop_si(ctx, NULL, ....)
>
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
Reviewed-by: Peter Robinson <pbrobinson@gmail.com>
Tested-by: Peter Robinson <pbrobinson@gmail.com>

> ---
> Changes since v2:
> - refactor even more code and remove the smbios_add_string calls from the
>   main code. Instead use smbios_add_prop() foir everything and add a
>   default value, in case the parsed one is NULL or emtpy
> Changes since v1:
> - None
>
>  lib/smbios.c | 73 +++++++++++++++++++++++++---------------------------
>  1 file changed, 35 insertions(+), 38 deletions(-)
>
> diff --git a/lib/smbios.c b/lib/smbios.c
> index d7f4999e8b2a..444aa245a273 100644
> --- a/lib/smbios.c
> +++ b/lib/smbios.c
> @@ -102,9 +102,6 @@ static int smbios_add_string(struct smbios_ctx *ctx, const char *str)
>         int i = 1;
>         char *p = ctx->eos;
>
> -       if (!*str)
> -               str = "Unknown";
> -
>         for (;;) {
>                 if (!*p) {
>                         ctx->last_str = p;
> @@ -134,11 +131,18 @@ static int smbios_add_string(struct smbios_ctx *ctx, const char *str)
>   *
>   * @ctx:       context for writing the tables
>   * @prop:      property to write
> + * @dval:      Default value to use if the string is not found or is empty
>   * Return:     0 if not found, else SMBIOS string number (1 or more)
>   */
>  static int smbios_add_prop_si(struct smbios_ctx *ctx, const char *prop,
> -                             int sysinfo_id)
> +                             int sysinfo_id, const char *dval)
>  {
> +       if (!dval || !*dval)
> +               dval = "Unknown";
> +
> +       if (!prop)
> +               return smbios_add_string(ctx, dval);
> +
>         if (sysinfo_id && ctx->dev) {
>                 char val[SMBIOS_STR_MAX];
>                 int ret;
> @@ -151,8 +155,8 @@ 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 ? str : dval);
>         }
>
>         return 0;
> @@ -161,12 +165,15 @@ static int smbios_add_prop_si(struct smbios_ctx *ctx, const char *prop,
>  /**
>   * smbios_add_prop() - Add a property from the devicetree
>   *
> - * @prop:      property to write
> + * @prop:      property to write. The default string will be written if
> + *             prop is NULL
> + * @dval:      Default value to use if the string is not found or is empty
>   * Return:     0 if not found, else SMBIOS string number (1 or more)
>   */
> -static int smbios_add_prop(struct smbios_ctx *ctx, const char *prop)
> +static int smbios_add_prop(struct smbios_ctx *ctx, const char *prop,
> +                          const char *dval)
>  {
> -       return smbios_add_prop_si(ctx, prop, SYSINFO_ID_NONE);
> +       return smbios_add_prop_si(ctx, prop, SYSINFO_ID_NONE, dval);
>  }
>
>  static void smbios_set_eos(struct smbios_ctx *ctx, char *eos)
> @@ -228,11 +235,9 @@ static int smbios_write_type0(ulong *current, int handle,
>         memset(t, 0, sizeof(struct smbios_type0));
>         fill_smbios_header(t, SMBIOS_BIOS_INFORMATION, len, handle);
>         smbios_set_eos(ctx, t->eos);
> -       t->vendor = smbios_add_string(ctx, "U-Boot");
> +       t->vendor = smbios_add_prop(ctx, NULL, "U-Boot");
>
> -       t->bios_ver = smbios_add_prop(ctx, "version");
> -       if (!t->bios_ver)
> -               t->bios_ver = smbios_add_string(ctx, PLAIN_VERSION);
> +       t->bios_ver = smbios_add_prop(ctx, "version", PLAIN_VERSION);
>         if (t->bios_ver)
>                 gd->smbios_version = ctx->last_str;
>         log_debug("smbios_version = %p: '%s'\n", gd->smbios_version,
> @@ -241,7 +246,7 @@ static int smbios_write_type0(ulong *current, int handle,
>         print_buffer((ulong)gd->smbios_version, gd->smbios_version,
>                      1, strlen(gd->smbios_version) + 1, 0);
>  #endif
> -       t->bios_release_date = smbios_add_string(ctx, U_BOOT_DMI_DATE);
> +       t->bios_release_date = smbios_add_prop(ctx, NULL, U_BOOT_DMI_DATE);
>  #ifdef CONFIG_ROM_SIZE
>         t->bios_rom_size = (CONFIG_ROM_SIZE / 65536) - 1;
>  #endif
> @@ -280,22 +285,19 @@ static int smbios_write_type1(ulong *current, int handle,
>         memset(t, 0, sizeof(struct smbios_type1));
>         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->manufacturer = smbios_add_prop(ctx, "manufacturer", "Unknown");
> +       t->product_name = smbios_add_prop(ctx, "product", "Unknown");
>         t->version = smbios_add_prop_si(ctx, "version",
> -                                       SYSINFO_ID_SMBIOS_SYSTEM_VERSION);
> +                                       SYSINFO_ID_SMBIOS_SYSTEM_VERSION,
> +                                       "Unknown");
>         if (serial_str) {
> -               t->serial_number = smbios_add_string(ctx, serial_str);
> +               t->serial_number = smbios_add_prop(ctx, NULL, serial_str);
>                 strncpy((char *)t->uuid, serial_str, sizeof(t->uuid));
>         } else {
> -               t->serial_number = smbios_add_prop(ctx, "serial");
> +               t->serial_number = smbios_add_prop(ctx, "serial", "Unknown");
>         }
> -       t->sku_number = smbios_add_prop(ctx, "sku");
> -       t->family = smbios_add_prop(ctx, "family");
> +       t->sku_number = smbios_add_prop(ctx, "sku", "Unknown");
> +       t->family = smbios_add_prop(ctx, "family", "Unknown");
>
>         len = t->length + smbios_string_table_len(ctx);
>         *current += len;
> @@ -314,15 +316,12 @@ static int smbios_write_type2(ulong *current, int handle,
>         memset(t, 0, sizeof(struct smbios_type2));
>         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->manufacturer = smbios_add_prop(ctx, "manufacturer", "Unknown");
> +       t->product_name = smbios_add_prop(ctx, "product", "Unknown");
>         t->version = smbios_add_prop_si(ctx, "version",
> -                                       SYSINFO_ID_SMBIOS_BASEBOARD_VERSION);
> -       t->asset_tag_number = smbios_add_prop(ctx, "asset-tag");
> +                                       SYSINFO_ID_SMBIOS_BASEBOARD_VERSION,
> +                                       "Unknown");
> +       t->asset_tag_number = smbios_add_prop(ctx, "asset-tag", "Unknown");
>         t->feature_flags = SMBIOS_BOARD_FEATURE_HOSTING;
>         t->board_type = SMBIOS_BOARD_MOTHERBOARD;
>
> @@ -343,9 +342,7 @@ static int smbios_write_type3(ulong *current, int handle,
>         memset(t, 0, sizeof(struct smbios_type3));
>         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->manufacturer = smbios_add_prop(ctx, "manufacturer", "Unknown");
>         t->chassis_type = SMBIOS_ENCLOSURE_DESKTOP;
>         t->bootup_state = SMBIOS_STATE_SAFE;
>         t->power_supply_state = SMBIOS_STATE_SAFE;
> @@ -388,8 +385,8 @@ static void smbios_write_type4_dm(struct smbios_type4 *t,
>  #endif
>
>         t->processor_family = processor_family;
> -       t->processor_manufacturer = smbios_add_string(ctx, vendor);
> -       t->processor_version = smbios_add_string(ctx, name);
> +       t->processor_manufacturer = smbios_add_prop(ctx, NULL, vendor);
> +       t->processor_version = smbios_add_prop(ctx, NULL, name);
>  }
>
>  static int smbios_write_type4(ulong *current, int handle,
> --
> 2.40.1
>

Re: [PATCH 2/2 v3] smbios: Fallback to the default DT if sysinfo nodes are missing

[Peter Robinson]

On Thu, Dec 7, 2023 at 12:57 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>. Unfortunately the 'model' string found in DTs is
> usually lacking the manufacturer so we can't use it for both
> 'Manufacturer' and 'Product Name' SMBIOS entries reliably.
>
> So let's add a last resort to our current smbios parsing.  If none of
> the sysinfo properties are found, scan for those information in the
> root node of the device tree. Use the 'model' to fill the 'Product
> Name' and the first value of 'compatible' for the 'Manufacturer', since
> that always contains one.
>
> pre-patch:
> Handle 0x0001, DMI type 1, 27 bytes
> System Information
>         Manufacturer: Unknown
>         Product Name: Unknown
>         Version: Unknown
>         Serial Number: 100000000bb24ceb
>         UUID: 30303031-3030-3030-3061-613234636435
>         Wake-up Type: Reserved
>         SKU Number: Unknown
>         Family: Unknown
> [...]
>
> and post patch:
> Handle 0x0001, DMI type 1, 27 bytes
> System Information
>         Manufacturer: raspberrypi
>         Product Name: Raspberry Pi 4 Model B Rev 1.1
>         Version: Unknown
>         Serial Number: 100000000bb24ceb
>         UUID: 30303031-3030-3030-3061-613234636435
>         Wake-up Type: Reserved
>         SKU Number: Unknown
>         Family: Unknown
> [...]
>
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
Reviewed-by: Peter Robinson <pbrobinson@gmail.com>
Tested-by: Peter Robinson <pbrobinson@gmail.com>
> ---
> Simon, I'll work with tou on the refactoring you wanted and
> remove the EFI requirement of SMBIOS in !x86 platforms.
> Since this code has no tests, I'll add some once the refactoring
> is done
>
> Changes since v2:
> - Spelling mistakes
> - rebase on top of patch #1 changes
> Changes since v1:
> - Tokenize the DT node entry and use the appropriate value instead of
>   the entire string
> - Removed Peters tested/reviewed-by tags due to the above
>  lib/smbios.c | 94 +++++++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 89 insertions(+), 5 deletions(-)
>
> diff --git a/lib/smbios.c b/lib/smbios.c
> index 444aa245a273..3f0e1d529537 100644
> --- a/lib/smbios.c
> +++ b/lib/smbios.c
> @@ -9,11 +9,14 @@
>  #include <dm.h>
>  #include <env.h>
>  #include <linux/stringify.h>
> +#include <linux/string.h>
>  #include <mapmem.h>
>  #include <smbios.h>
>  #include <sysinfo.h>
>  #include <tables_csum.h>
>  #include <version.h>
> +#include <malloc.h>
> +#include <dm/ofnode.h>
>  #ifdef CONFIG_CPU
>  #include <cpu.h>
>  #include <dm/uclass-internal.h>
> @@ -43,6 +46,25 @@
>
>  DECLARE_GLOBAL_DATA_PTR;
>
> +/**
> + * struct map_sysinfo - Mapping of sysinfo strings to DT
> + *
> + * @sysinfo_str: sysinfo string
> + * @dt_str: DT string
> + * @max: Max index of the tokenized string to pick. Counting starts from 0
> + *
> + */
> +struct map_sysinfo {
> +       const char *sysinfo_str;
> +       const char *dt_str;
> +       int max;
> +};
> +
> +static const struct map_sysinfo sysinfo_to_dt[] = {
> +       { .sysinfo_str = "product", .dt_str = "model", 2 },
> +       { .sysinfo_str = "manufacturer", .dt_str = "compatible", 1 },
> +};
> +
>  /**
>   * struct smbios_ctx - context for writing SMBIOS tables
>   *
> @@ -87,6 +109,18 @@ struct smbios_write_method {
>         const char *subnode_name;
>  };
>
> +static const struct map_sysinfo *convert_sysinfo_to_dt(const char *sysinfo_str)
> +{
> +       int i;
> +
> +       for (i = 0; i < ARRAY_SIZE(sysinfo_to_dt); i++) {
> +               if (!strcmp(sysinfo_str, sysinfo_to_dt[i].sysinfo_str))
> +                       return &sysinfo_to_dt[i];
> +       }
> +
> +       return NULL;
> +}
> +
>  /**
>   * smbios_add_string() - add a string to the string area
>   *
> @@ -124,6 +158,42 @@ static int smbios_add_string(struct smbios_ctx *ctx, const char *str)
>         }
>  }
>
> +/**
> + * get_str_from_dt - Get a substring from a DT property.
> + *                   After finding the property in the DT, the function
> + *                   will parse comma-separated values and return the value.
> + *                   If nprop->max exceeds the number of comma-separated
> + *                   elements, the last non NULL value will be returned.
> + *                   Counting starts from zero.
> + *
> + * @nprop: sysinfo property to use
> + * @str: pointer to fill with data
> + * @size: str buffer length
> + */
> +static
> +void get_str_from_dt(const struct map_sysinfo *nprop, char *str, size_t size)
> +{
> +       const char *dt_str;
> +       int cnt = 0;
> +       char *token;
> +
> +       memset(str, 0, size);
> +       if (!nprop || !nprop->max)
> +               return;
> +
> +       dt_str = ofnode_read_string(ofnode_root(), nprop->dt_str);
> +       if (!dt_str)
> +               return;
> +
> +       memcpy(str, dt_str, size);
> +       token = strtok(str, ",");
> +       while (token && cnt < nprop->max) {
> +               strlcpy(str, token, strlen(token) + 1);
> +               token = strtok(NULL, ",");
> +               cnt++;
> +       }
> +}
> +
>  /**
>   * smbios_add_prop_si() - Add a property from the devicetree or sysinfo
>   *
> @@ -137,6 +207,8 @@ static int smbios_add_string(struct smbios_ctx *ctx, const char *str)
>  static int smbios_add_prop_si(struct smbios_ctx *ctx, const char *prop,
>                               int sysinfo_id, const char *dval)
>  {
> +       int ret;
> +
>         if (!dval || !*dval)
>                 dval = "Unknown";
>
> @@ -145,18 +217,30 @@ static int smbios_add_prop_si(struct smbios_ctx *ctx, const char *prop,
>
>         if (sysinfo_id && ctx->dev) {
>                 char val[SMBIOS_STR_MAX];
> -               int ret;
>
>                 ret = sysinfo_get_str(ctx->dev, sysinfo_id, sizeof(val), val);
>                 if (!ret)
>                         return smbios_add_string(ctx, val);
>         }
>         if (IS_ENABLED(CONFIG_OF_CONTROL)) {
> -               const char *str;
> -
> -               str = ofnode_read_string(ctx->node, prop);
> +               const char *str = NULL;
> +               char str_dt[128] = { 0 };
> +               /*
> +                * If the node is not valid fallback and try the entire DT
> +                * so we can at least fill in manufacturer and board type
> +                */
> +               if (ofnode_valid(ctx->node)) {
> +                       str = ofnode_read_string(ctx->node, prop);
> +               } else {
> +                       const struct map_sysinfo *nprop;
> +
> +                       nprop = convert_sysinfo_to_dt(prop);
> +                       get_str_from_dt(nprop, str_dt, sizeof(str_dt));
> +                       str = (const char *)str_dt;
> +               }
>
> -               return smbios_add_string(ctx, str ? str : dval);
> +               ret = smbios_add_string(ctx, str && *str ? str : dval);
> +               return ret;
>         }
>
>         return 0;
> --
> 2.40.1
>

Re: [PATCH 1/2 v3] smbios: Simplify reporting of unknown values

[Simon Glass]

Hi Ilias,

On Thu, 7 Dec 2023 at 02:19, 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_prop_si() and provide an alternative string in case the
> primary is NULL or empty
>
> 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
>
> [...]
>
> 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
> [...]
>
> While at it make smbios_add_prop_si() add a string directly if the prop
> node is NULL and replace smbios_add_string() calls with
> smbios_add_prop_si(ctx, NULL, ....)
>
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> ---
> Changes since v2:
> - refactor even more code and remove the smbios_add_string calls from the
>   main code. Instead use smbios_add_prop() foir everything and add a
>   default value, in case the parsed one is NULL or emtpy
> Changes since v1:
> - None
>
>  lib/smbios.c | 73 +++++++++++++++++++++++++---------------------------
>  1 file changed, 35 insertions(+), 38 deletions(-)
>

I actually think we should just stop here and first write a test for
what we already have. I can take a crack at it if you like?

> diff --git a/lib/smbios.c b/lib/smbios.c
> index d7f4999e8b2a..444aa245a273 100644
> --- a/lib/smbios.c
> +++ b/lib/smbios.c
> @@ -102,9 +102,6 @@ static int smbios_add_string(struct smbios_ctx *ctx, const char *str)
>         int i = 1;
>         char *p = ctx->eos;
>
> -       if (!*str)
> -               str = "Unknown";
> -
>         for (;;) {
>                 if (!*p) {
>                         ctx->last_str = p;
> @@ -134,11 +131,18 @@ static int smbios_add_string(struct smbios_ctx *ctx, const char *str)
>   *
>   * @ctx:       context for writing the tables
>   * @prop:      property to write

which can now be NULL?

> + * @dval:      Default value to use if the string is not found or is empty
>   * Return:     0 if not found, else SMBIOS string number (1 or more)
>   */
>  static int smbios_add_prop_si(struct smbios_ctx *ctx, const char *prop,
> -                             int sysinfo_id)
> +                             int sysinfo_id, const char *dval)
>  {
> +       if (!dval || !*dval)
> +               dval = "Unknown";

You shouldn't need this, right? It is already the default valuie.

> +
> +       if (!prop)
> +               return smbios_add_string(ctx, dval);
> +
>         if (sysinfo_id && ctx->dev) {
>                 char val[SMBIOS_STR_MAX];
>                 int ret;
> @@ -151,8 +155,8 @@ 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 ? str : dval);
>         }
>
>         return 0;
> @@ -161,12 +165,15 @@ static int smbios_add_prop_si(struct smbios_ctx *ctx, const char *prop,
>  /**
>   * smbios_add_prop() - Add a property from the devicetree
>   *
> - * @prop:      property to write
> + * @prop:      property to write. The default string will be written if
> + *             prop is NULL
> + * @dval:      Default value to use if the string is not found or is empty
>   * Return:     0 if not found, else SMBIOS string number (1 or more)
>   */
> -static int smbios_add_prop(struct smbios_ctx *ctx, const char *prop)
> +static int smbios_add_prop(struct smbios_ctx *ctx, const char *prop,
> +                          const char *dval)
>  {
> -       return smbios_add_prop_si(ctx, prop, SYSINFO_ID_NONE);
> +       return smbios_add_prop_si(ctx, prop, SYSINFO_ID_NONE, dval);
>  }
>
>  static void smbios_set_eos(struct smbios_ctx *ctx, char *eos)
> @@ -228,11 +235,9 @@ static int smbios_write_type0(ulong *current, int handle,
>         memset(t, 0, sizeof(struct smbios_type0));
>         fill_smbios_header(t, SMBIOS_BIOS_INFORMATION, len, handle);
>         smbios_set_eos(ctx, t->eos);
> -       t->vendor = smbios_add_string(ctx, "U-Boot");
> +       t->vendor = smbios_add_prop(ctx, NULL, "U-Boot");

What is this doing exactly? I don't really understand this change.

>
> -       t->bios_ver = smbios_add_prop(ctx, "version");
> -       if (!t->bios_ver)
> -               t->bios_ver = smbios_add_string(ctx, PLAIN_VERSION);
> +       t->bios_ver = smbios_add_prop(ctx, "version", PLAIN_VERSION);
>         if (t->bios_ver)
>                 gd->smbios_version = ctx->last_str;
>         log_debug("smbios_version = %p: '%s'\n", gd->smbios_version,
> @@ -241,7 +246,7 @@ static int smbios_write_type0(ulong *current, int handle,
>         print_buffer((ulong)gd->smbios_version, gd->smbios_version,
>                      1, strlen(gd->smbios_version) + 1, 0);
>  #endif
> -       t->bios_release_date = smbios_add_string(ctx, U_BOOT_DMI_DATE);
> +       t->bios_release_date = smbios_add_prop(ctx, NULL, U_BOOT_DMI_DATE);
>  #ifdef CONFIG_ROM_SIZE
>         t->bios_rom_size = (CONFIG_ROM_SIZE / 65536) - 1;
>  #endif
> @@ -280,22 +285,19 @@ static int smbios_write_type1(ulong *current, int handle,
>         memset(t, 0, sizeof(struct smbios_type1));
>         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->manufacturer = smbios_add_prop(ctx, "manufacturer", "Unknown");
> +       t->product_name = smbios_add_prop(ctx, "product", "Unknown");
>         t->version = smbios_add_prop_si(ctx, "version",
> -                                       SYSINFO_ID_SMBIOS_SYSTEM_VERSION);
> +                                       SYSINFO_ID_SMBIOS_SYSTEM_VERSION,
> +                                       "Unknown");
>         if (serial_str) {
> -               t->serial_number = smbios_add_string(ctx, serial_str);
> +               t->serial_number = smbios_add_prop(ctx, NULL, serial_str);
>                 strncpy((char *)t->uuid, serial_str, sizeof(t->uuid));
>         } else {
> -               t->serial_number = smbios_add_prop(ctx, "serial");
> +               t->serial_number = smbios_add_prop(ctx, "serial", "Unknown");
>         }
> -       t->sku_number = smbios_add_prop(ctx, "sku");
> -       t->family = smbios_add_prop(ctx, "family");
> +       t->sku_number = smbios_add_prop(ctx, "sku", "Unknown");
> +       t->family = smbios_add_prop(ctx, "family", "Unknown");
>
>         len = t->length + smbios_string_table_len(ctx);
>         *current += len;
> @@ -314,15 +316,12 @@ static int smbios_write_type2(ulong *current, int handle,
>         memset(t, 0, sizeof(struct smbios_type2));
>         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->manufacturer = smbios_add_prop(ctx, "manufacturer", "Unknown");
> +       t->product_name = smbios_add_prop(ctx, "product", "Unknown");
>         t->version = smbios_add_prop_si(ctx, "version",
> -                                       SYSINFO_ID_SMBIOS_BASEBOARD_VERSION);
> -       t->asset_tag_number = smbios_add_prop(ctx, "asset-tag");
> +                                       SYSINFO_ID_SMBIOS_BASEBOARD_VERSION,
> +                                       "Unknown");
> +       t->asset_tag_number = smbios_add_prop(ctx, "asset-tag", "Unknown");
>         t->feature_flags = SMBIOS_BOARD_FEATURE_HOSTING;
>         t->board_type = SMBIOS_BOARD_MOTHERBOARD;
>
> @@ -343,9 +342,7 @@ static int smbios_write_type3(ulong *current, int handle,
>         memset(t, 0, sizeof(struct smbios_type3));
>         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->manufacturer = smbios_add_prop(ctx, "manufacturer", "Unknown");
>         t->chassis_type = SMBIOS_ENCLOSURE_DESKTOP;
>         t->bootup_state = SMBIOS_STATE_SAFE;
>         t->power_supply_state = SMBIOS_STATE_SAFE;
> @@ -388,8 +385,8 @@ static void smbios_write_type4_dm(struct smbios_type4 *t,
>  #endif
>
>         t->processor_family = processor_family;
> -       t->processor_manufacturer = smbios_add_string(ctx, vendor);
> -       t->processor_version = smbios_add_string(ctx, name);
> +       t->processor_manufacturer = smbios_add_prop(ctx, NULL, vendor);
> +       t->processor_version = smbios_add_prop(ctx, NULL, name);
>  }
>
>  static int smbios_write_type4(ulong *current, int handle,
> --
> 2.40.1
>

From my understanding, the only thing your fallback adds is the
manufacturer ('vendor' from DT compatible = "vendor,board"), and the
product (from DT model = "..."). This is an awful lot of code to make
that change and it seems very, confusing to me.

Instead, can you do something like:

if (!IS_ENABLED(CONFIG_SYSINFO)) {
   const char *compat = ofnode_read_string(ofnode_root(), "compatible");
   const char *model = ofnode_read_string(ofnode_root(), "model");
   const *p = strchr(compat, ',');
   char vendor[32];

  *vendor = '\0';
  if (p) {
      int len = p - compat + 1;

      strlcpy(vendor, compat, len);
  }

  // add these two strings to the ctx struct so that
smbios_write_type1() can pass them as defaults
}

You still need to make the 'default' change, but much of the other
code could go away?

Regards,
Simon

Re: [PATCH 2/2 v3] smbios: Fallback to the default DT if sysinfo nodes are missing

[Simon Glass]

Hi Ilias,

On Thu, 7 Dec 2023 at 02:19, 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>. Unfortunately the 'model' string found in DTs is
> usually lacking the manufacturer so we can't use it for both
> 'Manufacturer' and 'Product Name' SMBIOS entries reliably.
>
> So let's add a last resort to our current smbios parsing.  If none of
> the sysinfo properties are found, scan for those information in the
> root node of the device tree. Use the 'model' to fill the 'Product
> Name' and the first value of 'compatible' for the 'Manufacturer', since
> that always contains one.
>
> pre-patch:
> Handle 0x0001, DMI type 1, 27 bytes
> System Information
>         Manufacturer: Unknown
>         Product Name: Unknown
>         Version: Unknown
>         Serial Number: 100000000bb24ceb
>         UUID: 30303031-3030-3030-3061-613234636435
>         Wake-up Type: Reserved
>         SKU Number: Unknown
>         Family: Unknown
> [...]
>
> and post patch:
> Handle 0x0001, DMI type 1, 27 bytes
> System Information
>         Manufacturer: raspberrypi
>         Product Name: Raspberry Pi 4 Model B Rev 1.1
>         Version: Unknown
>         Serial Number: 100000000bb24ceb
>         UUID: 30303031-3030-3030-3061-613234636435
>         Wake-up Type: Reserved
>         SKU Number: Unknown
>         Family: Unknown
> [...]
>
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> ---
> Simon, I'll work with tou on the refactoring you wanted and
> remove the EFI requirement of SMBIOS in !x86 platforms.
> Since this code has no tests, I'll add some once the refactoring
> is done
>
> Changes since v2:
> - Spelling mistakes
> - rebase on top of patch #1 changes
> Changes since v1:
> - Tokenize the DT node entry and use the appropriate value instead of
>   the entire string
> - Removed Peters tested/reviewed-by tags due to the above
>  lib/smbios.c | 94 +++++++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 89 insertions(+), 5 deletions(-)

Can we add a Kconfig to enable this?

>
> diff --git a/lib/smbios.c b/lib/smbios.c
> index 444aa245a273..3f0e1d529537 100644
> --- a/lib/smbios.c
> +++ b/lib/smbios.c
> @@ -9,11 +9,14 @@
>  #include <dm.h>
>  #include <env.h>
>  #include <linux/stringify.h>
> +#include <linux/string.h>
>  #include <mapmem.h>
>  #include <smbios.h>
>  #include <sysinfo.h>
>  #include <tables_csum.h>
>  #include <version.h>
> +#include <malloc.h>
> +#include <dm/ofnode.h>
>  #ifdef CONFIG_CPU
>  #include <cpu.h>
>  #include <dm/uclass-internal.h>
> @@ -43,6 +46,25 @@
>
>  DECLARE_GLOBAL_DATA_PTR;
>
> +/**
> + * struct map_sysinfo - Mapping of sysinfo strings to DT
> + *
> + * @sysinfo_str: sysinfo string
> + * @dt_str: DT string
> + * @max: Max index of the tokenized string to pick. Counting starts from 0
> + *
> + */
> +struct map_sysinfo {
> +       const char *sysinfo_str;
> +       const char *dt_str;
> +       int max;
> +};
> +
> +static const struct map_sysinfo sysinfo_to_dt[] = {
> +       { .sysinfo_str = "product", .dt_str = "model", 2 },
> +       { .sysinfo_str = "manufacturer", .dt_str = "compatible", 1 },
> +};
> +
>  /**
>   * struct smbios_ctx - context for writing SMBIOS tables
>   *
> @@ -87,6 +109,18 @@ struct smbios_write_method {
>         const char *subnode_name;
>  };
>
> +static const struct map_sysinfo *convert_sysinfo_to_dt(const char *sysinfo_str)
> +{
> +       int i;
> +
> +       for (i = 0; i < ARRAY_SIZE(sysinfo_to_dt); i++) {
> +               if (!strcmp(sysinfo_str, sysinfo_to_dt[i].sysinfo_str))
> +                       return &sysinfo_to_dt[i];
> +       }
> +
> +       return NULL;
> +}
> +
>  /**
>   * smbios_add_string() - add a string to the string area
>   *
> @@ -124,6 +158,42 @@ static int smbios_add_string(struct smbios_ctx *ctx, const char *str)
>         }
>  }
>
> +/**
> + * get_str_from_dt - Get a substring from a DT property.
> + *                   After finding the property in the DT, the function
> + *                   will parse comma-separated values and return the value.
> + *                   If nprop->max exceeds the number of comma-separated
> + *                   elements, the last non NULL value will be returned.
> + *                   Counting starts from zero.
> + *
> + * @nprop: sysinfo property to use
> + * @str: pointer to fill with data
> + * @size: str buffer length

must be >0

> + */
> +static
> +void get_str_from_dt(const struct map_sysinfo *nprop, char *str, size_t size)
> +{
> +       const char *dt_str;
> +       int cnt = 0;
> +       char *token;
> +
> +       memset(str, 0, size);

*str = '\0' should be enough, assuming size is not 0

> +       if (!nprop || !nprop->max)
> +               return;
> +
> +       dt_str = ofnode_read_string(ofnode_root(), nprop->dt_str);
> +       if (!dt_str)
> +               return;
> +
> +       memcpy(str, dt_str, size);
> +       token = strtok(str, ",");
> +       while (token && cnt < nprop->max) {
> +               strlcpy(str, token, strlen(token) + 1);
> +               token = strtok(NULL, ",");
> +               cnt++;
> +       }
> +}
> +
>  /**
>   * smbios_add_prop_si() - Add a property from the devicetree or sysinfo
>   *
> @@ -137,6 +207,8 @@ static int smbios_add_string(struct smbios_ctx *ctx, const char *str)
>  static int smbios_add_prop_si(struct smbios_ctx *ctx, const char *prop,
>                               int sysinfo_id, const char *dval)
>  {
> +       int ret;
> +
>         if (!dval || !*dval)
>                 dval = "Unknown";
>
> @@ -145,18 +217,30 @@ static int smbios_add_prop_si(struct smbios_ctx *ctx, const char *prop,
>
>         if (sysinfo_id && ctx->dev) {
>                 char val[SMBIOS_STR_MAX];
> -               int ret;
>
>                 ret = sysinfo_get_str(ctx->dev, sysinfo_id, sizeof(val), val);
>                 if (!ret)
>                         return smbios_add_string(ctx, val);
>         }
>         if (IS_ENABLED(CONFIG_OF_CONTROL)) {
> -               const char *str;
> -
> -               str = ofnode_read_string(ctx->node, prop);
> +               const char *str = NULL;
> +               char str_dt[128] = { 0 };
> +               /*
> +                * If the node is not valid fallback and try the entire DT

s/and/then/ ?

> +                * so we can at least fill in manufacturer and board type
> +                */
> +               if (ofnode_valid(ctx->node)) {
> +                       str = ofnode_read_string(ctx->node, prop);
> +               } else {
> +                       const struct map_sysinfo *nprop;
> +
> +                       nprop = convert_sysinfo_to_dt(prop);
> +                       get_str_from_dt(nprop, str_dt, sizeof(str_dt));
> +                       str = (const char *)str_dt;

can't you drop the case?

> +               }
>
> -               return smbios_add_string(ctx, str ? str : dval);
> +               ret = smbios_add_string(ctx, str && *str ? str : dval);
> +               return ret;
>         }
>
>         return 0;
> --
> 2.40.1
>

Regards,
Simon

Re: [PATCH 2/2 v3] smbios: Fallback to the default DT if sysinfo nodes are missing

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 3333 bytes --]

On Wed, Dec 13, 2023 at 12:51:33PM -0700, Simon Glass wrote:
> Hi Ilias,
> 
> On Thu, 7 Dec 2023 at 02:19, 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>. Unfortunately the 'model' string found in DTs is
> > usually lacking the manufacturer so we can't use it for both
> > 'Manufacturer' and 'Product Name' SMBIOS entries reliably.
> >
> > So let's add a last resort to our current smbios parsing.  If none of
> > the sysinfo properties are found, scan for those information in the
> > root node of the device tree. Use the 'model' to fill the 'Product
> > Name' and the first value of 'compatible' for the 'Manufacturer', since
> > that always contains one.
> >
> > pre-patch:
> > Handle 0x0001, DMI type 1, 27 bytes
> > System Information
> >         Manufacturer: Unknown
> >         Product Name: Unknown
> >         Version: Unknown
> >         Serial Number: 100000000bb24ceb
> >         UUID: 30303031-3030-3030-3061-613234636435
> >         Wake-up Type: Reserved
> >         SKU Number: Unknown
> >         Family: Unknown
> > [...]
> >
> > and post patch:
> > Handle 0x0001, DMI type 1, 27 bytes
> > System Information
> >         Manufacturer: raspberrypi
> >         Product Name: Raspberry Pi 4 Model B Rev 1.1
> >         Version: Unknown
> >         Serial Number: 100000000bb24ceb
> >         UUID: 30303031-3030-3030-3061-613234636435
> >         Wake-up Type: Reserved
> >         SKU Number: Unknown
> >         Family: Unknown
> > [...]
> >
> > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > ---
> > Simon, I'll work with tou on the refactoring you wanted and
> > remove the EFI requirement of SMBIOS in !x86 platforms.
> > Since this code has no tests, I'll add some once the refactoring
> > is done
> >
> > Changes since v2:
> > - Spelling mistakes
> > - rebase on top of patch #1 changes
> > Changes since v1:
> > - Tokenize the DT node entry and use the appropriate value instead of
> >   the entire string
> > - Removed Peters tested/reviewed-by tags due to the above
> >  lib/smbios.c | 94 +++++++++++++++++++++++++++++++++++++++++++++++++---
> >  1 file changed, 89 insertions(+), 5 deletions(-)
> 
> Can we add a Kconfig to enable this?

Why? This is the default option that we want moving forward in order to
get the fields populated.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 2/2 v3] smbios: Fallback to the default DT if sysinfo nodes are missing

[Simon Glass]

Hi Tom,

On Wed, 13 Dec 2023 at 13:17, Tom Rini <trini@konsulko.com> wrote:
>
> On Wed, Dec 13, 2023 at 12:51:33PM -0700, Simon Glass wrote:
> > Hi Ilias,
> >
> > On Thu, 7 Dec 2023 at 02:19, 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>. Unfortunately the 'model' string found in DTs is
> > > usually lacking the manufacturer so we can't use it for both
> > > 'Manufacturer' and 'Product Name' SMBIOS entries reliably.
> > >
> > > So let's add a last resort to our current smbios parsing.  If none of
> > > the sysinfo properties are found, scan for those information in the
> > > root node of the device tree. Use the 'model' to fill the 'Product
> > > Name' and the first value of 'compatible' for the 'Manufacturer', since
> > > that always contains one.
> > >
> > > pre-patch:
> > > Handle 0x0001, DMI type 1, 27 bytes
> > > System Information
> > >         Manufacturer: Unknown
> > >         Product Name: Unknown
> > >         Version: Unknown
> > >         Serial Number: 100000000bb24ceb
> > >         UUID: 30303031-3030-3030-3061-613234636435
> > >         Wake-up Type: Reserved
> > >         SKU Number: Unknown
> > >         Family: Unknown
> > > [...]
> > >
> > > and post patch:
> > > Handle 0x0001, DMI type 1, 27 bytes
> > > System Information
> > >         Manufacturer: raspberrypi
> > >         Product Name: Raspberry Pi 4 Model B Rev 1.1
> > >         Version: Unknown
> > >         Serial Number: 100000000bb24ceb
> > >         UUID: 30303031-3030-3030-3061-613234636435
> > >         Wake-up Type: Reserved
> > >         SKU Number: Unknown
> > >         Family: Unknown
> > > [...]
> > >
> > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > > ---
> > > Simon, I'll work with tou on the refactoring you wanted and
> > > remove the EFI requirement of SMBIOS in !x86 platforms.
> > > Since this code has no tests, I'll add some once the refactoring
> > > is done
> > >
> > > Changes since v2:
> > > - Spelling mistakes
> > > - rebase on top of patch #1 changes
> > > Changes since v1:
> > > - Tokenize the DT node entry and use the appropriate value instead of
> > >   the entire string
> > > - Removed Peters tested/reviewed-by tags due to the above
> > >  lib/smbios.c | 94 +++++++++++++++++++++++++++++++++++++++++++++++++---
> > >  1 file changed, 89 insertions(+), 5 deletions(-)
> >
> > Can we add a Kconfig to enable this?
>
> Why? This is the default option that we want moving forward in order to
> get the fields populated.

The model name is fine. The manufacturer is in lower case (using the
DT vendor name), so not in the right format.

It only populates two fields, so far as I can tell.

Regards,
Simon

Re: [PATCH 2/2 v3] smbios: Fallback to the default DT if sysinfo nodes are missing

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 4247 bytes --]

On Wed, Dec 13, 2023 at 01:41:04PM -0700, Simon Glass wrote:
> Hi Tom,
> 
> On Wed, 13 Dec 2023 at 13:17, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Wed, Dec 13, 2023 at 12:51:33PM -0700, Simon Glass wrote:
> > > Hi Ilias,
> > >
> > > On Thu, 7 Dec 2023 at 02:19, 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>. Unfortunately the 'model' string found in DTs is
> > > > usually lacking the manufacturer so we can't use it for both
> > > > 'Manufacturer' and 'Product Name' SMBIOS entries reliably.
> > > >
> > > > So let's add a last resort to our current smbios parsing.  If none of
> > > > the sysinfo properties are found, scan for those information in the
> > > > root node of the device tree. Use the 'model' to fill the 'Product
> > > > Name' and the first value of 'compatible' for the 'Manufacturer', since
> > > > that always contains one.
> > > >
> > > > pre-patch:
> > > > Handle 0x0001, DMI type 1, 27 bytes
> > > > System Information
> > > >         Manufacturer: Unknown
> > > >         Product Name: Unknown
> > > >         Version: Unknown
> > > >         Serial Number: 100000000bb24ceb
> > > >         UUID: 30303031-3030-3030-3061-613234636435
> > > >         Wake-up Type: Reserved
> > > >         SKU Number: Unknown
> > > >         Family: Unknown
> > > > [...]
> > > >
> > > > and post patch:
> > > > Handle 0x0001, DMI type 1, 27 bytes
> > > > System Information
> > > >         Manufacturer: raspberrypi
> > > >         Product Name: Raspberry Pi 4 Model B Rev 1.1
> > > >         Version: Unknown
> > > >         Serial Number: 100000000bb24ceb
> > > >         UUID: 30303031-3030-3030-3061-613234636435
> > > >         Wake-up Type: Reserved
> > > >         SKU Number: Unknown
> > > >         Family: Unknown
> > > > [...]
> > > >
> > > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > > > ---
> > > > Simon, I'll work with tou on the refactoring you wanted and
> > > > remove the EFI requirement of SMBIOS in !x86 platforms.
> > > > Since this code has no tests, I'll add some once the refactoring
> > > > is done
> > > >
> > > > Changes since v2:
> > > > - Spelling mistakes
> > > > - rebase on top of patch #1 changes
> > > > Changes since v1:
> > > > - Tokenize the DT node entry and use the appropriate value instead of
> > > >   the entire string
> > > > - Removed Peters tested/reviewed-by tags due to the above
> > > >  lib/smbios.c | 94 +++++++++++++++++++++++++++++++++++++++++++++++++---
> > > >  1 file changed, 89 insertions(+), 5 deletions(-)
> > >
> > > Can we add a Kconfig to enable this?
> >
> > Why? This is the default option that we want moving forward in order to
> > get the fields populated.
> 
> The model name is fine. The manufacturer is in lower case (using the
> DT vendor name), so not in the right format.
> 
> It only populates two fields, so far as I can tell.

Maybe we need to turn this discussion on its head slightly. What do you
want to do to get SMBIOS fields to be widely populated with
generally-correct information? What we have been doing has seen very
little adoption so we need something else.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 1/2 v3] smbios: Simplify reporting of unknown values

[Ilias Apalodimas]

Hi Simon,

On Wed, 13 Dec 2023 at 21:52, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Ilias,
>
> On Thu, 7 Dec 2023 at 02:19, 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_prop_si() and provide an alternative string in case the
> > primary is NULL or empty
> >
> > 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
> >
> > [...]
> >
> > 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
> > [...]
> >
> > While at it make smbios_add_prop_si() add a string directly if the prop
> > node is NULL and replace smbios_add_string() calls with
> > smbios_add_prop_si(ctx, NULL, ....)
> >
> > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > ---
> > Changes since v2:
> > - refactor even more code and remove the smbios_add_string calls from the
> >   main code. Instead use smbios_add_prop() foir everything and add a
> >   default value, in case the parsed one is NULL or emtpy
> > Changes since v1:
> > - None
> >
> >  lib/smbios.c | 73 +++++++++++++++++++++++++---------------------------
> >  1 file changed, 35 insertions(+), 38 deletions(-)
> >
>
> I actually think we should just stop here and first write a test for
> what we already have. I can take a crack at it if you like?

I don't mind.  As I said in a previous email I'd rather do it after
the refactoring you had in mind. But I don't see why that should stop
the current patch from going forward.

>
> > diff --git a/lib/smbios.c b/lib/smbios.c
> > index d7f4999e8b2a..444aa245a273 100644
> > --- a/lib/smbios.c
> > +++ b/lib/smbios.c
> > @@ -102,9 +102,6 @@ static int smbios_add_string(struct smbios_ctx *ctx, const char *str)
> >         int i = 1;
> >         char *p = ctx->eos;
> >
> > -       if (!*str)
> > -               str = "Unknown";
> > -
> >         for (;;) {
> >                 if (!*p) {
> >                         ctx->last_str = p;
> > @@ -134,11 +131,18 @@ static int smbios_add_string(struct smbios_ctx *ctx, const char *str)
> >   *
> >   * @ctx:       context for writing the tables
> >   * @prop:      property to write
>
> which can now be NULL?

No, it can't because smbios_add_string can't do that. See commit 00a871d34e2f0a.
But instead of checking it here, I moved the code around and
smbios_add_string is only called from smbios_add_prop_si instead of
calling it on each failure of smbios_add_prop_si(). In that function,
we make sure the value isn't NULL, apart from the ->get_str sysinfo
calls -- none of these currently returns null though. I can move the
checking back to where it was if to shield us again dump 'get_str'
implementations, or in the future fix smbios_add_string properly, but
that's not the point of this patch.

>
> > + * @dval:      Default value to use if the string is not found or is empty
> >   * Return:     0 if not found, else SMBIOS string number (1 or more)
> >   */
> >  static int smbios_add_prop_si(struct smbios_ctx *ctx, const char *prop,
> > -                             int sysinfo_id)
> > +                             int sysinfo_id, const char *dval)
> >  {
> > +       if (!dval || !*dval)
> > +               dval = "Unknown";
>
> You shouldn't need this, right? It is already the default value.

Unless someone misuses smbios_add_prop_si() with both the prop and
dval being NULL. Also see above.

>
> > +
> > +       if (!prop)
> > +               return smbios_add_string(ctx, dval);
> > +
> >         if (sysinfo_id && ctx->dev) {
> >                 char val[SMBIOS_STR_MAX];
> >                 int ret;
> > @@ -151,8 +155,8 @@ 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 ? str : dval);
> >         }
> >
> >         return 0;
> > @@ -161,12 +165,15 @@ static int smbios_add_prop_si(struct smbios_ctx *ctx, const char *prop,
> >  /**
> >   * smbios_add_prop() - Add a property from the devicetree
> >   *
> > - * @prop:      property to write
> > + * @prop:      property to write. The default string will be written if
> > + *             prop is NULL
> > + * @dval:      Default value to use if the string is not found or is empty
> >   * Return:     0 if not found, else SMBIOS string number (1 or more)
> >   */
> > -static int smbios_add_prop(struct smbios_ctx *ctx, const char *prop)
> > +static int smbios_add_prop(struct smbios_ctx *ctx, const char *prop,
> > +                          const char *dval)
> >  {
> > -       return smbios_add_prop_si(ctx, prop, SYSINFO_ID_NONE);
> > +       return smbios_add_prop_si(ctx, prop, SYSINFO_ID_NONE, dval);
> >  }
> >
> >  static void smbios_set_eos(struct smbios_ctx *ctx, char *eos)
> > @@ -228,11 +235,9 @@ static int smbios_write_type0(ulong *current, int handle,
> >         memset(t, 0, sizeof(struct smbios_type0));
> >         fill_smbios_header(t, SMBIOS_BIOS_INFORMATION, len, handle);
> >         smbios_set_eos(ctx, t->eos);
> > -       t->vendor = smbios_add_string(ctx, "U-Boot");
> > +       t->vendor = smbios_add_prop(ctx, NULL, "U-Boot");
>
> What is this doing exactly? I don't really understand this change.

Re-read the patch description please, it's explained in the last paragraph.

>
> >

[...]

> > 2.40.1
> >
>
> From my understanding, the only thing your fallback adds is the
> manufacturer ('vendor' from DT compatible = "vendor,board"), and the
> product (from DT model = "..."). This is an awful lot of code to make
> that change and it seems very, confusing to me.

Can we please comment on the current patchset? Future history and
digging becomes a nightmare otherwise.

>
> Instead, can you do something like:
>
> if (!IS_ENABLED(CONFIG_SYSINFO)) {

That beats the purpose of the series though, we want this as a
*fallback* not disabled in the defconfig.

>    const char *compat = ofnode_read_string(ofnode_root(), "compatible");
>    const char *model = ofnode_read_string(ofnode_root(), "model");
>    const *p = strchr(compat, ',');

No. This is just a quick and dirty patch that allows you to split on
the first comma. On top of that it won't work for cases like 'model'
which, most of the times, only has a single value (which is again
explained in the patch description) and you have to add a bunch of ifs
on the code above. You could only parse compatible only, but model
tends to be way more accurate than the one added in the compatible
node. The first is a full description of the device while the latter
is just a trigger for driver probing etc.

random example
model = "Socionext Developer Box";
compatible = "socionext,developer-box", "socionext,synquacer";

>    char vendor[32];
>
>   *vendor = '\0';
>   if (p) {
>       int len = p - compat + 1;
>
>       strlcpy(vendor, compat, len);
>   }
>
>   // add these two strings to the ctx struct so that
> smbios_write_type1() can pass them as defaults
> }
>
> You still need to make the 'default' change, but much of the other
> code could go away?
>
> Regards,
> Simon

Thanks
/Ilias

Re: [PATCH 2/2 v3] smbios: Fallback to the default DT if sysinfo nodes are missing

[Ilias Apalodimas]

Hi Simon,

[...]

>
> Can we add a Kconfig to enable this?

I don't see the point, and I am not the only one.

> > + * @size: str buffer length

[...]

> > +       int cnt = 0;
> > +       char *token;
> > +
> > +       memset(str, 0, size);
>
> *str = '\0' should be enough, assuming size is not 0

I generally prefer initializing all memory, it's not that big of a burden here.

[...]

> > -               str = ofnode_read_string(ctx->node, prop);
> > +               const char *str = NULL;
> > +               char str_dt[128] = { 0 };
> > +               /*
> > +                * If the node is not valid fallback and try the entire DT
>
> s/and/then/ ?
>

Sure

> > +                * so we can at least fill in manufacturer and board type
> > +                */
> > +               if (ofnode_valid(ctx->node)) {
> > +                       str = ofnode_read_string(ctx->node, prop);
> > +               } else {
> > +                       const struct map_sysinfo *nprop;
> > +
> > +                       nprop = convert_sysinfo_to_dt(prop);
> > +                       get_str_from_dt(nprop, str_dt, sizeof(str_dt));
> > +                       str = (const char *)str_dt;
>
> can't you drop the case?

What case?

[...]

Thanks
/Ilias

Re: [PATCH 2/2 v3] smbios: Fallback to the default DT if sysinfo nodes are missing

[Simon Glass]

Hi Ilias,

On Wed, 13 Dec 2023 at 14:43, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> Hi Simon,
>
> [...]
>
> >
> > Can we add a Kconfig to enable this?
>
> I don't see the point, and I am not the only one.

The information is not curated and isn't accurate. E.g. it can end up
putting the vendor, device and version all into a single field,
instead of using the fields correctly.

>
> > > + * @size: str buffer length
>
> [...]
>
> > > +       int cnt = 0;
> > > +       char *token;
> > > +
> > > +       memset(str, 0, size);
> >
> > *str = '\0' should be enough, assuming size is not 0
>
> I generally prefer initializing all memory, it's not that big of a burden here.

OK, not a big deal, just odd. Also it is a character, so '\0' is better.

>
> [...]
>
> > > -               str = ofnode_read_string(ctx->node, prop);
> > > +               const char *str = NULL;
> > > +               char str_dt[128] = { 0 };
> > > +               /*
> > > +                * If the node is not valid fallback and try the entire DT
> >
> > s/and/then/ ?
> >
>
> Sure
>
> > > +                * so we can at least fill in manufacturer and board type
> > > +                */
> > > +               if (ofnode_valid(ctx->node)) {
> > > +                       str = ofnode_read_string(ctx->node, prop);
> > > +               } else {
> > > +                       const struct map_sysinfo *nprop;
> > > +
> > > +                       nprop = convert_sysinfo_to_dt(prop);
> > > +                       get_str_from_dt(nprop, str_dt, sizeof(str_dt));
> > > +                       str = (const char *)str_dt;
> >
> > can't you drop the case?
>
> What case?

Sorry, I meant 'cast'.

Regards,
Simon

Re: [PATCH 1/2 v3] smbios: Simplify reporting of unknown values

[Simon Glass]

Hi Ilias,

On Wed, 13 Dec 2023 at 14:37, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> Hi Simon,
>
> On Wed, 13 Dec 2023 at 21:52, Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Ilias,
> >
> > On Thu, 7 Dec 2023 at 02:19, 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_prop_si() and provide an alternative string in case the
> > > primary is NULL or empty
> > >
> > > 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
> > >
> > > [...]
> > >
> > > 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
> > > [...]
> > >
> > > While at it make smbios_add_prop_si() add a string directly if the prop
> > > node is NULL and replace smbios_add_string() calls with
> > > smbios_add_prop_si(ctx, NULL, ....)
> > >
> > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > > ---
> > > Changes since v2:
> > > - refactor even more code and remove the smbios_add_string calls from the
> > >   main code. Instead use smbios_add_prop() foir everything and add a
> > >   default value, in case the parsed one is NULL or emtpy
> > > Changes since v1:
> > > - None
> > >
> > >  lib/smbios.c | 73 +++++++++++++++++++++++++---------------------------
> > >  1 file changed, 35 insertions(+), 38 deletions(-)
> > >
> >
> > I actually think we should just stop here and first write a test for
> > what we already have. I can take a crack at it if you like?
>
> I don't mind.  As I said in a previous email I'd rather do it after
> the refactoring you had in mind. But I don't see why that should stop
> the current patch from going forward.

OK, well if this series could be simplified, I don't mind either. But
at present it is complicated and I think it really needs a test.

>
> >
> > > diff --git a/lib/smbios.c b/lib/smbios.c
> > > index d7f4999e8b2a..444aa245a273 100644
> > > --- a/lib/smbios.c
> > > +++ b/lib/smbios.c
> > > @@ -102,9 +102,6 @@ static int smbios_add_string(struct smbios_ctx *ctx, const char *str)
> > >         int i = 1;
> > >         char *p = ctx->eos;
> > >
> > > -       if (!*str)
> > > -               str = "Unknown";
> > > -
> > >         for (;;) {
> > >                 if (!*p) {
> > >                         ctx->last_str = p;
> > > @@ -134,11 +131,18 @@ static int smbios_add_string(struct smbios_ctx *ctx, const char *str)
> > >   *
> > >   * @ctx:       context for writing the tables
> > >   * @prop:      property to write
> >
> > which can now be NULL?
>
> No, it can't because smbios_add_string can't do that. See commit 00a871d34e2f0a.
> But instead of checking it here, I moved the code around and
> smbios_add_string is only called from smbios_add_prop_si instead of
> calling it on each failure of smbios_add_prop_si(). In that function,
> we make sure the value isn't NULL, apart from the ->get_str sysinfo
> calls -- none of these currently returns null though. I can move the
> checking back to where it was if to shield us again dump 'get_str'
> implementations, or in the future fix smbios_add_string properly, but
> that's not the point of this patch.

My point was that you have code to check for !prop and do something:

if (!prop)
    return smbios_add_string(ctx, dval);

Before, we needed prop (at least if OF_CONTROL), but now prop can be
NULL, so you should update the comment to mention that and explain
what it means.

>
> >
> > > + * @dval:      Default value to use if the string is not found or is empty
> > >   * Return:     0 if not found, else SMBIOS string number (1 or more)
> > >   */
> > >  static int smbios_add_prop_si(struct smbios_ctx *ctx, const char *prop,
> > > -                             int sysinfo_id)
> > > +                             int sysinfo_id, const char *dval)
> > >  {
> > > +       if (!dval || !*dval)
> > > +               dval = "Unknown";
> >
> > You shouldn't need this, right? It is already the default value.
>
> Unless someone misuses smbios_add_prop_si() with both the prop and
> dval being NULL. Also see above.

Don't allow that, then? It seems strange to have a default value for
the default value.

>
> >
> > > +
> > > +       if (!prop)
> > > +               return smbios_add_string(ctx, dval);
> > > +
> > >         if (sysinfo_id && ctx->dev) {
> > >                 char val[SMBIOS_STR_MAX];
> > >                 int ret;
> > > @@ -151,8 +155,8 @@ 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 ? str : dval);
> > >         }
> > >
> > >         return 0;
> > > @@ -161,12 +165,15 @@ static int smbios_add_prop_si(struct smbios_ctx *ctx, const char *prop,
> > >  /**
> > >   * smbios_add_prop() - Add a property from the devicetree
> > >   *
> > > - * @prop:      property to write
> > > + * @prop:      property to write. The default string will be written if
> > > + *             prop is NULL
> > > + * @dval:      Default value to use if the string is not found or is empty
> > >   * Return:     0 if not found, else SMBIOS string number (1 or more)
> > >   */
> > > -static int smbios_add_prop(struct smbios_ctx *ctx, const char *prop)
> > > +static int smbios_add_prop(struct smbios_ctx *ctx, const char *prop,
> > > +                          const char *dval)
> > >  {
> > > -       return smbios_add_prop_si(ctx, prop, SYSINFO_ID_NONE);
> > > +       return smbios_add_prop_si(ctx, prop, SYSINFO_ID_NONE, dval);
> > >  }
> > >
> > >  static void smbios_set_eos(struct smbios_ctx *ctx, char *eos)
> > > @@ -228,11 +235,9 @@ static int smbios_write_type0(ulong *current, int handle,
> > >         memset(t, 0, sizeof(struct smbios_type0));
> > >         fill_smbios_header(t, SMBIOS_BIOS_INFORMATION, len, handle);
> > >         smbios_set_eos(ctx, t->eos);
> > > -       t->vendor = smbios_add_string(ctx, "U-Boot");
> > > +       t->vendor = smbios_add_prop(ctx, NULL, "U-Boot");
> >
> > What is this doing exactly? I don't really understand this change.
>
> Re-read the patch description please, it's explained in the last paragraph.
>
> >
> > >
>
> [...]
>
> > > 2.40.1
> > >
> >
> > From my understanding, the only thing your fallback adds is the
> > manufacturer ('vendor' from DT compatible = "vendor,board"), and the
> > product (from DT model = "..."). This is an awful lot of code to make
> > that change and it seems very, confusing to me.
>
> Can we please comment on the current patchset? Future history and
> digging becomes a nightmare otherwise.
>
> >
> > Instead, can you do something like:
> >
> > if (!IS_ENABLED(CONFIG_SYSINFO)) {
>
> That beats the purpose of the series though, we want this as a
> *fallback* not disabled in the defconfig.
>
> >    const char *compat = ofnode_read_string(ofnode_root(), "compatible");
> >    const char *model = ofnode_read_string(ofnode_root(), "model");
> >    const *p = strchr(compat, ',');
>
> No. This is just a quick and dirty patch that allows you to split on
> the first comma. On top of that it won't work for cases like 'model'
> which, most of the times, only has a single value (which is again
> explained in the patch description) and you have to add a bunch of ifs
> on the code above. You could only parse compatible only, but model
> tends to be way more accurate than the one added in the compatible
> node. The first is a full description of the device while the latter
> is just a trigger for driver probing etc.
>
> random example
> model = "Socionext Developer Box";
> compatible = "socionext,developer-box", "socionext,synquacer";

But your code does the same as my fragment above, doesn't it? Only the
compatible string is split, and only the first part (before the ',')
is used.

For the model, the whole string is used, so having a function to split
the string, which doesn't have a separator in it, seems unnecessary.

I am not talking about the end result, just trying to make the code
easier to understand. It is very complex right now.


>
> >    char vendor[32];
> >
> >   *vendor = '\0';
> >   if (p) {
> >       int len = p - compat + 1;
> >
> >       strlcpy(vendor, compat, len);
> >   }
> >
> >   // add these two strings to the ctx struct so that
> > smbios_write_type1() can pass them as defaults
> > }
> >
> > You still need to make the 'default' change, but much of the other
> > code could go away?

Regards,
Simon

Re: [PATCH 2/2 v3] smbios: Fallback to the default DT if sysinfo nodes are missing

[Simon Glass]

Hi Tom,

On Wed, 13 Dec 2023 at 13:56, Tom Rini <trini@konsulko.com> wrote:
>
> On Wed, Dec 13, 2023 at 01:41:04PM -0700, Simon Glass wrote:
> > Hi Tom,
> >
> > On Wed, 13 Dec 2023 at 13:17, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Wed, Dec 13, 2023 at 12:51:33PM -0700, Simon Glass wrote:
> > > > Hi Ilias,
> > > >
> > > > On Thu, 7 Dec 2023 at 02:19, 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>. Unfortunately the 'model' string found in DTs is
> > > > > usually lacking the manufacturer so we can't use it for both
> > > > > 'Manufacturer' and 'Product Name' SMBIOS entries reliably.
> > > > >
> > > > > So let's add a last resort to our current smbios parsing.  If none of
> > > > > the sysinfo properties are found, scan for those information in the
> > > > > root node of the device tree. Use the 'model' to fill the 'Product
> > > > > Name' and the first value of 'compatible' for the 'Manufacturer', since
> > > > > that always contains one.
> > > > >
> > > > > pre-patch:
> > > > > Handle 0x0001, DMI type 1, 27 bytes
> > > > > System Information
> > > > >         Manufacturer: Unknown
> > > > >         Product Name: Unknown
> > > > >         Version: Unknown
> > > > >         Serial Number: 100000000bb24ceb
> > > > >         UUID: 30303031-3030-3030-3061-613234636435
> > > > >         Wake-up Type: Reserved
> > > > >         SKU Number: Unknown
> > > > >         Family: Unknown
> > > > > [...]
> > > > >
> > > > > and post patch:
> > > > > Handle 0x0001, DMI type 1, 27 bytes
> > > > > System Information
> > > > >         Manufacturer: raspberrypi
> > > > >         Product Name: Raspberry Pi 4 Model B Rev 1.1
> > > > >         Version: Unknown
> > > > >         Serial Number: 100000000bb24ceb
> > > > >         UUID: 30303031-3030-3030-3061-613234636435
> > > > >         Wake-up Type: Reserved
> > > > >         SKU Number: Unknown
> > > > >         Family: Unknown
> > > > > [...]
> > > > >
> > > > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > > > > ---
> > > > > Simon, I'll work with tou on the refactoring you wanted and
> > > > > remove the EFI requirement of SMBIOS in !x86 platforms.
> > > > > Since this code has no tests, I'll add some once the refactoring
> > > > > is done
> > > > >
> > > > > Changes since v2:
> > > > > - Spelling mistakes
> > > > > - rebase on top of patch #1 changes
> > > > > Changes since v1:
> > > > > - Tokenize the DT node entry and use the appropriate value instead of
> > > > >   the entire string
> > > > > - Removed Peters tested/reviewed-by tags due to the above
> > > > >  lib/smbios.c | 94 +++++++++++++++++++++++++++++++++++++++++++++++++---
> > > > >  1 file changed, 89 insertions(+), 5 deletions(-)
> > > >
> > > > Can we add a Kconfig to enable this?
> > >
> > > Why? This is the default option that we want moving forward in order to
> > > get the fields populated.
> >
> > The model name is fine. The manufacturer is in lower case (using the
> > DT vendor name), so not in the right format.
> >
> > It only populates two fields, so far as I can tell.
>
> Maybe we need to turn this discussion on its head slightly. What do you
> want to do to get SMBIOS fields to be widely populated with
> generally-correct information? What we have been doing has seen very
> little adoption so we need something else.

Well, who or what is actually using this info? Is the problem just
that it doesn't actually matter, so no one bothers? I'm just not sure.
The fact that on ARM you cannot pass it to the kernel without EFI
might be inhibiting its usefulness, too? Usefulness is the key
question for me.

Anyway, my suggestion (once we understand why we care about SMBIOS
tables) is patches like we just saw from rockpro64, perhaps even with
a few more fields filled out. This series seems like a backup for
board maintainers that don't care, which is fine, but we should at
least be able to disable it.

Regards,
Simon

Re: [PATCH 1/2 v3] smbios: Simplify reporting of unknown values

[Ilias Apalodimas]

On Thu, 14 Dec 2023 at 00:22, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Ilias,
>
> On Wed, 13 Dec 2023 at 14:37, Ilias Apalodimas
> <ilias.apalodimas@linaro.org> wrote:
> >
> > Hi Simon,
> >
> > On Wed, 13 Dec 2023 at 21:52, Simon Glass <sjg@chromium.org> wrote:
> > >
> > > Hi Ilias,
> > >
> > > On Thu, 7 Dec 2023 at 02:19, 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_prop_si() and provide an alternative string in case the
> > > > primary is NULL or empty
> > > >
> > > > 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
> > > >
> > > > [...]
> > > >
> > > > 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
> > > > [...]
> > > >
> > > > While at it make smbios_add_prop_si() add a string directly if the prop
> > > > node is NULL and replace smbios_add_string() calls with
> > > > smbios_add_prop_si(ctx, NULL, ....)
> > > >
> > > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > > > ---
> > > > Changes since v2:
> > > > - refactor even more code and remove the smbios_add_string calls from the
> > > >   main code. Instead use smbios_add_prop() foir everything and add a
> > > >   default value, in case the parsed one is NULL or emtpy
> > > > Changes since v1:
> > > > - None
> > > >
> > > >  lib/smbios.c | 73 +++++++++++++++++++++++++---------------------------
> > > >  1 file changed, 35 insertions(+), 38 deletions(-)
> > > >
> > >
> > > I actually think we should just stop here and first write a test for
> > > what we already have. I can take a crack at it if you like?
> >
> > I don't mind.  As I said in a previous email I'd rather do it after
> > the refactoring you had in mind. But I don't see why that should stop
> > the current patch from going forward.
>
> OK, well if this series could be simplified, I don't mind either. But
> at present it is complicated and I think it really needs a test.
>
> >
> > >
> > > > diff --git a/lib/smbios.c b/lib/smbios.c
> > > > index d7f4999e8b2a..444aa245a273 100644
> > > > --- a/lib/smbios.c
> > > > +++ b/lib/smbios.c
> > > > @@ -102,9 +102,6 @@ static int smbios_add_string(struct smbios_ctx *ctx, const char *str)
> > > >         int i = 1;
> > > >         char *p = ctx->eos;
> > > >
> > > > -       if (!*str)
> > > > -               str = "Unknown";
> > > > -
> > > >         for (;;) {
> > > >                 if (!*p) {
> > > >                         ctx->last_str = p;
> > > > @@ -134,11 +131,18 @@ static int smbios_add_string(struct smbios_ctx *ctx, const char *str)
> > > >   *
> > > >   * @ctx:       context for writing the tables
> > > >   * @prop:      property to write
> > >
> > > which can now be NULL?
> >
> > No, it can't because smbios_add_string can't do that. See commit 00a871d34e2f0a.
> > But instead of checking it here, I moved the code around and
> > smbios_add_string is only called from smbios_add_prop_si instead of
> > calling it on each failure of smbios_add_prop_si(). In that function,
> > we make sure the value isn't NULL, apart from the ->get_str sysinfo
> > calls -- none of these currently returns null though. I can move the
> > checking back to where it was if to shield us again dump 'get_str'
> > implementations, or in the future fix smbios_add_string properly, but
> > that's not the point of this patch.
>
> My point was that you have code to check for !prop and do something:
>
> if (!prop)
>     return smbios_add_string(ctx, dval);
>
> Before, we needed prop (at least if OF_CONTROL), but now prop can be
> NULL, so you should update the comment to mention that and explain
> what it means.

Sure

>
> >
> > >
> > > > + * @dval:      Default value to use if the string is not found or is empty
> > > >   * Return:     0 if not found, else SMBIOS string number (1 or more)
> > > >   */
> > > >  static int smbios_add_prop_si(struct smbios_ctx *ctx, const char *prop,
> > > > -                             int sysinfo_id)
> > > > +                             int sysinfo_id, const char *dval)
> > > >  {
> > > > +       if (!dval || !*dval)
> > > > +               dval = "Unknown";
> > >
> > > You shouldn't need this, right? It is already the default value.
> >
> > Unless someone misuses smbios_add_prop_si() with both the prop and
> > dval being NULL. Also see above.
>
> Don't allow that, then?

But I am not allowing it. Instead of returning an error though we
implicitly set it to unknown, which is exactly what we used to do.

>  It seems strange to have a default value for the default value.

That's how smbios_add_string works now. Since Heinrich found that
problem we should eventually fix smbios_add_string() to return and
error on NULL ptrs and empty strings . But the point of this patchset
is to clean up the random ad-hoc calls of it, not fix it (yet)

>
> >
> > >
> > > > +
> > > > +       if (!prop)
> > > > +               return smbios_add_string(ctx, dval);
> > > > +
> > > >         if (sysinfo_id && ctx->dev) {
> > > >                 char val[SMBIOS_STR_MAX];
> > > >                 int ret;
> > > > @@ -151,8 +155,8 @@ 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 ? str : dval);
> > > >         }
> > > >
> > > >         return 0;
> > > > @@ -161,12 +165,15 @@ static int smbios_add_prop_si(struct smbios_ctx *ctx, const char *prop,
> > > >  /**
> > > >   * smbios_add_prop() - Add a property from the devicetree
> > > >   *
> > > > - * @prop:      property to write
> > > > + * @prop:      property to write. The default string will be written if
> > > > + *             prop is NULL
> > > > + * @dval:      Default value to use if the string is not found or is empty
> > > >   * Return:     0 if not found, else SMBIOS string number (1 or more)
> > > >   */
> > > > -static int smbios_add_prop(struct smbios_ctx *ctx, const char *prop)
> > > > +static int smbios_add_prop(struct smbios_ctx *ctx, const char *prop,
> > > > +                          const char *dval)
> > > >  {
> > > > -       return smbios_add_prop_si(ctx, prop, SYSINFO_ID_NONE);
> > > > +       return smbios_add_prop_si(ctx, prop, SYSINFO_ID_NONE, dval);
> > > >  }
> > > >
> > > >  static void smbios_set_eos(struct smbios_ctx *ctx, char *eos)
> > > > @@ -228,11 +235,9 @@ static int smbios_write_type0(ulong *current, int handle,
> > > >         memset(t, 0, sizeof(struct smbios_type0));
> > > >         fill_smbios_header(t, SMBIOS_BIOS_INFORMATION, len, handle);
> > > >         smbios_set_eos(ctx, t->eos);
> > > > -       t->vendor = smbios_add_string(ctx, "U-Boot");
> > > > +       t->vendor = smbios_add_prop(ctx, NULL, "U-Boot");
> > >
> > > What is this doing exactly? I don't really understand this change.
> >
> > Re-read the patch description please, it's explained in the last paragraph.
> >
> > >
> > > >
> >
> > [...]
> >
> > > > 2.40.1
> > > >
> > >
> > > From my understanding, the only thing your fallback adds is the
> > > manufacturer ('vendor' from DT compatible = "vendor,board"), and the
> > > product (from DT model = "..."). This is an awful lot of code to make
> > > that change and it seems very, confusing to me.
> >
> > Can we please comment on the current patchset? Future history and
> > digging becomes a nightmare otherwise.
> >
> > >
> > > Instead, can you do something like:
> > >
> > > if (!IS_ENABLED(CONFIG_SYSINFO)) {
> >
> > That beats the purpose of the series though, we want this as a
> > *fallback* not disabled in the defconfig.
> >
> > >    const char *compat = ofnode_read_string(ofnode_root(), "compatible");
> > >    const char *model = ofnode_read_string(ofnode_root(), "model");
> > >    const *p = strchr(compat, ',');
> >
> > No. This is just a quick and dirty patch that allows you to split on
> > the first comma. On top of that it won't work for cases like 'model'
> > which, most of the times, only has a single value (which is again
> > explained in the patch description) and you have to add a bunch of ifs
> > on the code above. You could only parse compatible only, but model
> > tends to be way more accurate than the one added in the compatible
> > node. The first is a full description of the device while the latter
> > is just a trigger for driver probing etc.
> >
> > random example
> > model = "Socionext Developer Box";
> > compatible = "socionext,developer-box", "socionext,synquacer";
>
> But your code does the same as my fragment above, doesn't it? Only the
> compatible string is split, and only the first part (before the ',')
> is used.
>
> For the model, the whole string is used,
>  so having a function to split
> the string, which doesn't have a separator in it, seems unnecessary.

No, it's not. the spec says model = <manufacturer, model>. Some of the
existing DTs follow that pattern. In that case, you need to split
those as well and that code would start to look really ugly and non
extendable. NXP and Qualcomm boards are just some examples

>
> I am not talking about the end result, just trying to make the code
> easier to understand. It is very complex right now.

The function has a pretty clear comment on what it tries to do.
Also if we do what you suggest, adding chassis-id would mean that we
need another round of strchrs and ifs and who knows what else.  With
the current patch, you just need to add an entry to the array
something along the lines of
{ .sysinfo_str = "chassis", .dt_str = "chassis-type", 1}

Thanks
/Ilias
>
>
> >
> > >    char vendor[32];
> > >
> > >   *vendor = '\0';
> > >   if (p) {
> > >       int len = p - compat + 1;
> > >
> > >       strlcpy(vendor, compat, len);
> > >   }
> > >
> > >   // add these two strings to the ctx struct so that
> > > smbios_write_type1() can pass them as defaults
> > > }
> > >
> > > You still need to make the 'default' change, but much of the other
> > > code could go away?
>
> Regards,
> Simon

Re: [PATCH 2/2 v3] smbios: Fallback to the default DT if sysinfo nodes are missing

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 6343 bytes --]

On Wed, Dec 13, 2023 at 03:22:25PM -0700, Simon Glass wrote:
> Hi Tom,
> 
> On Wed, 13 Dec 2023 at 13:56, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Wed, Dec 13, 2023 at 01:41:04PM -0700, Simon Glass wrote:
> > > Hi Tom,
> > >
> > > On Wed, 13 Dec 2023 at 13:17, Tom Rini <trini@konsulko.com> wrote:
> > > >
> > > > On Wed, Dec 13, 2023 at 12:51:33PM -0700, Simon Glass wrote:
> > > > > Hi Ilias,
> > > > >
> > > > > On Thu, 7 Dec 2023 at 02:19, 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>. Unfortunately the 'model' string found in DTs is
> > > > > > usually lacking the manufacturer so we can't use it for both
> > > > > > 'Manufacturer' and 'Product Name' SMBIOS entries reliably.
> > > > > >
> > > > > > So let's add a last resort to our current smbios parsing.  If none of
> > > > > > the sysinfo properties are found, scan for those information in the
> > > > > > root node of the device tree. Use the 'model' to fill the 'Product
> > > > > > Name' and the first value of 'compatible' for the 'Manufacturer', since
> > > > > > that always contains one.
> > > > > >
> > > > > > pre-patch:
> > > > > > Handle 0x0001, DMI type 1, 27 bytes
> > > > > > System Information
> > > > > >         Manufacturer: Unknown
> > > > > >         Product Name: Unknown
> > > > > >         Version: Unknown
> > > > > >         Serial Number: 100000000bb24ceb
> > > > > >         UUID: 30303031-3030-3030-3061-613234636435
> > > > > >         Wake-up Type: Reserved
> > > > > >         SKU Number: Unknown
> > > > > >         Family: Unknown
> > > > > > [...]
> > > > > >
> > > > > > and post patch:
> > > > > > Handle 0x0001, DMI type 1, 27 bytes
> > > > > > System Information
> > > > > >         Manufacturer: raspberrypi
> > > > > >         Product Name: Raspberry Pi 4 Model B Rev 1.1
> > > > > >         Version: Unknown
> > > > > >         Serial Number: 100000000bb24ceb
> > > > > >         UUID: 30303031-3030-3030-3061-613234636435
> > > > > >         Wake-up Type: Reserved
> > > > > >         SKU Number: Unknown
> > > > > >         Family: Unknown
> > > > > > [...]
> > > > > >
> > > > > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > > > > > ---
> > > > > > Simon, I'll work with tou on the refactoring you wanted and
> > > > > > remove the EFI requirement of SMBIOS in !x86 platforms.
> > > > > > Since this code has no tests, I'll add some once the refactoring
> > > > > > is done
> > > > > >
> > > > > > Changes since v2:
> > > > > > - Spelling mistakes
> > > > > > - rebase on top of patch #1 changes
> > > > > > Changes since v1:
> > > > > > - Tokenize the DT node entry and use the appropriate value instead of
> > > > > >   the entire string
> > > > > > - Removed Peters tested/reviewed-by tags due to the above
> > > > > >  lib/smbios.c | 94 +++++++++++++++++++++++++++++++++++++++++++++++++---
> > > > > >  1 file changed, 89 insertions(+), 5 deletions(-)
> > > > >
> > > > > Can we add a Kconfig to enable this?
> > > >
> > > > Why? This is the default option that we want moving forward in order to
> > > > get the fields populated.
> > >
> > > The model name is fine. The manufacturer is in lower case (using the
> > > DT vendor name), so not in the right format.
> > >
> > > It only populates two fields, so far as I can tell.
> >
> > Maybe we need to turn this discussion on its head slightly. What do you
> > want to do to get SMBIOS fields to be widely populated with
> > generally-correct information? What we have been doing has seen very
> > little adoption so we need something else.
> 
> Well, who or what is actually using this info? Is the problem just
> that it doesn't actually matter, so no one bothers? I'm just not sure.

Users are using this information. Peter has explained that Fedora has
been carrying Ilias' original version of this patch since it was posted
so that bug reports say (something close enough and that can be tracked
down) what device they're on, rather than "Unknown" / "Unknown Product".

> The fact that on ARM you cannot pass it to the kernel without EFI
> might be inhibiting its usefulness, too? Usefulness is the key
> question for me.

I'm setting aside the discussion of how one will tell the kernel where
the SMBIOS is, outside of EFI as it's not a U-Boot problem and belongs
on other lists.

> Anyway, my suggestion (once we understand why we care about SMBIOS
> tables) is patches like we just saw from rockpro64, perhaps even with
> a few more fields filled out. This series seems like a backup for
> board maintainers that don't care, which is fine, but we should at
> least be able to disable it.

The issue is that this series adds, for free, useful and sufficiently
correct information in the common cases. The "for free" is a huge
feature too. Drawing on the rockpro64 patch, I would suggest seeing if
/version (or /hardware-rev or whatever) is something Rob is interested
in adding so it can be something filled out or not and then pulled from
DT instead of a new set of DT nodes that look a whole lot like the
existing DT nodes and properties that already exist.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 2/2 v3] smbios: Fallback to the default DT if sysinfo nodes are missing

[Mark Kettenis]

> Date: Wed, 13 Dec 2023 15:56:34 -0500
> From: Tom Rini <trini@konsulko.com>
> 
> On Wed, Dec 13, 2023 at 01:41:04PM -0700, Simon Glass wrote:
> > Hi Tom,
> > 
> > On Wed, 13 Dec 2023 at 13:17, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Wed, Dec 13, 2023 at 12:51:33PM -0700, Simon Glass wrote:
> > > > Hi Ilias,
> > > >
> > > > On Thu, 7 Dec 2023 at 02:19, 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>. Unfortunately the 'model' string found in DTs is
> > > > > usually lacking the manufacturer so we can't use it for both
> > > > > 'Manufacturer' and 'Product Name' SMBIOS entries reliably.
> > > > >
> > > > > So let's add a last resort to our current smbios parsing.  If none of
> > > > > the sysinfo properties are found, scan for those information in the
> > > > > root node of the device tree. Use the 'model' to fill the 'Product
> > > > > Name' and the first value of 'compatible' for the 'Manufacturer', since
> > > > > that always contains one.
> > > > >
> > > > > pre-patch:
> > > > > Handle 0x0001, DMI type 1, 27 bytes
> > > > > System Information
> > > > >         Manufacturer: Unknown
> > > > >         Product Name: Unknown
> > > > >         Version: Unknown
> > > > >         Serial Number: 100000000bb24ceb
> > > > >         UUID: 30303031-3030-3030-3061-613234636435
> > > > >         Wake-up Type: Reserved
> > > > >         SKU Number: Unknown
> > > > >         Family: Unknown
> > > > > [...]
> > > > >
> > > > > and post patch:
> > > > > Handle 0x0001, DMI type 1, 27 bytes
> > > > > System Information
> > > > >         Manufacturer: raspberrypi
> > > > >         Product Name: Raspberry Pi 4 Model B Rev 1.1
> > > > >         Version: Unknown
> > > > >         Serial Number: 100000000bb24ceb
> > > > >         UUID: 30303031-3030-3030-3061-613234636435
> > > > >         Wake-up Type: Reserved
> > > > >         SKU Number: Unknown
> > > > >         Family: Unknown
> > > > > [...]
> > > > >
> > > > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > > > > ---
> > > > > Simon, I'll work with tou on the refactoring you wanted and
> > > > > remove the EFI requirement of SMBIOS in !x86 platforms.
> > > > > Since this code has no tests, I'll add some once the refactoring
> > > > > is done
> > > > >
> > > > > Changes since v2:
> > > > > - Spelling mistakes
> > > > > - rebase on top of patch #1 changes
> > > > > Changes since v1:
> > > > > - Tokenize the DT node entry and use the appropriate value instead of
> > > > >   the entire string
> > > > > - Removed Peters tested/reviewed-by tags due to the above
> > > > >  lib/smbios.c | 94 +++++++++++++++++++++++++++++++++++++++++++++++++---
> > > > >  1 file changed, 89 insertions(+), 5 deletions(-)
> > > >
> > > > Can we add a Kconfig to enable this?
> > >
> > > Why? This is the default option that we want moving forward in order to
> > > get the fields populated.
> > 
> > The model name is fine. The manufacturer is in lower case (using the
> > DT vendor name), so not in the right format.
> > 
> > It only populates two fields, so far as I can tell.
> 
> Maybe we need to turn this discussion on its head slightly. What do you
> want to do to get SMBIOS fields to be widely populated with
> generally-correct information? What we have been doing has seen very
> little adoption so we need something else.

One possibility would be to have a script that maps the board
compatible to a vendor name using
Documentation/devicetree/bindings/vendor-prefixes.yaml

That would solve populating the manufacturer field, but not the
product_name unfortunately.

Re: [PATCH 2/2 v3] smbios: Fallback to the default DT if sysinfo nodes are missing

[Simon Glass]

Hi Tom,

On Wed, 13 Dec 2023 at 16:05, Tom Rini <trini@konsulko.com> wrote:
>
> On Wed, Dec 13, 2023 at 03:22:25PM -0700, Simon Glass wrote:
> > Hi Tom,
> >
> > On Wed, 13 Dec 2023 at 13:56, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Wed, Dec 13, 2023 at 01:41:04PM -0700, Simon Glass wrote:
> > > > Hi Tom,
> > > >
> > > > On Wed, 13 Dec 2023 at 13:17, Tom Rini <trini@konsulko.com> wrote:
> > > > >
> > > > > On Wed, Dec 13, 2023 at 12:51:33PM -0700, Simon Glass wrote:
> > > > > > Hi Ilias,
> > > > > >
> > > > > > On Thu, 7 Dec 2023 at 02:19, 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>. Unfortunately the 'model' string found in DTs is
> > > > > > > usually lacking the manufacturer so we can't use it for both
> > > > > > > 'Manufacturer' and 'Product Name' SMBIOS entries reliably.
> > > > > > >
> > > > > > > So let's add a last resort to our current smbios parsing.  If none of
> > > > > > > the sysinfo properties are found, scan for those information in the
> > > > > > > root node of the device tree. Use the 'model' to fill the 'Product
> > > > > > > Name' and the first value of 'compatible' for the 'Manufacturer', since
> > > > > > > that always contains one.
> > > > > > >
> > > > > > > pre-patch:
> > > > > > > Handle 0x0001, DMI type 1, 27 bytes
> > > > > > > System Information
> > > > > > >         Manufacturer: Unknown
> > > > > > >         Product Name: Unknown
> > > > > > >         Version: Unknown
> > > > > > >         Serial Number: 100000000bb24ceb
> > > > > > >         UUID: 30303031-3030-3030-3061-613234636435
> > > > > > >         Wake-up Type: Reserved
> > > > > > >         SKU Number: Unknown
> > > > > > >         Family: Unknown
> > > > > > > [...]
> > > > > > >
> > > > > > > and post patch:
> > > > > > > Handle 0x0001, DMI type 1, 27 bytes
> > > > > > > System Information
> > > > > > >         Manufacturer: raspberrypi
> > > > > > >         Product Name: Raspberry Pi 4 Model B Rev 1.1
> > > > > > >         Version: Unknown
> > > > > > >         Serial Number: 100000000bb24ceb
> > > > > > >         UUID: 30303031-3030-3030-3061-613234636435
> > > > > > >         Wake-up Type: Reserved
> > > > > > >         SKU Number: Unknown
> > > > > > >         Family: Unknown
> > > > > > > [...]
> > > > > > >
> > > > > > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > > > > > > ---
> > > > > > > Simon, I'll work with tou on the refactoring you wanted and
> > > > > > > remove the EFI requirement of SMBIOS in !x86 platforms.
> > > > > > > Since this code has no tests, I'll add some once the refactoring
> > > > > > > is done
> > > > > > >
> > > > > > > Changes since v2:
> > > > > > > - Spelling mistakes
> > > > > > > - rebase on top of patch #1 changes
> > > > > > > Changes since v1:
> > > > > > > - Tokenize the DT node entry and use the appropriate value instead of
> > > > > > >   the entire string
> > > > > > > - Removed Peters tested/reviewed-by tags due to the above
> > > > > > >  lib/smbios.c | 94 +++++++++++++++++++++++++++++++++++++++++++++++++---
> > > > > > >  1 file changed, 89 insertions(+), 5 deletions(-)
> > > > > >
> > > > > > Can we add a Kconfig to enable this?
> > > > >
> > > > > Why? This is the default option that we want moving forward in order to
> > > > > get the fields populated.
> > > >
> > > > The model name is fine. The manufacturer is in lower case (using the
> > > > DT vendor name), so not in the right format.
> > > >
> > > > It only populates two fields, so far as I can tell.
> > >
> > > Maybe we need to turn this discussion on its head slightly. What do you
> > > want to do to get SMBIOS fields to be widely populated with
> > > generally-correct information? What we have been doing has seen very
> > > little adoption so we need something else.
> >
> > Well, who or what is actually using this info? Is the problem just
> > that it doesn't actually matter, so no one bothers? I'm just not sure.
>
> Users are using this information. Peter has explained that Fedora has
> been carrying Ilias' original version of this patch since it was posted
> so that bug reports say (something close enough and that can be tracked
> down) what device they're on, rather than "Unknown" / "Unknown Product".

Unfortunately I failed to get the latest Fedora running on rpi4. Is
this the only ARM board it supports a download for? (I must be missing
something)

Anyway, so this presumably requires EFI? If so, this is the kind of
vertical integration and legacy promotion that really frustrates me.
If not, then what is the mechanism to supply SMBIOS to user space?

I believe we have been here before, too. What program is used to
collect the bug reports? Is it sos, perhaps?

>
> > The fact that on ARM you cannot pass it to the kernel without EFI
> > might be inhibiting its usefulness, too? Usefulness is the key
> > question for me.
>
> I'm setting aside the discussion of how one will tell the kernel where
> the SMBIOS is, outside of EFI as it's not a U-Boot problem and belongs
> on other lists.

OK, but in my mind this is a concern.

>
> > Anyway, my suggestion (once we understand why we care about SMBIOS
> > tables) is patches like we just saw from rockpro64, perhaps even with
> > a few more fields filled out. This series seems like a backup for
> > board maintainers that don't care, which is fine, but we should at
> > least be able to disable it.
>
> The issue is that this series adds, for free, useful and sufficiently
> correct information in the common cases. The "for free" is a huge
> feature too. Drawing on the rockpro64 patch, I would suggest seeing if
> /version (or /hardware-rev or whatever) is something Rob is interested
> in adding so it can be something filled out or not and then pulled from
> DT instead of a new set of DT nodes that look a whole lot like the
> existing DT nodes and properties that already exist.

The new DT nodes / SMBIOS binding [1] allows for the correct
information to be provided, though.

I'm fine with this as a Kconfig option, but it is a hack. It purports
to provide SMBIOS info but (for both of the two fields it fills in) it
is not in the correct format. It only works with EFI. If someone comes
along and adds the proper info for a board, the name will change in
the bug reports.

Which system-info tool is used? I see that hardinfo records the model
[2] and produces a sensible name for the compatible string [3].

Regards,
Simon

[1] https://github.com/u-boot/u-boot/blob/master/doc/device-tree-bindings/sysinfo/smbios.txt
[2] https://github.com/lpereira/hardinfo/blob/master/modules/devices.c#L547
[3] https://github.com/lpereira/hardinfo/blob/master/modules/devices/arm/processor.c#L433

Re: [PATCH 2/2 v3] smbios: Fallback to the default DT if sysinfo nodes are missing

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 435 bytes --]

On Wed, Dec 13, 2023 at 08:19:11PM -0700, Simon Glass wrote:

[snip]
> The new DT nodes / SMBIOS binding [1] allows for the correct
> information to be provided, though.
[snip]
> [1] https://github.com/u-boot/u-boot/blob/master/doc/device-tree-bindings/sysinfo/smbios.txt

I think the only feedback I can give on your message here is to please
go upstream that binding, and then we can see what to do afterwards.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 2/2 v3] smbios: Fallback to the default DT if sysinfo nodes are missing

[Simon Glass]

Hi Tom,

On Thu, 14 Dec 2023 at 06:11, Tom Rini <trini@konsulko.com> wrote:
>
> On Wed, Dec 13, 2023 at 08:19:11PM -0700, Simon Glass wrote:
>
> [snip]
> > The new DT nodes / SMBIOS binding [1] allows for the correct
> > information to be provided, though.
> [snip]
> > [1] https://github.com/u-boot/u-boot/blob/master/doc/device-tree-bindings/sysinfo/smbios.txt
>
> I think the only feedback I can give on your message here is to please
> go upstream that binding, and then we can see what to do afterwards.

I am still tearing my hair out waiting for the reserved-memory and
binman patches to be accepted. Every few weeks there is another
comment, but no action. Rob seems to be the only one engaged.

Perhaps I should do a conference talk about what is wrong with DT?
This is my experience so far:

- there is no urgency to apply anything
- no one wins acclaim for applying a patch
- everyone complains later if a patch is applied that they didn't agree with
- people chime in wanting to understand the use case, but don't/can't/won't
- any sort of expressed doubt results in delay
- maintainers hope that the submitter will lose interest and go away
- not enough people add review tags, even if they look at it
   ... because they are worried about looking bad to others on the ML

I would be happy to discuss how to improve matters, but that is what I see.

Anyway, the lowest-hanging fruit at present is the U-Boot /options
stuff. I was hoping someone else would step up to clean it up. There
should be no impediment.

Regards,
Simon

Re: [PATCH 2/2 v3] smbios: Fallback to the default DT if sysinfo nodes are missing

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 2151 bytes --]

On Sat, Dec 16, 2023 at 11:46:18AM -0700, Simon Glass wrote:
> Hi Tom,
> 
> On Thu, 14 Dec 2023 at 06:11, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Wed, Dec 13, 2023 at 08:19:11PM -0700, Simon Glass wrote:
> >
> > [snip]
> > > The new DT nodes / SMBIOS binding [1] allows for the correct
> > > information to be provided, though.
> > [snip]
> > > [1] https://github.com/u-boot/u-boot/blob/master/doc/device-tree-bindings/sysinfo/smbios.txt
> >
> > I think the only feedback I can give on your message here is to please
> > go upstream that binding, and then we can see what to do afterwards.
> 
> I am still tearing my hair out waiting for the reserved-memory and
> binman patches to be accepted. Every few weeks there is another
> comment, but no action. Rob seems to be the only one engaged.
> 
> Perhaps I should do a conference talk about what is wrong with DT?

Perhaps.

> This is my experience so far:
> 
> - there is no urgency to apply anything
> - no one wins acclaim for applying a patch
> - everyone complains later if a patch is applied that they didn't agree with
> - people chime in wanting to understand the use case, but don't/can't/won't
> - any sort of expressed doubt results in delay
> - maintainers hope that the submitter will lose interest and go away
> - not enough people add review tags, even if they look at it
>    ... because they are worried about looking bad to others on the ML
> 
> I would be happy to discuss how to improve matters, but that is what I see.
> 
> Anyway, the lowest-hanging fruit at present is the U-Boot /options
> stuff. I was hoping someone else would step up to clean it up. There
> should be no impediment.

And my point with the above is that other SoC maintainers (Neil, for
amlogic) have said (paraphrasing) he does not want to do N smbios node
patches. Which is why Ilias' patch is if not 1000% correct, it's Good
Enough and will, if it's really a problem to have all lower case
information displayed, spur people to see providing that information as
a real problem that needs to be solved. Or it will be seen as good
enough.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 2/2 v3] smbios: Fallback to the default DT if sysinfo nodes are missing

[neil.armstrong]

On 17/12/2023 19:41, Tom Rini wrote:
> On Sat, Dec 16, 2023 at 11:46:18AM -0700, Simon Glass wrote:
>> Hi Tom,
>>
>> On Thu, 14 Dec 2023 at 06:11, Tom Rini <trini@konsulko.com> wrote:
>>>
>>> On Wed, Dec 13, 2023 at 08:19:11PM -0700, Simon Glass wrote:
>>>
>>> [snip]
>>>> The new DT nodes / SMBIOS binding [1] allows for the correct
>>>> information to be provided, though.
>>> [snip]
>>>> [1] https://github.com/u-boot/u-boot/blob/master/doc/device-tree-bindings/sysinfo/smbios.txt
>>>
>>> I think the only feedback I can give on your message here is to please
>>> go upstream that binding, and then we can see what to do afterwards.
>>
>> I am still tearing my hair out waiting for the reserved-memory and
>> binman patches to be accepted. Every few weeks there is another
>> comment, but no action. Rob seems to be the only one engaged.
>>
>> Perhaps I should do a conference talk about what is wrong with DT?
> 
> Perhaps.
> 
>> This is my experience so far:
>>
>> - there is no urgency to apply anything
>> - no one wins acclaim for applying a patch
>> - everyone complains later if a patch is applied that they didn't agree with
>> - people chime in wanting to understand the use case, but don't/can't/won't
>> - any sort of expressed doubt results in delay
>> - maintainers hope that the submitter will lose interest and go away
>> - not enough people add review tags, even if they look at it
>>     ... because they are worried about looking bad to others on the ML

I agree some subsystems are not easy to deal with, but it's not the case for most of them (Qcom, Amlogic, TI, ST...),
and I think it's the case of some other Linux subsystems, and we all know it's
an issue but I do not have the power to solve that, so yes please do
a conference talk but not only about DT because it's simply not true.

>>
>> I would be happy to discuss how to improve matters, but that is what I see.
>>
>> Anyway, the lowest-hanging fruit at present is the U-Boot /options
>> stuff. I was hoping someone else would step up to clean it up. There
>> should be no impediment.
> 
> And my point with the above is that other SoC maintainers (Neil, for
> amlogic) have said (paraphrasing) he does not want to do N smbios node
> patches. Which is why Ilias' patch is if not 1000% correct, it's Good
> Enough and will, if it's really a problem to have all lower case
> information displayed, spur people to see providing that information as
> a real problem that needs to be solved. Or it will be seen as good
> enough.
> 

If some platforms requires a more "correct" smbios dataset, then they're
welcome adding the required smbios node, and it's perfectly understandable,
but for the other community-maintained platforms we need some valid fallback
data otherwise they'll be de facto excluded from some tools for no valid reasons.

Neil

Re: [PATCH 2/2 v3] smbios: Fallback to the default DT if sysinfo nodes are missing

[Simon Glass]

Hi Neil,

On Mon, 18 Dec 2023 at 02:54, <neil.armstrong@linaro.org> wrote:
>
> On 17/12/2023 19:41, Tom Rini wrote:
> > On Sat, Dec 16, 2023 at 11:46:18AM -0700, Simon Glass wrote:
> >> Hi Tom,
> >>
> >> On Thu, 14 Dec 2023 at 06:11, Tom Rini <trini@konsulko.com> wrote:
> >>>
[..]

> > And my point with the above is that other SoC maintainers (Neil, for
> > amlogic) have said (paraphrasing) he does not want to do N smbios node
> > patches. Which is why Ilias' patch is if not 1000% correct, it's Good
> > Enough and will, if it's really a problem to have all lower case
> > information displayed, spur people to see providing that information as
> > a real problem that needs to be solved. Or it will be seen as good
> > enough.
> >
>
> If some platforms requires a more "correct" smbios dataset, then they're
> welcome adding the required smbios node, and it's perfectly understandable,
> but for the other community-maintained platforms we need some valid fallback
> data otherwise they'll be de facto excluded from some tools for no valid reasons.

Do you know which tools require SMBIOS tables? I found sos and another
Redhat one.

Regards,
Simon

Re: [PATCH 1/2 v3] smbios: Simplify reporting of unknown values

[Simon Glass]

Hi Ilias,

On Wed, 13 Dec 2023 at 15:38, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> On Thu, 14 Dec 2023 at 00:22, Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Ilias,
> >
> > On Wed, 13 Dec 2023 at 14:37, Ilias Apalodimas
> > <ilias.apalodimas@linaro.org> wrote:
> > >
> > > Hi Simon,
> > >
> > > On Wed, 13 Dec 2023 at 21:52, Simon Glass <sjg@chromium.org> wrote:
> > > >
> > > > Hi Ilias,
> > > >
> > > > On Thu, 7 Dec 2023 at 02:19, 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_prop_si() and provide an alternative string in case the
> > > > > primary is NULL or empty
> > > > >
> > > > > 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
> > > > >
> > > > > [...]
> > > > >
> > > > > 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
> > > > > [...]
> > > > >
> > > > > While at it make smbios_add_prop_si() add a string directly if the prop
> > > > > node is NULL and replace smbios_add_string() calls with
> > > > > smbios_add_prop_si(ctx, NULL, ....)
> > > > >
> > > > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > > > > ---
> > > > > Changes since v2:
> > > > > - refactor even more code and remove the smbios_add_string calls from the
> > > > >   main code. Instead use smbios_add_prop() foir everything and add a
> > > > >   default value, in case the parsed one is NULL or emtpy
> > > > > Changes since v1:
> > > > > - None
> > > > >
> > > > >  lib/smbios.c | 73 +++++++++++++++++++++++++---------------------------
> > > > >  1 file changed, 35 insertions(+), 38 deletions(-)
> > > > >
> > > >
> > > > I actually think we should just stop here and first write a test for
> > > > what we already have. I can take a crack at it if you like?
> > >
> > > I don't mind.  As I said in a previous email I'd rather do it after
> > > the refactoring you had in mind. But I don't see why that should stop
> > > the current patch from going forward.
> >
> > OK, well if this series could be simplified, I don't mind either. But
> > at present it is complicated and I think it really needs a test.
> >
> > >
> > > >
> > > > > diff --git a/lib/smbios.c b/lib/smbios.c
> > > > > index d7f4999e8b2a..444aa245a273 100644
> > > > > --- a/lib/smbios.c
> > > > > +++ b/lib/smbios.c
> > > > > @@ -102,9 +102,6 @@ static int smbios_add_string(struct smbios_ctx *ctx, const char *str)
> > > > >         int i = 1;
> > > > >         char *p = ctx->eos;
> > > > >
> > > > > -       if (!*str)
> > > > > -               str = "Unknown";
> > > > > -
> > > > >         for (;;) {
> > > > >                 if (!*p) {
> > > > >                         ctx->last_str = p;
> > > > > @@ -134,11 +131,18 @@ static int smbios_add_string(struct smbios_ctx *ctx, const char *str)
> > > > >   *
> > > > >   * @ctx:       context for writing the tables
> > > > >   * @prop:      property to write
> > > >
> > > > which can now be NULL?
> > >
> > > No, it can't because smbios_add_string can't do that. See commit 00a871d34e2f0a.
> > > But instead of checking it here, I moved the code around and
> > > smbios_add_string is only called from smbios_add_prop_si instead of
> > > calling it on each failure of smbios_add_prop_si(). In that function,
> > > we make sure the value isn't NULL, apart from the ->get_str sysinfo
> > > calls -- none of these currently returns null though. I can move the
> > > checking back to where it was if to shield us again dump 'get_str'
> > > implementations, or in the future fix smbios_add_string properly, but
> > > that's not the point of this patch.
> >
> > My point was that you have code to check for !prop and do something:
> >
> > if (!prop)
> >     return smbios_add_string(ctx, dval);
> >
> > Before, we needed prop (at least if OF_CONTROL), but now prop can be
> > NULL, so you should update the comment to mention that and explain
> > what it means.
>
> Sure
>
> >
> > >
> > > >
> > > > > + * @dval:      Default value to use if the string is not found or is empty
> > > > >   * Return:     0 if not found, else SMBIOS string number (1 or more)
> > > > >   */
> > > > >  static int smbios_add_prop_si(struct smbios_ctx *ctx, const char *prop,
> > > > > -                             int sysinfo_id)
> > > > > +                             int sysinfo_id, const char *dval)
> > > > >  {
> > > > > +       if (!dval || !*dval)
> > > > > +               dval = "Unknown";
> > > >
> > > > You shouldn't need this, right? It is already the default value.
> > >
> > > Unless someone misuses smbios_add_prop_si() with both the prop and
> > > dval being NULL. Also see above.
> >
> > Don't allow that, then?
>
> But I am not allowing it. Instead of returning an error though we
> implicitly set it to unknown, which is exactly what we used to do.

Either:
- document that dval can be NULL
- don't handle it being NULL

I prefer the latter, since there are two levels of default with this code.

>
> >  It seems strange to have a default value for the default value.
>
> That's how smbios_add_string works now. Since Heinrich found that
> problem we should eventually fix smbios_add_string() to return and
> error on NULL ptrs and empty strings . But the point of this patchset
> is to clean up the random ad-hoc calls of it, not fix it (yet)

OK...

>
> >
> > >
> > > >
> > > > > +
> > > > > +       if (!prop)
> > > > > +               return smbios_add_string(ctx, dval);
> > > > > +
> > > > >         if (sysinfo_id && ctx->dev) {
> > > > >                 char val[SMBIOS_STR_MAX];
> > > > >                 int ret;
> > > > > @@ -151,8 +155,8 @@ 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 ? str : dval);
> > > > >         }
> > > > >
> > > > >         return 0;
> > > > > @@ -161,12 +165,15 @@ static int smbios_add_prop_si(struct smbios_ctx *ctx, const char *prop,
> > > > >  /**
> > > > >   * smbios_add_prop() - Add a property from the devicetree
> > > > >   *
> > > > > - * @prop:      property to write
> > > > > + * @prop:      property to write. The default string will be written if
> > > > > + *             prop is NULL
> > > > > + * @dval:      Default value to use if the string is not found or is empty
> > > > >   * Return:     0 if not found, else SMBIOS string number (1 or more)
> > > > >   */
> > > > > -static int smbios_add_prop(struct smbios_ctx *ctx, const char *prop)
> > > > > +static int smbios_add_prop(struct smbios_ctx *ctx, const char *prop,
> > > > > +                          const char *dval)
> > > > >  {
> > > > > -       return smbios_add_prop_si(ctx, prop, SYSINFO_ID_NONE);
> > > > > +       return smbios_add_prop_si(ctx, prop, SYSINFO_ID_NONE, dval);
> > > > >  }
> > > > >
> > > > >  static void smbios_set_eos(struct smbios_ctx *ctx, char *eos)
> > > > > @@ -228,11 +235,9 @@ static int smbios_write_type0(ulong *current, int handle,
> > > > >         memset(t, 0, sizeof(struct smbios_type0));
> > > > >         fill_smbios_header(t, SMBIOS_BIOS_INFORMATION, len, handle);
> > > > >         smbios_set_eos(ctx, t->eos);
> > > > > -       t->vendor = smbios_add_string(ctx, "U-Boot");
> > > > > +       t->vendor = smbios_add_prop(ctx, NULL, "U-Boot");
> > > >
> > > > What is this doing exactly? I don't really understand this change.
> > >
> > > Re-read the patch description please, it's explained in the last paragraph.
> > >
> > > >
> > > > >
> > >
> > > [...]
> > >
> > > > > 2.40.1
> > > > >
> > > >
> > > > From my understanding, the only thing your fallback adds is the
> > > > manufacturer ('vendor' from DT compatible = "vendor,board"), and the
> > > > product (from DT model = "..."). This is an awful lot of code to make
> > > > that change and it seems very, confusing to me.
> > >
> > > Can we please comment on the current patchset? Future history and
> > > digging becomes a nightmare otherwise.
> > >
> > > >
> > > > Instead, can you do something like:
> > > >
> > > > if (!IS_ENABLED(CONFIG_SYSINFO)) {
> > >
> > > That beats the purpose of the series though, we want this as a
> > > *fallback* not disabled in the defconfig.
> > >
> > > >    const char *compat = ofnode_read_string(ofnode_root(), "compatible");
> > > >    const char *model = ofnode_read_string(ofnode_root(), "model");
> > > >    const *p = strchr(compat, ',');
> > >
> > > No. This is just a quick and dirty patch that allows you to split on
> > > the first comma. On top of that it won't work for cases like 'model'
> > > which, most of the times, only has a single value (which is again
> > > explained in the patch description) and you have to add a bunch of ifs
> > > on the code above. You could only parse compatible only, but model
> > > tends to be way more accurate than the one added in the compatible
> > > node. The first is a full description of the device while the latter
> > > is just a trigger for driver probing etc.
> > >
> > > random example
> > > model = "Socionext Developer Box";
> > > compatible = "socionext,developer-box", "socionext,synquacer";
> >
> > But your code does the same as my fragment above, doesn't it? Only the
> > compatible string is split, and only the first part (before the ',')
> > is used.
> >
> > For the model, the whole string is used,
> >  so having a function to split
> > the string, which doesn't have a separator in it, seems unnecessary.
>
> No, it's not. the spec says model = <manufacturer, model>. Some of the
> existing DTs follow that pattern. In that case, you need to split
> those as well and that code would start to look really ugly and non
> extendable. NXP and Qualcomm boards are just some examples

Oh that's interesting, for example:

model = "Qualcomm Technologies, Inc. SM8550 QRD";

But I don't actually see any use of 'model' in Linux that has a
vendor,model approach. Also the existing values are clearly indented
for display to the user. I wonder if we should change the spec at this
point?

>
> >
> > I am not talking about the end result, just trying to make the code
> > easier to understand. It is very complex right now.
>
> The function has a pretty clear comment on what it tries to do.
> Also if we do what you suggest, adding chassis-id would mean that we
> need another round of strchrs and ifs and who knows what else.  With
> the current patch, you just need to add an entry to the array
> something along the lines of
> { .sysinfo_str = "chassis", .dt_str = "chassis-type", 1}

Yes I agree that it would be easier to do such a thing and now I
understand why you have done this in the code. But why would we do
that? We might as well use the proper sysinfo binding in U-Boot,
instead of inventing something like this? Or would what you propose
here be easier to upstream? I'm just not sure it makes sense, which is
why I feel the code is over-complicated (and still has no tests even
after all this work).

Regards,
Simon

Re: [PATCH 2/2 v3] smbios: Fallback to the default DT if sysinfo nodes are missing

[neil.armstrong]

Hi,

On 18/12/2023 16:01, Simon Glass wrote:
> Hi Neil,
> 
> On Mon, 18 Dec 2023 at 02:54, <neil.armstrong@linaro.org> wrote:
>>
>> On 17/12/2023 19:41, Tom Rini wrote:
>>> On Sat, Dec 16, 2023 at 11:46:18AM -0700, Simon Glass wrote:
>>>> Hi Tom,
>>>>
>>>> On Thu, 14 Dec 2023 at 06:11, Tom Rini <trini@konsulko.com> wrote:
>>>>>
> [..]
> 
>>> And my point with the above is that other SoC maintainers (Neil, for
>>> amlogic) have said (paraphrasing) he does not want to do N smbios node
>>> patches. Which is why Ilias' patch is if not 1000% correct, it's Good
>>> Enough and will, if it's really a problem to have all lower case
>>> information displayed, spur people to see providing that information as
>>> a real problem that needs to be solved. Or it will be seen as good
>>> enough.
>>>
>>
>> If some platforms requires a more "correct" smbios dataset, then they're
>> welcome adding the required smbios node, and it's perfectly understandable,
>> but for the other community-maintained platforms we need some valid fallback
>> data otherwise they'll be de facto excluded from some tools for no valid reasons.
> 
> Do you know which tools require SMBIOS tables? I found sos and another
> Redhat one.

SMBIOS data is translated into dmi informations in Linux, and a little
lookup in GitHub gives 6.4K files using something from /sys/devices/virtual/dmi/id/,
and by very commonly used tools like lshw and probably fwupd.

Neil

> 
> Regards,
> Simon

Re: [PATCH 2/2 v3] smbios: Fallback to the default DT if sysinfo nodes are missing

[Simon Glass]

Hi Neil,

On Mon, 18 Dec 2023 at 08:37, <neil.armstrong@linaro.org> wrote:
>
> Hi,
>
> On 18/12/2023 16:01, Simon Glass wrote:
> > Hi Neil,
> >
> > On Mon, 18 Dec 2023 at 02:54, <neil.armstrong@linaro.org> wrote:
> >>
> >> On 17/12/2023 19:41, Tom Rini wrote:
> >>> On Sat, Dec 16, 2023 at 11:46:18AM -0700, Simon Glass wrote:
> >>>> Hi Tom,
> >>>>
> >>>> On Thu, 14 Dec 2023 at 06:11, Tom Rini <trini@konsulko.com> wrote:
> >>>>>
> > [..]
> >
> >>> And my point with the above is that other SoC maintainers (Neil, for
> >>> amlogic) have said (paraphrasing) he does not want to do N smbios node
> >>> patches. Which is why Ilias' patch is if not 1000% correct, it's Good
> >>> Enough and will, if it's really a problem to have all lower case
> >>> information displayed, spur people to see providing that information as
> >>> a real problem that needs to be solved. Or it will be seen as good
> >>> enough.
> >>>
> >>
> >> If some platforms requires a more "correct" smbios dataset, then they're
> >> welcome adding the required smbios node, and it's perfectly understandable,
> >> but for the other community-maintained platforms we need some valid fallback
> >> data otherwise they'll be de facto excluded from some tools for no valid reasons.
> >
> > Do you know which tools require SMBIOS tables? I found sos and another
> > Redhat one.
>
> SMBIOS data is translated into dmi informations in Linux, and a little
> lookup in GitHub gives 6.4K files using something from /sys/devices/virtual/dmi/id/,
> and by very commonly used tools like lshw and probably fwupd.

lshw also uses devicetree, so should not also need SMBIOS.

fwupd uses UUIDs to indicate the device. So far as I know (and I wrote
a plugin for it, so at least know something), it does not rely on
SMBIOS tables.

Here is my main question: is SMBIOS:

1) just informational, not affecting the operation of the device
2) important and needed for the device to function

If it is (1), then I don't mind what is in the tables - we could
perhaps add a '?' at the start of each string to indicate it is
provisional?
If it is (2), then I want to avoid adding information that might be
wrong / might change over the life of the device

In either case, putting these workarounds behind a Kconfig seems
reasonable to me. What do you think?

Regards,
Simon

Re: [PATCH 2/2 v3] smbios: Fallback to the default DT if sysinfo nodes are missing

[Simon Glass]

Hi again,

On Mon, 18 Dec 2023 at 14:07, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Neil,
>
> On Mon, 18 Dec 2023 at 08:37, <neil.armstrong@linaro.org> wrote:
> >
> > Hi,
> >
> > On 18/12/2023 16:01, Simon Glass wrote:
> > > Hi Neil,
> > >
> > > On Mon, 18 Dec 2023 at 02:54, <neil.armstrong@linaro.org> wrote:
> > >>
> > >> On 17/12/2023 19:41, Tom Rini wrote:
> > >>> On Sat, Dec 16, 2023 at 11:46:18AM -0700, Simon Glass wrote:
> > >>>> Hi Tom,
> > >>>>
> > >>>> On Thu, 14 Dec 2023 at 06:11, Tom Rini <trini@konsulko.com> wrote:
> > >>>>>
> > > [..]
> > >
> > >>> And my point with the above is that other SoC maintainers (Neil, for
> > >>> amlogic) have said (paraphrasing) he does not want to do N smbios node
> > >>> patches. Which is why Ilias' patch is if not 1000% correct, it's Good
> > >>> Enough and will, if it's really a problem to have all lower case
> > >>> information displayed, spur people to see providing that information as
> > >>> a real problem that needs to be solved. Or it will be seen as good
> > >>> enough.
> > >>>
> > >>
> > >> If some platforms requires a more "correct" smbios dataset, then they're
> > >> welcome adding the required smbios node, and it's perfectly understandable,
> > >> but for the other community-maintained platforms we need some valid fallback
> > >> data otherwise they'll be de facto excluded from some tools for no valid reasons.
> > >
> > > Do you know which tools require SMBIOS tables? I found sos and another
> > > Redhat one.
> >
> > SMBIOS data is translated into dmi informations in Linux, and a little
> > lookup in GitHub gives 6.4K files using something from /sys/devices/virtual/dmi/id/,
> > and by very commonly used tools like lshw and probably fwupd.
>
> lshw also uses devicetree, so should not also need SMBIOS.
>
> fwupd uses UUIDs to indicate the device. So far as I know (and I wrote
> a plugin for it, so at least know something), it does not rely on
> SMBIOS tables.
>
> Here is my main question: is SMBIOS:
>
> 1) just informational, not affecting the operation of the device
> 2) important and needed for the device to function
>
> If it is (1), then I don't mind what is in the tables - we could
> perhaps add a '?' at the start of each string to indicate it is
> provisional?
> If it is (2), then I want to avoid adding information that might be
> wrong / might change over the life of the device
>
> In either case, putting these workarounds behind a Kconfig seems
> reasonable to me. What do you think?

Hmmm and I forgot the other problem, which is that there is no way to
pass an SMBIOS table to Linux without booting via EFI. But I don't
follow it closely, so perhaps that has been resolved?

Regards,
Simon

Re: [PATCH 2/2 v3] smbios: Fallback to the default DT if sysinfo nodes are missing

[Ilias Apalodimas]

Hi Simon,

On Mon, 18 Dec 2023 at 23:07, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Neil,
>
> On Mon, 18 Dec 2023 at 08:37, <neil.armstrong@linaro.org> wrote:
> >
> > Hi,
> >
> > On 18/12/2023 16:01, Simon Glass wrote:
> > > Hi Neil,
> > >
> > > On Mon, 18 Dec 2023 at 02:54, <neil.armstrong@linaro.org> wrote:
> > >>
> > >> On 17/12/2023 19:41, Tom Rini wrote:
> > >>> On Sat, Dec 16, 2023 at 11:46:18AM -0700, Simon Glass wrote:
> > >>>> Hi Tom,
> > >>>>
> > >>>> On Thu, 14 Dec 2023 at 06:11, Tom Rini <trini@konsulko.com> wrote:
> > >>>>>
> > > [..]
> > >
> > >>> And my point with the above is that other SoC maintainers (Neil, for
> > >>> amlogic) have said (paraphrasing) he does not want to do N smbios node
> > >>> patches. Which is why Ilias' patch is if not 1000% correct, it's Good
> > >>> Enough and will, if it's really a problem to have all lower case
> > >>> information displayed, spur people to see providing that information as
> > >>> a real problem that needs to be solved. Or it will be seen as good
> > >>> enough.
> > >>>
> > >>
> > >> If some platforms requires a more "correct" smbios dataset, then they're
> > >> welcome adding the required smbios node, and it's perfectly understandable,
> > >> but for the other community-maintained platforms we need some valid fallback
> > >> data otherwise they'll be de facto excluded from some tools for no valid reasons.
> > >
> > > Do you know which tools require SMBIOS tables? I found sos and another
> > > Redhat one.
> >
> > SMBIOS data is translated into dmi informations in Linux, and a little
> > lookup in GitHub gives 6.4K files using something from /sys/devices/virtual/dmi/id/,
> > and by very commonly used tools like lshw and probably fwupd.
>
> lshw also uses devicetree, so should not also need SMBIOS.
>
> fwupd uses UUIDs to indicate the device. So far as I know (and I wrote
> a plugin for it, so at least know something), it does not rely on
> SMBIOS tables.
>

It does use smbios tables. It also relies on them for some info for
capsule updates. Hence the fix in commit ff192304b699

> Here is my main question: is SMBIOS:
>
> 1) just informational, not affecting the operation of the device
> 2) important and needed for the device to function
>
> If it is (1), then I don't mind what is in the tables - we could
> perhaps add a '?' at the start of each string to indicate it is
> provisional?
> If it is (2), then I want to avoid adding information that might be
> wrong / might change over the life of the device
>
> In either case, putting these workarounds behind a Kconfig seems
> reasonable to me. What do you think?
>
> Regards,
> Simon

Re: [PATCH 0/2 v3] Provide a fallback to smbios tables

[Ilias Apalodimas]

Hi Heinrich,

On Mon, 18 Dec 2023 at 10:11, Heinrich Schuchardt
<heinrich.schuchardt@gmx.de> wrote:
>
> On 12/7/23 10:18, Ilias Apalodimas wrote:
> > Hi,
> >
> > This is v3 of the smbios series [0].
> > v3 has a bigger cleanup in the existing smbios code, folding in
> > smbios_add_string() that were sprinkled around  to smbios_add_prop().
> >
> > The latter is now the only caller of smbios_add_string().
> >
> > Simon asked for a selftest which makes sense, but we plan to refactor some
> > of the smbios creation code anyway. I am happy to add tests once we do that
> >
> > I've also removed the r-b tags from Peter and Tom, since some of the code
> > they reviewed changed.
>
> QEMU provides SMBIOS tables for x86, ARM, and Longsoon. I have just sent
> a patch QEMU upstream to add the missing SMBIOS support on RISC-V.
>
> Like we do for ACPI on QEMU we should not rely on the device-tree to
> build U-Boot specific SMBIOS tables but we should forward what QEMU
> provides.
>
> This can be done after the current patch series.

Fair enough. I'll add it to my backlog

Thanks
/Ilias
>
> Best regards
>
> Heinrich

Re: [PATCH 1/2 v3] smbios: Simplify reporting of unknown values

[Ilias Apalodimas]

[...]

> > > > [...]
> > > >
> > > > > > 2.40.1
> > > > > >
> > > > >
> > > > > From my understanding, the only thing your fallback adds is the
> > > > > manufacturer ('vendor' from DT compatible = "vendor,board"), and the
> > > > > product (from DT model = "..."). This is an awful lot of code to make
> > > > > that change and it seems very, confusing to me.
> > > >
> > > > Can we please comment on the current patchset? Future history and
> > > > digging becomes a nightmare otherwise.
> > > >
> > > > >
> > > > > Instead, can you do something like:
> > > > >
> > > > > if (!IS_ENABLED(CONFIG_SYSINFO)) {
> > > >
> > > > That beats the purpose of the series though, we want this as a
> > > > *fallback* not disabled in the defconfig.
> > > >
> > > > >    const char *compat = ofnode_read_string(ofnode_root(), "compatible");
> > > > >    const char *model = ofnode_read_string(ofnode_root(), "model");
> > > > >    const *p = strchr(compat, ',');
> > > >
> > > > No. This is just a quick and dirty patch that allows you to split on
> > > > the first comma. On top of that it won't work for cases like 'model'
> > > > which, most of the times, only has a single value (which is again
> > > > explained in the patch description) and you have to add a bunch of ifs
> > > > on the code above. You could only parse compatible only, but model
> > > > tends to be way more accurate than the one added in the compatible
> > > > node. The first is a full description of the device while the latter
> > > > is just a trigger for driver probing etc.
> > > >
> > > > random example
> > > > model = "Socionext Developer Box";
> > > > compatible = "socionext,developer-box", "socionext,synquacer";
> > >
> > > But your code does the same as my fragment above, doesn't it? Only the
> > > compatible string is split, and only the first part (before the ',')
> > > is used.
> > >
> > > For the model, the whole string is used,
> > >  so having a function to split
> > > the string, which doesn't have a separator in it, seems unnecessary.
> >
> > No, it's not. the spec says model = <manufacturer, model>. Some of the
> > existing DTs follow that pattern. In that case, you need to split
> > those as well and that code would start to look really ugly and non
> > extendable. NXP and Qualcomm boards are just some examples
>
> Oh that's interesting, for example:
>
> model = "Qualcomm Technologies, Inc. SM8550 QRD";
>
> But I don't actually see any use of 'model' in Linux that has a
> vendor,model approach. Also the existing values are clearly indented
> for display to the user. I wonder if we should change the spec at this
> point?
>

I don't have a strong opinion on this. I'd prefer if we left as-is and
slowly fix the existing DTs to include the manufacturer. It would be
way easier to parse those values.

> >
> > >
> > > I am not talking about the end result, just trying to make the code
> > > easier to understand. It is very complex right now.
> >
> > The function has a pretty clear comment on what it tries to do.
> > Also if we do what you suggest, adding chassis-id would mean that we
> > need another round of strchrs and ifs and who knows what else.  With
> > the current patch, you just need to add an entry to the array
> > something along the lines of
> > { .sysinfo_str = "chassis", .dt_str = "chassis-type", 1}
>
> Yes I agree that it would be easier to do such a thing and now I
> understand why you have done this in the code. But why would we do
> that? We might as well use the proper sysinfo binding in U-Boot,
> instead of inventing something like this?

This has been discussed over and over again on why we need this. I
don't see any point in repeating the arguments. FWIW the sysinfo node
on u-boot is the invention and this is re-using existing DT entries
that dont need extra work.

> Or would what you propose
> here be easier to upstream? I'm just not sure it makes sense, which is
> why I feel the code is over-complicated (and still has no tests even
> after all this work).

I don't see how a for loop and strtok are so overcomplicated.
The code *never* had any tests. In fact, it has been broken several
times and Heinrich and I are the ones to fix it. No one argues the
need for tests, I just don't see why this is a blocker.
As far as upstreaming is concerned, I would approach the problem
differently.  The model, manufacturer and chassis information are
already in the DT, so I don't see why we should touch that, apart from
amending the existing DTs with 'better' values. For the rest of the
info SMBIOS lacks (memory, cpu etc), I'd look into the existing DT
spec and add the missing bits if they can't be discovered
automatically.

Thanks
/Ilias
> Regards,
> Simon

Re: [PATCH 2/2 v3] smbios: Fallback to the default DT if sysinfo nodes are missing

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 3332 bytes --]

On Tue, Dec 19, 2023 at 09:46:22PM -0700, Simon Glass wrote:
> Hi again,
> 
> On Mon, 18 Dec 2023 at 14:07, Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Neil,
> >
> > On Mon, 18 Dec 2023 at 08:37, <neil.armstrong@linaro.org> wrote:
> > >
> > > Hi,
> > >
> > > On 18/12/2023 16:01, Simon Glass wrote:
> > > > Hi Neil,
> > > >
> > > > On Mon, 18 Dec 2023 at 02:54, <neil.armstrong@linaro.org> wrote:
> > > >>
> > > >> On 17/12/2023 19:41, Tom Rini wrote:
> > > >>> On Sat, Dec 16, 2023 at 11:46:18AM -0700, Simon Glass wrote:
> > > >>>> Hi Tom,
> > > >>>>
> > > >>>> On Thu, 14 Dec 2023 at 06:11, Tom Rini <trini@konsulko.com> wrote:
> > > >>>>>
> > > > [..]
> > > >
> > > >>> And my point with the above is that other SoC maintainers (Neil, for
> > > >>> amlogic) have said (paraphrasing) he does not want to do N smbios node
> > > >>> patches. Which is why Ilias' patch is if not 1000% correct, it's Good
> > > >>> Enough and will, if it's really a problem to have all lower case
> > > >>> information displayed, spur people to see providing that information as
> > > >>> a real problem that needs to be solved. Or it will be seen as good
> > > >>> enough.
> > > >>>
> > > >>
> > > >> If some platforms requires a more "correct" smbios dataset, then they're
> > > >> welcome adding the required smbios node, and it's perfectly understandable,
> > > >> but for the other community-maintained platforms we need some valid fallback
> > > >> data otherwise they'll be de facto excluded from some tools for no valid reasons.
> > > >
> > > > Do you know which tools require SMBIOS tables? I found sos and another
> > > > Redhat one.
> > >
> > > SMBIOS data is translated into dmi informations in Linux, and a little
> > > lookup in GitHub gives 6.4K files using something from /sys/devices/virtual/dmi/id/,
> > > and by very commonly used tools like lshw and probably fwupd.
> >
> > lshw also uses devicetree, so should not also need SMBIOS.
> >
> > fwupd uses UUIDs to indicate the device. So far as I know (and I wrote
> > a plugin for it, so at least know something), it does not rely on
> > SMBIOS tables.
> >
> > Here is my main question: is SMBIOS:
> >
> > 1) just informational, not affecting the operation of the device
> > 2) important and needed for the device to function
> >
> > If it is (1), then I don't mind what is in the tables - we could
> > perhaps add a '?' at the start of each string to indicate it is
> > provisional?
> > If it is (2), then I want to avoid adding information that might be
> > wrong / might change over the life of the device
> >
> > In either case, putting these workarounds behind a Kconfig seems
> > reasonable to me. What do you think?
> 
> Hmmm and I forgot the other problem, which is that there is no way to
> pass an SMBIOS table to Linux without booting via EFI. But I don't
> follow it closely, so perhaps that has been resolved?

This issue here is firmly NOT a U-Boot problem. It's a problem for
whomever wants to, I assume, design and upstream a DT binding to
describe how to find a populated SMBIOS table? There is IIRC some
MIPS-specific Linux Kernel option, but I imagine it wouldn't be allowed
to be added to other architectures as I assume it's magic location based
like non-EFI x86 ones?

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 2/2 v3] smbios: Fallback to the default DT if sysinfo nodes are missing

[Simon Glass]

Hi Ilias,

On Wed, 20 Dec 2023 at 00:26, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> Hi Simon,
>
> On Mon, 18 Dec 2023 at 23:07, Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Neil,
> >
> > On Mon, 18 Dec 2023 at 08:37, <neil.armstrong@linaro.org> wrote:
> > >
> > > Hi,
> > >
> > > On 18/12/2023 16:01, Simon Glass wrote:
> > > > Hi Neil,
> > > >
> > > > On Mon, 18 Dec 2023 at 02:54, <neil.armstrong@linaro.org> wrote:
> > > >>
> > > >> On 17/12/2023 19:41, Tom Rini wrote:
> > > >>> On Sat, Dec 16, 2023 at 11:46:18AM -0700, Simon Glass wrote:
> > > >>>> Hi Tom,
> > > >>>>
> > > >>>> On Thu, 14 Dec 2023 at 06:11, Tom Rini <trini@konsulko.com> wrote:
> > > >>>>>
> > > > [..]
> > > >
> > > >>> And my point with the above is that other SoC maintainers (Neil, for
> > > >>> amlogic) have said (paraphrasing) he does not want to do N smbios node
> > > >>> patches. Which is why Ilias' patch is if not 1000% correct, it's Good
> > > >>> Enough and will, if it's really a problem to have all lower case
> > > >>> information displayed, spur people to see providing that information as
> > > >>> a real problem that needs to be solved. Or it will be seen as good
> > > >>> enough.
> > > >>>
> > > >>
> > > >> If some platforms requires a more "correct" smbios dataset, then they're
> > > >> welcome adding the required smbios node, and it's perfectly understandable,
> > > >> but for the other community-maintained platforms we need some valid fallback
> > > >> data otherwise they'll be de facto excluded from some tools for no valid reasons.
> > > >
> > > > Do you know which tools require SMBIOS tables? I found sos and another
> > > > Redhat one.
> > >
> > > SMBIOS data is translated into dmi informations in Linux, and a little
> > > lookup in GitHub gives 6.4K files using something from /sys/devices/virtual/dmi/id/,
> > > and by very commonly used tools like lshw and probably fwupd.
> >
> > lshw also uses devicetree, so should not also need SMBIOS.
> >
> > fwupd uses UUIDs to indicate the device. So far as I know (and I wrote
> > a plugin for it, so at least know something), it does not rely on
> > SMBIOS tables.
> >
>
> It does use smbios tables. It also relies on them for some info for
> capsule updates. Hence the fix in commit ff192304b699

Just on that point, can you point to what is used there?

Regards,
Simon

>
> > Here is my main question: is SMBIOS:
> >
> > 1) just informational, not affecting the operation of the device
> > 2) important and needed for the device to function
> >
> > If it is (1), then I don't mind what is in the tables - we could
> > perhaps add a '?' at the start of each string to indicate it is
> > provisional?
> > If it is (2), then I want to avoid adding information that might be
> > wrong / might change over the life of the device
> >
> > In either case, putting these workarounds behind a Kconfig seems
> > reasonable to me. What do you think?
> >
> > Regards,
> > Simon

Re: [PATCH 0/2 v3] Provide a fallback to smbios tables

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1188 bytes --]

On Thu, Dec 07, 2023 at 11:18:48AM +0200, Ilias Apalodimas wrote:

> Hi,
> 
> This is v3 of the smbios series [0].
> v3 has a bigger cleanup in the existing smbios code, folding in
> smbios_add_string() that were sprinkled around  to smbios_add_prop().
> 
> The latter is now the only caller of smbios_add_string().
> 
> Simon asked for a selftest which makes sense, but we plan to refactor some
> of the smbios creation code anyway. I am happy to add tests once we do that
> 
> I've also removed the r-b tags from Peter and Tom, since some of the code
> they reviewed changed.
> 
> [0] https://lore.kernel.org/u-boot/20231127171058.165777-1-ilias.apalodimas@linaro.org/
> Ilias Apalodimas (2):
>   smbios: Simplify reporting of unknown values
>   smbios: Fallback to the default DT if sysinfo nodes are missing
> 
>  lib/smbios.c | 163 ++++++++++++++++++++++++++++++++++++++-------------
>  1 file changed, 122 insertions(+), 41 deletions(-)

With the understanding that tests will be added "soon" as well as any
reasonable cleanup of the code (along with adding features like
chassis-type being added), I've applied the series to u-boot/next now.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 2/2 v3] smbios: Fallback to the default DT if sysinfo nodes are missing

[Ilias Apalodimas]

Hi Simon,

On Wed, 20 Dec 2023 at 19:33, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Ilias,
>
> On Wed, 20 Dec 2023 at 00:26, Ilias Apalodimas
> <ilias.apalodimas@linaro.org> wrote:
> >
> > Hi Simon,
> >
> > On Mon, 18 Dec 2023 at 23:07, Simon Glass <sjg@chromium.org> wrote:
> > >
> > > Hi Neil,
> > >
> > > On Mon, 18 Dec 2023 at 08:37, <neil.armstrong@linaro.org> wrote:
> > > >
> > > > Hi,
> > > >
> > > > On 18/12/2023 16:01, Simon Glass wrote:
> > > > > Hi Neil,
> > > > >
> > > > > On Mon, 18 Dec 2023 at 02:54, <neil.armstrong@linaro.org> wrote:
> > > > >>
> > > > >> On 17/12/2023 19:41, Tom Rini wrote:
> > > > >>> On Sat, Dec 16, 2023 at 11:46:18AM -0700, Simon Glass wrote:
> > > > >>>> Hi Tom,
> > > > >>>>
> > > > >>>> On Thu, 14 Dec 2023 at 06:11, Tom Rini <trini@konsulko.com> wrote:
> > > > >>>>>
> > > > > [..]
> > > > >
> > > > >>> And my point with the above is that other SoC maintainers (Neil, for
> > > > >>> amlogic) have said (paraphrasing) he does not want to do N smbios node
> > > > >>> patches. Which is why Ilias' patch is if not 1000% correct, it's Good
> > > > >>> Enough and will, if it's really a problem to have all lower case
> > > > >>> information displayed, spur people to see providing that information as
> > > > >>> a real problem that needs to be solved. Or it will be seen as good
> > > > >>> enough.
> > > > >>>
> > > > >>
> > > > >> If some platforms requires a more "correct" smbios dataset, then they're
> > > > >> welcome adding the required smbios node, and it's perfectly understandable,
> > > > >> but for the other community-maintained platforms we need some valid fallback
> > > > >> data otherwise they'll be de facto excluded from some tools for no valid reasons.
> > > > >
> > > > > Do you know which tools require SMBIOS tables? I found sos and another
> > > > > Redhat one.
> > > >
> > > > SMBIOS data is translated into dmi informations in Linux, and a little
> > > > lookup in GitHub gives 6.4K files using something from /sys/devices/virtual/dmi/id/,
> > > > and by very commonly used tools like lshw and probably fwupd.
> > >
> > > lshw also uses devicetree, so should not also need SMBIOS.
> > >
> > > fwupd uses UUIDs to indicate the device. So far as I know (and I wrote
> > > a plugin for it, so at least know something), it does not rely on
> > > SMBIOS tables.
> > >
> >
> > It does use smbios tables. It also relies on them for some info for
> > capsule updates. Hence the fix in commit ff192304b699
>
> Just on that point, can you point to what is used there?

It's described in the commit message

Thanks
/Ilias
>
> Regards,
> Simon
>
> >
> > > Here is my main question: is SMBIOS:
> > >
> > > 1) just informational, not affecting the operation of the device
> > > 2) important and needed for the device to function
> > >
> > > If it is (1), then I don't mind what is in the tables - we could
> > > perhaps add a '?' at the start of each string to indicate it is
> > > provisional?
> > > If it is (2), then I want to avoid adding information that might be
> > > wrong / might change over the life of the device
> > >
> > > In either case, putting these workarounds behind a Kconfig seems
> > > reasonable to me. What do you think?
> > >
> > > Regards,
> > > Simon

Re: [PATCH 2/2 v3] smbios: Fallback to the default DT if sysinfo nodes are missing

[Peter Robinson]

> > > > Maybe we need to turn this discussion on its head slightly. What do you
> > > > want to do to get SMBIOS fields to be widely populated with
> > > > generally-correct information? What we have been doing has seen very
> > > > little adoption so we need something else.
> > >
> > > Well, who or what is actually using this info? Is the problem just
> > > that it doesn't actually matter, so no one bothers? I'm just not sure.
> >
> > Users are using this information. Peter has explained that Fedora has
> > been carrying Ilias' original version of this patch since it was posted
> > so that bug reports say (something close enough and that can be tracked
> > down) what device they're on, rather than "Unknown" / "Unknown Product".
>
> Unfortunately I failed to get the latest Fedora running on rpi4. Is
> this the only ARM board it supports a download for? (I must be missing
> something)

We support 100s of devices, I replied to your query there. Let's keep
that conversation on that thread.

> Anyway, so this presumably requires EFI? If so, this is the kind of
> vertical integration and legacy promotion that really frustrates me.
> If not, then what is the mechanism to supply SMBIOS to user space?

I don't know what you mean by legacy in this context, UEFI is far from
legacy and is being actively developed, it's widely used across
enterprise and numerous other verticals. It seems by legacy you mean
"Not used by ChromeOS" sure

> I believe we have been here before, too. What program is used to
> collect the bug reports? Is it sos, perhaps?

There's literally 100s across enterprise platforms, some open, some
closed. Two used in Fedora are:
sosreport: https://github.com/sosreport/sos
cockpit management: https://github.com/cockpit-project/cockpit/

But even in Fedora there's dozens more and with enterprise management
platforms I suspect there's 100s or more.

> > > The fact that on ARM you cannot pass it to the kernel without EFI
> > > might be inhibiting its usefulness, too? Usefulness is the key
> > > question for me.
> >
> > I'm setting aside the discussion of how one will tell the kernel where
> > the SMBIOS is, outside of EFI as it's not a U-Boot problem and belongs
> > on other lists.
>
> OK, but in my mind this is a concern.
>
> >
> > > Anyway, my suggestion (once we understand why we care about SMBIOS
> > > tables) is patches like we just saw from rockpro64, perhaps even with
> > > a few more fields filled out. This series seems like a backup for
> > > board maintainers that don't care, which is fine, but we should at
> > > least be able to disable it.
> >
> > The issue is that this series adds, for free, useful and sufficiently
> > correct information in the common cases. The "for free" is a huge
> > feature too. Drawing on the rockpro64 patch, I would suggest seeing if
> > /version (or /hardware-rev or whatever) is something Rob is interested
> > in adding so it can be something filled out or not and then pulled from
> > DT instead of a new set of DT nodes that look a whole lot like the
> > existing DT nodes and properties that already exist.
>
> The new DT nodes / SMBIOS binding [1] allows for the correct
> information to be provided, though.

You are completely ignoring the numerous points that myself, Tom and
others have made.

> I'm fine with this as a Kconfig option, but it is a hack. It purports
> to provide SMBIOS info but (for both of the two fields it fills in) it
> is not in the correct format. It only works with EFI. If someone comes
> along and adds the proper info for a board, the name will change in
> the bug reports.

Keep beating that drum and ignoring everyone else's opinions. This
patch set in my opinion sets two fields, I have ideas around expanding
it out to fll out more of the information. Just like DM or numerous
other ideas you have they start with initial useful things and expand
with time.

> Which system-info tool is used? I see that hardinfo records the model
> [2] and produces a sensible name for the compatible string [3].
>
> Regards,
> Simon
>
> [1] https://github.com/u-boot/u-boot/blob/master/doc/device-tree-bindings/sysinfo/smbios.txt
> [2] https://github.com/lpereira/hardinfo/blob/master/modules/devices.c#L547
> [3] https://github.com/lpereira/hardinfo/blob/master/modules/devices/arm/processor.c#L433

Re: [PATCH 2/2 v3] smbios: Fallback to the default DT if sysinfo nodes are missing

[Peter Robinson]

On Sat, Dec 16, 2023 at 6:57 PM Simon Glass <sjg@chromium.org> wrote:
>
> Hi Tom,
>
> On Thu, 14 Dec 2023 at 06:11, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Wed, Dec 13, 2023 at 08:19:11PM -0700, Simon Glass wrote:
> >
> > [snip]
> > > The new DT nodes / SMBIOS binding [1] allows for the correct
> > > information to be provided, though.
> > [snip]
> > > [1] https://github.com/u-boot/u-boot/blob/master/doc/device-tree-bindings/sysinfo/smbios.txt
> >
> > I think the only feedback I can give on your message here is to please
> > go upstream that binding, and then we can see what to do afterwards.
>
> I am still tearing my hair out waiting for the reserved-memory and
> binman patches to be accepted. Every few weeks there is another
> comment, but no action. Rob seems to be the only one engaged.
>
> Perhaps I should do a conference talk about what is wrong with DT?
> This is my experience so far:
>
> - there is no urgency to apply anything
> - no one wins acclaim for applying a patch
> - everyone complains later if a patch is applied that they didn't agree with
> - people chime in wanting to understand the use case, but don't/can't/won't
> - any sort of expressed doubt results in delay
> - maintainers hope that the submitter will lose interest and go away
> - not enough people add review tags, even if they look at it
>    ... because they are worried about looking bad to others on the ML

You miss this:
* Patch submitter ignores questions, feedback, concerns from
maintainers to the point that people mute the thread

> I would be happy to discuss how to improve matters, but that is what I see.
>
> Anyway, the lowest-hanging fruit at present is the U-Boot /options
> stuff. I was hoping someone else would step up to clean it up. There
> should be no impediment.
>
> Regards,
> Simon

Re: [PATCH 2/2 v3] smbios: Fallback to the default DT if sysinfo nodes are missing

[Simon Glass]

Hi Peter,

On Thu, Dec 21, 2023 at 9:34 AM Peter Robinson <pbrobinson@gmail.com> wrote:
>
> On Sat, Dec 16, 2023 at 6:57 PM Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Tom,
> >
> > On Thu, 14 Dec 2023 at 06:11, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Wed, Dec 13, 2023 at 08:19:11PM -0700, Simon Glass wrote:
> > >
> > > [snip]
> > > > The new DT nodes / SMBIOS binding [1] allows for the correct
> > > > information to be provided, though.
> > > [snip]
> > > > [1] https://github.com/u-boot/u-boot/blob/master/doc/device-tree-bindings/sysinfo/smbios.txt
> > >
> > > I think the only feedback I can give on your message here is to please
> > > go upstream that binding, and then we can see what to do afterwards.
> >
> > I am still tearing my hair out waiting for the reserved-memory and
> > binman patches to be accepted. Every few weeks there is another
> > comment, but no action. Rob seems to be the only one engaged.
> >
> > Perhaps I should do a conference talk about what is wrong with DT?
> > This is my experience so far:
> >
> > - there is no urgency to apply anything
> > - no one wins acclaim for applying a patch
> > - everyone complains later if a patch is applied that they didn't agree with
> > - people chime in wanting to understand the use case, but don't/can't/won't
> > - any sort of expressed doubt results in delay
> > - maintainers hope that the submitter will lose interest and go away
> > - not enough people add review tags, even if they look at it
> >    ... because they are worried about looking bad to others on the ML
>
> You miss this:
> * Patch submitter ignores questions, feedback, concerns from
> maintainers to the point that people mute the thread

Sure, I can add that. It would be great to get the other side of the
story here, from the schema maintainers.

>
> > I would be happy to discuss how to improve matters, but that is what I see.
> >
> > Anyway, the lowest-hanging fruit at present is the U-Boot /options
> > stuff. I was hoping someone else would step up to clean it up. There
> > should be no impediment.

Regards,
Simon