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

[Heinrich Schuchardt]

On 10/21/23 20:26, Tom Rini wrote:
> On Sat, Oct 21, 2023 at 08:43:08AM -0700, Simon Glass wrote:
>> Hi,
>>
>> On Thu, 19 Oct 2023 at 17:30, AKASHI Takahiro
>> <takahiro.akashi at linaro.org> wrote:
>>>
>>> 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 at 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 at 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".
>>
>> Strange that it only happens on the non-LTO board, though?
>
> There's two issues. The first of which is that I think you need to
> re-check your error exactly? With my series, and LTO also disabled the
> problem is a call to efi_get_image_parameters() as that's defined in
> cmd/bootefi.c, but also only used with cmdline invocations. So we can
> fix that CMDLINE=n && LTO=n case with a IS_ENABLED(CONFIG_CMDLINE)
> around that, and then discard efi_dp_from_name() entirely.
>
> The second issue is that with LTO we more completely find the cases
> where if x() calls y() and y() is undefined but nothing calls x() we can
> just discard x() and not care that y() is undefined.
>

I will send a patch for function efi_dp_from_eth().

@Simon

One thing that I don't understand is why we don't let the linker
eliminate the unused functions on the sandbox.

On other architectures we put each function into a separate text section
and let the linker eliminate the unused text sections:

arch/riscv/config.mk:29:
PLATFORM_RELFLAGS       += -fno-common -ffunction-sections -fdata-sections
LDFLAGS_u-boot          += --gc-sections -static -pie

Shouldn't we keep the sandbox close to what other architectures do?

Best regards

Heinrich