[Suggestion] arch/*/include/asm/bitops.h: about __set_bit() API.

[Suggestion] arch/*/include/asm/bitops.h: about __set_bit() API.

[Chen Gang]

Hello Maintainers:


Several architectures have different __set_bit() API to others, in
standard API, 2nd param of __set_bit() is 'unsigned long *', but:

  in 'min10300', it is 'unsigned char *',
  in 'ia64' and 'alpha', they are 'int' or 'unsigned int'.

If another sub-systems did not notice it, it may lead them to make
mistakes.

It seems related maintainer intended to implement it like that.

So could related maintainers provide the reason for it, so more guys
(e.g. me) can learn about it (at least can avoid related mistakes).


The related information in arch sub-system:

using 'unsigned char *':
  ./mn10300/include/asm/bitops.h:28:#define __set_bit(nr, addr)					\

using 'unsigned int *', implicitly: 
  ./ia64/include/asm/bitops.h:63:__set_bit (int nr, volatile void *addr)

using 'int *', implicitly: (need use 'unsigned', at least)
  ./alpha/include/asm/bitops.h:49:__set_bit(unsigned long nr, volatile void * addr)

using 'unsigned long *' implicitly:
  ./frv/include/asm/bitops.h:164:static inline void __set_bit(unsigned long nr, volatile void *addr)
  
standard API:  
  ./h8300/include/asm/bitops.h:74:#define __set_bit(nr,addr)    set_bit((nr),(addr))
  ./m68k/include/asm/bitops.h:67:#define __set_bit(nr, vaddr)	set_bit(nr, vaddr)
  ./arc/include/asm/bitops.h:293:static inline void __set_bit(unsigned long nr, volatile unsigned long *m)
  ./sh/include/asm/bitops-op32.h:20:static inline void __set_bit(int nr, volatile unsigned long *addr)
  ./hexagon/include/asm/bitops.h:151:static inline void __set_bit(int nr, volatile unsigned long *addr)
  ./s390/include/asm/bitops.h:212:static inline void __set_bit(unsigned long nr, volatile unsigned long *ptr)
  ./x86/include/asm/bitops.h:84:static inline void __set_bit(int nr, volatile unsigned long *addr)
  (others use 'generic')


Thanks.
-- 
Chen Gang

Asianux Corporation

[Suggestion] arch/*/include/asm/bitops.h: about __set_bit() API.

[Chen Gang]

Hello Maintainers:


Several architectures have different __set_bit() API to others, in
standard API, 2nd param of __set_bit() is 'unsigned long *', but:

  in 'min10300', it is 'unsigned char *',
  in 'ia64' and 'alpha', they are 'int' or 'unsigned int'.

If another sub-systems did not notice it, it may lead them to make
mistakes.

It seems related maintainer intended to implement it like that.

So could related maintainers provide the reason for it, so more guys
(e.g. me) can learn about it (at least can avoid related mistakes).


The related information in arch sub-system:

using 'unsigned char *':
  ./mn10300/include/asm/bitops.h:28:#define __set_bit(nr, addr)					\

using 'unsigned int *', implicitly: 
  ./ia64/include/asm/bitops.h:63:__set_bit (int nr, volatile void *addr)

using 'int *', implicitly: (need use 'unsigned', at least)
  ./alpha/include/asm/bitops.h:49:__set_bit(unsigned long nr, volatile void * addr)

using 'unsigned long *' implicitly:
  ./frv/include/asm/bitops.h:164:static inline void __set_bit(unsigned long nr, volatile void *addr)
  
standard API:  
  ./h8300/include/asm/bitops.h:74:#define __set_bit(nr,addr)    set_bit((nr),(addr))
  ./m68k/include/asm/bitops.h:67:#define __set_bit(nr, vaddr)	set_bit(nr, vaddr)
  ./arc/include/asm/bitops.h:293:static inline void __set_bit(unsigned long nr, volatile unsigned long *m)
  ./sh/include/asm/bitops-op32.h:20:static inline void __set_bit(int nr, volatile unsigned long *addr)
  ./hexagon/include/asm/bitops.h:151:static inline void __set_bit(int nr, volatile unsigned long *addr)
  ./s390/include/asm/bitops.h:212:static inline void __set_bit(unsigned long nr, volatile unsigned long *ptr)
  ./x86/include/asm/bitops.h:84:static inline void __set_bit(int nr, volatile unsigned long *addr)
  (others use 'generic')


Thanks.
-- 
Chen Gang

Asianux Corporation

Re: [Suggestion] arch/*/include/asm/bitops.h: about __set_bit() API.

[Tony Luck]

On Sat, Jun 8, 2013 at 3:08 AM, Chen Gang <gang.chen@asianux.com> wrote:
> using 'unsigned int *', implicitly:
>   ./ia64/include/asm/bitops.h:63:__set_bit (int nr, volatile void *addr)

There is some downside on ia64 to your suggestion.  If "addr" is properly
aligned for an "int", but misaligned for a long ... i.e. addr%8 == 4, then I'll
take an unaligned reference trap if I work with long* where the current code
working with int* does not.

Now perhaps all the callers do guarantee long* alignment?  But I don't know.

Apart from uniformity, there doesn't see to be any upside to changing this.

-Tony Luck

Re: [Suggestion] arch/*/include/asm/bitops.h: about __set_bit() API.

[Tony Luck]

On Sat, Jun 8, 2013 at 3:08 AM, Chen Gang <gang.chen@asianux.com> wrote:
> using 'unsigned int *', implicitly:
>   ./ia64/include/asm/bitops.h:63:__set_bit (int nr, volatile void *addr)

There is some downside on ia64 to your suggestion.  If "addr" is properly
aligned for an "int", but misaligned for a long ... i.e. addr%8 = 4, then I'll
take an unaligned reference trap if I work with long* where the current code
working with int* does not.

Now perhaps all the callers do guarantee long* alignment?  But I don't know.

Apart from uniformity, there doesn't see to be any upside to changing this.

-Tony Luck

Re: [Suggestion] arch/*/include/asm/bitops.h: about __set_bit() API.

[Geert Uytterhoeven]

On Tue, Jun 11, 2013 at 12:26 AM, Tony Luck <tony.luck@gmail.com> wrote:
> On Sat, Jun 8, 2013 at 3:08 AM, Chen Gang <gang.chen@asianux.com> wrote:
>> using 'unsigned int *', implicitly:
>>   ./ia64/include/asm/bitops.h:63:__set_bit (int nr, volatile void *addr)
>
> There is some downside on ia64 to your suggestion.  If "addr" is properly
> aligned for an "int", but misaligned for a long ... i.e. addr%8 == 4, then I'll
> take an unaligned reference trap if I work with long* where the current code
> working with int* does not.
>
> Now perhaps all the callers do guarantee long* alignment?  But I don't know.
>
> Apart from uniformity, there doesn't see to be any upside to changing this.

The address pointers have been supposed to be "long *" for a very long time.
Probably alpha (the second official Linux platform) did it
differently, and never
standardized to "long *".

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [Suggestion] arch/*/include/asm/bitops.h: about __set_bit() API.

[Geert Uytterhoeven]

On Tue, Jun 11, 2013 at 12:26 AM, Tony Luck <tony.luck@gmail.com> wrote:
> On Sat, Jun 8, 2013 at 3:08 AM, Chen Gang <gang.chen@asianux.com> wrote:
>> using 'unsigned int *', implicitly:
>>   ./ia64/include/asm/bitops.h:63:__set_bit (int nr, volatile void *addr)
>
> There is some downside on ia64 to your suggestion.  If "addr" is properly
> aligned for an "int", but misaligned for a long ... i.e. addr%8 = 4, then I'll
> take an unaligned reference trap if I work with long* where the current code
> working with int* does not.
>
> Now perhaps all the callers do guarantee long* alignment?  But I don't know.
>
> Apart from uniformity, there doesn't see to be any upside to changing this.

The address pointers have been supposed to be "long *" for a very long time.
Probably alpha (the second official Linux platform) did it
differently, and never
standardized to "long *".

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [Suggestion] arch/*/include/asm/bitops.h: about __set_bit() API.

[Chen Gang]

On 06/08/2013 06:08 PM, Chen Gang wrote:
> Several architectures have different __set_bit() API to others, in
> standard API, 2nd param of __set_bit() is 'unsigned long *', but:
> 
>   in 'min10300', it is 'unsigned char *',

Oh, it is my fault, for 'mn10300' it is no issue, it is not 'unsigned
char *' (it is a generic one which can match any type).

Also 'min10300' --> 'mn10300'.



Thanks.
-- 
Chen Gang

Asianux Corporation

Re: [Suggestion] arch/*/include/asm/bitops.h: about __set_bit() API.

[Chen Gang]

On 06/08/2013 06:08 PM, Chen Gang wrote:
> Several architectures have different __set_bit() API to others, in
> standard API, 2nd param of __set_bit() is 'unsigned long *', but:
> 
>   in 'min10300', it is 'unsigned char *',

Oh, it is my fault, for 'mn10300' it is no issue, it is not 'unsigned
char *' (it is a generic one which can match any type).

Also 'min10300' --> 'mn10300'.



Thanks.
-- 
Chen Gang

Asianux Corporation

Re: [Suggestion] arch/*/include/asm/bitops.h: about __set_bit() API.

[Chen Gang]

On 06/11/2013 03:32 PM, Geert Uytterhoeven wrote:
> On Tue, Jun 11, 2013 at 12:26 AM, Tony Luck <tony.luck@gmail.com> wrote:
>> > On Sat, Jun 8, 2013 at 3:08 AM, Chen Gang <gang.chen@asianux.com> wrote:
>>> >> using 'unsigned int *', implicitly:
>>> >>   ./ia64/include/asm/bitops.h:63:__set_bit (int nr, volatile void *addr)
>> >
>> > There is some downside on ia64 to your suggestion.  If "addr" is properly
>> > aligned for an "int", but misaligned for a long ... i.e. addr%8 == 4, then I'll
>> > take an unaligned reference trap if I work with long* where the current code
>> > working with int* does not.
>> >
>> > Now perhaps all the callers do guarantee long* alignment?  But I don't know.
>> >
>> > Apart from uniformity, there doesn't see to be any upside to changing this.
> The address pointers have been supposed to be "long *" for a very long time.
> Probably alpha (the second official Linux platform) did it
> differently, and never
> standardized to "long *".

Excuse me, I am not quite familiar with the details, but I guess, it is
about functional feature issues, not (or not only) about bug issues.

For the architectures which can fully support 64-bit OS, excluding ia64
and alpha, all of them can support setting 64 bits (from 0 to 63) under
64-bit machine.

I am not quite sure whether any sub-systems have already set higher bit
(> 31) under 64-bit machine, but in the future, it seems they could (at
least our API supposed so).


Thanks.
-- 
Chen Gang

Asianux Corporation

Re: [Suggestion] arch/*/include/asm/bitops.h: about __set_bit() API.

[Chen Gang]

On 06/11/2013 03:32 PM, Geert Uytterhoeven wrote:
> On Tue, Jun 11, 2013 at 12:26 AM, Tony Luck <tony.luck@gmail.com> wrote:
>> > On Sat, Jun 8, 2013 at 3:08 AM, Chen Gang <gang.chen@asianux.com> wrote:
>>> >> using 'unsigned int *', implicitly:
>>> >>   ./ia64/include/asm/bitops.h:63:__set_bit (int nr, volatile void *addr)
>> >
>> > There is some downside on ia64 to your suggestion.  If "addr" is properly
>> > aligned for an "int", but misaligned for a long ... i.e. addr%8 = 4, then I'll
>> > take an unaligned reference trap if I work with long* where the current code
>> > working with int* does not.
>> >
>> > Now perhaps all the callers do guarantee long* alignment?  But I don't know.
>> >
>> > Apart from uniformity, there doesn't see to be any upside to changing this.
> The address pointers have been supposed to be "long *" for a very long time.
> Probably alpha (the second official Linux platform) did it
> differently, and never
> standardized to "long *".

Excuse me, I am not quite familiar with the details, but I guess, it is
about functional feature issues, not (or not only) about bug issues.

For the architectures which can fully support 64-bit OS, excluding ia64
and alpha, all of them can support setting 64 bits (from 0 to 63) under
64-bit machine.

I am not quite sure whether any sub-systems have already set higher bit
(> 31) under 64-bit machine, but in the future, it seems they could (at
least our API supposed so).


Thanks.
-- 
Chen Gang

Asianux Corporation