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

From: Hitoshi Mitake
Date: Sun May 17 2009 - 03:19:32 EST

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

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


--
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/