Re: [PATCH] mips: Remove q-accessors from non-64bit platforms

[Arnd Bergmann]

On Fri, Jun 21, 2019 at 2:24 PM Maciej W. Rozycki <macro@xxxxxxxxxxxxxx> wrote:
> On Fri, 21 Jun 2019, Arnd Bergmann wrote:
> > > > The other property of packet memory and similar things is that you
> > > > basically want memcpy()-behavior with no byteswaps. This is one
> > > > of the few cases in which __raw_readq() is actually the right accessor
> > > > in (mostly) portable code.
> > >
> > >  Correct, but we're missing an `__raw_readq_relaxed', etc. interface and
> > > having additional barriers applied on every access would hit performance
> > > very badly;
> >
> > How so? __raw_readq() by definition has the least barriers of
> > all, you can't make it more relaxed than it already is.
>
>  Well, `__raw_readq' has all the barriers plain `readq' has except it does
> not ever do byte-swapping (which may be bad where address swizzling is
> also present).  Whereas `readq_relaxed' at least avoids the trailing DMA
> barrier.
>
>  This is what the MIPS version has:
>
> #define __BUILD_MEMORY_SINGLE(pfx, bwlq, type, barrier, relax, irq)     \
> [...]
>
> #define __BUILD_MEMORY_PFX(bus, bwlq, type, relax)                      \
>                                                                         \
> __BUILD_MEMORY_SINGLE(bus, bwlq, type, 1, relax, 1)
>
> #define BUILDIO_MEM(bwlq, type)                                         \
>                                                                         \
> __BUILD_MEMORY_PFX(__raw_, bwlq, type, 0)                               \
> __BUILD_MEMORY_PFX(__relaxed_, bwlq, type, 1)                           \
> __BUILD_MEMORY_PFX(__mem_, bwlq, type, 0)                               \
> __BUILD_MEMORY_PFX(, bwlq, type, 0)
>
> So `barrier' is always passed 1 and consequently all the accessors have a
> leading MMIO ordering barrier inserted and only `__relaxed_*' ones have
> `relax' set to 0 making them skip the trailing MMIO read vs DMA ordering
> barrier.  This is in accordance to Documentation/memory-barriers.txt I
> believe.

It is definitely not what other architectures do here. In particular, the
asm-generic implementation that is now used on most of them
defines raw_readl() as

static inline u32 __raw_readl(const volatile void __iomem *addr)
{
        return *(const volatile u32 __force *)addr;
}

and there are a number of drivers that depend on this behavior.
readl_relaxed() typically adds the byteswap on this, and readl() adds
the barriers on top of readl_relaxed().

>  NB I got one part wrong in the previous e-mail, sorry, as for packet
> memory accesses etc. the correct accessors are actually `__mem_*' rather
> than `__raw_*' ones, but the former ones are not portable.  I always
> forget about this peculiarity and it took us years to get it right with
> the MIPS port and the old IDE subsystem when doing PIO.
>
>  The `__mem_*' handlers still do whetever system-specific transformation
> is required to present data in the memory rather than CPU byte ordering.
> See arch/mips/include/asm/mach-ip27/mangle-port.h for a non-trivial
> example and arch/mips/include/asm/mach-generic/mangle-port.h for the
> general case.  Whereas `__raw_*' pass raw data unchanged and are generally
> only suitable for accesses to onchip SOC MMIO or similar resources that do
> not traverse any external bus where a system's endianness may be observed.

Ok, so what you have for __mem_* is actually what I had expected from
__raw_* for an architecture, except for the barriers that should have been
left out.

>  So contrary to what I have written before for the theoretical case of a
> big-endian system possibly doing address swizzling we'd have to define and
> use `__mem_readq_unordered', etc. here rather than `__raw_readq_relaxed',
> etc.

Right.

> > > in fact even the barriers `*_relaxed' accessors imply would
> > > best be removed in this use (which is why defza.c uses `readw_o' vs
> > > `readw_u', etc. internally), but after all the struggles over the years
> > > for weakly ordered internal APIs x86 people are so averse to I'm not sure
> > > if I want to start another one.  We can get away with `readq_relaxed' in
> > > this use though as all the systems this device can be used with are
> > > little-endian as is TURBOchannel, so no byte-swapping will ever actually
> > > occur.
> >
> > I still don't see any downside of using __raw_readq() here, while the
> > upsides are:
> >
> > - makes the driver portable to big-endian kernels (even though we don't
> >   care)
> > - avoids all barriers
> > - fixes the build regression.
>
>  Giving my observations above it would only address item #3 on your list,
> while addressing #1 and #2 would require defining `__mem_readq_unordered',
> etc. I am afraid.
>
>  Have I missed anything?

No, I think you are right based on how mips defines __raw_readl().

Unfortunately, this also means that all portable drivers using the
__raw_ accessors to do what you want here are broken on mips
(at least on big-endian), while mips drivers using __raw_* are not
portable to anything else.

      Arnd