Re: [PATCH v9 1/4] Consolidate IO memcpy/memset into iomem_copy.c

[Arnd Bergmann]

On Thu, Oct 10, 2024, at 12:36, Julian Vetter wrote:
> Various architectures have almost the same implementations for
> memcpy_{to,from}io and memset_io functions. So, consolidate them
> into a common lib/iomem_copy.c.
>
> Reviewed-by: Yann Sionneau <ysionneau@xxxxxxxxxxxxx>
> Signed-off-by: Julian Vetter <jvetter@xxxxxxxxxxxxx>
> ---
> Changes for v9:
> - Moved all functions to iomem_copy.c
> - Build the new iomem_copy.c unconditionally
> - Guard prototypes in asm-generic/io.h with ifdefs

I'm happy with the patch contents, but it looks like you forgot
to update the description as this is not what this version of the
patch does: Instead of consolidating the identical versions (which
you do in patches 2-4), this changes the ones that use the
memcpy/memset fallback: arc, microblaze, mips, nios2, openrisc,
riscv, and xtensa.

> --- a/include/asm-generic/io.h
> +++ b/include/asm-generic/io.h
> @@ -102,6 +102,16 @@ 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.
>   *
> @@ -1150,58 +1160,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
> - * @val:	The value to set the memory to
> - * @count:	The number of bytes to set
> - *
> - * 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);
> -}
> -#endif

Unless there is a technical reason to move the location of
these I think it would be clearer to change it in place
and keep the three #ifdef but replace the contents.

You can also choose to add the definition in one patch
and then change the header in the next patch, which would
let you have more descriptive changelog texts for the
new common implementation and the second patch that changes
the fallback.

     Arnd