[PATCH 2/2 v2] smbios: Fallback to the default DT if sysinfo nodes are missing
Peter Robinson
pbrobinson at gmail.com
Wed Dec 6 13:11:41 CET 2023
On Thu, Nov 30, 2023 at 2:45 AM Simon Glass <sjg at chromium.org> wrote:
>
> Hi Ilias,
>
> On Mon, 27 Nov 2023 at 10:11, Ilias Apalodimas
> <ilias.apalodimas at linaro.org> wrote:
> >
> > In order to fill in the SMBIOS tables U-Boot currently relies on a
> > "u-boot,sysinfo-smbios" compatible node. This is fine for the boards
> > that already include such nodes. However with some recent EFI changes,
> > the majority of boards can boot up distros, which usually rely on
> > things like dmidecode etc for their reporting. For boards that
> > lack this special node the SMBIOS output looks like:
> >
> > System Information
> > Manufacturer: Unknown
> > Product Name: Unknown
> > Version: Unknown
> > Serial Number: Unknown
> > UUID: Not Settable
> > Wake-up Type: Reserved
> > SKU Number: Unknown
> > Family: Unknown
> >
> > This looks problematic since most of the info are "Unknown". The DT spec
> > specifies standard properties containing relevant information like
> > 'model' and 'compatible' for which the suggested format is
> > <manufacturer,model>. Unfortunately the 'model' string found in DTs is
> > usually lacking the manufacturer so we can't use it for both
> > 'Manufacturer' and 'Product Name' SMBIOS entries reliably.
> >
> > So let's add a last resort to our current smbios parsing. If none of
> > the sysinfo properties are found, scan for those information in the
> > root node of the device tree. Use the 'model' to fill the 'Product
> > Name' and the first value of 'compatible' for the 'Manufacturer', since
> > that always contains one.
> >
> > pre-patch:
> > Handle 0x0001, DMI type 1, 27 bytes
> > System Information
> > Manufacturer: Unknown
> > Product Name: Unknown
> > Version: Unknown
> > Serial Number: 100000000bb24ceb
> > UUID: 30303031-3030-3030-3061-613234636435
> > Wake-up Type: Reserved
> > SKU Number: Unknown
> > Family: Unknown
> > [...]
> >
> > and post patch:
> > Handle 0x0001, DMI type 1, 27 bytes
> > System Information
> > Manufacturer: raspberrypi
> > Product Name: Raspberry Pi 4 Model B Rev 1.1
> > Version: Unknown
> > Serial Number: 100000000bb24ceb
> > UUID: 30303031-3030-3030-3061-613234636435
> > Wake-up Type: Reserved
> > SKU Number: Unknown
> > Family: Unknown
> > [...]
> >
> > Signed-off-by: Ilias Apalodimas <ilias.apalodimas at linaro.org>
> > ---
> > Changes since v1:
> > - Tokenize the DT node entry and use the appropriate value instead of
> > the entire string
> > - Removed Peters tested/reviewed-by tags due to the above
> > lib/smbios.c | 94 +++++++++++++++++++++++++++++++++++++++++++++++++---
> > 1 file changed, 90 insertions(+), 4 deletions(-)
> >
>
> Can this be put behind a Kconfig? It adds quite a bit of code which
> punishes those boards which do the right thing.
What's the size difference? By putting it behind a Kconfig you're
assuming the boards that don't do the right thing are going to enable
this at which point we punish users by not having some level of
usability. By "right thing" do you mean the smbios entries in the
-u-boot.dtsi?
> > diff --git a/lib/smbios.c b/lib/smbios.c
> > index fcc8686993ef..423893639ff0 100644
> > --- a/lib/smbios.c
> > +++ b/lib/smbios.c
> > @@ -9,11 +9,14 @@
> > #include <dm.h>
> > #include <env.h>
> > #include <linux/stringify.h>
> > +#include <linux/string.h>
> > #include <mapmem.h>
> > #include <smbios.h>
> > #include <sysinfo.h>
> > #include <tables_csum.h>
> > #include <version.h>
> > +#include <malloc.h>
> > +#include <dm/ofnode.h>
> > #ifdef CONFIG_CPU
> > #include <cpu.h>
> > #include <dm/uclass-internal.h>
> > @@ -43,6 +46,25 @@
> >
> > DECLARE_GLOBAL_DATA_PTR;
> >
> > +/**
> > + * struct map_sysinfo - Mapping of sysinfo strings to DT
> > + *
> > + * @sysinfo_str: sysinfo string
> > + * @dt_str: DT string
> > + * @max: Max index of the tokenized string to pick. Counting starts from 0
> > + *
> > + */
> > +struct map_sysinfo {
> > + const char *sysinfo_str;
> > + const char *dt_str;
> > + int max;
> > +};
> > +
> > +static const struct map_sysinfo sysinfo_to_dt[] = {
> > + { .sysinfo_str = "product", .dt_str = "model", 2 },
> > + { .sysinfo_str = "manufacturer", .dt_str = "compatible", 1 },
> > +};
> > +
> > /**
> > * struct smbios_ctx - context for writing SMBIOS tables
> > *
> > @@ -87,6 +109,18 @@ struct smbios_write_method {
> > const char *subnode_name;
> > };
> >
> > +static const struct map_sysinfo *convert_sysinfo_to_dt(const char *sysinfo_str)
> > +{
> > + int i;
> > +
> > + for (i = 0; i < ARRAY_SIZE(sysinfo_to_dt); i++) {
> > + if (!strcmp(sysinfo_str, sysinfo_to_dt[i].sysinfo_str))
> > + return &sysinfo_to_dt[i];
> > + }
> > +
> > + return NULL;
> > +}
> > +
> > /**
> > * smbios_add_string() - add a string to the string area
> > *
> > @@ -127,6 +161,42 @@ static int smbios_add_string(struct smbios_ctx *ctx, const char *str)
> > }
> > }
> >
> > +/**
> > + * get_str_from_dt - Get a substring from a DT property.
> > + * After finding the property in the DT, the function
> > + * will parse comma separted values and return the value.
>
> spelling
>
> > + * If nprop->max exceeds the number of comma separated
>
> comma-separated
>
> > + * elements the last non NULL value will be returned.
>
> elements, the
>
> > + * Counting starts from zero.
> > + *
> > + * @nprop: sysinfo property to use
> > + * @str: pointer to fill with data
> > + * @size: str buffer length
> > + */
> > +static
> > +void get_str_from_dt(const struct map_sysinfo *nprop, char *str, size_t size)
> > +{
> > + const char *dt_str;
> > + int cnt = 0;
> > + char *token;
> > +
> > + memset(str, 0, size);
> > + if (!nprop || !nprop->max)
> > + return;
> > +
> > + dt_str = ofnode_read_string(ofnode_root(), nprop->dt_str);
>
> Could this use ofnode_read_string_index() ?
>
> > + if (!dt_str)
> > + return;
> > +
> > + memcpy(str, dt_str, size);
> > + token = strtok(str, ",");
> > + while (token && cnt < nprop->max) {
> > + strlcpy(str, token, strlen(token) + 1);
> > + token = strtok(NULL, ",");
> > + cnt++;
> > + }
> > +}
> > +
> > /**
> > * smbios_add_prop_si() - Add a property from the devicetree or sysinfo
> > *
> > @@ -139,19 +209,35 @@ static int smbios_add_string(struct smbios_ctx *ctx, const char *str)
> > static int smbios_add_prop_si(struct smbios_ctx *ctx, const char *prop,
> > int sysinfo_id)
> > {
> > + int ret = 0;
> > +
> > if (sysinfo_id && ctx->dev) {
> > char val[SMBIOS_STR_MAX];
> > - int ret;
> >
> > ret = sysinfo_get_str(ctx->dev, sysinfo_id, sizeof(val), val);
> > if (!ret)
> > return smbios_add_string(ctx, val);
> > }
> > +
> > if (IS_ENABLED(CONFIG_OF_CONTROL)) {
> > - const char *str;
> > + const char *str = NULL;
> > + char str_dt[128] = { 0 };
> > + /*
> > + * If the node is not valid fallback and try the entire DT
> > + * so we can at least fill in maufacturer and board type
>
> spelling
>
> > + */
> > + if (ofnode_valid(ctx->node)) {
> > + str = ofnode_read_string(ctx->node, prop);
> > + } else {
> > + const struct map_sysinfo *nprop;
> > +
> > + nprop = convert_sysinfo_to_dt(prop);
> > + get_str_from_dt(nprop, str_dt, sizeof(str_dt));
> > + }
> >
> > - str = ofnode_read_string(ctx->node, prop);
> > - return smbios_add_string(ctx, str);
> > + ret = smbios_add_string(ctx, ofnode_valid(ctx->node) ?
> > + str : (const char *)str_dt);
> > + return ret;
> > }
> >
> > return 0;
> > --
> > 2.40.1
> >
>
> Any chance of a test for this code?
>
> Regards,
> Simon
More information about the U-Boot
mailing list