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

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

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

Best regards

Heinrich

> 
>>
>> -       return 0;
>> +       return ret;
>>   }
>>
>>   struct bootdev_ops eth_bootdev_ops = {
>> --
>> 2.45.2
>>
> 
> Regards,
> Simon