Received-SPF: pass (google.com: domain of linux-kernel+bounces-40159-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <70b8db3ec0f8730fdd23dae21edc1a93d274b048.camel@redhat.com>
Subject: Re: [PATCH v5 RESEND 5/5] lib, pci: unify generic pci_iounmap()
From: Philipp Stanner <pstanner@redhat.com>
To: Bjorn Helgaas <helgaas@kernel.org>
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>
Date: Fri, 26 Jan 2024 14:59:20 +0100
In-Reply-To: <20240123210553.GA326783@bhelgaas>
References: <20240123210553.GA326783@bhelgaas>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.48.4 (3.48.4-1.fc38) 
Precedence: bulk
MIME-Version: 1.0

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:
> > The implementation of pci_iounmap() is currently scattered over two
> > files, drivers/pci/iomap.c and lib/iomap.c. Additionally,
> > architectures can define their own version.
> >=20
> > To have only one version, it's necessary to create a helper
> > function,
> > iomem_is_ioport(), that tells pci_iounmap() whether the passed
> > address
> > points to an ioport or normal memory.
> >=20
> > iomem_is_ioport() can be provided through two different ways:
> > =C2=A0 1. The architecture itself provides it. As of today, the version
> > =C2=A0=C2=A0=C2=A0=C2=A0 coming from lib/iomap.c de facto is the x86-sp=
ecific version
> > and
> > =C2=A0=C2=A0=C2=A0=C2=A0 comes into play when CONFIG_GENERIC_IOMAP is s=
elected. This
> > rather
> > =C2=A0=C2=A0=C2=A0=C2=A0 confusing naming is an artifact left by the re=
moval of IA64.
> > =C2=A0 2. As a default version in include/asm-generic/io.h for those
> > =C2=A0=C2=A0=C2=A0=C2=A0 architectures that don't use CONFIG_GENERIC_IO=
MAP, but also
> > don't
> > =C2=A0=C2=A0=C2=A0=C2=A0 provide their own version of iomem_is_ioport()=
.
> >=20
> > Once all architectures that support ports provide
> > iomem_is_ioport(), the
> > arch-specific definitions for pci_iounmap() can be removed and the
> > archs
> > can use the generic implementation, instead.
> >=20
> > Create a unified version of pci_iounmap() in drivers/pci/iomap.c.
> > Provide the function iomem_is_ioport() in include/asm-generic/io.h
> > (generic) and lib/iomap.c ("pseudo-generic" for x86).
> >=20
> > Remove the CONFIG_GENERIC_IOMAP guard around
> > ARCH_WANTS_GENERIC_PCI_IOUNMAP so that configs that set
> > CONFIG_GENERIC_PCI_IOMAP without CONFIG_GENERIC_IOMAP still get the
> > function.
> >=20
> > Add TODOs for follow-up work on the "generic is not generic but
> > x86-specific"-Problem.
> > ...
>=20
> > +++ b/drivers/pci/iomap.c
> > @@ -135,44 +135,30 @@ void __iomem *pci_iomap_wc(struct pci_dev
> > *dev, int bar, unsigned long maxlen)
> > =C2=A0EXPORT_SYMBOL_GPL(pci_iomap_wc);
> > =C2=A0
> > =C2=A0/*
> > - * pci_iounmap() somewhat illogically comes from lib/iomap.c for
> > the
> > - * CONFIG_GENERIC_IOMAP case, because that's the code that knows
> > about
> > - * the different IOMAP ranges.
> > + * This check is still necessary due to legacy reasons.
> > =C2=A0 *
> > - * But if the architecture does not use the generic iomap code,
> > and if
> > - * it has _not_ defined it's own private pci_iounmap function, we
> > define
> > - * it here.
> > - *
> > - * NOTE! This default implementation assumes that if the
> > architecture
> > - * support ioport mapping (HAS_IOPORT_MAP), the ioport mapping
> > will
> > - * be fixed to the range [ PCI_IOBASE, PCI_IOBASE+IO_SPACE_LIMIT
> > [,
> > - * and does not need unmapping with 'ioport_unmap()'.
> > - *
> > - * If you have different rules for your architecture, you need to
> > - * implement your own pci_iounmap() that knows the rules for where
> > - * and how IO vs MEM get mapped.
> > - *
> > - * This code is odd, and the ARCH_HAS/ARCH_WANTS #define logic
> > comes
> > - * from legacy <asm-generic/io.h> header file behavior. In
> > particular,
> > - * it would seem to make sense to do the iounmap(p) for the non-
> > IO-space
> > - * case here regardless, but that's not what the old header file
> > code
> > - * did. Probably incorrectly, but this is meant to be bug-for-bug
> > - * compatible.
>=20
> Moving this comment update to the patch that adds the ioport_unmap()
> call would make that patch more consistent and simplify this patch.

The bugfix from patch #1 you mean.
I can take care of that when splitting that patch as you suggested


>=20
> > + * TODO: Have all architectures that provide their own
> > pci_iounmap() provide
> > + * iomem_is_ioport() instead. Remove this #if afterwards.
> > =C2=A0 */
> > =C2=A0#if defined(ARCH_WANTS_GENERIC_PCI_IOUNMAP)
> > =C2=A0
> > -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.
>=20
> s/Unmapp/Unmap/ (twice)

OK

>=20
> > + */
> > +void pci_iounmap(struct pci_dev *dev, void __iomem *addr)
>=20
> 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?=C2=A0 Then this patch
> would be a little more focused.

OK

>=20
> 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()?

>=20
> > =C2=A0{
> > -#ifdef ARCH_HAS_GENERIC_IOPORT_MAP
> > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0uintptr_t start =3D (uintptr=
_t) PCI_IOBASE;
> > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0uintptr_t addr =3D (uintptr_=
t) p;
> > -
> > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if (addr >=3D start && addr =
< start + IO_SPACE_LIMIT) {
> > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0ioport_unmap(p);
> > +#ifdef CONFIG_HAS_IOPORT_MAP
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if (iomem_is_ioport(addr)) {
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0ioport_unmap(addr);
> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0return;
> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0}
> > =C2=A0#endif
> > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0iounmap(p);
> > +
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0iounmap(addr);
> > =C2=A0}
>=20
> > + * If CONFIG_GENERIC_IOMAP is selected and the architecture does
> > NOT provide its
> > + * own version, ARCH_WANTS_GENERIC_IOMEM_IS_IOPORT makes sure that
> > the generic
> > + * version from asm-generic/io.h is NOT used and instead the
> > second "generic"
> > + * version from this file here is used.
> > + *
> > + * There are currently two generic versions because of a difficult
> > cleanup
> > + * process. Namely, the version in lib/iomap.c once was really
> > generic when IA64
> > + * still existed. Today, it's only really used by x86.
> > + *
> > + * TODO: Move this function to x86-specific code.
>=20
> Some of these TODOs look fairly simple.=C2=A0 Are they actually hard, or
> could they just be done now?

If they were simple from my humble POV I would have implemented them.
The information about the x86-specficity is from Arnd Bergmann, the
header-maintainer.

I myself am not that sure how much work it would be to move the entire
lib/iomap.c file to x86. At least some (possibley "dead") hooks to it
still exist, for example here:
arch/powerpc/platforms/Kconfig  # L.189

>=20
> 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.

sic

Thx,
P.

>=20
> > + */
> > +#if defined(ARCH_WANTS_GENERIC_IOMEM_IS_IOPORT)
> > +bool iomem_is_ioport(void __iomem *addr)
> > =C2=A0{
> > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0IO_COND(addr, /* nothing */,=
 iounmap(addr));
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0unsigned long port =3D (unsi=
gned long __force)addr;
> > +
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if (port > PIO_OFFSET && por=
t < PIO_RESERVED)
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0return true;
> > +
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0return false;
> > =C2=A0}
> > -EXPORT_SYMBOL(pci_iounmap);
> > -#endif /* CONFIG_PCI */
> > +#endif /* ARCH_WANTS_GENERIC_IOMEM_IS_IOPORT */
> > --=20
> > 2.43.0
> >=20
>=20