[PATCH v6 0/5] Consolidate IO memcpy functions

[PATCH v6 0/5] Consolidate IO memcpy functions

[Julian Vetter]

Thank you Catalin for the feedback. It's not a nitpick. I have addressed
it, and added the architecture before the message for the 3 commits that
modify arch code.

Signed-off-by: Julian Vetter <jvetter@kalrayinc.com>
---
Changes for v6:
- Added include of linux/align.h to fix build on arm arch
- Replaced compile-time check by ifdef for the CONFIG_64BIT otherwise we
  get a warning for the 'qc << 32' for archs with 32bit int types
- Suffixed arch commits by arch name
---
Julian Vetter (5):
  Consolidate __memcpy_{to,from}io and __memset_io into iomap_copy.c
  Replace generic memcpy and memset by IO memcpy functions
  arm64: Use generic io memcpy functions
  csky: Use generic io memcpy functions
  loongarch: Use generic io memcpy functions

 arch/arm64/kernel/io.c         |  87 --------------------------
 arch/csky/kernel/Makefile      |   2 +-
 arch/csky/kernel/io.c          |  91 ---------------------------
 arch/loongarch/kernel/Makefile |   2 +-
 arch/loongarch/kernel/io.c     |  94 ----------------------------
 include/asm-generic/io.h       |  18 +++++-
 lib/iomap_copy.c               | 109 +++++++++++++++++++++++++++++++++
 7 files changed, 126 insertions(+), 277 deletions(-)
 delete mode 100644 arch/csky/kernel/io.c
 delete mode 100644 arch/loongarch/kernel/io.c

-- 
2.34.1

[PATCH v6 1/5] Consolidate __memcpy_{to,from}io and __memset_io into iomap_copy.c

[Julian Vetter]

Various architectures have almost the same implementations for
__memcpy_{to,from}io and __memset_io functions. So, consolidate them
into the existing lib/iomap_copy.c.

Reviewed-by: Yann Sionneau <ysionneau@kalrayinc.com>
Signed-off-by: Julian Vetter <jvetter@kalrayinc.com>
---
Signed-off-by: Julian Vetter <jvetter@kalrayinc.com>
---
Changes for v6:
- Included linux/aslign.h
- Replaced compile time check by ifdef to remove compiler warning
---
 include/asm-generic/io.h |  12 +++++
 lib/iomap_copy.c         | 109 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 121 insertions(+)

diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h
index 80de699bf6af..9b8e0449da28 100644
--- a/include/asm-generic/io.h
+++ b/include/asm-generic/io.h
@@ -102,6 +102,18 @@ static inline void log_post_read_mmio(u64 val, u8 width, const volatile void __i
 
 #endif /* CONFIG_TRACE_MMIO_ACCESS */
 
+#ifndef __memcpy_fromio
+void __memcpy_fromio(void *to, const volatile void __iomem *from, size_t count);
+#endif
+
+#ifndef __memcpy_toio
+void __memcpy_toio(volatile void __iomem *to, const void *from, size_t count);
+#endif
+
+#ifndef __memset_io
+void __memset_io(volatile void __iomem *dst, int c, size_t count);
+#endif
+
 /*
  * __raw_{read,write}{b,w,l,q}() access memory in native endianness.
  *
diff --git a/lib/iomap_copy.c b/lib/iomap_copy.c
index 2fd5712fb7c0..c2cee6410151 100644
--- a/lib/iomap_copy.c
+++ b/lib/iomap_copy.c
@@ -3,9 +3,15 @@
  * Copyright 2006 PathScale, Inc.  All Rights Reserved.
  */
 
+#include <asm/unaligned.h>
+
+#include <linux/align.h>
 #include <linux/export.h>
+#include <linux/types.h>
 #include <linux/io.h>
 
+#define NATIVE_STORE_SIZE	(BITS_PER_LONG/8)
+
 /**
  * __iowrite32_copy - copy data to MMIO space, in 32-bit units
  * @to: destination, in MMIO space (must be 32-bit aligned)
@@ -76,3 +82,106 @@ void __iowrite64_copy(void __iomem *to, const void *from, size_t count)
 }
 EXPORT_SYMBOL_GPL(__iowrite64_copy);
 #endif
+
+
+#ifndef __memcpy_fromio
+void __memcpy_fromio(void *to, const volatile void __iomem *from, size_t count)
+{
+	while (count && !IS_ALIGNED((unsigned long)from, NATIVE_STORE_SIZE)) {
+		*(u8 *)to = __raw_readb(from);
+		from++;
+		to++;
+		count--;
+	}
+
+	while (count >= NATIVE_STORE_SIZE) {
+#ifdef CONFIG_64BIT
+			put_unaligned(__raw_readq(from), (uintptr_t *)to);
+#else
+			put_unaligned(__raw_readl(from), (uintptr_t *)to);
+#endif
+
+		from += NATIVE_STORE_SIZE;
+		to += NATIVE_STORE_SIZE;
+		count -= NATIVE_STORE_SIZE;
+	}
+
+	while (count) {
+		*(u8 *)to = __raw_readb(from);
+		from++;
+		to++;
+		count--;
+	}
+}
+EXPORT_SYMBOL(__memcpy_fromio);
+#endif
+
+#ifndef __memcpy_toio
+void __memcpy_toio(volatile void __iomem *to, const void *from, size_t count)
+{
+	while (count && !IS_ALIGNED((unsigned long)to, NATIVE_STORE_SIZE)) {
+		__raw_writeb(*(u8 *)from, to);
+		from++;
+		to++;
+		count--;
+	}
+
+	while (count >= NATIVE_STORE_SIZE) {
+#ifdef CONFIG_64BIT
+			__raw_writeq(get_unaligned((uintptr_t *)from), to);
+#else
+			__raw_writel(get_unaligned((uintptr_t *)from), to);
+#endif
+
+		from += NATIVE_STORE_SIZE;
+		to += NATIVE_STORE_SIZE;
+		count -= NATIVE_STORE_SIZE;
+	}
+
+	while (count) {
+		__raw_writeb(*(u8 *)from, to);
+		from++;
+		to++;
+		count--;
+	}
+}
+EXPORT_SYMBOL(__memcpy_toio);
+#endif
+
+#ifndef __memset_io
+void __memset_io(volatile void __iomem *dst, int c, size_t count)
+{
+	uintptr_t qc = (u8)c;
+
+	qc |= qc << 8;
+	qc |= qc << 16;
+
+#ifdef CONFIG_64BIT
+	qc |= qc << 32;
+#endif
+
+	while (count && !IS_ALIGNED((unsigned long)dst, NATIVE_STORE_SIZE)) {
+		__raw_writeb(c, dst);
+		dst++;
+		count--;
+	}
+
+	while (count >= NATIVE_STORE_SIZE) {
+#ifdef CONFIG_64BIT
+			__raw_writeq(qc, dst);
+#else
+			__raw_writel(qc, dst);
+#endif
+
+		dst += NATIVE_STORE_SIZE;
+		count -= NATIVE_STORE_SIZE;
+	}
+
+	while (count) {
+		__raw_writeb(c, dst);
+		dst++;
+		count--;
+	}
+}
+EXPORT_SYMBOL(__memset_io);
+#endif
-- 
2.34.1

[PATCH v6 2/5] Replace generic memcpy and memset by IO memcpy functions

[Julian Vetter]

Some architectures implement their own memcpy_{to,from}io and memset_io
functions, because the generic memcpy_{to,from}io and memset_io just use
memcpy/memset.This commit replaces the generic memcpy/memset functions
by IO memcpy/memset functions that respect the given architectures
alignment constraints. So, later we can get rid of the individual
implementations and use the generic ones.

Reviewed-by: Yann Sionneau <ysionneau@kalrayinc.com>
Signed-off-by: Julian Vetter <jvetter@kalrayinc.com>
---
Changes for v6:
- No changes
---
 include/asm-generic/io.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h
index 9b8e0449da28..3b7deb724a2c 100644
--- a/include/asm-generic/io.h
+++ b/include/asm-generic/io.h
@@ -1175,7 +1175,7 @@ static inline void unxlate_dev_mem_ptr(phys_addr_t phys, void *addr)
 static inline void memset_io(volatile void __iomem *addr, int value,
 			     size_t size)
 {
-	memset(__io_virt(addr), value, size);
+	__memset_io(__io_virt(addr), value, size);
 }
 #endif
 
@@ -1193,7 +1193,7 @@ static inline void memcpy_fromio(void *buffer,
 				 const volatile void __iomem *addr,
 				 size_t size)
 {
-	memcpy(buffer, __io_virt(addr), size);
+	__memcpy_fromio(buffer, __io_virt(addr), size);
 }
 #endif
 
@@ -1210,7 +1210,7 @@ static inline void memcpy_fromio(void *buffer,
 static inline void memcpy_toio(volatile void __iomem *addr, const void *buffer,
 			       size_t size)
 {
-	memcpy(__io_virt(addr), buffer, size);
+	__memcpy_toio(__io_virt(addr), buffer, size);
 }
 #endif
 
-- 
2.34.1

[PATCH v6 3/5] arm64: Use generic io memcpy functions

[Julian Vetter]

Use the generic __memcpy_{from,to}io and __memset_io functions on the
arm64 processor architecture.

Reviewed-by: Yann Sionneau <ysionneau@kalrayinc.com>
Signed-off-by: Julian Vetter <jvetter@kalrayinc.com>
---
Changes for v6:
- Added proper commit header suffix: 'arm64: ...'
---
 arch/arm64/kernel/io.c | 87 ------------------------------------------
 1 file changed, 87 deletions(-)

diff --git a/arch/arm64/kernel/io.c b/arch/arm64/kernel/io.c
index ef48089fbfe1..fe86ada23c7d 100644
--- a/arch/arm64/kernel/io.c
+++ b/arch/arm64/kernel/io.c
@@ -9,34 +9,6 @@
 #include <linux/types.h>
 #include <linux/io.h>
 
-/*
- * Copy data from IO memory space to "real" memory space.
- */
-void __memcpy_fromio(void *to, const volatile void __iomem *from, size_t count)
-{
-	while (count && !IS_ALIGNED((unsigned long)from, 8)) {
-		*(u8 *)to = __raw_readb(from);
-		from++;
-		to++;
-		count--;
-	}
-
-	while (count >= 8) {
-		*(u64 *)to = __raw_readq(from);
-		from += 8;
-		to += 8;
-		count -= 8;
-	}
-
-	while (count) {
-		*(u8 *)to = __raw_readb(from);
-		from++;
-		to++;
-		count--;
-	}
-}
-EXPORT_SYMBOL(__memcpy_fromio);
-
 /*
  * This generates a memcpy that works on a from/to address which is aligned to
  * bits. Count is in terms of the number of bits sized quantities to copy. It
@@ -78,62 +50,3 @@ void __iowrite32_copy_full(void __iomem *to, const void *from, size_t count)
 	dgh();
 }
 EXPORT_SYMBOL(__iowrite32_copy_full);
-
-/*
- * Copy data from "real" memory space to IO memory space.
- */
-void __memcpy_toio(volatile void __iomem *to, const void *from, size_t count)
-{
-	while (count && !IS_ALIGNED((unsigned long)to, 8)) {
-		__raw_writeb(*(u8 *)from, to);
-		from++;
-		to++;
-		count--;
-	}
-
-	while (count >= 8) {
-		__raw_writeq(*(u64 *)from, to);
-		from += 8;
-		to += 8;
-		count -= 8;
-	}
-
-	while (count) {
-		__raw_writeb(*(u8 *)from, to);
-		from++;
-		to++;
-		count--;
-	}
-}
-EXPORT_SYMBOL(__memcpy_toio);
-
-/*
- * "memset" on IO memory space.
- */
-void __memset_io(volatile void __iomem *dst, int c, size_t count)
-{
-	u64 qc = (u8)c;
-
-	qc |= qc << 8;
-	qc |= qc << 16;
-	qc |= qc << 32;
-
-	while (count && !IS_ALIGNED((unsigned long)dst, 8)) {
-		__raw_writeb(c, dst);
-		dst++;
-		count--;
-	}
-
-	while (count >= 8) {
-		__raw_writeq(qc, dst);
-		dst += 8;
-		count -= 8;
-	}
-
-	while (count) {
-		__raw_writeb(c, dst);
-		dst++;
-		count--;
-	}
-}
-EXPORT_SYMBOL(__memset_io);
-- 
2.34.1

[PATCH v6 4/5] csky: Use generic io memcpy functions

[Julian Vetter]

Use the generic __memcpy_{from,to}io and __memset_io functions.

Reviewed-by: Yann Sionneau <ysionneau@kalrayinc.com>
Signed-off-by: Julian Vetter <jvetter@kalrayinc.com>
---
Changes for v6:
- Added proper commit header suffix: 'csky: ...'
---
 arch/csky/kernel/Makefile |  2 +-
 arch/csky/kernel/io.c     | 91 ---------------------------------------
 2 files changed, 1 insertion(+), 92 deletions(-)
 delete mode 100644 arch/csky/kernel/io.c

diff --git a/arch/csky/kernel/Makefile b/arch/csky/kernel/Makefile
index 8a868316b912..de1c3472e8f0 100644
--- a/arch/csky/kernel/Makefile
+++ b/arch/csky/kernel/Makefile
@@ -2,7 +2,7 @@
 extra-y := vmlinux.lds
 
 obj-y += head.o entry.o atomic.o signal.o traps.o irq.o time.o vdso.o vdso/
-obj-y += power.o syscall.o syscall_table.o setup.o io.o
+obj-y += power.o syscall.o syscall_table.o setup.o
 obj-y += process.o cpu-probe.o ptrace.o stacktrace.o
 obj-y += probes/
 
diff --git a/arch/csky/kernel/io.c b/arch/csky/kernel/io.c
deleted file mode 100644
index 5883f13fa2b1..000000000000
--- a/arch/csky/kernel/io.c
+++ /dev/null
@@ -1,91 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0
-
-#include <linux/export.h>
-#include <linux/types.h>
-#include <linux/io.h>
-
-/*
- * Copy data from IO memory space to "real" memory space.
- */
-void __memcpy_fromio(void *to, const volatile void __iomem *from, size_t count)
-{
-	while (count && !IS_ALIGNED((unsigned long)from, 4)) {
-		*(u8 *)to = __raw_readb(from);
-		from++;
-		to++;
-		count--;
-	}
-
-	while (count >= 4) {
-		*(u32 *)to = __raw_readl(from);
-		from += 4;
-		to += 4;
-		count -= 4;
-	}
-
-	while (count) {
-		*(u8 *)to = __raw_readb(from);
-		from++;
-		to++;
-		count--;
-	}
-}
-EXPORT_SYMBOL(__memcpy_fromio);
-
-/*
- * Copy data from "real" memory space to IO memory space.
- */
-void __memcpy_toio(volatile void __iomem *to, const void *from, size_t count)
-{
-	while (count && !IS_ALIGNED((unsigned long)to, 4)) {
-		__raw_writeb(*(u8 *)from, to);
-		from++;
-		to++;
-		count--;
-	}
-
-	while (count >= 4) {
-		__raw_writel(*(u32 *)from, to);
-		from += 4;
-		to += 4;
-		count -= 4;
-	}
-
-	while (count) {
-		__raw_writeb(*(u8 *)from, to);
-		from++;
-		to++;
-		count--;
-	}
-}
-EXPORT_SYMBOL(__memcpy_toio);
-
-/*
- * "memset" on IO memory space.
- */
-void __memset_io(volatile void __iomem *dst, int c, size_t count)
-{
-	u32 qc = (u8)c;
-
-	qc |= qc << 8;
-	qc |= qc << 16;
-
-	while (count && !IS_ALIGNED((unsigned long)dst, 4)) {
-		__raw_writeb(c, dst);
-		dst++;
-		count--;
-	}
-
-	while (count >= 4) {
-		__raw_writel(qc, dst);
-		dst += 4;
-		count -= 4;
-	}
-
-	while (count) {
-		__raw_writeb(c, dst);
-		dst++;
-		count--;
-	}
-}
-EXPORT_SYMBOL(__memset_io);
-- 
2.34.1

[PATCH v6 5/5] loongarch: Use generic io memcpy functions

[Julian Vetter]

Use the generic __memcpy_{from,to}io and __memset_io functions on the
loongarch processor architecture.

Reviewed-by: Yann Sionneau <ysionneau@kalrayinc.com>
Signed-off-by: Julian Vetter <jvetter@kalrayinc.com>
---
Changes for v6:
- Added proper commit header suffix: 'loongarch: ...'
---
 arch/loongarch/kernel/Makefile |  2 +-
 arch/loongarch/kernel/io.c     | 94 ----------------------------------
 2 files changed, 1 insertion(+), 95 deletions(-)
 delete mode 100644 arch/loongarch/kernel/io.c

diff --git a/arch/loongarch/kernel/Makefile b/arch/loongarch/kernel/Makefile
index c9bfeda89e40..9497968ee158 100644
--- a/arch/loongarch/kernel/Makefile
+++ b/arch/loongarch/kernel/Makefile
@@ -8,7 +8,7 @@ OBJECT_FILES_NON_STANDARD_head.o := y
 extra-y		:= vmlinux.lds
 
 obj-y		+= head.o cpu-probe.o cacheinfo.o env.o setup.o entry.o genex.o \
-		   traps.o irq.o idle.o process.o dma.o mem.o io.o reset.o switch.o \
+		   traps.o irq.o idle.o process.o dma.o mem.o reset.o switch.o \
 		   elf.o syscall.o signal.o time.o topology.o inst.o ptrace.o vdso.o \
 		   alternative.o unwind.o
 
diff --git a/arch/loongarch/kernel/io.c b/arch/loongarch/kernel/io.c
deleted file mode 100644
index cb85bda5a6ad..000000000000
--- a/arch/loongarch/kernel/io.c
+++ /dev/null
@@ -1,94 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0
-/*
- * Copyright (C) 2020-2022 Loongson Technology Corporation Limited
- */
-#include <linux/export.h>
-#include <linux/types.h>
-#include <linux/io.h>
-
-/*
- * Copy data from IO memory space to "real" memory space.
- */
-void __memcpy_fromio(void *to, const volatile void __iomem *from, size_t count)
-{
-	while (count && !IS_ALIGNED((unsigned long)from, 8)) {
-		*(u8 *)to = __raw_readb(from);
-		from++;
-		to++;
-		count--;
-	}
-
-	while (count >= 8) {
-		*(u64 *)to = __raw_readq(from);
-		from += 8;
-		to += 8;
-		count -= 8;
-	}
-
-	while (count) {
-		*(u8 *)to = __raw_readb(from);
-		from++;
-		to++;
-		count--;
-	}
-}
-EXPORT_SYMBOL(__memcpy_fromio);
-
-/*
- * Copy data from "real" memory space to IO memory space.
- */
-void __memcpy_toio(volatile void __iomem *to, const void *from, size_t count)
-{
-	while (count && !IS_ALIGNED((unsigned long)to, 8)) {
-		__raw_writeb(*(u8 *)from, to);
-		from++;
-		to++;
-		count--;
-	}
-
-	while (count >= 8) {
-		__raw_writeq(*(u64 *)from, to);
-		from += 8;
-		to += 8;
-		count -= 8;
-	}
-
-	while (count) {
-		__raw_writeb(*(u8 *)from, to);
-		from++;
-		to++;
-		count--;
-	}
-}
-EXPORT_SYMBOL(__memcpy_toio);
-
-/*
- * "memset" on IO memory space.
- */
-void __memset_io(volatile void __iomem *dst, int c, size_t count)
-{
-	u64 qc = (u8)c;
-
-	qc |= qc << 8;
-	qc |= qc << 16;
-	qc |= qc << 32;
-
-	while (count && !IS_ALIGNED((unsigned long)dst, 8)) {
-		__raw_writeb(c, dst);
-		dst++;
-		count--;
-	}
-
-	while (count >= 8) {
-		__raw_writeq(qc, dst);
-		dst += 8;
-		count -= 8;
-	}
-
-	while (count) {
-		__raw_writeb(c, dst);
-		dst++;
-		count--;
-	}
-}
-EXPORT_SYMBOL(__memset_io);
-- 
2.34.1

Re: [PATCH v6 4/5] csky: Use generic io memcpy functions

[Guo Ren]

On Wed, Sep 25, 2024 at 9:24 PM Julian Vetter <jvetter@kalrayinc.com> wrote:
>
> Use the generic __memcpy_{from,to}io and __memset_io functions.
>
> Reviewed-by: Yann Sionneau <ysionneau@kalrayinc.com>
> Signed-off-by: Julian Vetter <jvetter@kalrayinc.com>
> ---
> Changes for v6:
> - Added proper commit header suffix: 'csky: ...'
> ---
>  arch/csky/kernel/Makefile |  2 +-
>  arch/csky/kernel/io.c     | 91 ---------------------------------------
>  2 files changed, 1 insertion(+), 92 deletions(-)
>  delete mode 100644 arch/csky/kernel/io.c
>
> diff --git a/arch/csky/kernel/Makefile b/arch/csky/kernel/Makefile
> index 8a868316b912..de1c3472e8f0 100644
> --- a/arch/csky/kernel/Makefile
> +++ b/arch/csky/kernel/Makefile
> @@ -2,7 +2,7 @@
>  extra-y := vmlinux.lds
>
>  obj-y += head.o entry.o atomic.o signal.o traps.o irq.o time.o vdso.o vdso/
> -obj-y += power.o syscall.o syscall_table.o setup.o io.o
> +obj-y += power.o syscall.o syscall_table.o setup.o
>  obj-y += process.o cpu-probe.o ptrace.o stacktrace.o
>  obj-y += probes/
>
> diff --git a/arch/csky/kernel/io.c b/arch/csky/kernel/io.c
> deleted file mode 100644
> index 5883f13fa2b1..000000000000
> --- a/arch/csky/kernel/io.c
> +++ /dev/null
> @@ -1,91 +0,0 @@
> -// SPDX-License-Identifier: GPL-2.0
> -
> -#include <linux/export.h>
> -#include <linux/types.h>
> -#include <linux/io.h>
> -
> -/*
> - * Copy data from IO memory space to "real" memory space.
> - */
> -void __memcpy_fromio(void *to, const volatile void __iomem *from, size_t count)
> -{
> -       while (count && !IS_ALIGNED((unsigned long)from, 4)) {
> -               *(u8 *)to = __raw_readb(from);
> -               from++;
> -               to++;
> -               count--;
> -       }
> -
> -       while (count >= 4) {
> -               *(u32 *)to = __raw_readl(from);
> -               from += 4;
> -               to += 4;
> -               count -= 4;
> -       }
> -
> -       while (count) {
> -               *(u8 *)to = __raw_readb(from);
> -               from++;
> -               to++;
> -               count--;
> -       }
> -}
> -EXPORT_SYMBOL(__memcpy_fromio);
> -
> -/*
> - * Copy data from "real" memory space to IO memory space.
> - */
> -void __memcpy_toio(volatile void __iomem *to, const void *from, size_t count)
> -{
> -       while (count && !IS_ALIGNED((unsigned long)to, 4)) {
> -               __raw_writeb(*(u8 *)from, to);
> -               from++;
> -               to++;
> -               count--;
> -       }
> -
> -       while (count >= 4) {
> -               __raw_writel(*(u32 *)from, to);
> -               from += 4;
> -               to += 4;
> -               count -= 4;
> -       }
> -
> -       while (count) {
> -               __raw_writeb(*(u8 *)from, to);
> -               from++;
> -               to++;
> -               count--;
> -       }
> -}
> -EXPORT_SYMBOL(__memcpy_toio);
> -
> -/*
> - * "memset" on IO memory space.
> - */
> -void __memset_io(volatile void __iomem *dst, int c, size_t count)
> -{
> -       u32 qc = (u8)c;
> -
> -       qc |= qc << 8;
> -       qc |= qc << 16;
> -
> -       while (count && !IS_ALIGNED((unsigned long)dst, 4)) {
> -               __raw_writeb(c, dst);
> -               dst++;
> -               count--;
> -       }
> -
> -       while (count >= 4) {
> -               __raw_writel(qc, dst);
> -               dst += 4;
> -               count -= 4;
> -       }
> -
> -       while (count) {
> -               __raw_writeb(c, dst);
> -               dst++;
> -               count--;
> -       }
> -}
> -EXPORT_SYMBOL(__memset_io);
> --
> 2.34.1
>
>
>
>
>
Thx for the clean-up.

Acked-by: Guo Ren <guoren@kernel.org>

-- 
Best Regards
 Guo Ren

Re: [PATCH v6 1/5] Consolidate __memcpy_{to,from}io and __memset_io into iomap_copy.c

[Arnd Bergmann]

On Wed, Sep 25, 2024, at 13:24, Julian Vetter wrote:
> Various architectures have almost the same implementations for
> __memcpy_{to,from}io and __memset_io functions. So, consolidate them
> into the existing lib/iomap_copy.c.
>
> Reviewed-by: Yann Sionneau <ysionneau@kalrayinc.com>
> Signed-off-by: Julian Vetter <jvetter@kalrayinc.com>
> ---
> Signed-off-by: Julian Vetter <jvetter@kalrayinc.com>

You have a duplicated signoff here.


> +#ifndef __memcpy_fromio
> +void __memcpy_fromio(void *to, const volatile void __iomem *from, 
> size_t count);
> +#endif
> +
> +#ifndef __memcpy_toio
> +void __memcpy_toio(volatile void __iomem *to, const void *from, size_t 
> count);
> +#endif
> +
> +#ifndef __memset_io
> +void __memset_io(volatile void __iomem *dst, int c, size_t count);
> +#endif

I'm not entirely sure about the purpose of the #ifdef here, since
nothing ever overrides the double-underscore versions, both before
and after your patches.

Unless I'm missing something here, I think a more logical
sequence would be:

1. add the definitions in this file without the underscores,
   as memcpy_fromio/memcpy_toio/memset_io, with the #ifdef
   for that name that is always set at this point

2. replace the default implementation in asm-generic/io.h
   with extern prototypes, remove the #define from those

3. convert the other architectures, removing both the
   implementations and the prototypes.

     Arnd

Re: [PATCH v6 2/5] Replace generic memcpy and memset by IO memcpy functions

[kernel test robot]

Hi Julian,

kernel test robot noticed the following build errors:

[auto build test ERROR on arm64/for-next/core]
[also build test ERROR on soc/for-next linus/master v6.11 next-20240926]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Julian-Vetter/Consolidate-__memcpy_-to-from-io-and-__memset_io-into-iomap_copy-c/20240925-225627
base:   https://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git for-next/core
patch link:    https://lore.kernel.org/r/20240925132420.821473-3-jvetter%40kalrayinc.com
patch subject: [PATCH v6 2/5] Replace generic memcpy and memset by IO memcpy functions
config: um-randconfig-001-20240927 (https://download.01.org/0day-ci/archive/20240927/202409271351.RPEyNO3U-lkp@intel.com/config)
compiler: clang version 17.0.6 (https://github.com/llvm/llvm-project 6009708b4367171ccdbf4b5905cb6a803753fe18)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240927/202409271351.RPEyNO3U-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202409271351.RPEyNO3U-lkp@intel.com/

All errors (new ones prefixed by >>):

   /usr/bin/ld: warning: .tmp_vmlinux1 has a LOAD segment with RWX permissions
   /usr/bin/ld: drivers/bus/mhi/ep/ring.o: in function `memcpy_toio':
>> include/asm-generic/io.h:1213: undefined reference to `__memcpy_toio'
   /usr/bin/ld: drivers/bus/mhi/ep/ring.o: in function `memcpy_fromio':
>> include/asm-generic/io.h:1196: undefined reference to `__memcpy_fromio'
>> /usr/bin/ld: include/asm-generic/io.h:1196: undefined reference to `__memcpy_fromio'
>> /usr/bin/ld: include/asm-generic/io.h:1196: undefined reference to `__memcpy_fromio'
>> /usr/bin/ld: include/asm-generic/io.h:1196: undefined reference to `__memcpy_fromio'
   clang: error: linker command failed with exit code 1 (use -v to see invocation)


vim +1213 include/asm-generic/io.h

  1181	
  1182	#ifndef memcpy_fromio
  1183	#define memcpy_fromio memcpy_fromio
  1184	/**
  1185	 * memcpy_fromio	Copy a block of data from I/O memory
  1186	 * @dst:		The (RAM) destination for the copy
  1187	 * @src:		The (I/O memory) source for the data
  1188	 * @count:		The number of bytes to copy
  1189	 *
  1190	 * Copy a block of data from I/O memory.
  1191	 */
  1192	static inline void memcpy_fromio(void *buffer,
  1193					 const volatile void __iomem *addr,
  1194					 size_t size)
  1195	{
> 1196		__memcpy_fromio(buffer, __io_virt(addr), size);
  1197	}
  1198	#endif
  1199	
  1200	#ifndef memcpy_toio
  1201	#define memcpy_toio memcpy_toio
  1202	/**
  1203	 * memcpy_toio		Copy a block of data into I/O memory
  1204	 * @dst:		The (I/O memory) destination for the copy
  1205	 * @src:		The (RAM) source for the data
  1206	 * @count:		The number of bytes to copy
  1207	 *
  1208	 * Copy a block of data to I/O memory.
  1209	 */
  1210	static inline void memcpy_toio(volatile void __iomem *addr, const void *buffer,
  1211				       size_t size)
  1212	{
> 1213		__memcpy_toio(__io_virt(addr), buffer, size);
  1214	}
  1215	#endif
  1216	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v6 2/5] Replace generic memcpy and memset by IO memcpy functions

[kernel test robot]

Hi Julian,

kernel test robot noticed the following build errors:

[auto build test ERROR on arm64/for-next/core]
[also build test ERROR on arnd-asm-generic/master soc/for-next linus/master v6.11 next-20240926]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Julian-Vetter/Consolidate-__memcpy_-to-from-io-and-__memset_io-into-iomap_copy-c/20240925-225627
base:   https://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git for-next/core
patch link:    https://lore.kernel.org/r/20240925132420.821473-3-jvetter%40kalrayinc.com
patch subject: [PATCH v6 2/5] Replace generic memcpy and memset by IO memcpy functions
config: um-randconfig-r072-20240927 (https://download.01.org/0day-ci/archive/20240927/202409271356.SXoT779W-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240927/202409271356.SXoT779W-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202409271356.SXoT779W-lkp@intel.com/

All errors (new ones prefixed by >>, old ones prefixed by <<):

WARNING: modpost: missing MODULE_DESCRIPTION() in lib/zlib_inflate/zlib_inflate.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/fpga/tests/fpga-mgr-test.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/fpga/tests/fpga-bridge-test.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/fpga/tests/fpga-region-test.o
>> ERROR: modpost: "__memcpy_fromio" [drivers/mtd/chips/cfi_cmdset_0002.ko] undefined!
>> ERROR: modpost: "__memcpy_fromio" [drivers/mtd/chips/map_ram.ko] undefined!
>> ERROR: modpost: "__memcpy_toio" [drivers/mtd/chips/map_ram.ko] undefined!
>> ERROR: modpost: "__memcpy_fromio" [drivers/mtd/chips/map_rom.ko] undefined!

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v6 1/5] Consolidate __memcpy_{to,from}io and __memset_io into iomap_copy.c

[Julian Vetter]

On 26.09.24 09:14, Arnd Bergmann wrote:
> On Wed, Sep 25, 2024, at 13:24, Julian Vetter wrote:
>> Various architectures have almost the same implementations for
>> __memcpy_{to,from}io and __memset_io functions. So, consolidate them
>> into the existing lib/iomap_copy.c.
>>
>> Reviewed-by: Yann Sionneau <ysionneau@kalrayinc.com>
>> Signed-off-by: Julian Vetter <jvetter@kalrayinc.com>
>> ---
>> Signed-off-by: Julian Vetter <jvetter@kalrayinc.com>
> 
> You have a duplicated signoff here.

Yes, thank you. I will remove it in the next patch revision.

> 
> 
>> +#ifndef __memcpy_fromio
>> +void __memcpy_fromio(void *to, const volatile void __iomem *from,
>> size_t count);
>> +#endif
>> +
>> +#ifndef __memcpy_toio
>> +void __memcpy_toio(volatile void __iomem *to, const void *from, size_t
>> count);
>> +#endif
>> +
>> +#ifndef __memset_io
>> +void __memset_io(volatile void __iomem *dst, int c, size_t count);
>> +#endif
> 
> I'm not entirely sure about the purpose of the #ifdef here, since
> nothing ever overrides the double-underscore versions, both before
> and after your patches.
> 
> Unless I'm missing something here, I think a more logical
> sequence would be:
> 
> 1. add the definitions in this file without the underscores,

by: "...in this file..." you mean the 'lib/iomap_copy.c' file, right? 
But what if an architecture does not select 'CONFIG_HAS_IOMEM'. Then 
'iomap_copy.c' is not compiled and we don't have an implementation, 
right? I tried to compile with ARCH=um, with some MTD chip driver, like 
the robot did and it indeed fails, because um has 'NO_IOMEM' set. and 
the driver uses memcpy_fromio. I mean it's a strange combination, 
because apparently we try to use IO memory? Is this an invalid 
combination? But shouldn't the driver then 'depends on HAS_IOMEM'?

>     as memcpy_fromio/memcpy_toio/memset_io, with the #ifdef
>     for that name that is always set at this point
> 
Right. I will remove it in my next patch revision.


> 2. replace the default implementation in asm-generic/io.h
>     with extern prototypes, remove the #define from those
> 
Yes, I have done this now.

> 3. convert the other architectures, removing both the
>     implementations and the prototypes.
> 

I have removed the prototypes and have aligned the function arguments in 
m68k, alpha, parisc, and sh, which all have their own implementation, 
but had slightly different function arguments. Btw, I have not removed 
their implementations because some of them seem to have optimized 
implementations (e.g., alpha and m68k), that I didn't want to touch. But 
you're right others (e.g., sh) just do byte wise accesses and have a 
comment "This needs to be optimized." Maybe I should remove these and 
let them use the new version?!

>       Arnd
> 
> 
> 
> 

Re: [PATCH v6 1/5] Consolidate __memcpy_{to,from}io and __memset_io into iomap_copy.c

[Arnd Bergmann]

On Fri, Sep 27, 2024, at 08:19, Julian Vetter wrote:
> On 26.09.24 09:14, Arnd Bergmann wrote:
>>> +#ifndef __memcpy_fromio
>>> +void __memcpy_fromio(void *to, const volatile void __iomem *from,
>>> size_t count);
>>> +#endif
>>> +
>>> +#ifndef __memcpy_toio
>>> +void __memcpy_toio(volatile void __iomem *to, const void *from, size_t
>>> count);
>>> +#endif
>>> +
>>> +#ifndef __memset_io
>>> +void __memset_io(volatile void __iomem *dst, int c, size_t count);
>>> +#endif
>> 
>> I'm not entirely sure about the purpose of the #ifdef here, since
>> nothing ever overrides the double-underscore versions, both before
>> and after your patches.
>> 
>> Unless I'm missing something here, I think a more logical
>> sequence would be:
>> 
>> 1. add the definitions in this file without the underscores,
>
> by: "...in this file..." you mean the 'lib/iomap_copy.c' file, right? 

Yes

> But what if an architecture does not select 'CONFIG_HAS_IOMEM'. Then 
> 'iomap_copy.c' is not compiled and we don't have an implementation, 
> right?
> I tried to compile with ARCH=um, with some MTD chip driver, like 
> the robot did and it indeed fails, because um has 'NO_IOMEM' set. and 
> the driver uses memcpy_fromio. I mean it's a strange combination, 
> because apparently we try to use IO memory? Is this an invalid 
> combination? But shouldn't the driver then 'depends on HAS_IOMEM'?

Yes, I think that would be the best way to do it. Alternatively,
arch/um could provide a dummy implementation of these.

>> 3. convert the other architectures, removing both the
>>     implementations and the prototypes.
>> 
>
> I have removed the prototypes and have aligned the function arguments in 
> m68k, alpha, parisc, and sh, which all have their own implementation, 
> but had slightly different function arguments.

Sorry for being unclear, I meant only the architectures that
you are already touching.

> Btw, I have not removed 
> their implementations because some of them seem to have optimized 
> implementations (e.g., alpha and m68k), that I didn't want to touch. But 
> you're right others (e.g., sh) just do byte wise accesses and have a 
> comment "This needs to be optimized." Maybe I should remove these and 
> let them use the new version?!

Ideally we should end up with only one copy, but I'd leave the
rest for a future cleanup. In particular, alpha probably still
needs a custom function.

      Arnd

RE: [PATCH v6 1/5] Consolidate __memcpy_{to,from}io and __memset_io into iomap_copy.c

[David Laight]

From: Julian Vetter
> Sent: 25 September 2024 14:24
> 
> Various architectures have almost the same implementations for
> __memcpy_{to,from}io and __memset_io functions. So, consolidate them
> into the existing lib/iomap_copy.c.
> 
> Reviewed-by: Yann Sionneau <ysionneau@kalrayinc.com>
> Signed-off-by: Julian Vetter <jvetter@kalrayinc.com>
> ---
> Signed-off-by: Julian Vetter <jvetter@kalrayinc.com>
> ---
> Changes for v6:
> - Included linux/aslign.h
> - Replaced compile time check by ifdef to remove compiler warning
> ---
>  include/asm-generic/io.h |  12 +++++
>  lib/iomap_copy.c         | 109 +++++++++++++++++++++++++++++++++++++++
>  2 files changed, 121 insertions(+)
> 
> diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h
> index 80de699bf6af..9b8e0449da28 100644
> --- a/include/asm-generic/io.h
> +++ b/include/asm-generic/io.h
> @@ -102,6 +102,18 @@ static inline void log_post_read_mmio(u64 val, u8 width, const volatile void __i
> 
>  #endif /* CONFIG_TRACE_MMIO_ACCESS */
> 
> +#ifndef __memcpy_fromio
> +void __memcpy_fromio(void *to, const volatile void __iomem *from, size_t count);
> +#endif
> +
> +#ifndef __memcpy_toio
> +void __memcpy_toio(volatile void __iomem *to, const void *from, size_t count);
> +#endif
> +
> +#ifndef __memset_io
> +void __memset_io(volatile void __iomem *dst, int c, size_t count);
> +#endif
> +
>  /*
>   * __raw_{read,write}{b,w,l,q}() access memory in native endianness.
>   *
> diff --git a/lib/iomap_copy.c b/lib/iomap_copy.c
> index 2fd5712fb7c0..c2cee6410151 100644
> --- a/lib/iomap_copy.c
> +++ b/lib/iomap_copy.c
> @@ -3,9 +3,15 @@
>   * Copyright 2006 PathScale, Inc.  All Rights Reserved.
>   */
> 
> +#include <asm/unaligned.h>
> +
> +#include <linux/align.h>
>  #include <linux/export.h>
> +#include <linux/types.h>
>  #include <linux/io.h>
> 
> +#define NATIVE_STORE_SIZE	(BITS_PER_LONG/8)

(sizeof (long))

> +
>  /**
>   * __iowrite32_copy - copy data to MMIO space, in 32-bit units
>   * @to: destination, in MMIO space (must be 32-bit aligned)
> @@ -76,3 +82,106 @@ void __iowrite64_copy(void __iomem *to, const void *from, size_t count)
>  }
>  EXPORT_SYMBOL_GPL(__iowrite64_copy);
>  #endif
> +
> +
> +#ifndef __memcpy_fromio
> +void __memcpy_fromio(void *to, const volatile void __iomem *from, size_t count)
> +{
> +	while (count && !IS_ALIGNED((unsigned long)from, NATIVE_STORE_SIZE)) {
> +		*(u8 *)to = __raw_readb(from);
> +		from++;
> +		to++;
> +		count--;
> +	}
> +
> +	while (count >= NATIVE_STORE_SIZE) {
> +#ifdef CONFIG_64BIT
> +			put_unaligned(__raw_readq(from), (uintptr_t *)to);
> +#else
> +			put_unaligned(__raw_readl(from), (uintptr_t *)to);
> +#endif

That looks horrid to me.
You seem to be mixing several different types and tests.
NATIVE_STORE_SIZE is based on the 'long ' type (indirectly and by assumption).
CONFIG_64BiIT (probably) implies LP64.
readl() reads 4 bytes and readq() 8 (for both 32bit and 64bit kernels)
uintptr is an unsigned integer large enough to hold a pointer.
The sizes might all happen to match, but there is no need to rely on all of them.

I might be best to just use 'sizeof (long)' except that you might
get a compile error on some 32bit archs for the:
	long val = sizeof (val) == 8) ? readq(from) : readl(from);
	put_unaligned(val, (long *)to);
(due to there being no declaration readq())
so it might need a #if somewhere.
OTOH there might always be an 'extern' for readq().

If you are using the __raw_readx() functions don't you need the
synchronisation barriers top and bottom?

Also if put_unaligned() is non-trivial the code will be horrid.
An initial test for ((to | from) & (sizeof (long) - 1) == 0) for an
aligned copy may be worthwhile.

There is the question of whether the code is allowed to do full
word reads - valid if the io area behaves like memory.
In which case you don't want to do byte transfers for alignment
and tail transfers - just read the full word that contains the data.

PCIe reads can be horribly slow (writes are 'posted' so much better).
I'm not sure how long they take into a 'normal' target, but back to
back reads into our fpga are about 128 clocks apart on its internal
125Mhz clock - the host cpu will stall for the entire period.
So you definitely want to use the largest register possible.
(Or try very hard to never do non-dma reads in either direction.)

	David

> +
> +		from += NATIVE_STORE_SIZE;
> +		to += NATIVE_STORE_SIZE;
> +		count -= NATIVE_STORE_SIZE;
> +	}
> +
> +	while (count) {
> +		*(u8 *)to = __raw_readb(from);
> +		from++;
> +		to++;
> +		count--;
> +	}
> +}
> +EXPORT_SYMBOL(__memcpy_fromio);
> +#endif
> +
> +#ifndef __memcpy_toio
> +void __memcpy_toio(volatile void __iomem *to, const void *from, size_t count)
> +{
> +	while (count && !IS_ALIGNED((unsigned long)to, NATIVE_STORE_SIZE)) {
> +		__raw_writeb(*(u8 *)from, to);
> +		from++;
> +		to++;
> +		count--;
> +	}
> +
> +	while (count >= NATIVE_STORE_SIZE) {
> +#ifdef CONFIG_64BIT
> +			__raw_writeq(get_unaligned((uintptr_t *)from), to);
> +#else
> +			__raw_writel(get_unaligned((uintptr_t *)from), to);
> +#endif
> +
> +		from += NATIVE_STORE_SIZE;
> +		to += NATIVE_STORE_SIZE;
> +		count -= NATIVE_STORE_SIZE;
> +	}
> +
> +	while (count) {
> +		__raw_writeb(*(u8 *)from, to);
> +		from++;
> +		to++;
> +		count--;
> +	}
> +}
> +EXPORT_SYMBOL(__memcpy_toio);
> +#endif
> +
> +#ifndef __memset_io
> +void __memset_io(volatile void __iomem *dst, int c, size_t count)
> +{
> +	uintptr_t qc = (u8)c;
> +
> +	qc |= qc << 8;
> +	qc |= qc << 16;
> +
> +#ifdef CONFIG_64BIT
> +	qc |= qc << 32;
> +#endif
> +
> +	while (count && !IS_ALIGNED((unsigned long)dst, NATIVE_STORE_SIZE)) {
> +		__raw_writeb(c, dst);
> +		dst++;
> +		count--;
> +	}
> +
> +	while (count >= NATIVE_STORE_SIZE) {
> +#ifdef CONFIG_64BIT
> +			__raw_writeq(qc, dst);
> +#else
> +			__raw_writel(qc, dst);
> +#endif
> +
> +		dst += NATIVE_STORE_SIZE;
> +		count -= NATIVE_STORE_SIZE;
> +	}
> +
> +	while (count) {
> +		__raw_writeb(c, dst);
> +		dst++;
> +		count--;
> +	}
> +}
> +EXPORT_SYMBOL(__memset_io);
> +#endif
> --
> 2.34.1
> 
> 
> 
> 
> 

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)