[U-Boot] DHCP regression on 2009-06

newer
[U-Boot] [PATCH] 85xx: Bump up the...

older
[U-Boot] [PATCH] ppc4xx: Add 405EP...

Robin Getz

10 Jul 2009 10 Jul '09

6:27 p.m.

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

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....

Show replies by date

Robin Getz

10 Jul 10 Jul

9:18 p.m.

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

Michael Zaidman

12 Jul 12 Jul

3:14 p.m.

On Fri, Jul 10, 2009 at 10:18 PM, Robin Getz rgetz@blackfin.uclinux.orgwrote:

...

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

Wolfgang Denk

4:43 p.m.

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@denx.de Star Trek Lives!

Robin Getz

9:47 p.m.

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=3c172c4fdbbb5858f...

...

...
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

Michael Zaidman

13 Jul 13 Jul

5:11 p.m.

...

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

Robin Getz

5:58 p.m.

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?

Robin Getz

8:40 p.m.

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

Michael Zaidman

14 Jul 14 Jul

11:26 a.m.

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.

Ben Warren

4:57 p.m.

On Tue, Jul 14, 2009 at 2:26 AM, Michael Zaidman michael.zaidman@gmail.comwrote:

...

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

Michael Zaidman

15 Jul 15 Jul

7:19 a.m.

On Tue, Jul 14, 2009 at 5:57 PM, Ben Warrenbiggerbadderben@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

Ben Warren

7:20 a.m.

Michael Zaidman wrote:

...

On Tue, Jul 14, 2009 at 5:57 PM, Ben Warrenbiggerbadderben@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

5777

Age (days ago)

5782

Last active (days ago)

List overview

Download

11 comments

4 participants

tags (0)

participants (4)

Ben Warren
Michael Zaidman
Robin Getz
Wolfgang Denk