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

[Simon Glass]

Hi Ilias,

On Wed, 6 Dec 2023 at 04:36, Ilias Apalodimas
<ilias.apalodimas at 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