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@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.
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
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).

-- 
Tom