[PATCH 1/2] smbios: Simplify reporting of unknown values
Tom Rini
trini at konsulko.com
Fri Sep 30 16:25:16 CEST 2022
On Thu, Sep 29, 2022 at 10:02:48AM -0400, Sean Anderson wrote:
> On 9/29/22 05:59, Simon Glass wrote:
> > Hi,
> >
> > On Wed, 28 Sept 2022 at 22:34, Sean Anderson <seanga2 at gmail.com> wrote:
> > >
> > > On 9/26/22 06:56, Ilias Apalodimas wrote:
> > > > Hi Sean
> > > >
> > > > On Sat, 17 Sept 2022 at 19:55, Sean Anderson <seanga2 at gmail.com> wrote:
> > > > >
> > > > > On 9/16/22 16:30, Ilias Apalodimas wrote:
> > > > > > Hi Simon,
> > > > > >
> > > > > > [...]
> > > > > >
> > > > > > > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas at linaro.org>
> > > > > > > > ---
> > > > > > > > lib/smbios.c | 17 +++--------------
> > > > > > > > 1 file changed, 3 insertions(+), 14 deletions(-)
> > > > > > >
> > > > > > > Perhaps a better fix is to drop the smbios info?
> > > > > >
> > > > > > Unfortunately there's a ton of userspace tools still using it. So I think
> > > > > > we still need it
> > > > > >
> > > > > > >
> > > > > > > What upstream projects use this information to show things to the
> > > > > > > user? You showed a screenshot of some sort of system-info app. We
> > > > > > > could teach it about falling back to the device tree. That way we are
> > > > > > > not adding fake information to SMBIOS.
> > > > > > >
> > > > > >
> > > > > > What's fake here? The model and compatible are taken directly from the DT
> > > > > > and that should be accurate. I'd rather fix the DT if that's problematic.
> > > > > > What would make sense for me to change is take the first token of the
> > > > > > compatible node instead of the entire string as it's format is expected to
> > > > > > be <manufacturer, model> anyway.
> > > > >
> > > > > > Manufacturer: socionext,developer-box
> > > > > > Product Name: Socionext Developer Box
> > > > >
> > > > > Well, firstly, the manufacturer is "Socionext", not
> > > > > "socionext,developer-box". Compatibles are not suitable for
> > > > > user-visible identifiers. The product name should also be something like
> > > > > "Socionext Developerbox" or maybe "SynQuacer E-series", but this more of
> > > > > a "bug" in the devicetree model property.
> > > >
> > > > Yea as I said we can get rid of the everything after the ',' on the
> > > > compatible node. Ideally if vendors followed the DT spec, we could
> > > > also just use manufacturer node, the reality is that we can't though.
> > >
> > > This is another one of the problems with this approach. There's no
> > > consistency in existing device trees, because at most this info is
> > > printed in the boot log.
> > >
> > > > The whole point of the patchset is provide something reasonable
> > > > without having to add a .dtsi smbios node for all our devices. We can
> > > > then go back to fixing the DT with proper values if it's a DT "bug".
> > > > >
> > > > > Second, these identifiers are not suitable for all structures you want
> > > > > to use it for. For example, the chassis is really a "INWIN industrial PC
> > > > > case: MicroATX mini-tower case IW-BK623/300-H E USB 3.0 Black with 300W
> > > > > SFX power supply" [1]. I would describe this as something like
> > > >
> > > > The chassis isn't even addressed in the series. IIRC it's currently
> > > > hardcoded in smbios.c.
> > >
> > > You showed it as different in the commit message.
> > >
> > > > >
> > > > > Handle 0x0003, DMI type 3, 21 bytes
> > > > > Chassis Information
> > > > > Manufacturer: INWIN
> > > > > Type: Mini Tower
> > > > > Lock: Not Present
> > > > > Version: Unknown
> > > > > Serial Number: Not Specified
> > > > > Asset Tag: Not Specified
> > > > > Boot-up State: Safe
> > > > > Power Supply State: Safe
> > > > > Thermal State: Safe
> > > > > Security Status: None
> > > > > OEM Information: 0x00000000
> > > > > Height: Unspecified
> > > > > Number Of Power Cords: 1
> > > > > Contained Elements: 0
> > > > >
> > > > > The exact values are not particularly important, but I would certainly
> > > > > classify a manufacturer of "socionext,developer-box" as fake. We might
> > > > > not even know what the chassis is; what's to stop a user from using a
> > > > > different case?
> > > >
> > > > But the chassis isn't even addressed in the series? Again I am mostly
> > > > interested in a sane fallback for device and manufacturer.
> > >
> > > ditto
> > >
> > > > >
> > > > > [1] https://www.96boards.org/documentation/enterprise/developerbox/hardware-docs/MN04-00002-3E.pdf
> > > > >
> > > > > > > Also, SMBIOS is a legacy thing and a PITA to work with. How about we
> > > > > > > use the device tree binding for the same info:
> > > > > > >
> > > > > > > smbios {
> > > > > > > compatible = "u-boot,sysinfo-smbios";
> > > > > > >
> > > > > > > smbios {
> > > > > > > system {
> > > > > > > manufacturer = "pine64";
> > > > > > > product = "rock64_rk3328";
> > > > > > > };
> > > > > > >
> > > > > > > baseboard {
> > > > > > > manufacturer = "pine64";
> > > > > > > product = "rock64_rk3328";
> > > > > > > };
> > > > > > >
> > > > > > > chassis {
> > > > > > > manufacturer = "pine64";
> > > > > > > product = "rock64_rk3328";
> > > > > > > };
> > > > > > > };
> > > > > > > };
> > > > > > >
> > > > > > > This is easy to parse and gets us away from all this legacy junk that
> > > > > > > we don't need.
> > > > > >
> > > > > > That's the exact opposite of the patch description. Most of these info are
> > > > > > already included in the DT in it's standard properties. So if U-Boot ends
> > > > > > up with a DT without these we get a usable smbios table. For example a DT
> > > > > > handed over by the previous stage bootloader would not include these nodes.
> > > > >
> > > > > I agree. I think a better example would fill in these fields with
> > > > > descriptive values.
> > > >
> > > > We are off to a chicken and egg problem now. Can you provide U-Boot
> > > > with a 'configuration' DT, which would be disjoint from the DT that
> > > > describes hardware?
> > >
> > > Sorry, I misread the context there.
> > >
> > > I still don't think this is the right approach for this... better to fix
> > > the prior stage's devicetree.
> > >
> > > > >
> > > > > > As far as sysinfo-smbios node is concerned, it's only present in 13
> > > > > > boards, so it's not like it's used by the majority of boards. Yes we
> > > > > > could fix them, but imho we are better off re-using what's already there
> > > > > > and defined on the DT spec at least for the simplistic values.
> > > > >
> > > > > IMO SYS_VENDOR and SYS_BOARD are more descriptive than the devicetree
> > > > > values, but neither is good...
> > > >
> > > > Didn't we use to do that? IOW fill in smbios nodes based on Kconfig
> > > > values. But then we moved away from that in favor of the
> > > > sysinfo-smbios node, but a very small amount of boards got converted.
> > >
> > > I mean that SYS_VENDOR and SYS_BOARD have content which more closely
> > > matches the content of the SMBios tables, not that we should use them
> > > ("neither is good...").
> > >
> > > > >
> > > > > How many boards do we have which actually use the SMBIOS tables? There
> > > > > are a lot of boards with EFI_LOADER enabled by default, but I suspect
> > > > > most never boot anything EFI.
> > > >
> > > > I don't see how that's relevant? If someone for any reason enables
> > > > smbios it shouldn't report always "Unknown".
> > >
> > > I'm mostly trying to figure out how much effort it would be to just add
> > > nodes for all devices which boot with SMBios. I know that most boards
> > > which have it enabled don't actually use it, since it's enabled by
> > > default.
> >
> > It is a patch like this:
> >
> > https://patchwork.ozlabs.org/project/uboot/patch/20220929001520.9095-1-christian@kohlschutter.com/
> >
> > I just found out that this option is enabled for hundreds of boards.
>
> I first noticed it when doing the K210 and wondering why I had EFI
> enabled.
>
> > Perhaps the solution is to turn it off unless the board enables it?
>
> But how do we determine if the board enables it? Since it was on by
> default, it's not so easy. One way would be to look at the boards which
> use bootefi, but from what I can tell, that's enabled by distroboot.
> Which has a similar problem where include/configs/mycpu_common.h might
> enable it, but (most of) the boards for that cpu might not care.
I think the point that's trying to be made in the thread is that this
bit of code is common and widely used / visible as it's part of the
regular commodity Linux distro "tell the user useful things" tools. So
it should default to be as correct as can be.
--
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/20220930/ab783d18/attachment.sig>
More information about the U-Boot
mailing list