linux-kernel - Re: [PATCH v5 RESEND 5/5] lib, pci: unify generic pci

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20240129181425.GA469470@bhelgaas>
Date: Mon, 29 Jan 2024 12:14:25 -0600
From: Bjorn Helgaas <helgaas@...nel.org>
To: Philipp Stanner <pstanner@...hat.com>
Cc: Bjorn Helgaas <bhelgaas@...gle.com>, Arnd Bergmann <arnd@...db.de>,
	Johannes Berg <johannes@...solutions.net>,
	Randy Dunlap <rdunlap@...radead.org>, NeilBrown <neilb@...e.de>,
	John Sanpe <sanpeqf@...il.com>,
	Kent Overstreet <kent.overstreet@...il.com>,
	Niklas Schnelle <schnelle@...ux.ibm.com>,
	Dave Jiang <dave.jiang@...el.com>,
	Uladzislau Koshchanka <koshchanka@...il.com>,
	"Masami Hiramatsu (Google)" <mhiramat@...nel.org>,
	David Gow <davidgow@...gle.com>, Kees Cook <keescook@...omium.org>,
	Rae Moar <rmoar@...gle.com>,
	Geert Uytterhoeven <geert@...ux-m68k.org>,
	"wuqiang.matt" <wuqiang.matt@...edance.com>,
	Yury Norov <yury.norov@...il.com>, Jason Baron <jbaron@...mai.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	Marco Elver <elver@...gle.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Ben Dooks <ben.dooks@...ethink.co.uk>, dakr@...hat.com,
	linux-kernel@...r.kernel.org, linux-pci@...r.kernel.org,
	linux-arch@...r.kernel.org, stable@...r.kernel.org,
	Arnd Bergmann <arnd@...nel.org>
Subject: Re: [PATCH v5 RESEND 5/5] lib, pci: unify generic pci_iounmap()

On Mon, Jan 29, 2024 at 11:43:34AM +0100, Philipp Stanner wrote:
> On Sat, 2024-01-27 at 16:39 -0600, Bjorn Helgaas wrote:
> > On Fri, Jan 26, 2024 at 02:59:20PM +0100, Philipp Stanner wrote:
> > > On Tue, 2024-01-23 at 15:05 -0600, Bjorn Helgaas wrote:
> > > > On Thu, Jan 11, 2024 at 09:55:40AM +0100, Philipp Stanner wrote:
> > > ...
> > 
> > > > > -void pci_iounmap(struct pci_dev *dev, void __iomem *p)
> > > > > +/**
> > > > > + * pci_iounmap - Unmapp a mapping
> > > > > + * @dev: PCI device the mapping belongs to
> > > > > + * @addr: start address of the mapping
> > > > > + *
> > > > > + * Unmapp a PIO or MMIO mapping.
> > > > > + */
> > > > > +void pci_iounmap(struct pci_dev *dev, void __iomem *addr)
> > > > 
> > > > Maybe move the "p" to "addr" rename to the patch that fixes the
> > > > pci_iounmap() #ifdef problem, since that's a trivial change that
> > > > already has to do with handling both PIO and MMIO?  Then this
> > > > patch
> > > > would be a little more focused.
> > > > 
> > > > The kernel-doc addition could possibly also move there since it
> > > > isn't
> > > > related to the unification.
> > > 
> > > You mean the one from my devres-patch-series? Or documentation
> > > specifically about pci_iounmap()?
> > 
> > I had in mind the patch that fixes the pci_iounmap() #ifdef problem,
> > which (if you split it out from 1/5) would be a relatively trivial
> > patch.  Or the kernel-doc addition could be its own separate patch.
> > The point is that this unification patch is fairly complicated, so
> > anything we can do to move things unrelated to unification elsewhere
> > makes this one easier to review.
> 
> I think it should be a separate patch, then, as it doesn't belong by
> 100% to any of the patches here. If I had to pick one, I'd have
> included the docu into patch #2 or #3.
> 
> Let's make it a separate one, following as a 6th patch in this series

Sounds good.

> > > > It seems like implementing iomem_is_ioport() for the other arches
> > > > would be straightforward and if done first, could make this patch
> > > > look
> > > > tidier.
> > > 
> > > That would be the cleanest solution. But the cleaner you want to
> > > be,
> > > the more time you have to spend ;)
> > > I can take another look and see if I could do that with reasonable
> > > effort.
> > > Otherwise I'd go for:
> > > 
> > > > Or if the TODOs can't be done now, maybe the iomem_is_ioport()
> > > > addition could be done as a separate patch to make the
> > > > unification
> > > > more obvious.
> > 
> > It looks like iomem_is_ioport() is basically the guards in
> > pci_iounmap() implementations that, if true, prevent calling
> > iounmap(), so it it seems like they should be trivial, e.g.,
> > 
> >   return !__is_mmio(addr); # alpha
> > 
> >   return (addr < VMALLOC_START || addr >= VMALLOC_END); # arm
> > 
> >   return isa_vaddr_is_ioport(addr) || pcibios_vaddr_is_ioport(addr);
> > # microblaze
> > 
> > Unless they're significantly more complicated than that, I don't see
> > the point of deferring them.

> ...
> This series' purpose actually always has been to move PCI functions to
> where they belong, i.e. from lib/ to drivers/pci.
> I originally didn't want to touch pci_iounmap(), since I deemed it too
> complicated. Arnd pushed for unifying it.
> 
> Anyways, investing much more time into this is beyond my time budget. I
> only started this series to have a cleaner basis to do the devres
> functions.
> 
> So my suggestions is that we either go with this cleanup here, which
> improves the situation at least somewhat, or we simply drop patch #5
> and leave pci_iounmap() as the last pci_ function in lib/

OK, let's drop #5 for now.  It definitely seems like where we should
go in the future, but I think it will make more sense when we can
include a few of the simple conversions that will show how it all fits
together.

Bjorn