[PATCH v2 036/169] Correct SPL uses of CMD_BOOTEFI_BOOTMGR

[Tom Rini]

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, since there is
> > > > no SPL_CMD_BOOTEFI_BOOTMGR defined in Kconfig
> > > > 
> > > > Signed-off-by: Simon Glass <sjg at chromium.org>
> > > > ---
> > > > 
> > > > (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) += bootmeth_efi.o
> > > >    obj-$(CONFIG_$(SPL_TPL_)BOOTMETH_SANDBOX) += bootmeth_sandbox.o
> > > >    obj-$(CONFIG_$(SPL_TPL_)BOOTMETH_SCRIPT) += bootmeth_script.o
> > > >    ifdef CONFIG_$(SPL_TPL_)BOOTSTD_FULL
> > > > -obj-$(CONFIG_$(SPL_TPL_)CMD_BOOTEFI_BOOTMGR) += bootmeth_efi_mgr.o
> > > > +obj-$(CONFIG_CMD_BOOTEFI_BOOTMGR) += bootmeth_efi_mgr.o
> > > >    obj-$(CONFIG_$(SPL_TPL_)BOOTSTD) += 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 bootmenu_data *menu,
> > > >    	return 1;
> > > >    }
> > > > -#if (CONFIG_IS_ENABLED(CMD_BOOTEFI_BOOTMGR)) && (CONFIG_IS_ENABLED(CMD_EFICONFIG))
> > > > +#if (IS_ENABLED(CONFIG_CMD_BOOTEFI_BOOTMGR)) && (CONFIG_IS_ENABLED(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 motivation 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-letter
> 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.

I will be taking most of these patches in the next day or two, but I can
avoid the EFI ones if you insist. They are however I believe correct.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20230207/8fd21517/attachment.sig>