| Message ID | 20231207091850.17776-3-ilias.apalodimas@linaro.org |
|---|---|
| State | Accepted |
| Commit | 738b34668f289ba98e3657f86e3a58385c09059d |
| Headers | show |
| Series | Provide a fallback to smbios tables | expand |
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
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;
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