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

[Tom Rini]

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?

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20231213/83513431/attachment.sig>