[PATCH] Revert "net: eth_bootdev_hunt() should not run DHCP"

[PATCH] Revert "net: eth_bootdev_hunt() should not run DHCP"

[Heiko Stuebner]

This reverts commit 1f68057e03206e6597ca8b2be8bb1c49d4bd47d0.

Commit 1f68057e0320 ("net: eth_bootdev_hunt() should not run DHCP")
aims to reduce EFI boot times by disabling the dhcp_run when
checking ethernet bootdevices, by preventing it from running double,
with the reasoning

    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.

That might by true for EFI, but not for everything else, because when
running distro-boot and for example the PXE method in it, nothing will
set up an IP address now.

So for example in my setup of Rockchip boards, no dhcp runs and pxe then
fails with messages like:

  Retrieving file: pxelinux.cfg/01-....
  Speed: 100, full duplex
  *** ERROR: `serverip' not set
  Retrieving file: pxelinux.cfg/00000000
  Speed: 100, full duplex
  *** ERROR: `serverip' not set

As the original commit message talks about speed efficiencies for the EFI
side mostly, reverting this should just restore previous functionality
(and in fact does so on my Rockchip boards).

Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
 net/eth_bootdev.c | 30 ++++++++++++------------------
 1 file changed, 12 insertions(+), 18 deletions(-)

diff --git a/net/eth_bootdev.c b/net/eth_bootdev.c
index b0fca6e8313..6ee54e3c790 100644
--- a/net/eth_bootdev.c
+++ b/net/eth_bootdev.c
@@ -64,23 +64,9 @@ 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;
@@ -92,11 +78,19 @@ static int eth_bootdev_hunt(struct bootdev_hunter *info, bool show)
 			log_warning("Failed to init PCI (%dE)\n", ret);
 	}
 
-	ret = -EINVAL;
-	uclass_foreach_dev_probe(UCLASS_ETH, dev)
-		ret = 0;
+	/*
+	 * 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.
+	 */
+	if (IS_ENABLED(CONFIG_CMD_DHCP)) {
+		ret = dhcp_run(0, NULL, false);
+		if (ret)
+			return -EINVAL;
+	}
 
-	return ret;
+	return 0;
 }
 
 struct bootdev_ops eth_bootdev_ops = {
-- 
2.47.2

Re: [PATCH] Revert "net: eth_bootdev_hunt() should not run DHCP"

[Simon Glass]

On Tue, 1 Apr 2025 at 21:38, Heiko Stuebner <heiko@sntech.de> wrote:
>
> This reverts commit 1f68057e03206e6597ca8b2be8bb1c49d4bd47d0.
>
> Commit 1f68057e0320 ("net: eth_bootdev_hunt() should not run DHCP")
> aims to reduce EFI boot times by disabling the dhcp_run when
> checking ethernet bootdevices, by preventing it from running double,
> with the reasoning
>
>     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.
>
> That might by true for EFI, but not for everything else, because when
> running distro-boot and for example the PXE method in it, nothing will
> set up an IP address now.
>
> So for example in my setup of Rockchip boards, no dhcp runs and pxe then
> fails with messages like:
>
>   Retrieving file: pxelinux.cfg/01-....
>   Speed: 100, full duplex
>   *** ERROR: `serverip' not set
>   Retrieving file: pxelinux.cfg/00000000
>   Speed: 100, full duplex
>   *** ERROR: `serverip' not set
>
> As the original commit message talks about speed efficiencies for the EFI
> side mostly, reverting this should just restore previous functionality
> (and in fact does so on my Rockchip boards).
>
> Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> ---
>  net/eth_bootdev.c | 30 ++++++++++++------------------
>  1 file changed, 12 insertions(+), 18 deletions(-)

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

Re: [PATCH] Revert "net: eth_bootdev_hunt() should not run DHCP"

[Heinrich Schuchardt]

On 01.04.25 17:51, Simon Glass wrote:
> On Tue, 1 Apr 2025 at 21:38, Heiko Stuebner <heiko@sntech.de> wrote:
>>
>> This reverts commit 1f68057e03206e6597ca8b2be8bb1c49d4bd47d0.
>>
>> Commit 1f68057e0320 ("net: eth_bootdev_hunt() should not run DHCP")
>> aims to reduce EFI boot times by disabling the dhcp_run when
>> checking ethernet bootdevices, by preventing it from running double,
>> with the reasoning
>>
>>      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.
>>
>> That might by true for EFI, but not for everything else, because when
>> running distro-boot and for example the PXE method in it, nothing will
>> set up an IP address now.

The removed call was dhcp_run(addr, NULL, true);

We have:

distro_efi_read_bootflow_net():
boot/bootmeth_efi.c:205:        ret = dhcp_run(addr, NULL, true);

script_read_bootflow_net():
boot/bootmeth_script.c:132:     ret = dhcp_run(addr, fname, true);

extlinux_pxe_read_bootflow() seems to be lacking the call.

So instead of reverting
1f68057e0320 ("net: eth_bootdev_hunt() should not run DHCP")
we should add the missing call in extlinux_pxe_read_bootflow().

Best regards

Heinrich

>>
>> So for example in my setup of Rockchip boards, no dhcp runs and pxe then
>> fails with messages like:
>>
>>    Retrieving file: pxelinux.cfg/01-....
>>    Speed: 100, full duplex
>>    *** ERROR: `serverip' not set
>>    Retrieving file: pxelinux.cfg/00000000
>>    Speed: 100, full duplex
>>    *** ERROR: `serverip' not set
>>
>> As the original commit message talks about speed efficiencies for the EFI
>> side mostly, reverting this should just restore previous functionality
>> (and in fact does so on my Rockchip boards).
>>
>> Signed-off-by: Heiko Stuebner <heiko@sntech.de>
>> ---
>>   net/eth_bootdev.c | 30 ++++++++++++------------------
>>   1 file changed, 12 insertions(+), 18 deletions(-)
>
> Reviewed-by: Simon Glass <sjg@chromium.org>

Re: [PATCH] Revert "net: eth_bootdev_hunt() should not run DHCP"

[Simon Glass]

Hi,

On Wed, 2 Apr 2025 at 05:13, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 01.04.25 17:51, Simon Glass wrote:
> > On Tue, 1 Apr 2025 at 21:38, Heiko Stuebner <heiko@sntech.de> wrote:
> >>
> >> This reverts commit 1f68057e03206e6597ca8b2be8bb1c49d4bd47d0.
> >>
> >> Commit 1f68057e0320 ("net: eth_bootdev_hunt() should not run DHCP")
> >> aims to reduce EFI boot times by disabling the dhcp_run when
> >> checking ethernet bootdevices, by preventing it from running double,
> >> with the reasoning
> >>
> >>      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.
> >>
> >> That might by true for EFI, but not for everything else, because when
> >> running distro-boot and for example the PXE method in it, nothing will
> >> set up an IP address now.
>
> The removed call was dhcp_run(addr, NULL, true);
>
> We have:
>
> distro_efi_read_bootflow_net():
> boot/bootmeth_efi.c:205:        ret = dhcp_run(addr, NULL, true);
>
> script_read_bootflow_net():
> boot/bootmeth_script.c:132:     ret = dhcp_run(addr, fname, true);
>
> extlinux_pxe_read_bootflow() seems to be lacking the call.
>
> So instead of reverting
> 1f68057e0320 ("net: eth_bootdev_hunt() should not run DHCP")
> we should add the missing call in extlinux_pxe_read_bootflow().
>

The original design was to run DHCP once, in the hunter. That seems to
have changed, due to it causing problems with EFI?

I'd like to go back to doing it in the hunter. It really doesn't make
sense for each bootmeth to do its own DHCP again, does it?

Regards,
Simon

Re: [PATCH] Revert "net: eth_bootdev_hunt() should not run DHCP"

[Heiko Stübner]

Am Dienstag, 1. April 2025, 18:13:35 MESZ schrieb Heinrich Schuchardt:
> On 01.04.25 17:51, Simon Glass wrote:
> > On Tue, 1 Apr 2025 at 21:38, Heiko Stuebner <heiko@sntech.de> wrote:
> >>
> >> This reverts commit 1f68057e03206e6597ca8b2be8bb1c49d4bd47d0.
> >>
> >> Commit 1f68057e0320 ("net: eth_bootdev_hunt() should not run DHCP")
> >> aims to reduce EFI boot times by disabling the dhcp_run when
> >> checking ethernet bootdevices, by preventing it from running double,
> >> with the reasoning
> >>
> >>      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.
> >>
> >> That might by true for EFI, but not for everything else, because when
> >> running distro-boot and for example the PXE method in it, nothing will
> >> set up an IP address now.
> 
> The removed call was dhcp_run(addr, NULL, true);
> 
> We have:
> 
> distro_efi_read_bootflow_net():
> boot/bootmeth_efi.c:205:        ret = dhcp_run(addr, NULL, true);
> 
> script_read_bootflow_net():
> boot/bootmeth_script.c:132:     ret = dhcp_run(addr, fname, true);
> 
> extlinux_pxe_read_bootflow() seems to be lacking the call.
> 
> So instead of reverting
> 1f68057e0320 ("net: eth_bootdev_hunt() should not run DHCP")
> we should add the missing call in extlinux_pxe_read_bootflow().

doing

----- 8< -----
diff --git a/boot/bootmeth_pxe.c b/boot/bootmeth_pxe.c
index b91e61bcbc4..6e5e0f99ea4 100644
--- a/boot/bootmeth_pxe.c
+++ b/boot/bootmeth_pxe.c
@@ -73,6 +73,10 @@ static int extlinux_pxe_read_bootflow(struct udevice *dev,
                return log_msg_ret("pxeb", -EPERM);
        addr = simple_strtoul(addr_str, NULL, 16);
 
+       ret = dhcp_run(addr, NULL, false);
+       if (ret)
+               return log_msg_ret("dhc", ret);
+
        log_debug("calling pxe_get()\n");
        ret = pxe_get(addr, &bootdir, &size, false);
        log_debug("pxe_get() returned %d\n", ret);
----- 8< -----

does seem to work in _my_ usecase and gets me loading stuff over pxe
again.

I guess this whole thing now becomes a strategy decision ;-) :

- it's very late in the release, do we revert to the known working state
  or hope the above fixes all issues
- Simon's wish of not sprinkling dhcp_run calls in multiple places

I guess I'm fine with both way, as either fixes my use-case ;-)

Heiko

Re: [PATCH] Revert "net: eth_bootdev_hunt() should not run DHCP"

[Heiko Stübner]

Am Mittwoch, 2. April 2025, 23:36:46 MESZ schrieb Heiko Stübner:
> Am Dienstag, 1. April 2025, 18:13:35 MESZ schrieb Heinrich Schuchardt:
> > On 01.04.25 17:51, Simon Glass wrote:
> > > On Tue, 1 Apr 2025 at 21:38, Heiko Stuebner <heiko@sntech.de> wrote:
> > >>
> > >> This reverts commit 1f68057e03206e6597ca8b2be8bb1c49d4bd47d0.
> > >>
> > >> Commit 1f68057e0320 ("net: eth_bootdev_hunt() should not run DHCP")
> > >> aims to reduce EFI boot times by disabling the dhcp_run when
> > >> checking ethernet bootdevices, by preventing it from running double,
> > >> with the reasoning
> > >>
> > >>      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.
> > >>
> > >> That might by true for EFI, but not for everything else, because when
> > >> running distro-boot and for example the PXE method in it, nothing will
> > >> set up an IP address now.
> > 
> > The removed call was dhcp_run(addr, NULL, true);
> > 
> > We have:
> > 
> > distro_efi_read_bootflow_net():
> > boot/bootmeth_efi.c:205:        ret = dhcp_run(addr, NULL, true);
> > 
> > script_read_bootflow_net():
> > boot/bootmeth_script.c:132:     ret = dhcp_run(addr, fname, true);
> > 
> > extlinux_pxe_read_bootflow() seems to be lacking the call.
> > 
> > So instead of reverting
> > 1f68057e0320 ("net: eth_bootdev_hunt() should not run DHCP")
> > we should add the missing call in extlinux_pxe_read_bootflow().
> 
> doing
> 
> ----- 8< -----
> diff --git a/boot/bootmeth_pxe.c b/boot/bootmeth_pxe.c
> index b91e61bcbc4..6e5e0f99ea4 100644
> --- a/boot/bootmeth_pxe.c
> +++ b/boot/bootmeth_pxe.c
> @@ -73,6 +73,10 @@ static int extlinux_pxe_read_bootflow(struct udevice *dev,
>                 return log_msg_ret("pxeb", -EPERM);
>         addr = simple_strtoul(addr_str, NULL, 16);
>  
> +       ret = dhcp_run(addr, NULL, false);
> +       if (ret)
> +               return log_msg_ret("dhc", ret);
> +
>         log_debug("calling pxe_get()\n");
>         ret = pxe_get(addr, &bootdir, &size, false);
>         log_debug("pxe_get() returned %d\n", ret);
> ----- 8< -----
> 
> does seem to work in _my_ usecase and gets me loading stuff over pxe
> again.

For reference, I implemented that now in
https://lore.kernel.org/u-boot/20250402215025.2655384-1-heiko@sntech.de/

So people can decide which way to go :-)


Heiko


> I guess this whole thing now becomes a strategy decision ;-) :
> 
> - it's very late in the release, do we revert to the known working state
>   or hope the above fixes all issues
> - Simon's wish of not sprinkling dhcp_run calls in multiple places
> 
> I guess I'm fine with both way, as either fixes my use-case ;-)
> 
> Heiko

Re: [PATCH] Revert "net: eth_bootdev_hunt() should not run DHCP"

[Simon Glass]

Hi Heiko,

On Thu, 3 Apr 2025 at 10:36, Heiko Stübner <heiko@sntech.de> wrote:
>
> Am Dienstag, 1. April 2025, 18:13:35 MESZ schrieb Heinrich Schuchardt:
> > On 01.04.25 17:51, Simon Glass wrote:
> > > On Tue, 1 Apr 2025 at 21:38, Heiko Stuebner <heiko@sntech.de> wrote:
> > >>
> > >> This reverts commit 1f68057e03206e6597ca8b2be8bb1c49d4bd47d0.
> > >>
> > >> Commit 1f68057e0320 ("net: eth_bootdev_hunt() should not run DHCP")
> > >> aims to reduce EFI boot times by disabling the dhcp_run when
> > >> checking ethernet bootdevices, by preventing it from running double,
> > >> with the reasoning
> > >>
> > >>      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.
> > >>
> > >> That might by true for EFI, but not for everything else, because when
> > >> running distro-boot and for example the PXE method in it, nothing will
> > >> set up an IP address now.
> >
> > The removed call was dhcp_run(addr, NULL, true);
> >
> > We have:
> >
> > distro_efi_read_bootflow_net():
> > boot/bootmeth_efi.c:205:        ret = dhcp_run(addr, NULL, true);
> >
> > script_read_bootflow_net():
> > boot/bootmeth_script.c:132:     ret = dhcp_run(addr, fname, true);
> >
> > extlinux_pxe_read_bootflow() seems to be lacking the call.
> >
> > So instead of reverting
> > 1f68057e0320 ("net: eth_bootdev_hunt() should not run DHCP")
> > we should add the missing call in extlinux_pxe_read_bootflow().
>
> doing
>
> ----- 8< -----
> diff --git a/boot/bootmeth_pxe.c b/boot/bootmeth_pxe.c
> index b91e61bcbc4..6e5e0f99ea4 100644
> --- a/boot/bootmeth_pxe.c
> +++ b/boot/bootmeth_pxe.c
> @@ -73,6 +73,10 @@ static int extlinux_pxe_read_bootflow(struct udevice *dev,
>                 return log_msg_ret("pxeb", -EPERM);
>         addr = simple_strtoul(addr_str, NULL, 16);
>
> +       ret = dhcp_run(addr, NULL, false);
> +       if (ret)
> +               return log_msg_ret("dhc", ret);
> +
>         log_debug("calling pxe_get()\n");
>         ret = pxe_get(addr, &bootdir, &size, false);
>         log_debug("pxe_get() returned %d\n", ret);
> ----- 8< -----
>
> does seem to work in _my_ usecase and gets me loading stuff over pxe
> again.
>
> I guess this whole thing now becomes a strategy decision ;-) :
>
> - it's very late in the release, do we revert to the known working state
>   or hope the above fixes all issues
> - Simon's wish of not sprinkling dhcp_run calls in multiple places
>
> I guess I'm fine with both way, as either fixes my use-case ;-)

For the reasons you patch seems fine to me. As you say, it is very
late in the cycle.

I already gave this feedback to Heinrich, so once the revert is in he
may have time to take a look.

Regards,
Simon