* [U-Boot] [Resend RFC PATCH v2 0/1] MIPS64 timer bug
@ 2015-07-14 10:54 Chris Packham
2015-07-14 10:54 ` [U-Boot] [Resend RFC PATCH v2] mips: Use unsigned int when reading c0 registers Chris Packham
0 siblings, 1 reply; 6+ messages in thread
From: Chris Packham @ 2015-07-14 10:54 UTC (permalink / raw)
To: u-boot
We found this on a custom MIPS64 based board. Basically we were seeing
udelay and friends do odd things occasionally. The problem appears to be
when we read the c0 32-bit counter sometimes we get unexpected values
when this is returned as an unsigned long on a 64-bit system. I've
written a little test program (below) that demonstrates the what I think
is problem, it'll work on a x86_64 desktop. I don't know the ins and
outs of the C specifications to comment on if this is a compiler bug but
I still think we should be treating the c0 registers as unsigned
regardless.
$ cat test.c
#include <stdio.h>
int main(int argc, char *argv[])
{
int a;
unsigned long b;
a = 0x80000000;
b = a;
printf("sizeof(int) = %zu\n", sizeof(int));
printf("sizeof(unsigned long) = %zu\n", sizeof(unsigned long));
printf("a = %d\n", a);
printf("a = %x\n", a);
printf("b = %lu\n", b);
printf("b = %lx\n", b);
return 0;
}
Which yields the following output
$ ./test
sizeof(int) = 4
sizeof(unsigned long) = 8
a = -2147483648
a = 80000000
b = 18446744071562067968
b = ffffffff80000000
And for completeness
$ gcc --version
gcc (Ubuntu 4.8.4-2ubuntu1~14.04) 4.8.4
Copyright (C) 2013 Free Software Foundation, Inc.
Our embedded target is using
$ mips64-octeon-linux-gnu-gcc --version
mips64-octeon-linux-gnu-gcc (GCC) 4.7.0
Copyright (C) 2012 Free Software Foundation, Inc.
Changes in v2:
- Use Rob's current email address
Chris Packham (1):
mips: Use unsigned int when reading c0 registers
arch/mips/include/asm/mipsregs.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
--
2.5.0.rc0
^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] [Resend RFC PATCH v2] mips: Use unsigned int when reading c0 registers
2015-07-14 10:54 [U-Boot] [Resend RFC PATCH v2 0/1] MIPS64 timer bug Chris Packham
@ 2015-07-14 10:54 ` Chris Packham
2015-07-14 19:01 ` Daniel Schwierzeck
2015-07-18 9:29 ` Daniel Schwierzeck
0 siblings, 2 replies; 6+ messages in thread
From: Chris Packham @ 2015-07-14 10:54 UTC (permalink / raw)
To: u-boot
In commit a18a477 (MIPS: use common code from lib/time.c) MIPS platforms
started using common the common timer functions which are based around
the fact that many platforms have a 32-bit free running counter register
that can be used see commit 8dfafdd (Introduce common timer functions).
Even MIPS64 has such a 32-bit register (some have an additional 64-bit free
running counter, but that's something for another time).
The problem is that in __read_32bit_c0_register() we read the value from
this register into an _signed_ int and as it's returned up the call
chain to timer_read_counter() it gets assigned to an unsigned long. On a
32-bit system there is no problem. On a 64-bit system odd things happen,
sign extension seems to kick in and all of a sudden if the counter
register happens to have the MSb (i.e. the sign bit) set the negative
int gets sign extended into a very large unsigned long value. This in
turn throws out things from get_ticks() up.
Update __read_32bit_c0_register() and __read_32bit_c0_ctrl_register() to
use "unsigned int res;" instead of "int res;". There seems to be little
reason to treat these register values as signed. They are either
counters (which by definition are unsigned) or are made up of various
bit fields to be interpreted as per the CPU datasheet.
Reported-by: Sachin Surendran <sachin.surendran@alliedtelesis.co.nz>
Signed-off-by: Chris Packham <judge.packham@gmail.com>
---
Changes in v2:
- Use Rob's current email address
arch/mips/include/asm/mipsregs.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/mips/include/asm/mipsregs.h b/arch/mips/include/asm/mipsregs.h
index 3571e4f..c7a0849 100644
--- a/arch/mips/include/asm/mipsregs.h
+++ b/arch/mips/include/asm/mipsregs.h
@@ -594,7 +594,7 @@ do { \
*/
#define __read_32bit_c0_register(source, sel) \
-({ int __res; \
+({ unsigned int __res; \
if (sel == 0) \
__asm__ __volatile__( \
"mfc0\t%0, " #source "\n\t" \
@@ -676,7 +676,7 @@ do { \
* On RM7000/RM9000 these are uses to access cop0 set 1 registers
*/
#define __read_32bit_c0_ctrl_register(source) \
-({ int __res; \
+({ unsigned int __res; \
__asm__ __volatile__( \
"cfc0\t%0, " #source "\n\t" \
: "=r" (__res)); \
--
2.5.0.rc0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [U-Boot] [Resend RFC PATCH v2] mips: Use unsigned int when reading c0 registers
2015-07-14 10:54 ` [U-Boot] [Resend RFC PATCH v2] mips: Use unsigned int when reading c0 registers Chris Packham
@ 2015-07-14 19:01 ` Daniel Schwierzeck
2015-07-14 21:12 ` Chris Packham
2015-07-18 9:29 ` Daniel Schwierzeck
1 sibling, 1 reply; 6+ messages in thread
From: Daniel Schwierzeck @ 2015-07-14 19:01 UTC (permalink / raw)
To: u-boot
Hi Chris,
sorry for the delay.
Am 14.07.2015 um 12:54 schrieb Chris Packham:
> In commit a18a477 (MIPS: use common code from lib/time.c) MIPS platforms
> started using common the common timer functions which are based around
> the fact that many platforms have a 32-bit free running counter register
> that can be used see commit 8dfafdd (Introduce common timer functions).
>
> Even MIPS64 has such a 32-bit register (some have an additional 64-bit free
> running counter, but that's something for another time).
>
> The problem is that in __read_32bit_c0_register() we read the value from
> this register into an _signed_ int and as it's returned up the call
> chain to timer_read_counter() it gets assigned to an unsigned long. On a
> 32-bit system there is no problem. On a 64-bit system odd things happen,
> sign extension seems to kick in and all of a sudden if the counter
> register happens to have the MSb (i.e. the sign bit) set the negative
> int gets sign extended into a very large unsigned long value. This in
> turn throws out things from get_ticks() up.
>
> Update __read_32bit_c0_register() and __read_32bit_c0_ctrl_register() to
> use "unsigned int res;" instead of "int res;". There seems to be little
> reason to treat these register values as signed. They are either
> counters (which by definition are unsigned) or are made up of various
> bit fields to be interpreted as per the CPU datasheet.
I agree that those macros should always use unsigned int's. Also some
similar but newer macros use unsigned int's. But that header file is
imported from Linux kernel and I'd like to keep it in sync. Could you
post a similar patch to Linux MIPS mailing list? Maybe someone there
know why signed int's are used and if a change would have side-effects.
Thanks.
>
> Reported-by: Sachin Surendran <sachin.surendran@alliedtelesis.co.nz>
> Signed-off-by: Chris Packham <judge.packham@gmail.com>
>
> ---
>
> Changes in v2:
> - Use Rob's current email address
>
> arch/mips/include/asm/mipsregs.h | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/mips/include/asm/mipsregs.h b/arch/mips/include/asm/mipsregs.h
> index 3571e4f..c7a0849 100644
> --- a/arch/mips/include/asm/mipsregs.h
> +++ b/arch/mips/include/asm/mipsregs.h
> @@ -594,7 +594,7 @@ do { \
> */
>
> #define __read_32bit_c0_register(source, sel) \
> -({ int __res; \
> +({ unsigned int __res; \
> if (sel == 0) \
> __asm__ __volatile__( \
> "mfc0\t%0, " #source "\n\t" \
> @@ -676,7 +676,7 @@ do { \
> * On RM7000/RM9000 these are uses to access cop0 set 1 registers
> */
> #define __read_32bit_c0_ctrl_register(source) \
> -({ int __res; \
> +({ unsigned int __res; \
> __asm__ __volatile__( \
> "cfc0\t%0, " #source "\n\t" \
> : "=r" (__res)); \
>
--
- Daniel
^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] [Resend RFC PATCH v2] mips: Use unsigned int when reading c0 registers
2015-07-14 19:01 ` Daniel Schwierzeck
@ 2015-07-14 21:12 ` Chris Packham
2015-07-16 0:14 ` Chris Packham
0 siblings, 1 reply; 6+ messages in thread
From: Chris Packham @ 2015-07-14 21:12 UTC (permalink / raw)
To: u-boot
On Wed, Jul 15, 2015 at 7:01 AM, Daniel Schwierzeck
<daniel.schwierzeck@gmail.com> wrote:
> Hi Chris,
>
> sorry for the delay.
No problem. It only just occurred to me that it's probably peak
holiday season for people in the northern hemisphere.
> Am 14.07.2015 um 12:54 schrieb Chris Packham:
>> In commit a18a477 (MIPS: use common code from lib/time.c) MIPS platforms
>> started using common the common timer functions which are based around
>> the fact that many platforms have a 32-bit free running counter register
>> that can be used see commit 8dfafdd (Introduce common timer functions).
>>
>> Even MIPS64 has such a 32-bit register (some have an additional 64-bit free
>> running counter, but that's something for another time).
>>
>> The problem is that in __read_32bit_c0_register() we read the value from
>> this register into an _signed_ int and as it's returned up the call
>> chain to timer_read_counter() it gets assigned to an unsigned long. On a
>> 32-bit system there is no problem. On a 64-bit system odd things happen,
>> sign extension seems to kick in and all of a sudden if the counter
>> register happens to have the MSb (i.e. the sign bit) set the negative
>> int gets sign extended into a very large unsigned long value. This in
>> turn throws out things from get_ticks() up.
>>
>> Update __read_32bit_c0_register() and __read_32bit_c0_ctrl_register() to
>> use "unsigned int res;" instead of "int res;". There seems to be little
>> reason to treat these register values as signed. They are either
>> counters (which by definition are unsigned) or are made up of various
>> bit fields to be interpreted as per the CPU datasheet.
>
> I agree that those macros should always use unsigned int's. Also some
> similar but newer macros use unsigned int's. But that header file is
> imported from Linux kernel and I'd like to keep it in sync. Could you
> post a similar patch to Linux MIPS mailing list? Maybe someone there
> know why signed int's are used and if a change would have side-effects.
> Thanks.
OK I'll go looking there, they may have already fixed it.
>
>>
>> Reported-by: Sachin Surendran <sachin.surendran@alliedtelesis.co.nz>
>> Signed-off-by: Chris Packham <judge.packham@gmail.com>
>>
>> ---
>>
>> Changes in v2:
>> - Use Rob's current email address
>>
>> arch/mips/include/asm/mipsregs.h | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/mips/include/asm/mipsregs.h b/arch/mips/include/asm/mipsregs.h
>> index 3571e4f..c7a0849 100644
>> --- a/arch/mips/include/asm/mipsregs.h
>> +++ b/arch/mips/include/asm/mipsregs.h
>> @@ -594,7 +594,7 @@ do { \
>> */
>>
>> #define __read_32bit_c0_register(source, sel) \
>> -({ int __res; \
>> +({ unsigned int __res; \
>> if (sel == 0) \
>> __asm__ __volatile__( \
>> "mfc0\t%0, " #source "\n\t" \
>> @@ -676,7 +676,7 @@ do { \
>> * On RM7000/RM9000 these are uses to access cop0 set 1 registers
>> */
>> #define __read_32bit_c0_ctrl_register(source) \
>> -({ int __res; \
>> +({ unsigned int __res; \
>> __asm__ __volatile__( \
>> "cfc0\t%0, " #source "\n\t" \
>> : "=r" (__res)); \
>>
>
> --
> - Daniel
^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] [Resend RFC PATCH v2] mips: Use unsigned int when reading c0 registers
2015-07-14 21:12 ` Chris Packham
@ 2015-07-16 0:14 ` Chris Packham
0 siblings, 0 replies; 6+ messages in thread
From: Chris Packham @ 2015-07-16 0:14 UTC (permalink / raw)
To: u-boot
On Wed, Jul 15, 2015 at 9:12 AM, Chris Packham <judge.packham@gmail.com> wrote:
> On Wed, Jul 15, 2015 at 7:01 AM, Daniel Schwierzeck
> <daniel.schwierzeck@gmail.com> wrote:
>> Hi Chris,
>>
>> sorry for the delay.
>
> No problem. It only just occurred to me that it's probably peak
> holiday season for people in the northern hemisphere.
>
>> Am 14.07.2015 um 12:54 schrieb Chris Packham:
>>> In commit a18a477 (MIPS: use common code from lib/time.c) MIPS platforms
>>> started using common the common timer functions which are based around
>>> the fact that many platforms have a 32-bit free running counter register
>>> that can be used see commit 8dfafdd (Introduce common timer functions).
>>>
>>> Even MIPS64 has such a 32-bit register (some have an additional 64-bit free
>>> running counter, but that's something for another time).
>>>
>>> The problem is that in __read_32bit_c0_register() we read the value from
>>> this register into an _signed_ int and as it's returned up the call
>>> chain to timer_read_counter() it gets assigned to an unsigned long. On a
>>> 32-bit system there is no problem. On a 64-bit system odd things happen,
>>> sign extension seems to kick in and all of a sudden if the counter
>>> register happens to have the MSb (i.e. the sign bit) set the negative
>>> int gets sign extended into a very large unsigned long value. This in
>>> turn throws out things from get_ticks() up.
>>>
>>> Update __read_32bit_c0_register() and __read_32bit_c0_ctrl_register() to
>>> use "unsigned int res;" instead of "int res;". There seems to be little
>>> reason to treat these register values as signed. They are either
>>> counters (which by definition are unsigned) or are made up of various
>>> bit fields to be interpreted as per the CPU datasheet.
>>
>> I agree that those macros should always use unsigned int's. Also some
>> similar but newer macros use unsigned int's. But that header file is
>> imported from Linux kernel and I'd like to keep it in sync. Could you
>> post a similar patch to Linux MIPS mailing list? Maybe someone there
>> know why signed int's are used and if a change would have side-effects.
>> Thanks.
>
> OK I'll go looking there, they may have already fixed it.
>
Linux patch is working it's way through
http://www.linux-mips.org/archives/linux-mips/2015-07/msg00262.html.
Looks like I've missed the merge window for 4.2 so this is queued up
for 4.3. It doesn't appear to be in any public repo yet. If I wait for
the Linux 4.3 merge window to open before syncing this change to
u-boot I'll miss the 2015.10 merge window.
How do you want to proceed? Would it be possible to apply this patch
to u-boot now with the knowledge that it could be sync'd in the
future.
>>
>>>
>>> Reported-by: Sachin Surendran <sachin.surendran@alliedtelesis.co.nz>
>>> Signed-off-by: Chris Packham <judge.packham@gmail.com>
>>>
>>> ---
>>>
>>> Changes in v2:
>>> - Use Rob's current email address
>>>
>>> arch/mips/include/asm/mipsregs.h | 4 ++--
>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/arch/mips/include/asm/mipsregs.h b/arch/mips/include/asm/mipsregs.h
>>> index 3571e4f..c7a0849 100644
>>> --- a/arch/mips/include/asm/mipsregs.h
>>> +++ b/arch/mips/include/asm/mipsregs.h
>>> @@ -594,7 +594,7 @@ do { \
>>> */
>>>
>>> #define __read_32bit_c0_register(source, sel) \
>>> -({ int __res; \
>>> +({ unsigned int __res; \
>>> if (sel == 0) \
>>> __asm__ __volatile__( \
>>> "mfc0\t%0, " #source "\n\t" \
>>> @@ -676,7 +676,7 @@ do { \
>>> * On RM7000/RM9000 these are uses to access cop0 set 1 registers
>>> */
>>> #define __read_32bit_c0_ctrl_register(source) \
>>> -({ int __res; \
>>> +({ unsigned int __res; \
>>> __asm__ __volatile__( \
>>> "cfc0\t%0, " #source "\n\t" \
>>> : "=r" (__res)); \
>>>
>>
>> --
>> - Daniel
^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] [Resend RFC PATCH v2] mips: Use unsigned int when reading c0 registers
2015-07-14 10:54 ` [U-Boot] [Resend RFC PATCH v2] mips: Use unsigned int when reading c0 registers Chris Packham
2015-07-14 19:01 ` Daniel Schwierzeck
@ 2015-07-18 9:29 ` Daniel Schwierzeck
1 sibling, 0 replies; 6+ messages in thread
From: Daniel Schwierzeck @ 2015-07-18 9:29 UTC (permalink / raw)
To: u-boot
Am 14.07.2015 um 12:54 schrieb Chris Packham:
> In commit a18a477 (MIPS: use common code from lib/time.c) MIPS platforms
> started using common the common timer functions which are based around
> the fact that many platforms have a 32-bit free running counter register
> that can be used see commit 8dfafdd (Introduce common timer functions).
>
> Even MIPS64 has such a 32-bit register (some have an additional 64-bit free
> running counter, but that's something for another time).
>
> The problem is that in __read_32bit_c0_register() we read the value from
> this register into an _signed_ int and as it's returned up the call
> chain to timer_read_counter() it gets assigned to an unsigned long. On a
> 32-bit system there is no problem. On a 64-bit system odd things happen,
> sign extension seems to kick in and all of a sudden if the counter
> register happens to have the MSb (i.e. the sign bit) set the negative
> int gets sign extended into a very large unsigned long value. This in
> turn throws out things from get_ticks() up.
>
> Update __read_32bit_c0_register() and __read_32bit_c0_ctrl_register() to
> use "unsigned int res;" instead of "int res;". There seems to be little
> reason to treat these register values as signed. They are either
> counters (which by definition are unsigned) or are made up of various
> bit fields to be interpreted as per the CPU datasheet.
>
> Reported-by: Sachin Surendran <sachin.surendran@alliedtelesis.co.nz>
> Signed-off-by: Chris Packham <judge.packham@gmail.com>
>
> ---
>
> Changes in v2:
> - Use Rob's current email address
>
> arch/mips/include/asm/mipsregs.h | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
applied to u-boot-mips/next, thanks
--
- Daniel
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-07-18 9:29 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-07-14 10:54 [U-Boot] [Resend RFC PATCH v2 0/1] MIPS64 timer bug Chris Packham
2015-07-14 10:54 ` [U-Boot] [Resend RFC PATCH v2] mips: Use unsigned int when reading c0 registers Chris Packham
2015-07-14 19:01 ` Daniel Schwierzeck
2015-07-14 21:12 ` Chris Packham
2015-07-16 0:14 ` Chris Packham
2015-07-18 9:29 ` Daniel Schwierzeck
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.