| Message ID | 20140112105958.GA9791@n2100.arm.linux.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sunday 12 January 2014 05:59 AM, Russell King - ARM Linux wrote: > On Mon, Dec 09, 2013 at 08:02:30PM -0500, Santosh Shilimkar wrote: >> On Monday 09 December 2013 07:54 PM, Russell King - ARM Linux wrote: >>> The underlying reason is that - as I've already explained - ARM's __ffs() >>> differs from other architectures in that it ends up being an int, whereas >>> almost everyone else is unsigned long. >>> >>> The fix is to fix ARMs __ffs() to conform to other architectures. >>> >> I was just about to cross-post your reply here. Obviously I didn't think >> this far when I made $subject fix. >> >> So lets ignore the $subject patch which is not correct. Sorry for noise > > Well, here we are, a month on, and this still remains unfixed despite > my comments pointing to what the problem is. So, here's a patch to fix > this problem the correct way. I took the time to add some comments to > these functions as I find that I wonder about their return values, and > these comments make the patch a little larger than it otherwise would be. > The $subject warning fix [1] is already picked by Andrew with your ack and its in his queue [2] > This patch makes their types match exactly with x86's definitions of > the same, which is the basic problem: on ARM, they all took "int" values > and returned "int"s, which leads to min() in nobootmem.c complaining. > Not sure if you missed the thread but the right fix was picked. Ofcourse you do have additional clz optimisation in updated patch and some comments on those functions. Regards, Santosh [1] https://lkml.org/lkml/2013/12/9/811 [2] http://ozlabs.org/~akpm/mmotm/broken-out/mm-arm-fix-arms-__ffs-to-conform-to-avoid-warning-with-no_bootmem.patch
On Sunday 12 January 2014 10:42 AM, Santosh Shilimkar wrote: > On Sunday 12 January 2014 05:59 AM, Russell King - ARM Linux wrote: >> On Mon, Dec 09, 2013 at 08:02:30PM -0500, Santosh Shilimkar wrote: >>> On Monday 09 December 2013 07:54 PM, Russell King - ARM Linux wrote: >>>> The underlying reason is that - as I've already explained - ARM's __ffs() >>>> differs from other architectures in that it ends up being an int, whereas >>>> almost everyone else is unsigned long. >>>> >>>> The fix is to fix ARMs __ffs() to conform to other architectures. >>>> >>> I was just about to cross-post your reply here. Obviously I didn't think >>> this far when I made $subject fix. >>> >>> So lets ignore the $subject patch which is not correct. Sorry for noise >> >> Well, here we are, a month on, and this still remains unfixed despite >> my comments pointing to what the problem is. So, here's a patch to fix >> this problem the correct way. I took the time to add some comments to >> these functions as I find that I wonder about their return values, and >> these comments make the patch a little larger than it otherwise would be. >> > The $subject warning fix [1] is already picked by Andrew with your ack > and its in his queue [2] > >> This patch makes their types match exactly with x86's definitions of >> the same, which is the basic problem: on ARM, they all took "int" values >> and returned "int"s, which leads to min() in nobootmem.c complaining. >> > Not sure if you missed the thread but the right fix was picked. Ofcourse > you do have additional clz optimisation in updated patch and some comments > on those functions. > > Regards, > Santosh > > [1] https://lkml.org/lkml/2013/12/9/811 fixing the link since above was the link to the $subject thread and below is the correct link for updated patch [1] https://lkml.org/lkml/2013/12/20/497 > [2] http://ozlabs.org/~akpm/mmotm/broken-out/mm-arm-fix-arms-__ffs-to-conform-to-avoid-warning-with-no_bootmem.patch > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >
On Sun, Jan 12, 2014 at 10:42:00AM -0500, Santosh Shilimkar wrote: > On Sunday 12 January 2014 05:59 AM, Russell King - ARM Linux wrote: > > On Mon, Dec 09, 2013 at 08:02:30PM -0500, Santosh Shilimkar wrote: > >> On Monday 09 December 2013 07:54 PM, Russell King - ARM Linux wrote: > >>> The underlying reason is that - as I've already explained - ARM's __ffs() > >>> differs from other architectures in that it ends up being an int, whereas > >>> almost everyone else is unsigned long. > >>> > >>> The fix is to fix ARMs __ffs() to conform to other architectures. > >>> > >> I was just about to cross-post your reply here. Obviously I didn't think > >> this far when I made $subject fix. > >> > >> So lets ignore the $subject patch which is not correct. Sorry for noise > > > > Well, here we are, a month on, and this still remains unfixed despite > > my comments pointing to what the problem is. So, here's a patch to fix > > this problem the correct way. I took the time to add some comments to > > these functions as I find that I wonder about their return values, and > > these comments make the patch a little larger than it otherwise would be. > > > The $subject warning fix [1] is already picked by Andrew with your ack > and its in his queue [2] > > > This patch makes their types match exactly with x86's definitions of > > the same, which is the basic problem: on ARM, they all took "int" values > > and returned "int"s, which leads to min() in nobootmem.c complaining. > > > Not sure if you missed the thread but the right fix was picked. Ofcourse > you do have additional clz optimisation in updated patch and some comments > on those functions. The problem here is that the patch fixing this is going via akpm's tree (why?) yet you want the code which introduces the warning to be merged via my tree. It seems to me to be absolutely silly to have code introduce a warning yet push the fix for the warning via a completely different tree...
On Monday 13 January 2014 07:37 AM, Russell King - ARM Linux wrote: > On Sun, Jan 12, 2014 at 10:42:00AM -0500, Santosh Shilimkar wrote: >> On Sunday 12 January 2014 05:59 AM, Russell King - ARM Linux wrote: >>> On Mon, Dec 09, 2013 at 08:02:30PM -0500, Santosh Shilimkar wrote: >>>> On Monday 09 December 2013 07:54 PM, Russell King - ARM Linux wrote: >>>>> The underlying reason is that - as I've already explained - ARM's __ffs() >>>>> differs from other architectures in that it ends up being an int, whereas >>>>> almost everyone else is unsigned long. >>>>> >>>>> The fix is to fix ARMs __ffs() to conform to other architectures. >>>>> >>>> I was just about to cross-post your reply here. Obviously I didn't think >>>> this far when I made $subject fix. >>>> >>>> So lets ignore the $subject patch which is not correct. Sorry for noise >>> >>> Well, here we are, a month on, and this still remains unfixed despite >>> my comments pointing to what the problem is. So, here's a patch to fix >>> this problem the correct way. I took the time to add some comments to >>> these functions as I find that I wonder about their return values, and >>> these comments make the patch a little larger than it otherwise would be. >>> >> The $subject warning fix [1] is already picked by Andrew with your ack >> and its in his queue [2] >> >>> This patch makes their types match exactly with x86's definitions of >>> the same, which is the basic problem: on ARM, they all took "int" values >>> and returned "int"s, which leads to min() in nobootmem.c complaining. >>> >> Not sure if you missed the thread but the right fix was picked. Ofcourse >> you do have additional clz optimisation in updated patch and some comments >> on those functions. > > The problem here is that the patch fixing this is going via akpm's tree > (why?) yet you want the code which introduces the warning to be merged > via my tree. > > It seems to me to be absolutely silly to have code introduce a warning > yet push the fix for the warning via a completely different tree... > I mixed it up. Sorry. Some how I thought there was some other build configuration thrown the same warning with memblock series and hence suggested the patch to go via Andrew's tree. Regards, Santosh
On Mon, 13 Jan 2014 09:27:44 -0500 Santosh Shilimkar <santosh.shilimkar@ti.com> wrote: > > It seems to me to be absolutely silly to have code introduce a warning > > yet push the fix for the warning via a completely different tree... > > > I mixed it up. Sorry. Some how I thought there was some other build > configuration thrown the same warning with memblock series and hence > suggested the patch to go via Andrew's tree. Yes, I too had assumed that the warning was caused by the bootmem patches in -mm. But it in fact occurs in Linus's current tree. I'll drop mm-arm-fix-arms-__ffs-to-conform-to-avoid-warning-with-no_bootmem.patch and I'll assume that rmk will fix this up at an appropriate time.
On Mon, Jan 13, 2014 at 03:31:28PM -0800, Andrew Morton wrote: > On Mon, 13 Jan 2014 09:27:44 -0500 Santosh Shilimkar <santosh.shilimkar@ti.com> wrote: > > > > It seems to me to be absolutely silly to have code introduce a warning > > > yet push the fix for the warning via a completely different tree... > > > > > I mixed it up. Sorry. Some how I thought there was some other build > > configuration thrown the same warning with memblock series and hence > > suggested the patch to go via Andrew's tree. > > Yes, I too had assumed that the warning was caused by the bootmem > patches in -mm. > > But it in fact occurs in Linus's current tree. I'll drop > mm-arm-fix-arms-__ffs-to-conform-to-avoid-warning-with-no_bootmem.patch > and I'll assume that rmk will fix this up at an appropriate time. Thanks. I'll apply my version and then I can pull Santosh's nobootmem changes (which I've had a couple of times already) without adding to the warnings.
On Monday 13 January 2014 06:33 PM, Russell King - ARM Linux wrote: > On Mon, Jan 13, 2014 at 03:31:28PM -0800, Andrew Morton wrote: >> On Mon, 13 Jan 2014 09:27:44 -0500 Santosh Shilimkar <santosh.shilimkar@ti.com> wrote: >> >>>> It seems to me to be absolutely silly to have code introduce a warning >>>> yet push the fix for the warning via a completely different tree... >>>> >>> I mixed it up. Sorry. Some how I thought there was some other build >>> configuration thrown the same warning with memblock series and hence >>> suggested the patch to go via Andrew's tree. >> >> Yes, I too had assumed that the warning was caused by the bootmem >> patches in -mm. >> >> But it in fact occurs in Linus's current tree. I'll drop >> mm-arm-fix-arms-__ffs-to-conform-to-avoid-warning-with-no_bootmem.patch >> and I'll assume that rmk will fix this up at an appropriate time. > > Thanks. I'll apply my version and then I can pull Santosh's nobootmem > changes (which I've had a couple of times already) without adding to > the warnings. > Thanks Andrew and Russell for sorting out the patch queue. Regards, Santosh
On Sun, 12 Jan 2014, Russell King - ARM Linux wrote: > This patch makes their types match exactly with x86's definitions of > the same, which is the basic problem: on ARM, they all took "int" values > and returned "int"s, which leads to min() in nobootmem.c complaining. > > arch/arm/include/asm/bitops.h | 54 +++++++++++++++++++++++++++++++++++-------- > 1 file changed, 44 insertions(+), 10 deletions(-) For the record: Acked-by: Nicolas Pitre <nico@linaro.org> The reason why macros were used at the time this was originally written is because gcc used to have issues forwarding the constant nature of a variable down multiple levels of inline functions and __builtin_constant_p() always returned false. But that was quite a long time ago. > diff --git a/arch/arm/include/asm/bitops.h b/arch/arm/include/asm/bitops.h > index e691ec91e4d3..b2e298a90d76 100644 > --- a/arch/arm/include/asm/bitops.h > +++ b/arch/arm/include/asm/bitops.h > @@ -254,25 +254,59 @@ static inline int constant_fls(int x) > } > > /* > - * On ARMv5 and above those functions can be implemented around > - * the clz instruction for much better code efficiency. > + * On ARMv5 and above those functions can be implemented around the > + * clz instruction for much better code efficiency. __clz returns > + * the number of leading zeros, zero input will return 32, and > + * 0x80000000 will return 0. > */ > +static inline unsigned int __clz(unsigned int x) > +{ > + unsigned int ret; > + > + asm("clz\t%0, %1" : "=r" (ret) : "r" (x)); > > + return ret; > +} > + > +/* > + * fls() returns zero if the input is zero, otherwise returns the bit > + * position of the last set bit, where the LSB is 1 and MSB is 32. > + */ > static inline int fls(int x) > { > - int ret; > - > if (__builtin_constant_p(x)) > return constant_fls(x); > > - asm("clz\t%0, %1" : "=r" (ret) : "r" (x)); > - ret = 32 - ret; > - return ret; > + return 32 - __clz(x); > +} > + > +/* > + * __fls() returns the bit position of the last bit set, where the > + * LSB is 0 and MSB is 31. Zero input is undefined. > + */ > +static inline unsigned long __fls(unsigned long x) > +{ > + return fls(x) - 1; > +} > + > +/* > + * ffs() returns zero if the input was zero, otherwise returns the bit > + * position of the first set bit, where the LSB is 1 and MSB is 32. > + */ > +static inline int ffs(int x) > +{ > + return fls(x & -x); > +} > + > +/* > + * __ffs() returns the bit position of the first bit set, where the > + * LSB is 0 and MSB is 31. Zero input is undefined. > + */ > +static inline unsigned long __ffs(unsigned long x) > +{ > + return ffs(x) - 1; > } > > -#define __fls(x) (fls(x) - 1) > -#define ffs(x) ({ unsigned long __t = (x); fls(__t & -__t); }) > -#define __ffs(x) (ffs(x) - 1) > #define ffz(x) __ffs( ~(x) ) > > #endif > > > -- > FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up. Estimation > in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad. > Estimate before purchase was "up to 13.2Mbit". > -- > 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/ >
diff --git a/arch/arm/include/asm/bitops.h b/arch/arm/include/asm/bitops.h index e691ec91e4d3..b2e298a90d76 100644 --- a/arch/arm/include/asm/bitops.h +++ b/arch/arm/include/asm/bitops.h @@ -254,25 +254,59 @@ static inline int constant_fls(int x) } /* - * On ARMv5 and above those functions can be implemented around - * the clz instruction for much better code efficiency. + * On ARMv5 and above those functions can be implemented around the + * clz instruction for much better code efficiency. __clz returns + * the number of leading zeros, zero input will return 32, and + * 0x80000000 will return 0. */ +static inline unsigned int __clz(unsigned int x) +{ + unsigned int ret; + + asm("clz\t%0, %1" : "=r" (ret) : "r" (x)); + return ret; +} + +/* + * fls() returns zero if the input is zero, otherwise returns the bit + * position of the last set bit, where the LSB is 1 and MSB is 32. + */ static inline int fls(int x) { - int ret; - if (__builtin_constant_p(x)) return constant_fls(x); - asm("clz\t%0, %1" : "=r" (ret) : "r" (x)); - ret = 32 - ret; - return ret; + return 32 - __clz(x); +} + +/* + * __fls() returns the bit position of the last bit set, where the + * LSB is 0 and MSB is 31. Zero input is undefined. + */ +static inline unsigned long __fls(unsigned long x) +{ + return fls(x) - 1; +} + +/* + * ffs() returns zero if the input was zero, otherwise returns the bit + * position of the first set bit, where the LSB is 1 and MSB is 32. + */ +static inline int ffs(int x) +{ + return fls(x & -x); +} + +/* + * __ffs() returns the bit position of the first bit set, where the + * LSB is 0 and MSB is 31. Zero input is undefined. + */ +static inline unsigned long __ffs(unsigned long x) +{ + return ffs(x) - 1; } -#define __fls(x) (fls(x) - 1) -#define ffs(x) ({ unsigned long __t = (x); fls(__t & -__t); }) -#define __ffs(x) (ffs(x) - 1) #define ffz(x) __ffs( ~(x) ) #endif