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

[Simon Glass]

Hi Heinrich,

On Wed, 4 Dec 2024 at 02:10, Heinrich Schuchardt
<heinrich.schuchardt at canonical.com> wrote:
>
> On 27.11.24 15:14, Simon Glass wrote:
> > Hi Heinrich,
> >
> > On Wed, 27 Nov 2024 at 07:00, Heinrich Schuchardt
> > <heinrich.schuchardt at 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 at 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 at linaro.org>
> >>>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt at 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.

OK, thanks for the info. This patch seems OK since it fixes your problem,

Reviewed-by: Simon Glass <sjg at chromium.org>

Here's my understanding of what could be next:
- land my PXE series (which unfortunately needs another spin)
- update dhcp_run() so that it controls autostart, perhaps put
autostart in struct bootm_info?
- update bootstd so it only runs DHCP once on a device

Regards,
Simon