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 <s...@chromium.org> wrote:
>
> Hi Ilias,
>
> On Wed, 6 Dec 2023 at 04:36, Ilias Apalodimas
> <ilias.apalodi...@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
