| Message ID | 20231127171058.165777-2-ilias.apalodimas@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | Provide a fallback to smbios tables | expand |
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 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
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
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;
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(-) -- 2.40.1