Possible regression in arm/io.h

Possible regression in arm/io.h

[Bastian Hecht]

Hello Will,

your introduction of the "+Qo" in arch/arm/include/asm/io.h makes some
drivers fail to compile on newer gnu ARM gccs. Like in
drivers/mtd/nand/docg4.c there is an offset of 0x800 from a pointer
used to address I/O memory. This leads to the error message:

/tmp/ccwLMdCy.s: Error: bad immediate value for 8-bit offset (2048)

The gnu gcc people tracked it down to the asm directive "+Qo". Do we
really want to enforce to allow only addresses that are offsetable
with one byte? If I understand it correctly from the gnu gcc docs, the
"o" is doing this.

excerpt from asm/io.h:
static inline void __raw_writew(u16 val, volatile void __iomem *addr)
{
        asm volatile("strh %1, %0"
                     : "+Qo" (*(volatile u16 __force *)addr)
                     : "r" (val));
}

Bug report:
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54983

So do we want to remove the "o" from all __raw_readX and __raw_writeX
functions? I've tried it and stuff compiles again without errors.

cheers,

Bastian Hecht

Possible regression in arm/io.h

[Will Deacon]

On Wed, Oct 24, 2012 at 11:38:14AM +0100, Bastian Hecht wrote:
> Hello Will,

Hello,

> your introduction of the "+Qo" in arch/arm/include/asm/io.h makes some
> drivers fail to compile on newer gnu ARM gccs. Like in
> drivers/mtd/nand/docg4.c there is an offset of 0x800 from a pointer
> used to address I/O memory. This leads to the error message:
> 
> /tmp/ccwLMdCy.s: Error: bad immediate value for 8-bit offset (2048)

Urgh.

> The gnu gcc people tracked it down to the asm directive "+Qo". Do we
> really want to enforce to allow only addresses that are offsetable
> with one byte? If I understand it correctly from the gnu gcc docs, the
> "o" is doing this.
> 
> excerpt from asm/io.h:
> static inline void __raw_writew(u16 val, volatile void __iomem *addr)
> {
>         asm volatile("strh %1, %0"
>                      : "+Qo" (*(volatile u16 __force *)addr)
>                      : "r" (val));
> }
> 
> Bug report:
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54983
> 
> So do we want to remove the "o" from all __raw_readX and __raw_writeX
> functions? I've tried it and stuff compiles again without errors.

I was under the impression that specifying multiple constraints would mean
that it would try "o", then if that didn't work it would fall back to "Q".
That's certainly the behaviour we've seen in the past... has this changed
with recent toolchains?

The problem with using "Q" on its own is that the compiler tends to generate
the address computation twice: once for "Q" and once for "r" -- this doesn't
happen with "o", where the address is computed once. This leads to an
increase in register pressure and I have seen the compiler choke claiming
that the inline asm block contains "impossible constraints" because it
insists on generating the address twice.

It sounds like we need to:

	(a) Understand what has changed in GCC to cause this error to start
	    cropping up.

	(b) Have a look at the impact of using only "Q" on the generated
	    code (especially register usage for the address).

Will

Possible regression in arm/io.h

[Bastian Hecht]

2012/10/24 Will Deacon <will.deacon@arm.com>:
> On Wed, Oct 24, 2012 at 11:38:14AM +0100, Bastian Hecht wrote:
>> Hello Will,
>
> Hello,
>
>> your introduction of the "+Qo" in arch/arm/include/asm/io.h makes some
>> drivers fail to compile on newer gnu ARM gccs. Like in
>> drivers/mtd/nand/docg4.c there is an offset of 0x800 from a pointer
>> used to address I/O memory. This leads to the error message:
>>
>> /tmp/ccwLMdCy.s: Error: bad immediate value for 8-bit offset (2048)
>
> Urgh.

xD

>> The gnu gcc people tracked it down to the asm directive "+Qo". Do we
>> really want to enforce to allow only addresses that are offsetable
>> with one byte? If I understand it correctly from the gnu gcc docs, the
>> "o" is doing this.
>>
>> excerpt from asm/io.h:
>> static inline void __raw_writew(u16 val, volatile void __iomem *addr)
>> {
>>         asm volatile("strh %1, %0"
>>                      : "+Qo" (*(volatile u16 __force *)addr)
>>                      : "r" (val));
>> }
>>
>> Bug report:
>> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54983
>>
>> So do we want to remove the "o" from all __raw_readX and __raw_writeX
>> functions? I've tried it and stuff compiles again without errors.
>
> I was under the impression that specifying multiple constraints would mean
> that it would try "o", then if that didn't work it would fall back to "Q".
> That's certainly the behaviour we've seen in the past... has this changed
> with recent toolchains?
>
> The problem with using "Q" on its own is that the compiler tends to generate
> the address computation twice: once for "Q" and once for "r" -- this doesn't
> happen with "o", where the address is computed once. This leads to an
> increase in register pressure and I have seen the compiler choke claiming
> that the inline asm block contains "impossible constraints" because it
> insists on generating the address twice.
>
> It sounds like we need to:
>
>         (a) Understand what has changed in GCC to cause this error to start
>             cropping up.
>
>         (b) Have a look at the impact of using only "Q" on the generated
>             code (especially register usage for the address).
>
> Will

Uff... I've just started to write ARM assembly and have no practical
experience with the inner workings of real world compilers. So this
time I'm afraid I was just good enough to report this. It sounds
interesting to hunt it, but would take ages for me at this point.

cheers,

 Bastian

Possible regression in arm/io.h

[Will Deacon]

On Wed, Oct 24, 2012 at 01:34:18PM +0100, Bastian Hecht wrote:
> 2012/10/24 Will Deacon <will.deacon@arm.com>:
> > It sounds like we need to:
> >
> >         (a) Understand what has changed in GCC to cause this error to start
> >             cropping up.
> >
> >         (b) Have a look at the impact of using only "Q" on the generated
> >             code (especially register usage for the address).
> >
> 
> Uff... I've just started to write ARM assembly and have no practical
> experience with the inner workings of real world compilers. So this
> time I'm afraid I was just good enough to report this. It sounds
> interesting to hunt it, but would take ages for me at this point.

Ok, I'll have a look at the impact of moving exclusively to "Q" when I get a
chance. Which toolchain are you using?

Will

Possible regression in arm/io.h

[Bastian Hecht]

2012/10/24 Will Deacon <will.deacon@arm.com>:
> On Wed, Oct 24, 2012 at 01:34:18PM +0100, Bastian Hecht wrote:
>> 2012/10/24 Will Deacon <will.deacon@arm.com>:
>> > It sounds like we need to:
>> >
>> >         (a) Understand what has changed in GCC to cause this error to start
>> >             cropping up.
>> >
>> >         (b) Have a look at the impact of using only "Q" on the generated
>> >             code (especially register usage for the address).
>> >
>>
>> Uff... I've just started to write ARM assembly and have no practical
>> experience with the inner workings of real world compilers. So this
>> time I'm afraid I was just good enough to report this. It sounds
>> interesting to hunt it, but would take ages for me at this point.
>
> Ok, I'll have a look at the impact of moving exclusively to "Q" when I get a
> chance. Which toolchain are you using?

gcc version 4.6.3 (Ubuntu/Linaro 4.6.3-1ubuntu5)
For a more verbose info take a look at the bug report link.

> Will

Possible regression in arm/io.h

[Will Deacon]

On Wed, Oct 24, 2012 at 02:35:11PM +0100, Bastian Hecht wrote:
> 2012/10/24 Will Deacon <will.deacon@arm.com>:
> > On Wed, Oct 24, 2012 at 01:34:18PM +0100, Bastian Hecht wrote:
> >> 2012/10/24 Will Deacon <will.deacon@arm.com>:
> >> > It sounds like we need to:
> >> >
> >> >         (a) Understand what has changed in GCC to cause this error to start
> >> >             cropping up.
> >> >
> >> >         (b) Have a look at the impact of using only "Q" on the generated
> >> >             code (especially register usage for the address).
> >> >
> >>
> >> Uff... I've just started to write ARM assembly and have no practical
> >> experience with the inner workings of real world compilers. So this
> >> time I'm afraid I was just good enough to report this. It sounds
> >> interesting to hunt it, but would take ages for me at this point.
> >
> > Ok, I'll have a look at the impact of moving exclusively to "Q" when I get a
> > chance. Which toolchain are you using?
> 
> gcc version 4.6.3 (Ubuntu/Linaro 4.6.3-1ubuntu5)
> For a more verbose info take a look at the bug report link.

Ok, thanks. One other thing you could try while I try to find a copy of that
toolchain is changing the "+" modifier to an "=", like I proposed in this
version of the patch:

  http://lists.infradead.org/pipermail/linux-arm-kernel/2012-August/114971.html

Cheers,

Will

Possible regression in arm/io.h

[Bastian Hecht]

>> > Ok, I'll have a look at the impact of moving exclusively to "Q" when I get a
>> > chance. Which toolchain are you using?
>>
>> gcc version 4.6.3 (Ubuntu/Linaro 4.6.3-1ubuntu5)
>> For a more verbose info take a look at the bug report link.
>
> Ok, thanks. One other thing you could try while I try to find a copy of that
> toolchain is changing the "+" modifier to an "=", like I proposed in this
> version of the patch:

So if alter it to fit to your patch scheme the result is:

  CC      arch/arm/kernel/asm-offsets.s
In file included from include/linux/scatterlist.h:10,
                 from include/linux/dma-mapping.h:9,
                 from arch/arm/kernel/asm-offsets.c:15:
/home/basti/renesas/flctl_patch/arch/arm/include/asm/io.h: In function
'__raw_readb':
/home/basti/renesas/flctl_patch/arch/arm/include/asm/io.h:100: error:
output operand constraint lacks '='
/home/basti/renesas/flctl_patch/arch/arm/include/asm/io.h:98: error:
output operand constraint lacks '='
/home/basti/renesas/flctl_patch/arch/arm/include/asm/io.h:98: error:
invalid lvalue in asm output 0
/home/basti/renesas/flctl_patch/arch/arm/include/asm/io.h: In function
'__raw_readl':
/home/basti/renesas/flctl_patch/arch/arm/include/asm/io.h:109: error:
output operand constraint lacks '='
/home/basti/renesas/flctl_patch/arch/arm/include/asm/io.h:107: error:
output operand constraint lacks '='
/home/basti/renesas/flctl_patch/arch/arm/include/asm/io.h:107: error:
invalid lvalue in asm output 0

and if I just replace all +Q with =Q it's this:
  LD      kernel/time/built-in.o
drivers/sh/intc/access.c: In function 'test_8':
drivers/sh/intc/access.c:78: warning: passing argument 1 of
'__raw_readb' makes pointer from integer without a cast
/home/basti/renesas/flctl_patch/arch/arm/include/asm/io.h:95: note:
expected 'const volatile void *' but argument is of type 'long
unsigned int'
drivers/sh/intc/access.c: In function 'test_16':
drivers/sh/intc/access.c:84: warning: passing argument 1 of
'__raw_readw' makes pointer from integer without a cast
/home/basti/renesas/flctl_patch/arch/arm/include/asm/io.h:71: note:
expected 'const volatile void *' but argument is of type 'long
unsigned int'
drivers/sh/intc/access.c: In function 'test_32':
drivers/sh/intc/access.c:90: warning: passing argument 1 of
'__raw_readl' makes pointer from integer without a cast
/home/basti/renesas/flctl_patch/arch/arm/include/asm/io.h:104: note:
expected 'const volatile void *' but argument is of type 'long
unsigned int'
drivers/sh/intc/access.c: In function 'write_8':
drivers/sh/intc/access.c:96: warning: passing argument 2 of
'__raw_writeb' makes pointer from integer without a cast
/home/basti/renesas/flctl_patch/arch/arm/include/asm/io.h:81: note:
expected 'volatile void *' but argument is of type 'long unsigned int'
... and so on

cheers,

 Bastian

Possible regression in arm/io.h

[Will Deacon]

On Wed, Oct 24, 2012 at 04:04:46PM +0100, Bastian Hecht wrote:
> >> > Ok, I'll have a look at the impact of moving exclusively to "Q" when I get a
> >> > chance. Which toolchain are you using?
> >>
> >> gcc version 4.6.3 (Ubuntu/Linaro 4.6.3-1ubuntu5)
> >> For a more verbose info take a look at the bug report link.
> >
> > Ok, thanks. One other thing you could try while I try to find a copy of that
> > toolchain is changing the "+" modifier to an "=", like I proposed in this
> > version of the patch:
> 
> So if alter it to fit to your patch scheme the result is:

Sorry, the change wasn't as trivial as I thought -- you have to reorder the
constraints because an output becomes an input for the load accessors. I
tried it myself and it doesn't fix the issue (I can reproduce it now).

Will

Possible regression in arm/io.h

[Artem Bityutskiy]

On Wed, 2012-10-24 at 11:52 +0100, Will Deacon wrote:
> 	(a) Understand what has changed in GCC to cause this error to start
> 	    cropping up.

This is about already quite old gcc 4.6.3, which I use for about 4 last
kernel releases already. So it is only the kernel that changed.

You can download the GCC I used from here:
http://kernel.org/pub/tools/crosstool/

-- 
Best Regards,
Artem Bityutskiy
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20121025/15484097/attachment.sig>

Possible regression in arm/io.h

[Will Deacon]

On Thu, Oct 25, 2012 at 07:55:22AM +0100, Artem Bityutskiy wrote:
> On Wed, 2012-10-24 at 11:52 +0100, Will Deacon wrote:
> > 	(a) Understand what has changed in GCC to cause this error to start
> > 	    cropping up.
> 
> This is about already quite old gcc 4.6.3, which I use for about 4 last
> kernel releases already. So it is only the kernel that changed.

Looks like it's broken with gcc 4.7 too, so it might be that it's never
worked. The problem seems to be that offsettable addresses are assumed by
GCC to have a 12-bit immediate range, which isn't true for half- and double-
work accessors, so GAS barfs when presented with the final (invalid) code.

Since we don't have double-word I/O accessors, we can just fallback to "Q"
for the half-word case:


diff --git a/arch/arm/include/asm/io.h b/arch/arm/include/asm/io.h
index 35c1ed8..42f042e 100644
--- a/arch/arm/include/asm/io.h
+++ b/arch/arm/include/asm/io.h
@@ -64,7 +64,7 @@ extern void __raw_readsl(const void __iomem *addr, void *data, int longlen);
 static inline void __raw_writew(u16 val, volatile void __iomem *addr)
 {
        asm volatile("strh %1, %0"
-                    : "+Qo" (*(volatile u16 __force *)addr)
+                    : "+Q" (*(volatile u16 __force *)addr)
                     : "r" (val));
 }
 
@@ -72,7 +72,7 @@ static inline u16 __raw_readw(const volatile void __iomem *addr)
 {
        u16 val;
        asm volatile("ldrh %1, %0"
-                    : "+Qo" (*(volatile u16 __force *)addr),
+                    : "+Q" (*(volatile u16 __force *)addr),
                       "=r" (val));
        return val;
 }


but this has the downside of *always* generating the target address into a
register and then using the basic [rN] addressing mode. A simple example
being smsc911x_rx_readfifo, where we see:

  c021bbe4:       e1d230b2        ldrh    r3, [r2, #2]

changed into:

  c021bd50:       e2820002        add     r0, r2, #2
  c021bd54:       e1d030b0        ldrh    r3, [r0]

which sucks, frankly. Unfortunately, GCC doesn't give us another constraint
that we can use for this, so I think we just have to grin and bear it.

Will

Possible regression in arm/io.h

[Mikael Pettersson]

Will Deacon writes:
 > On Thu, Oct 25, 2012 at 07:55:22AM +0100, Artem Bityutskiy wrote:
 > > On Wed, 2012-10-24 at 11:52 +0100, Will Deacon wrote:
 > > > 	(a) Understand what has changed in GCC to cause this error to start
 > > > 	    cropping up.
 > > 
 > > This is about already quite old gcc 4.6.3, which I use for about 4 last
 > > kernel releases already. So it is only the kernel that changed.
 > 
 > Looks like it's broken with gcc 4.7 too, so it might be that it's never
 > worked. The problem seems to be that offsettable addresses are assumed by
 > GCC to have a 12-bit immediate range, which isn't true for half- and double-
 > work accessors, so GAS barfs when presented with the final (invalid) code.
...
 > Unfortunately, GCC doesn't give us another constraint
 > that we can use for this, so I think we just have to grin and bear it.

Please file a GCC bugzilla entry with an enhancement request to support
ARM constraint letters for other immediate sizes than 12 (and list which
sizes are missing).

Possible regression in arm/io.h

[Artem Bityutskiy]

On Wed, 2012-10-24 at 16:27 +0100, Will Deacon wrote:
> On Wed, Oct 24, 2012 at 04:04:46PM +0100, Bastian Hecht wrote:
> > >> > Ok, I'll have a look at the impact of moving exclusively to "Q" when I get a
> > >> > chance. Which toolchain are you using?
> > >>
> > >> gcc version 4.6.3 (Ubuntu/Linaro 4.6.3-1ubuntu5)
> > >> For a more verbose info take a look at the bug report link.
> > >
> > > Ok, thanks. One other thing you could try while I try to find a copy of that
> > > toolchain is changing the "+" modifier to an "=", like I proposed in this
> > > version of the patch:
> > 
> > So if alter it to fit to your patch scheme the result is:
> 
> Sorry, the change wasn't as trivial as I thought -- you have to reorder the
> constraints because an output becomes an input for the load accessors. I
> tried it myself and it doesn't fix the issue (I can reproduce it now).

Any progress with this? Do we end up with gcc 4.6+ - uncompilable MTD
drivers in 3.7?

-- 
Best Regards,
Artem Bityutskiy
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20121122/169d1411/attachment.sig>

Possible regression in arm/io.h

[Will Deacon]

On Thu, Nov 22, 2012 at 07:57:35AM +0000, Artem Bityutskiy wrote:
> On Wed, 2012-10-24 at 16:27 +0100, Will Deacon wrote:
> > On Wed, Oct 24, 2012 at 04:04:46PM +0100, Bastian Hecht wrote:
> > > >> > Ok, I'll have a look at the impact of moving exclusively to "Q" when I get a
> > > >> > chance. Which toolchain are you using?
> > > >>
> > > >> gcc version 4.6.3 (Ubuntu/Linaro 4.6.3-1ubuntu5)
> > > >> For a more verbose info take a look at the bug report link.
> > > >
> > > > Ok, thanks. One other thing you could try while I try to find a copy of that
> > > > toolchain is changing the "+" modifier to an "=", like I proposed in this
> > > > version of the patch:
> > > 
> > > So if alter it to fit to your patch scheme the result is:
> > 
> > Sorry, the change wasn't as trivial as I thought -- you have to reorder the
> > constraints because an output becomes an input for the load accessors. I
> > tried it myself and it doesn't fix the issue (I can reproduce it now).
> 
> Any progress with this? Do we end up with gcc 4.6+ - uncompilable MTD
> drivers in 3.7?

I fixed it in 7629a9f661f7 ("ARM: 7567/1: io: avoid GCC's offsettable
addressing modes for halfword accesses").

Will

Possible regression in arm/io.h

[Artem Bityutskiy]

On Thu, 2012-11-22 at 10:19 +0000, Will Deacon wrote:
> > Any progress with this? Do we end up with gcc 4.6+ - uncompilable MTD
> > drivers in 3.7?
> 
> I fixed it in 7629a9f661f7 ("ARM: 7567/1: io: avoid GCC's offsettable
> addressing modes for halfword accesses").

Ah, this is in v3.7-rc5, I am using -rc4 and this is why I did not
notice this. Thanks!

-- 
Best Regards,
Artem Bityutskiy
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20121122/2e31cd37/attachment.sig>