| Message ID | 20181129041912.5918-1-nick.desaulniers@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: io: specify asm operand width for __iormb() | expand |
On 29/11/18 04:19, Nick Desaulniers wrote: > Fixes the warning produced from Clang: > ./include/asm-generic/io.h:711:9: warning: value size does not match > register size specified by the constraint and modifier > [-Wasm-operand-widths] > return readl(addr); > ^ > ./arch/arm64/include/asm/io.h:149:58: note: expanded from macro 'readl' > ^ > ./include/asm-generic/io.h:711:9: note: use constraint modifier "w" > ./arch/arm64/include/asm/io.h:149:50: note: expanded from macro 'readl' > ^ > ./arch/arm64/include/asm/io.h:118:25: note: expanded from macro '__iormb' > asm volatile("eor %w0, %1, %1\n" \ > ^ Why does the "eor %0, %1, %1" become "eor %w0, %1, %1" ? The variable passed to the inline assembly for %0 is unsigned long, so always 64-bits wide on arm64. Why is clang trying to use a 32-bit register for it? Although it's not really important since all this is just introducing a control dependency, I find it a bit odd. Thanks, > Though we disable Clang's integrated assembler with -no-integrated-as, > it still tries to do some validation of assembler constraints. > > While __iormb() is type agnostic to operand widths for argument v, its > lone use is to zero'd out via eor (exclusive or). > > Fixes commit 6460d3201471 ("arm64: io: Ensure calls to delay routines > are ordered against prior readX()") > Link: https://github.com/ClangBuiltLinux/continuous-integration/issues/78 > Suggested-by: Nathan Chancellor <natechancellor@gmail.com> > Reviewed-by: Nathan Chancellor <natechancellor@gmail.com> > Signed-off-by: Nick Desaulniers <nick.desaulniers@gmail.com> > --- > Side note: is it not correct to cite SHAs from linux-next in "Fixes > commit ..." lines? I guess we can drop it. > > Link to regression build: > https://travis-ci.com/ClangBuiltLinux/continuous-integration/builds/92799938 > > Link to build w/ this patch: > https://travis-ci.com/ClangBuiltLinux/continuous-integration/builds/92935901 > > > arch/arm64/include/asm/io.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/arm64/include/asm/io.h b/arch/arm64/include/asm/io.h > index d42d00d8d5b6..dbdebf81162b 100644 > --- a/arch/arm64/include/asm/io.h > +++ b/arch/arm64/include/asm/io.h > @@ -115,7 +115,7 @@ static inline u64 __raw_readq(const volatile void __iomem *addr) > * later instructions. This ensures that a subsequent call to\ > * udelay() will be ordered due to the ISB in get_cycles().\ > */\ > -asm volatile("eor%0, %1, %1\n"\ > +asm volatile("eor%0, %x1, %x1\n"\ > "cbnz%0, ."\ > : "=r" (tmp) : "r" (v) : "memory");\ > }) > -- Julien Thierry IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
On Thu, Nov 29, 2018 at 09:03:54AM +0000, Julien Thierry wrote: > > > On 29/11/18 04:19, Nick Desaulniers wrote: > > Fixes the warning produced from Clang: > > ./include/asm-generic/io.h:711:9: warning: value size does not match > > register size specified by the constraint and modifier > > [-Wasm-operand-widths] > > return readl(addr); > > ^ > > ./arch/arm64/include/asm/io.h:149:58: note: expanded from macro 'readl' > > ^ > > ./include/asm-generic/io.h:711:9: note: use constraint modifier "w" > > ./arch/arm64/include/asm/io.h:149:50: note: expanded from macro 'readl' > > ^ > > ./arch/arm64/include/asm/io.h:118:25: note: expanded from macro '__iormb' > > asm volatile("eor %w0, %1, %1\n" \ > > ^ > > Why does the "eor %0, %1, %1" become "eor %w0, %1, %1" ? > The variable passed to the inline assembly for %0 is unsigned long, so > always 64-bits wide on arm64. Why is clang trying to use a 32-bit > register for it? Yeah, the message above looks bogus to me. I can see %1 being 32-bit for read[bwl], so maybe clang is just getting the diagnostic wrong. If so, I wonder if the following fixes the problem: diff --git a/arch/arm64/include/asm/io.h b/arch/arm64/include/asm/io.h index d42d00d8d5b6..13befec8b64e 100644 --- a/arch/arm64/include/asm/io.h +++ b/arch/arm64/include/asm/io.h @@ -117,7 +117,7 @@ static inline u64 __raw_readq(const volatile void __iomem *addr) */ \ asm volatile("eor %0, %1, %1\n" \ "cbnz %0, ." \ - : "=r" (tmp) : "r" (v) : "memory"); \ + : "=r" (tmp) : "r" (unsigned long)(v) : "memory"); \ }) #define __iowmb() wmb() Will
On Thu, Nov 29, 2018 at 10:49:03AM +0000, Will Deacon wrote: > On Thu, Nov 29, 2018 at 09:03:54AM +0000, Julien Thierry wrote: > > > > > > On 29/11/18 04:19, Nick Desaulniers wrote: > > > Fixes the warning produced from Clang: > > > ./include/asm-generic/io.h:711:9: warning: value size does not match > > > register size specified by the constraint and modifier > > > [-Wasm-operand-widths] > > > return readl(addr); > > > ^ > > > ./arch/arm64/include/asm/io.h:149:58: note: expanded from macro 'readl' > > > ^ > > > ./include/asm-generic/io.h:711:9: note: use constraint modifier "w" > > > ./arch/arm64/include/asm/io.h:149:50: note: expanded from macro 'readl' > > > ^ > > > ./arch/arm64/include/asm/io.h:118:25: note: expanded from macro '__iormb' > > > asm volatile("eor %w0, %1, %1\n" \ > > > ^ > > > > Why does the "eor %0, %1, %1" become "eor %w0, %1, %1" ? > > The variable passed to the inline assembly for %0 is unsigned long, so > > always 64-bits wide on arm64. Why is clang trying to use a 32-bit > > register for it? Sorry, this was my fault, I accidentally added a w during testing to see what constraints were valid (given that my assembly knowledge is nearly non-existence so forgive the non-sensical experimentation) and I used that message rather than the original one. This is the unadulterated one. In file included from arch/arm64/kernel/asm-offsets.c:24: In file included from ./include/linux/dma-mapping.h:11: In file included from ./include/linux/scatterlist.h:9: In file included from ./arch/arm64/include/asm/io.h:209: ./include/asm-generic/io.h:695:9: warning: value size does not match register size specified by the constraint and modifier [-Wasm-operand-widths] return readb(addr); ^ ./arch/arm64/include/asm/io.h:147:58: note: expanded from macro 'readb' #define readb(c) ({ u8 __v = readb_relaxed(c); __iormb(__v); __v; }) ^ ./include/asm-generic/io.h:695:9: note: use constraint modifier "w" ./arch/arm64/include/asm/io.h:147:50: note: expanded from macro 'readb' #define readb(c) ({ u8 __v = readb_relaxed(c); __iormb(__v); __v; }) ^ ./arch/arm64/include/asm/io.h:118:24: note: expanded from macro '__iormb' asm volatile("eor %0, %1, %1\n" \ ^ > > Yeah, the message above looks bogus to me. I can see %1 being 32-bit for > read[bwl], so maybe clang is just getting the diagnostic wrong. If so, > I wonder if the following fixes the problem: > This doesn't appear to work, I get this error: In file included from arch/arm64/kernel/asm-offsets.c:24: In file included from ./include/linux/dma-mapping.h:11: In file included from ./include/linux/scatterlist.h:9: In file included from ./arch/arm64/include/asm/io.h:209: ./include/asm-generic/io.h:695:9: error: expected expression return readb(addr); ^ ./arch/arm64/include/asm/io.h:147:50: note: expanded from macro 'readb' #define readb(c) ({ u8 __v = readb_relaxed(c); __iormb(__v); __v; }) ^ ./arch/arm64/include/asm/io.h:120:28: note: expanded from macro '__iormb' : "=r" (tmp) : "r" (unsigned long)(v) : "memory"); \ ^ > > diff --git a/arch/arm64/include/asm/io.h b/arch/arm64/include/asm/io.h > index d42d00d8d5b6..13befec8b64e 100644 > --- a/arch/arm64/include/asm/io.h > +++ b/arch/arm64/include/asm/io.h > @@ -117,7 +117,7 @@ static inline u64 __raw_readq(const volatile void __iomem *addr) > */ \ > asm volatile("eor %0, %1, %1\n" \ > "cbnz %0, ." \ > - : "=r" (tmp) : "r" (v) : "memory"); \ > + : "=r" (tmp) : "r" (unsigned long)(v) : "memory"); \ > }) > > #define __iowmb() wmb() > > > Will
On Thu, Nov 29, 2018 at 09:10:39AM -0700, Nathan Chancellor wrote: > On Thu, Nov 29, 2018 at 10:49:03AM +0000, Will Deacon wrote: > > On Thu, Nov 29, 2018 at 09:03:54AM +0000, Julien Thierry wrote: > > > On 29/11/18 04:19, Nick Desaulniers wrote: > > > > Fixes the warning produced from Clang: > > > > ./include/asm-generic/io.h:711:9: warning: value size does not match > > > > register size specified by the constraint and modifier > > > > [-Wasm-operand-widths] > > > > return readl(addr); > > > > ^ > > > > ./arch/arm64/include/asm/io.h:149:58: note: expanded from macro 'readl' > > > > ^ > > > > ./include/asm-generic/io.h:711:9: note: use constraint modifier "w" > > > > ./arch/arm64/include/asm/io.h:149:50: note: expanded from macro 'readl' > > > > ^ > > > > ./arch/arm64/include/asm/io.h:118:25: note: expanded from macro '__iormb' > > > > asm volatile("eor %w0, %1, %1\n" \ > > > > ^ > > > > > > Why does the "eor %0, %1, %1" become "eor %w0, %1, %1" ? > > > The variable passed to the inline assembly for %0 is unsigned long, so > > > always 64-bits wide on arm64. Why is clang trying to use a 32-bit > > > register for it? > > Sorry, this was my fault, I accidentally added a w during testing to see > what constraints were valid (given that my assembly knowledge is nearly > non-existence so forgive the non-sensical experimentation) and I used > that message rather than the original one. This is the unadulterated one. Aha, that explains it. Thanks for clearing that up. > In file included from arch/arm64/kernel/asm-offsets.c:24: > In file included from ./include/linux/dma-mapping.h:11: > In file included from ./include/linux/scatterlist.h:9: > In file included from ./arch/arm64/include/asm/io.h:209: > ./include/asm-generic/io.h:695:9: warning: value size does not match register size specified by the constraint and modifier [-Wasm-operand-widths] > return readb(addr); > ^ > ./arch/arm64/include/asm/io.h:147:58: note: expanded from macro 'readb' > #define readb(c) ({ u8 __v = readb_relaxed(c); __iormb(__v); __v; }) > ^ > ./include/asm-generic/io.h:695:9: note: use constraint modifier "w" > ./arch/arm64/include/asm/io.h:147:50: note: expanded from macro 'readb' > #define readb(c) ({ u8 __v = readb_relaxed(c); __iormb(__v); __v; }) > ^ > ./arch/arm64/include/asm/io.h:118:24: note: expanded from macro '__iormb' > asm volatile("eor %0, %1, %1\n" \ > ^ > > > > > Yeah, the message above looks bogus to me. I can see %1 being 32-bit for > > read[bwl], so maybe clang is just getting the diagnostic wrong. If so, > > I wonder if the following fixes the problem: > > > > This doesn't appear to work, I get this error: > > In file included from arch/arm64/kernel/asm-offsets.c:24: > In file included from ./include/linux/dma-mapping.h:11: > In file included from ./include/linux/scatterlist.h:9: > In file included from ./arch/arm64/include/asm/io.h:209: > ./include/asm-generic/io.h:695:9: error: expected expression > return readb(addr); > ^ > ./arch/arm64/include/asm/io.h:147:50: note: expanded from macro 'readb' > #define readb(c) ({ u8 __v = readb_relaxed(c); __iormb(__v); __v; }) > ^ > ./arch/arm64/include/asm/io.h:120:28: note: expanded from macro '__iormb' > : "=r" (tmp) : "r" (unsigned long)(v) : "memory"); \ > ^ Can you try throwing another set of brackets around it, please? ((unsigned long)(v)) Will
On Thu, Nov 29, 2018 at 09:10:39AM -0700, Nathan Chancellor wrote: > On Thu, Nov 29, 2018 at 10:49:03AM +0000, Will Deacon wrote: > > On Thu, Nov 29, 2018 at 09:03:54AM +0000, Julien Thierry wrote: > > > > > > > > > On 29/11/18 04:19, Nick Desaulniers wrote: > > > > Fixes the warning produced from Clang: > > > > ./include/asm-generic/io.h:711:9: warning: value size does not match > > > > register size specified by the constraint and modifier > > > > [-Wasm-operand-widths] > > > > return readl(addr); > > > > ^ > > > > ./arch/arm64/include/asm/io.h:149:58: note: expanded from macro 'readl' > > > > ^ > > > > ./include/asm-generic/io.h:711:9: note: use constraint modifier "w" > > > > ./arch/arm64/include/asm/io.h:149:50: note: expanded from macro 'readl' > > > > ^ > > > > ./arch/arm64/include/asm/io.h:118:25: note: expanded from macro '__iormb' > > > > asm volatile("eor %w0, %1, %1\n" \ > > > > ^ > > > > > > Why does the "eor %0, %1, %1" become "eor %w0, %1, %1" ? > > > The variable passed to the inline assembly for %0 is unsigned long, so > > > always 64-bits wide on arm64. Why is clang trying to use a 32-bit > > > register for it? > > Sorry, this was my fault, I accidentally added a w during testing to see > what constraints were valid (given that my assembly knowledge is nearly > non-existence so forgive the non-sensical experimentation) and I used > that message rather than the original one. This is the unadulterated one. > > In file included from arch/arm64/kernel/asm-offsets.c:24: > In file included from ./include/linux/dma-mapping.h:11: > In file included from ./include/linux/scatterlist.h:9: > In file included from ./arch/arm64/include/asm/io.h:209: > ./include/asm-generic/io.h:695:9: warning: value size does not match register size specified by the constraint and modifier [-Wasm-operand-widths] > return readb(addr); > ^ > ./arch/arm64/include/asm/io.h:147:58: note: expanded from macro 'readb' > #define readb(c) ({ u8 __v = readb_relaxed(c); __iormb(__v); __v; }) > ^ > ./include/asm-generic/io.h:695:9: note: use constraint modifier "w" > ./arch/arm64/include/asm/io.h:147:50: note: expanded from macro 'readb' > #define readb(c) ({ u8 __v = readb_relaxed(c); __iormb(__v); __v; }) > ^ > ./arch/arm64/include/asm/io.h:118:24: note: expanded from macro '__iormb' > asm volatile("eor %0, %1, %1\n" \ > ^ > > > > > Yeah, the message above looks bogus to me. I can see %1 being 32-bit for > > read[bwl], so maybe clang is just getting the diagnostic wrong. If so, > > I wonder if the following fixes the problem: > > > > This doesn't appear to work, I get this error: > > In file included from arch/arm64/kernel/asm-offsets.c:24: > In file included from ./include/linux/dma-mapping.h:11: > In file included from ./include/linux/scatterlist.h:9: > In file included from ./arch/arm64/include/asm/io.h:209: > ./include/asm-generic/io.h:695:9: error: expected expression > return readb(addr); > ^ > ./arch/arm64/include/asm/io.h:147:50: note: expanded from macro 'readb' > #define readb(c) ({ u8 __v = readb_relaxed(c); __iormb(__v); __v; }) > ^ > ./arch/arm64/include/asm/io.h:120:28: note: expanded from macro '__iormb' > : "=r" (tmp) : "r" (unsigned long)(v) : "memory"); \ > ^ > > > > > diff --git a/arch/arm64/include/asm/io.h b/arch/arm64/include/asm/io.h > > index d42d00d8d5b6..13befec8b64e 100644 > > --- a/arch/arm64/include/asm/io.h > > +++ b/arch/arm64/include/asm/io.h > > @@ -117,7 +117,7 @@ static inline u64 __raw_readq(const volatile void __iomem *addr) > > */ \ > > asm volatile("eor %0, %1, %1\n" \ > > "cbnz %0, ." \ > > - : "=r" (tmp) : "r" (v) : "memory"); \ > > + : "=r" (tmp) : "r" (unsigned long)(v) : "memory"); \ The parens around the passed value are part of the asm() syntax, which is: "contraint" (expression) so should be: + : "=r" (tmp) : "r" ((unsigned long)(v)) : "memory"); \
On Thu, Nov 29, 2018 at 04:13:37PM +0000, Will Deacon wrote: > On Thu, Nov 29, 2018 at 09:10:39AM -0700, Nathan Chancellor wrote: > > On Thu, Nov 29, 2018 at 10:49:03AM +0000, Will Deacon wrote: > > > On Thu, Nov 29, 2018 at 09:03:54AM +0000, Julien Thierry wrote: > > > > On 29/11/18 04:19, Nick Desaulniers wrote: > > > > > Fixes the warning produced from Clang: > > > > > ./include/asm-generic/io.h:711:9: warning: value size does not match > > > > > register size specified by the constraint and modifier > > > > > [-Wasm-operand-widths] > > > > > return readl(addr); > > > > > ^ > > > > > ./arch/arm64/include/asm/io.h:149:58: note: expanded from macro 'readl' > > > > > ^ > > > > > ./include/asm-generic/io.h:711:9: note: use constraint modifier "w" > > > > > ./arch/arm64/include/asm/io.h:149:50: note: expanded from macro 'readl' > > > > > ^ > > > > > ./arch/arm64/include/asm/io.h:118:25: note: expanded from macro '__iormb' > > > > > asm volatile("eor %w0, %1, %1\n" \ > > > > > ^ > > > > > > > > Why does the "eor %0, %1, %1" become "eor %w0, %1, %1" ? > > > > The variable passed to the inline assembly for %0 is unsigned long, so > > > > always 64-bits wide on arm64. Why is clang trying to use a 32-bit > > > > register for it? > > > > Sorry, this was my fault, I accidentally added a w during testing to see > > what constraints were valid (given that my assembly knowledge is nearly > > non-existence so forgive the non-sensical experimentation) and I used > > that message rather than the original one. This is the unadulterated one. > > Aha, that explains it. Thanks for clearing that up. > > > In file included from arch/arm64/kernel/asm-offsets.c:24: > > In file included from ./include/linux/dma-mapping.h:11: > > In file included from ./include/linux/scatterlist.h:9: > > In file included from ./arch/arm64/include/asm/io.h:209: > > ./include/asm-generic/io.h:695:9: warning: value size does not match register size specified by the constraint and modifier [-Wasm-operand-widths] > > return readb(addr); > > ^ > > ./arch/arm64/include/asm/io.h:147:58: note: expanded from macro 'readb' > > #define readb(c) ({ u8 __v = readb_relaxed(c); __iormb(__v); __v; }) > > ^ > > ./include/asm-generic/io.h:695:9: note: use constraint modifier "w" > > ./arch/arm64/include/asm/io.h:147:50: note: expanded from macro 'readb' > > #define readb(c) ({ u8 __v = readb_relaxed(c); __iormb(__v); __v; }) > > ^ > > ./arch/arm64/include/asm/io.h:118:24: note: expanded from macro '__iormb' > > asm volatile("eor %0, %1, %1\n" \ > > ^ > > > > > > > > Yeah, the message above looks bogus to me. I can see %1 being 32-bit for > > > read[bwl], so maybe clang is just getting the diagnostic wrong. If so, > > > I wonder if the following fixes the problem: > > > > > > > This doesn't appear to work, I get this error: > > > > In file included from arch/arm64/kernel/asm-offsets.c:24: > > In file included from ./include/linux/dma-mapping.h:11: > > In file included from ./include/linux/scatterlist.h:9: > > In file included from ./arch/arm64/include/asm/io.h:209: > > ./include/asm-generic/io.h:695:9: error: expected expression > > return readb(addr); > > ^ > > ./arch/arm64/include/asm/io.h:147:50: note: expanded from macro 'readb' > > #define readb(c) ({ u8 __v = readb_relaxed(c); __iormb(__v); __v; }) > > ^ > > ./arch/arm64/include/asm/io.h:120:28: note: expanded from macro '__iormb' > > : "=r" (tmp) : "r" (unsigned long)(v) : "memory"); \ > > ^ > > Can you try throwing another set of brackets around it, please? > > ((unsigned long)(v)) > > Will Thanks, that fixes the warning as well. Nathan
On Thu, Nov 29, 2018 at 09:17:38AM -0700, Nathan Chancellor wrote: > On Thu, Nov 29, 2018 at 04:13:37PM +0000, Will Deacon wrote: > > On Thu, Nov 29, 2018 at 09:10:39AM -0700, Nathan Chancellor wrote: > > > This doesn't appear to work, I get this error: > > > > > > In file included from arch/arm64/kernel/asm-offsets.c:24: > > > In file included from ./include/linux/dma-mapping.h:11: > > > In file included from ./include/linux/scatterlist.h:9: > > > In file included from ./arch/arm64/include/asm/io.h:209: > > > ./include/asm-generic/io.h:695:9: error: expected expression > > > return readb(addr); > > > ^ > > > ./arch/arm64/include/asm/io.h:147:50: note: expanded from macro 'readb' > > > #define readb(c) ({ u8 __v = readb_relaxed(c); __iormb(__v); __v; }) > > > ^ > > > ./arch/arm64/include/asm/io.h:120:28: note: expanded from macro '__iormb' > > > : "=r" (tmp) : "r" (unsigned long)(v) : "memory"); \ > > > ^ > > > > Can you try throwing another set of brackets around it, please? > > > > ((unsigned long)(v)) > > > Thanks, that fixes the warning as well. Great, I'll push this out tomorrow with your reported-by on it. Will
diff --git a/arch/arm64/include/asm/io.h b/arch/arm64/include/asm/io.h index d42d00d8d5b6..dbdebf81162b 100644 --- a/arch/arm64/include/asm/io.h +++ b/arch/arm64/include/asm/io.h @@ -115,7 +115,7 @@ static inline u64 __raw_readq(const volatile void __iomem *addr) * later instructions. This ensures that a subsequent call to \ * udelay() will be ordered due to the ISB in get_cycles(). \ */ \ - asm volatile("eor %0, %1, %1\n" \ + asm volatile("eor %0, %x1, %x1\n" \ "cbnz %0, ." \ : "=r" (tmp) : "r" (v) : "memory"); \ })
Fixes the warning produced from Clang: ./include/asm-generic/io.h:711:9: warning: value size does not match register size specified by the constraint and modifier [-Wasm-operand-widths] return readl(addr); ^ ./arch/arm64/include/asm/io.h:149:58: note: expanded from macro 'readl' ^ ./include/asm-generic/io.h:711:9: note: use constraint modifier "w" ./arch/arm64/include/asm/io.h:149:50: note: expanded from macro 'readl' ^ ./arch/arm64/include/asm/io.h:118:25: note: expanded from macro '__iormb' asm volatile("eor %w0, %1, %1\n" \ ^ Though we disable Clang's integrated assembler with -no-integrated-as, it still tries to do some validation of assembler constraints. While __iormb() is type agnostic to operand widths for argument v, its lone use is to zero'd out via eor (exclusive or). Fixes commit 6460d3201471 ("arm64: io: Ensure calls to delay routines are ordered against prior readX()") Link: https://github.com/ClangBuiltLinux/continuous-integration/issues/78 Suggested-by: Nathan Chancellor <natechancellor@gmail.com> Reviewed-by: Nathan Chancellor <natechancellor@gmail.com> Signed-off-by: Nick Desaulniers <nick.desaulniers@gmail.com> --- Side note: is it not correct to cite SHAs from linux-next in "Fixes commit ..." lines? I guess we can drop it. Link to regression build: https://travis-ci.com/ClangBuiltLinux/continuous-integration/builds/92799938 Link to build w/ this patch: https://travis-ci.com/ClangBuiltLinux/continuous-integration/builds/92935901 arch/arm64/include/asm/io.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)