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

[Hitoshi Mitake <h.mitake@gmail.com>]

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?