Prototype of find_first_zero_bit in bitops.h

Prototype of find_first_zero_bit in bitops.h

[Mason]

Hello Russell,

Just wanted to run something by you.

In some driver code, I was passing the address of a u32 to
find_first_zero_bit() and the maintainer smacked me, pointing
out that find_first_zero_bit only accepted (unsigned long *)

Would it make sense to change the prototype of
_find_first_zero_bit_le()
so that my mistake would be caught by the compiler like this:

  CC      drivers/pci/host/pcie-tango.o
In file included from ./include/linux/bitops.h:36:0,
                 from ./include/linux/kernel.h:10,
                 from ./include/linux/list.h:8,
                 from ./include/linux/smp.h:11,
                 from ./include/linux/irq.h:12,
                 from ./include/linux/irqchip/chained_irq.h:21,
                 from drivers/pci/host/pcie-tango.c:1:
drivers/pci/host/pcie-tango.c: In function 'tango_irq_domain_alloc':
drivers/pci/host/pcie-tango.c:122:28: error:
passing argument 1 of '_find_first_zero_bit_le' from incompatible pointer type [-Werror=incompatible-pointer-types]
  pos = find_first_zero_bit(&mask, 32);
                            ^
./arch/arm/include/asm/bitops.h:199:59: note: in definition of macro 'find_first_zero_bit'
 #define find_first_zero_bit(p,sz) _find_first_zero_bit_le(p,sz)
                                                           ^
./arch/arm/include/asm/bitops.h:162:12: note: expected 'const long unsigned int *' but argument is of type 'u32 * {aka unsigned int *}'
 extern int _find_first_zero_bit_le(const unsigned long * p, unsigned size);
            ^


Proposed trivial patch:

--- a/arch/arm/include/asm/bitops.h
+++ b/arch/arm/include/asm/bitops.h
@@ -159,8 +159,8 @@ static inline void ____atomic_change_bit(unsigned int bit, volatile unsigned lon
 /*
  * Little endian assembly bitops.  nr = 0 -> byte 0 bit 0.
  */
-extern int _find_first_zero_bit_le(const void * p, unsigned size);
-extern int _find_next_zero_bit_le(const void * p, int size, int offset);
+extern int _find_first_zero_bit_le(const unsigned long *p, unsigned size);
+extern int _find_next_zero_bit_le(const unsigned long *p, int size, int offset);
 extern int _find_first_bit_le(const unsigned long *p, unsigned size);
 extern int _find_next_bit_le(const unsigned long *p, int size, int offset);
 

-- 
Regards.

Prototype of find_first_zero_bit in bitops.h

[Mason]

Top-posting to address relevance.

Is the proposed patch acceptable, or would it break
something I didn't foresee? Would "make allyesconfig"
or "make allmodconfig" flag potential issues?

Should I submit to the patch mill?

Regards.


On 29/03/2017 13:54, Mason wrote:

> Hello Russell,
> 
> Just wanted to run something by you.
> 
> In some driver code, I was passing the address of a u32 to
> find_first_zero_bit() and the maintainer smacked me, pointing
> out that find_first_zero_bit only accepted (unsigned long *)
> 
> Would it make sense to change the prototype of
> _find_first_zero_bit_le()
> so that my mistake would be caught by the compiler like this:
> 
>   CC      drivers/pci/host/pcie-tango.o
> In file included from ./include/linux/bitops.h:36:0,
>                  from ./include/linux/kernel.h:10,
>                  from ./include/linux/list.h:8,
>                  from ./include/linux/smp.h:11,
>                  from ./include/linux/irq.h:12,
>                  from ./include/linux/irqchip/chained_irq.h:21,
>                  from drivers/pci/host/pcie-tango.c:1:
> drivers/pci/host/pcie-tango.c: In function 'tango_irq_domain_alloc':
> drivers/pci/host/pcie-tango.c:122:28: error:
> passing argument 1 of '_find_first_zero_bit_le' from incompatible pointer type [-Werror=incompatible-pointer-types]
>   pos = find_first_zero_bit(&mask, 32);
>                             ^
> ./arch/arm/include/asm/bitops.h:199:59: note: in definition of macro 'find_first_zero_bit'
>  #define find_first_zero_bit(p,sz) _find_first_zero_bit_le(p,sz)
>                                                            ^
> ./arch/arm/include/asm/bitops.h:162:12: note: expected 'const long unsigned int *' but argument is of type 'u32 * {aka unsigned int *}'
>  extern int _find_first_zero_bit_le(const unsigned long * p, unsigned size);
>             ^
> 
> 
> Proposed trivial patch:
> 
> --- a/arch/arm/include/asm/bitops.h
> +++ b/arch/arm/include/asm/bitops.h
> @@ -159,8 +159,8 @@ static inline void ____atomic_change_bit(unsigned int bit, volatile unsigned lon
>  /*
>   * Little endian assembly bitops.  nr = 0 -> byte 0 bit 0.
>   */
> -extern int _find_first_zero_bit_le(const void * p, unsigned size);
> -extern int _find_next_zero_bit_le(const void * p, int size, int offset);
> +extern int _find_first_zero_bit_le(const unsigned long *p, unsigned size);
> +extern int _find_next_zero_bit_le(const unsigned long *p, int size, int offset);
>  extern int _find_first_bit_le(const unsigned long *p, unsigned size);
>  extern int _find_next_bit_le(const unsigned long *p, int size, int offset);
>  

Prototype of find_first_zero_bit in bitops.h

[Mason]

On 29/03/2017 13:54, Mason wrote:

> Hello Russell,
> 
> Just wanted to run something by you.
> 
> --- a/arch/arm/include/asm/bitops.h
> +++ b/arch/arm/include/asm/bitops.h
> @@ -159,8 +159,8 @@ static inline void ____atomic_change_bit(unsigned int bit, volatile unsigned lon
>  /*
>   * Little endian assembly bitops.  nr = 0 -> byte 0 bit 0.
>   */
> -extern int _find_first_zero_bit_le(const void * p, unsigned size);
> -extern int _find_next_zero_bit_le(const void * p, int size, int offset);
> +extern int _find_first_zero_bit_le(const unsigned long *p, unsigned size);
> +extern int _find_next_zero_bit_le(const unsigned long *p, int size, int offset);
>  extern int _find_first_bit_le(const unsigned long *p, unsigned size);
>  extern int _find_next_bit_le(const unsigned long *p, int size, int offset);
>  

On IRC, Arnd wrote:
> make find_first_zero_bit() an inline function taking a unsigned long pointer
> instead of a macro, but leave find_first_zero_bit_le taking a void pointer


Can someone point out why the current code handles finding "zero"
(unset) bits differently than finding "one" (set) bits?

_find_first_bit_le() requires a const unsigned long *p argument.
_find_first_zero_bit_le() only requires a const void *p argument.


FWIW, using v4.9 with the proposed patch applied, I ran
make allyesconfig
make -j24 ARCH=arm CROSS_COMPILE=arm-linux-gnueabihf- zImage

I did not see a single warning during compilation, although the build
fails at link-time with:

  LD      vmlinux.o
  MODPOST vmlinux.o
  GEN     .version
  CHK     include/generated/compile.h
  UPD     include/generated/compile.h
  CC      init/version.o
  LD      init/built-in.o
drivers/built-in.o: In function `alpine_msix_middle_domain_alloc':
zynq-fpga.c:(.text+0xb8): relocation truncated to fit: R_ARM_CALL against symbol `_raw_spin_lock' defined in .spinlock.text section in kernel/built-in.o
zynq-fpga.c:(.text+0xf0): relocation truncated to fit: R_ARM_CALL against symbol `_raw_spin_unlock' defined in .spinlock.text section in kernel/built-in.o
zynq-fpga.c:(.text+0x114): relocation truncated to fit: R_ARM_CALL against symbol `_raw_spin_unlock' defined in .spinlock.text section in kernel/built-in.o
zynq-fpga.c:(.text+0x23c): relocation truncated to fit: R_ARM_CALL against symbol `_raw_spin_lock' defined in .spinlock.text section in kernel/built-in.o
zynq-fpga.c:(.text+0x254): relocation truncated to fit: R_ARM_CALL against symbol `_raw_spin_unlock' defined in .spinlock.text section in kernel/built-in.o
drivers/built-in.o: In function `alpine_msix_init_domains':
zynq-fpga.c:(.text+0x2cc): relocation truncated to fit: R_ARM_CALL against symbol `of_irq_find_parent' defined in .text section in drivers/built-in.o
drivers/built-in.o: In function `alpine_msix_init':
zynq-fpga.c:(.text+0x408): relocation truncated to fit: R_ARM_CALL against symbol `of_address_to_resource' defined in .text section in drivers/built-in.o
zynq-fpga.c:(.text+0x448): relocation truncated to fit: R_ARM_CALL against symbol `of_property_read_variable_u32_array' defined in .text section in drivers/built-in.o
zynq-fpga.c:(.text+0x4c0): relocation truncated to fit: R_ARM_CALL against symbol `of_property_read_variable_u32_array' defined in .text section in drivers/built-in.o
drivers/built-in.o: In function `alpine_msix_middle_domain_free':
zynq-fpga.c:(.text+0x59c): relocation truncated to fit: R_ARM_CALL against symbol `_raw_spin_lock' defined in .spinlock.text section in kernel/built-in.o
zynq-fpga.c:(.text+0x5b4): additional relocation overflows omitted from the output
make: *** [vmlinux] Error 1

Which I don't think is related to the bitops prototypes.

Regards.

Prototype of find_first_zero_bit in bitops.h

[Arnd Bergmann]

On Mon, Apr 3, 2017 at 2:37 PM, Mason <slash.tmp@free.fr> wrote:
> On 29/03/2017 13:54, Mason wrote:>>
>> --- a/arch/arm/include/asm/bitops.h
>> +++ b/arch/arm/include/asm/bitops.h
>> @@ -159,8 +159,8 @@ static inline void ____atomic_change_bit(unsigned int bit, volatile unsigned lon
>>  /*
>>   * Little endian assembly bitops.  nr = 0 -> byte 0 bit 0.
>>   */
>> -extern int _find_first_zero_bit_le(const void * p, unsigned size);
>> -extern int _find_next_zero_bit_le(const void * p, int size, int offset);
>> +extern int _find_first_zero_bit_le(const unsigned long *p, unsigned size);
>> +extern int _find_next_zero_bit_le(const unsigned long *p, int size, int offset);
>>  extern int _find_first_bit_le(const unsigned long *p, unsigned size);
>>  extern int _find_next_bit_le(const unsigned long *p, int size, int offset);
>>
>
> On IRC, Arnd wrote:
>> make find_first_zero_bit() an inline function taking a unsigned long pointer
>> instead of a macro, but leave find_first_zero_bit_le taking a void pointer
>
>
> Can someone point out why the current code handles finding "zero"
> (unset) bits differently than finding "one" (set) bits?
>
> _find_first_bit_le() requires a const unsigned long *p argument.
> _find_first_zero_bit_le() only requires a const void *p argument.

find_first_bit_le appears to be unused, and is only defined on ARM.

> FWIW, using v4.9 with the proposed patch applied, I ran
> make allyesconfig
> make -j24 ARCH=arm CROSS_COMPILE=arm-linux-gnueabihf- zImage
>
> I did not see a single warning during compilation, although the build
> fails at link-time with:
>
>   LD      vmlinux.o
>   MODPOST vmlinux.o
>   GEN     .version
>   CHK     include/generated/compile.h
>   UPD     include/generated/compile.h
>   CC      init/version.o
>   LD      init/built-in.o
> drivers/built-in.o: In function `alpine_msix_middle_domain_alloc':
> zynq-fpga.c:(.text+0xb8): relocation truncated to fit: R_ARM_CALL against symbol `_raw_spin_lock' defined in .spinlock.text section in kernel/built-in.o
> zynq-fpga.c:(.text+0xf0): relocation truncated to fit: R_ARM_CALL against symbol `_raw_spin_unlock' defined in .spinlock.text section in kernel/built-in.o
...
>
> Which I don't think is related to the bitops prototypes.

It's a known problem. I have an experimental patch that turns on
CONFIG_LD_DEAD_CODE_DATA_ELIMINATION and
CONFIG_THIN_ARCHIVES, which fixes this, but needs to be
refreshed.

Just use 'allmodconfig' for build testing instead of 'allyesconfig'.

       Arnd

[PATCH] arm: bitops: Align prototypes to generic API

[Mason]

include/asm-generic/bitops/find.h declares:

extern unsigned long
find_first_zero_bit(const unsigned long *addr, unsigned long size);

while arch/arm/include/asm/bitops.h declares:

#define find_first_zero_bit(p,sz)	_find_first_zero_bit_le(p,sz)
extern int _find_first_zero_bit_le(const void * p, unsigned size);

Align the arm prototypes to the generic API, to have gcc report
inadequate arguments, such as pointer to u32.

Signed-off-by: Mason <slash.tmp@free.fr>
---
 arch/arm/include/asm/bitops.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/arm/include/asm/bitops.h b/arch/arm/include/asm/bitops.h
index e943e6cee254..f308c8c40cb9 100644
--- a/arch/arm/include/asm/bitops.h
+++ b/arch/arm/include/asm/bitops.h
@@ -159,16 +159,16 @@ extern int _test_and_change_bit(int nr, volatile unsigned long * p);
 /*
  * Little endian assembly bitops.  nr = 0 -> byte 0 bit 0.
  */
-extern int _find_first_zero_bit_le(const void * p, unsigned size);
-extern int _find_next_zero_bit_le(const void * p, int size, int offset);
+extern int _find_first_zero_bit_le(const unsigned long *p, unsigned size);
+extern int _find_next_zero_bit_le(const unsigned long *p, int size, int offset);
 extern int _find_first_bit_le(const unsigned long *p, unsigned size);
 extern int _find_next_bit_le(const unsigned long *p, int size, int offset);
 
 /*
  * Big endian assembly bitops.  nr = 0 -> byte 3 bit 0.
  */
-extern int _find_first_zero_bit_be(const void * p, unsigned size);
-extern int _find_next_zero_bit_be(const void * p, int size, int offset);
+extern int _find_first_zero_bit_be(const unsigned long *p, unsigned size);
+extern int _find_next_zero_bit_be(const unsigned long *p, int size, int offset);
 extern int _find_first_bit_be(const unsigned long *p, unsigned size);
 extern int _find_next_bit_be(const unsigned long *p, int size, int offset);
 
-- 
2.11.0

[PATCH] arm: bitops: Align prototypes to generic API

[Mason]

Tracking number in the patch mill:
http://www.armlinux.org.uk/developer/patches/viewpatch.php?id=8669/1

[PATCH] arm: bitops: Align prototypes to generic API

[Mason]

On 05/04/2017 14:21, Mason wrote:

> include/asm-generic/bitops/find.h declares:
> 
> extern unsigned long
> find_first_zero_bit(const unsigned long *addr, unsigned long size);
> 
> while arch/arm/include/asm/bitops.h declares:
> 
> #define find_first_zero_bit(p,sz)	_find_first_zero_bit_le(p,sz)
> extern int _find_first_zero_bit_le(const void * p, unsigned size);
> 
> Align the arm prototypes to the generic API, to have gcc report
> inadequate arguments, such as pointer to u32.
> 
> Signed-off-by: Mason <slash.tmp@free.fr>
> ---
>  arch/arm/include/asm/bitops.h | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm/include/asm/bitops.h b/arch/arm/include/asm/bitops.h
> index e943e6cee254..f308c8c40cb9 100644
> --- a/arch/arm/include/asm/bitops.h
> +++ b/arch/arm/include/asm/bitops.h
> @@ -159,16 +159,16 @@ extern int _test_and_change_bit(int nr, volatile unsigned long * p);
>  /*
>   * Little endian assembly bitops.  nr = 0 -> byte 0 bit 0.
>   */
> -extern int _find_first_zero_bit_le(const void * p, unsigned size);
> -extern int _find_next_zero_bit_le(const void * p, int size, int offset);
> +extern int _find_first_zero_bit_le(const unsigned long *p, unsigned size);
> +extern int _find_next_zero_bit_le(const unsigned long *p, int size, int offset);
>  extern int _find_first_bit_le(const unsigned long *p, unsigned size);
>  extern int _find_next_bit_le(const unsigned long *p, int size, int offset);
>  
>  /*
>   * Big endian assembly bitops.  nr = 0 -> byte 3 bit 0.
>   */
> -extern int _find_first_zero_bit_be(const void * p, unsigned size);
> -extern int _find_next_zero_bit_be(const void * p, int size, int offset);
> +extern int _find_first_zero_bit_be(const unsigned long *p, unsigned size);
> +extern int _find_next_zero_bit_be(const unsigned long *p, int size, int offset);
>  extern int _find_first_bit_be(const unsigned long *p, unsigned size);
>  extern int _find_next_bit_be(const unsigned long *p, int size, int offset);

Marc, it was you who pointed out that it is not valid to pass
the address of a u32 to find_first_zero_bit()

What are your thoughts on this trivial patch?

Russell, same question.

If no one thinks this patch is useful, I'll drop it and move on.

Regards.