[PATCH v2 0/3] make io{read|write}64 more globally usable

[PATCH v2 0/3] make io{read|write}64 more globally usable

[Logan Gunthorpe]

Hi,

This is my second attempt to cleanup the io{read|write}64 functions so
that I can use them in a new driver[1]. This time, per a suggestion from Arnd,
we simply add the io64 functions to the io-64-nonatomic headers and
cleanup the two obvious drivers. (Horia provided me with a corrected patch
for the crypto caam which I have included.)

I've droped the tilcdc patch as it does not appear to support
non-atomic accesses, so it's hack around iowrite64 will remain.

Thanks,

Logan

[1] https://marc.info/?l=linux-kernel&m=149774601910663&w=2

Horia Geantă (1):
  crypto: caam: cleanup CONFIG_64BIT ifdefs when using io{read|write}64

Logan Gunthorpe (2):
  io-64-nonatomic: add io{read|write}64[be] macros
  ntb: ntb_hw_intel: use io-64-nonatomic instead of in-driver hacks

 drivers/crypto/caam/regs.h            | 35 +++++------------------------------
 drivers/ntb/hw/intel/ntb_hw_intel.c   | 31 +------------------------------
 include/linux/io-64-nonatomic-hi-lo.h | 16 ++++++++++++++++
 include/linux/io-64-nonatomic-lo-hi.h | 16 ++++++++++++++++
 4 files changed, 38 insertions(+), 60 deletions(-)

--
2.11.0

[PATCH v2 1/3] io-64-nonatomic: add io{read|write}64[be] macros

[Logan Gunthorpe]

This patch adds io{read|write}64[be] macros to point to the readq/writeq
in use.

This is because new drivers are encouraged to use ioreadXX, et al instead
of readX[1], et al -- and mixing ioreadXX with readq is pretty ugly.

[1] ldd3: section 9.4.2

Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
cc: Christoph Hellwig <hch@lst.de>
cc: Arnd Bergmann <arnd@arndb.de>
cc: Alan Cox <gnomes@lxorguk.ukuu.org.uk>
cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 include/linux/io-64-nonatomic-hi-lo.h | 16 ++++++++++++++++
 include/linux/io-64-nonatomic-lo-hi.h | 16 ++++++++++++++++
 2 files changed, 32 insertions(+)

diff --git a/include/linux/io-64-nonatomic-hi-lo.h b/include/linux/io-64-nonatomic-hi-lo.h
index defcc4644ce3..07a75831244f 100644
--- a/include/linux/io-64-nonatomic-hi-lo.h
+++ b/include/linux/io-64-nonatomic-hi-lo.h
@@ -54,4 +54,20 @@ static inline void hi_lo_writeq_relaxed(__u64 val, volatile void __iomem *addr)
 #define writeq_relaxed hi_lo_writeq_relaxed
 #endif
 
+#ifndef ioread64
+#define ioread64 readq
+#endif
+
+#ifndef iowrite64
+#define iowrite64 writeq
+#endif
+
+#ifndef ioread64be
+#define ioread64be(p) be64_to_cpu(ioread64(p))
+#endif
+
+#ifndef iowrite64be
+#define iowrite64be(v, p) iowrite64(cpu_to_be64(v), (p))
+#endif
+
 #endif	/* _LINUX_IO_64_NONATOMIC_HI_LO_H_ */
diff --git a/include/linux/io-64-nonatomic-lo-hi.h b/include/linux/io-64-nonatomic-lo-hi.h
index 084461a4e5ab..3d0565e67175 100644
--- a/include/linux/io-64-nonatomic-lo-hi.h
+++ b/include/linux/io-64-nonatomic-lo-hi.h
@@ -54,4 +54,20 @@ static inline void lo_hi_writeq_relaxed(__u64 val, volatile void __iomem *addr)
 #define writeq_relaxed lo_hi_writeq_relaxed
 #endif
 
+#ifndef ioread64
+#define ioread64 readq
+#endif
+
+#ifndef iowrite64
+#define iowrite64 writeq
+#endif
+
+#ifndef ioread64be
+#define ioread64be(p) be64_to_cpu(ioread64(p))
+#endif
+
+#ifndef iowrite64be
+#define iowrite64be(v, p) iowrite64(cpu_to_be64(v), (p))
+#endif
+
 #endif	/* _LINUX_IO_64_NONATOMIC_LO_HI_H_ */
-- 
2.11.0

Re: [PATCH v2 1/3] io-64-nonatomic: add io{read|write}64[be] macros

[Arnd Bergmann]

On Wed, Jun 28, 2017 at 1:02 AM, Logan Gunthorpe <logang@deltatee.com> wrote:
> This patch adds io{read|write}64[be] macros to point to the readq/writeq
> in use.
>
> This is because new drivers are encouraged to use ioreadXX, et al instead
> of readX[1], et al -- and mixing ioreadXX with readq is pretty ugly.
>
> [1] ldd3: section 9.4.2
>
> Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
> cc: Christoph Hellwig <hch@lst.de>
> cc: Arnd Bergmann <arnd@arndb.de>
> cc: Alan Cox <gnomes@lxorguk.ukuu.org.uk>
> cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
>  include/linux/io-64-nonatomic-hi-lo.h | 16 ++++++++++++++++
>  include/linux/io-64-nonatomic-lo-hi.h | 16 ++++++++++++++++
>  2 files changed, 32 insertions(+)
>
> diff --git a/include/linux/io-64-nonatomic-hi-lo.h b/include/linux/io-64-nonatomic-hi-lo.h
> index defcc4644ce3..07a75831244f 100644
> --- a/include/linux/io-64-nonatomic-hi-lo.h
> +++ b/include/linux/io-64-nonatomic-hi-lo.h
> @@ -54,4 +54,20 @@ static inline void hi_lo_writeq_relaxed(__u64 val, volatile void __iomem *addr)
>  #define writeq_relaxed hi_lo_writeq_relaxed
>  #endif
>
> +#ifndef ioread64
> +#define ioread64 readq
> +#endif

This is wrong since ioread* is not the same read* on x86 and other
architectures that have native PCI PIO accessors, or that require
additional barriers for those.

You have to copy hi_lo_readq() here, and call ioread32 twice instead
of calling readl() twice. Same for iowrite64.

> +#ifndef ioread64be
> +#define ioread64be(p) be64_to_cpu(ioread64(p))
> +#endif

This has another problem: ioread64() is defined to access little-endian
registers, just like readq(). This means that instead of be64_to_cpu()
you need swab64() and always perform the byte swap, otherwise this
would be broken on big-endian architectures.

       Arnd

Re: [PATCH v2 1/3] io-64-nonatomic: add io{read|write}64[be] macros

[Logan Gunthorpe]

On 28/06/17 04:11 AM, Arnd Bergmann wrote:
> This is wrong since ioread* is not the same read* on x86 and other
> architectures that have native PCI PIO accessors, or that require
> additional barriers for those.
> 
> You have to copy hi_lo_readq() here, and call ioread32 twice instead
> of calling readl() twice. Same for iowrite64.
> 
>> +#ifndef ioread64be
>> +#define ioread64be(p) be64_to_cpu(ioread64(p))
>> +#endif
> 
> This has another problem: ioread64() is defined to access little-endian
> registers, just like readq(). This means that instead of be64_to_cpu()
> you need swab64() and always perform the byte swap, otherwise this
> would be broken on big-endian architectures.

Ah, ok, understood. I'll see if I can fix both these issues.

Thanks,

Logan

[PATCH v2 2/3] ntb: ntb_hw_intel: use io-64-nonatomic instead of in-driver hacks

[Logan Gunthorpe]

Now that ioread64 and iowrite64 are available in io-64-nonatomic,
we can remove the hack at the top of ntb_hw_intel.c and replace it
with an include.

Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
Cc: Jon Mason <jdmason@kudzu.us>
Cc: Allen Hubbe <Allen.Hubbe@emc.com>
Acked-by: Dave Jiang <dave.jiang@intel.com>
---
 drivers/ntb/hw/intel/ntb_hw_intel.c | 31 +------------------------------
 1 file changed, 1 insertion(+), 30 deletions(-)

diff --git a/drivers/ntb/hw/intel/ntb_hw_intel.c b/drivers/ntb/hw/intel/ntb_hw_intel.c
index 7b3b6fd63d7d..13978bca47b8 100644
--- a/drivers/ntb/hw/intel/ntb_hw_intel.c
+++ b/drivers/ntb/hw/intel/ntb_hw_intel.c
@@ -57,6 +57,7 @@
 #include <linux/random.h>
 #include <linux/slab.h>
 #include <linux/ntb.h>
+#include <linux/io-64-nonatomic-lo-hi.h>
 
 #include "ntb_hw_intel.h"
 
@@ -153,35 +154,6 @@ MODULE_PARM_DESC(xeon_b2b_dsd_bar5_addr32,
 static inline enum ntb_topo xeon_ppd_topo(struct intel_ntb_dev *ndev, u8 ppd);
 static int xeon_init_isr(struct intel_ntb_dev *ndev);
 
-#ifndef ioread64
-#ifdef readq
-#define ioread64 readq
-#else
-#define ioread64 _ioread64
-static inline u64 _ioread64(void __iomem *mmio)
-{
-	u64 low, high;
-
-	low = ioread32(mmio);
-	high = ioread32(mmio + sizeof(u32));
-	return low | (high << 32);
-}
-#endif
-#endif
-
-#ifndef iowrite64
-#ifdef writeq
-#define iowrite64 writeq
-#else
-#define iowrite64 _iowrite64
-static inline void _iowrite64(u64 val, void __iomem *mmio)
-{
-	iowrite32(val, mmio);
-	iowrite32(val >> 32, mmio + sizeof(u32));
-}
-#endif
-#endif
-
 static inline int pdev_is_atom(struct pci_dev *pdev)
 {
 	switch (pdev->device) {
@@ -3008,4 +2980,3 @@ static void __exit intel_ntb_pci_driver_exit(void)
 	debugfs_remove_recursive(debugfs_dir);
 }
 module_exit(intel_ntb_pci_driver_exit);
-
-- 
2.11.0

[PATCH v2 3/3] crypto: caam: cleanup CONFIG_64BIT ifdefs when using io{read|write}64

[Logan Gunthorpe]

From: Horia Geantă <horia.geanta@nxp.com>

We can now make use of the io-64-nonatomic-hi-lo header to always
provide 64 bit IO operations. So this patch cleans up the extra
CONFIG_64BIT ifdefs.

Signed-off-by: Horia Geantă <horia.geanta@nxp.com>
Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
Cc: Horia Geantă <horia.geanta@nxp.com>
Cc: Dan Douglass <dan.douglass@nxp.com>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: "David S. Miller" <davem@davemloft.net>
---
 drivers/crypto/caam/regs.h | 35 +++++------------------------------
 1 file changed, 5 insertions(+), 30 deletions(-)

diff --git a/drivers/crypto/caam/regs.h b/drivers/crypto/caam/regs.h
index 84d2f838a063..fdcf719ecfbe 100644
--- a/drivers/crypto/caam/regs.h
+++ b/drivers/crypto/caam/regs.h
@@ -9,7 +9,7 @@
 
 #include <linux/types.h>
 #include <linux/bitops.h>
-#include <linux/io.h>
+#include <linux/io-64-nonatomic-hi-lo.h>
 
 /*
  * Architecture-specific register access methods
@@ -134,50 +134,25 @@ static inline void clrsetbits_32(void __iomem *reg, u32 clear, u32 set)
  *    base + 0x0000 : least-significant 32 bits
  *    base + 0x0004 : most-significant 32 bits
  */
-#ifdef CONFIG_64BIT
 static inline void wr_reg64(void __iomem *reg, u64 data)
 {
+#ifndef CONFIG_CRYPTO_DEV_FSL_CAAM_IMX
 	if (caam_little_end)
 		iowrite64(data, reg);
 	else
-		iowrite64be(data, reg);
-}
-
-static inline u64 rd_reg64(void __iomem *reg)
-{
-	if (caam_little_end)
-		return ioread64(reg);
-	else
-		return ioread64be(reg);
-}
-
-#else /* CONFIG_64BIT */
-static inline void wr_reg64(void __iomem *reg, u64 data)
-{
-#ifndef CONFIG_CRYPTO_DEV_FSL_CAAM_IMX
-	if (caam_little_end) {
-		wr_reg32((u32 __iomem *)(reg) + 1, data >> 32);
-		wr_reg32((u32 __iomem *)(reg), data);
-	} else
 #endif
-	{
-		wr_reg32((u32 __iomem *)(reg), data >> 32);
-		wr_reg32((u32 __iomem *)(reg) + 1, data);
-	}
+		iowrite64be(data, reg);
 }
 
 static inline u64 rd_reg64(void __iomem *reg)
 {
 #ifndef CONFIG_CRYPTO_DEV_FSL_CAAM_IMX
 	if (caam_little_end)
-		return ((u64)rd_reg32((u32 __iomem *)(reg) + 1) << 32 |
-			(u64)rd_reg32((u32 __iomem *)(reg)));
+		return ioread64(reg);
 	else
 #endif
-		return ((u64)rd_reg32((u32 __iomem *)(reg)) << 32 |
-			(u64)rd_reg32((u32 __iomem *)(reg) + 1));
+		return ioread64be(reg);
 }
-#endif /* CONFIG_64BIT  */
 
 #ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
 #ifdef CONFIG_SOC_IMX7D
-- 
2.11.0

Re: [PATCH v2 3/3] crypto: caam: cleanup CONFIG_64BIT ifdefs when using io{read|write}64

[Arnd Bergmann]

On Wed, Jun 28, 2017 at 1:02 AM, Logan Gunthorpe <logang@deltatee.com> wrote:
>  #include <linux/types.h>
>  #include <linux/bitops.h>
> -#include <linux/io.h>
> +#include <linux/io-64-nonatomic-hi-lo.h>

Here you include the hi-lo variant unconditionally.

> -#else /* CONFIG_64BIT */
> -static inline void wr_reg64(void __iomem *reg, u64 data)
> -{
> -#ifndef CONFIG_CRYPTO_DEV_FSL_CAAM_IMX
> -       if (caam_little_end) {
> -               wr_reg32((u32 __iomem *)(reg) + 1, data >> 32);
> -               wr_reg32((u32 __iomem *)(reg), data);
> -       } else
>  #endif
> -       {
> -               wr_reg32((u32 __iomem *)(reg), data >> 32);
> -               wr_reg32((u32 __iomem *)(reg) + 1, data);
> -       }
> +               iowrite64be(data, reg);
>  }

However, the #else path here uses lo-hi instead. I guess we have
to decide how to define iowrite64be_lo_hi() first: it could
either byteswap the 64-bit value first, then write the two halves,
or it could write the two halves, doing a 32-bit byte swap on
each.

       Arnd

Re: [PATCH v2 3/3] crypto: caam: cleanup CONFIG_64BIT ifdefs when using io{read|write}64

[Logan Gunthorpe]

On 28/06/17 04:20 AM, Arnd Bergmann wrote:
> On Wed, Jun 28, 2017 at 1:02 AM, Logan Gunthorpe <logang@deltatee.com> wrote:
>>  #include <linux/types.h>
>>  #include <linux/bitops.h>
>> -#include <linux/io.h>
>> +#include <linux/io-64-nonatomic-hi-lo.h>
> 
> Here you include the hi-lo variant unconditionally.
> 
>> -#else /* CONFIG_64BIT */
>> -static inline void wr_reg64(void __iomem *reg, u64 data)
>> -{
>> -#ifndef CONFIG_CRYPTO_DEV_FSL_CAAM_IMX
>> -       if (caam_little_end) {
>> -               wr_reg32((u32 __iomem *)(reg) + 1, data >> 32);
>> -               wr_reg32((u32 __iomem *)(reg), data);
>> -       } else
>>  #endif
>> -       {
>> -               wr_reg32((u32 __iomem *)(reg), data >> 32);
>> -               wr_reg32((u32 __iomem *)(reg) + 1, data);
>> -       }
>> +               iowrite64be(data, reg);
>>  }
> 
> However, the #else path here uses lo-hi instead. I guess we have
> to decide how to define iowrite64be_lo_hi() first: it could
> either byteswap the 64-bit value first, then write the two halves,
> or it could write the two halves, doing a 32-bit byte swap on
> each.

Ok, I studied this a bit more:

The lo_hi/hi_lo functions seem to always refer to the data being written
or read not to the address operated on. So, in the v3 version of this
set, which I'm working on, I've defined:

static inline void iowrite64_hi_lo(u64 val, void __iomem *addr)
{
        iowrite32(val >> 32, addr + sizeof(u32));
        iowrite32(val, addr);
}

static inline void iowrite64be_hi_lo(u64 val, void __iomem *addr)
{
        iowrite32be(val >> 32, addr);
        iowrite32be(val, addr + sizeof(u32));
}

So the two hi_lo functions match both paths of the #if and thus, I
believe, the patch will be correct in v3 without changes.

Thanks,

Logan

Re: [PATCH v2 3/3] crypto: caam: cleanup CONFIG_64BIT ifdefs when using io{read|write}64

[Horia Geantă]

On 6/28/2017 7:51 PM, Logan Gunthorpe wrote:
> 
> 
> On 28/06/17 04:20 AM, Arnd Bergmann wrote:
>> On Wed, Jun 28, 2017 at 1:02 AM, Logan Gunthorpe <logang@deltatee.com> wrote:
>>>  #include <linux/types.h>
>>>  #include <linux/bitops.h>
>>> -#include <linux/io.h>
>>> +#include <linux/io-64-nonatomic-hi-lo.h>
>>
>> Here you include the hi-lo variant unconditionally.
>>
Arnd, thanks for spotting this.

This was not in the patch I signed off.
The lo-hi variant should be used instead for CAAM, see further below.

>>> -#else /* CONFIG_64BIT */
>>> -static inline void wr_reg64(void __iomem *reg, u64 data)
>>> -{
>>> -#ifndef CONFIG_CRYPTO_DEV_FSL_CAAM_IMX
>>> -       if (caam_little_end) {
>>> -               wr_reg32((u32 __iomem *)(reg) + 1, data >> 32);
>>> -               wr_reg32((u32 __iomem *)(reg), data);
>>> -       } else
>>>  #endif
>>> -       {
>>> -               wr_reg32((u32 __iomem *)(reg), data >> 32);
>>> -               wr_reg32((u32 __iomem *)(reg) + 1, data);
>>> -       }
>>> +               iowrite64be(data, reg);
>>>  }
>>
>> However, the #else path here uses lo-hi instead. I guess we have
>> to decide how to define iowrite64be_lo_hi() first: it could
>> either byteswap the 64-bit value first, then write the two halves,
>> or it could write the two halves, doing a 32-bit byte swap on
>> each.
> 
> Ok, I studied this a bit more:
> 
> The lo_hi/hi_lo functions seem to always refer to the data being written
> or read not to the address operated on. So, in the v3 version of this
> set, which I'm working on, I've defined:
> 
> static inline void iowrite64_hi_lo(u64 val, void __iomem *addr)
> {
>         iowrite32(val >> 32, addr + sizeof(u32));
>         iowrite32(val, addr);
> }
> 
> static inline void iowrite64be_hi_lo(u64 val, void __iomem *addr)
> {
>         iowrite32be(val >> 32, addr);
>         iowrite32be(val, addr + sizeof(u32));
> }
> 
> So the two hi_lo functions match both paths of the #if and thus, I
> believe, the patch will be correct in v3 without changes.
> 
To be consistent with CAAM engine HW spec: in case of 64-bit registers,
irrespective of device endianness, the lower address should be read from
/ written to first, followed by the upper address.

Indeed the I/O accessors in CAAM driver currently don't follow the spec,
however this is a good opportunity to fix the code.
I don't consider this requires a separate patch, as we haven't noticed
any problem. I'd say a simple note in the commit message mentioning the
change (lo-hi r/w order replacing hi-lo for little endian case) is enough.

Thanks,
Horia