| Message ID | 87371ad9-012b-358d-f186-f8caa79feb0f@free.fr (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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); >
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.
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
--- 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);
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: