Re: [PATCH 3/7] asm-generic/io.h: make ioread64 and iowrite64 universally available

[Logan Gunthorpe]

On 6/22/2017 2:36 PM, Alan Cox wrote:

I think that makes sense for the platforms with that problem. I'm not
sure there are many that can't do it for mmio at least. 486SX can't do it
and I guess some ARM32 but I think almost everyone else can including
most 32bit x86.

What's more of a problem is a lot of platforms can do 64bit MMIO via
ioread/write64 but not 64bit port I/O, and it's not clear how you
represent that via an ioread/write API that abstracts it away.


In Patch 2, we call bad_io_access for anyone trying to do 64bit accesses 
on port I/O.


Logan

Re: [PATCH 3/7] asm-generic/io.h: make ioread64 and iowrite64 universally available

[Alan Cox]

On Thu, 22 Jun 2017 14:24:58 -0600
Logan Gunthorpe  wrote:

> On 6/22/2017 2:14 PM, Alan Cox wrote:
> > If a platform doesn't support 64bit I/O operations from the CPU then you
> > either need to use some kind of platform/architecture specific interface
> > if present or accept you don't have one.  
> 
> Yes, I understand that.
> 
> The thing is that every user that's currently using it right now is 
> patching in their own version that splits it on non-64bit systems.
> 
> > It's not safe to split it. Possibly for some use cases you could add an
> > ioread64_maysplit()  
> 
> I'm open to doing something like that.

I think that makes sense for the platforms with that problem. I'm not
sure there are many that can't do it for mmio at least. 486SX can't do it
and I guess some ARM32 but I think almost everyone else can including
most 32bit x86.

What's more of a problem is a lot of platforms can do 64bit MMIO via
ioread/write64 but not 64bit port I/O, and it's not clear how you
represent that via an ioread/write API that abstracts it away.

Alan

Re: [PATCH 3/7] asm-generic/io.h: make ioread64 and iowrite64 universally available

[Logan Gunthorpe]

On 6/22/2017 2:14 PM, Alan Cox wrote:

If a platform doesn't support 64bit I/O operations from the CPU then you
either need to use some kind of platform/architecture specific interface
if present or accept you don't have one.


Yes, I understand that.

The thing is that every user that's currently using it right now is 
patching in their own version that splits it on non-64bit systems.



It's not safe to split it. Possibly for some use cases you could add an
ioread64_maysplit()


I'm open to doing something like that.


What btw is the actual ARM compiler warning ? Is the compiler also trying
to tell you it's a bad idea ?


It's just the compiler noting that you are mixing volatile and 
non-volatile pointers. Strangely some io{read|write}XX use volatile but 
most do not. But it's nothing crazy.


Logan

Re: [PATCH 3/7] asm-generic/io.h: make ioread64 and iowrite64 universally available

[Alan Cox]

On Thu, 22 Jun 2017 10:48:13 -0600
Logan Gunthorpe  wrote:

> Currently, ioread64 and iowrite64 are only available io CONFIG_64BIT=y
> and CONFIG_GENERIC_IOMAP=n. Thus, seeing the functions are not
> universally available, it makes them unusable for driver developers.
> This leads to ugly hacks such as those at the top of
> 
> drivers/ntb/hw/intel/ntb_hw_intel.c
> 
> This patch adds fallback implementations for when CONFIG_64BIT and
> CONFIG_GENERIC_IOMAP are not set. These functions use two io32 based
> calls to complete the operation.
> 
> Note, we do not use the volatile keyword in these functions like the
> others in the same file. It is necessary to avoid a compiler warning
> on arm.

This is a really really bad idea as per the Alpha comment.

ioread64 and iowrite64 generate a single 64bit bus transaction. There is
hardware where mmio operations have side effects so simply using a pair
of 32bit operations blindly does not work (consider something as trivial
as reading a 64bit performance counter or incrementing pointer).

If a platform doesn't support 64bit I/O operations from the CPU then you
either need to use some kind of platform/architecture specific interface
if present or accept you don't have one.

It's not safe to split it. Possibly for some use cases you could add an

ioread64_maysplit()

but you cannot blindly break ioread64/write64() and expect it to
magically allow you to use drivers that depend upon it.

What btw is the actual ARM compiler warning ? Is the compiler also trying
to tell you it's a bad idea ?

Alan

[PATCH 3/7] asm-generic/io.h: make ioread64 and iowrite64 universally available

[Logan Gunthorpe]

Currently, ioread64 and iowrite64 are only available io CONFIG_64BIT=y
and CONFIG_GENERIC_IOMAP=n. Thus, seeing the functions are not
universally available, it makes them unusable for driver developers.
This leads to ugly hacks such as those at the top of

drivers/ntb/hw/intel/ntb_hw_intel.c

This patch adds fallback implementations for when CONFIG_64BIT and
CONFIG_GENERIC_IOMAP are not set. These functions use two io32 based
calls to complete the operation.

Note, we do not use the volatile keyword in these functions like the
others in the same file. It is necessary to avoid a compiler warning
on arm.

Signed-off-by: Logan Gunthorpe 
Cc: Arnd Bergmann 
---
 include/asm-generic/io.h | 54 +---
 1 file changed, 42 insertions(+), 12 deletions(-)

diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h
index 7ef015eb3403..817edaa3da78 100644
--- a/include/asm-generic/io.h
+++ b/include/asm-generic/io.h
@@ -585,15 +585,24 @@ static inline u32 ioread32(const volatile void __iomem 
*addr)
 }
 #endif
 
-#ifdef CONFIG_64BIT
 #ifndef ioread64
 #define ioread64 ioread64
-static inline u64 ioread64(const volatile void __iomem *addr)
+#ifdef readq
+static inline u64 ioread64(const void __iomem *addr)
 {
return readq(addr);
 }
+#else
+static inline u64 ioread64(const void __iomem *addr)
+{
+   u64 low, high;
+
+   low = ioread32(addr);
+   high = ioread32(addr + sizeof(u32));
+   return low | (high << 32);
+}
+#endif
 #endif
-#endif /* CONFIG_64BIT */
 
 #ifndef iowrite8
 #define iowrite8 iowrite8
@@ -619,15 +628,21 @@ static inline void iowrite32(u32 value, volatile void 
__iomem *addr)
 }
 #endif
 
-#ifdef CONFIG_64BIT
 #ifndef iowrite64
 #define iowrite64 iowrite64
-static inline void iowrite64(u64 value, volatile void __iomem *addr)
+#ifdef writeq
+static inline void iowrite64(u64 value, void __iomem *addr)
 {
writeq(value, addr);
 }
+#else
+static inline void iowrite64(u64 value, void __iomem *addr)
+{
+   iowrite32(value, addr);
+   iowrite32(value >> 32, addr + sizeof(u32));
+}
+#endif
 #endif
-#endif /* CONFIG_64BIT */
 
 #ifndef ioread16be
 #define ioread16be ioread16be
@@ -645,15 +660,24 @@ static inline u32 ioread32be(const volatile void __iomem 
*addr)
 }
 #endif
 
-#ifdef CONFIG_64BIT
 #ifndef ioread64be
 #define ioread64be ioread64be
-static inline u64 ioread64be(const volatile void __iomem *addr)
+#ifdef readq
+static inline u64 ioread64be(const void __iomem *addr)
 {
return swab64(readq(addr));
 }
+#else
+static inline u64 ioread64be(const void __iomem *addr)
+{
+   u64 low, high;
+
+   low = ioread32be(addr + sizeof(u32));
+   high = ioread32be(addr);
+   return low | (high << 32);
+}
+#endif
 #endif
-#endif /* CONFIG_64BIT */
 
 #ifndef iowrite16be
 #define iowrite16be iowrite16be
@@ -671,15 +695,21 @@ static inline void iowrite32be(u32 value, volatile void 
__iomem *addr)
 }
 #endif
 
-#ifdef CONFIG_64BIT
 #ifndef iowrite64be
 #define iowrite64be iowrite64be
-static inline void iowrite64be(u64 value, volatile void __iomem *addr)
+#ifdef writeq
+static inline void iowrite64be(u64 value, void __iomem *addr)
 {
writeq(swab64(value), addr);
 }
+#else
+static inline void iowrite64be(u64 value, void __iomem *addr)
+{
+   iowrite32be(value >> 32, addr);
+   iowrite32be(value, addr + sizeof(u32));
+}
+#endif
 #endif
-#endif /* CONFIG_64BIT */
 
 #ifndef ioread8_rep
 #define ioread8_rep ioread8_rep
-- 
2.11.0