Re: [PATCH v2] MTD: Fix Orion NAND driver compilation with ARM OABI

[Jamie Lokier <jamie@shareable.org>]

Nicolas Pitre wrote:
> On Thu, 25 Mar 2010, Jamie Lokier wrote:
> > >  		asm volatile ("ldrd\t%0, [%1]" : "=&r" (x) : "r" (io_base));
> > >  		buf64[i++] = x;
> > 
> > The "register...asm" looks fine, but it occurs to me the constraints
> > are too weak (and they were before), so GCC could optimise that to the
> > wrong behaviour.
> > 
> > The "volatile" prevents GCC deleting the asm if it's output isn't
> > used, but it doesn't stop GCC from reordering the asms, for example if
> > it decides to unroll the loop.  It probably won't reorder in that
> > case, but it could.  The result would be out of order values stored
> > into buf[].  It could even move the ldrd earlier than the prior byte
> > accesses, or after the later byte accesses.
> 
> I don't see how that could happen.  The store into buf[] puts a 
> dependency on the output constraint of the inline asm statement.  And by 
> vertue of being volatile, gcc cannot cache the result of the output from 
> the asm as if it was a pure function.

The store into buf[] dependency doesn't stop this, after unrolling:

    asm volatile ("ldrd\t%0, [%1]" : "=&r" (x) : "r" (io_base));
    buf64[i++] = x;
    asm volatile ("ldrd\t%0, [%1]" : "=&r" (x) : "r" (io_base));
    buf64[i++] = x;

from being reordered as this

    asm volatile ("ldrd\t%0, [%1]" : "=&r" (x2) : "r" (io_base));
    asm volatile ("ldrd\t%0, [%1]" : "=&r" (x1) : "r" (io_base));
    buf64[i++] = x1;
    buf64[i++] = x2;

because the asm doesn't depend on memory, just register inputs and
outputs;

I'm not sure what you mean about the volatile stopping gcc from
treating the asm as a pure function.  Is that meaning of volatile in
the asm documentation?  (volatile on asm doesn't mean the same as
volatile on a function, or volatile on a pointer).

> > Any one of these should fix it:
> > 
> >    - Make io_base a pointer-to-volatile-u64 or cast it in the asm, and
> >      make sure to dereference it and use an "m" constraint (or
> >      tighter, such as "Q", if ldrd needs it).  It must be u64, not
> >      pointer-to-void, to tell GCC the size.  That tells GCC which memory
> >      the asm accesses, and the volatile dereference should tell GCC
> >      not to reorder them in principle (but the GCC manual doesn't
> >      make a specific promise about this for asms).
> 
> The LDRD has special range constraints on its addressing mode which is 
> not expressable with any of the available gcc memory constraints.

    'Q'
          A memory reference where the exact address is in a single
          register (''m'' is preferable for 'asm' statements)

If 'r' is good enough for io_base, 'Q' should be good enough for *io_base.

-- Jamie