Re: [PATCH] x86: Remove readq()/writeq() on 32-bit
From: Hitoshi Mitake
Date: Thu May 21 2009 - 08:00:54 EST
On Thu, 21 May 2009 20:35:54 +0900
Hitoshi Mitake <h.mitake@xxxxxxxxx> wrote:
> On Sun, May 17, 2009 at 17:06, Jeff Garzik <jeff@xxxxxxxxxx> wrote:
> > 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
> >
> >
> >
>
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@xxxxxxxxxxxxxxxxxxxxx>
Cc: Jeff Garzik <jeff@xxxxxxxxxx>
Cc: Roland Dreier <rolandd@xxxxxxxxx>
Cc: David S. Miller <davem@xxxxxxxxxxxxx>
---
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
--
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/