Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.37 as permitted sender) client-ip=23.128.96.37;
Message-ID: <c902e7e690dcdf57c2e8402e083d27ce84aee21b.camel@redhat.com>
Subject: Re: [PATCH v2 4/4] lib, pci: unify generic pci_iounmap()
From:   Philipp Stanner <pstanner@redhat.com>
To:     Arnd Bergmann <arnd@arndb.de>, Bjorn Helgaas <bhelgaas@google.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        Dan Williams <dan.j.williams@intel.com>,
        Jonathan Cameron <Jonathan.Cameron@huawei.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Dave Jiang <dave.jiang@intel.com>,
        Uladzislau Koshchanka <koshchanka@gmail.com>,
        Neil Brown <neilb@suse.de>,
        Niklas Schnelle <schnelle@linux.ibm.com>,
        John Sanpe <sanpeqf@gmail.com>,
        Kent Overstreet <kent.overstreet@gmail.com>,
        Masami Hiramatsu <mhiramat@kernel.org>,
        Kees Cook <keescook@chromium.org>,
        David Gow <davidgow@google.com>,
        Yury Norov <yury.norov@gmail.com>,
        "wuqiang.matt" <wuqiang.matt@bytedance.com>,
        Jason Baron <jbaron@akamai.com>,
        Kefeng Wang <wangkefeng.wang@huawei.com>,
        Ben Dooks <ben.dooks@codethink.co.uk>,
        Danilo Krummrich <dakr@redhat.com>
Cc:     linux-kernel@vger.kernel.org, linux-pci@vger.kernel.org,
        Linux-Arch <linux-arch@vger.kernel.org>,
        Arnd Bergmann <arnd@kernel.org>, pstanner@redhat.com
Date:   Fri, 01 Dec 2023 22:56:05 +0100
In-Reply-To: <330df2f8-3796-4f74-8856-06ae1e46ec9b@app.fastmail.com>
References: <20231201121622.16343-1-pstanner@redhat.com>
         <20231201121622.16343-5-pstanner@redhat.com>
         <619ea619-29e4-42fb-9b27-1d1a32e0ee66@app.fastmail.com>
         <b54e5d57624dae0b045d8ff129ac2a41f72e182d.camel@redhat.com>
         <330df2f8-3796-4f74-8856-06ae1e46ec9b@app.fastmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.48.4 (3.48.4-1.fc38) 
MIME-Version: 1.0
Precedence: bulk

On Fri, 2023-12-01 at 22:32 +0100, Arnd Bergmann wrote:
> On Fri, Dec 1, 2023, at 20:37, Philipp Stanner wrote:
> > On Fri, 2023-12-01 at 16:26 +0100, Arnd Bergmann wrote:
> > >=20
> > > static inline bool struct iomem_is_ioport(void __iomem *p)
> > > {
> > > #ifdef CONFIG_HAS_IOPORT
> > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 uintptr_t start =3D (uintp=
tr_t) PCI_IOBASE;
> > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 uintptr_t addr =3D (uintpt=
r_t) p;
> > >=20
> > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (addr >=3D start && add=
r < 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=A0 return true;
> > > #endif
> > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return false;
> > > }
> > >=20
> > > > +#else /* CONFIG_GENERIC_IOMAP. Version from lib/iomap.c will
> > > > be
> > > > used.=20
> > > > */
> > > > +bool iomem_is_ioport(void __iomem *addr);
> > > > +#define ARCH_WANTS_GENERIC_IOMEM_IS_IOPORT
> > >=20
> > > I'm not sure what this macro is for, since it appears to
> > > do the opposite of what its name suggests: rather than
> > > provide the generic version of iomem_is_ioport(), it
> > > skips that and provides a custom one to go with lib/iomap.c
> >=20
> > Hmmm well now it's getting tricky.
> >=20
> > This else-branch is the one where CONFIG_GENERIC_IOMAP is actually
> > set.
> >=20
> > I think we're running into the "generic not being generic now that
> > IA64
> > has died" problem you were hinting at.
> >=20
> > If we build for x86 and have CONFIG_GENERIC set, only then do we
> > want
> > iomem_is_ioport() from lib/iomap.c. So the macro serves avoiding a
> > collision between symbols. Because lib/iomap.c might be compiled
> > even
> > if someone else already has defined iomem_is_ioport().
> > I also don't like it, but it was the least bad solution I could
> > come up
> > with
> > Suggestions?
>=20
> The best I can think of is to move the lib/iomap.c version
> of iomem_is_ioport() to include/asm-generic/iomap.h with
> an #ifndef iomem_is_ioport / #define iomem_is_ioport
> check around it. This file is only included on parisc, alpha,
> sh and when CONFIG_GENERIC_IOMAP is set.

My implementation from lib/iomap.c basically just throws away the
IO_COND macro and does the checks manually:

#if defined(ARCH_WANTS_GENERIC_IOMEM_IS_IOPORT)
bool iomem_is_ioport(void __iomem *addr)
{
	unsigned long port =3D (unsigned long __force)addr;

	if (port > PIO_OFFSET && port < PIO_RESERVED)
		return true;

	return false;
}
#endif /* ARCH_WANTS_GENERIC_IOMEM_IS_IOPORT */

So we'd also need PIO_OFFSET and PIO_RESERVED, which are not present in
asm-generic/iomap.h.

Sure, we could move them there or into a common header. But I'm not
sure if that is wise, meaning: is it really better than the ugly
WANTS_GENERIC_IOMEM... macro?

Our entire goal in this series is, after all, to simplify the
implementation.


P.

>=20
> The default version in asm-generic/io.h then just needs
> one more #ifdef iomem_is_ioport check around it.
>=20
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 Arnd
>=20