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

[Ilias Apalodimas]

[...]

> > > > [...]
> > > >
> > > > > > 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";
> > >
> > > But your code does the same as my fragment above, doesn't it? Only the
> > > compatible string is split, and only the first part (before the ',')
> > > is used.
> > >
> > > For the model, the whole string is used,
> > >  so having a function to split
> > > the string, which doesn't have a separator in it, seems unnecessary.
> >
> > No, it's not. the spec says model = <manufacturer, model>. Some of the
> > existing DTs follow that pattern. In that case, you need to split
> > those as well and that code would start to look really ugly and non
> > extendable. NXP and Qualcomm boards are just some examples
>
> Oh that's interesting, for example:
>
> model = "Qualcomm Technologies, Inc. SM8550 QRD";
>
> But I don't actually see any use of 'model' in Linux that has a
> vendor,model approach. Also the existing values are clearly indented
> for display to the user. I wonder if we should change the spec at this
> point?
>

I don't have a strong opinion on this. I'd prefer if we left as-is and
slowly fix the existing DTs to include the manufacturer. It would be
way easier to parse those values.

> >
> > >
> > > I am not talking about the end result, just trying to make the code
> > > easier to understand. It is very complex right now.
> >
> > The function has a pretty clear comment on what it tries to do.
> > Also if we do what you suggest, adding chassis-id would mean that we
> > need another round of strchrs and ifs and who knows what else.  With
> > the current patch, you just need to add an entry to the array
> > something along the lines of
> > { .sysinfo_str = "chassis", .dt_str = "chassis-type", 1}
>
> Yes I agree that it would be easier to do such a thing and now I
> understand why you have done this in the code. But why would we do
> that? We might as well use the proper sysinfo binding in U-Boot,
> instead of inventing something like this?

This has been discussed over and over again on why we need this. I
don't see any point in repeating the arguments. FWIW the sysinfo node
on u-boot is the invention and this is re-using existing DT entries
that dont need extra work.

> Or would what you propose
> here be easier to upstream? I'm just not sure it makes sense, which is
> why I feel the code is over-complicated (and still has no tests even
> after all this work).

I don't see how a for loop and strtok are so overcomplicated.
The code *never* had any tests. In fact, it has been broken several
times and Heinrich and I are the ones to fix it. No one argues the
need for tests, I just don't see why this is a blocker.
As far as upstreaming is concerned, I would approach the problem
differently.  The model, manufacturer and chassis information are
already in the DT, so I don't see why we should touch that, apart from
amending the existing DTs with 'better' values. For the rest of the
info SMBIOS lacks (memory, cpu etc), I'd look into the existing DT
spec and add the missing bits if they can't be discovered
automatically.

Thanks
/Ilias
> Regards,
> Simon