[PATCH 2/2 v2] smbios: Fallback to the default DT if sysinfo nodes are missing

[Simon Glass]

Hi Tom,

On Wed, 13 Dec 2023 at 13:19, Tom Rini <trini at konsulko.com> wrote:
>
> On Wed, Dec 13, 2023 at 12:50:03PM -0700, Simon Glass wrote:
> > Hi Ilias,
> >
> > On Mon, 11 Dec 2023 at 00:49, Ilias Apalodimas
> > <ilias.apalodimas at linaro.org> wrote:
> > >
> > > Hi Tom,
> > >
> > > On Sat, 9 Dec 2023 at 20:49, Tom Rini <trini at konsulko.com> wrote:
> > > >
> > > > On Wed, Dec 06, 2023 at 06:03:14PM +0200, Ilias Apalodimas wrote:
> > > > > Hi Simon,
> > > > >
> > > > > On Sat, 2 Dec 2023 at 20:28, Simon Glass <sjg at chromium.org> wrote:
> > > > > >
> > > > > > Hi Ilias,
> > > > > >
> > > > > > On Wed, 29 Nov 2023 at 23:50, Ilias Apalodimas
> > > > > > <ilias.apalodimas at linaro.org> wrote:
> > > > > > >
> > > > > > > Hi Simon,
> > > > > > >
> > > > > > > [...]
> > > > > > >
> > > > > > >> > 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.
> > > > > > >
> > > > > > >
> > > > > > > Sure but OTOH the code increase should be really minimal. But I don't mind I can add a Kconfig
> > > > > > >
> > > > > > >>
> > > > > > >> > +
> > > > > > >> > +       dt_str = ofnode_read_string(ofnode_root(), nprop->dt_str);
> > > > > > >>
> > > > > > >> Could this use ofnode_read_string_index() ?
> > > > > > >
> > > > > > >
> > > > > > > Maybe, I'll have a look and change it if that works
> > > > >
> > > > > Unless I am missing something this doesn't work.
> > > > > This is designed to return a string index from a DT property that's defined as
> > > > > foo_property = "value1", "value2" isn't it?
> > > > >
> > > > > The code above is trying to read an existing property (e.g compatible)
> > > > > and get the string after the comma delimiter.
> > > > > Perhaps I should add this in drivers/core/ofnode.c instead?
> > > > >
> > > > > > >
> > > > > > > [...]
> > > > > > >
> > > > > > >>
> > > > > > >> Any chance of a test for this code?
> > > > > > >
> > > > > > >
> > > > > > > Sure, but any suggestions on where to add the test?
> > > > > > > SMBIOS tables are populated on OS booting, do we have a test somewhere that boots an OS?
> > > > > >
> > > > > > They are written on startup, right? They should certainly be in place
> > > > > > before U-Boot enters the command line.
> > > > >
> > > > > Not always.  I am not sure if x86 does that, but on the rest of the
> > > > > architectures, they are only initialized when the efi smbios code
> > > > > runs. Wasn't this something you were trying to change?
> > > >
> > > > One of those things I keep repeating is that we don't know for sure what
> > > > the right values here are until we load the DT the OS _will_ use. It's
> > > > quite valid to start U-Boot and pass it a generic "good enough" DT at
> > > > run time until U-Boot can see (GPIOs, EEPROM, whatever) what it's on and
> > > > what the real DT to load before passing to the OS is. Since U-Boot
> > > > doesn't need SMBIOS tables itself, we should make these "late" not
> > > > "early".
> > >
> > > Fair enough, we can defer the init and testing of those late, just
> > > before we are about to boot. But this is irrelevant to what this patch
> > > does, can we get the fallback mechanism in first, assuming everyone is
> > > ok with the patch now?
> > >
> >
> > I would like this behind a Kconfig. Making it the default means people
> > are going to start relying on (in user space) and then discover later
> > that it is wrong.
>
> What do you mean wrong, exactly?

"raspberrypi" instead of "Raspberry Pi", for example, or "friendlyarm"
instead of "FriendlyElec"

I just wonder what this information is actually used for. If it is
just for fun, that is fine, but I believe some tools do use dmidecode,
at which point it needs to be accurate and should not change later,
e.g. when someone decides to actually add the info.

Regards,
Simon