* Prototype of find_first_zero_bit in bitops.h
@ 2017-03-29 11:54 Mason
2017-03-31 17:58 ` Mason
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Mason @ 2017-03-29 11:54 UTC (permalink / raw)
To: linux-arm-kernel
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Prototype of find_first_zero_bit in bitops.h
2017-03-29 11:54 Prototype of find_first_zero_bit in bitops.h Mason
@ 2017-03-31 17:58 ` Mason
2017-04-03 12:37 ` Mason
2017-04-05 12:21 ` [PATCH] arm: bitops: Align prototypes to generic API Mason
2 siblings, 0 replies; 7+ messages in thread
From: Mason @ 2017-03-31 17:58 UTC (permalink / raw)
To: linux-arm-kernel
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);
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Prototype of find_first_zero_bit in bitops.h
2017-03-29 11:54 Prototype of find_first_zero_bit in bitops.h Mason
2017-03-31 17:58 ` Mason
@ 2017-04-03 12:37 ` Mason
2017-04-03 13:32 ` Arnd Bergmann
2017-04-05 12:21 ` [PATCH] arm: bitops: Align prototypes to generic API Mason
2 siblings, 1 reply; 7+ messages in thread
From: Mason @ 2017-04-03 12:37 UTC (permalink / raw)
To: linux-arm-kernel
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Prototype of find_first_zero_bit in bitops.h
2017-04-03 12:37 ` Mason
@ 2017-04-03 13:32 ` Arnd Bergmann
0 siblings, 0 replies; 7+ messages in thread
From: Arnd Bergmann @ 2017-04-03 13:32 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] arm: bitops: Align prototypes to generic API
2017-03-29 11:54 Prototype of find_first_zero_bit in bitops.h Mason
2017-03-31 17:58 ` Mason
2017-04-03 12:37 ` Mason
@ 2017-04-05 12:21 ` Mason
2017-04-05 12:33 ` Mason
2017-04-18 9:36 ` Mason
2 siblings, 2 replies; 7+ messages in thread
From: Mason @ 2017-04-05 12:21 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH] arm: bitops: Align prototypes to generic API
2017-04-05 12:21 ` [PATCH] arm: bitops: Align prototypes to generic API Mason
@ 2017-04-05 12:33 ` Mason
2017-04-18 9:36 ` Mason
1 sibling, 0 replies; 7+ messages in thread
From: Mason @ 2017-04-05 12:33 UTC (permalink / raw)
To: linux-arm-kernel
Tracking number in the patch mill:
http://www.armlinux.org.uk/developer/patches/viewpatch.php?id=8669/1
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] arm: bitops: Align prototypes to generic API
2017-04-05 12:21 ` [PATCH] arm: bitops: Align prototypes to generic API Mason
2017-04-05 12:33 ` Mason
@ 2017-04-18 9:36 ` Mason
1 sibling, 0 replies; 7+ messages in thread
From: Mason @ 2017-04-18 9:36 UTC (permalink / raw)
To: linux-arm-kernel
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2017-04-18 9:36 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-03-29 11:54 Prototype of find_first_zero_bit in bitops.h Mason
2017-03-31 17:58 ` Mason
2017-04-03 12:37 ` Mason
2017-04-03 13:32 ` Arnd Bergmann
2017-04-05 12:21 ` [PATCH] arm: bitops: Align prototypes to generic API Mason
2017-04-05 12:33 ` Mason
2017-04-18 9:36 ` Mason
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).