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

[Heinrich Schuchardt]

On 27.11.24 14:08, Simon Glass wrote:
> 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?

You asked on the Network protocol.

If your idea is that the concept of extensions is not yet deployed, is 
there a good way to exclude BOOTDEV_HUNTER(extension_bootdev_hunter)?

Best regards

Heinrich

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