Re: [PATCH v2 4/6] net: eth_bootdev_hunt() should not run DHCP

[Heinrich Schuchardt <heinrich.schuchardt@canonical.com>]

On 27.11.24 15:14, Simon Glass wrote:
> Hi Heinrich,
> 
> On Wed, 27 Nov 2024 at 07:00, Heinrich Schuchardt
> <heinrich.schuchardt@canonical.com> wrote:
>>
>> On 27.11.24 14:07, Simon Glass wrote:
>>> Hi Heinrich,
>>>
>>> On Wed, 27 Nov 2024 at 00:07, Heinrich Schuchardt
>>> <heinrich.schuchardt@canonical.com> wrote:
>>>>
>>>> Currently when booting dhcp_run() may be executed multiple times:
>>>> once in eth_bootdev_hunt() and once in the network booting bootmeth.
>>>>
>>>> We need to call eth_bootdev_hunt() when setting up the EFI sub-system to
>>>> supply the simple network protocol. We don't need an IP address set up.
>>>>
>>>> We can reduce the bootime by not executing dhcp_run() in
>>>> eth_bootdev_hunt().
>>>>
>>>> Furthermore eth_bootdev_hunt() with autostart=yes leads on the legacy
>>>> network statck leads to downloading a file via TFTP and to booting the
>>>
>>> spelling

I will handle that in my pull request.

>>>
>>>> downloaded file.
>>>
>>> I have now found the feature you're referring to - the calls to
>>> env_get_autostart(). Perhaps we can remove this feature? It seems
>>> pretty old and I don't see boards using it.

Commands controlled by autostart include bootelf, dhcp, tftboot.

For tftpboot autostart=no makes sense if you may want to download 
multiple files (kernel, initrd, dtb) before booting or the downloaded 
file is a firmware upgrade.

But for PXE booting you will need autostart=yes. Many routers allow 
recovery via TFTP.

For bootelf autostart=no leads to only checking the file in memory but 
not actually booting.

I don't think that we can simply remove the setting looking at the 
diverse use cases.

>>>
>>> That feature stops bootstd working properly.
>>>
>>> But if we don't remove it, we need to disable autostart in dhcp_run(),
>>> since in that case it does not behave correctly. I can do a patch for
>>> that if needed.

That change can be implemented separately from this patch series.

>>>
>>>>
>>>> Instead of running dchp_run() just check that there is a network device
>>>> in eth_bootdev_hunt().
>>>>
>>>> Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
>>>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
>>>> ---
>>>> v2:
>>>>           reword the commit message
>>>> ---
>>>>    net/eth_bootdev.c | 30 ++++++++++++++++++------------
>>>>    1 file changed, 18 insertions(+), 12 deletions(-)
>>>>
>>>> diff --git a/net/eth_bootdev.c b/net/eth_bootdev.c
>>>> index 6ee54e3c790..b0fca6e8313 100644
>>>> --- a/net/eth_bootdev.c
>>>> +++ b/net/eth_bootdev.c
>>>> @@ -64,9 +64,23 @@ static int eth_bootdev_bind(struct udevice *dev)
>>>>           return 0;
>>>>    }
>>>>
>>>> +/**
>>>> + * eth_bootdev_hunt() - probe all network devices
>>>> + *
>>>> + * Network devices can also come from USB, but that is a higher
>>>> + * priority (BOOTDEVP_5_SCAN_SLOW) than network, so it should have been
>>>> + * enumerated already. If something like 'bootflow scan dhcp' is used,
>>>> + * then the user will need to run 'usb start' first.
>>>> + *
>>>> + * @info:      info structure describing this hunter
>>>> + * @show:      true to show information from the hunter
>>>> + *
>>>> + * Return:     0 if device found, -EINVAL otherwise
>>>> + */
>>>>    static int eth_bootdev_hunt(struct bootdev_hunter *info, bool show)
>>>>    {
>>>>           int ret;
>>>> +       struct udevice *dev = NULL;
>>>>
>>>>           if (!test_eth_enabled())
>>>>                   return 0;
>>>> @@ -78,19 +92,11 @@ static int eth_bootdev_hunt(struct bootdev_hunter *info, bool show)
>>>>                           log_warning("Failed to init PCI (%dE)\n", ret);
>>>>           }
>>>>
>>>> -       /*
>>>> -        * Ethernet devices can also come from USB, but that is a higher
>>>> -        * priority (BOOTDEVP_5_SCAN_SLOW) than ethernet, so it should have been
>>>> -        * enumerated already. If something like 'bootflow scan dhcp' is used
>>>> -        * then the user will need to run 'usb start' first.
>>>> -        */
>>>
>>> Please keep this comment
>>
>> I have moved the comment to the function description. See above. I think
>> that is the adequate place.
>>
>>>
>>>> -       if (IS_ENABLED(CONFIG_CMD_DHCP)) {
>>>> -               ret = dhcp_run(0, NULL, false);
>>>> -               if (ret)
>>>> -                       return -EINVAL;
>>>> -       }
>>>> +       ret = -EINVAL;
>>>> +       uclass_foreach_dev_probe(UCLASS_ETH, dev)
>>>> +               ret = 0;
>>>
>>> There is a uclass_probe_all() function.
>>>
>>> My suggestion from the original series was to just do the dhcp once.
>>>
>>> How does probing the ethernet devices actually help EFI? It is not
>>> needed for bootstd, since it probes any devices it uses.
>>
>> bootstd is not the only way to start an EFI application, you could use
>> bootefi or bootm.
> 
> That's fine, but I'm just trying to understand what probing does, in
> this particular case. Doesn't EFI probe a device before it uses it? I
> think I am just missing some context.

EFI block devices will not even exist if not probed. See function 
efi_disk_probe().

For network devices I would like to change the code in a similar fashion 
in future to make all network devices available in the EFI sub-system.

Best regards

Heinrich