[PATCH v2 4/6] net: eth_bootdev_hunt() must not try to boot

[Simon Glass]

Hi Heinrich,

On Tue, 26 Nov 2024 at 09:16, Heinrich Schuchardt
<heinrich.schuchardt at canonical.com> wrote:
>
> On 26.11.24 16:38, Simon Glass wrote:
> > Hi Heinrich,
> >
> > On Tue, 26 Nov 2024 at 00:42, Heinrich Schuchardt
> > <heinrich.schuchardt at canonical.com> wrote:
> >>
> >> On 11/26/24 01:32, Simon Glass wrote:
> >>> Hi Heinrich,
> >>>
> >>> On Sat, 23 Nov 2024 at 14:46, Heinrich Schuchardt
> >>> <heinrich.schuchardt at canonical.com> wrote:
> >>>>
> >>>> eth_bootdev_hunt() should not execute dhcp_run() as this itself would load
> >>>> a file and boot it if autostart=yes.
> >>>>
> >>>> Instead just check that there is a network device.
> >>>
> >>> If you look at dhcp_run() you will see an 'autoload' parameter. This
> >>> is 'false' in this code, so it never loads a file.
> >>
> >> This is not the complete truth.
> >>
> >> If $autostart=true and CONFIG_NET=y, dhcp_run(false) still tries to load
> >> a file.
> >
> > Here is the code:
> >
> > int dhcp_run(ulong addr, const char *fname, bool autoload)
> > {
> > char *dhcp_argv[] = {"dhcp", NULL, (char *)fname, NULL};
> > struct cmd_tbl cmdtp = {}; /* dummy */
> > char file_addr[17];
> > int old_autoload;
> > int ret, result;
> >
> > log_debug("addr=%lx, fname=%s, autoload=%d\n", addr, fname, autoload);
> > old_autoload = env_get_yesno("autoload");
> > ret = env_set("autoload", autoload ? "y" : "n");
> > if (ret)
> > return log_msg_ret("en1", -EINVAL);
> >
> > It respects the autoload parameter. If there is a bug in that, we
> > should fix it, but it seems to work OK to me. Please fix your commit
> > message.
>
> The cited code only sets "autoload". It does not adjust "autostart".
>
> As I have shown below this leads to trying to boot via DHCP.
>
> This is exactly what the commit message says: "load a file and boot it
> if autostart=yes".

Oh, I just thought they were the same and I didn't remember about autostart

>
> >
> >>
> >> In all cases dhcp_run() wastes boot time.
> >
> > Yes, it uses time. With bootstd the hunters are only called as needed.
> > In your case you are calling all of them at once.
> >
> > Is there a way to just call hunters which relate to block devices, perhaps?
>
> We need to probe network to provide the simple network protocol
> (lib/efi_loader/efi_net.c) but we don't need to execute DHCP.
>
> We also need the correct device-tree provided by the extension hunter.

It would be good to get some use of that, as it has no tests really.

As I ask on the other patch, why do you need to probe?

>
> Best regards
>
> Heinrich
>
> >
> >>
> >> CONFIG_NET=y
> >> => dhcp_run
> >> BOOTP broadcast 1
> >> BOOTP broadcast 2
> >> BOOTP broadcast 3
> >> DHCP client bound to address 10.0.2.15 (1004 ms)
> >>
> >> => setenv autostart true
> >> => dhcp run
> >> BOOTP broadcast 1
> >> BOOTP broadcast 2
> >> BOOTP broadcast 3
> >> DHCP client bound to address 10.0.2.15 (1002 ms)
> >> Using e1000#0 device
> >> TFTP from server 10.0.2.2; our IP address is 10.0.2.15
> >> Filename 'run'.
> >> Load address: 0x40200000
> >> Loading: *
> >> TFTP error: 'File not found' (1)
> >> Not retrying...
> >>
> >> CONFIG_NET_LWIP=y
> >> => dhcp_run
> >> DHCP client bound to address 10.0.2.15 (1007 ms)
> >>
> >> Where command dhcp_run is provided by:
> >>
> >> #include <command.h>
> >> #include <env.h>
> >> #include <net-common.h>
> >> #include <vsprintf.h>
> >>
> >> static int do_dhcp_run(struct cmd_tbl *cmdtp, int flag, int argc,
> >>                     char *const argv[])
> >> {
> >>           ulong addr = hextoul(env_get("loadaddr"), NULL);
> >>
> >>           dhcp_run(addr, NULL, false);
> >>
> >>           return 0;
> >> }
> >>
> >> U_BOOT_CMD(
> >>           dhcp_run,       CONFIG_SYS_MAXARGS,     1,      do_dhcp_run,
> >>           "dhcp_run",
> >>           NULL
> >> );
> >
> > See above
> >
> > [..]
> >
> > Regards,
> > Simon
>

Regards,
Simon