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

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

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