[RFC PATCH 1/2] net: ifup: prefer interfaces where the link is already up

[RFC PATCH 1/2] net: ifup: prefer interfaces where the link is already up

[Ahmad Fatoum]

ifup -a on a platform with a DSA switch can take quite a while, because
barebox will attempt sending a DHCP DISCOVER on every port in sequence
and waiting until timeout. This could use some refactoring to make it
possible to make dhcp() non-blocking, but let's do an easier workaround
for that: Let's prefer ports that already have a link-up, which is the
case if the user has been sitting idly on the shell for a few seconds,
because we enable all interfaces that weren't explicitly disabled then.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 include/net.h | 1 +
 net/eth.c     | 2 +-
 net/ifup.c    | 8 ++++++++
 3 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/include/net.h b/include/net.h
index 0555b0bd6bed..fdd21481dbf5 100644
--- a/include/net.h
+++ b/include/net.h
@@ -110,6 +110,7 @@ int eth_register(struct eth_device* dev);    /* Register network device		*/
 void eth_unregister(struct eth_device* dev); /* Unregister network device	*/
 int eth_set_ethaddr(struct eth_device *edev, const char *ethaddr);
 int eth_open(struct eth_device *edev);
+int eth_carrier_check(struct eth_device *edev, bool may_wait);
 void eth_close(struct eth_device *edev);
 int eth_send(struct eth_device *edev, void *packet, int length);	   /* Send a packet		*/
 int eth_rx(void);			/* Check for received packets	*/
diff --git a/net/eth.c b/net/eth.c
index 6fb64afea024..6c201d7dfc30 100644
--- a/net/eth.c
+++ b/net/eth.c
@@ -179,7 +179,7 @@ int eth_complete(struct string_list *sl, char *instr)
 /*
  * Check for link if we haven't done so for longer.
  */
-static int eth_carrier_check(struct eth_device *edev, bool may_wait)
+int eth_carrier_check(struct eth_device *edev, bool may_wait)
 {
 	int ret;
 
diff --git a/net/ifup.c b/net/ifup.c
index e4d5374db3ae..a0167eeb8a82 100644
--- a/net/ifup.c
+++ b/net/ifup.c
@@ -285,6 +285,14 @@ int ifup_all(unsigned flags)
 	    list_empty(&netdev_list))
 		device_detect_all();
 
+	/* Prefer interfaces where the link is already up */
+	for_each_netdev(edev) {
+		if (eth_carrier_check(edev, false))
+			continue;
+
+		ifup_edev(edev, flags);
+	}
+
 	for_each_netdev(edev)
 		ifup_edev(edev, flags);
 
-- 
2.30.2

[RFC PATCH 2/2] net: ifup: have ifup -a stop at the first interface

[Ahmad Fatoum]

The normal use case for ifup -a is to get *some* interface working and
not really wait for all interfaces to come up and then timeout waiting
for those without link up to get their DHCP lease. Thus repurpose ifup
-a to mean bring up all interfaces until first success with the new
ifup -A restoring the old behavior.

The optimal action would be to make dhcp() non-blocking, but that would
be somewhat more involved.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 include/net.h |  1 +
 net/ifup.c    | 21 ++++++++++++++++-----
 2 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/include/net.h b/include/net.h
index fdd21481dbf5..1046d1bf3681 100644
--- a/include/net.h
+++ b/include/net.h
@@ -512,6 +512,7 @@ int net_icmp_send(struct net_connection *con, int len);
 void led_trigger_network(enum led_trigger trigger);
 
 #define IFUP_FLAG_FORCE		(1 << 0)
+#define IFUP_FLAG_UNTIL_FIRST	(1 << 1)
 
 int ifup_edev(struct eth_device *edev, unsigned flags);
 int ifup(const char *name, unsigned flags);
diff --git a/net/ifup.c b/net/ifup.c
index a0167eeb8a82..1ae7dad76b87 100644
--- a/net/ifup.c
+++ b/net/ifup.c
@@ -291,11 +291,17 @@ int ifup_all(unsigned flags)
 			continue;
 
 		ifup_edev(edev, flags);
+		if ((flags & IFUP_FLAG_UNTIL_FIRST) && edev->ifup)
+			return 0;
 	}
 
-	for_each_netdev(edev)
+	for_each_netdev(edev) {
 		ifup_edev(edev, flags);
 
+		if ((flags & IFUP_FLAG_UNTIL_FIRST) && edev->ifup)
+			return 0;
+	}
+
 	return 0;
 }
 
@@ -326,12 +332,15 @@ static int do_ifup(int argc, char *argv[])
 	unsigned flags = 0;
 	int all = 0;
 
-	while ((opt = getopt(argc, argv, "af")) > 0) {
+	while ((opt = getopt(argc, argv, "aAf")) > 0) {
 		switch (opt) {
 		case 'f':
 			flags |= IFUP_FLAG_FORCE;
 			break;
 		case 'a':
+			flags |= IFUP_FLAG_UNTIL_FIRST;
+			fallthrough;
+		case 'A':
 			all = 1;
 			break;
 		}
@@ -353,14 +362,15 @@ BAREBOX_CMD_HELP_TEXT("Network interfaces are configured with a NV variables or
 BAREBOX_CMD_HELP_TEXT("/env/network/<intf> file. See Documentation/user/networking.rst")
 BAREBOX_CMD_HELP_TEXT("")
 BAREBOX_CMD_HELP_TEXT("Options:")
-BAREBOX_CMD_HELP_OPT ("-a",  "bring up all interfaces")
+BAREBOX_CMD_HELP_OPT ("-A",  "bring up all interfaces")
+BAREBOX_CMD_HELP_OPT ("-a",  "bring up all interfaces until first successful one")
 BAREBOX_CMD_HELP_OPT ("-f",  "Force. Configure even if ip already set")
 BAREBOX_CMD_HELP_END
 
 BAREBOX_CMD_START(ifup)
 	.cmd		= do_ifup,
 	BAREBOX_CMD_DESC("bring a network interface up")
-	BAREBOX_CMD_OPTS("[-af] [INTF]")
+	BAREBOX_CMD_OPTS("[-aAf] [INTF]")
 	BAREBOX_CMD_GROUP(CMD_GRP_NET)
 	BAREBOX_CMD_COMPLETE(eth_complete)
 	BAREBOX_CMD_HELP(cmd_ifup_help)
@@ -371,8 +381,9 @@ static int do_ifdown(int argc, char *argv[])
 	int opt;
 	int all = 0;
 
-	while ((opt = getopt(argc, argv, "a")) > 0) {
+	while ((opt = getopt(argc, argv, "aA")) > 0) {
 		switch (opt) {
+		case 'A':
 		case 'a':
 			all = 1;
 			break;
-- 
2.30.2

Re: [RFC PATCH 1/2] net: ifup: prefer interfaces where the link is already up

[Sascha Hauer]

On Wed, Jan 25, 2023 at 10:52:35AM +0100, Ahmad Fatoum wrote:
> ifup -a on a platform with a DSA switch can take quite a while, because
> barebox will attempt sending a DHCP DISCOVER on every port in sequence
> and waiting until timeout. This could use some refactoring to make it
> possible to make dhcp() non-blocking, but let's do an easier workaround
> for that: Let's prefer ports that already have a link-up, which is the
> case if the user has been sitting idly on the shell for a few seconds,
> because we enable all interfaces that weren't explicitly disabled then.
> 
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
>  include/net.h | 1 +
>  net/eth.c     | 2 +-
>  net/ifup.c    | 8 ++++++++
>  3 files changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/include/net.h b/include/net.h
> index 0555b0bd6bed..fdd21481dbf5 100644
> --- a/include/net.h
> +++ b/include/net.h
> @@ -110,6 +110,7 @@ int eth_register(struct eth_device* dev);    /* Register network device		*/
>  void eth_unregister(struct eth_device* dev); /* Unregister network device	*/
>  int eth_set_ethaddr(struct eth_device *edev, const char *ethaddr);
>  int eth_open(struct eth_device *edev);
> +int eth_carrier_check(struct eth_device *edev, bool may_wait);
>  void eth_close(struct eth_device *edev);
>  int eth_send(struct eth_device *edev, void *packet, int length);	   /* Send a packet		*/
>  int eth_rx(void);			/* Check for received packets	*/
> diff --git a/net/eth.c b/net/eth.c
> index 6fb64afea024..6c201d7dfc30 100644
> --- a/net/eth.c
> +++ b/net/eth.c
> @@ -179,7 +179,7 @@ int eth_complete(struct string_list *sl, char *instr)
>  /*
>   * Check for link if we haven't done so for longer.
>   */
> -static int eth_carrier_check(struct eth_device *edev, bool may_wait)
> +int eth_carrier_check(struct eth_device *edev, bool may_wait)
>  {
>  	int ret;
>  
> diff --git a/net/ifup.c b/net/ifup.c
> index e4d5374db3ae..a0167eeb8a82 100644
> --- a/net/ifup.c
> +++ b/net/ifup.c
> @@ -285,6 +285,14 @@ int ifup_all(unsigned flags)
>  	    list_empty(&netdev_list))
>  		device_detect_all();
>  
> +	/* Prefer interfaces where the link is already up */
> +	for_each_netdev(edev) {
> +		if (eth_carrier_check(edev, false))
> +			continue;
> +
> +		ifup_edev(edev, flags);
> +	}
> +

This patch confused my while reading it. It doesn't "prefer" interfaces,
instead it only changes the order they are brought up. This change
doesn't make sense by itself, only reading patch 2/2 makes your intention
clear. I think you should merge both patches together.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [RFC PATCH 2/2] net: ifup: have ifup -a stop at the first interface

[Sascha Hauer]

On Wed, Jan 25, 2023 at 10:52:36AM +0100, Ahmad Fatoum wrote:
> The normal use case for ifup -a is to get *some* interface working and
> not really wait for all interfaces to come up and then timeout waiting
> for those without link up to get their DHCP lease. Thus repurpose ifup
> -a to mean bring up all interfaces until first success with the new
> ifup -A restoring the old behavior.

I am not very keen on changing the meaning of the -a option. We should
rather keep -a like it is and add a new option for your purpose, like
-s (for "s"ome interface) or -o ("o"ne interface).
The occurences of "ifup -a" in the tree can/should be changed then
accordingly as well.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |