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

[Tom Rini <trini@konsulko.com>]

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

On Fri, Oct 27, 2023 at 09:59:02AM +0900, AKASHI Takahiro wrote:
> 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.

Looking at all of the ways to solve this particular case again, OK, I
think we can go with #8, #10 and #11 as they are in this series, thanks
again.

-- 
Tom

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