[PATCH] ARM: Correct WFE() in asm/spinlock.h for Thumb-2

[PATCH] ARM: Correct WFE() in asm/spinlock.h for Thumb-2

[Dave Martin]

The content for ALT_SMP() in the definition of WFE() expands to 6
bytes (IT cc ; WFEcc.W), which breaks the assumptions of the fixup
code, leading to lockups when the affected code gets run.

This patch works around the problem by explicitly using an
IT + WFEcc.N pair.

Signed-off-by: Dave Martin <dave.martin@linaro.org>
---
 arch/arm/include/asm/spinlock.h |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/arch/arm/include/asm/spinlock.h b/arch/arm/include/asm/spinlock.h
index da1af52..7cba79c 100644
--- a/arch/arm/include/asm/spinlock.h
+++ b/arch/arm/include/asm/spinlock.h
@@ -18,7 +18,12 @@
 
 #ifdef CONFIG_THUMB2_KERNEL
 #define SEV		ALT_SMP("sev.w", "nop.w")
-#define WFE(cond)	ALT_SMP("wfe" cond ".w", "nop.w")
+#define WFE(cond)	ALT_SMP(		\
+	"it " cond "\n\t"			\
+	"wfe" cond ".n",			\
+						\
+	"nop.w"					\
+)
 #else
 #define SEV		ALT_SMP("sev", "nop")
 #define WFE(cond)	ALT_SMP("wfe" cond, "nop")
-- 
1.7.1

[PATCH] ARM: Correct WFE() in asm/spinlock.h for Thumb-2

[Russell King - ARM Linux]

On Tue, Feb 08, 2011 at 03:20:14PM +0000, Dave Martin wrote:
> The content for ALT_SMP() in the definition of WFE() expands to 6
> bytes (IT cc ; WFEcc.W), which breaks the assumptions of the fixup
> code, leading to lockups when the affected code gets run.

Why does it?  My understanding is that the .w suffix forces the assembler
to use the ARM encoding for the instruction.

Thumb2 is just far too weird to understand.  I think I'll just stick to
writing good ARM code and let people sort out the Thumb2 weirdnesses.

[PATCH] ARM: Correct WFE() in asm/spinlock.h for Thumb-2

[Dave Martin]

Hi,

On Tue, Feb 8, 2011 at 4:40 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Tue, Feb 08, 2011 at 03:20:14PM +0000, Dave Martin wrote:
>> The content for ALT_SMP() in the definition of WFE() expands to 6
>> bytes (IT cc ; WFEcc.W), which breaks the assumptions of the fixup
>> code, leading to lockups when the affected code gets run.
>
> Why does it? ?My understanding is that the .w suffix forces the assembler
> to use the ARM encoding for the instruction.

The .w suffix forces a 32-bit encoding of Thumb-2 instructions for
which a 16-bit encoding is available.  Unfortunately, the 32-bit
Thumb-2 encodings are different from the ARM encodings (though there
are some odd similarities ... for example, all MCR and MRC encodings
are the same ... but with the halfwords swapped (!))

Like the 16-bit forms, most 32-bit Thumb-2 instructions don't contain
a condition field, so the assembler -mimplicit-it feature will need
insert an IT instruction if you write something conditional.  This
applies to everything except branches.

wfe: wfene.w becomes:

00000000 <wfe>:
   0:	bf18      	it	ne
   2:	f3af 8002 	wfene.w

Fortunately, wfe also has a 16-bit form, so we can squeeze the IT and
the WFE into the 4 bytes expected by ALT_SMP().  For safety, I write
it out explicitly: ALT_SMP("wfene", ...) would work sometimes but
might not work as expected if the previous instruction has an EQ or NE
condition, because then the assembler wouldn't need to insert an extra
IT when it generates its output...

> Thumb2 is just far too weird to understand. ?I think I'll just stick to
> writing good ARM code and let people sort out the Thumb2 weirdnesses.

Certainly life is much easier in ARM-land :)

I'm hoping that getting this working for OMAP will allow more people
to play with it though.

Cheers
---Dave

[PATCH] ARM: Correct WFE() in asm/spinlock.h for Thumb-2

[Will Deacon]

Hi Dave,

> On Tue, Feb 8, 2011 at 4:40 PM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> > On Tue, Feb 08, 2011 at 03:20:14PM +0000, Dave Martin wrote:
> >> The content for ALT_SMP() in the definition of WFE() expands to 6
> >> bytes (IT cc ; WFEcc.W), which breaks the assumptions of the fixup
> >> code, leading to lockups when the affected code gets run.
> >
> > Why does it?  My understanding is that the .w suffix forces the assembler
> > to use the ARM encoding for the instruction.
> 
> The .w suffix forces a 32-bit encoding of Thumb-2 instructions for
> which a 16-bit encoding is available.  Unfortunately, the 32-bit

[...]

> the WFE into the 4 bytes expected by ALT_SMP().  For safety, I write
> it out explicitly: ALT_SMP("wfene", ...) would work sometimes but
> might not work as expected if the previous instruction has an EQ or NE
> condition, because then the assembler wouldn't need to insert an extra
> IT when it generates its output...

That's really horrible! It might be worth commenting the explicit
IT in your WFE macro to explain why we can't rely on the assembler
to generate it for us.

With that minor change:

Acked-by: Will Deacon <will.deacon@arm.com>

Will

[PATCH] ARM: Correct WFE() in asm/spinlock.h for Thumb-2

[Russell King - ARM Linux]

On Wed, Feb 09, 2011 at 10:20:29AM -0000, Will Deacon wrote:
> > the WFE into the 4 bytes expected by ALT_SMP().  For safety, I write
> > it out explicitly: ALT_SMP("wfene", ...) would work sometimes but
> > might not work as expected if the previous instruction has an EQ or NE
> > condition, because then the assembler wouldn't need to insert an extra
> > IT when it generates its output...
> 
> That's really horrible! It might be worth commenting the explicit
> IT in your WFE macro to explain why we can't rely on the assembler
> to generate it for us.

Yes, it's really really horrible.  As I say, I think in future I'm just
going to stick to writing good ARM code and let others worry about how
to make it work properly on T2.

[PATCH] ARM: Correct WFE() in asm/spinlock.h for Thumb-2

[Nicolas Pitre]

On Tue, 8 Feb 2011, Dave Martin wrote:

> The content for ALT_SMP() in the definition of WFE() expands to 6
> bytes (IT cc ; WFEcc.W), which breaks the assumptions of the fixup
> code, leading to lockups when the affected code gets run.
> 
> This patch works around the problem by explicitly using an
> IT + WFEcc.N pair.
> 
> Signed-off-by: Dave Martin <dave.martin@linaro.org>
> ---
>  arch/arm/include/asm/spinlock.h |    7 ++++++-
>  1 files changed, 6 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/arm/include/asm/spinlock.h b/arch/arm/include/asm/spinlock.h
> index da1af52..7cba79c 100644
> --- a/arch/arm/include/asm/spinlock.h
> +++ b/arch/arm/include/asm/spinlock.h
> @@ -18,7 +18,12 @@
>  
>  #ifdef CONFIG_THUMB2_KERNEL
>  #define SEV		ALT_SMP("sev.w", "nop.w")
> -#define WFE(cond)	ALT_SMP("wfe" cond ".w", "nop.w")
> +#define WFE(cond)	ALT_SMP(		\
> +	"it " cond "\n\t"			\
> +	"wfe" cond ".n",			\
> +						\
> +	"nop.w"					\
> +)

In addition to a comment inline for this as others have suggested 
already, I'd write it differently to save some realestate:

#define WFE(cond)	ALT_SMP("it " cond " ; wfe" cond ".n", "nop.w")


Nicolas

[PATCH] ARM: Correct WFE() in asm/spinlock.h for Thumb-2

[Dave Martin]

On Wed, Feb 9, 2011 at 4:38 PM, Nicolas Pitre <nicolas.pitre@linaro.org> wrote:
> On Tue, 8 Feb 2011, Dave Martin wrote:
>
>> The content for ALT_SMP() in the definition of WFE() expands to 6
>> bytes (IT cc ; WFEcc.W), which breaks the assumptions of the fixup
>> code, leading to lockups when the affected code gets run.
>>
>> This patch works around the problem by explicitly using an
>> IT + WFEcc.N pair.
>>
>> Signed-off-by: Dave Martin <dave.martin@linaro.org>
>> ---
>> ?arch/arm/include/asm/spinlock.h | ? ?7 ++++++-
>> ?1 files changed, 6 insertions(+), 1 deletions(-)
>>
>> diff --git a/arch/arm/include/asm/spinlock.h b/arch/arm/include/asm/spinlock.h
>> index da1af52..7cba79c 100644
>> --- a/arch/arm/include/asm/spinlock.h
>> +++ b/arch/arm/include/asm/spinlock.h
>> @@ -18,7 +18,12 @@
>>
>> ?#ifdef CONFIG_THUMB2_KERNEL
>> ?#define SEV ? ? ? ? ?ALT_SMP("sev.w", "nop.w")
>> -#define WFE(cond) ? ?ALT_SMP("wfe" cond ".w", "nop.w")
>> +#define WFE(cond) ? ?ALT_SMP( ? ? ? ? ? ? ? ?\
>> + ? ? "it " cond "\n\t" ? ? ? ? ? ? ? ? ? ? ? \
>> + ? ? "wfe" cond ".n", ? ? ? ? ? ? ? ? ? ? ? ?\
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? \
>> + ? ? "nop.w" ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? \
>> +)
>
> In addition to a comment inline for this as others have suggested
> already, I'd write it differently to save some realestate:
>
> #define WFE(cond) ? ? ? ALT_SMP("it " cond " ; wfe" cond ".n", "nop.w")

Because of the subtlety of this, I wanted it to be presented in as
clear a way as possible.

Russell, the patch is in your patch system--- if you prefer the
shortened form, fell free to reject the existing patch and I'll
repost.

Cheers
---Dave