* [U-Boot] DHCP regression on 2009-06
@ 2009-07-10 16:27 Robin Getz
2009-07-10 19:18 ` Robin Getz
0 siblings, 1 reply; 12+ messages in thread
From: Robin Getz @ 2009-07-10 16:27 UTC (permalink / raw)
To: u-boot
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....
^ permalink raw reply [flat|nested] 12+ messages in thread
* [U-Boot] DHCP regression on 2009-06
2009-07-10 16:27 [U-Boot] DHCP regression on 2009-06 Robin Getz
@ 2009-07-10 19:18 ` Robin Getz
2009-07-12 13:14 ` Michael Zaidman
0 siblings, 1 reply; 12+ messages in thread
From: Robin Getz @ 2009-07-10 19:18 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* [U-Boot] DHCP regression on 2009-06
2009-07-10 19:18 ` Robin Getz
@ 2009-07-12 13:14 ` Michael Zaidman
2009-07-12 14:43 ` Wolfgang Denk
2009-07-12 19:47 ` Robin Getz
0 siblings, 2 replies; 12+ messages in thread
From: Michael Zaidman @ 2009-07-12 13:14 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* [U-Boot] DHCP regression on 2009-06
2009-07-12 13:14 ` Michael Zaidman
@ 2009-07-12 14:43 ` Wolfgang Denk
2009-07-12 19:47 ` Robin Getz
1 sibling, 0 replies; 12+ messages in thread
From: Wolfgang Denk @ 2009-07-12 14:43 UTC (permalink / raw)
To: u-boot
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!
^ permalink raw reply [flat|nested] 12+ messages in thread
* [U-Boot] DHCP regression on 2009-06
2009-07-12 13:14 ` Michael Zaidman
2009-07-12 14:43 ` Wolfgang Denk
@ 2009-07-12 19:47 ` Robin Getz
2009-07-13 15:11 ` Michael Zaidman
1 sibling, 1 reply; 12+ messages in thread
From: Robin Getz @ 2009-07-12 19:47 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* [U-Boot] DHCP regression on 2009-06
2009-07-12 19:47 ` Robin Getz
@ 2009-07-13 15:11 ` Michael Zaidman
2009-07-13 15:58 ` Robin Getz
2009-07-13 18:40 ` Robin Getz
0 siblings, 2 replies; 12+ messages in thread
From: Michael Zaidman @ 2009-07-13 15:11 UTC (permalink / raw)
To: u-boot
> 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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [U-Boot] DHCP regression on 2009-06
2009-07-13 15:11 ` Michael Zaidman
@ 2009-07-13 15:58 ` Robin Getz
2009-07-13 18:40 ` Robin Getz
1 sibling, 0 replies; 12+ messages in thread
From: Robin Getz @ 2009-07-13 15:58 UTC (permalink / raw)
To: u-boot
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?
^ permalink raw reply [flat|nested] 12+ messages in thread
* [U-Boot] DHCP regression on 2009-06
2009-07-13 15:11 ` Michael Zaidman
2009-07-13 15:58 ` Robin Getz
@ 2009-07-13 18:40 ` Robin Getz
2009-07-14 9:26 ` Michael Zaidman
1 sibling, 1 reply; 12+ messages in thread
From: Robin Getz @ 2009-07-13 18:40 UTC (permalink / raw)
To: u-boot
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>
^ permalink raw reply [flat|nested] 12+ messages in thread
* [U-Boot] DHCP regression on 2009-06
2009-07-13 18:40 ` Robin Getz
@ 2009-07-14 9:26 ` Michael Zaidman
2009-07-14 14:57 ` Ben Warren
0 siblings, 1 reply; 12+ messages in thread
From: Michael Zaidman @ 2009-07-14 9:26 UTC (permalink / raw)
To: u-boot
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.
^ permalink raw reply [flat|nested] 12+ messages in thread
* [U-Boot] DHCP regression on 2009-06
2009-07-14 9:26 ` Michael Zaidman
@ 2009-07-14 14:57 ` Ben Warren
2009-07-15 5:19 ` Michael Zaidman
0 siblings, 1 reply; 12+ messages in thread
From: Ben Warren @ 2009-07-14 14:57 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* [U-Boot] DHCP regression on 2009-06
2009-07-14 14:57 ` Ben Warren
@ 2009-07-15 5:19 ` Michael Zaidman
2009-07-15 5:20 ` Ben Warren
0 siblings, 1 reply; 12+ messages in thread
From: Michael Zaidman @ 2009-07-15 5:19 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* [U-Boot] DHCP regression on 2009-06
2009-07-15 5:19 ` Michael Zaidman
@ 2009-07-15 5:20 ` Ben Warren
0 siblings, 0 replies; 12+ messages in thread
From: Ben Warren @ 2009-07-15 5:20 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2009-07-15 5:20 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2009-07-10 16:27 [U-Boot] DHCP regression on 2009-06 Robin Getz
2009-07-10 19:18 ` Robin Getz
2009-07-12 13:14 ` Michael Zaidman
2009-07-12 14:43 ` Wolfgang Denk
2009-07-12 19:47 ` Robin Getz
2009-07-13 15:11 ` Michael Zaidman
2009-07-13 15:58 ` Robin Getz
2009-07-13 18:40 ` Robin Getz
2009-07-14 9:26 ` Michael Zaidman
2009-07-14 14:57 ` Ben Warren
2009-07-15 5:19 ` Michael Zaidman
2009-07-15 5:20 ` Ben Warren
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.