mbox series

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

Message ID 20200915093203.16934-1-lorenzo.pieralisi@arm.com
Headers show
Series Fix pci_iounmap() on !CONFIG_GENERIC_IOMAP | expand

Message

Lorenzo Pieralisi Sept. 15, 2020, 9:32 a.m. UTC
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(-)

Comments

Lorenzo Pieralisi Sept. 16, 2020, 11:06 a.m. UTC | #1
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(-)
Catalin Marinas Sept. 16, 2020, 2:51 p.m. UTC | #2
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>
Lorenzo Pieralisi Sept. 17, 2020, 9:29 a.m. UTC | #3
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
Lorenzo Pieralisi Sept. 18, 2020, 11:45 a.m. UTC | #4
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
Arnd Bergmann Sept. 18, 2020, 7:58 p.m. UTC | #5
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
Lorenzo Pieralisi Sept. 28, 2020, 9:31 a.m. UTC | #6
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