[U-Boot] [PATCH] NetLoop initialization bug

Michael Zaidman michael.zaidman at gmail.com
Mon Mar 30 15:07:11 CEST 2009


Hello Heiko,

Please see my comments and updated patch below.

On Mon, Mar 30, 2009 at 7:12 AM, Heiko Schocher <hs at 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 at gmail.com>
>> Date: Sun, Mar 22, 2009 at 8:35 PM
>> Subject: [U-Boot] [PATCH] NetLoop initialization bug
>> To: u-boot at 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 at 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
-- 
1.5.6.3

Thanks,
Michael


More information about the U-Boot mailing list