arm64/io: add constant-argument check
PATCH v2 1/1 arm64/io: add constant-argument check
From: Arnd Bergmann <[email protected]>
In some configurations __const_iowrite32_copy() does not get inlined and gcc runs into the BUILD_BUG():
In file included from <command-line>:
In function '__const_memcpy_toio_aligned32',
inlined from '__const_iowrite32_copy' at arch/arm64/include/asm/io.h:203:3,
inlined from '__const_iowrite32_copy' at arch/arm64/include/asm/io.h:199:20:include/linux/compiler_types.h:487:45: error: call to '__compiletime_assert_538' declared with attribute error: BUILD_BUG failed
487 | _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
| ^include/linux/compiler_types.h:468:25: note: in definition of macro '__compiletime_assert'
468 | prefix ## suffix(); \
| ^~~~~~
include/linux/compiler_types.h:487:9: note: in expansion of macro '_compiletime_assert'
487 | _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
| ^~~~~~~~~~~~~~~~~~~
include/linux/build_bug.h:39:37: note: in expansion of macro 'compiletime_assert'
39 | #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
| ^~~~~~~~~~~~~~~~~~
include/linux/build_bug.h:59:21: note: in expansion of macro 'BUILD_BUG_ON_MSG'
59 | #define BUILD_BUG() BUILD_BUG_ON_MSG(1, "BUILD_BUG failed")
| ^~~~~~~~~~~~~~~~
arch/arm64/include/asm/io.h:193:17: note: in expansion of macro 'BUILD_BUG'
193 | BUILD_BUG();
| ^~~~~~~~~Move the check for constant arguments into the inline function to ensure it is still constant if the compiler decides against inlining it.
---
v2:
- fix both 32-bit and 64-bit copies
- remove now-redundant macros
--- arch/arm64/include/asm/io.h | 24 +++++++++--------------- 1 file changed, 9 insertions(+), 15 deletions(-)
arch/arm64/include/asm/io.h +9 -15
@@ -196,21 +196,18 @@ static inline void __const_memcpy_toio_aligned32(volatile u32 __iomem *to,void __iowrite32_copy_full (void __iomem *to,const void *from,size_t count); -static inline void __const_iowrite32_copy (void __iomem *to,const void *from, -size_t count) +static inline void __iowrite32_copy (void __iomem *to,const void *from, +size_t count) { -if (count ==8 || count ==4 || count ==2 || count ==1 ) { +if (__builtin_constant_p (count) && + (count ==8 || count ==4 || count ==2 || count ==1 )) {__const_memcpy_toio_aligned32 (to, from, count);dgh (); }else {__iowrite32_copy_full (to, from, count); } } - -#define __iowrite32_copy (to, from, count) \ - (__builtin_constant_p(count) ? \ - __const_iowrite32_copy(to, from, count) : \ - __iowrite32_copy_full(to, from, count)) +#define __iowrite32_copy (to, from, count) __iowrite32_copy(to, from, count)static inline void __const_memcpy_toio_aligned64 (volatile u64 __iomem *to,const u64 *from,size_t count)
@@ -255,21 +252,18 @@ static inline void __const_memcpy_toio_aligned64(volatile u64 __iomem *to,void __iowrite64_copy_full (void __iomem *to,const void *from,size_t count); -static inline void __const_iowrite64_copy (void __iomem *to,const void *from, +static inline void __iowrite64_copy (void __iomem *to,const void *from,size_t count) { -if (count ==8 || count ==4 || count ==2 || count ==1 ) { +if (__builtin_constant_p (count) && + (count ==8 || count ==4 || count ==2 || count ==1 )) {__const_memcpy_toio_aligned64 (to, from, count);dgh (); }else {__iowrite64_copy_full (to, from, count); } } - -#define __iowrite64_copy (to, from, count) \ - (__builtin_constant_p(count) ? \ - __const_iowrite64_copy(to, from, count) : \ - __iowrite64_copy_full(to, from, count)) +#define __iowrite64_copy (to, from, count) __iowrite64_copy(to, from, count)/* * I/O memory mapping functions.
Signature · 1 line
2.39.2
Will Deacon
maintainerQuoted 33 lines · Arnd Bergmann
From: Arnd Bergmann <[email protected]>
In some configurations
__const_iowrite32_copy()does not get inlined and gcc runs into theBUILD_BUG():In file included from <command-line>:
In function '__const_memcpy_toio_aligned32',inlined from '__const_iowrite32_copy' at arch/arm64/include/asm/io.h:203:3, inlined from '__const_iowrite32_copy' at arch/arm64/include/asm/io.h:199:20:
include/linux/compiler_types.h:487:45: error: call to '__compiletime_assert_538' declared with attribute error:BUILD_BUGfailed
487 |_compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
| ^include/linux/compiler_types.h:468:25: note: in definition of macro '__compiletime_assert'468 | prefix ## suffix(); \ | ^~~~~~ include/linux/compiler_types.h:487:9: note: in expansion of macro '_compiletime_assert' 487 | _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__) | ^~~~~~~~~~~~~~~~~~~ include/linux/build_bug.h:39:37: note: in expansion of macro 'compiletime_assert' 39 | #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg) | ^~~~~~~~~~~~~~~~~~ include/linux/build_bug.h:59:21: note: in expansion of macro 'BUILD_BUG_ON_MSG' 59 | #define BUILD_BUG() BUILD_BUG_ON_MSG(1, "BUILD_BUG failed") | ^~~~~~~~~~~~~~~~ arch/arm64/include/asm/io.h:193:17: note: in expansion of macro 'BUILD_BUG' 193 | BUILD_BUG(); | ^~~~~~~~~Move the check for constant arguments into the inline function to ensure it is still constant if the compiler decides against inlining it.
Fixes ead79118dae6 ("arm64/io: Provide a WC friendly __iowriteXX_copy()")Signed-off-by Arnd Bergmann <[email protected]>
Quoted 7 lines
---
v2:
- fix both 32-bit and 64-bit copies
- remove now-redundant macros--- arch/arm64/include/asm/io.h | 24 +++++++++--------------- 1 file changed, 9 insertions(+), 15 deletions(-)
I think this is superseded by Mark's diff in reply to v1, right?
https://lore.kernel.org/r/ZlcODqVXTDh6n0h-@J2N7QTR9R3
If so, Mark, please can you post that as a proper patch so that we can get this fixed?
Cheers,
Will
Quoted 42 lines · Will Deacon
Quoted 33 lines · Arnd Bergmann
From: Arnd Bergmann <[email protected]>
In some configurations
__const_iowrite32_copy()does not get inlined and gcc runs into theBUILD_BUG():In file included from <command-line>:
In function '__const_memcpy_toio_aligned32',inlined from '__const_iowrite32_copy' at arch/arm64/include/asm/io.h:203:3, inlined from '__const_iowrite32_copy' at arch/arm64/include/asm/io.h:199:20:
include/linux/compiler_types.h:487:45: error: call to '__compiletime_assert_538' declared with attribute error:BUILD_BUGfailed
487 |_compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
| ^include/linux/compiler_types.h:468:25: note: in definition of macro '__compiletime_assert'468 | prefix ## suffix(); \ | ^~~~~~ include/linux/compiler_types.h:487:9: note: in expansion of macro '_compiletime_assert' 487 | _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__) | ^~~~~~~~~~~~~~~~~~~ include/linux/build_bug.h:39:37: note: in expansion of macro 'compiletime_assert' 39 | #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg) | ^~~~~~~~~~~~~~~~~~ include/linux/build_bug.h:59:21: note: in expansion of macro 'BUILD_BUG_ON_MSG' 59 | #define BUILD_BUG() BUILD_BUG_ON_MSG(1, "BUILD_BUG failed") | ^~~~~~~~~~~~~~~~ arch/arm64/include/asm/io.h:193:17: note: in expansion of macro 'BUILD_BUG' 193 | BUILD_BUG(); | ^~~~~~~~~Move the check for constant arguments into the inline function to ensure it is still constant if the compiler decides against inlining it.
Fixes ead79118dae6 ("arm64/io: Provide a WC friendly __iowriteXX_copy()")Signed-off-by Arnd Bergmann <[email protected]>Quoted 7 lines
---
v2:
- fix both 32-bit and 64-bit copies
- remove now-redundant macros--- arch/arm64/include/asm/io.h | 24 +++++++++--------------- 1 file changed, 9 insertions(+), 15 deletions(-)
Quoted 6 lines
I think this is superseded by Mark's diff in reply to v1, right?
https://lore.kernel.org/r/ZlcODqVXTDh6n0h-@J2N7QTR9R3
If so, Mark, please can you post that as a proper patch so that we can get this fixed?
I wouldn't say superseded, but I agree with Mark that we should have the __always_inline added to the __iowrite64_copy() and __const_memcpy_toio_aligned64() in addition to the stuff here.
When I originally wrote this I copied the fairly common pattern of having the builtin_constant_p test inside a macro, but I see now it is quite common to put that into an inline. Putting it in the inline is definately much better, so I like Arnd's patch.
Arnd can you just make that addition and repost this?
Thanks,
Jason
Quoted 14 lines · Jason Gunthorpe
Quoted 8 lines · Will Deacon
On Tue, May 28, 2024 at 05:37:11PM +0200, Arnd Bergmann wrote:
I think this is superseded by Mark's diff in reply to v1, right?
https://lore.kernel.org/r/ZlcODqVXTDh6n0h-@J2N7QTR9R3
If so, Mark, please can you post that as a proper patch so that we can get this fixed?
I wouldn't say superseded, but I agree with Mark that we should have the
__always_inlineadded to the__iowrite64_copy()and__const_memcpy_toio_aligned64()in addition to the stuff here.
Quoted 7 lines
When I originally wrote this I copied the fairly common pattern of having the
builtin_constant_ptest inside a macro, but I see now it is quite common to put that into an inline. Putting it in the inline is definately much better, so I like Arnd's patch.Arnd can you just make that addition and repost this?
Yes, sorry for failing to follow up earlier. I'm doing a little more build testing now, will send soon.
Arnd