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

[Jeff Garzik <jeff@garzik.org>]

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