[U-Boot] [PATCH] 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
diff --git a/net/net.c b/net/net.c index 77e83b5..b9326de 100644 --- a/net/net.c +++ b/net/net.c @@ -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 */ }
On Fri, Nov 28, 2008 at 05:25:29PM +0100, Daniel Mack wrote:
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
Hi Daniel,
Daniel Mack wrote:
On Fri, Nov 28, 2008 at 05:25:29PM +0100, Daniel Mack wrote:
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)
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
Hi Ben,
On Tue, Dec 02, 2008 at 04:43:32PM -0800, Ben Warren wrote:
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':
- No signed-off-by
- No diffstat
- 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.
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
Daniel Mack wrote:
Hi Ben,
On Tue, Dec 02, 2008 at 04:43:32PM -0800, Ben Warren wrote:
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':
- No signed-off-by
- No diffstat
- 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.
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
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 daniel@caiaq.de
--- net.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/net/net.c b/net/net.c index 77e83b5..b9326de 100644 --- a/net/net.c +++ b/net/net.c @@ -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 */ }
Hi Daniel,
Daniel Mack wrote:
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.
Signed-off-by: Daniel Mack daniel@caiaq.de
net.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/net/net.c b/net/net.c index 77e83b5..b9326de 100644 --- a/net/net.c +++ b/net/net.c @@ -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
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 daniel@caiaq.de --- net/net.c | 15 ++++++++++----- 1 files changed, 10 insertions(+), 5 deletions(-)
diff --git a/net/net.c b/net/net.c index 77e83b5..1c48236 100644 --- a/net/net.c +++ b/net/net.c @@ -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 */ }
Dear Daniel Mack,
In message 20081205144031.GB1821@buzzloop.caiaq.de 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 daniel@caiaq.de
I reject this patch.
net/net.c | 15 ++++++++++----- 1 files changed, 10 insertions(+), 5 deletions(-)
diff --git a/net/net.c b/net/net.c index 77e83b5..1c48236 100644 --- a/net/net.c +++ b/net/net.c @@ -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
Hi,
On Fri, Dec 05, 2008 at 09:26:27PM +0100, Wolfgang Denk wrote:
net/net.c | 15 ++++++++++----- 1 files changed, 10 insertions(+), 5 deletions(-)
diff --git a/net/net.c b/net/net.c index 77e83b5..1c48236 100644 --- a/net/net.c +++ b/net/net.c @@ -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.
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.
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?
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).
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
Dear Daniel Mack,
In message 20081205204357.GD3940@buzzloop.caiaq.de you wrote:
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.
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.
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".
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.
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
On Fri, Dec 05, 2008 at 10:01:44PM +0100, Wolfgang Denk wrote:
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
Dear Daniel Mack,
In message 20081205210719.GA7036@buzzloop.caiaq.de you wrote:
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
On Fri, Dec 05, 2008 at 10:16:22PM +0100, Wolfgang Denk wrote:
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
Dear Ben Warren,
In message 4938CECF.6090901@gmail.com you wrote:
+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
Wolfgang Denk wrote:
Dear Ben Warren,
In message 4938CECF.6090901@gmail.com you wrote:
+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
OK
participants (3)
-
Ben Warren -
Daniel Mack -
Wolfgang Denk