[PATCH 1/1] ARM : missing corrupted reg in __do_div_asm

[PATCH 1/1] ARM : missing corrupted reg in __do_div_asm

[Chen Gang]

__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

[PATCH 1/1] ARM : missing corrupted reg in __do_div_asm

[Arnd Bergmann]

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

[PATCH 1/1] ARM : missing corrupted reg in __do_div_asm

[Dave Martin]

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

[PATCH 1/1] ARM : missing corrupted reg in __do_div_asm

[Russell King - ARM Linux]

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.

[PATCH 1/1] ARM : missing corrupted reg in __do_div_asm

[Dave Martin]

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

[PATCH 1/1] ARM : missing corrupted reg in __do_div_asm

[陈刚(Gangchen)]

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

[PATCH 1/1] ARM : missing corrupted reg in __do_div_asm

[Dave Martin]

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

[PATCH 1/1] ARM : missing corrupted reg in __do_div_asm

[陈刚(Gangchen)]

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

[PATCH 1/1] ARM : missing corrupted reg in __do_div_asm

[Dave Martin]

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

[PATCH 1/1] ARM : missing corrupted reg in __do_div_asm

[陈刚(Gangchen)]

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

[PATCH 1/1] ARM : missing corrupted reg in __do_div_asm

[Dave Martin]

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

[PATCH V3 1/1] ARM : missing corrupted reg in __div64_32

[chengang]

__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

[PATCH V3 1/1] ARM : missing corrupted reg in __div64_32

[Dave Martin]

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

[PATCH V4 1/1] ARM : missing corrupted reg in __div64_32

[chengang]

__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

[PATCH V4 1/1] ARM : missing corrupted reg in __div64_32

[Dave Martin]

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

[PATCH V5 1/1] ARM : missing corrupted reg in __div64_32

[chengang]

__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

[PATCH V5 1/1] ARM : missing corrupted reg in __div64_32

[Dave P Martin]

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

[PATCH V4 1/1] ARM : missing corrupted reg in __div64_32

[陈刚(Gangchen)]

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

[PATCH v2 1/1] ARM : missing corrupted reg in __div64_32

[Chen Gang]

__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

[PATCH 1/1] ARM : missing corrupted reg in __do_div_asm

[陈刚(Gangchen)]

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

[PATCH 1/1] ARM : missing corrupted reg in __do_div_asm

[Chen Gang]

__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

[PATCH 1/1] ARM : missing corrupted reg in __do_div_asm

[Chen Gang]

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

[PATCH 1/1] ARM : missing corrupted reg in __do_div_asm

[Russell King - ARM Linux]

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.

[PATCH 1/1] ARM : missing corrupted reg in __do_div_asm

[Chen Gang]

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