[PATCH v2 078/169] Correct SPL use of EFI_UNICODE_COLLATION_PROTOCOL2

[Heinrich Schuchardt]

On 2/7/23 02:36, Tom Rini wrote:
> On Tue, Feb 07, 2023 at 02:24:22AM +0100, Heinrich Schuchardt wrote:
>>
>>
>> On 2/7/23 00:38, Tom Rini wrote:
>>> On Tue, Feb 07, 2023 at 12:36:51AM +0100, Heinrich Schuchardt wrote:
>>>> On 2/5/23 23:39, Simon Glass wrote:
>>>>> This converts 1 usage of this option to the non-SPL form, since there is
>>>>> no SPL_EFI_UNICODE_COLLATION_PROTOCOL2 defined in Kconfig
>>>>>
>>>>> Signed-off-by: Simon Glass <sjg at chromium.org>
>>>>> ---
>>>>>
>>>>> (no changes since v1)
>>>>>
>>>>>     lib/efi_loader/efi_root_node.c | 2 +-
>>>>>     1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/lib/efi_loader/efi_root_node.c b/lib/efi_loader/efi_root_node.c
>>>>> index 21a014d7c21..108c14b95bd 100644
>>>>> --- a/lib/efi_loader/efi_root_node.c
>>>>> +++ b/lib/efi_loader/efi_root_node.c
>>>>> @@ -68,7 +68,7 @@ efi_status_t efi_root_node_register(void)
>>>>>     		 &efi_guid_dt_fixup_protocol,
>>>>>     		 &efi_dt_fixup_prot,
>>>>>     #endif
>>>>> -#if CONFIG_IS_ENABLED(EFI_UNICODE_COLLATION_PROTOCOL2)
>>>>> +#if IS_ENABLED(CONFIG_EFI_UNICODE_COLLATION_PROTOCOL2)
>>>>
>>>> I never received this patch in my inbox. Expect series with more than 50
>>>> mails not even to be copied to the spam folder. They are outright rejected
>>>> by my mail provider.
>>>>
>>>> I cannot see any problem solved by this patch. Why did you send it?
>>>
>>> You should look in to setting up lei to fetch the list then, as this is
>>> well explained in the cover letter.
>>
>> lei = circle of flowers is what I find in a dictionary. Do I miss an
>> idiomatic expression?
> 
> Yes, https://public-inbox.org/lei.html is a tool widely used in the
> kernel community, and also works for U-Boot as we're mirrored on
> lore.kernel.org. In short, it's what I'm using to download the mailing
> list now, without Google screwing up and putting some messages in the
> spam folder and then I miss them. Also nice when U-Boot gets cc'd on
> something later and it'll just pull the whole thread in from the other
> lists for me, for context.
> 
>> The cover letter describes an observation but does not point out any problem
>> relating to this observation.
> [snip to reorder]
>>
>> Is anything wrong about this use of CONFIG_IS_ENABLED(EFI_LOADER)?
>>
>> What is the reasoning behind wanting to use CONFIG_IS_ENABLED() only if an
>> SPL config option exists?
> 
> We have two macros, IS_ENABLED(CONFIG_FOO) and CONFIG_IS_ENABLED(FOO).
> The case where we use CONFIG_IS_ENABLED(FOO) and CONFIG_SPL_FOO (or
> _TPL_ or _VPL_) is not a valid symbol AND it makes sense to use
> CONFIG_IS_ENABLED(FOO) because we need to have the test evaluate to
> false because IS_ENABLED(CONFIG_FOO) would be true but we can rely on
> CONFIG_SPL_FOO being false due to being undefined is, on the whole, very
> rare. In specifics however, code supporting the EFI loader subsystem
> makes use of this because as for example yes, we modify printf related
> code. So that needs to be tested with CONFIG_IS_ENABLED(). However, in
> the example here never will we ever build lib/efi_loader/efi_root_node.c
> outside of the main U-Boot case, so we should be testing via
> IS_ENABLED().
> 
> [paste to re-order]
>> We don't have a CONFIG_SPL_EFI_LOADER. In vsnprintf_internal() we use
>> CONFIG_IS_ENABLED(EFI_LOADER). The code inside the #if condition is not
>> compiled in SPL. This is the desired behavior.
>>
>> How is moveconfig meant to accept this?
> [back in order]
>> Where is the documentation change describing that CONFIG_IS_ENABLED() should
>> only be used if an SPL config option exists?
> 
> Yes, in the final related series that moves to a "split config" build.

No related series was mentioned in the cover-letter.
Which series do you relate to?

> And if what's in there isn't enough, that series should get more. That's
> where CONFIG_IS_ENABLED(EFI_LOADER) in the vsprintf could would be

If the target is to remove CONFIG_IS_ENABLED() I would expect this 
mentioned in the individual commit messages so that we get this 
information into the git log.

Furthermore the reasoning behind eliminating CONFIG_IS_ENABLED() should 
be mentioned in the cover-letter.

Best regards

Heinrich

> handled because that series is where Simon removes CONFIG_IS_ENABLED(),
> adds more SPL_FOO def_bool n options, and moves to IS_ENABLED() for
> everything. And that series is still under discussion.
> 
> However, the vast majority if CONFIG_IS_ENABLED(FOO) where SPL_FOO, etc,
> are never defined should be using IS_ENABLED(CONFIG_FOO).
>