[lkml]   [2009]   [Apr]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
SubjectRe: arch/x86/Kconfig selects invalid HAVE_READQ, HAVE_WRITEQ vars

* Roland Dreier <> 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);

... 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.


 \ /
  Last update: 2009-04-19 23:49    [W:0.089 / U:5.192 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site