Re: [KJ] BIT macro cleanup

Re: [KJ] BIT macro cleanup

[walter harms]

Milind Choudhary wrote:
> 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

> 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
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] BIT macro cleanup

[Milind Choudhary]

On 2/8/07, walter harms <wharms@bfs.de> wrote:
>
>
> > 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
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] BIT macro cleanup

[Jaco Kroon]

[-- Attachment #1.1: Type: text/plain, Size: 1215 bytes --]

Milind Choudhary wrote:
> On 2/8/07, walter harms <wharms@bfs.de> wrote:
> 
>>
>>>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))

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 ...

> #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

[-- Attachment #1.2: S/MIME Cryptographic Signature --]
[-- Type: application/x-pkcs7-signature, Size: 3233 bytes --]

[-- Attachment #2: Type: text/plain, Size: 168 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] BIT macro cleanup

[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 }

./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
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] BIT macro cleanup

[Richard Knutsson]

Milind Choudhary wrote:
> ./drivers/net/s2io.h:17:
> #define BIT(loc)                (0x8000000000000000ULL >> (loc))
>   
[snip]
> 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
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] BIT macro cleanup

[Milind Choudhary]

On 2/21/07, Richard Knutsson <ricknu-0@student.ltu.se> wrote:
> Milind Choudhary wrote:
> > ./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

> 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
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] BIT macro cleanup

[Richard Knutsson]

Milind Choudhary wrote:
> On 2/21/07, Richard Knutsson <ricknu-0@student.ltu.se> wrote:
>> 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.
> 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
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] BIT macro cleanup

[Milind Choudhary]

Richard Knutsson wrote:

>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 :)]
   ~ ~
----------------
 /|\       /|\


>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
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] BIT macro cleanup

[Richard Knutsson]

Milind Choudhary wrote:
> Richard Knutsson wrote:
>
>> 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 :)]
>   ~ ~
> ----------------
> /|\       /|\
>
Nice :)
>> 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);

> & for others
> #define BIT(nr) (1UL<<(nr))
> #define LLBIT(nr) (1ULL<<(nr))
Looks good
>
> 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
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] BIT macro cleanup

[Milind Choudhary]

On 2/21/07, Richard Knutsson <ricknu-0@student.ltu.se> wrote:
> > 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

> > & for others
> > #define BIT(nr) (1UL<<(nr))
> > #define LLBIT(nr) (1ULL<<(nr))
> Looks good
> >
> > 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.

-- 
Milind Arun Choudhary
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] BIT macro cleanup

[Richard Knutsson]

Milind Choudhary wrote:
> On 2/21/07, Richard Knutsson <ricknu-0@student.ltu.se> wrote:
>> > 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.
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
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] BIT macro cleanup

[Richard Knutsson]

Milind Choudhary wrote:
> 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.
> ./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
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] BIT macro cleanup

[Darren Jenkins]

G'day people

On 2/21/07, Milind Choudhary <milindchoudhary@gmail.com> wrote:
> On 2/21/07, Richard Knutsson <ricknu-0@student.ltu.se> 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
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] BIT macro cleanup

[Darren Jenkins]

On 2/21/07, Darren Jenkins <darrenrjenkins@gmail.com> wrote:

> 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 ?

Hang on that is obviously wrong, and I should think before I type.

Darren J.
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] BIT macro cleanup

[Milind Choudhary]

On 2/21/07, Richard Knutsson <ricknu-0@student.ltu.se> wrote:

> > ./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

> -#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 :;
> +
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;

-- 
Milind Arun Choudhary
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] BIT macro cleanup

[Milind Choudhary]

On 2/21/07, Richard Knutsson <ricknu-0@student.ltu.se> wrote:
> Milind Choudhary wrote:
> > ./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
-- 
Milind Arun Choudhary
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] BIT macro cleanup

[Håkon Løvdal]

On 21/02/07, Milind Choudhary <milindchoudhary@gmail.com> 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
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] BIT macro cleanup

[Richard Knutsson]

Milind Choudhary 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)
Sounds good! But I think Håkon got a point, both on RESET -> CLEAR and 
that mask can be more then one bit.
>
> 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
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

[KJ] BIT macro cleanup

[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.

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?
Thoughts.

Milind A Choudhary
Pune-India
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors