[PATCH] arm64: io: specify asm operand width for __iormb()

[PATCH] arm64: io: specify asm operand width for __iormb()

[Nick Desaulniers]

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(-)

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");		\
 })
-- 
2.17.1

[PATCH] arm64: io: specify asm operand width for __iormb()

[Nick Desaulniers]

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(-)

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");		\
 })
-- 
2.17.1

[PATCH] arm64: io: specify asm operand width for __iormb()

[Julien Thierry]

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.

Re: [PATCH] arm64: io: specify asm operand width for __iormb()

[Julien Thierry]

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.

[PATCH] arm64: io: specify asm operand width for __iormb()

[Will Deacon]

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

Re: [PATCH] arm64: io: specify asm operand width for __iormb()

[Will Deacon]

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

Re: [PATCH] arm64: io: specify asm operand width for __iormb()

[Nathan Chancellor]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64: io: specify asm operand width for __iormb()

[Nathan Chancellor]

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

Re: [PATCH] arm64: io: specify asm operand width for __iormb()

[Will Deacon]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64: io: specify asm operand width for __iormb()

[Will Deacon]

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

Re: [PATCH] arm64: io: specify asm operand width for __iormb()

[Nathan Chancellor]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64: io: specify asm operand width for __iormb()

[Nathan Chancellor]

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

Re: [PATCH] arm64: io: specify asm operand width for __iormb()

[Will Deacon]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64: io: specify asm operand width for __iormb()

[Will Deacon]

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

Re: [PATCH] arm64: io: specify asm operand width for __iormb()

[Russell King - ARM Linux]

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"); \

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64: io: specify asm operand width for __iormb()

[Russell King - ARM Linux]

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"); \

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up