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

[Russell King - ARM Linux <linux@arm.linux.org.uk>]

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.