| Message ID | 20231207091850.17776-2-ilias.apalodimas@linaro.org |
|---|---|
| State | Accepted |
| Commit | a986ccea541b8e38ca219dc142f1bfef9a93ab66 |
| Headers | show |
| Series | Provide a fallback to smbios tables | expand |
On Thu, Dec 7, 2023 at 10:17 PM 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_prop_si() and provide an alternative string in case the > primary is NULL or empty > > 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 > [...] > > While at it make smbios_add_prop_si() add a string directly if the prop > node is NULL and replace smbios_add_string() calls with > smbios_add_prop_si(ctx, NULL, ....) > > 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 v2: > - refactor even more code and remove the smbios_add_string calls from the > main code. Instead use smbios_add_prop() foir everything and add a > default value, in case the parsed one is NULL or emtpy > Changes since v1: > - None > > lib/smbios.c | 73 +++++++++++++++++++++++++--------------------------- > 1 file changed, 35 insertions(+), 38 deletions(-) > > diff --git a/lib/smbios.c b/lib/smbios.c > index d7f4999e8b2a..444aa245a273 100644 > --- a/lib/smbios.c > +++ b/lib/smbios.c > @@ -102,9 +102,6 @@ static int smbios_add_string(struct smbios_ctx *ctx, const char *str) > int i = 1; > char *p = ctx->eos; > > - if (!*str) > - str = "Unknown"; > - > for (;;) { > if (!*p) { > ctx->last_str = p; > @@ -134,11 +131,18 @@ static int smbios_add_string(struct smbios_ctx *ctx, const char *str) > * > * @ctx: context for writing the tables > * @prop: property to write > + * @dval: Default value to use if the string is not found or is empty > * Return: 0 if not found, else SMBIOS string number (1 or more) > */ > static int smbios_add_prop_si(struct smbios_ctx *ctx, const char *prop, > - int sysinfo_id) > + int sysinfo_id, const char *dval) > { > + if (!dval || !*dval) > + dval = "Unknown"; > + > + if (!prop) > + return smbios_add_string(ctx, dval); > + > if (sysinfo_id && ctx->dev) { > char val[SMBIOS_STR_MAX]; > int ret; > @@ -151,8 +155,8 @@ 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 ? str : dval); > } > > return 0; > @@ -161,12 +165,15 @@ static int smbios_add_prop_si(struct smbios_ctx *ctx, const char *prop, > /** > * smbios_add_prop() - Add a property from the devicetree > * > - * @prop: property to write > + * @prop: property to write. The default string will be written if > + * prop is NULL > + * @dval: Default value to use if the string is not found or is empty > * Return: 0 if not found, else SMBIOS string number (1 or more) > */ > -static int smbios_add_prop(struct smbios_ctx *ctx, const char *prop) > +static int smbios_add_prop(struct smbios_ctx *ctx, const char *prop, > + const char *dval) > { > - return smbios_add_prop_si(ctx, prop, SYSINFO_ID_NONE); > + return smbios_add_prop_si(ctx, prop, SYSINFO_ID_NONE, dval); > } > > static void smbios_set_eos(struct smbios_ctx *ctx, char *eos) > @@ -228,11 +235,9 @@ static int smbios_write_type0(ulong *current, int handle, > memset(t, 0, sizeof(struct smbios_type0)); > fill_smbios_header(t, SMBIOS_BIOS_INFORMATION, len, handle); > smbios_set_eos(ctx, t->eos); > - t->vendor = smbios_add_string(ctx, "U-Boot"); > + t->vendor = smbios_add_prop(ctx, NULL, "U-Boot"); > > - t->bios_ver = smbios_add_prop(ctx, "version"); > - if (!t->bios_ver) > - t->bios_ver = smbios_add_string(ctx, PLAIN_VERSION); > + t->bios_ver = smbios_add_prop(ctx, "version", PLAIN_VERSION); > if (t->bios_ver) > gd->smbios_version = ctx->last_str; > log_debug("smbios_version = %p: '%s'\n", gd->smbios_version, > @@ -241,7 +246,7 @@ static int smbios_write_type0(ulong *current, int handle, > print_buffer((ulong)gd->smbios_version, gd->smbios_version, > 1, strlen(gd->smbios_version) + 1, 0); > #endif > - t->bios_release_date = smbios_add_string(ctx, U_BOOT_DMI_DATE); > + t->bios_release_date = smbios_add_prop(ctx, NULL, U_BOOT_DMI_DATE); > #ifdef CONFIG_ROM_SIZE > t->bios_rom_size = (CONFIG_ROM_SIZE / 65536) - 1; > #endif > @@ -280,22 +285,19 @@ static int smbios_write_type1(ulong *current, int handle, > memset(t, 0, sizeof(struct smbios_type1)); > 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->manufacturer = smbios_add_prop(ctx, "manufacturer", "Unknown"); > + t->product_name = smbios_add_prop(ctx, "product", "Unknown"); > t->version = smbios_add_prop_si(ctx, "version", > - SYSINFO_ID_SMBIOS_SYSTEM_VERSION); > + SYSINFO_ID_SMBIOS_SYSTEM_VERSION, > + "Unknown"); > if (serial_str) { > - t->serial_number = smbios_add_string(ctx, serial_str); > + t->serial_number = smbios_add_prop(ctx, NULL, serial_str); > strncpy((char *)t->uuid, serial_str, sizeof(t->uuid)); > } else { > - t->serial_number = smbios_add_prop(ctx, "serial"); > + t->serial_number = smbios_add_prop(ctx, "serial", "Unknown"); > } > - t->sku_number = smbios_add_prop(ctx, "sku"); > - t->family = smbios_add_prop(ctx, "family"); > + t->sku_number = smbios_add_prop(ctx, "sku", "Unknown"); > + t->family = smbios_add_prop(ctx, "family", "Unknown"); > > len = t->length + smbios_string_table_len(ctx); > *current += len; > @@ -314,15 +316,12 @@ static int smbios_write_type2(ulong *current, int handle, > memset(t, 0, sizeof(struct smbios_type2)); > 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->manufacturer = smbios_add_prop(ctx, "manufacturer", "Unknown"); > + t->product_name = smbios_add_prop(ctx, "product", "Unknown"); > t->version = smbios_add_prop_si(ctx, "version", > - SYSINFO_ID_SMBIOS_BASEBOARD_VERSION); > - t->asset_tag_number = smbios_add_prop(ctx, "asset-tag"); > + SYSINFO_ID_SMBIOS_BASEBOARD_VERSION, > + "Unknown"); > + t->asset_tag_number = smbios_add_prop(ctx, "asset-tag", "Unknown"); > t->feature_flags = SMBIOS_BOARD_FEATURE_HOSTING; > t->board_type = SMBIOS_BOARD_MOTHERBOARD; > > @@ -343,9 +342,7 @@ static int smbios_write_type3(ulong *current, int handle, > memset(t, 0, sizeof(struct smbios_type3)); > 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->manufacturer = smbios_add_prop(ctx, "manufacturer", "Unknown"); > t->chassis_type = SMBIOS_ENCLOSURE_DESKTOP; > t->bootup_state = SMBIOS_STATE_SAFE; > t->power_supply_state = SMBIOS_STATE_SAFE; > @@ -388,8 +385,8 @@ static void smbios_write_type4_dm(struct smbios_type4 *t, > #endif > > t->processor_family = processor_family; > - t->processor_manufacturer = smbios_add_string(ctx, vendor); > - t->processor_version = smbios_add_string(ctx, name); > + t->processor_manufacturer = smbios_add_prop(ctx, NULL, vendor); > + t->processor_version = smbios_add_prop(ctx, NULL, name); > } > > static int smbios_write_type4(ulong *current, int handle, > -- > 2.40.1 >
Hi Ilias, On Thu, 7 Dec 2023 at 02:19, 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_prop_si() and provide an alternative string in case the > primary is NULL or empty > > 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 > [...] > > While at it make smbios_add_prop_si() add a string directly if the prop > node is NULL and replace smbios_add_string() calls with > smbios_add_prop_si(ctx, NULL, ....) > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > --- > Changes since v2: > - refactor even more code and remove the smbios_add_string calls from the > main code. Instead use smbios_add_prop() foir everything and add a > default value, in case the parsed one is NULL or emtpy > Changes since v1: > - None > > lib/smbios.c | 73 +++++++++++++++++++++++++--------------------------- > 1 file changed, 35 insertions(+), 38 deletions(-) > I actually think we should just stop here and first write a test for what we already have. I can take a crack at it if you like? > diff --git a/lib/smbios.c b/lib/smbios.c > index d7f4999e8b2a..444aa245a273 100644 > --- a/lib/smbios.c > +++ b/lib/smbios.c > @@ -102,9 +102,6 @@ static int smbios_add_string(struct smbios_ctx *ctx, const char *str) > int i = 1; > char *p = ctx->eos; > > - if (!*str) > - str = "Unknown"; > - > for (;;) { > if (!*p) { > ctx->last_str = p; > @@ -134,11 +131,18 @@ static int smbios_add_string(struct smbios_ctx *ctx, const char *str) > * > * @ctx: context for writing the tables > * @prop: property to write which can now be NULL? > + * @dval: Default value to use if the string is not found or is empty > * Return: 0 if not found, else SMBIOS string number (1 or more) > */ > static int smbios_add_prop_si(struct smbios_ctx *ctx, const char *prop, > - int sysinfo_id) > + int sysinfo_id, const char *dval) > { > + if (!dval || !*dval) > + dval = "Unknown"; You shouldn't need this, right? It is already the default valuie. > + > + if (!prop) > + return smbios_add_string(ctx, dval); > + > if (sysinfo_id && ctx->dev) { > char val[SMBIOS_STR_MAX]; > int ret; > @@ -151,8 +155,8 @@ 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 ? str : dval); > } > > return 0; > @@ -161,12 +165,15 @@ static int smbios_add_prop_si(struct smbios_ctx *ctx, const char *prop, > /** > * smbios_add_prop() - Add a property from the devicetree > * > - * @prop: property to write > + * @prop: property to write. The default string will be written if > + * prop is NULL > + * @dval: Default value to use if the string is not found or is empty > * Return: 0 if not found, else SMBIOS string number (1 or more) > */ > -static int smbios_add_prop(struct smbios_ctx *ctx, const char *prop) > +static int smbios_add_prop(struct smbios_ctx *ctx, const char *prop, > + const char *dval) > { > - return smbios_add_prop_si(ctx, prop, SYSINFO_ID_NONE); > + return smbios_add_prop_si(ctx, prop, SYSINFO_ID_NONE, dval); > } > > static void smbios_set_eos(struct smbios_ctx *ctx, char *eos) > @@ -228,11 +235,9 @@ static int smbios_write_type0(ulong *current, int handle, > memset(t, 0, sizeof(struct smbios_type0)); > fill_smbios_header(t, SMBIOS_BIOS_INFORMATION, len, handle); > smbios_set_eos(ctx, t->eos); > - t->vendor = smbios_add_string(ctx, "U-Boot"); > + t->vendor = smbios_add_prop(ctx, NULL, "U-Boot"); What is this doing exactly? I don't really understand this change. > > - t->bios_ver = smbios_add_prop(ctx, "version"); > - if (!t->bios_ver) > - t->bios_ver = smbios_add_string(ctx, PLAIN_VERSION); > + t->bios_ver = smbios_add_prop(ctx, "version", PLAIN_VERSION); > if (t->bios_ver) > gd->smbios_version = ctx->last_str; > log_debug("smbios_version = %p: '%s'\n", gd->smbios_version, > @@ -241,7 +246,7 @@ static int smbios_write_type0(ulong *current, int handle, > print_buffer((ulong)gd->smbios_version, gd->smbios_version, > 1, strlen(gd->smbios_version) + 1, 0); > #endif > - t->bios_release_date = smbios_add_string(ctx, U_BOOT_DMI_DATE); > + t->bios_release_date = smbios_add_prop(ctx, NULL, U_BOOT_DMI_DATE); > #ifdef CONFIG_ROM_SIZE > t->bios_rom_size = (CONFIG_ROM_SIZE / 65536) - 1; > #endif > @@ -280,22 +285,19 @@ static int smbios_write_type1(ulong *current, int handle, > memset(t, 0, sizeof(struct smbios_type1)); > 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->manufacturer = smbios_add_prop(ctx, "manufacturer", "Unknown"); > + t->product_name = smbios_add_prop(ctx, "product", "Unknown"); > t->version = smbios_add_prop_si(ctx, "version", > - SYSINFO_ID_SMBIOS_SYSTEM_VERSION); > + SYSINFO_ID_SMBIOS_SYSTEM_VERSION, > + "Unknown"); > if (serial_str) { > - t->serial_number = smbios_add_string(ctx, serial_str); > + t->serial_number = smbios_add_prop(ctx, NULL, serial_str); > strncpy((char *)t->uuid, serial_str, sizeof(t->uuid)); > } else { > - t->serial_number = smbios_add_prop(ctx, "serial"); > + t->serial_number = smbios_add_prop(ctx, "serial", "Unknown"); > } > - t->sku_number = smbios_add_prop(ctx, "sku"); > - t->family = smbios_add_prop(ctx, "family"); > + t->sku_number = smbios_add_prop(ctx, "sku", "Unknown"); > + t->family = smbios_add_prop(ctx, "family", "Unknown"); > > len = t->length + smbios_string_table_len(ctx); > *current += len; > @@ -314,15 +316,12 @@ static int smbios_write_type2(ulong *current, int handle, > memset(t, 0, sizeof(struct smbios_type2)); > 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->manufacturer = smbios_add_prop(ctx, "manufacturer", "Unknown"); > + t->product_name = smbios_add_prop(ctx, "product", "Unknown"); > t->version = smbios_add_prop_si(ctx, "version", > - SYSINFO_ID_SMBIOS_BASEBOARD_VERSION); > - t->asset_tag_number = smbios_add_prop(ctx, "asset-tag"); > + SYSINFO_ID_SMBIOS_BASEBOARD_VERSION, > + "Unknown"); > + t->asset_tag_number = smbios_add_prop(ctx, "asset-tag", "Unknown"); > t->feature_flags = SMBIOS_BOARD_FEATURE_HOSTING; > t->board_type = SMBIOS_BOARD_MOTHERBOARD; > > @@ -343,9 +342,7 @@ static int smbios_write_type3(ulong *current, int handle, > memset(t, 0, sizeof(struct smbios_type3)); > 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->manufacturer = smbios_add_prop(ctx, "manufacturer", "Unknown"); > t->chassis_type = SMBIOS_ENCLOSURE_DESKTOP; > t->bootup_state = SMBIOS_STATE_SAFE; > t->power_supply_state = SMBIOS_STATE_SAFE; > @@ -388,8 +385,8 @@ static void smbios_write_type4_dm(struct smbios_type4 *t, > #endif > > t->processor_family = processor_family; > - t->processor_manufacturer = smbios_add_string(ctx, vendor); > - t->processor_version = smbios_add_string(ctx, name); > + t->processor_manufacturer = smbios_add_prop(ctx, NULL, vendor); > + t->processor_version = smbios_add_prop(ctx, NULL, name); > } > > static int smbios_write_type4(ulong *current, int handle, > -- > 2.40.1 > From my understanding, the only thing your fallback adds is the manufacturer ('vendor' from DT compatible = "vendor,board"), and the product (from DT model = "..."). This is an awful lot of code to make that change and it seems very, confusing to me. Instead, can you do something like: if (!IS_ENABLED(CONFIG_SYSINFO)) { const char *compat = ofnode_read_string(ofnode_root(), "compatible"); const char *model = ofnode_read_string(ofnode_root(), "model"); const *p = strchr(compat, ','); char vendor[32]; *vendor = '\0'; if (p) { int len = p - compat + 1; strlcpy(vendor, compat, len); } // add these two strings to the ctx struct so that smbios_write_type1() can pass them as defaults } You still need to make the 'default' change, but much of the other code could go away? Regards, Simon
Hi Simon, On Wed, 13 Dec 2023 at 21:52, Simon Glass <sjg@chromium.org> wrote: > > Hi Ilias, > > On Thu, 7 Dec 2023 at 02:19, 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_prop_si() and provide an alternative string in case the > > primary is NULL or empty > > > > 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 > > [...] > > > > While at it make smbios_add_prop_si() add a string directly if the prop > > node is NULL and replace smbios_add_string() calls with > > smbios_add_prop_si(ctx, NULL, ....) > > > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > > --- > > Changes since v2: > > - refactor even more code and remove the smbios_add_string calls from the > > main code. Instead use smbios_add_prop() foir everything and add a > > default value, in case the parsed one is NULL or emtpy > > Changes since v1: > > - None > > > > lib/smbios.c | 73 +++++++++++++++++++++++++--------------------------- > > 1 file changed, 35 insertions(+), 38 deletions(-) > > > > I actually think we should just stop here and first write a test for > what we already have. I can take a crack at it if you like? I don't mind. As I said in a previous email I'd rather do it after the refactoring you had in mind. But I don't see why that should stop the current patch from going forward. > > > diff --git a/lib/smbios.c b/lib/smbios.c > > index d7f4999e8b2a..444aa245a273 100644 > > --- a/lib/smbios.c > > +++ b/lib/smbios.c > > @@ -102,9 +102,6 @@ static int smbios_add_string(struct smbios_ctx *ctx, const char *str) > > int i = 1; > > char *p = ctx->eos; > > > > - if (!*str) > > - str = "Unknown"; > > - > > for (;;) { > > if (!*p) { > > ctx->last_str = p; > > @@ -134,11 +131,18 @@ static int smbios_add_string(struct smbios_ctx *ctx, const char *str) > > * > > * @ctx: context for writing the tables > > * @prop: property to write > > which can now be NULL? No, it can't because smbios_add_string can't do that. See commit 00a871d34e2f0a. But instead of checking it here, I moved the code around and smbios_add_string is only called from smbios_add_prop_si instead of calling it on each failure of smbios_add_prop_si(). In that function, we make sure the value isn't NULL, apart from the ->get_str sysinfo calls -- none of these currently returns null though. I can move the checking back to where it was if to shield us again dump 'get_str' implementations, or in the future fix smbios_add_string properly, but that's not the point of this patch. > > > + * @dval: Default value to use if the string is not found or is empty > > * Return: 0 if not found, else SMBIOS string number (1 or more) > > */ > > static int smbios_add_prop_si(struct smbios_ctx *ctx, const char *prop, > > - int sysinfo_id) > > + int sysinfo_id, const char *dval) > > { > > + if (!dval || !*dval) > > + dval = "Unknown"; > > You shouldn't need this, right? It is already the default value. Unless someone misuses smbios_add_prop_si() with both the prop and dval being NULL. Also see above. > > > + > > + if (!prop) > > + return smbios_add_string(ctx, dval); > > + > > if (sysinfo_id && ctx->dev) { > > char val[SMBIOS_STR_MAX]; > > int ret; > > @@ -151,8 +155,8 @@ 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 ? str : dval); > > } > > > > return 0; > > @@ -161,12 +165,15 @@ static int smbios_add_prop_si(struct smbios_ctx *ctx, const char *prop, > > /** > > * smbios_add_prop() - Add a property from the devicetree > > * > > - * @prop: property to write > > + * @prop: property to write. The default string will be written if > > + * prop is NULL > > + * @dval: Default value to use if the string is not found or is empty > > * Return: 0 if not found, else SMBIOS string number (1 or more) > > */ > > -static int smbios_add_prop(struct smbios_ctx *ctx, const char *prop) > > +static int smbios_add_prop(struct smbios_ctx *ctx, const char *prop, > > + const char *dval) > > { > > - return smbios_add_prop_si(ctx, prop, SYSINFO_ID_NONE); > > + return smbios_add_prop_si(ctx, prop, SYSINFO_ID_NONE, dval); > > } > > > > static void smbios_set_eos(struct smbios_ctx *ctx, char *eos) > > @@ -228,11 +235,9 @@ static int smbios_write_type0(ulong *current, int handle, > > memset(t, 0, sizeof(struct smbios_type0)); > > fill_smbios_header(t, SMBIOS_BIOS_INFORMATION, len, handle); > > smbios_set_eos(ctx, t->eos); > > - t->vendor = smbios_add_string(ctx, "U-Boot"); > > + t->vendor = smbios_add_prop(ctx, NULL, "U-Boot"); > > What is this doing exactly? I don't really understand this change. Re-read the patch description please, it's explained in the last paragraph. > > > [...] > > 2.40.1 > > > > From my understanding, the only thing your fallback adds is the > manufacturer ('vendor' from DT compatible = "vendor,board"), and the > product (from DT model = "..."). This is an awful lot of code to make > that change and it seems very, confusing to me. Can we please comment on the current patchset? Future history and digging becomes a nightmare otherwise. > > Instead, can you do something like: > > if (!IS_ENABLED(CONFIG_SYSINFO)) { That beats the purpose of the series though, we want this as a *fallback* not disabled in the defconfig. > const char *compat = ofnode_read_string(ofnode_root(), "compatible"); > const char *model = ofnode_read_string(ofnode_root(), "model"); > const *p = strchr(compat, ','); No. This is just a quick and dirty patch that allows you to split on the first comma. On top of that it won't work for cases like 'model' which, most of the times, only has a single value (which is again explained in the patch description) and you have to add a bunch of ifs on the code above. You could only parse compatible only, but model tends to be way more accurate than the one added in the compatible node. The first is a full description of the device while the latter is just a trigger for driver probing etc. random example model = "Socionext Developer Box"; compatible = "socionext,developer-box", "socionext,synquacer"; > char vendor[32]; > > *vendor = '\0'; > if (p) { > int len = p - compat + 1; > > strlcpy(vendor, compat, len); > } > > // add these two strings to the ctx struct so that > smbios_write_type1() can pass them as defaults > } > > You still need to make the 'default' change, but much of the other > code could go away? > > Regards, > Simon Thanks /Ilias
Hi Ilias, On Wed, 13 Dec 2023 at 14:37, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > Hi Simon, > > On Wed, 13 Dec 2023 at 21:52, Simon Glass <sjg@chromium.org> wrote: > > > > Hi Ilias, > > > > On Thu, 7 Dec 2023 at 02:19, 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_prop_si() and provide an alternative string in case the > > > primary is NULL or empty > > > > > > 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 > > > [...] > > > > > > While at it make smbios_add_prop_si() add a string directly if the prop > > > node is NULL and replace smbios_add_string() calls with > > > smbios_add_prop_si(ctx, NULL, ....) > > > > > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > > > --- > > > Changes since v2: > > > - refactor even more code and remove the smbios_add_string calls from the > > > main code. Instead use smbios_add_prop() foir everything and add a > > > default value, in case the parsed one is NULL or emtpy > > > Changes since v1: > > > - None > > > > > > lib/smbios.c | 73 +++++++++++++++++++++++++--------------------------- > > > 1 file changed, 35 insertions(+), 38 deletions(-) > > > > > > > I actually think we should just stop here and first write a test for > > what we already have. I can take a crack at it if you like? > > I don't mind. As I said in a previous email I'd rather do it after > the refactoring you had in mind. But I don't see why that should stop > the current patch from going forward. OK, well if this series could be simplified, I don't mind either. But at present it is complicated and I think it really needs a test. > > > > > > diff --git a/lib/smbios.c b/lib/smbios.c > > > index d7f4999e8b2a..444aa245a273 100644 > > > --- a/lib/smbios.c > > > +++ b/lib/smbios.c > > > @@ -102,9 +102,6 @@ static int smbios_add_string(struct smbios_ctx *ctx, const char *str) > > > int i = 1; > > > char *p = ctx->eos; > > > > > > - if (!*str) > > > - str = "Unknown"; > > > - > > > for (;;) { > > > if (!*p) { > > > ctx->last_str = p; > > > @@ -134,11 +131,18 @@ static int smbios_add_string(struct smbios_ctx *ctx, const char *str) > > > * > > > * @ctx: context for writing the tables > > > * @prop: property to write > > > > which can now be NULL? > > No, it can't because smbios_add_string can't do that. See commit 00a871d34e2f0a. > But instead of checking it here, I moved the code around and > smbios_add_string is only called from smbios_add_prop_si instead of > calling it on each failure of smbios_add_prop_si(). In that function, > we make sure the value isn't NULL, apart from the ->get_str sysinfo > calls -- none of these currently returns null though. I can move the > checking back to where it was if to shield us again dump 'get_str' > implementations, or in the future fix smbios_add_string properly, but > that's not the point of this patch. My point was that you have code to check for !prop and do something: if (!prop) return smbios_add_string(ctx, dval); Before, we needed prop (at least if OF_CONTROL), but now prop can be NULL, so you should update the comment to mention that and explain what it means. > > > > > > + * @dval: Default value to use if the string is not found or is empty > > > * Return: 0 if not found, else SMBIOS string number (1 or more) > > > */ > > > static int smbios_add_prop_si(struct smbios_ctx *ctx, const char *prop, > > > - int sysinfo_id) > > > + int sysinfo_id, const char *dval) > > > { > > > + if (!dval || !*dval) > > > + dval = "Unknown"; > > > > You shouldn't need this, right? It is already the default value. > > Unless someone misuses smbios_add_prop_si() with both the prop and > dval being NULL. Also see above. Don't allow that, then? It seems strange to have a default value for the default value. > > > > > > + > > > + if (!prop) > > > + return smbios_add_string(ctx, dval); > > > + > > > if (sysinfo_id && ctx->dev) { > > > char val[SMBIOS_STR_MAX]; > > > int ret; > > > @@ -151,8 +155,8 @@ 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 ? str : dval); > > > } > > > > > > return 0; > > > @@ -161,12 +165,15 @@ static int smbios_add_prop_si(struct smbios_ctx *ctx, const char *prop, > > > /** > > > * smbios_add_prop() - Add a property from the devicetree > > > * > > > - * @prop: property to write > > > + * @prop: property to write. The default string will be written if > > > + * prop is NULL > > > + * @dval: Default value to use if the string is not found or is empty > > > * Return: 0 if not found, else SMBIOS string number (1 or more) > > > */ > > > -static int smbios_add_prop(struct smbios_ctx *ctx, const char *prop) > > > +static int smbios_add_prop(struct smbios_ctx *ctx, const char *prop, > > > + const char *dval) > > > { > > > - return smbios_add_prop_si(ctx, prop, SYSINFO_ID_NONE); > > > + return smbios_add_prop_si(ctx, prop, SYSINFO_ID_NONE, dval); > > > } > > > > > > static void smbios_set_eos(struct smbios_ctx *ctx, char *eos) > > > @@ -228,11 +235,9 @@ static int smbios_write_type0(ulong *current, int handle, > > > memset(t, 0, sizeof(struct smbios_type0)); > > > fill_smbios_header(t, SMBIOS_BIOS_INFORMATION, len, handle); > > > smbios_set_eos(ctx, t->eos); > > > - t->vendor = smbios_add_string(ctx, "U-Boot"); > > > + t->vendor = smbios_add_prop(ctx, NULL, "U-Boot"); > > > > What is this doing exactly? I don't really understand this change. > > Re-read the patch description please, it's explained in the last paragraph. > > > > > > > > [...] > > > > 2.40.1 > > > > > > > From my understanding, the only thing your fallback adds is the > > manufacturer ('vendor' from DT compatible = "vendor,board"), and the > > product (from DT model = "..."). This is an awful lot of code to make > > that change and it seems very, confusing to me. > > Can we please comment on the current patchset? Future history and > digging becomes a nightmare otherwise. > > > > > Instead, can you do something like: > > > > if (!IS_ENABLED(CONFIG_SYSINFO)) { > > That beats the purpose of the series though, we want this as a > *fallback* not disabled in the defconfig. > > > const char *compat = ofnode_read_string(ofnode_root(), "compatible"); > > const char *model = ofnode_read_string(ofnode_root(), "model"); > > const *p = strchr(compat, ','); > > No. This is just a quick and dirty patch that allows you to split on > the first comma. On top of that it won't work for cases like 'model' > which, most of the times, only has a single value (which is again > explained in the patch description) and you have to add a bunch of ifs > on the code above. You could only parse compatible only, but model > tends to be way more accurate than the one added in the compatible > node. The first is a full description of the device while the latter > is just a trigger for driver probing etc. > > random example > model = "Socionext Developer Box"; > compatible = "socionext,developer-box", "socionext,synquacer"; But your code does the same as my fragment above, doesn't it? Only the compatible string is split, and only the first part (before the ',') is used. For the model, the whole string is used, so having a function to split the string, which doesn't have a separator in it, seems unnecessary. I am not talking about the end result, just trying to make the code easier to understand. It is very complex right now. > > > char vendor[32]; > > > > *vendor = '\0'; > > if (p) { > > int len = p - compat + 1; > > > > strlcpy(vendor, compat, len); > > } > > > > // add these two strings to the ctx struct so that > > smbios_write_type1() can pass them as defaults > > } > > > > You still need to make the 'default' change, but much of the other > > code could go away? Regards, Simon
On Thu, 14 Dec 2023 at 00:22, Simon Glass <sjg@chromium.org> wrote: > > Hi Ilias, > > On Wed, 13 Dec 2023 at 14:37, Ilias Apalodimas > <ilias.apalodimas@linaro.org> wrote: > > > > Hi Simon, > > > > On Wed, 13 Dec 2023 at 21:52, Simon Glass <sjg@chromium.org> wrote: > > > > > > Hi Ilias, > > > > > > On Thu, 7 Dec 2023 at 02:19, 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_prop_si() and provide an alternative string in case the > > > > primary is NULL or empty > > > > > > > > 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 > > > > [...] > > > > > > > > While at it make smbios_add_prop_si() add a string directly if the prop > > > > node is NULL and replace smbios_add_string() calls with > > > > smbios_add_prop_si(ctx, NULL, ....) > > > > > > > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > > > > --- > > > > Changes since v2: > > > > - refactor even more code and remove the smbios_add_string calls from the > > > > main code. Instead use smbios_add_prop() foir everything and add a > > > > default value, in case the parsed one is NULL or emtpy > > > > Changes since v1: > > > > - None > > > > > > > > lib/smbios.c | 73 +++++++++++++++++++++++++--------------------------- > > > > 1 file changed, 35 insertions(+), 38 deletions(-) > > > > > > > > > > I actually think we should just stop here and first write a test for > > > what we already have. I can take a crack at it if you like? > > > > I don't mind. As I said in a previous email I'd rather do it after > > the refactoring you had in mind. But I don't see why that should stop > > the current patch from going forward. > > OK, well if this series could be simplified, I don't mind either. But > at present it is complicated and I think it really needs a test. > > > > > > > > > > diff --git a/lib/smbios.c b/lib/smbios.c > > > > index d7f4999e8b2a..444aa245a273 100644 > > > > --- a/lib/smbios.c > > > > +++ b/lib/smbios.c > > > > @@ -102,9 +102,6 @@ static int smbios_add_string(struct smbios_ctx *ctx, const char *str) > > > > int i = 1; > > > > char *p = ctx->eos; > > > > > > > > - if (!*str) > > > > - str = "Unknown"; > > > > - > > > > for (;;) { > > > > if (!*p) { > > > > ctx->last_str = p; > > > > @@ -134,11 +131,18 @@ static int smbios_add_string(struct smbios_ctx *ctx, const char *str) > > > > * > > > > * @ctx: context for writing the tables > > > > * @prop: property to write > > > > > > which can now be NULL? > > > > No, it can't because smbios_add_string can't do that. See commit 00a871d34e2f0a. > > But instead of checking it here, I moved the code around and > > smbios_add_string is only called from smbios_add_prop_si instead of > > calling it on each failure of smbios_add_prop_si(). In that function, > > we make sure the value isn't NULL, apart from the ->get_str sysinfo > > calls -- none of these currently returns null though. I can move the > > checking back to where it was if to shield us again dump 'get_str' > > implementations, or in the future fix smbios_add_string properly, but > > that's not the point of this patch. > > My point was that you have code to check for !prop and do something: > > if (!prop) > return smbios_add_string(ctx, dval); > > Before, we needed prop (at least if OF_CONTROL), but now prop can be > NULL, so you should update the comment to mention that and explain > what it means. Sure > > > > > > > > > > + * @dval: Default value to use if the string is not found or is empty > > > > * Return: 0 if not found, else SMBIOS string number (1 or more) > > > > */ > > > > static int smbios_add_prop_si(struct smbios_ctx *ctx, const char *prop, > > > > - int sysinfo_id) > > > > + int sysinfo_id, const char *dval) > > > > { > > > > + if (!dval || !*dval) > > > > + dval = "Unknown"; > > > > > > You shouldn't need this, right? It is already the default value. > > > > Unless someone misuses smbios_add_prop_si() with both the prop and > > dval being NULL. Also see above. > > Don't allow that, then? But I am not allowing it. Instead of returning an error though we implicitly set it to unknown, which is exactly what we used to do. > It seems strange to have a default value for the default value. That's how smbios_add_string works now. Since Heinrich found that problem we should eventually fix smbios_add_string() to return and error on NULL ptrs and empty strings . But the point of this patchset is to clean up the random ad-hoc calls of it, not fix it (yet) > > > > > > > > > > + > > > > + if (!prop) > > > > + return smbios_add_string(ctx, dval); > > > > + > > > > if (sysinfo_id && ctx->dev) { > > > > char val[SMBIOS_STR_MAX]; > > > > int ret; > > > > @@ -151,8 +155,8 @@ 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 ? str : dval); > > > > } > > > > > > > > return 0; > > > > @@ -161,12 +165,15 @@ static int smbios_add_prop_si(struct smbios_ctx *ctx, const char *prop, > > > > /** > > > > * smbios_add_prop() - Add a property from the devicetree > > > > * > > > > - * @prop: property to write > > > > + * @prop: property to write. The default string will be written if > > > > + * prop is NULL > > > > + * @dval: Default value to use if the string is not found or is empty > > > > * Return: 0 if not found, else SMBIOS string number (1 or more) > > > > */ > > > > -static int smbios_add_prop(struct smbios_ctx *ctx, const char *prop) > > > > +static int smbios_add_prop(struct smbios_ctx *ctx, const char *prop, > > > > + const char *dval) > > > > { > > > > - return smbios_add_prop_si(ctx, prop, SYSINFO_ID_NONE); > > > > + return smbios_add_prop_si(ctx, prop, SYSINFO_ID_NONE, dval); > > > > } > > > > > > > > static void smbios_set_eos(struct smbios_ctx *ctx, char *eos) > > > > @@ -228,11 +235,9 @@ static int smbios_write_type0(ulong *current, int handle, > > > > memset(t, 0, sizeof(struct smbios_type0)); > > > > fill_smbios_header(t, SMBIOS_BIOS_INFORMATION, len, handle); > > > > smbios_set_eos(ctx, t->eos); > > > > - t->vendor = smbios_add_string(ctx, "U-Boot"); > > > > + t->vendor = smbios_add_prop(ctx, NULL, "U-Boot"); > > > > > > What is this doing exactly? I don't really understand this change. > > > > Re-read the patch description please, it's explained in the last paragraph. > > > > > > > > > > > > > [...] > > > > > > 2.40.1 > > > > > > > > > > From my understanding, the only thing your fallback adds is the > > > manufacturer ('vendor' from DT compatible = "vendor,board"), and the > > > product (from DT model = "..."). This is an awful lot of code to make > > > that change and it seems very, confusing to me. > > > > Can we please comment on the current patchset? Future history and > > digging becomes a nightmare otherwise. > > > > > > > > Instead, can you do something like: > > > > > > if (!IS_ENABLED(CONFIG_SYSINFO)) { > > > > That beats the purpose of the series though, we want this as a > > *fallback* not disabled in the defconfig. > > > > > const char *compat = ofnode_read_string(ofnode_root(), "compatible"); > > > const char *model = ofnode_read_string(ofnode_root(), "model"); > > > const *p = strchr(compat, ','); > > > > No. This is just a quick and dirty patch that allows you to split on > > the first comma. On top of that it won't work for cases like 'model' > > which, most of the times, only has a single value (which is again > > explained in the patch description) and you have to add a bunch of ifs > > on the code above. You could only parse compatible only, but model > > tends to be way more accurate than the one added in the compatible > > node. The first is a full description of the device while the latter > > is just a trigger for driver probing etc. > > > > random example > > model = "Socionext Developer Box"; > > compatible = "socionext,developer-box", "socionext,synquacer"; > > But your code does the same as my fragment above, doesn't it? Only the > compatible string is split, and only the first part (before the ',') > is used. > > For the model, the whole string is used, > so having a function to split > the string, which doesn't have a separator in it, seems unnecessary. No, it's not. the spec says model = <manufacturer, model>. Some of the existing DTs follow that pattern. In that case, you need to split those as well and that code would start to look really ugly and non extendable. NXP and Qualcomm boards are just some examples > > I am not talking about the end result, just trying to make the code > easier to understand. It is very complex right now. The function has a pretty clear comment on what it tries to do. Also if we do what you suggest, adding chassis-id would mean that we need another round of strchrs and ifs and who knows what else. With the current patch, you just need to add an entry to the array something along the lines of { .sysinfo_str = "chassis", .dt_str = "chassis-type", 1} Thanks /Ilias > > > > > > > char vendor[32]; > > > > > > *vendor = '\0'; > > > if (p) { > > > int len = p - compat + 1; > > > > > > strlcpy(vendor, compat, len); > > > } > > > > > > // add these two strings to the ctx struct so that > > > smbios_write_type1() can pass them as defaults > > > } > > > > > > You still need to make the 'default' change, but much of the other > > > code could go away? > > Regards, > Simon
Hi Ilias, On Wed, 13 Dec 2023 at 15:38, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > On Thu, 14 Dec 2023 at 00:22, Simon Glass <sjg@chromium.org> wrote: > > > > Hi Ilias, > > > > On Wed, 13 Dec 2023 at 14:37, Ilias Apalodimas > > <ilias.apalodimas@linaro.org> wrote: > > > > > > Hi Simon, > > > > > > On Wed, 13 Dec 2023 at 21:52, Simon Glass <sjg@chromium.org> wrote: > > > > > > > > Hi Ilias, > > > > > > > > On Thu, 7 Dec 2023 at 02:19, 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_prop_si() and provide an alternative string in case the > > > > > primary is NULL or empty > > > > > > > > > > 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 > > > > > [...] > > > > > > > > > > While at it make smbios_add_prop_si() add a string directly if the prop > > > > > node is NULL and replace smbios_add_string() calls with > > > > > smbios_add_prop_si(ctx, NULL, ....) > > > > > > > > > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > > > > > --- > > > > > Changes since v2: > > > > > - refactor even more code and remove the smbios_add_string calls from the > > > > > main code. Instead use smbios_add_prop() foir everything and add a > > > > > default value, in case the parsed one is NULL or emtpy > > > > > Changes since v1: > > > > > - None > > > > > > > > > > lib/smbios.c | 73 +++++++++++++++++++++++++--------------------------- > > > > > 1 file changed, 35 insertions(+), 38 deletions(-) > > > > > > > > > > > > > I actually think we should just stop here and first write a test for > > > > what we already have. I can take a crack at it if you like? > > > > > > I don't mind. As I said in a previous email I'd rather do it after > > > the refactoring you had in mind. But I don't see why that should stop > > > the current patch from going forward. > > > > OK, well if this series could be simplified, I don't mind either. But > > at present it is complicated and I think it really needs a test. > > > > > > > > > > > > > > diff --git a/lib/smbios.c b/lib/smbios.c > > > > > index d7f4999e8b2a..444aa245a273 100644 > > > > > --- a/lib/smbios.c > > > > > +++ b/lib/smbios.c > > > > > @@ -102,9 +102,6 @@ static int smbios_add_string(struct smbios_ctx *ctx, const char *str) > > > > > int i = 1; > > > > > char *p = ctx->eos; > > > > > > > > > > - if (!*str) > > > > > - str = "Unknown"; > > > > > - > > > > > for (;;) { > > > > > if (!*p) { > > > > > ctx->last_str = p; > > > > > @@ -134,11 +131,18 @@ static int smbios_add_string(struct smbios_ctx *ctx, const char *str) > > > > > * > > > > > * @ctx: context for writing the tables > > > > > * @prop: property to write > > > > > > > > which can now be NULL? > > > > > > No, it can't because smbios_add_string can't do that. See commit 00a871d34e2f0a. > > > But instead of checking it here, I moved the code around and > > > smbios_add_string is only called from smbios_add_prop_si instead of > > > calling it on each failure of smbios_add_prop_si(). In that function, > > > we make sure the value isn't NULL, apart from the ->get_str sysinfo > > > calls -- none of these currently returns null though. I can move the > > > checking back to where it was if to shield us again dump 'get_str' > > > implementations, or in the future fix smbios_add_string properly, but > > > that's not the point of this patch. > > > > My point was that you have code to check for !prop and do something: > > > > if (!prop) > > return smbios_add_string(ctx, dval); > > > > Before, we needed prop (at least if OF_CONTROL), but now prop can be > > NULL, so you should update the comment to mention that and explain > > what it means. > > Sure > > > > > > > > > > > > > > > + * @dval: Default value to use if the string is not found or is empty > > > > > * Return: 0 if not found, else SMBIOS string number (1 or more) > > > > > */ > > > > > static int smbios_add_prop_si(struct smbios_ctx *ctx, const char *prop, > > > > > - int sysinfo_id) > > > > > + int sysinfo_id, const char *dval) > > > > > { > > > > > + if (!dval || !*dval) > > > > > + dval = "Unknown"; > > > > > > > > You shouldn't need this, right? It is already the default value. > > > > > > Unless someone misuses smbios_add_prop_si() with both the prop and > > > dval being NULL. Also see above. > > > > Don't allow that, then? > > But I am not allowing it. Instead of returning an error though we > implicitly set it to unknown, which is exactly what we used to do. Either: - document that dval can be NULL - don't handle it being NULL I prefer the latter, since there are two levels of default with this code. > > > It seems strange to have a default value for the default value. > > That's how smbios_add_string works now. Since Heinrich found that > problem we should eventually fix smbios_add_string() to return and > error on NULL ptrs and empty strings . But the point of this patchset > is to clean up the random ad-hoc calls of it, not fix it (yet) OK... > > > > > > > > > > > > > > > + > > > > > + if (!prop) > > > > > + return smbios_add_string(ctx, dval); > > > > > + > > > > > if (sysinfo_id && ctx->dev) { > > > > > char val[SMBIOS_STR_MAX]; > > > > > int ret; > > > > > @@ -151,8 +155,8 @@ 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 ? str : dval); > > > > > } > > > > > > > > > > return 0; > > > > > @@ -161,12 +165,15 @@ static int smbios_add_prop_si(struct smbios_ctx *ctx, const char *prop, > > > > > /** > > > > > * smbios_add_prop() - Add a property from the devicetree > > > > > * > > > > > - * @prop: property to write > > > > > + * @prop: property to write. The default string will be written if > > > > > + * prop is NULL > > > > > + * @dval: Default value to use if the string is not found or is empty > > > > > * Return: 0 if not found, else SMBIOS string number (1 or more) > > > > > */ > > > > > -static int smbios_add_prop(struct smbios_ctx *ctx, const char *prop) > > > > > +static int smbios_add_prop(struct smbios_ctx *ctx, const char *prop, > > > > > + const char *dval) > > > > > { > > > > > - return smbios_add_prop_si(ctx, prop, SYSINFO_ID_NONE); > > > > > + return smbios_add_prop_si(ctx, prop, SYSINFO_ID_NONE, dval); > > > > > } > > > > > > > > > > static void smbios_set_eos(struct smbios_ctx *ctx, char *eos) > > > > > @@ -228,11 +235,9 @@ static int smbios_write_type0(ulong *current, int handle, > > > > > memset(t, 0, sizeof(struct smbios_type0)); > > > > > fill_smbios_header(t, SMBIOS_BIOS_INFORMATION, len, handle); > > > > > smbios_set_eos(ctx, t->eos); > > > > > - t->vendor = smbios_add_string(ctx, "U-Boot"); > > > > > + t->vendor = smbios_add_prop(ctx, NULL, "U-Boot"); > > > > > > > > What is this doing exactly? I don't really understand this change. > > > > > > Re-read the patch description please, it's explained in the last paragraph. > > > > > > > > > > > > > > > > > > [...] > > > > > > > > 2.40.1 > > > > > > > > > > > > > From my understanding, the only thing your fallback adds is the > > > > manufacturer ('vendor' from DT compatible = "vendor,board"), and the > > > > product (from DT model = "..."). This is an awful lot of code to make > > > > that change and it seems very, confusing to me. > > > > > > Can we please comment on the current patchset? Future history and > > > digging becomes a nightmare otherwise. > > > > > > > > > > > Instead, can you do something like: > > > > > > > > if (!IS_ENABLED(CONFIG_SYSINFO)) { > > > > > > That beats the purpose of the series though, we want this as a > > > *fallback* not disabled in the defconfig. > > > > > > > const char *compat = ofnode_read_string(ofnode_root(), "compatible"); > > > > const char *model = ofnode_read_string(ofnode_root(), "model"); > > > > const *p = strchr(compat, ','); > > > > > > No. This is just a quick and dirty patch that allows you to split on > > > the first comma. On top of that it won't work for cases like 'model' > > > which, most of the times, only has a single value (which is again > > > explained in the patch description) and you have to add a bunch of ifs > > > on the code above. You could only parse compatible only, but model > > > tends to be way more accurate than the one added in the compatible > > > node. The first is a full description of the device while the latter > > > is just a trigger for driver probing etc. > > > > > > random example > > > model = "Socionext Developer Box"; > > > compatible = "socionext,developer-box", "socionext,synquacer"; > > > > But your code does the same as my fragment above, doesn't it? Only the > > compatible string is split, and only the first part (before the ',') > > is used. > > > > For the model, the whole string is used, > > so having a function to split > > the string, which doesn't have a separator in it, seems unnecessary. > > No, it's not. the spec says model = <manufacturer, model>. Some of the > existing DTs follow that pattern. In that case, you need to split > those as well and that code would start to look really ugly and non > extendable. NXP and Qualcomm boards are just some examples Oh that's interesting, for example: model = "Qualcomm Technologies, Inc. SM8550 QRD"; But I don't actually see any use of 'model' in Linux that has a vendor,model approach. Also the existing values are clearly indented for display to the user. I wonder if we should change the spec at this point? > > > > > I am not talking about the end result, just trying to make the code > > easier to understand. It is very complex right now. > > The function has a pretty clear comment on what it tries to do. > Also if we do what you suggest, adding chassis-id would mean that we > need another round of strchrs and ifs and who knows what else. With > the current patch, you just need to add an entry to the array > something along the lines of > { .sysinfo_str = "chassis", .dt_str = "chassis-type", 1} Yes I agree that it would be easier to do such a thing and now I understand why you have done this in the code. But why would we do that? We might as well use the proper sysinfo binding in U-Boot, instead of inventing something like this? Or would what you propose here be easier to upstream? I'm just not sure it makes sense, which is why I feel the code is over-complicated (and still has no tests even after all this work). Regards, Simon
[...] > > > > [...] > > > > > > > > > > 2.40.1 > > > > > > > > > > > > > > > > From my understanding, the only thing your fallback adds is the > > > > > manufacturer ('vendor' from DT compatible = "vendor,board"), and the > > > > > product (from DT model = "..."). This is an awful lot of code to make > > > > > that change and it seems very, confusing to me. > > > > > > > > Can we please comment on the current patchset? Future history and > > > > digging becomes a nightmare otherwise. > > > > > > > > > > > > > > Instead, can you do something like: > > > > > > > > > > if (!IS_ENABLED(CONFIG_SYSINFO)) { > > > > > > > > That beats the purpose of the series though, we want this as a > > > > *fallback* not disabled in the defconfig. > > > > > > > > > const char *compat = ofnode_read_string(ofnode_root(), "compatible"); > > > > > const char *model = ofnode_read_string(ofnode_root(), "model"); > > > > > const *p = strchr(compat, ','); > > > > > > > > No. This is just a quick and dirty patch that allows you to split on > > > > the first comma. On top of that it won't work for cases like 'model' > > > > which, most of the times, only has a single value (which is again > > > > explained in the patch description) and you have to add a bunch of ifs > > > > on the code above. You could only parse compatible only, but model > > > > tends to be way more accurate than the one added in the compatible > > > > node. The first is a full description of the device while the latter > > > > is just a trigger for driver probing etc. > > > > > > > > random example > > > > model = "Socionext Developer Box"; > > > > compatible = "socionext,developer-box", "socionext,synquacer"; > > > > > > But your code does the same as my fragment above, doesn't it? Only the > > > compatible string is split, and only the first part (before the ',') > > > is used. > > > > > > For the model, the whole string is used, > > > so having a function to split > > > the string, which doesn't have a separator in it, seems unnecessary. > > > > No, it's not. the spec says model = <manufacturer, model>. Some of the > > existing DTs follow that pattern. In that case, you need to split > > those as well and that code would start to look really ugly and non > > extendable. NXP and Qualcomm boards are just some examples > > Oh that's interesting, for example: > > model = "Qualcomm Technologies, Inc. SM8550 QRD"; > > But I don't actually see any use of 'model' in Linux that has a > vendor,model approach. Also the existing values are clearly indented > for display to the user. I wonder if we should change the spec at this > point? > I don't have a strong opinion on this. I'd prefer if we left as-is and slowly fix the existing DTs to include the manufacturer. It would be way easier to parse those values. > > > > > > > > I am not talking about the end result, just trying to make the code > > > easier to understand. It is very complex right now. > > > > The function has a pretty clear comment on what it tries to do. > > Also if we do what you suggest, adding chassis-id would mean that we > > need another round of strchrs and ifs and who knows what else. With > > the current patch, you just need to add an entry to the array > > something along the lines of > > { .sysinfo_str = "chassis", .dt_str = "chassis-type", 1} > > Yes I agree that it would be easier to do such a thing and now I > understand why you have done this in the code. But why would we do > that? We might as well use the proper sysinfo binding in U-Boot, > instead of inventing something like this? This has been discussed over and over again on why we need this. I don't see any point in repeating the arguments. FWIW the sysinfo node on u-boot is the invention and this is re-using existing DT entries that dont need extra work. > Or would what you propose > here be easier to upstream? I'm just not sure it makes sense, which is > why I feel the code is over-complicated (and still has no tests even > after all this work). I don't see how a for loop and strtok are so overcomplicated. The code *never* had any tests. In fact, it has been broken several times and Heinrich and I are the ones to fix it. No one argues the need for tests, I just don't see why this is a blocker. As far as upstreaming is concerned, I would approach the problem differently. The model, manufacturer and chassis information are already in the DT, so I don't see why we should touch that, apart from amending the existing DTs with 'better' values. For the rest of the info SMBIOS lacks (memory, cpu etc), I'd look into the existing DT spec and add the missing bits if they can't be discovered automatically. Thanks /Ilias > Regards, > Simon
diff --git a/lib/smbios.c b/lib/smbios.c index d7f4999e8b2a..444aa245a273 100644 --- a/lib/smbios.c +++ b/lib/smbios.c @@ -102,9 +102,6 @@ static int smbios_add_string(struct smbios_ctx *ctx, const char *str) int i = 1; char *p = ctx->eos; - if (!*str) - str = "Unknown"; - for (;;) { if (!*p) { ctx->last_str = p; @@ -134,11 +131,18 @@ static int smbios_add_string(struct smbios_ctx *ctx, const char *str) * * @ctx: context for writing the tables * @prop: property to write + * @dval: Default value to use if the string is not found or is empty * Return: 0 if not found, else SMBIOS string number (1 or more) */ static int smbios_add_prop_si(struct smbios_ctx *ctx, const char *prop, - int sysinfo_id) + int sysinfo_id, const char *dval) { + if (!dval || !*dval) + dval = "Unknown"; + + if (!prop) + return smbios_add_string(ctx, dval); + if (sysinfo_id && ctx->dev) { char val[SMBIOS_STR_MAX]; int ret; @@ -151,8 +155,8 @@ 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 ? str : dval); } return 0; @@ -161,12 +165,15 @@ static int smbios_add_prop_si(struct smbios_ctx *ctx, const char *prop, /** * smbios_add_prop() - Add a property from the devicetree * - * @prop: property to write + * @prop: property to write. The default string will be written if + * prop is NULL + * @dval: Default value to use if the string is not found or is empty * Return: 0 if not found, else SMBIOS string number (1 or more) */ -static int smbios_add_prop(struct smbios_ctx *ctx, const char *prop) +static int smbios_add_prop(struct smbios_ctx *ctx, const char *prop, + const char *dval) { - return smbios_add_prop_si(ctx, prop, SYSINFO_ID_NONE); + return smbios_add_prop_si(ctx, prop, SYSINFO_ID_NONE, dval); } static void smbios_set_eos(struct smbios_ctx *ctx, char *eos) @@ -228,11 +235,9 @@ static int smbios_write_type0(ulong *current, int handle, memset(t, 0, sizeof(struct smbios_type0)); fill_smbios_header(t, SMBIOS_BIOS_INFORMATION, len, handle); smbios_set_eos(ctx, t->eos); - t->vendor = smbios_add_string(ctx, "U-Boot"); + t->vendor = smbios_add_prop(ctx, NULL, "U-Boot"); - t->bios_ver = smbios_add_prop(ctx, "version"); - if (!t->bios_ver) - t->bios_ver = smbios_add_string(ctx, PLAIN_VERSION); + t->bios_ver = smbios_add_prop(ctx, "version", PLAIN_VERSION); if (t->bios_ver) gd->smbios_version = ctx->last_str; log_debug("smbios_version = %p: '%s'\n", gd->smbios_version, @@ -241,7 +246,7 @@ static int smbios_write_type0(ulong *current, int handle, print_buffer((ulong)gd->smbios_version, gd->smbios_version, 1, strlen(gd->smbios_version) + 1, 0); #endif - t->bios_release_date = smbios_add_string(ctx, U_BOOT_DMI_DATE); + t->bios_release_date = smbios_add_prop(ctx, NULL, U_BOOT_DMI_DATE); #ifdef CONFIG_ROM_SIZE t->bios_rom_size = (CONFIG_ROM_SIZE / 65536) - 1; #endif @@ -280,22 +285,19 @@ static int smbios_write_type1(ulong *current, int handle, memset(t, 0, sizeof(struct smbios_type1)); 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->manufacturer = smbios_add_prop(ctx, "manufacturer", "Unknown"); + t->product_name = smbios_add_prop(ctx, "product", "Unknown"); t->version = smbios_add_prop_si(ctx, "version", - SYSINFO_ID_SMBIOS_SYSTEM_VERSION); + SYSINFO_ID_SMBIOS_SYSTEM_VERSION, + "Unknown"); if (serial_str) { - t->serial_number = smbios_add_string(ctx, serial_str); + t->serial_number = smbios_add_prop(ctx, NULL, serial_str); strncpy((char *)t->uuid, serial_str, sizeof(t->uuid)); } else { - t->serial_number = smbios_add_prop(ctx, "serial"); + t->serial_number = smbios_add_prop(ctx, "serial", "Unknown"); } - t->sku_number = smbios_add_prop(ctx, "sku"); - t->family = smbios_add_prop(ctx, "family"); + t->sku_number = smbios_add_prop(ctx, "sku", "Unknown"); + t->family = smbios_add_prop(ctx, "family", "Unknown"); len = t->length + smbios_string_table_len(ctx); *current += len; @@ -314,15 +316,12 @@ static int smbios_write_type2(ulong *current, int handle, memset(t, 0, sizeof(struct smbios_type2)); 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->manufacturer = smbios_add_prop(ctx, "manufacturer", "Unknown"); + t->product_name = smbios_add_prop(ctx, "product", "Unknown"); t->version = smbios_add_prop_si(ctx, "version", - SYSINFO_ID_SMBIOS_BASEBOARD_VERSION); - t->asset_tag_number = smbios_add_prop(ctx, "asset-tag"); + SYSINFO_ID_SMBIOS_BASEBOARD_VERSION, + "Unknown"); + t->asset_tag_number = smbios_add_prop(ctx, "asset-tag", "Unknown"); t->feature_flags = SMBIOS_BOARD_FEATURE_HOSTING; t->board_type = SMBIOS_BOARD_MOTHERBOARD; @@ -343,9 +342,7 @@ static int smbios_write_type3(ulong *current, int handle, memset(t, 0, sizeof(struct smbios_type3)); 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->manufacturer = smbios_add_prop(ctx, "manufacturer", "Unknown"); t->chassis_type = SMBIOS_ENCLOSURE_DESKTOP; t->bootup_state = SMBIOS_STATE_SAFE; t->power_supply_state = SMBIOS_STATE_SAFE; @@ -388,8 +385,8 @@ static void smbios_write_type4_dm(struct smbios_type4 *t, #endif t->processor_family = processor_family; - t->processor_manufacturer = smbios_add_string(ctx, vendor); - t->processor_version = smbios_add_string(ctx, name); + t->processor_manufacturer = smbios_add_prop(ctx, NULL, vendor); + t->processor_version = smbios_add_prop(ctx, NULL, name); } static int smbios_write_type4(ulong *current, int handle,
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_prop_si() and provide an alternative string in case the primary is NULL or empty 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 [...] While at it make smbios_add_prop_si() add a string directly if the prop node is NULL and replace smbios_add_string() calls with smbios_add_prop_si(ctx, NULL, ....) Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> --- Changes since v2: - refactor even more code and remove the smbios_add_string calls from the main code. Instead use smbios_add_prop() foir everything and add a default value, in case the parsed one is NULL or emtpy Changes since v1: - None lib/smbios.c | 73 +++++++++++++++++++++++++--------------------------- 1 file changed, 35 insertions(+), 38 deletions(-) -- 2.40.1