[PATCH 1/2 v3] smbios: Simplify reporting of unknown values

[Ilias Apalodimas]

Hi Simon,

On Wed, 13 Dec 2023 at 21:52, Simon Glass <sjg at chromium.org> wrote:
>
> Hi Ilias,
>
> On Thu, 7 Dec 2023 at 02:19, Ilias Apalodimas
> <ilias.apalodimas at 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 at 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