mbox series

[v7,00/10] Consolidate IO memcpy functions

Message ID 20240930132321.2785718-1-jvetter@kalrayinc.com (mailing list archive)
Headers show
Series Consolidate IO memcpy functions | expand

Message

Julian Vetter Sept. 30, 2024, 1:23 p.m. UTC
Thank you all for your remarks. I have addressed your feedback. I have
also added the full history of the patchset, because it now targets
additional architectures.

Arnd: Unfortunately when adding the prototypes as 'extern ..' into
asm-generic/io.h they conflict with some individual implementations
(namely m68k, alpha, parisc, and sh). So, I have aligned these functions
to match the prototypes in asm-generic/io.h.
For the um problem, unfortunately there are A LOT of drivers that use
these IO memcpy functions, so I went a bit the lazy route and added
dummy functions to um's io.h.

David: Thank you for your remarks. I have replaced the mix of long,
uintptr_t, etc. all by long + sizeof(long). I have also split the
read/write operation from the put/get_unaligned into two lines for
better readability.

Signed-off-by: Julian Vetter <jvetter@kalrayinc.com>
---
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 (10):
  Consolidate IO memcpy/memset into iomap_copy.c
  arm64: Use generic IO memcpy/memset
  csky: Use generic IO memcpy/memset
  loongarch: Use generic IO memcpy/memset
  m68k: Align prototypes of IO memcpy/memset
  alpha: Align prototypes of IO memcpy/memset
  parisc: Align prototypes of IO memcpy/memset
  sh: Align prototypes of IO memcpy/memset
  um: Add dummy implementation for IO memcpy/memset
  arm: Align prototype of IO memset

 arch/alpha/include/asm/io.h     |   6 +-
 arch/alpha/kernel/io.c          |   4 +-
 arch/arm/include/asm/io.h       |   2 +-
 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 ----------------------
 arch/m68k/include/asm/kmap.h    |   8 +-
 arch/parisc/include/asm/io.h    |   3 -
 arch/parisc/lib/io.c            |   6 +-
 arch/sh/include/asm/io.h        |   3 -
 arch/sh/kernel/io.c             |   6 +-
 arch/um/include/asm/io.h        |  17 ++++
 include/asm-generic/io.h        |  58 ++------------
 lib/iomap_copy.c                | 133 ++++++++++++++++++++++++++++++++
 19 files changed, 173 insertions(+), 381 deletions(-)
 delete mode 100644 arch/csky/kernel/io.c
 delete mode 100644 arch/loongarch/kernel/io.c

Comments

Niklas Schnelle Oct. 1, 2024, 12:46 p.m. UTC | #1
On Mon, 2024-09-30 at 15:23 +0200, Julian Vetter wrote:
> Thank you all for your remarks. I have addressed your feedback. I have
> also added the full history of the patchset, because it now targets
> additional architectures.
> 
> Arnd: Unfortunately when adding the prototypes as 'extern ..' into
> asm-generic/io.h they conflict with some individual implementations
> (namely m68k, alpha, parisc, and sh). So, I have aligned these functions
> to match the prototypes in asm-generic/io.h.
> For the um problem, unfortunately there are A LOT of drivers that use
> these IO memcpy functions, so I went a bit the lazy route and added
> dummy functions to um's io.h.
> 
> David: Thank you for your remarks. I have replaced the mix of long,
> uintptr_t, etc. all by long + sizeof(long). I have also split the
> read/write operation from the put/get_unaligned into two lines for
> better readability.
> 
> Signed-off-by: Julian Vetter <jvetter@kalrayinc.com>
> ---
> 

Hi Julian,

It seems you missed the memcpy_toio()/memcpy_fromio() in
arch/s390/include/asm/io.h, probably because these are macros
and minimal configs might not build s390x with PCI enabled.

One snippet from the error output is:

In file included from arch/s390/kernel/asm-offsets.c:11:
In file included from ./include/linux/kvm_host.h:19:
In file included from ./include/linux/msi.h:24:
In file included from ./include/linux/irq.h:20:
In file included from ./include/linux/io.h:14:
In file included from ./arch/s390/include/asm/io.h:93:
./include/asm-generic/io.h:105:13: error: conflicting types for 'zpci_memcpy_fromio'
  105 | extern void memcpy_fromio(void *to, const volatile void __iomem *from,
      |             ^
./arch/s390/include/asm/io.h:61:40: note: expanded from macro 'memcpy_fromio'
   61 | #define memcpy_fromio(dst, src, count)  zpci_memcpy_fromio(dst, src, count)
      |                                         ^
./arch/s390/include/asm/pci_io.h:144:19: note: previous definition is here
  144 | static inline int zpci_memcpy_fromio(void *dst,
      |                   ^

I think the best course of action might be to change the
zpci_memcpy_…() functions to match the generic signatures. While the
generic implementation might work it would be very inefficient for us
as we really need to use the PCI Store Block instructions.

Thanks,
Niklas