Re: [PATCH] x86: Remove readq()/writeq() on 32-bit
From: Jeff Garzik
Date: Sun May 17 2009 - 04:08:06 EST
Hitoshi Mitake wrote:
On Fri, 15 May 2009 19:44:03 -0400
Jeff Garzik <jeff@xxxxxxxxxx> wrote:
Hitoshi Mitake wrote:
On Wed, 13 May 2009 20:49:26 -0400
Jeff Garzik <jeff@xxxxxxxxxx> 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@xxxxxxxxx>
---
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@xxxxxxxxx>
---
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
--
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/