On Fri, Aug 30, 2019 at 08:52:20AM +0100, Will Deacon wrote:

On Fri, Aug 30, 2019 at 01:08:03AM +0100, Andrew Murray wrote:

quoted

On Thu, Aug 29, 2019 at 05:54:58PM +0100, Will Deacon wrote:

quoted

On Thu, Aug 29, 2019 at 04:48:34PM +0100, Will Deacon wrote:

quoted

diff --git a/arch/arm64/include/asm/atomic_ll_sc.h b/arch/arm64/include/asm/atomic_ll_sc.h
index 95091f72228b..7fa042f5444e 100644
--- a/arch/arm64/include/asm/atomic_ll_sc.h
+++ b/arch/arm64/include/asm/atomic_ll_sc.h

@@ -23,6 +23,10 @@ asm_ops "\n"								\
 #define __LL_SC_FALLBACK(asm_ops) asm_ops
 #endif

I downloaded your original patches and tried them, and also got the
build error. After playing with this I think something isn't quite right...

This is your current test:

 echo 'int main(void) {asm volatile("and w0, w0, %w0" :: "K" (4294967295)); return 0; }' |  aarch64-linux-gnu-gcc -S -x c  - ; echo $?

But on my machine this returns 0, i.e. no error. If I drop the -S:

 echo 'int main(void) {asm volatile("and w0, w0, %w0" :: "K" (4294967295)); return 0; }' |  aarch64-linux-gnu-gcc -x c  - ; echo $?

Then this returns 1.

So I guess the -S flag or something similar is needed.

quoted

quoted

quoted

 
+#ifndef CONFIG_CC_HAS_K_CONSTRAINT
+#define K
+#endif

Also, isn't this the wrong way around?

It looks like when using $(call try-run,echo - it's the last argument that is
used when the condition is false. Thus at present we seem to be setting 
CONFIG_CC_HAS_K_CONSTRAINT when 'K' is broken.

quoted

quoted

Bah, I need to use something like __stringify when the constraint is used
in order for this to get expanded properly. Updated diff below.

I don't think the changes in your updated diff are required. We successfully
combine 'asm_op' with the remainder of the assembly string without using
 __stringify, and this is no different to how the original patch combined
'constraint' with "r".

It's a hack: __stringify expands its arguments, so I figured I may as well
use that rather than do it manually with an extra macro.

quoted

You can verify this by looking at the preprocessed .i files generated with
something like:

make ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu- drivers/spi/spi-rockchip.i

I see no difference (with GCC 7.3.1) between the original approach and your
use of __stringify. Incidentally you end up with "K" "r" instead of "Kr" but
it seems to have the desired effect (e.g. supress/emit out of range errors).
I have a couple of macros that resolves this to "Kr" but I don't think it's
necessary.

Did you find that it didn't work without your changes? I found it hard to
reproduce the out-of-range errors until I made the following change, I could
then easily see the effect of changing the constraint:

        : "=&r" (result), "=&r" (tmp), "+Q" (v->counter)                \
-       : #constraint "r" (i));                                         \
+       : #constraint) "r" (4294967295));                                               \
 }

Without the __stringify I get a compilation failure when building
kernel/panic.o because it tries to cmpxchg a 32-bit variable with -1
(PANIC_CPU_INVALID). Looking at panic.s, I see that constraint parameter
isn't being expanded. For example if I do:

  #ifndef CONFIG_CC_HAS_K_CONSTRAINT
  #define INVALID_CONSTRAINT
  #else
  #define INVALID_CONSTRAINT	K
  #endif

and then pass INVALID_CONSTRAINT to the generator macros, we'll end up
with INVALID_CONSTRAINT in the .s file and gas will barf.

This still isn't an issue for me. Your patches cause the build to fail because
it's using the K flag - if I invert the CONFIG_CC_HAS_K_CONSTRAINT test then
it builds correctly (because it expands the K to nothing).

If there is an issue with the expansion of constraint, shouldn't we also
__stringify 'asm_op'?

Thanks,

Andrew Murray

The reason I didn't see this initially is because my silly testcase had
a typo and was using atomic_add instead of atomic_and.

Will

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