[PATCH 04/22] arm: introduce little endian bitops

[PATCH 04/22] arm: introduce little endian bitops

[Akinobu Mita]

Introduce little endian bit operations by renaming native ext2 bit
operations. The ext2 bit operations are kept by using little endian
bit operations until the conversions are finished.

Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
Cc: Russell King <linux@arm.linux.org.uk>
Cc: linux-arm-kernel at lists.infradead.org
---
 arch/arm/include/asm/bitops.h |   46 +++++++++++++++++++++++++++++-----------
 1 files changed, 33 insertions(+), 13 deletions(-)

diff --git a/arch/arm/include/asm/bitops.h b/arch/arm/include/asm/bitops.h
index 338ff19..d8af4af 100644
--- a/arch/arm/include/asm/bitops.h
+++ b/arch/arm/include/asm/bitops.h
@@ -303,41 +303,61 @@ static inline int fls(int x)
 #include <asm-generic/bitops/hweight.h>
 #include <asm-generic/bitops/lock.h>
 
+#define __set_le_bit(nr, p)			\
+	__set_bit(WORD_BITOFF_TO_LE(nr), (p))
+#define __clear_le_bit(nr, p)			\
+	__clear_bit(WORD_BITOFF_TO_LE(nr), (p))
+#define __test_and_set_le_bit(nr, p)			\
+		__test_and_set_bit(WORD_BITOFF_TO_LE(nr), (p))
+#define test_and_set_le_bit(lock, nr, p)          \
+		test_and_set_bit(WORD_BITOFF_TO_LE(nr), (p))
+#define __test_and_clear_le_bit(nr, p)			\
+		__test_and_clear_bit(WORD_BITOFF_TO_LE(nr), (p))
+#define test_and_clear_le_bit(lock, nr, p)	\
+		test_and_clear_bit(WORD_BITOFF_TO_LE(nr), (p))
+#define test_le_bit(nr, p)			\
+		test_bit(WORD_BITOFF_TO_LE(nr), (p))
+#define find_first_zero_le_bit(p, sz)		\
+		_find_first_zero_bit_le(p, sz)
+#define find_next_zero_le_bit(p, sz, off)	\
+		_find_next_zero_bit_le(p, sz, off)
+#define find_next_le_bit(p, sz, off) \
+		_find_next_bit_le(p, sz, off)
 /*
  * Ext2 is defined to use little-endian byte ordering.
  * These do not need to be atomic.
  */
 #define ext2_set_bit(nr,p)			\
-		__test_and_set_bit(WORD_BITOFF_TO_LE(nr), (unsigned long *)(p))
+		__test_and_set_le_bit(nr, (unsigned long *)(p))
 #define ext2_set_bit_atomic(lock,nr,p)          \
-                test_and_set_bit(WORD_BITOFF_TO_LE(nr), (unsigned long *)(p))
+		test_and_set_le_bit(nr, (unsigned long *)(p))
 #define ext2_clear_bit(nr,p)			\
-		__test_and_clear_bit(WORD_BITOFF_TO_LE(nr), (unsigned long *)(p))
+		__test_and_clear_le_bit(nr, (unsigned long *)(p))
 #define ext2_clear_bit_atomic(lock,nr,p)        \
-                test_and_clear_bit(WORD_BITOFF_TO_LE(nr), (unsigned long *)(p))
+		test_and_clear_le_bit(nr, (unsigned long *)(p))
 #define ext2_test_bit(nr,p)			\
-		test_bit(WORD_BITOFF_TO_LE(nr), (unsigned long *)(p))
+		test_le_bit(nr, (unsigned long *)(p))
 #define ext2_find_first_zero_bit(p,sz)		\
-		_find_first_zero_bit_le(p,sz)
+		find_first_zero_le_bit((unsigned long *)(p), sz)
 #define ext2_find_next_zero_bit(p,sz,off)	\
-		_find_next_zero_bit_le(p,sz,off)
+		find_next_zero_le_bit((unsigned long *)(p), sz, off)
 #define ext2_find_next_bit(p, sz, off) \
-		_find_next_bit_le(p, sz, off)
+		find_next_le_bit((unsigned long *)(p), sz, off)
 
 /*
  * Minix is defined to use little-endian byte ordering.
  * These do not need to be atomic.
  */
 #define minix_set_bit(nr,p)			\
-		__set_bit(WORD_BITOFF_TO_LE(nr), (unsigned long *)(p))
+		__set_le_bit(nr, (unsigned long *)(p))
 #define minix_test_bit(nr,p)			\
-		test_bit(WORD_BITOFF_TO_LE(nr), (unsigned long *)(p))
+		test_le_bit(nr, (unsigned long *)(p))
 #define minix_test_and_set_bit(nr,p)		\
-		__test_and_set_bit(WORD_BITOFF_TO_LE(nr), (unsigned long *)(p))
+		__test_and_set_le_bit(nr, (unsigned long *)(p))
 #define minix_test_and_clear_bit(nr,p)		\
-		__test_and_clear_bit(WORD_BITOFF_TO_LE(nr), (unsigned long *)(p))
+		__test_and_clear_le_bit(nr, (unsigned long *)(p))
 #define minix_find_first_zero_bit(p,sz)		\
-		_find_first_zero_bit_le(p,sz)
+		find_first_zero_le_bit((unsigned long *)(p), sz)
 
 #endif /* __KERNEL__ */
 
-- 
1.7.1.231.gd0b16

[PATCH 04/22] arm: introduce little endian bitops

[Russell King - ARM Linux]

On Fri, Oct 15, 2010 at 06:46:03PM +0900, Akinobu Mita wrote:
> Introduce little endian bit operations by renaming native ext2 bit
> operations. The ext2 bit operations are kept by using little endian
> bit operations until the conversions are finished.

Can you explain why we need another level of indirection rather than
using asm-generic/bitops/le.h, asm-generic/bitops/minix.h and
asm-generic/bitops/ext2-non-atomic.h ?

[PATCH 04/22] arm: introduce little endian bitops

[Akinobu Mita]

2010/10/18 Russell King - ARM Linux <linux@arm.linux.org.uk>:
> On Fri, Oct 15, 2010 at 06:46:03PM +0900, Akinobu Mita wrote:
>> Introduce little endian bit operations by renaming native ext2 bit
>> operations. The ext2 bit operations are kept by using little endian
>> bit operations until the conversions are finished.
>
> Can you explain why we need another level of indirection rather than
> using asm-generic/bitops/le.h, asm-generic/bitops/minix.h and
> asm-generic/bitops/ext2-non-atomic.h ?

Sorry for not CCing the cover letter of this patch series.

Currently there are no common little-endian bit operations for all
architectures, although some architectures implicitly include
asm-generic/bitops/le.h through asm-generic/bitops/minix-le.h or
asm-generic/bitops/ext2-non-atomic.h.

So some drivers (net/rds/cong.c and virt/kvm/kvm_main.c) need to
include asm/bitops/le.h directly. When I tried to remove the
direct inclusion of asm-generic/bitops/le.h by using ext2_*(),
several people prefer another solution like this patch series does.

This patch series introduces little-endian bit operations for
all architectures and convert all ext2 non-atomic bit operations
and minix bit operations to use little-endian bit operations.
it enables to remove ext2 non-atomic and minix bit operations
from asm/bitops.h. The reason they should be removed from
asm/bitops.h is as follows:

For ext2 non-atomic bit operations, they are used for little-endian
byte order bitmap access by some filesystems and modules.
But using ext2_*() functions on a module other than ext2 filesystem
makes someone feel strange.

For minix bit operations, they are only used by minix filesystem
and useless by other modules. Because byte order of inode and block
bitmap is defferent on each architectures.

There are several issues including arm part. So I'm now fixing them.

[PATCH 04/22] arm: introduce little endian bitops

[Russell King - ARM Linux]

On Mon, Oct 18, 2010 at 10:22:59PM +0900, Akinobu Mita wrote:
> 2010/10/18 Russell King - ARM Linux <linux@arm.linux.org.uk>:
> > On Fri, Oct 15, 2010 at 06:46:03PM +0900, Akinobu Mita wrote:
> >> Introduce little endian bit operations by renaming native ext2 bit
> >> operations. The ext2 bit operations are kept by using little endian
> >> bit operations until the conversions are finished.
> >
> > Can you explain why we need another level of indirection rather than
> > using asm-generic/bitops/le.h, asm-generic/bitops/minix.h and
> > asm-generic/bitops/ext2-non-atomic.h ?
> 
> Sorry for not CCing the cover letter of this patch series.
> 
> Currently there are no common little-endian bit operations for all
> architectures, although some architectures implicitly include
> asm-generic/bitops/le.h through asm-generic/bitops/minix-le.h or
> asm-generic/bitops/ext2-non-atomic.h.
> 
> So some drivers (net/rds/cong.c and virt/kvm/kvm_main.c) need to
> include asm/bitops/le.h directly. When I tried to remove the
> direct inclusion of asm-generic/bitops/le.h by using ext2_*(),
> several people prefer another solution like this patch series does.
> 
> This patch series introduces little-endian bit operations for
> all architectures and convert all ext2 non-atomic bit operations
> and minix bit operations to use little-endian bit operations.
> it enables to remove ext2 non-atomic and minix bit operations
> from asm/bitops.h. The reason they should be removed from
> asm/bitops.h is as follows:
> 
> For ext2 non-atomic bit operations, they are used for little-endian
> byte order bitmap access by some filesystems and modules.
> But using ext2_*() functions on a module other than ext2 filesystem
> makes someone feel strange.
> 
> For minix bit operations, they are only used by minix filesystem
> and useless by other modules. Because byte order of inode and block
> bitmap is defferent on each architectures.
> 
> There are several issues including arm part. So I'm now fixing them.

le.h provides:

	generic___set_le_bit
	generic___test_and_set_le_bit
	...

Your new patches provide:

	__set_le_bit
	__test_and_set_le_bit

Would it not be better to have le.h provide one set of these LE bitops
(called either generic___set_le_bit or __set_le_bit - which ever you
prefer), and then have everyone using those common names (including
minix-le.h and ext2-non-atomic.h) rather than adding a whole series of
new bitop macros to the current mess?

To put it another way, if we're providing a set of guaranteed-little-endian
bitops, I'd like to see ARM doing this:

#define __test_and_set_le_bit(nr,p) ...

#include <asm-generic/bitops/minix-le.h>
#include <asm-generic/bitops/ext2-non-atomic.h>

where ext2-non-atomic.h could just be:

#define ext2_set_bit(nr,addr) __test_and_set_le_bit((nr),(unsigned long *)(addr))

instead of defining its own minix and ext2 bitops as we do now:

#ifndef __ARMEB__
#define WORD_BITOFF_TO_LE(x)            ((x))
#else
#define WORD_BITOFF_TO_LE(x)            ((x) ^ 0x18)
#endif
#define ext2_set_bit(nr,p)                      \
                __test_and_set_bit(WORD_BITOFF_TO_LE(nr), (unsigned long *)(p))

vs the current 'generic' stuff:

ARM byteorder.h (roughly):
#ifdef __ARMEB__
#define __BIG_ENDIAN 4321
#else
#define __LITTLE_ENDIAN 1234
#endif

le.h:
#define BITOP_LE_SWIZZLE        ((BITS_PER_LONG-1) & ~0x7)
#if defined(__LITTLE_ENDIAN)
#define generic___test_and_set_le_bit(nr, addr) __test_and_set_bit(nr, addr)
#elif defined(__BIG_ENDIAN)
#define generic___test_and_set_le_bit(nr, addr) \
        __test_and_set_bit((nr) ^ BITOP_LE_SWIZZLE, (addr))
#endif

ext2-non-atomic.h:
#define ext2_set_bit(nr,addr)   \
        generic___test_and_set_le_bit((nr),(unsigned long *)(addr))

What I'm trying to say is please don't make the existing mess of bitops
any worse than it currently is.

[PATCH 04/22] arm: introduce little endian bitops

[Russell King - ARM Linux]

On Mon, Oct 18, 2010 at 10:22:59PM +0900, Akinobu Mita wrote:
> 2010/10/18 Russell King - ARM Linux <linux@arm.linux.org.uk>:
> > On Fri, Oct 15, 2010 at 06:46:03PM +0900, Akinobu Mita wrote:
> >> Introduce little endian bit operations by renaming native ext2 bit
> >> operations. The ext2 bit operations are kept by using little endian
> >> bit operations until the conversions are finished.
> >
> > Can you explain why we need another level of indirection rather than
> > using asm-generic/bitops/le.h, asm-generic/bitops/minix.h and
> > asm-generic/bitops/ext2-non-atomic.h ?
> 
> Sorry for not CCing the cover letter of this patch series.

BTW, I've not seen any of the others in this patch series, not even via
linux-arch...

[PATCH 04/22] arm: introduce little endian bitops

[Arnd Bergmann]

On Monday 18 October 2010, Russell King - ARM Linux wrote:
> le.h provides:
> 
>         generic___set_le_bit
>         generic___test_and_set_le_bit
>         ...
> 
> Your new patches provide:
> 
>         __set_le_bit
>         __test_and_set_le_bit
> 
> Would it not be better to have le.h provide one set of these LE bitops
> (called either generic___set_le_bit or __set_le_bit - which ever you
> prefer), and then have everyone using those common names (including
> minix-le.h and ext2-non-atomic.h) rather than adding a whole series of
> new bitop macros to the current mess?

One optimization I can think of for the ARM headers would be to only
define find_first_zero_le_bit, find_next_zero_le_bit and find_next_le_bit
in arch/arm/include/asm/bitops.h and take the other definitions from
asm-generic/bitops/le.h, which encloses then duplicate ones in #ifdef.

> To put it another way, if we're providing a set of guaranteed-little-endian
> bitops, I'd like to see ARM doing this:
> 
> #define __test_and_set_le_bit(nr,p) ...
> 
> #include <asm-generic/bitops/minix-le.h>
> #include <asm-generic/bitops/ext2-non-atomic.h>
> 
> where ext2-non-atomic.h could just be:
> 
> #define ext2_set_bit(nr,addr) __test_and_set_le_bit((nr),(unsigned long *)(addr))

Note that patches 20 and 22 of the series completely eliminate the
the minix and ext2 definitions, putting them into architecture independent
code in those two file systems where they belong.

Adding the new definitions in patch 4 is just a logical step before removing
the old definitions in the later patches while maintaining bisectability.

> What I'm trying to say is please don't make the existing mess of bitops
> any worse than it currently is.

The series currently adds 20 lines to the arm code (could be reduced to
6 lines), but removes 26 lines which are essentially architecture
independent and shouldn't be there to start with. I'd call that the
opposite of making the mess worse.

	Arnd

[PATCH 04/22] arm: introduce little endian bitops

[Russell King - ARM Linux]

On Mon, Oct 18, 2010 at 04:45:12PM +0200, Arnd Bergmann wrote:
> Note that patches 20 and 22 of the series completely eliminate the
> the minix and ext2 definitions, putting them into architecture independent
> code in those two file systems where they belong.

Good.

> Adding the new definitions in patch 4 is just a logical step before removing
> the old definitions in the later patches while maintaining bisectability.

In which case I don't have a problem with the series.

> > What I'm trying to say is please don't make the existing mess of bitops
> > any worse than it currently is.
> 
> The series currently adds 20 lines to the arm code (could be reduced to
> 6 lines), but removes 26 lines which are essentially architecture
> independent and shouldn't be there to start with. I'd call that the
> opposite of making the mess worse.

Right - if I could've seen the rest of the series, then maybe I'd have
known that.  However, I seemed to have silently dropped off linux-arch
back in April and only just noticed, which means I've missed rather a
lot...