* [PATCH] asm-generic/bitops/fls64.h
@ 2008-05-04 18:58 Виноградов Николай Михайлович
2008-05-12 21:45 ` Andrew Morton
2008-05-13 11:17 ` Alexander van Heukelum
0 siblings, 2 replies; 13+ messages in thread
From: Виноградов Николай Михайлович @ 2008-05-04 18:58 UTC (permalink / raw
To: linux-kernel
bugfix in fls64 on a big endian systems(against 2.6.25).
Signed-off-by: Nickolay Vinogradov <nickolay@protei.ru>
--
diff --git a/include/asm-generic/bitops/fls64.h
b/include/asm-generic/bitops/fls64.h
index 1b6b17c..2eedb6f 100644
--- a/include/asm-generic/bitops/fls64.h
+++ b/include/asm-generic/bitops/fls64.h
@@ -8,7 +8,7 @@ static inline int fls64(__u64 x)
__u32 h = x >> 32;
if (h)
return fls(h) + 32;
- return fls(x);
+ return fls((__u32)x);
}
#endif /* _ASM_GENERIC_BITOPS_FLS64_H_ */
--
Nickolay Vinogradov
Protei Research and Development Center
St.Petersburg, 194044, Russia
Tel.: +7 812 449 47 27
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] asm-generic/bitops/fls64.h
2008-05-04 18:58 [PATCH] asm-generic/bitops/fls64.h Виноградов Николай Михайлович
@ 2008-05-12 21:45 ` Andrew Morton
2008-05-13 10:43 ` Nickolay Vinogradov
2008-05-13 11:17 ` Alexander van Heukelum
1 sibling, 1 reply; 13+ messages in thread
From: Andrew Morton @ 2008-05-12 21:45 UTC (permalink / raw
To: ____________________ ______________ ____________________; +Cc: linux-kernel
On Sun, 04 May 2008 22:58:50 +0400
____________________ ______________ ____________________ <nickolay@protei.ru> wrote:
> bugfix in fls64 on a big endian systems(against 2.6.25).
>
> Signed-off-by: Nickolay Vinogradov <nickolay@protei.ru>
>
> --
>
> diff --git a/include/asm-generic/bitops/fls64.h
> b/include/asm-generic/bitops/fls64.h
> index 1b6b17c..2eedb6f 100644
> --- a/include/asm-generic/bitops/fls64.h
> +++ b/include/asm-generic/bitops/fls64.h
> @@ -8,7 +8,7 @@ static inline int fls64(__u64 x)
> __u32 h = x >> 32;
> if (h)
> return fls(h) + 32;
> - return fls(x);
> + return fls((__u32)x);
> }
>
> #endif /* _ASM_GENERIC_BITOPS_FLS64_H_ */
Please describe the bug which you are fixing?
Perhaps a more robust fix to <whatever the bug is> would be to
repair fls() so that it works correctly when passed a u64. Perhaps.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] asm-generic/bitops/fls64.h
2008-05-12 21:45 ` Andrew Morton
@ 2008-05-13 10:43 ` Nickolay Vinogradov
2008-05-13 15:31 ` Andrew Morton
0 siblings, 1 reply; 13+ messages in thread
From: Nickolay Vinogradov @ 2008-05-13 10:43 UTC (permalink / raw
To: Andrew Morton; +Cc: linux-kernel
Andrew Morton пишет:
> On Sun, 04 May 2008 22:58:50 +0400
> ____________________ ______________ ____________________ <nickolay@protei.ru> wrote:
>
>> bugfix in fls64 on a big endian systems(against 2.6.25).
>>
>> Signed-off-by: Nickolay Vinogradov <nickolay@protei.ru>
>>
>> --
>>
>> diff --git a/include/asm-generic/bitops/fls64.h
>> b/include/asm-generic/bitops/fls64.h
>> index 1b6b17c..2eedb6f 100644
>> --- a/include/asm-generic/bitops/fls64.h
>> +++ b/include/asm-generic/bitops/fls64.h
>> @@ -8,7 +8,7 @@ static inline int fls64(__u64 x)
>> __u32 h = x >> 32;
>> if (h)
>> return fls(h) + 32;
>> - return fls(x);
>> + return fls((__u32)x);
>> }
>>
>> #endif /* _ASM_GENERIC_BITOPS_FLS64_H_ */
>
> Please describe the bug which you are fixing?
>
> Perhaps a more robust fix to <whatever the bug is> would be to
> repair fls() so that it works correctly when passed a u64. Perhaps.
Repair fls64() so that it works correctly when passed a u64.
--
Nickolay Vinogradov
Protei Research and Development Center
St.Petersburg, 194044, Russia
Tel.: +7 812 449 47 27
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] asm-generic/bitops/fls64.h
2008-05-04 18:58 [PATCH] asm-generic/bitops/fls64.h Виноградов Николай Михайлович
2008-05-12 21:45 ` Andrew Morton
@ 2008-05-13 11:17 ` Alexander van Heukelum
2008-05-13 12:29 ` Nickolay Vinogradov
1 sibling, 1 reply; 13+ messages in thread
From: Alexander van Heukelum @ 2008-05-13 11:17 UTC (permalink / raw
To: nickolay, linux-kernel
> bugfix in fls64 on a big endian systems(against 2.6.25).
>
> Signed-off-by: Nickolay Vinogradov <nickolay@protei.ru>
>
> --
>
> diff --git a/include/asm-generic/bitops/fls64.h
> b/include/asm-generic/bitops/fls64.h
> index 1b6b17c..2eedb6f 100644
> --- a/include/asm-generic/bitops/fls64.h
> +++ b/include/asm-generic/bitops/fls64.h
> @@ -8,7 +8,7 @@ static inline int fls64(__u64 x)
> __u32 h = x >> 32;
> if (h)
> return fls(h) + 32;
> - return fls(x);
> + return fls((__u32)x);
> }
Hi Nickolay,
The change is ok, I guess, but the cast should be a no-op (fls
takes an int, which is always 32 bit in linux). What is the problem
you are seeing? Does fls64() return a wrong value in some cases? If
so, what cpu? Which values?
Why would this be a bug on big endian systems only? There is no
pointer magic involved, so the compiler should take care of the
casts in a correct way.
Maybe you see a compiler warning? Which compiler version?
(also note that current (development) kernels now have separate
versions for 32-bit and 64-bit environments.)
Greetings,
Alexander
> #endif /* _ASM_GENERIC_BITOPS_FLS64_H_ */
>
> --
> Nickolay Vinogradov
> Protei Research and Development Center
> St.Petersburg, 194044, Russia
> Tel.: +7 812 449 47 27
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel"
> in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
>
--
Alexander van Heukelum
heukelum@fastmail.fm
--
http://www.fastmail.fm - Or how I learned to stop worrying and
love email again
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] asm-generic/bitops/fls64.h
2008-05-13 11:17 ` Alexander van Heukelum
@ 2008-05-13 12:29 ` Nickolay Vinogradov
2008-05-13 13:24 ` Alexander van Heukelum
0 siblings, 1 reply; 13+ messages in thread
From: Nickolay Vinogradov @ 2008-05-13 12:29 UTC (permalink / raw
To: Alexander van Heukelum, Andrew Morton; +Cc: linux-kernel
Alexander van Heukelum пишет:
> Hi Nickolay,
>
> The change is ok, I guess, but the cast should be a no-op (fls
> takes an int, which is always 32 bit in linux). What is the problem
> you are seeing? Does fls64() return a wrong value in some cases? If
> so, what cpu? Which values?
>
> Why would this be a bug on big endian systems only? There is no
> pointer magic involved, so the compiler should take care of the
> casts in a correct way.
>
> Maybe you see a compiler warning? Which compiler version?
>
> (also note that current (development) kernels now have separate
> versions for 32-bit and 64-bit environments.)
Because fls() is a macro for asm-arm:
#define fls(x) \
( __builtin_constant_p(x) ? constant_fls(x) : \
({ int __r; asm("clz\t%0, %1" : "=r"(__r) : "r"(x) : "cc");
32-__r; }) )
We can fix it right here:
diff --git a/include/asm-arm/bitops.h b/include/asm-arm/bitops.h
index 5c60bfc..ce3fb6f 100644
--- a/include/asm-arm/bitops.h
+++ b/include/asm-arm/bitops.h
@@ -279,7 +279,7 @@ static inline int constant_fls(int x)
#define fls(x) \
( __builtin_constant_p(x) ? constant_fls(x) : \
- ({ int __r; asm("clz\t%0, %1" : "=r"(__r) : "r"(x) : "cc");
32-__r; }) )
+ ({ int __r; asm("clz\t%0, %1" : "=r"(__r) : "r"((__u32)x) :
"cc"); 32-__r; }) )
#define ffs(x) ({ unsigned long __t = (x); fls(__t & -__t); })
#define __ffs(x) (ffs(x) - 1)
#define ffz(x) __ffs( ~(x) )
--
Nickolay Vinogradov
Protei Research and Development Center
St.Petersburg, 194044, Russia
Tel.: +7 812 449 47 27
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] asm-generic/bitops/fls64.h
2008-05-13 12:29 ` Nickolay Vinogradov
@ 2008-05-13 13:24 ` Alexander van Heukelum
2008-05-13 13:58 ` Russell King
0 siblings, 1 reply; 13+ messages in thread
From: Alexander van Heukelum @ 2008-05-13 13:24 UTC (permalink / raw
To: Nickolay Vinogradov; +Cc: linux-kernel, Russell King, Andrew Morton
On Tue, 13 May 2008 16:29:04 +0400, "Nickolay Vinogradov"
<nickolay@protei.ru> said:
> Alexander van Heukelum пишет:
>
> > Hi Nickolay,
> >
> > The change is ok, I guess, but the cast should be a no-op (fls
> > takes an int, which is always 32 bit in linux). What is the problem
> > you are seeing? Does fls64() return a wrong value in some cases? If
> > so, what cpu? Which values?
> >
> > Why would this be a bug on big endian systems only? There is no
> > pointer magic involved, so the compiler should take care of the
> > casts in a correct way.
> >
> > Maybe you see a compiler warning? Which compiler version?
> >
> > (also note that current (development) kernels now have separate
> > versions for 32-bit and 64-bit environments.)
>
> Because fls() is a macro for asm-arm:
>
> #define fls(x) \
> ( __builtin_constant_p(x) ? constant_fls(x) : \
> ({ int __r; asm("clz\t%0, %1" : "=r"(__r) : "r"(x) : "cc");
> 32-__r; }) )
>
> We can fix it right here:
>
> diff --git a/include/asm-arm/bitops.h b/include/asm-arm/bitops.h
> index 5c60bfc..ce3fb6f 100644
> --- a/include/asm-arm/bitops.h
> +++ b/include/asm-arm/bitops.h
> @@ -279,7 +279,7 @@ static inline int constant_fls(int x)
>
> #define fls(x) \
> ( __builtin_constant_p(x) ? constant_fls(x) : \
> - ({ int __r; asm("clz\t%0, %1" : "=r"(__r) : "r"(x) : "cc");
> 32-__r; }) )
> + ({ int __r; asm("clz\t%0, %1" : "=r"(__r) : "r"((__u32)x) :
> "cc"); 32-__r; }) )
> #define ffs(x) ({ unsigned long __t = (x); fls(__t & -__t); })
> #define __ffs(x) (ffs(x) - 1)
> #define ffz(x) __ffs( ~(x) )
Hmm, indeed.
Maybe: ({ int __r, __x = (x); asm("clz\t%0, %1" : "=r"(__r) : "r"(__x) :
"cc"); 32-__r; }) ?
This is a 32-bit machine, right? Doesn't the compiler complain about
feeding a long long into a 32-bit register?
Greetings,
Alexander
--
Alexander van Heukelum
heukelum@fastmail.fm
--
http://www.fastmail.fm - I mean, what is it about a decent email service?
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] asm-generic/bitops/fls64.h
2008-05-13 13:24 ` Alexander van Heukelum
@ 2008-05-13 13:58 ` Russell King
2008-05-13 14:24 ` Alexander van Heukelum
2008-05-13 14:35 ` Andreas Schwab
0 siblings, 2 replies; 13+ messages in thread
From: Russell King @ 2008-05-13 13:58 UTC (permalink / raw
To: Alexander van Heukelum; +Cc: Nickolay Vinogradov, linux-kernel, Andrew Morton
On Tue, May 13, 2008 at 03:24:13PM +0200, Alexander van Heukelum wrote:
> On Tue, 13 May 2008 16:29:04 +0400, "Nickolay Vinogradov"
> <nickolay@protei.ru> said:
> > Alexander van Heukelum пишет:
> >
> > > Hi Nickolay,
> > >
> > > The change is ok, I guess, but the cast should be a no-op (fls
> > > takes an int, which is always 32 bit in linux). What is the problem
> > > you are seeing? Does fls64() return a wrong value in some cases? If
> > > so, what cpu? Which values?
> > >
> > > Why would this be a bug on big endian systems only? There is no
> > > pointer magic involved, so the compiler should take care of the
> > > casts in a correct way.
> > >
> > > Maybe you see a compiler warning? Which compiler version?
> > >
> > > (also note that current (development) kernels now have separate
> > > versions for 32-bit and 64-bit environments.)
> >
> > Because fls() is a macro for asm-arm:
> >
> > #define fls(x) \
> > ( __builtin_constant_p(x) ? constant_fls(x) : \
> > ({ int __r; asm("clz\t%0, %1" : "=r"(__r) : "r"(x) : "cc");
> > 32-__r; }) )
> >
> > We can fix it right here:
No. "fls" is for finding the last set bit in an _int_. It is not
supposed to have random crap passed to it, such as types longer than
sizeof(int).
If you're going to pass long long (64-bit) arguments to fls, and then
cast them to a u32, you're truncating the value, and you'll get the
wrong answer if bit 33 or greater is set. If you don't actually care
about the upper bits, don't pass a 64-bit quantity to fls().
If you want to use fls with a long long, use fls64 instead. Or for top
marks, use a u64 and fls64.
--
Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of:
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] asm-generic/bitops/fls64.h
2008-05-13 13:58 ` Russell King
@ 2008-05-13 14:24 ` Alexander van Heukelum
2008-05-13 14:46 ` Nickolay Vinogradov
2008-05-13 14:35 ` Andreas Schwab
1 sibling, 1 reply; 13+ messages in thread
From: Alexander van Heukelum @ 2008-05-13 14:24 UTC (permalink / raw
To: Russell King; +Cc: Nickolay Vinogradov, linux-kernel, Andrew Morton
On Tue, 13 May 2008 14:58:39 +0100, "Russell King"
<rmk+lkml@arm.linux.org.uk> said:
> On Tue, May 13, 2008 at 03:24:13PM +0200, Alexander van Heukelum wrote:
> > On Tue, 13 May 2008 16:29:04 +0400, "Nickolay Vinogradov"
> > <nickolay@protei.ru> said:
> > > Alexander van Heukelum пишет:
> > >
> > > > Hi Nickolay,
> > > >
> > > > The change is ok, I guess, but the cast should be a no-op (fls
> > > > takes an int, which is always 32 bit in linux). What is the problem
> > > > you are seeing? Does fls64() return a wrong value in some cases? If
> > > > so, what cpu? Which values?
> > > >
> > > > Why would this be a bug on big endian systems only? There is no
> > > > pointer magic involved, so the compiler should take care of the
> > > > casts in a correct way.
> > > >
> > > > Maybe you see a compiler warning? Which compiler version?
> > > >
> > > > (also note that current (development) kernels now have separate
> > > > versions for 32-bit and 64-bit environments.)
> > >
> > > Because fls() is a macro for asm-arm:
> > >
> > > #define fls(x) \
> > > ( __builtin_constant_p(x) ? constant_fls(x) : \
> > > ({ int __r; asm("clz\t%0, %1" : "=r"(__r) : "r"(x) : "cc");
> > > 32-__r; }) )
> > >
> > > We can fix it right here:
>
> No. "fls" is for finding the last set bit in an _int_. It is not
> supposed to have random crap passed to it, such as types longer than
> sizeof(int).
>
> If you're going to pass long long (64-bit) arguments to fls, and then
> cast them to a u32, you're truncating the value, and you'll get the
> wrong answer if bit 33 or greater is set. If you don't actually care
> about the upper bits, don't pass a 64-bit quantity to fls().
>
> If you want to use fls with a long long, use fls64 instead. Or for top
> marks, use a u64 and fls64.
But that was the problem we began with: the generic fls64 passes an u64
to fls. Nickolay's original patch solves that by putting a cast to u32
in fls64. I did not, however, understand why the cast was needed. It
should not be needed for correctness, imho, because fls is expected to
behave as if it was a function "int fls(int)", like ffs. Values passed
to fls should thus be truncated if necessary.
Making the truncation explicit in fls64 is still a good idea, though.
Greetings,
Alexander
> --
> Russell King
> Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
> maintainer of:
--
Alexander van Heukelum
heukelum@fastmail.fm
--
http://www.fastmail.fm - The way an email service should be
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] asm-generic/bitops/fls64.h
2008-05-13 13:58 ` Russell King
2008-05-13 14:24 ` Alexander van Heukelum
@ 2008-05-13 14:35 ` Andreas Schwab
2008-05-13 16:11 ` Andrew Morton
1 sibling, 1 reply; 13+ messages in thread
From: Andreas Schwab @ 2008-05-13 14:35 UTC (permalink / raw
To: Russell King
Cc: Alexander van Heukelum, Nickolay Vinogradov, linux-kernel,
Andrew Morton
Russell King <rmk+lkml@arm.linux.org.uk> writes:
> On Tue, May 13, 2008 at 03:24:13PM +0200, Alexander van Heukelum wrote:
>> On Tue, 13 May 2008 16:29:04 +0400, "Nickolay Vinogradov"
>> <nickolay@protei.ru> said:
>> > Alexander van Heukelum пишет:
>> >
>> > > Hi Nickolay,
>> > >
>> > > The change is ok, I guess, but the cast should be a no-op (fls
>> > > takes an int, which is always 32 bit in linux). What is the problem
>> > > you are seeing? Does fls64() return a wrong value in some cases? If
>> > > so, what cpu? Which values?
>> > >
>> > > Why would this be a bug on big endian systems only? There is no
>> > > pointer magic involved, so the compiler should take care of the
>> > > casts in a correct way.
>> > >
>> > > Maybe you see a compiler warning? Which compiler version?
>> > >
>> > > (also note that current (development) kernels now have separate
>> > > versions for 32-bit and 64-bit environments.)
>> >
>> > Because fls() is a macro for asm-arm:
>> >
>> > #define fls(x) \
>> > ( __builtin_constant_p(x) ? constant_fls(x) : \
>> > ({ int __r; asm("clz\t%0, %1" : "=r"(__r) : "r"(x) : "cc");
>> > 32-__r; }) )
>> >
>> > We can fix it right here:
>
> No. "fls" is for finding the last set bit in an _int_. It is not
> supposed to have random crap passed to it, such as types longer than
> sizeof(int).
If you write fls as an (inline) function then the argument is implicitly
converted.
Andreas.
--
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5
"And now for something completely different."
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] asm-generic/bitops/fls64.h
2008-05-13 14:24 ` Alexander van Heukelum
@ 2008-05-13 14:46 ` Nickolay Vinogradov
0 siblings, 0 replies; 13+ messages in thread
From: Nickolay Vinogradov @ 2008-05-13 14:46 UTC (permalink / raw
To: Alexander van Heukelum; +Cc: Russell King, linux-kernel, Andrew Morton
Alexander van Heukelum пишет:
>> No. "fls" is for finding the last set bit in an _int_. It is not
>> supposed to have random crap passed to it, such as types longer than
>> sizeof(int).
>>
>> If you're going to pass long long (64-bit) arguments to fls, and then
>> cast them to a u32, you're truncating the value, and you'll get the
>> wrong answer if bit 33 or greater is set. If you don't actually care
>> about the upper bits, don't pass a 64-bit quantity to fls().
>>
>> If you want to use fls with a long long, use fls64 instead. Or for top
>> marks, use a u64 and fls64.
>
> But that was the problem we began with: the generic fls64 passes an u64
> to fls. Nickolay's original patch solves that by putting a cast to u32
> in fls64. I did not, however, understand why the cast was needed.
The cast was needed because fls is a macro, not a function.
--
Nickolay Vinogradov
Protei Research and Development Center
St.Petersburg, 194044, Russia
Tel.: +7 812 449 47 27
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] asm-generic/bitops/fls64.h
2008-05-13 10:43 ` Nickolay Vinogradov
@ 2008-05-13 15:31 ` Andrew Morton
2008-05-13 15:45 ` Nickolay Vinogradov
0 siblings, 1 reply; 13+ messages in thread
From: Andrew Morton @ 2008-05-13 15:31 UTC (permalink / raw
To: Nickolay Vinogradov; +Cc: linux-kernel
On Tue, 13 May 2008 14:43:43 +0400 Nickolay Vinogradov <nickolay@protei.ru> wrote:
> Andrew Morton __________:
> > On Sun, 04 May 2008 22:58:50 +0400
> > ____________________ ______________ ____________________ <nickolay@protei.ru> wrote:
> >
> >> bugfix in fls64 on a big endian systems(against 2.6.25).
> >>
> >> Signed-off-by: Nickolay Vinogradov <nickolay@protei.ru>
> >>
> >> --
> >>
> >> diff --git a/include/asm-generic/bitops/fls64.h
> >> b/include/asm-generic/bitops/fls64.h
> >> index 1b6b17c..2eedb6f 100644
> >> --- a/include/asm-generic/bitops/fls64.h
> >> +++ b/include/asm-generic/bitops/fls64.h
> >> @@ -8,7 +8,7 @@ static inline int fls64(__u64 x)
> >> __u32 h = x >> 32;
> >> if (h)
> >> return fls(h) + 32;
> >> - return fls(x);
> >> + return fls((__u32)x);
> >> }
> >>
> >> #endif /* _ASM_GENERIC_BITOPS_FLS64_H_ */
> >
> > Please describe the bug which you are fixing?
> >
> > Perhaps a more robust fix to <whatever the bug is> would be to
> > repair fls() so that it works correctly when passed a u64. Perhaps.
>
> Repair fls64() so that it works correctly when passed a u64.
>
Yes, but what's wrong with it now?
The fls() in include/asm-generic/bitops/fls.h takes an int.
The fls() in include/asm-x86/bitops.h takes an int.
So both of these will already trucate the incoming argument to 32-bits.
It seems that you are using a version of fls() which doesn't do this.
Why? Which architecture are you using? Would it not be more robust to
fix fls()?
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] asm-generic/bitops/fls64.h
2008-05-13 15:31 ` Andrew Morton
@ 2008-05-13 15:45 ` Nickolay Vinogradov
0 siblings, 0 replies; 13+ messages in thread
From: Nickolay Vinogradov @ 2008-05-13 15:45 UTC (permalink / raw
To: Andrew Morton; +Cc: linux-kernel
Andrew Morton пишет:
> On Tue, 13 May 2008 14:43:43 +0400 Nickolay Vinogradov <nickolay@protei.ru> wrote:
>
>> Andrew Morton __________:
>>> On Sun, 04 May 2008 22:58:50 +0400
>>> ____________________ ______________ ____________________ <nickolay@protei.ru> wrote:
>>>
>>>> bugfix in fls64 on a big endian systems(against 2.6.25).
>>>>
>>>> Signed-off-by: Nickolay Vinogradov <nickolay@protei.ru>
>>>>
>>>> --
>>>>
>>>> diff --git a/include/asm-generic/bitops/fls64.h
>>>> b/include/asm-generic/bitops/fls64.h
>>>> index 1b6b17c..2eedb6f 100644
>>>> --- a/include/asm-generic/bitops/fls64.h
>>>> +++ b/include/asm-generic/bitops/fls64.h
>>>> @@ -8,7 +8,7 @@ static inline int fls64(__u64 x)
>>>> __u32 h = x >> 32;
>>>> if (h)
>>>> return fls(h) + 32;
>>>> - return fls(x);
>>>> + return fls((__u32)x);
>>>> }
>>>>
>>>> #endif /* _ASM_GENERIC_BITOPS_FLS64_H_ */
>>> Please describe the bug which you are fixing?
>>>
>>> Perhaps a more robust fix to <whatever the bug is> would be to
>>> repair fls() so that it works correctly when passed a u64. Perhaps.
>> Repair fls64() so that it works correctly when passed a u64.
>>
>
> Yes, but what's wrong with it now?
>
> The fls() in include/asm-generic/bitops/fls.h takes an int.
>
> The fls() in include/asm-x86/bitops.h takes an int.
>
> So both of these will already trucate the incoming argument to 32-bits.
>
> It seems that you are using a version of fls() which doesn't do this.
> Why? Which architecture are you using? Would it not be more robust to
> fix fls()?
include/asm-arm/bitops.h
fls() defined here as a macro, not as a function with 32-bit argument.
Therefore, there are two choices:
1. explicit cast to u32 before calling.
2. rewrite fls macro as an inline function.
--
Nickolay Vinogradov
Protei Research and Development Center
St.Petersburg, 194044, Russia
Tel.: +7 812 449 47 27
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] asm-generic/bitops/fls64.h
2008-05-13 14:35 ` Andreas Schwab
@ 2008-05-13 16:11 ` Andrew Morton
0 siblings, 0 replies; 13+ messages in thread
From: Andrew Morton @ 2008-05-13 16:11 UTC (permalink / raw
To: Andreas Schwab
Cc: Russell King, Alexander van Heukelum, Nickolay Vinogradov,
linux-kernel
On Tue, 13 May 2008 16:35:25 +0200 Andreas Schwab <schwab@suse.de> wrote:
> Russell King <rmk+lkml@arm.linux.org.uk> writes:
>
> > On Tue, May 13, 2008 at 03:24:13PM +0200, Alexander van Heukelum wrote:
> >> On Tue, 13 May 2008 16:29:04 +0400, "Nickolay Vinogradov"
> >> <nickolay@protei.ru> said:
> >> > Alexander van Heukelum пишет:
> >> >
> >> > > Hi Nickolay,
> >> > >
> >> > > The change is ok, I guess, but the cast should be a no-op (fls
> >> > > takes an int, which is always 32 bit in linux). What is the problem
> >> > > you are seeing? Does fls64() return a wrong value in some cases? If
> >> > > so, what cpu? Which values?
> >> > >
> >> > > Why would this be a bug on big endian systems only? There is no
> >> > > pointer magic involved, so the compiler should take care of the
> >> > > casts in a correct way.
> >> > >
> >> > > Maybe you see a compiler warning? Which compiler version?
> >> > >
> >> > > (also note that current (development) kernels now have separate
> >> > > versions for 32-bit and 64-bit environments.)
> >> >
> >> > Because fls() is a macro for asm-arm:
> >> >
> >> > #define fls(x) \
> >> > ( __builtin_constant_p(x) ? constant_fls(x) : \
> >> > ({ int __r; asm("clz\t%0, %1" : "=r"(__r) : "r"(x) : "cc");
> >> > 32-__r; }) )
> >> >
> >> > We can fix it right here:
> >
> > No. "fls" is for finding the last set bit in an _int_. It is not
> > supposed to have random crap passed to it, such as types longer than
> > sizeof(int).
>
> If you write fls as an (inline) function then the argument is implicitly
> converted.
>
Yes, and that's what other architectures do.
It's not pretty, but I do think the arm implementation should zap the
upper 32 bits as other architectures do, and as one would expect from
the usual C type conversion rules.
Converting it to a C implementation would be a suitable means...
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2008-05-13 16:11 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2008-05-04 18:58 [PATCH] asm-generic/bitops/fls64.h Виноградов Николай Михайлович
2008-05-12 21:45 ` Andrew Morton
2008-05-13 10:43 ` Nickolay Vinogradov
2008-05-13 15:31 ` Andrew Morton
2008-05-13 15:45 ` Nickolay Vinogradov
2008-05-13 11:17 ` Alexander van Heukelum
2008-05-13 12:29 ` Nickolay Vinogradov
2008-05-13 13:24 ` Alexander van Heukelum
2008-05-13 13:58 ` Russell King
2008-05-13 14:24 ` Alexander van Heukelum
2008-05-13 14:46 ` Nickolay Vinogradov
2008-05-13 14:35 ` Andreas Schwab
2008-05-13 16:11 ` Andrew Morton
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).