[U-Boot] [PATCH] net: Improve the speed of netconsole

[U-Boot] [PATCH] net: Improve the speed of netconsole

[Joe Hershberger]

Previously u-boot would initialize the network interface for every
network operation and then shut it down again.  This makes sense for
most operations where the network in not known to be needed soon after
the operation is complete.  In the case of netconsole, it will use the
network for every interaction with the shell or every printf.  This
means that the network is being reinitialized very often.  On many
devices, this intialization is very slow.

This patch checks for consecutive netconsole actions and leaves the
ethernet hardware initialized between them.  It will still behave the
same old way for all other network operations and any time another
network operation happens between netconsole operations.

Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
Cc: Stefano Babic <sbabic@denx.de>
---
 doc/README.NetConsole    |    3 +++
 drivers/net/netconsole.c |   26 ++++++++++++++++++++++----
 include/net.h            |    6 ++++++
 net/eth.c                |   14 ++++++++++++++
 net/net.c                |   44 +++++++++++++++++++++++++++++++++++++++-----
 5 files changed, 84 insertions(+), 9 deletions(-)

diff --git a/doc/README.NetConsole b/doc/README.NetConsole
index c8bcb90..7a20bf5 100644
--- a/doc/README.NetConsole
+++ b/doc/README.NetConsole
@@ -6,6 +6,9 @@ serial and network input/output devices by adjusting the 'stdin' and
 set either of these variables to "nc". Input and output can be
 switched independently.
 
+CONFIG_NETCONSOLE_PERSIST_ETH - Don't reinitialize the Ethernet driver
+		between consecutive calls to netconsole.
+
 We use an environment variable 'ncip' to set the IP address and the
 port of the destination. The format is <ip_addr>:<port>. If <port> is
 omitted, the value of 6666 is used. If the env var doesn't exist, the
diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
index 14243b8..0218302 100644
--- a/drivers/net/netconsole.c
+++ b/drivers/net/netconsole.c
@@ -131,8 +131,17 @@ static void nc_send_packet(const char *buf, int len)
 	}
 
 	if (eth->state != ETH_STATE_ACTIVE) {
-		if (eth_init(gd->bd) < 0)
-			return;
+#ifdef CONFIG_NETCONSOLE_PERSIST_ETH
+		if (net_loop_last_protocol != NETCONS) {
+#endif
+			if (eth_init(gd->bd) < 0)
+				return;
+#ifdef CONFIG_NETCONSOLE_PERSIST_ETH
+			net_loop_last_protocol = NETCONS;
+		} else {
+			eth_init_state_only(gd->bd);
+		}
+#endif
 		inited = 1;
 	}
 	pkt = (uchar *)NetTxPacket + NetEthHdrSize() + IP_UDP_HDR_SIZE;
@@ -141,8 +150,17 @@ static void nc_send_packet(const char *buf, int len)
 	ip = nc_ip;
 	NetSendUDPPacket(ether, ip, nc_port, nc_port, len);
 
-	if (inited)
-		eth_halt();
+	if (inited) {
+#ifdef CONFIG_NETCONSOLE_PERSIST_ETH
+		if (net_loop_last_protocol != NETCONS) {
+#endif
+			eth_halt();
+#ifdef CONFIG_NETCONSOLE_PERSIST_ETH
+		} else {
+			eth_halt_state_only();
+		}
+#endif
+	}
 }
 
 static int nc_start(void)
diff --git a/include/net.h b/include/net.h
index 6d2d6cd..0b75e10 100644
--- a/include/net.h
+++ b/include/net.h
@@ -151,6 +151,12 @@ extern int eth_rx(void);			/* Check for received packets */
 extern void eth_halt(void);			/* stop SCC */
 extern char *eth_get_name(void);		/* get name of current device */
 
+#ifdef CONFIG_NETCONSOLE_PERSIST_ETH
+extern int eth_init_state_only(bd_t *bis);		/* Set active state */
+extern void eth_halt_state_only(void);		/* Set passive state */
+extern enum proto_t net_loop_last_protocol;
+#endif
+
 /*
  * Set the hardware address for an ethernet interface based on 'eth%daddr'
  * environment variable (or just 'ethaddr' if eth_number is 0).
diff --git a/net/eth.c b/net/eth.c
index d526264..54841b9 100644
--- a/net/eth.c
+++ b/net/eth.c
@@ -436,6 +436,20 @@ void eth_halt(void)
 	eth_current->state = ETH_STATE_PASSIVE;
 }
 
+#ifdef CONFIG_NETCONSOLE_PERSIST_ETH
+int eth_init_state_only(bd_t *bis)
+{
+	eth_current->state = ETH_STATE_ACTIVE;
+
+	return 0;
+}
+
+void eth_halt_state_only(void)
+{
+	eth_current->state = ETH_STATE_PASSIVE;
+}
+#endif
+
 int eth_send(void *packet, int length)
 {
 	if (!eth_current)
diff --git a/net/net.c b/net/net.c
index 9de7d92..184c182 100644
--- a/net/net.c
+++ b/net/net.c
@@ -180,6 +180,14 @@ IPaddr_t	NetNtpServerIP;
 int		NetTimeOffset;
 #endif
 
+#ifdef CONFIG_NETCONSOLE_PERSIST_ETH
+/*
+ * Start with a default last protocol.
+ * We are only interested in NETCONS or not.
+ */
+enum proto_t net_loop_last_protocol = BOOTP;
+#endif
+
 uchar PktBuf[(PKTBUFSRX+1) * PKTSIZE_ALIGN + PKTALIGN];
 
 /* Receive packet */
@@ -314,12 +322,20 @@ int NetLoop(enum proto_t protocol)
 
 	bootstage_mark_name(BOOTSTAGE_ID_ETH_START, "eth_start");
 	net_init();
-	eth_halt();
-	eth_set_current();
-	if (eth_init(bd) < 0) {
+#ifdef CONFIG_NETCONSOLE_PERSIST_ETH
+	if (protocol != NETCONS || net_loop_last_protocol != NETCONS) {
+#endif
 		eth_halt();
-		return -1;
+		eth_set_current();
+		if (eth_init(bd) < 0) {
+			eth_halt();
+			return -1;
+		}
+#ifdef CONFIG_NETCONSOLE_PERSIST_ETH
+	} else {
+		eth_init_state_only(bd);
 	}
+#endif
 
 restart:
 	memcpy(NetOurEther, eth_get_dev()->enetaddr, 6);
@@ -461,6 +477,10 @@ restart:
 
 			net_cleanup_loop();
 			eth_halt();
+#ifdef CONFIG_NETCONSOLE_PERSIST_ETH
+			/* Invalidate the last protocol */
+			net_loop_last_protocol = BOOTP;
+#endif
 			puts("\nAbort\n");
 			/* include a debug print as well incase the debug
 			   messages are directed to stderr */
@@ -518,13 +538,27 @@ restart:
 				sprintf(buf, "%lX", (unsigned long)load_addr);
 				setenv("fileaddr", buf);
 			}
-			eth_halt();
+#ifdef CONFIG_NETCONSOLE_PERSIST_ETH
+			if (protocol != NETCONS) {
+#endif
+				eth_halt();
+#ifdef CONFIG_NETCONSOLE_PERSIST_ETH
+			} else {
+				eth_halt_state_only();
+			}
+			/* Remember what the last protocol was */
+			net_loop_last_protocol = protocol;
+#endif
 			ret = NetBootFileXferSize;
 			debug_cond(DEBUG_INT_STATE, "--- NetLoop Success!\n");
 			goto done;
 
 		case NETLOOP_FAIL:
 			net_cleanup_loop();
+#ifdef CONFIG_NETCONSOLE_PERSIST_ETH
+			/* Invalidate the last protocol */
+			net_loop_last_protocol = BOOTP;
+#endif
 			debug_cond(DEBUG_INT_STATE, "--- NetLoop Fail!\n");
 			goto done;
 
-- 
1.6.0.2

[U-Boot] [PATCH] net: Improve the speed of netconsole

[Stefano Babic]

On 24/07/2012 22:11, Joe Hershberger wrote:
> Previously u-boot would initialize the network interface for every
> network operation and then shut it down again.  This makes sense for
> most operations where the network in not known to be needed soon after
> the operation is complete.  In the case of netconsole, it will use the
> network for every interaction with the shell or every printf.  This
> means that the network is being reinitialized very often.  On many
> devices, this intialization is very slow.

Hi Joe,

agree with you - and in some cases, such as ethernet on USB, netconsole
is not yet useful.

Which is the status of the ethernet controller before starting the
kernel ? Up now we guarantee that the hardware is turned off before
kernel takes over. This patch changes this behavior, and I know from the
past (really with USB, we have a usb_stop before calling the kernel)
that this can be dangerous.

Maybe you can add a eth_halt() call into do_bootm().

Second general question is if the behavior of
CONFIG_NETCONSOLE_PERSIST_ETH should be always turned on when Netconsole
is activated. I mean, your patch maintain a high compatibility with the
past, but I suppose that everyone using netconsole likes the new
behavior that you are introducing and everyone will turn
CONFIG_NETCONSOLE_PERSIST_ETH on. Then we can simply use
CONFIG_NETCONSOLE as switch.

Best regards,
Stefano

-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================

[U-Boot] [PATCH] net: Improve the speed of netconsole

[Mike Frysinger]

On Tuesday 24 July 2012 16:11:15 Joe Hershberger wrote:
> --- a/drivers/net/netconsole.c
> +++ b/drivers/net/netconsole.c
> @@ -131,8 +131,17 @@ static void nc_send_packet(const char *buf, int len)
>  	}
> 
>  	if (eth->state != ETH_STATE_ACTIVE) {
> -		if (eth_init(gd->bd) < 0)
> -			return;
> +#ifdef CONFIG_NETCONSOLE_PERSIST_ETH
> +		if (net_loop_last_protocol != NETCONS) {
> +#endif
> +			if (eth_init(gd->bd) < 0)
> +				return;
> +#ifdef CONFIG_NETCONSOLE_PERSIST_ETH
> +			net_loop_last_protocol = NETCONS;
> +		} else {
> +			eth_init_state_only(gd->bd);
> +		}
> +#endif

seems to me these pre/post clauses should really get refactored someway.  the 
ifdef noise is significant here.

so in the header, something like:

static inline int eth_reinit_protocol(int protocol)
{
#ifdef CONFIG_NETCONSOLE_PERSIST_ETH
	return protocol != NETCONS;
#else
	return 1;
#endif
}
static inline void eth_set_last_protocol(int protocol)
{
#ifdef CONFIG_NETCONSOLE_PERSIST_ETH
	net_loop_last_protocol = protocol;
#endif
}
#ifdef CONFIG_NETCONSOLE_PERSIST_ETH
extern enum proto_t net_loop_last_protocol;
#else
# define net_loop_last_protocol -1
#endif

then the source becomes the more manageable:

if (eth_reinit_protocol(net_loop_last_protocol)) {
	if (eth_init(gd->bd) < 0)
		return;
	eth_set_last_protocol(NETCONS);
} else
	eth_init_state_only(gd->bd);

> --- a/net/eth.c
> +++ b/net/eth.c
> 
> +#ifdef CONFIG_NETCONSOLE_PERSIST_ETH
> +int eth_init_state_only(bd_t *bis)
> +{
> +	eth_current->state = ETH_STATE_ACTIVE;
> +
> +	return 0;
> +}
> +
> +void eth_halt_state_only(void)
> +{
> +	eth_current->state = ETH_STATE_PASSIVE;
> +}
> +#endif

these *really* should be static inlines in the global header.  they're so dirt 
simple, the overhead of the function call is probably much higher than the 
single memory store.
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20120725/a3cff26c/attachment.pgp>

[U-Boot] [PATCH] net: Improve the speed of netconsole

[Joe Hershberger]

Hi Mike,

On Wed, Jul 25, 2012 at 1:49 PM, Mike Frysinger <vapier@gentoo.org> wrote:
> On Tuesday 24 July 2012 16:11:15 Joe Hershberger wrote:
>> --- a/drivers/net/netconsole.c
>> +++ b/drivers/net/netconsole.c
>> @@ -131,8 +131,17 @@ static void nc_send_packet(const char *buf, int len)
>>       }
>>
>>       if (eth->state != ETH_STATE_ACTIVE) {
>> -             if (eth_init(gd->bd) < 0)
>> -                     return;
>> +#ifdef CONFIG_NETCONSOLE_PERSIST_ETH
>> +             if (net_loop_last_protocol != NETCONS) {
>> +#endif
>> +                     if (eth_init(gd->bd) < 0)
>> +                             return;
>> +#ifdef CONFIG_NETCONSOLE_PERSIST_ETH
>> +                     net_loop_last_protocol = NETCONS;
>> +             } else {
>> +                     eth_init_state_only(gd->bd);
>> +             }
>> +#endif
>
> seems to me these pre/post clauses should really get refactored someway.  the
> ifdef noise is significant here.

I agree.

> so in the header, something like:
>
> static inline int eth_reinit_protocol(int protocol)
> {
> #ifdef CONFIG_NETCONSOLE_PERSIST_ETH
>         return protocol != NETCONS;
> #else
>         return 1;
> #endif
> }
> static inline void eth_set_last_protocol(int protocol)
> {
> #ifdef CONFIG_NETCONSOLE_PERSIST_ETH
>         net_loop_last_protocol = protocol;
> #endif
> }
> #ifdef CONFIG_NETCONSOLE_PERSIST_ETH
> extern enum proto_t net_loop_last_protocol;
> #else
> # define net_loop_last_protocol -1
> #endif
>
> then the source becomes the more manageable:
>
> if (eth_reinit_protocol(net_loop_last_protocol)) {
>         if (eth_init(gd->bd) < 0)
>                 return;
>         eth_set_last_protocol(NETCONS);
> } else
>         eth_init_state_only(gd->bd);

Much nicer.  With static inline it should compile to the same thing.

>> --- a/net/eth.c
>> +++ b/net/eth.c
>>
>> +#ifdef CONFIG_NETCONSOLE_PERSIST_ETH
>> +int eth_init_state_only(bd_t *bis)
>> +{
>> +     eth_current->state = ETH_STATE_ACTIVE;
>> +
>> +     return 0;
>> +}
>> +
>> +void eth_halt_state_only(void)
>> +{
>> +     eth_current->state = ETH_STATE_PASSIVE;
>> +}
>> +#endif
>
> these *really* should be static inlines in the global header.  they're so dirt
> simple, the overhead of the function call is probably much higher than the
> single memory store.

I can do that, but I don't think it will save anything.  Since
eth_current is static, I would have to change it to eth_get_dev(), and
we're back to a function call.  Thoughts?

Thanks,
-Joe

[U-Boot] [PATCH] net: Improve the speed of netconsole

[Mike Frysinger]

On Monday 30 July 2012 17:08:41 Joe Hershberger wrote:
> On Wed, Jul 25, 2012 at 1:49 PM, Mike Frysinger wrote:
> > On Tuesday 24 July 2012 16:11:15 Joe Hershberger wrote:
> >> --- a/net/eth.c
> >> +++ b/net/eth.c
> >> 
> >> +#ifdef CONFIG_NETCONSOLE_PERSIST_ETH
> >> +int eth_init_state_only(bd_t *bis)
> >> +{
> >> +     eth_current->state = ETH_STATE_ACTIVE;
> >> +
> >> +     return 0;
> >> +}
> >> +
> >> +void eth_halt_state_only(void)
> >> +{
> >> +     eth_current->state = ETH_STATE_PASSIVE;
> >> +}
> >> +#endif
> > 
> > these *really* should be static inlines in the global header.  they're so
> > dirt simple, the overhead of the function call is probably much higher
> > than the single memory store.
> 
> I can do that, but I don't think it will save anything.  Since
> eth_current is static, I would have to change it to eth_get_dev(), and
> we're back to a function call.  Thoughts?

i wonder why eth_get_dev is an external func then :)
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20120801/6be7c85a/attachment.pgp>

[U-Boot] [PATCH] net: Improve the speed of netconsole

[Joe Hershberger]

Hi Mike,

On Wed, Aug 1, 2012 at 11:35 AM, Mike Frysinger <vapier@gentoo.org> wrote:
> On Monday 30 July 2012 17:08:41 Joe Hershberger wrote:
>> On Wed, Jul 25, 2012 at 1:49 PM, Mike Frysinger wrote:
>> > On Tuesday 24 July 2012 16:11:15 Joe Hershberger wrote:
>> >> --- a/net/eth.c
>> >> +++ b/net/eth.c
>> >>
>> >> +#ifdef CONFIG_NETCONSOLE_PERSIST_ETH
>> >> +int eth_init_state_only(bd_t *bis)
>> >> +{
>> >> +     eth_current->state = ETH_STATE_ACTIVE;
>> >> +
>> >> +     return 0;
>> >> +}
>> >> +
>> >> +void eth_halt_state_only(void)
>> >> +{
>> >> +     eth_current->state = ETH_STATE_PASSIVE;
>> >> +}
>> >> +#endif
>> >
>> > these *really* should be static inlines in the global header.  they're so
>> > dirt simple, the overhead of the function call is probably much higher
>> > than the single memory store.
>>
>> I can do that, but I don't think it will save anything.  Since
>> eth_current is static, I would have to change it to eth_get_dev(), and
>> we're back to a function call.  Thoughts?
>
> i wonder why eth_get_dev is an external func then :)

An attempt at encapsulation I assume.  I guess I could just make it global.

-Joe

[U-Boot] [PATCH v2] net: Improve the speed of netconsole

[Joe Hershberger]

Previously u-boot would initialize the network interface for every
network operation and then shut it down again.  This makes sense for
most operations where the network in not known to be needed soon after
the operation is complete.  In the case of netconsole, it will use the
network for every interaction with the shell or every printf.  This
means that the network is being reinitialized very often.  On many
devices, this intialization is very slow.

This patch checks for consecutive netconsole actions and leaves the
ethernet hardware initialized between them.  It will still behave the
same old way for all other network operations and any time another
network operation happens between netconsole operations.

Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
Cc: Stefano Babic <sbabic@denx.de>
---
 common/cmd_bootm.c       |   17 +++++++++++++++++
 drivers/net/netconsole.c |   22 ++++++++++++++++++----
 include/net.h            |   42 +++++++++++++++++++++++++++++++++++++++++-
 net/eth.c                |    8 ++------
 net/net.c                |   26 ++++++++++++++++++++------
 5 files changed, 98 insertions(+), 17 deletions(-)

diff --git a/common/cmd_bootm.c b/common/cmd_bootm.c
index 45e726a..83fa5d7 100644
--- a/common/cmd_bootm.c
+++ b/common/cmd_bootm.c
@@ -564,6 +564,13 @@ int do_bootm_subcommand(cmd_tbl_t *cmdtp, int flag, int argc,
 			break;
 		case BOOTM_STATE_OS_GO:
 			disable_interrupts();
+#ifdef CONFIG_NETCONSOLE
+			/*
+			 * Stop the ethernet stack if NetConsole could have
+			 * left it up
+			 */
+			eth_halt();
+#endif
 			arch_preboot_os();
 			boot_fn(BOOTM_STATE_OS_GO, argc, argv, &images);
 			break;
@@ -622,6 +629,11 @@ int do_bootm(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 	 */
 	iflag = disable_interrupts();
 
+#ifdef CONFIG_NETCONSOLE
+	/* Stop the ethernet stack if NetConsole could have left it up */
+	eth_halt();
+#endif
+
 #if defined(CONFIG_CMD_USB)
 	/*
 	 * turn off USB to prevent the host controller from writing to the
@@ -1599,6 +1611,11 @@ static int do_bootz(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 	 */
 	disable_interrupts();
 
+#ifdef CONFIG_NETCONSOLE
+	/* Stop the ethernet stack if NetConsole could have left it up */
+	eth_halt();
+#endif
+
 #if defined(CONFIG_CMD_USB)
 	/*
 	 * turn off USB to prevent the host controller from writing to the
diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
index 14243b8..069ad87 100644
--- a/drivers/net/netconsole.c
+++ b/drivers/net/netconsole.c
@@ -39,6 +39,11 @@ static IPaddr_t nc_ip; /* server ip */
 static short nc_port; /* source/target port */
 static const char *output_packet; /* used by first send udp */
 static int output_packet_len;
+/*
+ * Start with a default last protocol.
+ * We are only interested in NETCONS or not.
+ */
+enum proto_t net_loop_last_protocol = BOOTP;
 
 static void nc_wait_arp_handler(uchar *pkt, unsigned dest,
 				 IPaddr_t sip, unsigned src,
@@ -131,8 +136,13 @@ static void nc_send_packet(const char *buf, int len)
 	}
 
 	if (eth->state != ETH_STATE_ACTIVE) {
-		if (eth_init(gd->bd) < 0)
-			return;
+		if (eth_is_on_demand_init()) {
+			if (eth_init(gd->bd) < 0)
+				return;
+			eth_set_last_protocol(NETCONS);
+		} else
+			eth_init_state_only(gd->bd);
+
 		inited = 1;
 	}
 	pkt = (uchar *)NetTxPacket + NetEthHdrSize() + IP_UDP_HDR_SIZE;
@@ -141,8 +151,12 @@ static void nc_send_packet(const char *buf, int len)
 	ip = nc_ip;
 	NetSendUDPPacket(ether, ip, nc_port, nc_port, len);
 
-	if (inited)
-		eth_halt();
+	if (inited) {
+		if (eth_is_on_demand_init())
+			eth_halt();
+		else
+			eth_halt_state_only();
+	}
 }
 
 static int nc_start(void)
diff --git a/include/net.h b/include/net.h
index 6d2d6cd..e193b7b 100644
--- a/include/net.h
+++ b/include/net.h
@@ -102,7 +102,14 @@ extern int eth_register(struct eth_device* dev);/* Register network device */
 extern int eth_unregister(struct eth_device *dev);/* Remove network device */
 extern void eth_try_another(int first_restart);	/* Change the device */
 extern void eth_set_current(void);		/* set nterface to ethcur var */
-extern struct eth_device *eth_get_dev(void);	/* get the current device MAC */
+/* get the current device MAC */
+static inline __attribute__((always_inline))
+struct eth_device *eth_get_dev(void)
+{
+	extern struct eth_device *eth_current;
+
+	return eth_current;
+}
 extern struct eth_device *eth_get_dev_by_name(const char *devname);
 extern struct eth_device *eth_get_dev_by_index(int index); /* get dev @ index */
 extern int eth_get_dev_index(void);		/* get the device index */
@@ -151,6 +158,19 @@ extern int eth_rx(void);			/* Check for received packets */
 extern void eth_halt(void);			/* stop SCC */
 extern char *eth_get_name(void);		/* get name of current device */
 
+/* Set active state */
+static inline __attribute__((always_inline)) int eth_init_state_only(bd_t *bis)
+{
+	eth_get_dev()->state = ETH_STATE_ACTIVE;
+
+	return 0;
+}
+/* Set passive state */
+static inline __attribute__((always_inline)) void eth_halt_state_only(void)
+{
+	eth_get_dev()->state = ETH_STATE_PASSIVE;
+}
+
 /*
  * Set the hardware address for an ethernet interface based on 'eth%daddr'
  * environment variable (or just 'ethaddr' if eth_number is 0).
@@ -532,6 +552,26 @@ void NcStart(void);
 int nc_input_packet(uchar *pkt, unsigned dest, unsigned src, unsigned len);
 #endif
 
+static inline __attribute__((always_inline)) int eth_is_on_demand_init(void)
+{
+#ifdef CONFIG_NETCONSOLE
+	extern enum proto_t net_loop_last_protocol;
+
+	return net_loop_last_protocol != NETCONS;
+#else
+	return 1;
+#endif
+}
+
+static inline void eth_set_last_protocol(int protocol)
+{
+#ifdef CONFIG_NETCONSOLE
+	extern enum proto_t net_loop_last_protocol;
+
+	net_loop_last_protocol = protocol;
+#endif
+}
+
 /*
  * Check if autoload is enabled. If so, use either NFS or TFTP to download
  * the boot file.
diff --git a/net/eth.c b/net/eth.c
index 1a11ce1..66295b6 100644
--- a/net/eth.c
+++ b/net/eth.c
@@ -121,12 +121,8 @@ static struct {
 static unsigned int eth_rcv_current, eth_rcv_last;
 #endif
 
-static struct eth_device *eth_devices, *eth_current;
-
-struct eth_device *eth_get_dev(void)
-{
-	return eth_current;
-}
+static struct eth_device *eth_devices;
+struct eth_device *eth_current;
 
 struct eth_device *eth_get_dev_by_name(const char *devname)
 {
diff --git a/net/net.c b/net/net.c
index e8ff066..f002cda 100644
--- a/net/net.c
+++ b/net/net.c
@@ -315,12 +315,15 @@ int NetLoop(enum proto_t protocol)
 
 	bootstage_mark_name(BOOTSTAGE_ID_ETH_START, "eth_start");
 	net_init();
-	eth_halt();
-	eth_set_current();
-	if (eth_init(bd) < 0) {
+	if (eth_is_on_demand_init() || protocol != NETCONS) {
 		eth_halt();
-		return -1;
-	}
+		eth_set_current();
+		if (eth_init(bd) < 0) {
+			eth_halt();
+			return -1;
+		}
+	} else
+		eth_init_state_only(bd);
 
 restart:
 	net_set_state(NETLOOP_CONTINUE);
@@ -460,6 +463,9 @@ restart:
 
 			net_cleanup_loop();
 			eth_halt();
+			/* Invalidate the last protocol */
+			eth_set_last_protocol(BOOTP);
+
 			puts("\nAbort\n");
 			/* include a debug print as well incase the debug
 			   messages are directed to stderr */
@@ -517,13 +523,21 @@ restart:
 				sprintf(buf, "%lX", (unsigned long)load_addr);
 				setenv("fileaddr", buf);
 			}
-			eth_halt();
+			if (protocol != NETCONS)
+				eth_halt();
+			else
+				eth_halt_state_only();
+
+			eth_set_last_protocol(protocol);
+
 			ret = NetBootFileXferSize;
 			debug_cond(DEBUG_INT_STATE, "--- NetLoop Success!\n");
 			goto done;
 
 		case NETLOOP_FAIL:
 			net_cleanup_loop();
+			/* Invalidate the last protocol */
+			eth_set_last_protocol(BOOTP);
 			debug_cond(DEBUG_INT_STATE, "--- NetLoop Fail!\n");
 			goto done;
 
-- 
1.6.0.2

[U-Boot] [PATCH v2] net: Improve the speed of netconsole

[Stefano Babic]

On 03/08/2012 22:59, Joe Hershberger wrote:
> Previously u-boot would initialize the network interface for every
> network operation and then shut it down again.  This makes sense for
> most operations where the network in not known to be needed soon after
> the operation is complete.  In the case of netconsole, it will use the
> network for every interaction with the shell or every printf.  This
> means that the network is being reinitialized very often.  On many
> devices, this intialization is very slow.
> 
> This patch checks for consecutive netconsole actions and leaves the
> ethernet hardware initialized between them.  It will still behave the
> same old way for all other network operations and any time another
> network operation happens between netconsole operations.
> 

Hi Joe,

> Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
> Cc: Stefano Babic <sbabic@denx.de>
> ---
>  common/cmd_bootm.c       |   17 +++++++++++++++++
>  drivers/net/netconsole.c |   22 ++++++++++++++++++----
>  include/net.h            |   42 +++++++++++++++++++++++++++++++++++++++++-
>  net/eth.c                |    8 ++------
>  net/net.c                |   26 ++++++++++++++++++++------
>  5 files changed, 98 insertions(+), 17 deletions(-)
> 
> diff --git a/common/cmd_bootm.c b/common/cmd_bootm.c
> index 45e726a..83fa5d7 100644
> --- a/common/cmd_bootm.c
> +++ b/common/cmd_bootm.c
> @@ -564,6 +564,13 @@ int do_bootm_subcommand(cmd_tbl_t *cmdtp, int flag, int argc,
>  			break;
>  		case BOOTM_STATE_OS_GO:
>  			disable_interrupts();
> +#ifdef CONFIG_NETCONSOLE
> +			/*
> +			 * Stop the ethernet stack if NetConsole could have
> +			 * left it up
> +			 */
> +			eth_halt();
> +#endif
>  			arch_preboot_os();
>  			boot_fn(BOOTM_STATE_OS_GO, argc, argv, &images);
>  			break;
> @@ -622,6 +629,11 @@ int do_bootm(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>  	 */
>  	iflag = disable_interrupts();
>  
> +#ifdef CONFIG_NETCONSOLE
> +	/* Stop the ethernet stack if NetConsole could have left it up */
> +	eth_halt();
> +#endif
> +
>  #if defined(CONFIG_CMD_USB)
>  	/*
>  	 * turn off USB to prevent the host controller from writing to the
> @@ -1599,6 +1611,11 @@ static int do_bootz(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>  	 */
>  	disable_interrupts();
>  
> +#ifdef CONFIG_NETCONSOLE
> +	/* Stop the ethernet stack if NetConsole could have left it up */
> +	eth_halt();
> +#endif
> +
>  #if defined(CONFIG_CMD_USB)
>  	/*
>  	 * turn off USB to prevent the host controller from writing to the
> diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
> index 14243b8..069ad87 100644
> --- a/drivers/net/netconsole.c
> +++ b/drivers/net/netconsole.c
> @@ -39,6 +39,11 @@ static IPaddr_t nc_ip; /* server ip */
>  static short nc_port; /* source/target port */
>  static const char *output_packet; /* used by first send udp */
>  static int output_packet_len;
> +/*
> + * Start with a default last protocol.
> + * We are only interested in NETCONS or not.
> + */
> +enum proto_t net_loop_last_protocol = BOOTP;
>  
>  static void nc_wait_arp_handler(uchar *pkt, unsigned dest,
>  				 IPaddr_t sip, unsigned src,
> @@ -131,8 +136,13 @@ static void nc_send_packet(const char *buf, int len)
>  	}
>  
>  	if (eth->state != ETH_STATE_ACTIVE) {
> -		if (eth_init(gd->bd) < 0)
> -			return;
> +		if (eth_is_on_demand_init()) {
> +			if (eth_init(gd->bd) < 0)
> +				return;
> +			eth_set_last_protocol(NETCONS);
> +		} else
> +			eth_init_state_only(gd->bd);
> +
>  		inited = 1;
>  	}
>  	pkt = (uchar *)NetTxPacket + NetEthHdrSize() + IP_UDP_HDR_SIZE;
> @@ -141,8 +151,12 @@ static void nc_send_packet(const char *buf, int len)
>  	ip = nc_ip;
>  	NetSendUDPPacket(ether, ip, nc_port, nc_port, len);
>  
> -	if (inited)
> -		eth_halt();
> +	if (inited) {
> +		if (eth_is_on_demand_init())
> +			eth_halt();
> +		else
> +			eth_halt_state_only();
> +	}
>  }
>  
>  static int nc_start(void)
> diff --git a/include/net.h b/include/net.h
> index 6d2d6cd..e193b7b 100644
> --- a/include/net.h
> +++ b/include/net.h
> @@ -102,7 +102,14 @@ extern int eth_register(struct eth_device* dev);/* Register network device */
>  extern int eth_unregister(struct eth_device *dev);/* Remove network device */
>  extern void eth_try_another(int first_restart);	/* Change the device */
>  extern void eth_set_current(void);		/* set nterface to ethcur var */
> -extern struct eth_device *eth_get_dev(void);	/* get the current device MAC */
> +/* get the current device MAC */
> +static inline __attribute__((always_inline))
> +struct eth_device *eth_get_dev(void)
> +{
> +	extern struct eth_device *eth_current;
> +
> +	return eth_current;
> +}
>  extern struct eth_device *eth_get_dev_by_name(const char *devname);
>  extern struct eth_device *eth_get_dev_by_index(int index); /* get dev @ index */
>  extern int eth_get_dev_index(void);		/* get the device index */
> @@ -151,6 +158,19 @@ extern int eth_rx(void);			/* Check for received packets */
>  extern void eth_halt(void);			/* stop SCC */
>  extern char *eth_get_name(void);		/* get name of current device */
>  
> +/* Set active state */
> +static inline __attribute__((always_inline)) int eth_init_state_only(bd_t *bis)
> +{
> +	eth_get_dev()->state = ETH_STATE_ACTIVE;
> +
> +	return 0;
> +}
> +/* Set passive state */
> +static inline __attribute__((always_inline)) void eth_halt_state_only(void)
> +{
> +	eth_get_dev()->state = ETH_STATE_PASSIVE;
> +}
> +
>  /*
>   * Set the hardware address for an ethernet interface based on 'eth%daddr'
>   * environment variable (or just 'ethaddr' if eth_number is 0).
> @@ -532,6 +552,26 @@ void NcStart(void);
>  int nc_input_packet(uchar *pkt, unsigned dest, unsigned src, unsigned len);
>  #endif
>  
> +static inline __attribute__((always_inline)) int eth_is_on_demand_init(void)
> +{
> +#ifdef CONFIG_NETCONSOLE
> +	extern enum proto_t net_loop_last_protocol;
> +
> +	return net_loop_last_protocol != NETCONS;
> +#else
> +	return 1;
> +#endif
> +}
> +
> +static inline void eth_set_last_protocol(int protocol)
> +{
> +#ifdef CONFIG_NETCONSOLE
> +	extern enum proto_t net_loop_last_protocol;
> +
> +	net_loop_last_protocol = protocol;
> +#endif
> +}
> +
>  /*
>   * Check if autoload is enabled. If so, use either NFS or TFTP to download
>   * the boot file.
> diff --git a/net/eth.c b/net/eth.c
> index 1a11ce1..66295b6 100644
> --- a/net/eth.c
> +++ b/net/eth.c
> @@ -121,12 +121,8 @@ static struct {
>  static unsigned int eth_rcv_current, eth_rcv_last;
>  #endif
>  
> -static struct eth_device *eth_devices, *eth_current;
> -
> -struct eth_device *eth_get_dev(void)
> -{
> -	return eth_current;
> -}
> +static struct eth_device *eth_devices;
> +struct eth_device *eth_current;
>  
>  struct eth_device *eth_get_dev_by_name(const char *devname)
>  {
> diff --git a/net/net.c b/net/net.c
> index e8ff066..f002cda 100644
> --- a/net/net.c
> +++ b/net/net.c
> @@ -315,12 +315,15 @@ int NetLoop(enum proto_t protocol)
>  
>  	bootstage_mark_name(BOOTSTAGE_ID_ETH_START, "eth_start");
>  	net_init();
> -	eth_halt();
> -	eth_set_current();
> -	if (eth_init(bd) < 0) {
> +	if (eth_is_on_demand_init() || protocol != NETCONS) {
>  		eth_halt();
> -		return -1;
> -	}
> +		eth_set_current();
> +		if (eth_init(bd) < 0) {
> +			eth_halt();
> +			return -1;
> +		}
> +	} else
> +		eth_init_state_only(bd);
>  
>  restart:
>  	net_set_state(NETLOOP_CONTINUE);
> @@ -460,6 +463,9 @@ restart:
>  
>  			net_cleanup_loop();
>  			eth_halt();
> +			/* Invalidate the last protocol */
> +			eth_set_last_protocol(BOOTP);
> +
>  			puts("\nAbort\n");
>  			/* include a debug print as well incase the debug
>  			   messages are directed to stderr */
> @@ -517,13 +523,21 @@ restart:
>  				sprintf(buf, "%lX", (unsigned long)load_addr);
>  				setenv("fileaddr", buf);
>  			}
> -			eth_halt();
> +			if (protocol != NETCONS)
> +				eth_halt();
> +			else
> +				eth_halt_state_only();
> +
> +			eth_set_last_protocol(protocol);
> +
>  			ret = NetBootFileXferSize;
>  			debug_cond(DEBUG_INT_STATE, "--- NetLoop Success!\n");
>  			goto done;
>  
>  		case NETLOOP_FAIL:
>  			net_cleanup_loop();
> +			/* Invalidate the last protocol */
> +			eth_set_last_protocol(BOOTP);
>  			debug_cond(DEBUG_INT_STATE, "--- NetLoop Fail!\n");
>  			goto done;
>  
> 

Acked-by: Stefano Babic <sbabic@denx.de>

Best regards,
Stefano


-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================

[U-Boot] [PATCH v2] net: Improve the speed of netconsole

[Joe Hershberger]

Hi Mike,

On Fri, Aug 3, 2012 at 3:59 PM, Joe Hershberger <joe.hershberger@ni.com> wrote:
> Previously u-boot would initialize the network interface for every
> network operation and then shut it down again.  This makes sense for
> most operations where the network in not known to be needed soon after
> the operation is complete.  In the case of netconsole, it will use the
> network for every interaction with the shell or every printf.  This
> means that the network is being reinitialized very often.  On many
> devices, this intialization is very slow.
>
> This patch checks for consecutive netconsole actions and leaves the
> ethernet hardware initialized between them.  It will still behave the
> same old way for all other network operations and any time another
> network operation happens between netconsole operations.
>
> Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
> Cc: Stefano Babic <sbabic@denx.de>

Sorry I forgot to Cc you on the update... I also forgot the revision notes...

Changes since v1:
 - Halt Ethernet stack before booting Linux
 - Clean up CPP guard noise
 - Reduce overhead of no-re-init case

> ---
>  common/cmd_bootm.c       |   17 +++++++++++++++++
>  drivers/net/netconsole.c |   22 ++++++++++++++++++----
>  include/net.h            |   42 +++++++++++++++++++++++++++++++++++++++++-
>  net/eth.c                |    8 ++------
>  net/net.c                |   26 ++++++++++++++++++++------
>  5 files changed, 98 insertions(+), 17 deletions(-)
>
> diff --git a/common/cmd_bootm.c b/common/cmd_bootm.c
> index 45e726a..83fa5d7 100644
> --- a/common/cmd_bootm.c
> +++ b/common/cmd_bootm.c
> @@ -564,6 +564,13 @@ int do_bootm_subcommand(cmd_tbl_t *cmdtp, int flag, int argc,
>                         break;
>                 case BOOTM_STATE_OS_GO:
>                         disable_interrupts();
> +#ifdef CONFIG_NETCONSOLE
> +                       /*
> +                        * Stop the ethernet stack if NetConsole could have
> +                        * left it up
> +                        */
> +                       eth_halt();
> +#endif
>                         arch_preboot_os();
>                         boot_fn(BOOTM_STATE_OS_GO, argc, argv, &images);
>                         break;
> @@ -622,6 +629,11 @@ int do_bootm(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>          */
>         iflag = disable_interrupts();
>
> +#ifdef CONFIG_NETCONSOLE
> +       /* Stop the ethernet stack if NetConsole could have left it up */
> +       eth_halt();
> +#endif
> +
>  #if defined(CONFIG_CMD_USB)
>         /*
>          * turn off USB to prevent the host controller from writing to the
> @@ -1599,6 +1611,11 @@ static int do_bootz(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>          */
>         disable_interrupts();
>
> +#ifdef CONFIG_NETCONSOLE
> +       /* Stop the ethernet stack if NetConsole could have left it up */
> +       eth_halt();
> +#endif
> +
>  #if defined(CONFIG_CMD_USB)
>         /*
>          * turn off USB to prevent the host controller from writing to the
> diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
> index 14243b8..069ad87 100644
> --- a/drivers/net/netconsole.c
> +++ b/drivers/net/netconsole.c
> @@ -39,6 +39,11 @@ static IPaddr_t nc_ip; /* server ip */
>  static short nc_port; /* source/target port */
>  static const char *output_packet; /* used by first send udp */
>  static int output_packet_len;
> +/*
> + * Start with a default last protocol.
> + * We are only interested in NETCONS or not.
> + */
> +enum proto_t net_loop_last_protocol = BOOTP;
>
>  static void nc_wait_arp_handler(uchar *pkt, unsigned dest,
>                                  IPaddr_t sip, unsigned src,
> @@ -131,8 +136,13 @@ static void nc_send_packet(const char *buf, int len)
>         }
>
>         if (eth->state != ETH_STATE_ACTIVE) {
> -               if (eth_init(gd->bd) < 0)
> -                       return;
> +               if (eth_is_on_demand_init()) {
> +                       if (eth_init(gd->bd) < 0)
> +                               return;
> +                       eth_set_last_protocol(NETCONS);
> +               } else
> +                       eth_init_state_only(gd->bd);
> +
>                 inited = 1;
>         }
>         pkt = (uchar *)NetTxPacket + NetEthHdrSize() + IP_UDP_HDR_SIZE;
> @@ -141,8 +151,12 @@ static void nc_send_packet(const char *buf, int len)
>         ip = nc_ip;
>         NetSendUDPPacket(ether, ip, nc_port, nc_port, len);
>
> -       if (inited)
> -               eth_halt();
> +       if (inited) {
> +               if (eth_is_on_demand_init())
> +                       eth_halt();
> +               else
> +                       eth_halt_state_only();
> +       }
>  }
>
>  static int nc_start(void)
> diff --git a/include/net.h b/include/net.h
> index 6d2d6cd..e193b7b 100644
> --- a/include/net.h
> +++ b/include/net.h
> @@ -102,7 +102,14 @@ extern int eth_register(struct eth_device* dev);/* Register network device */
>  extern int eth_unregister(struct eth_device *dev);/* Remove network device */
>  extern void eth_try_another(int first_restart);        /* Change the device */
>  extern void eth_set_current(void);             /* set nterface to ethcur var */
> -extern struct eth_device *eth_get_dev(void);   /* get the current device MAC */
> +/* get the current device MAC */
> +static inline __attribute__((always_inline))
> +struct eth_device *eth_get_dev(void)
> +{
> +       extern struct eth_device *eth_current;
> +
> +       return eth_current;
> +}
>  extern struct eth_device *eth_get_dev_by_name(const char *devname);
>  extern struct eth_device *eth_get_dev_by_index(int index); /* get dev @ index */
>  extern int eth_get_dev_index(void);            /* get the device index */
> @@ -151,6 +158,19 @@ extern int eth_rx(void);                   /* Check for received packets */
>  extern void eth_halt(void);                    /* stop SCC */
>  extern char *eth_get_name(void);               /* get name of current device */
>
> +/* Set active state */
> +static inline __attribute__((always_inline)) int eth_init_state_only(bd_t *bis)
> +{
> +       eth_get_dev()->state = ETH_STATE_ACTIVE;
> +
> +       return 0;
> +}
> +/* Set passive state */
> +static inline __attribute__((always_inline)) void eth_halt_state_only(void)
> +{
> +       eth_get_dev()->state = ETH_STATE_PASSIVE;
> +}
> +
>  /*
>   * Set the hardware address for an ethernet interface based on 'eth%daddr'
>   * environment variable (or just 'ethaddr' if eth_number is 0).
> @@ -532,6 +552,26 @@ void NcStart(void);
>  int nc_input_packet(uchar *pkt, unsigned dest, unsigned src, unsigned len);
>  #endif
>
> +static inline __attribute__((always_inline)) int eth_is_on_demand_init(void)
> +{
> +#ifdef CONFIG_NETCONSOLE
> +       extern enum proto_t net_loop_last_protocol;
> +
> +       return net_loop_last_protocol != NETCONS;
> +#else
> +       return 1;
> +#endif
> +}
> +
> +static inline void eth_set_last_protocol(int protocol)
> +{
> +#ifdef CONFIG_NETCONSOLE
> +       extern enum proto_t net_loop_last_protocol;
> +
> +       net_loop_last_protocol = protocol;
> +#endif
> +}
> +
>  /*
>   * Check if autoload is enabled. If so, use either NFS or TFTP to download
>   * the boot file.
> diff --git a/net/eth.c b/net/eth.c
> index 1a11ce1..66295b6 100644
> --- a/net/eth.c
> +++ b/net/eth.c
> @@ -121,12 +121,8 @@ static struct {
>  static unsigned int eth_rcv_current, eth_rcv_last;
>  #endif
>
> -static struct eth_device *eth_devices, *eth_current;
> -
> -struct eth_device *eth_get_dev(void)
> -{
> -       return eth_current;
> -}
> +static struct eth_device *eth_devices;
> +struct eth_device *eth_current;
>
>  struct eth_device *eth_get_dev_by_name(const char *devname)
>  {
> diff --git a/net/net.c b/net/net.c
> index e8ff066..f002cda 100644
> --- a/net/net.c
> +++ b/net/net.c
> @@ -315,12 +315,15 @@ int NetLoop(enum proto_t protocol)
>
>         bootstage_mark_name(BOOTSTAGE_ID_ETH_START, "eth_start");
>         net_init();
> -       eth_halt();
> -       eth_set_current();
> -       if (eth_init(bd) < 0) {
> +       if (eth_is_on_demand_init() || protocol != NETCONS) {
>                 eth_halt();
> -               return -1;
> -       }
> +               eth_set_current();
> +               if (eth_init(bd) < 0) {
> +                       eth_halt();
> +                       return -1;
> +               }
> +       } else
> +               eth_init_state_only(bd);
>
>  restart:
>         net_set_state(NETLOOP_CONTINUE);
> @@ -460,6 +463,9 @@ restart:
>
>                         net_cleanup_loop();
>                         eth_halt();
> +                       /* Invalidate the last protocol */
> +                       eth_set_last_protocol(BOOTP);
> +
>                         puts("\nAbort\n");
>                         /* include a debug print as well incase the debug
>                            messages are directed to stderr */
> @@ -517,13 +523,21 @@ restart:
>                                 sprintf(buf, "%lX", (unsigned long)load_addr);
>                                 setenv("fileaddr", buf);
>                         }
> -                       eth_halt();
> +                       if (protocol != NETCONS)
> +                               eth_halt();
> +                       else
> +                               eth_halt_state_only();
> +
> +                       eth_set_last_protocol(protocol);
> +
>                         ret = NetBootFileXferSize;
>                         debug_cond(DEBUG_INT_STATE, "--- NetLoop Success!\n");
>                         goto done;
>
>                 case NETLOOP_FAIL:
>                         net_cleanup_loop();
> +                       /* Invalidate the last protocol */
> +                       eth_set_last_protocol(BOOTP);
>                         debug_cond(DEBUG_INT_STATE, "--- NetLoop Fail!\n");
>                         goto done;
>
> --
> 1.6.0.2
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot