Re: [PATCH v3 22/32] efi: Update EFI_LOADER to depend on DM_ETH

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

On Thu, Oct 19, 2023 at 08:01:11AM -0600, Simon Glass wrote:
> Hi Heinrich,
> 
> On Wed, 18 Oct 2023 at 06:55, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >
> > On 10/17/23 16:09, Tom Rini wrote:
> > > On Mon, Oct 16, 2023 at 04:28:13PM -0600, Simon Glass wrote:
> > >
> > >> Since efi_device_path.c calls eth_get_dev() and assumes that Ethernet is
> > >> available, add it as an explicit dependency.
> > >>
> > >> Signed-off-by: Simon Glass <sjg@chromium.org>
> > >> ---
> > >>
> > >> (no changes since v2)
> > >>
> > >> Changes in v2:
> > >> - Add new patch to update EFI_LOADER to depend on DM_ETH
> > >>
> > >>   lib/efi_loader/Kconfig | 1 +
> > >>   1 file changed, 1 insertion(+)
> > >>
> > >> diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> > >> index 13cad6342c36..fca4b3eef270 100644
> > >> --- a/lib/efi_loader/Kconfig
> > >> +++ b/lib/efi_loader/Kconfig
> > >> @@ -11,6 +11,7 @@ config EFI_LOADER
> > >>      # We need EFI_STUB_32BIT to be set on x86_32 with EFI_STUB
> > >>      depends on !EFI_STUB || !X86 || X86_64 || EFI_STUB_32BIT
> > >>      depends on BLK
> > >> +    depends on DM_ETH
> > >>      depends on !EFI_APP
> > >>      default y if !ARM || SYS_CPU = armv7 || SYS_CPU = armv8
> > >>      select CHARSET
> > >
> > > Does this work for you Heinrich, or do you want to clarify the
> > > dependencies (and re-organize the code as needed) around networking?
> > >
> >
> > We should be able to boot via EFI on devices without U-Boot network support.
> >
> > We already use IS_ENABLED(CONFIG_NETDEVICES) to avoid invoking
> > eth_get_dev() if there is no network. CONFIG_NETDEVICES=y selects
> > CONFIG_DM_ETH.
> >
> > Why is this not sufficient?
> > Is there a configuration that does not build?
> 
> The point of this series is to disable CMDLINE and fix up what breaks.
> 
> In this case we have some sort of breakage...perhaps Tom has already
> found it, but otherwise could you take a look?
> 
> We should be able to disable NET and LTO in sandbox and still build.
> But this fails at present[1]. You can try it on -master

Obviously, it would be necessary to enclose efi_dp_from_eth()
with "if defined(CONFIG_NETDEVICES)" (or DM_ETH).
Then, we could drop "depends on DM_ETH".

Another possible place for completeness is "case UCLASS_ETH" clause
in dp_size(), but it seems to be harmless.

-Takahiro Akashi


> Regards,
> Simon
> 
> [1] sjg@sjg1:~/u$ crosfw sandbox -L
> cmd: make -j4 'CROSS_COMPILE=' --no-print-directory 'HOSTSTRIP=true'
> 'QEMU_ARCH=' 'KCONFIG_NOSILENTUPDATE=1' 'O=/tmp/b/sandbox' 'NO_LTO=1'
> -s 'BUILD_ROM=1' all
> /usr/bin/ld: lib/efi_loader/efi_device_path.o: in function `efi_dp_from_eth':
> /home/sjg/c/src/third_party/u-boot/files/lib/efi_loader/efi_device_path.c:985:(.text+0xca4):
> undefined reference to `eth_get_dev'
> /usr/bin/ld: /home/sjg/c/src/third_party/u-boot/files/lib/efi_loader/efi_device_path.c:987:(.text+0xca9):
> undefined reference to `eth_get_dev'
> /usr/bin/ld: /home/sjg/c/src/third_party/u-boot/files/lib/efi_loader/efi_device_path.c:993:(.text+0xcc9):
> undefined reference to `eth_get_dev'
> collect2: error: ld returned 1 exit status
> make[1]: *** [/home/sjg/c/src/third_party/u-boot/files/Makefile:1765:
> u-boot] Error 1
> make: *** [Makefile:177: sub-make] Error 2
> 
> sjg@sjg1:~/u$ crosfw sandbox
> sjg@sjg1:~/u$ (passes)
> 
> 'crosfw xx' is just 'buildman --bo xxx'
> 
> diff --git a/boot/pxe_utils.c b/boot/pxe_utils.c
> index a92bb896c63e..4e9996a92342 100644
> --- a/boot/pxe_utils.c
> +++ b/boot/pxe_utils.c
> @@ -48,7 +48,7 @@ int pxe_get_file_size(ulong *sizep)
> 
>   return 0;
>  }
> -
> +#if 0
>  /**
>   * format_mac_pxe() - obtain a MAC address in the PXE format
>   *
> @@ -82,7 +82,7 @@ int format_mac_pxe(char *outbuf, size_t outbuf_len)
> 
>   return 1;
>  }
> -
> +#endif
>  /**
>   * get_relfile() - read a file relative to the PXE file
>   *
> diff --git a/configs/sandbox_defconfig b/configs/sandbox_defconfig
> index 47417cb0391d..57bb6202dbb5 100644
> --- a/configs/sandbox_defconfig
> +++ b/configs/sandbox_defconfig
> @@ -349,3 +349,4 @@ CONFIG_TEST_FDTDEC=y
>  CONFIG_UNIT_TEST=y
>  CONFIG_UT_TIME=y
>  CONFIG_UT_DM=y
> +# CONFIG_NET is not set
> 
> Regards,
> Simon