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()
Date: Mon, 29 Jan 2024 12:14:25 -0600 [thread overview]
Message-ID: <20240129181425.GA469470@bhelgaas> (raw)
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
next prev parent reply other threads:[~2024-01-29 18:14 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-01-11 8:55 [PATCH v5 RESEND 0/5] Regather scattered PCI-Code Philipp Stanner
2024-01-11 8:55 ` [PATCH v5 RESEND 1/5] lib/pci_iomap.c: fix cleanup bugs in pci_iounmap() Philipp Stanner
2024-01-23 18:46 ` Bjorn Helgaas
2024-01-25 16:06 ` Philipp Stanner
2024-01-11 8:55 ` [PATCH v5 RESEND 2/5] lib: move pci_iomap.c to drivers/pci/ Philipp Stanner
2024-01-23 20:20 ` Bjorn Helgaas
2024-01-25 14:54 ` Philipp Stanner
2024-01-25 18:31 ` Bjorn Helgaas
2024-01-11 8:55 ` [PATCH v5 RESEND 3/5] lib: move pci-specific devres code " Philipp Stanner
2024-01-11 8:55 ` [PATCH v5 RESEND 4/5] pci: move devres code from pci.c to devres.c Philipp Stanner
2024-01-11 8:55 ` [PATCH v5 RESEND 5/5] lib, pci: unify generic pci_iounmap() Philipp Stanner
2024-01-23 21:05 ` Bjorn Helgaas
2024-01-26 13:59 ` Philipp Stanner
2024-01-26 14:23 ` Arnd Bergmann
2024-01-27 22:39 ` Bjorn Helgaas
2024-01-29 10:43 ` Philipp Stanner
2024-01-29 18:14 ` Bjorn Helgaas [this message]
2024-01-11 9:03 ` [PATCH v5 RESEND 0/5] Regather scattered PCI-Code Johannes Berg
2024-01-11 9:14 ` Philipp Stanner
2024-01-11 9:22 ` Johannes Berg
2024-01-11 14:53 ` Bjorn Helgaas
2024-01-11 15:32 ` Philipp Stanner
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20240129181425.GA469470@bhelgaas \
--to=helgaas@kernel.org \
--cc=akpm@linux-foundation.org \
--cc=arnd@arndb.de \
--cc=arnd@kernel.org \
--cc=ben.dooks@codethink.co.uk \
--cc=bhelgaas@google.com \
--cc=dakr@redhat.com \
--cc=dave.jiang@intel.com \
--cc=davidgow@google.com \
--cc=elver@google.com \
--cc=geert@linux-m68k.org \
--cc=jbaron@akamai.com \
--cc=johannes@sipsolutions.net \
--cc=keescook@chromium.org \
--cc=kent.overstreet@gmail.com \
--cc=koshchanka@gmail.com \
--cc=linux-arch@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pci@vger.kernel.org \
--cc=mhiramat@kernel.org \
--cc=neilb@suse.de \
--cc=pstanner@redhat.com \
--cc=rdunlap@infradead.org \
--cc=rmoar@google.com \
--cc=sanpeqf@gmail.com \
--cc=schnelle@linux.ibm.com \
--cc=stable@vger.kernel.org \
--cc=tglx@linutronix.de \
--cc=wuqiang.matt@bytedance.com \
--cc=yury.norov@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).