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

[Tom Rini <trini@konsulko.com>]

[-- Attachment #1: Type: text/plain, Size: 4686 bytes --]

On Tue, Feb 07, 2023 at 08:31:59AM -0700, Simon Glass wrote:
> Hi Tom,
> 
> On Tue, 7 Feb 2023 at 07:50, Tom Rini <trini@konsulko.com> 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, since there is
> > > > > > no SPL_CMD_BOOTEFI_BOOTMGR defined in Kconfig
> > > > > >
> > > > > > Signed-off-by: Simon Glass <sjg@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.
> 
> 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*.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]