[U-Boot] [PATCH] net/net.c: correct timeout function

[U-Boot] [PATCH] net/net.c: correct timeout function

[Daniel Mack]

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 */
 }

[U-Boot] [PATCH] net/net.c: correct timeout function

[Daniel Mack]

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

[U-Boot] [PATCH] net/net.c: correct timeout function

[Ben Warren]

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

[U-Boot] [PATCH] net/net.c: correct timeout function

[Daniel Mack]

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

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

[U-Boot] [PATCH] net/net.c: correct timeout function

[Ben Warren]

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

[U-Boot] [PATCH] net/net.c: correct timeout function

[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 <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 */
 }

[U-Boot] [PATCH] net/net.c: correct timeout function

[Ben Warren]

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

[U-Boot] [PATCH] net/net.c: add get_timer_ms()

[Daniel Mack]

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 */
 }
-- 
1.5.6.5

[U-Boot] [PATCH] net/net.c: add get_timer_ms()

[Wolfgang Denk]

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

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

[U-Boot] [PATCH] net/net.c: add get_timer_ms()

[Daniel Mack]

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

[U-Boot] [PATCH] net/net.c: add get_timer_ms()

[Wolfgang Denk]

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

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

[U-Boot] [PATCH] net/net.c: add get_timer_ms()

[Daniel Mack]

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

[U-Boot] [PATCH] net/net.c: add get_timer_ms()

[Wolfgang Denk]

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

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

[U-Boot] [PATCH] net/net.c: add get_timer_ms()

[Daniel Mack]

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

[U-Boot] [PATCH] net/net.c: correct timeout function

[Wolfgang Denk]

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

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

[U-Boot] [PATCH] net/net.c: correct timeout function

[Ben Warren]

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