Thank you again for your remarks Arnd and Christoph! I have updated the
patchset, and placed the functions directly in asm-generic/io.h. I have
dropped the libs/iomem_copy.c and have updated/clarified the commit
message in the first patch.

Signed-off-by: Julian Vetter <jvetter@kalrayinc.com>
---
Changes for v10:
- Removed libs/iomem_copy.c again
- Replaced the three functions in asm-generic/io.h directly
- Updated description for the first patch to clarify the matter
- Slightly updated description in patches 2 to 4

Changes for v9:
- Moved functions into a new file iomem_copy.c which is built
  unconditionally
- Guard prototypes with '#ifndef memcpy_fromio', etc.
- Dropped patches 5 to 14 for now. I will send some of the changes in
  separate patches or patchsets to the appropriate mailinglists
- Added proper reviewed-by and acked-by to arm64 and csky patches

Changes for v8:
- Dropped the arch/um patch that adds dummy implementations for IO
  memcpy functions
- Added 3 new patches that fix the dependency problem for UM (added
  dependencies on HAS_IOMEM || INDIRECT_IOMEM)
- Added new patch for s390 to internally call the zpci_memcpy functions
  and not the generic ones from libs/iomap_copy.c
- Addressed reviewer comments and replaced 2 or 3 shifts by
  'qc *= ~0UL / 0xff;'
- Addressed reviewer comments on pasrisc (masking the int value)
- Addressed reviewer comments on alpha (masking the int value)

Changes for v7:
- Added dummy implementations for memcpy_{to,from}io and memset_io on um
  architecture so drivers that use these functions build for um
- Replaced all accesses and checks by long type
- Added function prototypes as extern to asm-generic/io.h
- Removed '__' from the 3 new function names
- Some archs implement their own version of these IO functions with
  slightly different prototypes. So, I added 3 new patches to align
  prototypes with new ones in iomap_copy.c + io.h

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

Changes for v5:
- Added functions to iomap_copy.c as proposed by Arndt
- Removed again the new io_copy.c and related objects
- Removed GENERIC_IO_COPY symbol and instead rely on the existing
  HAS_IOMEM symbol
- Added prototypes of __memcpy_{to,from}io and __memset_io functions to
  asm-generic/io.h

Changes for v4:
- Replaced memcpy/memset in asm-generic/io.h by the new
  __memcpy_{to,from}io and __memset_io, so individual architectures can
  use it instead of using their own implementation.

Changes for v3:
- Replaced again 'if(IS_ENABLED(CONFIG_64BIT))' by '#ifdef CONFIG_64BIT'
  because on 32bit architectures (e.g., csky), __raw_{read,write}q are
  not defined. So, it leads to compilation errors

Changes for v2:
- Renamed io.c -> io_copy.c
- Updated flag to 'GENERIC_IO_COPY'
- Replaced pointer dereferences by 'put_unaligned()'/'get_unaligned()'
- Replaced '#ifdef CONFIG_64BIT' by 'if(IS_ENABLED(CONFIG_64BIT))'
- Removed '__raw_{read,write}_native' and replaced by
  'if(IS_ENABLED(CONFIG_64BIT))' -> '__raw_write{l,q}'
---
Julian Vetter (4):
  Replace fallback for IO memcpy and IO memset
  arm64: Use new fallback IO memcpy/memset
  csky: Use new fallback IO memcpy/memset
  loongarch: Use new fallback IO memcpy/memset

 arch/arm64/include/asm/io.h     |  11 ---
 arch/arm64/kernel/io.c          |  87 ------------------------
 arch/csky/include/asm/io.h      |  11 ---
 arch/csky/kernel/Makefile       |   2 +-
 arch/csky/kernel/io.c           |  91 -------------------------
 arch/loongarch/include/asm/io.h |  10 ---
 arch/loongarch/kernel/Makefile  |   2 +-
 arch/loongarch/kernel/io.c      |  94 --------------------------
 include/asm-generic/io.h        | 116 ++++++++++++++++++++++++++------
 9 files changed, 98 insertions(+), 326 deletions(-)
 delete mode 100644 arch/csky/kernel/io.c
 delete mode 100644 arch/loongarch/kernel/io.c

-- 
2.34.1

From: Julian Vetter
> Sent: 21 October 2024 14:32
> 
> Thank you again for your remarks Arnd and Christoph! I have updated the
> patchset, and placed the functions directly in asm-generic/io.h. I have
> dropped the libs/iomem_copy.c and have updated/clarified the commit
> message in the first patch.

Apart from build 'issues' what is the justification for inlining
these functions?

They are quite large for inlining and some drivers could easily
call them many times.

The I/O cycles themselves are likely to be slow enough that
the cost of a function call is pretty much likely to be noise.

	David

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

On Mon, Oct 21, 2024, at 14:16, David Laight wrote:
> From: Julian Vetter
>> Sent: 21 October 2024 14:32
>> 
>> Thank you again for your remarks Arnd and Christoph! I have updated the
>> patchset, and placed the functions directly in asm-generic/io.h. I have
>> dropped the libs/iomem_copy.c and have updated/clarified the commit
>> message in the first patch.
>
> Apart from build 'issues' what is the justification for inlining
> these functions?

I think I wasn't clear enough with my previous comment, and Julian
just misunderstood what I was asking him to do. Sorry about causing
extra work here.

> They are quite large for inlining and some drivers could easily
> call them many times.
>
> The I/O cycles themselves are likely to be slow enough that
> the cost of a function call is pretty much likely to be noise.

I'm not overly worried about the this, as the functions are
not that big and there are not that many callers. If a file
contains multiple calls to this function, we can expect the
compiler to be smart enough to keep it out of line, though it
still gets duplicated in each driver calling it.

The bit that I am worried about however is the extra #include
for linux/unaligned.h that pulls in fairly large headers
and may lead to circular header dependencies.

To be clear: what I had expected here was to not have any
changes to the v9 version of lib/iomem_copy.c and to simplify
the asm-generic/io.h change to the version below.

       Arnd

---
--- a/include/asm-generic/io.h
+++ b/include/asm-generic/io.h
@@ -1211,7 +1211,6 @@ static inline void unxlate_dev_mem_ptr(phys_addr_t phys, void *addr)
 #endif
 
 #ifndef memset_io
-#define memset_io memset_io
 /**
  * memset_io   Set a range of I/O memory to a constant value
  * @addr:      The beginning of the I/O-memory range to set
@@ -1220,15 +1219,10 @@ static inline void unxlate_dev_mem_ptr(phys_addr_t phys, void *addr)
  *
  * Set a range of I/O memory to a given value.
  */
-static inline void memset_io(volatile void __iomem *addr, int value,
-                            size_t size)
-{
-       memset(__io_virt(addr), value, size);
-}
+void memset_io(volatile void __iomem *addr, int value, size_t size);
 #endif
 
 #ifndef memcpy_fromio
-#define memcpy_fromio memcpy_fromio
 /**
  * memcpy_fromio       Copy a block of data from I/O memory
  * @dst:               The (RAM) destination for the copy
@@ -1237,16 +1231,11 @@ static inline void memset_io(volatile void __iomem *addr, int value,
  *
  * Copy a block of data from I/O memory.
  */
-static inline void memcpy_fromio(void *buffer,
-                                const volatile void __iomem *addr,
-                                size_t size)
-{
-       memcpy(buffer, __io_virt(addr), size);
-}
+void memcpy_fromio(void *buffer, const volatile void __iomem *addr,
+                  size_t size);
 #endif
 
 #ifndef memcpy_toio
-#define memcpy_toio memcpy_toio
 /**
  * memcpy_toio         Copy a block of data into I/O memory
  * @dst:               The (I/O memory) destination for the copy
@@ -1255,11 +1244,8 @@ static inline void memcpy_fromio(void *buffer,
  *
  * Copy a block of data to I/O memory.
  */
-static inline void memcpy_toio(volatile void __iomem *addr, const void *buffer,
-                              size_t size)
-{
-       memcpy(__io_virt(addr), buffer, size);
-}
+void memcpy_toio(volatile void __iomem *addr, const void *buffer,
+                size_t size);
 #endif
 
 extern int devmem_is_allowed(unsigned long pfn);