From: Hitoshi Mitake <[email protected]>
This patch removes some readq()s in drivers (added in dbee8a0affd5e6eaa5d7c816)
and provides unified readq()/writeq() for the drivers.
For some people, readq/writeq without atomicity is harmful, and order of io
access has to be specified explicitly. So in this patch, new two header files
which contain non-atomic readq/writeq are added. io-64-nonatomic-lo-hi.h
provides non-atomic readq/writeq with the order of lower address -> higher
address. io-64-nonatomic-hi-lo.h provides non-atomic readq/writeq with reversed
order. All of them are endian awared.
If this patch is applied, the drivers which need readq/writeq must add the line:
#include <asm-generic/io-64-nonatomic-lo-hi.h> /* or hi-lo.h */
But this will be nop in 64-bit environments, and no other #ifdefs are required.
So I believe that this patch can solve the problem of
1. driver-specific readq/writeq
2. atomicity and order of io access
This patch is tested with building allyesconfig and allmodconfig as ARCH=x86 and
ARCH=i386 on top of tip/master.
Cc: Kashyap Desai <[email protected]>
Cc: Len Brown <[email protected]>
Cc: Ravi Anand <[email protected]>
Cc: Vikas Chaudhary <[email protected]>
Cc: Matthew Garrett <[email protected]>
Cc: Jason Uhlenkott <[email protected]>
Cc: James Bottomley <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: "H. Peter Anvin" <[email protected]>
Cc: Roland Dreier <[email protected]>
Cc: James Bottomley <[email protected]>
Cc: Alan Cox <[email protected]>
Cc: Matthew Wilcox <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: Linus Torvalds <[email protected]>
Signed-off-by: Hitoshi Mitake <[email protected]>
---
drivers/edac/i3200_edac.c | 15 +------
drivers/platform/x86/ibm_rtl.c | 15 +------
drivers/platform/x86/intel_ips.c | 15 +------
drivers/scsi/qla4xxx/ql4_nx.c | 23 +---------
include/asm-generic/io-64-nonatomic-hi-lo.h | 63 +++++++++++++++++++++++++++
include/asm-generic/io-64-nonatomic-lo-hi.h | 63 +++++++++++++++++++++++++++
6 files changed, 134 insertions(+), 60 deletions(-)
create mode 100644 include/asm-generic/io-64-nonatomic-hi-lo.h
create mode 100644 include/asm-generic/io-64-nonatomic-lo-hi.h
diff --git a/drivers/edac/i3200_edac.c b/drivers/edac/i3200_edac.c
index aa08497..73f55e200 100644
--- a/drivers/edac/i3200_edac.c
+++ b/drivers/edac/i3200_edac.c
@@ -15,6 +15,8 @@
#include <linux/io.h>
#include "edac_core.h"
+#include <asm-generic/io-64-nonatomic-lo-hi.h>
+
#define I3200_REVISION "1.1"
#define EDAC_MOD_STR "i3200_edac"
@@ -101,19 +103,6 @@ struct i3200_priv {
static int nr_channels;
-#ifndef readq
-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
-
static int how_many_channels(struct pci_dev *pdev)
{
unsigned char capid0_8b; /* 8th byte of CAPID0 */
diff --git a/drivers/platform/x86/ibm_rtl.c b/drivers/platform/x86/ibm_rtl.c
index 42a7d60..7481146 100644
--- a/drivers/platform/x86/ibm_rtl.c
+++ b/drivers/platform/x86/ibm_rtl.c
@@ -33,6 +33,8 @@
#include <linux/mutex.h>
#include <asm/bios_ebda.h>
+#include <asm-generic/io-64-nonatomic-lo-hi.h>
+
static bool force;
module_param(force, bool, 0);
MODULE_PARM_DESC(force, "Force driver load, ignore DMI data");
@@ -83,19 +85,6 @@ static void __iomem *rtl_cmd_addr;
static u8 rtl_cmd_type;
static u8 rtl_cmd_width;
-#ifndef readq
-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
-
static void __iomem *rtl_port_map(phys_addr_t addr, unsigned long len)
{
if (rtl_cmd_type == RTL_ADDR_TYPE_MMIO)
diff --git a/drivers/platform/x86/intel_ips.c b/drivers/platform/x86/intel_ips.c
index 809a3ae..88a98cf 100644
--- a/drivers/platform/x86/intel_ips.c
+++ b/drivers/platform/x86/intel_ips.c
@@ -77,6 +77,8 @@
#include <asm/processor.h>
#include "intel_ips.h"
+#include <asm-generic/io-64-nonatomic-lo-hi.h>
+
#define PCI_DEVICE_ID_INTEL_THERMAL_SENSOR 0x3b32
/*
@@ -344,19 +346,6 @@ struct ips_driver {
static bool
ips_gpu_turbo_enabled(struct ips_driver *ips);
-#ifndef readq
-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
-
/**
* ips_cpu_busy - is CPU busy?
* @ips: IPS driver struct
diff --git a/drivers/scsi/qla4xxx/ql4_nx.c b/drivers/scsi/qla4xxx/ql4_nx.c
index 78f1111..65253df 100644
--- a/drivers/scsi/qla4xxx/ql4_nx.c
+++ b/drivers/scsi/qla4xxx/ql4_nx.c
@@ -10,6 +10,8 @@
#include "ql4_def.h"
#include "ql4_glbl.h"
+#include <asm-generic/io-64-nonatomic-lo-hi.h>
+
#define MASK(n) DMA_BIT_MASK(n)
#define MN_WIN(addr) (((addr & 0x1fc0000) >> 1) | ((addr >> 25) & 0x3ff))
#define OCM_WIN(addr) (((addr & 0x1ff0000) >> 1) | ((addr >> 25) & 0x3ff))
@@ -655,27 +657,6 @@ static int qla4_8xxx_pci_is_same_window(struct scsi_qla_host *ha,
return 0;
}
-#ifndef readq
-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
-
-#ifndef writeq
-static inline void writeq(__u64 val, volatile void __iomem *addr)
-{
- writel(val, addr);
- writel(val >> 32, addr+4);
-}
-#endif
-
static int qla4_8xxx_pci_mem_read_direct(struct scsi_qla_host *ha,
u64 off, void *data, int size)
{
diff --git a/include/asm-generic/io-64-nonatomic-hi-lo.h b/include/asm-generic/io-64-nonatomic-hi-lo.h
new file mode 100644
index 0000000..34e61cc
--- /dev/null
+++ b/include/asm-generic/io-64-nonatomic-hi-lo.h
@@ -0,0 +1,63 @@
+#ifndef _ASM_IO_64_NONATOMIC_HI_LO_H_
+#define _ASM_IO_64_NONATOMIC_HI_LO_H_
+
+#include <linux/io.h>
+#include <asm-generic/int-ll64.h>
+
+#ifdef CPU_LITTLE_ENDIAN
+
+#ifndef readq
+
+static inline __u64 readq(const volatile void __iomem *addr)
+{
+ const volatile u32 __iomem *p = addr;
+ u32 low, high;
+
+ high = readl(p + 1);
+ low = readl(p);
+
+ return low + ((u64)high << 32);
+}
+
+#endif
+
+#ifndef writeq
+
+static inline void writeq(__u64 val, volatile void __iomem *addr)
+{
+ writel(val >> 32, addr + 4);
+ writel(val, addr);
+}
+
+#endif
+
+#else /* big endian */
+
+#ifndef readq
+
+static inline __u64 readq(const volatile void __iomem *addr)
+{
+ const volatile u32 __iomem *p = addr;
+ u32 low, high;
+
+ low = readl(p + 1);
+ high = readl(p);
+
+ return low + ((u64)high << 32);
+}
+
+#endif
+
+#ifndef writeq
+
+static inline void writeq(__u64 val, volatile void __iomem *addr)
+{
+ writel(val >> 32, addr);
+ writel(val, addr + 4);
+}
+
+#endif
+
+#endif
+
+#endif /* _ASM_IO_64_NONATOMIC_HI_LO_H_ */
diff --git a/include/asm-generic/io-64-nonatomic-lo-hi.h b/include/asm-generic/io-64-nonatomic-lo-hi.h
new file mode 100644
index 0000000..bf2fe0f
--- /dev/null
+++ b/include/asm-generic/io-64-nonatomic-lo-hi.h
@@ -0,0 +1,63 @@
+#ifndef _ASM_IO_64_NONATOMIC_LO_HI_H_
+#define _ASM_IO_64_NONATOMIC_LO_HI_H_
+
+#include <linux/io.h>
+#include <asm-generic/int-ll64.h>
+
+#ifdef CPU_LITTLE_ENDIAN
+
+#ifndef readq
+
+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
+
+#ifndef writeq
+
+static inline void writeq(__u64 val, volatile void __iomem *addr)
+{
+ writel(val, addr);
+ writel(val >> 32, addr + 4);
+}
+
+#endif
+
+#else /* big endian */
+
+#ifndef readq
+
+static inline __u64 readq(const volatile void __iomem *addr)
+{
+ const volatile u32 __iomem *p = addr;
+ u32 low, high;
+
+ high = readl(p);
+ low = readl(p + 1);
+
+ return low + ((u64)high << 32);
+}
+
+#endif
+
+#ifndef writeq
+
+static inline void writeq(__u64 val, volatile void __iomem *addr)
+{
+ writel(val, addr + 4);
+ writel(val >> 32, addr);
+}
+
+#endif
+
+#endif
+
+#endif /* _ASM_IO_64_NONATOMIC_LO_HI_H_ */
--
1.7.5.1
Should be volatile u64 * not volatile void *...
Hitoshi Mitake <[email protected]> wrote:
>From: Hitoshi Mitake <[email protected]>
>
>This patch removes some readq()s in drivers (added in
>dbee8a0affd5e6eaa5d7c816)
>and provides unified readq()/writeq() for the drivers.
>
>For some people, readq/writeq without atomicity is harmful, and order
>of io
>access has to be specified explicitly. So in this patch, new two header
>files
>which contain non-atomic readq/writeq are added.
>io-64-nonatomic-lo-hi.h
>provides non-atomic readq/writeq with the order of lower address ->
>higher
>address. io-64-nonatomic-hi-lo.h provides non-atomic readq/writeq with
>reversed
>order. All of them are endian awared.
>
>If this patch is applied, the drivers which need readq/writeq must add
>the line:
>#include <asm-generic/io-64-nonatomic-lo-hi.h> /* or hi-lo.h */
>
>But this will be nop in 64-bit environments, and no other #ifdefs are
>required.
>So I believe that this patch can solve the problem of
>1. driver-specific readq/writeq
>2. atomicity and order of io access
>
>This patch is tested with building allyesconfig and allmodconfig as
>ARCH=x86 and
>ARCH=i386 on top of tip/master.
>
>Cc: Kashyap Desai <[email protected]>
>Cc: Len Brown <[email protected]>
>Cc: Ravi Anand <[email protected]>
>Cc: Vikas Chaudhary <[email protected]>
>Cc: Matthew Garrett <[email protected]>
>Cc: Jason Uhlenkott <[email protected]>
>Cc: James Bottomley <[email protected]>
>Cc: Thomas Gleixner <[email protected]>
>Cc: "H. Peter Anvin" <[email protected]>
>Cc: Roland Dreier <[email protected]>
>Cc: James Bottomley <[email protected]>
>Cc: Alan Cox <[email protected]>
>Cc: Matthew Wilcox <[email protected]>
>Cc: Andrew Morton <[email protected]>
>Cc: Linus Torvalds <[email protected]>
>Signed-off-by: Hitoshi Mitake <[email protected]>
>
>---
> drivers/edac/i3200_edac.c | 15 +------
> drivers/platform/x86/ibm_rtl.c | 15 +------
> drivers/platform/x86/intel_ips.c | 15 +------
> drivers/scsi/qla4xxx/ql4_nx.c | 23 +---------
>include/asm-generic/io-64-nonatomic-hi-lo.h | 63
>+++++++++++++++++++++++++++
>include/asm-generic/io-64-nonatomic-lo-hi.h | 63
>+++++++++++++++++++++++++++
> 6 files changed, 134 insertions(+), 60 deletions(-)
> create mode 100644 include/asm-generic/io-64-nonatomic-hi-lo.h
> create mode 100644 include/asm-generic/io-64-nonatomic-lo-hi.h
>
>diff --git a/drivers/edac/i3200_edac.c b/drivers/edac/i3200_edac.c
>index aa08497..73f55e200 100644
>--- a/drivers/edac/i3200_edac.c
>+++ b/drivers/edac/i3200_edac.c
>@@ -15,6 +15,8 @@
> #include <linux/io.h>
> #include "edac_core.h"
>
>+#include <asm-generic/io-64-nonatomic-lo-hi.h>
>+
> #define I3200_REVISION "1.1"
>
> #define EDAC_MOD_STR "i3200_edac"
>@@ -101,19 +103,6 @@ struct i3200_priv {
>
> static int nr_channels;
>
>-#ifndef readq
>-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
>-
> static int how_many_channels(struct pci_dev *pdev)
> {
> unsigned char capid0_8b; /* 8th byte of CAPID0 */
>diff --git a/drivers/platform/x86/ibm_rtl.c
>b/drivers/platform/x86/ibm_rtl.c
>index 42a7d60..7481146 100644
>--- a/drivers/platform/x86/ibm_rtl.c
>+++ b/drivers/platform/x86/ibm_rtl.c
>@@ -33,6 +33,8 @@
> #include <linux/mutex.h>
> #include <asm/bios_ebda.h>
>
>+#include <asm-generic/io-64-nonatomic-lo-hi.h>
>+
> static bool force;
> module_param(force, bool, 0);
> MODULE_PARM_DESC(force, "Force driver load, ignore DMI data");
>@@ -83,19 +85,6 @@ static void __iomem *rtl_cmd_addr;
> static u8 rtl_cmd_type;
> static u8 rtl_cmd_width;
>
>-#ifndef readq
>-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
>-
> static void __iomem *rtl_port_map(phys_addr_t addr, unsigned long len)
> {
> if (rtl_cmd_type == RTL_ADDR_TYPE_MMIO)
>diff --git a/drivers/platform/x86/intel_ips.c
>b/drivers/platform/x86/intel_ips.c
>index 809a3ae..88a98cf 100644
>--- a/drivers/platform/x86/intel_ips.c
>+++ b/drivers/platform/x86/intel_ips.c
>@@ -77,6 +77,8 @@
> #include <asm/processor.h>
> #include "intel_ips.h"
>
>+#include <asm-generic/io-64-nonatomic-lo-hi.h>
>+
> #define PCI_DEVICE_ID_INTEL_THERMAL_SENSOR 0x3b32
>
> /*
>@@ -344,19 +346,6 @@ struct ips_driver {
> static bool
> ips_gpu_turbo_enabled(struct ips_driver *ips);
>
>-#ifndef readq
>-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
>-
> /**
> * ips_cpu_busy - is CPU busy?
> * @ips: IPS driver struct
>diff --git a/drivers/scsi/qla4xxx/ql4_nx.c
>b/drivers/scsi/qla4xxx/ql4_nx.c
>index 78f1111..65253df 100644
>--- a/drivers/scsi/qla4xxx/ql4_nx.c
>+++ b/drivers/scsi/qla4xxx/ql4_nx.c
>@@ -10,6 +10,8 @@
> #include "ql4_def.h"
> #include "ql4_glbl.h"
>
>+#include <asm-generic/io-64-nonatomic-lo-hi.h>
>+
> #define MASK(n) DMA_BIT_MASK(n)
>#define MN_WIN(addr) (((addr & 0x1fc0000) >> 1) | ((addr >> 25) &
>0x3ff))
>#define OCM_WIN(addr) (((addr & 0x1ff0000) >> 1) | ((addr >> 25) &
>0x3ff))
>@@ -655,27 +657,6 @@ static int qla4_8xxx_pci_is_same_window(struct
>scsi_qla_host *ha,
> return 0;
> }
>
>-#ifndef readq
>-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
>-
>-#ifndef writeq
>-static inline void writeq(__u64 val, volatile void __iomem *addr)
>-{
>- writel(val, addr);
>- writel(val >> 32, addr+4);
>-}
>-#endif
>-
> static int qla4_8xxx_pci_mem_read_direct(struct scsi_qla_host *ha,
> u64 off, void *data, int size)
> {
>diff --git a/include/asm-generic/io-64-nonatomic-hi-lo.h
>b/include/asm-generic/io-64-nonatomic-hi-lo.h
>new file mode 100644
>index 0000000..34e61cc
>--- /dev/null
>+++ b/include/asm-generic/io-64-nonatomic-hi-lo.h
>@@ -0,0 +1,63 @@
>+#ifndef _ASM_IO_64_NONATOMIC_HI_LO_H_
>+#define _ASM_IO_64_NONATOMIC_HI_LO_H_
>+
>+#include <linux/io.h>
>+#include <asm-generic/int-ll64.h>
>+
>+#ifdef CPU_LITTLE_ENDIAN
>+
>+#ifndef readq
>+
>+static inline __u64 readq(const volatile void __iomem *addr)
>+{
>+ const volatile u32 __iomem *p = addr;
>+ u32 low, high;
>+
>+ high = readl(p + 1);
>+ low = readl(p);
>+
>+ return low + ((u64)high << 32);
>+}
>+
>+#endif
>+
>+#ifndef writeq
>+
>+static inline void writeq(__u64 val, volatile void __iomem *addr)
>+{
>+ writel(val >> 32, addr + 4);
>+ writel(val, addr);
>+}
>+
>+#endif
>+
>+#else /* big endian */
>+
>+#ifndef readq
>+
>+static inline __u64 readq(const volatile void __iomem *addr)
>+{
>+ const volatile u32 __iomem *p = addr;
>+ u32 low, high;
>+
>+ low = readl(p + 1);
>+ high = readl(p);
>+
>+ return low + ((u64)high << 32);
>+}
>+
>+#endif
>+
>+#ifndef writeq
>+
>+static inline void writeq(__u64 val, volatile void __iomem *addr)
>+{
>+ writel(val >> 32, addr);
>+ writel(val, addr + 4);
>+}
>+
>+#endif
>+
>+#endif
>+
>+#endif /* _ASM_IO_64_NONATOMIC_HI_LO_H_ */
>diff --git a/include/asm-generic/io-64-nonatomic-lo-hi.h
>b/include/asm-generic/io-64-nonatomic-lo-hi.h
>new file mode 100644
>index 0000000..bf2fe0f
>--- /dev/null
>+++ b/include/asm-generic/io-64-nonatomic-lo-hi.h
>@@ -0,0 +1,63 @@
>+#ifndef _ASM_IO_64_NONATOMIC_LO_HI_H_
>+#define _ASM_IO_64_NONATOMIC_LO_HI_H_
>+
>+#include <linux/io.h>
>+#include <asm-generic/int-ll64.h>
>+
>+#ifdef CPU_LITTLE_ENDIAN
>+
>+#ifndef readq
>+
>+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
>+
>+#ifndef writeq
>+
>+static inline void writeq(__u64 val, volatile void __iomem *addr)
>+{
>+ writel(val, addr);
>+ writel(val >> 32, addr + 4);
>+}
>+
>+#endif
>+
>+#else /* big endian */
>+
>+#ifndef readq
>+
>+static inline __u64 readq(const volatile void __iomem *addr)
>+{
>+ const volatile u32 __iomem *p = addr;
>+ u32 low, high;
>+
>+ high = readl(p);
>+ low = readl(p + 1);
>+
>+ return low + ((u64)high << 32);
>+}
>+
>+#endif
>+
>+#ifndef writeq
>+
>+static inline void writeq(__u64 val, volatile void __iomem *addr)
>+{
>+ writel(val, addr + 4);
>+ writel(val >> 32, addr);
>+}
>+
>+#endif
>+
>+#endif
>+
>+#endif /* _ASM_IO_64_NONATOMIC_LO_HI_H_ */
>--
>1.7.5.1
--
Sent from my Android phone with K-9 Mail. Please excuse my brevity.
On Tue, Feb 7, 2012 at 12:56, Linus Torvalds
<[email protected]> wrote:
>
> On Feb 6, 2012 6:47 PM, "Hitoshi Mitake" <[email protected]> wrote:
>>
>>? All of them are endian aware
>
> I think that part is wrong.
>
> Pci is little-endian, so readl and writel are always already little-endian.
> Trying to make readq be endian-aware is wrong.
Is every memory area which can be read/written by read[wl]/write[wl]
always little-endian?
If so, of course I have to eliminate the big-endian version.
But I'm not sure about this point because the new readq/writeq is
in asm-generic which is used by every archs, so I'd like to ask you about it.
>
> ????????????????????????? Linus
>
> On Feb 6, 2012 6:47 PM, "Hitoshi Mitake" <[email protected]> wrote:
--
Hitoshi Mitake
[email protected]
On Wed, Feb 8, 2012 at 8:14 AM, Hitoshi Mitake <[email protected]> wrote:
> On Tue, Feb 7, 2012 at 12:56, Linus Torvalds
> <[email protected]> wrote:
>>
>> On Feb 6, 2012 6:47 PM, "Hitoshi Mitake" <[email protected]> wrote:
>>>
>>>? All of them are endian aware
>>
>> I think that part is wrong.
>>
>> Pci is little-endian, so readl and writel are always already little-endian.
>> Trying to make readq be endian-aware is wrong.
>
> Is every memory area which can be read/written by read[wl]/write[wl]
> always little-endian?
>
> If so, of course I have to eliminate the big-endian version.
> But I'm not sure about this point because the new readq/writeq is
> in asm-generic which is used by every archs, so I'd like to ask you about it.
Yes, PCI is always little-endian everywhere.
Now, some architectures may have some unified IO space where parts of
it is PCI, and other parts are programmed the same way but is for some
other bus, but the PCI part will always be little-endian because
otherwise no common driver would ever work. But those non-PCI things
would never be relevant for an emulated readq/writeq anyway, and would
need some bus-specific accessor functions.
Linus
On Wed, Feb 8, 2012 at 17:28, Linus Torvalds
<[email protected]> wrote:
> On Wed, Feb 8, 2012 at 8:14 AM, Hitoshi Mitake <[email protected]> wrote:
>> On Tue, Feb 7, 2012 at 12:56, Linus Torvalds
>> <[email protected]> wrote:
>>>
>>> On Feb 6, 2012 6:47 PM, "Hitoshi Mitake" <[email protected]> wrote:
>>>>
>>>> All of them are endian aware
>>>
>>> I think that part is wrong.
>>>
>>> Pci is little-endian, so readl and writel are always already little-endian.
>>> Trying to make readq be endian-aware is wrong.
>>
>> Is every memory area which can be read/written by read[wl]/write[wl]
>> always little-endian?
>>
>> If so, of course I have to eliminate the big-endian version.
>> But I'm not sure about this point because the new readq/writeq is
>> in asm-generic which is used by every archs, so I'd like to ask you about it.
>
> Yes, PCI is always little-endian everywhere.
>
> Now, some architectures may have some unified IO space where parts of
> it is PCI, and other parts are programmed the same way but is for some
> other bus, but the PCI part will always be little-endian because
> otherwise no common driver would ever work. But those non-PCI things
> would never be relevant for an emulated readq/writeq anyway, and would
> need some bus-specific accessor functions.
IIRC, some crazy hardware manufacturers did swizzle PCI busses in hardware.
So doing a PCI "little endian" 32-bit read must be performed using a native
(big endian) 32-bit read. And it causes more headaches for 16-bit accesses.
Of course this must all be hidden in your readX() implementations, to make the
common drivers work.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On Tue, Feb 7, 2012 at 12:47, [email protected] <[email protected]> wrote:
> Should be volatile u64 * not volatile void *...
Is this the type of parameters?
The parameters of atomic readq/writeq defined in arch/x86/include/asm/io.h
are defined as void *. I think that atomic readq/writeq and non-atomic
readq/writeq
should have same typed parameters and return values.
>
>
>
> Hitoshi Mitake <[email protected]> wrote:
>
>>From: Hitoshi Mitake <[email protected]>
>>
>>This patch removes some readq()s in drivers (added in
>>dbee8a0affd5e6eaa5d7c816)
>>and provides unified readq()/writeq() for the drivers.
>>
>>For some people, readq/writeq without atomicity is harmful, and order
>>of io
>>access has to be specified explicitly. So in this patch, new two header
>>files
>>which contain non-atomic readq/writeq are added.
>>io-64-nonatomic-lo-hi.h
>>provides non-atomic readq/writeq with the order of lower address ->
>>higher
>>address. io-64-nonatomic-hi-lo.h provides non-atomic readq/writeq with
>>reversed
>>order. All of them are endian awared.
>>
>>If this patch is applied, the drivers which need readq/writeq must add
>>the line:
>>#include <asm-generic/io-64-nonatomic-lo-hi.h> /* or hi-lo.h */
>>
>>But this will be nop in 64-bit environments, and no other #ifdefs are
>>required.
>>So I believe that this patch can solve the problem of
>>1. driver-specific readq/writeq
>>2. atomicity and order of io access
>>
>>This patch is tested with building allyesconfig and allmodconfig as
>>ARCH=x86 and
>>ARCH=i386 on top of tip/master.
>>
>>Cc: Kashyap Desai <[email protected]>
>>Cc: Len Brown <[email protected]>
>>Cc: Ravi Anand <[email protected]>
>>Cc: Vikas Chaudhary <[email protected]>
>>Cc: Matthew Garrett <[email protected]>
>>Cc: Jason Uhlenkott <[email protected]>
>>Cc: James Bottomley <[email protected]>
>>Cc: Thomas Gleixner <[email protected]>
>>Cc: "H. Peter Anvin" <[email protected]>
>>Cc: Roland Dreier <[email protected]>
>>Cc: James Bottomley <[email protected]>
>>Cc: Alan Cox <[email protected]>
>>Cc: Matthew Wilcox <[email protected]>
>>Cc: Andrew Morton <[email protected]>
>>Cc: Linus Torvalds <[email protected]>
>>Signed-off-by: Hitoshi Mitake <[email protected]>
>>
>>---
>> drivers/edac/i3200_edac.c ? ? ? ? ? ? ? ? ? | ? 15 +------
>> drivers/platform/x86/ibm_rtl.c ? ? ? ? ? ? ?| ? 15 +------
>> drivers/platform/x86/intel_ips.c ? ? ? ? ? ?| ? 15 +------
>> drivers/scsi/qla4xxx/ql4_nx.c ? ? ? ? ? ? ? | ? 23 +---------
>>include/asm-generic/io-64-nonatomic-hi-lo.h | ? 63
>>+++++++++++++++++++++++++++
>>include/asm-generic/io-64-nonatomic-lo-hi.h | ? 63
>>+++++++++++++++++++++++++++
>> 6 files changed, 134 insertions(+), 60 deletions(-)
>> create mode 100644 include/asm-generic/io-64-nonatomic-hi-lo.h
>> create mode 100644 include/asm-generic/io-64-nonatomic-lo-hi.h
>>
>>diff --git a/drivers/edac/i3200_edac.c b/drivers/edac/i3200_edac.c
>>index aa08497..73f55e200 100644
>>--- a/drivers/edac/i3200_edac.c
>>+++ b/drivers/edac/i3200_edac.c
>>@@ -15,6 +15,8 @@
>> #include <linux/io.h>
>> #include "edac_core.h"
>>
>>+#include <asm-generic/io-64-nonatomic-lo-hi.h>
>>+
>> #define I3200_REVISION ? ? ? ?"1.1"
>>
>> #define EDAC_MOD_STR ? ? ? ?"i3200_edac"
>>@@ -101,19 +103,6 @@ struct i3200_priv {
>>
>> static int nr_channels;
>>
>>-#ifndef readq
>>-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
>>-
>> static int how_many_channels(struct pci_dev *pdev)
>> {
>> ? ? ? unsigned char capid0_8b; /* 8th byte of CAPID0 */
>>diff --git a/drivers/platform/x86/ibm_rtl.c
>>b/drivers/platform/x86/ibm_rtl.c
>>index 42a7d60..7481146 100644
>>--- a/drivers/platform/x86/ibm_rtl.c
>>+++ b/drivers/platform/x86/ibm_rtl.c
>>@@ -33,6 +33,8 @@
>> #include <linux/mutex.h>
>> #include <asm/bios_ebda.h>
>>
>>+#include <asm-generic/io-64-nonatomic-lo-hi.h>
>>+
>> static bool force;
>> module_param(force, bool, 0);
>> MODULE_PARM_DESC(force, "Force driver load, ignore DMI data");
>>@@ -83,19 +85,6 @@ static void __iomem *rtl_cmd_addr;
>> static u8 rtl_cmd_type;
>> static u8 rtl_cmd_width;
>>
>>-#ifndef readq
>>-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
>>-
>> static void __iomem *rtl_port_map(phys_addr_t addr, unsigned long len)
>> {
>> ? ? ? if (rtl_cmd_type == RTL_ADDR_TYPE_MMIO)
>>diff --git a/drivers/platform/x86/intel_ips.c
>>b/drivers/platform/x86/intel_ips.c
>>index 809a3ae..88a98cf 100644
>>--- a/drivers/platform/x86/intel_ips.c
>>+++ b/drivers/platform/x86/intel_ips.c
>>@@ -77,6 +77,8 @@
>> #include <asm/processor.h>
>> #include "intel_ips.h"
>>
>>+#include <asm-generic/io-64-nonatomic-lo-hi.h>
>>+
>> #define PCI_DEVICE_ID_INTEL_THERMAL_SENSOR 0x3b32
>>
>> /*
>>@@ -344,19 +346,6 @@ struct ips_driver {
>> static bool
>> ips_gpu_turbo_enabled(struct ips_driver *ips);
>>
>>-#ifndef readq
>>-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
>>-
>> /**
>> ?* ips_cpu_busy - is CPU busy?
>> ?* @ips: IPS driver struct
>>diff --git a/drivers/scsi/qla4xxx/ql4_nx.c
>>b/drivers/scsi/qla4xxx/ql4_nx.c
>>index 78f1111..65253df 100644
>>--- a/drivers/scsi/qla4xxx/ql4_nx.c
>>+++ b/drivers/scsi/qla4xxx/ql4_nx.c
>>@@ -10,6 +10,8 @@
>> #include "ql4_def.h"
>> #include "ql4_glbl.h"
>>
>>+#include <asm-generic/io-64-nonatomic-lo-hi.h>
>>+
>> #define MASK(n) ? ? ? ? ? ? ? DMA_BIT_MASK(n)
>>#define MN_WIN(addr) ? (((addr & 0x1fc0000) >> 1) | ((addr >> 25) &
>>0x3ff))
>>#define OCM_WIN(addr) ?(((addr & 0x1ff0000) >> 1) | ((addr >> 25) &
>>0x3ff))
>>@@ -655,27 +657,6 @@ static int qla4_8xxx_pci_is_same_window(struct
>>scsi_qla_host *ha,
>> ? ? ? return 0;
>> }
>>
>>-#ifndef readq
>>-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
>>-
>>-#ifndef writeq
>>-static inline void writeq(__u64 val, volatile void __iomem *addr)
>>-{
>>- ? ? ?writel(val, addr);
>>- ? ? ?writel(val >> 32, addr+4);
>>-}
>>-#endif
>>-
>> static int qla4_8xxx_pci_mem_read_direct(struct scsi_qla_host *ha,
>> ? ? ? ? ? ? ? u64 off, void *data, int size)
>> {
>>diff --git a/include/asm-generic/io-64-nonatomic-hi-lo.h
>>b/include/asm-generic/io-64-nonatomic-hi-lo.h
>>new file mode 100644
>>index 0000000..34e61cc
>>--- /dev/null
>>+++ b/include/asm-generic/io-64-nonatomic-hi-lo.h
>>@@ -0,0 +1,63 @@
>>+#ifndef _ASM_IO_64_NONATOMIC_HI_LO_H_
>>+#define _ASM_IO_64_NONATOMIC_HI_LO_H_
>>+
>>+#include <linux/io.h>
>>+#include <asm-generic/int-ll64.h>
>>+
>>+#ifdef CPU_LITTLE_ENDIAN
>>+
>>+#ifndef readq
>>+
>>+static inline __u64 readq(const volatile void __iomem *addr)
>>+{
>>+ ? ? ?const volatile u32 __iomem *p = addr;
>>+ ? ? ?u32 low, high;
>>+
>>+ ? ? ?high = readl(p + 1);
>>+ ? ? ?low = readl(p);
>>+
>>+ ? ? ?return low + ((u64)high << 32);
>>+}
>>+
>>+#endif
>>+
>>+#ifndef writeq
>>+
>>+static inline void writeq(__u64 val, volatile void __iomem *addr)
>>+{
>>+ ? ? ?writel(val >> 32, addr + 4);
>>+ ? ? ?writel(val, addr);
>>+}
>>+
>>+#endif
>>+
>>+#else ?/* big endian */
>>+
>>+#ifndef readq
>>+
>>+static inline __u64 readq(const volatile void __iomem *addr)
>>+{
>>+ ? ? ?const volatile u32 __iomem *p = addr;
>>+ ? ? ?u32 low, high;
>>+
>>+ ? ? ?low = readl(p + 1);
>>+ ? ? ?high = readl(p);
>>+
>>+ ? ? ?return low + ((u64)high << 32);
>>+}
>>+
>>+#endif
>>+
>>+#ifndef writeq
>>+
>>+static inline void writeq(__u64 val, volatile void __iomem *addr)
>>+{
>>+ ? ? ?writel(val >> 32, addr);
>>+ ? ? ?writel(val, addr + 4);
>>+}
>>+
>>+#endif
>>+
>>+#endif
>>+
>>+#endif ? ? ? ?/* _ASM_IO_64_NONATOMIC_HI_LO_H_ */
>>diff --git a/include/asm-generic/io-64-nonatomic-lo-hi.h
>>b/include/asm-generic/io-64-nonatomic-lo-hi.h
>>new file mode 100644
>>index 0000000..bf2fe0f
>>--- /dev/null
>>+++ b/include/asm-generic/io-64-nonatomic-lo-hi.h
>>@@ -0,0 +1,63 @@
>>+#ifndef _ASM_IO_64_NONATOMIC_LO_HI_H_
>>+#define _ASM_IO_64_NONATOMIC_LO_HI_H_
>>+
>>+#include <linux/io.h>
>>+#include <asm-generic/int-ll64.h>
>>+
>>+#ifdef CPU_LITTLE_ENDIAN
>>+
>>+#ifndef readq
>>+
>>+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
>>+
>>+#ifndef writeq
>>+
>>+static inline void writeq(__u64 val, volatile void __iomem *addr)
>>+{
>>+ ? ? ?writel(val, addr);
>>+ ? ? ?writel(val >> 32, addr + 4);
>>+}
>>+
>>+#endif
>>+
>>+#else ?/* big endian */
>>+
>>+#ifndef readq
>>+
>>+static inline __u64 readq(const volatile void __iomem *addr)
>>+{
>>+ ? ? ?const volatile u32 __iomem *p = addr;
>>+ ? ? ?u32 low, high;
>>+
>>+ ? ? ?high = readl(p);
>>+ ? ? ?low = readl(p + 1);
>>+
>>+ ? ? ?return low + ((u64)high << 32);
>>+}
>>+
>>+#endif
>>+
>>+#ifndef writeq
>>+
>>+static inline void writeq(__u64 val, volatile void __iomem *addr)
>>+{
>>+ ? ? ?writel(val, addr + 4);
>>+ ? ? ?writel(val >> 32, addr);
>>+}
>>+
>>+#endif
>>+
>>+#endif
>>+
>>+#endif ? ? ? ?/* _ASM_IO_64_NONATOMIC_LO_HI_H_ */
>>--
>>1.7.5.1
>
> --
> Sent from my Android phone with K-9 Mail. Please excuse my brevity.
>
--
Hitoshi Mitake
[email protected]
On Thu, Feb 9, 2012 at 19:58, Geert Uytterhoeven <[email protected]> wrote:
> On Wed, Feb 8, 2012 at 17:28, Linus Torvalds
> <[email protected]> wrote:
>> On Wed, Feb 8, 2012 at 8:14 AM, Hitoshi Mitake <[email protected]> wrote:
>>> On Tue, Feb 7, 2012 at 12:56, Linus Torvalds
>>> <[email protected]> wrote:
>>>>
>>>> On Feb 6, 2012 6:47 PM, "Hitoshi Mitake" <[email protected]> wrote:
>>>>>
>>>>>? All of them are endian aware
>>>>
>>>> I think that part is wrong.
>>>>
>>>> Pci is little-endian, so readl and writel are always already little-endian.
>>>> Trying to make readq be endian-aware is wrong.
>>>
>>> Is every memory area which can be read/written by read[wl]/write[wl]
>>> always little-endian?
>>>
>>> If so, of course I have to eliminate the big-endian version.
>>> But I'm not sure about this point because the new readq/writeq is
>>> in asm-generic which is used by every archs, so I'd like to ask you about it.
>>
>> Yes, PCI is always little-endian everywhere.
>>
>> Now, some architectures may have some unified IO space where parts of
>> it is PCI, and other parts are programmed the same way but is for some
>> other bus, but the PCI part will always be little-endian because
>> otherwise no common driver would ever work. But those non-PCI things
>> would never be relevant for an emulated readq/writeq anyway, and would
>> need some bus-specific accessor functions.
>
> IIRC, some crazy hardware manufacturers did swizzle PCI busses in hardware.
> So doing a PCI "little endian" 32-bit read must be performed using a native
> (big endian) 32-bit read. And it causes more headaches for 16-bit accesses.
>
> Of course this must all be hidden in your readX() implementations, to make the
> common drivers work.
Thanks a lot for your description. I understand the standard requirements for
read[wlq]/write[wlq].
>
> Gr{oetje,eeting}s,
>
> ? ? ? ? ? ? ? ? ? ? ? ? Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
> ? ? ? ? ? ? ? ? ? ? ? ? ? ?? ?? -- Linus Torvalds
--
Hitoshi Mitake
[email protected]
On 02/09/2012 05:37 AM, Hitoshi Mitake wrote:
> On Tue, Feb 7, 2012 at 12:47, [email protected]<[email protected]> wrote:
>> Should be volatile u64 * not volatile void *...
>
> Is this the type of parameters?
> The parameters of atomic readq/writeq defined in arch/x86/include/asm/io.h
> are defined as void *. I think that atomic readq/writeq and non-atomic
> readq/writeq
> should have same typed parameters and return values.
>
That sounds like a bug.
-hpa
On Thu, Feb 9, 2012 at 23:45, H. Peter Anvin <[email protected]> wrote:
> On 02/09/2012 05:37 AM, Hitoshi Mitake wrote:
>>
>> On Tue, Feb 7, 2012 at 12:47, [email protected]<[email protected]> ?wrote:
>>>
>>> Should be volatile u64 * not volatile void *...
>>
>>
>> Is this the type of parameters?
>> The parameters of atomic readq/writeq defined in arch/x86/include/asm/io.h
>> are defined as void *. I think that atomic readq/writeq and non-atomic
>> readq/writeq
>> should have same typed parameters and return values.
>>
>
> That sounds like a bug.
>
> ? ? ? ?-hpa
>
Sorry for my bad writing, I didn't mention:
the parameter type == the return value type
My intention is:
parameter type of atomic readq/writeq == parameter type of non-atomic
readq/writeq
&&
return value type of atomic readq/writeq == return value type of
non-atomic readq/writeq
# == means "should be equal"
--
Hitoshi Mitake
[email protected]
The point is that the type for all readq/writeq should be u64.
Hitoshi Mitake <[email protected]> wrote:
>On Thu, Feb 9, 2012 at 23:45, H. Peter Anvin <[email protected]> wrote:
>> On 02/09/2012 05:37 AM, Hitoshi Mitake wrote:
>>>
>>> On Tue, Feb 7, 2012 at 12:47, [email protected]<[email protected]>
> wrote:
>>>>
>>>> Should be volatile u64 * not volatile void *...
>>>
>>>
>>> Is this the type of parameters?
>>> The parameters of atomic readq/writeq defined in
>arch/x86/include/asm/io.h
>>> are defined as void *. I think that atomic readq/writeq and
>non-atomic
>>> readq/writeq
>>> should have same typed parameters and return values.
>>>
>>
>> That sounds like a bug.
>>
>> -hpa
>>
>
>Sorry for my bad writing, I didn't mention:
>the parameter type == the return value type
>
>My intention is:
>parameter type of atomic readq/writeq == parameter type of non-atomic
>readq/writeq
>&&
>return value type of atomic readq/writeq == return value type of
>non-atomic readq/writeq
>
># == means "should be equal"
>
>--
>Hitoshi Mitake
>[email protected]
--
Sent from my Android phone with K-9 Mail. Please excuse my brevity.
On Thu, Feb 16, 2012 at 14:59, [email protected] <[email protected]> wrote:
> The point is that the type for all readq/writeq should be u64.
So do you mean that readq/writeq should be defined without
bulid_mmio_read/write?
(in the case of x86)
bulid_mmio_read/write define the parameter type of read/write[bwlq] as void *.
And are there some situations that void * typed parameter of
readq/writeq causes problems?
>
> Hitoshi Mitake <[email protected]> wrote:
>
>>On Thu, Feb 9, 2012 at 23:45, H. Peter Anvin <[email protected]> wrote:
>>> On 02/09/2012 05:37 AM, Hitoshi Mitake wrote:
>>>>
>>>> On Tue, Feb 7, 2012 at 12:47, [email protected]<[email protected]>
>>?wrote:
>>>>>
>>>>> Should be volatile u64 * not volatile void *...
>>>>
>>>>
>>>> Is this the type of parameters?
>>>> The parameters of atomic readq/writeq defined in
>>arch/x86/include/asm/io.h
>>>> are defined as void *. I think that atomic readq/writeq and
>>non-atomic
>>>> readq/writeq
>>>> should have same typed parameters and return values.
>>>>
>>>
>>> That sounds like a bug.
>>>
>>> ? ? ? ?-hpa
>>>
>>
>>Sorry for my bad writing, I didn't mention:
>>the parameter type == the return value type
>>
>>My intention is:
>>parameter type of atomic readq/writeq == parameter type of non-atomic
>>readq/writeq
>>&&
>>return value type of atomic readq/writeq == return value type of
>>non-atomic readq/writeq
>>
>># == means "should be equal"
>>
>>--
>>Hitoshi Mitake
>>[email protected]
>
> --
> Sent from my Android phone with K-9 Mail. Please excuse my brevity.
--
Hitoshi Mitake
[email protected]