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 <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. > > 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
signature.asc
Description: PGP signature
