[PATCH 0/2] Fix pci_iounmap() on !CONFIG_GENERIC_IOMAP

[PATCH 0/2] Fix pci_iounmap() on !CONFIG_GENERIC_IOMAP

[Lorenzo Pieralisi]

Fix the empty pci_iounmap() implementation that is causing memory leaks on
!CONFIG_GENERIC_IOMAP configs relying on asm-generic/io.h.

A small tweak is required on sparc32 to pull in some declarations,
hopefully nothing problematic, subject to changes as requested.

Previous tentatives:
https://lore.kernel.org/lkml/20200905024811.74701-1-yangyingliang@huawei.com
https://lore.kernel.org/lkml/20200824132046.3114383-1-george.cherian@marvell.com

Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: George Cherian <george.cherian@marvell.com>
Cc: Yang Yingliang <yangyingliang@huawei.com>

Lorenzo Pieralisi (2):
  sparc32: Move ioremap/iounmap declaration before asm-generic/io.h
    include
  asm-generic/io.h: Fix !CONFIG_GENERIC_IOMAP pci_iounmap()
    implementation

 arch/sparc/include/asm/io_32.h | 16 ++++++++------
 include/asm-generic/io.h       | 39 +++++++++++++++++++++++-----------
 2 files changed, 37 insertions(+), 18 deletions(-)

-- 
2.26.1

[PATCH 1/2] sparc32: Move ioremap/iounmap declaration before asm-generic/io.h include

[Lorenzo Pieralisi]

Move the ioremap/iounmap declaration before asm-generic/io.h is
included so that it is visible within it.

Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: "David S. Miller" <davem@davemloft.net>
---
 arch/sparc/include/asm/io_32.h | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/arch/sparc/include/asm/io_32.h b/arch/sparc/include/asm/io_32.h
index 9a52d9506f80..042201c79ad1 100644
--- a/arch/sparc/include/asm/io_32.h
+++ b/arch/sparc/include/asm/io_32.h
@@ -11,6 +11,16 @@
 #define memcpy_fromio(d,s,sz) _memcpy_fromio(d,s,sz)
 #define memcpy_toio(d,s,sz)   _memcpy_toio(d,s,sz)
 
+#ifdef __KERNEL__
+
+/*
+ * Bus number may be embedded in the higher bits of the physical address.
+ * This is why we have no bus number argument to ioremap().
+ */
+void __iomem *ioremap(phys_addr_t offset, size_t size);
+void iounmap(volatile void __iomem *addr);
+#endif
+
 #include <asm-generic/io.h>
 
 static inline void _memset_io(volatile void __iomem *dst,
@@ -123,12 +133,6 @@ static inline void sbus_memcpy_toio(volatile void __iomem *dst,
 
 #ifdef __KERNEL__
 
-/*
- * Bus number may be embedded in the higher bits of the physical address.
- * This is why we have no bus number argument to ioremap().
- */
-void __iomem *ioremap(phys_addr_t offset, size_t size);
-void iounmap(volatile void __iomem *addr);
 /* Create a virtual mapping cookie for an IO port range */
 void __iomem *ioport_map(unsigned long port, unsigned int nr);
 void ioport_unmap(void __iomem *);
-- 
2.26.1

Re: [PATCH 1/2] sparc32: Move ioremap/iounmap declaration before asm-generic/io.h include

[Christoph Hellwig]

> diff --git a/arch/sparc/include/asm/io_32.h b/arch/sparc/include/asm/io_32.h
> index 9a52d9506f80..042201c79ad1 100644
> --- a/arch/sparc/include/asm/io_32.h
> +++ b/arch/sparc/include/asm/io_32.h
> @@ -11,6 +11,16 @@
>  #define memcpy_fromio(d,s,sz) _memcpy_fromio(d,s,sz)
>  #define memcpy_toio(d,s,sz)   _memcpy_toio(d,s,sz)
>  
> +#ifdef __KERNEL__
> +
> +/*
> + * Bus number may be embedded in the higher bits of the physical address.
> + * This is why we have no bus number argument to ioremap().
> + */
> +void __iomem *ioremap(phys_addr_t offset, size_t size);
> +void iounmap(volatile void __iomem *addr);
> +#endif

No need for an __KERNEL__ in non-uapi headers.

Re: [PATCH 1/2] sparc32: Move ioremap/iounmap declaration before asm-generic/io.h include

[Lorenzo Pieralisi]

On Tue, Sep 15, 2020 at 03:14:52PM +0100, Christoph Hellwig wrote:
> > diff --git a/arch/sparc/include/asm/io_32.h b/arch/sparc/include/asm/io_32.h
> > index 9a52d9506f80..042201c79ad1 100644
> > --- a/arch/sparc/include/asm/io_32.h
> > +++ b/arch/sparc/include/asm/io_32.h
> > @@ -11,6 +11,16 @@
> >  #define memcpy_fromio(d,s,sz) _memcpy_fromio(d,s,sz)
> >  #define memcpy_toio(d,s,sz)   _memcpy_toio(d,s,sz)
> >  
> > +#ifdef __KERNEL__
> > +
> > +/*
> > + * Bus number may be embedded in the higher bits of the physical address.
> > + * This is why we have no bus number argument to ioremap().
> > + */
> > +void __iomem *ioremap(phys_addr_t offset, size_t size);
> > +void iounmap(volatile void __iomem *addr);
> > +#endif
> 
> No need for an __KERNEL__ in non-uapi headers.

Sure, just kept the same preproc guard as current code, will add a patch
to remove the guard first before this one then.

Thanks,
Lorenzo

Re: [PATCH 1/2] sparc32: Move ioremap/iounmap declaration before asm-generic/io.h include

[David Miller]

From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Date: Tue, 15 Sep 2020 10:32:02 +0100

> Move the ioremap/iounmap declaration before asm-generic/io.h is
> included so that it is visible within it.
> 
> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>

Acked-by: David S. Miller <davem@davemloft.net>

Re: [PATCH 1/2] sparc32: Move ioremap/iounmap declaration before asm-generic/io.h include

[Lorenzo Pieralisi]

On Tue, Sep 15, 2020 at 01:11:21PM -0700, David Miller wrote:
> From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Date: Tue, 15 Sep 2020 10:32:02 +0100
> 
> > Move the ioremap/iounmap declaration before asm-generic/io.h is
> > included so that it is visible within it.
> > 
> > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> 
> Acked-by: David S. Miller <davem@davemloft.net>

Hi David,

can I apply your Acked-by to v2 (where I had to split this patch in 2):

https://lore.kernel.org/lkml/cover.1600254147.git.lorenzo.pieralisi@arm.com
I am about to merge it - please let me know.

Thanks,
Lorenzo

Re: [PATCH 1/2] sparc32: Move ioremap/iounmap declaration before asm-generic/io.h include

[David Miller]

From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Date: Fri, 2 Oct 2020 15:50:29 +0100

> On Tue, Sep 15, 2020 at 01:11:21PM -0700, David Miller wrote:
>> From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
>> Date: Tue, 15 Sep 2020 10:32:02 +0100
>> 
>> > Move the ioremap/iounmap declaration before asm-generic/io.h is
>> > included so that it is visible within it.
>> > 
>> > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
>> 
>> Acked-by: David S. Miller <davem@davemloft.net>
> 
> Hi David,
> 
> can I apply your Acked-by to v2 (where I had to split this patch in 2):
> 
> https://lore.kernel.org/lkml/cover.1600254147.git.lorenzo.pieralisi@arm.com
> I am about to merge it - please let me know.

Yes, you can.

[PATCH 2/2] asm-generic/io.h: Fix !CONFIG_GENERIC_IOMAP pci_iounmap() implementation

[Lorenzo Pieralisi]

For arches that do not select CONFIG_GENERIC_IOMAP, the current
pci_iounmap() function does nothing causing obvious memory leaks
for mapped regions that are backed by MMIO physical space.

In order to detect if a mapped pointer is IO vs MMIO, a check must made
available to the pci_iounmap() function so that it can actually detect
whether the pointer has to be unmapped.

In configurations where CONFIG_HAS_IOPORT_MAP && !CONFIG_GENERIC_IOMAP,
a mapped port is detected using an ioport_map() stub defined in
asm-generic/io.h.

Use the same logic to implement a stub (ie __pci_ioport_unmap()) that
detects if the passed in pointer in pci_iounmap() is IO vs MMIO to
iounmap conditionally and call it in pci_iounmap() fixing the issue.

Leave __pci_ioport_unmap() as a NOP for all other config options.

Reported-by: George Cherian <george.cherian@marvell.com>
Link: https://lore.kernel.org/lkml/20200905024811.74701-1-yangyingliang@huawei.com
Link: https://lore.kernel.org/lkml/20200824132046.3114383-1-george.cherian@marvell.com
Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: George Cherian <george.cherian@marvell.com>
Cc: Will Deacon <will@kernel.org>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Yang Yingliang <yangyingliang@huawei.com>
---
 include/asm-generic/io.h | 39 +++++++++++++++++++++++++++------------
 1 file changed, 27 insertions(+), 12 deletions(-)

diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h
index dabf8cb7203b..9ea83d80eb6f 100644
--- a/include/asm-generic/io.h
+++ b/include/asm-generic/io.h
@@ -911,18 +911,6 @@ static inline void iowrite64_rep(volatile void __iomem *addr,
 #include <linux/vmalloc.h>
 #define __io_virt(x) ((void __force *)(x))
 
-#ifndef CONFIG_GENERIC_IOMAP
-struct pci_dev;
-extern void __iomem *pci_iomap(struct pci_dev *dev, int bar, unsigned long max);
-
-#ifndef pci_iounmap
-#define pci_iounmap pci_iounmap
-static inline void pci_iounmap(struct pci_dev *dev, void __iomem *p)
-{
-}
-#endif
-#endif /* CONFIG_GENERIC_IOMAP */
-
 /*
  * Change virtual addresses to physical addresses and vv.
  * These are pretty trivial
@@ -1016,6 +1004,16 @@ static inline void __iomem *ioport_map(unsigned long port, unsigned int nr)
 	port &= IO_SPACE_LIMIT;
 	return (port > MMIO_UPPER_LIMIT) ? NULL : PCI_IOBASE + port;
 }
+#define __pci_ioport_unmap __pci_ioport_unmap
+static inline void __pci_ioport_unmap(void __iomem *p)
+{
+	uintptr_t start = (uintptr_t) PCI_IOBASE;
+	uintptr_t addr = (uintptr_t) p;
+
+	if (addr >= start && addr < start + IO_SPACE_LIMIT)
+		return;
+	iounmap(p);
+}
 #endif
 
 #ifndef ioport_unmap
@@ -1030,6 +1028,23 @@ extern void ioport_unmap(void __iomem *p);
 #endif /* CONFIG_GENERIC_IOMAP */
 #endif /* CONFIG_HAS_IOPORT_MAP */
 
+#ifndef CONFIG_GENERIC_IOMAP
+struct pci_dev;
+extern void __iomem *pci_iomap(struct pci_dev *dev, int bar, unsigned long max);
+
+#ifndef __pci_ioport_unmap
+static inline void __pci_ioport_unmap(void __iomem *p) {}
+#endif
+
+#ifndef pci_iounmap
+#define pci_iounmap pci_iounmap
+static inline void pci_iounmap(struct pci_dev *dev, void __iomem *p)
+{
+	__pci_ioport_unmap(p);
+}
+#endif
+#endif /* CONFIG_GENERIC_IOMAP */
+
 /*
  * Convert a virtual cached pointer to an uncached pointer
  */
-- 
2.26.1

[PATCH v2 0/3] Fix pci_iounmap() on !CONFIG_GENERIC_IOMAP

[Lorenzo Pieralisi]

v2 of a previous posting.

v1->v2:
	- Added additional patch to remove sparc32 useless __KERNEL__
	  guard

v1: https://lore.kernel.org/lkml/20200915093203.16934-1-lorenzo.pieralisi@arm.com

Original cover letter
---

Fix the empty pci_iounmap() implementation that is causing memory leaks on
!CONFIG_GENERIC_IOMAP configs relying on asm-generic/io.h.

A small tweak is required on sparc32 to pull in some declarations,
hopefully nothing problematic, subject to changes as requested.

Previous tentatives:
https://lore.kernel.org/lkml/20200905024811.74701-1-yangyingliang@huawei.com
https://lore.kernel.org/lkml/20200824132046.3114383-1-george.cherian@marvell.com

Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: George Cherian <george.cherian@marvell.com>
Cc: Yang Yingliang <yangyingliang@huawei.com>

Lorenzo Pieralisi (3):
  sparc32: Remove useless io_32.h __KERNEL__ preprocessor guard
  sparc32: Move ioremap/iounmap declaration before asm-generic/io.h
    include
  asm-generic/io.h: Fix !CONFIG_GENERIC_IOMAP pci_iounmap()
    implementation

 arch/sparc/include/asm/io_32.h | 17 ++++++---------
 include/asm-generic/io.h       | 39 +++++++++++++++++++++++-----------
 2 files changed, 34 insertions(+), 22 deletions(-)

-- 
2.26.1

[PATCH v2 1/3] sparc32: Remove useless io_32.h __KERNEL__ preprocessor guard

[Lorenzo Pieralisi]

The __KERNEL_ preprocessor guard is useless in non-uapi headers.

Remove it.

Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: David S. Miller <davem@davemloft.net>
---
 arch/sparc/include/asm/io_32.h | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/arch/sparc/include/asm/io_32.h b/arch/sparc/include/asm/io_32.h
index 9a52d9506f80..8179958e3ce1 100644
--- a/arch/sparc/include/asm/io_32.h
+++ b/arch/sparc/include/asm/io_32.h
@@ -121,8 +121,6 @@ static inline void sbus_memcpy_toio(volatile void __iomem *dst,
 	}
 }
 
-#ifdef __KERNEL__
-
 /*
  * Bus number may be embedded in the higher bits of the physical address.
  * This is why we have no bus number argument to ioremap().
@@ -148,8 +146,6 @@ static inline int sbus_can_burst64(void)
 struct device;
 void sbus_set_sbus64(struct device *, int);
 
-#endif
-
 #define __ARCH_HAS_NO_PAGE_ZERO_MAPPED		1
 
 
-- 
2.26.1

[PATCH v2 2/3] sparc32: Move ioremap/iounmap declaration before asm-generic/io.h include

[Lorenzo Pieralisi]

Move the ioremap/iounmap declaration before asm-generic/io.h is
included so that it is visible within it.

Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: "David S. Miller" <davem@davemloft.net>
---
 arch/sparc/include/asm/io_32.h | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/arch/sparc/include/asm/io_32.h b/arch/sparc/include/asm/io_32.h
index 8179958e3ce1..549f0a72280d 100644
--- a/arch/sparc/include/asm/io_32.h
+++ b/arch/sparc/include/asm/io_32.h
@@ -11,6 +11,13 @@
 #define memcpy_fromio(d,s,sz) _memcpy_fromio(d,s,sz)
 #define memcpy_toio(d,s,sz)   _memcpy_toio(d,s,sz)
 
+/*
+ * Bus number may be embedded in the higher bits of the physical address.
+ * This is why we have no bus number argument to ioremap().
+ */
+void __iomem *ioremap(phys_addr_t offset, size_t size);
+void iounmap(volatile void __iomem *addr);
+
 #include <asm-generic/io.h>
 
 static inline void _memset_io(volatile void __iomem *dst,
@@ -121,12 +128,6 @@ static inline void sbus_memcpy_toio(volatile void __iomem *dst,
 	}
 }
 
-/*
- * Bus number may be embedded in the higher bits of the physical address.
- * This is why we have no bus number argument to ioremap().
- */
-void __iomem *ioremap(phys_addr_t offset, size_t size);
-void iounmap(volatile void __iomem *addr);
 /* Create a virtual mapping cookie for an IO port range */
 void __iomem *ioport_map(unsigned long port, unsigned int nr);
 void ioport_unmap(void __iomem *);
-- 
2.26.1

[PATCH v2 3/3] asm-generic/io.h: Fix !CONFIG_GENERIC_IOMAP pci_iounmap() implementation

[Lorenzo Pieralisi]

For arches that do not select CONFIG_GENERIC_IOMAP, the current
pci_iounmap() function does nothing causing obvious memory leaks
for mapped regions that are backed by MMIO physical space.

In order to detect if a mapped pointer is IO vs MMIO, a check must made
available to the pci_iounmap() function so that it can actually detect
whether the pointer has to be unmapped.

In configurations where CONFIG_HAS_IOPORT_MAP && !CONFIG_GENERIC_IOMAP,
a mapped port is detected using an ioport_map() stub defined in
asm-generic/io.h.

Use the same logic to implement a stub (ie __pci_ioport_unmap()) that
detects if the passed in pointer in pci_iounmap() is IO vs MMIO to
iounmap conditionally and call it in pci_iounmap() fixing the issue.

Leave __pci_ioport_unmap() as a NOP for all other config options.

Reported-by: George Cherian <george.cherian@marvell.com>
Link: https://lore.kernel.org/lkml/20200905024811.74701-1-yangyingliang@huawei.com
Link: https://lore.kernel.org/lkml/20200824132046.3114383-1-george.cherian@marvell.com
Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: George Cherian <george.cherian@marvell.com>
Cc: Will Deacon <will@kernel.org>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Yang Yingliang <yangyingliang@huawei.com>
---
 include/asm-generic/io.h | 39 +++++++++++++++++++++++++++------------
 1 file changed, 27 insertions(+), 12 deletions(-)

diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h
index dabf8cb7203b..9ea83d80eb6f 100644
--- a/include/asm-generic/io.h
+++ b/include/asm-generic/io.h
@@ -911,18 +911,6 @@ static inline void iowrite64_rep(volatile void __iomem *addr,
 #include <linux/vmalloc.h>
 #define __io_virt(x) ((void __force *)(x))
 
-#ifndef CONFIG_GENERIC_IOMAP
-struct pci_dev;
-extern void __iomem *pci_iomap(struct pci_dev *dev, int bar, unsigned long max);
-
-#ifndef pci_iounmap
-#define pci_iounmap pci_iounmap
-static inline void pci_iounmap(struct pci_dev *dev, void __iomem *p)
-{
-}
-#endif
-#endif /* CONFIG_GENERIC_IOMAP */
-
 /*
  * Change virtual addresses to physical addresses and vv.
  * These are pretty trivial
@@ -1016,6 +1004,16 @@ static inline void __iomem *ioport_map(unsigned long port, unsigned int nr)
 	port &= IO_SPACE_LIMIT;
 	return (port > MMIO_UPPER_LIMIT) ? NULL : PCI_IOBASE + port;
 }
+#define __pci_ioport_unmap __pci_ioport_unmap
+static inline void __pci_ioport_unmap(void __iomem *p)
+{
+	uintptr_t start = (uintptr_t) PCI_IOBASE;
+	uintptr_t addr = (uintptr_t) p;
+
+	if (addr >= start && addr < start + IO_SPACE_LIMIT)
+		return;
+	iounmap(p);
+}
 #endif
 
 #ifndef ioport_unmap
@@ -1030,6 +1028,23 @@ extern void ioport_unmap(void __iomem *p);
 #endif /* CONFIG_GENERIC_IOMAP */
 #endif /* CONFIG_HAS_IOPORT_MAP */
 
+#ifndef CONFIG_GENERIC_IOMAP
+struct pci_dev;
+extern void __iomem *pci_iomap(struct pci_dev *dev, int bar, unsigned long max);
+
+#ifndef __pci_ioport_unmap
+static inline void __pci_ioport_unmap(void __iomem *p) {}
+#endif
+
+#ifndef pci_iounmap
+#define pci_iounmap pci_iounmap
+static inline void pci_iounmap(struct pci_dev *dev, void __iomem *p)
+{
+	__pci_ioport_unmap(p);
+}
+#endif
+#endif /* CONFIG_GENERIC_IOMAP */
+
 /*
  * Convert a virtual cached pointer to an uncached pointer
  */
-- 
2.26.1

Re: [PATCH v2 3/3] asm-generic/io.h: Fix !CONFIG_GENERIC_IOMAP pci_iounmap() implementation

[Catalin Marinas]

On Wed, Sep 16, 2020 at 12:06:58PM +0100, Lorenzo Pieralisi wrote:
> For arches that do not select CONFIG_GENERIC_IOMAP, the current
> pci_iounmap() function does nothing causing obvious memory leaks
> for mapped regions that are backed by MMIO physical space.
> 
> In order to detect if a mapped pointer is IO vs MMIO, a check must made
> available to the pci_iounmap() function so that it can actually detect
> whether the pointer has to be unmapped.
> 
> In configurations where CONFIG_HAS_IOPORT_MAP && !CONFIG_GENERIC_IOMAP,
> a mapped port is detected using an ioport_map() stub defined in
> asm-generic/io.h.
> 
> Use the same logic to implement a stub (ie __pci_ioport_unmap()) that
> detects if the passed in pointer in pci_iounmap() is IO vs MMIO to
> iounmap conditionally and call it in pci_iounmap() fixing the issue.
> 
> Leave __pci_ioport_unmap() as a NOP for all other config options.
> 
> Reported-by: George Cherian <george.cherian@marvell.com>
> Link: https://lore.kernel.org/lkml/20200905024811.74701-1-yangyingliang@huawei.com
> Link: https://lore.kernel.org/lkml/20200824132046.3114383-1-george.cherian@marvell.com
> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: George Cherian <george.cherian@marvell.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Yang Yingliang <yangyingliang@huawei.com>
> ---
>  include/asm-generic/io.h | 39 +++++++++++++++++++++++++++------------
>  1 file changed, 27 insertions(+), 12 deletions(-)

This works for me. The only question I have is whether pci_iomap.h is
better than io.h for __pci_ioport_unmap(). These headers are really
confusing.

Either way:

Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>

Re: [PATCH v2 3/3] asm-generic/io.h: Fix !CONFIG_GENERIC_IOMAP pci_iounmap() implementation

[Lorenzo Pieralisi]

On Wed, Sep 16, 2020 at 03:51:11PM +0100, Catalin Marinas wrote:
> On Wed, Sep 16, 2020 at 12:06:58PM +0100, Lorenzo Pieralisi wrote:
> > For arches that do not select CONFIG_GENERIC_IOMAP, the current
> > pci_iounmap() function does nothing causing obvious memory leaks
> > for mapped regions that are backed by MMIO physical space.
> > 
> > In order to detect if a mapped pointer is IO vs MMIO, a check must made
> > available to the pci_iounmap() function so that it can actually detect
> > whether the pointer has to be unmapped.
> > 
> > In configurations where CONFIG_HAS_IOPORT_MAP && !CONFIG_GENERIC_IOMAP,
> > a mapped port is detected using an ioport_map() stub defined in
> > asm-generic/io.h.
> > 
> > Use the same logic to implement a stub (ie __pci_ioport_unmap()) that
> > detects if the passed in pointer in pci_iounmap() is IO vs MMIO to
> > iounmap conditionally and call it in pci_iounmap() fixing the issue.
> > 
> > Leave __pci_ioport_unmap() as a NOP for all other config options.
> > 
> > Reported-by: George Cherian <george.cherian@marvell.com>
> > Link: https://lore.kernel.org/lkml/20200905024811.74701-1-yangyingliang@huawei.com
> > Link: https://lore.kernel.org/lkml/20200824132046.3114383-1-george.cherian@marvell.com
> > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > Cc: Arnd Bergmann <arnd@arndb.de>
> > Cc: George Cherian <george.cherian@marvell.com>
> > Cc: Will Deacon <will@kernel.org>
> > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > Cc: Yang Yingliang <yangyingliang@huawei.com>
> > ---
> >  include/asm-generic/io.h | 39 +++++++++++++++++++++++++++------------
> >  1 file changed, 27 insertions(+), 12 deletions(-)
> 
> This works for me. The only question I have is whether pci_iomap.h is
> better than io.h for __pci_ioport_unmap(). These headers are really
> confusing.

Yes they are, in total honesty there is much more to do to make them
sane, this patch is just a band-aid.

I thought about moving this stuff into pci_iomap.h, though that
file is included _independently_ from io.h from some arches so
I tried to keep everything in io.h to minimize disruption.

We can merge this patch - since it is a fix after all - and then I can
try to improve the whole pci_iounmap() includes.

> Either way:
> 
> Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>

Thanks a lot. I'd appreciate a tested-by from the George as he is
the one who reported the problem.

Lorenzo

Re: [PATCH v2 0/3] Fix pci_iounmap() on !CONFIG_GENERIC_IOMAP

[Lorenzo Pieralisi]

On Wed, Sep 16, 2020 at 12:06:55PM +0100, Lorenzo Pieralisi wrote:
> v2 of a previous posting.
> 
> v1->v2:
> 	- Added additional patch to remove sparc32 useless __KERNEL__
> 	  guard
> 
> v1: https://lore.kernel.org/lkml/20200915093203.16934-1-lorenzo.pieralisi@arm.com
> 
> Original cover letter
> ---
> 
> Fix the empty pci_iounmap() implementation that is causing memory leaks on
> !CONFIG_GENERIC_IOMAP configs relying on asm-generic/io.h.
> 
> A small tweak is required on sparc32 to pull in some declarations,
> hopefully nothing problematic, subject to changes as requested.
> 
> Previous tentatives:
> https://lore.kernel.org/lkml/20200905024811.74701-1-yangyingliang@huawei.com
> https://lore.kernel.org/lkml/20200824132046.3114383-1-george.cherian@marvell.com
> 
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: George Cherian <george.cherian@marvell.com>
> Cc: Yang Yingliang <yangyingliang@huawei.com>
> 
> Lorenzo Pieralisi (3):
>   sparc32: Remove useless io_32.h __KERNEL__ preprocessor guard
>   sparc32: Move ioremap/iounmap declaration before asm-generic/io.h
>     include
>   asm-generic/io.h: Fix !CONFIG_GENERIC_IOMAP pci_iounmap()
>     implementation
> 
>  arch/sparc/include/asm/io_32.h | 17 ++++++---------
>  include/asm-generic/io.h       | 39 +++++++++++++++++++++++-----------
>  2 files changed, 34 insertions(+), 22 deletions(-)

Arnd, David, Bjorn,

I have got review/test tags, is it OK if we merge this series please ?

Can we pull it in the PCI tree or you want it to go via a different
route upstream ?

Please let me know.

Thanks,
Lorenzo

Re: [PATCH v2 0/3] Fix pci_iounmap() on !CONFIG_GENERIC_IOMAP

[Arnd Bergmann]

On Fri, Sep 18, 2020 at 1:45 PM Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> wrote:
> >
> > Lorenzo Pieralisi (3):
> >   sparc32: Remove useless io_32.h __KERNEL__ preprocessor guard
> >   sparc32: Move ioremap/iounmap declaration before asm-generic/io.h
> >     include
> >   asm-generic/io.h: Fix !CONFIG_GENERIC_IOMAP pci_iounmap()
> >     implementation
> >
> >  arch/sparc/include/asm/io_32.h | 17 ++++++---------
> >  include/asm-generic/io.h       | 39 +++++++++++++++++++++++-----------
> >  2 files changed, 34 insertions(+), 22 deletions(-)
>
> Arnd, David, Bjorn,
>
> I have got review/test tags, is it OK if we merge this series please ?
>
> Can we pull it in the PCI tree or you want it to go via a different
> route upstream ?
>
> Please let me know.

Going through the PCI tree sounds good to me, but I can
take it through the asm-generic tree if Bjorn doesn't want to
pick it up there.

       Arnd

Re: [PATCH v2 0/3] Fix pci_iounmap() on !CONFIG_GENERIC_IOMAP

[Lorenzo Pieralisi]

On Fri, Sep 18, 2020 at 09:58:51PM +0200, Arnd Bergmann wrote:
> On Fri, Sep 18, 2020 at 1:45 PM Lorenzo Pieralisi
> <lorenzo.pieralisi@arm.com> wrote:
> > >
> > > Lorenzo Pieralisi (3):
> > >   sparc32: Remove useless io_32.h __KERNEL__ preprocessor guard
> > >   sparc32: Move ioremap/iounmap declaration before asm-generic/io.h
> > >     include
> > >   asm-generic/io.h: Fix !CONFIG_GENERIC_IOMAP pci_iounmap()
> > >     implementation
> > >
> > >  arch/sparc/include/asm/io_32.h | 17 ++++++---------
> > >  include/asm-generic/io.h       | 39 +++++++++++++++++++++++-----------
> > >  2 files changed, 34 insertions(+), 22 deletions(-)
> >
> > Arnd, David, Bjorn,
> >
> > I have got review/test tags, is it OK if we merge this series please ?
> >
> > Can we pull it in the PCI tree or you want it to go via a different
> > route upstream ?
> >
> > Please let me know.
> 
> Going through the PCI tree sounds good to me, but I can
> take it through the asm-generic tree if Bjorn doesn't want to
> pick it up there.

Bjorn, can we pull this series into PCI tree please - if that's OK
with you ?

Thanks,
Lorenzo

Re: [PATCH v2 3/3] asm-generic/io.h: Fix !CONFIG_GENERIC_IOMAP pci_iounmap() implementation

[George Cherian]

> -----Original Message-----
> From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Sent: Thursday, September 17, 2020 3:00 PM
> To: Catalin Marinas <catalin.marinas@arm.com>
> Cc: linux-kernel@vger.kernel.org; George Cherian <gcherian@marvell.com>;
> Arnd Bergmann <arnd@arndb.de>; Will Deacon <will@kernel.org>; Bjorn
> Helgaas <bhelgaas@google.com>; Yang Yingliang
> <yangyingliang@huawei.com>; linux-pci@vger.kernel.org; linux-
> arch@vger.kernel.org; linux-arm-kernel@lists.infradead.org; David S. Miller
> <davem@davemloft.net>
> Subject: Re: [PATCH v2 3/3] asm-generic/io.h: Fix
> !CONFIG_GENERIC_IOMAP pci_iounmap() implementation
> 
> 
> ----------------------------------------------------------------------
> On Wed, Sep 16, 2020 at 03:51:11PM +0100, Catalin Marinas wrote:
> > On Wed, Sep 16, 2020 at 12:06:58PM +0100, Lorenzo Pieralisi wrote:
> > > For arches that do not select CONFIG_GENERIC_IOMAP, the current
> > > pci_iounmap() function does nothing causing obvious memory leaks for
> > > mapped regions that are backed by MMIO physical space.
> > >
> > > In order to detect if a mapped pointer is IO vs MMIO, a check must
> > > made available to the pci_iounmap() function so that it can actually
> > > detect whether the pointer has to be unmapped.
> > >
> > > In configurations where CONFIG_HAS_IOPORT_MAP &&
> > > !CONFIG_GENERIC_IOMAP, a mapped port is detected using an
> > > ioport_map() stub defined in asm-generic/io.h.
> > >
> > > Use the same logic to implement a stub (ie __pci_ioport_unmap())
> > > that detects if the passed in pointer in pci_iounmap() is IO vs MMIO
> > > to iounmap conditionally and call it in pci_iounmap() fixing the issue.
> > >
> > > Leave __pci_ioport_unmap() as a NOP for all other config options.
> > >
> > > Reported-by: George Cherian <george.cherian@marvell.com>
> > > Link:
> > > https://urldefense.proofpoint.com/v2/url?u=https-3A__lore.kernel.org
> > > _lkml_20200905024811.74701-2D1-2Dyangyingliang-
> 40huawei.com&d=DwIBAg
> > >
> &c=nKjWec2b6R0mOyPaz7xtfQ&r=TjMsEFPc7dirkF6u2D3eSIS0cA8FeYpzRkk
> Mzr4a
> > > Cbk&m=UO5qU5LtNtCn6_gnT0rCkBxIm-w8jCaxHO6v7oK-U-
> I&s=CSGHQpKoVdNiqb1e
> > > DFuRUhka_Xv5o2PosWZ1rR8oOD4&e=
> > > Link:
> > > https://urldefense.proofpoint.com/v2/url?u=https-3A__lore.kernel.org
> > > _lkml_20200824132046.3114383-2D1-2Dgeorge.cherian-
> 40marvell.com&d=Dw
> > >
> IBAg&c=nKjWec2b6R0mOyPaz7xtfQ&r=TjMsEFPc7dirkF6u2D3eSIS0cA8FeYpz
> RkkM
> > > zr4aCbk&m=UO5qU5LtNtCn6_gnT0rCkBxIm-w8jCaxHO6v7oK-U-
> I&s=3B83oan7i1g3
> > > KaPgQmFK6PudR9GzvAPk33Z5Yyv-CMI&e=
> > > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > > Cc: Arnd Bergmann <arnd@arndb.de>
> > > Cc: George Cherian <george.cherian@marvell.com>
> > > Cc: Will Deacon <will@kernel.org>
> > > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > > Cc: Yang Yingliang <yangyingliang@huawei.com>
> > > ---
> > >  include/asm-generic/io.h | 39
> > > +++++++++++++++++++++++++++------------
> > >  1 file changed, 27 insertions(+), 12 deletions(-)
> >
> > This works for me. The only question I have is whether pci_iomap.h is
> > better than io.h for __pci_ioport_unmap(). These headers are really
> > confusing.
> 
> Yes they are, in total honesty there is much more to do to make them sane,
> this patch is just a band-aid.
> 
> I thought about moving this stuff into pci_iomap.h, though that file is
> included _independently_ from io.h from some arches so I tried to keep
> everything in io.h to minimize disruption.
> 
> We can merge this patch - since it is a fix after all - and then I can try to
> improve the whole pci_iounmap() includes.
> 
> > Either way:
> >
> > Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
> 
> Thanks a lot. I'd appreciate a tested-by from the George as he is the one who
> reported the problem.

Verified this patch and it works as expected.
Tested-by: George Cherian <george.cherian@marvell.com>
 
> Lorenzo