arch/x86/Kconfig selects invalid HAVE_READQ, HAVE_WRITEQ vars

arch/x86/Kconfig selects invalid HAVE_READQ, HAVE_WRITEQ vars

[Robert P. J. Day]

from arch/x86/Kconfig:
	...
        select HAVE_READQ
        select HAVE_WRITEQ
	...

yet there are no such defined Kconfig vars anywhere.  thoughts?

rday
-


========================================================================
Robert P. J. Day                               Waterloo, Ontario, CANADA

        Linux Consulting, Training and Annoying Kernel Pedantry.

Web page:                                          http://crashcourse.ca
Linked In:                             http://www.linkedin.com/in/rpjday
Twitter:                                       http://twitter.com/rpjday
========================================================================

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

[Roland Dreier]

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

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 -- any portable driver still needs to provide readq()/writeq() for
other 32-bit architectures, so it doesn't really help anyone.

 - R.

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

[Ingo Molnar]

* 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

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

[H. Peter Anvin]

Ingo Molnar wrote:
> 
> 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.
> 

I think his point is that they're not atomic.  For what it's worth,
atomic readq()/writeq() *are* possible with any x86-32 CPU which
supports MMX, but it is very costly to do in the kernel since it
involves touching the FPU state.

For the vast number of users, a non-atomic primitive which is available
for both 32- and 64-bit x86 is a win.  For a small number of users,
it'll be confusing, and for a very small minority it's going to be
desirable to have the atomic primitive.

The reason the non-atomic is generally fine is because most device
drivers are inherently single-threaded on a per-hardware device basis.

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

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

[Ingo Molnar]

* H. Peter Anvin <hpa@zytor.com> wrote:

> Ingo Molnar wrote:
> > 
> > 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.
> 
> I think his point is that they're not atomic. [...]

Ok - i didnt really consider atomicity, because that's not really 
feasible for a number of reasons anyway:

> [...] For what it's worth, atomic readq()/writeq() *are* possible 
> with any x86-32 CPU which supports MMX, but it is very costly to 
> do in the kernel since it involves touching the FPU state.
> 
> For the vast number of users, a non-atomic primitive which is 
> available for both 32- and 64-bit x86 is a win.  For a small 
> number of users, it'll be confusing, and for a very small minority 
> it's going to be desirable to have the atomic primitive.
> 
> The reason the non-atomic is generally fine is because most device 
> drivers are inherently single-threaded on a per-hardware device 
> basis.

Also, atomicity might not be possible to guarantee on the bus level: 
say the device sits on a 32-bit PCI bus. (No matter what instruction 
the CPU gets, a readq/writeq there has to be done as two 32-bit bus 
accesses.)

(Also, even a genuine 64-bit device might be bridged over 32-bit 
pathways so a driver cannot really assume atomicity on that level.)

	Ingo

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

[Roland Dreier]

 > Also, atomicity might not be possible to guarantee on the bus level: 
 > say the device sits on a 32-bit PCI bus. (No matter what instruction 
 > the CPU gets, a readq/writeq there has to be done as two 32-bit bus 
 > accesses.)

Well, the conventional PCI devices I know of with 64-bit registers were
PCI-X cards, keyed so they would only fit into a 64-bit slot.  And of
course there is no such thing as 32-bit PCI Express.

 > (Also, even a genuine 64-bit device might be bridged over 32-bit 
 > pathways so a driver cannot really assume atomicity on that level.)

I have never even heard of a system with a 64-bit PCI slot that went
through a 32-bit pathway -- in fact I'm not sure how one could build
that.

But yes, for example on 32-bit PowerPC I don't think it's possible to
generate a 64-bit bus transaction in general.  So if a device requires
such a cycle then it simply can't work on such a system.  But there is
also the case where racing accesses to other registers must be avoided
(the mthca example I gave in my previous example) where the current
32-bit x86 definition is broken, but it could be fixed in a
driver-specific version that used a spinlock.

 - R.

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

[Robert Hancock]

Roland Dreier wrote:
>  > Also, atomicity might not be possible to guarantee on the bus level: 
>  > say the device sits on a 32-bit PCI bus. (No matter what instruction 
>  > the CPU gets, a readq/writeq there has to be done as two 32-bit bus 
>  > accesses.)
> 
> Well, the conventional PCI devices I know of with 64-bit registers were
> PCI-X cards, keyed so they would only fit into a 64-bit slot.  And of
> course there is no such thing as 32-bit PCI Express.

It's quite possible to use most 64-bit PCI-X cards in a 32-bit slot, 
with the 64-bit part of the card not plugged into anything. It's 
supposed to work (as long as the card can handle the voltage the slot uses).

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

[Roland Dreier]

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

But look at, say, drivers/infiniband/hw/amso100/c2.h:

#ifndef readq
static inline u64 readq(const void __iomem * addr)
{
	u64 ret = readl(addr + 4);
	ret <<= 32;
	ret |= readl(addr);

	return ret;
}
#endif

Notice that it reads from addr+4 *before* it reads from addr, rather
than after as in your example (and in fact your example depends on
undefined compiler semantics, since there is no sequence point between
the two operands of the | operator).  Now, I don't know that hardware,
so I don't know if it makes a difference, but the niu example I gave in
my original email shows that given hardware with clear-on-read
registers, the order does very much matter.

In a similar vein, drivers/infiniband/hw/mthca (which I wrote) deals
with hardware that has 64-bit registers, where we can write in two
32-bit chunks, as long as we have the right order and no other writes to
the same page of registers come in between.  So on 32-bit architectures,
the driver must use a spinlock around the pair of 32-bit writes (see
drivers/infiniband/hw/mthca/mthca_doorbell.h for the code).  And the
simple fact is that if that driver used "#ifdef writeq" (instead of "#if
BITS_PER_LONG == 64" as it actually does) then it would be broken on
32-bit x86 right now.

 > > 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? ;-)

Well, most of the drivers using readq()/writeq() are probably driving
"high-end" hardware (InfiniBand, 10G ethernet, "enterprise" SCSI) that
is much more tilted to 64-bit architectures.  But yes, such bugs would
probably be seen quickly -- but the effort to debug "works on x86-64,
fails on x86-32 under high load" bugs is pretty big, given that the
symptoms of non-atomic access to a 64-bit register are probably pretty
mysterious (you can read about how the niu bug I mentioned was fixed --
it took a while to zero in on the root cause).

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

Yes, I am arguing for exactly that, because dealing with the semantics
of non-atomic access to 64-bit registers involved low-level knowledge of
the specific hardware being driven.

As it stands 32-bit x86 has readq()/writeq() that are subtly different
subtly different from all other 64-bit architectures, in a way that sets
a booby trap for any driver that uses them.  So yes I stick to my
original point that the commit that added them for 32-bit x86 should be
reverted.

 - R.

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

[H. Peter Anvin]

Roland Dreier wrote:
> 
> Notice that it reads from addr+4 *before* it reads from addr, rather
> than after as in your example (and in fact your example depends on
> undefined compiler semantics, since there is no sequence point between
> the two operands of the | operator).  Now, I don't know that hardware,
> so I don't know if it makes a difference, but the niu example I gave in
> my original email shows that given hardware with clear-on-read
> registers, the order does very much matter.
> 

At least for x86, the order should be low-high, because that is the
order that those two transactions would be seen on a 32-bit bus
downstream from the CPU if the CPU issued a 64-bit transaction.

The only sane way to handle this as something other than per-driver
hacks would be something like:

#include <linux/io64.h>		/* Any 64-bit I/O OK */

#include <linux/io64lh.h>	/* Low-high splitting OK */

#include <linux/io64hl.h>	/* High-low splitting OK */

#include <linux/io64atomic.h>	/* 64-bit I/O must be atomic */

... i.e. letting the driver choose what fallback method it will accept.

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

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

[Ingo Molnar]

* H. Peter Anvin <hpa@zytor.com> wrote:

> Roland Dreier wrote:
> > 
> > Notice that it reads from addr+4 *before* it reads from addr, rather
> > than after as in your example (and in fact your example depends on
> > undefined compiler semantics, since there is no sequence point between
> > the two operands of the | operator).  Now, I don't know that hardware,
> > so I don't know if it makes a difference, but the niu example I gave in
> > my original email shows that given hardware with clear-on-read
> > registers, the order does very much matter.
> > 
> 
> At least for x86, the order should be low-high, because that is the
> order that those two transactions would be seen on a 32-bit bus
> downstream from the CPU if the CPU issued a 64-bit transaction.
> 
> The only sane way to handle this as something other than per-driver
> hacks would be something like:
> 
> #include <linux/io64.h>		/* Any 64-bit I/O OK */
> 
> #include <linux/io64lh.h>	/* Low-high splitting OK */
> 
> #include <linux/io64hl.h>	/* High-low splitting OK */
> 
> #include <linux/io64atomic.h>	/* 64-bit I/O must be atomic */
> 
> ... i.e. letting the driver choose what fallback method it will accept.

Yeah - with the default being the natural low-high order.

The other argument is that if a driver really wants some rare, oddly 
different order it should better define its own method that is not 
named in the same (or in a similar) way as an existing generic API. 
Otherwise, confusion will ensue.

	Ingo

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

[Hitoshi Mitake]

On Mon, Apr 20, 2009 at 19:53, Ingo Molnar <mingo@elte.hu> wrote:
>
> * H. Peter Anvin <hpa@zytor.com> wrote:
>
>> Roland Dreier wrote:
>> >
>> > Notice that it reads from addr+4 *before* it reads from addr, rather
>> > than after as in your example (and in fact your example depends on
>> > undefined compiler semantics, since there is no sequence point between
>> > the two operands of the | operator).  Now, I don't know that hardware,
>> > so I don't know if it makes a difference, but the niu example I gave in
>> > my original email shows that given hardware with clear-on-read
>> > registers, the order does very much matter.
>> >
>>
>> At least for x86, the order should be low-high, because that is the
>> order that those two transactions would be seen on a 32-bit bus
>> downstream from the CPU if the CPU issued a 64-bit transaction.
>>
>> The only sane way to handle this as something other than per-driver
>> hacks would be something like:
>>
>> #include <linux/io64.h>               /* Any 64-bit I/O OK */
>>
>> #include <linux/io64lh.h>     /* Low-high splitting OK */
>>
>> #include <linux/io64hl.h>     /* High-low splitting OK */
>>
>> #include <linux/io64atomic.h> /* 64-bit I/O must be atomic */
>>
>> ... i.e. letting the driver choose what fallback method it will accept.
>
> Yeah - with the default being the natural low-high order.
>
> The other argument is that if a driver really wants some rare, oddly
> different order it should better define its own method that is not
> named in the same (or in a similar) way as an existing generic API.
> Otherwise, confusion will ensue.
I think this is a good way.
readq/writeq are already in Linus's tree, removing these is not a good idea.

And I've sent the patch to fix a little problem of Kconfig about
readq/writeq to you.
http://marc.info/?l=linux-kernel&m=123521109218008&w=2
Did you notice?

Adding cautions about accessing order or non-atomic to Kconfig's help
part may be benefit.

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

[Ingo Molnar]

* Hitoshi Mitake <h.mitake@gmail.com> wrote:

> On Mon, Apr 20, 2009 at 19:53, Ingo Molnar <mingo@elte.hu> wrote:
> >
> > * H. Peter Anvin <hpa@zytor.com> wrote:
> >
> >> Roland Dreier wrote:
> >> >
> >> > Notice that it reads from addr+4 *before* it reads from addr, rather
> >> > than after as in your example (and in fact your example depends on
> >> > undefined compiler semantics, since there is no sequence point between
> >> > the two operands of the | operator).  Now, I don't know that hardware,
> >> > so I don't know if it makes a difference, but the niu example I gave in
> >> > my original email shows that given hardware with clear-on-read
> >> > registers, the order does very much matter.
> >> >
> >>
> >> At least for x86, the order should be low-high, because that is the
> >> order that those two transactions would be seen on a 32-bit bus
> >> downstream from the CPU if the CPU issued a 64-bit transaction.
> >>
> >> The only sane way to handle this as something other than per-driver
> >> hacks would be something like:
> >>
> >> #include <linux/io64.h>               /* Any 64-bit I/O OK */
> >>
> >> #include <linux/io64lh.h>     /* Low-high splitting OK */
> >>
> >> #include <linux/io64hl.h>     /* High-low splitting OK */
> >>
> >> #include <linux/io64atomic.h> /* 64-bit I/O must be atomic */
> >>
> >> ... i.e. letting the driver choose what fallback method it will accept.
> >
> > Yeah - with the default being the natural low-high order.
> >
> > The other argument is that if a driver really wants some rare, oddly
> > different order it should better define its own method that is not
> > named in the same (or in a similar) way as an existing generic API.
> > Otherwise, confusion will ensue.
> I think this is a good way.
> readq/writeq are already in Linus's tree, removing these is not a good idea.
> 
> And I've sent the patch to fix a little problem of Kconfig about
> readq/writeq to you.
> http://marc.info/?l=linux-kernel&m=123521109218008&w=2
> Did you notice?
> 
> Adding cautions about accessing order or non-atomic to Kconfig's help
> part may be benefit.

It's better to add add such non-interactive help text as Makefile 
comments:

#
# This option ...
#

and they should be invisible in make menuconfig. This is a facility 
provided by architectures.

Note, the whole patchset is still incomplete - readq/writeq wrappers 
should be provided on all 32-bit architectures. Are those in the 
works?

	Ingo

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

[Hitoshi Mitake]

On Tue, Apr 21, 2009 at 01:03, Ingo Molnar <mingo@elte.hu> wrote:
>
> * Hitoshi Mitake <h.mitake@gmail.com> wrote:
>
>> On Mon, Apr 20, 2009 at 19:53, Ingo Molnar <mingo@elte.hu> wrote:
>> >
>> > * H. Peter Anvin <hpa@zytor.com> wrote:
>> >
>> >> Roland Dreier wrote:
>> >> >
>> >> > Notice that it reads from addr+4 *before* it reads from addr, rather
>> >> > than after as in your example (and in fact your example depends on
>> >> > undefined compiler semantics, since there is no sequence point between
>> >> > the two operands of the | operator).  Now, I don't know that hardware,
>> >> > so I don't know if it makes a difference, but the niu example I gave in
>> >> > my original email shows that given hardware with clear-on-read
>> >> > registers, the order does very much matter.
>> >> >
>> >>
>> >> At least for x86, the order should be low-high, because that is the
>> >> order that those two transactions would be seen on a 32-bit bus
>> >> downstream from the CPU if the CPU issued a 64-bit transaction.
>> >>
>> >> The only sane way to handle this as something other than per-driver
>> >> hacks would be something like:
>> >>
>> >> #include <linux/io64.h>               /* Any 64-bit I/O OK */
>> >>
>> >> #include <linux/io64lh.h>     /* Low-high splitting OK */
>> >>
>> >> #include <linux/io64hl.h>     /* High-low splitting OK */
>> >>
>> >> #include <linux/io64atomic.h> /* 64-bit I/O must be atomic */
>> >>
>> >> ... i.e. letting the driver choose what fallback method it will accept.
>> >
>> > Yeah - with the default being the natural low-high order.
>> >
>> > The other argument is that if a driver really wants some rare, oddly
>> > different order it should better define its own method that is not
>> > named in the same (or in a similar) way as an existing generic API.
>> > Otherwise, confusion will ensue.
>> I think this is a good way.
>> readq/writeq are already in Linus's tree, removing these is not a good idea.
>>
>> And I've sent the patch to fix a little problem of Kconfig about
>> readq/writeq to you.
>> http://marc.info/?l=linux-kernel&m=123521109218008&w=2
>> Did you notice?
>>
>> Adding cautions about accessing order or non-atomic to Kconfig's help
>> part may be benefit.
>
> It's better to add add such non-interactive help text as Makefile
> comments:
>
> #
> # This option ...
> #
>
> and they should be invisible in make menuconfig. This is a facility
> provided by architectures.
I'll move the help text from Kconfig to Makefile.
(My original patch also doesn't make help text visible in make menuconfig.)

>
> Note, the whole patchset is still incomplete - readq/writeq wrappers
> should be provided on all 32-bit architectures. Are those in the
> works?

I'm not working on porting readq/writeq on all 32-bit architectures.
If I port these, HAVE_READQ will be needless. Because there's no reason
to judge that architecture provides readq/writeq.

Porting readq/writeq on all architectures is radical way to solve.
But the problem related to order of accessing and non-atomic still exists.

I think there are 3 ways to choose:

1) Removing readq/writeq from x86_32
This is the way Roland mentioned.
This way removes the bugs related to order of accessing and non-atomic forever.
But driver programmers must implement their own version of readq/writeq,
and Andrew Morton said such case is sucks.
http://marc.info/?l=linux-kernel&m=122625885124798&w=2

2) Adding HAVE_READQ and HAVE_WRITEQ to Kconfigs of architectures
which provides readq/writeq
   despite of 32/64bit
This is the nearest with current state of Linux.
But some day non-atomic or order of accessing which driver programmers
didn't expect may cause
subtle bugs.

3) Porting readq/writeq on all architectures despite of 32/64bit
This is a very radical way.
This frees us from the problem of "#ifdef readq <implement driver own
version> #endif"
or HAVE_READQ forever.
But the possibility of subtle bugs caused by non-atomic or order of
accessing still exists.

Which one should we choose?

I suggest 2) (or 3)).
Because there's no problem since ported readq/writeq on x86_32.
And as H. Peter Anvin mentioned non-atomic is generally fine.

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

[Ingo Molnar]

* Hitoshi Mitake <h.mitake@gmail.com> wrote:

> On Tue, Apr 21, 2009 at 01:03, Ingo Molnar <mingo@elte.hu> wrote:
> >
> > * Hitoshi Mitake <h.mitake@gmail.com> wrote:
> >
> >> On Mon, Apr 20, 2009 at 19:53, Ingo Molnar <mingo@elte.hu> wrote:
> >> >
> >> > * H. Peter Anvin <hpa@zytor.com> wrote:
> >> >
> >> >> Roland Dreier wrote:
> >> >> >
> >> >> > Notice that it reads from addr+4 *before* it reads from addr, rather
> >> >> > than after as in your example (and in fact your example depends on
> >> >> > undefined compiler semantics, since there is no sequence point between
> >> >> > the two operands of the | operator).  Now, I don't know that hardware,
> >> >> > so I don't know if it makes a difference, but the niu example I gave in
> >> >> > my original email shows that given hardware with clear-on-read
> >> >> > registers, the order does very much matter.
> >> >> >
> >> >>
> >> >> At least for x86, the order should be low-high, because that is the
> >> >> order that those two transactions would be seen on a 32-bit bus
> >> >> downstream from the CPU if the CPU issued a 64-bit transaction.
> >> >>
> >> >> The only sane way to handle this as something other than per-driver
> >> >> hacks would be something like:
> >> >>
> >> >> #include <linux/io64.h>               /* Any 64-bit I/O OK */
> >> >>
> >> >> #include <linux/io64lh.h>     /* Low-high splitting OK */
> >> >>
> >> >> #include <linux/io64hl.h>     /* High-low splitting OK */
> >> >>
> >> >> #include <linux/io64atomic.h> /* 64-bit I/O must be atomic */
> >> >>
> >> >> ... i.e. letting the driver choose what fallback method it will accept.
> >> >
> >> > Yeah - with the default being the natural low-high order.
> >> >
> >> > The other argument is that if a driver really wants some rare, oddly
> >> > different order it should better define its own method that is not
> >> > named in the same (or in a similar) way as an existing generic API.
> >> > Otherwise, confusion will ensue.
> >> I think this is a good way.
> >> readq/writeq are already in Linus's tree, removing these is not a good idea.
> >>
> >> And I've sent the patch to fix a little problem of Kconfig about
> >> readq/writeq to you.
> >> http://marc.info/?l=linux-kernel&m=123521109218008&w=2
> >> Did you notice?
> >>
> >> Adding cautions about accessing order or non-atomic to Kconfig's help
> >> part may be benefit.
> >
> > It's better to add add such non-interactive help text as Makefile
> > comments:
> >
> > #
> > # This option ...
> > #
> >
> > and they should be invisible in make menuconfig. This is a facility
> > provided by architectures.
> I'll move the help text from Kconfig to Makefile.
> (My original patch also doesn't make help text visible in make menuconfig.)

sorry i meant the Kconfig file - you can put comments in there too. 
help text is really meant for things that are interactive.

	Ingo

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

[Hitoshi Mitake]

On Tue, Apr 21, 2009 at 17:45, Ingo Molnar <mingo@elte.hu> wrote:
>
> * Hitoshi Mitake <h.mitake@gmail.com> wrote:
>
>> On Tue, Apr 21, 2009 at 01:03, Ingo Molnar <mingo@elte.hu> wrote:
>> >
>> > * Hitoshi Mitake <h.mitake@gmail.com> wrote:
>> >
>> >> On Mon, Apr 20, 2009 at 19:53, Ingo Molnar <mingo@elte.hu> wrote:
>> >> >
>> >> > * H. Peter Anvin <hpa@zytor.com> wrote:
>> >> >
>> >> >> Roland Dreier wrote:
>> >> >> >
>> >> >> > Notice that it reads from addr+4 *before* it reads from addr, rather
>> >> >> > than after as in your example (and in fact your example depends on
>> >> >> > undefined compiler semantics, since there is no sequence point between
>> >> >> > the two operands of the | operator).  Now, I don't know that hardware,
>> >> >> > so I don't know if it makes a difference, but the niu example I gave in
>> >> >> > my original email shows that given hardware with clear-on-read
>> >> >> > registers, the order does very much matter.
>> >> >> >
>> >> >>
>> >> >> At least for x86, the order should be low-high, because that is the
>> >> >> order that those two transactions would be seen on a 32-bit bus
>> >> >> downstream from the CPU if the CPU issued a 64-bit transaction.
>> >> >>
>> >> >> The only sane way to handle this as something other than per-driver
>> >> >> hacks would be something like:
>> >> >>
>> >> >> #include <linux/io64.h>               /* Any 64-bit I/O OK */
>> >> >>
>> >> >> #include <linux/io64lh.h>     /* Low-high splitting OK */
>> >> >>
>> >> >> #include <linux/io64hl.h>     /* High-low splitting OK */
>> >> >>
>> >> >> #include <linux/io64atomic.h> /* 64-bit I/O must be atomic */
>> >> >>
>> >> >> ... i.e. letting the driver choose what fallback method it will accept.
>> >> >
>> >> > Yeah - with the default being the natural low-high order.
>> >> >
>> >> > The other argument is that if a driver really wants some rare, oddly
>> >> > different order it should better define its own method that is not
>> >> > named in the same (or in a similar) way as an existing generic API.
>> >> > Otherwise, confusion will ensue.
>> >> I think this is a good way.
>> >> readq/writeq are already in Linus's tree, removing these is not a good idea.
>> >>
>> >> And I've sent the patch to fix a little problem of Kconfig about
>> >> readq/writeq to you.
>> >> http://marc.info/?l=linux-kernel&m=123521109218008&w=2
>> >> Did you notice?
>> >>
>> >> Adding cautions about accessing order or non-atomic to Kconfig's help
>> >> part may be benefit.
>> >
>> > It's better to add add such non-interactive help text as Makefile
>> > comments:
>> >
>> > #
>> > # This option ...
>> > #
>> >
>> > and they should be invisible in make menuconfig. This is a facility
>> > provided by architectures.
>> I'll move the help text from Kconfig to Makefile.
>> (My original patch also doesn't make help text visible in make menuconfig.)
>
> sorry i meant the Kconfig file - you can put comments in there too.
> help text is really meant for things that are interactive.

Can I think that I should add help text to both of Kconfig and Makefile?
(I'm not a native English speaker, and I'm not good at English.
I'm sorry if I misunderstand something...)

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

[H. Peter Anvin]

Hitoshi Mitake wrote:
> I suggest 2) (or 3)).
> Because there's no problem since ported readq/writeq on x86_32.
> And as H. Peter Anvin mentioned non-atomic is generally fine.

Okay, I'm going to throw in yet another wrench in the machinery.

There are devices which really only want to use writeq() if it is an
inexpensive (no x86 MMX hacks) atomic operation.

What follows is *NOT* a hypothetical example, I worked on a real device
which behaved this way:

Consider a device with a set of control registers in device memory.  You
can issue a whole command sequence at one point, but the command isn't
activated until a write is received with a certain bit set (located in
the high half of the last qword.)

For performance, the device provides its registers mapped to two
different physical addresses, with the intent that the OS will map one
of them write-combining.

Thus, what you want is to perform all writes but the very last one as a
write-combining write; the very last write should be a conventional I/O
write that flushes the write combiners ahead of itself and triggers the
write.

Now, in theory you could use writel() for this, but if you have a
working writeq() [e.g. 64-bit mode] you want to use it.

So it's important that the driver can know when readq/writeq are
emulated at all.

One way to deal with that is the <linux/io64*.h> method, another is with
feature test macros.

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

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

[Roland Dreier]

 > Okay, I'm going to throw in yet another wrench in the machinery.

 > There are devices which really only want to use writeq() if it is an
 > inexpensive (no x86 MMX hacks) atomic operation.

Interesting example.  It does seem there are several reasonable hardware
design patters where a driver needs to know if writeq() and/or readq()
is atomic.

 > One way to deal with that is the <linux/io64*.h> method, another is with
 > feature test macros.

A further idea would be to add readq_atomic()/writeq_atomic() that
behave as the current 64-bit versions do: a single instruction,
generates a single bus cycle, etc.  Then drivers that really need to
use the full semantics that the 64-bit versions gave can use those,
while drivers that just want a convenient way to read a 64-bit register
can use readq()/writeq().

This only makes sense if we define a 32-bit fallback for
readq()/writeq() for all 32-bit architectures -- in fact it would be
good to do it in asm-generic so that there can be a single
implementation that guarantees that non-atomic versions always do, say,
low 32 bits then high 32 bits.  (So eg niu can use the generic version)
And then drivers like drivers/infiniband/hw/mthca can be switched to
"#ifndef writeq_atomic <...hardware specific fallback...>"

However I worry that this just leaves driver authors too much rope.
Choosing readq_atomic() vs. readq() is just one more thing to get wrong.

 - R.

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

[H. Peter Anvin]

Roland Dreier wrote:
> 
> However I worry that this just leaves driver authors too much rope.
> Choosing readq_atomic() vs. readq() is just one more thing to get wrong.
> 

... as is having each driver implementing their own substitutes.

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

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

[Roland Dreier]

 > > However I worry that this just leaves driver authors too much rope.
 > > Choosing readq_atomic() vs. readq() is just one more thing to get wrong.

 > ... as is having each driver implementing their own substitutes.

Yes, I agree with that.  However at least status quo ante (readq/writeq
64-bit only) means that driver authors who use readq/writeq are forced
(by a compile error) to spend a little thought on what 32-bit fallback
they should use.

I guess one possibility is to make readq/writeq the atomic version, and
add readq_nonatomic()/writeq_nonatomic() for 32-bit architectures.  Then
it's much more opt-in -- but then that makes the (perhaps) more common
case look a bit uglier.

 - R.

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

[H. Peter Anvin]

Roland Dreier wrote:
>  > > However I worry that this just leaves driver authors too much rope.
>  > > Choosing readq_atomic() vs. readq() is just one more thing to get wrong.
> 
>  > ... as is having each driver implementing their own substitutes.
> 
> Yes, I agree with that.  However at least status quo ante (readq/writeq
> 64-bit only) means that driver authors who use readq/writeq are forced
> (by a compile error) to spend a little thought on what 32-bit fallback
> they should use.
> 
> I guess one possibility is to make readq/writeq the atomic version, and
> add readq_nonatomic()/writeq_nonatomic() for 32-bit architectures.  Then
> it's much more opt-in -- but then that makes the (perhaps) more common
> case look a bit uglier.
> 

Do you really expect driver authors to type writeq_nonatomic() for every 
register reference?

I think an #include at the top is one thing, but something that 
heavyweight for each call site really isn't going to fly.

	-hpa

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

[Roland Dreier]

 > Do you really expect driver authors to type writeq_nonatomic() for
 > every register reference?

No -- that's why I didn't even bring it up at first and why I consider
it ugly.

 > I think an #include at the top is one thing, but something that
 > heavyweight for each call site really isn't going to fly.

Yeah, I guess that could work, although I do worry that debugging the
wrong choice of #include might be a pain (mysterious symptoms on 32-bit
architectures caused by the name of an include file would be hard to
track).

To be honest I think the status quo ante was not really that bad.

 - R.

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

[H. Peter Anvin]

Roland Dreier wrote:
> 
> To be honest I think the status quo ante was not really that bad.
> 

That I have to vehemently disagree with.

	-hpa

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

[David Miller]

From: "H. Peter Anvin" <hpa@zytor.com>
Date: Tue, 21 Apr 2009 14:16:31 -0700

> Roland Dreier wrote:
>> To be honest I think the status quo ante was not really that bad.
> 
> That I have to vehemently disagree with.

I have to agree with Roland.

Unless you make it a compile failure, no driver author is going to
spend any amount of time trying to figure out how to deal with this
situation properly.

So in this sense, the current situation works really well.

If you make it just compile and make an arbitrary choice of whether
the top-32bits is read first or not, you're going to end up with
mysterious driver failures that only occur on some machines and the
cause of which won't be determined until after a lot of painful
digging.

This painful debugging experience is eliminated if the driver author
is told with a compile failure that there is an issue to resolve.

And that is what happens right now.

[PATCH] x86: Remove readq()/writeq() on 32-bit

[Roland Dreier]

As discussed in <http://lkml.org/lkml/2009/4/19/164> and follow-ups,
readq()/writeq() for 32-bit x86 are implemented as two readl()/writel()
operations.  This is not atomic (in the sense that another MMIO
operation from another CPU or thread can be done in the middle of the
two read/writes), and may not access the two halves of the register in
the correct order to work with hardware.

Rather than silently providing a 32-bit fallback that leaves a
possibility for strange driver bugs, it's better to provide readq()
and writeq() only for 64-bit architectures, and have a compile failure
on 32-bit architectures that forces driver authors to think about what
the correct solution is.

This essentially reverts 2c5643b1 ("x86: provide readq()/writeq() on
32-bit too") and follow-on commits.  If in the future someone wants to
provide a generic solution for all 32-bit architectures, that's great,
but there's not much point in providing (arguably broken)
implementations for only one architecture, since any portable driver
will have to implement fallbacks for other architectures anyway.

Signed-off-by: Roland Dreier <rolandd@cisco.com>
---
We never seemed to reach closure on this.  I would strongly suggest
merging something like this, and then if someone has a grand plan to
unify all fallbacks, we can add that when it shows up.  As it stands,
the x86-32 situation is not progress towards that grand unified plans,
and does nothing that I can tell beyond setting a trap for drivers.

 arch/x86/Kconfig          |    2 --
 arch/x86/include/asm/io.h |   23 ++---------------------
 2 files changed, 2 insertions(+), 23 deletions(-)

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index df9e885..0be277b 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -19,8 +19,6 @@ config X86_64
 config X86
 	def_bool y
 	select HAVE_AOUT if X86_32
-	select HAVE_READQ
-	select HAVE_WRITEQ
 	select HAVE_UNSTABLE_SCHED_CLOCK
 	select HAVE_IDE
 	select HAVE_OPROFILE
diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h
index 7373932..199c7b9 100644
--- a/arch/x86/include/asm/io.h
+++ b/arch/x86/include/asm/io.h
@@ -51,27 +51,6 @@ build_mmio_write(__writel, "l", unsigned int, "r", )
 build_mmio_read(readq, "q", unsigned long, "=r", :"memory")
 build_mmio_write(writeq, "q", unsigned long, "r", :"memory")
 
-#else
-
-static inline __u64 readq(const volatile void __iomem *addr)
-{
-	const volatile u32 __iomem *p = addr;
-	u32 low, high;
-
-	low = readl(p);
-	high = readl(p + 1);
-
-	return low + ((u64)high << 32);
-}
-
-static inline void writeq(__u64 val, volatile void __iomem *addr)
-{
-	writel(val, addr);
-	writel(val >> 32, addr+4);
-}
-
-#endif
-
 #define readq_relaxed(a)	readq(a)
 
 #define __raw_readq(a)		readq(a)
@@ -81,6 +60,8 @@ static inline void writeq(__u64 val, volatile void __iomem *addr)
 #define readq			readq
 #define writeq			writeq
 
+#endif
+
 /**
  *	virt_to_phys	-	map virtual addresses to physical
  *	@address: address to remap

Re: [PATCH] x86: Remove readq()/writeq() on 32-bit

[David Miller]

From: Roland Dreier <rdreier@cisco.com>
Date: Tue, 28 Apr 2009 12:05:10 -0700

> As discussed in <http://lkml.org/lkml/2009/4/19/164> and follow-ups,
> readq()/writeq() for 32-bit x86 are implemented as two readl()/writel()
> operations.  This is not atomic (in the sense that another MMIO
> operation from another CPU or thread can be done in the middle of the
> two read/writes), and may not access the two halves of the register in
> the correct order to work with hardware.
> 
> Rather than silently providing a 32-bit fallback that leaves a
> possibility for strange driver bugs, it's better to provide readq()
> and writeq() only for 64-bit architectures, and have a compile failure
> on 32-bit architectures that forces driver authors to think about what
> the correct solution is.
> 
> This essentially reverts 2c5643b1 ("x86: provide readq()/writeq() on
> 32-bit too") and follow-on commits.  If in the future someone wants to
> provide a generic solution for all 32-bit architectures, that's great,
> but there's not much point in providing (arguably broken)
> implementations for only one architecture, since any portable driver
> will have to implement fallbacks for other architectures anyway.
> 
> Signed-off-by: Roland Dreier <rolandd@cisco.com>

Acked-by: David S. Miller <davem@davemloft.net>

Re: [PATCH] x86: Remove readq()/writeq() on 32-bit

[Ingo Molnar]

(Linus Cc:-ed)

* David Miller <davem@davemloft.net> wrote:

> From: Roland Dreier <rdreier@cisco.com>
> Date: Tue, 28 Apr 2009 12:05:10 -0700
> 
> > As discussed in <http://lkml.org/lkml/2009/4/19/164> and follow-ups,
> > readq()/writeq() for 32-bit x86 are implemented as two readl()/writel()
> > operations.  This is not atomic (in the sense that another MMIO
> > operation from another CPU or thread can be done in the middle of the
> > two read/writes), and may not access the two halves of the register in
> > the correct order to work with hardware.
> > 
> > Rather than silently providing a 32-bit fallback that leaves a
> > possibility for strange driver bugs, it's better to provide readq()
> > and writeq() only for 64-bit architectures, and have a compile failure
> > on 32-bit architectures that forces driver authors to think about what
> > the correct solution is.
> > 
> > This essentially reverts 2c5643b1 ("x86: provide readq()/writeq() on
> > 32-bit too") and follow-on commits.  If in the future someone wants to
> > provide a generic solution for all 32-bit architectures, that's great,
> > but there's not much point in providing (arguably broken)
> > implementations for only one architecture, since any portable driver
> > will have to implement fallbacks for other architectures anyway.
> > 
> > Signed-off-by: Roland Dreier <rolandd@cisco.com>
> 
> Acked-by: David S. Miller <davem@davemloft.net>
[...]
> > We never seemed to reach closure on this.  I would strongly 
> > suggest merging something like this, and then if someone has a 
> > grand plan to unify all fallbacks, we can add that when it shows 
> > up.  As it stands, the x86-32 situation is not progress towards 
> > that grand unified plans, and does nothing that I can tell 
> > beyond setting a trap for drivers.

I still have no particularly strong opinion on this - other the 
reluctance i expressed already in the previous threads. My arguments 
are not reflected (and not addressed) in the changelog AFAICS, so 
let me repeat them here:

Firstly, it doesnt really matter in practice because such use is 
very rare and non-spinlocked access to IO regions is even rarer.

What caused 2c5643b1 was that right now we have ugly per driver
#defines and inlines for readq/writeq. See:

  git grep 'define.*readq' drivers/

for a rough estimation of what the current practices are. The 32-bit 
wrapper we added 6 months ago is the obvious implementation on x86 
and that it matches existing wrappers.

Atomicity of a 64-bit IO space access on 32-bit platforms, on an 
unknown-bitness transport (it might even be a 64-bit PCI device 
bridged over a 32-bit bridge) is obviously not guaranteed.

Not even 64-bit-on-64-bit is really guaranteed to be atomic. The 
bitness here is what the CPU runs its _own_ code in (and how it 
accesses its cached memory space) - it does not transform over to 
the uncached IO bus.

So trying to suggest that 64-bit readq/writeq when running on a 
64-bit kernel is somehow atomic (or can be made atomic) is really 
wrong IMO. The 32-bit wrapper is simply the expression of how the 
CPU would do a 64-bit access even in 64-bit mode anyway [if the 
transport is 32-bit].

aligned 32-bit access can be assumed atomic to a certain degree by 
virtue of PCI being 32 bit or better - but assuming any 64-bit 
IO-space read/write atomicity is wrong on many levels.

Driver authors will have to think about it anyway _even on 64-bit_, 
regardless of the existence of a 32-bit fallback.

A driver and hw _might_ be quirky and might require atomicity or 
might define a different order of access ... but then _that_ driver 
should become ugly, not all the others, right?

So my (slight) preference would be to keep the default generic 
implementation and not make any atomicity guarantees - we never made 
any. _If_ you want atomicity then provide a readq_atomic() / 
writeq_atomic() facility, with various higher level checks that make 
it sure that the IO transport is really atomic. (i dont see this 
happening any time soon for anything else but some really rare 
high-end IO transport.)

For the common case of there not being any atomicity assumption on 
the driver case it should result in cleaner code. (assuming all 
other 64-bit architectures implement a fallback too)

But ... i might be wrong about it, so i've Cc:-ed Linus who usually 
has a rather strong opinion about IO APIs. I'll apply the patch if 
Linus acks it (or Linus might take it straight out of email).

Thanks,

	Ingo

Re: [PATCH] x86: Remove readq()/writeq() on 32-bit

[Jeff Garzik]

Ingo Molnar wrote:
> (Linus Cc:-ed)
> 
> * David Miller <davem@davemloft.net> wrote:
> 
>> From: Roland Dreier <rdreier@cisco.com>
>> Date: Tue, 28 Apr 2009 12:05:10 -0700
>>
>>> As discussed in <http://lkml.org/lkml/2009/4/19/164> and follow-ups,
>>> readq()/writeq() for 32-bit x86 are implemented as two readl()/writel()
>>> operations.  This is not atomic (in the sense that another MMIO
>>> operation from another CPU or thread can be done in the middle of the
>>> two read/writes), and may not access the two halves of the register in
>>> the correct order to work with hardware.
>>>
>>> Rather than silently providing a 32-bit fallback that leaves a
>>> possibility for strange driver bugs, it's better to provide readq()
>>> and writeq() only for 64-bit architectures, and have a compile failure
>>> on 32-bit architectures that forces driver authors to think about what
>>> the correct solution is.
>>>
>>> This essentially reverts 2c5643b1 ("x86: provide readq()/writeq() on
>>> 32-bit too") and follow-on commits.  If in the future someone wants to
>>> provide a generic solution for all 32-bit architectures, that's great,
>>> but there's not much point in providing (arguably broken)
>>> implementations for only one architecture, since any portable driver
>>> will have to implement fallbacks for other architectures anyway.
>>>
>>> Signed-off-by: Roland Dreier <rolandd@cisco.com>
>> Acked-by: David S. Miller <davem@davemloft.net>
> [...]
>>> We never seemed to reach closure on this.  I would strongly 
>>> suggest merging something like this, and then if someone has a 
>>> grand plan to unify all fallbacks, we can add that when it shows 
>>> up.  As it stands, the x86-32 situation is not progress towards 
>>> that grand unified plans, and does nothing that I can tell 
>>> beyond setting a trap for drivers.
> 
> I still have no particularly strong opinion on this - other the 
> reluctance i expressed already in the previous threads. My arguments 
> are not reflected (and not addressed) in the changelog AFAICS, so 
> let me repeat them here:

I do.


> What caused 2c5643b1 was that right now we have ugly per driver
> #defines and inlines for readq/writeq. See:
> 
>   git grep 'define.*readq' drivers/
> 
> for a rough estimation of what the current practices are. The 32-bit 
> wrapper we added 6 months ago is the obvious implementation on x86 
> and that it matches existing wrappers.

This is the key...


> So my (slight) preference would be to keep the default generic 
> implementation and not make any atomicity guarantees - we never made 
> any.

Agreed.

This removal patch is completely pointless, because it moves us 
backwards to the point where we had a bunch of drivers defining it.

Why is that any better?

"Forcing driver writers to think" translates in the real world to each 
hardware vendor putting the common "#define readq" into their driver.

At least the networking drivers I messed with (until 11/2008) were 
always fine with a non-atomic readq.

The x86 kernel 32-bit implementation of readq/writeq is the code that 
every hardware vendor otherwise would re-create, when doing a Linux driver.

	Jeff

Re: [PATCH] x86: Remove readq()/writeq() on 32-bit

[Roland Dreier]

 > This removal patch is completely pointless, because it moves us
 > backwards to the point where we had a bunch of drivers defining it.

No, the current kernel still requires drivers to define it anyway,
because there are tons of 32-bit architectures that are not x86.

And more than that, centralizing the definition makes the API much more
dangerous for driver authors.

 > At least the networking drivers I messed with (until 11/2008) were
 > always fine with a non-atomic readq.

The commit to niu I keep citing (e23a59e1, "niu: Fix readq
implementation when architecture does not provide one.") shows that
drivers need to take care.  Now, the x86 implementation would happen to
work for that hardware, but eg drivers/infiniband/hw/amso1100 defines
readq with the opposite order -- whether that's required or just an
arbitrary choice, I don't know.  And drivers/infiniband/hw/mthca has
some uses of __raw_writeq() that only work if no other CPU accesses to
the same page can happen between the two halves, so it adds a per-page
spinlock for 32-bit architectures.  etc.

 - R.

Re: [PATCH] x86: Remove readq()/writeq() on 32-bit

[Jeff Garzik]

Roland Dreier wrote:
>  > This removal patch is completely pointless, because it moves us
>  > backwards to the point where we had a bunch of drivers defining it.
> 
> No, the current kernel still requires drivers to define it anyway,
> because there are tons of 32-bit architectures that are not x86.

Then let's fix that issue...  by propagating the common definition to 
other platforms that properly implement {read,write}[bwl] in terms of 
the PCI bus.


> And more than that, centralizing the definition makes the API much more
> dangerous for driver authors.

I think that's really cranking the hyperbole level to 11.

The common definition is... the one found most commonly in the wild. 
For weird drivers, they will do their own thing.

That's pretty much how other drivers handle things.

Apply your logic here to _any_ API in the kernel, for the same result.


>  > At least the networking drivers I messed with (until 11/2008) were
>  > always fine with a non-atomic readq.
> 
> The commit to niu I keep citing (e23a59e1, "niu: Fix readq
> implementation when architecture does not provide one.") shows that
> drivers need to take care.  Now, the x86 implementation would happen to

That commit also shows that, had the driver been using a common 
definition, problems would not have arisen.


> work for that hardware, but eg drivers/infiniband/hw/amso1100 defines
> readq with the opposite order -- whether that's required or just an

'required' seems unlikely, given that

a) their readq only exists when #ifndef readq, thus implying the 
driver-local readq is far less tested, on their most-tested, 
highest-volume platform.

b) their readq still operates in LE order -- as it should: 
read,write[bwl] were defined in terms of PCI originally, and thus 
defined to be LE.

c) their __raw_writeq writes in lower-32-bits-first, as one would expect


> arbitrary choice, I don't know.  And drivers/infiniband/hw/mthca has
> some uses of __raw_writeq() that only work if no other CPU accesses to
> the same page can happen between the two halves, so it adds a per-page
> spinlock for 32-bit architectures.  etc.

Any use of __raw_xxx implies that You Know What You're Doing And Accept 
The Consequences.  __raw_xxx means _you_ handle endian conversions, 
barriers, and other arch-specific details.  I don't think that a driver 
intentionally using the "raw" APIs is a good source of ideas and 
generalizations.

So, for your three examples,

	1) niu - common definition is OK

	2) amso1100 - common definition is OK; driver-local definition
	   never used on common PCI platforms

	3) mthca - intentionally uses raw API, an API which ditches
	   arch-specific barriers, endian conversions, and other
	   guarantees.

Given that, I see zero justification for API removal.  I see 
justification for propagating this code to other PCI-capable platforms.

Finally, I think given all this time we've had driver-define writeq and 
readq, and "driver authors were forced to think about this API" -- the 
result was the obvious definition now in place!

	Jeff

Re: [PATCH] x86: Remove readq()/writeq() on 32-bit

[Hitoshi Mitake]

On Thu, Apr 30, 2009 at 04:59, Jeff Garzik <jeff@garzik.org> wrote:
> Roland Dreier wrote:
>>
>>  > This removal patch is completely pointless, because it moves us
>>  > backwards to the point where we had a bunch of drivers defining it.
>>
>> No, the current kernel still requires drivers to define it anyway,
>> because there are tons of 32-bit architectures that are not x86.
>
> Then let's fix that issue...  by propagating the common definition to other
> platforms that properly implement {read,write}[bwl] in terms of the PCI bus.
>
>
>> And more than that, centralizing the definition makes the API much more
>> dangerous for driver authors.
>
> I think that's really cranking the hyperbole level to 11.
>
> The common definition is... the one found most commonly in the wild. For
> weird drivers, they will do their own thing.
>
> That's pretty much how other drivers handle things.
>
> Apply your logic here to _any_ API in the kernel, for the same result.
>
>
>>  > At least the networking drivers I messed with (until 11/2008) were
>>  > always fine with a non-atomic readq.
>>
>> The commit to niu I keep citing (e23a59e1, "niu: Fix readq
>> implementation when architecture does not provide one.") shows that
>> drivers need to take care.  Now, the x86 implementation would happen to
>
> That commit also shows that, had the driver been using a common definition,
> problems would not have arisen.
>
>
>> work for that hardware, but eg drivers/infiniband/hw/amso1100 defines
>> readq with the opposite order -- whether that's required or just an
>
> 'required' seems unlikely, given that
>
> a) their readq only exists when #ifndef readq, thus implying the
> driver-local readq is far less tested, on their most-tested, highest-volume
> platform.
>
> b) their readq still operates in LE order -- as it should: read,write[bwl]
> were defined in terms of PCI originally, and thus defined to be LE.
>
> c) their __raw_writeq writes in lower-32-bits-first, as one would expect
>
>
>> arbitrary choice, I don't know.  And drivers/infiniband/hw/mthca has
>> some uses of __raw_writeq() that only work if no other CPU accesses to
>> the same page can happen between the two halves, so it adds a per-page
>> spinlock for 32-bit architectures.  etc.
>
> Any use of __raw_xxx implies that You Know What You're Doing And Accept The
> Consequences.  __raw_xxx means _you_ handle endian conversions, barriers,
> and other arch-specific details.  I don't think that a driver intentionally
> using the "raw" APIs is a good source of ideas and generalizations.
>
> So, for your three examples,
>
>        1) niu - common definition is OK
>
>        2) amso1100 - common definition is OK; driver-local definition
>           never used on common PCI platforms
>
>        3) mthca - intentionally uses raw API, an API which ditches
>           arch-specific barriers, endian conversions, and other
>           guarantees.
>
> Given that, I see zero justification for API removal.  I see justification
> for propagating this code to other PCI-capable platforms.
>
> Finally, I think given all this time we've had driver-define writeq and
> readq, and "driver authors were forced to think about this API" -- the
> result was the obvious definition now in place!
>
>        Jeff
>
>
>
>
>

I think it's good time to decide making all architectures
which have readq/writeq provide HAVE_READQ/HAVE_WRITEQ or not.

Adding HAVE_READQ/HAVE_WRITEQ to Kconfig of architectures needs
agreement of all maintainers of these.

But, David Miller, maintainer of SPARC architecture, acked Roland's patch
because of the possibility of bugs non-atomicity of readq/writeq of
x86-32 will cause.

And, Jeff Garzik said that he saw zero justification for API removal.

Which way should we choose?
Remove readq/writeq from x86-32?
Or add HAVE... to all architectures with readq/writeq?

Re: [PATCH] x86: Remove readq()/writeq() on 32-bit

[H. Peter Anvin]

Hitoshi Mitake wrote:
> 
> I think it's good time to decide making all architectures
> which have readq/writeq provide HAVE_READQ/HAVE_WRITEQ or not.
> 
> Adding HAVE_READQ/HAVE_WRITEQ to Kconfig of architectures needs
> agreement of all maintainers of these.
> 
> But, David Miller, maintainer of SPARC architecture, acked Roland's patch
> because of the possibility of bugs non-atomicity of readq/writeq of
> x86-32 will cause.
> 
> And, Jeff Garzik said that he saw zero justification for API removal.
> 
> Which way should we choose?
> Remove readq/writeq from x86-32?
> Or add HAVE... to all architectures with readq/writeq?

There is another option, which is to have canned include variants, that
drivers can explicitly opt-in with:

#include <linux/io64_lh.h>

linux/io64_lh.h would then look like:

#ifndef _LINUX_IO64_LH_H
#define _LINUX_IO64_LH_H

#include <linux/io.h>

#ifndef HAVE_READQ

/* Low-High nonatomic readq() */

#endif

#ifndef HAVE_WRITEQ

/* Low-High nonatomic writeq() */

#endif

#endif


If we need more than io64_lh.h we can then add other variants, but I
suspect we won't need them -- except possibly an io64_hl.h.

	-hpa

Re: [PATCH] x86: Remove readq()/writeq() on 32-bit

[Jeff Garzik]

H. Peter Anvin wrote:
> #include <linux/io64_lh.h>
> 
> linux/io64_lh.h would then look like:
> 
> #ifndef _LINUX_IO64_LH_H
> #define _LINUX_IO64_LH_H
> 
> #include <linux/io.h>
> 
> #ifndef HAVE_READQ
> 
> /* Low-High nonatomic readq() */
> 
> #endif
> 
> #ifndef HAVE_WRITEQ
> 
> /* Low-High nonatomic writeq() */


Judging from this thread and past, I think people will continue to 
complain and get confused, even with the above.

How about

1) tree-wide rename: readq -> readq_na, writeq -> writeq_na

2) make all PCI writel-enabled arches provide readq_na and writeq_na

3) 64-bit PCI writel-enabled arches provide readq and HAVE_READQ, ditto 
for writeq.  32-bit provides neither readq nor HAVE_READQ.

4) If your hardware has non-standard ordering, handle it the obvious way 
in the driver, as you would do with any other special case.

	Jeff



P.S. I use "writel-enabled" to note that some platforms do not provide 
PCI read*/write* functions at all.

Re: [PATCH] x86: Remove readq()/writeq() on 32-bit

[H. Peter Anvin]

[-- Attachment #1: Type: text/plain, Size: 332 bytes --]

Jeff Garzik wrote:
> 
> Judging from this thread and past, I think people will continue to 
> complain and get confused, even with the above.
> 

Do you really think so?  Seems unfortunate, since an API rename would be
way more invasive.  This is the entirety of the header patch
(compile-tested using 32-bit allyesconfig).

	-hpa


[-- Attachment #2: diff --]
[-- Type: text/plain, Size: 5111 bytes --]

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index df9e885..0be277b 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -19,8 +19,6 @@ config X86_64
 config X86
 	def_bool y
 	select HAVE_AOUT if X86_32
-	select HAVE_READQ
-	select HAVE_WRITEQ
 	select HAVE_UNSTABLE_SCHED_CLOCK
 	select HAVE_IDE
 	select HAVE_OPROFILE
diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h
index 7373932..199c7b9 100644
--- a/arch/x86/include/asm/io.h
+++ b/arch/x86/include/asm/io.h
@@ -51,27 +51,6 @@ build_mmio_write(__writel, "l", unsigned int, "r", )
 build_mmio_read(readq, "q", unsigned long, "=r", :"memory")
 build_mmio_write(writeq, "q", unsigned long, "r", :"memory")
 
-#else
-
-static inline __u64 readq(const volatile void __iomem *addr)
-{
-	const volatile u32 __iomem *p = addr;
-	u32 low, high;
-
-	low = readl(p);
-	high = readl(p + 1);
-
-	return low + ((u64)high << 32);
-}
-
-static inline void writeq(__u64 val, volatile void __iomem *addr)
-{
-	writel(val, addr);
-	writel(val >> 32, addr+4);
-}
-
-#endif
-
 #define readq_relaxed(a)	readq(a)
 
 #define __raw_readq(a)		readq(a)
@@ -81,6 +60,8 @@ static inline void writeq(__u64 val, volatile void __iomem *addr)
 #define readq			readq
 #define writeq			writeq
 
+#endif
+
 /**
  *	virt_to_phys	-	map virtual addresses to physical
  *	@address: address to remap
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 2506592..43f14f5 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -33,6 +33,7 @@
 #include "i915_reg.h"
 #include "intel_bios.h"
 #include <linux/io-mapping.h>
+#include <linux/io64_lh.h>	/* Simulate 64-bit I/O on 32 bits */
 
 /* General customization:
  */
@@ -705,13 +706,7 @@ extern void intel_modeset_cleanup(struct drm_device *dev);
 #define I915_WRITE16(reg, val)	writel(val, dev_priv->regs + (reg))
 #define I915_READ8(reg)		readb(dev_priv->regs + (reg))
 #define I915_WRITE8(reg, val)	writeb(val, dev_priv->regs + (reg))
-#ifdef writeq
 #define I915_WRITE64(reg, val)	writeq(val, dev_priv->regs + (reg))
-#else
-#define I915_WRITE64(reg, val)	(writel(val, dev_priv->regs + (reg)), \
-				 writel(upper_32_bits(val), dev_priv->regs + \
-					(reg) + 4))
-#endif
 #define POSTING_READ(reg)	(void)I915_READ(reg)
 
 #define I915_VERBOSE 0
diff --git a/include/drm/drm_os_linux.h b/include/drm/drm_os_linux.h
index 26641e9..3925946 100644
--- a/include/drm/drm_os_linux.h
+++ b/include/drm/drm_os_linux.h
@@ -5,19 +5,7 @@
 
 #include <linux/interrupt.h>	/* For task queue support */
 #include <linux/delay.h>
-
-#ifndef readq
-static inline u64 readq(void __iomem *reg)
-{
-	return ((u64) readl(reg)) | (((u64) readl(reg + 4UL)) << 32);
-}
-
-static inline void writeq(u64 val, void __iomem *reg)
-{
-	writel(val & 0xffffffff, reg);
-	writel(val >> 32, reg + 0x4UL);
-}
-#endif
+#include <linux/io64_lh.h>	/* Simulate 64-bit I/O on 32 bits */
 
 /** Current process ID */
 #define DRM_CURRENTPID			task_pid_nr(current)
diff --git a/include/linux/io64_lh.h b/include/linux/io64_lh.h
new file mode 100644
index 0000000..361931b
--- /dev/null
+++ b/include/linux/io64_lh.h
@@ -0,0 +1,74 @@
+/* ----------------------------------------------------------------------- *
+ *
+ *  Copyright (C) 2009 Intel Corporation. All rights reserved.
+ *
+ *  This program is free software; you can redistribute it and/or
+ *  modify it under the terms of the GNU General Public License version
+ *  2 as published by the Free Software Foundation.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, write to the Free Software
+ *  Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
+ *  02110-1301, USA.
+ *
+ *  H. Peter Anvin <hpa@linux.intel.com>
+ *
+ * ----------------------------------------------------------------------- */
+
+/*
+ * io64_lh.h
+ *
+ * One possible implementation of readq()/writeq() on 32-bit platforms.
+ * Emulate 64-bit I/O using nonatomic low-high 32-bit references.
+ */
+
+#ifndef _LINUX_IO64_LH_H
+#define _LINUX_IO64_LH_H
+
+#include <linux/io.h>
+#include <linux/types.h>
+
+#ifndef readq
+static inline __u64 readq(const volatile void __iomem *addr)
+{
+	const volatile u32 __iomem *p = addr;
+	u32 l, h;
+
+	l = readl(p);
+	h = readl(p + 1);
+
+	return l + ((u64)h << 32);
+}
+#define readq		readq
+
+static inline __u64 readq_relaxed(const volatile void __iomem *addr)
+{
+	const volatile u32 __iomem *p = addr;
+	u32 l, h;
+
+	l = readl_relaxed(p);
+	h = readl_relaxed(p + 1);
+
+	return l + ((u64)h << 32);
+}
+#define readq_relaxed	readq_relaxed
+
+#endif /* readq */
+
+#ifndef writeq
+
+static inline void writeq(__u64 val, volatile void __iomem *addr)
+{
+	writel(val, addr);
+	writel(val >> 32, addr+4);
+}
+#define writeq		writeq
+
+#endif /* writeq */
+
+#endif /* _LINUX_IO64_LH_H */

Re: [PATCH] x86: Remove readq()/writeq() on 32-bit

[Jeff Garzik]

H. Peter Anvin wrote:
> Jeff Garzik wrote:
>> Judging from this thread and past, I think people will continue to 
>> complain and get confused, even with the above.
>>
> 
> Do you really think so?  Seems unfortunate, since an API rename would be
> way more invasive.  This is the entirety of the header patch
> (compile-tested using 32-bit allyesconfig).

The header patch does not lessen the confusion, because you cannot look 
at the code and immediately tell what is going on...

Having a single function's behavior change based on #include selection 
is /not/ intuitive at all, particularly for driver writers.  That is 
unlike almost every other Linux API, where functions' behavior stays 
constant across platforms, regardless of magic "under the hood."

That sort of trick is reserved for arch maintainers who know what they 
are doing :)

	Jeff

Re: [PATCH] x86: Remove readq()/writeq() on 32-bit

[Hitoshi Mitake]

On Wed, 13 May 2009 20:49:26 -0400
Jeff Garzik <jeff@garzik.org> wrote:

> H. Peter Anvin wrote:
> > Jeff Garzik wrote:
> >> Judging from this thread and past, I think people will continue to 
> >> complain and get confused, even with the above.
> >>
> > 
> > Do you really think so?  Seems unfortunate, since an API rename would be
> > way more invasive.  This is the entirety of the header patch
> > (compile-tested using 32-bit allyesconfig).
> 
> The header patch does not lessen the confusion, because you cannot look 
> at the code and immediately tell what is going on...
> 
> Having a single function's behavior change based on #include selection 
> is /not/ intuitive at all, particularly for driver writers.  That is 
> unlike almost every other Linux API, where functions' behavior stays 
> constant across platforms, regardless of magic "under the hood."
> 
> That sort of trick is reserved for arch maintainers who know what they 
> are doing :)
> 
> 	Jeff
> 
> 
> 

I found another way:
Making architecture with atomic readq/writeq provide HAVE_READQ_ATOMIC/HAVE_WRITEQ_ATOMIC
and making architecture with non-atomic readq/writeq provide HAVE_READQ/HAVE_WRITEQ.
(HAVE_READQ_ATOMIC/HAVE_WRITEQ_ATOMIC should double as HAVE_READQ/HAVE_WRITEQ.)

So driver programmers who need atomic readq/writeq can judge existence of API they really need.
If platform doesn't provide atomic readq/writeq, drivers need these can be disabled by Kconfig.
And bugs Roland and David talking about will be banished.
How about this? > Roland and David
I wrote a test patch. Request for comments.

Signed-off-by: Hitoshi Mitake <h.mitake@gmail.com>

---
 arch/x86/Kconfig |   16 ++++++++++++++--
 1 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index df9e885..c94fc48 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -19,8 +19,6 @@ config X86_64
 config X86
 	def_bool y
 	select HAVE_AOUT if X86_32
-	select HAVE_READQ
-	select HAVE_WRITEQ
 	select HAVE_UNSTABLE_SCHED_CLOCK
 	select HAVE_IDE
 	select HAVE_OPROFILE
@@ -2022,6 +2020,20 @@ config HAVE_ATOMIC_IOMAP
 	def_bool y
 	depends on X86_32
 
+config HAVE_READQ
+	def_bool y
+
+config HAVE_WRITEQ
+	def_bool y
+
+config HAVE_READQ_ATOMIC
+	def_bool y
+	depends on X86_64
+
+config HAVE_WRITEQ_ATOMIC
+	def_bool y
+	depends on X86_64
+
 source "net/Kconfig"
 
 source "drivers/Kconfig"
-- 
1.5.6.5

Re: [PATCH] x86: Remove readq()/writeq() on 32-bit

[Jeff Garzik]

Hitoshi Mitake wrote:
> On Wed, 13 May 2009 20:49:26 -0400
> Jeff Garzik <jeff@garzik.org> wrote:
> 
>> H. Peter Anvin wrote:
>>> Jeff Garzik wrote:
>>>> Judging from this thread and past, I think people will continue to 
>>>> complain and get confused, even with the above.
>>>>
>>> Do you really think so?  Seems unfortunate, since an API rename would be
>>> way more invasive.  This is the entirety of the header patch
>>> (compile-tested using 32-bit allyesconfig).
>> The header patch does not lessen the confusion, because you cannot look 
>> at the code and immediately tell what is going on...
>>
>> Having a single function's behavior change based on #include selection 
>> is /not/ intuitive at all, particularly for driver writers.  That is 
>> unlike almost every other Linux API, where functions' behavior stays 
>> constant across platforms, regardless of magic "under the hood."
>>
>> That sort of trick is reserved for arch maintainers who know what they 
>> are doing :)
>>
>> 	Jeff
>>
>>
>>
> 
> I found another way:
> Making architecture with atomic readq/writeq provide HAVE_READQ_ATOMIC/HAVE_WRITEQ_ATOMIC
> and making architecture with non-atomic readq/writeq provide HAVE_READQ/HAVE_WRITEQ.
> (HAVE_READQ_ATOMIC/HAVE_WRITEQ_ATOMIC should double as HAVE_READQ/HAVE_WRITEQ.)
> 
> So driver programmers who need atomic readq/writeq can judge existence of API they really need.
> If platform doesn't provide atomic readq/writeq, drivers need these can be disabled by Kconfig.
> And bugs Roland and David talking about will be banished.
> How about this? > Roland and David
> I wrote a test patch. Request for comments.
> 
> Signed-off-by: Hitoshi Mitake <h.mitake@gmail.com>
> 
> ---
>  arch/x86/Kconfig |   16 ++++++++++++++--
>  1 files changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index df9e885..c94fc48 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -19,8 +19,6 @@ config X86_64
>  config X86
>  	def_bool y
>  	select HAVE_AOUT if X86_32
> -	select HAVE_READQ
> -	select HAVE_WRITEQ
>  	select HAVE_UNSTABLE_SCHED_CLOCK
>  	select HAVE_IDE
>  	select HAVE_OPROFILE
> @@ -2022,6 +2020,20 @@ config HAVE_ATOMIC_IOMAP
>  	def_bool y
>  	depends on X86_32
>  
> +config HAVE_READQ
> +	def_bool y
> +
> +config HAVE_WRITEQ
> +	def_bool y
> +
> +config HAVE_READQ_ATOMIC
> +	def_bool y
> +	depends on X86_64
> +
> +config HAVE_WRITEQ_ATOMIC
> +	def_bool y
> +	depends on X86_64

If you create HAVE_{READQ,WRITEQ}_ATOMIC, then you don't really need 
HAVE_READQ -- the other relevant 32-bit platforms simply need a 
definition of readq and writeq.  Probably easy enough to have a common 
definition in asm-generic.

	Jeff

Re: [PATCH] x86: Remove readq()/writeq() on 32-bit

[Hitoshi Mitake]

On Fri, 15 May 2009 19:44:03 -0400
Jeff Garzik <jeff@garzik.org> wrote:

> Hitoshi Mitake wrote:
> > On Wed, 13 May 2009 20:49:26 -0400
> > Jeff Garzik <jeff@garzik.org> wrote:
> > 
> >> H. Peter Anvin wrote:
> >>> Jeff Garzik wrote:
> >>>> Judging from this thread and past, I think people will continue to 
> >>>> complain and get confused, even with the above.
> >>>>
> >>> Do you really think so?  Seems unfortunate, since an API rename would be
> >>> way more invasive.  This is the entirety of the header patch
> >>> (compile-tested using 32-bit allyesconfig).
> >> The header patch does not lessen the confusion, because you cannot look 
> >> at the code and immediately tell what is going on...
> >>
> >> Having a single function's behavior change based on #include selection 
> >> is /not/ intuitive at all, particularly for driver writers.  That is 
> >> unlike almost every other Linux API, where functions' behavior stays 
> >> constant across platforms, regardless of magic "under the hood."
> >>
> >> That sort of trick is reserved for arch maintainers who know what they 
> >> are doing :)
> >>
> >> 	Jeff
> >>
> >>
> >>
> > 
> > I found another way:
> > Making architecture with atomic readq/writeq provide HAVE_READQ_ATOMIC/HAVE_WRITEQ_ATOMIC
> > and making architecture with non-atomic readq/writeq provide HAVE_READQ/HAVE_WRITEQ.
> > (HAVE_READQ_ATOMIC/HAVE_WRITEQ_ATOMIC should double as HAVE_READQ/HAVE_WRITEQ.)
> > 
> > So driver programmers who need atomic readq/writeq can judge existence of API they really need.
> > If platform doesn't provide atomic readq/writeq, drivers need these can be disabled by Kconfig.
> > And bugs Roland and David talking about will be banished.
> > How about this? > Roland and David
> > I wrote a test patch. Request for comments.
> > 
> > Signed-off-by: Hitoshi Mitake <h.mitake@gmail.com>
> > 
> > ---
> >  arch/x86/Kconfig |   16 ++++++++++++++--
> >  1 files changed, 14 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> > index df9e885..c94fc48 100644
> > --- a/arch/x86/Kconfig
> > +++ b/arch/x86/Kconfig
> > @@ -19,8 +19,6 @@ config X86_64
> >  config X86
> >  	def_bool y
> >  	select HAVE_AOUT if X86_32
> > -	select HAVE_READQ
> > -	select HAVE_WRITEQ
> >  	select HAVE_UNSTABLE_SCHED_CLOCK
> >  	select HAVE_IDE
> >  	select HAVE_OPROFILE
> > @@ -2022,6 +2020,20 @@ config HAVE_ATOMIC_IOMAP
> >  	def_bool y
> >  	depends on X86_32
> >  
> > +config HAVE_READQ
> > +	def_bool y
> > +
> > +config HAVE_WRITEQ
> > +	def_bool y
> > +
> > +config HAVE_READQ_ATOMIC
> > +	def_bool y
> > +	depends on X86_64
> > +
> > +config HAVE_WRITEQ_ATOMIC
> > +	def_bool y
> > +	depends on X86_64
> 
> If you create HAVE_{READQ,WRITEQ}_ATOMIC, then you don't really need 
> HAVE_READQ -- the other relevant 32-bit platforms simply need a 
> definition of readq and writeq.  Probably easy enough to have a common 
> definition in asm-generic.
> 
That's good idea. I didn't noticed the way to use asm-generic. Thanks.

How is this?

Signed-off-by: Hitoshi Mitake <h.mitake@gmail.com>

---
 arch/x86/Kconfig             |   10 ++++++++--
 arch/x86/include/asm/io.h    |   27 ++++++---------------------
 include/asm-generic/quadrw.h |   34 ++++++++++++++++++++++++++++++++++
 3 files changed, 48 insertions(+), 23 deletions(-)
 create mode 100644 include/asm-generic/quadrw.h

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index df9e885..151b6a0 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -19,8 +19,6 @@ config X86_64
 config X86
 	def_bool y
 	select HAVE_AOUT if X86_32
-	select HAVE_READQ
-	select HAVE_WRITEQ
 	select HAVE_UNSTABLE_SCHED_CLOCK
 	select HAVE_IDE
 	select HAVE_OPROFILE
@@ -2022,6 +2020,14 @@ config HAVE_ATOMIC_IOMAP
 	def_bool y
 	depends on X86_32
 
+config HAVE_READQ_ATOMIC
+	def_bool y
+	depends on X86_64
+
+config HAVE_WRITEQ_ATOMIC
+	def_bool y
+	depends on X86_64
+
 source "net/Kconfig"
 
 source "drivers/Kconfig"
diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h
index 7373932..bad940d 100644
--- a/arch/x86/include/asm/io.h
+++ b/arch/x86/include/asm/io.h
@@ -51,32 +51,17 @@ build_mmio_write(__writel, "l", unsigned int, "r", )
 build_mmio_read(readq, "q", unsigned long, "=r", :"memory")
 build_mmio_write(writeq, "q", unsigned long, "r", :"memory")
 
-#else
-
-static inline __u64 readq(const volatile void __iomem *addr)
-{
-	const volatile u32 __iomem *p = addr;
-	u32 low, high;
-
-	low = readl(p);
-	high = readl(p + 1);
-
-	return low + ((u64)high << 32);
-}
-
-static inline void writeq(__u64 val, volatile void __iomem *addr)
-{
-	writel(val, addr);
-	writel(val >> 32, addr+4);
-}
-
-#endif
-
 #define readq_relaxed(a)	readq(a)
 
 #define __raw_readq(a)		readq(a)
 #define __raw_writeq(val, addr)	writeq(val, addr)
 
+#endif	/* CONFIG_X86_64 */
+
+#ifdef CONFIG_X86_32
+#include <asm-generic/quadrw.h>
+#endif	/* CONFIG_X86_32 */
+
 /* Let people know that we have them */
 #define readq			readq
 #define writeq			writeq
diff --git a/include/asm-generic/quadrw.h b/include/asm-generic/quadrw.h
new file mode 100644
index 0000000..78159a2
--- /dev/null
+++ b/include/asm-generic/quadrw.h
@@ -0,0 +1,34 @@
+#ifndef GENERIC_QUADRW_H
+#define GENERIC_QUADRW_H
+
+#include <asm/io.h>
+
+/*
+ * General readq/writeq implementation.
+ * These are not atomic operations.
+ * The drivers which need atomic version readq/writeq,
+ * they should depend on HAVE_{READQ,WRITEQ}_ATOMIC in Kconfig level.
+ */
+
+#ifndef CONFIG_HAVE_READQ_ATOMIC
+static inline __u64 readq(const volatile void __iomem *addr)
+{
+	const volatile u32 __iomem *p = addr;
+	u32 low, high;
+
+	low = readl(p);
+	high = readl(p + 1);
+
+	return low + ((u64)high << 32);
+}
+#endif	/* CONFIG_HAVE_READQ_ATOMIC */
+
+#ifndef CONFIG_HAVE_WRITEQ_ATOMIC
+static inline void writeq(__u64 val, volatile void __iomem *addr)
+{
+	writel(val, addr);
+	writel(val >> 32, addr+4);
+}
+#endif	/* CONFIG_HAVE_WRITEQ_ATOMIC */
+
+#endif	/* GENERIC_QUADRW_H */
-- 
1.5.6.5

Re: [PATCH] x86: Remove readq()/writeq() on 32-bit

[Jeff Garzik]

Hitoshi Mitake wrote:
> On Fri, 15 May 2009 19:44:03 -0400
> Jeff Garzik <jeff@garzik.org> wrote:
> 
>> Hitoshi Mitake wrote:
>>> On Wed, 13 May 2009 20:49:26 -0400
>>> Jeff Garzik <jeff@garzik.org> wrote:
>>>
>>>> H. Peter Anvin wrote:
>>>>> Jeff Garzik wrote:
>>>>>> Judging from this thread and past, I think people will continue to 
>>>>>> complain and get confused, even with the above.
>>>>>>
>>>>> Do you really think so?  Seems unfortunate, since an API rename would be
>>>>> way more invasive.  This is the entirety of the header patch
>>>>> (compile-tested using 32-bit allyesconfig).
>>>> The header patch does not lessen the confusion, because you cannot look 
>>>> at the code and immediately tell what is going on...
>>>>
>>>> Having a single function's behavior change based on #include selection 
>>>> is /not/ intuitive at all, particularly for driver writers.  That is 
>>>> unlike almost every other Linux API, where functions' behavior stays 
>>>> constant across platforms, regardless of magic "under the hood."
>>>>
>>>> That sort of trick is reserved for arch maintainers who know what they 
>>>> are doing :)
>>>>
>>>> 	Jeff
>>>>
>>>>
>>>>
>>> I found another way:
>>> Making architecture with atomic readq/writeq provide HAVE_READQ_ATOMIC/HAVE_WRITEQ_ATOMIC
>>> and making architecture with non-atomic readq/writeq provide HAVE_READQ/HAVE_WRITEQ.
>>> (HAVE_READQ_ATOMIC/HAVE_WRITEQ_ATOMIC should double as HAVE_READQ/HAVE_WRITEQ.)
>>>
>>> So driver programmers who need atomic readq/writeq can judge existence of API they really need.
>>> If platform doesn't provide atomic readq/writeq, drivers need these can be disabled by Kconfig.
>>> And bugs Roland and David talking about will be banished.
>>> How about this? > Roland and David
>>> I wrote a test patch. Request for comments.
>>>
>>> Signed-off-by: Hitoshi Mitake <h.mitake@gmail.com>
>>>
>>> ---
>>>  arch/x86/Kconfig |   16 ++++++++++++++--
>>>  1 files changed, 14 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
>>> index df9e885..c94fc48 100644
>>> --- a/arch/x86/Kconfig
>>> +++ b/arch/x86/Kconfig
>>> @@ -19,8 +19,6 @@ config X86_64
>>>  config X86
>>>  	def_bool y
>>>  	select HAVE_AOUT if X86_32
>>> -	select HAVE_READQ
>>> -	select HAVE_WRITEQ
>>>  	select HAVE_UNSTABLE_SCHED_CLOCK
>>>  	select HAVE_IDE
>>>  	select HAVE_OPROFILE
>>> @@ -2022,6 +2020,20 @@ config HAVE_ATOMIC_IOMAP
>>>  	def_bool y
>>>  	depends on X86_32
>>>  
>>> +config HAVE_READQ
>>> +	def_bool y
>>> +
>>> +config HAVE_WRITEQ
>>> +	def_bool y
>>> +
>>> +config HAVE_READQ_ATOMIC
>>> +	def_bool y
>>> +	depends on X86_64
>>> +
>>> +config HAVE_WRITEQ_ATOMIC
>>> +	def_bool y
>>> +	depends on X86_64
>> If you create HAVE_{READQ,WRITEQ}_ATOMIC, then you don't really need 
>> HAVE_READQ -- the other relevant 32-bit platforms simply need a 
>> definition of readq and writeq.  Probably easy enough to have a common 
>> definition in asm-generic.
>>
> That's good idea. I didn't noticed the way to use asm-generic. Thanks.
> 
> How is this?
> 
> Signed-off-by: Hitoshi Mitake <h.mitake@gmail.com>
> 
> ---
>  arch/x86/Kconfig             |   10 ++++++++--
>  arch/x86/include/asm/io.h    |   27 ++++++---------------------
>  include/asm-generic/quadrw.h |   34 ++++++++++++++++++++++++++++++++++
>  3 files changed, 48 insertions(+), 23 deletions(-)
>  create mode 100644 include/asm-generic/quadrw.h


Seems fine to me, no objections here...



> +#ifndef CONFIG_HAVE_READQ_ATOMIC
> +static inline __u64 readq(const volatile void __iomem *addr)
> +{
> +	const volatile u32 __iomem *p = addr;
> +	u32 low, high;
> +
> +	low = readl(p);
> +	high = readl(p + 1);
> +
> +	return low + ((u64)high << 32);
> +}
> +#endif	/* CONFIG_HAVE_READQ_ATOMIC */

Personally I would do

	static inline __u64 readq(const volatile void __iomem *addr)
	{
		__u64 low, high;

		low = readl(addr);
		high = readl(addr + 4);

		return (high << 32) | low;
	}

but maybe that's just me.

It seems inconsistent that the generic readq and writeq internals, 
simple as they are, differ to such a degree in this implementation.

But overall, as mentioned above, the approach seems sound to me.

Cheers!

	Jeff

Re: [PATCH] x86: Remove readq()/writeq() on 32-bit

[Hitoshi Mitake]

On Sun, May 17, 2009 at 17:06, Jeff Garzik <jeff@garzik.org> wrote:
> Hitoshi Mitake wrote:
>>
>> On Fri, 15 May 2009 19:44:03 -0400
>> Jeff Garzik <jeff@garzik.org> wrote:
>>
>>> Hitoshi Mitake wrote:
>>>>
>>>> On Wed, 13 May 2009 20:49:26 -0400
>>>> Jeff Garzik <jeff@garzik.org> wrote:
>>>>
>>>>> H. Peter Anvin wrote:
>>>>>>
>>>>>> Jeff Garzik wrote:
>>>>>>>
>>>>>>> Judging from this thread and past, I think people will continue to
>>>>>>> complain and get confused, even with the above.
>>>>>>>
>>>>>> Do you really think so?  Seems unfortunate, since an API rename would
>>>>>> be
>>>>>> way more invasive.  This is the entirety of the header patch
>>>>>> (compile-tested using 32-bit allyesconfig).
>>>>>
>>>>> The header patch does not lessen the confusion, because you cannot look
>>>>> at the code and immediately tell what is going on...
>>>>>
>>>>> Having a single function's behavior change based on #include selection
>>>>> is /not/ intuitive at all, particularly for driver writers.  That is unlike
>>>>> almost every other Linux API, where functions' behavior stays constant
>>>>> across platforms, regardless of magic "under the hood."
>>>>>
>>>>> That sort of trick is reserved for arch maintainers who know what they
>>>>> are doing :)
>>>>>
>>>>>        Jeff
>>>>>
>>>>>
>>>>>
>>>> I found another way:
>>>> Making architecture with atomic readq/writeq provide
>>>> HAVE_READQ_ATOMIC/HAVE_WRITEQ_ATOMIC
>>>> and making architecture with non-atomic readq/writeq provide
>>>> HAVE_READQ/HAVE_WRITEQ.
>>>> (HAVE_READQ_ATOMIC/HAVE_WRITEQ_ATOMIC should double as
>>>> HAVE_READQ/HAVE_WRITEQ.)
>>>>
>>>> So driver programmers who need atomic readq/writeq can judge existence
>>>> of API they really need.
>>>> If platform doesn't provide atomic readq/writeq, drivers need these can
>>>> be disabled by Kconfig.
>>>> And bugs Roland and David talking about will be banished.
>>>> How about this? > Roland and David
>>>> I wrote a test patch. Request for comments.
>>>>
>>>> Signed-off-by: Hitoshi Mitake <h.mitake@gmail.com>
>>>>
>>>> ---
>>>>  arch/x86/Kconfig |   16 ++++++++++++++--
>>>>  1 files changed, 14 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
>>>> index df9e885..c94fc48 100644
>>>> --- a/arch/x86/Kconfig
>>>> +++ b/arch/x86/Kconfig
>>>> @@ -19,8 +19,6 @@ config X86_64
>>>>  config X86
>>>>        def_bool y
>>>>        select HAVE_AOUT if X86_32
>>>> -       select HAVE_READQ
>>>> -       select HAVE_WRITEQ
>>>>        select HAVE_UNSTABLE_SCHED_CLOCK
>>>>        select HAVE_IDE
>>>>        select HAVE_OPROFILE
>>>> @@ -2022,6 +2020,20 @@ config HAVE_ATOMIC_IOMAP
>>>>        def_bool y
>>>>        depends on X86_32
>>>>  +config HAVE_READQ
>>>> +       def_bool y
>>>> +
>>>> +config HAVE_WRITEQ
>>>> +       def_bool y
>>>> +
>>>> +config HAVE_READQ_ATOMIC
>>>> +       def_bool y
>>>> +       depends on X86_64
>>>> +
>>>> +config HAVE_WRITEQ_ATOMIC
>>>> +       def_bool y
>>>> +       depends on X86_64
>>>
>>> If you create HAVE_{READQ,WRITEQ}_ATOMIC, then you don't really need
>>> HAVE_READQ -- the other relevant 32-bit platforms simply need a definition
>>> of readq and writeq.  Probably easy enough to have a common definition in
>>> asm-generic.
>>>
>> That's good idea. I didn't noticed the way to use asm-generic. Thanks.
>>
>> How is this?
>>
>> Signed-off-by: Hitoshi Mitake <h.mitake@gmail.com>
>>
>> ---
>>  arch/x86/Kconfig             |   10 ++++++++--
>>  arch/x86/include/asm/io.h    |   27 ++++++---------------------
>>  include/asm-generic/quadrw.h |   34 ++++++++++++++++++++++++++++++++++
>>  3 files changed, 48 insertions(+), 23 deletions(-)
>>  create mode 100644 include/asm-generic/quadrw.h
>
>
> Seems fine to me, no objections here...
>
>
>
>> +#ifndef CONFIG_HAVE_READQ_ATOMIC
>> +static inline __u64 readq(const volatile void __iomem *addr)
>> +{
>> +       const volatile u32 __iomem *p = addr;
>> +       u32 low, high;
>> +
>> +       low = readl(p);
>> +       high = readl(p + 1);
>> +
>> +       return low + ((u64)high << 32);
>> +}
>> +#endif /* CONFIG_HAVE_READQ_ATOMIC */
>
> Personally I would do
>
>        static inline __u64 readq(const volatile void __iomem *addr)
>        {
>                __u64 low, high;
>
>                low = readl(addr);
>                high = readl(addr + 4);
>
>                return (high << 32) | low;
>        }
>
> but maybe that's just me.
>
> It seems inconsistent that the generic readq and writeq internals, simple as
> they are, differ to such a degree in this implementation.
>
> But overall, as mentioned above, the approach seems sound to me.
>
> Cheers!
>
>        Jeff
>
>
>

Please discard this mail, this is only Ccing to my another email
address of university.
Because IMAP of Gmail is too slow to read LKML...
So I'll switch my email address from h.mitake@gmail.com to
mitake@dcl.info.waseda.ac.jp,
and reply from new one.

Re: [PATCH] x86: Remove readq()/writeq() on 32-bit

[Hitoshi Mitake]

On Thu, 21 May 2009 20:35:54 +0900
Hitoshi Mitake <h.mitake@gmail.com> wrote:

> On Sun, May 17, 2009 at 17:06, Jeff Garzik <jeff@garzik.org> wrote:
> > Hitoshi Mitake wrote:
> >>
> >> On Fri, 15 May 2009 19:44:03 -0400
> >> Jeff Garzik <jeff@garzik.org> wrote:
> >>
> >>> Hitoshi Mitake wrote:
> >>>>
> >>>> On Wed, 13 May 2009 20:49:26 -0400
> >>>> Jeff Garzik <jeff@garzik.org> wrote:
> >>>>
> >>>>> H. Peter Anvin wrote:
> >>>>>>
> >>>>>> Jeff Garzik wrote:
> >>>>>>>
> >>>>>>> Judging from this thread and past, I think people will continue to
> >>>>>>> complain and get confused, even with the above.
> >>>>>>>
> >>>>>> Do you really think so?  Seems unfortunate, since an API rename would
> >>>>>> be
> >>>>>> way more invasive.  This is the entirety of the header patch
> >>>>>> (compile-tested using 32-bit allyesconfig).
> >>>>>
> >>>>> The header patch does not lessen the confusion, because you cannot look
> >>>>> at the code and immediately tell what is going on...
> >>>>>
> >>>>> Having a single function's behavior change based on #include selection
> >>>>> is /not/ intuitive at all, particularly for driver writers.  That is unlike
> >>>>> almost every other Linux API, where functions' behavior stays constant
> >>>>> across platforms, regardless of magic "under the hood."
> >>>>>
> >>>>> That sort of trick is reserved for arch maintainers who know what they
> >>>>> are doing :)
> >>>>>
> >>>>>        Jeff
> >>>>>
> >>>>>
> >>>>>
> >>>> I found another way:
> >>>> Making architecture with atomic readq/writeq provide
> >>>> HAVE_READQ_ATOMIC/HAVE_WRITEQ_ATOMIC
> >>>> and making architecture with non-atomic readq/writeq provide
> >>>> HAVE_READQ/HAVE_WRITEQ.
> >>>> (HAVE_READQ_ATOMIC/HAVE_WRITEQ_ATOMIC should double as
> >>>> HAVE_READQ/HAVE_WRITEQ.)
> >>>>
> >>>> So driver programmers who need atomic readq/writeq can judge existence
> >>>> of API they really need.
> >>>> If platform doesn't provide atomic readq/writeq, drivers need these can
> >>>> be disabled by Kconfig.
> >>>> And bugs Roland and David talking about will be banished.
> >>>> How about this? > Roland and David
> >>>> I wrote a test patch. Request for comments.
> >>>>
> >>>> Signed-off-by: Hitoshi Mitake <h.mitake@gmail.com>
> >>>>
> >>>> ---
> >>>>  arch/x86/Kconfig |   16 ++++++++++++++--
> >>>>  1 files changed, 14 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> >>>> index df9e885..c94fc48 100644
> >>>> --- a/arch/x86/Kconfig
> >>>> +++ b/arch/x86/Kconfig
> >>>> @@ -19,8 +19,6 @@ config X86_64
> >>>>  config X86
> >>>>        def_bool y
> >>>>        select HAVE_AOUT if X86_32
> >>>> -       select HAVE_READQ
> >>>> -       select HAVE_WRITEQ
> >>>>        select HAVE_UNSTABLE_SCHED_CLOCK
> >>>>        select HAVE_IDE
> >>>>        select HAVE_OPROFILE
> >>>> @@ -2022,6 +2020,20 @@ config HAVE_ATOMIC_IOMAP
> >>>>        def_bool y
> >>>>        depends on X86_32
> >>>>  +config HAVE_READQ
> >>>> +       def_bool y
> >>>> +
> >>>> +config HAVE_WRITEQ
> >>>> +       def_bool y
> >>>> +
> >>>> +config HAVE_READQ_ATOMIC
> >>>> +       def_bool y
> >>>> +       depends on X86_64
> >>>> +
> >>>> +config HAVE_WRITEQ_ATOMIC
> >>>> +       def_bool y
> >>>> +       depends on X86_64
> >>>
> >>> If you create HAVE_{READQ,WRITEQ}_ATOMIC, then you don't really need
> >>> HAVE_READQ -- the other relevant 32-bit platforms simply need a definition
> >>> of readq and writeq.  Probably easy enough to have a common definition in
> >>> asm-generic.
> >>>
> >> That's good idea. I didn't noticed the way to use asm-generic. Thanks.
> >>
> >> How is this?
> >>
> >> Signed-off-by: Hitoshi Mitake <h.mitake@gmail.com>
> >>
> >> ---
> >>  arch/x86/Kconfig             |   10 ++++++++--
> >>  arch/x86/include/asm/io.h    |   27 ++++++---------------------
> >>  include/asm-generic/quadrw.h |   34 ++++++++++++++++++++++++++++++++++
> >>  3 files changed, 48 insertions(+), 23 deletions(-)
> >>  create mode 100644 include/asm-generic/quadrw.h
> >
> >
> > Seems fine to me, no objections here...
> >
> >
> >
> >> +#ifndef CONFIG_HAVE_READQ_ATOMIC
> >> +static inline __u64 readq(const volatile void __iomem *addr)
> >> +{
> >> +       const volatile u32 __iomem *p = addr;
> >> +       u32 low, high;
> >> +
> >> +       low = readl(p);
> >> +       high = readl(p + 1);
> >> +
> >> +       return low + ((u64)high << 32);
> >> +}
> >> +#endif /* CONFIG_HAVE_READQ_ATOMIC */
> >
> > Personally I would do
> >
> >        static inline __u64 readq(const volatile void __iomem *addr)
> >        {
> >                __u64 low, high;
> >
> >                low = readl(addr);
> >                high = readl(addr + 4);
> >
> >                return (high << 32) | low;
> >        }
> >
> > but maybe that's just me.
> >
> > It seems inconsistent that the generic readq and writeq internals, simple as
> > they are, differ to such a degree in this implementation.
> >
> > But overall, as mentioned above, the approach seems sound to me.
> >
> > Cheers!
> >
> >        Jeff
> >
> >
> >
> 

I fixed readq according to Jeff's advice. I think his readq is smarter than mine.
This is new version of the patch.

So I want to hear Roland and David's opinion.
We will be able to avoid making terrible bugs with this patch.
And generic readq/writeq will be provided for the cases
without requirement of atomic readq/writeq.

This patch would make our life easily. How do you think?

Signed-off-by: Hitoshi Mitake <mitake@dcl.info.waseda.ac.jp>
Cc: Jeff Garzik <jeff@garzik.org>
Cc: Roland Dreier <rolandd@cisco.com>
Cc: David S. Miller <davem@davemloft.net>

---
 arch/x86/Kconfig             |   10 ++++++++--
 arch/x86/include/asm/io.h    |   27 ++++++---------------------
 include/asm-generic/quadrw.h |   34 ++++++++++++++++++++++++++++++++++
 3 files changed, 48 insertions(+), 23 deletions(-)
 create mode 100644 include/asm-generic/quadrw.h

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index a6efe0a..46ea47c 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -19,8 +19,6 @@ config X86_64
 config X86
 	def_bool y
 	select HAVE_AOUT if X86_32
-	select HAVE_READQ
-	select HAVE_WRITEQ
 	select HAVE_UNSTABLE_SCHED_CLOCK
 	select HAVE_IDE
 	select HAVE_OPROFILE
@@ -2035,6 +2033,14 @@ config HAVE_ATOMIC_IOMAP
 	def_bool y
 	depends on X86_32
 
+config HAVE_READQ_ATOMIC
+	def_bool y
+	depends on X86_64
+
+config HAVE_WRITEQ_ATOMIC
+	def_bool y
+	depends on X86_64
+
 source "net/Kconfig"
 
 source "drivers/Kconfig"
diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h
index 7373932..bad940d 100644
--- a/arch/x86/include/asm/io.h
+++ b/arch/x86/include/asm/io.h
@@ -51,32 +51,17 @@ build_mmio_write(__writel, "l", unsigned int, "r", )
 build_mmio_read(readq, "q", unsigned long, "=r", :"memory")
 build_mmio_write(writeq, "q", unsigned long, "r", :"memory")
 
-#else
-
-static inline __u64 readq(const volatile void __iomem *addr)
-{
-	const volatile u32 __iomem *p = addr;
-	u32 low, high;
-
-	low = readl(p);
-	high = readl(p + 1);
-
-	return low + ((u64)high << 32);
-}
-
-static inline void writeq(__u64 val, volatile void __iomem *addr)
-{
-	writel(val, addr);
-	writel(val >> 32, addr+4);
-}
-
-#endif
-
 #define readq_relaxed(a)	readq(a)
 
 #define __raw_readq(a)		readq(a)
 #define __raw_writeq(val, addr)	writeq(val, addr)
 
+#endif	/* CONFIG_X86_64 */
+
+#ifdef CONFIG_X86_32
+#include <asm-generic/quadrw.h>
+#endif	/* CONFIG_X86_32 */
+
 /* Let people know that we have them */
 #define readq			readq
 #define writeq			writeq
diff --git a/include/asm-generic/quadrw.h b/include/asm-generic/quadrw.h
new file mode 100644
index 0000000..15856ac
--- /dev/null
+++ b/include/asm-generic/quadrw.h
@@ -0,0 +1,34 @@
+#ifndef GENERIC_QUADRW_H
+#define GENERIC_QUADRW_H
+
+#include <asm/io.h>
+
+/*
+ * General readq/writeq implementation.
+ * These are not atomic operations.
+ * The drivers which need atomic version readq/writeq,
+ * they should depend on HAVE_{READQ,WRITEQ}_ATOMIC in Kconfig level.
+ */
+
+#ifndef CONFIG_HAVE_READQ_ATOMIC
+static inline __u64 readq(const volatile void __iomem *addr)
+{
+	__u64 low, high;
+
+	low = readl(addr);
+	high = readl(addr + 4);
+
+	return (high << 32) | low;
+}
+
+#endif	/* CONFIG_HAVE_READQ_ATOMIC */
+
+#ifndef CONFIG_HAVE_WRITEQ_ATOMIC
+static inline void writeq(__u64 val, volatile void __iomem *addr)
+{
+	writel(val, addr);
+	writel(val >> 32, addr+4);
+}
+#endif	/* CONFIG_HAVE_WRITEQ_ATOMIC */
+
+#endif	/* GENERIC_QUADRW_H */
-- 
1.5.6.5

Re: [PATCH] x86: Remove readq()/writeq() on 32-bit

[Jeff Garzik]

Hitoshi Mitake wrote:
> I think it's good time to decide making all architectures
> which have readq/writeq provide HAVE_READQ/HAVE_WRITEQ or not.
> 
> Adding HAVE_READQ/HAVE_WRITEQ to Kconfig of architectures needs
> agreement of all maintainers of these.
> 
> But, David Miller, maintainer of SPARC architecture, acked Roland's patch
> because of the possibility of bugs non-atomicity of readq/writeq of
> x86-32 will cause.
> 
> And, Jeff Garzik said that he saw zero justification for API removal.
> 
> Which way should we choose?
> Remove readq/writeq from x86-32?
> Or add HAVE... to all architectures with readq/writeq?

To repeat what has already been stated, each case was re-evaluated: 
http://marc.info/?l=linux-kernel&m=124103527326835&w=2

Roland's patch was acked, apparently, _in spite of_ the commonly 
accepted readq() definition already being in use!

Thusfar, I see two things:

(1) years of history has shown that non-atomic readq/writeq on 32-bit 
platforms has been sufficient, based on testing and experience.  In 
fact, in niu's case, a common readq/writeq would have PREVENTED a bug.

(2) unspecified fears continue to linger about non-atomicity

We should not base decisions on fear, particularly when the weight of 
evidence and experience points in the other direction.

	Jeff

Re: [PATCH] x86: Remove readq()/writeq() on 32-bit

[H. Peter Anvin]

Jeff Garzik wrote:
> 
> Roland's patch was acked, apparently, _in spite of_ the commonly 
> accepted readq() definition already being in use!
> 
> Thusfar, I see two things:
> 
> (1) years of history has shown that non-atomic readq/writeq on 32-bit 
> platforms has been sufficient, based on testing and experience.  In 
> fact, in niu's case, a common readq/writeq would have PREVENTED a bug.
> 
> (2) unspecified fears continue to linger about non-atomicity
> 
> We should not base decisions on fear, particularly when the weight of 
> evidence and experience points in the other direction.
> 

I have personally dealt with at least one device who'd want to opt out
of a standard readq/writeq (it's not in-tree because it never shipped,
unfortunately.)  Doing the opt-in headers seems like a reasonable thing
to do to me, but perhaps I'm just being overly paranoid.

	-hpa

Re: [PATCH] x86: Remove readq()/writeq() on 32-bit

[Jeff Garzik]

H. Peter Anvin wrote:
> Jeff Garzik wrote:
>> Roland's patch was acked, apparently, _in spite of_ the commonly 
>> accepted readq() definition already being in use!
>>
>> Thusfar, I see two things:
>>
>> (1) years of history has shown that non-atomic readq/writeq on 32-bit 
>> platforms has been sufficient, based on testing and experience.  In 
>> fact, in niu's case, a common readq/writeq would have PREVENTED a bug.
>>
>> (2) unspecified fears continue to linger about non-atomicity
>>
>> We should not base decisions on fear, particularly when the weight of 
>> evidence and experience points in the other direction.
>>
> 
> I have personally dealt with at least one device who'd want to opt out
> of a standard readq/writeq (it's not in-tree because it never shipped,
> unfortunately.)  Doing the opt-in headers seems like a reasonable thing
> to do to me, but perhaps I'm just being overly paranoid.

Isn't that a variant of "punish all sane hardware, because bizarre 
unshipped hardware exists"?

IMO the best fix is to document existing readq assumptions, and 
standardize that definition on other platforms.

The burden of special casing for bizarre hardware should not fall on 
/sane/ drivers and hardware, who should be the ones opting _out_ of the 
standard regime.

	Jeff

Re: [PATCH] x86: Remove readq()/writeq() on 32-bit

[Jeff Garzik]

Jeff Garzik wrote:
> The burden of special casing for bizarre hardware should not fall on 
> /sane/ drivers and hardware, who should be the ones opting _out_ of the 
> standard regime.

opting out refers to the bizarre hardware, of course.

Poor sentence construction.  Sometimes I myself wonder if English is my 
primary language...

	Jeff

Re: [PATCH] x86: Remove readq()/writeq() on 32-bit

[H. Peter Anvin]

Jeff Garzik wrote:
>>>
>> I have personally dealt with at least one device who'd want to opt out
>> of a standard readq/writeq (it's not in-tree because it never shipped,
>> unfortunately.)  Doing the opt-in headers seems like a reasonable thing
>> to do to me, but perhaps I'm just being overly paranoid.
> 
> Isn't that a variant of "punish all sane hardware, because bizarre 
> unshipped hardware exists"?
> 
> IMO the best fix is to document existing readq assumptions, and 
> standardize that definition on other platforms.
> 
> The burden of special casing for bizarre hardware should not fall on 
> /sane/ drivers and hardware, who should be the ones opting _out_ of the 
> standard regime.
> 

It sort of is, but it's also a case of "explicitly documenting your
assumptions".  You have do admit that having to #include a single extra
header file is hardly a hardship.

	-hpa

Re: [PATCH] x86: Remove readq()/writeq() on 32-bit

[Roland Dreier]

 > To repeat what has already been stated, each case was re-evaluated:
 > http://marc.info/?l=linux-kernel&m=124103527326835&w=2
 > 
 > Roland's patch was acked, apparently, _in spite of_ the commonly
 > accepted readq() definition already being in use!
 > 
 > Thusfar, I see two things:
 > 
 > (1) years of history has shown that non-atomic readq/writeq on 32-bit
 > platforms has been sufficient, based on testing and experience.  In
 > fact, in niu's case, a common readq/writeq would have PREVENTED a bug.

But the fact that the 32-bit x86 define would have worked for niu is
pure luck -- if the clear-on-read bits had been in the other half of the
register in question, then it would have caused a bug.  And I really
don't trust all ASIC designers writing RTL to think about which half of
a 64-bit register is going to be read first.

To me, the point is that the current situation of having the defines for
32-bit x86 has zero benefit -- not one driver-specific definition can be
removed, because there are other 32-bit architectures to worry about.
So we just added another copy of the compatibility wrapper, in a not
particularly good location -- we certainly don't want to have the same
defines copied into every 32-bit architecture's <asm/io.h> header.

And the risk introduced is not zero -- very few devices have 64-bit
registers and very few drivers use readq or writeq, but perhaps as
end-to-end 64-bit buses become more prevalent with PCIe, we may see
more.  And it's certainly the case that emulation 64-bit register
operations by doing to 32-bit operations on the register halves carries
a non-zero risk of making the hardware do something wacky.

So to me the it's pretty clear: the current situation has benefit == 0
&& risk > 0, so we should revert to the previous situation until someone
implements something more complete like hpa's opt-in header scheme.

 - R.

Re: [PATCH] x86: Remove readq()/writeq() on 32-bit

[Jeff Garzik]

Roland Dreier wrote:
>  > To repeat what has already been stated, each case was re-evaluated:
>  > http://marc.info/?l=linux-kernel&m=124103527326835&w=2
>  > 
>  > Roland's patch was acked, apparently, _in spite of_ the commonly
>  > accepted readq() definition already being in use!
>  > 
>  > Thusfar, I see two things:
>  > 
>  > (1) years of history has shown that non-atomic readq/writeq on 32-bit
>  > platforms has been sufficient, based on testing and experience.  In
>  > fact, in niu's case, a common readq/writeq would have PREVENTED a bug.
> 
> But the fact that the 32-bit x86 define would have worked for niu is
> pure luck -- if the clear-on-read bits had been in the other half of the
> register in question, then it would have caused a bug.  And I really
> don't trust all ASIC designers writing RTL to think about which half of
> a 64-bit register is going to be read first.

AFAICS things have unerringly occurred in PCI ordering, which is what 
one would expect.

What you call pure luck, others call 100% track record.


> To me, the point is that the current situation of having the defines for
> 32-bit x86 has zero benefit -- not one driver-specific definition can be
> removed, because there are other 32-bit architectures to worry about.

Um, this is precisely what Mitake-san is trying to address, hence the 
discussion...


> And the risk introduced is not zero -- very few devices have 64-bit
> registers and very few drivers use readq or writeq, but perhaps as
> end-to-end 64-bit buses become more prevalent with PCIe, we may see
> more.  And it's certainly the case that emulation 64-bit register
> operations by doing to 32-bit operations on the register halves carries
> a non-zero risk of making the hardware do something wacky.

Again, fear vs. reality, 0% case versus 100% case.

You continue to lack CONCRETE examples of problems, while existing cases 
CONTINUE to work with the obvious ordering.

	Jeff

Re: [PATCH] x86: Remove readq()/writeq() on 32-bit

[Roland Dreier]

 > What caused 2c5643b1 was that right now we have ugly per driver
 > #defines and inlines for readq/writeq. See:

but 2c5643b1 doesn't fix that situation -- a portable driver still needs
the #defines for other 32-bit architectures.  And 2c5643b1 isn't really
a particularly good step towards rectifying the situation, since if
every 32-bit architecture follows suit and adds its own compatibility
versions, then we'll want someone to go through and unify them into a
generic implementation.  In other words removing the x86 private version
will be part of the work in getting to a final solution.

 > Atomicity of a 64-bit IO space access on 32-bit platforms, on an 
 > unknown-bitness transport (it might even be a 64-bit PCI device 
 > bridged over a 32-bit bridge) is obviously not guaranteed.

Yes, some platforms may not be able to give true atomicity.  eg 32-bit
PowerPC has no instructions that generate 64-bit cycles, even on the CPU
bus.  I do think the 32-bit PCI thing is a bit of a red herring, since
eg PCIe devices can rely on a 64-bit bus.

 > So trying to suggest that 64-bit readq/writeq when running on a 
 > 64-bit kernel is somehow atomic (or can be made atomic) is really 
 > wrong IMO. The 32-bit wrapper is simply the expression of how the 
 > CPU would do a 64-bit access even in 64-bit mode anyway [if the 
 > transport is 32-bit].

As far as I know, all 64-bit CPUs doing 64-bit accesses to a PCIe device
(eg the NIC driven by the niu driver) will generate 64-bit bus cycles.

The issue for me is that the benefit of having this compatibility define
is rather minimal, while the cost is potentially high: spending a long
time debugging platform-specific bugs -- the symptoms will not point
immediately to the IO define, of course.

 - R.

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

[David Miller]

From: "H. Peter Anvin" <hpa@zytor.com>
Date: Tue, 21 Apr 2009 12:09:20 -0700

> Do you really expect driver authors to type writeq_nonatomic() for
> every register reference?

They'll write local driver macros, as every driver does
to save typing.  It also allows to get rid of the
redundant "foop->regs" in every register access too.

Look at what all of these drivers do:

#define nr64(reg)		readq(np->regs + (reg))
#define nw64(reg, val)		writeq((val), np->regs + (reg))

So nobody actually types readq() for every register access,
just as they don't type foop->regs for every register access
either :-)

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

[David Miller]

From: Roland Dreier <rdreier@cisco.com>
Date: Tue, 21 Apr 2009 10:07:50 -0700

> This only makes sense if we define a 32-bit fallback for
> readq()/writeq() for all 32-bit architectures -- in fact it would be
> good to do it in asm-generic so that there can be a single
> implementation that guarantees that non-atomic versions always do, say,
> low 32 bits then high 32 bits.  (So eg niu can use the generic version)
> And then drivers like drivers/infiniband/hw/mthca can be switched to
> "#ifndef writeq_atomic <...hardware specific fallback...>"

I think if you want to do this right you have to provide two versions
of the 32-bit implementations, when the cpu cannot generate full
64-bit transactions.  Especially for readq().

Some devices clear the status bits of a 64-bit register when read, so
it might matter deeply whether the top-half or the bottom-half 32-bits
are read first.

The following are ugly names, but something like "readq_hifirst()"
and "readq_lofirst()" and they just get defined to play "readq()"
in situations where a full 64-bit transaction can be generated by
the cpu.

The driver author has to figure out which is appropriate.

And I'm pretty sure similar high-first/low-first issues can exist
for writeq() as well.