Re: [PATCH v8 11/14] s390: Add wrappers around zpci_memcpy/zpci_memset

On Tue, 2024-10-08 at 09:50 +0200, Julian Vetter wrote:
> Add wrapper functions around zpci_memcpy_{from,to}io and zpci_memset_io,
> which have aligned prototypes with the ones from iomap_copy.c. These
> wrappers are necessary because the prototypes of the zpci_ functions
> can't be changed. In s390 arch code they are directly being called and
> the return value is checked, At the same time they serve as generic
> memcpy_{from,to}io and memset_io functions, without a return value.
> 
> Reviewed-by: Yann Sionneau <[email protected]>
> Signed-off-by: Julian Vetter <[email protected]>
> ---
> Changes for v8:
> - New patch
> ---
>  arch/s390/include/asm/io.h     | 27 +++++++++++++++++++++++----
>  arch/s390/include/asm/pci_io.h |  6 +++---
>  2 files changed, 26 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/s390/include/asm/io.h b/arch/s390/include/asm/io.h
> index 0fbc992d7a5e..f3ef6d4130b3 100644
> --- a/arch/s390/include/asm/io.h
> +++ b/arch/s390/include/asm/io.h
> @@ -58,10 +58,6 @@ static inline void ioport_unmap(void __iomem *p)
>  #define pci_iomap_wc pci_iomap_wc
>  #define pci_iomap_wc_range pci_iomap_wc_range
>  
> -#define memcpy_fromio(dst, src, count)	zpci_memcpy_fromio(dst, src, count)
> -#define memcpy_toio(dst, src, count)	zpci_memcpy_toio(dst, src, count)
> -#define memset_io(dst, val, count)	zpci_memset_io(dst, val, count)
> -
>  #define mmiowb()	zpci_barrier()
>  
>  #define __raw_readb	zpci_read_u8
> @@ -73,6 +69,10 @@ static inline void ioport_unmap(void __iomem *p)
>  #define __raw_writel	zpci_write_u32
>  #define __raw_writeq	zpci_write_u64
>  
> +#define memcpy_fromio memcpy_fromio
> +#define memcpy_toio memcpy_toio
> +#define memset_io memset_io
> +
>  /* combine single writes by using store-block insn */
>  static inline void __iowrite32_copy(void __iomem *to, const void *from,
>  				    size_t count)
> @@ -88,6 +88,25 @@ static inline void __iowrite64_copy(void __iomem *to, const void *from,
>  }
>  #define __iowrite64_copy __iowrite64_copy
>  
> +static inline void memcpy_fromio(void *dst, const volatile void __iomem *src,
> +			    size_t n)
> +{
> +	zpci_memcpy_fromio(dst, src, n);
> +}
> +
> +static inline void memcpy_toio(volatile void __iomem *dst,
> +			      const void *src, size_t n)
> +{
> +	zpci_memcpy_toio(dst, src, n);
> +}
> +
> +static inline void memset_io(volatile void __iomem *dst,
> +			    int val, size_t count)
> +{
> +	zpci_memset_io(dst, val, count);
> +}
> +
> +
>  #endif /* CONFIG_PCI */
>  
>  #include <asm-generic/io.h>
> diff --git a/arch/s390/include/asm/pci_io.h b/arch/s390/include/asm/pci_io.h
> index 2686bee800e3..43a5ea4ee20f 100644
> --- a/arch/s390/include/asm/pci_io.h
> +++ b/arch/s390/include/asm/pci_io.h
> @@ -143,7 +143,7 @@ static inline int zpci_get_max_io_size(u64 src, u64 dst, int len, int max)
>  
>  static inline int zpci_memcpy_fromio(void *dst,
>  				     const volatile void __iomem *src,
> -				     unsigned long n)
> +				     size_t n)
>  {
>  	int size, rc = 0;
>  
> @@ -162,7 +162,7 @@ static inline int zpci_memcpy_fromio(void *dst,
>  }
>  
>  static inline int zpci_memcpy_toio(volatile void __iomem *dst,
> -				   const void *src, unsigned long n)
> +				   const void *src, size_t n)
>  {
>  	int size, rc = 0;
>  
> @@ -187,7 +187,7 @@ static inline int zpci_memcpy_toio(volatile void __iomem *dst,
>  }
>  
>  static inline int zpci_memset_io(volatile void __iomem *dst,
> -				 unsigned char val, size_t count)
> +				 int val, size_t count)
>  {
>  	u8 *src = kmalloc(count, GFP_KERNEL);
>  	int rc;

The code makes sense to me. If I understand Arnd's comment correctly
the wrappers may not be necessary if you don't have the global
prototypes though, right? In this case I think we might want to still
align the parameter types as this seems generally cleaner and the
internal callers already use size_t anyway.

Either way I gave the whole series a quick test on s390x with PCI
devices and all looks good. So feel free to add:

Acked-by: Niklas Schnelle <[email protected]>
Tested-by: Niklas Schnelle <[email protected]>