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

[Simon Glass]

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
> >
> >> 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.
> >
> > 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.
> >
> >>
> >> 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.

Regards,
Simon