Received-SPF: pass (google.com: domain of linux-kernel+bounces-43295-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Mon, 29 Jan 2024 12:14:25 -0600
From: Bjorn Helgaas <helgaas@kernel.org>
To: Philipp Stanner <pstanner@redhat.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>, Arnd Bergmann <arnd@arndb.de>,
	Johannes Berg <johannes@sipsolutions.net>,
	Randy Dunlap <rdunlap@infradead.org>, NeilBrown <neilb@suse.de>,
	John Sanpe <sanpeqf@gmail.com>,
	Kent Overstreet <kent.overstreet@gmail.com>,
	Niklas Schnelle <schnelle@linux.ibm.com>,
	Dave Jiang <dave.jiang@intel.com>,
	Uladzislau Koshchanka <koshchanka@gmail.com>,
	"Masami Hiramatsu (Google)" <mhiramat@kernel.org>,
	David Gow <davidgow@google.com>, Kees Cook <keescook@chromium.org>,
	Rae Moar <rmoar@google.com>,
	Geert Uytterhoeven <geert@linux-m68k.org>,
	"wuqiang.matt" <wuqiang.matt@bytedance.com>,
	Yury Norov <yury.norov@gmail.com>, Jason Baron <jbaron@akamai.com>,
	Thomas Gleixner <tglx@linutronix.de>,
	Marco Elver <elver@google.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	Ben Dooks <ben.dooks@codethink.co.uk>, dakr@redhat.com,
	linux-kernel@vger.kernel.org, linux-pci@vger.kernel.org,
	linux-arch@vger.kernel.org, stable@vger.kernel.org,
	Arnd Bergmann <arnd@kernel.org>
Subject: Re: [PATCH v5 RESEND 5/5] lib, pci: unify generic pci_iounmap()
Message-ID: <20240129181425.GA469470@bhelgaas>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <e655978d5f06ca58f36d531a9f789420fe959fd1.camel@redhat.com>

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