On Mon, Oct 18, 2010 at 10:22:59PM +0900, Akinobu Mita wrote: > 2010/10/18 Russell King - ARM Linux <linux@xxxxxxxxxxxxxxxx>: > > 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. -- To unsubscribe from this list: send the line "unsubscribe linux-arch" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html