[U-Boot] DHCP regression on 2009-06

[U-Boot] DHCP regression on 2009-06

[Robin Getz]

http://git.denx.de/?p=u-boot/u-boot-net.git;a=commitdiff;h=3c172c4fdbbb5858fae38478d6399be4a16be3fc

causes a regression on my network's DHCP server.

The part of the diff that causes the problem:

 #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;

Since we are leaving the "NetOurIP" to whatever it was... The test at:
NetReceive():

         case PROT_IP:
            [snip]
                tmp = NetReadIP(&ip->ip_dst);
                if (NetOurIP && tmp != NetOurIP && tmp != 0xFFFFFFFF) {
#ifdef CONFIG_MCAST_TFTP
                        if (Mcast_addr != tmp)
#endif
                        return;
                }

Will return - (we leave the 'NetOurIP' set to the old value, the offered 
address (what is in tmp) is not our's and tmp is not 0xFFFFFFFF).

You never process the DHCP_OFFER...

There are multiple ways of fixing things - setting "NetOurIP = 0;" (revert one 
line of the change, which may expose the bug that Michael was trying to fix), 
or try to be more tricky in the "not our address" check....

[U-Boot] DHCP regression on 2009-06

[Robin Getz]

On Fri 10 Jul 2009 12:27, Robin Getz pondered:
> http://git.denx.de/?p=u-boot/u-boot-net.git;a=commitdiff;h=3c172c4fdbbb5
> 858fae38478d6399be4a16be3fc
> 
> causes a regression on my network's DHCP server.
> 
> The part of the diff that causes the problem:
> 
>  #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;
> 
> Since we are leaving the "NetOurIP" to whatever it was... The test at:
> NetReceive():
> 
>          case PROT_IP:
>             [snip]
>                 tmp = NetReadIP(&ip->ip_dst);
>                 if (NetOurIP && tmp != NetOurIP && tmp != 0xFFFFFFFF) {
> #ifdef CONFIG_MCAST_TFTP
>                         if (Mcast_addr != tmp)
> #endif
>                         return;
>                 }
> 
> Will return - (we leave the 'NetOurIP' set to the old value, the offered
> 
> address (what is in tmp) is not our's and tmp is not 0xFFFFFFFF).
> 
> You never process the DHCP_OFFER...
> 
> There are multiple ways of fixing things - setting "NetOurIP = 0;"
> (revert one line of the change, which may expose the bug that Michael 
> was trying to fix) or try to be more tricky in the "not our address"
> check.... 
> 

I did verify that reverting the line exposes the bug that Michael fixed,
and so did this -- which seemed to work for my limited testing...

Index: net/bootp.c
===================================================================
--- net/bootp.c	(revision 1961)
+++ net/bootp.c	(working copy)
@@ -83,7 +83,7 @@
 
 #endif
 
-static int BootpCheckPkt(uchar *pkt, unsigned dest, unsigned src, unsigned len)
+int BootpCheckPkt(uchar *pkt, unsigned dest, unsigned src, unsigned len)
 {
 	Bootp_t *bp = (Bootp_t *) pkt;
 	int retval = 0;
Index: net/net.c
===================================================================
--- net/net.c	(revision 1961)
+++ net/net.c	(working copy)
@@ -1368,7 +1377,11 @@
 			return;
 		}
 		tmp = NetReadIP(&ip->ip_dst);
-		if (NetOurIP && tmp != NetOurIP && tmp != 0xFFFFFFFF) {
+		if (NetOurIP && tmp != NetOurIP && tmp != 0xFFFFFFFF &&
+			BootpCheckPkt((uchar *)ip +IP_HDR_SIZE,
+                                                ntohs(ip->udp_dst),
+                                                ntohs(ip->udp_src),
+                                                ntohs(ip->udp_len) - 8)) {
 #ifdef CONFIG_MCAST_TFTP
 			if (Mcast_addr != tmp)
 #endif

[U-Boot] DHCP regression on 2009-06

[Michael Zaidman]

On Fri, Jul 10, 2009 at 10:18 PM, Robin Getz <rgetz@blackfin.uclinux.org>wrote:

> On Fri 10 Jul 2009 12:27, Robin Getz pondered:
> > http://git.denx.de/?p=u-boot/u-boot-net.git;a=commitdiff;h=3c172c4fdbbb5
> > 858fae38478d6399be4a16be3fc
> >
> > causes a regression on my network's DHCP server.
> > The part of the diff that causes the problem:
> >
> >  #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;
> >
> > Since we are leaving the "NetOurIP" to whatever it was... The test at:
> > NetReceive():
> >
> >          case PROT_IP:
> >             [snip]
> >                 tmp = NetReadIP(&ip->ip_dst);
> >                 if (NetOurIP && tmp != NetOurIP && tmp != 0xFFFFFFFF) {
> >                         return;
> >                 }
> >
> > Will return - (we leave the 'NetOurIP' set to the old value, the offered
> > address (what is in tmp) is not our's and tmp is not 0xFFFFFFFF).
> >
> > You never process the DHCP_OFFER...
> >
>

Did you try to remove the CONFIG_IPADDR from the board's config file?
In this case the NetOurIP will get 0.  That should solve the problem.

Best regards,
Michael

[U-Boot] DHCP regression on 2009-06

[Wolfgang Denk]

Dear Michael Zaidman,

In message <660c0f820907120614i2b33d8a1g9ac46c103e548bed@mail.gmail.com> you wrote:
> 
> Did you try to remove the CONFIG_IPADDR from the board's config file?
> In this case the NetOurIP will get 0.  That should solve the problem.


Setting  or  not  setting  CONFIG_IPADDR  only  changes  the  default
environment,  and  it should have zero impact whether "ipaddr" is set
in the U-Boot environment or not.

If this should not be the case, then there is indeed a bug (but the
not defining CONFIG_IPADDR will not fix it either).


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Star Trek Lives!

[U-Boot] DHCP regression on 2009-06

[Robin Getz]

On Sun 12 Jul 2009 09:14, Michael Zaidman pondered:
> 
> On Fri, Jul 10, 2009 at 10:18 PM, Robin Getz <rgetz@blackfin.uclinux.org> 
wrote:
> 
> 
> On Fri 10 Jul 2009 12:27, Robin Getz pondered:
> > > 
http://git.denx.de/?p=u-boot/u-boot-net.git;a=commitdiff;h=3c172c4fdbbb5858fae38478d6399be4a16be3fc
> >
> > causes a regression on my network's DHCP server.
> > The part of the diff that causes the problem:
> >
> >  #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;
> >
> > Since we are leaving the "NetOurIP" to whatever it was... The test at:
> > NetReceive():
> >
> >          case PROT_IP:
> >             [snip]
> >                 tmp = NetReadIP(&ip->ip_dst);
> >                 if (NetOurIP && tmp != NetOurIP && tmp != 0xFFFFFFFF) { 
> >                         return;
> >                 }
> >
> > Will return - (we leave the 'NetOurIP' set to the old value, the offered
> > address (what is in tmp) is not our's and tmp is not 0xFFFFFFFF).
> >
> > You never process the DHCP_OFFER...
> >
>  
> Did you try to remove the CONFIG_IPADDR from the board's config file?
> In this case the NetOurIP will get 0.  That should solve the problem.

No it does not - the problem occurs if you do dhcp, save, and then move to a 
different subnet, and a dhcp again (which is when I found it - recently moved 
offices, and needed new IP addresses for all my development systems)

As Wolfgang stated: the initial state (what CONFIG_IPADDR controls) doesn't 
change the issue that the bug exists - it just controls when the bug 
appears - sooner or later - but it is still there....

Rather than call BootpCheckPkt() as I suggested - Ben could just check the 
value of packetHandler... (if it is DhcpHandler, don't return)...



-Robin

[U-Boot] DHCP regression on 2009-06

[Michael Zaidman]

> I did verify that reverting the line exposes the bug that Michael fixed, ...

Ok, my target uses static IP configuration so I did not verified the DHCP
behavior. Now I did it. I also reverted the line and did DHCP afterwards
changed the subnet and did DHCP again. It works as expected.

diff --git a/net/net.c b/net/net.c
index 5637cf5..5e43dd1 100644
--- a/net/net.c
+++ b/net/net.c
@@ -388,6 +388,7 @@ restart:
 #if defined(CONFIG_CMD_DHCP)
                case DHCP:
                        BootpTry = 0;
+                       NetOurIP = 0;
                        DhcpRequest();          /* Basically same as BOOTP */
                        break;
 #endif

[U-Boot] DHCP regression on 2009-06

[Robin Getz]

On Mon 13 Jul 2009 11:11, Michael Zaidman pondered:
> > I did verify that reverting the line exposes the bug that Michael fixed, ...
> 
> Ok, my target uses static IP configuration so I did not verified the DHCP
> behavior. Now I did it. I also reverted the line and did DHCP afterwards
> changed the subnet and did DHCP again. It works as expected.
> 
> diff --git a/net/net.c b/net/net.c
> index 5637cf5..5e43dd1 100644
> --- a/net/net.c
> +++ b/net/net.c
> @@ -388,6 +388,7 @@ restart:
>  #if defined(CONFIG_CMD_DHCP)
>                 case DHCP:
>                         BootpTry = 0;
> +                       NetOurIP = 0;
>                         DhcpRequest();          /* Basically same as BOOTP */
>                         break;
>  #endif
> 

But -- doesn't this expose the ping issue you were trying to fix?

[U-Boot] DHCP regression on 2009-06

[Robin Getz]

On Mon 13 Jul 2009 11:11, Michael Zaidman pondered:
> > I did verify that reverting the line exposes the bug that Michael fixed, ...
> 
> Ok, my target uses static IP configuration so I did not verified the DHCP
> behavior. Now I did it. I also reverted the line and did DHCP afterwards
> changed the subnet and did DHCP again. It works as expected.
> 
> diff --git a/net/net.c b/net/net.c
> index 5637cf5..5e43dd1 100644
> --- a/net/net.c
> +++ b/net/net.c
> @@ -388,6 +388,7 @@ restart:
>  #if defined(CONFIG_CMD_DHCP)
>                 case DHCP:
>                         BootpTry = 0;
> +                       NetOurIP = 0;
>                         DhcpRequest();          /* Basically same as BOOTP */
>                         break;
>  #endif
> 

OK - I tested it - and it seems to work for me.

Some of my debugging crap when I was tracking this down must have messed this up.

Ack-by: Robin Getz <rgetz@blackfin.uclinux.org>

[U-Boot] DHCP regression on 2009-06

[Michael Zaidman]

On Mon, Jul 13, 2009 at 9:40 PM, Robin Getz <rgetz@blackfin.uclinux.org> wrote:
>
> On Mon 13 Jul 2009 11:11, Michael Zaidman pondered:
> > > I did verify that reverting the line exposes the bug that Michael fixed, ...
> >
> > Ok, my target uses static IP configuration so I did not verified the DHCP
> > behavior. Now I did it. I also reverted the line and did DHCP afterwards
> > changed the subnet and did DHCP again. It works as expected.
> >
> > diff --git a/net/net.c b/net/net.c
> > index 5637cf5..5e43dd1 100644
> > --- a/net/net.c
> > +++ b/net/net.c
> > @@ -388,6 +388,7 @@ restart:
> > ?#if defined(CONFIG_CMD_DHCP)
> > ? ? ? ? ? ? ? ? case DHCP:
> > ? ? ? ? ? ? ? ? ? ? ? ? BootpTry = 0;
> > + ? ? ? ? ? ? ? ? ? ? ? NetOurIP = 0;
> > ? ? ? ? ? ? ? ? ? ? ? ? DhcpRequest(); ? ? ? ? ?/* Basically same as BOOTP */
> > ? ? ? ? ? ? ? ? ? ? ? ? break;
> > ?#endif
> >
>
> OK - I tested it - and it seems to work for me.
>
> Some of my debugging crap when I was tracking this down must have messed this up.
>
> Ack-by: Robin Getz <rgetz@blackfin.uclinux.org>

If nobody has objections I can submit this fix.

Michael.

[U-Boot] DHCP regression on 2009-06

[Ben Warren]

On Tue, Jul 14, 2009 at 2:26 AM, Michael Zaidman
<michael.zaidman@gmail.com>wrote:

> On Mon, Jul 13, 2009 at 9:40 PM, Robin Getz <rgetz@blackfin.uclinux.org>
> wrote:
> >
> > On Mon 13 Jul 2009 11:11, Michael Zaidman pondered:
> > > > I did verify that reverting the line exposes the bug that Michael
> fixed, ...
> > >
> > > Ok, my target uses static IP configuration so I did not verified the
> DHCP
> > > behavior. Now I did it. I also reverted the line and did DHCP
> afterwards
> > > changed the subnet and did DHCP again. It works as expected.
> > >
> > > diff --git a/net/net.c b/net/net.c
> > > index 5637cf5..5e43dd1 100644
> > > --- a/net/net.c
> > > +++ b/net/net.c
> > > @@ -388,6 +388,7 @@ restart:
> > >  #if defined(CONFIG_CMD_DHCP)
> > >                 case DHCP:
> > >                         BootpTry = 0;
> > > +                       NetOurIP = 0;
> > >                         DhcpRequest();          /* Basically same as
> BOOTP */
> > >                         break;
> > >  #endif
> > >
> >
> > OK - I tested it - and it seems to work for me.
> >
> > Some of my debugging crap when I was tracking this down must have messed
> this up.
> >
> > Ack-by: Robin Getz <rgetz@blackfin.uclinux.org>
>
> If nobody has objections I can submit this fix.
>
> Michael.
>

Please do.

regards,
Ben

[U-Boot] DHCP regression on 2009-06

[Michael Zaidman]

On Tue, Jul 14, 2009 at 5:57 PM, Ben Warren<biggerbadderben@gmail.com> wrote:
>
> On Tue, Jul 14, 2009 at 2:26 AM, Michael Zaidman <michael.zaidman@gmail.com>
> wrote:
>>
>> If nobody has objections I can submit this fix.
>>
>> Michael.
>
> Please do.
>
> regards,
> Ben
>

Done.

http://lists.denx.de/pipermail/u-boot/2009-July/056354.html

Regards,
Michael

[U-Boot] DHCP regression on 2009-06

[Ben Warren]

Michael Zaidman wrote:
> On Tue, Jul 14, 2009 at 5:57 PM, Ben Warren<biggerbadderben@gmail.com> wrote:
>   
>> On Tue, Jul 14, 2009 at 2:26 AM, Michael Zaidman <michael.zaidman@gmail.com>
>> wrote:
>>     
>>> If nobody has objections I can submit this fix.
>>>
>>> Michael.
>>>       
>> Please do.
>>
>> regards,
>> Ben
>>
>>     
>
> Done.
>
> http://lists.denx.de/pipermail/u-boot/2009-July/056354.html
>
> Regards,
> Michael
>   
Thanks, got it.  Will incorporate soon.

regards,
Ben