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 1F0B4C636CC for ; Tue, 7 Feb 2023 16:26:49 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 6CBA085E30; Tue, 7 Feb 2023 17:26:47 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=pass (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="uBA4hT7S"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 2D9A685E0A; Tue, 7 Feb 2023 17:26:45 +0100 (CET) Received: from mail-qt1-x831.google.com (mail-qt1-x831.google.com [IPv6:2607:f8b0:4864:20::831]) (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 34EC485E3A for ; Tue, 7 Feb 2023 17:26:42 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=konsulko.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=trini@konsulko.com Received: by mail-qt1-x831.google.com with SMTP id h24so17380026qtr.0 for ; Tue, 07 Feb 2023 08:26:42 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=konsulko.com; s=google; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=pqfnl8S7F2d2qb4/j0yQPRuCjOzKmjftK4o/hP/8Ans=; b=uBA4hT7SI0VHd49iGwhdAuWtetiZPE7fY5uYUhXMkeQIFN17UQ5NQMXDJarALScVMz 1vHm8/4/S8o1Mxkbpva5RQIq/iVfpfRkCfFmur5QwY3bT6D0URIkfpVBaFUmSzy10O23 LFgVUJy2HFrm3OFjGyPpQfrHEEOjNswsJhmCE= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=pqfnl8S7F2d2qb4/j0yQPRuCjOzKmjftK4o/hP/8Ans=; b=VqZ2KFCTDplMQ96i4nEOsK1zs1igNEISAZwdBK2y02W2+lPNGmXok4Ah395TJ3Ir/p mnXeqe8UyUfKoBKQ48HGq52FoiGLh3ddv6TbYSdg6nvtBuOrZ6pKDymN5oI7zTOh6VM9 bnGrzpnGWIiZXEM7zOFXo4oFuTJ3hDe8JPC0hRXXW+ru9/h4UoKCWa/EsaGEpL9Uql0L NuUbkLhjx5q7OTztXfK+M12HyB7gWk0IemfMeU3azTlprGAq6Z+aXpiYpbrHrzWxUNCu wgY64Xg38lQCIzyzH9+6tQyQaNL4fGb8rezvBgiEZ53fnRlHc3WFYO+YBdyNJJj7TsZE ymxg== X-Gm-Message-State: AO0yUKVE+plHycRw05o3gv5pMztCoErTfkblDMv4T+506J81umDzTL86 5odrNNeOrgi/Sgd3xq1zGv0PuA== X-Google-Smtp-Source: AK7set/r9sAm4GkcrVNGNOFW1GJPGD0tqCr62GnKT2LOnxXCG/VDnuhAO68INqLX9P/tA7+n+m2B5Q== X-Received: by 2002:ac8:5aca:0:b0:3b8:2ea9:a08d with SMTP id d10-20020ac85aca000000b003b82ea9a08dmr6933060qtd.52.1675787200797; Tue, 07 Feb 2023 08:26:40 -0800 (PST) Received: from bill-the-cat (2603-6081-7b00-6400-7494-fb31-9b5f-683c.res6.spectrum.com. [2603:6081:7b00:6400:7494:fb31:9b5f:683c]) by smtp.gmail.com with ESMTPSA id bk19-20020a05620a1a1300b006f9f3c0c63csm9898116qkb.32.2023.02.07.08.26.39 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 07 Feb 2023 08:26:39 -0800 (PST) Date: Tue, 7 Feb 2023 11:26:37 -0500 From: Tom Rini To: Simon Glass Cc: Heinrich Schuchardt , U-Boot Mailing List Subject: Re: [PATCH v2 036/169] Correct SPL uses of CMD_BOOTEFI_BOOTMGR Message-ID: References: <20230206005537.254541-27-sjg@chromium.org> <20230206005537.254541-37-sjg@chromium.org> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="/17Nyzp6J8+L4/Ob" Content-Disposition: inline In-Reply-To: X-Clacks-Overhead: GNU Terry Pratchett 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 --/17Nyzp6J8+L4/Ob Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Feb 07, 2023 at 08:31:59AM -0700, Simon Glass wrote: > Hi Tom, >=20 > On Tue, 7 Feb 2023 at 07:50, Tom Rini wrote: > > > > On Tue, Feb 07, 2023 at 08:39:38AM +0100, Heinrich Schuchardt wrote: > > > > > > > > > On 2/7/23 01:00, Tom Rini wrote: > > > > On Tue, Feb 07, 2023 at 12:54:03AM +0100, Heinrich Schuchardt wrote: > > > > > > > > > > > > > > > On 2/6/23 01:53, Simon Glass wrote: > > > > > > This converts 3 usages of this option to the non-SPL form, sinc= e there is > > > > > > no SPL_CMD_BOOTEFI_BOOTMGR defined in Kconfig > > > > > > > > > > > > Signed-off-by: Simon Glass > > > > > > --- > > > > > > > > > > > > (no changes since v1) > > > > > > > > > > > > boot/Makefile | 2 +- > > > > > > cmd/bootmenu.c | 4 ++-- > > > > > > 2 files changed, 3 insertions(+), 3 deletions(-) > > > > > > > > > > > > diff --git a/boot/Makefile b/boot/Makefile > > > > > > index 69c31adb77d..73b5b19816b 100644 > > > > > > --- a/boot/Makefile > > > > > > +++ b/boot/Makefile > > > > > > @@ -29,7 +29,7 @@ obj-$(CONFIG_$(SPL_TPL_)BOOTMETH_EFILOADER) += =3D bootmeth_efi.o > > > > > > obj-$(CONFIG_$(SPL_TPL_)BOOTMETH_SANDBOX) +=3D bootmeth_sand= box.o > > > > > > obj-$(CONFIG_$(SPL_TPL_)BOOTMETH_SCRIPT) +=3D bootmeth_scrip= t.o > > > > > > ifdef CONFIG_$(SPL_TPL_)BOOTSTD_FULL > > > > > > -obj-$(CONFIG_$(SPL_TPL_)CMD_BOOTEFI_BOOTMGR) +=3D bootmeth_efi= _mgr.o > > > > > > +obj-$(CONFIG_CMD_BOOTEFI_BOOTMGR) +=3D bootmeth_efi_mgr.o > > > > > > obj-$(CONFIG_$(SPL_TPL_)BOOTSTD) +=3D bootflow_menu.o > > > > > > endif > > > > > > diff --git a/cmd/bootmenu.c b/cmd/bootmenu.c > > > > > > index 3236ca5d799..422ab411252 100644 > > > > > > --- a/cmd/bootmenu.c > > > > > > +++ b/cmd/bootmenu.c > > > > > > @@ -223,7 +223,7 @@ static int prepare_bootmenu_entry(struct bo= otmenu_data *menu, > > > > > > return 1; > > > > > > } > > > > > > -#if (CONFIG_IS_ENABLED(CMD_BOOTEFI_BOOTMGR)) && (CONFIG_IS_ENA= BLED(CMD_EFICONFIG)) > > > > > > +#if (IS_ENABLED(CONFIG_CMD_BOOTEFI_BOOTMGR)) && (CONFIG_IS_ENA= BLED(CMD_EFICONFIG)) > > > > > > > > > > There is no reason whatsoever for using different macros for the = two > > > > > options. > > > > > > > > Here and elsewhere, one CONFIG is being fixed at a time. If at the = end > > > > of the series this is not fixed, then that's an issue to address. > > > > > > This cannot be reviewed easily. I never received the complete series. > > > > This, and the related series, are among the most reviewed we've had in > > quite some time. Just FWIW. > > > > > CONFIG_IS_ENABLED() is more restrictive than IS_ENABLED(). No motivat= ion is > > > provided why the condition should be relaxed in the commit message. > > > > The idea of "restrictive" is not how either of those macros should be > > evaluated. > > > > > Cover-letters are not in the commit history. But anyway the cover-let= ter > > > does not provide any motivation for the change either. > > > > > > NAK to this patch. > > > > It's incorrect to use CONFIG_IS_ENABLED() instead of IS_ENABLED() > > outside of: > > - CONFIG_FOO, CONFIG_SPL_FOO (etc) exist > > - The code in question is compiled in the SPL (etc) context and we do > > need to know if the code block in question is required here and the > > implicit value of SPL_FOO being false is useful. > > This case is why Simon insists that adding def_bool n for > > SPL_EFI_LOADER, etc, is correct, but I'm not convinced. >=20 > The issue here is that we need a way to determine whether an option > like CONFIG_FOO should apply to all build phases, or only U-Boot > proper. The def_bool business creates an SPL symbol, so U-Boot then > knows that the option applies only to U-Boot proper, with a separate > one for CONFIG_SPL_FOO Please bear in mind that today a lack of definition works, and is a feature, not a bug nor glitch. It's why, today, the tests in lib/vsprintf.c are correct. > There is a file call scripts/conf_nospl (in splc) which lists options > that are exceptions. Otherwise we would need more of these def_bool > things. The thing is, I don't really like the conf_nospl file, since > it is configuring the operation of Kconfig but is not actually in the > Kconfig description. So for things where I thought it was defensible, > I added a def_bool. Keep in mind that everything works as intended, today. What's being done in the EFI loader code works, but is a bad practice. It's what lead to the MMC_QUIRKS actual bug and time being lost debugging a problem that shouldn't have been. What split config introduces / requires is for discussion over *there*. --=20 Tom --/17Nyzp6J8+L4/Ob Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQGzBAABCgAdFiEEGjx/cOCPqxcHgJu/FHw5/5Y0tywFAmPie7oACgkQFHw5/5Y0 tyxmfAv/cek97vlqtkEYcXQOIJP8jxoY2QLop2Km+JxBgs3EtorNE11C6Qc1Zbf1 J2KzSLOEXWcTs7KBHGFkRnDYk0FcwCFpZIkAdSIPklSHiuR3/0oAorC8EA1fgzUD 5OtCiRICmQUH5cJFFPf+1pkj9PROnng7TKrkwlYhayDyO+/x9gRF286NpXHGHgye h5EOvJ13vDYmCBy9xN0LYgHX9hixRMMaUaUsErHDekfVksysQ3Bp7uwAgV4NQnht R9ObGW83mE/ntA+SQVDriPPxGXJ2TXUdbDYwa2qJKAS2k2Db+Y38IaReicKxwF92 D/KugAA4AeEiX45Aovwb2IDajXDdn9pFi/Jf1setOUGFKkEDEskXJz3IbL+0jPBu zA+KcGqQ4Oae/zEH7KT38dbqnrkLxY3U/z9ew0cdgVO3hjI2vNlMpkkOMt+YzVdk jhMrWYoWTRZ5gXbIC1NRYsvOOCU3yx1AK7kE6YeMEe1vqgVCPH9n+g3vySnu/ChV wkrHzTH/ =+IoP -----END PGP SIGNATURE----- --/17Nyzp6J8+L4/Ob--