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

[Hitoshi Mitake <mitake@dcl.info.waseda.ac.jp>]

On Thu, 21 May 2009 20:35:54 +0900
Hitoshi Mitake <h.mitake@gmail.com> wrote:

> On Sun, May 17, 2009 at 17:06, Jeff Garzik <jeff@garzik.org> wrote:
> > Hitoshi Mitake wrote:
> >>
> >> 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
> >
> >
> > 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
> >
> >
> >
> 

I fixed readq according to Jeff's advice. I think his readq is smarter than mine.
This is new version of the patch.

So I want to hear Roland and David's opinion.
We will be able to avoid making terrible bugs with this patch.
And generic readq/writeq will be provided for the cases
without requirement of atomic readq/writeq.

This patch would make our life easily. How do you think?

Signed-off-by: Hitoshi Mitake <mitake@dcl.info.waseda.ac.jp>
Cc: Jeff Garzik <jeff@garzik.org>
Cc: Roland Dreier <rolandd@cisco.com>
Cc: David S. Miller <davem@davemloft.net>

---
 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 a6efe0a..46ea47c 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
@@ -2035,6 +2033,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..15856ac
--- /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)
+{
+	__u64 low, high;
+
+	low = readl(addr);
+	high = readl(addr + 4);
+
+	return (high << 32) | low;
+}
+
+#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