Re: [PATCH v3 00/11] mm: ioremap: Convert architectures to take GENERIC_IOREMAP way
From: Christophe Leroy
Date: Wed Oct 12 2022 - 06:08:25 EST
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@xxxxxxxxxxxxx/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@xxxxxxxxxx/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
>