Re: [RFC 11/13] fs: remove explicit efi configuration dependency

[AKASHI Takahiro <takahiro.akashi@linaro.org>]

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

On Thu, Oct 26, 2023 at 08:47:52AM -0400, Tom Rini wrote:
> On Thu, Oct 26, 2023 at 05:48:30PM +0900, AKASHI Takahiro wrote:
> > On Thu, Oct 26, 2023 at 09:58:53AM +0200, Heinrich Schuchardt wrote:
> > > 
> > > 
> > > Am 26. Oktober 2023 07:30:50 MESZ schrieb AKASHI Takahiro <takahiro.akashi@linaro.org>:
> > > >Now it is clear that the feature actually depends on efi interfaces,
> > > >not "bootefi" command. efi_set_bootdev() will automatically be nullified
> > > >if necessary efi component is disabled.
> > > >
> > > >Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > > >---
> > > > fs/fs.c | 7 +++----
> > > > 1 file changed, 3 insertions(+), 4 deletions(-)
> > > >
> > > >diff --git a/fs/fs.c b/fs/fs.c
> > > >index 4cb4310c9cc2..70cdb594c4c8 100644
> > > >--- a/fs/fs.c
> > > >+++ b/fs/fs.c
> > > >@@ -791,10 +791,9 @@ int do_load(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[],
> > > > 		return 1;
> > > > 	}
> > > > 
> > > >-	if (IS_ENABLED(CONFIG_CMD_BOOTEFI))
> > > >-		efi_set_bootdev(argv[1], (argc > 2) ? argv[2] : "",
> > > >-				(argc > 4) ? argv[4] : "", map_sysmem(addr, 0),
> > > >-				len_read);
> > > >+	efi_set_bootdev(argv[1], (argc > 2) ? argv[2] : "",
> > > 
> > > This function should not exist for CONFIG_EFI_LOADER=n. There are other places where the function is invoked. Please, review all of them.
> > 
> > Please go through the whole patch set, especially patch#8
> > "efi_loader: split unrelated code from efi_bootmgr.c".
> > 
> > efi_[set|clear]_bootdev() will be nullified if not necessary.
> 
> In this case I think what we have here today is more readable / clearer.
> We don't need empty functions as we can either do like this section of
> the code does today or the linker will discard things correctly as it's
> a case of funcB() calls funcA() and nothing calls funcB() so it will be
> discarded. I don't know without digging through the series more what the
> correct IS_ENABLED() guard should be here (and then also in patch #10).

If I correctly understand your question, it is my point in this commit.

I believe that a caller should not be bothered by thinking of what efi CONFIG
be used for the guard. All that should be done is to just put a right hook
(efi_set_bootdev() in this case) at a right place as a caller (do_load()
in this case) is apparently irrelevant to UEFI subsystem.
Hereafter, whatever rework may be done on UEFI side (like my refactoring
here), other code won't need to be modified.

If you want to rely on an intelligent linker behavior, I would suggest
another approach, modifying efi_set_bootdev() as follows;

efi_set_bootdev(...)
{
    if (!IS_ENABLED(EFI_BINARY_EXEC))
        return;
    ...
}

I hope it would also work.

-Takahiro Akashi

> 
> -- 
> Tom



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