Re: arch/x86/Kconfig selects invalid HAVE_READQ, HAVE_WRITEQ vars

[Ingo Molnar <mingo@elte.hu>]

* Roland Dreier <rdreier@cisco.com> wrote:

>  > from arch/x86/Kconfig:
>  > 	...
>  >         select HAVE_READQ
>  >         select HAVE_WRITEQ
>  > 	...
>  > 
>  > yet there are no such defined Kconfig vars anywhere.  thoughts?
> 
> git blame shows that this came in from 2c5643b1 ("x86: provide 
> readq()/writeq() on 32-bit too").  And that commit looks very 
> dubious indeed to me -- it defines readq() and writeq() in a way 
> that is not atomic and probably won't generate single 64-bit bus 
> cycles.

Look at the drivers that define their own wrappers:

#ifndef readq
static inline unsigned long long readq(void __iomem *addr)
{
        return readl(addr) | (((unsigned long long)readl(addr + 4)) << 32LL);
}
#endif

... it's the obvious 32-bit semantics for reading a 64-bit value 
from an mmio address. We made that available on 32-bit too.

It's being used ... and has been in use for some time. Where's the 
problem? readl is serializing on all default-ioremap mmio targets on 
x86 so there's no ambiguity in ordering.

> Now, many drivers do "#ifndef readq <define my own implementation>
> #endif" but exactly what is required is very hardware-dependent,
> and accessing 32-bit halves in the wrong order may lead to very 
> subtle bugs. For example, the changelog for e23a59e1 ("niu: Fix 
> readq implementation when architecture does not provide one.") 
> says:
> 
>     In particular one of the issues is whether the top 32-bits
>     or the bottom 32-bits of the 64-bit register should be read
>     first.  There could be side effects, and in fact that is
>     exactly the problem here.
> 
> By coincidence, the 32-bit x86 implementation is actually OK for 
> niu, but I didn't audit every similar driver, and I don't think 
> any implementation of readq()/writeq() that generates multiple bus 
> cycles is suitable in general -- it doesn't meet the requirements 
> of the API.
>
> So I would strongly suggest reverting 2c5643b1 since as far as I 
> can tell it just sets a trap for subtle bugs that only show up on 
> 32-bit x86 [...]

Heh. It "only" shows up on the platform that ~80% of all our kernel 
testers use? ;-)

> [...] -- any portable driver still needs to provide 
> readq()/writeq() for other 32-bit architectures, so it doesn't 
> really help anyone.

So, are you arguing for a per driver definition of readq/writeq? If 
so then that does not make much technical sense. If not ... then 
what is your technical point?

Your complains are unfounded i think:

Firstly, any such bug would have shown up already.

Secondly, currently there's about 4 drivers in mainline that define 
readq/writeq methods: one of them is a very popular x86 driver and 
works fine, the other one is niu.c that you just mentioned is fine - 
the other two are for PARISC.

Thirdly, a driver simply has no business defining such a low-level 
hardware API that just happens not to be implemented on 32-bit (but 
is implemented on 64-bit).

Unless you can point to a real breakage that happened in the past ~6 
months since this commit has been upstream, i'm not sure what the 
fuss is about. Drivers found a hole in our APIs and filled it in an 
ad-hoc way - we plugged the hole on x86.

Pretty much the only valid argument to make here is that it should 
be filled in on all the other platforms as well - but you cant blame 
the x86 commit for that, can you?

So the only beef i have with that commit at the moment is that the 
HAVE_READQ / HAVE_WRITEQ Kconfig symbols are still orphan - this 
stuff should have been implemented in a tree-wide way long ago. 
Note, there's ongoing work regarding that in -mm - i saw related 
threads about that, recently. Obviously it take a lot of time to 
propagate something across a space of 20 architectures.

	Ingo