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

[Heinrich Schuchardt]

On 2/7/23 15: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, 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.
> 

The code changes are ok with me. It were simply the commit messages 
which were insufficient.

Acked-by: Heinrich Schuchardt <heinrich.schuchardt at canonical.com>