[PATCH v2 09/11] s390: mm: Convert to GENERIC_IOREMAP

[PATCH v2 09/11] s390: mm: Convert to GENERIC_IOREMAP

[Baoquan He]

Add hooks arch_ioremap() and arch_iounmap() for s390's special
operation when ioremap() and iounmap(), then ioremap_[wc|wt]() are
converted to use ioremap_prot() from GENERIC_IOREMAP.

Signed-off-by: Baoquan He <bhe@redhat.com>
Cc: Heiko Carstens <hca@linux.ibm.com>
Cc: Vasily Gorbik <gor@linux.ibm.com>
Cc: Alexander Gordeev <agordeev@linux.ibm.com>
Cc: Christian Borntraeger <borntraeger@linux.ibm.com>
Cc: Sven Schnelle <svens@linux.ibm.com>
Cc: linux-s390@vger.kernel.org
---
 arch/s390/Kconfig          |  1 +
 arch/s390/include/asm/io.h | 26 +++++++++++------
 arch/s390/pci/pci.c        | 60 +++++---------------------------------
 3 files changed, 26 insertions(+), 61 deletions(-)

diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
index 318fce77601d..c59e1b25f59d 100644
--- a/arch/s390/Kconfig
+++ b/arch/s390/Kconfig
@@ -135,6 +135,7 @@ config S390
 	select GENERIC_SMP_IDLE_THREAD
 	select GENERIC_TIME_VSYSCALL
 	select GENERIC_VDSO_TIME_NS
+	select GENERIC_IOREMAP
 	select HAVE_ALIGNED_STRUCT_PAGE if SLUB
 	select HAVE_ARCH_AUDITSYSCALL
 	select HAVE_ARCH_JUMP_LABEL
diff --git a/arch/s390/include/asm/io.h b/arch/s390/include/asm/io.h
index e3882b012bfa..f837e20b7bbd 100644
--- a/arch/s390/include/asm/io.h
+++ b/arch/s390/include/asm/io.h
@@ -22,11 +22,23 @@ void unxlate_dev_mem_ptr(phys_addr_t phys, void *addr);
 
 #define IO_SPACE_LIMIT 0
 
-void __iomem *ioremap_prot(phys_addr_t addr, size_t size, unsigned long prot);
-void __iomem *ioremap(phys_addr_t addr, size_t size);
-void __iomem *ioremap_wc(phys_addr_t addr, size_t size);
-void __iomem *ioremap_wt(phys_addr_t addr, size_t size);
-void iounmap(volatile void __iomem *addr);
+
+/*
+ * I/O memory mapping functions.
+ */
+void __iomem *
+arch_ioremap(phys_addr_t *paddr, size_t size, unsigned long *prot_val);
+#define arch_ioremap arch_ioremap
+
+int arch_iounmap(void __iomem *addr);
+#define arch_iounmap arch_iounmap
+
+#define _PAGE_IOREMAP pgprot_val(PAGE_KERNEL)
+
+#define ioremap_wc(addr, size)  \
+	ioremap_prot((addr), (size), pgprot_val(pgprot_writecombine(PAGE_KERNEL)))
+#define ioremap_wt(addr, size)  \
+	ioremap_prot((addr), (size), pgprot_val(pgprot_writethrough(PAGE_KERNEL)))
 
 static inline void __iomem *ioport_map(unsigned long port, unsigned int nr)
 {
@@ -51,10 +63,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 ioremap ioremap
-#define ioremap_wt ioremap_wt
-#define ioremap_wc ioremap_wc
-
 #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)
diff --git a/arch/s390/pci/pci.c b/arch/s390/pci/pci.c
index 73cdc5539384..984cad9cd5a1 100644
--- a/arch/s390/pci/pci.c
+++ b/arch/s390/pci/pci.c
@@ -244,64 +244,20 @@ void __iowrite64_copy(void __iomem *to, const void *from, size_t count)
        zpci_memcpy_toio(to, from, count);
 }
 
-static void __iomem *__ioremap(phys_addr_t addr, size_t size, pgprot_t prot)
+void __iomem *
+arch_ioremap(phys_addr_t *paddr, size_t size, unsigned long *prot_val)
 {
-	unsigned long offset, vaddr;
-	struct vm_struct *area;
-	phys_addr_t last_addr;
-
-	last_addr = addr + size - 1;
-	if (!size || last_addr < addr)
-		return NULL;
-
 	if (!static_branch_unlikely(&have_mio))
-		return (void __iomem *) addr;
-
-	offset = addr & ~PAGE_MASK;
-	addr &= PAGE_MASK;
-	size = PAGE_ALIGN(size + offset);
-	area = get_vm_area(size, VM_IOREMAP);
-	if (!area)
-		return NULL;
-
-	vaddr = (unsigned long) area->addr;
-	if (ioremap_page_range(vaddr, vaddr + size, addr, prot)) {
-		free_vm_area(area);
-		return NULL;
-	}
-	return (void __iomem *) ((unsigned long) area->addr + offset);
-}
-
-void __iomem *ioremap_prot(phys_addr_t addr, size_t size, unsigned long prot)
-{
-	return __ioremap(addr, size, __pgprot(prot));
+		return (void __iomem *) *paddr;
+	return NULL;
 }
-EXPORT_SYMBOL(ioremap_prot);
 
-void __iomem *ioremap(phys_addr_t addr, size_t size)
+int arch_iounmap(void __iomem *addr)
 {
-	return __ioremap(addr, size, PAGE_KERNEL);
-}
-EXPORT_SYMBOL(ioremap);
-
-void __iomem *ioremap_wc(phys_addr_t addr, size_t size)
-{
-	return __ioremap(addr, size, pgprot_writecombine(PAGE_KERNEL));
-}
-EXPORT_SYMBOL(ioremap_wc);
-
-void __iomem *ioremap_wt(phys_addr_t addr, size_t size)
-{
-	return __ioremap(addr, size, pgprot_writethrough(PAGE_KERNEL));
-}
-EXPORT_SYMBOL(ioremap_wt);
-
-void iounmap(volatile void __iomem *addr)
-{
-	if (static_branch_likely(&have_mio))
-		vunmap((__force void *) ((unsigned long) addr & PAGE_MASK));
+	if (!static_branch_likely(&have_mio))
+		return -EINVAL;
+	return 0;
 }
-EXPORT_SYMBOL(iounmap);
 
 /* Create a virtual mapping cookie for a PCI BAR */
 static void __iomem *pci_iomap_range_fh(struct pci_dev *pdev, int bar,
-- 
2.34.1

Re: [PATCH v2 09/11] s390: mm: Convert to GENERIC_IOREMAP

[Christoph Hellwig]

> +void __iomem *
> +arch_ioremap(phys_addr_t *paddr, size_t size, unsigned long *prot_val)
>  {
>  	if (!static_branch_unlikely(&have_mio))
> +		return (void __iomem *) *paddr;
> +	return NULL;

This logic isn't new in the patch, but it could really use a comment
as it is rather non-obvious.

Re: [PATCH v2 09/11] s390: mm: Convert to GENERIC_IOREMAP

[Niklas Schnelle]

On Sun, 2022-08-21 at 00:05 -0700, Christoph Hellwig wrote:
> > +void __iomem *
> > +arch_ioremap(phys_addr_t *paddr, size_t size, unsigned long *prot_val)
> >  {
> >  	if (!static_branch_unlikely(&have_mio))
> > +		return (void __iomem *) *paddr;
> > +	return NULL;
> 
> This logic isn't new in the patch, but it could really use a comment
> as it is rather non-obvious.

Yes, makes sense. Basically we fake MMIO addresses because the s390
architecture doesn't have MMIO as a concept. That is until the PCI MIO
instructions introduced pseudo-MMIO though only for specific PCI
load/store instructions. Without those PCI BAR spaces as well as config
space is accessed with so called function handles. As these are a bad
fit for Linux' MMIO based APIs we create fake MMIO addresses (called
address cookies) that encode an index into the zpci_iomap_start[] which
can be decoded by our implementation of ioread*/iowrite*().

I don't think this is the right place to describe this overall scheme
in detail but maybe we can leave a a good bread crumb. Maybe something
like below?

/* 
 * When PCI MIO instructions are unavailable the "physical" address encodes
 * a hint for accessing the PCI memory space it represents. Just pass it 
 * unchanged such that ioread/iowrite can decode it.
 */

Re: [PATCH v2 09/11] s390: mm: Convert to GENERIC_IOREMAP

[Baoquan He]

On 08/22/22 at 05:08pm, Niklas Schnelle wrote:
> On Sun, 2022-08-21 at 00:05 -0700, Christoph Hellwig wrote:
> > > +void __iomem *
> > > +arch_ioremap(phys_addr_t *paddr, size_t size, unsigned long *prot_val)
> > >  {
> > >  	if (!static_branch_unlikely(&have_mio))
> > > +		return (void __iomem *) *paddr;
> > > +	return NULL;
> > 
> > This logic isn't new in the patch, but it could really use a comment
> > as it is rather non-obvious.
> 
> Yes, makes sense. Basically we fake MMIO addresses because the s390
> architecture doesn't have MMIO as a concept. That is until the PCI MIO
> instructions introduced pseudo-MMIO though only for specific PCI
> load/store instructions. Without those PCI BAR spaces as well as config
> space is accessed with so called function handles. As these are a bad
> fit for Linux' MMIO based APIs we create fake MMIO addresses (called
> address cookies) that encode an index into the zpci_iomap_start[] which
> can be decoded by our implementation of ioread*/iowrite*().
> 
> I don't think this is the right place to describe this overall scheme
> in detail but maybe we can leave a a good bread crumb. Maybe something
> like below?
> 
> /* 
>  * When PCI MIO instructions are unavailable the "physical" address encodes
>  * a hint for accessing the PCI memory space it represents. Just pass it 
>  * unchanged such that ioread/iowrite can decode it.
>  */

Thanks. Looks good to me, I will add these to above the code.

> 

Re: [PATCH v2 09/11] s390: mm: Convert to GENERIC_IOREMAP

[Niklas Schnelle]

On Sat, 2022-08-20 at 08:31 +0800, Baoquan He wrote:
> Add hooks arch_ioremap() and arch_iounmap() for s390's special
> operation when ioremap() and iounmap(), then ioremap_[wc|wt]() are
> converted to use ioremap_prot() from GENERIC_IOREMAP.
> 
> Signed-off-by: Baoquan He <bhe@redhat.com>
> Cc: Heiko Carstens <hca@linux.ibm.com>
> Cc: Vasily Gorbik <gor@linux.ibm.com>
> Cc: Alexander Gordeev <agordeev@linux.ibm.com>
> Cc: Christian Borntraeger <borntraeger@linux.ibm.com>
> Cc: Sven Schnelle <svens@linux.ibm.com>
> Cc: linux-s390@vger.kernel.org
> ---
>  arch/s390/Kconfig          |  1 +
>  arch/s390/include/asm/io.h | 26 +++++++++++------
>  arch/s390/pci/pci.c        | 60 +++++---------------------------------
>  3 files changed, 26 insertions(+), 61 deletions(-)

Sorry I missed this mail until now and will still need a bit of time to
review and test the code as this is indeed pretty special on s390. From
a first glance this does look like a nice simplification.

Just out of curiosity, I wonder why get_maintainers.pl didn't add me
nor Gerald for direct CC despite the bulk of the changes affecting
arch/s390/pci/*.

> 
> diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
---8<---

Re: [PATCH v2 09/11] s390: mm: Convert to GENERIC_IOREMAP

[Baoquan He]

On 08/22/22 at 05:19pm, Niklas Schnelle wrote:
> On Sat, 2022-08-20 at 08:31 +0800, Baoquan He wrote:
> > Add hooks arch_ioremap() and arch_iounmap() for s390's special
> > operation when ioremap() and iounmap(), then ioremap_[wc|wt]() are
> > converted to use ioremap_prot() from GENERIC_IOREMAP.
> > 
> > Signed-off-by: Baoquan He <bhe@redhat.com>
> > Cc: Heiko Carstens <hca@linux.ibm.com>
> > Cc: Vasily Gorbik <gor@linux.ibm.com>
> > Cc: Alexander Gordeev <agordeev@linux.ibm.com>
> > Cc: Christian Borntraeger <borntraeger@linux.ibm.com>
> > Cc: Sven Schnelle <svens@linux.ibm.com>
> > Cc: linux-s390@vger.kernel.org
> > ---
> >  arch/s390/Kconfig          |  1 +
> >  arch/s390/include/asm/io.h | 26 +++++++++++------
> >  arch/s390/pci/pci.c        | 60 +++++---------------------------------
> >  3 files changed, 26 insertions(+), 61 deletions(-)
> 
> Sorry I missed this mail until now and will still need a bit of time to
> review and test the code as this is indeed pretty special on s390. From
> a first glance this does look like a nice simplification.
> 
> Just out of curiosity, I wonder why get_maintainers.pl didn't add me
> nor Gerald for direct CC despite the bulk of the changes affecting
> arch/s390/pci/*.

I run below script again, it does print you and Gerald's name in the
list. It must be me who made mistake when copying maintainers' name into
Cc. Sorry about that.
./scripts/get_maintainer.pl 0009-s390-mm-Convert-to-GENERIC_IOREMAP.patch

> 
> > 
> > diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
> ---8<---
> 

Re: [PATCH v2 09/11] s390: mm: Convert to GENERIC_IOREMAP

[Niklas Schnelle]

On Sat, 2022-08-20 at 08:31 +0800, Baoquan He wrote:
> Add hooks arch_ioremap() and arch_iounmap() for s390's special
> operation when ioremap() and iounmap(), then ioremap_[wc|wt]() are
> converted to use ioremap_prot() from GENERIC_IOREMAP.
> 
> Signed-off-by: Baoquan He <bhe@redhat.com>
> Cc: Heiko Carstens <hca@linux.ibm.com>
> Cc: Vasily Gorbik <gor@linux.ibm.com>
> Cc: Alexander Gordeev <agordeev@linux.ibm.com>
> Cc: Christian Borntraeger <borntraeger@linux.ibm.com>
> Cc: Sven Schnelle <svens@linux.ibm.com>
> Cc: linux-s390@vger.kernel.org
> ---
>  arch/s390/Kconfig          |  1 +
>  arch/s390/include/asm/io.h | 26 +++++++++++------
>  arch/s390/pci/pci.c        | 60 +++++---------------------------------
>  3 files changed, 26 insertions(+), 61 deletions(-)
> 
> diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
> index 318fce77601d..c59e1b25f59d 100644
> --- a/arch/s390/Kconfig
> +++ b/arch/s390/Kconfig
> @@ -135,6 +135,7 @@ config S390
>  	select GENERIC_SMP_IDLE_THREAD
>  	select GENERIC_TIME_VSYSCALL
>  	select GENERIC_VDSO_TIME_NS
> +	select GENERIC_IOREMAP
>  	select HAVE_ALIGNED_STRUCT_PAGE if SLUB
>  	select HAVE_ARCH_AUDITSYSCALL
>  	select HAVE_ARCH_JUMP_LABEL
> diff --git a/arch/s390/include/asm/io.h b/arch/s390/include/asm/io.h
> index e3882b012bfa..f837e20b7bbd 100644
> --- a/arch/s390/include/asm/io.h
> +++ b/arch/s390/include/asm/io.h
> @@ -22,11 +22,23 @@ void unxlate_dev_mem_ptr(phys_addr_t phys, void *addr);
>  
>  #define IO_SPACE_LIMIT 0
>  
> -void __iomem *ioremap_prot(phys_addr_t addr, size_t size, unsigned long prot);
> -void __iomem *ioremap(phys_addr_t addr, size_t size);
> -void __iomem *ioremap_wc(phys_addr_t addr, size_t size);
> -void __iomem *ioremap_wt(phys_addr_t addr, size_t size);
> -void iounmap(volatile void __iomem *addr);
> +

Checkpatch nitpick, remove the empty line addition above so as not to
create two consecutive empty lines.

> +/*
> + * I/O memory mapping functions.
> + */
> +void __iomem *
> +arch_ioremap(phys_addr_t *paddr, size_t size, unsigned long *prot_val);
> +#define arch_ioremap arch_ioremap
> +
> +int arch_iounmap(void __iomem *addr);
> +#define arch_iounmap arch_iounmap
> +
> +#define _PAGE_IOREMAP pgprot_val(PAGE_KERNEL)
> +
> +#define ioremap_wc(addr, size)  \
> +	ioremap_prot((addr), (size), pgprot_val(pgprot_writecombine(PAGE_KERNEL)))
> +#define ioremap_wt(addr, size)  \
> +	ioremap_prot((addr), (size), pgprot_val(pgprot_writethrough(PAGE_KERNEL)))
>  
>  static inline void __iomem *ioport_map(unsigned long port, unsigned int nr)
>  {
> @@ -51,10 +63,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 ioremap ioremap
> -#define ioremap_wt ioremap_wt
> -#define ioremap_wc ioremap_wc
> -
>  #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)
> diff --git a/arch/s390/pci/pci.c b/arch/s390/pci/pci.c
> index 73cdc5539384..984cad9cd5a1 100644
> --- a/arch/s390/pci/pci.c
> +++ b/arch/s390/pci/pci.c
> @@ -244,64 +244,20 @@ void __iowrite64_copy(void __iomem *to, const void *from, size_t count)
>         zpci_memcpy_toio(to, from, count);
>  }
>  
> -static void __iomem *__ioremap(phys_addr_t addr, size_t size, pgprot_t prot)
> +void __iomem *
> +arch_ioremap(phys_addr_t *paddr, size_t size, unsigned long *prot_val)
>  {
> -	unsigned long offset, vaddr;
> -	struct vm_struct *area;
> -	phys_addr_t last_addr;
> -
> -	last_addr = addr + size - 1;
> -	if (!size || last_addr < addr)
> -		return NULL;
> -
>  	if (!static_branch_unlikely(&have_mio))
> -		return (void __iomem *) addr;
> -
> -	offset = addr & ~PAGE_MASK;
> -	addr &= PAGE_MASK;
> -	size = PAGE_ALIGN(size + offset);
> -	area = get_vm_area(size, VM_IOREMAP);
> -	if (!area)
> -		return NULL;
> -
> -	vaddr = (unsigned long) area->addr;
> -	if (ioremap_page_range(vaddr, vaddr + size, addr, prot)) {
> -		free_vm_area(area);
> -		return NULL;
> -	}
> -	return (void __iomem *) ((unsigned long) area->addr + offset);
> -}
> -
> -void __iomem *ioremap_prot(phys_addr_t addr, size_t size, unsigned long prot)
> -{
> -	return __ioremap(addr, size, __pgprot(prot));
> +		return (void __iomem *) *paddr;

Another checkpatch nitpick no space after the cast.

> +	return NULL;
>  }
> -EXPORT_SYMBOL(ioremap_prot);
>  
> -void __iomem *ioremap(phys_addr_t addr, size_t size)
> +int arch_iounmap(void __iomem *addr)
>  {
> -	return __ioremap(addr, size, PAGE_KERNEL);
> -}
> -EXPORT_SYMBOL(ioremap);
> -
> -void __iomem *ioremap_wc(phys_addr_t addr, size_t size)
> -{
> -	return __ioremap(addr, size, pgprot_writecombine(PAGE_KERNEL));
> -}
> -EXPORT_SYMBOL(ioremap_wc);
> -
> -void __iomem *ioremap_wt(phys_addr_t addr, size_t size)
> -{
> -	return __ioremap(addr, size, pgprot_writethrough(PAGE_KERNEL));
> -}
> -EXPORT_SYMBOL(ioremap_wt);
> -
> -void iounmap(volatile void __iomem *addr)
> -{
> -	if (static_branch_likely(&have_mio))
> -		vunmap((__force void *) ((unsigned long) addr & PAGE_MASK));
> +	if (!static_branch_likely(&have_mio))
> +		return -EINVAL;

As Christoph suggested this might be a good opportunity to add a
comment for this branch.

One other nitpick. The return value doesn't really matter here since
anything != NULL turns iounmap() into a no-op so this looks correct but
semantically I think returning -EINVAL wrongly suggests that addr was
invalid. Maybe -ENXIO would be better at conveying that there is
nothing to unmap.

Looking at your patch 1 another idea would be to have 3 kinds of return
values for arch_iounmap() too e.g.:

 arch_iounmap() return an __iomem pointer
   - IS_ERR means skip vunmap and return directly
   - NULL means continue to vunmap
   - a non-NULL, non-IS_ERR pointer has been unmapped successfully

Then we would simply return addr in case of
!static_branch_likely(&have_mio) and NULL otherwise.

What do you think? Either way no strong opinion on my side,
functionally it makes no difference.

> +	return 0;
>  }
> -EXPORT_SYMBOL(iounmap);
>  
>  /* Create a virtual mapping cookie for a PCI BAR */
>  static void __iomem *pci_iomap_range_fh(struct pci_dev *pdev, int bar,

Apart from the above nitpicks and suggestion this looks good to me.
I did also test this with and without PCI MIO support including use of
PCI MIO instructions in user-space (added in rdma-core v40).

So feel free to add:
Tested-by: Niklas Schnelle <schnelle@linux.ibm.com>
Acked-by: Niklas Schnelle <schnelle@linux.ibm.com>

Since it looks like there will be a v3 due to other comments anyway
please Cc me on that directly and I'm sure I can upgrade the Acked-by
to Reviewed-by when we're closer to the final code.

Re: [PATCH v2 09/11] s390: mm: Convert to GENERIC_IOREMAP

[Baoquan He]

Hi Niklas,

On 08/23/22 at 02:30pm, Niklas Schnelle wrote:
> On Sat, 2022-08-20 at 08:31 +0800, Baoquan He wrote:
> > Add hooks arch_ioremap() and arch_iounmap() for s390's special
> > operation when ioremap() and iounmap(), then ioremap_[wc|wt]() are
> > converted to use ioremap_prot() from GENERIC_IOREMAP.
> > 
> > Signed-off-by: Baoquan He <bhe@redhat.com>
> > Cc: Heiko Carstens <hca@linux.ibm.com>
> > Cc: Vasily Gorbik <gor@linux.ibm.com>
> > Cc: Alexander Gordeev <agordeev@linux.ibm.com>
> > Cc: Christian Borntraeger <borntraeger@linux.ibm.com>
> > Cc: Sven Schnelle <svens@linux.ibm.com>
> > Cc: linux-s390@vger.kernel.org
> > ---
> >  arch/s390/Kconfig          |  1 +
> >  arch/s390/include/asm/io.h | 26 +++++++++++------
> >  arch/s390/pci/pci.c        | 60 +++++---------------------------------
> >  3 files changed, 26 insertions(+), 61 deletions(-)
> > 
> > diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
> > index 318fce77601d..c59e1b25f59d 100644
> > --- a/arch/s390/Kconfig
> > +++ b/arch/s390/Kconfig
> > @@ -135,6 +135,7 @@ config S390
> >  	select GENERIC_SMP_IDLE_THREAD
> >  	select GENERIC_TIME_VSYSCALL
> >  	select GENERIC_VDSO_TIME_NS
> > +	select GENERIC_IOREMAP
> >  	select HAVE_ALIGNED_STRUCT_PAGE if SLUB
> >  	select HAVE_ARCH_AUDITSYSCALL
> >  	select HAVE_ARCH_JUMP_LABEL
> > diff --git a/arch/s390/include/asm/io.h b/arch/s390/include/asm/io.h
> > index e3882b012bfa..f837e20b7bbd 100644
> > --- a/arch/s390/include/asm/io.h
> > +++ b/arch/s390/include/asm/io.h
> > @@ -22,11 +22,23 @@ void unxlate_dev_mem_ptr(phys_addr_t phys, void *addr);
> >  
> >  #define IO_SPACE_LIMIT 0
> >  
> > -void __iomem *ioremap_prot(phys_addr_t addr, size_t size, unsigned long prot);
> > -void __iomem *ioremap(phys_addr_t addr, size_t size);
> > -void __iomem *ioremap_wc(phys_addr_t addr, size_t size);
> > -void __iomem *ioremap_wt(phys_addr_t addr, size_t size);
> > -void iounmap(volatile void __iomem *addr);
> > +
> 

Thanks a lot for reviewing, and sorry for late response.

> Checkpatch nitpick, remove the empty line addition above so as not to
> create two consecutive empty lines.

Will do, thanks.

> 
> > +/*
> > + * I/O memory mapping functions.
> > + */
> > +void __iomem *
> > +arch_ioremap(phys_addr_t *paddr, size_t size, unsigned long *prot_val);
> > +#define arch_ioremap arch_ioremap
> > +
> > +int arch_iounmap(void __iomem *addr);
> > +#define arch_iounmap arch_iounmap
> > +
> > +#define _PAGE_IOREMAP pgprot_val(PAGE_KERNEL)
> > +
> > +#define ioremap_wc(addr, size)  \
> > +	ioremap_prot((addr), (size), pgprot_val(pgprot_writecombine(PAGE_KERNEL)))
> > +#define ioremap_wt(addr, size)  \
> > +	ioremap_prot((addr), (size), pgprot_val(pgprot_writethrough(PAGE_KERNEL)))
> >  
> >  static inline void __iomem *ioport_map(unsigned long port, unsigned int nr)
> >  {
> > @@ -51,10 +63,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 ioremap ioremap
> > -#define ioremap_wt ioremap_wt
> > -#define ioremap_wc ioremap_wc
> > -
> >  #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)
> > diff --git a/arch/s390/pci/pci.c b/arch/s390/pci/pci.c
> > index 73cdc5539384..984cad9cd5a1 100644
> > --- a/arch/s390/pci/pci.c
> > +++ b/arch/s390/pci/pci.c
> > @@ -244,64 +244,20 @@ void __iowrite64_copy(void __iomem *to, const void *from, size_t count)
> >         zpci_memcpy_toio(to, from, count);
> >  }
> >  
> > -static void __iomem *__ioremap(phys_addr_t addr, size_t size, pgprot_t prot)
> > +void __iomem *
> > +arch_ioremap(phys_addr_t *paddr, size_t size, unsigned long *prot_val)
> >  {
> > -	unsigned long offset, vaddr;
> > -	struct vm_struct *area;
> > -	phys_addr_t last_addr;
> > -
> > -	last_addr = addr + size - 1;
> > -	if (!size || last_addr < addr)
> > -		return NULL;
> > -
> >  	if (!static_branch_unlikely(&have_mio))
> > -		return (void __iomem *) addr;
> > -
> > -	offset = addr & ~PAGE_MASK;
> > -	addr &= PAGE_MASK;
> > -	size = PAGE_ALIGN(size + offset);
> > -	area = get_vm_area(size, VM_IOREMAP);
> > -	if (!area)
> > -		return NULL;
> > -
> > -	vaddr = (unsigned long) area->addr;
> > -	if (ioremap_page_range(vaddr, vaddr + size, addr, prot)) {
> > -		free_vm_area(area);
> > -		return NULL;
> > -	}
> > -	return (void __iomem *) ((unsigned long) area->addr + offset);
> > -}
> > -
> > -void __iomem *ioremap_prot(phys_addr_t addr, size_t size, unsigned long prot)
> > -{
> > -	return __ioremap(addr, size, __pgprot(prot));
> > +		return (void __iomem *) *paddr;
> 
> Another checkpatch nitpick no space after the cast.

Will fix.

> 
> > +	return NULL;
> >  }
> > -EXPORT_SYMBOL(ioremap_prot);
> >  
> > -void __iomem *ioremap(phys_addr_t addr, size_t size)
> > +int arch_iounmap(void __iomem *addr)
> >  {
> > -	return __ioremap(addr, size, PAGE_KERNEL);
> > -}
> > -EXPORT_SYMBOL(ioremap);
> > -
> > -void __iomem *ioremap_wc(phys_addr_t addr, size_t size)
> > -{
> > -	return __ioremap(addr, size, pgprot_writecombine(PAGE_KERNEL));
> > -}
> > -EXPORT_SYMBOL(ioremap_wc);
> > -
> > -void __iomem *ioremap_wt(phys_addr_t addr, size_t size)
> > -{
> > -	return __ioremap(addr, size, pgprot_writethrough(PAGE_KERNEL));
> > -}
> > -EXPORT_SYMBOL(ioremap_wt);
> > -
> > -void iounmap(volatile void __iomem *addr)
> > -{
> > -	if (static_branch_likely(&have_mio))
> > -		vunmap((__force void *) ((unsigned long) addr & PAGE_MASK));
> > +	if (!static_branch_likely(&have_mio))
> > +		return -EINVAL;
> 
> As Christoph suggested this might be a good opportunity to add a
> comment for this branch.
> 
> One other nitpick. The return value doesn't really matter here since
> anything != NULL turns iounmap() into a no-op so this looks correct but
> semantically I think returning -EINVAL wrongly suggests that addr was
> invalid. Maybe -ENXIO would be better at conveying that there is
> nothing to unmap.
> 
> Looking at your patch 1 another idea would be to have 3 kinds of return
> values for arch_iounmap() too e.g.:
> 
>  arch_iounmap() return an __iomem pointer
>    - IS_ERR means skip vunmap and return directly
>    - NULL means continue to vunmap
>    - a non-NULL, non-IS_ERR pointer has been unmapped successfully

I agree with you, The returned -EINVAL does cause confusion.

I checked all ARCHes related in this patchset, we only do the arch
specific address filtering (or address checking), then call vunmap().
Seems no error case or error handling in iounmap(). I'm thinking if
we should make the return value as boolean value. Ture indicates
address checking passed, false indicates not passed.


> 
> Then we would simply return addr in case of
> !static_branch_likely(&have_mio) and NULL otherwise.
> 
> What do you think? Either way no strong opinion on my side,
> functionally it makes no difference.
> 
> > +	return 0;
> >  }
> > -EXPORT_SYMBOL(iounmap);
> >  
> >  /* Create a virtual mapping cookie for a PCI BAR */
> >  static void __iomem *pci_iomap_range_fh(struct pci_dev *pdev, int bar,
> 
> Apart from the above nitpicks and suggestion this looks good to me.
> I did also test this with and without PCI MIO support including use of
> PCI MIO instructions in user-space (added in rdma-core v40).
> 
> So feel free to add:
> Tested-by: Niklas Schnelle <schnelle@linux.ibm.com>
> Acked-by: Niklas Schnelle <schnelle@linux.ibm.com>
> 
> Since it looks like there will be a v3 due to other comments anyway
> please Cc me on that directly and I'm sure I can upgrade the Acked-by
> to Reviewed-by when we're closer to the final code.

Sure, I will add you in Cc when repost. Thank again for these details.


>