[PATCH 0/2 v2] Provide a fallback to smbios tables
Hi, Tom asked me to clean up and resend [0]. I've made some adjustments -- The DT nodes are tokenized and a single string is used. The DT node 'model' is used to fill the SMBIOS 'Product Name' and 'compatible' is used for 'Manufacturer'.
The reason is explained in patch#2 but the tl;dr is that model doesn't always include the manufacturer despite the suggestions of the DT spec.
[0] https://lore.kernel.org/u-boot/20220906134426.53748-1-ilias.apalodimas@linar...
Ilias Apalodimas (2): smbios: Simplify reporting of unknown values smbios: Fallback to the default DT if sysinfo nodes are missing
lib/smbios.c | 109 +++++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 92 insertions(+), 17 deletions(-)
-- 2.40.1
If a value is not valid during the DT or SYSINFO parsing, we explicitly set that to "Unknown Product" and "Unknown" for the product and manufacturer respectively. It's cleaner if we move the checks insisde smbios_add_string() and always report "Unknown" regardless of the missing field.
pre-patch dmidecode <snip> Handle 0x0001, DMI type 1, 27 bytes System Information Manufacturer: Unknown Product Name: Unknown Product Version: Not Specified Serial Number: Not Specified UUID: Not Settable Wake-up Type: Reserved SKU Number: Not Specified Family: Not Specified
[...]
post-patch dmidecode:
Handle 0x0001, DMI type 1, 27 bytes System Information Manufacturer: Unknown Product Name: Unknown Version: Unknown Serial Number: Unknown UUID: Not Settable Wake-up Type: Reserved SKU Number: Unknown Family: Unknown [...]
Signed-off-by: Ilias Apalodimas ilias.apalodimas@linaro.org Reviewed-by: Peter Robinson pbrobinson@gmail.com Tested-by: Peter Robinson pbrobinson@gmail.com --- Changes since v1: - None
lib/smbios.c | 17 +++-------------- 1 file changed, 3 insertions(+), 14 deletions(-)
diff --git a/lib/smbios.c b/lib/smbios.c index d7f4999e8b2a..fcc8686993ef 100644 --- a/lib/smbios.c +++ b/lib/smbios.c @@ -102,7 +102,7 @@ static int smbios_add_string(struct smbios_ctx *ctx, const char *str) int i = 1; char *p = ctx->eos;
- if (!*str) + if (!str || !*str) str = "Unknown";
for (;;) { @@ -151,8 +151,7 @@ static int smbios_add_prop_si(struct smbios_ctx *ctx, const char *prop, const char *str;
str = ofnode_read_string(ctx->node, prop); - if (str) - return smbios_add_string(ctx, str); + return smbios_add_string(ctx, str); }
return 0; @@ -231,7 +230,7 @@ static int smbios_write_type0(ulong *current, int handle, t->vendor = smbios_add_string(ctx, "U-Boot");
t->bios_ver = smbios_add_prop(ctx, "version"); - if (!t->bios_ver) + if (!strcmp(ctx->last_str, "Unknown")) t->bios_ver = smbios_add_string(ctx, PLAIN_VERSION); if (t->bios_ver) gd->smbios_version = ctx->last_str; @@ -281,11 +280,7 @@ static int smbios_write_type1(ulong *current, int handle, fill_smbios_header(t, SMBIOS_SYSTEM_INFORMATION, len, handle); smbios_set_eos(ctx, t->eos); t->manufacturer = smbios_add_prop(ctx, "manufacturer"); - if (!t->manufacturer) - t->manufacturer = smbios_add_string(ctx, "Unknown"); t->product_name = smbios_add_prop(ctx, "product"); - if (!t->product_name) - t->product_name = smbios_add_string(ctx, "Unknown Product"); t->version = smbios_add_prop_si(ctx, "version", SYSINFO_ID_SMBIOS_SYSTEM_VERSION); if (serial_str) { @@ -315,11 +310,7 @@ static int smbios_write_type2(ulong *current, int handle, fill_smbios_header(t, SMBIOS_BOARD_INFORMATION, len, handle); smbios_set_eos(ctx, t->eos); t->manufacturer = smbios_add_prop(ctx, "manufacturer"); - if (!t->manufacturer) - t->manufacturer = smbios_add_string(ctx, "Unknown"); t->product_name = smbios_add_prop(ctx, "product"); - if (!t->product_name) - t->product_name = smbios_add_string(ctx, "Unknown Product"); t->version = smbios_add_prop_si(ctx, "version", SYSINFO_ID_SMBIOS_BASEBOARD_VERSION); t->asset_tag_number = smbios_add_prop(ctx, "asset-tag"); @@ -344,8 +335,6 @@ static int smbios_write_type3(ulong *current, int handle, fill_smbios_header(t, SMBIOS_SYSTEM_ENCLOSURE, len, handle); smbios_set_eos(ctx, t->eos); t->manufacturer = smbios_add_prop(ctx, "manufacturer"); - if (!t->manufacturer) - t->manufacturer = smbios_add_string(ctx, "Unknown"); t->chassis_type = SMBIOS_ENCLOSURE_DESKTOP; t->bootup_state = SMBIOS_STATE_SAFE; t->power_supply_state = SMBIOS_STATE_SAFE; -- 2.40.1
On Mon, Nov 27, 2023 at 07:10:57PM +0200, Ilias Apalodimas wrote:
If a value is not valid during the DT or SYSINFO parsing, we explicitly set that to "Unknown Product" and "Unknown" for the product and manufacturer respectively. It's cleaner if we move the checks insisde smbios_add_string() and always report "Unknown" regardless of the missing field.
pre-patch dmidecode
<snip> Handle 0x0001, DMI type 1, 27 bytes System Information Manufacturer: Unknown Product Name: Unknown Product Version: Not Specified Serial Number: Not Specified UUID: Not Settable Wake-up Type: Reserved SKU Number: Not Specified Family: Not Specified
[...]
post-patch dmidecode:
Handle 0x0001, DMI type 1, 27 bytes System Information Manufacturer: Unknown Product Name: Unknown Version: Unknown Serial Number: Unknown UUID: Not Settable Wake-up Type: Reserved SKU Number: Unknown Family: Unknown [...]
Signed-off-by: Ilias Apalodimas ilias.apalodimas@linaro.org Reviewed-by: Peter Robinson pbrobinson@gmail.com Tested-by: Peter Robinson pbrobinson@gmail.com
Reviewed-by: Tom Rini trini@konsulko.com
Hi Ilias,
On Mon, 27 Nov 2023 at 10:11, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
If a value is not valid during the DT or SYSINFO parsing, we explicitly set that to "Unknown Product" and "Unknown" for the product and manufacturer respectively. It's cleaner if we move the checks insisde smbios_add_string() and always report "Unknown" regardless of the missing field.
pre-patch dmidecode
<snip> Handle 0x0001, DMI type 1, 27 bytes System Information Manufacturer: Unknown Product Name: Unknown Product Version: Not Specified Serial Number: Not Specified UUID: Not Settable Wake-up Type: Reserved SKU Number: Not Specified Family: Not Specified
[...]
post-patch dmidecode:
Handle 0x0001, DMI type 1, 27 bytes System Information Manufacturer: Unknown Product Name: Unknown Version: Unknown Serial Number: Unknown UUID: Not Settable Wake-up Type: Reserved SKU Number: Unknown Family: Unknown [...]
Signed-off-by: Ilias Apalodimas ilias.apalodimas@linaro.org Reviewed-by: Peter Robinson pbrobinson@gmail.com Tested-by: Peter Robinson pbrobinson@gmail.com
Changes since v1:
- None
lib/smbios.c | 17 +++-------------- 1 file changed, 3 insertions(+), 14 deletions(-)
diff --git a/lib/smbios.c b/lib/smbios.c index d7f4999e8b2a..fcc8686993ef 100644 --- a/lib/smbios.c +++ b/lib/smbios.c @@ -102,7 +102,7 @@ static int smbios_add_string(struct smbios_ctx *ctx, const char *str)
I suggest that this function have an additional str property indicating the default string. Then you can pass DEFAULT_VAL or something like that, to each call.
int i = 1; char *p = ctx->eos;
if (!*str)
if (!str || !*str)
Does it ever happen that the string is present but empty?
str = "Unknown"; for (;;) {@@ -151,8 +151,7 @@ static int smbios_add_prop_si(struct smbios_ctx *ctx, const char *prop, const char *str;
str = ofnode_read_string(ctx->node, prop);
if (str)return smbios_add_string(ctx, str);
return smbios_add_string(ctx, str); } return 0;@@ -231,7 +230,7 @@ static int smbios_write_type0(ulong *current, int handle, t->vendor = smbios_add_string(ctx, "U-Boot");
t->bios_ver = smbios_add_prop(ctx, "version");
if (!t->bios_ver)
if (!strcmp(ctx->last_str, "Unknown"))
That is really ugly...checking the ctx value looking for a side effect.
Can you not have smbios_add_prop() continue to return NULL in this case?
t->bios_ver = smbios_add_string(ctx, PLAIN_VERSION); if (t->bios_ver) gd->smbios_version = ctx->last_str;@@ -281,11 +280,7 @@ static int smbios_write_type1(ulong *current, int handle, fill_smbios_header(t, SMBIOS_SYSTEM_INFORMATION, len, handle); smbios_set_eos(ctx, t->eos); t->manufacturer = smbios_add_prop(ctx, "manufacturer");
if (!t->manufacturer)t->manufacturer = smbios_add_string(ctx, "Unknown"); t->product_name = smbios_add_prop(ctx, "product");if (!t->product_name)t->product_name = smbios_add_string(ctx, "Unknown Product"); t->version = smbios_add_prop_si(ctx, "version", SYSINFO_ID_SMBIOS_SYSTEM_VERSION); if (serial_str) {@@ -315,11 +310,7 @@ static int smbios_write_type2(ulong *current, int handle, fill_smbios_header(t, SMBIOS_BOARD_INFORMATION, len, handle); smbios_set_eos(ctx, t->eos); t->manufacturer = smbios_add_prop(ctx, "manufacturer");
if (!t->manufacturer)t->manufacturer = smbios_add_string(ctx, "Unknown"); t->product_name = smbios_add_prop(ctx, "product");if (!t->product_name)t->product_name = smbios_add_string(ctx, "Unknown Product"); t->version = smbios_add_prop_si(ctx, "version", SYSINFO_ID_SMBIOS_BASEBOARD_VERSION); t->asset_tag_number = smbios_add_prop(ctx, "asset-tag");@@ -344,8 +335,6 @@ static int smbios_write_type3(ulong *current, int handle, fill_smbios_header(t, SMBIOS_SYSTEM_ENCLOSURE, len, handle); smbios_set_eos(ctx, t->eos); t->manufacturer = smbios_add_prop(ctx, "manufacturer");
if (!t->manufacturer)t->manufacturer = smbios_add_string(ctx, "Unknown"); t->chassis_type = SMBIOS_ENCLOSURE_DESKTOP; t->bootup_state = SMBIOS_STATE_SAFE; t->power_supply_state = SMBIOS_STATE_SAFE;-- 2.40.1
Regards, Simon
Hi Simon,
On Thu, 30 Nov 2023 at 04:46, Simon Glass sjg@chromium.org wrote:
Hi Ilias,
On Mon, 27 Nov 2023 at 10:11, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
If a value is not valid during the DT or SYSINFO parsing, we explicitly set that to "Unknown Product" and "Unknown" for the product and manufacturer respectively. It's cleaner if we move the checks insisde smbios_add_string() and always report "Unknown" regardless of the missing field.
pre-patch dmidecode
<snip> Handle 0x0001, DMI type 1, 27 bytes System Information Manufacturer: Unknown Product Name: Unknown Product Version: Not Specified Serial Number: Not Specified UUID: Not Settable Wake-up Type: Reserved SKU Number: Not Specified Family: Not Specified
[...]
post-patch dmidecode:
Handle 0x0001, DMI type 1, 27 bytes System Information Manufacturer: Unknown Product Name: Unknown Version: Unknown Serial Number: Unknown UUID: Not Settable Wake-up Type: Reserved SKU Number: Unknown Family: Unknown [...]
Signed-off-by: Ilias Apalodimas ilias.apalodimas@linaro.org Reviewed-by: Peter Robinson pbrobinson@gmail.com Tested-by: Peter Robinson pbrobinson@gmail.com
Changes since v1:
- None
lib/smbios.c | 17 +++-------------- 1 file changed, 3 insertions(+), 14 deletions(-)
diff --git a/lib/smbios.c b/lib/smbios.c index d7f4999e8b2a..fcc8686993ef 100644 --- a/lib/smbios.c +++ b/lib/smbios.c @@ -102,7 +102,7 @@ static int smbios_add_string(struct smbios_ctx *ctx,
const char *str)
I suggest that this function have an additional str property indicating the default string. Then you can pass DEFAULT_VAL or something like that, to each call.
Why? Do you think we need to fill in something different that "unknown"?
int i = 1; char *p = ctx->eos;
if (!*str)
if (!str || !*str)Does it ever happen that the string is present but empty?
With the changes on this patchset yes.
str = "Unknown"; for (;;) {@@ -151,8 +151,7 @@ static int smbios_add_prop_si(struct smbios_ctx
*ctx, const char *prop,
const char *str; str = ofnode_read_string(ctx->node, prop);
if (str)return smbios_add_string(ctx, str);
return smbios_add_string(ctx, str); } return 0;@@ -231,7 +230,7 @@ static int smbios_write_type0(ulong *current, int
handle,
t->vendor = smbios_add_string(ctx, "U-Boot"); t->bios_ver = smbios_add_prop(ctx, "version");
if (!t->bios_ver)
if (!strcmp(ctx->last_str, "Unknown"))That is really ugly...checking the ctx value looking for a side effect.
Can you not have smbios_add_prop() continue to return NULL in this case?
Hmm I don't know, but I wonder why I am not just checking t->bios_ver for Unknown. I'll have a look and change it
[...]
Thanks /Ilias
[...]
str = "Unknown"; for (;;) {@@ -151,8 +151,7 @@ static int smbios_add_prop_si(struct smbios_ctx *ctx, const char *prop, const char *str;
str = ofnode_read_string(ctx->node, prop);
if (str)return smbios_add_string(ctx, str);
return smbios_add_string(ctx, str); } return 0;@@ -231,7 +230,7 @@ static int smbios_write_type0(ulong *current, int handle, t->vendor = smbios_add_string(ctx, "U-Boot");
t->bios_ver = smbios_add_prop(ctx, "version");
if (!t->bios_ver)
if (!strcmp(ctx->last_str, "Unknown"))That is really ugly...checking the ctx value looking for a side effect.
Can you not have smbios_add_prop() continue to return NULL in this case?
Hmm I don't know, but I wonder why I am not just checking t->bios_ver for Unknown. I'll have a look and change it
Ok, this can't be changed as easily. smbios_add_prop() will not return NULL in any case. It returns an integer. With the cleanup, it will always writes 'Uknown' and not return 0 anymore. I can add that default value you suggested but the ctx->last_str is still used on the next line anyway.
Thanks /Ilias
[...]
Thanks /Ilias
Hi Ilias,
On Wed, 6 Dec 2023 at 04:36, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
[...]
str = "Unknown"; for (;;) {@@ -151,8 +151,7 @@ static int smbios_add_prop_si(struct smbios_ctx *ctx, const char *prop, const char *str;
str = ofnode_read_string(ctx->node, prop);
if (str)return smbios_add_string(ctx, str);
return smbios_add_string(ctx, str); } return 0;@@ -231,7 +230,7 @@ static int smbios_write_type0(ulong *current, int handle, t->vendor = smbios_add_string(ctx, "U-Boot");
t->bios_ver = smbios_add_prop(ctx, "version");
if (!t->bios_ver)
if (!strcmp(ctx->last_str, "Unknown"))That is really ugly...checking the ctx value looking for a side effect.
Can you not have smbios_add_prop() continue to return NULL in this case?
Hmm I don't know, but I wonder why I am not just checking t->bios_ver for Unknown. I'll have a look and change it
Ok, this can't be changed as easily. smbios_add_prop() will not return NULL in any case. It returns an integer. With the cleanup, it will always writes 'Uknown' and not return 0 anymore. I can add that default value you suggested but the ctx->last_str is still used on the next line anyway.
Would you mind if I tried to create a version of the patch which does the same thing but with less code, and perhaps a test? It might be easier to discuss it then. I can't claim to understand all the different code paths at this point.
My main concern is that with so many code paths it will be hard even to refactor the code in the future, since it will become 'load-bearing' and anyone might turn up and say it breaks their board because different information is provided.
Overall, so long as the information isn't used for anything important in userspace, and we find a way to report SMBIOS to Linux without EFI, it is OK to enable this feature (with a new Kconfig so it can be disabled). But there is already authoritative info in the DT, so this transformation into SMBIOS really should just be used for user display, etc., not for anything which affects operation of the device. Do you agree? If you do, how to ensure that? Perhaps a special string in the table like 'GENERATED'?
Regards, Simon
Hi Simon,
On Mon, Dec 18, 2023 at 3:02 PM Simon Glass sjg@chromium.org wrote:
Hi Ilias,
On Wed, 6 Dec 2023 at 04:36, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
[...]
str = "Unknown"; for (;;) {@@ -151,8 +151,7 @@ static int smbios_add_prop_si(struct smbios_ctx *ctx, const char *prop, const char *str;
str = ofnode_read_string(ctx->node, prop);
if (str)return smbios_add_string(ctx, str);
return smbios_add_string(ctx, str); } return 0;@@ -231,7 +230,7 @@ static int smbios_write_type0(ulong *current, int handle, t->vendor = smbios_add_string(ctx, "U-Boot");
t->bios_ver = smbios_add_prop(ctx, "version");
if (!t->bios_ver)
if (!strcmp(ctx->last_str, "Unknown"))That is really ugly...checking the ctx value looking for a side effect.
Can you not have smbios_add_prop() continue to return NULL in this case?
Hmm I don't know, but I wonder why I am not just checking t->bios_ver for Unknown. I'll have a look and change it
Ok, this can't be changed as easily. smbios_add_prop() will not return NULL in any case. It returns an integer. With the cleanup, it will always writes 'Uknown' and not return 0 anymore. I can add that default value you suggested but the ctx->last_str is still used on the next line anyway.
Would you mind if I tried to create a version of the patch which does the same thing but with less code, and perhaps a test? It might be easier to discuss it then. I can't claim to understand all the different code paths at this point.
My main concern is that with so many code paths it will be hard even to refactor the code in the future, since it will become 'load-bearing' and anyone might turn up and say it breaks their board because different information is provided.
I don't buy this argument at all, sorry.
Overall, so long as the information isn't used for anything important in userspace, and we find a way to report SMBIOS to Linux without EFI,
No, you can't tie random requirements to improving the SMBIOS support. We *already* report SMBIOS to Linux, reporting SMBIOS to Linux without EFI is changing things that will need different or extra standards, that could take years.
You are arbitrarily adding extra requirements just to suite yourself, please STOP trying to hold things like this hostage.
it is OK to enable this feature (with a new Kconfig so it can be disabled). But there is already authoritative info in the DT, so this
There is two types of information in DT, the smbios "entries" in DT are not standardised in the dtschema and in most cases they're unnecessarily replicating data ALREADY in DT which is being produced automatically with these patches, making it zero effort for vendors to produce.
transformation into SMBIOS really should just be used for user display, etc., not for anything which affects operation of the device.
Well SMBIOS tables are used for a number of different things already in the kernel.
Do you agree? If you do, how to ensure that? Perhaps a special string in the table like 'GENERATED'?
Nope, I do not agree, at all.
Regards, Peter
Hi Peter,
On Tue, 19 Dec 2023 at 13:40, Peter Robinson pbrobinson@gmail.com wrote:
Hi Simon,
On Mon, Dec 18, 2023 at 3:02 PM Simon Glass sjg@chromium.org wrote:
Hi Ilias,
On Wed, 6 Dec 2023 at 04:36, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
[...]
str = "Unknown"; for (;;) {@@ -151,8 +151,7 @@ static int smbios_add_prop_si(struct smbios_ctx *ctx, const char *prop, const char *str;
str = ofnode_read_string(ctx->node, prop);
if (str)return smbios_add_string(ctx, str);
return smbios_add_string(ctx, str); } return 0;@@ -231,7 +230,7 @@ static int smbios_write_type0(ulong *current, int handle, t->vendor = smbios_add_string(ctx, "U-Boot");
t->bios_ver = smbios_add_prop(ctx, "version");
if (!t->bios_ver)
if (!strcmp(ctx->last_str, "Unknown"))That is really ugly...checking the ctx value looking for a side effect.
Can you not have smbios_add_prop() continue to return NULL in this case?
Hmm I don't know, but I wonder why I am not just checking t->bios_ver for Unknown. I'll have a look and change it
Ok, this can't be changed as easily. smbios_add_prop() will not return NULL in any case. It returns an integer. With the cleanup, it will always writes 'Uknown' and not return 0 anymore. I can add that default value you suggested but the ctx->last_str is still used on the next line anyway.
Would you mind if I tried to create a version of the patch which does the same thing but with less code, and perhaps a test? It might be easier to discuss it then. I can't claim to understand all the different code paths at this point.
My main concern is that with so many code paths it will be hard even to refactor the code in the future, since it will become 'load-bearing' and anyone might turn up and say it breaks their board because different information is provided.
I don't buy this argument at all, sorry.
OK.
Overall, so long as the information isn't used for anything important in userspace, and we find a way to report SMBIOS to Linux without EFI,
No, you can't tie random requirements to improving the SMBIOS support. We *already* report SMBIOS to Linux, reporting SMBIOS to Linux without EFI is changing things that will need different or extra standards, that could take years.
You are arbitrarily adding extra requirements just to suite yourself, please STOP trying to hold things like this hostage.
Isn't that what is happening with Linux and ffi? My understanding is that there is no way to pass SMBIOS to Linux without EFI. Is that correct? I know some people at their wit's end about that.
You may know that I have tried for years to get bindings upstream to Linux....right now there is a reserved-memory binding which has been held up for longer than I can remember, because of EFI. How about a little give and take?
it is OK to enable this feature (with a new Kconfig so it can be disabled). But there is already authoritative info in the DT, so this
There is two types of information in DT, the smbios "entries" in DT are not standardised in the dtschema and in most cases they're unnecessarily replicating data ALREADY in DT which is being produced automatically with these patches, making it zero effort for vendors to produce.
transformation into SMBIOS really should just be used for user display, etc., not for anything which affects operation of the device.
Well SMBIOS tables are used for a number of different things already in the kernel.
Do you agree? If you do, how to ensure that? Perhaps a special string in the table like 'GENERATED'?
Nope, I do not agree, at all.
OK, well there it is.
Anyway, as I said, I am happy for this to go in with a Kconfig controlling it (so it can be enabled/disabled). Then SoCs that don't want to go to the bother of adding authoritative info can just enable this Kconfig.
I would very much like to see some signal that it is not authoritative. That should not be a big imposition.
For my own interest, I would like to understand what actually uses it as I suspect it is just for display. The two programs I managed to find both handle devicetree and don't need SMBIOS.
Regards, Simon
On Wed, Dec 20, 2023 at 10:32:26AM -0700, Simon Glass wrote:
Hi Peter,
On Tue, 19 Dec 2023 at 13:40, Peter Robinson pbrobinson@gmail.com wrote:
Hi Simon,
On Mon, Dec 18, 2023 at 3:02 PM Simon Glass sjg@chromium.org wrote:
Hi Ilias,
On Wed, 6 Dec 2023 at 04:36, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
[...]
> str = "Unknown"; > > for (;;) { > @@ -151,8 +151,7 @@ static int smbios_add_prop_si(struct smbios_ctx *ctx, const char *prop, > const char *str; > > str = ofnode_read_string(ctx->node, prop); > - if (str) > - return smbios_add_string(ctx, str); > + return smbios_add_string(ctx, str); > } > > return 0; > @@ -231,7 +230,7 @@ static int smbios_write_type0(ulong *current, int handle, > t->vendor = smbios_add_string(ctx, "U-Boot"); > > t->bios_ver = smbios_add_prop(ctx, "version"); > - if (!t->bios_ver) > + if (!strcmp(ctx->last_str, "Unknown"))
That is really ugly...checking the ctx value looking for a side effect.
Can you not have smbios_add_prop() continue to return NULL in this case?
Hmm I don't know, but I wonder why I am not just checking t->bios_ver for Unknown. I'll have a look and change it
Ok, this can't be changed as easily. smbios_add_prop() will not return NULL in any case. It returns an integer. With the cleanup, it will always writes 'Uknown' and not return 0 anymore. I can add that default value you suggested but the ctx->last_str is still used on the next line anyway.
Would you mind if I tried to create a version of the patch which does the same thing but with less code, and perhaps a test? It might be easier to discuss it then. I can't claim to understand all the different code paths at this point.
My main concern is that with so many code paths it will be hard even to refactor the code in the future, since it will become 'load-bearing' and anyone might turn up and say it breaks their board because different information is provided.
I don't buy this argument at all, sorry.
OK.
Overall, so long as the information isn't used for anything important in userspace, and we find a way to report SMBIOS to Linux without EFI,
No, you can't tie random requirements to improving the SMBIOS support. We *already* report SMBIOS to Linux, reporting SMBIOS to Linux without EFI is changing things that will need different or extra standards, that could take years.
You are arbitrarily adding extra requirements just to suite yourself, please STOP trying to hold things like this hostage.
Isn't that what is happening with Linux and ffi? My understanding is that there is no way to pass SMBIOS to Linux without EFI. Is that correct? I know some people at their wit's end about that.
You may know that I have tried for years to get bindings upstream to Linux....right now there is a reserved-memory binding which has been held up for longer than I can remember, because of EFI. How about a little give and take?
You aren't actually saying that since some patches are being held up by "but what about EFI?" you're trying to hold this up as retaliation with "but what about without EFI?". Right? And aside, I can point you at other people that got fed up with trying to make ARM+(EFI+ACPI) work because the feedback was "but what about DT?".
it is OK to enable this feature (with a new Kconfig so it can be disabled). But there is already authoritative info in the DT, so this
There is two types of information in DT, the smbios "entries" in DT are not standardised in the dtschema and in most cases they're unnecessarily replicating data ALREADY in DT which is being produced automatically with these patches, making it zero effort for vendors to produce.
transformation into SMBIOS really should just be used for user display, etc., not for anything which affects operation of the device.
Well SMBIOS tables are used for a number of different things already in the kernel.
Do you agree? If you do, how to ensure that? Perhaps a special string in the table like 'GENERATED'?
Nope, I do not agree, at all.
OK, well there it is.
Anyway, as I said, I am happy for this to go in with a Kconfig controlling it (so it can be enabled/disabled). Then SoCs that don't want to go to the bother of adding authoritative info can just enable this Kconfig.
I would very much like to see some signal that it is not authoritative. That should not be a big imposition.
Lets try a different track here. We've had 3 years now of the u-boot,sysinfo-smbios binding now. And there's been 14 ARM platforms that tried to fill it out. With Ilias' series, now there's a much larger chance that someone will see values populated and want to update them, rather than "Unknown" and assume there's no way to have them populated.
For my own interest, I would like to understand what actually uses it as I suspect it is just for display. The two programs I managed to find both handle devicetree and don't need SMBIOS.
Please re-read the examples where people have already explained where they're used, today.
Hi Simon,
Hmm I don't know, but I wonder why I am not just checking t->bios_ver for Unknown. I'll have a look and change it
Ok, this can't be changed as easily. smbios_add_prop() will not return NULL in any case. It returns an integer. With the cleanup, it will always writes 'Uknown' and not return 0 anymore. I can add that default value you suggested but the ctx->last_str is still used on the next line anyway.
Would you mind if I tried to create a version of the patch which does the same thing but with less code, and perhaps a test? It might be easier to discuss it then. I can't claim to understand all the different code paths at this point.
My main concern is that with so many code paths it will be hard even to refactor the code in the future, since it will become 'load-bearing' and anyone might turn up and say it breaks their board because different information is provided.
I don't buy this argument at all, sorry.
OK.
Overall, so long as the information isn't used for anything important in userspace, and we find a way to report SMBIOS to Linux without EFI,
No, you can't tie random requirements to improving the SMBIOS support. We *already* report SMBIOS to Linux, reporting SMBIOS to Linux without EFI is changing things that will need different or extra standards, that could take years.
You are arbitrarily adding extra requirements just to suite yourself, please STOP trying to hold things like this hostage.
Isn't that what is happening with Linux and ffi? My understanding is that there is no way to pass SMBIOS to Linux without EFI. Is that correct? I know some people at their wit's end about that.
Maybe the uses that want to go from a minimal firmware straight to a minimal kernel don't care about SMBIOS tables for their use cases, things don't need to be full parity to move forward the existing well defined usecase.
You may know that I have tried for years to get bindings upstream to Linux....right now there is a reserved-memory binding which has been held up for longer than I can remember, because of EFI. How about a little give and take?
I actually caught up on the reserved-memory binding thread a week or so ago and my general thoughts from that thread was that there was a lot of outstanding questions being asked of you that you haven't clearly articulated or even replied to and the thread ended with you asking a number of times "can this be merged now?" and my thought at the time was "No, because there's a bunch of outstanding details". May I suggest you re-read that thread and take some notes while you do so and make sure all the outstanding questions have been answered and reply with a single response addressing the remaining details, from there we may be able to move on.
it is OK to enable this feature (with a new Kconfig so it can be disabled). But there is already authoritative info in the DT, so this
There is two types of information in DT, the smbios "entries" in DT are not standardised in the dtschema and in most cases they're unnecessarily replicating data ALREADY in DT which is being produced automatically with these patches, making it zero effort for vendors to produce.
transformation into SMBIOS really should just be used for user display, etc., not for anything which affects operation of the device.
Well SMBIOS tables are used for a number of different things already in the kernel.
Do you agree? If you do, how to ensure that? Perhaps a special string in the table like 'GENERATED'?
Nope, I do not agree, at all.
OK, well there it is.
Anyway, as I said, I am happy for this to go in with a Kconfig controlling it (so it can be enabled/disabled). Then SoCs that don't want to go to the bother of adding authoritative info can just enable this Kconfig.
I would very much like to see some signal that it is not authoritative. That should not be a big imposition.
For my own interest, I would like to understand what actually uses it as I suspect it is just for display. The two programs I managed to find both handle devicetree and don't need SMBIOS.
Regards, Simon
Hi Peter,
On Thu, Dec 21, 2023 at 9:13 AM Peter Robinson pbrobinson@gmail.com wrote:
Hi Simon,
Hmm I don't know, but I wonder why I am not just checking t->bios_ver for Unknown. I'll have a look and change it
Ok, this can't be changed as easily. smbios_add_prop() will not return NULL in any case. It returns an integer. With the cleanup, it will always writes 'Uknown' and not return 0 anymore. I can add that default value you suggested but the ctx->last_str is still used on the next line anyway.
Would you mind if I tried to create a version of the patch which does the same thing but with less code, and perhaps a test? It might be easier to discuss it then. I can't claim to understand all the different code paths at this point.
My main concern is that with so many code paths it will be hard even to refactor the code in the future, since it will become 'load-bearing' and anyone might turn up and say it breaks their board because different information is provided.
I don't buy this argument at all, sorry.
OK.
Overall, so long as the information isn't used for anything important in userspace, and we find a way to report SMBIOS to Linux without EFI,
No, you can't tie random requirements to improving the SMBIOS support. We *already* report SMBIOS to Linux, reporting SMBIOS to Linux without EFI is changing things that will need different or extra standards, that could take years.
You are arbitrarily adding extra requirements just to suite yourself, please STOP trying to hold things like this hostage.
Isn't that what is happening with Linux and ffi? My understanding is that there is no way to pass SMBIOS to Linux without EFI. Is that correct? I know some people at their wit's end about that.
Maybe the uses that want to go from a minimal firmware straight to a minimal kernel don't care about SMBIOS tables for their use cases, things don't need to be full parity to move forward the existing well defined usecase.
Yes, maybe.
You may know that I have tried for years to get bindings upstream to Linux....right now there is a reserved-memory binding which has been held up for longer than I can remember, because of EFI. How about a little give and take?
I actually caught up on the reserved-memory binding thread a week or so ago and my general thoughts from that thread was that there was a lot of outstanding questions being asked of you that you haven't clearly articulated or even replied to and the thread ended with you asking a number of times "can this be merged now?" and my thought at the time was "No, because there's a bunch of outstanding details". May I suggest you re-read that thread and take some notes while you do so and make sure all the outstanding questions have been answered and reply with a single response addressing the remaining details, from there we may be able to move on.
Note that I am just the facilitator for the binding. I volunteered to send it since I have some knowledge and experience with devicetree. I believe (from reading the thread) that at least Rob understood the use case, but has no interest in it. At one point he asked for buy-in from Tianocore/EDK2 people - they were already on cc but then started trying to answer the questions.
Also note that it has been 3 months since v7 was sent. I have just gone through the thread again. I don't see a "bunch of outstanding details". I do see a some EDK2-specific questions, if that is what you mean, but I don't know enough to answer those. The deep discussion of EDK2-specific implementation details worries me, actually, since the whole point of UPL is to be able to swap out the Platform Init and Payload.
Conceptually the binding is simple...if we need another node with a phandle we can do that. But why is it getting so stuck in the weeds?
If you think you can help on that thread, please do.
[..]
Regards, Simon
Hi Simon,
The discussion is mostly on v3 now, so I assume this is outdated?
Thanks /Ilias On Mon, 18 Dec 2023 at 17:02, Simon Glass sjg@chromium.org wrote:
Hi Ilias,
On Wed, 6 Dec 2023 at 04:36, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
[...]
str = "Unknown"; for (;;) {@@ -151,8 +151,7 @@ static int smbios_add_prop_si(struct smbios_ctx *ctx, const char *prop, const char *str;
str = ofnode_read_string(ctx->node, prop);
if (str)return smbios_add_string(ctx, str);
return smbios_add_string(ctx, str); } return 0;@@ -231,7 +230,7 @@ static int smbios_write_type0(ulong *current, int handle, t->vendor = smbios_add_string(ctx, "U-Boot");
t->bios_ver = smbios_add_prop(ctx, "version");
if (!t->bios_ver)
if (!strcmp(ctx->last_str, "Unknown"))That is really ugly...checking the ctx value looking for a side effect.
Can you not have smbios_add_prop() continue to return NULL in this case?
Hmm I don't know, but I wonder why I am not just checking t->bios_ver for Unknown. I'll have a look and change it
Ok, this can't be changed as easily. smbios_add_prop() will not return NULL in any case. It returns an integer. With the cleanup, it will always writes 'Uknown' and not return 0 anymore. I can add that default value you suggested but the ctx->last_str is still used on the next line anyway.
Would you mind if I tried to create a version of the patch which does the same thing but with less code, and perhaps a test? It might be easier to discuss it then. I can't claim to understand all the different code paths at this point.
My main concern is that with so many code paths it will be hard even to refactor the code in the future, since it will become 'load-bearing' and anyone might turn up and say it breaks their board because different information is provided.
Overall, so long as the information isn't used for anything important in userspace, and we find a way to report SMBIOS to Linux without EFI, it is OK to enable this feature (with a new Kconfig so it can be disabled). But there is already authoritative info in the DT, so this transformation into SMBIOS really should just be used for user display, etc., not for anything which affects operation of the device. Do you agree? If you do, how to ensure that? Perhaps a special string in the table like 'GENERATED'?
Regards, Simon
In order to fill in the SMBIOS tables U-Boot currently relies on a "u-boot,sysinfo-smbios" compatible node. This is fine for the boards that already include such nodes. However with some recent EFI changes, the majority of boards can boot up distros, which usually rely on things like dmidecode etc for their reporting. For boards that lack this special node the SMBIOS output looks like:
System Information Manufacturer: Unknown Product Name: Unknown Version: Unknown Serial Number: Unknown UUID: Not Settable Wake-up Type: Reserved SKU Number: Unknown Family: Unknown
This looks problematic since most of the info are "Unknown". The DT spec specifies standard properties containing relevant information like 'model' and 'compatible' for which the suggested format is <manufacturer,model>. Unfortunately the 'model' string found in DTs is usually lacking the manufacturer so we can't use it for both 'Manufacturer' and 'Product Name' SMBIOS entries reliably.
So let's add a last resort to our current smbios parsing. If none of the sysinfo properties are found, scan for those information in the root node of the device tree. Use the 'model' to fill the 'Product Name' and the first value of 'compatible' for the 'Manufacturer', since that always contains one.
pre-patch: Handle 0x0001, DMI type 1, 27 bytes System Information Manufacturer: Unknown Product Name: Unknown Version: Unknown Serial Number: 100000000bb24ceb UUID: 30303031-3030-3030-3061-613234636435 Wake-up Type: Reserved SKU Number: Unknown Family: Unknown [...]
and post patch: Handle 0x0001, DMI type 1, 27 bytes System Information Manufacturer: raspberrypi Product Name: Raspberry Pi 4 Model B Rev 1.1 Version: Unknown Serial Number: 100000000bb24ceb UUID: 30303031-3030-3030-3061-613234636435 Wake-up Type: Reserved SKU Number: Unknown Family: Unknown [...]
Signed-off-by: Ilias Apalodimas ilias.apalodimas@linaro.org --- Changes since v1: - Tokenize the DT node entry and use the appropriate value instead of the entire string - Removed Peters tested/reviewed-by tags due to the above lib/smbios.c | 94 +++++++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 90 insertions(+), 4 deletions(-)
diff --git a/lib/smbios.c b/lib/smbios.c index fcc8686993ef..423893639ff0 100644 --- a/lib/smbios.c +++ b/lib/smbios.c @@ -9,11 +9,14 @@ #include <dm.h> #include <env.h> #include <linux/stringify.h> +#include <linux/string.h> #include <mapmem.h> #include <smbios.h> #include <sysinfo.h> #include <tables_csum.h> #include <version.h> +#include <malloc.h> +#include <dm/ofnode.h> #ifdef CONFIG_CPU #include <cpu.h> #include <dm/uclass-internal.h> @@ -43,6 +46,25 @@
DECLARE_GLOBAL_DATA_PTR;
+/** + * struct map_sysinfo - Mapping of sysinfo strings to DT + * + * @sysinfo_str: sysinfo string + * @dt_str: DT string + * @max: Max index of the tokenized string to pick. Counting starts from 0 + * + */ +struct map_sysinfo { + const char *sysinfo_str; + const char *dt_str; + int max; +}; + +static const struct map_sysinfo sysinfo_to_dt[] = { + { .sysinfo_str = "product", .dt_str = "model", 2 }, + { .sysinfo_str = "manufacturer", .dt_str = "compatible", 1 }, +}; + /** * struct smbios_ctx - context for writing SMBIOS tables * @@ -87,6 +109,18 @@ struct smbios_write_method { const char *subnode_name; };
+static const struct map_sysinfo *convert_sysinfo_to_dt(const char *sysinfo_str) +{ + int i; + + for (i = 0; i < ARRAY_SIZE(sysinfo_to_dt); i++) { + if (!strcmp(sysinfo_str, sysinfo_to_dt[i].sysinfo_str)) + return &sysinfo_to_dt[i]; + } + + return NULL; +} + /** * smbios_add_string() - add a string to the string area * @@ -127,6 +161,42 @@ static int smbios_add_string(struct smbios_ctx *ctx, const char *str) } }
+/** + * get_str_from_dt - Get a substring from a DT property. + * After finding the property in the DT, the function + * will parse comma separted values and return the value. + * If nprop->max exceeds the number of comma separated + * elements the last non NULL value will be returned. + * Counting starts from zero. + * + * @nprop: sysinfo property to use + * @str: pointer to fill with data + * @size: str buffer length + */ +static +void get_str_from_dt(const struct map_sysinfo *nprop, char *str, size_t size) +{ + const char *dt_str; + int cnt = 0; + char *token; + + memset(str, 0, size); + if (!nprop || !nprop->max) + return; + + dt_str = ofnode_read_string(ofnode_root(), nprop->dt_str); + if (!dt_str) + return; + + memcpy(str, dt_str, size); + token = strtok(str, ","); + while (token && cnt < nprop->max) { + strlcpy(str, token, strlen(token) + 1); + token = strtok(NULL, ","); + cnt++; + } +} + /** * smbios_add_prop_si() - Add a property from the devicetree or sysinfo * @@ -139,19 +209,35 @@ static int smbios_add_string(struct smbios_ctx *ctx, const char *str) static int smbios_add_prop_si(struct smbios_ctx *ctx, const char *prop, int sysinfo_id) { + int ret = 0; + if (sysinfo_id && ctx->dev) { char val[SMBIOS_STR_MAX]; - int ret;
ret = sysinfo_get_str(ctx->dev, sysinfo_id, sizeof(val), val); if (!ret) return smbios_add_string(ctx, val); } + if (IS_ENABLED(CONFIG_OF_CONTROL)) { - const char *str; + const char *str = NULL; + char str_dt[128] = { 0 }; + /* + * If the node is not valid fallback and try the entire DT + * so we can at least fill in maufacturer and board type + */ + if (ofnode_valid(ctx->node)) { + str = ofnode_read_string(ctx->node, prop); + } else { + const struct map_sysinfo *nprop; + + nprop = convert_sysinfo_to_dt(prop); + get_str_from_dt(nprop, str_dt, sizeof(str_dt)); + }
- str = ofnode_read_string(ctx->node, prop); - return smbios_add_string(ctx, str); + ret = smbios_add_string(ctx, ofnode_valid(ctx->node) ? + str : (const char *)str_dt); + return ret; }
return 0; -- 2.40.1
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
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
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
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.tx...
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 separatedcomma-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 typespelling
*/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
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 27/11/2023 18:10, Ilias Apalodimas wrote:
Hi, Tom asked me to clean up and resend [0]. I've made some adjustments -- The DT nodes are tokenized and a single string is used. The DT node 'model' is used to fill the SMBIOS 'Product Name' and 'compatible' is used for 'Manufacturer'.
The reason is explained in patch#2 but the tl;dr is that model doesn't always include the manufacturer despite the suggestions of the DT spec.
[0] https://lore.kernel.org/u-boot/20220906134426.53748-1-ilias.apalodimas@linar...
Ilias Apalodimas (2): smbios: Simplify reporting of unknown values smbios: Fallback to the default DT if sysinfo nodes are missing
lib/smbios.c | 109 +++++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 92 insertions(+), 17 deletions(-)
-- 2.40.1
I did it already offline, but:
Reviewed-by: Neil Armstrong neil.armstrong@linaro.org Tested-by: Neil Armstrong neil.armstrong@linaro.org # on libretech-cc
Thanks, Neil
On Thu, 21 Dec 2023 at 11:18, neil.armstrong@linaro.org wrote:
On 27/11/2023 18:10, Ilias Apalodimas wrote:
Hi, Tom asked me to clean up and resend [0]. I've made some adjustments -- The DT nodes are tokenized and a single string is used. The DT node 'model' is used to fill the SMBIOS 'Product Name' and 'compatible' is used for 'Manufacturer'.
The reason is explained in patch#2 but the tl;dr is that model doesn't always include the manufacturer despite the suggestions of the DT spec.
[0] https://lore.kernel.org/u-boot/20220906134426.53748-1-ilias.apalodimas@linar...
Ilias Apalodimas (2): smbios: Simplify reporting of unknown values smbios: Fallback to the default DT if sysinfo nodes are missing
lib/smbios.c | 109 +++++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 92 insertions(+), 17 deletions(-)
-- 2.40.1
I did it already offline, but:
Reviewed-by: Neil Armstrong neil.armstrong@linaro.org Tested-by: Neil Armstrong neil.armstrong@linaro.org # on libretech-cc
Thanks, Neil
Thanks for testing Neil! /Ilias
participants (6)
-
Ilias Apalodimas -
Mark Kettenis -
neil.armstrong＠linaro.org -
Peter Robinson -
Simon Glass -
Tom Rini