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

[Jeff Garzik]

Ingo Molnar wrote:
> (Linus Cc:-ed)
>
> * David Miller <davem@xxxxxxxxxxxxx> wrote:
>
> > From: Roland Dreier <rdreier@xxxxxxxxx>
> > 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@xxxxxxxxx>
> > Acked-by: David S. Miller <davem@xxxxxxxxxxxxx>
> [...]
> > > 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



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/