Sooooooooo. I reworked v1.
Please review this carefully, the IO-Ranges are obviously a bit tricky,
as is the build-system / ifdef-ery.
Arnd has suggested that architectures defining a custom inb() need their
own iomem_is_ioport(), as well. I've grepped for inb() and found the
following list of archs that define their own:
- alpha
- arm
- m68k <--
- parisc
- powerpc
- sh
- sparc
- x86 <--
All of those have their own definitons of pci_iounmap(). Therefore, they
don't need our generic version in the first place and, thus, also need
no iomem_is_ioport().
The two exceptions are x86 and m68k. The former uses lib/iomap.c through
CONFIG_GENERIC_IOMAP, as Arnd pointed out in the previous discussion
(thus, CONFIG_GENERIC_IOMAP is not really generic in this regard).
So as I see it, only m68k WOULD need its own custom definition of
iomem_is_ioport(). But as I understand it it doesn't because it uses the
one from asm-generic/pci_iomap.h ??
I wasn't entirely sure how to deal with the address ranges for the
generic implementation in asm-generic/io.h. It's marked with a TODO.
Input appreciated.
I removed the guard around define pci_iounmap in asm-generic/io.h. An
alternative would be to have it be guarded by CONFIG_GENERIC_IOMAP and
CONFIG_GENERIC_PCI_IOMAP, both. Without such a guard, there is no
collision however, because generic pci_iounmap() from
drivers/pci/iomap.c will only get pulled in when
CONFIG_GENERIC_PCI_IOMAP is actually set.
I cross-built this for a variety of architectures, including the usual
suspects (s390, m68k). So far successfully. But let's see what Intel's
robots say :O
P.
Changes in v2:
- Replace patch 4, previously extending the comment about pci_iounmap()
in lib/iomap.c, with a patch that moves pci_iounmap() from that file
to drivers/pci/iomap.c, creating a unified version there. (Arnd)
- Implement iomem_is_ioport() as a new helper in asm-generic/io.h and
lib/iomap.c. (Arnd)
- Move the build rule in drivers/pci/Makefile for iomap.o under the
guard of #if PCI. This had to be done because when just checking for
GENERIC_PCI_IOMAP being defined, the functions don't disappear, which
was the case previously in lib/pci_iomap.c, where the entire file was
made empty if PCI was not set by the guard #ifdef PCI. (Intel's Bots)
- Rephares all patches' commit messages a little bit.
Original cover letter:
Hi!
So it seems that since ca. 2007 the PCI code has been scattered a bit.
PCI's devres code, which is only ever used by users of the entire
PCI-subsystem anyways, resides in lib/devres.c and is guarded by an
ifdef PCI, just as the content of lib/pci_iomap.c is.
It, thus, seems reasonable to move all of that.
As I were at it, I moved as much of the devres-specific code from pci.c
to devres.c, too. The only exceptions are four functions that are
currently difficult to move. More information about that can be read
here [1].
I noticed these scattered files while working on (new) PCI-specific
devres functions. If we can get this here merged, I'll soon send another
patch series that addresses some API-inconsistencies and could move the
devres-part of the four remaining functions.
I don't want to do that in this series as this here is only about moving
code, whereas the next series would have to actually change API
behavior.
I successfully (cross-)built this for x86, x86_64, AARCH64 and ARM
(allyesconfig). I booted a kernel with it on x86_64, with a Fedora
desktop environment as payload. The OS came up fine
I hope this is OK. If we can get it in, we'd soon have a very
consistent PCI API again.
Regards,
P.
Philipp Stanner (4):
lib: move pci_iomap.c to drivers/pci/
lib: move pci-specific devres code to drivers/pci/
pci: move devres code from pci.c to devres.c
lib, pci: unify generic pci_iounmap()
drivers/pci/Kconfig | 5 +
drivers/pci/Makefile | 3 +-
drivers/pci/devres.c | 450 +++++++++++++++++++++++++
lib/pci_iomap.c => drivers/pci/iomap.c | 43 +--
drivers/pci/pci.c | 249 --------------
drivers/pci/pci.h | 24 ++
include/asm-generic/io.h | 37 +-
lib/Kconfig | 3 -
lib/Makefile | 1 -
lib/devres.c | 208 +-----------
lib/iomap.c | 16 +-
11 files changed, 536 insertions(+), 503 deletions(-)
create mode 100644 drivers/pci/devres.c
rename lib/pci_iomap.c => drivers/pci/iomap.c (75%)
--
2.43.0
The implementation of pci_iounmap() is currently scattered over two
files, drivers/pci/iounmap.c and lib/iomap.c. Additionally,
architectures can define their own version.
Besides one unified version being desirable in the first place, the old
version in drivers/pci/iounmap.c contained a bug and could leak memory
mappings. The bug was that #ifdef ARCH_HAS_GENERIC_IOPORT_MAP should not
have guarded iounmap(p); in addition to the preceding code.
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.
iomem_is_ioport() can be provided through three different ways:
1. The architecture itself provides it.
2. As a default version in include/asm-generic/io.h for those
architectures that don't use CONFIG_GENERIC_IOMAP, but also don't
provide their own version of iomem_is_ioport().
3. As a default version in lib/iomap.c for those architectures that
define and use CONFIG_GENERIC_IOMAP (currently, only x86 really
uses the functions in lib/iomap.c)
Create a unified version of pci_iounmap() in drivers/pci/iomap.c.
Provide the function iomem_is_ioport() in include/asm-generic/io.h and
lib/iomap.c.
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.
Fixes: 316e8d79a095 ("pci_iounmap'2: Electric Boogaloo: try to make sense of it all")
Suggested-by: Arnd Bergmann <[email protected]>
Signed-off-by: Philipp Stanner <[email protected]>
---
drivers/pci/iomap.c | 40 +++++++++++-----------------------------
include/asm-generic/io.h | 37 ++++++++++++++++++++++++++++++++++---
lib/iomap.c | 16 +++++++++-------
3 files changed, 54 insertions(+), 39 deletions(-)
diff --git a/drivers/pci/iomap.c b/drivers/pci/iomap.c
index 0a9d503ba533..cb7f86371b7d 100644
--- a/drivers/pci/iomap.c
+++ b/drivers/pci/iomap.c
@@ -135,42 +135,24 @@ void __iomem *pci_iomap_wc(struct pci_dev *dev, int bar, unsigned long maxlen)
EXPORT_SYMBOL_GPL(pci_iomap_wc);
/*
- * 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.
+ * Generic version of pci_iounmap() that is used if the architecture does not
+ * provide its own version.
*
- * 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.
+ * If you have special rules for your architecture, you need to implement your
+ * own pci_iounmap() in ARCH/asm/io.h that knows the rules for where and how IO
+ * vs MEM get mapped.
*/
#if defined(ARCH_WANTS_GENERIC_PCI_IOUNMAP)
-void pci_iounmap(struct pci_dev *dev, void __iomem *p)
+void pci_iounmap(struct pci_dev *dev, void __iomem *addr)
{
-#ifdef ARCH_HAS_GENERIC_IOPORT_MAP
- uintptr_t start = (uintptr_t) PCI_IOBASE;
- uintptr_t addr = (uintptr_t) p;
-
- if (addr >= start && addr < start + IO_SPACE_LIMIT)
+#ifdef CONFIG_HAS_IOPORT
+ if (iomem_is_ioport(addr)) {
+ ioport_unmap(addr);
return;
- iounmap(p);
+ }
#endif
+ iounmap(addr);
}
EXPORT_SYMBOL(pci_iounmap);
diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h
index bac63e874c7b..4177d6b97e0b 100644
--- a/include/asm-generic/io.h
+++ b/include/asm-generic/io.h
@@ -1129,11 +1129,42 @@ extern void ioport_unmap(void __iomem *p);
#endif /* CONFIG_GENERIC_IOMAP */
#endif /* CONFIG_HAS_IOPORT_MAP */
-#ifndef CONFIG_GENERIC_IOMAP
#ifndef pci_iounmap
+#define pci_iounmap pci_iounmap
#define ARCH_WANTS_GENERIC_PCI_IOUNMAP
-#endif
-#endif
+#endif /* pci_iounmap */
+
+
+/*
+ * This function is a helper only needed for the generic pci_iounmap().
+ * It's provided here if the architecture does not select GENERIC_IOMAP and does
+ * not provide its own version.
+ */
+#ifdef CONFIG_HAS_IOPORT
+#ifndef iomem_is_ioport /* i.e., if the architecture hasn't defined its own. */
+#define iomem_is_ioport iomem_is_ioport
+
+#ifndef CONFIG_GENERIC_IOMAP
+static inline bool iomem_is_ioport(void __iomem *addr)
+{
+ unsigned long port = (unsigned long __force)addr;
+
+ // TODO: do we have to take IO_SPACE_LIMIT and PCI_IOBASE into account
+ // similar as in ioport_map() ?
+
+ if (port > MMIO_UPPER_LIMIT)
+ return false;
+
+ return true;
+}
+#else /* CONFIG_GENERIC_IOMAP. Version from lib/iomap.c will be used. */
+bool iomem_is_ioport(void __iomem *addr);
+#define ARCH_WANTS_GENERIC_IOMEM_IS_IOPORT
+#endif /* CONFIG_GENERIC_IOMAP */
+
+#endif /* iomem_is_ioport */
+#endif /* CONFIG_HAS_IOPORT */
+
#ifndef xlate_dev_mem_ptr
#define xlate_dev_mem_ptr xlate_dev_mem_ptr
diff --git a/lib/iomap.c b/lib/iomap.c
index 4f8b31baa575..adaf6246f892 100644
--- a/lib/iomap.c
+++ b/lib/iomap.c
@@ -418,12 +418,14 @@ EXPORT_SYMBOL(ioport_map);
EXPORT_SYMBOL(ioport_unmap);
#endif /* CONFIG_HAS_IOPORT_MAP */
-#ifdef CONFIG_PCI
-/* Hide the details if this is a MMIO or PIO address space and just do what
- * you expect in the correct way. */
-void pci_iounmap(struct pci_dev *dev, void __iomem * addr)
+#if defined(ARCH_WANTS_GENERIC_IOMEM_IS_IOPORT)
+bool iomem_is_ioport(void __iomem *addr)
{
- IO_COND(addr, /* nothing */, iounmap(addr));
+ unsigned long port = (unsigned long __force)addr;
+
+ if (port > PIO_OFFSET && port < PIO_RESERVED)
+ return true;
+
+ return false;
}
-EXPORT_SYMBOL(pci_iounmap);
-#endif /* CONFIG_PCI */
+#endif /* ARCH_WANTS_GENERIC_IOMEM_IS_IOPORT */
--
2.43.0
The pcim_*() functions in lib/devres.c are guarded by an #ifdef
CONFIG_PCI and, thus, don't belong to this file. They are only ever used
for pci and are not generic infrastructure.
Move all pcim_*() functions in lib/devres.c to drivers/pci/devres.c.
Adjust the Makefile.
Suggested-by: Danilo Krummrich <[email protected]>
Signed-off-by: Philipp Stanner <[email protected]>
---
drivers/pci/Makefile | 2 +-
drivers/pci/devres.c | 207 ++++++++++++++++++++++++++++++++++++++++++
lib/devres.c | 208 +------------------------------------------
3 files changed, 209 insertions(+), 208 deletions(-)
create mode 100644 drivers/pci/devres.c
diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile
index 64dcedccfc87..ed65299b42b5 100644
--- a/drivers/pci/Makefile
+++ b/drivers/pci/Makefile
@@ -5,7 +5,7 @@
obj-$(CONFIG_PCI) += access.o bus.o probe.o host-bridge.o \
remove.o pci.o pci-driver.o search.o \
pci-sysfs.o rom.o setup-res.o irq.o vpd.o \
- setup-bus.o vc.o mmap.o setup-irq.o
+ setup-bus.o vc.o mmap.o setup-irq.o devres.o
obj-$(CONFIG_PCI) += msi/
obj-$(CONFIG_PCI) += pcie/
diff --git a/drivers/pci/devres.c b/drivers/pci/devres.c
new file mode 100644
index 000000000000..a3fd0d65cef1
--- /dev/null
+++ b/drivers/pci/devres.c
@@ -0,0 +1,207 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <linux/pci.h>
+#include "pci.h"
+
+/*
+ * PCI iomap devres
+ */
+#define PCIM_IOMAP_MAX PCI_STD_NUM_BARS
+
+struct pcim_iomap_devres {
+ void __iomem *table[PCIM_IOMAP_MAX];
+};
+
+static void pcim_iomap_release(struct device *gendev, void *res)
+{
+ struct pci_dev *dev = to_pci_dev(gendev);
+ struct pcim_iomap_devres *this = res;
+ int i;
+
+ for (i = 0; i < PCIM_IOMAP_MAX; i++)
+ if (this->table[i])
+ pci_iounmap(dev, this->table[i]);
+}
+
+/**
+ * pcim_iomap_table - access iomap allocation table
+ * @pdev: PCI device to access iomap table for
+ *
+ * Access iomap allocation table for @dev. If iomap table doesn't
+ * exist and @pdev is managed, it will be allocated. All iomaps
+ * recorded in the iomap table are automatically unmapped on driver
+ * detach.
+ *
+ * This function might sleep when the table is first allocated but can
+ * be safely called without context and guaranteed to succeed once
+ * allocated.
+ */
+void __iomem * const *pcim_iomap_table(struct pci_dev *pdev)
+{
+ struct pcim_iomap_devres *dr, *new_dr;
+
+ dr = devres_find(&pdev->dev, pcim_iomap_release, NULL, NULL);
+ if (dr)
+ return dr->table;
+
+ new_dr = devres_alloc_node(pcim_iomap_release, sizeof(*new_dr), GFP_KERNEL,
+ dev_to_node(&pdev->dev));
+ if (!new_dr)
+ return NULL;
+ dr = devres_get(&pdev->dev, new_dr, NULL, NULL);
+ return dr->table;
+}
+EXPORT_SYMBOL(pcim_iomap_table);
+
+/**
+ * pcim_iomap - Managed pcim_iomap()
+ * @pdev: PCI device to iomap for
+ * @bar: BAR to iomap
+ * @maxlen: Maximum length of iomap
+ *
+ * Managed pci_iomap(). Map is automatically unmapped on driver
+ * detach.
+ */
+void __iomem *pcim_iomap(struct pci_dev *pdev, int bar, unsigned long maxlen)
+{
+ void __iomem **tbl;
+
+ BUG_ON(bar >= PCIM_IOMAP_MAX);
+
+ tbl = (void __iomem **)pcim_iomap_table(pdev);
+ if (!tbl || tbl[bar]) /* duplicate mappings not allowed */
+ return NULL;
+
+ tbl[bar] = pci_iomap(pdev, bar, maxlen);
+ return tbl[bar];
+}
+EXPORT_SYMBOL(pcim_iomap);
+
+/**
+ * pcim_iounmap - Managed pci_iounmap()
+ * @pdev: PCI device to iounmap for
+ * @addr: Address to unmap
+ *
+ * Managed pci_iounmap(). @addr must have been mapped using pcim_iomap().
+ */
+void pcim_iounmap(struct pci_dev *pdev, void __iomem *addr)
+{
+ void __iomem **tbl;
+ int i;
+
+ pci_iounmap(pdev, addr);
+
+ tbl = (void __iomem **)pcim_iomap_table(pdev);
+ BUG_ON(!tbl);
+
+ for (i = 0; i < PCIM_IOMAP_MAX; i++)
+ if (tbl[i] == addr) {
+ tbl[i] = NULL;
+ return;
+ }
+ WARN_ON(1);
+}
+EXPORT_SYMBOL(pcim_iounmap);
+
+/**
+ * pcim_iomap_regions - Request and iomap PCI BARs
+ * @pdev: PCI device to map IO resources for
+ * @mask: Mask of BARs to request and iomap
+ * @name: Name used when requesting regions
+ *
+ * Request and iomap regions specified by @mask.
+ */
+int pcim_iomap_regions(struct pci_dev *pdev, int mask, const char *name)
+{
+ void __iomem * const *iomap;
+ int i, rc;
+
+ iomap = pcim_iomap_table(pdev);
+ if (!iomap)
+ return -ENOMEM;
+
+ for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
+ unsigned long len;
+
+ if (!(mask & (1 << i)))
+ continue;
+
+ rc = -EINVAL;
+ len = pci_resource_len(pdev, i);
+ if (!len)
+ goto err_inval;
+
+ rc = pci_request_region(pdev, i, name);
+ if (rc)
+ goto err_inval;
+
+ rc = -ENOMEM;
+ if (!pcim_iomap(pdev, i, 0))
+ goto err_region;
+ }
+
+ return 0;
+
+ err_region:
+ pci_release_region(pdev, i);
+ err_inval:
+ while (--i >= 0) {
+ if (!(mask & (1 << i)))
+ continue;
+ pcim_iounmap(pdev, iomap[i]);
+ pci_release_region(pdev, i);
+ }
+
+ return rc;
+}
+EXPORT_SYMBOL(pcim_iomap_regions);
+
+/**
+ * pcim_iomap_regions_request_all - Request all BARs and iomap specified ones
+ * @pdev: PCI device to map IO resources for
+ * @mask: Mask of BARs to iomap
+ * @name: Name used when requesting regions
+ *
+ * Request all PCI BARs and iomap regions specified by @mask.
+ */
+int pcim_iomap_regions_request_all(struct pci_dev *pdev, int mask,
+ const char *name)
+{
+ int request_mask = ((1 << 6) - 1) & ~mask;
+ int rc;
+
+ rc = pci_request_selected_regions(pdev, request_mask, name);
+ if (rc)
+ return rc;
+
+ rc = pcim_iomap_regions(pdev, mask, name);
+ if (rc)
+ pci_release_selected_regions(pdev, request_mask);
+ return rc;
+}
+EXPORT_SYMBOL(pcim_iomap_regions_request_all);
+
+/**
+ * pcim_iounmap_regions - Unmap and release PCI BARs
+ * @pdev: PCI device to map IO resources for
+ * @mask: Mask of BARs to unmap and release
+ *
+ * Unmap and release regions specified by @mask.
+ */
+void pcim_iounmap_regions(struct pci_dev *pdev, int mask)
+{
+ void __iomem * const *iomap;
+ int i;
+
+ iomap = pcim_iomap_table(pdev);
+ if (!iomap)
+ return;
+
+ for (i = 0; i < PCIM_IOMAP_MAX; i++) {
+ if (!(mask & (1 << i)))
+ continue;
+
+ pcim_iounmap(pdev, iomap[i]);
+ pci_release_region(pdev, i);
+ }
+}
+EXPORT_SYMBOL(pcim_iounmap_regions);
diff --git a/lib/devres.c b/lib/devres.c
index c44f104b58d5..fe0c63caeb68 100644
--- a/lib/devres.c
+++ b/lib/devres.c
@@ -1,6 +1,6 @@
// SPDX-License-Identifier: GPL-2.0
+#include <linux/device.h>
#include <linux/err.h>
-#include <linux/pci.h>
#include <linux/io.h>
#include <linux/gfp.h>
#include <linux/export.h>
@@ -311,212 +311,6 @@ void devm_ioport_unmap(struct device *dev, void __iomem *addr)
EXPORT_SYMBOL(devm_ioport_unmap);
#endif /* CONFIG_HAS_IOPORT_MAP */
-#ifdef CONFIG_PCI
-/*
- * PCI iomap devres
- */
-#define PCIM_IOMAP_MAX PCI_STD_NUM_BARS
-
-struct pcim_iomap_devres {
- void __iomem *table[PCIM_IOMAP_MAX];
-};
-
-static void pcim_iomap_release(struct device *gendev, void *res)
-{
- struct pci_dev *dev = to_pci_dev(gendev);
- struct pcim_iomap_devres *this = res;
- int i;
-
- for (i = 0; i < PCIM_IOMAP_MAX; i++)
- if (this->table[i])
- pci_iounmap(dev, this->table[i]);
-}
-
-/**
- * pcim_iomap_table - access iomap allocation table
- * @pdev: PCI device to access iomap table for
- *
- * Access iomap allocation table for @dev. If iomap table doesn't
- * exist and @pdev is managed, it will be allocated. All iomaps
- * recorded in the iomap table are automatically unmapped on driver
- * detach.
- *
- * This function might sleep when the table is first allocated but can
- * be safely called without context and guaranteed to succeed once
- * allocated.
- */
-void __iomem * const *pcim_iomap_table(struct pci_dev *pdev)
-{
- struct pcim_iomap_devres *dr, *new_dr;
-
- dr = devres_find(&pdev->dev, pcim_iomap_release, NULL, NULL);
- if (dr)
- return dr->table;
-
- new_dr = devres_alloc_node(pcim_iomap_release, sizeof(*new_dr), GFP_KERNEL,
- dev_to_node(&pdev->dev));
- if (!new_dr)
- return NULL;
- dr = devres_get(&pdev->dev, new_dr, NULL, NULL);
- return dr->table;
-}
-EXPORT_SYMBOL(pcim_iomap_table);
-
-/**
- * pcim_iomap - Managed pcim_iomap()
- * @pdev: PCI device to iomap for
- * @bar: BAR to iomap
- * @maxlen: Maximum length of iomap
- *
- * Managed pci_iomap(). Map is automatically unmapped on driver
- * detach.
- */
-void __iomem *pcim_iomap(struct pci_dev *pdev, int bar, unsigned long maxlen)
-{
- void __iomem **tbl;
-
- BUG_ON(bar >= PCIM_IOMAP_MAX);
-
- tbl = (void __iomem **)pcim_iomap_table(pdev);
- if (!tbl || tbl[bar]) /* duplicate mappings not allowed */
- return NULL;
-
- tbl[bar] = pci_iomap(pdev, bar, maxlen);
- return tbl[bar];
-}
-EXPORT_SYMBOL(pcim_iomap);
-
-/**
- * pcim_iounmap - Managed pci_iounmap()
- * @pdev: PCI device to iounmap for
- * @addr: Address to unmap
- *
- * Managed pci_iounmap(). @addr must have been mapped using pcim_iomap().
- */
-void pcim_iounmap(struct pci_dev *pdev, void __iomem *addr)
-{
- void __iomem **tbl;
- int i;
-
- pci_iounmap(pdev, addr);
-
- tbl = (void __iomem **)pcim_iomap_table(pdev);
- BUG_ON(!tbl);
-
- for (i = 0; i < PCIM_IOMAP_MAX; i++)
- if (tbl[i] == addr) {
- tbl[i] = NULL;
- return;
- }
- WARN_ON(1);
-}
-EXPORT_SYMBOL(pcim_iounmap);
-
-/**
- * pcim_iomap_regions - Request and iomap PCI BARs
- * @pdev: PCI device to map IO resources for
- * @mask: Mask of BARs to request and iomap
- * @name: Name used when requesting regions
- *
- * Request and iomap regions specified by @mask.
- */
-int pcim_iomap_regions(struct pci_dev *pdev, int mask, const char *name)
-{
- void __iomem * const *iomap;
- int i, rc;
-
- iomap = pcim_iomap_table(pdev);
- if (!iomap)
- return -ENOMEM;
-
- for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
- unsigned long len;
-
- if (!(mask & (1 << i)))
- continue;
-
- rc = -EINVAL;
- len = pci_resource_len(pdev, i);
- if (!len)
- goto err_inval;
-
- rc = pci_request_region(pdev, i, name);
- if (rc)
- goto err_inval;
-
- rc = -ENOMEM;
- if (!pcim_iomap(pdev, i, 0))
- goto err_region;
- }
-
- return 0;
-
- err_region:
- pci_release_region(pdev, i);
- err_inval:
- while (--i >= 0) {
- if (!(mask & (1 << i)))
- continue;
- pcim_iounmap(pdev, iomap[i]);
- pci_release_region(pdev, i);
- }
-
- return rc;
-}
-EXPORT_SYMBOL(pcim_iomap_regions);
-
-/**
- * pcim_iomap_regions_request_all - Request all BARs and iomap specified ones
- * @pdev: PCI device to map IO resources for
- * @mask: Mask of BARs to iomap
- * @name: Name used when requesting regions
- *
- * Request all PCI BARs and iomap regions specified by @mask.
- */
-int pcim_iomap_regions_request_all(struct pci_dev *pdev, int mask,
- const char *name)
-{
- int request_mask = ((1 << 6) - 1) & ~mask;
- int rc;
-
- rc = pci_request_selected_regions(pdev, request_mask, name);
- if (rc)
- return rc;
-
- rc = pcim_iomap_regions(pdev, mask, name);
- if (rc)
- pci_release_selected_regions(pdev, request_mask);
- return rc;
-}
-EXPORT_SYMBOL(pcim_iomap_regions_request_all);
-
-/**
- * pcim_iounmap_regions - Unmap and release PCI BARs
- * @pdev: PCI device to map IO resources for
- * @mask: Mask of BARs to unmap and release
- *
- * Unmap and release regions specified by @mask.
- */
-void pcim_iounmap_regions(struct pci_dev *pdev, int mask)
-{
- void __iomem * const *iomap;
- int i;
-
- iomap = pcim_iomap_table(pdev);
- if (!iomap)
- return;
-
- for (i = 0; i < PCIM_IOMAP_MAX; i++) {
- if (!(mask & (1 << i)))
- continue;
-
- pcim_iounmap(pdev, iomap[i]);
- pci_release_region(pdev, i);
- }
-}
-EXPORT_SYMBOL(pcim_iounmap_regions);
-#endif /* CONFIG_PCI */
-
static void devm_arch_phys_ac_add_release(struct device *dev, void *res)
{
arch_phys_wc_del(*((int *)res));
--
2.43.0
On Fri, Dec 1, 2023, at 13:16, Philipp Stanner wrote:
> The pcim_*() functions in lib/devres.c are guarded by an #ifdef
> CONFIG_PCI and, thus, don't belong to this file. They are only ever used
> for pci and are not generic infrastructure.
>
> Move all pcim_*() functions in lib/devres.c to drivers/pci/devres.c.
> Adjust the Makefile.
>
> Suggested-by: Danilo Krummrich <[email protected]>
> Signed-off-by: Philipp Stanner <[email protected]>
> ---
> drivers/pci/Makefile | 2 +-
> drivers/pci/devres.c | 207 ++++++++++++++++++++++++++++++++++++++++++
> lib/devres.c | 208 +------------------------------------------
I still think this should go into drivers/pci/pci_iomap.c along
with the other functions.
Arnd
On Fri, Dec 1, 2023, at 13:16, Philipp Stanner wrote:
> The implementation of pci_iounmap() is currently scattered over two
> files, drivers/pci/iounmap.c and lib/iomap.c. Additionally,
> architectures can define their own version.
>
> Besides one unified version being desirable in the first place, the old
> version in drivers/pci/iounmap.c contained a bug and could leak memory
> mappings. The bug was that #ifdef ARCH_HAS_GENERIC_IOPORT_MAP should not
> have guarded iounmap(p); in addition to the preceding code.
>
> 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.
>
> iomem_is_ioport() can be provided through three different ways:
> 1. The architecture itself provides it.
> 2. As a default version in include/asm-generic/io.h for those
> architectures that don't use CONFIG_GENERIC_IOMAP, but also don't
> provide their own version of iomem_is_ioport().
> 3. As a default version in lib/iomap.c for those architectures that
> define and use CONFIG_GENERIC_IOMAP (currently, only x86 really
> uses the functions in lib/iomap.c)
I would count 3 as a special case of 1 here.
> Create a unified version of pci_iounmap() in drivers/pci/iomap.c.
> Provide the function iomem_is_ioport() in include/asm-generic/io.h and
> lib/iomap.c.
>
> 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.
>
> Fixes: 316e8d79a095 ("pci_iounmap'2: Electric Boogaloo: try to make
> sense of it all")
> Suggested-by: Arnd Bergmann <[email protected]>
> Signed-off-by: Philipp Stanner <[email protected]>
Looks good overall. It would be nice to go further than this
and replace all the custom pci_iounmap() variants with custom
iomem_is_ioport() implementations, but that can be a follow-up
along with removing the incorrect or useless 'select GENERIC_IOMAP'
parts.
> return;
> - iounmap(p);
> + }
> #endif
> + iounmap(addr);
> }
I think the bugfix should be a separate patch so we can backport
it to stable kernels.
> +#ifndef CONFIG_GENERIC_IOMAP
> +static inline bool iomem_is_ioport(void __iomem *addr)
> +{
> + unsigned long port = (unsigned long __force)addr;
> +
> + // TODO: do we have to take IO_SPACE_LIMIT and PCI_IOBASE into account
> + // similar as in ioport_map() ?
> +
> + if (port > MMIO_UPPER_LIMIT)
> + return false;
> +
> + return true;
> +}
This has to have the exact logic that was present in the
old pci_iounmap(). For the default version that is currently
in lib/pci_iomap.c, this means something along the linens of
static inline bool struct iomem_is_ioport(void __iomem *p)
{
#ifdef CONFIG_HAS_IOPORT
uintptr_t start = (uintptr_t) PCI_IOBASE;
uintptr_t addr = (uintptr_t) p;
if (addr >= start && addr < start + IO_SPACE_LIMIT)
return true;
#endif
return false;
}
> +#else /* CONFIG_GENERIC_IOMAP. Version from lib/iomap.c will be used.
> */
> +bool iomem_is_ioport(void __iomem *addr);
> +#define ARCH_WANTS_GENERIC_IOMEM_IS_IOPORT
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
Arnd
On Fri, Dec 1, 2023, at 13:16, Philipp Stanner wrote:
>
> Arnd has suggested that architectures defining a custom inb() need their
> own iomem_is_ioport(), as well. I've grepped for inb() and found the
> following list of archs that define their own:
> - alpha
> - arm
> - m68k <--
> - parisc
> - powerpc
> - sh
> - sparc
> - x86 <--
>
> All of those have their own definitons of pci_iounmap(). Therefore, they
> don't need our generic version in the first place and, thus, also need
> no iomem_is_ioport().
What I meant of course is that they should define iomem_is_ioport()
in order to drop the custom pci_iounmap() and have only one remaining
definition of that function left.
The one special case that I missed the last time is s390, which
does not use GENERIC_PCI_IOMAP and will just require a separate
copy of pci_iounmap() to go along with the is custom pci_iomap().
> The two exceptions are x86 and m68k. The former uses lib/iomap.c through
> CONFIG_GENERIC_IOMAP, as Arnd pointed out in the previous discussion
> (thus, CONFIG_GENERIC_IOMAP is not really generic in this regard).
>
> So as I see it, only m68k WOULD need its own custom definition of
> iomem_is_ioport(). But as I understand it it doesn't because it uses the
> one from asm-generic/pci_iomap.h ??
At the moment, m68k gets the pci_iounmap() from lib/iomap.c
if PCI is enabled for coldfire, but that incorrectly calls
iounmap() on PCI_IO_PA if it gets passed a PIO address.
The version from asm-generic/io.h should fix this.
For classic m68k, there is no PCI, so nothing calls pci_iounmap().
> I wasn't entirely sure how to deal with the address ranges for the
> generic implementation in asm-generic/io.h. It's marked with a TODO.
> Input appreciated.
I commented on the function directly. To clarify, I think we should
be able to directly turn each pci_iounmap() definition into
a iomem_is_ioport() definition by keeping the logic unchanged
and just return 'true' for the PIO variant or 'false' for the MMIO
version.
> I removed the guard around define pci_iounmap in asm-generic/io.h. An
> alternative would be to have it be guarded by CONFIG_GENERIC_IOMAP and
> CONFIG_GENERIC_PCI_IOMAP, both. Without such a guard, there is no
> collision however, because generic pci_iounmap() from
> drivers/pci/iomap.c will only get pulled in when
> CONFIG_GENERIC_PCI_IOMAP is actually set.
The "#define pci_iomap" can be removed entirely I think.
Arnd
On Fri, 2023-12-01 at 15:44 +0100, Arnd Bergmann wrote:
> On Fri, Dec 1, 2023, at 13:16, Philipp Stanner wrote:
> > The pcim_*() functions in lib/devres.c are guarded by an #ifdef
> > CONFIG_PCI and, thus, don't belong to this file. They are only ever
> > used
> > for pci and are not generic infrastructure.
> >
> > Move all pcim_*() functions in lib/devres.c to
> > drivers/pci/devres.c.
> > Adjust the Makefile.
> >
> > Suggested-by: Danilo Krummrich <[email protected]>
> > Signed-off-by: Philipp Stanner <[email protected]>
> > ---
> > drivers/pci/Makefile | 2 +-
> > drivers/pci/devres.c | 207
> > ++++++++++++++++++++++++++++++++++++++++++
> > lib/devres.c | 208 +--------------------------------------
> > ----
>
> I still think this should go into drivers/pci/pci_iomap.c along
> with the other functions.
I understand where you're coming from, but the technical reason I'm
doing it this way is the same topic as in patch №1:
@@ -14,6 +14,7 @@ ifdef CONFIG_PCI
obj-$(CONFIG_PROC_FS) += proc.o
obj-$(CONFIG_SYSFS) += slot.o
obj-$(CONFIG_ACPI) += pci-acpi.o
+obj-$(CONFIG_GENERIC_PCI_IOMAP) += iomap.o
endif
As you pointed out very correctly, I removed the #ifdef PCI in the
iomap.c functions, which would result in build failure, because the
file has to be made empty if GENERIC_PCI_IOMAP is selected and PCI is
not
The devres functions have different compile rules than the iomap
functions have.
I would dislike it very much to need yet another preprocessor
instruction, especially if we're talking about #ifdef PCI within the
very PCI driver.
P.
>
> Arnd
>
On Fri, 2023-12-01 at 17:27 +0100, Arnd Bergmann wrote:
> On Fri, Dec 1, 2023, at 13:16, Philipp Stanner wrote:
> >
> > Arnd has suggested that architectures defining a custom inb() need
> > their
> > own iomem_is_ioport(), as well. I've grepped for inb() and found
> > the
> > following list of archs that define their own:
> > - alpha
> > - arm
> > - m68k <--
> > - parisc
> > - powerpc
> > - sh
> > - sparc
> > - x86 <--
> >
> > All of those have their own definitons of pci_iounmap(). Therefore,
> > they
> > don't need our generic version in the first place and, thus, also
> > need
> > no iomem_is_ioport().
>
> What I meant of course is that they should define iomem_is_ioport()
> in order to drop the custom pci_iounmap() and have only one remaining
> definition of that function left.
Ah, gotcha!
Yes, that would be neat. Would also allow for droping
ARCH_WANTS_GENERIC_PCI_IOUNMAP.
>
> The one special case that I missed the last time is s390, which
> does not use GENERIC_PCI_IOMAP and will just require a separate
> copy of pci_iounmap() to go along with the is custom pci_iomap().
>
> > The two exceptions are x86 and m68k. The former uses lib/iomap.c
> > through
> > CONFIG_GENERIC_IOMAP, as Arnd pointed out in the previous
> > discussion
> > (thus, CONFIG_GENERIC_IOMAP is not really generic in this regard).
> >
> > So as I see it, only m68k WOULD need its own custom definition of
> > iomem_is_ioport(). But as I understand it it doesn't because it
> > uses the
> > one from asm-generic/pci_iomap.h ??
>
> At the moment, m68k gets the pci_iounmap() from lib/iomap.c
> if PCI is enabled for coldfire, but that incorrectly calls
> iounmap() on PCI_IO_PA if it gets passed a PIO address.
>
> The version from asm-generic/io.h should fix this.
So, to be sure: m68k will use the generic iomem_is_ioport() despite
defining its own inb()?
>
> For classic m68k, there is no PCI, so nothing calls pci_iounmap().
>
> > I wasn't entirely sure how to deal with the address ranges for the
> > generic implementation in asm-generic/io.h. It's marked with a
> > TODO.
> > Input appreciated.
>
> I commented on the function directly. To clarify, I think we should
> be able to directly turn each pci_iounmap() definition into
> a iomem_is_ioport() definition by keeping the logic unchanged
> and just return 'true' for the PIO variant or 'false' for the MMIO
> version.
>
> > I removed the guard around define pci_iounmap in asm-generic/io.h.
> > An
> > alternative would be to have it be guarded by CONFIG_GENERIC_IOMAP
> > and
> > CONFIG_GENERIC_PCI_IOMAP, both. Without such a guard, there is no
> > collision however, because generic pci_iounmap() from
> > drivers/pci/iomap.c will only get pulled in when
> > CONFIG_GENERIC_PCI_IOMAP is actually set.
>
> The "#define pci_iomap" can be removed entirely I think.
I also think it can, because first arch/asm/io.h includes asm-
generic/io.h.
I was just wondering why many other functions in asm-generic/io.h
always define their own names..
It's obviously very hard to test which config will break, so I thought
it's better safe than sorry here
P.
>
> Arnd
>
On Fri, 2023-12-01 at 16:26 +0100, Arnd Bergmann wrote:
> On Fri, Dec 1, 2023, at 13:16, Philipp Stanner wrote:
> > The implementation of pci_iounmap() is currently scattered over two
> > files, drivers/pci/iounmap.c and lib/iomap.c. Additionally,
> > architectures can define their own version.
> >
> > Besides one unified version being desirable in the first place, the
> > old
> > version in drivers/pci/iounmap.c contained a bug and could leak
> > memory
> > mappings. The bug was that #ifdef ARCH_HAS_GENERIC_IOPORT_MAP
> > should not
> > have guarded iounmap(p); in addition to the preceding code.
> >
> > 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.
> >
> > iomem_is_ioport() can be provided through three different ways:
> > 1. The architecture itself provides it.
> > 2. As a default version in include/asm-generic/io.h for those
> > architectures that don't use CONFIG_GENERIC_IOMAP, but also
> > don't
> > provide their own version of iomem_is_ioport().
> > 3. As a default version in lib/iomap.c for those architectures
> > that
> > define and use CONFIG_GENERIC_IOMAP (currently, only x86
> > really
> > uses the functions in lib/iomap.c)
>
> I would count 3 as a special case of 1 here.
ACK
>
> > Create a unified version of pci_iounmap() in drivers/pci/iomap.c.
> > Provide the function iomem_is_ioport() in include/asm-generic/io.h
> > and
> > lib/iomap.c.
> >
> > 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.
> >
> > Fixes: 316e8d79a095 ("pci_iounmap'2: Electric Boogaloo: try to make
> > sense of it all")
> > Suggested-by: Arnd Bergmann <[email protected]>
> > Signed-off-by: Philipp Stanner <[email protected]>
>
> Looks good overall. It would be nice to go further than this
> and replace all the custom pci_iounmap() variants with custom
> iomem_is_ioport() implementations, but that can be a follow-up
> along with removing the incorrect or useless 'select GENERIC_IOMAP'
> parts.
Yes, let's schedule that for a follow up. The way my project plans
sound currently, it's likely that I'll stay close to PCI for the next
months anyways, so it's likely we'll get an opportunity to pick this up
on the run
>
> > return;
> > - iounmap(p);
> > + }
> > #endif
> > + iounmap(addr);
> > }
>
> I think the bugfix should be a separate patch so we can backport
> it to stable kernels.
ACK, good idea
>
> > +#ifndef CONFIG_GENERIC_IOMAP
> > +static inline bool iomem_is_ioport(void __iomem *addr)
> > +{
> > + unsigned long port = (unsigned long __force)addr;
> > +
> > + // TODO: do we have to take IO_SPACE_LIMIT and PCI_IOBASE
> > into account
> > + // similar as in ioport_map() ?
> > +
> > + if (port > MMIO_UPPER_LIMIT)
> > + return false;
> > +
> > + return true;
> > +}
>
> This has to have the exact logic that was present in the
> old pci_iounmap(). For the default version that is currently
> in lib/pci_iomap.c, this means something along the linens of
OK, I see, so iomem_is_ioport() takes the form derived from
lib/pci_iomap.c for asm-generic/io.h, and the form of lib/iomap.c for
the one in lib/iomap.c (obviously)
>
> static inline bool struct iomem_is_ioport(void __iomem *p)
> {
> #ifdef CONFIG_HAS_IOPORT
> uintptr_t start = (uintptr_t) PCI_IOBASE;
> uintptr_t addr = (uintptr_t) p;
>
> if (addr >= start && addr < start + IO_SPACE_LIMIT)
> return true;
> #endif
> return false;
> }
>
> > +#else /* CONFIG_GENERIC_IOMAP. Version from lib/iomap.c will be
> > used.
> > */
> > +bool iomem_is_ioport(void __iomem *addr);
> > +#define ARCH_WANTS_GENERIC_IOMEM_IS_IOPORT
>
> 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
Hmmm well now it's getting tricky.
This else-branch is the one where CONFIG_GENERIC_IOMAP is actually set.
I think we're running into the "generic not being generic now that IA64
has died" problem you were hinting at.
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?
P.
>
> Arnd
>
On Fri, Dec 1, 2023, at 20:37, Philipp Stanner wrote:
> On Fri, 2023-12-01 at 16:26 +0100, Arnd Bergmann wrote:
>>
>> static inline bool struct iomem_is_ioport(void __iomem *p)
>> {
>> #ifdef CONFIG_HAS_IOPORT
>> uintptr_t start = (uintptr_t) PCI_IOBASE;
>> uintptr_t addr = (uintptr_t) p;
>>
>> if (addr >= start && addr < start + IO_SPACE_LIMIT)
>> return true;
>> #endif
>> return false;
>> }
>>
>> > +#else /* CONFIG_GENERIC_IOMAP. Version from lib/iomap.c will be
>> > used.
>> > */
>> > +bool iomem_is_ioport(void __iomem *addr);
>> > +#define ARCH_WANTS_GENERIC_IOMEM_IS_IOPORT
>>
>> 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
>
> Hmmm well now it's getting tricky.
>
> This else-branch is the one where CONFIG_GENERIC_IOMAP is actually set.
>
> I think we're running into the "generic not being generic now that IA64
> has died" problem you were hinting at.
>
> 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?
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.
The default version in asm-generic/io.h then just needs
one more #ifdef iomem_is_ioport check around it.
Arnd
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:
> > >
> > > static inline bool struct iomem_is_ioport(void __iomem *p)
> > > {
> > > #ifdef CONFIG_HAS_IOPORT
> > > uintptr_t start = (uintptr_t) PCI_IOBASE;
> > > uintptr_t addr = (uintptr_t) p;
> > >
> > > if (addr >= start && addr < start + IO_SPACE_LIMIT)
> > > return true;
> > > #endif
> > > return false;
> > > }
> > >
> > > > +#else /* CONFIG_GENERIC_IOMAP. Version from lib/iomap.c will
> > > > be
> > > > used.
> > > > */
> > > > +bool iomem_is_ioport(void __iomem *addr);
> > > > +#define ARCH_WANTS_GENERIC_IOMEM_IS_IOPORT
> > >
> > > 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
> >
> > Hmmm well now it's getting tricky.
> >
> > This else-branch is the one where CONFIG_GENERIC_IOMAP is actually
> > set.
> >
> > I think we're running into the "generic not being generic now that
> > IA64
> > has died" problem you were hinting at.
> >
> > 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?
>
> 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 = (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.
>
> The default version in asm-generic/io.h then just needs
> one more #ifdef iomem_is_ioport check around it.
>
> Arnd
>
On Fri, Dec 1, 2023, at 22:56, Philipp Stanner wrote:
> On Fri, 2023-12-01 at 22:32 +0100, Arnd Bergmann wrote:
>> On Fri, Dec 1, 2023, at 20:37, Philipp Stanner wrote:
>> 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 = (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.
Right, in that case it's probably better to leave it in lib/iomap.c,
just keep the ARCH_WANTS_GENERIC_IOMEM_IS_IOPORT definition
in include/asm-generic/iomap.h as well then and keep it out of
the normal asm-generic/io.h path.
Arnd
On Fri, Dec 1, 2023, at 20:09, Philipp Stanner wrote:
> On Fri, 2023-12-01 at 17:27 +0100, Arnd Bergmann wrote:
>> On Fri, Dec 1, 2023, at 13:16, Philipp Stanner wrote:
>>
>> The one special case that I missed the last time is s390, which
>> does not use GENERIC_PCI_IOMAP and will just require a separate
>> copy of pci_iounmap() to go along with the is custom pci_iomap().
>>
>> > The two exceptions are x86 and m68k. The former uses lib/iomap.c
>> > through
>> > CONFIG_GENERIC_IOMAP, as Arnd pointed out in the previous
>> > discussion
>> > (thus, CONFIG_GENERIC_IOMAP is not really generic in this regard).
>> >
>> > So as I see it, only m68k WOULD need its own custom definition of
>> > iomem_is_ioport(). But as I understand it it doesn't because it
>> > uses the
>> > one from asm-generic/pci_iomap.h ??
>>
>> At the moment, m68k gets the pci_iounmap() from lib/iomap.c
>> if PCI is enabled for coldfire, but that incorrectly calls
>> iounmap() on PCI_IO_PA if it gets passed a PIO address.
>>
>> The version from asm-generic/io.h should fix this.
>
> So, to be sure: m68k will use the generic iomem_is_ioport() despite
> defining its own inb()?
It depends, as m68k has two separate asm/io.h implementations:
- arch/m68k/include/asm/io_no.h uses the default inb()
from asm-generic/io.h, so it should use the asm-generic
version of iomem_is_ioport().
- arch/m68k/include/asm/io_mm.h is rather special when
it comes to inb()/outb(), but since there is no PCI,
I would just use the default iomem_is_ioport() because
it doesn't matter as long as there are no callers.
If we ever need a working iomem_is_ioport() here, it would
need the same special cases as isa_itb().
>> The "#define pci_iomap" can be removed entirely I think.
>
> I also think it can, because first arch/asm/io.h includes asm-
> generic/io.h.
> I was just wondering why many other functions in asm-generic/io.h
> always define their own names..
>
> It's obviously very hard to test which config will break, so I thought
> it's better safe than sorry here
I'm fairly sure it's not actually needed, but since the entire file
does it, there is probably no harm keeping it consistent for the next
added function.
This is one more thing to maybe clean up eventually in the future,
possibly as part of moving the contents of asm-generic/io.h into
linux/io.h, which is something I'd like to do now that all
architectures finally started using the asm-generic version.
Arnd
On Fri, Dec 1, 2023, at 20:00, Philipp Stanner wrote:
>
> The devres functions have different compile rules than the iomap
> functions have.
>
> I would dislike it very much to need yet another preprocessor
> instruction, especially if we're talking about #ifdef PCI within the
> very PCI driver.
Ah right, I had forgotten about s390 zpci being special here,
otherwise we wouldn't need an #ifdef here.
Arnd