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 X-Spam-Level: X-Spam-Status: No, score=-17.2 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED, USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 4F2F6C432BE for ; Mon, 30 Aug 2021 12:01:16 +0000 (UTC) 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 mail.kernel.org (Postfix) with ESMTPS id 812BE60F5C for ; Mon, 30 Aug 2021 12:01:15 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org 812BE60F5C Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=konsulko.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=lists.denx.de Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id E4C1982BB4; Mon, 30 Aug 2021 14:01:12 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=none (p=none dis=none) header.from=konsulko.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (1024-bit key; unprotected) header.d=konsulko.com header.i=@konsulko.com header.b="lgUO7CUq"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id E33CF82C4D; Mon, 30 Aug 2021 14:01:10 +0200 (CEST) Received: from mail-qk1-x72c.google.com (mail-qk1-x72c.google.com [IPv6:2607:f8b0:4864:20::72c]) (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 1144D829FC for ; Mon, 30 Aug 2021 14:01:06 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=none (p=none dis=none) header.from=konsulko.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=trini@konsulko.com Received: by mail-qk1-x72c.google.com with SMTP id t190so15234267qke.7 for ; Mon, 30 Aug 2021 05:01:06 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=konsulko.com; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=FaAw+63B5EMoVJJfDbtpSoxu38WtvKVQZ7FEYapCJGc=; b=lgUO7CUqZ7r0tRIeZI/bEDrZWawpBxllzKy9w5/O2Pl/xpoJJM6b6yszj5QMiRNMGK 0bknoLUSXxEUUSXuvtLXzdBz6S6y0bwGgcuuWEGTB4PRCIgCN8R9ZxwuJCuZwgXjgwAP WEwCHZ1DetQTiXEa8DibPh7NaTByZlKu4T5KU= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=FaAw+63B5EMoVJJfDbtpSoxu38WtvKVQZ7FEYapCJGc=; b=hsZep8ayRc4u/bG8nurujMEb3OivcDy9SqEcf3mSKfLdHzfXXKaHb2ck2XCaLf6pSA 9mWSFEcTxI4TEskS4/N3OYK7AkWnQKPvDv+8EmWp+vQMaMWpJDx2HZu7Y8ZcnzFOUL8J X6qmGIlAx/IuZ1MJ1LOc5pGnslNSNJF2+oTRnu7RNAf1++BRmA3CTN651iG6MFEzofNa GcxQLkost9HH9epMnqr3m+teeSxYOVdktW04HXdcK369+hNzDXV9qxgjwfTGm0G3zCuy QREt1bzqoft1UDr360lkY0cyE4PSKCYYU7DCvi5nGyWUq8sojMPLW7v+Rbff7DK3Hrtx JcvQ== X-Gm-Message-State: AOAM531BIifgA8oYVAyv2Y1XubdnywY7PxXra0jvDPx4M0q+FDezzpxb WjeySwNXAZukrGd0P2YMANknuQ== X-Google-Smtp-Source: ABdhPJwhkFRllHPoFjFiFdrxMs2xpj5VJ+vDgejdFMpNYOs95SZQakc84A9xNG9E7krAEyWvqpmm6Q== X-Received: by 2002:a05:620a:802:: with SMTP id s2mr22005622qks.11.1630324864758; Mon, 30 Aug 2021 05:01:04 -0700 (PDT) Received: from bill-the-cat (2603-6081-7b01-cbda-8054-417e-d97b-7576.res6.spectrum.com. [2603:6081:7b01:cbda:8054:417e:d97b:7576]) by smtp.gmail.com with ESMTPSA id s18sm8515545qtn.46.2021.08.30.05.01.03 (version=TLS1_2 cipher=ECDHE-ECDSA-CHACHA20-POLY1305 bits=256/256); Mon, 30 Aug 2021 05:01:04 -0700 (PDT) Date: Mon, 30 Aug 2021 08:01:02 -0400 From: Tom Rini To: Marek Vasut Cc: u-boot@lists.denx.de, Simon Glass , Simon Goldschmidt Subject: Re: [PATCH 02/14] lmb: Use CONFIG_LMB_*_REGIONS only if they are defined Message-ID: <20210830120102.GO858@bill-the-cat> References: <20210829193239.GH858@bill-the-cat> <20210829221008.GK858@bill-the-cat> <0f735fc1-c331-ee13-4bbf-5be71d45fecd@denx.de> <20210829222356.GL858@bill-the-cat> <2cffb09e-23aa-afce-962e-1eb9727457db@denx.de> <20210829225126.GM858@bill-the-cat> <20210829231145.GN858@bill-the-cat> <95c965a3-2da8-1212-fb54-0d5628d7743b@denx.de> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="foR1EOCFxkuZE0WY" Content-Disposition: inline In-Reply-To: <95c965a3-2da8-1212-fb54-0d5628d7743b@denx.de> X-Clacks-Overhead: GNU Terry Pratchett User-Agent: Mutt/1.9.4 (2018-02-28) X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.34 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.2 at phobos.denx.de X-Virus-Status: Clean --foR1EOCFxkuZE0WY Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Aug 30, 2021 at 11:45:02AM +0200, Marek Vasut wrote: > On 8/30/21 1:11 AM, Tom Rini wrote: > > On Mon, Aug 30, 2021 at 01:00:02AM +0200, Marek Vasut wrote: > > > On 8/30/21 12:51 AM, Tom Rini wrote: > > > > On Mon, Aug 30, 2021 at 12:40:07AM +0200, Marek Vasut wrote: > > > > > On 8/30/21 12:23 AM, Tom Rini wrote: > > > > > > On Mon, Aug 30, 2021 at 12:19:59AM +0200, Marek Vasut wrote: > > > > > > > On 8/30/21 12:10 AM, Tom Rini wrote: > > > > > > > > On Sun, Aug 29, 2021 at 11:47:58PM +0200, Marek Vasut wrote: > > > > > > > > > On 8/29/21 9:32 PM, Tom Rini wrote: > > > > > > > > > > On Sun, Aug 29, 2021 at 09:24:46PM +0200, Marek Vasut w= rote: > > > > > > > > > > > On 8/29/21 8:02 PM, Tom Rini wrote: > > > > > > > > > > > > On Sun, Aug 29, 2021 at 06:26:23PM +0200, Marek Vas= ut wrote: > > > > > > > > > > > > > On 8/15/21 9:47 PM, Tom Rini wrote: > > > > > > > > > > > > > > On Sun, Aug 15, 2021 at 08:13:02PM +0200, Marek= Vasut wrote: > > > > > > > > > > > > > >=20 > > > > > > > > > > > > > > > The CONFIG_LMB_*_REGIONS are defined only if = CONFIG_LMB is enabled, > > > > > > > > > > > > > > > protect access to those two config options to= avoid undefined macro > > > > > > > > > > > > > > > errors. > > > > > > > > > > > > > > >=20 > > > > > > > > > > > > > > > Signed-off-by: Marek Vasut > > > > > > > > > > > > > > > Cc: Simon Glass > > > > > > > > > > > > > > > Cc: Simon Goldschmidt > > > > > > > > > > > > > > > Cc: Tom Rini > > > > > > > > > > > > > > > --- > > > > > > > > > > > > > > > include/lmb.h | 4 ++-- > > > > > > > > > > > > > > > 1 file changed, 2 insertions(+), 2 de= letions(-) > > > > > > > > > > > > > > >=20 > > > > > > > > > > > > > > > diff --git a/include/lmb.h b/include/lmb.h > > > > > > > > > > > > > > > index 3c4afdf9f0..fa1474a360 100644 > > > > > > > > > > > > > > > --- a/include/lmb.h > > > > > > > > > > > > > > > +++ b/include/lmb.h > > > > > > > > > > > > > > > @@ -44,7 +44,7 @@ struct lmb_property { > > > > > > > > > > > > > > > struct lmb_region { > > > > > > > > > > > > > > > unsigned long cnt; > > > > > > > > > > > > > > > unsigned long max; > > > > > > > > > > > > > > > -#if IS_ENABLED(CONFIG_LMB_USE_MAX_REGIONS) > > > > > > > > > > > > > > > +#if IS_ENABLED(CONFIG_LMB) && IS_ENABLED(CON= FIG_LMB_USE_MAX_REGIONS) > > > > > > > > > > > >=20 > > > > > > > > > > > > This doesn't make sense to me, still. You cannot e= nable > > > > > > > > > > > > CONFIG_LMB_USE_MAX_REGIONS without CONFIG_LMB as th= e former depends on > > > > > > > > > > > > the latter in Kconfig. > > > > > > > > > > > >=20 > > > > > > > > > > > > > > > struct lmb_property region[CONFIG_LM= B_MAX_REGIONS]; > > > > > > > > > > > > > > > #else > > > > > > > > > > > > > > > struct lmb_property *region; > > > > > > > > > > > > > > > @@ -67,7 +67,7 @@ struct lmb_region { > > > > > > > > > > > > > > > struct lmb { > > > > > > > > > > > > > > > struct lmb_region memory; > > > > > > > > > > > > > > > struct lmb_region reserved; > > > > > > > > > > > > > > > -#if !IS_ENABLED(CONFIG_LMB_USE_MAX_REGIONS) > > > > > > > > > > > > > > > +#if IS_ENABLED(CONFIG_LMB) && !IS_ENABLED(CO= NFIG_LMB_USE_MAX_REGIONS) > > > > > > > > > > > > > > > struct lmb_property memory_regions[C= ONFIG_LMB_MEMORY_REGIONS]; > > > > > > > > > > > > > > > struct lmb_property reserved_regions= [CONFIG_LMB_RESERVED_REGIONS]; > > > > > > > > > > > > > > > #endif > > > > > > > > > > > > > >=20 > > > > > > > > > > > > > > We shouldn't need this at all. LMB and LMB_USE= _MAX_REGIONS are both in > > > > > > > > > > > > > > Kconfig and have the dependencies expressed tha= t way. > > > > > > > > > > > > >=20 > > > > > > > > > > > > > However, CONFIG_LMB_MEMORY_REGIONS and CONFIG_LMB= _RESERVED_REGIONS may be > > > > > > > > > > > > > undefined if CONFIG_LMB and !CONFIG_LMB_USE_MAX_R= EGIONS . They are four > > > > > > > > > > > > > different symbols. > > > > > > > > > > > >=20 > > > > > > > > > > > > I'm still not seeing it, sorry. Is there some case= where we're trying > > > > > > > > > > > > to access a struct lmb without CONFIG_LMB enabled? > > > > > > > > > > > >=20 > > > > > > > > > > >=20 > > > > > > > > > > > See build failure > > > > > > > > > > > https://source.denx.de/u-boot/custodians/u-boot-sh/-/= jobs/315331 > > > > > > > > > >=20 > > > > > > > > > > Ah, progress. Drop from since we alr= eady have a > > > > > > > > > > forward declaration of struct lmb? But it's not failin= g without this > > > > > > > > > > series too, so what's changing? > > > > > > > > >=20 > > > > > > > > > See 01/14 in this series. > > > > > > > >=20 > > > > > > > > Ah, so drop 1/14 then. > > > > > > >=20 > > > > > > > Why ? That patch is correct. > > > > > >=20 > > > > > > It's not quite right, 1/14 and then 2/14 are papering over the = fact that > > > > > > lmb.h, and it's including headers / files, need to be cleaned u= p so that > > > > > > we don't need to have redundant tests in the header. > > > > >=20 > > > > > 1/14 disables LMB and CMD_BDI for tools build, we do not need tho= se, so 1/14 > > > > > is correct. > > > >=20 > > > > We don't need to build u-boot at all for tools-only, only the tools= -only > > > > build target. It's just annoying to exclude the tools-only_defconf= ig from > > > > "sandbox" in CI. > > >=20 > > > So, what exactly is the problem with that 01/14 ? Please elaborate, I > > > believe the patch is correct. > >=20 > > You disable LMB in a target that's only building "all" in CI because > > wasn't ever worth adding ",sandbox" to the all other arches job until > > perhaps now. > >=20 > > Disabling LMB in tools-only_defconfig then exposes that can only > > be included safely when CONFIG_LMB is set. > >=20 > > Adding / extending an #if test in code for something that's already > > checked for in Kconfig is bad. We spent so much time already removing > > and shrinking #if tests in the code. >=20 > So, the patch is correct, the headers need further clean up. No, it's not. The first patch is wrong because disabling CONFIG_LMB is invalid. The second patch is conceptually wrong because if we're enforcing a check in C for a dependency that's enforced in Kconfig, we have another problem to investigate. Which I did, LMB is non-optional. > > > > > What kind of cleanup of lmb.h do you have in mind ? > > > >=20 > > > > Remove it from include/image.h and fix any fall-out from that of fi= les > > > > that got indirectly when they needed it directly instead. > > >=20 > > > Uh ... that is likely for a separate series, and a big one. > >=20 > > Honestly, checking again, I'm not sure LMB=3Dn is valid, ever. >=20 > Why wouldn't it be ? For tools, LMB=3Dn is perfectly valid. Because it's never valid to disable LMB, LMB is what protects the running U-Boot. It's nonsense to build u-boot on tools-only_defconfig but we don't have a way currently to remove "u-boot" from the all target. Maybe once a few more of the hard/tricky CONFIG symbols get migrated to Kconfig we can then set tools-only_defconfig to NOT build u-boot at all. > > That's how > > we keep our running U-Boot from being trivially overwritten and a huge > > number of security issues from being re-opened. >=20 > Tools are not running U-Boot. >=20 > > At this point, I think you should rework things to stop making > > CONFIG_LMB be optional, it should be a def_bool y. >=20 > I disagree, see above. The only reason "tools-only_defconfig" builds a useless u-boot binary today is in CI where it would be more work than it's worth to make CI exclude that from the build list. But if you want to just do that instead, I'll also accept adding -x tools-only to the azure/gitlab jobs that build all other architectures, as tools-only is tested in its own build job, for it's only valid build target. --=20 Tom --foR1EOCFxkuZE0WY Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQGzBAABCgAdFiEEGjx/cOCPqxcHgJu/FHw5/5Y0tywFAmEsyHoACgkQFHw5/5Y0 tywHVwwAuo6iTRLBLuYfMk+fhFDlpaKXKAGpbD+A6vwHaQiU1oavWc5B3CnJ6hxM gGR6xvvlWHd+ozsi9rMYas0j6hMleBLZ7e/lnDbD/PjphtRuLhOjXc2r4GNf6eZT LvZc9e95pQ9mkAEXWp40ghEAzVVp+85JZGQzGTUUh4CLAZCzYOarpu/lY3bAqecc mKmWwPr/rRCfpHPhsHZPNXm1Gy7EMkppnMhDRHSvtm32tE016XyHjY921Pq9tKhd FqyrhkiAQN3x+P1DxuyhqcrSQAJIFQBU8b9btaO2b0t6jiU1nuHv2y1RzI/vU7z1 sgH9OMHQp6xIESalS9TLbvg0gLc4pSZaaxqSm0tY50/EaXy0cPCeKKpTxbQtIZZJ V7WUtzoB85DligZXUpGuQxlBblTqT31aD2MuT7WVqi11I9A+u/26VjhAQ1baX2kl fOpxsjR2gTZKDZCNRHhnmzRQ/FY+1lrjnX2mU+vZMbPif0qMksR2pfh5oN9ZsgKQ Q7s0mqJH =TvgP -----END PGP SIGNATURE----- --foR1EOCFxkuZE0WY--