From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 71229C433F5 for ; Thu, 29 Sep 2022 14:03:00 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id D287C84AD4; Thu, 29 Sep 2022 16:02:57 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (2048-bit key; unprotected) header.d=gmail.com header.i=@gmail.com header.b="NpNKARJ7"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 5941184BD6; Thu, 29 Sep 2022 16:02:55 +0200 (CEST) Received: from mail-qt1-x833.google.com (mail-qt1-x833.google.com [IPv6:2607:f8b0:4864:20::833]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id 88D2684942 for ; Thu, 29 Sep 2022 16:02:51 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=seanga2@gmail.com Received: by mail-qt1-x833.google.com with SMTP id w2so793465qtv.9 for ; Thu, 29 Sep 2022 07:02:51 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=content-transfer-encoding:in-reply-to:content-language:references :cc:to:subject:from:user-agent:mime-version:date:message-id:from:to :cc:subject:date; bh=wt/nX2SsAQ9Oh5jQsVISpnj1qk9VPfEPkrHd5iwp05c=; b=NpNKARJ7O5cBJa0pEQAphbUVy2IGsbAEP/55OBIdUx9uUir2x0L2ZNgOSi16a8I5Kt 4fAaGk3B5lu43TiZdH0FwvJhgh/dNOa0fIvifpfVrGuY3FzYBPJk8H1mcqxCaB7HMPfg HNmocVXrIdjdsQzH6JDukIODMGhlKFqEF/x719NXyWZSgHZD9Xos99GNUMHT9DM6mFtr JCFv0KlW0mrhMWklcegAUa5DnDxEvPvuPrni7BZZRy71ydHSsGJGnmc1V9ay4HtxU1vP RzI1V9rSEtnLYj86uzQBK8qvEm8roaDFkSGUmWrj4GSe3QeKfKg227VhtCwSE+M52jQP Wtug== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:in-reply-to:content-language:references :cc:to:subject:from:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date; bh=wt/nX2SsAQ9Oh5jQsVISpnj1qk9VPfEPkrHd5iwp05c=; b=pU84OYmhJmCIjgTQecaSAvXc4+7r9pUBFAJLVSE8F/gH3zuLPm4+NzqpmRTq2DDVSe D7fGws1+642Cneki+c4QKBw6m61NQ9es0zWdL6DfMMkkxnrvbAkYszI9JhvBArVemhbr PbSrzIiF/fgW211g0TNBD3Nyt+2ow2/kVQXwPYNhGcnlG36QPRxLEdLhMxsen0FzN/il DRUTxMXrWZd/Ztomk1HbU9RWNCKn8Lr4HJKjswNIGo8nKxPHFK43zODtifiqt9OSVS+i ZI4N+ZSaSBSUvygw8P1Voest1kvqjrvyF43ZL2KUvNXb5Jv3TTlMgPwyRdb+N548cD+V 74Dg== X-Gm-Message-State: ACrzQf1eRoQeu/ZVd1sl/pVC7RJNOf8BNjnHcQTSLEnlx52CBZAhIYwK 45keOxfq5kclpw+lzXavoYQ= X-Google-Smtp-Source: AMsMyM64TsRRlKrUdb9rpM61kqbPvxYciwX3WgdpCSB6JWMvULXT/4YlscqeLhScEJ5xlXxx0jhz5g== X-Received: by 2002:ac8:7f8f:0:b0:35c:cbd2:9261 with SMTP id z15-20020ac87f8f000000b0035ccbd29261mr2495070qtj.485.1664460169917; Thu, 29 Sep 2022 07:02:49 -0700 (PDT) Received: from [192.168.1.201] (pool-173-73-95-180.washdc.fios.verizon.net. [173.73.95.180]) by smtp.gmail.com with ESMTPSA id g15-20020a05620a40cf00b006b9264191b5sm6186335qko.32.2022.09.29.07.02.48 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 29 Sep 2022 07:02:49 -0700 (PDT) Message-ID: Date: Thu, 29 Sep 2022 10:02:48 -0400 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.11.0 From: Sean Anderson Subject: Re: [PATCH 1/2] smbios: Simplify reporting of unknown values To: Simon Glass Cc: Ilias Apalodimas , U-Boot Mailing List , Tom Rini , Heinrich Schuchardt , Peter Robinson References: <20220906134426.53748-1-ilias.apalodimas@linaro.org> <0e250d37-de96-330b-4666-af07fd65e2e8@gmail.com> Content-Language: en-US In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.39 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.6 at phobos.denx.de X-Virus-Status: Clean On 9/29/22 05:59, Simon Glass wrote: > Hi, > > On Wed, 28 Sept 2022 at 22:34, Sean Anderson wrote: >> >> On 9/26/22 06:56, Ilias Apalodimas wrote: >>> Hi Sean >>> >>> On Sat, 17 Sept 2022 at 19:55, Sean Anderson wrote: >>>> >>>> On 9/16/22 16:30, Ilias Apalodimas wrote: >>>>> Hi Simon, >>>>> >>>>> [...] >>>>> >>>>>>> Signed-off-by: Ilias Apalodimas >>>>>>> --- >>>>>>> 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 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. --Sean