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=-18.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,NICE_REPLY_A,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 35DC9C432BE for ; Mon, 30 Aug 2021 09:49:14 +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 165F760724 for ; Mon, 30 Aug 2021 09:49:13 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org 165F760724 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=denx.de 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 D8CCF83358; Mon, 30 Aug 2021 11:49:10 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=none (p=none dis=none) header.from=denx.de Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=denx.de; s=phobos-20191101; t=1630316951; bh=bXJpuuWOGYunbxrcZmFUZi8Y3Rt5DDUXImIuNIfY2iE=; h=Subject:To:Cc:References:From:Date:In-Reply-To:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From; b=stRBdLTYGy2Cbv1JE5wzTb3SRjJD71J9Oc+viZF5wzGZUmcDZaZpQpf3NS+4JONVZ SmweDnxoZTFFZLu7B5RzS4dID+BWFWPnLxkIo24jnpctGfM2A6tN4s2UwoKLiiKMQa UQ1vVN/eYay4bjL0QbA3H5Qh35KJkkNu4HUiRwiqS304Dhe9oO4B+Zbp1DwDldmesv Tzr5WEwqultaanETX8xmx/brEqxC+1uEpE/7snB5KUofBFZozJ7mIrNl547Dy9ZYkr xOX/K4/bEObGfZyE1wq85MfsSt4A6RV6mq6XgIpVdsGYo8FOQwj3SXpJadDTFhJ/Qz 0Tb9OERueAAQw== Received: from [IPv6:::1] (p578adb1c.dip0.t-ipconnect.de [87.138.219.28]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits)) (No client certificate requested) (Authenticated sender: marex@denx.de) by phobos.denx.de (Postfix) with ESMTPSA id 8E76182D5B; Mon, 30 Aug 2021 11:49:08 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=denx.de; s=phobos-20191101; t=1630316948; bh=bXJpuuWOGYunbxrcZmFUZi8Y3Rt5DDUXImIuNIfY2iE=; h=Subject:To:Cc:References:From:Date:In-Reply-To:From; b=GRtxSL/CeHqZN/LGVo0kFH3Gpiw3QEPQs590tjvEnxQwCkHOX3cBcJtJIHSssXC1p O/ca9okUjthsV5k2DZYZJx2lLB2eSX/ce4XcBPNHBiQKuSZKNVnM6Jf4buZ7A4zHdF xWu2qgokOQs4T6Txw/PeG5on5KQ6R8P971CFuzn2WcRMC7lrssRYYDmhVRXYqD02mO drIEDDMqjHXKJv+VUfySx7SJFrVCu/TBNKUVCj7TzsjXMYpwO8Dw6UbQUxQw3rHsc9 IjyOb/umnitqTKBVfVLnGZFjKSkix0jxh4/E00NXlKgpkP55sISXG5uW3/EvCjnuQf 8U4xKcXrv/o5g== Subject: Re: [PATCH 02/14] lmb: Use CONFIG_LMB_*_REGIONS only if they are defined To: Tom Rini Cc: u-boot@lists.denx.de, Simon Glass , Simon Goldschmidt References: <20210829180202.GG858@bill-the-cat> <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> From: Marek Vasut Message-ID: <95c965a3-2da8-1212-fb54-0d5628d7743b@denx.de> Date: Mon, 30 Aug 2021 11:45:02 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.13.0 MIME-Version: 1.0 In-Reply-To: <20210829231145.GN858@bill-the-cat> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit 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 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 wrote: >>>>>>>>>> On 8/29/21 8:02 PM, Tom Rini wrote: >>>>>>>>>>> On Sun, Aug 29, 2021 at 06:26:23PM +0200, Marek Vasut wrote: >>>>>>>>>>>> On 8/15/21 9:47 PM, Tom Rini wrote: >>>>>>>>>>>>> On Sun, Aug 15, 2021 at 08:13:02PM +0200, Marek Vasut wrote: >>>>>>>>>>>>> >>>>>>>>>>>>>> The CONFIG_LMB_*_REGIONS are defined only if CONFIG_LMB is enabled, >>>>>>>>>>>>>> protect access to those two config options to avoid undefined macro >>>>>>>>>>>>>> errors. >>>>>>>>>>>>>> >>>>>>>>>>>>>> Signed-off-by: Marek Vasut >>>>>>>>>>>>>> Cc: Simon Glass >>>>>>>>>>>>>> Cc: Simon Goldschmidt >>>>>>>>>>>>>> Cc: Tom Rini >>>>>>>>>>>>>> --- >>>>>>>>>>>>>> include/lmb.h | 4 ++-- >>>>>>>>>>>>>> 1 file changed, 2 insertions(+), 2 deletions(-) >>>>>>>>>>>>>> >>>>>>>>>>>>>> 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(CONFIG_LMB_USE_MAX_REGIONS) >>>>>>>>>>> >>>>>>>>>>> This doesn't make sense to me, still. You cannot enable >>>>>>>>>>> CONFIG_LMB_USE_MAX_REGIONS without CONFIG_LMB as the former depends on >>>>>>>>>>> the latter in Kconfig. >>>>>>>>>>> >>>>>>>>>>>>>> struct lmb_property region[CONFIG_LMB_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(CONFIG_LMB_USE_MAX_REGIONS) >>>>>>>>>>>>>> struct lmb_property memory_regions[CONFIG_LMB_MEMORY_REGIONS]; >>>>>>>>>>>>>> struct lmb_property reserved_regions[CONFIG_LMB_RESERVED_REGIONS]; >>>>>>>>>>>>>> #endif >>>>>>>>>>>>> >>>>>>>>>>>>> We shouldn't need this at all. LMB and LMB_USE_MAX_REGIONS are both in >>>>>>>>>>>>> Kconfig and have the dependencies expressed that way. >>>>>>>>>>>> >>>>>>>>>>>> However, CONFIG_LMB_MEMORY_REGIONS and CONFIG_LMB_RESERVED_REGIONS may be >>>>>>>>>>>> undefined if CONFIG_LMB and !CONFIG_LMB_USE_MAX_REGIONS . They are four >>>>>>>>>>>> different symbols. >>>>>>>>>>> >>>>>>>>>>> I'm still not seeing it, sorry. Is there some case where we're trying >>>>>>>>>>> to access a struct lmb without CONFIG_LMB enabled? >>>>>>>>>>> >>>>>>>>>> >>>>>>>>>> See build failure >>>>>>>>>> https://source.denx.de/u-boot/custodians/u-boot-sh/-/jobs/315331 >>>>>>>>> >>>>>>>>> Ah, progress. Drop from since we already have a >>>>>>>>> forward declaration of struct lmb? But it's not failing without this >>>>>>>>> series too, so what's changing? >>>>>>>> >>>>>>>> See 01/14 in this series. >>>>>>> >>>>>>> Ah, so drop 1/14 then. >>>>>> >>>>>> Why ? That patch is correct. >>>>> >>>>> 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 up so that >>>>> we don't need to have redundant tests in the header. >>>> >>>> 1/14 disables LMB and CMD_BDI for tools build, we do not need those, so 1/14 >>>> is correct. >>> >>> 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_defconfig from >>> "sandbox" in CI. >> >> So, what exactly is the problem with that 01/14 ? Please elaborate, I >> believe the patch is correct. > > 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. > > Disabling LMB in tools-only_defconfig then exposes that can only > be included safely when CONFIG_LMB is set. > > 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. So, the patch is correct, the headers need further clean up. >>>> What kind of cleanup of lmb.h do you have in mind ? >>> >>> Remove it from include/image.h and fix any fall-out from that of files >>> that got indirectly when they needed it directly instead. >> >> Uh ... that is likely for a separate series, and a big one. > > Honestly, checking again, I'm not sure LMB=n is valid, ever. Why wouldn't it be ? For tools, LMB=n is perfectly valid. > That's how > we keep our running U-Boot from being trivially overwritten and a huge > number of security issues from being re-opened. Tools are not running U-Boot. > At this point, I think you should rework things to stop making > CONFIG_LMB be optional, it should be a def_bool y. I disagree, see above.