Re: [PATCH v3 00/11] mm: ioremap: Convert architectures to take GENERIC_IOREMAP way

[Christophe Leroy <christophe.leroy@csgroup.eu>]

Hi,

Le 09/10/2022 à 12:31, Baoquan He a écrit :
> Currently, many architecutres have't taken the standard GENERIC_IOREMAP
> way to implement ioremap_prot(), iounmap(), and ioremap_xx(), but make
> these functions specifically under each arch's folder. Those cause many
> duplicated codes of ioremap() and iounmap().
> 
> In this patchset, firstly adapt the hooks io[re|un]map_allowed, then
> make use of them to convert those ARCH-es to take GENERIC_IOREMAP method.
> With these change, duplicated ioremap/iounmap() code uder ARCH-es are
> removed.
> 
> This is suggested by Christoph in below discussion:
> https://lore.kernel.org/all/Yp7h0Jv6vpgt6xdZ@infradead.org/T/#u
> 
> And it's basically further action after arm64 has converted to
> GENERIC_IOREMAP way in below patchset.
> [PATCH v5 0/6] arm64: Cleanup ioremap() and support ioremap_prot()
> https://lore.kernel.org/all/20220607125027.44946-1-wangkefeng.wang@huawei.com/T/#u
> 

I'm still puzzled with your series, it seems more churn than needed, and 
I still deeply dislike the approach where a function called 
arch_ioremap() says "Today I don't feel like doing the job so I return 
NULL and you'll do it for me"

In order to better illustrate what I have in mind as discussed 
previously, I have prepared a short RFC series based on your v3, taking 
into account the first two architectures (ARC and IA64) of your series, 
and also adding POWERPC architecture. I will send it out shortly.

Christophe

> v2->v3:
> - Rewrite log of all patches to add more details as Christoph suggested.
> 
> - Merge the old patch 1 and 2 which adjusts return values and
>    parameters of arch_ioremap() into one patch, namely the current
>    patch 3. Christoph suggested this.
> 
> - Change the return value of arch_iounmap() to bool type since we only
>    do arch specific address filtering or address checking, bool value
>    can reflect the checking better. This is pointed out by Niklas when
>    he reviewed the s390 patch.
>    
> - Put hexagon patch at the beginning of patchset since hexagon has the
>    same ioremap() and iounmap() as standard ones, no arch_ioremap() and
>    arch_iounmap() hooks need be introduced. So the later arch_ioremap
>    and arch_iounmap() adjustment are not related in hexagon. Christophe
>    suggested this.
> 
> - Remove the early ioremap code from openrisc ioremap() firstly since
>    openrisc doesn't have early ioremap handling in openrisc arch code.
>    This simplifies the later converting to GENERIC_IOREMAP method.
>    Christoph and Stafford suggersted this.
> 
> - Fix compiling erorrs reported by lkp in parisc and sh patches.
>    Adding macro defintions for those port|mem io functions in
>    <asm/io.h> to avoid repeated definition in <asm-generic/io.h>.
> 
> v1->v2:
> - Rename io[re|un]map_allowed() to arch_io[re|un]map() and made
>    some minor changes in patch 1~2 as per Alexander and Kefeng's
>    suggestions. Accordingly, adjust patches~4~11 because of the renaming
>    arch_io[re|un]map().
> 
> Testing:
> - It's running well on arm64 system with the latest upstream kernel
>    and this patchset applied.
> - Cross compiling passed on arc, ia64, parisc, s390, sh, xtensa.
> - cross compiling is not tried on hexagon and openrisc because
>    - Didn't find cross compiling tools for hexagon;
>    - there's error with openrisc compiling, while I have no idea how to
>      fix it. Please see below pasted log:
>      ---------------------------------------------------------------------
>      [root@intel-knightslanding-lb-02 linux]# make ARCH=openrisc defconfig
>      *** Default configuration is based on 'or1ksim_defconfig'
>      #
>      # configuration written to .config
>      #
>      [root@intel-knightslanding-lb-02 linux]# make ARCH=openrisc -j320 CROSS_COMPILE=/usr/bin/openrisc-linux-gnu-
>        SYNC    include/config/auto.conf.cmd
>        CC      scripts/mod/empty.o
>      ./scripts/check-local-export: /usr/bin/openrisc-linux-gnu-nm failed
>      make[1]: *** [scripts/Makefile.build:250: scripts/mod/empty.o] Error 1
>      make[1]: *** Deleting file 'scripts/mod/empty.o'
>      make: *** [Makefile:1275: prepare0] Error 2
>      ----------------------------------------------------------------------
> 
> Baoquan He (11):
>    hexagon: mm: Convert to GENERIC_IOREMAP
>    openrisc: mm: remove unneeded early ioremap code
>    mm/ioremap: change the return value of io[re|un]map_allowed and rename
>    mm: ioremap: allow ARCH to have its own ioremap definition
>    arc: mm: Convert to GENERIC_IOREMAP
>    ia64: mm: Convert to GENERIC_IOREMAP
>    openrisc: mm: Convert to GENERIC_IOREMAP
>    parisc: mm: Convert to GENERIC_IOREMAP
>    s390: mm: Convert to GENERIC_IOREMAP
>    sh: mm: Convert to GENERIC_IOREMAP
>    xtensa: mm: Convert to GENERIC_IOREMAP
> 
>   arch/arc/Kconfig                  |  1 +
>   arch/arc/include/asm/io.h         | 19 ++++++---
>   arch/arc/mm/ioremap.c             | 60 ++++-----------------------
>   arch/arm64/include/asm/io.h       |  5 ++-
>   arch/arm64/mm/ioremap.c           | 16 +++++---
>   arch/hexagon/Kconfig              |  1 +
>   arch/hexagon/include/asm/io.h     |  9 ++++-
>   arch/hexagon/mm/ioremap.c         | 44 --------------------
>   arch/ia64/Kconfig                 |  1 +
>   arch/ia64/include/asm/io.h        | 26 +++++++-----
>   arch/ia64/mm/ioremap.c            | 50 +++++------------------
>   arch/openrisc/Kconfig             |  1 +
>   arch/openrisc/include/asm/io.h    | 12 ++++--
>   arch/openrisc/mm/ioremap.c        | 62 ++--------------------------
>   arch/parisc/Kconfig               |  1 +
>   arch/parisc/include/asm/io.h      | 19 ++++++---
>   arch/parisc/mm/ioremap.c          | 65 +++---------------------------
>   arch/s390/Kconfig                 |  1 +
>   arch/s390/include/asm/io.h        | 25 +++++++-----
>   arch/s390/pci/pci.c               | 65 ++++++------------------------
>   arch/sh/Kconfig                   |  1 +
>   arch/sh/include/asm/io.h          | 67 +++++++++++++++++--------------
>   arch/sh/include/asm/io_noioport.h |  7 ++++
>   arch/sh/mm/ioremap.c              | 63 ++++++-----------------------
>   arch/xtensa/Kconfig               |  1 +
>   arch/xtensa/include/asm/io.h      | 39 ++++++++----------
>   arch/xtensa/mm/ioremap.c          | 56 ++++++--------------------
>   include/asm-generic/io.h          | 30 ++++++++------
>   mm/ioremap.c                      | 13 ++++--
>   29 files changed, 248 insertions(+), 512 deletions(-)
>   delete mode 100644 arch/hexagon/mm/ioremap.c
>