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

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

[Ilias Apalodimas]

Hi,
Tom asked me to clean up and resend [0].
I've made some adjustments -- The DT nodes are tokenized and a single
string is used. The DT node 'model' is used to fill the SMBIOS
'Product Name' and 'compatible' is used for 'Manufacturer'.

The reason is explained in patch#2 but the tl;dr is that model doesn't
always include the manufacturer despite the suggestions of the DT spec.

[0] https://lore.kernel.org/u-boot/20220906134426.53748-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 | 109 +++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 92 insertions(+), 17 deletions(-)

--
2.40.1

[PATCH 1/2 v2] 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_string() and always report "Unknown" regardless of the missing
field.

pre-patch dmidecode
<snip>
Handle 0x0001, DMI type 1, 27 bytes
System Information
        Manufacturer: Unknown
        Product Name: Unknown Product
        Version: Not Specified
        Serial Number: Not Specified
        UUID: Not Settable
        Wake-up Type: Reserved
        SKU Number: Not Specified
        Family: Not Specified

[...]

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

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 v1:
- None

 lib/smbios.c | 17 +++--------------
 1 file changed, 3 insertions(+), 14 deletions(-)

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

-	if (!*str)
+	if (!str || !*str)
 		str = "Unknown";

 	for (;;) {
@@ -151,8 +151,7 @@ static int smbios_add_prop_si(struct smbios_ctx *ctx, const char *prop,
 		const char *str;

 		str = ofnode_read_string(ctx->node, prop);
-		if (str)
-			return smbios_add_string(ctx, str);
+		return smbios_add_string(ctx, str);
 	}

 	return 0;
@@ -231,7 +230,7 @@ static int smbios_write_type0(ulong *current, int handle,
 	t->vendor = smbios_add_string(ctx, "U-Boot");

 	t->bios_ver = smbios_add_prop(ctx, "version");
-	if (!t->bios_ver)
+	if (!strcmp(ctx->last_str, "Unknown"))
 		t->bios_ver = smbios_add_string(ctx, PLAIN_VERSION);
 	if (t->bios_ver)
 		gd->smbios_version = ctx->last_str;
@@ -281,11 +280,7 @@ static int smbios_write_type1(ulong *current, int handle,
 	fill_smbios_header(t, SMBIOS_SYSTEM_INFORMATION, len, handle);
 	smbios_set_eos(ctx, t->eos);
 	t->manufacturer = smbios_add_prop(ctx, "manufacturer");
-	if (!t->manufacturer)
-		t->manufacturer = smbios_add_string(ctx, "Unknown");
 	t->product_name = smbios_add_prop(ctx, "product");
-	if (!t->product_name)
-		t->product_name = smbios_add_string(ctx, "Unknown Product");
 	t->version = smbios_add_prop_si(ctx, "version",
 					SYSINFO_ID_SMBIOS_SYSTEM_VERSION);
 	if (serial_str) {
@@ -315,11 +310,7 @@ static int smbios_write_type2(ulong *current, int handle,
 	fill_smbios_header(t, SMBIOS_BOARD_INFORMATION, len, handle);
 	smbios_set_eos(ctx, t->eos);
 	t->manufacturer = smbios_add_prop(ctx, "manufacturer");
-	if (!t->manufacturer)
-		t->manufacturer = smbios_add_string(ctx, "Unknown");
 	t->product_name = smbios_add_prop(ctx, "product");
-	if (!t->product_name)
-		t->product_name = smbios_add_string(ctx, "Unknown Product");
 	t->version = smbios_add_prop_si(ctx, "version",
 					SYSINFO_ID_SMBIOS_BASEBOARD_VERSION);
 	t->asset_tag_number = smbios_add_prop(ctx, "asset-tag");
@@ -344,8 +335,6 @@ static int smbios_write_type3(ulong *current, int handle,
 	fill_smbios_header(t, SMBIOS_SYSTEM_ENCLOSURE, len, handle);
 	smbios_set_eos(ctx, t->eos);
 	t->manufacturer = smbios_add_prop(ctx, "manufacturer");
-	if (!t->manufacturer)
-		t->manufacturer = smbios_add_string(ctx, "Unknown");
 	t->chassis_type = SMBIOS_ENCLOSURE_DESKTOP;
 	t->bootup_state = SMBIOS_STATE_SAFE;
 	t->power_supply_state = SMBIOS_STATE_SAFE;
--
2.40.1

[PATCH 2/2 v2] 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>
---
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, 90 insertions(+), 4 deletions(-)

diff --git a/lib/smbios.c b/lib/smbios.c
index fcc8686993ef..423893639ff0 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
  *
@@ -127,6 +161,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 separted 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
  *
@@ -139,19 +209,35 @@ 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)
 {
+	int ret = 0;
+
 	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;
+		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 maufacturer 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 = ofnode_read_string(ctx->node, prop);
-		return smbios_add_string(ctx, str);
+		ret = smbios_add_string(ctx, ofnode_valid(ctx->node) ?
+					str : (const char *)str_dt);
+		return ret;
 	}

 	return 0;
--
2.40.1

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

[Tom Rini]

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

On Mon, Nov 27, 2023 at 07:10:57PM +0200, Ilias Apalodimas wrote:

> If a value is not valid during the DT or SYSINFO parsing,  we explicitly
> set that to "Unknown Product" and "Unknown" for the product and
> manufacturer respectively.  It's cleaner if we move the checks insisde
> smbios_add_string() and always report "Unknown" regardless of the missing
> field.
> 
> pre-patch dmidecode
> <snip>
> Handle 0x0001, DMI type 1, 27 bytes
> System Information
>         Manufacturer: Unknown
>         Product Name: Unknown Product
>         Version: Not Specified
>         Serial Number: Not Specified
>         UUID: Not Settable
>         Wake-up Type: Reserved
>         SKU Number: Not Specified
>         Family: Not Specified
> 
> [...]
> 
> 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
> [...]
> 
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> Reviewed-by: Peter Robinson <pbrobinson@gmail.com>
> Tested-by: Peter Robinson <pbrobinson@gmail.com>

Reviewed-by: Tom Rini <trini@konsulko.com>

-- 
Tom

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

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

[Simon Glass]

Hi Ilias,

On Mon, 27 Nov 2023 at 10:11, 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>
> ---
> 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, 90 insertions(+), 4 deletions(-)
>

Can this be put behind a Kconfig? It adds quite a bit of code which
punishes those boards which do the right thing.


> diff --git a/lib/smbios.c b/lib/smbios.c
> index fcc8686993ef..423893639ff0 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
>   *
> @@ -127,6 +161,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 separted values and return the value.

spelling

> + *                   If nprop->max exceeds the number of comma separated

comma-separated

> + *                   elements the last non NULL value will be returned.

elements, the

> + *                   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);

Could this use ofnode_read_string_index() ?

> +       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
>   *
> @@ -139,19 +209,35 @@ 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)
>  {
> +       int ret = 0;
> +
>         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;
> +               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 maufacturer and board type

spelling

> +                */
> +               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 = ofnode_read_string(ctx->node, prop);
> -               return smbios_add_string(ctx, str);
> +               ret = smbios_add_string(ctx, ofnode_valid(ctx->node) ?
> +                                       str : (const char *)str_dt);
> +               return ret;
>         }
>
>         return 0;
> --
> 2.40.1
>

Any chance of a test for this code?

Regards,
Simon

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

[Simon Glass]

Hi Ilias,

On Mon, 27 Nov 2023 at 10:11, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> If a value is not valid during the DT or SYSINFO parsing,  we explicitly
> set that to "Unknown Product" and "Unknown" for the product and
> manufacturer respectively.  It's cleaner if we move the checks insisde
> smbios_add_string() and always report "Unknown" regardless of the missing
> field.
>
> pre-patch dmidecode
> <snip>
> Handle 0x0001, DMI type 1, 27 bytes
> System Information
>         Manufacturer: Unknown
>         Product Name: Unknown Product
>         Version: Not Specified
>         Serial Number: Not Specified
>         UUID: Not Settable
>         Wake-up Type: Reserved
>         SKU Number: Not Specified
>         Family: Not Specified
>
> [...]
>
> 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
> [...]
>
> 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 v1:
> - None
>
>  lib/smbios.c | 17 +++--------------
>  1 file changed, 3 insertions(+), 14 deletions(-)
>
> diff --git a/lib/smbios.c b/lib/smbios.c
> index d7f4999e8b2a..fcc8686993ef 100644
> --- a/lib/smbios.c
> +++ b/lib/smbios.c
> @@ -102,7 +102,7 @@ static int smbios_add_string(struct smbios_ctx *ctx, const char *str)

I suggest that this function have an additional str property
indicating the default string. Then you can pass DEFAULT_VAL or
something like that, to each call.

>         int i = 1;
>         char *p = ctx->eos;
>
> -       if (!*str)
> +       if (!str || !*str)

Does it ever happen that the string is present but empty?

>                 str = "Unknown";
>
>         for (;;) {
> @@ -151,8 +151,7 @@ static int smbios_add_prop_si(struct smbios_ctx *ctx, const char *prop,
>                 const char *str;
>
>                 str = ofnode_read_string(ctx->node, prop);
> -               if (str)
> -                       return smbios_add_string(ctx, str);
> +               return smbios_add_string(ctx, str);
>         }
>
>         return 0;
> @@ -231,7 +230,7 @@ static int smbios_write_type0(ulong *current, int handle,
>         t->vendor = smbios_add_string(ctx, "U-Boot");
>
>         t->bios_ver = smbios_add_prop(ctx, "version");
> -       if (!t->bios_ver)
> +       if (!strcmp(ctx->last_str, "Unknown"))

That is really ugly...checking the ctx value looking for a side effect.

Can you not have smbios_add_prop() continue to return NULL in this case?

>                 t->bios_ver = smbios_add_string(ctx, PLAIN_VERSION);
>         if (t->bios_ver)
>                 gd->smbios_version = ctx->last_str;
> @@ -281,11 +280,7 @@ static int smbios_write_type1(ulong *current, int handle,
>         fill_smbios_header(t, SMBIOS_SYSTEM_INFORMATION, len, handle);
>         smbios_set_eos(ctx, t->eos);
>         t->manufacturer = smbios_add_prop(ctx, "manufacturer");
> -       if (!t->manufacturer)
> -               t->manufacturer = smbios_add_string(ctx, "Unknown");
>         t->product_name = smbios_add_prop(ctx, "product");
> -       if (!t->product_name)
> -               t->product_name = smbios_add_string(ctx, "Unknown Product");
>         t->version = smbios_add_prop_si(ctx, "version",
>                                         SYSINFO_ID_SMBIOS_SYSTEM_VERSION);
>         if (serial_str) {
> @@ -315,11 +310,7 @@ static int smbios_write_type2(ulong *current, int handle,
>         fill_smbios_header(t, SMBIOS_BOARD_INFORMATION, len, handle);
>         smbios_set_eos(ctx, t->eos);
>         t->manufacturer = smbios_add_prop(ctx, "manufacturer");
> -       if (!t->manufacturer)
> -               t->manufacturer = smbios_add_string(ctx, "Unknown");
>         t->product_name = smbios_add_prop(ctx, "product");
> -       if (!t->product_name)
> -               t->product_name = smbios_add_string(ctx, "Unknown Product");
>         t->version = smbios_add_prop_si(ctx, "version",
>                                         SYSINFO_ID_SMBIOS_BASEBOARD_VERSION);
>         t->asset_tag_number = smbios_add_prop(ctx, "asset-tag");
> @@ -344,8 +335,6 @@ static int smbios_write_type3(ulong *current, int handle,
>         fill_smbios_header(t, SMBIOS_SYSTEM_ENCLOSURE, len, handle);
>         smbios_set_eos(ctx, t->eos);
>         t->manufacturer = smbios_add_prop(ctx, "manufacturer");
> -       if (!t->manufacturer)
> -               t->manufacturer = smbios_add_string(ctx, "Unknown");
>         t->chassis_type = SMBIOS_ENCLOSURE_DESKTOP;
>         t->bootup_state = SMBIOS_STATE_SAFE;
>         t->power_supply_state = SMBIOS_STATE_SAFE;
> --
> 2.40.1
>

Regards,
Simon

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

[Ilias Apalodimas]

Hi Simon,


On Thu, 30 Nov 2023 at 04:46, Simon Glass <sjg@chromium.org> wrote:

> Hi Ilias,
>
> On Mon, 27 Nov 2023 at 10:11, Ilias Apalodimas
> <ilias.apalodimas@linaro.org> wrote:
> >
> > If a value is not valid during the DT or SYSINFO parsing,  we explicitly
> > set that to "Unknown Product" and "Unknown" for the product and
> > manufacturer respectively.  It's cleaner if we move the checks insisde
> > smbios_add_string() and always report "Unknown" regardless of the missing
> > field.
> >
> > pre-patch dmidecode
> > <snip>
> > Handle 0x0001, DMI type 1, 27 bytes
> > System Information
> >         Manufacturer: Unknown
> >         Product Name: Unknown Product
> >         Version: Not Specified
> >         Serial Number: Not Specified
> >         UUID: Not Settable
> >         Wake-up Type: Reserved
> >         SKU Number: Not Specified
> >         Family: Not Specified
> >
> > [...]
> >
> > 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
> > [...]
> >
> > 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 v1:
> > - None
> >
> >  lib/smbios.c | 17 +++--------------
> >  1 file changed, 3 insertions(+), 14 deletions(-)
> >
> > diff --git a/lib/smbios.c b/lib/smbios.c
> > index d7f4999e8b2a..fcc8686993ef 100644
> > --- a/lib/smbios.c
> > +++ b/lib/smbios.c
> > @@ -102,7 +102,7 @@ static int smbios_add_string(struct smbios_ctx *ctx,
> const char *str)
>
> I suggest that this function have an additional str property
> indicating the default string. Then you can pass DEFAULT_VAL or
> something like that, to each call.
>

Why? Do you think we need to fill in something different that "unknown"?


> >         int i = 1;
> >         char *p = ctx->eos;
> >
> > -       if (!*str)
> > +       if (!str || !*str)
>
> Does it ever happen that the string is present but empty?
>

With the changes on this patchset yes.


> >                 str = "Unknown";
> >
> >         for (;;) {
> > @@ -151,8 +151,7 @@ static int smbios_add_prop_si(struct smbios_ctx
> *ctx, const char *prop,
> >                 const char *str;
> >
> >                 str = ofnode_read_string(ctx->node, prop);
> > -               if (str)
> > -                       return smbios_add_string(ctx, str);
> > +               return smbios_add_string(ctx, str);
> >         }
> >
> >         return 0;
> > @@ -231,7 +230,7 @@ static int smbios_write_type0(ulong *current, int
> handle,
> >         t->vendor = smbios_add_string(ctx, "U-Boot");
> >
> >         t->bios_ver = smbios_add_prop(ctx, "version");
> > -       if (!t->bios_ver)
> > +       if (!strcmp(ctx->last_str, "Unknown"))
>
> That is really ugly...checking the ctx value looking for a side effect.
>
> Can you not have smbios_add_prop() continue to return NULL in this case?
>

Hmm I don't know, but I wonder why I am not just checking t->bios_ver for
Unknown.
I'll have a look and change it

[...]

Thanks
/Ilias

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

[Ilias Apalodimas]

Hi Simon,

[...]

> 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, 90 insertions(+), 4 deletions(-)
> >
>
> Can this be put behind a Kconfig? It adds quite a bit of code which
> punishes those boards which do the right thing.
>

Sure but OTOH the code increase should be really minimal. But I don't mind
I can add a Kconfig


> > +
> > +       dt_str = ofnode_read_string(ofnode_root(), nprop->dt_str);
>
> Could this use ofnode_read_string_index() ?
>

Maybe, I'll have a look and change it if that works

[...]


> Any chance of a test for this code?
>

Sure, but any suggestions on where to add the test?
SMBIOS tables are populated on OS booting, do we have a test somewhere that
boots an OS?
Any other ideas?

Thanks
/Ilias

>
> Regards,
> Simon
>

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

[Simon Glass]

Hi Ilias,

On Wed, 29 Nov 2023 at 23:50, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> Hi Simon,
>
> [...]
>
>> > 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, 90 insertions(+), 4 deletions(-)
>> >
>>
>> Can this be put behind a Kconfig? It adds quite a bit of code which
>> punishes those boards which do the right thing.
>
>
> Sure but OTOH the code increase should be really minimal. But I don't mind I can add a Kconfig
>
>>
>> > +
>> > +       dt_str = ofnode_read_string(ofnode_root(), nprop->dt_str);
>>
>> Could this use ofnode_read_string_index() ?
>
>
> Maybe, I'll have a look and change it if that works
>
> [...]
>
>>
>> Any chance of a test for this code?
>
>
> Sure, but any suggestions on where to add the test?
> SMBIOS tables are populated on OS booting, do we have a test somewhere that boots an OS?

They are written on startup, right? They should certainly be in place
before U-Boot enters the command line.

> Any other ideas?

Probably a test in test/lib/ would work. We actually have
smbios-parser.c which might help?

Regards,
Simon

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

[Ilias Apalodimas]

[...]

>
>>
>> >                 str = "Unknown";
>> >
>> >         for (;;) {
>> > @@ -151,8 +151,7 @@ static int smbios_add_prop_si(struct smbios_ctx *ctx, const char *prop,
>> >                 const char *str;
>> >
>> >                 str = ofnode_read_string(ctx->node, prop);
>> > -               if (str)
>> > -                       return smbios_add_string(ctx, str);
>> > +               return smbios_add_string(ctx, str);
>> >         }
>> >
>> >         return 0;
>> > @@ -231,7 +230,7 @@ static int smbios_write_type0(ulong *current, int handle,
>> >         t->vendor = smbios_add_string(ctx, "U-Boot");
>> >
>> >         t->bios_ver = smbios_add_prop(ctx, "version");
>> > -       if (!t->bios_ver)
>> > +       if (!strcmp(ctx->last_str, "Unknown"))
>>
>> That is really ugly...checking the ctx value looking for a side effect.
>>
>> Can you not have smbios_add_prop() continue to return NULL in this case?
>
>
> Hmm I don't know, but I wonder why I am not just checking t->bios_ver for Unknown.
> I'll have a look and change it

Ok, this can't be changed as easily.  smbios_add_prop() will not
return NULL in any case. It returns an integer. With the cleanup, it
will always writes 'Uknown' and not return 0 anymore.
I can add that default value you suggested but the ctx->last_str is
still used on the next line anyway.

Thanks
/Ilias

>
> [...]
>
> Thanks
> /Ilias

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

[Peter Robinson]

On Mon, Nov 27, 2023 at 5:11 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>

> ---
> 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, 90 insertions(+), 4 deletions(-)
>
> diff --git a/lib/smbios.c b/lib/smbios.c
> index fcc8686993ef..423893639ff0 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
>   *
> @@ -127,6 +161,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 separted 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
>   *
> @@ -139,19 +209,35 @@ 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)
>  {
> +       int ret = 0;
> +
>         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;
> +               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 maufacturer 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 = ofnode_read_string(ctx->node, prop);
> -               return smbios_add_string(ctx, str);
> +               ret = smbios_add_string(ctx, ofnode_valid(ctx->node) ?
> +                                       str : (const char *)str_dt);
> +               return ret;
>         }
>
>         return 0;
> --
> 2.40.1
>

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

[Peter Robinson]

On Thu, Nov 30, 2023 at 2:45 AM Simon Glass <sjg@chromium.org> wrote:
>
> Hi Ilias,
>
> On Mon, 27 Nov 2023 at 10:11, 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>
> > ---
> > 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, 90 insertions(+), 4 deletions(-)
> >
>
> Can this be put behind a Kconfig? It adds quite a bit of code which
> punishes those boards which do the right thing.

What's the size difference? By putting it behind a Kconfig you're
assuming the boards that don't do the right thing are going to enable
this at which point we punish users by not having some level of
usability. By "right thing" do you mean the smbios entries in the
-u-boot.dtsi?

> > diff --git a/lib/smbios.c b/lib/smbios.c
> > index fcc8686993ef..423893639ff0 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
> >   *
> > @@ -127,6 +161,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 separted values and return the value.
>
> spelling
>
> > + *                   If nprop->max exceeds the number of comma separated
>
> comma-separated
>
> > + *                   elements the last non NULL value will be returned.
>
> elements, the
>
> > + *                   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);
>
> Could this use ofnode_read_string_index() ?
>
> > +       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
> >   *
> > @@ -139,19 +209,35 @@ 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)
> >  {
> > +       int ret = 0;
> > +
> >         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;
> > +               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 maufacturer and board type
>
> spelling
>
> > +                */
> > +               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 = ofnode_read_string(ctx->node, prop);
> > -               return smbios_add_string(ctx, str);
> > +               ret = smbios_add_string(ctx, ofnode_valid(ctx->node) ?
> > +                                       str : (const char *)str_dt);
> > +               return ret;
> >         }
> >
> >         return 0;
> > --
> > 2.40.1
> >
>
> Any chance of a test for this code?
>
> Regards,
> Simon

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

[Ilias Apalodimas]

Hi Simon,

On Sat, 2 Dec 2023 at 20:28, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Ilias,
>
> On Wed, 29 Nov 2023 at 23:50, Ilias Apalodimas
> <ilias.apalodimas@linaro.org> wrote:
> >
> > Hi Simon,
> >
> > [...]
> >
> >> > 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, 90 insertions(+), 4 deletions(-)
> >> >
> >>
> >> Can this be put behind a Kconfig? It adds quite a bit of code which
> >> punishes those boards which do the right thing.
> >
> >
> > Sure but OTOH the code increase should be really minimal. But I don't mind I can add a Kconfig
> >
> >>
> >> > +
> >> > +       dt_str = ofnode_read_string(ofnode_root(), nprop->dt_str);
> >>
> >> Could this use ofnode_read_string_index() ?
> >
> >
> > Maybe, I'll have a look and change it if that works

Unless I am missing something this doesn't work.
This is designed to return a string index from a DT property that's defined as
foo_property = "value1", "value2" isn't it?

The code above is trying to read an existing property (e.g compatible)
and get the string after the comma delimiter.
Perhaps I should add this in drivers/core/ofnode.c instead?

> >
> > [...]
> >
> >>
> >> Any chance of a test for this code?
> >
> >
> > Sure, but any suggestions on where to add the test?
> > SMBIOS tables are populated on OS booting, do we have a test somewhere that boots an OS?
>
> They are written on startup, right? They should certainly be in place
> before U-Boot enters the command line.

Not always.  I am not sure if x86 does that, but on the rest of the
architectures, they are only initialized when the efi smbios code
runs. Wasn't this something you were trying to change?

>
> > Any other ideas?
>
> Probably a test in test/lib/ would work. We actually have
> smbios-parser.c which might help?

That doesn't seem too helpful either. But I can add a test in
test/dm/ofnode.c if we move the parsing function to ofnode.c. The
generic SMBIOS tests can be added when the SMBIOS code is refactored.

Regards
/Ilias
>
> Regards,
> Simon

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

[Tom Rini]

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

On Wed, Dec 06, 2023 at 06:03:14PM +0200, Ilias Apalodimas wrote:
> Hi Simon,
> 
> On Sat, 2 Dec 2023 at 20:28, Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Ilias,
> >
> > On Wed, 29 Nov 2023 at 23:50, Ilias Apalodimas
> > <ilias.apalodimas@linaro.org> wrote:
> > >
> > > Hi Simon,
> > >
> > > [...]
> > >
> > >> > 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, 90 insertions(+), 4 deletions(-)
> > >> >
> > >>
> > >> Can this be put behind a Kconfig? It adds quite a bit of code which
> > >> punishes those boards which do the right thing.
> > >
> > >
> > > Sure but OTOH the code increase should be really minimal. But I don't mind I can add a Kconfig
> > >
> > >>
> > >> > +
> > >> > +       dt_str = ofnode_read_string(ofnode_root(), nprop->dt_str);
> > >>
> > >> Could this use ofnode_read_string_index() ?
> > >
> > >
> > > Maybe, I'll have a look and change it if that works
> 
> Unless I am missing something this doesn't work.
> This is designed to return a string index from a DT property that's defined as
> foo_property = "value1", "value2" isn't it?
> 
> The code above is trying to read an existing property (e.g compatible)
> and get the string after the comma delimiter.
> Perhaps I should add this in drivers/core/ofnode.c instead?
> 
> > >
> > > [...]
> > >
> > >>
> > >> Any chance of a test for this code?
> > >
> > >
> > > Sure, but any suggestions on where to add the test?
> > > SMBIOS tables are populated on OS booting, do we have a test somewhere that boots an OS?
> >
> > They are written on startup, right? They should certainly be in place
> > before U-Boot enters the command line.
> 
> Not always.  I am not sure if x86 does that, but on the rest of the
> architectures, they are only initialized when the efi smbios code
> runs. Wasn't this something you were trying to change?

One of those things I keep repeating is that we don't know for sure what
the right values here are until we load the DT the OS _will_ use. It's
quite valid to start U-Boot and pass it a generic "good enough" DT at
run time until U-Boot can see (GPIOs, EEPROM, whatever) what it's on and
what the real DT to load before passing to the OS is. Since U-Boot
doesn't need SMBIOS tables itself, we should make these "late" not
"early".

-- 
Tom

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

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

[Ilias Apalodimas]

Hi Tom,

On Sat, 9 Dec 2023 at 20:49, Tom Rini <trini@konsulko.com> wrote:
>
> On Wed, Dec 06, 2023 at 06:03:14PM +0200, Ilias Apalodimas wrote:
> > Hi Simon,
> >
> > On Sat, 2 Dec 2023 at 20:28, Simon Glass <sjg@chromium.org> wrote:
> > >
> > > Hi Ilias,
> > >
> > > On Wed, 29 Nov 2023 at 23:50, Ilias Apalodimas
> > > <ilias.apalodimas@linaro.org> wrote:
> > > >
> > > > Hi Simon,
> > > >
> > > > [...]
> > > >
> > > >> > 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, 90 insertions(+), 4 deletions(-)
> > > >> >
> > > >>
> > > >> Can this be put behind a Kconfig? It adds quite a bit of code which
> > > >> punishes those boards which do the right thing.
> > > >
> > > >
> > > > Sure but OTOH the code increase should be really minimal. But I don't mind I can add a Kconfig
> > > >
> > > >>
> > > >> > +
> > > >> > +       dt_str = ofnode_read_string(ofnode_root(), nprop->dt_str);
> > > >>
> > > >> Could this use ofnode_read_string_index() ?
> > > >
> > > >
> > > > Maybe, I'll have a look and change it if that works
> >
> > Unless I am missing something this doesn't work.
> > This is designed to return a string index from a DT property that's defined as
> > foo_property = "value1", "value2" isn't it?
> >
> > The code above is trying to read an existing property (e.g compatible)
> > and get the string after the comma delimiter.
> > Perhaps I should add this in drivers/core/ofnode.c instead?
> >
> > > >
> > > > [...]
> > > >
> > > >>
> > > >> Any chance of a test for this code?
> > > >
> > > >
> > > > Sure, but any suggestions on where to add the test?
> > > > SMBIOS tables are populated on OS booting, do we have a test somewhere that boots an OS?
> > >
> > > They are written on startup, right? They should certainly be in place
> > > before U-Boot enters the command line.
> >
> > Not always.  I am not sure if x86 does that, but on the rest of the
> > architectures, they are only initialized when the efi smbios code
> > runs. Wasn't this something you were trying to change?
>
> One of those things I keep repeating is that we don't know for sure what
> the right values here are until we load the DT the OS _will_ use. It's
> quite valid to start U-Boot and pass it a generic "good enough" DT at
> run time until U-Boot can see (GPIOs, EEPROM, whatever) what it's on and
> what the real DT to load before passing to the OS is. Since U-Boot
> doesn't need SMBIOS tables itself, we should make these "late" not
> "early".

Fair enough, we can defer the init and testing of those late, just
before we are about to boot. But this is irrelevant to what this patch
does, can we get the fallback mechanism in first, assuming everyone is
ok with the patch now?

Thanks
/Ilias
>
> --
> Tom

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

[Simon Glass]

Hi Ilias,

On Mon, 11 Dec 2023 at 00:49, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> Hi Tom,
>
> On Sat, 9 Dec 2023 at 20:49, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Wed, Dec 06, 2023 at 06:03:14PM +0200, Ilias Apalodimas wrote:
> > > Hi Simon,
> > >
> > > On Sat, 2 Dec 2023 at 20:28, Simon Glass <sjg@chromium.org> wrote:
> > > >
> > > > Hi Ilias,
> > > >
> > > > On Wed, 29 Nov 2023 at 23:50, Ilias Apalodimas
> > > > <ilias.apalodimas@linaro.org> wrote:
> > > > >
> > > > > Hi Simon,
> > > > >
> > > > > [...]
> > > > >
> > > > >> > 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, 90 insertions(+), 4 deletions(-)
> > > > >> >
> > > > >>
> > > > >> Can this be put behind a Kconfig? It adds quite a bit of code which
> > > > >> punishes those boards which do the right thing.
> > > > >
> > > > >
> > > > > Sure but OTOH the code increase should be really minimal. But I don't mind I can add a Kconfig
> > > > >
> > > > >>
> > > > >> > +
> > > > >> > +       dt_str = ofnode_read_string(ofnode_root(), nprop->dt_str);
> > > > >>
> > > > >> Could this use ofnode_read_string_index() ?
> > > > >
> > > > >
> > > > > Maybe, I'll have a look and change it if that works
> > >
> > > Unless I am missing something this doesn't work.
> > > This is designed to return a string index from a DT property that's defined as
> > > foo_property = "value1", "value2" isn't it?
> > >
> > > The code above is trying to read an existing property (e.g compatible)
> > > and get the string after the comma delimiter.
> > > Perhaps I should add this in drivers/core/ofnode.c instead?
> > >
> > > > >
> > > > > [...]
> > > > >
> > > > >>
> > > > >> Any chance of a test for this code?
> > > > >
> > > > >
> > > > > Sure, but any suggestions on where to add the test?
> > > > > SMBIOS tables are populated on OS booting, do we have a test somewhere that boots an OS?
> > > >
> > > > They are written on startup, right? They should certainly be in place
> > > > before U-Boot enters the command line.
> > >
> > > Not always.  I am not sure if x86 does that, but on the rest of the
> > > architectures, they are only initialized when the efi smbios code
> > > runs. Wasn't this something you were trying to change?
> >
> > One of those things I keep repeating is that we don't know for sure what
> > the right values here are until we load the DT the OS _will_ use. It's
> > quite valid to start U-Boot and pass it a generic "good enough" DT at
> > run time until U-Boot can see (GPIOs, EEPROM, whatever) what it's on and
> > what the real DT to load before passing to the OS is. Since U-Boot
> > doesn't need SMBIOS tables itself, we should make these "late" not
> > "early".
>
> Fair enough, we can defer the init and testing of those late, just
> before we are about to boot. But this is irrelevant to what this patch
> does, can we get the fallback mechanism in first, assuming everyone is
> ok with the patch now?
>

I would like this behind a Kconfig. Making it the default means people
are going to start relying on (in user space) and then discover later
that it is wrong.

Regards,
Simon

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

[Tom Rini]

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

On Wed, Dec 13, 2023 at 12:50:03PM -0700, Simon Glass wrote:
> Hi Ilias,
> 
> On Mon, 11 Dec 2023 at 00:49, Ilias Apalodimas
> <ilias.apalodimas@linaro.org> wrote:
> >
> > Hi Tom,
> >
> > On Sat, 9 Dec 2023 at 20:49, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Wed, Dec 06, 2023 at 06:03:14PM +0200, Ilias Apalodimas wrote:
> > > > Hi Simon,
> > > >
> > > > On Sat, 2 Dec 2023 at 20:28, Simon Glass <sjg@chromium.org> wrote:
> > > > >
> > > > > Hi Ilias,
> > > > >
> > > > > On Wed, 29 Nov 2023 at 23:50, Ilias Apalodimas
> > > > > <ilias.apalodimas@linaro.org> wrote:
> > > > > >
> > > > > > Hi Simon,
> > > > > >
> > > > > > [...]
> > > > > >
> > > > > >> > 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, 90 insertions(+), 4 deletions(-)
> > > > > >> >
> > > > > >>
> > > > > >> Can this be put behind a Kconfig? It adds quite a bit of code which
> > > > > >> punishes those boards which do the right thing.
> > > > > >
> > > > > >
> > > > > > Sure but OTOH the code increase should be really minimal. But I don't mind I can add a Kconfig
> > > > > >
> > > > > >>
> > > > > >> > +
> > > > > >> > +       dt_str = ofnode_read_string(ofnode_root(), nprop->dt_str);
> > > > > >>
> > > > > >> Could this use ofnode_read_string_index() ?
> > > > > >
> > > > > >
> > > > > > Maybe, I'll have a look and change it if that works
> > > >
> > > > Unless I am missing something this doesn't work.
> > > > This is designed to return a string index from a DT property that's defined as
> > > > foo_property = "value1", "value2" isn't it?
> > > >
> > > > The code above is trying to read an existing property (e.g compatible)
> > > > and get the string after the comma delimiter.
> > > > Perhaps I should add this in drivers/core/ofnode.c instead?
> > > >
> > > > > >
> > > > > > [...]
> > > > > >
> > > > > >>
> > > > > >> Any chance of a test for this code?
> > > > > >
> > > > > >
> > > > > > Sure, but any suggestions on where to add the test?
> > > > > > SMBIOS tables are populated on OS booting, do we have a test somewhere that boots an OS?
> > > > >
> > > > > They are written on startup, right? They should certainly be in place
> > > > > before U-Boot enters the command line.
> > > >
> > > > Not always.  I am not sure if x86 does that, but on the rest of the
> > > > architectures, they are only initialized when the efi smbios code
> > > > runs. Wasn't this something you were trying to change?
> > >
> > > One of those things I keep repeating is that we don't know for sure what
> > > the right values here are until we load the DT the OS _will_ use. It's
> > > quite valid to start U-Boot and pass it a generic "good enough" DT at
> > > run time until U-Boot can see (GPIOs, EEPROM, whatever) what it's on and
> > > what the real DT to load before passing to the OS is. Since U-Boot
> > > doesn't need SMBIOS tables itself, we should make these "late" not
> > > "early".
> >
> > Fair enough, we can defer the init and testing of those late, just
> > before we are about to boot. But this is irrelevant to what this patch
> > does, can we get the fallback mechanism in first, assuming everyone is
> > ok with the patch now?
> >
> 
> I would like this behind a Kconfig. Making it the default means people
> are going to start relying on (in user space) and then discover later
> that it is wrong.

What do you mean wrong, exactly?

-- 
Tom

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

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

[Simon Glass]

Hi Tom,

On Wed, 13 Dec 2023 at 13:19, Tom Rini <trini@konsulko.com> wrote:
>
> On Wed, Dec 13, 2023 at 12:50:03PM -0700, Simon Glass wrote:
> > Hi Ilias,
> >
> > On Mon, 11 Dec 2023 at 00:49, Ilias Apalodimas
> > <ilias.apalodimas@linaro.org> wrote:
> > >
> > > Hi Tom,
> > >
> > > On Sat, 9 Dec 2023 at 20:49, Tom Rini <trini@konsulko.com> wrote:
> > > >
> > > > On Wed, Dec 06, 2023 at 06:03:14PM +0200, Ilias Apalodimas wrote:
> > > > > Hi Simon,
> > > > >
> > > > > On Sat, 2 Dec 2023 at 20:28, Simon Glass <sjg@chromium.org> wrote:
> > > > > >
> > > > > > Hi Ilias,
> > > > > >
> > > > > > On Wed, 29 Nov 2023 at 23:50, Ilias Apalodimas
> > > > > > <ilias.apalodimas@linaro.org> wrote:
> > > > > > >
> > > > > > > Hi Simon,
> > > > > > >
> > > > > > > [...]
> > > > > > >
> > > > > > >> > 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, 90 insertions(+), 4 deletions(-)
> > > > > > >> >
> > > > > > >>
> > > > > > >> Can this be put behind a Kconfig? It adds quite a bit of code which
> > > > > > >> punishes those boards which do the right thing.
> > > > > > >
> > > > > > >
> > > > > > > Sure but OTOH the code increase should be really minimal. But I don't mind I can add a Kconfig
> > > > > > >
> > > > > > >>
> > > > > > >> > +
> > > > > > >> > +       dt_str = ofnode_read_string(ofnode_root(), nprop->dt_str);
> > > > > > >>
> > > > > > >> Could this use ofnode_read_string_index() ?
> > > > > > >
> > > > > > >
> > > > > > > Maybe, I'll have a look and change it if that works
> > > > >
> > > > > Unless I am missing something this doesn't work.
> > > > > This is designed to return a string index from a DT property that's defined as
> > > > > foo_property = "value1", "value2" isn't it?
> > > > >
> > > > > The code above is trying to read an existing property (e.g compatible)
> > > > > and get the string after the comma delimiter.
> > > > > Perhaps I should add this in drivers/core/ofnode.c instead?
> > > > >
> > > > > > >
> > > > > > > [...]
> > > > > > >
> > > > > > >>
> > > > > > >> Any chance of a test for this code?
> > > > > > >
> > > > > > >
> > > > > > > Sure, but any suggestions on where to add the test?
> > > > > > > SMBIOS tables are populated on OS booting, do we have a test somewhere that boots an OS?
> > > > > >
> > > > > > They are written on startup, right? They should certainly be in place
> > > > > > before U-Boot enters the command line.
> > > > >
> > > > > Not always.  I am not sure if x86 does that, but on the rest of the
> > > > > architectures, they are only initialized when the efi smbios code
> > > > > runs. Wasn't this something you were trying to change?
> > > >
> > > > One of those things I keep repeating is that we don't know for sure what
> > > > the right values here are until we load the DT the OS _will_ use. It's
> > > > quite valid to start U-Boot and pass it a generic "good enough" DT at
> > > > run time until U-Boot can see (GPIOs, EEPROM, whatever) what it's on and
> > > > what the real DT to load before passing to the OS is. Since U-Boot
> > > > doesn't need SMBIOS tables itself, we should make these "late" not
> > > > "early".
> > >
> > > Fair enough, we can defer the init and testing of those late, just
> > > before we are about to boot. But this is irrelevant to what this patch
> > > does, can we get the fallback mechanism in first, assuming everyone is
> > > ok with the patch now?
> > >
> >
> > I would like this behind a Kconfig. Making it the default means people
> > are going to start relying on (in user space) and then discover later
> > that it is wrong.
>
> What do you mean wrong, exactly?

"raspberrypi" instead of "Raspberry Pi", for example, or "friendlyarm"
instead of "FriendlyElec"

I just wonder what this information is actually used for. If it is
just for fun, that is fine, but I believe some tools do use dmidecode,
at which point it needs to be accurate and should not change later,
e.g. when someone decides to actually add the info.

Regards,
Simon

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

[Mark Kettenis]

> From: Simon Glass <sjg@chromium.org>
> Date: Wed, 13 Dec 2023 13:41:05 -0700
> 
> Hi Tom,
> 
> On Wed, 13 Dec 2023 at 13:19, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Wed, Dec 13, 2023 at 12:50:03PM -0700, Simon Glass wrote:
> > > Hi Ilias,
> > >
> > > On Mon, 11 Dec 2023 at 00:49, Ilias Apalodimas
> > > <ilias.apalodimas@linaro.org> wrote:
> > > >
> > > > Hi Tom,
> > > >
> > > > On Sat, 9 Dec 2023 at 20:49, Tom Rini <trini@konsulko.com> wrote:
> > > > >
> > > > > On Wed, Dec 06, 2023 at 06:03:14PM +0200, Ilias Apalodimas wrote:
> > > > > > Hi Simon,
> > > > > >
> > > > > > On Sat, 2 Dec 2023 at 20:28, Simon Glass <sjg@chromium.org> wrote:
> > > > > > >
> > > > > > > Hi Ilias,
> > > > > > >
> > > > > > > On Wed, 29 Nov 2023 at 23:50, Ilias Apalodimas
> > > > > > > <ilias.apalodimas@linaro.org> wrote:
> > > > > > > >
> > > > > > > > Hi Simon,
> > > > > > > >
> > > > > > > > [...]
> > > > > > > >
> > > > > > > >> > 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, 90 insertions(+), 4 deletions(-)
> > > > > > > >> >
> > > > > > > >>
> > > > > > > >> Can this be put behind a Kconfig? It adds quite a bit of code which
> > > > > > > >> punishes those boards which do the right thing.
> > > > > > > >
> > > > > > > >
> > > > > > > > Sure but OTOH the code increase should be really minimal. But I don't mind I can add a Kconfig
> > > > > > > >
> > > > > > > >>
> > > > > > > >> > +
> > > > > > > >> > +       dt_str = ofnode_read_string(ofnode_root(), nprop->dt_str);
> > > > > > > >>
> > > > > > > >> Could this use ofnode_read_string_index() ?
> > > > > > > >
> > > > > > > >
> > > > > > > > Maybe, I'll have a look and change it if that works
> > > > > >
> > > > > > Unless I am missing something this doesn't work.
> > > > > > This is designed to return a string index from a DT property that's defined as
> > > > > > foo_property = "value1", "value2" isn't it?
> > > > > >
> > > > > > The code above is trying to read an existing property (e.g compatible)
> > > > > > and get the string after the comma delimiter.
> > > > > > Perhaps I should add this in drivers/core/ofnode.c instead?
> > > > > >
> > > > > > > >
> > > > > > > > [...]
> > > > > > > >
> > > > > > > >>
> > > > > > > >> Any chance of a test for this code?
> > > > > > > >
> > > > > > > >
> > > > > > > > Sure, but any suggestions on where to add the test?
> > > > > > > > SMBIOS tables are populated on OS booting, do we have a test somewhere that boots an OS?
> > > > > > >
> > > > > > > They are written on startup, right? They should certainly be in place
> > > > > > > before U-Boot enters the command line.
> > > > > >
> > > > > > Not always.  I am not sure if x86 does that, but on the rest of the
> > > > > > architectures, they are only initialized when the efi smbios code
> > > > > > runs. Wasn't this something you were trying to change?
> > > > >
> > > > > One of those things I keep repeating is that we don't know for sure what
> > > > > the right values here are until we load the DT the OS _will_ use. It's
> > > > > quite valid to start U-Boot and pass it a generic "good enough" DT at
> > > > > run time until U-Boot can see (GPIOs, EEPROM, whatever) what it's on and
> > > > > what the real DT to load before passing to the OS is. Since U-Boot
> > > > > doesn't need SMBIOS tables itself, we should make these "late" not
> > > > > "early".
> > > >
> > > > Fair enough, we can defer the init and testing of those late, just
> > > > before we are about to boot. But this is irrelevant to what this patch
> > > > does, can we get the fallback mechanism in first, assuming everyone is
> > > > ok with the patch now?
> > > >
> > >
> > > I would like this behind a Kconfig. Making it the default means people
> > > are going to start relying on (in user space) and then discover later
> > > that it is wrong.
> >
> > What do you mean wrong, exactly?
> 
> "raspberrypi" instead of "Raspberry Pi", for example, or "friendlyarm"
> instead of "FriendlyElec"

Heh, well, even in the x86 world vendors can't even spell their own
name consistently.

> I just wonder what this information is actually used for. If it is
> just for fun, that is fine, but I believe some tools do use dmidecode,
> at which point it needs to be accurate and should not change later,
> e.g. when someone decides to actually add the info.

So the Linux kernel uses this information to apply quirks for specific
machines.  But I hope that on platforms that have a device tree folks
will just use the board compatible string for that purpose.

Otherwise I think this information is mostly used to populate system
information UIs and asset databases.

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

[Simon Glass]

Hi Mark,

On Wed, 13 Dec 2023 at 14:00, Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
>
> > From: Simon Glass <sjg@chromium.org>
> > Date: Wed, 13 Dec 2023 13:41:05 -0700
> >
> > Hi Tom,
> >
> > On Wed, 13 Dec 2023 at 13:19, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Wed, Dec 13, 2023 at 12:50:03PM -0700, Simon Glass wrote:
> > > > Hi Ilias,
> > > >
> > > > On Mon, 11 Dec 2023 at 00:49, Ilias Apalodimas
> > > > <ilias.apalodimas@linaro.org> wrote:
> > > > >
> > > > > Hi Tom,
> > > > >
> > > > > On Sat, 9 Dec 2023 at 20:49, Tom Rini <trini@konsulko.com> wrote:
> > > > > >
> > > > > > On Wed, Dec 06, 2023 at 06:03:14PM +0200, Ilias Apalodimas wrote:
> > > > > > > Hi Simon,
> > > > > > >
> > > > > > > On Sat, 2 Dec 2023 at 20:28, Simon Glass <sjg@chromium.org> wrote:
> > > > > > > >
> > > > > > > > Hi Ilias,
> > > > > > > >
> > > > > > > > On Wed, 29 Nov 2023 at 23:50, Ilias Apalodimas
> > > > > > > > <ilias.apalodimas@linaro.org> wrote:
> > > > > > > > >
> > > > > > > > > Hi Simon,
> > > > > > > > >
> > > > > > > > > [...]
> > > > > > > > >
> > > > > > > > >> > 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, 90 insertions(+), 4 deletions(-)
> > > > > > > > >> >
> > > > > > > > >>
> > > > > > > > >> Can this be put behind a Kconfig? It adds quite a bit of code which
> > > > > > > > >> punishes those boards which do the right thing.
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > Sure but OTOH the code increase should be really minimal. But I don't mind I can add a Kconfig
> > > > > > > > >
> > > > > > > > >>
> > > > > > > > >> > +
> > > > > > > > >> > +       dt_str = ofnode_read_string(ofnode_root(), nprop->dt_str);
> > > > > > > > >>
> > > > > > > > >> Could this use ofnode_read_string_index() ?
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > Maybe, I'll have a look and change it if that works
> > > > > > >
> > > > > > > Unless I am missing something this doesn't work.
> > > > > > > This is designed to return a string index from a DT property that's defined as
> > > > > > > foo_property = "value1", "value2" isn't it?
> > > > > > >
> > > > > > > The code above is trying to read an existing property (e.g compatible)
> > > > > > > and get the string after the comma delimiter.
> > > > > > > Perhaps I should add this in drivers/core/ofnode.c instead?
> > > > > > >
> > > > > > > > >
> > > > > > > > > [...]
> > > > > > > > >
> > > > > > > > >>
> > > > > > > > >> Any chance of a test for this code?
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > Sure, but any suggestions on where to add the test?
> > > > > > > > > SMBIOS tables are populated on OS booting, do we have a test somewhere that boots an OS?
> > > > > > > >
> > > > > > > > They are written on startup, right? They should certainly be in place
> > > > > > > > before U-Boot enters the command line.
> > > > > > >
> > > > > > > Not always.  I am not sure if x86 does that, but on the rest of the
> > > > > > > architectures, they are only initialized when the efi smbios code
> > > > > > > runs. Wasn't this something you were trying to change?
> > > > > >
> > > > > > One of those things I keep repeating is that we don't know for sure what
> > > > > > the right values here are until we load the DT the OS _will_ use. It's
> > > > > > quite valid to start U-Boot and pass it a generic "good enough" DT at
> > > > > > run time until U-Boot can see (GPIOs, EEPROM, whatever) what it's on and
> > > > > > what the real DT to load before passing to the OS is. Since U-Boot
> > > > > > doesn't need SMBIOS tables itself, we should make these "late" not
> > > > > > "early".
> > > > >
> > > > > Fair enough, we can defer the init and testing of those late, just
> > > > > before we are about to boot. But this is irrelevant to what this patch
> > > > > does, can we get the fallback mechanism in first, assuming everyone is
> > > > > ok with the patch now?
> > > > >
> > > >
> > > > I would like this behind a Kconfig. Making it the default means people
> > > > are going to start relying on (in user space) and then discover later
> > > > that it is wrong.
> > >
> > > What do you mean wrong, exactly?
> >
> > "raspberrypi" instead of "Raspberry Pi", for example, or "friendlyarm"
> > instead of "FriendlyElec"
>
> Heh, well, even in the x86 world vendors can't even spell their own
> name consistently.
>
> > I just wonder what this information is actually used for. If it is
> > just for fun, that is fine, but I believe some tools do use dmidecode,
> > at which point it needs to be accurate and should not change later,
> > e.g. when someone decides to actually add the info.
>
> So the Linux kernel uses this information to apply quirks for specific
> machines.  But I hope that on platforms that have a device tree folks
> will just use the board compatible string for that purpose.

Right.

>
> Otherwise I think this information is mostly used to populate system
> information UIs and asset databases.

So perhaps the devicetree should be used instead? How do these things
work today? And what are they, exactly?

Regards,
Simon

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

[Simon Glass]

Hi Ilias,

On Wed, 6 Dec 2023 at 04:36, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> [...]
>
> >
> >>
> >> >                 str = "Unknown";
> >> >
> >> >         for (;;) {
> >> > @@ -151,8 +151,7 @@ static int smbios_add_prop_si(struct smbios_ctx *ctx, const char *prop,
> >> >                 const char *str;
> >> >
> >> >                 str = ofnode_read_string(ctx->node, prop);
> >> > -               if (str)
> >> > -                       return smbios_add_string(ctx, str);
> >> > +               return smbios_add_string(ctx, str);
> >> >         }
> >> >
> >> >         return 0;
> >> > @@ -231,7 +230,7 @@ static int smbios_write_type0(ulong *current, int handle,
> >> >         t->vendor = smbios_add_string(ctx, "U-Boot");
> >> >
> >> >         t->bios_ver = smbios_add_prop(ctx, "version");
> >> > -       if (!t->bios_ver)
> >> > +       if (!strcmp(ctx->last_str, "Unknown"))
> >>
> >> That is really ugly...checking the ctx value looking for a side effect.
> >>
> >> Can you not have smbios_add_prop() continue to return NULL in this case?
> >
> >
> > Hmm I don't know, but I wonder why I am not just checking t->bios_ver for Unknown.
> > I'll have a look and change it
>
> Ok, this can't be changed as easily.  smbios_add_prop() will not
> return NULL in any case. It returns an integer. With the cleanup, it
> will always writes 'Uknown' and not return 0 anymore.
> I can add that default value you suggested but the ctx->last_str is
> still used on the next line anyway.

Would you mind if I tried to create a version of the patch which does
the same thing but with less code, and perhaps a test? It might be
easier to discuss it then. I can't claim to understand all the
different code paths at this point.

My main concern is that with so many code paths it will be hard even
to refactor the code in the future, since it will become
'load-bearing' and anyone might turn up and say it breaks their board
because different information is provided.

Overall, so long as the information isn't used for anything important
in userspace, and we find a way to report SMBIOS to Linux without EFI,
it is OK to enable this feature (with a new Kconfig so it can be
disabled). But there is already authoritative info in the DT, so this
transformation into SMBIOS really should just be used for user
display, etc., not for anything which affects operation of the device.
Do you agree? If you do, how to ensure that? Perhaps a special string
in the table like 'GENERATED'?

Regards,
Simon

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

[Peter Robinson]

> > > > > > Not always.  I am not sure if x86 does that, but on the rest of the
> > > > > > architectures, they are only initialized when the efi smbios code
> > > > > > runs. Wasn't this something you were trying to change?
> > > > >
> > > > > One of those things I keep repeating is that we don't know for sure what
> > > > > the right values here are until we load the DT the OS _will_ use. It's
> > > > > quite valid to start U-Boot and pass it a generic "good enough" DT at
> > > > > run time until U-Boot can see (GPIOs, EEPROM, whatever) what it's on and
> > > > > what the real DT to load before passing to the OS is. Since U-Boot
> > > > > doesn't need SMBIOS tables itself, we should make these "late" not
> > > > > "early".
> > > >
> > > > Fair enough, we can defer the init and testing of those late, just
> > > > before we are about to boot. But this is irrelevant to what this patch
> > > > does, can we get the fallback mechanism in first, assuming everyone is
> > > > ok with the patch now?
> > > >
> > >
> > > I would like this behind a Kconfig. Making it the default means people
> > > are going to start relying on (in user space) and then discover later
> > > that it is wrong.
> >
> > What do you mean wrong, exactly?
>
> "raspberrypi" instead of "Raspberry Pi", for example, or "friendlyarm"
> instead of "FriendlyElec"

Well "raspberrypi" is better that what we get on the Raspberry Pi with
my testing at the moment which is "Unknown", which from what I can
tell from commit 330419727 should have at least the minimal amount but
it doesn't.

> I just wonder what this information is actually used for. If it is
> just for fun, that is fine, but I believe some tools do use dmidecode,
> at which point it needs to be accurate and should not change later,
> e.g. when someone decides to actually add the info.

So a lot of tools use the output of dmidecode, it's used extensively
through various support platforms and management tools.

In Fedora I would generally get around a dozen reports every few
months because anything booted with U-Boot was basically populated
with "Unknown".

At the moment, on the RPi4 with default upstream I get Unkown [1] with
these patches I get at least some useful information [2]. I've been
shipping this patch set, in various versions, for a while and have not
had a single bug report about wrong information.

When Ilias and I first spoke about these patches the intention was to
get initial useful information, them they could be possibly expanded
using things like information from eFuses (serial numbers etc).

In some cases with U-Boot you get SMBIOS tables that are incorrect and
crash the tools.

With this patch set you get at least the basics which is important
because support teams can easily work out the basics of what they're
working with even if they don't have physical access all without the
vendor of the HW having to work out what to do in firmware to make
something work.

This patch set moves the needle forward, if we wait for everything to
be properly formatted we will wait for ever, with information we
already have available ON EVERY DEVICE.

Peter

[1] https://pbrobinson.fedorapeople.org/dmi-decode-rpi-defaults.txt
[2] https://pbrobinson.fedorapeople.org/dmi-decode-rpi-auto-fallback-patchset.txt

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

[Peter Robinson]

> > > > What do you mean wrong, exactly?
> > >
> > > "raspberrypi" instead of "Raspberry Pi", for example, or "friendlyarm"
> > > instead of "FriendlyElec"
> >
> > Heh, well, even in the x86 world vendors can't even spell their own
> > name consistently.
> >
> > > I just wonder what this information is actually used for. If it is
> > > just for fun, that is fine, but I believe some tools do use dmidecode,
> > > at which point it needs to be accurate and should not change later,
> > > e.g. when someone decides to actually add the info.
> >
> > So the Linux kernel uses this information to apply quirks for specific
> > machines.  But I hope that on platforms that have a device tree folks
> > will just use the board compatible string for that purpose.
>
> Right.
>
> >
> > Otherwise I think this information is mostly used to populate system
> > information UIs and asset databases.
>
> So perhaps the devicetree should be used instead? How do these things
> work today? And what are they, exactly?

Device tree used for what? To populate the SMBIOS tables? Or to
populate "system information UIs and asset databases."

If you mean the later, that is not going to work. These platforms are
often proprietary, and are generally remote. They have local agents
that use dmidecode to populate things for support and management. It's
kind of in the name "System Management". To get all these platforms
updated to "just use devicetree" will take decades. No thanks!

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

[Peter Robinson]

Hi Simon,

On Mon, Dec 18, 2023 at 3:02 PM Simon Glass <sjg@chromium.org> wrote:
>
> Hi Ilias,
>
> On Wed, 6 Dec 2023 at 04:36, Ilias Apalodimas
> <ilias.apalodimas@linaro.org> wrote:
> >
> > [...]
> >
> > >
> > >>
> > >> >                 str = "Unknown";
> > >> >
> > >> >         for (;;) {
> > >> > @@ -151,8 +151,7 @@ static int smbios_add_prop_si(struct smbios_ctx *ctx, const char *prop,
> > >> >                 const char *str;
> > >> >
> > >> >                 str = ofnode_read_string(ctx->node, prop);
> > >> > -               if (str)
> > >> > -                       return smbios_add_string(ctx, str);
> > >> > +               return smbios_add_string(ctx, str);
> > >> >         }
> > >> >
> > >> >         return 0;
> > >> > @@ -231,7 +230,7 @@ static int smbios_write_type0(ulong *current, int handle,
> > >> >         t->vendor = smbios_add_string(ctx, "U-Boot");
> > >> >
> > >> >         t->bios_ver = smbios_add_prop(ctx, "version");
> > >> > -       if (!t->bios_ver)
> > >> > +       if (!strcmp(ctx->last_str, "Unknown"))
> > >>
> > >> That is really ugly...checking the ctx value looking for a side effect.
> > >>
> > >> Can you not have smbios_add_prop() continue to return NULL in this case?
> > >
> > >
> > > Hmm I don't know, but I wonder why I am not just checking t->bios_ver for Unknown.
> > > I'll have a look and change it
> >
> > Ok, this can't be changed as easily.  smbios_add_prop() will not
> > return NULL in any case. It returns an integer. With the cleanup, it
> > will always writes 'Uknown' and not return 0 anymore.
> > I can add that default value you suggested but the ctx->last_str is
> > still used on the next line anyway.
>
> Would you mind if I tried to create a version of the patch which does
> the same thing but with less code, and perhaps a test? It might be
> easier to discuss it then. I can't claim to understand all the
> different code paths at this point.
>
> My main concern is that with so many code paths it will be hard even
> to refactor the code in the future, since it will become
> 'load-bearing' and anyone might turn up and say it breaks their board
> because different information is provided.

I don't buy this argument at all, sorry.

> Overall, so long as the information isn't used for anything important
> in userspace, and we find a way to report SMBIOS to Linux without EFI,

No, you can't tie random requirements to improving the SMBIOS support.
We *already* report SMBIOS to Linux, reporting SMBIOS to Linux without
EFI is changing things that will need different or extra standards,
that could take years.

You are arbitrarily adding extra requirements just to suite yourself,
please STOP trying to hold things like this hostage.

> it is OK to enable this feature (with a new Kconfig so it can be
> disabled). But there is already authoritative info in the DT, so this

There is two types of information in DT, the smbios "entries" in DT
are not standardised in the dtschema and in most cases they're
unnecessarily replicating data ALREADY in DT which is being produced
automatically with these patches, making it zero effort for vendors to
produce.

> transformation into SMBIOS really should just be used for user
> display, etc., not for anything which affects operation of the device.

Well SMBIOS tables are used for a number of different things already
in the kernel.

> Do you agree? If you do, how to ensure that? Perhaps a special string
> in the table like 'GENERATED'?

Nope, I do not agree, at all.

Regards,
Peter

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

[Ilias Apalodimas]

Hi Simon,

The discussion is mostly on v3 now, so I assume this is outdated?

Thanks
/Ilias
On Mon, 18 Dec 2023 at 17:02, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Ilias,
>
> On Wed, 6 Dec 2023 at 04:36, Ilias Apalodimas
> <ilias.apalodimas@linaro.org> wrote:
> >
> > [...]
> >
> > >
> > >>
> > >> >                 str = "Unknown";
> > >> >
> > >> >         for (;;) {
> > >> > @@ -151,8 +151,7 @@ static int smbios_add_prop_si(struct smbios_ctx *ctx, const char *prop,
> > >> >                 const char *str;
> > >> >
> > >> >                 str = ofnode_read_string(ctx->node, prop);
> > >> > -               if (str)
> > >> > -                       return smbios_add_string(ctx, str);
> > >> > +               return smbios_add_string(ctx, str);
> > >> >         }
> > >> >
> > >> >         return 0;
> > >> > @@ -231,7 +230,7 @@ static int smbios_write_type0(ulong *current, int handle,
> > >> >         t->vendor = smbios_add_string(ctx, "U-Boot");
> > >> >
> > >> >         t->bios_ver = smbios_add_prop(ctx, "version");
> > >> > -       if (!t->bios_ver)
> > >> > +       if (!strcmp(ctx->last_str, "Unknown"))
> > >>
> > >> That is really ugly...checking the ctx value looking for a side effect.
> > >>
> > >> Can you not have smbios_add_prop() continue to return NULL in this case?
> > >
> > >
> > > Hmm I don't know, but I wonder why I am not just checking t->bios_ver for Unknown.
> > > I'll have a look and change it
> >
> > Ok, this can't be changed as easily.  smbios_add_prop() will not
> > return NULL in any case. It returns an integer. With the cleanup, it
> > will always writes 'Uknown' and not return 0 anymore.
> > I can add that default value you suggested but the ctx->last_str is
> > still used on the next line anyway.
>
> Would you mind if I tried to create a version of the patch which does
> the same thing but with less code, and perhaps a test? It might be
> easier to discuss it then. I can't claim to understand all the
> different code paths at this point.
>
> My main concern is that with so many code paths it will be hard even
> to refactor the code in the future, since it will become
> 'load-bearing' and anyone might turn up and say it breaks their board
> because different information is provided.
>
> Overall, so long as the information isn't used for anything important
> in userspace, and we find a way to report SMBIOS to Linux without EFI,
> it is OK to enable this feature (with a new Kconfig so it can be
> disabled). But there is already authoritative info in the DT, so this
> transformation into SMBIOS really should just be used for user
> display, etc., not for anything which affects operation of the device.
> Do you agree? If you do, how to ensure that? Perhaps a special string
> in the table like 'GENERATED'?
>
> Regards,
> Simon

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

[Simon Glass]

Hi Peter,

On Tue, 19 Dec 2023 at 13:23, Peter Robinson <pbrobinson@gmail.com> wrote:
>
> > > > > What do you mean wrong, exactly?
> > > >
> > > > "raspberrypi" instead of "Raspberry Pi", for example, or "friendlyarm"
> > > > instead of "FriendlyElec"
> > >
> > > Heh, well, even in the x86 world vendors can't even spell their own
> > > name consistently.
> > >
> > > > I just wonder what this information is actually used for. If it is
> > > > just for fun, that is fine, but I believe some tools do use dmidecode,
> > > > at which point it needs to be accurate and should not change later,
> > > > e.g. when someone decides to actually add the info.
> > >
> > > So the Linux kernel uses this information to apply quirks for specific
> > > machines.  But I hope that on platforms that have a device tree folks
> > > will just use the board compatible string for that purpose.
> >
> > Right.
> >
> > >
> > > Otherwise I think this information is mostly used to populate system
> > > information UIs and asset databases.
> >
> > So perhaps the devicetree should be used instead? How do these things
> > work today? And what are they, exactly?
>
> Device tree used for what? To populate the SMBIOS tables? Or to
> populate "system information UIs and asset databases."
>
> If you mean the later, that is not going to work. These platforms are
> often proprietary, and are generally remote. They have local agents
> that use dmidecode to populate things for support and management. It's
> kind of in the name "System Management". To get all these platforms
> updated to "just use devicetree" will take decades. No thanks!

Do we care about these platforms? Do we even know what they are? Do
they even run on ARM?

I found a thing called 'sos' and it does actually use the devicetree,
which is actually the correct thing to do, on platforms which use
device tree.

At this point I'm wondering what problem is being solved here? We know
we cannot pass the SMBIOS table without EFI in any case. A huge
argument in Linux with hundreds of emails results in 'no patch
applied', so far as I am aware. At the very least, that seems odd.

Just to restate my suggestions:
- Put this feature behind an #ifdef
- Allow people to add the authoritative info if they want to
- Add something to the SMBIOS info that indicates it is provisional /
auto-generated
- Figure out how to handle this when booting without EFI (or decide
that SMBIOS is not used then?)
- minor: simplify the code to just handle the two pieces of info
currently decoded; add tests

Do you agree with any or all of those?

Regards,
Simon

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

[Simon Glass]

Hi Peter,

On Tue, 19 Dec 2023 at 13:40, Peter Robinson <pbrobinson@gmail.com> wrote:
>
> Hi Simon,
>
> On Mon, Dec 18, 2023 at 3:02 PM Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Ilias,
> >
> > On Wed, 6 Dec 2023 at 04:36, Ilias Apalodimas
> > <ilias.apalodimas@linaro.org> wrote:
> > >
> > > [...]
> > >
> > > >
> > > >>
> > > >> >                 str = "Unknown";
> > > >> >
> > > >> >         for (;;) {
> > > >> > @@ -151,8 +151,7 @@ static int smbios_add_prop_si(struct smbios_ctx *ctx, const char *prop,
> > > >> >                 const char *str;
> > > >> >
> > > >> >                 str = ofnode_read_string(ctx->node, prop);
> > > >> > -               if (str)
> > > >> > -                       return smbios_add_string(ctx, str);
> > > >> > +               return smbios_add_string(ctx, str);
> > > >> >         }
> > > >> >
> > > >> >         return 0;
> > > >> > @@ -231,7 +230,7 @@ static int smbios_write_type0(ulong *current, int handle,
> > > >> >         t->vendor = smbios_add_string(ctx, "U-Boot");
> > > >> >
> > > >> >         t->bios_ver = smbios_add_prop(ctx, "version");
> > > >> > -       if (!t->bios_ver)
> > > >> > +       if (!strcmp(ctx->last_str, "Unknown"))
> > > >>
> > > >> That is really ugly...checking the ctx value looking for a side effect.
> > > >>
> > > >> Can you not have smbios_add_prop() continue to return NULL in this case?
> > > >
> > > >
> > > > Hmm I don't know, but I wonder why I am not just checking t->bios_ver for Unknown.
> > > > I'll have a look and change it
> > >
> > > Ok, this can't be changed as easily.  smbios_add_prop() will not
> > > return NULL in any case. It returns an integer. With the cleanup, it
> > > will always writes 'Uknown' and not return 0 anymore.
> > > I can add that default value you suggested but the ctx->last_str is
> > > still used on the next line anyway.
> >
> > Would you mind if I tried to create a version of the patch which does
> > the same thing but with less code, and perhaps a test? It might be
> > easier to discuss it then. I can't claim to understand all the
> > different code paths at this point.
> >
> > My main concern is that with so many code paths it will be hard even
> > to refactor the code in the future, since it will become
> > 'load-bearing' and anyone might turn up and say it breaks their board
> > because different information is provided.
>
> I don't buy this argument at all, sorry.

OK.

>
> > Overall, so long as the information isn't used for anything important
> > in userspace, and we find a way to report SMBIOS to Linux without EFI,
>
> No, you can't tie random requirements to improving the SMBIOS support.
> We *already* report SMBIOS to Linux, reporting SMBIOS to Linux without
> EFI is changing things that will need different or extra standards,
> that could take years.
>
> You are arbitrarily adding extra requirements just to suite yourself,
> please STOP trying to hold things like this hostage.

Isn't that what is happening with Linux and ffi? My understanding is
that there is no way to pass SMBIOS to Linux without EFI. Is that
correct? I know some people at their wit's end about that.

You may know that I have tried for years to get bindings upstream to
Linux....right now there is a reserved-memory binding which has been
held up for longer than I can remember, because of EFI. How about a
little give and take?

>
> > it is OK to enable this feature (with a new Kconfig so it can be
> > disabled). But there is already authoritative info in the DT, so this
>
> There is two types of information in DT, the smbios "entries" in DT
> are not standardised in the dtschema and in most cases they're
> unnecessarily replicating data ALREADY in DT which is being produced
> automatically with these patches, making it zero effort for vendors to
> produce.
>
> > transformation into SMBIOS really should just be used for user
> > display, etc., not for anything which affects operation of the device.
>
> Well SMBIOS tables are used for a number of different things already
> in the kernel.
>
> > Do you agree? If you do, how to ensure that? Perhaps a special string
> > in the table like 'GENERATED'?
>
> Nope, I do not agree, at all.

OK, well there it is.

Anyway, as I said, I am happy for this to go in with a Kconfig
controlling it (so it can be enabled/disabled). Then SoCs that don't
want to go to the bother of adding authoritative info can just enable
this Kconfig.

I would very much like to see some signal that it is not
authoritative. That should not be a big imposition.

For my own interest, I would like to understand what actually uses it
as I suspect it is just for display. The two programs I managed to
find both handle devicetree and don't need SMBIOS.

Regards,
Simon

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

[Tom Rini]

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

On Wed, Dec 20, 2023 at 10:32:26AM -0700, Simon Glass wrote:
> Hi Peter,
> 
> On Tue, 19 Dec 2023 at 13:40, Peter Robinson <pbrobinson@gmail.com> wrote:
> >
> > Hi Simon,
> >
> > On Mon, Dec 18, 2023 at 3:02 PM Simon Glass <sjg@chromium.org> wrote:
> > >
> > > Hi Ilias,
> > >
> > > On Wed, 6 Dec 2023 at 04:36, Ilias Apalodimas
> > > <ilias.apalodimas@linaro.org> wrote:
> > > >
> > > > [...]
> > > >
> > > > >
> > > > >>
> > > > >> >                 str = "Unknown";
> > > > >> >
> > > > >> >         for (;;) {
> > > > >> > @@ -151,8 +151,7 @@ static int smbios_add_prop_si(struct smbios_ctx *ctx, const char *prop,
> > > > >> >                 const char *str;
> > > > >> >
> > > > >> >                 str = ofnode_read_string(ctx->node, prop);
> > > > >> > -               if (str)
> > > > >> > -                       return smbios_add_string(ctx, str);
> > > > >> > +               return smbios_add_string(ctx, str);
> > > > >> >         }
> > > > >> >
> > > > >> >         return 0;
> > > > >> > @@ -231,7 +230,7 @@ static int smbios_write_type0(ulong *current, int handle,
> > > > >> >         t->vendor = smbios_add_string(ctx, "U-Boot");
> > > > >> >
> > > > >> >         t->bios_ver = smbios_add_prop(ctx, "version");
> > > > >> > -       if (!t->bios_ver)
> > > > >> > +       if (!strcmp(ctx->last_str, "Unknown"))
> > > > >>
> > > > >> That is really ugly...checking the ctx value looking for a side effect.
> > > > >>
> > > > >> Can you not have smbios_add_prop() continue to return NULL in this case?
> > > > >
> > > > >
> > > > > Hmm I don't know, but I wonder why I am not just checking t->bios_ver for Unknown.
> > > > > I'll have a look and change it
> > > >
> > > > Ok, this can't be changed as easily.  smbios_add_prop() will not
> > > > return NULL in any case. It returns an integer. With the cleanup, it
> > > > will always writes 'Uknown' and not return 0 anymore.
> > > > I can add that default value you suggested but the ctx->last_str is
> > > > still used on the next line anyway.
> > >
> > > Would you mind if I tried to create a version of the patch which does
> > > the same thing but with less code, and perhaps a test? It might be
> > > easier to discuss it then. I can't claim to understand all the
> > > different code paths at this point.
> > >
> > > My main concern is that with so many code paths it will be hard even
> > > to refactor the code in the future, since it will become
> > > 'load-bearing' and anyone might turn up and say it breaks their board
> > > because different information is provided.
> >
> > I don't buy this argument at all, sorry.
> 
> OK.
> 
> >
> > > Overall, so long as the information isn't used for anything important
> > > in userspace, and we find a way to report SMBIOS to Linux without EFI,
> >
> > No, you can't tie random requirements to improving the SMBIOS support.
> > We *already* report SMBIOS to Linux, reporting SMBIOS to Linux without
> > EFI is changing things that will need different or extra standards,
> > that could take years.
> >
> > You are arbitrarily adding extra requirements just to suite yourself,
> > please STOP trying to hold things like this hostage.
> 
> Isn't that what is happening with Linux and ffi? My understanding is
> that there is no way to pass SMBIOS to Linux without EFI. Is that
> correct? I know some people at their wit's end about that.
> 
> You may know that I have tried for years to get bindings upstream to
> Linux....right now there is a reserved-memory binding which has been
> held up for longer than I can remember, because of EFI. How about a
> little give and take?

You aren't actually saying that since some patches are being held up by
"but what about EFI?" you're trying to hold this up as retaliation with
"but what about without EFI?". Right? And aside, I can point you at
other people that got fed up with trying to make ARM+(EFI+ACPI) work
because the feedback was "but what about DT?".

> > > it is OK to enable this feature (with a new Kconfig so it can be
> > > disabled). But there is already authoritative info in the DT, so this
> >
> > There is two types of information in DT, the smbios "entries" in DT
> > are not standardised in the dtschema and in most cases they're
> > unnecessarily replicating data ALREADY in DT which is being produced
> > automatically with these patches, making it zero effort for vendors to
> > produce.
> >
> > > transformation into SMBIOS really should just be used for user
> > > display, etc., not for anything which affects operation of the device.
> >
> > Well SMBIOS tables are used for a number of different things already
> > in the kernel.
> >
> > > Do you agree? If you do, how to ensure that? Perhaps a special string
> > > in the table like 'GENERATED'?
> >
> > Nope, I do not agree, at all.
> 
> OK, well there it is.
> 
> Anyway, as I said, I am happy for this to go in with a Kconfig
> controlling it (so it can be enabled/disabled). Then SoCs that don't
> want to go to the bother of adding authoritative info can just enable
> this Kconfig.
> 
> I would very much like to see some signal that it is not
> authoritative. That should not be a big imposition.

Lets try a different track here. We've had 3 years now of the
u-boot,sysinfo-smbios binding now. And there's been 14 ARM platforms
that tried to fill it out. With Ilias' series, now there's a much larger
chance that someone will see values populated and want to update them,
rather than "Unknown" and assume there's no way to have them populated.

> For my own interest, I would like to understand what actually uses it
> as I suspect it is just for display. The two programs I managed to
> find both handle devicetree and don't need SMBIOS.

Please re-read the examples where people have already explained where
they're used, today.

-- 
Tom

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

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

[Peter Robinson]

Hi Simon,

> > > > > Hmm I don't know, but I wonder why I am not just checking t->bios_ver for Unknown.
> > > > > I'll have a look and change it
> > > >
> > > > Ok, this can't be changed as easily.  smbios_add_prop() will not
> > > > return NULL in any case. It returns an integer. With the cleanup, it
> > > > will always writes 'Uknown' and not return 0 anymore.
> > > > I can add that default value you suggested but the ctx->last_str is
> > > > still used on the next line anyway.
> > >
> > > Would you mind if I tried to create a version of the patch which does
> > > the same thing but with less code, and perhaps a test? It might be
> > > easier to discuss it then. I can't claim to understand all the
> > > different code paths at this point.
> > >
> > > My main concern is that with so many code paths it will be hard even
> > > to refactor the code in the future, since it will become
> > > 'load-bearing' and anyone might turn up and say it breaks their board
> > > because different information is provided.
> >
> > I don't buy this argument at all, sorry.
>
> OK.
>
> >
> > > Overall, so long as the information isn't used for anything important
> > > in userspace, and we find a way to report SMBIOS to Linux without EFI,
> >
> > No, you can't tie random requirements to improving the SMBIOS support.
> > We *already* report SMBIOS to Linux, reporting SMBIOS to Linux without
> > EFI is changing things that will need different or extra standards,
> > that could take years.
> >
> > You are arbitrarily adding extra requirements just to suite yourself,
> > please STOP trying to hold things like this hostage.
>
> Isn't that what is happening with Linux and ffi? My understanding is
> that there is no way to pass SMBIOS to Linux without EFI. Is that
> correct? I know some people at their wit's end about that.

Maybe the uses that want to go from a minimal firmware straight to a
minimal kernel don't care about SMBIOS tables for their use cases,
things don't need to be full parity to move forward the existing well
defined usecase.

> You may know that I have tried for years to get bindings upstream to
> Linux....right now there is a reserved-memory binding which has been
> held up for longer than I can remember, because of EFI. How about a
> little give and take?

I actually caught up on the reserved-memory binding thread a week or
so ago and my general thoughts from that thread was that there was a
lot of outstanding questions being asked of you that you haven't
clearly articulated or even replied to and the thread ended with you
asking a number of times "can this be merged now?" and my thought at
the time was "No, because there's a bunch of outstanding details". May
I suggest you re-read that thread and take some notes while you do so
and make sure all the outstanding questions have been answered and
reply with a single response addressing the remaining details, from
there we may be able to move on.

> >
> > > it is OK to enable this feature (with a new Kconfig so it can be
> > > disabled). But there is already authoritative info in the DT, so this
> >
> > There is two types of information in DT, the smbios "entries" in DT
> > are not standardised in the dtschema and in most cases they're
> > unnecessarily replicating data ALREADY in DT which is being produced
> > automatically with these patches, making it zero effort for vendors to
> > produce.
> >
> > > transformation into SMBIOS really should just be used for user
> > > display, etc., not for anything which affects operation of the device.
> >
> > Well SMBIOS tables are used for a number of different things already
> > in the kernel.
> >
> > > Do you agree? If you do, how to ensure that? Perhaps a special string
> > > in the table like 'GENERATED'?
> >
> > Nope, I do not agree, at all.
>
> OK, well there it is.
>
> Anyway, as I said, I am happy for this to go in with a Kconfig
> controlling it (so it can be enabled/disabled). Then SoCs that don't
> want to go to the bother of adding authoritative info can just enable
> this Kconfig.
>
> I would very much like to see some signal that it is not
> authoritative. That should not be a big imposition.
>
> For my own interest, I would like to understand what actually uses it
> as I suspect it is just for display. The two programs I managed to
> find both handle devicetree and don't need SMBIOS.
>
> Regards,
> Simon

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

[neil.armstrong]

On 27/11/2023 18:10, Ilias Apalodimas wrote:
> Hi,
> Tom asked me to clean up and resend [0].
> I've made some adjustments -- The DT nodes are tokenized and a single
> string is used. The DT node 'model' is used to fill the SMBIOS
> 'Product Name' and 'compatible' is used for 'Manufacturer'.
> 
> The reason is explained in patch#2 but the tl;dr is that model doesn't
> always include the manufacturer despite the suggestions of the DT spec.
> 
> [0] https://lore.kernel.org/u-boot/20220906134426.53748-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 | 109 +++++++++++++++++++++++++++++++++++++++++++--------
>   1 file changed, 92 insertions(+), 17 deletions(-)
> 
> --
> 2.40.1
> 

I did it already offline, but:

Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>
Tested-by: Neil Armstrong <neil.armstrong@linaro.org> # on libretech-cc

Thanks,
Neil

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

[Ilias Apalodimas]

On Thu, 21 Dec 2023 at 11:18, <neil.armstrong@linaro.org> wrote:
>
> On 27/11/2023 18:10, Ilias Apalodimas wrote:
> > Hi,
> > Tom asked me to clean up and resend [0].
> > I've made some adjustments -- The DT nodes are tokenized and a single
> > string is used. The DT node 'model' is used to fill the SMBIOS
> > 'Product Name' and 'compatible' is used for 'Manufacturer'.
> >
> > The reason is explained in patch#2 but the tl;dr is that model doesn't
> > always include the manufacturer despite the suggestions of the DT spec.
> >
> > [0] https://lore.kernel.org/u-boot/20220906134426.53748-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 | 109 +++++++++++++++++++++++++++++++++++++++++++--------
> >   1 file changed, 92 insertions(+), 17 deletions(-)
> >
> > --
> > 2.40.1
> >
>
> I did it already offline, but:
>
> Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>
> Tested-by: Neil Armstrong <neil.armstrong@linaro.org> # on libretech-cc
>
> Thanks,
> Neil

Thanks for testing Neil!
/Ilias

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

[Simon Glass]

Hi Peter,

On Thu, Dec 21, 2023 at 9:13 AM Peter Robinson <pbrobinson@gmail.com> wrote:
>
> Hi Simon,
>
> > > > > > Hmm I don't know, but I wonder why I am not just checking t->bios_ver for Unknown.
> > > > > > I'll have a look and change it
> > > > >
> > > > > Ok, this can't be changed as easily.  smbios_add_prop() will not
> > > > > return NULL in any case. It returns an integer. With the cleanup, it
> > > > > will always writes 'Uknown' and not return 0 anymore.
> > > > > I can add that default value you suggested but the ctx->last_str is
> > > > > still used on the next line anyway.
> > > >
> > > > Would you mind if I tried to create a version of the patch which does
> > > > the same thing but with less code, and perhaps a test? It might be
> > > > easier to discuss it then. I can't claim to understand all the
> > > > different code paths at this point.
> > > >
> > > > My main concern is that with so many code paths it will be hard even
> > > > to refactor the code in the future, since it will become
> > > > 'load-bearing' and anyone might turn up and say it breaks their board
> > > > because different information is provided.
> > >
> > > I don't buy this argument at all, sorry.
> >
> > OK.
> >
> > >
> > > > Overall, so long as the information isn't used for anything important
> > > > in userspace, and we find a way to report SMBIOS to Linux without EFI,
> > >
> > > No, you can't tie random requirements to improving the SMBIOS support.
> > > We *already* report SMBIOS to Linux, reporting SMBIOS to Linux without
> > > EFI is changing things that will need different or extra standards,
> > > that could take years.
> > >
> > > You are arbitrarily adding extra requirements just to suite yourself,
> > > please STOP trying to hold things like this hostage.
> >
> > Isn't that what is happening with Linux and ffi? My understanding is
> > that there is no way to pass SMBIOS to Linux without EFI. Is that
> > correct? I know some people at their wit's end about that.
>
> Maybe the uses that want to go from a minimal firmware straight to a
> minimal kernel don't care about SMBIOS tables for their use cases,
> things don't need to be full parity to move forward the existing well
> defined usecase.

Yes, maybe.

>
> > You may know that I have tried for years to get bindings upstream to
> > Linux....right now there is a reserved-memory binding which has been
> > held up for longer than I can remember, because of EFI. How about a
> > little give and take?
>
> I actually caught up on the reserved-memory binding thread a week or
> so ago and my general thoughts from that thread was that there was a
> lot of outstanding questions being asked of you that you haven't
> clearly articulated or even replied to and the thread ended with you
> asking a number of times "can this be merged now?" and my thought at
> the time was "No, because there's a bunch of outstanding details". May
> I suggest you re-read that thread and take some notes while you do so
> and make sure all the outstanding questions have been answered and
> reply with a single response addressing the remaining details, from
> there we may be able to move on.

Note that I am just the facilitator for the binding. I volunteered to
send it since I have some knowledge and experience with devicetree. I
believe (from reading the thread) that at least Rob understood the use
case, but has no interest in it. At one point he asked for buy-in from
Tianocore/EDK2 people - they were already on cc but then started
trying to answer the questions.

Also note that it has been 3 months since v7 was sent. I have just
gone through the thread again. I don't see a "bunch of outstanding
details". I do see a some EDK2-specific questions, if that is what you
mean, but I don't know enough to answer those. The deep discussion of
EDK2-specific implementation details worries me, actually, since the
whole point of UPL is to be able to swap out the Platform Init and
Payload.

Conceptually the binding is simple...if we need another node with a
phandle we can do that. But why is it getting so stuck in the weeds?

If you think you can help on that thread, please do.

[..]

Regards,
Simon