Hello Heiko,
Please see my comments and updated patch below.
On Mon, Mar 30, 2009 at 7:12 AM, Heiko Schocher hs@denx.de wrote:
Hello Michael,
Michael Zaidman wrote:
Hi Heiko,
The patch "netloop: speed up NetLoop" you delivered into the u-boot-2009.03 introduced bug I have described below a few days ago. Could you please take a look at the proposed fix?
Sorry, seems I missed it.
Thanks, Michael
---------- Forwarded message ---------- From: Michael Zaidman michael.zaidman@gmail.com Date: Sun, Mar 22, 2009 at 8:35 PM Subject: [U-Boot] [PATCH] NetLoop initialization bug To: u-boot@lists.denx.de
[U-Boot] [PATCH] NetLoop initialization bug
Thanks for catching this. Your patch doesn;t apply, I get "malformed patch". Before sending a new one, please have a look at my comments:
Upon u-boot's start up the first ping command causes a failure of the consequent TFTP command. It happens in the recently added mechanism of the NetLoop initialization where initialization of global network parameters is separated in the NetInitLoop routine which is called per env_id change. Thus, ping request will initialize the network parameters necessary for ping operation only, afterwards the env_changed_id will be set to the env_id that will prevent all following initialization requests from other protocols. The problem is that the initialized by ping subset of network parameters is not sufficient for other protocols and particularly for TFTP which requires the NetServerIp also.
Signed-off-by: michael.zaidman@gmail.com
diff --git a/net/net.c b/net/net.c index a89f6a0..dc98d0f 100644 --- a/net/net.c +++ b/net/net.c @@ -288,64 +288,13 @@ NetInitLoop(proto_t protocol) if (env_changed_id == env_id) return 0;
- switch (protocol) {
-#if defined(CONFIG_CMD_NFS)
- case NFS:
-#endif -#if defined(CONFIG_CMD_PING)
- case PING:
-#endif -#if defined(CONFIG_CMD_SNTP)
- case SNTP:
-#endif
- case NETCONS:
- case TFTP:
- NetCopyIP(&NetOurIP, &bd->bi_ip_addr);
- NetOurGatewayIP = getenv_IPaddr ("gatewayip");
- NetOurSubnetMask= getenv_IPaddr ("netmask");
- NetOurVLAN = getenv_VLAN("vlan");
- NetOurNativeVLAN = getenv_VLAN("nvlan");
- NetCopyIP(&NetOurIP, &bd->bi_ip_addr);
- NetOurGatewayIP = getenv_IPaddr ("gatewayip");
- NetOurSubnetMask= getenv_IPaddr ("netmask");
- NetServerIP = getenv_IPaddr ("serverip");
The following 2 vars are just used, if CONFIG_CMD_CDP is used, can we do a "#if defined" around it?
- NetOurNativeVLAN = getenv_VLAN("nvlan");
- NetOurVLAN = getenv_VLAN("vlan");
These two variables have been initialized in original code for all protocols supported by u-boot. As I can see, at least the NetOurVLAN is used by most of them. This was the reason for keeping them in place.
[...]
@@ -443,18 +392,19 @@ restart: /* Start with a clean slate... */ BootpTry = 0; NetOurIP = 0;
- NetServerIP = getenv_IPaddr ("serverip");
DhcpRequest(); /* Basically same as BOOTP */ break; #endif
case BOOTP: BootpTry = 0;
- NetOurIP = 0;
why we need this here?
Generally, for the same reason the DHCP does - "Start with a clean state..." On the other hand you are right - we do not need to clear the NetOurIP address for BOOTP, DHCP and RARP because the current implementation does not check it. So it has been removed in my new patch below.
BootpRequest (); break;
case RARP: RarpTry = 0;
- NetOurIP = 0;
also here? Has this to do something with the bugfix?
The same as before.
bye Heiko
-- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Here is the updated patch:
Subject: [U-Boot] [PATCH] NetLoop initialization bug
--- net/net.c | 66 +++++------------------------------------------------------- 1 files changed, 6 insertions(+), 60 deletions(-)
diff --git a/net/net.c b/net/net.c index a89f6a0..91ed0d7 100644 --- a/net/net.c +++ b/net/net.c @@ -288,64 +288,13 @@ NetInitLoop(proto_t protocol) if (env_changed_id == env_id) return 0;
- switch (protocol) { -#if defined(CONFIG_CMD_NFS) - case NFS: -#endif -#if defined(CONFIG_CMD_PING) - case PING: -#endif -#if defined(CONFIG_CMD_SNTP) - case SNTP: -#endif - case NETCONS: - case TFTP: - NetCopyIP(&NetOurIP, &bd->bi_ip_addr); - NetOurGatewayIP = getenv_IPaddr ("gatewayip"); - NetOurSubnetMask= getenv_IPaddr ("netmask"); - NetOurVLAN = getenv_VLAN("vlan"); - NetOurNativeVLAN = getenv_VLAN("nvlan"); + NetCopyIP(&NetOurIP, &bd->bi_ip_addr); + NetOurGatewayIP = getenv_IPaddr ("gatewayip"); + NetOurSubnetMask= getenv_IPaddr ("netmask"); + NetServerIP = getenv_IPaddr ("serverip"); + NetOurNativeVLAN = getenv_VLAN("nvlan"); + NetOurVLAN = getenv_VLAN("vlan");
- switch (protocol) { -#if defined(CONFIG_CMD_NFS) - case NFS: -#endif - case NETCONS: - case TFTP: - NetServerIP = getenv_IPaddr ("serverip"); - break; -#if defined(CONFIG_CMD_PING) - case PING: - /* nothing */ - break; -#endif -#if defined(CONFIG_CMD_SNTP) - case SNTP: - /* nothing */ - break; -#endif - default: - break; - } - - break; - case BOOTP: - case RARP: - /* - * initialize our IP addr to 0 in order to accept ANY - * IP addr assigned to us by the BOOTP / RARP server - */ - NetOurIP = 0; - NetServerIP = getenv_IPaddr ("serverip"); - NetOurVLAN = getenv_VLAN("vlan"); /* VLANs must be read */ - NetOurNativeVLAN = getenv_VLAN("nvlan"); - case CDP: - NetOurVLAN = getenv_VLAN("vlan"); /* VLANs must be read */ - NetOurNativeVLAN = getenv_VLAN("nvlan"); - break; - default: - break; - } env_changed_id = env_id; return 0; } @@ -440,10 +389,7 @@ restart:
#if defined(CONFIG_CMD_DHCP) case DHCP: - /* Start with a clean slate... */ BootpTry = 0; - NetOurIP = 0; - NetServerIP = getenv_IPaddr ("serverip"); DhcpRequest(); /* Basically same as BOOTP */ break; #endif