[PATCH] sh: Work around GCC bug in set_bl_bit()

[PATCH] sh: Work around GCC bug in set_bl_bit()

[Matt Fleming]

It seems that some versions of GCC do not handle negative constants in
inline assembly very well, specifically any negative constant less that
-129. Here is a reduced example of the problematic code,

static inline void set_bl_bit(void)
{
	unsigned long __dummy0;

	__asm__ __volatile__ (
		"and	%1, %0\n\t"
		: "=r" (__dummy0)
		: "r" (0xffffff0f)
		: "memory"
	);
}

Now, what GCC should do is place 0xffffff0f in a constant pool and load
it into a register with a mov.l instruction. What actually happens is
that GCC truncates 0xffffff0f to 16-bits (0xff0f), places it in a
constant pool and loads it with mov.w. Since the BL bit of the status
register is at bit-position 28, it doesn't even get set.

To work around this issue simply replace 0xffffff0f with 0xffffffff. It
is safe to include the IMASK field of the sr register in the 'and' mask
because we're blocking interrupts anyway by setting the BL bit, so
there's no need to disable them. The original mask was buggy anyway as
if any bits were set in IMASK we'd drop them on the floor and they'd
never be restored.

This bug was discovered after applying commit
73a38b839b9295216e8d44dabf54de88270e77b8 ("sh: Only use bl bit toggling
for sleeping idle."), which made my sh7785lcr board reset after calling
local_irq_enable() in default_idle().

Signed-off-by: Matt Fleming <matt@console-pimps.org>
---

Paul, it may be worth letting this one bake for another -rc period in
the hope that if there are any problems, people will hit them. I am
unsure when this bug was introduced and am pretty confused about why no
one else is seeing it. It's completely reproducible for me. I've tried
both Sourcery G++ Lite 4.3-143 for SuperH GNU/Linux and Sourcery G++
Lite 4.4-200 for SuperH GNU/Linux releases.

 arch/sh/include/asm/system_32.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/sh/include/asm/system_32.h b/arch/sh/include/asm/system_32.h
index 9bd2684..d933748 100644
--- a/arch/sh/include/asm/system_32.h
+++ b/arch/sh/include/asm/system_32.h
@@ -250,7 +250,7 @@ static inline void set_bl_bit(void)
 		"and	%3, %0\n\t"
 		"ldc	%0, sr\n\t"
 		: "=&r" (__dummy0), "=r" (__dummy1)
-		: "r" (0x10000000), "r" (0xffffff0f)
+		: "r" (0x10000000), "r" (0xffffffff)
 		: "memory"
 	);
 }
-- 
1.7.1

Re: [PATCH] sh: Work around GCC bug in set_bl_bit()

[Matt Fleming]

On Sun, Sep 26, 2010 at 12:08:37PM +0100, Matt Fleming wrote:
> It seems that some versions of GCC do not handle negative constants in
> inline assembly very well, specifically any negative constant less that
> -129. Here is a reduced example of the problematic code,
> 
> static inline void set_bl_bit(void)
> {
> 	unsigned long __dummy0;
> 
> 	__asm__ __volatile__ (
> 		"and	%1, %0\n\t"
> 		: "=r" (__dummy0)
> 		: "r" (0xffffff0f)
> 		: "memory"
> 	);
> }
> 
> Now, what GCC should do is place 0xffffff0f in a constant pool and load
> it into a register with a mov.l instruction. What actually happens is
> that GCC truncates 0xffffff0f to 16-bits (0xff0f), places it in a
> constant pool and loads it with mov.w. Since the BL bit of the status
> register is at bit-position 28, it doesn't even get set.
> 
> To work around this issue simply replace 0xffffff0f with 0xffffffff. It
> is safe to include the IMASK field of the sr register in the 'and' mask
> because we're blocking interrupts anyway by setting the BL bit, so
> there's no need to disable them. The original mask was buggy anyway as
> if any bits were set in IMASK we'd drop them on the floor and they'd
> never be restored.
> 
> This bug was discovered after applying commit
> 73a38b839b9295216e8d44dabf54de88270e77b8 ("sh: Only use bl bit toggling
> for sleeping idle."), which made my sh7785lcr board reset after calling
> local_irq_enable() in default_idle().
> 
> Signed-off-by: Matt Fleming <matt@console-pimps.org>

Someone just kindly pointed out to me that this diagnosis is
incorrect. mov.w will sign-extend the value loaded from memory before
storing it in a register, so there's no problem there and this isn't a
GCC bug. However, there's still something odd going on with my board and
the change to set_bl_bit() to use 0xffffffff instead of 0xffffff0f
_does_ stop my board resetting.

I'll investigate further.