Hi, On Wed, 28 Sept 2022 at 22:34, Sean Anderson <sean...@gmail.com> wrote: > > On 9/26/22 06:56, Ilias Apalodimas wrote: > > Hi Sean > > > > On Sat, 17 Sept 2022 at 19:55, Sean Anderson <sean...@gmail.com> wrote: > >> > >> On 9/16/22 16:30, Ilias Apalodimas wrote: > >>> Hi Simon, > >>> > >>> [...] > >>> > >>>>> Signed-off-by: Ilias Apalodimas <ilias.apalodi...@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-christ...@kohlschutter.com/ I just found out that this option is enabled for hundreds of boards. Perhaps the solution is to turn it off unless the board enables it? Regards, Simon
