Skip to contentSkip to conversation

BIT macro cleanup

KJ walter harms 6 participants 18 messages Feb 8, 2007 – Feb 21, 2007 Kernel janitors

lore
Share & tools
lore

walter harms

author
#0 lore raw
Quoted 8 lines · Milind Choudhary

Hi all

I would like to take up the BIT macro cleanup.
http://www.fsdev.net/wiki/index.php?title=BIT

I think include/linux/bitops.h would be the best place to add the BIT macro. Let me know if this is fine.

sounds obvious

Quoted 4 lines

There are different variants of BIT using (long|unsigned long|unsigned|unsigned char) etc.
Do we need to create different macros for each type Or a single one which takes type as input?

bit operations make only sense on integer, so why not stay with long and cast it down if needed ? so you have only one function doing so.

please what ever you do do not forget to document that and write some examples. the linux kernal is large and imho the bigest problem is that the programmers do not know what nice supportfunctions are available.

my 2 cents,
walter

_______________________________________________
Kernel-janitors mailing list
[email protected]
https://lists.osdl.org/mailman/listinfo/kernel-janitors

#1 lore raw
Quoted 9 lines · walter harms
Quoted 6 lines

There are different variants of BIT using (long|unsigned long|unsigned|unsigned char) etc.
Do we need to create different macros for each type Or a single one which takes type as input?

bit operations make only sense on integer, so why not stay with long and cast it down if needed ? so you have only one function doing so.

Michael Veeck had submitted a patch for the same before
http://lists.osdl.org/pipermail/kernel-janitors/2005-August/004737.html

there was some discussion on the list,which did not reach any conclusion I guess.
so we have
#define BIT(x) (1UL << (x) )
#define BIT(x) (1UL << ((x) % BITS_PER_LONG))
#define BIT(x) (1ULL << (__builtin_constant_p(x) ? (x) : (x) % 64)

can any one please throw some light on ...
Milind A Choudhary
_______________________________________________
Kernel-janitors mailing list
[email protected]
https://lists.osdl.org/mailman/listinfo/kernel-janitors

#2 lore raw
In reply to walter harms
Quoted 15 lines · Milind Choudhary
Quoted 8 lines · walter harms
Quoted 5 lines

There are different variants of BIT using (long|unsigned long|unsigned|unsigned char) etc.
Do we need to create different macros for each type Or a single one which takes type as input?

bit operations make only sense on integer, so why not stay with long and cast it down if needed ? so you have only one function doing so.

Michael Veeck had submitted a patch for the same before
http://lists.osdl.org/pipermail/kernel-janitors/2005-August/004737.html

Quoted 6 lines

there was some discussion on the list,which did not reach any conclusion I guess.
so we have

#define BIT(x) (1UL << (x) )
#define BIT(x) (1UL << ((x) % BITS_PER_LONG))

These two are identical (on x86 hardware at least), except that BITS_PER_LONG depends on the architecture, 32 on x86 and 64 on x86_64, and the modulus applied by the CPU depends on the size of x ...

Quoted 1 line

#define BIT(x) (1ULL << (__builtin_constant_p(x) ? (x) : (x) % 64)

This won't compile (bracket mismatch), but I can't see why you'd want to differentiate between x being a compiletime constant or a runtime calculated value ... it just doesn't make sense for me.

Jaco

_______________________________________________
Kernel-janitors mailing list
[email protected]
https://lists.osdl.org/mailman/listinfo/kernel-janitors

#3 lore raw
In reply to walter harms

There are 3 BIT macros which aren't really upto their name

./drivers/i2c/busses/i2c-pxa.c:71:
#define BIT(m, s, u) { .mask = m, .set = s, .unset = u }

./drivers/serial/amba-pl011.c:265:
#define BIT(uartbit, tiocmbit) \

./drivers/net/s2io.h:17:
#define BIT(loc) (0x8000000000000000ULL >> (loc))

they will now clash with the new BIT macro in bitops.h for the time being i just did #undef BIT

looking for some meaningful names.

third one is close to our BIT...but defines the bit position in reverse order thought of moving it to bitops.h with name BITREV..
wouldn't it be mistaken for bit reversal?
suggest a name plz.

Milind Arun Choudhary
_______________________________________________
Kernel-janitors mailing list
[email protected]
https://lists.osdl.org/mailman/listinfo/kernel-janitors

#4 lore raw
In reply to walter harms
Quoted 3 lines · Milind Choudhary

./drivers/net/s2io.h:17:
#define BIT(loc) (0x8000000000000000ULL >> (loc))

[snip]

Quoted 5 lines

third one is close to our BIT...but defines the bit position in reverse order thought of moving it to bitops.h with name BITREV..
wouldn't it be mistaken for bit reversal?
suggest a name plz.

What about just delete that definition and fix those who uses it to"our" BIT? (since it is always 64 bits it should be BIT(63 - x), right?)

Richard Knutsson

PS
BTW, how is BIT() supposed to be defined? Is it:

#define BIT(x) (1ULL << (x))

_______________________________________________
Kernel-janitors mailing list
[email protected]
https://lists.osdl.org/mailman/listinfo/kernel-janitors

#5 lore raw
In reply to walter harms
Quoted 5 lines · Richard Knutsson
Quoted 2 lines · Milind Choudhary

./drivers/net/s2io.h:17:
#define BIT(loc) (0x8000000000000000ULL >> (loc))

What about just delete that definition and fix those who uses it to"our" BIT? (since it is always 64 bits it should be BIT(63 - x), right?)

sure . would look into that

Quoted 2 lines

BTW, how is BIT() supposed to be defined? Is it:
#define BIT(x) (1ULL << (x))

initially i started of with the same
but it broke for users of linux/input.h..overflow!

just keying in my understanding about the same CMIIW in <linux/input.h> there are some macros like #define NBITS(x) (((x)/BITS_PER_LONG)+1) #define BIT(x) (1UL<<((x)%BITS_PER_LONG)) #define LONG(x) ((x)/BITS_PER_LONG)

These macros are used to get the bitmasks in an array, when the number of bits exceeds BITS_PER_LONG.

NBITS macro tells us number of elements required in the array say i have 78 different keys & BITS_PER_LONG is 32 then I need 3 elements in the array to have unique bitmask for each key.

e.g.
unsigned long keybit[NBITS(KEY_MAX)];
keybit[LONG(BTN_TOUCH)] = BIT(BTN_TOUCH);

LONG indexes in the array & BIT gives a 32/64 bit mask.

so i've now changed BIT to something like

#define BIT(nr) (1UL<<((nr)%BITS_PER_LONG))

Milind Arun Choudhary
_______________________________________________
Kernel-janitors mailing list
[email protected]
https://lists.osdl.org/mailman/listinfo/kernel-janitors

#6 lore raw
In reply to walter harms
Quoted 5 lines · Milind Choudhary
Quoted 2 lines · Richard Knutsson

BTW, how is BIT() supposed to be defined? Is it:
#define BIT(x) (1ULL << (x))

initially i started of with the same
but it broke for users of linux/input.h..overflow!

Ouch. I did some random tests with other BIT() were I changed it from UL to ULL and they all compiled without problem.

Quoted 3 lines

so i've now changed BIT to something like

#define BIT(nr) (1UL<<((nr)%BITS_PER_LONG))

Am not sure what the %BITS_PER_LONG adds other then hiding bugs. I tested with:
long a = 1UL << 32;
and GCC (4.1) said:
test.c:28: warning: left shift count >= width of type

And what about an LLBIT() for long long?

Richard Knutsson

_______________________________________________
Kernel-janitors mailing list
[email protected]
https://lists.osdl.org/mailman/listinfo/kernel-janitors

#7 lore raw
In reply to walter harms
Quoted 4 lines · Richard Knutsson

I tested with:
long a = 1UL << 32;
and GCC (4.1) said:
test.c:28: warning: left shift count >= width of type

with a 32 bit long
the bits in 1UL would be [in binary]
00000000000000000000000000000001

if I do 1UL << 31,I'd get

10000000000000000000000000000000

but with 1UL <<32 the poor "1" would fall down & die

           00000000000000000000000000000000
         1
        1
       1
      1
     1

~ ~
~ * ~ [inspired by the ASCII art..just trying my hand at it :)]
~ ~
----------------
/|\ /|\

Quoted 2 lines

And what about an LLBIT() for long long?
Am not sure what the %BITS_PER_LONG adds other then hiding bugs.

yes agree.
but we need it for users of <linux/input.h>
how abt providing them a separate one

#define BITWRAP(nr) (1UL<<((nr)%BITS_PER_LONG))

& for others
#define BIT(nr) (1UL<<(nr))
#define LLBIT(nr) (1ULL<<(nr))

so that gcc only will guide them whether to use BIT or LLBIT depending upon size of long & the "nr" value .

Milind Arun Choudhary
_______________________________________________
Kernel-janitors mailing list
[email protected]
https://lists.osdl.org/mailman/listinfo/kernel-janitors

#8 lore raw
In reply to walter harms
Quoted 22 lines · Milind Choudhary
Quoted 4 lines · Richard Knutsson

I tested with:
long a = 1UL << 32;
and GCC (4.1) said:
test.c:28: warning: left shift count >= width of type

with a 32 bit long
the bits in 1UL would be [in binary]
00000000000000000000000000000001

if I do 1UL << 31,I'd get

10000000000000000000000000000000

but with 1UL <<32 the poor "1" would fall down & die

          00000000000000000000000000000000
        1
       1
      1
     1
    1
Quoted 6 lines

~ ~
~ * ~ [inspired by the ASCII art..just trying my hand at it :)]
~ ~
----------------
/|\ /|\

Nice :)

Quoted 7 lines
Quoted 2 lines

And what about an LLBIT() for long long?
Am not sure what the %BITS_PER_LONG adds other then hiding bugs.

yes agree.
but we need it for users of <linux/input.h>
how abt providing them a separate one

#define BITWRAP(nr) (1UL<<((nr)%BITS_PER_LONG))

Would it be ugly to just do:
BIT(nr % BITS_PER_LONG);
I mean, it clearly shows then what they want + what about if someone would like to wrap on a byte? With this, it is just:
BIT(nr % BITS_PER_BYTE);

Quoted 3 lines

& for others
#define BIT(nr) (1UL<<(nr))
#define LLBIT(nr) (1ULL<<(nr))

Looks good

Quoted 3 lines

so that gcc only will guide them whether to use BIT or LLBIT depending upon size of long & the "nr" value .

How do you mean, gcc "will guide them"?

Richard Knutsson

_______________________________________________
Kernel-janitors mailing list
[email protected]
https://lists.osdl.org/mailman/listinfo/kernel-janitors

#9 lore raw
In reply to walter harms
Quoted 8 lines · Richard Knutsson
Quoted 3 lines

how abt providing them a separate one

#define BITWRAP(nr) (1UL<<((nr)%BITS_PER_LONG))

Would it be ugly to just do:
BIT(nr % BITS_PER_LONG);
I mean, it clearly shows then what they want + what about if someone would like to wrap on a byte? With this, it is just:
BIT(nr % BITS_PER_BYTE);

hmm..good point

Quoted 9 lines
Quoted 3 lines

& for others
#define BIT(nr) (1UL<<(nr))
#define LLBIT(nr) (1ULL<<(nr))

Looks good

Quoted 3 lines

so that gcc only will guide them whether to use BIT or LLBIT depending upon size of long & the "nr" value .

How do you mean, gcc "will guide them"?

I mean if somebody blindly passes a value greater than BITS_PER_LONG [on architectures where long & long long aren't of same size] gcc would complain with
:warning: left shift count >= width of type
so they will backtrack & come to see whats cooking inside BIT & on the very next line they notice that there is LLBIT which they can use & live happily ever after.

Signature · 5 lines
Milind Arun Choudhary
_______________________________________________
Kernel-janitors mailing list
[email protected]
https://lists.osdl.org/mailman/listinfo/kernel-janitors
#10 lore raw
In reply to walter harms
Quoted 5 lines · Milind Choudhary
Quoted 4 lines · Richard Knutsson
Quoted 2 lines

so that gcc only will guide them whether to use BIT or LLBIT depending upon size of long & the "nr" value .

How do you mean, gcc "will guide them"?

Quoted 7 lines

I mean if somebody blindly passes a value greater than BITS_PER_LONG [on architectures where long & long long aren't of same size] gcc would complain with
:warning: left shift count >= width of type
so they will backtrack & come to see whats cooking inside BIT & on the very next line they notice that there is LLBIT which they can use & live happily ever after.

Ok, I took it first as gcc were suppose to choose between them.

BTW, (out of curiosity) is there still much work before releasing the BIT-patch?

Richard Knutsson

_______________________________________________
Kernel-janitors mailing list
[email protected]
https://lists.osdl.org/mailman/listinfo/kernel-janitors

#11 lore raw
In reply to walter harms
Quoted 5 lines · Milind Choudhary

There are 3 BIT macros which aren't really upto their name

./drivers/i2c/busses/i2c-pxa.c:71:
#define BIT(m, s, u) { .mask = m, .set = s, .unset = u }

BIT seems like a strange name for it. But the least intrusive would be something like SETBIT, I think.

Quoted 3 lines

./drivers/serial/amba-pl011.c:265:
#define BIT(uartbit, tiocmbit) \

Since they are only defined within the function, what about get rid of them? The first one (on 265) seems just straightforward:

-#define BIT(uartbit, tiocmbit) \
- if (status & uartbit) \
- result |= tiocmbit
-
- BIT(UART01x_FR_DCD, TIOCM_CAR);
- BIT(UART01x_FR_DSR, TIOCM_DSR);
- BIT(UART01x_FR_CTS, TIOCM_CTS);
- BIT(UART011_FR_RI, TIOCM_RNG);
-#undef BIT

+	result |= (UART01x_FR_DCD & TIOCM_CAR) ? TIOCM_CAR :;
+	result |= (UART01x_FR_DSR & TIOCM_DSR) ? TIOCM_DSR :;
+	result |= (UART01x_FR_CTS & TIOCM_CTS) ? TIOCM_CTS :;
+	result |= (UART011_FR_RI  & TIOCM_RNG) ? TIOCM_RNG :;

+

but the next is trickier (on 280):

#define BIT(tiocmbit, uartbit)		\
	if (mctrl & tiocmbit)		\
		cr |= uartbit;		\
	else				\
		cr &= ~uartbit

so maybe just call them both LOCAL_SETBIT or so...

Just my 2c
Richard Knutsson

_______________________________________________
Kernel-janitors mailing list
[email protected]
https://lists.osdl.org/mailman/listinfo/kernel-janitors

#12 lore raw
In reply to walter harms

G'day people

Quoted 8 lines · Milind Choudhary

On 2/21/07, Richard Knutsson <[email protected]> wrote:
I mean if somebody blindly passes a value greater than BITS_PER_LONG [on architectures where long & long long aren't of same size] gcc would complain with
:warning: left shift count >= width of type
so they will backtrack & come to see whats cooking inside BIT & on the very next line they notice that there is LLBIT which they can use & live happily ever after.

Sounds fine, but if you want to wrap it, wouldn't it just be easier to do

#define BIT(n) (1<<(n%(sizeof(n) * 8)))

and have it all done automaticly ?

Darren J.
_______________________________________________
Kernel-janitors mailing list
[email protected]
https://lists.osdl.org/mailman/listinfo/kernel-janitors

#14 lore raw
In reply to walter harms
Quoted 4 lines · Richard Knutsson
Quoted 1 line

./drivers/serial/amba-pl011.c:265:

Since they are only defined within the function, what about get rid of them? The first one (on 265) seems just straightforward:

the macro plays with status

unsigned int status = readw(uap->port.membase + UART01x_FR); you seem to have missed that

Quoted 4 lines
(quoted diff) +0 -4
-#define BIT(uartbit, tiocmbit)         \
-       if (status & uartbit)           \
-               result |= tiocmbit
-

Review

Quoted 10 lines
(quoted diff) +5 -5
-       BIT(UART01x_FR_DCD, TIOCM_CAR);
-       BIT(UART01x_FR_DSR, TIOCM_DSR);
-       BIT(UART01x_FR_CTS, TIOCM_CTS);
-       BIT(UART011_FR_RI, TIOCM_RNG);
-#undef BIT
+       result |= (UART01x_FR_DCD & TIOCM_CAR) ? TIOCM_CAR :;
+       result |= (UART01x_FR_DSR & TIOCM_DSR) ? TIOCM_DSR :;
+       result |= (UART01x_FR_CTS & TIOCM_CTS) ? TIOCM_CTS :;
+       result |= (UART011_FR_RI  & TIOCM_RNG) ? TIOCM_RNG :;
+

2 things
1. its (status & uartbit) and not (uartbit & tiocmbit)
2. can't keep the false holder blank ..need to put a zero otherwise gcc will yell at you
error: parse error before ';' token

 +       result |= (status & UART01x_FR_DCD ) ? TIOCM_CAR :0;
 +       result |= (status & UART01x_FR_DSR ) ? TIOCM_DSR :0;
 +       result |= (status & UART01x_FR_CTS ) ? TIOCM_CTS :0;
 +       result |= (status & UART011_FR_RI )    ? TIOCM_RNG :0;
Signature · 5 lines
Milind Arun Choudhary
_______________________________________________
Kernel-janitors mailing list
[email protected]
https://lists.osdl.org/mailman/listinfo/kernel-janitors
#15 lore raw
In reply to walter harms
Quoted 10 lines · Richard Knutsson
Quoted 1 line · Milind Choudhary

./drivers/serial/amba-pl011.c:265:

but the next is trickier (on 280):

#define BIT(tiocmbit, uartbit)          \
        if (mctrl & tiocmbit)           \
                cr |= uartbit;          \
        else                            \

cr &= ~uartbit
so maybe just call them both LOCAL_SETBIT or so...

here its either setting or resetting the bits in "cr" taking "uartbit" as a mask.

there are some setbit & resetbit macros scattered here n there e.g.
./drivers/net/irda/Via-ircc.h

#define SetBit(val,bit) val= (unsigned char ) (val | (0x1 << bit)) // Sets bit to 1
#define ResetBit(val,bit) val= (unsigned char ) (val & ~(0x1 << bit)) // Sets bit to 0

if we move these somewhere near BIT

#define SETBIT(data,mask) (data |= mask)
#define RESETBIT(data,mask) (data &= ~mask)

usually the mask would be generated with BIT

we can use the new ones to get rid of above code
by using "cr" as data & "uartbit" as mask

what say

Signature · 5 lines
Milind Arun Choudhary
_______________________________________________
Kernel-janitors mailing list
[email protected]
https://lists.osdl.org/mailman/listinfo/kernel-janitors
#16 lore raw
In reply to walter harms

On 21/02/07, Milind Choudhary <[email protected]> wrote:> there are some setbit & resetbit macros scattered here n there> e.g.> ./drivers/net/irda/Via-ircc.h>> #define SetBit(val,bit) val= (unsigned char ) (val | (0x1 << bit))> // Sets bit to 1> #define ResetBit(val,bit) val= (unsigned char ) (val & ~(0x1 << bit))> // Sets bit to 0>> if we move these somewhere near BIT>> #define SETBIT(data,mask) (data |= mask)> #define RESETBIT(data,mask) (data &= ~mask)
I would find the term "clear" to be better than "reset" in this context,i.e. SETBIT + CLEARBIT. Also, the macros are not neccessarilyrestricted to operate on a single bit so SETBITS + CLEARBITSwould probably be better.Maybe SET_BITMASK + CLEAR_BITMASK also could be used. BR Håkon Løvdal
_______________________________________________
Kernel-janitors mailing list
[email protected]
https://lists.osdl.org/mailman/listinfo/kernel-janitors

#17 lore raw
In reply to walter harms
Quoted 9 lines · Milind Choudhary

there are some setbit & resetbit macros scattered here n there e.g.
./drivers/net/irda/Via-ircc.h

#define SetBit(val,bit) val= (unsigned char ) (val | (0x1 << bit)) // Sets bit to 1
#define ResetBit(val,bit) val= (unsigned char ) (val & ~(0x1 << bit)) // Sets bit to 0

Quoted 4 lines

if we move these somewhere near BIT

#define SETBIT(data,mask) (data |= mask)
#define RESETBIT(data,mask) (data &= ~mask)

Sounds good! But I think Håkon got a point, both on RESET -> CLEAR and that mask can be more then one bit.

Quoted 7 lines

usually the mask would be generated with BIT

we can use the new ones to get rid of above code
by using "cr" as data & "uartbit" as mask

what say

Mmm, so what about this?
(This I actually compile-tested ;) (after removing <asm/sizes.h> and defining SZ_4K))

	(mctrl & TIOCM_RTS)  ?
		SETBITS(cr, UART011_CR_DTR)  : CLRBITS(cr, UART011_CR_DTR);
	(mctrl & TIOCM_DTR)  ?
		SETBITS(cr, UART011_CR_DTR)  : CLRBITS(cr, UART011_CR_DTR);
	(mctrl & TIOCM_OUT1) ?
		SETBITS(cr, UART011_CR_OUT1) : CLRBITS(cr, UART011_CR_OUT1);
	(mctrl & TIOCM_OUT2) ?
		SETBITS(cr, UART011_CR_OUT2) : CLRBITS(cr, UART011_CR_OUT2);
	(mctrl & TIOCM_LOOP) ?
		SETBITS(cr, UART011_CR_LBE)  : CLRBITS(cr, UART011_CR_LBE);

(I just set them to SETBITS and CLRBITS 'cause I think it looks alright)

Richard Knutsson

_______________________________________________
Kernel-janitors mailing list
[email protected]
https://lists.osdl.org/mailman/listinfo/kernel-janitors