* [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; 20+ 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] 20+ 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; 20+ 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] 20+ 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; 20+ 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] 20+ 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; 20+ 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] 20+ 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; 20+ 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] 20+ 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; 20+ 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] 20+ 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; 20+ 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] 20+ 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; 20+ 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] 20+ 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; 20+ 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] 20+ 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; 20+ 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] 20+ 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; 20+ 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] 20+ 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; 20+ 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] 20+ 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; 20+ 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] 20+ 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; 20+ 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] 20+ 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; 20+ 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] 20+ 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; 20+ 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] 20+ 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; 20+ 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] 20+ 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; 20+ 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] 20+ 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; 20+ 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] 20+ 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; 20+ 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] 20+ messages in thread
end of thread, other threads:[~2016-04-01 11:35 UTC | newest] Thread overview: 20+ 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)
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.