* [PATCH v2 1/3] sparc32: Remove useless io_32.h __KERNEL__ preprocessor guard
2020-09-16 11:06 ` [PATCH v2 0/3] Fix pci_iounmap() on !CONFIG_GENERIC_IOMAP Lorenzo Pieralisi
@ 2020-09-16 11:06 ` Lorenzo Pieralisi
2020-09-16 11:06 ` [PATCH v2 2/3] sparc32: Move ioremap/iounmap declaration before asm-generic/io.h include Lorenzo Pieralisi
` (2 subsequent siblings)
3 siblings, 0 replies; 17+ messages in thread
From: Lorenzo Pieralisi @ 2020-09-16 11:06 UTC (permalink / raw)
To: linux-kernel
Cc: Lorenzo Pieralisi, David S . Miller, linux-pci, linux-arch,
linux-arm-kernel, Bjorn Helgaas, Catalin Marinas, Will Deacon,
Arnd Bergmann, George Cherian, Yang Yingliang
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
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH v2 2/3] sparc32: Move ioremap/iounmap declaration before asm-generic/io.h include
2020-09-16 11:06 ` [PATCH v2 0/3] Fix pci_iounmap() on !CONFIG_GENERIC_IOMAP Lorenzo Pieralisi
2020-09-16 11:06 ` [PATCH v2 1/3] sparc32: Remove useless io_32.h __KERNEL__ preprocessor guard Lorenzo Pieralisi
@ 2020-09-16 11:06 ` Lorenzo Pieralisi
2020-09-16 11:06 ` [PATCH v2 3/3] asm-generic/io.h: Fix !CONFIG_GENERIC_IOMAP pci_iounmap() implementation Lorenzo Pieralisi
2020-09-18 11:45 ` [PATCH v2 0/3] Fix pci_iounmap() on !CONFIG_GENERIC_IOMAP Lorenzo Pieralisi
3 siblings, 0 replies; 17+ messages in thread
From: Lorenzo Pieralisi @ 2020-09-16 11:06 UTC (permalink / raw)
To: linux-kernel
Cc: Lorenzo Pieralisi, David S. Miller, linux-pci, linux-arch,
linux-arm-kernel, Bjorn Helgaas, Catalin Marinas, Will Deacon,
Arnd Bergmann, George Cherian, Yang Yingliang
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
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v2 3/3] asm-generic/io.h: Fix !CONFIG_GENERIC_IOMAP pci_iounmap() implementation
2020-09-16 11:06 ` [PATCH v2 0/3] Fix pci_iounmap() on !CONFIG_GENERIC_IOMAP Lorenzo Pieralisi
2020-09-16 11:06 ` [PATCH v2 1/3] sparc32: Remove useless io_32.h __KERNEL__ preprocessor guard Lorenzo Pieralisi
2020-09-16 11:06 ` [PATCH v2 2/3] sparc32: Move ioremap/iounmap declaration before asm-generic/io.h include Lorenzo Pieralisi
@ 2020-09-16 11:06 ` Lorenzo Pieralisi
2020-09-16 14:51 ` Catalin Marinas
2020-09-18 11:45 ` [PATCH v2 0/3] Fix pci_iounmap() on !CONFIG_GENERIC_IOMAP Lorenzo Pieralisi
3 siblings, 1 reply; 17+ messages in thread
From: Lorenzo Pieralisi @ 2020-09-16 11:06 UTC (permalink / raw)
To: linux-kernel
Cc: Lorenzo Pieralisi, George Cherian, Arnd Bergmann, Will Deacon,
Bjorn Helgaas, Catalin Marinas, Yang Yingliang, linux-pci,
linux-arch, linux-arm-kernel, David S. Miller
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
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH v2 3/3] asm-generic/io.h: Fix !CONFIG_GENERIC_IOMAP pci_iounmap() implementation
2020-09-16 11:06 ` [PATCH v2 3/3] asm-generic/io.h: Fix !CONFIG_GENERIC_IOMAP pci_iounmap() implementation Lorenzo Pieralisi
@ 2020-09-16 14:51 ` Catalin Marinas
2020-09-17 9:29 ` Lorenzo Pieralisi
0 siblings, 1 reply; 17+ messages in thread
From: Catalin Marinas @ 2020-09-16 14:51 UTC (permalink / raw)
To: Lorenzo Pieralisi
Cc: linux-kernel, George Cherian, Arnd Bergmann, Will Deacon,
Bjorn Helgaas, Yang Yingliang, linux-pci, linux-arch,
linux-arm-kernel, David S. Miller
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>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 3/3] asm-generic/io.h: Fix !CONFIG_GENERIC_IOMAP pci_iounmap() implementation
2020-09-16 14:51 ` Catalin Marinas
@ 2020-09-17 9:29 ` Lorenzo Pieralisi
0 siblings, 0 replies; 17+ messages in thread
From: Lorenzo Pieralisi @ 2020-09-17 9:29 UTC (permalink / raw)
To: Catalin Marinas
Cc: linux-kernel, George Cherian, Arnd Bergmann, Will Deacon,
Bjorn Helgaas, Yang Yingliang, linux-pci, linux-arch,
linux-arm-kernel, David S. Miller
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 0/3] Fix pci_iounmap() on !CONFIG_GENERIC_IOMAP
2020-09-16 11:06 ` [PATCH v2 0/3] Fix pci_iounmap() on !CONFIG_GENERIC_IOMAP Lorenzo Pieralisi
` (2 preceding siblings ...)
2020-09-16 11:06 ` [PATCH v2 3/3] asm-generic/io.h: Fix !CONFIG_GENERIC_IOMAP pci_iounmap() implementation Lorenzo Pieralisi
@ 2020-09-18 11:45 ` Lorenzo Pieralisi
2020-09-18 19:58 ` Arnd Bergmann
3 siblings, 1 reply; 17+ messages in thread
From: Lorenzo Pieralisi @ 2020-09-18 11:45 UTC (permalink / raw)
To: linux-kernel, Arnd Bergmann, David S. Miller, bhelgaas
Cc: Catalin Marinas, Will Deacon, George Cherian, Yang Yingliang,
linux-pci, linux-arch, linux-arm-kernel
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
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v2 0/3] Fix pci_iounmap() on !CONFIG_GENERIC_IOMAP
2020-09-18 11:45 ` [PATCH v2 0/3] Fix pci_iounmap() on !CONFIG_GENERIC_IOMAP Lorenzo Pieralisi
@ 2020-09-18 19:58 ` Arnd Bergmann
2020-09-28 9:31 ` Lorenzo Pieralisi
0 siblings, 1 reply; 17+ messages in thread
From: Arnd Bergmann @ 2020-09-18 19:58 UTC (permalink / raw)
To: Lorenzo Pieralisi
Cc: linux-kernel, David S. Miller, Bjorn Helgaas, Catalin Marinas,
Will Deacon, George Cherian, Yang Yingliang, linux-pci,
linux-arch, Linux ARM
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
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v2 0/3] Fix pci_iounmap() on !CONFIG_GENERIC_IOMAP
2020-09-18 19:58 ` Arnd Bergmann
@ 2020-09-28 9:31 ` Lorenzo Pieralisi
0 siblings, 0 replies; 17+ messages in thread
From: Lorenzo Pieralisi @ 2020-09-28 9:31 UTC (permalink / raw)
To: Arnd Bergmann, Bjorn Helgaas
Cc: linux-kernel, David S. Miller, Catalin Marinas, Will Deacon,
George Cherian, Yang Yingliang, linux-pci, linux-arch, Linux ARM
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
^ permalink raw reply [flat|nested] 17+ messages in thread