Skip to contentSkip to conversation

net/net.c: correct timeout function

PATCH New Daniel Mack 3 participants 16 messages Nov 28, 2008 – Dec 6, 2008 Networking

lore
Share & tools
lore

Daniel Mack

author
#0 lore raw

PATCH 1/1 net/net.c: correct timeout function

Hi,

the net/net.c implemenation of timeouts assumes that get_timer() returns values in milliseconds. As this is true for most platforms, it does not apply to PXA3x where the OSCR register increments with more than 3MHz.

The following patch fixes the problem by calculation with the CONFIG_SYS_HZ variable.

Best regards,
Daniel

net/net.c +10 -5
index 77e83b5..b9326de 100644
@@ -206,6 +206,11 @@ uchar		NetArpWaitPacketBuf[PKTSIZE_ALIGN + PKTALIGN];
 ulong		NetArpWaitTimerStart;
 int		NetArpWaitTry;
 
+static long net_get_timer(long base)
+{
+	return get_timer(base) / (CONFIG_SYS_HZ / 1000);
+}
+
 void ArpRequest (void)
 {
 	int i;

@@ -256,7 +261,7 @@ void ArpTimeoutCheck(void)
 	if (!NetArpWaitPacketIP)
 		return;
 
-	t = get_timer(0);
+	t = net_get_timer(0);
 
 	/* check for arp timeout */
 	if ((t - NetArpWaitTimerStart) > ARP_TIMEOUT) {

@@ -517,7 +522,7 @@ restart:
 		 *	Check for a timeout, and run the timeout handler
 		 *	if we have one.
 		 */
-		if (timeHandler && ((get_timer(0) - timeStart) > timeDelta)) {
+		if (timeHandler && ((net_get_timer(0) - timeStart) > timeDelta)) {
 			thand_f *x;
 
 #if defined(CONFIG_MII) || defined(CONFIG_CMD_MII)

@@ -639,7 +644,7 @@ NetSetTimeout(ulong iv, thand_f * f)
 		timeHandler = (thand_f *)0;
 	} else {
 		timeHandler = f;
-		timeStart = get_timer(0);
+		timeStart = net_get_timer(0);
 		timeDelta = iv;
 	}
 }

@@ -684,7 +689,7 @@ NetSendUDPPacket(uchar *ether, IPaddr_t dest, int dport, int sport, int len)
 
 		/* and do the ARP request */
 		NetArpWaitTry = 1;
-		NetArpWaitTimerStart = get_timer(0);
+		NetArpWaitTimerStart = net_get_timer(0);
 		ArpRequest();
 		return 1;	/* waiting */
 	}

@@ -755,7 +760,7 @@ int PingSend(void)
 
 	/* and do the ARP request */
 	NetArpWaitTry = 1;
-	NetArpWaitTimerStart = get_timer(0);
+	NetArpWaitTimerStart = net_get_timer(0);
 	ArpRequest();
 	return 1;	/* waiting */
 }
author
#1 lore raw

PATCH 1/1 net/net.c: correct timeout function

Quoted 6 lines · Daniel Mack

the net/net.c implemenation of timeouts assumes that get_timer() returns values in milliseconds. As this is true for most platforms, it does not apply to PXA3x where the OSCR register increments with more than 3MHz.

The following patch fixes the problem by calculation with the CONFIG_SYS_HZ variable.

[...]

Just curious - was this patch taken? Same question for the ones sent in mails labeled

[PATCH] ne2000: take MAC address from config if available (+follow-ups) [PATCH] PXA3xx: fix CKEN[AB]

Thanks,
Daniel

#2 lore raw

PATCH 1/1 net/net.c: correct timeout function

Hi Daniel,

Quoted 10 lines · Daniel Mack
Quoted 7 lines · Daniel Mack

the net/net.c implemenation of timeouts assumes that get_timer() returns values in milliseconds. As this is true for most platforms, it does not apply to PXA3x where the OSCR register increments with more than 3MHz.

The following patch fixes the problem by calculation with the CONFIG_SYS_HZ variable.

Quoted 6 lines

[...]

Just curious - was this patch taken? Same question for the ones sent in mails labeled

[PATCH] ne2000: take MAC address from config if available (+follow-ups)

Sorry, I've been bogged down with other stuff. Some things are missing from your patches, which can mainly be fixed by using 'git format-patch -C':
1. No signed-off-by
2. No diffstat
3. Place text that shouldn't be in the commit message below the '---' line

If you take care of this easy stuff and resubmit I'll take a hard look at the content in the next couple of days. Please understand that the merge window is currently closed, so your code won't make it into the mainline U-boot until January sometime, although it will go in the net tree as soon as it's compliant.

regards,
Ben

author
#3 lore raw

PATCH 1/1 net/net.c: correct timeout function

Hi Ben,

Quoted 7 lines · Ben Warren

Sorry, I've been bogged down with other stuff. Some things are missing from your patches, which can mainly be fixed by using 'git format-patch -C':
1. No signed-off-by
2. No diffstat
3. Place text that shouldn't be in the commit message below the '---' line

Quoted 5 lines

If you take care of this easy stuff and resubmit I'll take a hard look at the content in the next couple of days. Please understand that the merge window is currently closed, so your code won't make it into the mainline U-boot until January sometime, although it will go in the net tree as soon as it's compliant.

I re-submitted them, thanks.

No offence meant, btw. I was just fearing my mails to be drowning in the flood of emails on this list.

Thanks and best regards,
Daniel

#4 lore raw

PATCH 1/1 net/net.c: correct timeout function

Quoted 18 lines · Daniel Mack

Hi Ben,

Quoted 7 lines · Ben Warren

Sorry, I've been bogged down with other stuff. Some things are missing from your patches, which can mainly be fixed by using 'git format-patch -C':
1. No signed-off-by
2. No diffstat
3. Place text that shouldn't be in the commit message below the '---' line

Quoted 6 lines

If you take care of this easy stuff and resubmit I'll take a hard look at the content in the next couple of days. Please understand that the merge window is currently closed, so your code won't make it into the mainline U-boot until January sometime, although it will go in the net tree as soon as it's compliant.

Quoted 9 lines

I re-submitted them, thanks.

No offence meant, btw. I was just fearing my mails to be drowning in the flood of emails on this list.

Thanks and best regards,
Daniel

No problem, and I'll try to give feedback soon.

regards,
Ben

author
#5 lore raw

PATCH 1/1 net/net.c: correct timeout function

In reply to Daniel Mack

the net/net.c implementation of timeouts assumes that get_timer() returns values in milliseconds. As this is true for most platforms, it does not apply to PXA3x where the OSCR register increments with more than 3MHz.

The following patch fixes the problem by calculation with the CONFIG_SYS_HZ variable.

Signed-off-by Daniel Mack <[email protected]>
---
 net.c |   15 ++++++++++-----
  1 file changed, 10 insertions(+), 5 deletions(-)
net/net.c +10 -5
index 77e83b5..b9326de 100644
@@ -206,6 +206,11 @@ uchar		NetArpWaitPacketBuf[PKTSIZE_ALIGN + PKTALIGN];
 ulong		NetArpWaitTimerStart;
 int		NetArpWaitTry;
 
+static long net_get_timer(long base)
+{
+	return get_timer(base) / (CONFIG_SYS_HZ / 1000);
+}
+
 void ArpRequest (void)
 {
 	int i;

@@ -256,7 +261,7 @@ void ArpTimeoutCheck(void)
 	if (!NetArpWaitPacketIP)
 		return;
 
-	t = get_timer(0);
+	t = net_get_timer(0);
 
 	/* check for arp timeout */
 	if ((t - NetArpWaitTimerStart) > ARP_TIMEOUT) {

@@ -517,7 +522,7 @@ restart:
 		 *	Check for a timeout, and run the timeout handler
 		 *	if we have one.
 		 */
-		if (timeHandler && ((get_timer(0) - timeStart) > timeDelta)) {
+		if (timeHandler && ((net_get_timer(0) - timeStart) > timeDelta)) {
 			thand_f *x;
 
 #if defined(CONFIG_MII) || defined(CONFIG_CMD_MII)

@@ -639,7 +644,7 @@ NetSetTimeout(ulong iv, thand_f * f)
 		timeHandler = (thand_f *)0;
 	} else {
 		timeHandler = f;
-		timeStart = get_timer(0);
+		timeStart = net_get_timer(0);
 		timeDelta = iv;
 	}
 }

@@ -684,7 +689,7 @@ NetSendUDPPacket(uchar *ether, IPaddr_t dest, int dport, int sport, int len)
 
 		/* and do the ARP request */
 		NetArpWaitTry = 1;
-		NetArpWaitTimerStart = get_timer(0);
+		NetArpWaitTimerStart = net_get_timer(0);
 		ArpRequest();
 		return 1;	/* waiting */
 	}

@@ -755,7 +760,7 @@ int PingSend(void)
 
 	/* and do the ARP request */
 	NetArpWaitTry = 1;
-	NetArpWaitTimerStart = get_timer(0);
+	NetArpWaitTimerStart = net_get_timer(0);
 	ArpRequest();
 	return 1;	/* waiting */
 }
#6 lore raw

PATCH 1/1 net/net.c: correct timeout function

Hi Daniel,

Quoted 8 lines · Daniel Mack

the net/net.c implementation of timeouts assumes that get_timer() returns values in milliseconds. As this is true for most platforms, it does not apply to PXA3x where the OSCR register increments with more than 3MHz.

The following patch fixes the problem by calculation with the CONFIG_SYS_HZ variable.

I apologize for taking so long to respond to this.

Quoted 11 lines
Signed-off-by Daniel Mack <[email protected]>
---
 net.c |   15 ++++++++++-----
  1 file changed, 10 insertions(+), 5 deletions(-)
net/net.c +0 -0
index 77e83b5..b9326de 100644

net/net.c uchar NetArpWaitPacketBuf[PKTSIZE_ALIGN + PKTALIGN]; · line 214

Quoted 9 lines
net/net.c +5 -0
@@ -206,6 +206,11 @@ uchar		NetArpWaitPacketBuf[PKTSIZE_ALIGN + PKTALIGN];
 ulong		NetArpWaitTimerStart;
 int		NetArpWaitTry;
 
+static long net_get_timer(long base)
+{
+	return get_timer(base) / (CONFIG_SYS_HZ / 1000);
+}
+

Please change this name to something more meaningful, maybe get_timer_ms(). Apart from that, looks good. If you re-submit soon I'll pull it in.

regards,
Ben

author
#7 lore raw

PATCH 1/1 net/net.c: add get_timer_ms()

Make timeout implementation in net/net.c take into account the CONFIG_SYS_HZ variable. This is needed for all CPUs where the default timer is running on anything else than 1000.

Signed-off-by Daniel Mack <[email protected]>
---
 net/net.c |   15 ++++++++++-----
 1 files changed, 10 insertions(+), 5 deletions(-)
net/net.c +10 -5
index 77e83b5..1c48236 100644
@@ -206,6 +206,11 @@ uchar		NetArpWaitPacketBuf[PKTSIZE_ALIGN + PKTALIGN];
 ulong		NetArpWaitTimerStart;
 int		NetArpWaitTry;
 
+static long get_timer_ms(long base)
+{
+	return get_timer(base) / (CONFIG_SYS_HZ / 1000);
+}
+
 void ArpRequest (void)
 {
 	int i;

@@ -256,7 +261,7 @@ void ArpTimeoutCheck(void)
 	if (!NetArpWaitPacketIP)
 		return;
 
-	t = get_timer(0);
+	t = get_timer_ms(0);
 
 	/* check for arp timeout */
 	if ((t - NetArpWaitTimerStart) > ARP_TIMEOUT) {

@@ -517,7 +522,7 @@ restart:
 		 *	Check for a timeout, and run the timeout handler
 		 *	if we have one.
 		 */
-		if (timeHandler && ((get_timer(0) - timeStart) > timeDelta)) {
+		if (timeHandler && ((get_timer_ms(0) - timeStart) > timeDelta)) {
 			thand_f *x;
 
 #if defined(CONFIG_MII) || defined(CONFIG_CMD_MII)

@@ -639,7 +644,7 @@ NetSetTimeout(ulong iv, thand_f * f)
 		timeHandler = (thand_f *)0;
 	} else {
 		timeHandler = f;
-		timeStart = get_timer(0);
+		timeStart = get_timer_ms(0);
 		timeDelta = iv;
 	}
 }

@@ -684,7 +689,7 @@ NetSendUDPPacket(uchar *ether, IPaddr_t dest, int dport, int sport, int len)
 
 		/* and do the ARP request */
 		NetArpWaitTry = 1;
-		NetArpWaitTimerStart = get_timer(0);
+		NetArpWaitTimerStart = get_timer_ms(0);
 		ArpRequest();
 		return 1;	/* waiting */
 	}

@@ -755,7 +760,7 @@ int PingSend(void)
 
 	/* and do the ARP request */
 	NetArpWaitTry = 1;
-	NetArpWaitTimerStart = get_timer(0);
+	NetArpWaitTimerStart = get_timer_ms(0);
 	ArpRequest();
 	return 1;	/* waiting */
 }
Signature · 1 line
1.5.6.5
NAK
#8 lore raw

PATCH 1/1 net/net.c: add get_timer_ms()

Dear Daniel Mack,

Quoted 5 lines · In message <[email protected]> you wrote:

Make timeout implementation in net/net.c take into account the CONFIG_SYS_HZ variable. This is needed for all CPUs where the default timer is running on anything else than 1000.

Signed-off-by Daniel Mack <[email protected]>

I reject this patch.

Quoted 8 lines
 net/net.c |   15 ++++++++++-----
 1 files changed, 10 insertions(+), 5 deletions(-)
net/net.c +0 -0
index 77e83b5..1c48236 100644

net/net.c uchar NetArpWaitPacketBuf[PKTSIZE_ALIGN + PKTALIGN]; · line 213

Quoted 8 lines
net/net.c +5 -0
@@ -206,6 +206,11 @@ uchar		NetArpWaitPacketBuf[PKTSIZE_ALIGN + PKTALIGN];
 ulong		NetArpWaitTimerStart;
 int		NetArpWaitTry;
 
+static long get_timer_ms(long base)
+{
+	return get_timer(base) / (CONFIG_SYS_HZ / 1000);
+}
+

This is by definition a NO-OP at best, and misleading and wrong otherwise. get_timer() is defined to return millisecond resolution, and CONFIG_SYS_HZ is supposed to be 1000.

So in a correct configuration get_timer_ms() is the same as
get_timer(), and if CONFIG_SYS_HZ is (incorrectly) not set to 1000 while get_timer() is implemented correctly, then get_timer_ms() willnot do what it claims to do.

Not to mention what happens if someone has CONFIG_SYS_HZ defined as 999, for example.

The whole aproach is broken - instead of coating bugs we should fix the cause of the bugs.

NAK.

Best regards,

Wolfgang Denk

Signature · 4 lines
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
I read part of it all the way through.
author
#9 lore raw

PATCH 1/1 net/net.c: add get_timer_ms()

Hi,

Quoted 11 lines · Wolfgang Denk
Quoted 11 lines
 net/net.c |   15 ++++++++++-----
 1 files changed, 10 insertions(+), 5 deletions(-)
net/net.c +0 -0
index 77e83b5..1c48236 100644
@@ -206,6 +206,11 @@ uchar		NetArpWaitPacketBuf[PKTSIZE_ALIGN + PKTALIGN];
 ulong		NetArpWaitTimerStart;
 int		NetArpWaitTry;
Quoted 9 lines

net/net.c uchar NetArpWaitPacketBuf[PKTSIZE_ALIGN + PKTALIGN]; · line 213

Quoted 5 lines
net/net.c +5 -0
+static long get_timer_ms(long base)
+{
+	return get_timer(base) / (CONFIG_SYS_HZ / 1000);
+}
+

This is by definition a NO-OP at best, and misleading and wrong otherwise. get_timer() is defined to return millisecond resolution, and CONFIG_SYS_HZ is supposed to be 1000.

The timer implementation (at least the one for PXA processors) assumes that the OSCR register increments 1000 times a second. Which it doesn't for PXA3xx variants. Hence, all functions from cpu/pxa/interrupts.c will behave entirely differently on a PXA270 compared to a PXA3xx, and so all code using this functions will break.

That's what I've experienced, and as I didn't find a proper place to fix it at a sane level, I fixed the problem locally. I agree that this might not be the best place, so I'll happily accept better proposals.

Quoted 2 lines

So in a correct configuration get_timer_ms() is the same as
get_timer(), and if CONFIG_SYS_HZ is (incorrectly) not set to 1000

Why is a CONFIG_SYS_HZ != 1000 considered incorrect? Or let me spin it that way: if that's incorrect, what does this variable exist for at all?

Quoted 2 lines

while get_timer() is implemented correctly, then get_timer_ms() willnot do what it claims to do.

What is get_timer() supposed to return, anyway? I didn't find any documentation about it and assumed that it straightly returns the primary system timer of the CPU (which it perfectly does for PXAs).

Quoted 2 lines

Not to mention what happens if someone has CONFIG_SYS_HZ defined as 999, for example.

Not sure whether I got your point here. If the system timer increments 999 times per second and CONFIG_SYS_HZ is set accordingly, my function does the right thing, doesn't it? I'm not up to any flamewar, I just want to understand where you see the problem.

As fas as I understand the big picture, a function like mine should exist somewhere in the code. Probably not in net/net.c, though.

Best regards,
Daniel

#10 lore raw

PATCH 1/1 net/net.c: add get_timer_ms()

Dear Daniel Mack,

Quoted 10 lines · In message <[email protected]> you wrote:
Quoted 4 lines

This is by definition a NO-OP at best, and misleading and wrong otherwise. get_timer() is defined to return millisecond resolution, and CONFIG_SYS_HZ is supposed to be 1000.

The timer implementation (at least the one for PXA processors) assumes that the OSCR register increments 1000 times a second. Which it doesn't for PXA3xx variants. Hence, all functions from cpu/pxa/interrupts.c will behave entirely differently on a PXA270 compared to a PXA3xx, and so all code using this functions will break.

So this is a bug, and needs to be fixed.

Quoted 2 lines

Why is a CONFIG_SYS_HZ != 1000 considered incorrect? Or let me spin it that way: if that's incorrect, what does this variable exist for at all?

It exists only for historic reasons, I used it to optimize between timer accuracy and overhead on slow (33 MHz) MP8xx systems some 8+ years ago. It has been considered a constant of 1000 ever since. Only some broken ports used it differently. Unfortunately this went unnoticed way too long.

Quoted 3 lines

What is get_timer() supposed to return, anyway? I didn't find any documentation about it and assumed that it straightly returns the primary system timer of the CPU (which it perfectly does for PXAs).

get_timer(base) returns the number of milliseconds since "base".

Quoted 7 lines
Quoted 2 lines

Not to mention what happens if someone has CONFIG_SYS_HZ defined as 999, for example.

Not sure whether I got your point here. If the system timer increments 999 times per second and CONFIG_SYS_HZ is set accordingly, my function does the right thing, doesn't it? I'm not up to any flamewar, I just want to understand where you see the problem.

It doesn't:

	return get_timer(base) / (CONFIG_SYS_HZ / 1000);

gives

	return get_timer(base) / (999 / 1000);

which gives

	return get_timer(base) / 0;

which gives a problem.

Quoted 2 lines

As fas as I understand the big picture, a function like mine should exist somewhere in the code. Probably not in net/net.c, though.

No, it should not exist at all. It is not needed for all correct implementations where get_timer() is already returning milliseconds.

Best regards,

Wolfgang Denk

Signature · 5 lines
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
Do not simplify the design of a program if a way can be found to make
it complex and wonderful.
author
#11 lore raw

PATCH 1/1 net/net.c: add get_timer_ms()

Quoted 7 lines · Wolfgang Denk
Quoted 5 lines

The timer implementation (at least the one for PXA processors) assumes that the OSCR register increments 1000 times a second. Which it doesn't for PXA3xx variants. Hence, all functions from cpu/pxa/interrupts.c will behave entirely differently on a PXA270 compared to a PXA3xx, and so all code using this functions will break.

So this is a bug, and needs to be fixed.

Ok. Do you want me to submit some "#ifdef MONAHANS" patch for the pxa ge_timer() function or is anyone working on major reworks for this anyway?

Best regards,
Daniel

#12 lore raw

PATCH 1/1 net/net.c: add get_timer_ms()

Dear Daniel Mack,

Quoted 6 lines · In message <[email protected]> you wrote:
Quoted 2 lines

So this is a bug, and needs to be fixed.

Ok. Do you want me to submit some "#ifdef MONAHANS" patch for the pxa ge_timer() function or is anyone working on major reworks for this anyway?

You best coordinate this with Jean-Christophe, the PXA custodian.

Best regards,

Wolfgang Denk

Signature · 7 lines
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
Software suppliers are trying to make their  software  packages  more
``user-friendly''.  .  .  .  Their best approach, so far, has been to
take all the old brochures, and stamp the words, ``user-friendly'' on
the cover.                                               - Bill Gates
author
#13 lore raw

PATCH 1/1 net/net.c: add get_timer_ms()

Quoted 5 lines · Wolfgang Denk
Quoted 3 lines

Ok. Do you want me to submit some "#ifdef MONAHANS" patch for the pxa ge_timer() function or is anyone working on major reworks for this anyway?

You best coordinate this with Jean-Christophe, the PXA custodian.

I guess he's reading this list? So I'll just sit and wait for a comment, also about the other ARM specific patches I sent in.

Thanks and best regards,
Daniel

#14 lore raw

PATCH 1/1 net/net.c: correct timeout function

In reply to Ben Warren

Dear Ben Warren,

Quoted 10 lines · In message <[email protected]> you wrote:

net/net.c uchar NetArpWaitPacketBuf[PKTSIZE_ALIGN + PKTALIGN]; · line 213

Quoted 7 lines
net/net.c +5 -0
+static long net_get_timer(long base)
+{
+	return get_timer(base) / (CONFIG_SYS_HZ / 1000);
+}
+

Please change this name to something more meaningful, maybe get_timer_ms(). Apart from that, looks good. If you re-submit soon I'll pull it in.

Please do not pull this. It just covers bugs elsewhere in the code.

We need to fix the real bugs, not put paint and wallpaper over it.

Best regards,

Wolfgang Denk

Signature · 5 lines
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
In accord with UNIX philosophy, Perl gives you enough  rope  to  hang
yourself.              - L. Wall & R. L. Schwartz, _Programming Perl_
#15 lore raw

PATCH 1/1 net/net.c: correct timeout function

Quoted 16 lines · Wolfgang Denk

Dear Ben Warren,

Quoted 11 lines · In message <[email protected]> you wrote:

net/net.c uchar NetArpWaitPacketBuf[PKTSIZE_ALIGN + PKTALIGN]; · line 213

Quoted 7 lines
net/net.c +5 -0
+static long net_get_timer(long base)
+{
+	return get_timer(base) / (CONFIG_SYS_HZ / 1000);
+}
+

Please change this name to something more meaningful, maybe get_timer_ms(). Apart from that, looks good. If you re-submit soon I'll pull it in.

Quoted 9 lines

Please do not pull this. It just covers bugs elsewhere in the code.

We need to fix the real bugs, not put paint and wallpaper over it.

Best regards,

Wolfgang Denk

OK