| Message ID | 20231127171058.165777-3-ilias.apalodimas@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Provide a fallback to smbios tables | expand |
Hi Ilias, On Mon, 27 Nov 2023 at 10:11, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > In order to fill in the SMBIOS tables U-Boot currently relies on a > "u-boot,sysinfo-smbios" compatible node. This is fine for the boards > that already include such nodes. However with some recent EFI changes, > the majority of boards can boot up distros, which usually rely on > things like dmidecode etc for their reporting. For boards that > lack this special node the SMBIOS output looks like: > > System Information > Manufacturer: Unknown > Product Name: Unknown > Version: Unknown > Serial Number: Unknown > UUID: Not Settable > Wake-up Type: Reserved > SKU Number: Unknown > Family: Unknown > > This looks problematic since most of the info are "Unknown". The DT spec > specifies standard properties containing relevant information like > 'model' and 'compatible' for which the suggested format is > <manufacturer,model>. Unfortunately the 'model' string found in DTs is > usually lacking the manufacturer so we can't use it for both > 'Manufacturer' and 'Product Name' SMBIOS entries reliably. > > So let's add a last resort to our current smbios parsing. If none of > the sysinfo properties are found, scan for those information in the > root node of the device tree. Use the 'model' to fill the 'Product > Name' and the first value of 'compatible' for the 'Manufacturer', since > that always contains one. > > pre-patch: > Handle 0x0001, DMI type 1, 27 bytes > System Information > Manufacturer: Unknown > Product Name: Unknown > Version: Unknown > Serial Number: 100000000bb24ceb > UUID: 30303031-3030-3030-3061-613234636435 > Wake-up Type: Reserved > SKU Number: Unknown > Family: Unknown > [...] > > and post patch: > Handle 0x0001, DMI type 1, 27 bytes > System Information > Manufacturer: raspberrypi > Product Name: Raspberry Pi 4 Model B Rev 1.1 > Version: Unknown > Serial Number: 100000000bb24ceb > UUID: 30303031-3030-3030-3061-613234636435 > Wake-up Type: Reserved > SKU Number: Unknown > Family: Unknown > [...] > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > --- > Changes since v1: > - Tokenize the DT node entry and use the appropriate value instead of > the entire string > - Removed Peters tested/reviewed-by tags due to the above > lib/smbios.c | 94 +++++++++++++++++++++++++++++++++++++++++++++++++--- > 1 file changed, 90 insertions(+), 4 deletions(-) > Can this be put behind a Kconfig? It adds quite a bit of code which punishes those boards which do the right thing. > diff --git a/lib/smbios.c b/lib/smbios.c > index fcc8686993ef..423893639ff0 100644 > --- a/lib/smbios.c > +++ b/lib/smbios.c > @@ -9,11 +9,14 @@ > #include <dm.h> > #include <env.h> > #include <linux/stringify.h> > +#include <linux/string.h> > #include <mapmem.h> > #include <smbios.h> > #include <sysinfo.h> > #include <tables_csum.h> > #include <version.h> > +#include <malloc.h> > +#include <dm/ofnode.h> > #ifdef CONFIG_CPU > #include <cpu.h> > #include <dm/uclass-internal.h> > @@ -43,6 +46,25 @@ > > DECLARE_GLOBAL_DATA_PTR; > > +/** > + * struct map_sysinfo - Mapping of sysinfo strings to DT > + * > + * @sysinfo_str: sysinfo string > + * @dt_str: DT string > + * @max: Max index of the tokenized string to pick. Counting starts from 0 > + * > + */ > +struct map_sysinfo { > + const char *sysinfo_str; > + const char *dt_str; > + int max; > +}; > + > +static const struct map_sysinfo sysinfo_to_dt[] = { > + { .sysinfo_str = "product", .dt_str = "model", 2 }, > + { .sysinfo_str = "manufacturer", .dt_str = "compatible", 1 }, > +}; > + > /** > * struct smbios_ctx - context for writing SMBIOS tables > * > @@ -87,6 +109,18 @@ struct smbios_write_method { > const char *subnode_name; > }; > > +static const struct map_sysinfo *convert_sysinfo_to_dt(const char *sysinfo_str) > +{ > + int i; > + > + for (i = 0; i < ARRAY_SIZE(sysinfo_to_dt); i++) { > + if (!strcmp(sysinfo_str, sysinfo_to_dt[i].sysinfo_str)) > + return &sysinfo_to_dt[i]; > + } > + > + return NULL; > +} > + > /** > * smbios_add_string() - add a string to the string area > * > @@ -127,6 +161,42 @@ static int smbios_add_string(struct smbios_ctx *ctx, const char *str) > } > } > > +/** > + * get_str_from_dt - Get a substring from a DT property. > + * After finding the property in the DT, the function > + * will parse comma separted values and return the value. spelling > + * If nprop->max exceeds the number of comma separated comma-separated > + * elements the last non NULL value will be returned. elements, the > + * Counting starts from zero. > + * > + * @nprop: sysinfo property to use > + * @str: pointer to fill with data > + * @size: str buffer length > + */ > +static > +void get_str_from_dt(const struct map_sysinfo *nprop, char *str, size_t size) > +{ > + const char *dt_str; > + int cnt = 0; > + char *token; > + > + memset(str, 0, size); > + if (!nprop || !nprop->max) > + return; > + > + dt_str = ofnode_read_string(ofnode_root(), nprop->dt_str); Could this use ofnode_read_string_index() ? > + if (!dt_str) > + return; > + > + memcpy(str, dt_str, size); > + token = strtok(str, ","); > + while (token && cnt < nprop->max) { > + strlcpy(str, token, strlen(token) + 1); > + token = strtok(NULL, ","); > + cnt++; > + } > +} > + > /** > * smbios_add_prop_si() - Add a property from the devicetree or sysinfo > * > @@ -139,19 +209,35 @@ static int smbios_add_string(struct smbios_ctx *ctx, const char *str) > static int smbios_add_prop_si(struct smbios_ctx *ctx, const char *prop, > int sysinfo_id) > { > + int ret = 0; > + > if (sysinfo_id && ctx->dev) { > char val[SMBIOS_STR_MAX]; > - int ret; > > ret = sysinfo_get_str(ctx->dev, sysinfo_id, sizeof(val), val); > if (!ret) > return smbios_add_string(ctx, val); > } > + > if (IS_ENABLED(CONFIG_OF_CONTROL)) { > - const char *str; > + const char *str = NULL; > + char str_dt[128] = { 0 }; > + /* > + * If the node is not valid fallback and try the entire DT > + * so we can at least fill in maufacturer and board type spelling > + */ > + if (ofnode_valid(ctx->node)) { > + str = ofnode_read_string(ctx->node, prop); > + } else { > + const struct map_sysinfo *nprop; > + > + nprop = convert_sysinfo_to_dt(prop); > + get_str_from_dt(nprop, str_dt, sizeof(str_dt)); > + } > > - str = ofnode_read_string(ctx->node, prop); > - return smbios_add_string(ctx, str); > + ret = smbios_add_string(ctx, ofnode_valid(ctx->node) ? > + str : (const char *)str_dt); > + return ret; > } > > return 0; > -- > 2.40.1 > Any chance of a test for this code? Regards, Simon
Hi Simon, [...] > Changes since v1: > > - Tokenize the DT node entry and use the appropriate value instead of > > the entire string > > - Removed Peters tested/reviewed-by tags due to the above > > lib/smbios.c | 94 +++++++++++++++++++++++++++++++++++++++++++++++++--- > > 1 file changed, 90 insertions(+), 4 deletions(-) > > > > Can this be put behind a Kconfig? It adds quite a bit of code which > punishes those boards which do the right thing. > Sure but OTOH the code increase should be really minimal. But I don't mind I can add a Kconfig > > + > > + dt_str = ofnode_read_string(ofnode_root(), nprop->dt_str); > > Could this use ofnode_read_string_index() ? > Maybe, I'll have a look and change it if that works [...] > Any chance of a test for this code? > Sure, but any suggestions on where to add the test? SMBIOS tables are populated on OS booting, do we have a test somewhere that boots an OS? Any other ideas? Thanks /Ilias > > Regards, > Simon >
Hi Ilias, On Wed, 29 Nov 2023 at 23:50, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > Hi Simon, > > [...] > >> > Changes since v1: >> > - Tokenize the DT node entry and use the appropriate value instead of >> > the entire string >> > - Removed Peters tested/reviewed-by tags due to the above >> > lib/smbios.c | 94 +++++++++++++++++++++++++++++++++++++++++++++++++--- >> > 1 file changed, 90 insertions(+), 4 deletions(-) >> > >> >> Can this be put behind a Kconfig? It adds quite a bit of code which >> punishes those boards which do the right thing. > > > Sure but OTOH the code increase should be really minimal. But I don't mind I can add a Kconfig > >> >> > + >> > + dt_str = ofnode_read_string(ofnode_root(), nprop->dt_str); >> >> Could this use ofnode_read_string_index() ? > > > Maybe, I'll have a look and change it if that works > > [...] > >> >> Any chance of a test for this code? > > > Sure, but any suggestions on where to add the test? > SMBIOS tables are populated on OS booting, do we have a test somewhere that boots an OS? They are written on startup, right? They should certainly be in place before U-Boot enters the command line. > Any other ideas? Probably a test in test/lib/ would work. We actually have smbios-parser.c which might help? Regards, Simon
On Mon, Nov 27, 2023 at 5:11 PM Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > In order to fill in the SMBIOS tables U-Boot currently relies on a > "u-boot,sysinfo-smbios" compatible node. This is fine for the boards > that already include such nodes. However with some recent EFI changes, > the majority of boards can boot up distros, which usually rely on > things like dmidecode etc for their reporting. For boards that > lack this special node the SMBIOS output looks like: > > System Information > Manufacturer: Unknown > Product Name: Unknown > Version: Unknown > Serial Number: Unknown > UUID: Not Settable > Wake-up Type: Reserved > SKU Number: Unknown > Family: Unknown > > This looks problematic since most of the info are "Unknown". The DT spec > specifies standard properties containing relevant information like > 'model' and 'compatible' for which the suggested format is > <manufacturer,model>. Unfortunately the 'model' string found in DTs is > usually lacking the manufacturer so we can't use it for both > 'Manufacturer' and 'Product Name' SMBIOS entries reliably. > > So let's add a last resort to our current smbios parsing. If none of > the sysinfo properties are found, scan for those information in the > root node of the device tree. Use the 'model' to fill the 'Product > Name' and the first value of 'compatible' for the 'Manufacturer', since > that always contains one. > > pre-patch: > Handle 0x0001, DMI type 1, 27 bytes > System Information > Manufacturer: Unknown > Product Name: Unknown > Version: Unknown > Serial Number: 100000000bb24ceb > UUID: 30303031-3030-3030-3061-613234636435 > Wake-up Type: Reserved > SKU Number: Unknown > Family: Unknown > [...] > > and post patch: > Handle 0x0001, DMI type 1, 27 bytes > System Information > Manufacturer: raspberrypi > Product Name: Raspberry Pi 4 Model B Rev 1.1 > Version: Unknown > Serial Number: 100000000bb24ceb > UUID: 30303031-3030-3030-3061-613234636435 > Wake-up Type: Reserved > SKU Number: Unknown > Family: Unknown > [...] > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> Reviewed-by: Peter Robinson <pbrobinson@gmail.com> Tested-by: Peter Robinson <pbrobinson@gmail.com> > --- > Changes since v1: > - Tokenize the DT node entry and use the appropriate value instead of > the entire string > - Removed Peters tested/reviewed-by tags due to the above > lib/smbios.c | 94 +++++++++++++++++++++++++++++++++++++++++++++++++--- > 1 file changed, 90 insertions(+), 4 deletions(-) > > diff --git a/lib/smbios.c b/lib/smbios.c > index fcc8686993ef..423893639ff0 100644 > --- a/lib/smbios.c > +++ b/lib/smbios.c > @@ -9,11 +9,14 @@ > #include <dm.h> > #include <env.h> > #include <linux/stringify.h> > +#include <linux/string.h> > #include <mapmem.h> > #include <smbios.h> > #include <sysinfo.h> > #include <tables_csum.h> > #include <version.h> > +#include <malloc.h> > +#include <dm/ofnode.h> > #ifdef CONFIG_CPU > #include <cpu.h> > #include <dm/uclass-internal.h> > @@ -43,6 +46,25 @@ > > DECLARE_GLOBAL_DATA_PTR; > > +/** > + * struct map_sysinfo - Mapping of sysinfo strings to DT > + * > + * @sysinfo_str: sysinfo string > + * @dt_str: DT string > + * @max: Max index of the tokenized string to pick. Counting starts from 0 > + * > + */ > +struct map_sysinfo { > + const char *sysinfo_str; > + const char *dt_str; > + int max; > +}; > + > +static const struct map_sysinfo sysinfo_to_dt[] = { > + { .sysinfo_str = "product", .dt_str = "model", 2 }, > + { .sysinfo_str = "manufacturer", .dt_str = "compatible", 1 }, > +}; > + > /** > * struct smbios_ctx - context for writing SMBIOS tables > * > @@ -87,6 +109,18 @@ struct smbios_write_method { > const char *subnode_name; > }; > > +static const struct map_sysinfo *convert_sysinfo_to_dt(const char *sysinfo_str) > +{ > + int i; > + > + for (i = 0; i < ARRAY_SIZE(sysinfo_to_dt); i++) { > + if (!strcmp(sysinfo_str, sysinfo_to_dt[i].sysinfo_str)) > + return &sysinfo_to_dt[i]; > + } > + > + return NULL; > +} > + > /** > * smbios_add_string() - add a string to the string area > * > @@ -127,6 +161,42 @@ static int smbios_add_string(struct smbios_ctx *ctx, const char *str) > } > } > > +/** > + * get_str_from_dt - Get a substring from a DT property. > + * After finding the property in the DT, the function > + * will parse comma separted values and return the value. > + * If nprop->max exceeds the number of comma separated > + * elements the last non NULL value will be returned. > + * Counting starts from zero. > + * > + * @nprop: sysinfo property to use > + * @str: pointer to fill with data > + * @size: str buffer length > + */ > +static > +void get_str_from_dt(const struct map_sysinfo *nprop, char *str, size_t size) > +{ > + const char *dt_str; > + int cnt = 0; > + char *token; > + > + memset(str, 0, size); > + if (!nprop || !nprop->max) > + return; > + > + dt_str = ofnode_read_string(ofnode_root(), nprop->dt_str); > + if (!dt_str) > + return; > + > + memcpy(str, dt_str, size); > + token = strtok(str, ","); > + while (token && cnt < nprop->max) { > + strlcpy(str, token, strlen(token) + 1); > + token = strtok(NULL, ","); > + cnt++; > + } > +} > + > /** > * smbios_add_prop_si() - Add a property from the devicetree or sysinfo > * > @@ -139,19 +209,35 @@ static int smbios_add_string(struct smbios_ctx *ctx, const char *str) > static int smbios_add_prop_si(struct smbios_ctx *ctx, const char *prop, > int sysinfo_id) > { > + int ret = 0; > + > if (sysinfo_id && ctx->dev) { > char val[SMBIOS_STR_MAX]; > - int ret; > > ret = sysinfo_get_str(ctx->dev, sysinfo_id, sizeof(val), val); > if (!ret) > return smbios_add_string(ctx, val); > } > + > if (IS_ENABLED(CONFIG_OF_CONTROL)) { > - const char *str; > + const char *str = NULL; > + char str_dt[128] = { 0 }; > + /* > + * If the node is not valid fallback and try the entire DT > + * so we can at least fill in maufacturer and board type > + */ > + if (ofnode_valid(ctx->node)) { > + str = ofnode_read_string(ctx->node, prop); > + } else { > + const struct map_sysinfo *nprop; > + > + nprop = convert_sysinfo_to_dt(prop); > + get_str_from_dt(nprop, str_dt, sizeof(str_dt)); > + } > > - str = ofnode_read_string(ctx->node, prop); > - return smbios_add_string(ctx, str); > + ret = smbios_add_string(ctx, ofnode_valid(ctx->node) ? > + str : (const char *)str_dt); > + return ret; > } > > return 0; > -- > 2.40.1 >
On Thu, Nov 30, 2023 at 2:45 AM Simon Glass <sjg@chromium.org> wrote: > > Hi Ilias, > > On Mon, 27 Nov 2023 at 10:11, Ilias Apalodimas > <ilias.apalodimas@linaro.org> wrote: > > > > In order to fill in the SMBIOS tables U-Boot currently relies on a > > "u-boot,sysinfo-smbios" compatible node. This is fine for the boards > > that already include such nodes. However with some recent EFI changes, > > the majority of boards can boot up distros, which usually rely on > > things like dmidecode etc for their reporting. For boards that > > lack this special node the SMBIOS output looks like: > > > > System Information > > Manufacturer: Unknown > > Product Name: Unknown > > Version: Unknown > > Serial Number: Unknown > > UUID: Not Settable > > Wake-up Type: Reserved > > SKU Number: Unknown > > Family: Unknown > > > > This looks problematic since most of the info are "Unknown". The DT spec > > specifies standard properties containing relevant information like > > 'model' and 'compatible' for which the suggested format is > > <manufacturer,model>. Unfortunately the 'model' string found in DTs is > > usually lacking the manufacturer so we can't use it for both > > 'Manufacturer' and 'Product Name' SMBIOS entries reliably. > > > > So let's add a last resort to our current smbios parsing. If none of > > the sysinfo properties are found, scan for those information in the > > root node of the device tree. Use the 'model' to fill the 'Product > > Name' and the first value of 'compatible' for the 'Manufacturer', since > > that always contains one. > > > > pre-patch: > > Handle 0x0001, DMI type 1, 27 bytes > > System Information > > Manufacturer: Unknown > > Product Name: Unknown > > Version: Unknown > > Serial Number: 100000000bb24ceb > > UUID: 30303031-3030-3030-3061-613234636435 > > Wake-up Type: Reserved > > SKU Number: Unknown > > Family: Unknown > > [...] > > > > and post patch: > > Handle 0x0001, DMI type 1, 27 bytes > > System Information > > Manufacturer: raspberrypi > > Product Name: Raspberry Pi 4 Model B Rev 1.1 > > Version: Unknown > > Serial Number: 100000000bb24ceb > > UUID: 30303031-3030-3030-3061-613234636435 > > Wake-up Type: Reserved > > SKU Number: Unknown > > Family: Unknown > > [...] > > > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > > --- > > Changes since v1: > > - Tokenize the DT node entry and use the appropriate value instead of > > the entire string > > - Removed Peters tested/reviewed-by tags due to the above > > lib/smbios.c | 94 +++++++++++++++++++++++++++++++++++++++++++++++++--- > > 1 file changed, 90 insertions(+), 4 deletions(-) > > > > Can this be put behind a Kconfig? It adds quite a bit of code which > punishes those boards which do the right thing. What's the size difference? By putting it behind a Kconfig you're assuming the boards that don't do the right thing are going to enable this at which point we punish users by not having some level of usability. By "right thing" do you mean the smbios entries in the -u-boot.dtsi? > > diff --git a/lib/smbios.c b/lib/smbios.c > > index fcc8686993ef..423893639ff0 100644 > > --- a/lib/smbios.c > > +++ b/lib/smbios.c > > @@ -9,11 +9,14 @@ > > #include <dm.h> > > #include <env.h> > > #include <linux/stringify.h> > > +#include <linux/string.h> > > #include <mapmem.h> > > #include <smbios.h> > > #include <sysinfo.h> > > #include <tables_csum.h> > > #include <version.h> > > +#include <malloc.h> > > +#include <dm/ofnode.h> > > #ifdef CONFIG_CPU > > #include <cpu.h> > > #include <dm/uclass-internal.h> > > @@ -43,6 +46,25 @@ > > > > DECLARE_GLOBAL_DATA_PTR; > > > > +/** > > + * struct map_sysinfo - Mapping of sysinfo strings to DT > > + * > > + * @sysinfo_str: sysinfo string > > + * @dt_str: DT string > > + * @max: Max index of the tokenized string to pick. Counting starts from 0 > > + * > > + */ > > +struct map_sysinfo { > > + const char *sysinfo_str; > > + const char *dt_str; > > + int max; > > +}; > > + > > +static const struct map_sysinfo sysinfo_to_dt[] = { > > + { .sysinfo_str = "product", .dt_str = "model", 2 }, > > + { .sysinfo_str = "manufacturer", .dt_str = "compatible", 1 }, > > +}; > > + > > /** > > * struct smbios_ctx - context for writing SMBIOS tables > > * > > @@ -87,6 +109,18 @@ struct smbios_write_method { > > const char *subnode_name; > > }; > > > > +static const struct map_sysinfo *convert_sysinfo_to_dt(const char *sysinfo_str) > > +{ > > + int i; > > + > > + for (i = 0; i < ARRAY_SIZE(sysinfo_to_dt); i++) { > > + if (!strcmp(sysinfo_str, sysinfo_to_dt[i].sysinfo_str)) > > + return &sysinfo_to_dt[i]; > > + } > > + > > + return NULL; > > +} > > + > > /** > > * smbios_add_string() - add a string to the string area > > * > > @@ -127,6 +161,42 @@ static int smbios_add_string(struct smbios_ctx *ctx, const char *str) > > } > > } > > > > +/** > > + * get_str_from_dt - Get a substring from a DT property. > > + * After finding the property in the DT, the function > > + * will parse comma separted values and return the value. > > spelling > > > + * If nprop->max exceeds the number of comma separated > > comma-separated > > > + * elements the last non NULL value will be returned. > > elements, the > > > + * Counting starts from zero. > > + * > > + * @nprop: sysinfo property to use > > + * @str: pointer to fill with data > > + * @size: str buffer length > > + */ > > +static > > +void get_str_from_dt(const struct map_sysinfo *nprop, char *str, size_t size) > > +{ > > + const char *dt_str; > > + int cnt = 0; > > + char *token; > > + > > + memset(str, 0, size); > > + if (!nprop || !nprop->max) > > + return; > > + > > + dt_str = ofnode_read_string(ofnode_root(), nprop->dt_str); > > Could this use ofnode_read_string_index() ? > > > + if (!dt_str) > > + return; > > + > > + memcpy(str, dt_str, size); > > + token = strtok(str, ","); > > + while (token && cnt < nprop->max) { > > + strlcpy(str, token, strlen(token) + 1); > > + token = strtok(NULL, ","); > > + cnt++; > > + } > > +} > > + > > /** > > * smbios_add_prop_si() - Add a property from the devicetree or sysinfo > > * > > @@ -139,19 +209,35 @@ static int smbios_add_string(struct smbios_ctx *ctx, const char *str) > > static int smbios_add_prop_si(struct smbios_ctx *ctx, const char *prop, > > int sysinfo_id) > > { > > + int ret = 0; > > + > > if (sysinfo_id && ctx->dev) { > > char val[SMBIOS_STR_MAX]; > > - int ret; > > > > ret = sysinfo_get_str(ctx->dev, sysinfo_id, sizeof(val), val); > > if (!ret) > > return smbios_add_string(ctx, val); > > } > > + > > if (IS_ENABLED(CONFIG_OF_CONTROL)) { > > - const char *str; > > + const char *str = NULL; > > + char str_dt[128] = { 0 }; > > + /* > > + * If the node is not valid fallback and try the entire DT > > + * so we can at least fill in maufacturer and board type > > spelling > > > + */ > > + if (ofnode_valid(ctx->node)) { > > + str = ofnode_read_string(ctx->node, prop); > > + } else { > > + const struct map_sysinfo *nprop; > > + > > + nprop = convert_sysinfo_to_dt(prop); > > + get_str_from_dt(nprop, str_dt, sizeof(str_dt)); > > + } > > > > - str = ofnode_read_string(ctx->node, prop); > > - return smbios_add_string(ctx, str); > > + ret = smbios_add_string(ctx, ofnode_valid(ctx->node) ? > > + str : (const char *)str_dt); > > + return ret; > > } > > > > return 0; > > -- > > 2.40.1 > > > > Any chance of a test for this code? > > Regards, > Simon
Hi Simon, On Sat, 2 Dec 2023 at 20:28, Simon Glass <sjg@chromium.org> wrote: > > Hi Ilias, > > On Wed, 29 Nov 2023 at 23:50, Ilias Apalodimas > <ilias.apalodimas@linaro.org> wrote: > > > > Hi Simon, > > > > [...] > > > >> > Changes since v1: > >> > - Tokenize the DT node entry and use the appropriate value instead of > >> > the entire string > >> > - Removed Peters tested/reviewed-by tags due to the above > >> > lib/smbios.c | 94 +++++++++++++++++++++++++++++++++++++++++++++++++--- > >> > 1 file changed, 90 insertions(+), 4 deletions(-) > >> > > >> > >> Can this be put behind a Kconfig? It adds quite a bit of code which > >> punishes those boards which do the right thing. > > > > > > Sure but OTOH the code increase should be really minimal. But I don't mind I can add a Kconfig > > > >> > >> > + > >> > + dt_str = ofnode_read_string(ofnode_root(), nprop->dt_str); > >> > >> Could this use ofnode_read_string_index() ? > > > > > > Maybe, I'll have a look and change it if that works Unless I am missing something this doesn't work. This is designed to return a string index from a DT property that's defined as foo_property = "value1", "value2" isn't it? The code above is trying to read an existing property (e.g compatible) and get the string after the comma delimiter. Perhaps I should add this in drivers/core/ofnode.c instead? > > > > [...] > > > >> > >> Any chance of a test for this code? > > > > > > Sure, but any suggestions on where to add the test? > > SMBIOS tables are populated on OS booting, do we have a test somewhere that boots an OS? > > They are written on startup, right? They should certainly be in place > before U-Boot enters the command line. Not always. I am not sure if x86 does that, but on the rest of the architectures, they are only initialized when the efi smbios code runs. Wasn't this something you were trying to change? > > > Any other ideas? > > Probably a test in test/lib/ would work. We actually have > smbios-parser.c which might help? That doesn't seem too helpful either. But I can add a test in test/dm/ofnode.c if we move the parsing function to ofnode.c. The generic SMBIOS tests can be added when the SMBIOS code is refactored. Regards /Ilias > > Regards, > Simon
On Wed, Dec 06, 2023 at 06:03:14PM +0200, Ilias Apalodimas wrote: > Hi Simon, > > On Sat, 2 Dec 2023 at 20:28, Simon Glass <sjg@chromium.org> wrote: > > > > Hi Ilias, > > > > On Wed, 29 Nov 2023 at 23:50, Ilias Apalodimas > > <ilias.apalodimas@linaro.org> wrote: > > > > > > Hi Simon, > > > > > > [...] > > > > > >> > Changes since v1: > > >> > - Tokenize the DT node entry and use the appropriate value instead of > > >> > the entire string > > >> > - Removed Peters tested/reviewed-by tags due to the above > > >> > lib/smbios.c | 94 +++++++++++++++++++++++++++++++++++++++++++++++++--- > > >> > 1 file changed, 90 insertions(+), 4 deletions(-) > > >> > > > >> > > >> Can this be put behind a Kconfig? It adds quite a bit of code which > > >> punishes those boards which do the right thing. > > > > > > > > > Sure but OTOH the code increase should be really minimal. But I don't mind I can add a Kconfig > > > > > >> > > >> > + > > >> > + dt_str = ofnode_read_string(ofnode_root(), nprop->dt_str); > > >> > > >> Could this use ofnode_read_string_index() ? > > > > > > > > > Maybe, I'll have a look and change it if that works > > Unless I am missing something this doesn't work. > This is designed to return a string index from a DT property that's defined as > foo_property = "value1", "value2" isn't it? > > The code above is trying to read an existing property (e.g compatible) > and get the string after the comma delimiter. > Perhaps I should add this in drivers/core/ofnode.c instead? > > > > > > > [...] > > > > > >> > > >> Any chance of a test for this code? > > > > > > > > > Sure, but any suggestions on where to add the test? > > > SMBIOS tables are populated on OS booting, do we have a test somewhere that boots an OS? > > > > They are written on startup, right? They should certainly be in place > > before U-Boot enters the command line. > > Not always. I am not sure if x86 does that, but on the rest of the > architectures, they are only initialized when the efi smbios code > runs. Wasn't this something you were trying to change? One of those things I keep repeating is that we don't know for sure what the right values here are until we load the DT the OS _will_ use. It's quite valid to start U-Boot and pass it a generic "good enough" DT at run time until U-Boot can see (GPIOs, EEPROM, whatever) what it's on and what the real DT to load before passing to the OS is. Since U-Boot doesn't need SMBIOS tables itself, we should make these "late" not "early".
Hi Tom, On Sat, 9 Dec 2023 at 20:49, Tom Rini <trini@konsulko.com> wrote: > > On Wed, Dec 06, 2023 at 06:03:14PM +0200, Ilias Apalodimas wrote: > > Hi Simon, > > > > On Sat, 2 Dec 2023 at 20:28, Simon Glass <sjg@chromium.org> wrote: > > > > > > Hi Ilias, > > > > > > On Wed, 29 Nov 2023 at 23:50, Ilias Apalodimas > > > <ilias.apalodimas@linaro.org> wrote: > > > > > > > > Hi Simon, > > > > > > > > [...] > > > > > > > >> > Changes since v1: > > > >> > - Tokenize the DT node entry and use the appropriate value instead of > > > >> > the entire string > > > >> > - Removed Peters tested/reviewed-by tags due to the above > > > >> > lib/smbios.c | 94 +++++++++++++++++++++++++++++++++++++++++++++++++--- > > > >> > 1 file changed, 90 insertions(+), 4 deletions(-) > > > >> > > > > >> > > > >> Can this be put behind a Kconfig? It adds quite a bit of code which > > > >> punishes those boards which do the right thing. > > > > > > > > > > > > Sure but OTOH the code increase should be really minimal. But I don't mind I can add a Kconfig > > > > > > > >> > > > >> > + > > > >> > + dt_str = ofnode_read_string(ofnode_root(), nprop->dt_str); > > > >> > > > >> Could this use ofnode_read_string_index() ? > > > > > > > > > > > > Maybe, I'll have a look and change it if that works > > > > Unless I am missing something this doesn't work. > > This is designed to return a string index from a DT property that's defined as > > foo_property = "value1", "value2" isn't it? > > > > The code above is trying to read an existing property (e.g compatible) > > and get the string after the comma delimiter. > > Perhaps I should add this in drivers/core/ofnode.c instead? > > > > > > > > > > [...] > > > > > > > >> > > > >> Any chance of a test for this code? > > > > > > > > > > > > Sure, but any suggestions on where to add the test? > > > > SMBIOS tables are populated on OS booting, do we have a test somewhere that boots an OS? > > > > > > They are written on startup, right? They should certainly be in place > > > before U-Boot enters the command line. > > > > Not always. I am not sure if x86 does that, but on the rest of the > > architectures, they are only initialized when the efi smbios code > > runs. Wasn't this something you were trying to change? > > One of those things I keep repeating is that we don't know for sure what > the right values here are until we load the DT the OS _will_ use. It's > quite valid to start U-Boot and pass it a generic "good enough" DT at > run time until U-Boot can see (GPIOs, EEPROM, whatever) what it's on and > what the real DT to load before passing to the OS is. Since U-Boot > doesn't need SMBIOS tables itself, we should make these "late" not > "early". Fair enough, we can defer the init and testing of those late, just before we are about to boot. But this is irrelevant to what this patch does, can we get the fallback mechanism in first, assuming everyone is ok with the patch now? Thanks /Ilias > > -- > Tom
Hi Ilias, On Mon, 11 Dec 2023 at 00:49, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > Hi Tom, > > On Sat, 9 Dec 2023 at 20:49, Tom Rini <trini@konsulko.com> wrote: > > > > On Wed, Dec 06, 2023 at 06:03:14PM +0200, Ilias Apalodimas wrote: > > > Hi Simon, > > > > > > On Sat, 2 Dec 2023 at 20:28, Simon Glass <sjg@chromium.org> wrote: > > > > > > > > Hi Ilias, > > > > > > > > On Wed, 29 Nov 2023 at 23:50, Ilias Apalodimas > > > > <ilias.apalodimas@linaro.org> wrote: > > > > > > > > > > Hi Simon, > > > > > > > > > > [...] > > > > > > > > > >> > Changes since v1: > > > > >> > - Tokenize the DT node entry and use the appropriate value instead of > > > > >> > the entire string > > > > >> > - Removed Peters tested/reviewed-by tags due to the above > > > > >> > lib/smbios.c | 94 +++++++++++++++++++++++++++++++++++++++++++++++++--- > > > > >> > 1 file changed, 90 insertions(+), 4 deletions(-) > > > > >> > > > > > >> > > > > >> Can this be put behind a Kconfig? It adds quite a bit of code which > > > > >> punishes those boards which do the right thing. > > > > > > > > > > > > > > > Sure but OTOH the code increase should be really minimal. But I don't mind I can add a Kconfig > > > > > > > > > >> > > > > >> > + > > > > >> > + dt_str = ofnode_read_string(ofnode_root(), nprop->dt_str); > > > > >> > > > > >> Could this use ofnode_read_string_index() ? > > > > > > > > > > > > > > > Maybe, I'll have a look and change it if that works > > > > > > Unless I am missing something this doesn't work. > > > This is designed to return a string index from a DT property that's defined as > > > foo_property = "value1", "value2" isn't it? > > > > > > The code above is trying to read an existing property (e.g compatible) > > > and get the string after the comma delimiter. > > > Perhaps I should add this in drivers/core/ofnode.c instead? > > > > > > > > > > > > > [...] > > > > > > > > > >> > > > > >> Any chance of a test for this code? > > > > > > > > > > > > > > > Sure, but any suggestions on where to add the test? > > > > > SMBIOS tables are populated on OS booting, do we have a test somewhere that boots an OS? > > > > > > > > They are written on startup, right? They should certainly be in place > > > > before U-Boot enters the command line. > > > > > > Not always. I am not sure if x86 does that, but on the rest of the > > > architectures, they are only initialized when the efi smbios code > > > runs. Wasn't this something you were trying to change? > > > > One of those things I keep repeating is that we don't know for sure what > > the right values here are until we load the DT the OS _will_ use. It's > > quite valid to start U-Boot and pass it a generic "good enough" DT at > > run time until U-Boot can see (GPIOs, EEPROM, whatever) what it's on and > > what the real DT to load before passing to the OS is. Since U-Boot > > doesn't need SMBIOS tables itself, we should make these "late" not > > "early". > > Fair enough, we can defer the init and testing of those late, just > before we are about to boot. But this is irrelevant to what this patch > does, can we get the fallback mechanism in first, assuming everyone is > ok with the patch now? > I would like this behind a Kconfig. Making it the default means people are going to start relying on (in user space) and then discover later that it is wrong. Regards, Simon
On Wed, Dec 13, 2023 at 12:50:03PM -0700, Simon Glass wrote: > Hi Ilias, > > On Mon, 11 Dec 2023 at 00:49, Ilias Apalodimas > <ilias.apalodimas@linaro.org> wrote: > > > > Hi Tom, > > > > On Sat, 9 Dec 2023 at 20:49, Tom Rini <trini@konsulko.com> wrote: > > > > > > On Wed, Dec 06, 2023 at 06:03:14PM +0200, Ilias Apalodimas wrote: > > > > Hi Simon, > > > > > > > > On Sat, 2 Dec 2023 at 20:28, Simon Glass <sjg@chromium.org> wrote: > > > > > > > > > > Hi Ilias, > > > > > > > > > > On Wed, 29 Nov 2023 at 23:50, Ilias Apalodimas > > > > > <ilias.apalodimas@linaro.org> wrote: > > > > > > > > > > > > Hi Simon, > > > > > > > > > > > > [...] > > > > > > > > > > > >> > Changes since v1: > > > > > >> > - Tokenize the DT node entry and use the appropriate value instead of > > > > > >> > the entire string > > > > > >> > - Removed Peters tested/reviewed-by tags due to the above > > > > > >> > lib/smbios.c | 94 +++++++++++++++++++++++++++++++++++++++++++++++++--- > > > > > >> > 1 file changed, 90 insertions(+), 4 deletions(-) > > > > > >> > > > > > > >> > > > > > >> Can this be put behind a Kconfig? It adds quite a bit of code which > > > > > >> punishes those boards which do the right thing. > > > > > > > > > > > > > > > > > > Sure but OTOH the code increase should be really minimal. But I don't mind I can add a Kconfig > > > > > > > > > > > >> > > > > > >> > + > > > > > >> > + dt_str = ofnode_read_string(ofnode_root(), nprop->dt_str); > > > > > >> > > > > > >> Could this use ofnode_read_string_index() ? > > > > > > > > > > > > > > > > > > Maybe, I'll have a look and change it if that works > > > > > > > > Unless I am missing something this doesn't work. > > > > This is designed to return a string index from a DT property that's defined as > > > > foo_property = "value1", "value2" isn't it? > > > > > > > > The code above is trying to read an existing property (e.g compatible) > > > > and get the string after the comma delimiter. > > > > Perhaps I should add this in drivers/core/ofnode.c instead? > > > > > > > > > > > > > > > > [...] > > > > > > > > > > > >> > > > > > >> Any chance of a test for this code? > > > > > > > > > > > > > > > > > > Sure, but any suggestions on where to add the test? > > > > > > SMBIOS tables are populated on OS booting, do we have a test somewhere that boots an OS? > > > > > > > > > > They are written on startup, right? They should certainly be in place > > > > > before U-Boot enters the command line. > > > > > > > > Not always. I am not sure if x86 does that, but on the rest of the > > > > architectures, they are only initialized when the efi smbios code > > > > runs. Wasn't this something you were trying to change? > > > > > > One of those things I keep repeating is that we don't know for sure what > > > the right values here are until we load the DT the OS _will_ use. It's > > > quite valid to start U-Boot and pass it a generic "good enough" DT at > > > run time until U-Boot can see (GPIOs, EEPROM, whatever) what it's on and > > > what the real DT to load before passing to the OS is. Since U-Boot > > > doesn't need SMBIOS tables itself, we should make these "late" not > > > "early". > > > > Fair enough, we can defer the init and testing of those late, just > > before we are about to boot. But this is irrelevant to what this patch > > does, can we get the fallback mechanism in first, assuming everyone is > > ok with the patch now? > > > > I would like this behind a Kconfig. Making it the default means people > are going to start relying on (in user space) and then discover later > that it is wrong. What do you mean wrong, exactly?
Hi Tom, On Wed, 13 Dec 2023 at 13:19, Tom Rini <trini@konsulko.com> wrote: > > On Wed, Dec 13, 2023 at 12:50:03PM -0700, Simon Glass wrote: > > Hi Ilias, > > > > On Mon, 11 Dec 2023 at 00:49, Ilias Apalodimas > > <ilias.apalodimas@linaro.org> wrote: > > > > > > Hi Tom, > > > > > > On Sat, 9 Dec 2023 at 20:49, Tom Rini <trini@konsulko.com> wrote: > > > > > > > > On Wed, Dec 06, 2023 at 06:03:14PM +0200, Ilias Apalodimas wrote: > > > > > Hi Simon, > > > > > > > > > > On Sat, 2 Dec 2023 at 20:28, Simon Glass <sjg@chromium.org> wrote: > > > > > > > > > > > > Hi Ilias, > > > > > > > > > > > > On Wed, 29 Nov 2023 at 23:50, Ilias Apalodimas > > > > > > <ilias.apalodimas@linaro.org> wrote: > > > > > > > > > > > > > > Hi Simon, > > > > > > > > > > > > > > [...] > > > > > > > > > > > > > >> > Changes since v1: > > > > > > >> > - Tokenize the DT node entry and use the appropriate value instead of > > > > > > >> > the entire string > > > > > > >> > - Removed Peters tested/reviewed-by tags due to the above > > > > > > >> > lib/smbios.c | 94 +++++++++++++++++++++++++++++++++++++++++++++++++--- > > > > > > >> > 1 file changed, 90 insertions(+), 4 deletions(-) > > > > > > >> > > > > > > > >> > > > > > > >> Can this be put behind a Kconfig? It adds quite a bit of code which > > > > > > >> punishes those boards which do the right thing. > > > > > > > > > > > > > > > > > > > > > Sure but OTOH the code increase should be really minimal. But I don't mind I can add a Kconfig > > > > > > > > > > > > > >> > > > > > > >> > + > > > > > > >> > + dt_str = ofnode_read_string(ofnode_root(), nprop->dt_str); > > > > > > >> > > > > > > >> Could this use ofnode_read_string_index() ? > > > > > > > > > > > > > > > > > > > > > Maybe, I'll have a look and change it if that works > > > > > > > > > > Unless I am missing something this doesn't work. > > > > > This is designed to return a string index from a DT property that's defined as > > > > > foo_property = "value1", "value2" isn't it? > > > > > > > > > > The code above is trying to read an existing property (e.g compatible) > > > > > and get the string after the comma delimiter. > > > > > Perhaps I should add this in drivers/core/ofnode.c instead? > > > > > > > > > > > > > > > > > > > [...] > > > > > > > > > > > > > >> > > > > > > >> Any chance of a test for this code? > > > > > > > > > > > > > > > > > > > > > Sure, but any suggestions on where to add the test? > > > > > > > SMBIOS tables are populated on OS booting, do we have a test somewhere that boots an OS? > > > > > > > > > > > > They are written on startup, right? They should certainly be in place > > > > > > before U-Boot enters the command line. > > > > > > > > > > Not always. I am not sure if x86 does that, but on the rest of the > > > > > architectures, they are only initialized when the efi smbios code > > > > > runs. Wasn't this something you were trying to change? > > > > > > > > One of those things I keep repeating is that we don't know for sure what > > > > the right values here are until we load the DT the OS _will_ use. It's > > > > quite valid to start U-Boot and pass it a generic "good enough" DT at > > > > run time until U-Boot can see (GPIOs, EEPROM, whatever) what it's on and > > > > what the real DT to load before passing to the OS is. Since U-Boot > > > > doesn't need SMBIOS tables itself, we should make these "late" not > > > > "early". > > > > > > Fair enough, we can defer the init and testing of those late, just > > > before we are about to boot. But this is irrelevant to what this patch > > > does, can we get the fallback mechanism in first, assuming everyone is > > > ok with the patch now? > > > > > > > I would like this behind a Kconfig. Making it the default means people > > are going to start relying on (in user space) and then discover later > > that it is wrong. > > What do you mean wrong, exactly? "raspberrypi" instead of "Raspberry Pi", for example, or "friendlyarm" instead of "FriendlyElec" I just wonder what this information is actually used for. If it is just for fun, that is fine, but I believe some tools do use dmidecode, at which point it needs to be accurate and should not change later, e.g. when someone decides to actually add the info. Regards, Simon
> From: Simon Glass <sjg@chromium.org> > Date: Wed, 13 Dec 2023 13:41:05 -0700 > > Hi Tom, > > On Wed, 13 Dec 2023 at 13:19, Tom Rini <trini@konsulko.com> wrote: > > > > On Wed, Dec 13, 2023 at 12:50:03PM -0700, Simon Glass wrote: > > > Hi Ilias, > > > > > > On Mon, 11 Dec 2023 at 00:49, Ilias Apalodimas > > > <ilias.apalodimas@linaro.org> wrote: > > > > > > > > Hi Tom, > > > > > > > > On Sat, 9 Dec 2023 at 20:49, Tom Rini <trini@konsulko.com> wrote: > > > > > > > > > > On Wed, Dec 06, 2023 at 06:03:14PM +0200, Ilias Apalodimas wrote: > > > > > > Hi Simon, > > > > > > > > > > > > On Sat, 2 Dec 2023 at 20:28, Simon Glass <sjg@chromium.org> wrote: > > > > > > > > > > > > > > Hi Ilias, > > > > > > > > > > > > > > On Wed, 29 Nov 2023 at 23:50, Ilias Apalodimas > > > > > > > <ilias.apalodimas@linaro.org> wrote: > > > > > > > > > > > > > > > > Hi Simon, > > > > > > > > > > > > > > > > [...] > > > > > > > > > > > > > > > >> > Changes since v1: > > > > > > > >> > - Tokenize the DT node entry and use the appropriate value instead of > > > > > > > >> > the entire string > > > > > > > >> > - Removed Peters tested/reviewed-by tags due to the above > > > > > > > >> > lib/smbios.c | 94 +++++++++++++++++++++++++++++++++++++++++++++++++--- > > > > > > > >> > 1 file changed, 90 insertions(+), 4 deletions(-) > > > > > > > >> > > > > > > > > >> > > > > > > > >> Can this be put behind a Kconfig? It adds quite a bit of code which > > > > > > > >> punishes those boards which do the right thing. > > > > > > > > > > > > > > > > > > > > > > > > Sure but OTOH the code increase should be really minimal. But I don't mind I can add a Kconfig > > > > > > > > > > > > > > > >> > > > > > > > >> > + > > > > > > > >> > + dt_str = ofnode_read_string(ofnode_root(), nprop->dt_str); > > > > > > > >> > > > > > > > >> Could this use ofnode_read_string_index() ? > > > > > > > > > > > > > > > > > > > > > > > > Maybe, I'll have a look and change it if that works > > > > > > > > > > > > Unless I am missing something this doesn't work. > > > > > > This is designed to return a string index from a DT property that's defined as > > > > > > foo_property = "value1", "value2" isn't it? > > > > > > > > > > > > The code above is trying to read an existing property (e.g compatible) > > > > > > and get the string after the comma delimiter. > > > > > > Perhaps I should add this in drivers/core/ofnode.c instead? > > > > > > > > > > > > > > > > > > > > > > [...] > > > > > > > > > > > > > > > >> > > > > > > > >> Any chance of a test for this code? > > > > > > > > > > > > > > > > > > > > > > > > Sure, but any suggestions on where to add the test? > > > > > > > > SMBIOS tables are populated on OS booting, do we have a test somewhere that boots an OS? > > > > > > > > > > > > > > They are written on startup, right? They should certainly be in place > > > > > > > before U-Boot enters the command line. > > > > > > > > > > > > Not always. I am not sure if x86 does that, but on the rest of the > > > > > > architectures, they are only initialized when the efi smbios code > > > > > > runs. Wasn't this something you were trying to change? > > > > > > > > > > One of those things I keep repeating is that we don't know for sure what > > > > > the right values here are until we load the DT the OS _will_ use. It's > > > > > quite valid to start U-Boot and pass it a generic "good enough" DT at > > > > > run time until U-Boot can see (GPIOs, EEPROM, whatever) what it's on and > > > > > what the real DT to load before passing to the OS is. Since U-Boot > > > > > doesn't need SMBIOS tables itself, we should make these "late" not > > > > > "early". > > > > > > > > Fair enough, we can defer the init and testing of those late, just > > > > before we are about to boot. But this is irrelevant to what this patch > > > > does, can we get the fallback mechanism in first, assuming everyone is > > > > ok with the patch now? > > > > > > > > > > I would like this behind a Kconfig. Making it the default means people > > > are going to start relying on (in user space) and then discover later > > > that it is wrong. > > > > What do you mean wrong, exactly? > > "raspberrypi" instead of "Raspberry Pi", for example, or "friendlyarm" > instead of "FriendlyElec" Heh, well, even in the x86 world vendors can't even spell their own name consistently. > I just wonder what this information is actually used for. If it is > just for fun, that is fine, but I believe some tools do use dmidecode, > at which point it needs to be accurate and should not change later, > e.g. when someone decides to actually add the info. So the Linux kernel uses this information to apply quirks for specific machines. But I hope that on platforms that have a device tree folks will just use the board compatible string for that purpose. Otherwise I think this information is mostly used to populate system information UIs and asset databases.
Hi Mark, On Wed, 13 Dec 2023 at 14:00, Mark Kettenis <mark.kettenis@xs4all.nl> wrote: > > > From: Simon Glass <sjg@chromium.org> > > Date: Wed, 13 Dec 2023 13:41:05 -0700 > > > > Hi Tom, > > > > On Wed, 13 Dec 2023 at 13:19, Tom Rini <trini@konsulko.com> wrote: > > > > > > On Wed, Dec 13, 2023 at 12:50:03PM -0700, Simon Glass wrote: > > > > Hi Ilias, > > > > > > > > On Mon, 11 Dec 2023 at 00:49, Ilias Apalodimas > > > > <ilias.apalodimas@linaro.org> wrote: > > > > > > > > > > Hi Tom, > > > > > > > > > > On Sat, 9 Dec 2023 at 20:49, Tom Rini <trini@konsulko.com> wrote: > > > > > > > > > > > > On Wed, Dec 06, 2023 at 06:03:14PM +0200, Ilias Apalodimas wrote: > > > > > > > Hi Simon, > > > > > > > > > > > > > > On Sat, 2 Dec 2023 at 20:28, Simon Glass <sjg@chromium.org> wrote: > > > > > > > > > > > > > > > > Hi Ilias, > > > > > > > > > > > > > > > > On Wed, 29 Nov 2023 at 23:50, Ilias Apalodimas > > > > > > > > <ilias.apalodimas@linaro.org> wrote: > > > > > > > > > > > > > > > > > > Hi Simon, > > > > > > > > > > > > > > > > > > [...] > > > > > > > > > > > > > > > > > >> > Changes since v1: > > > > > > > > >> > - Tokenize the DT node entry and use the appropriate value instead of > > > > > > > > >> > the entire string > > > > > > > > >> > - Removed Peters tested/reviewed-by tags due to the above > > > > > > > > >> > lib/smbios.c | 94 +++++++++++++++++++++++++++++++++++++++++++++++++--- > > > > > > > > >> > 1 file changed, 90 insertions(+), 4 deletions(-) > > > > > > > > >> > > > > > > > > > >> > > > > > > > > >> Can this be put behind a Kconfig? It adds quite a bit of code which > > > > > > > > >> punishes those boards which do the right thing. > > > > > > > > > > > > > > > > > > > > > > > > > > > Sure but OTOH the code increase should be really minimal. But I don't mind I can add a Kconfig > > > > > > > > > > > > > > > > > >> > > > > > > > > >> > + > > > > > > > > >> > + dt_str = ofnode_read_string(ofnode_root(), nprop->dt_str); > > > > > > > > >> > > > > > > > > >> Could this use ofnode_read_string_index() ? > > > > > > > > > > > > > > > > > > > > > > > > > > > Maybe, I'll have a look and change it if that works > > > > > > > > > > > > > > Unless I am missing something this doesn't work. > > > > > > > This is designed to return a string index from a DT property that's defined as > > > > > > > foo_property = "value1", "value2" isn't it? > > > > > > > > > > > > > > The code above is trying to read an existing property (e.g compatible) > > > > > > > and get the string after the comma delimiter. > > > > > > > Perhaps I should add this in drivers/core/ofnode.c instead? > > > > > > > > > > > > > > > > > > > > > > > > > [...] > > > > > > > > > > > > > > > > > >> > > > > > > > > >> Any chance of a test for this code? > > > > > > > > > > > > > > > > > > > > > > > > > > > Sure, but any suggestions on where to add the test? > > > > > > > > > SMBIOS tables are populated on OS booting, do we have a test somewhere that boots an OS? > > > > > > > > > > > > > > > > They are written on startup, right? They should certainly be in place > > > > > > > > before U-Boot enters the command line. > > > > > > > > > > > > > > Not always. I am not sure if x86 does that, but on the rest of the > > > > > > > architectures, they are only initialized when the efi smbios code > > > > > > > runs. Wasn't this something you were trying to change? > > > > > > > > > > > > One of those things I keep repeating is that we don't know for sure what > > > > > > the right values here are until we load the DT the OS _will_ use. It's > > > > > > quite valid to start U-Boot and pass it a generic "good enough" DT at > > > > > > run time until U-Boot can see (GPIOs, EEPROM, whatever) what it's on and > > > > > > what the real DT to load before passing to the OS is. Since U-Boot > > > > > > doesn't need SMBIOS tables itself, we should make these "late" not > > > > > > "early". > > > > > > > > > > Fair enough, we can defer the init and testing of those late, just > > > > > before we are about to boot. But this is irrelevant to what this patch > > > > > does, can we get the fallback mechanism in first, assuming everyone is > > > > > ok with the patch now? > > > > > > > > > > > > > I would like this behind a Kconfig. Making it the default means people > > > > are going to start relying on (in user space) and then discover later > > > > that it is wrong. > > > > > > What do you mean wrong, exactly? > > > > "raspberrypi" instead of "Raspberry Pi", for example, or "friendlyarm" > > instead of "FriendlyElec" > > Heh, well, even in the x86 world vendors can't even spell their own > name consistently. > > > I just wonder what this information is actually used for. If it is > > just for fun, that is fine, but I believe some tools do use dmidecode, > > at which point it needs to be accurate and should not change later, > > e.g. when someone decides to actually add the info. > > So the Linux kernel uses this information to apply quirks for specific > machines. But I hope that on platforms that have a device tree folks > will just use the board compatible string for that purpose. Right. > > Otherwise I think this information is mostly used to populate system > information UIs and asset databases. So perhaps the devicetree should be used instead? How do these things work today? And what are they, exactly? Regards, Simon
> > > > > > Not always. I am not sure if x86 does that, but on the rest of the > > > > > > architectures, they are only initialized when the efi smbios code > > > > > > runs. Wasn't this something you were trying to change? > > > > > > > > > > One of those things I keep repeating is that we don't know for sure what > > > > > the right values here are until we load the DT the OS _will_ use. It's > > > > > quite valid to start U-Boot and pass it a generic "good enough" DT at > > > > > run time until U-Boot can see (GPIOs, EEPROM, whatever) what it's on and > > > > > what the real DT to load before passing to the OS is. Since U-Boot > > > > > doesn't need SMBIOS tables itself, we should make these "late" not > > > > > "early". > > > > > > > > Fair enough, we can defer the init and testing of those late, just > > > > before we are about to boot. But this is irrelevant to what this patch > > > > does, can we get the fallback mechanism in first, assuming everyone is > > > > ok with the patch now? > > > > > > > > > > I would like this behind a Kconfig. Making it the default means people > > > are going to start relying on (in user space) and then discover later > > > that it is wrong. > > > > What do you mean wrong, exactly? > > "raspberrypi" instead of "Raspberry Pi", for example, or "friendlyarm" > instead of "FriendlyElec" Well "raspberrypi" is better that what we get on the Raspberry Pi with my testing at the moment which is "Unknown", which from what I can tell from commit 330419727 should have at least the minimal amount but it doesn't. > I just wonder what this information is actually used for. If it is > just for fun, that is fine, but I believe some tools do use dmidecode, > at which point it needs to be accurate and should not change later, > e.g. when someone decides to actually add the info. So a lot of tools use the output of dmidecode, it's used extensively through various support platforms and management tools. In Fedora I would generally get around a dozen reports every few months because anything booted with U-Boot was basically populated with "Unknown". At the moment, on the RPi4 with default upstream I get Unkown [1] with these patches I get at least some useful information [2]. I've been shipping this patch set, in various versions, for a while and have not had a single bug report about wrong information. When Ilias and I first spoke about these patches the intention was to get initial useful information, them they could be possibly expanded using things like information from eFuses (serial numbers etc). In some cases with U-Boot you get SMBIOS tables that are incorrect and crash the tools. With this patch set you get at least the basics which is important because support teams can easily work out the basics of what they're working with even if they don't have physical access all without the vendor of the HW having to work out what to do in firmware to make something work. This patch set moves the needle forward, if we wait for everything to be properly formatted we will wait for ever, with information we already have available ON EVERY DEVICE. Peter [1] https://pbrobinson.fedorapeople.org/dmi-decode-rpi-defaults.txt [2] https://pbrobinson.fedorapeople.org/dmi-decode-rpi-auto-fallback-patchset.txt
> > > > What do you mean wrong, exactly? > > > > > > "raspberrypi" instead of "Raspberry Pi", for example, or "friendlyarm" > > > instead of "FriendlyElec" > > > > Heh, well, even in the x86 world vendors can't even spell their own > > name consistently. > > > > > I just wonder what this information is actually used for. If it is > > > just for fun, that is fine, but I believe some tools do use dmidecode, > > > at which point it needs to be accurate and should not change later, > > > e.g. when someone decides to actually add the info. > > > > So the Linux kernel uses this information to apply quirks for specific > > machines. But I hope that on platforms that have a device tree folks > > will just use the board compatible string for that purpose. > > Right. > > > > > Otherwise I think this information is mostly used to populate system > > information UIs and asset databases. > > So perhaps the devicetree should be used instead? How do these things > work today? And what are they, exactly? Device tree used for what? To populate the SMBIOS tables? Or to populate "system information UIs and asset databases." If you mean the later, that is not going to work. These platforms are often proprietary, and are generally remote. They have local agents that use dmidecode to populate things for support and management. It's kind of in the name "System Management". To get all these platforms updated to "just use devicetree" will take decades. No thanks!
Hi Peter, On Tue, 19 Dec 2023 at 13:23, Peter Robinson <pbrobinson@gmail.com> wrote: > > > > > > What do you mean wrong, exactly? > > > > > > > > "raspberrypi" instead of "Raspberry Pi", for example, or "friendlyarm" > > > > instead of "FriendlyElec" > > > > > > Heh, well, even in the x86 world vendors can't even spell their own > > > name consistently. > > > > > > > I just wonder what this information is actually used for. If it is > > > > just for fun, that is fine, but I believe some tools do use dmidecode, > > > > at which point it needs to be accurate and should not change later, > > > > e.g. when someone decides to actually add the info. > > > > > > So the Linux kernel uses this information to apply quirks for specific > > > machines. But I hope that on platforms that have a device tree folks > > > will just use the board compatible string for that purpose. > > > > Right. > > > > > > > > Otherwise I think this information is mostly used to populate system > > > information UIs and asset databases. > > > > So perhaps the devicetree should be used instead? How do these things > > work today? And what are they, exactly? > > Device tree used for what? To populate the SMBIOS tables? Or to > populate "system information UIs and asset databases." > > If you mean the later, that is not going to work. These platforms are > often proprietary, and are generally remote. They have local agents > that use dmidecode to populate things for support and management. It's > kind of in the name "System Management". To get all these platforms > updated to "just use devicetree" will take decades. No thanks! Do we care about these platforms? Do we even know what they are? Do they even run on ARM? I found a thing called 'sos' and it does actually use the devicetree, which is actually the correct thing to do, on platforms which use device tree. At this point I'm wondering what problem is being solved here? We know we cannot pass the SMBIOS table without EFI in any case. A huge argument in Linux with hundreds of emails results in 'no patch applied', so far as I am aware. At the very least, that seems odd. Just to restate my suggestions: - Put this feature behind an #ifdef - Allow people to add the authoritative info if they want to - Add something to the SMBIOS info that indicates it is provisional / auto-generated - Figure out how to handle this when booting without EFI (or decide that SMBIOS is not used then?) - minor: simplify the code to just handle the two pieces of info currently decoded; add tests Do you agree with any or all of those? Regards, Simon
diff --git a/lib/smbios.c b/lib/smbios.c index fcc8686993ef..423893639ff0 100644 --- a/lib/smbios.c +++ b/lib/smbios.c @@ -9,11 +9,14 @@ #include <dm.h> #include <env.h> #include <linux/stringify.h> +#include <linux/string.h> #include <mapmem.h> #include <smbios.h> #include <sysinfo.h> #include <tables_csum.h> #include <version.h> +#include <malloc.h> +#include <dm/ofnode.h> #ifdef CONFIG_CPU #include <cpu.h> #include <dm/uclass-internal.h> @@ -43,6 +46,25 @@ DECLARE_GLOBAL_DATA_PTR; +/** + * struct map_sysinfo - Mapping of sysinfo strings to DT + * + * @sysinfo_str: sysinfo string + * @dt_str: DT string + * @max: Max index of the tokenized string to pick. Counting starts from 0 + * + */ +struct map_sysinfo { + const char *sysinfo_str; + const char *dt_str; + int max; +}; + +static const struct map_sysinfo sysinfo_to_dt[] = { + { .sysinfo_str = "product", .dt_str = "model", 2 }, + { .sysinfo_str = "manufacturer", .dt_str = "compatible", 1 }, +}; + /** * struct smbios_ctx - context for writing SMBIOS tables * @@ -87,6 +109,18 @@ struct smbios_write_method { const char *subnode_name; }; +static const struct map_sysinfo *convert_sysinfo_to_dt(const char *sysinfo_str) +{ + int i; + + for (i = 0; i < ARRAY_SIZE(sysinfo_to_dt); i++) { + if (!strcmp(sysinfo_str, sysinfo_to_dt[i].sysinfo_str)) + return &sysinfo_to_dt[i]; + } + + return NULL; +} + /** * smbios_add_string() - add a string to the string area * @@ -127,6 +161,42 @@ static int smbios_add_string(struct smbios_ctx *ctx, const char *str) } } +/** + * get_str_from_dt - Get a substring from a DT property. + * After finding the property in the DT, the function + * will parse comma separted values and return the value. + * If nprop->max exceeds the number of comma separated + * elements the last non NULL value will be returned. + * Counting starts from zero. + * + * @nprop: sysinfo property to use + * @str: pointer to fill with data + * @size: str buffer length + */ +static +void get_str_from_dt(const struct map_sysinfo *nprop, char *str, size_t size) +{ + const char *dt_str; + int cnt = 0; + char *token; + + memset(str, 0, size); + if (!nprop || !nprop->max) + return; + + dt_str = ofnode_read_string(ofnode_root(), nprop->dt_str); + if (!dt_str) + return; + + memcpy(str, dt_str, size); + token = strtok(str, ","); + while (token && cnt < nprop->max) { + strlcpy(str, token, strlen(token) + 1); + token = strtok(NULL, ","); + cnt++; + } +} + /** * smbios_add_prop_si() - Add a property from the devicetree or sysinfo * @@ -139,19 +209,35 @@ static int smbios_add_string(struct smbios_ctx *ctx, const char *str) static int smbios_add_prop_si(struct smbios_ctx *ctx, const char *prop, int sysinfo_id) { + int ret = 0; + if (sysinfo_id && ctx->dev) { char val[SMBIOS_STR_MAX]; - int ret; ret = sysinfo_get_str(ctx->dev, sysinfo_id, sizeof(val), val); if (!ret) return smbios_add_string(ctx, val); } + if (IS_ENABLED(CONFIG_OF_CONTROL)) { - const char *str; + const char *str = NULL; + char str_dt[128] = { 0 }; + /* + * If the node is not valid fallback and try the entire DT + * so we can at least fill in maufacturer and board type + */ + if (ofnode_valid(ctx->node)) { + str = ofnode_read_string(ctx->node, prop); + } else { + const struct map_sysinfo *nprop; + + nprop = convert_sysinfo_to_dt(prop); + get_str_from_dt(nprop, str_dt, sizeof(str_dt)); + } - str = ofnode_read_string(ctx->node, prop); - return smbios_add_string(ctx, str); + ret = smbios_add_string(ctx, ofnode_valid(ctx->node) ? + str : (const char *)str_dt); + return ret; } return 0;
In order to fill in the SMBIOS tables U-Boot currently relies on a "u-boot,sysinfo-smbios" compatible node. This is fine for the boards that already include such nodes. However with some recent EFI changes, the majority of boards can boot up distros, which usually rely on things like dmidecode etc for their reporting. For boards that lack this special node the SMBIOS output looks like: System Information Manufacturer: Unknown Product Name: Unknown Version: Unknown Serial Number: Unknown UUID: Not Settable Wake-up Type: Reserved SKU Number: Unknown Family: Unknown This looks problematic since most of the info are "Unknown". The DT spec specifies standard properties containing relevant information like 'model' and 'compatible' for which the suggested format is <manufacturer,model>. Unfortunately the 'model' string found in DTs is usually lacking the manufacturer so we can't use it for both 'Manufacturer' and 'Product Name' SMBIOS entries reliably. So let's add a last resort to our current smbios parsing. If none of the sysinfo properties are found, scan for those information in the root node of the device tree. Use the 'model' to fill the 'Product Name' and the first value of 'compatible' for the 'Manufacturer', since that always contains one. pre-patch: Handle 0x0001, DMI type 1, 27 bytes System Information Manufacturer: Unknown Product Name: Unknown Version: Unknown Serial Number: 100000000bb24ceb UUID: 30303031-3030-3030-3061-613234636435 Wake-up Type: Reserved SKU Number: Unknown Family: Unknown [...] and post patch: Handle 0x0001, DMI type 1, 27 bytes System Information Manufacturer: raspberrypi Product Name: Raspberry Pi 4 Model B Rev 1.1 Version: Unknown Serial Number: 100000000bb24ceb UUID: 30303031-3030-3030-3061-613234636435 Wake-up Type: Reserved SKU Number: Unknown Family: Unknown [...] Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> --- Changes since v1: - Tokenize the DT node entry and use the appropriate value instead of the entire string - Removed Peters tested/reviewed-by tags due to the above lib/smbios.c | 94 +++++++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 90 insertions(+), 4 deletions(-) -- 2.40.1