Re: [PATCH v4 09/16] s390: mm: Convert to GENERIC_IOREMAP

[Niklas Schnelle]

On Thu, 2023-02-16 at 20:34 +0800, Baoquan He wrote:
> By taking GENERIC_IOREMAP method, the generic generic_ioremap_prot(),
> generic_iounmap(), and their generic wrapper ioremap_prot(), ioremap()
> and iounmap() are all visible and available to arch. Arch needs to
> provide wrapper functions to override the generic versions if there's
> arch specific handling in its ioremap_prot(), ioremap() or iounmap().
> This change will simplify implementation by removing duplicated codes
> with generic_ioremap_prot() and generic_iounmap(), and has the equivalent
> functioality as before.
> 
> Here, add wrapper functions ioremap_prot() and iounmap() for s390's
> special operation when ioremap() and iounmap().
> 
> Signed-off-by: Baoquan He <bhe@xxxxxxxxxx>
> Cc: Niklas Schnelle <schnelle@xxxxxxxxxxxxx>
> Cc: Gerald Schaefer <gerald.schaefer@xxxxxxxxxxxxx>
> Cc: Heiko Carstens <hca@xxxxxxxxxxxxx>
> Cc: Vasily Gorbik <gor@xxxxxxxxxxxxx>
> Cc: Alexander Gordeev <agordeev@xxxxxxxxxxxxx>
> Cc: Christian Borntraeger <borntraeger@xxxxxxxxxxxxx>
> Cc: Sven Schnelle <svens@xxxxxxxxxxxxx>
> Cc: linux-s390@xxxxxxxxxxxxxxx
> ---
>  arch/s390/Kconfig          |  1 +
>  arch/s390/include/asm/io.h | 21 ++++++++------
>  arch/s390/pci/pci.c        | 57 +++++++-------------------------------
>  3 files changed, 23 insertions(+), 56 deletions(-)

Thanks for the patch. This is a very clear improvement for us! I tested
this series with s390 systems with and without the PCI memory-I/O (MIO)
support and everything works as expected.

One thing I did stumble upon but which is independent from this patch
is that I think memremap(…, MEMREMAP_WB) on system RAM outside the
direct map is broken for us. At least on systems without PCI memory-I/O
support. I think with this series it would be much easier to fix
though. Basically I think we would have to define arch_memremap_wb() to
generic_ioremap_prot(…, PAGE_KERNEL) and then have iounmap() check
is_ioremap_addr() to see if it is an actual mapping or an address
cookie. But again this is independent of this patch which doesn't
change the behavior in this area.

So feel free to add my:

Reviewed-by: Niklas Schnelle <schnelle@xxxxxxxxxxxxx>
Tested-by: Niklas Schnelle <schnelle@xxxxxxxxxxxxx>