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

In order to fill in the SMBIOS tables U-Boot currently relies on a
"u-boot,sysinfo-smbios" compatible node.  This is fine for the boards
that already include such nodes.  However with some recent EFI changes,
the majority of boards can boot up distros, which usually rely on
things like dmidecode etc for their reporting.  For boards that
lack this special node the SMBIOS output looks like:

System Information
        Manufacturer: Unknown
        Product Name: Unknown
        Version: Unknown
        Serial Number: Unknown
        UUID: Not Settable
        Wake-up Type: Reserved
        SKU Number: Unknown
        Family: Unknown

This looks problematic since most of the info are "Unknown".  The DT spec
specifies standard properties containing relevant information like
'model' and 'compatible' for which the suggested format is
<manufacturer,model>. Unfortunately the 'model' string found in DTs is
usually lacking the manufacturer so we can't use it for both
'Manufacturer' and 'Product Name' SMBIOS entries reliably.

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

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

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

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

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

--
2.40.1

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

Hi Ilias,

On Thu, 7 Dec 2023 at 02:19, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> In order to fill in the SMBIOS tables U-Boot currently relies on a
> "u-boot,sysinfo-smbios" compatible node.  This is fine for the boards
> that already include such nodes.  However with some recent EFI changes,
> the majority of boards can boot up distros, which usually rely on
> things like dmidecode etc for their reporting.  For boards that
> lack this special node the SMBIOS output looks like:
>
> System Information
>         Manufacturer: Unknown
>         Product Name: Unknown
>         Version: Unknown
>         Serial Number: Unknown
>         UUID: Not Settable
>         Wake-up Type: Reserved
>         SKU Number: Unknown
>         Family: Unknown
>
> This looks problematic since most of the info are "Unknown".  The DT spec
> specifies standard properties containing relevant information like
> 'model' and 'compatible' for which the suggested format is
> <manufacturer,model>. Unfortunately the 'model' string found in DTs is
> usually lacking the manufacturer so we can't use it for both
> 'Manufacturer' and 'Product Name' SMBIOS entries reliably.
>
> So let's add a last resort to our current smbios parsing.  If none of
> the sysinfo properties are found, scan for those information in the
> root node of the device tree. Use the 'model' to fill the 'Product
> Name' and the first value of 'compatible' for the 'Manufacturer', since
> that always contains one.
>
> pre-patch:
> Handle 0x0001, DMI type 1, 27 bytes
> System Information
>         Manufacturer: Unknown
>         Product Name: Unknown
>         Version: Unknown
>         Serial Number: 100000000bb24ceb
>         UUID: 30303031-3030-3030-3061-613234636435
>         Wake-up Type: Reserved
>         SKU Number: Unknown
>         Family: Unknown
> [...]
>
> and post patch:
> Handle 0x0001, DMI type 1, 27 bytes
> System Information
>         Manufacturer: raspberrypi
>         Product Name: Raspberry Pi 4 Model B Rev 1.1
>         Version: Unknown
>         Serial Number: 100000000bb24ceb
>         UUID: 30303031-3030-3030-3061-613234636435
>         Wake-up Type: Reserved
>         SKU Number: Unknown
>         Family: Unknown
> [...]
>
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> ---
> Simon, I'll work with tou on the refactoring you wanted and
> remove the EFI requirement of SMBIOS in !x86 platforms.
> Since this code has no tests, I'll add some once the refactoring
> is done
>
> Changes since v2:
> - Spelling mistakes
> - rebase on top of patch #1 changes
> Changes since v1:
> - Tokenize the DT node entry and use the appropriate value instead of
>   the entire string
> - Removed Peters tested/reviewed-by tags due to the above
>  lib/smbios.c | 94 +++++++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 89 insertions(+), 5 deletions(-)

Can we add a Kconfig to enable this?

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

must be >0

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

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

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

s/and/then/ ?

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

can't you drop the case?

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

Regards,
Simon

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

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

Hi Tom,

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

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

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

Regards,
Simon

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

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

Hi Simon,

[...]

>
> Can we add a Kconfig to enable this?

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

> > + * @size: str buffer length

[...]

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

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

[...]

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

Sure

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

What case?

[...]

Thanks
/Ilias

Hi Ilias,

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

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

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

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

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

Sorry, I meant 'cast'.

Regards,
Simon

Hi Tom,

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

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

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

Regards,
Simon

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

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

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

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

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

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

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

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

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

Hi Tom,

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

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

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

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

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

OK, but in my mind this is a concern.

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

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

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

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

Regards,
Simon

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

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

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

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

Hi Tom,

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

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

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

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

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

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

Regards,
Simon

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

Perhaps.

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

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

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

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

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

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

Neil

Hi Neil,

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

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

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

Regards,
Simon

Hi,

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

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

Neil

> 
> Regards,
> Simon

Hi Neil,

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

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

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

Here is my main question: is SMBIOS:

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

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

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

Regards,
Simon

Hi again,

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

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

Regards,
Simon

Hi Simon,

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

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

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

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

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

Hi Ilias,

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

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

Regards,
Simon

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

Hi Simon,

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

It's described in the commit message

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

Hi Peter,

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

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

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

Regards,
Simon