* [PATCH 1/1] ARM : missing corrupted reg in __do_div_asm
@ 2016-03-28 4:19 Chen Gang
2016-03-29 10:19 ` Arnd Bergmann
0 siblings, 1 reply; 24+ messages in thread
From: Chen Gang @ 2016-03-28 4:19 UTC (permalink / raw)
To: linux-arm-kernel
__xl(R0 in little endian system, or R1 in big endian system) is corrupted
after calling __do_div64 and compiler is not informed about this in
macro __do_div_asm. If n is used again afterwards, __xl won't be
reloaded and n will contain incorrect value.
Signed-off-by: Chen Gang <gangchen@rdamicro.com>
Signed-off-by: Chen Gang <chengang.beijing@gmail.com>
---
arch/arm/include/asm/div64.h | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/arch/arm/include/asm/div64.h b/arch/arm/include/asm/div64.h
index e1f0776..1a6e91a 100644
--- a/arch/arm/include/asm/div64.h
+++ b/arch/arm/include/asm/div64.h
@@ -35,12 +35,14 @@ static inline uint32_t __div64_32(uint64_t *n, uint32_t base)
register unsigned long long __n asm("r0") = *n;
register unsigned long long __res asm("r2");
register unsigned int __rem asm(__xh);
+ register unsigned int __clobber asm(__xl);
asm( __asmeq("%0", __xh)
__asmeq("%1", "r2")
+ __asmeq("%3", "r0")
+ __asmeq("%4", "r4")
__asmeq("%2", "r0")
- __asmeq("%3", "r4")
"bl __do_div64"
- : "=r" (__rem), "=r" (__res)
+ : "=r" (__rem), "=r" (__res), "=r" (__clobber)
: "r" (__n), "r" (__base)
: "ip", "lr", "cc");
*n = __res;
--
2.4.3
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH 1/1] ARM : missing corrupted reg in __do_div_asm
2016-03-28 4:19 [PATCH 1/1] ARM : missing corrupted reg in __do_div_asm Chen Gang
@ 2016-03-29 10:19 ` Arnd Bergmann
2016-03-29 10:26 ` Dave Martin
0 siblings, 1 reply; 24+ messages in thread
From: Arnd Bergmann @ 2016-03-29 10:19 UTC (permalink / raw)
To: linux-arm-kernel
On Monday 28 March 2016 12:19:03 Chen Gang wrote:
> __xl(R0 in little endian system, or R1 in big endian system) is corrupted
> after calling __do_div64 and compiler is not informed about this in
> macro __do_div_asm. If n is used again afterwards, __xl won't be
> reloaded and n will contain incorrect value.
>
> Signed-off-by: Chen Gang <gangchen@rdamicro.com>
> Signed-off-by: Chen Gang <chengang.beijing@gmail.com>
> ---
How did you find this? Did you run into this problem on a live system
or see it through inspection?
> arch/arm/include/asm/div64.h | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm/include/asm/div64.h b/arch/arm/include/asm/div64.h
> index e1f0776..1a6e91a 100644
> --- a/arch/arm/include/asm/div64.h
> +++ b/arch/arm/include/asm/div64.h
> @@ -35,12 +35,14 @@ static inline uint32_t __div64_32(uint64_t *n, uint32_t base)
> register unsigned long long __n asm("r0") = *n;
> register unsigned long long __res asm("r2");
> register unsigned int __rem asm(__xh);
> + register unsigned int __clobber asm(__xl);
> asm( __asmeq("%0", __xh)
> __asmeq("%1", "r2")
> + __asmeq("%3", "r0")
> + __asmeq("%4", "r4")
> __asmeq("%2", "r0")
> - __asmeq("%3", "r4")
> "bl __do_div64"
> - : "=r" (__rem), "=r" (__res)
> + : "=r" (__rem), "=r" (__res), "=r" (__clobber)
> : "r" (__n), "r" (__base)
> : "ip", "lr", "cc");
> *n = __res;
Doesn't the clobber normally go in the third line along with
"ip" and "lr"?
Arnd
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 1/1] ARM : missing corrupted reg in __do_div_asm
2016-03-29 10:19 ` Arnd Bergmann
@ 2016-03-29 10:26 ` Dave Martin
2016-03-29 10:34 ` Russell King - ARM Linux
0 siblings, 1 reply; 24+ messages in thread
From: Dave Martin @ 2016-03-29 10:26 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Mar 29, 2016 at 12:19:49PM +0200, Arnd Bergmann wrote:
> On Monday 28 March 2016 12:19:03 Chen Gang wrote:
> > __xl(R0 in little endian system, or R1 in big endian system) is corrupted
> > after calling __do_div64 and compiler is not informed about this in
> > macro __do_div_asm. If n is used again afterwards, __xl won't be
> > reloaded and n will contain incorrect value.
> >
> > Signed-off-by: Chen Gang <gangchen@rdamicro.com>
> > Signed-off-by: Chen Gang <chengang.beijing@gmail.com>
> > ---
>
> How did you find this? Did you run into this problem on a live system
> or see it through inspection?
>
> > arch/arm/include/asm/div64.h | 6 ++++--
> > 1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/arm/include/asm/div64.h b/arch/arm/include/asm/div64.h
> > index e1f0776..1a6e91a 100644
> > --- a/arch/arm/include/asm/div64.h
> > +++ b/arch/arm/include/asm/div64.h
> > @@ -35,12 +35,14 @@ static inline uint32_t __div64_32(uint64_t *n, uint32_t base)
> > register unsigned long long __n asm("r0") = *n;
> > register unsigned long long __res asm("r2");
> > register unsigned int __rem asm(__xh);
> > + register unsigned int __clobber asm(__xl);
> > asm( __asmeq("%0", __xh)
> > __asmeq("%1", "r2")
> > + __asmeq("%3", "r0")
> > + __asmeq("%4", "r4")
> > __asmeq("%2", "r0")
> > - __asmeq("%3", "r4")
> > "bl __do_div64"
> > - : "=r" (__rem), "=r" (__res)
> > + : "=r" (__rem), "=r" (__res), "=r" (__clobber)
> > : "r" (__n), "r" (__base)
> > : "ip", "lr", "cc");
> > *n = __res;
>
> Doesn't the clobber normally go in the third line along with
> "ip" and "lr"?
Since __xl is not used for any real argument to the asm, I think
we can just add __xl to the clobber list directly, without needing
to introduce an extra register variable ... no?
Cheers
---Dave
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 1/1] ARM : missing corrupted reg in __do_div_asm
2016-03-29 10:26 ` Dave Martin
@ 2016-03-29 10:34 ` Russell King - ARM Linux
2016-03-29 10:56 ` Dave Martin
2016-03-29 10:58 ` [PATCH 1/1] ARM : missing corrupted reg in __do_div_asm 陈刚(Gangchen)
0 siblings, 2 replies; 24+ messages in thread
From: Russell King - ARM Linux @ 2016-03-29 10:34 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Mar 29, 2016 at 11:26:05AM +0100, Dave Martin wrote:
> On Tue, Mar 29, 2016 at 12:19:49PM +0200, Arnd Bergmann wrote:
> > On Monday 28 March 2016 12:19:03 Chen Gang wrote:
> > > __xl(R0 in little endian system, or R1 in big endian system) is corrupted
> > > after calling __do_div64 and compiler is not informed about this in
> > > macro __do_div_asm. If n is used again afterwards, __xl won't be
> > > reloaded and n will contain incorrect value.
> > >
> > > Signed-off-by: Chen Gang <gangchen@rdamicro.com>
> > > Signed-off-by: Chen Gang <chengang.beijing@gmail.com>
> > > ---
> >
> > How did you find this? Did you run into this problem on a live system
> > or see it through inspection?
> >
> > > arch/arm/include/asm/div64.h | 6 ++++--
> > > 1 file changed, 4 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/arch/arm/include/asm/div64.h b/arch/arm/include/asm/div64.h
> > > index e1f0776..1a6e91a 100644
> > > --- a/arch/arm/include/asm/div64.h
> > > +++ b/arch/arm/include/asm/div64.h
> > > @@ -35,12 +35,14 @@ static inline uint32_t __div64_32(uint64_t *n, uint32_t base)
> > > register unsigned long long __n asm("r0") = *n;
> > > register unsigned long long __res asm("r2");
> > > register unsigned int __rem asm(__xh);
> > > + register unsigned int __clobber asm(__xl);
> > > asm( __asmeq("%0", __xh)
> > > __asmeq("%1", "r2")
> > > + __asmeq("%3", "r0")
> > > + __asmeq("%4", "r4")
> > > __asmeq("%2", "r0")
> > > - __asmeq("%3", "r4")
> > > "bl __do_div64"
> > > - : "=r" (__rem), "=r" (__res)
> > > + : "=r" (__rem), "=r" (__res), "=r" (__clobber)
> > > : "r" (__n), "r" (__base)
> > > : "ip", "lr", "cc");
> > > *n = __res;
> >
> > Doesn't the clobber normally go in the third line along with
> > "ip" and "lr"?
>
> Since __xl is not used for any real argument to the asm, I think
> we can just add __xl to the clobber list directly, without needing
> to introduce an extra register variable ... no?
No, you can't. The clobber list is not allowed to specify registers
that may be used for input or output operands, and since __xl may be
r0, and __n _is_ r0, you can't specify r0 in the clobber list.
--
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 1/1] ARM : missing corrupted reg in __do_div_asm
2016-03-29 10:34 ` Russell King - ARM Linux
@ 2016-03-29 10:56 ` Dave Martin
2016-03-30 3:27 ` 陈刚(Gangchen)
2016-03-29 10:58 ` [PATCH 1/1] ARM : missing corrupted reg in __do_div_asm 陈刚(Gangchen)
1 sibling, 1 reply; 24+ messages in thread
From: Dave Martin @ 2016-03-29 10:56 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Mar 29, 2016 at 11:34:18AM +0100, Russell King - ARM Linux wrote:
> On Tue, Mar 29, 2016 at 11:26:05AM +0100, Dave Martin wrote:
> > On Tue, Mar 29, 2016 at 12:19:49PM +0200, Arnd Bergmann wrote:
> > > On Monday 28 March 2016 12:19:03 Chen Gang wrote:
> > > > __xl(R0 in little endian system, or R1 in big endian system) is corrupted
> > > > after calling __do_div64 and compiler is not informed about this in
> > > > macro __do_div_asm. If n is used again afterwards, __xl won't be
> > > > reloaded and n will contain incorrect value.
> > > >
> > > > Signed-off-by: Chen Gang <gangchen@rdamicro.com>
> > > > Signed-off-by: Chen Gang <chengang.beijing@gmail.com>
> > > > ---
> > >
> > > How did you find this? Did you run into this problem on a live system
> > > or see it through inspection?
> > >
> > > > arch/arm/include/asm/div64.h | 6 ++++--
> > > > 1 file changed, 4 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/arch/arm/include/asm/div64.h b/arch/arm/include/asm/div64.h
> > > > index e1f0776..1a6e91a 100644
> > > > --- a/arch/arm/include/asm/div64.h
> > > > +++ b/arch/arm/include/asm/div64.h
> > > > @@ -35,12 +35,14 @@ static inline uint32_t __div64_32(uint64_t *n, uint32_t base)
> > > > register unsigned long long __n asm("r0") = *n;
> > > > register unsigned long long __res asm("r2");
> > > > register unsigned int __rem asm(__xh);
> > > > + register unsigned int __clobber asm(__xl);
> > > > asm( __asmeq("%0", __xh)
> > > > __asmeq("%1", "r2")
> > > > + __asmeq("%3", "r0")
> > > > + __asmeq("%4", "r4")
> > > > __asmeq("%2", "r0")
> > > > - __asmeq("%3", "r4")
> > > > "bl __do_div64"
> > > > - : "=r" (__rem), "=r" (__res)
> > > > + : "=r" (__rem), "=r" (__res), "=r" (__clobber)
> > > > : "r" (__n), "r" (__base)
> > > > : "ip", "lr", "cc");
> > > > *n = __res;
> > >
> > > Doesn't the clobber normally go in the third line along with
> > > "ip" and "lr"?
> >
> > Since __xl is not used for any real argument to the asm, I think
> > we can just add __xl to the clobber list directly, without needing
> > to introduce an extra register variable ... no?
>
> No, you can't. The clobber list is not allowed to specify registers
> that may be used for input or output operands, and since __xl may be
> r0, and __n _is_ r0, you can't specify r0 in the clobber list.
Hmm, you're right -- in which case the change looks reasonable.
I wonder whether the following would be cleaner than having these
aliased arguments:
asm( /* ... */
"bl __do_div64"
: "+r" (__n), "=r" (__res)
: "r" (__base)
: "ip", "lr", "cc");
*n = __res;
return __n >> 32;
(providing that GCC doesn't make a mess of the "easy" shift).
Cheers
---Dave
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 1/1] ARM : missing corrupted reg in __do_div_asm
2016-03-29 10:34 ` Russell King - ARM Linux
2016-03-29 10:56 ` Dave Martin
@ 2016-03-29 10:58 ` 陈刚(Gangchen)
1 sibling, 0 replies; 24+ messages in thread
From: 陈刚(Gangchen) @ 2016-03-29 10:58 UTC (permalink / raw)
To: linux-arm-kernel
On 03/29/2016 06:34 PM, Russell King - ARM Linux wrote:
> On Tue, Mar 29, 2016 at 11:26:05AM +0100, Dave Martin wrote:
>> On Tue, Mar 29, 2016 at 12:19:49PM +0200, Arnd Bergmann wrote:
>>> On Monday 28 March 2016 12:19:03 Chen Gang wrote:
>>>> __xl(R0 in little endian system, or R1 in big endian system) is corrupted
>>>> after calling __do_div64 and compiler is not informed about this in
>>>> macro __do_div_asm. If n is used again afterwards, __xl won't be
>>>> reloaded and n will contain incorrect value.
>>>>
>>>> Signed-off-by: Chen Gang <gangchen@rdamicro.com>
>>>> Signed-off-by: Chen Gang <chengang.beijing@gmail.com>
>>>> ---
>>> How did you find this? Did you run into this problem on a live system
>>> or see it through inspection?
>>>
We (in RDAMicro) found the problem when we added TLC nand support in
MTD driver.
I submit this patch several time last year.
And this patch also fix one problems of XFS:
http://permalink.gmane.org/gmane.comp.file-systems.xfs.general/69224
>>>> arch/arm/include/asm/div64.h | 6 ++++--
>>>> 1 file changed, 4 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/arch/arm/include/asm/div64.h b/arch/arm/include/asm/div64.h
>>>> index e1f0776..1a6e91a 100644
>>>> --- a/arch/arm/include/asm/div64.h
>>>> +++ b/arch/arm/include/asm/div64.h
>>>> @@ -35,12 +35,14 @@ static inline uint32_t __div64_32(uint64_t *n, uint32_t base)
>>>> register unsigned long long __n asm("r0") = *n;
>>>> register unsigned long long __res asm("r2");
>>>> register unsigned int __rem asm(__xh);
>>>> + register unsigned int __clobber asm(__xl);
>>>> asm( __asmeq("%0", __xh)
>>>> __asmeq("%1", "r2")
>>>> + __asmeq("%3", "r0")
>>>> + __asmeq("%4", "r4")
>>>> __asmeq("%2", "r0")
>>>> - __asmeq("%3", "r4")
>>>> "bl __do_div64"
>>>> - : "=r" (__rem), "=r" (__res)
>>>> + : "=r" (__rem), "=r" (__res), "=r" (__clobber)
>>>> : "r" (__n), "r" (__base)
>>>> : "ip", "lr", "cc");
>>>> *n = __res;
>>> Doesn't the clobber normally go in the third line along with
>>> "ip" and "lr"?
>> Since __xl is not used for any real argument to the asm, I think
>> we can just add __xl to the clobber list directly, without needing
>> to introduce an extra register variable ... no?
> No, you can't. The clobber list is not allowed to specify registers
> that may be used for input or output operands, and since __xl may be
> r0, and __n _is_ r0, you can't specify r0 in the clobber list.
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 1/1] ARM : missing corrupted reg in __do_div_asm
2016-03-29 10:56 ` Dave Martin
@ 2016-03-30 3:27 ` 陈刚(Gangchen)
2016-03-30 14:07 ` Dave Martin
0 siblings, 1 reply; 24+ messages in thread
From: 陈刚(Gangchen) @ 2016-03-30 3:27 UTC (permalink / raw)
To: linux-arm-kernel
On 03/29/2016 06:56 PM, Dave Martin wrote:
> On Tue, Mar 29, 2016 at 11:34:18AM +0100, Russell King - ARM Linux wrote:
>> On Tue, Mar 29, 2016 at 11:26:05AM +0100, Dave Martin wrote:
>>> On Tue, Mar 29, 2016 at 12:19:49PM +0200, Arnd Bergmann wrote:
>>>> On Monday 28 March 2016 12:19:03 Chen Gang wrote:
>>>>> __xl(R0 in little endian system, or R1 in big endian system) is corrupted
>>>>> after calling __do_div64 and compiler is not informed about this in
>>>>> macro __do_div_asm. If n is used again afterwards, __xl won't be
>>>>> reloaded and n will contain incorrect value.
>>>>>
>>>>> Signed-off-by: Chen Gang <gangchen@rdamicro.com>
>>>>> Signed-off-by: Chen Gang <chengang.beijing@gmail.com>
>>>>> ---
>>>> How did you find this? Did you run into this problem on a live system
>>>> or see it through inspection?
>>>>
>>>>> arch/arm/include/asm/div64.h | 6 ++++--
>>>>> 1 file changed, 4 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/arch/arm/include/asm/div64.h b/arch/arm/include/asm/div64.h
>>>>> index e1f0776..1a6e91a 100644
>>>>> --- a/arch/arm/include/asm/div64.h
>>>>> +++ b/arch/arm/include/asm/div64.h
>>>>> @@ -35,12 +35,14 @@ static inline uint32_t __div64_32(uint64_t *n, uint32_t base)
>>>>> register unsigned long long __n asm("r0") = *n;
>>>>> register unsigned long long __res asm("r2");
>>>>> register unsigned int __rem asm(__xh);
>>>>> + register unsigned int __clobber asm(__xl);
>>>>> asm( __asmeq("%0", __xh)
>>>>> __asmeq("%1", "r2")
>>>>> + __asmeq("%3", "r0")
>>>>> + __asmeq("%4", "r4")
>>>>> __asmeq("%2", "r0")
>>>>> - __asmeq("%3", "r4")
>>>>> "bl __do_div64"
>>>>> - : "=r" (__rem), "=r" (__res)
>>>>> + : "=r" (__rem), "=r" (__res), "=r" (__clobber)
>>>>> : "r" (__n), "r" (__base)
>>>>> : "ip", "lr", "cc");
>>>>> *n = __res;
>>>> Doesn't the clobber normally go in the third line along with
>>>> "ip" and "lr"?
>>> Since __xl is not used for any real argument to the asm, I think
>>> we can just add __xl to the clobber list directly, without needing
>>> to introduce an extra register variable ... no?
>> No, you can't. The clobber list is not allowed to specify registers
>> that may be used for input or output operands, and since __xl may be
>> r0, and __n _is_ r0, you can't specify r0 in the clobber list.
> Hmm, you're right -- in which case the change looks reasonable.
>
> I wonder whether the following would be cleaner than having these
> aliased arguments:
>
> asm( /* ... */
> "bl __do_div64"
> : "+r" (__n), "=r" (__res)
> : "r" (__base)
> : "ip", "lr", "cc");
> *n = __res;
> return __n >> 32;
>
> (providing that GCC doesn't make a mess of the "easy" shift).
I tried your proposal. It didn't make any difference: this is inline
function and gcc just ignores your trick.
>
> Cheers
> ---Dave
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 1/1] ARM : missing corrupted reg in __do_div_asm
2016-03-30 3:27 ` 陈刚(Gangchen)
@ 2016-03-30 14:07 ` Dave Martin
2016-03-31 7:56 ` 陈刚(Gangchen)
0 siblings, 1 reply; 24+ messages in thread
From: Dave Martin @ 2016-03-30 14:07 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Mar 30, 2016 at 03:27:01AM +0000, ??(Gangchen) wrote:
> On 03/29/2016 06:56 PM, Dave Martin wrote:
> > On Tue, Mar 29, 2016 at 11:34:18AM +0100, Russell King - ARM Linux wrote:
> >> On Tue, Mar 29, 2016 at 11:26:05AM +0100, Dave Martin wrote:
> >>> On Tue, Mar 29, 2016 at 12:19:49PM +0200, Arnd Bergmann wrote:
> >>>> On Monday 28 March 2016 12:19:03 Chen Gang wrote:
> >>>>> __xl(R0 in little endian system, or R1 in big endian system) is corrupted
> >>>>> after calling __do_div64 and compiler is not informed about this in
> >>>>> macro __do_div_asm. If n is used again afterwards, __xl won't be
> >>>>> reloaded and n will contain incorrect value.
> >>>>>
> >>>>> Signed-off-by: Chen Gang <gangchen@rdamicro.com>
> >>>>> Signed-off-by: Chen Gang <chengang.beijing@gmail.com>
> >>>>> ---
> >>>> How did you find this? Did you run into this problem on a live system
> >>>> or see it through inspection?
> >>>>
> >>>>> arch/arm/include/asm/div64.h | 6 ++++--
> >>>>> 1 file changed, 4 insertions(+), 2 deletions(-)
> >>>>>
> >>>>> diff --git a/arch/arm/include/asm/div64.h b/arch/arm/include/asm/div64.h
> >>>>> index e1f0776..1a6e91a 100644
> >>>>> --- a/arch/arm/include/asm/div64.h
> >>>>> +++ b/arch/arm/include/asm/div64.h
> >>>>> @@ -35,12 +35,14 @@ static inline uint32_t __div64_32(uint64_t *n, uint32_t base)
> >>>>> register unsigned long long __n asm("r0") = *n;
> >>>>> register unsigned long long __res asm("r2");
> >>>>> register unsigned int __rem asm(__xh);
> >>>>> + register unsigned int __clobber asm(__xl);
> >>>>> asm( __asmeq("%0", __xh)
> >>>>> __asmeq("%1", "r2")
> >>>>> + __asmeq("%3", "r0")
> >>>>> + __asmeq("%4", "r4")
> >>>>> __asmeq("%2", "r0")
> >>>>> - __asmeq("%3", "r4")
> >>>>> "bl __do_div64"
> >>>>> - : "=r" (__rem), "=r" (__res)
> >>>>> + : "=r" (__rem), "=r" (__res), "=r" (__clobber)
> >>>>> : "r" (__n), "r" (__base)
> >>>>> : "ip", "lr", "cc");
> >>>>> *n = __res;
> >>>> Doesn't the clobber normally go in the third line along with
> >>>> "ip" and "lr"?
> >>> Since __xl is not used for any real argument to the asm, I think
> >>> we can just add __xl to the clobber list directly, without needing
> >>> to introduce an extra register variable ... no?
> >> No, you can't. The clobber list is not allowed to specify registers
> >> that may be used for input or output operands, and since __xl may be
> >> r0, and __n _is_ r0, you can't specify r0 in the clobber list.
> > Hmm, you're right -- in which case the change looks reasonable.
> >
> > I wonder whether the following would be cleaner than having these
> > aliased arguments:
> >
> > asm( /* ... */
> > "bl __do_div64"
> > : "+r" (__n), "=r" (__res)
> > : "r" (__base)
> > : "ip", "lr", "cc");
> > *n = __res;
> > return __n >> 32;
> >
> > (providing that GCC doesn't make a mess of the "easy" shift).
> I tried your proposal. It didn't make any difference: this is inline
> function and gcc just ignores your trick.
What doesn't work for you when using this method?
Why does the fact that this is an inline function make a difference?
Cheers
---Dave
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 1/1] ARM : missing corrupted reg in __do_div_asm
2016-03-30 14:07 ` Dave Martin
@ 2016-03-31 7:56 ` 陈刚(Gangchen)
2016-03-31 10:30 ` Dave Martin
0 siblings, 1 reply; 24+ messages in thread
From: 陈刚(Gangchen) @ 2016-03-31 7:56 UTC (permalink / raw)
To: linux-arm-kernel
On 03/30/2016 10:07 PM, Dave Martin wrote:
> On Wed, Mar 30, 2016 at 03:27:01AM +0000, ??(Gangchen) wrote:
>> On 03/29/2016 06:56 PM, Dave Martin wrote:
>>> On Tue, Mar 29, 2016 at 11:34:18AM +0100, Russell King - ARM Linux wrote:
>>>> On Tue, Mar 29, 2016 at 11:26:05AM +0100, Dave Martin wrote:
>>>>> On Tue, Mar 29, 2016 at 12:19:49PM +0200, Arnd Bergmann wrote:
>>>>>> On Monday 28 March 2016 12:19:03 Chen Gang wrote:
>>>>>>> __xl(R0 in little endian system, or R1 in big endian system) is corrupted
>>>>>>> after calling __do_div64 and compiler is not informed about this in
>>>>>>> macro __do_div_asm. If n is used again afterwards, __xl won't be
>>>>>>> reloaded and n will contain incorrect value.
>>>>>>>
>>>>>>> Signed-off-by: Chen Gang <gangchen@rdamicro.com>
>>>>>>> Signed-off-by: Chen Gang <chengang.beijing@gmail.com>
>>>>>>> ---
>>>>>> How did you find this? Did you run into this problem on a live system
>>>>>> or see it through inspection?
>>>>>>
>>>>>>> arch/arm/include/asm/div64.h | 6 ++++--
>>>>>>> 1 file changed, 4 insertions(+), 2 deletions(-)
>>>>>>>
>>>>>>> diff --git a/arch/arm/include/asm/div64.h b/arch/arm/include/asm/div64.h
>>>>>>> index e1f0776..1a6e91a 100644
>>>>>>> --- a/arch/arm/include/asm/div64.h
>>>>>>> +++ b/arch/arm/include/asm/div64.h
>>>>>>> @@ -35,12 +35,14 @@ static inline uint32_t __div64_32(uint64_t *n, uint32_t base)
>>>>>>> register unsigned long long __n asm("r0") = *n;
>>>>>>> register unsigned long long __res asm("r2");
>>>>>>> register unsigned int __rem asm(__xh);
>>>>>>> + register unsigned int __clobber asm(__xl);
>>>>>>> asm( __asmeq("%0", __xh)
>>>>>>> __asmeq("%1", "r2")
>>>>>>> + __asmeq("%3", "r0")
>>>>>>> + __asmeq("%4", "r4")
>>>>>>> __asmeq("%2", "r0")
>>>>>>> - __asmeq("%3", "r4")
>>>>>>> "bl __do_div64"
>>>>>>> - : "=r" (__rem), "=r" (__res)
>>>>>>> + : "=r" (__rem), "=r" (__res), "=r" (__clobber)
>>>>>>> : "r" (__n), "r" (__base)
>>>>>>> : "ip", "lr", "cc");
>>>>>>> *n = __res;
>>>>>> Doesn't the clobber normally go in the third line along with
>>>>>> "ip" and "lr"?
>>>>> Since __xl is not used for any real argument to the asm, I think
>>>>> we can just add __xl to the clobber list directly, without needing
>>>>> to introduce an extra register variable ... no?
>>>> No, you can't. The clobber list is not allowed to specify registers
>>>> that may be used for input or output operands, and since __xl may be
>>>> r0, and __n _is_ r0, you can't specify r0 in the clobber list.
>>> Hmm, you're right -- in which case the change looks reasonable.
>>>
>>> I wonder whether the following would be cleaner than having these
>>> aliased arguments:
>>>
>>> asm( /* ... */
>>> "bl __do_div64"
>>> : "+r" (__n), "=r" (__res)
>>> : "r" (__base)
>>> : "ip", "lr", "cc");
>>> *n = __res;
>>> return __n >> 32;
>>>
>>> (providing that GCC doesn't make a mess of the "easy" shift).
>> I tried your proposal. It didn't make any difference: this is inline
>> function and gcc just ignores your trick.
> What doesn't work for you when using this method?
>
> Why does the fact that this is an inline function make a difference?
With the help of other colleagues, I understand your proposal now.
I create a patch and I can verify that it works!
Should I submit it, as it seems better than this one I sent?
Cheers ---Dave
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 1/1] ARM : missing corrupted reg in __do_div_asm
2016-03-31 7:56 ` 陈刚(Gangchen)
@ 2016-03-31 10:30 ` Dave Martin
2016-03-31 11:20 ` 陈刚(Gangchen)
2016-03-31 11:41 ` [PATCH v2 " Chen Gang
0 siblings, 2 replies; 24+ messages in thread
From: Dave Martin @ 2016-03-31 10:30 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, Mar 31, 2016 at 07:56:05AM +0000, ??(Gangchen) wrote:
> On 03/30/2016 10:07 PM, Dave Martin wrote:
> > On Wed, Mar 30, 2016 at 03:27:01AM +0000, ??(Gangchen) wrote:
> >> On 03/29/2016 06:56 PM, Dave Martin wrote:
[...]
> >>> I wonder whether the following would be cleaner than having these
> >>> aliased arguments:
> >>>
> >>> asm( /* ... */
> >>> "bl __do_div64"
> >>> : "+r" (__n), "=r" (__res)
> >>> : "r" (__base)
> >>> : "ip", "lr", "cc");
> >>> *n = __res;
> >>> return __n >> 32;
> >>>
> >>> (providing that GCC doesn't make a mess of the "easy" shift).
> >> I tried your proposal. It didn't make any difference: this is inline
> >> function and gcc just ignores your trick.
> > What doesn't work for you when using this method?
> >
> > Why does the fact that this is an inline function make a difference?
> With the help of other colleagues, I understand your proposal now.
> I create a patch and I can verify that it works!
Ah, OK. I was wondering whether I made a mistake somewhere.
> Should I submit it, as it seems better than this one I sent?
It's up to you -- I think my approach is a bit cleaner, but your
approach worked too and is not vulnerable to compilers that generate
silly code for (uint64_t) >> 32.
Note that I only tested my code for little endian -- it should do the
right thing for BE, but I recommend that you try it and examine the
generated code, to make sure.
Cheers
---Date
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 1/1] ARM : missing corrupted reg in __do_div_asm
2016-03-31 10:30 ` Dave Martin
@ 2016-03-31 11:20 ` 陈刚(Gangchen)
2016-03-31 11:41 ` Dave Martin
2016-03-31 11:41 ` [PATCH v2 " Chen Gang
1 sibling, 1 reply; 24+ messages in thread
From: 陈刚(Gangchen) @ 2016-03-31 11:20 UTC (permalink / raw)
To: linux-arm-kernel
On 03/31/2016 06:30 PM, Dave Martin wrote:
> On Thu, Mar 31, 2016 at 07:56:05AM +0000, ??(Gangchen) wrote:
>> On 03/30/2016 10:07 PM, Dave Martin wrote:
>>> On Wed, Mar 30, 2016 at 03:27:01AM +0000, ??(Gangchen) wrote:
>>>> On 03/29/2016 06:56 PM, Dave Martin wrote:
> [...]
>
>>>>> I wonder whether the following would be cleaner than having these
>>>>> aliased arguments:
>>>>>
>>>>> asm( /* ... */
>>>>> "bl __do_div64"
>>>>> : "+r" (__n), "=r" (__res)
>>>>> : "r" (__base)
>>>>> : "ip", "lr", "cc");
>>>>> *n = __res;
>>>>> return __n >> 32;
>>>>>
>>>>> (providing that GCC doesn't make a mess of the "easy" shift).
>>>> I tried your proposal. It didn't make any difference: this is inline
>>>> function and gcc just ignores your trick.
>>> What doesn't work for you when using this method?
>>>
>>> Why does the fact that this is an inline function make a difference?
>> With the help of other colleagues, I understand your proposal now.
>> I create a patch and I can verify that it works!
> Ah, OK. I was wondering whether I made a mistake somewhere.
>
>> Should I submit it, as it seems better than this one I sent?
> It's up to you -- I think my approach is a bit cleaner, but your
> approach worked too and is not vulnerable to compilers that generate
> silly code for (uint64_t) >> 32.
(uint64_t) >> 32 is pretty much the standard way to get upper half of an
uint64 variable, compiler should not
generate silly code for this.
> Note that I only tested my code for little endian -- it should do the
> right thing for BE, but I recommend that you try it and examine the
> generated code, to make sure.
I don't have a BE system to test, but I did check assembly code
generated for BE system and didn't find
anything wrong.
I will update the new patch soon.
The following is my test code and assembly dump for BE system of the
function.
typedef unsigned long long ull;
ull mydiv64y(ull tt, unsigned base, unsigned *p)
{
ull t = tt;
*p = do_div(t, base);
*p = do_div(tt, base+1);
return tt;
}
/mnt/2nd_disk/rdaMicro/aosp_4.4/test_modules/div_test/.tmp_test_div.o:
file format elf32-bigarm
Disassembly of section .text:
00000000 <mydiv64y>:
0: e92d 47f0 stmdb sp!, {r4, r5, r6, r7, r8, r9, sl, lr}
4: 4607 mov r7, r0
6: 2600 movs r6, #0
8: 4698 mov r8, r3
a: ea57 0306 orrs.w r3, r7, r6
e: 4681 mov r9, r0
10: 460d mov r5, r1
12: 4692 mov sl, r2
14: d111 bne.n 3a <mydiv64y+0x3a>
16: f102 0401 add.w r4, r2, #1
1a: 4608 mov r0, r1
1c: 4621 mov r1, r4
1e: f7ff fffe bl 0 <__aeabi_uidivmod>
22: 4628 mov r0, r5
24: 460f mov r7, r1
26: 4621 mov r1, r4
28: f7ff fffe bl 0 <__aeabi_uidiv>
2c: 4632 mov r2, r6
2e: 4601 mov r1, r0
30: f8c8 7000 str.w r7, [r8]
34: 4610 mov r0, r2
36: e8bd 87f0 ldmia.w sp!, {r4, r5, r6, r7, r8, r9, sl, pc}
3a: 4614 mov r4, r2
3c: f7ff fffe bl 0 <__do_div64>
40: f10a 0401 add.w r4, sl, #1
44: f8c8 0000 str.w r0, [r8]
48: 4629 mov r1, r5
4a: 4648 mov r0, r9
4c: f7ff fffe bl 0 <__do_div64>
50: 4619 mov r1, r3
52: 4607 mov r7, r0
54: e7ec b.n 30 <mydiv64y+0x30>
56: bf00 nop
>
> Cheers
> ---Date
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v2 1/1] ARM : missing corrupted reg in __div64_32
2016-03-31 10:30 ` Dave Martin
2016-03-31 11:20 ` 陈刚(Gangchen)
@ 2016-03-31 11:41 ` Chen Gang
1 sibling, 0 replies; 24+ messages in thread
From: Chen Gang @ 2016-03-31 11:41 UTC (permalink / raw)
To: linux-arm-kernel
__xl(R0 in little endian system, or R1 in big endian system) is corrupted
after calling __do_div64 and compiler is not informed about this.
If n is used again afterwards, __xl won't be reloaded and n will
contain incorrect value.
Signed-off-by: Chen Gang <gangchen@rdamicro.com>
Signed-off-by: Chen Gang <chengang_rda@163.com>
---
arch/arm/include/asm/div64.h | 20 ++++++--------------
1 file changed, 6 insertions(+), 14 deletions(-)
diff --git a/arch/arm/include/asm/div64.h b/arch/arm/include/asm/div64.h
index e1f0776..ba3f36c 100644
--- a/arch/arm/include/asm/div64.h
+++ b/arch/arm/include/asm/div64.h
@@ -21,30 +21,22 @@
* for arguments and results (beware).
*/
-#ifdef __ARMEB__
-#define __xh "r0"
-#define __xl "r1"
-#else
-#define __xl "r0"
-#define __xh "r1"
-#endif
static inline uint32_t __div64_32(uint64_t *n, uint32_t base)
{
register unsigned int __base asm("r4") = base;
register unsigned long long __n asm("r0") = *n;
register unsigned long long __res asm("r2");
- register unsigned int __rem asm(__xh);
- asm( __asmeq("%0", __xh)
+ asm(
+ __asmeq("%0", "r0")
__asmeq("%1", "r2")
- __asmeq("%2", "r0")
- __asmeq("%3", "r4")
+ __asmeq("%2", "r4")
"bl __do_div64"
- : "=r" (__rem), "=r" (__res)
- : "r" (__n), "r" (__base)
+ : "+r" (__n), "=r" (__res)
+ : "r" (__base)
: "ip", "lr", "cc");
*n = __res;
- return __rem;
+ return __n >> 32;
}
#define __div64_32 __div64_32
--
2.4.3
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH 1/1] ARM : missing corrupted reg in __do_div_asm
2016-03-31 11:20 ` 陈刚(Gangchen)
@ 2016-03-31 11:41 ` Dave Martin
2016-04-01 4:22 ` [PATCH V3 1/1] ARM : missing corrupted reg in __div64_32 chengang
0 siblings, 1 reply; 24+ messages in thread
From: Dave Martin @ 2016-03-31 11:41 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, Mar 31, 2016 at 11:20:24AM +0000, ??(Gangchen) wrote:
>
>
> On 03/31/2016 06:30 PM, Dave Martin wrote:
> > On Thu, Mar 31, 2016 at 07:56:05AM +0000, ??(Gangchen) wrote:
> >> On 03/30/2016 10:07 PM, Dave Martin wrote:
> >>> On Wed, Mar 30, 2016 at 03:27:01AM +0000, ??(Gangchen) wrote:
> >>>> On 03/29/2016 06:56 PM, Dave Martin wrote:
> > [...]
> >
> >>>>> I wonder whether the following would be cleaner than having these
> >>>>> aliased arguments:
> >>>>>
> >>>>> asm( /* ... */
> >>>>> "bl __do_div64"
> >>>>> : "+r" (__n), "=r" (__res)
> >>>>> : "r" (__base)
> >>>>> : "ip", "lr", "cc");
> >>>>> *n = __res;
> >>>>> return __n >> 32;
> >>>>>
> >>>>> (providing that GCC doesn't make a mess of the "easy" shift).
> >>>> I tried your proposal. It didn't make any difference: this is inline
> >>>> function and gcc just ignores your trick.
> >>> What doesn't work for you when using this method?
> >>>
> >>> Why does the fact that this is an inline function make a difference?
> >> With the help of other colleagues, I understand your proposal now.
> >> I create a patch and I can verify that it works!
> > Ah, OK. I was wondering whether I made a mistake somewhere.
> >
> >> Should I submit it, as it seems better than this one I sent?
> > It's up to you -- I think my approach is a bit cleaner, but your
> > approach worked too and is not vulnerable to compilers that generate
> > silly code for (uint64_t) >> 32.
> (uint64_t) >> 32 is pretty much the standard way to get upper half of an
> uint64 variable, compiler should not
> generate silly code for this.
> > Note that I only tested my code for little endian -- it should do the
> > right thing for BE, but I recommend that you try it and examine the
> > generated code, to make sure.
> I don't have a BE system to test, but I did check assembly code
> generated for BE system and didn't find
> anything wrong.
That should be enough -- the code generation is simple enough in this
case to review directly.
> I will update the new patch soon.
>
> The following is my test code and assembly dump for BE system of the
> function.
>
> typedef unsigned long long ull;
> ull mydiv64y(ull tt, unsigned base, unsigned *p)
> {
> ull t = tt;
> *p = do_div(t, base);
> *p = do_div(tt, base+1);
> return tt;
> }
>
> /mnt/2nd_disk/rdaMicro/aosp_4.4/test_modules/div_test/.tmp_test_div.o:
> file format elf32-bigarm
>
>
> Disassembly of section .text:
>
> 00000000 <mydiv64y>:
> 0: e92d 47f0 stmdb sp!, {r4, r5, r6, r7, r8, r9, sl, lr}
> 4: 4607 mov r7, r0
> 6: 2600 movs r6, #0
> 8: 4698 mov r8, r3
> a: ea57 0306 orrs.w r3, r7, r6
[...]
> 14: d111 bne.n 3a <mydiv64y+0x3a>
[...]
> 30: f8c8 7000 str.w r7, [r8]
> 34: 4610 mov r0, r2
> 36: e8bd 87f0 ldmia.w sp!, {r4, r5, r6, r7, r8, r9, sl, pc}
[...]
> 3a: 4614 mov r4, r2
> 3c: f7ff fffe bl 0 <__do_div64>
[...]
> 44: f8c8 0000 str.w r0, [r8]
[...]
> 4c: f7ff fffe bl 0 <__do_div64>
> 50: 4619 mov r1, r3
> 52: 4607 mov r7, r0
> 54: e7ec b.n 30 <mydiv64y+0x30>
This looks sensible for the BE case. I see the high part of the
remainder (r0) being stored back to *p in each case...
For the LE case, we should see r1 stored instead, IIUC.
Cheers
---Dave
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH V3 1/1] ARM : missing corrupted reg in __div64_32
2016-03-31 11:41 ` Dave Martin
@ 2016-04-01 4:22 ` chengang
2016-04-01 8:56 ` Dave Martin
0 siblings, 1 reply; 24+ messages in thread
From: chengang @ 2016-04-01 4:22 UTC (permalink / raw)
To: linux-arm-kernel
__xl(R0 in little endian system, or R1 in big endian system) is corrupted
after calling __do_div64 and compiler is not informed about this.
If n is used again afterwards, __xl won't be reloaded and n will
contain incorrect value.
Signed-off-by: Chen Gang <gangchen@rdamicro.com>
Signed-off-by: Chen Gang <changeang.beijing@outlook.com>
---
arch/arm/include/asm/div64.h | 22 ++++++++--------------
1 file changed, 8 insertions(+), 14 deletions(-)
diff --git a/arch/arm/include/asm/div64.h b/arch/arm/include/asm/div64.h
index e1f0776..8d1b49a 100644
--- a/arch/arm/include/asm/div64.h
+++ b/arch/arm/include/asm/div64.h
@@ -21,30 +21,24 @@
* for arguments and results (beware).
*/
-#ifdef __ARMEB__
-#define __xh "r0"
-#define __xl "r1"
-#else
-#define __xl "r0"
-#define __xh "r1"
-#endif
static inline uint32_t __div64_32(uint64_t *n, uint32_t base)
{
register unsigned int __base asm("r4") = base;
register unsigned long long __n asm("r0") = *n;
register unsigned long long __res asm("r2");
- register unsigned int __rem asm(__xh);
- asm( __asmeq("%0", __xh)
+ asm( __asmeq("%0", "r0")
__asmeq("%1", "r2")
- __asmeq("%2", "r0")
- __asmeq("%3", "r4")
+ __asmeq("%2", "r4")
"bl __do_div64"
- : "=r" (__rem), "=r" (__res)
- : "r" (__n), "r" (__base)
+ : "+r" (__n), "=r" (__res)
+ : "r" (__base)
: "ip", "lr", "cc");
*n = __res;
- return __rem;
+ /*
+ * rem is saved at the up half of __n, see __do_div64 for more
+ */
+ return __n >> 32;
}
#define __div64_32 __div64_32
--
2.4.3
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH V3 1/1] ARM : missing corrupted reg in __div64_32
2016-04-01 4:22 ` [PATCH V3 1/1] ARM : missing corrupted reg in __div64_32 chengang
@ 2016-04-01 8:56 ` Dave Martin
2016-04-01 10:28 ` [PATCH V4 " chengang
0 siblings, 1 reply; 24+ messages in thread
From: Dave Martin @ 2016-04-01 8:56 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, Apr 01, 2016 at 12:22:11PM +0800, chengang wrote:
> __xl(R0 in little endian system, or R1 in big endian system) is corrupted
> after calling __do_div64 and compiler is not informed about this.
> If n is used again afterwards, __xl won't be reloaded and n will
> contain incorrect value.
You should briefly explain the fix here.
>
> Signed-off-by: Chen Gang <gangchen@rdamicro.com>
> Signed-off-by: Chen Gang <changeang.beijing@outlook.com>
> ---
> arch/arm/include/asm/div64.h | 22 ++++++++--------------
> 1 file changed, 8 insertions(+), 14 deletions(-)
>
> diff --git a/arch/arm/include/asm/div64.h b/arch/arm/include/asm/div64.h
> index e1f0776..8d1b49a 100644
> --- a/arch/arm/include/asm/div64.h
> +++ b/arch/arm/include/asm/div64.h
> @@ -21,30 +21,24 @@
> * for arguments and results (beware).
> */
>
> -#ifdef __ARMEB__
> -#define __xh "r0"
> -#define __xl "r1"
> -#else
> -#define __xl "r0"
> -#define __xh "r1"
> -#endif
These defines aren't used by anything else, right?
I grepped for them, but I didn't see any other matches.
>
> static inline uint32_t __div64_32(uint64_t *n, uint32_t base)
> {
> register unsigned int __base asm("r4") = base;
> register unsigned long long __n asm("r0") = *n;
> register unsigned long long __res asm("r2");
> - register unsigned int __rem asm(__xh);
> - asm( __asmeq("%0", __xh)
> + asm( __asmeq("%0", "r0")
> __asmeq("%1", "r2")
> - __asmeq("%2", "r0")
> - __asmeq("%3", "r4")
> + __asmeq("%2", "r4")
> "bl __do_div64"
> - : "=r" (__rem), "=r" (__res)
> - : "r" (__n), "r" (__base)
> + : "+r" (__n), "=r" (__res)
> + : "r" (__base)
> : "ip", "lr", "cc");
> *n = __res;
> - return __rem;
> + /*
> + * rem is saved at the up half of __n, see __do_div64 for more
> + */
Minor nit:
It's useful to have a comment here, but rem is gone, and you might want
to say "upper half", e.g., something like:
/* __do_div returns the remainder in the upper word of __n: */
> + return __n >> 32;
> }
> #define __div64_32 __div64_32
>
> --
> 2.4.3
Cheers
---Dave
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH V4 1/1] ARM : missing corrupted reg in __div64_32
2016-04-01 8:56 ` Dave Martin
@ 2016-04-01 10:28 ` chengang
2016-04-01 11:03 ` Dave Martin
0 siblings, 1 reply; 24+ messages in thread
From: chengang @ 2016-04-01 10:28 UTC (permalink / raw)
To: linux-arm-kernel
__xl(R0 in little endian system, or R1 in big endian system) is corrupted
by the call to __do_div64i(). The compiler is not aware of this, so if
n is used afterwards __xl won't be reloaded and n will contain incorrect value.
Mark 64bits variable __n as inputi/output to inform compiler both r0 and r1
are changed by the funciton call.
Signed-off-by: Chen Gang <gangchen@rdamicro.com>
Signed-off-by: Chen Gang <changeang.beijing@outlook.com>
---
arch/arm/include/asm/div64.h | 22 ++++++++--------------
1 file changed, 8 insertions(+), 14 deletions(-)
diff --git a/arch/arm/include/asm/div64.h b/arch/arm/include/asm/div64.h
index e1f0776..1153e6d 100644
--- a/arch/arm/include/asm/div64.h
+++ b/arch/arm/include/asm/div64.h
@@ -21,30 +21,24 @@
* for arguments and results (beware).
*/
-#ifdef __ARMEB__
-#define __xh "r0"
-#define __xl "r1"
-#else
-#define __xl "r0"
-#define __xh "r1"
-#endif
static inline uint32_t __div64_32(uint64_t *n, uint32_t base)
{
register unsigned int __base asm("r4") = base;
register unsigned long long __n asm("r0") = *n;
register unsigned long long __res asm("r2");
- register unsigned int __rem asm(__xh);
- asm( __asmeq("%0", __xh)
+ asm( __asmeq("%0", "r0")
__asmeq("%1", "r2")
- __asmeq("%2", "r0")
- __asmeq("%3", "r4")
+ __asmeq("%2", "r4")
"bl __do_div64"
- : "=r" (__rem), "=r" (__res)
- : "r" (__n), "r" (__base)
+ : "+r" (__n), "=r" (__res)
+ : "r" (__base)
: "ip", "lr", "cc");
*n = __res;
- return __rem;
+ /*
+ * __do_div64 returns the remainder in the upper word of __n
+ */
+ return __n >> 32;
}
#define __div64_32 __div64_32
--
2.4.3
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH V4 1/1] ARM : missing corrupted reg in __div64_32
2016-04-01 10:28 ` [PATCH V4 " chengang
@ 2016-04-01 11:03 ` Dave Martin
2016-04-01 11:25 ` [PATCH V5 " chengang
2016-04-01 11:35 ` [PATCH V4 " 陈刚(Gangchen)
0 siblings, 2 replies; 24+ messages in thread
From: Dave Martin @ 2016-04-01 11:03 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, Apr 01, 2016 at 06:28:49PM +0800, chengang wrote:
> __xl(R0 in little endian system, or R1 in big endian system) is corrupted
> by the call to __do_div64i(). The compiler is not aware of this, so if
Should that be "__do_div64", not "__do_div64i"?
> n is used afterwards __xl won't be reloaded and n will contain incorrect value.
>
> Mark 64bits variable __n as inputi/output to inform compiler both r0 and r1
Typo: "inputi"
> are changed by the funciton call.
>
> Signed-off-by: Chen Gang <gangchen@rdamicro.com>
> Signed-off-by: Chen Gang <changeang.beijing@outlook.com>
> ---
> arch/arm/include/asm/div64.h | 22 ++++++++--------------
> 1 file changed, 8 insertions(+), 14 deletions(-)
>
> diff --git a/arch/arm/include/asm/div64.h b/arch/arm/include/asm/div64.h
> index e1f0776..1153e6d 100644
> --- a/arch/arm/include/asm/div64.h
> +++ b/arch/arm/include/asm/div64.h
> @@ -21,30 +21,24 @@
> * for arguments and results (beware).
> */
>
> -#ifdef __ARMEB__
> -#define __xh "r0"
> -#define __xl "r1"
> -#else
> -#define __xl "r0"
> -#define __xh "r1"
> -#endif
>
> static inline uint32_t __div64_32(uint64_t *n, uint32_t base)
> {
> register unsigned int __base asm("r4") = base;
> register unsigned long long __n asm("r0") = *n;
> register unsigned long long __res asm("r2");
> - register unsigned int __rem asm(__xh);
> - asm( __asmeq("%0", __xh)
> + asm( __asmeq("%0", "r0")
> __asmeq("%1", "r2")
> - __asmeq("%2", "r0")
> - __asmeq("%3", "r4")
> + __asmeq("%2", "r4")
> "bl __do_div64"
> - : "=r" (__rem), "=r" (__res)
> - : "r" (__n), "r" (__base)
> + : "+r" (__n), "=r" (__res)
> + : "r" (__base)
> : "ip", "lr", "cc");
> *n = __res;
> - return __rem;
> + /*
> + * __do_div64 returns the remainder in the upper word of __n
> + */
Minor nit: ^ this is short enough to be a one-line comment
> + return __n >> 32;
> }
> #define __div64_32 __div64_32
With those changes,
Reviewed-by: Dave Martin <Dave.Martin@arm.com>
Cheers
---Dave
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH V5 1/1] ARM : missing corrupted reg in __div64_32
2016-04-01 11:03 ` Dave Martin
@ 2016-04-01 11:25 ` chengang
2016-04-01 11:34 ` Dave P Martin
2016-04-01 11:35 ` [PATCH V4 " 陈刚(Gangchen)
1 sibling, 1 reply; 24+ messages in thread
From: chengang @ 2016-04-01 11:25 UTC (permalink / raw)
To: linux-arm-kernel
__xl(R0 in little endian system, or R1 in big endian system) is corrupted
by the call to __do_div64(). The compiler is not aware of this, so if
n is used afterwards __xl won't be reloaded and n will contain incorrect value.
Mark 64bits variable __n as input/output to inform compiler both r0 and r1
are changed by the funciton call.
Signed-off-by: Chen Gang <gangchen@rdamicro.com>
Signed-off-by: Chen Gang <changeang.beijing@outlook.com>
---
arch/arm/include/asm/div64.h | 20 ++++++--------------
1 file changed, 6 insertions(+), 14 deletions(-)
diff --git a/arch/arm/include/asm/div64.h b/arch/arm/include/asm/div64.h
index e1f0776..ba3a5fc 100644
--- a/arch/arm/include/asm/div64.h
+++ b/arch/arm/include/asm/div64.h
@@ -21,30 +21,22 @@
* for arguments and results (beware).
*/
-#ifdef __ARMEB__
-#define __xh "r0"
-#define __xl "r1"
-#else
-#define __xl "r0"
-#define __xh "r1"
-#endif
static inline uint32_t __div64_32(uint64_t *n, uint32_t base)
{
register unsigned int __base asm("r4") = base;
register unsigned long long __n asm("r0") = *n;
register unsigned long long __res asm("r2");
- register unsigned int __rem asm(__xh);
- asm( __asmeq("%0", __xh)
+ asm( __asmeq("%0", "r0")
__asmeq("%1", "r2")
- __asmeq("%2", "r0")
- __asmeq("%3", "r4")
+ __asmeq("%2", "r4")
"bl __do_div64"
- : "=r" (__rem), "=r" (__res)
- : "r" (__n), "r" (__base)
+ : "+r" (__n), "=r" (__res)
+ : "r" (__base)
: "ip", "lr", "cc");
*n = __res;
- return __rem;
+ /* __do_div64 returns the remainder in the upper word of __n */
+ return __n >> 32;
}
#define __div64_32 __div64_32
--
2.4.3
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH V5 1/1] ARM : missing corrupted reg in __div64_32
2016-04-01 11:25 ` [PATCH V5 " chengang
@ 2016-04-01 11:34 ` Dave P Martin
0 siblings, 0 replies; 24+ messages in thread
From: Dave P Martin @ 2016-04-01 11:34 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, Apr 01, 2016 at 07:25:34PM +0800, chengang wrote:
> __xl(R0 in little endian system, or R1 in big endian system) is corrupted
> by the call to __do_div64(). The compiler is not aware of this, so if
> n is used afterwards __xl won't be reloaded and n will contain incorrect value.
>
> Mark 64bits variable __n as input/output to inform compiler both r0 and r1
> are changed by the funciton call.
Looks good for me now.
Reviewed-by: Dave Martin <Dave.Martin@arm.com>
>
> Signed-off-by: Chen Gang <gangchen@rdamicro.com>
> Signed-off-by: Chen Gang <changeang.beijing@outlook.com>
> ---
> arch/arm/include/asm/div64.h | 20 ++++++--------------
> 1 file changed, 6 insertions(+), 14 deletions(-)
>
> diff --git a/arch/arm/include/asm/div64.h b/arch/arm/include/asm/div64.h
> index e1f0776..ba3a5fc 100644
> --- a/arch/arm/include/asm/div64.h
> +++ b/arch/arm/include/asm/div64.h
> @@ -21,30 +21,22 @@
> * for arguments and results (beware).
> */
>
> -#ifdef __ARMEB__
> -#define __xh "r0"
> -#define __xl "r1"
> -#else
> -#define __xl "r0"
> -#define __xh "r1"
> -#endif
>
> static inline uint32_t __div64_32(uint64_t *n, uint32_t base)
> {
> register unsigned int __base asm("r4") = base;
> register unsigned long long __n asm("r0") = *n;
> register unsigned long long __res asm("r2");
> - register unsigned int __rem asm(__xh);
> - asm( __asmeq("%0", __xh)
> + asm( __asmeq("%0", "r0")
> __asmeq("%1", "r2")
> - __asmeq("%2", "r0")
> - __asmeq("%3", "r4")
> + __asmeq("%2", "r4")
> "bl __do_div64"
> - : "=r" (__rem), "=r" (__res)
> - : "r" (__n), "r" (__base)
> + : "+r" (__n), "=r" (__res)
> + : "r" (__base)
> : "ip", "lr", "cc");
> *n = __res;
> - return __rem;
> + /* __do_div64 returns the remainder in the upper word of __n */
> + return __n >> 32;
> }
> #define __div64_32 __div64_32
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH V4 1/1] ARM : missing corrupted reg in __div64_32
2016-04-01 11:03 ` Dave Martin
2016-04-01 11:25 ` [PATCH V5 " chengang
@ 2016-04-01 11:35 ` 陈刚(Gangchen)
1 sibling, 0 replies; 24+ messages in thread
From: 陈刚(Gangchen) @ 2016-04-01 11:35 UTC (permalink / raw)
To: linux-arm-kernel
On 04/01/2016 07:03 PM, Dave Martin wrote:
> On Fri, Apr 01, 2016 at 06:28:49PM +0800, chengang wrote:
>> __xl(R0 in little endian system, or R1 in big endian system) is corrupted
>> by the call to __do_div64i(). The compiler is not aware of this, so if
> Should that be "__do_div64", not "__do_div64i"?
>
>> n is used afterwards __xl won't be reloaded and n will contain incorrect value.
>>
>> Mark 64bits variable __n as inputi/output to inform compiler both r0 and r1
> Typo: "inputi"
>
>> are changed by the funciton call.
>>
>> Signed-off-by: Chen Gang <gangchen@rdamicro.com>
>> Signed-off-by: Chen Gang <changeang.beijing@outlook.com>
>> ---
>> arch/arm/include/asm/div64.h | 22 ++++++++--------------
>> 1 file changed, 8 insertions(+), 14 deletions(-)
>>
>> diff --git a/arch/arm/include/asm/div64.h b/arch/arm/include/asm/div64.h
>> index e1f0776..1153e6d 100644
>> --- a/arch/arm/include/asm/div64.h
>> +++ b/arch/arm/include/asm/div64.h
>> @@ -21,30 +21,24 @@
>> * for arguments and results (beware).
>> */
>>
>> -#ifdef __ARMEB__
>> -#define __xh "r0"
>> -#define __xl "r1"
>> -#else
>> -#define __xl "r0"
>> -#define __xh "r1"
>> -#endif
>>
>> static inline uint32_t __div64_32(uint64_t *n, uint32_t base)
>> {
>> register unsigned int __base asm("r4") = base;
>> register unsigned long long __n asm("r0") = *n;
>> register unsigned long long __res asm("r2");
>> - register unsigned int __rem asm(__xh);
>> - asm( __asmeq("%0", __xh)
>> + asm( __asmeq("%0", "r0")
>> __asmeq("%1", "r2")
>> - __asmeq("%2", "r0")
>> - __asmeq("%3", "r4")
>> + __asmeq("%2", "r4")
>> "bl __do_div64"
>> - : "=r" (__rem), "=r" (__res)
>> - : "r" (__n), "r" (__base)
>> + : "+r" (__n), "=r" (__res)
>> + : "r" (__base)
>> : "ip", "lr", "cc");
>> *n = __res;
>> - return __rem;
>> + /*
>> + * __do_div64 returns the remainder in the upper word of __n
>> + */
> Minor nit: ^ this is short enough to be a one-line comment
>
>> + return __n >> 32;
>> }
>> #define __div64_32 __div64_32
> With those changes,
>
> Reviewed-by: Dave Martin <Dave.Martin@arm.com>
Thanks a lot , especially for your patience! I just sent V5, cheers!
>
> Cheers
> ---Dave
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 1/1] ARM : missing corrupted reg in __do_div_asm
2015-06-09 13:09 ` Russell King - ARM Linux
@ 2015-06-28 11:29 ` Chen Gang
0 siblings, 0 replies; 24+ messages in thread
From: Chen Gang @ 2015-06-28 11:29 UTC (permalink / raw)
To: linux-arm-kernel
Hi, Russell
I have uploaded V2 of the patch based on your comments on Jun-19. Hope
you can review that again.
Br, Chen Gang
On 06/09/2015 09:09 PM, Russell King - ARM Linux wrote:
> On Tue, Jun 02, 2015 at 02:13:29PM +0800, Chen Gang wrote:
>> __xl (r0 in little endian system, or R1 in big endian system) is corrupted
>> after calling __do_div64 and compiler is not informed about this in
>> macro __do_div_asm. If n is used again afterwards, __xl won't be
>> reloaded and n will contain incorrect value.
>> ---
> This commit message needs improving. Please either upper case both r0
> and r1 in the first line, or lower case them both. Secondly, the Linux
> kernel has a requirement that all contributions are signed-off. Please
> read Documentation/SubmittingPatches for information, particularly
> section 11.
>
> Other than that, the patch looks fine, thanks.
>
>> arch/arm/include/asm/div64.h | 10 ++++++----
>> 1 file changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/arm/include/asm/div64.h b/arch/arm/include/asm/div64.h
>> index 662c7bd..fa9489e 100644
>> --- a/arch/arm/include/asm/div64.h
>> +++ b/arch/arm/include/asm/div64.h
>> @@ -34,12 +34,14 @@
>> register unsigned long long __n asm("r0") = n; \
>> register unsigned long long __res asm("r2"); \
>> register unsigned int __rem asm(__xh); \
>> + register unsigned int __clobber asm(__xl); \
>> asm( __asmeq("%0", __xh) \
>> - __asmeq("%1", "r2") \
>> - __asmeq("%2", "r0") \
>> - __asmeq("%3", "r4") \
>> + __asmeq("%1", __xl) \
>> + __asmeq("%2", "r2") \
>> + __asmeq("%3", "r0") \
>> + __asmeq("%4", "r4") \
>> "bl __do_div64" \
>> - : "=r" (__rem), "=r" (__res) \
>> + : "=r" (__rem), "=r" (__clobber), "=r" (__res) \
>> : "r" (__n), "r" (__base) \
>> : "ip", "lr", "cc"); \
>> n = __res; \
>> --
>> 1.9.1
>>
>>
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 1/1] ARM : missing corrupted reg in __do_div_asm
2015-06-02 6:13 ` Chen Gang
2015-06-08 5:30 ` Chen Gang
@ 2015-06-09 13:09 ` Russell King - ARM Linux
2015-06-28 11:29 ` Chen Gang
1 sibling, 1 reply; 24+ messages in thread
From: Russell King - ARM Linux @ 2015-06-09 13:09 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Jun 02, 2015 at 02:13:29PM +0800, Chen Gang wrote:
> __xl (r0 in little endian system, or R1 in big endian system) is corrupted
> after calling __do_div64 and compiler is not informed about this in
> macro __do_div_asm. If n is used again afterwards, __xl won't be
> reloaded and n will contain incorrect value.
> ---
This commit message needs improving. Please either upper case both r0
and r1 in the first line, or lower case them both. Secondly, the Linux
kernel has a requirement that all contributions are signed-off. Please
read Documentation/SubmittingPatches for information, particularly
section 11.
Other than that, the patch looks fine, thanks.
> arch/arm/include/asm/div64.h | 10 ++++++----
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm/include/asm/div64.h b/arch/arm/include/asm/div64.h
> index 662c7bd..fa9489e 100644
> --- a/arch/arm/include/asm/div64.h
> +++ b/arch/arm/include/asm/div64.h
> @@ -34,12 +34,14 @@
> register unsigned long long __n asm("r0") = n; \
> register unsigned long long __res asm("r2"); \
> register unsigned int __rem asm(__xh); \
> + register unsigned int __clobber asm(__xl); \
> asm( __asmeq("%0", __xh) \
> - __asmeq("%1", "r2") \
> - __asmeq("%2", "r0") \
> - __asmeq("%3", "r4") \
> + __asmeq("%1", __xl) \
> + __asmeq("%2", "r2") \
> + __asmeq("%3", "r0") \
> + __asmeq("%4", "r4") \
> "bl __do_div64" \
> - : "=r" (__rem), "=r" (__res) \
> + : "=r" (__rem), "=r" (__clobber), "=r" (__res) \
> : "r" (__n), "r" (__base) \
> : "ip", "lr", "cc"); \
> n = __res; \
> --
> 1.9.1
>
>
--
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 1/1] ARM : missing corrupted reg in __do_div_asm
2015-06-02 6:13 ` Chen Gang
@ 2015-06-08 5:30 ` Chen Gang
2015-06-09 13:09 ` Russell King - ARM Linux
1 sibling, 0 replies; 24+ messages in thread
From: Chen Gang @ 2015-06-08 5:30 UTC (permalink / raw)
To: linux-arm-kernel
Hi,
I tried to make a test program to illustrate the patch.
Without the patch, the log shows
"[250970.715392] remains1 is 2, remains2 is 0"
And the remains2 is wrong
With the patch, the log is
"[250977.724181] remains1 is 2, remains2 is 4".
now both remains1 and remains2 are correct.
I compared the output assembly and it showed in the faulty version,
r0 ( I am using little endian system) is not reload after the
first do_div and before the second do_div call in function mydiv64.
Br, Chen Gang
#include <asm/div64.h>
#include <linux/init.h>
#include <linux/printk.h>
typedef unsigned long long ull;
void mydiv64(ull tt, unsigned base1, unsigned base2,
unsigned *rems1, unsigned *rems2)
{
ull t = tt;
*rems1 = do_div(t, base1);
*rems2 = do_div(tt, base2);
return ;
}
static int __init div_test(void)
{
ull t;
unsigned int remains1;
unsigned int remains2;
unsigned base1, base2;
t = 100;
base1 = 7;
base2 = 8;
mydiv64(t, base1, base2, &remains1, &remains2);
printk(KERN_ERR"remains1 is %d, remains2 is %d\n",
remains1, remains2);
return -1;
}
static void __exit div_test_exit(void)
{
}
module_init(div_test);
module_exit(div_test_exit);
On 06/02/2015 02:13 PM, Chen Gang wrote:
> __xl (r0 in little endian system, or R1 in big endian system) is corrupted
> after calling __do_div64 and compiler is not informed about this in
> macro __do_div_asm. If n is used again afterwards, __xl won't be
> reloaded and n will contain incorrect value.
> ---
> arch/arm/include/asm/div64.h | 10 ++++++----
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm/include/asm/div64.h b/arch/arm/include/asm/div64.h
> index 662c7bd..fa9489e 100644
> --- a/arch/arm/include/asm/div64.h
> +++ b/arch/arm/include/asm/div64.h
> @@ -34,12 +34,14 @@
> register unsigned long long __n asm("r0") = n; \
> register unsigned long long __res asm("r2"); \
> register unsigned int __rem asm(__xh); \
> + register unsigned int __clobber asm(__xl); \
> asm( __asmeq("%0", __xh) \
> - __asmeq("%1", "r2") \
> - __asmeq("%2", "r0") \
> - __asmeq("%3", "r4") \
> + __asmeq("%1", __xl) \
> + __asmeq("%2", "r2") \
> + __asmeq("%3", "r0") \
> + __asmeq("%4", "r4") \
> "bl __do_div64" \
> - : "=r" (__rem), "=r" (__res) \
> + : "=r" (__rem), "=r" (__clobber), "=r" (__res) \
> : "r" (__n), "r" (__base) \
> : "ip", "lr", "cc"); \
> n = __res; \
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 1/1] ARM : missing corrupted reg in __do_div_asm
[not found] </home/gachen/Re: [PATCH 1_1] ARM:r0 is also corrupted after calling __do_div64..eml>
@ 2015-06-02 6:13 ` Chen Gang
2015-06-08 5:30 ` Chen Gang
2015-06-09 13:09 ` Russell King - ARM Linux
0 siblings, 2 replies; 24+ messages in thread
From: Chen Gang @ 2015-06-02 6:13 UTC (permalink / raw)
To: linux-arm-kernel
__xl (r0 in little endian system, or R1 in big endian system) is corrupted
after calling __do_div64 and compiler is not informed about this in
macro __do_div_asm. If n is used again afterwards, __xl won't be
reloaded and n will contain incorrect value.
---
arch/arm/include/asm/div64.h | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/arch/arm/include/asm/div64.h b/arch/arm/include/asm/div64.h
index 662c7bd..fa9489e 100644
--- a/arch/arm/include/asm/div64.h
+++ b/arch/arm/include/asm/div64.h
@@ -34,12 +34,14 @@
register unsigned long long __n asm("r0") = n; \
register unsigned long long __res asm("r2"); \
register unsigned int __rem asm(__xh); \
+ register unsigned int __clobber asm(__xl); \
asm( __asmeq("%0", __xh) \
- __asmeq("%1", "r2") \
- __asmeq("%2", "r0") \
- __asmeq("%3", "r4") \
+ __asmeq("%1", __xl) \
+ __asmeq("%2", "r2") \
+ __asmeq("%3", "r0") \
+ __asmeq("%4", "r4") \
"bl __do_div64" \
- : "=r" (__rem), "=r" (__res) \
+ : "=r" (__rem), "=r" (__clobber), "=r" (__res) \
: "r" (__n), "r" (__base) \
: "ip", "lr", "cc"); \
n = __res; \
--
1.9.1
^ permalink raw reply related [flat|nested] 24+ messages in thread
end of thread, other threads:[~2016-04-01 11:35 UTC | newest]
Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-03-28 4:19 [PATCH 1/1] ARM : missing corrupted reg in __do_div_asm Chen Gang
2016-03-29 10:19 ` Arnd Bergmann
2016-03-29 10:26 ` Dave Martin
2016-03-29 10:34 ` Russell King - ARM Linux
2016-03-29 10:56 ` Dave Martin
2016-03-30 3:27 ` 陈刚(Gangchen)
2016-03-30 14:07 ` Dave Martin
2016-03-31 7:56 ` 陈刚(Gangchen)
2016-03-31 10:30 ` Dave Martin
2016-03-31 11:20 ` 陈刚(Gangchen)
2016-03-31 11:41 ` Dave Martin
2016-04-01 4:22 ` [PATCH V3 1/1] ARM : missing corrupted reg in __div64_32 chengang
2016-04-01 8:56 ` Dave Martin
2016-04-01 10:28 ` [PATCH V4 " chengang
2016-04-01 11:03 ` Dave Martin
2016-04-01 11:25 ` [PATCH V5 " chengang
2016-04-01 11:34 ` Dave P Martin
2016-04-01 11:35 ` [PATCH V4 " 陈刚(Gangchen)
2016-03-31 11:41 ` [PATCH v2 " Chen Gang
2016-03-29 10:58 ` [PATCH 1/1] ARM : missing corrupted reg in __do_div_asm 陈刚(Gangchen)
[not found] </home/gachen/Re: [PATCH 1_1] ARM:r0 is also corrupted after calling __do_div64..eml>
2015-06-02 6:13 ` Chen Gang
2015-06-08 5:30 ` Chen Gang
2015-06-09 13:09 ` Russell King - ARM Linux
2015-06-28 11:29 ` Chen Gang
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.