LinuxLists.cc - [PATCH 0/4] Regather scattered PCI-Code

[permalink] [raw]

Subject: [PATCH 1/4] lib: move pci_iomap.c to drivers/pci/

This file is guarded by an #ifdef CONFIG_PCI. It, consequently, does not
belong to lib/ because it is not generic infrastructure.

Move the file to drivers/pci/ and implement the necessary changes to
Makefiles and Kconfigs.

Suggested-by: Danilo Krummrich <[email protected]>
Signed-off-by: Philipp Stanner <[email protected]>
---
drivers/pci/Kconfig | 3 +++
drivers/pci/Makefile | 1 +
lib/pci_iomap.c => drivers/pci/iomap.c | 3 ---
lib/Kconfig | 3 ---
lib/Makefile | 1 -
5 files changed, 4 insertions(+), 7 deletions(-)
rename lib/pci_iomap.c => drivers/pci/iomap.c (99%)

diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig
index 74147262625b..b54cacbc1160 100644
--- a/drivers/pci/Kconfig
+++ b/drivers/pci/Kconfig
@@ -34,6 +34,9 @@ config PCI_DOMAINS_GENERIC
config PCI_SYSCALL
bool

+config GENERIC_PCI_IOMAP
+ bool
+
source "drivers/pci/pcie/Kconfig"

config PCI_MSI
diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile
index cc8b4e01e29d..d6d0abfe59e2 100644
--- a/drivers/pci/Makefile
+++ b/drivers/pci/Makefile
@@ -33,6 +33,7 @@ obj-$(CONFIG_XEN_PCIDEV_FRONTEND) += xen-pcifront.o
obj-$(CONFIG_VGA_ARB) += vgaarb.o
obj-$(CONFIG_PCI_DOE) += doe.o
obj-$(CONFIG_PCI_DYNAMIC_OF_NODES) += of_property.o
+obj-$(CONFIG_GENERIC_PCI_IOMAP) += iomap.o

# Endpoint library must be initialized before its users
obj-$(CONFIG_PCI_ENDPOINT) += endpoint/
diff --git a/lib/pci_iomap.c b/drivers/pci/iomap.c
similarity index 99%
rename from lib/pci_iomap.c
rename to drivers/pci/iomap.c
index ce39ce9f3526..0a9d503ba533 100644
--- a/lib/pci_iomap.c
+++ b/drivers/pci/iomap.c
@@ -9,7 +9,6 @@

#include <linux/export.h>

-#ifdef CONFIG_PCI
/**
* pci_iomap_range - create a virtual mapping cookie for a PCI BAR
* @dev: PCI device that owns the BAR
@@ -176,5 +175,3 @@ void pci_iounmap(struct pci_dev *dev, void __iomem *p)
EXPORT_SYMBOL(pci_iounmap);

#endif /* ARCH_WANTS_GENERIC_PCI_IOUNMAP */
-
-#endif /* CONFIG_PCI */
diff --git a/lib/Kconfig b/lib/Kconfig
index 3ea1c830efab..1bf859166ac7 100644
--- a/lib/Kconfig
+++ b/lib/Kconfig
@@ -70,9 +70,6 @@ source "lib/math/Kconfig"
config NO_GENERIC_PCI_IOPORT_MAP
bool

-config GENERIC_PCI_IOMAP
- bool
-
config GENERIC_IOMAP
bool
select GENERIC_PCI_IOMAP
diff --git a/lib/Makefile b/lib/Makefile
index 6b09731d8e61..0800289ec6c5 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -153,7 +153,6 @@ CFLAGS_debug_info.o += $(call cc-option, -femit-struct-debug-detailed=any)
obj-y += math/ crypto/

obj-$(CONFIG_GENERIC_IOMAP) += iomap.o
-obj-$(CONFIG_GENERIC_PCI_IOMAP) += pci_iomap.o
obj-$(CONFIG_HAS_IOMEM) += iomap_copy.o devres.o
obj-$(CONFIG_CHECK_SIGNATURE) += check_signature.o
obj-$(CONFIG_DEBUG_LOCKING_API_SELFTESTS) += locking-selftest.o
--
2.41.0

2023-11-20 22:04:07

[permalink] [raw]

Subject: [PATCH 2/4] lib: move pci-specific devres code to drivers/pci/

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 not generic infrastructure.

Move all pcim_*() functions in lib/devres.c to drivers/pci/devres.c

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 d6d0abfe59e2..e04a492d4465 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.41.0

2023-11-20 22:04:10

[permalink] [raw]

Subject: [PATCH 3/4] pci: move devres code from pci.c to devres.c

The file pci.c is very large and contains a number of devres-functions.
These functions should now reside in devres.c

There are a few callers left in pci.c that do devres operations. These
should be ported in the future. Corresponding TODOs are added by this
commit.

The reason they are not moved right now in this commit is that pci's
devres currently implements a sort of "hybrid-mode":
pci_request_region(), for instance, does not have a corresponding pcim_
equivalent, yet. Instead, the function can be made managed by previously
calling pcim_enable_device() (instead of pci_enable_device()). This
makes it unreasonable to move pci_request_region() to devres.c
Moving the functions would require changes to pci's API and is,
therefore, left for future work.

Move as much devres-specific code from pci.c to devres.c as possible.

Suggested-by: Danilo Krummrich <[email protected]>
Signed-off-by: Philipp Stanner <[email protected]>
---
drivers/pci/devres.c | 243 +++++++++++++++++++++++++++++++++++++++++
drivers/pci/pci.c | 249 -------------------------------------------
drivers/pci/pci.h | 24 +++++
3 files changed, 267 insertions(+), 249 deletions(-)

diff --git a/drivers/pci/devres.c b/drivers/pci/devres.c
index a3fd0d65cef1..55f76a2c3748 100644
--- a/drivers/pci/devres.c
+++ b/drivers/pci/devres.c
@@ -1,4 +1,5 @@
// SPDX-License-Identifier: GPL-2.0
+#include <linux/device.h>
#include <linux/pci.h>
#include "pci.h"

@@ -11,6 +12,248 @@ struct pcim_iomap_devres {
void __iomem *table[PCIM_IOMAP_MAX];
};

+
+static void devm_pci_unmap_iospace(struct device *dev, void *ptr)
+{
+ struct resource **res = ptr;
+
+ pci_unmap_iospace(*res);
+}
+
+/**
+ * devm_pci_remap_iospace - Managed pci_remap_iospace()
+ * @dev: Generic device to remap IO address for
+ * @res: Resource describing the I/O space
+ * @phys_addr: physical address of range to be mapped
+ *
+ * Managed pci_remap_iospace(). Map is automatically unmapped on driver
+ * detach.
+ */
+int devm_pci_remap_iospace(struct device *dev, const struct resource *res,
+ phys_addr_t phys_addr)
+{
+ const struct resource **ptr;
+ int error;
+
+ ptr = devres_alloc(devm_pci_unmap_iospace, sizeof(*ptr), GFP_KERNEL);
+ if (!ptr)
+ return -ENOMEM;
+
+ error = pci_remap_iospace(res, phys_addr);
+ if (error) {
+ devres_free(ptr);
+ } else {
+ *ptr = res;
+ devres_add(dev, ptr);
+ }
+
+ return error;
+}
+EXPORT_SYMBOL(devm_pci_remap_iospace);
+
+/**
+ * devm_pci_remap_cfgspace - Managed pci_remap_cfgspace()
+ * @dev: Generic device to remap IO address for
+ * @offset: Resource address to map
+ * @size: Size of map
+ *
+ * Managed pci_remap_cfgspace(). Map is automatically unmapped on driver
+ * detach.
+ */
+void __iomem *devm_pci_remap_cfgspace(struct device *dev,
+ resource_size_t offset,
+ resource_size_t size)
+{
+ void __iomem **ptr, *addr;
+
+ ptr = devres_alloc(devm_ioremap_release, sizeof(*ptr), GFP_KERNEL);
+ if (!ptr)
+ return NULL;
+
+ addr = pci_remap_cfgspace(offset, size);
+ if (addr) {
+ *ptr = addr;
+ devres_add(dev, ptr);
+ } else
+ devres_free(ptr);
+
+ return addr;
+}
+EXPORT_SYMBOL(devm_pci_remap_cfgspace);
+
+/**
+ * devm_pci_remap_cfg_resource - check, request region and ioremap cfg resource
+ * @dev: generic device to handle the resource for
+ * @res: configuration space resource to be handled
+ *
+ * Checks that a resource is a valid memory region, requests the memory
+ * region and ioremaps with pci_remap_cfgspace() API that ensures the
+ * proper PCI configuration space memory attributes are guaranteed.
+ *
+ * All operations are managed and will be undone on driver detach.
+ *
+ * Returns a pointer to the remapped memory or an ERR_PTR() encoded error code
+ * on failure. Usage example::
+ *
+ * res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ * base = devm_pci_remap_cfg_resource(&pdev->dev, res);
+ * if (IS_ERR(base))
+ * return PTR_ERR(base);
+ */
+void __iomem *devm_pci_remap_cfg_resource(struct device *dev,
+ struct resource *res)
+{
+ resource_size_t size;
+ const char *name;
+ void __iomem *dest_ptr;
+
+ BUG_ON(!dev);
+
+ if (!res || resource_type(res) != IORESOURCE_MEM) {
+ dev_err(dev, "invalid resource\n");
+ return IOMEM_ERR_PTR(-EINVAL);
+ }
+
+ size = resource_size(res);
+
+ if (res->name)
+ name = devm_kasprintf(dev, GFP_KERNEL, "%s %s", dev_name(dev),
+ res->name);
+ else
+ name = devm_kstrdup(dev, dev_name(dev), GFP_KERNEL);
+ if (!name)
+ return IOMEM_ERR_PTR(-ENOMEM);
+
+ if (!devm_request_mem_region(dev, res->start, size, name)) {
+ dev_err(dev, "can't request region for resource %pR\n", res);
+ return IOMEM_ERR_PTR(-EBUSY);
+ }
+
+ dest_ptr = devm_pci_remap_cfgspace(dev, res->start, size);
+ if (!dest_ptr) {
+ dev_err(dev, "ioremap failed for resource %pR\n", res);
+ devm_release_mem_region(dev, res->start, size);
+ dest_ptr = IOMEM_ERR_PTR(-ENOMEM);
+ }
+
+ return dest_ptr;
+}
+EXPORT_SYMBOL(devm_pci_remap_cfg_resource);
+
+/**
+ * pcim_set_mwi - a device-managed pci_set_mwi()
+ * @dev: the PCI device for which MWI is enabled
+ *
+ * Managed pci_set_mwi().
+ *
+ * RETURNS: An appropriate -ERRNO error value on error, or zero for success.
+ */
+int pcim_set_mwi(struct pci_dev *dev)
+{
+ struct pci_devres *dr;
+
+ dr = find_pci_dr(dev);
+ if (!dr)
+ return -ENOMEM;
+
+ dr->mwi = 1;
+ return pci_set_mwi(dev);
+}
+EXPORT_SYMBOL(pcim_set_mwi);
+
+
+static void pcim_release(struct device *gendev, void *res)
+{
+ struct pci_dev *dev = to_pci_dev(gendev);
+ struct pci_devres *this = res;
+ int i;
+
+ for (i = 0; i < DEVICE_COUNT_RESOURCE; i++)
+ if (this->region_mask & (1 << i))
+ pci_release_region(dev, i);
+
+ if (this->mwi)
+ pci_clear_mwi(dev);
+
+ if (this->restore_intx)
+ pci_intx(dev, this->orig_intx);
+
+ if (this->enabled && !this->pinned)
+ pci_disable_device(dev);
+}
+
+/*
+ * TODO:
+ * Once the last four callers in pci.c are ported, this function here needs to
+ * be made static again.
+ */
+struct pci_devres *find_pci_dr(struct pci_dev *pdev)
+{
+ if (pci_is_managed(pdev))
+ return devres_find(&pdev->dev, pcim_release, NULL, NULL);
+ return NULL;
+}
+EXPORT_SYMBOL(find_pci_dr); // TODO do we need this?
+
+static struct pci_devres *get_pci_dr(struct pci_dev *pdev)
+{
+ struct pci_devres *dr, *new_dr;
+
+ dr = devres_find(&pdev->dev, pcim_release, NULL, NULL);
+ if (dr)
+ return dr;
+
+ new_dr = devres_alloc(pcim_release, sizeof(*new_dr), GFP_KERNEL);
+ if (!new_dr)
+ return NULL;
+ return devres_get(&pdev->dev, new_dr, NULL, NULL);
+}
+
+/**
+ * pcim_enable_device - Managed pci_enable_device()
+ * @pdev: PCI device to be initialized
+ *
+ * Managed pci_enable_device().
+ */
+int pcim_enable_device(struct pci_dev *pdev)
+{
+ struct pci_devres *dr;
+ int rc;
+
+ dr = get_pci_dr(pdev);
+ if (unlikely(!dr))
+ return -ENOMEM;
+ if (dr->enabled)
+ return 0;
+
+ rc = pci_enable_device(pdev);
+ if (!rc) {
+ pdev->is_managed = 1;
+ dr->enabled = 1;
+ }
+ return rc;
+}
+EXPORT_SYMBOL(pcim_enable_device);
+
+/**
+ * pcim_pin_device - Pin managed PCI device
+ * @pdev: PCI device to pin
+ *
+ * Pin managed PCI device @pdev. Pinned device won't be disabled on
+ * driver detach. @pdev must have been enabled with
+ * pcim_enable_device().
+ */
+void pcim_pin_device(struct pci_dev *pdev)
+{
+ struct pci_devres *dr;
+
+ dr = find_pci_dr(pdev);
+ WARN_ON(!dr || !dr->enabled);
+ if (dr)
+ dr->pinned = 1;
+}
+EXPORT_SYMBOL(pcim_pin_device);
+
static void pcim_iomap_release(struct device *gendev, void *res)
{
struct pci_dev *dev = to_pci_dev(gendev);
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 55bc3576a985..742b0a6545b6 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -2088,107 +2088,6 @@ int pci_enable_device(struct pci_dev *dev)
}
EXPORT_SYMBOL(pci_enable_device);

-/*
- * Managed PCI resources. This manages device on/off, INTx/MSI/MSI-X
- * on/off and BAR regions. pci_dev itself records MSI/MSI-X status, so
- * there's no need to track it separately. pci_devres is initialized
- * when a device is enabled using managed PCI device enable interface.
- */
-struct pci_devres {
- unsigned int enabled:1;
- unsigned int pinned:1;
- unsigned int orig_intx:1;
- unsigned int restore_intx:1;
- unsigned int mwi:1;
- u32 region_mask;
-};
-
-static void pcim_release(struct device *gendev, void *res)
-{
- struct pci_dev *dev = to_pci_dev(gendev);
- struct pci_devres *this = res;
- int i;
-
- for (i = 0; i < DEVICE_COUNT_RESOURCE; i++)
- if (this->region_mask & (1 << i))
- pci_release_region(dev, i);
-
- if (this->mwi)
- pci_clear_mwi(dev);
-
- if (this->restore_intx)
- pci_intx(dev, this->orig_intx);
-
- if (this->enabled && !this->pinned)
- pci_disable_device(dev);
-}
-
-static struct pci_devres *get_pci_dr(struct pci_dev *pdev)
-{
- struct pci_devres *dr, *new_dr;
-
- dr = devres_find(&pdev->dev, pcim_release, NULL, NULL);
- if (dr)
- return dr;
-
- new_dr = devres_alloc(pcim_release, sizeof(*new_dr), GFP_KERNEL);
- if (!new_dr)
- return NULL;
- return devres_get(&pdev->dev, new_dr, NULL, NULL);
-}
-
-static struct pci_devres *find_pci_dr(struct pci_dev *pdev)
-{
- if (pci_is_managed(pdev))
- return devres_find(&pdev->dev, pcim_release, NULL, NULL);
- return NULL;
-}
-
-/**
- * pcim_enable_device - Managed pci_enable_device()
- * @pdev: PCI device to be initialized
- *
- * Managed pci_enable_device().
- */
-int pcim_enable_device(struct pci_dev *pdev)
-{
- struct pci_devres *dr;
- int rc;
-
- dr = get_pci_dr(pdev);
- if (unlikely(!dr))
- return -ENOMEM;
- if (dr->enabled)
- return 0;
-
- rc = pci_enable_device(pdev);
- if (!rc) {
- pdev->is_managed = 1;
- dr->enabled = 1;
- }
- return rc;
-}
-EXPORT_SYMBOL(pcim_enable_device);
-
-/**
- * pcim_pin_device - Pin managed PCI device
- * @pdev: PCI device to pin
- *
- * Pin managed PCI device @pdev. Pinned device won't be disabled on
- * driver detach. @pdev must have been enabled with
- * pcim_enable_device().
- */
-void pcim_pin_device(struct pci_dev *pdev)
-{
- struct pci_devres *dr;
-
- dr = find_pci_dr(pdev);
- WARN_ON(!dr || !dr->enabled);
- if (dr)
- dr->pinned = 1;
-}
-EXPORT_SYMBOL(pcim_pin_device);
-
/*
* pcibios_device_add - provide arch specific hooks when adding device dev
* @dev: the PCI device being added
@@ -4281,133 +4180,6 @@ void pci_unmap_iospace(struct resource *res)
}
EXPORT_SYMBOL(pci_unmap_iospace);

-static void devm_pci_unmap_iospace(struct device *dev, void *ptr)
-{
- struct resource **res = ptr;
-
- pci_unmap_iospace(*res);
-}
-
-/**
- * devm_pci_remap_iospace - Managed pci_remap_iospace()
- * @dev: Generic device to remap IO address for
- * @res: Resource describing the I/O space
- * @phys_addr: physical address of range to be mapped
- *
- * Managed pci_remap_iospace(). Map is automatically unmapped on driver
- * detach.
- */
-int devm_pci_remap_iospace(struct device *dev, const struct resource *res,
- phys_addr_t phys_addr)
-{
- const struct resource **ptr;
- int error;
-
- ptr = devres_alloc(devm_pci_unmap_iospace, sizeof(*ptr), GFP_KERNEL);
- if (!ptr)
- return -ENOMEM;
-
- error = pci_remap_iospace(res, phys_addr);
- if (error) {
- devres_free(ptr);
- } else {
- *ptr = res;
- devres_add(dev, ptr);
- }
-
- return error;
-}
-EXPORT_SYMBOL(devm_pci_remap_iospace);
-
-/**
- * devm_pci_remap_cfgspace - Managed pci_remap_cfgspace()
- * @dev: Generic device to remap IO address for
- * @offset: Resource address to map
- * @size: Size of map
- *
- * Managed pci_remap_cfgspace(). Map is automatically unmapped on driver
- * detach.
- */
-void __iomem *devm_pci_remap_cfgspace(struct device *dev,
- resource_size_t offset,
- resource_size_t size)
-{
- void __iomem **ptr, *addr;
-
- ptr = devres_alloc(devm_ioremap_release, sizeof(*ptr), GFP_KERNEL);
- if (!ptr)
- return NULL;
-
- addr = pci_remap_cfgspace(offset, size);
- if (addr) {
- *ptr = addr;
- devres_add(dev, ptr);
- } else
- devres_free(ptr);
-
- return addr;
-}
-EXPORT_SYMBOL(devm_pci_remap_cfgspace);
-
-/**
- * devm_pci_remap_cfg_resource - check, request region and ioremap cfg resource
- * @dev: generic device to handle the resource for
- * @res: configuration space resource to be handled
- *
- * Checks that a resource is a valid memory region, requests the memory
- * region and ioremaps with pci_remap_cfgspace() API that ensures the
- * proper PCI configuration space memory attributes are guaranteed.
- *
- * All operations are managed and will be undone on driver detach.
- *
- * Returns a pointer to the remapped memory or an ERR_PTR() encoded error code
- * on failure. Usage example::
- *
- * res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- * base = devm_pci_remap_cfg_resource(&pdev->dev, res);
- * if (IS_ERR(base))
- * return PTR_ERR(base);
- */
-void __iomem *devm_pci_remap_cfg_resource(struct device *dev,
- struct resource *res)
-{
- resource_size_t size;
- const char *name;
- void __iomem *dest_ptr;
-
- BUG_ON(!dev);
-
- if (!res || resource_type(res) != IORESOURCE_MEM) {
- dev_err(dev, "invalid resource\n");
- return IOMEM_ERR_PTR(-EINVAL);
- }
-
- size = resource_size(res);
-
- if (res->name)
- name = devm_kasprintf(dev, GFP_KERNEL, "%s %s", dev_name(dev),
- res->name);
- else
- name = devm_kstrdup(dev, dev_name(dev), GFP_KERNEL);
- if (!name)
- return IOMEM_ERR_PTR(-ENOMEM);
-
- if (!devm_request_mem_region(dev, res->start, size, name)) {
- dev_err(dev, "can't request region for resource %pR\n", res);
- return IOMEM_ERR_PTR(-EBUSY);
- }
-
- dest_ptr = devm_pci_remap_cfgspace(dev, res->start, size);
- if (!dest_ptr) {
- dev_err(dev, "ioremap failed for resource %pR\n", res);
- devm_release_mem_region(dev, res->start, size);
- dest_ptr = IOMEM_ERR_PTR(-ENOMEM);
- }
-
- return dest_ptr;
-}
-EXPORT_SYMBOL(devm_pci_remap_cfg_resource);
-
static void __pci_set_master(struct pci_dev *dev, bool enable)
{
u16 old_cmd, cmd;
@@ -4557,27 +4329,6 @@ int pci_set_mwi(struct pci_dev *dev)
}
EXPORT_SYMBOL(pci_set_mwi);

-/**
- * pcim_set_mwi - a device-managed pci_set_mwi()
- * @dev: the PCI device for which MWI is enabled
- *
- * Managed pci_set_mwi().
- *
- * RETURNS: An appropriate -ERRNO error value on error, or zero for success.
- */
-int pcim_set_mwi(struct pci_dev *dev)
-{
- struct pci_devres *dr;
-
- dr = find_pci_dr(dev);
- if (!dr)
- return -ENOMEM;
-
- dr->mwi = 1;
- return pci_set_mwi(dev);
-}
-EXPORT_SYMBOL(pcim_set_mwi);
-
/**
* pci_try_set_mwi - enables memory-write-invalidate PCI transaction
* @dev: the PCI device for which MWI is enabled
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 5ecbcf041179..69052059dbd2 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -793,6 +793,30 @@ static inline pci_power_t mid_pci_get_power_state(struct pci_dev *pdev)
}
#endif

+/*
+ * TODO:
+ * The following two components wouldn't need to be here if they weren't used at
+ * four last places in pci.c
+ * Port or move these functions to devres.c and then remove the
+ * pci_devres-components from this header file here.
+ */
+/*
+ * Managed PCI resources. This manages device on/off, INTx/MSI/MSI-X
+ * on/off and BAR regions. pci_dev itself records MSI/MSI-X status, so
+ * there's no need to track it separately. pci_devres is initialized
+ * when a device is enabled using managed PCI device enable interface.
+ */
+struct pci_devres {
+ unsigned int enabled:1;
+ unsigned int pinned:1;
+ unsigned int orig_intx:1;
+ unsigned int restore_intx:1;
+ unsigned int mwi:1;
+ u32 region_mask;
+};
+
+struct pci_devres *find_pci_dr(struct pci_dev *pdev);
+
/*
* Config Address for PCI Configuration Mechanism #1
*
--
2.41.0

2023-11-20 22:04:15

[permalink] [raw]

Subject: [PATCH 4/4] lib/iomap.c: improve comment about pci anomaly

lib/iomap.c contains one of the definitions of pci_iounmap(). The
current comment above this out-of-place function does not clarify WHY
the function is defined here.

Linus's detailed comment above pci_iounmap() in drivers/pci/iomap.c
clarifies that in a far better way.

Extend the existing comment with an excerpt from Linus's and hint at the
other implementation in drivers/pci/iomap.c

Signed-off-by: Philipp Stanner <[email protected]>
---
lib/iomap.c | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/lib/iomap.c b/lib/iomap.c
index 4f8b31baa575..647aac8ea3e3 100644
--- a/lib/iomap.c
+++ b/lib/iomap.c
@@ -419,8 +419,17 @@ 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. */
+/*
+ * Hide the details if this is a MMIO or PIO address space and just do what
+ * you expect in the correct way.
+ *
+ * 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.
+ *
+ * For more details see also the pci_iounmap() implementation in
+ * drivers/pci/iomap.c
+ */
void pci_iounmap(struct pci_dev *dev, void __iomem * addr)
{
IO_COND(addr, /* nothing */, iounmap(addr));
--
2.41.0

2023-11-21 04:21:55

[permalink] [raw]

Subject: Re: [PATCH 1/4] lib: move pci_iomap.c to drivers/pci/

Hi Philipp,

kernel test robot noticed the following build errors:

[auto build test ERROR on pci/next]
[also build test ERROR on pci/for-linus linus/master v6.7-rc2 next-20231120]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url: https://github.com/intel-lab-lkp/linux/commits/Philipp-Stanner/lib-move-pci_iomap-c-to-drivers-pci/20231121-060258
base: https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git next
patch link: https://lore.kernel.org/r/20231120215945.52027-3-pstanner%40redhat.com
patch subject: [PATCH 1/4] lib: move pci_iomap.c to drivers/pci/
config: openrisc-allnoconfig (https://download.01.org/0day-ci/archive/20231121/[email protected]/config)
compiler: or1k-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231121/[email protected]/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/

All error/warnings (new ones prefixed by >>):

>> drivers/pci/iomap.c:27:15: error: redefinition of 'pci_iomap_range'
27 | void __iomem *pci_iomap_range(struct pci_dev *dev,
| ^~~~~~~~~~~~~~~
In file included from include/asm-generic/io.h:20,
from arch/openrisc/include/asm/io.h:37,
from include/linux/io.h:13,
from include/linux/irq.h:20,
from include/asm-generic/hardirq.h:17,
from ./arch/openrisc/include/generated/asm/hardirq.h:1,
from include/linux/hardirq.h:11,
from include/linux/interrupt.h:11,
from include/linux/pci.h:38,
from drivers/pci/iomap.c:7:
include/asm-generic/pci_iomap.h:44:29: note: previous definition of 'pci_iomap_range' with type 'void *(struct pci_dev *, int, long unsigned int, long unsigned int)'
44 | static inline void __iomem *pci_iomap_range(struct pci_dev *dev, int bar,
| ^~~~~~~~~~~~~~~
drivers/pci/iomap.c: In function 'pci_iomap_range':
>> drivers/pci/iomap.c:43:24: error: implicit declaration of function '__pci_ioport_map'; did you mean 'devm_ioport_map'? [-Werror=implicit-function-declaration]
43 | return __pci_ioport_map(dev, start, len);
| ^~~~~~~~~~~~~~~~
| devm_ioport_map
>> drivers/pci/iomap.c:43:24: warning: returning 'int' from a function with return type 'void *' makes pointer from integer without a cast [-Wint-conversion]
43 | return __pci_ioport_map(dev, start, len);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/pci/iomap.c: At top level:
>> drivers/pci/iomap.c:67:15: error: redefinition of 'pci_iomap_wc_range'
67 | void __iomem *pci_iomap_wc_range(struct pci_dev *dev,
| ^~~~~~~~~~~~~~~~~~
include/asm-generic/pci_iomap.h:50:29: note: previous definition of 'pci_iomap_wc_range' with type 'void *(struct pci_dev *, int, long unsigned int, long unsigned int)'
50 | static inline void __iomem *pci_iomap_wc_range(struct pci_dev *dev, int bar,
| ^~~~~~~~~~~~~~~~~~
>> drivers/pci/iomap.c:110:15: error: redefinition of 'pci_iomap'
110 | void __iomem *pci_iomap(struct pci_dev *dev, int bar, unsigned long maxlen)
| ^~~~~~~~~
include/asm-generic/pci_iomap.h:35:29: note: previous definition of 'pci_iomap' with type 'void *(struct pci_dev *, int, long unsigned int)'
35 | static inline void __iomem *pci_iomap(struct pci_dev *dev, int bar, unsigned long max)
| ^~~~~~~~~
>> drivers/pci/iomap.c:131:15: error: redefinition of 'pci_iomap_wc'
131 | void __iomem *pci_iomap_wc(struct pci_dev *dev, int bar, unsigned long maxlen)
| ^~~~~~~~~~~~
include/asm-generic/pci_iomap.h:40:29: note: previous definition of 'pci_iomap_wc' with type 'void *(struct pci_dev *, int, long unsigned int)'
40 | static inline void __iomem *pci_iomap_wc(struct pci_dev *dev, int bar, unsigned long max)
| ^~~~~~~~~~~~
>> drivers/pci/iomap.c:164:6: error: redefinition of 'pci_iounmap'
164 | void pci_iounmap(struct pci_dev *dev, void __iomem *p)
| ^~~~~~~~~~~
include/asm-generic/pci_iomap.h:56:20: note: previous definition of 'pci_iounmap' with type 'void(struct pci_dev *, void *)'
56 | static inline void pci_iounmap(struct pci_dev *dev, void __iomem *addr)
| ^~~~~~~~~~~
cc1: some warnings being treated as errors

Kconfig warnings: (for reference only)
WARNING: unmet direct dependencies detected for GENERIC_PCI_IOMAP
Depends on [n]: PCI [=n]
Selected by [y]:
- OPENRISC [=y]

vim +/pci_iomap_range +27 drivers/pci/iomap.c

66eab4df288aae lib/pci_iomap.c Michael S. Tsirkin 2011-11-24 11
66eab4df288aae lib/pci_iomap.c Michael S. Tsirkin 2011-11-24 12 /**
eb29d8d2aad706 lib/pci_iomap.c Michael S. Tsirkin 2013-05-29 13 * pci_iomap_range - create a virtual mapping cookie for a PCI BAR
66eab4df288aae lib/pci_iomap.c Michael S. Tsirkin 2011-11-24 14 * @dev: PCI device that owns the BAR
66eab4df288aae lib/pci_iomap.c Michael S. Tsirkin 2011-11-24 15 * @bar: BAR number
eb29d8d2aad706 lib/pci_iomap.c Michael S. Tsirkin 2013-05-29 16 * @offset: map memory at the given offset in BAR
eb29d8d2aad706 lib/pci_iomap.c Michael S. Tsirkin 2013-05-29 17 * @maxlen: max length of the memory to map
66eab4df288aae lib/pci_iomap.c Michael S. Tsirkin 2011-11-24 18 *
66eab4df288aae lib/pci_iomap.c Michael S. Tsirkin 2011-11-24 19 * Using this function you will get a __iomem address to your device BAR.
66eab4df288aae lib/pci_iomap.c Michael S. Tsirkin 2011-11-24 20 * You can access it using ioread*() and iowrite*(). These functions hide
66eab4df288aae lib/pci_iomap.c Michael S. Tsirkin 2011-11-24 21 * the details if this is a MMIO or PIO address space and will just do what
66eab4df288aae lib/pci_iomap.c Michael S. Tsirkin 2011-11-24 22 * you expect from them in the correct way.
66eab4df288aae lib/pci_iomap.c Michael S. Tsirkin 2011-11-24 23 *
66eab4df288aae lib/pci_iomap.c Michael S. Tsirkin 2011-11-24 24 * @maxlen specifies the maximum length to map. If you want to get access to
eb29d8d2aad706 lib/pci_iomap.c Michael S. Tsirkin 2013-05-29 25 * the complete BAR from offset to the end, pass %0 here.
66eab4df288aae lib/pci_iomap.c Michael S. Tsirkin 2011-11-24 26 * */
eb29d8d2aad706 lib/pci_iomap.c Michael S. Tsirkin 2013-05-29 @27 void __iomem *pci_iomap_range(struct pci_dev *dev,
eb29d8d2aad706 lib/pci_iomap.c Michael S. Tsirkin 2013-05-29 28 int bar,
eb29d8d2aad706 lib/pci_iomap.c Michael S. Tsirkin 2013-05-29 29 unsigned long offset,
eb29d8d2aad706 lib/pci_iomap.c Michael S. Tsirkin 2013-05-29 30 unsigned long maxlen)
66eab4df288aae lib/pci_iomap.c Michael S. Tsirkin 2011-11-24 31 {
66eab4df288aae lib/pci_iomap.c Michael S. Tsirkin 2011-11-24 32 resource_size_t start = pci_resource_start(dev, bar);
66eab4df288aae lib/pci_iomap.c Michael S. Tsirkin 2011-11-24 33 resource_size_t len = pci_resource_len(dev, bar);
66eab4df288aae lib/pci_iomap.c Michael S. Tsirkin 2011-11-24 34 unsigned long flags = pci_resource_flags(dev, bar);
66eab4df288aae lib/pci_iomap.c Michael S. Tsirkin 2011-11-24 35
eb29d8d2aad706 lib/pci_iomap.c Michael S. Tsirkin 2013-05-29 36 if (len <= offset || !start)
66eab4df288aae lib/pci_iomap.c Michael S. Tsirkin 2011-11-24 37 return NULL;
eb29d8d2aad706 lib/pci_iomap.c Michael S. Tsirkin 2013-05-29 38 len -= offset;
eb29d8d2aad706 lib/pci_iomap.c Michael S. Tsirkin 2013-05-29 39 start += offset;
66eab4df288aae lib/pci_iomap.c Michael S. Tsirkin 2011-11-24 40 if (maxlen && len > maxlen)
66eab4df288aae lib/pci_iomap.c Michael S. Tsirkin 2011-11-24 41 len = maxlen;
66eab4df288aae lib/pci_iomap.c Michael S. Tsirkin 2011-11-24 42 if (flags & IORESOURCE_IO)
b923650b84068b lib/pci_iomap.c Michael S. Tsirkin 2012-01-30 @43 return __pci_ioport_map(dev, start, len);
92b19ff50e8f24 lib/pci_iomap.c Dan Williams 2015-08-10 44 if (flags & IORESOURCE_MEM)
66eab4df288aae lib/pci_iomap.c Michael S. Tsirkin 2011-11-24 45 return ioremap(start, len);
66eab4df288aae lib/pci_iomap.c Michael S. Tsirkin 2011-11-24 46 /* What? */
66eab4df288aae lib/pci_iomap.c Michael S. Tsirkin 2011-11-24 47 return NULL;
66eab4df288aae lib/pci_iomap.c Michael S. Tsirkin 2011-11-24 48 }
eb29d8d2aad706 lib/pci_iomap.c Michael S. Tsirkin 2013-05-29 49 EXPORT_SYMBOL(pci_iomap_range);
66eab4df288aae lib/pci_iomap.c Michael S. Tsirkin 2011-11-24 50
1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24 51 /**
1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24 52 * pci_iomap_wc_range - create a virtual WC mapping cookie for a PCI BAR
1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24 53 * @dev: PCI device that owns the BAR
1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24 54 * @bar: BAR number
1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24 55 * @offset: map memory at the given offset in BAR
1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24 56 * @maxlen: max length of the memory to map
1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24 57 *
1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24 58 * Using this function you will get a __iomem address to your device BAR.
1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24 59 * You can access it using ioread*() and iowrite*(). These functions hide
1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24 60 * the details if this is a MMIO or PIO address space and will just do what
1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24 61 * you expect from them in the correct way. When possible write combining
1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24 62 * is used.
1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24 63 *
1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24 64 * @maxlen specifies the maximum length to map. If you want to get access to
1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24 65 * the complete BAR from offset to the end, pass %0 here.
1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24 66 * */
1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24 @67 void __iomem *pci_iomap_wc_range(struct pci_dev *dev,
1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24 68 int bar,
1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24 69 unsigned long offset,
1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24 70 unsigned long maxlen)
1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24 71 {
1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24 72 resource_size_t start = pci_resource_start(dev, bar);
1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24 73 resource_size_t len = pci_resource_len(dev, bar);
1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24 74 unsigned long flags = pci_resource_flags(dev, bar);
1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24 75
1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24 76
1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24 77 if (flags & IORESOURCE_IO)
1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24 78 return NULL;
1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24 79
1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24 80 if (len <= offset || !start)
1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24 81 return NULL;
1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24 82
1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24 83 len -= offset;
1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24 84 start += offset;
1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24 85 if (maxlen && len > maxlen)
1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24 86 len = maxlen;
1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24 87
1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24 88 if (flags & IORESOURCE_MEM)
1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24 89 return ioremap_wc(start, len);
1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24 90
1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24 91 /* What? */
1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24 92 return NULL;
1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24 93 }
1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24 94 EXPORT_SYMBOL_GPL(pci_iomap_wc_range);
1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24 95
eb29d8d2aad706 lib/pci_iomap.c Michael S. Tsirkin 2013-05-29 96 /**
eb29d8d2aad706 lib/pci_iomap.c Michael S. Tsirkin 2013-05-29 97 * pci_iomap - create a virtual mapping cookie for a PCI BAR
eb29d8d2aad706 lib/pci_iomap.c Michael S. Tsirkin 2013-05-29 98 * @dev: PCI device that owns the BAR
eb29d8d2aad706 lib/pci_iomap.c Michael S. Tsirkin 2013-05-29 99 * @bar: BAR number
eb29d8d2aad706 lib/pci_iomap.c Michael S. Tsirkin 2013-05-29 100 * @maxlen: length of the memory to map
eb29d8d2aad706 lib/pci_iomap.c Michael S. Tsirkin 2013-05-29 101 *
eb29d8d2aad706 lib/pci_iomap.c Michael S. Tsirkin 2013-05-29 102 * Using this function you will get a __iomem address to your device BAR.
eb29d8d2aad706 lib/pci_iomap.c Michael S. Tsirkin 2013-05-29 103 * You can access it using ioread*() and iowrite*(). These functions hide
eb29d8d2aad706 lib/pci_iomap.c Michael S. Tsirkin 2013-05-29 104 * the details if this is a MMIO or PIO address space and will just do what
eb29d8d2aad706 lib/pci_iomap.c Michael S. Tsirkin 2013-05-29 105 * you expect from them in the correct way.
eb29d8d2aad706 lib/pci_iomap.c Michael S. Tsirkin 2013-05-29 106 *
eb29d8d2aad706 lib/pci_iomap.c Michael S. Tsirkin 2013-05-29 107 * @maxlen specifies the maximum length to map. If you want to get access to
eb29d8d2aad706 lib/pci_iomap.c Michael S. Tsirkin 2013-05-29 108 * the complete BAR without checking for its length first, pass %0 here.
eb29d8d2aad706 lib/pci_iomap.c Michael S. Tsirkin 2013-05-29 109 * */
eb29d8d2aad706 lib/pci_iomap.c Michael S. Tsirkin 2013-05-29 @110 void __iomem *pci_iomap(struct pci_dev *dev, int bar, unsigned long maxlen)
eb29d8d2aad706 lib/pci_iomap.c Michael S. Tsirkin 2013-05-29 111 {
eb29d8d2aad706 lib/pci_iomap.c Michael S. Tsirkin 2013-05-29 112 return pci_iomap_range(dev, bar, 0, maxlen);
eb29d8d2aad706 lib/pci_iomap.c Michael S. Tsirkin 2013-05-29 113 }
66eab4df288aae lib/pci_iomap.c Michael S. Tsirkin 2011-11-24 114 EXPORT_SYMBOL(pci_iomap);
1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24 115
1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24 116 /**
1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24 117 * pci_iomap_wc - create a virtual WC mapping cookie for a PCI BAR
1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24 118 * @dev: PCI device that owns the BAR
1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24 119 * @bar: BAR number
1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24 120 * @maxlen: length of the memory to map
1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24 121 *
1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24 122 * Using this function you will get a __iomem address to your device BAR.
1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24 123 * You can access it using ioread*() and iowrite*(). These functions hide
1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24 124 * the details if this is a MMIO or PIO address space and will just do what
1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24 125 * you expect from them in the correct way. When possible write combining
1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24 126 * is used.
1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24 127 *
1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24 128 * @maxlen specifies the maximum length to map. If you want to get access to
1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24 129 * the complete BAR without checking for its length first, pass %0 here.
1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24 130 * */
1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24 @131 void __iomem *pci_iomap_wc(struct pci_dev *dev, int bar, unsigned long maxlen)
1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24 132 {
1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24 133 return pci_iomap_wc_range(dev, bar, 0, maxlen);
1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24 134 }
1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24 135 EXPORT_SYMBOL_GPL(pci_iomap_wc);
316e8d79a0959c lib/pci_iomap.c Linus Torvalds 2021-09-19 136
316e8d79a0959c lib/pci_iomap.c Linus Torvalds 2021-09-19 137 /*
316e8d79a0959c lib/pci_iomap.c Linus Torvalds 2021-09-19 138 * pci_iounmap() somewhat illogically comes from lib/iomap.c for the
316e8d79a0959c lib/pci_iomap.c Linus Torvalds 2021-09-19 139 * CONFIG_GENERIC_IOMAP case, because that's the code that knows about
316e8d79a0959c lib/pci_iomap.c Linus Torvalds 2021-09-19 140 * the different IOMAP ranges.
316e8d79a0959c lib/pci_iomap.c Linus Torvalds 2021-09-19 141 *
316e8d79a0959c lib/pci_iomap.c Linus Torvalds 2021-09-19 142 * But if the architecture does not use the generic iomap code, and if
316e8d79a0959c lib/pci_iomap.c Linus Torvalds 2021-09-19 143 * it has _not_ defined it's own private pci_iounmap function, we define
316e8d79a0959c lib/pci_iomap.c Linus Torvalds 2021-09-19 144 * it here.
316e8d79a0959c lib/pci_iomap.c Linus Torvalds 2021-09-19 145 *
316e8d79a0959c lib/pci_iomap.c Linus Torvalds 2021-09-19 146 * NOTE! This default implementation assumes that if the architecture
316e8d79a0959c lib/pci_iomap.c Linus Torvalds 2021-09-19 147 * support ioport mapping (HAS_IOPORT_MAP), the ioport mapping will
316e8d79a0959c lib/pci_iomap.c Linus Torvalds 2021-09-19 148 * be fixed to the range [ PCI_IOBASE, PCI_IOBASE+IO_SPACE_LIMIT [,
316e8d79a0959c lib/pci_iomap.c Linus Torvalds 2021-09-19 149 * and does not need unmapping with 'ioport_unmap()'.
316e8d79a0959c lib/pci_iomap.c Linus Torvalds 2021-09-19 150 *
316e8d79a0959c lib/pci_iomap.c Linus Torvalds 2021-09-19 151 * If you have different rules for your architecture, you need to
316e8d79a0959c lib/pci_iomap.c Linus Torvalds 2021-09-19 152 * implement your own pci_iounmap() that knows the rules for where
316e8d79a0959c lib/pci_iomap.c Linus Torvalds 2021-09-19 153 * and how IO vs MEM get mapped.
316e8d79a0959c lib/pci_iomap.c Linus Torvalds 2021-09-19 154 *
316e8d79a0959c lib/pci_iomap.c Linus Torvalds 2021-09-19 155 * This code is odd, and the ARCH_HAS/ARCH_WANTS #define logic comes
316e8d79a0959c lib/pci_iomap.c Linus Torvalds 2021-09-19 156 * from legacy <asm-generic/io.h> header file behavior. In particular,
316e8d79a0959c lib/pci_iomap.c Linus Torvalds 2021-09-19 157 * it would seem to make sense to do the iounmap(p) for the non-IO-space
316e8d79a0959c lib/pci_iomap.c Linus Torvalds 2021-09-19 158 * case here regardless, but that's not what the old header file code
316e8d79a0959c lib/pci_iomap.c Linus Torvalds 2021-09-19 159 * did. Probably incorrectly, but this is meant to be bug-for-bug
316e8d79a0959c lib/pci_iomap.c Linus Torvalds 2021-09-19 160 * compatible.
316e8d79a0959c lib/pci_iomap.c Linus Torvalds 2021-09-19 161 */
316e8d79a0959c lib/pci_iomap.c Linus Torvalds 2021-09-19 162 #if defined(ARCH_WANTS_GENERIC_PCI_IOUNMAP)
316e8d79a0959c lib/pci_iomap.c Linus Torvalds 2021-09-19 163
316e8d79a0959c lib/pci_iomap.c Linus Torvalds 2021-09-19 @164 void pci_iounmap(struct pci_dev *dev, void __iomem *p)
316e8d79a0959c lib/pci_iomap.c Linus Torvalds 2021-09-19 165 {
316e8d79a0959c lib/pci_iomap.c Linus Torvalds 2021-09-19 166 #ifdef ARCH_HAS_GENERIC_IOPORT_MAP
316e8d79a0959c lib/pci_iomap.c Linus Torvalds 2021-09-19 167 uintptr_t start = (uintptr_t) PCI_IOBASE;
316e8d79a0959c lib/pci_iomap.c Linus Torvalds 2021-09-19 168 uintptr_t addr = (uintptr_t) p;
316e8d79a0959c lib/pci_iomap.c Linus Torvalds 2021-09-19 169
316e8d79a0959c lib/pci_iomap.c Linus Torvalds 2021-09-19 170 if (addr >= start && addr < start + IO_SPACE_LIMIT)
316e8d79a0959c lib/pci_iomap.c Linus Torvalds 2021-09-19 171 return;
316e8d79a0959c lib/pci_iomap.c Linus Torvalds 2021-09-19 172 iounmap(p);
316e8d79a0959c lib/pci_iomap.c Linus Torvalds 2021-09-19 173 #endif
316e8d79a0959c lib/pci_iomap.c Linus Torvalds 2021-09-19 174 }
316e8d79a0959c lib/pci_iomap.c Linus Torvalds 2021-09-19 175 EXPORT_SYMBOL(pci_iounmap);
316e8d79a0959c lib/pci_iomap.c Linus Torvalds 2021-09-19 176

--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

2023-11-21 06:50:35

[permalink] [raw]

Subject: Re: [PATCH 1/4] lib: move pci_iomap.c to drivers/pci/

2023-11-21 07:22:54

[permalink] [raw]

Subject: Re: [PATCH 1/4] lib: move pci_iomap.c to drivers/pci/

2023-11-21 07:23:15

[permalink] [raw]

Subject: Re: [PATCH 1/4] lib: move pci_iomap.c to drivers/pci/

On Tue, Nov 21, 2023, at 07:48, kernel test robot wrote:
>
> If you fix the issue in a separate patch/commit (i.e. not just a new
> version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <[email protected]>
> | Closes:
> https://lore.kernel.org/oe-kbuild-all/[email protected]/
>
> All errors (new ones prefixed by >>):
>
> drivers/pci/iomap.c:27:15: error: redefinition of 'pci_iomap_range'
> void __iomem *pci_iomap_range(struct pci_dev *dev,
> ^
> include/asm-generic/pci_iomap.h:44:29: note: previous definition is here
> static inline void __iomem *pci_iomap_range(struct pci_dev *dev, int bar,
> ^
>>> drivers/pci/iomap.c:43:10: error: call to undeclared function '__pci_ioport_map'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
> return __pci_ioport_map(dev, start, len);

From what I can tell looking at the header, I think we can
just remove the "#elif defined(CONFIG_GENERIC_PCI_IOMAP)"
bit entirely, as it no longer serves the purpose it originally
had.

It also looks like s390 is the only architecture that actually
uses a custom implementation of pci_iomap*(), and this already has

#define pci_iomap pci_iomap
#define pci_iomap_range pci_iomap_range
#define pci_iounmap pci_iounmap
#define pci_iomap_wc pci_iomap_wc
#define pci_iomap_wc_range pci_iomap_wc_range

so the entire CONFIG_GENERIC_PCI_IOMAP symbol can probably
be replaced with individual checks here, using CONFIG_PCI
as the conditional in the Makefile.

Arnd

2023-11-21 07:29:52

[permalink] [raw]

Subject: Re: [PATCH 2/4] lib: move pci-specific devres code to drivers/pci/

On Mon, Nov 20, 2023, at 22:59, 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 not generic infrastructure.
>
> Move all pcim_*() functions in lib/devres.c to drivers/pci/devres.c
>
> 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 +------------------------------------------

Since you are moving the pci_iomap() code into drivers/pci/ already,
I'd suggest merging this one into the same file and keep the two
halves of this interface together.

Arnd

2023-11-21 07:47:38

[permalink] [raw]

Subject: Re: [PATCH 1/4] lib: move pci_iomap.c to drivers/pci/

2023-11-21 08:01:58

[permalink] [raw]

Subject: Re: [PATCH 1/4] lib: move pci_iomap.c to drivers/pci/

2023-11-21 08:03:34

[permalink] [raw]

Subject: Re: [PATCH 2/4] lib: move pci-specific devres code to drivers/pci/

On Tue, 2023-11-21 at 08:29 +0100, Arnd Bergmann wrote:
> On Mon, Nov 20, 2023, at 22:59, 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 not generic infrastructure.
> >
> > Move all pcim_*() functions in lib/devres.c to drivers/pci/devres.c
> >
> > 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 +--------------------------------------
> > ----
>
> Since you are moving the pci_iomap() code into drivers/pci/ already,
> I'd suggest merging this one into the same file and keep the two
> halves of this interface together.

I'd argue that they are as much together as they were before:

Previously:
* PCI-IOMAP-code in folder lib/ in its own file (pci_iomap.c)
* PCI-Devres-code in folder lib/ in a distinct file (devres.c)

Now:
* PCI-IOMAP-code in folder drivers/pci/ in its own file (iomap.c)
* PCI-Devres-code in folder drivers/pci/ in its own file (devres.c)

Or am I misunderstanding something?

P.

>
>      Arnd
>

2023-11-21 08:48:28

[permalink] [raw]

Subject: Re: [PATCH 1/4] lib: move pci_iomap.c to drivers/pci/

Hi Philipp,

kernel test robot noticed the following build warnings:

[auto build test WARNING on pci/next]
[also build test WARNING on pci/for-linus linus/master v6.7-rc2 next-20231121]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url: https://github.com/intel-lab-lkp/linux/commits/Philipp-Stanner/lib-move-pci_iomap-c-to-drivers-pci/20231121-060258
base: https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git next
patch link: https://lore.kernel.org/r/20231120215945.52027-3-pstanner%40redhat.com
patch subject: [PATCH 1/4] lib: move pci_iomap.c to drivers/pci/
config: x86_64-buildonly-randconfig-004-20231121 (https://download.01.org/0day-ci/archive/20231121/[email protected]/config)
compiler: gcc-7 (Ubuntu 7.5.0-6ubuntu2) 7.5.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231121/[email protected]/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/

All warnings (new ones prefixed by >>):

drivers/pci/iomap.c:27:15: error: redefinition of 'pci_iomap_range'
void __iomem *pci_iomap_range(struct pci_dev *dev,
^~~~~~~~~~~~~~~
In file included from include/asm-generic/iomap.h:113:0,
from include/asm-generic/io.h:16,
from arch/x86/include/asm/io.h:327,
from include/linux/io.h:13,
from include/linux/pci.h:39,
from drivers/pci/iomap.c:7:
include/asm-generic/pci_iomap.h:44:29: note: previous definition of 'pci_iomap_range' was here
static inline void __iomem *pci_iomap_range(struct pci_dev *dev, int bar,
^~~~~~~~~~~~~~~
drivers/pci/iomap.c: In function 'pci_iomap_range':
drivers/pci/iomap.c:43:10: error: implicit declaration of function '__pci_ioport_map'; did you mean 'devm_ioport_map'? [-Werror=implicit-function-declaration]
return __pci_ioport_map(dev, start, len);
^~~~~~~~~~~~~~~~
devm_ioport_map
>> drivers/pci/iomap.c:43:10: warning: return makes pointer from integer without a cast [-Wint-conversion]
return __pci_ioport_map(dev, start, len);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/pci/iomap.c: At top level:
drivers/pci/iomap.c:67:15: error: redefinition of 'pci_iomap_wc_range'
void __iomem *pci_iomap_wc_range(struct pci_dev *dev,
^~~~~~~~~~~~~~~~~~
In file included from include/asm-generic/iomap.h:113:0,
from include/asm-generic/io.h:16,
from arch/x86/include/asm/io.h:327,
from include/linux/io.h:13,
from include/linux/pci.h:39,
from drivers/pci/iomap.c:7:
include/asm-generic/pci_iomap.h:50:29: note: previous definition of 'pci_iomap_wc_range' was here
static inline void __iomem *pci_iomap_wc_range(struct pci_dev *dev, int bar,
^~~~~~~~~~~~~~~~~~
drivers/pci/iomap.c:110:15: error: redefinition of 'pci_iomap'
void __iomem *pci_iomap(struct pci_dev *dev, int bar, unsigned long maxlen)
^~~~~~~~~
In file included from include/asm-generic/iomap.h:113:0,
from include/asm-generic/io.h:16,
from arch/x86/include/asm/io.h:327,
from include/linux/io.h:13,
from include/linux/pci.h:39,
from drivers/pci/iomap.c:7:
include/asm-generic/pci_iomap.h:35:29: note: previous definition of 'pci_iomap' was here
static inline void __iomem *pci_iomap(struct pci_dev *dev, int bar, unsigned long max)
^~~~~~~~~
drivers/pci/iomap.c:131:15: error: redefinition of 'pci_iomap_wc'
void __iomem *pci_iomap_wc(struct pci_dev *dev, int bar, unsigned long maxlen)
^~~~~~~~~~~~
In file included from include/asm-generic/iomap.h:113:0,
from include/asm-generic/io.h:16,
from arch/x86/include/asm/io.h:327,
from include/linux/io.h:13,
from include/linux/pci.h:39,
from drivers/pci/iomap.c:7:
include/asm-generic/pci_iomap.h:40:29: note: previous definition of 'pci_iomap_wc' was here
static inline void __iomem *pci_iomap_wc(struct pci_dev *dev, int bar, unsigned long max)
^~~~~~~~~~~~
cc1: some warnings being treated as errors

Kconfig warnings: (for reference only)
WARNING: unmet direct dependencies detected for GENERIC_PCI_IOMAP
Depends on [n]: PCI [=n]
Selected by [y]:
- GENERIC_IOMAP [=y]

vim +43 drivers/pci/iomap.c

66eab4df288aae lib/pci_iomap.c Michael S. Tsirkin 2011-11-24 11
66eab4df288aae lib/pci_iomap.c Michael S. Tsirkin 2011-11-24 12 /**
eb29d8d2aad706 lib/pci_iomap.c Michael S. Tsirkin 2013-05-29 13 * pci_iomap_range - create a virtual mapping cookie for a PCI BAR
66eab4df288aae lib/pci_iomap.c Michael S. Tsirkin 2011-11-24 14 * @dev: PCI device that owns the BAR
66eab4df288aae lib/pci_iomap.c Michael S. Tsirkin 2011-11-24 15 * @bar: BAR number
eb29d8d2aad706 lib/pci_iomap.c Michael S. Tsirkin 2013-05-29 16 * @offset: map memory at the given offset in BAR
eb29d8d2aad706 lib/pci_iomap.c Michael S. Tsirkin 2013-05-29 17 * @maxlen: max length of the memory to map
66eab4df288aae lib/pci_iomap.c Michael S. Tsirkin 2011-11-24 18 *
66eab4df288aae lib/pci_iomap.c Michael S. Tsirkin 2011-11-24 19 * Using this function you will get a __iomem address to your device BAR.
66eab4df288aae lib/pci_iomap.c Michael S. Tsirkin 2011-11-24 20 * You can access it using ioread*() and iowrite*(). These functions hide
66eab4df288aae lib/pci_iomap.c Michael S. Tsirkin 2011-11-24 21 * the details if this is a MMIO or PIO address space and will just do what
66eab4df288aae lib/pci_iomap.c Michael S. Tsirkin 2011-11-24 22 * you expect from them in the correct way.
66eab4df288aae lib/pci_iomap.c Michael S. Tsirkin 2011-11-24 23 *
66eab4df288aae lib/pci_iomap.c Michael S. Tsirkin 2011-11-24 24 * @maxlen specifies the maximum length to map. If you want to get access to
eb29d8d2aad706 lib/pci_iomap.c Michael S. Tsirkin 2013-05-29 25 * the complete BAR from offset to the end, pass %0 here.
66eab4df288aae lib/pci_iomap.c Michael S. Tsirkin 2011-11-24 26 * */
eb29d8d2aad706 lib/pci_iomap.c Michael S. Tsirkin 2013-05-29 27 void __iomem *pci_iomap_range(struct pci_dev *dev,
eb29d8d2aad706 lib/pci_iomap.c Michael S. Tsirkin 2013-05-29 28 int bar,
eb29d8d2aad706 lib/pci_iomap.c Michael S. Tsirkin 2013-05-29 29 unsigned long offset,
eb29d8d2aad706 lib/pci_iomap.c Michael S. Tsirkin 2013-05-29 30 unsigned long maxlen)
66eab4df288aae lib/pci_iomap.c Michael S. Tsirkin 2011-11-24 31 {
66eab4df288aae lib/pci_iomap.c Michael S. Tsirkin 2011-11-24 32 resource_size_t start = pci_resource_start(dev, bar);
66eab4df288aae lib/pci_iomap.c Michael S. Tsirkin 2011-11-24 33 resource_size_t len = pci_resource_len(dev, bar);
66eab4df288aae lib/pci_iomap.c Michael S. Tsirkin 2011-11-24 34 unsigned long flags = pci_resource_flags(dev, bar);
66eab4df288aae lib/pci_iomap.c Michael S. Tsirkin 2011-11-24 35
eb29d8d2aad706 lib/pci_iomap.c Michael S. Tsirkin 2013-05-29 36 if (len <= offset || !start)
66eab4df288aae lib/pci_iomap.c Michael S. Tsirkin 2011-11-24 37 return NULL;
eb29d8d2aad706 lib/pci_iomap.c Michael S. Tsirkin 2013-05-29 38 len -= offset;
eb29d8d2aad706 lib/pci_iomap.c Michael S. Tsirkin 2013-05-29 39 start += offset;
66eab4df288aae lib/pci_iomap.c Michael S. Tsirkin 2011-11-24 40 if (maxlen && len > maxlen)
66eab4df288aae lib/pci_iomap.c Michael S. Tsirkin 2011-11-24 41 len = maxlen;
66eab4df288aae lib/pci_iomap.c Michael S. Tsirkin 2011-11-24 42 if (flags & IORESOURCE_IO)
b923650b84068b lib/pci_iomap.c Michael S. Tsirkin 2012-01-30 @43 return __pci_ioport_map(dev, start, len);
92b19ff50e8f24 lib/pci_iomap.c Dan Williams 2015-08-10 44 if (flags & IORESOURCE_MEM)
66eab4df288aae lib/pci_iomap.c Michael S. Tsirkin 2011-11-24 45 return ioremap(start, len);
66eab4df288aae lib/pci_iomap.c Michael S. Tsirkin 2011-11-24 46 /* What? */
66eab4df288aae lib/pci_iomap.c Michael S. Tsirkin 2011-11-24 47 return NULL;
66eab4df288aae lib/pci_iomap.c Michael S. Tsirkin 2011-11-24 48 }
eb29d8d2aad706 lib/pci_iomap.c Michael S. Tsirkin 2013-05-29 49 EXPORT_SYMBOL(pci_iomap_range);
66eab4df288aae lib/pci_iomap.c Michael S. Tsirkin 2011-11-24 50

--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

2023-11-21 10:03:54

[permalink] [raw]

Subject: Re: [PATCH 4/4] lib/iomap.c: improve comment about pci anomaly

On Mon, Nov 20, 2023, at 22:59, Philipp Stanner wrote:
> lib/iomap.c contains one of the definitions of pci_iounmap(). The
> current comment above this out-of-place function does not clarify WHY
> the function is defined here.
>
> Linus's detailed comment above pci_iounmap() in drivers/pci/iomap.c
> clarifies that in a far better way.
>
> Extend the existing comment with an excerpt from Linus's and hint at the
> other implementation in drivers/pci/iomap.c
>
> Signed-off-by: Philipp Stanner <[email protected]>

I think instead of explaining why the code is so complicated
here, I'd prefer to make it more logical and not have to
explain it.

We should be able to define a generic version like

void pci_iounmap(struct pci_dev *dev, void __iomem * addr)
{
#ifdef CONFIG_HAS_IOPORT
if (iomem_is_ioport(addr)) {
ioport_unmap(addr);
return;
}
#endif
iounmap(addr)
}

and then define iomem_is_ioport() in lib/iomap.c for x86,
while defining it in asm-generic/io.h for the rest,
with an override in asm/io.h for those architectures
that need a custom inb().

Note that with ia64 gone, GENERIC_IOMAP is not at all
generic any more and could just move it to x86 or name
it something else. This is what currently uses it:

arch/hexagon/Kconfig: select GENERIC_IOMAP
arch/um/Kconfig: select GENERIC_IOMAP

These have no port I/O at all, so it doesn't do anything.

arch/m68k/Kconfig: select GENERIC_IOMAP

on m68knommu, the default implementation from asm-generic/io.h
as the same effect as GENERIC_IOMAP but is more efficient.
On classic m68k, GENERIC_IOMAP does not do what it is
meant to because I/O ports on ISA devices have port
numbers above PIO_OFFSET. Also they don't have PCI.

arch/mips/Kconfig: select GENERIC_IOMAP

This looks completely bogus because it sets PIO_RESERVED
to 0 and always uses the mmio part of lib/iomap.c.

arch/powerpc/platforms/Kconfig: select GENERIC_IOMAP

This is only used for two platforms: cell and powernv,
though on Cell it no longer does anything after the
commit f4981a00636 ("powerpc: Remove the celleb support");
I think the entire io_workarounds code now be folded
back into spider_pci.c if we wanted to.

The PowerNV LPC support does seem to still rely on it.
This tries to do the exact same thing as lib/logic_pio.c
for Huawei arm64 servers. I suspect that neither of them
does it entirely correctly since the powerpc side appears
to just override any non-LPC PIO support while the arm64
side is missing the ioread/iowrite support.

Arnd

2023-11-21 10:11:33

[permalink] [raw]

Subject: Re: [PATCH 2/4] lib: move pci-specific devres code to drivers/pci/

On Tue, Nov 21, 2023, at 09:00, Philipp Stanner wrote:
> On Tue, 2023-11-21 at 08:29 +0100, Arnd Bergmann wrote:
>> On Mon, Nov 20, 2023, at 22:59, Philipp Stanner wrote:
>>
>> Since you are moving the pci_iomap() code into drivers/pci/ already,
>> I'd suggest merging this one into the same file and keep the two
>> halves of this interface together.
>
>
> I'd argue that they are as much together as they were before:
>
> Previously:
> * PCI-IOMAP-code in folder lib/ in its own file (pci_iomap.c)
> * PCI-Devres-code in folder lib/ in a distinct file (devres.c)
>
> Now:
> * PCI-IOMAP-code in folder drivers/pci/ in its own file (iomap.c)
> * PCI-Devres-code in folder drivers/pci/ in its own file (devres.c)
>
> Or am I misunderstanding something?

They are indeed closer together now, just not in the same file.

Looking across subsystems at the output of

git grep -l EXPORT.*devm

I see that 10 out of 182 files have split the devres functions
into a separate file, while the others just keep the devm_*
function in the same place as the normal one. Since you never
have one of these files without the other, and they do
almost the exact same thing, a single file is the simpler
option.

Note that there are also three pcim_*() functions in
drivers/pci/pci.c. I think that is the correct place for
them, but if you wanted to split out pci devres functions
into a separate file, they would now also have to go
into drivers/pci/devres.c.

Arnd

2023-11-21 10:19:57

[permalink] [raw]

Subject: Re: [PATCH 3/4] pci: move devres code from pci.c to devres.c

On Mon, Nov 20, 2023, at 22:59, Philipp Stanner wrote:
> The file pci.c is very large and contains a number of devres-functions.
> These functions should now reside in devres.c
>
> There are a few callers left in pci.c that do devres operations. These
> should be ported in the future. Corresponding TODOs are added by this
> commit.
>
> The reason they are not moved right now in this commit is that pci's
> devres currently implements a sort of "hybrid-mode":
> pci_request_region(), for instance, does not have a corresponding pcim_
> equivalent, yet. Instead, the function can be made managed by previously
> calling pcim_enable_device() (instead of pci_enable_device()). This
> makes it unreasonable to move pci_request_region() to devres.c
> Moving the functions would require changes to pci's API and is,
> therefore, left for future work.
>
> Move as much devres-specific code from pci.c to devres.c as possible.
>
> Suggested-by: Danilo Krummrich <[email protected]>
> Signed-off-by: Philipp Stanner <[email protected]>
> ---
> drivers/pci/devres.c | 243 +++++++++++++++++++++++++++++++++++++++++
> drivers/pci/pci.c | 249 -------------------------------------------
> drivers/pci/pci.h | 24 +++++
> 3 files changed, 267 insertions(+), 249 deletions(-)

I had just commented in the other mail that you'd have to move
these functions to devres.c for the file to make sense, but that
I think the existing state is better.

Just to clarify again here: this patch does not seem to improve
anything to me, I'd much prefer leaving it the way it is, and
moving the pcim_iomap family to corresponding drivers/pci/iomap.c.

Arnd

2023-11-21 10:36:28

[permalink] [raw]

Subject: Re: [PATCH 3/4] pci: move devres code from pci.c to devres.c

On Tue, 2023-11-21 at 11:17 +0100, Arnd Bergmann wrote:
> On Mon, Nov 20, 2023, at 22:59, Philipp Stanner wrote:
> > The file pci.c is very large and contains a number of devres-
> > functions.
> > These functions should now reside in devres.c
> >
> > There are a few callers left in pci.c that do devres operations.
> > These
> > should be ported in the future. Corresponding TODOs are added by
> > this
> > commit.
> >
> > The reason they are not moved right now in this commit is that
> > pci's
> > devres currently implements a sort of "hybrid-mode":
> > pci_request_region(), for instance, does not have a corresponding
> > pcim_
> > equivalent, yet. Instead, the function can be made managed by
> > previously
> > calling pcim_enable_device() (instead of pci_enable_device()). This
> > makes it unreasonable to move pci_request_region() to devres.c
> > Moving the functions would require changes to pci's API and is,
> > therefore, left for future work.
> >
> > Move as much devres-specific code from pci.c to devres.c as
> > possible.
> >
> > Suggested-by: Danilo Krummrich <[email protected]>
> > Signed-off-by: Philipp Stanner <[email protected]>
> > ---
> > drivers/pci/devres.c | 243
> > +++++++++++++++++++++++++++++++++++++++++
> > drivers/pci/pci.c    | 249 ---------------------------------------
> > ----
> > drivers/pci/pci.h    | 24 +++++
> > 3 files changed, 267 insertions(+), 249 deletions(-)
>
> I had just commented in the other mail that you'd have to move
> these functions to devres.c for the file to make sense, but that
> I think the existing state is better.
>
> Just to clarify again here: this patch does not seem to improve
> anything to me,

Have you read the cover letter? It elaborates on that.

The idea behind centralizing devres-pci-code in a separate file is that
the current implementation is strangely torn.
My mid-term goal would be to fix that, but that's beyond the scope of
this series.

PCI has some separate devres functions, prefixed with pcim_ – and then
there are some other functions that use a crazy hybrid mode:

drivers/pci/pci.c:

void pci_release_region(struct pci_dev *pdev, int bar)
{
/* ..... SNIP ......... */

dr = find_pci_dr(pdev);
if (dr)
dr->region_mask &= ~(1 << bar);
}

Some functions without pcim_ prefix switch to managed mode if
pcim_enable_device() instead of pci_enable_device() was called.

So some functions are sometimes managed and others never are, which is
totally inconsistent.

This is bug-provoking because programmers won't know without looking
very closely which functions become managed and which don't.

That should be fixed. And a first step towards that goal is to cleanly
split them. Everything managed should reside, on the long term, in
drivers/pci/devres.c

> I'd much prefer leaving it the way it is, and
> moving the pcim_iomap family to corresponding drivers/pci/iomap.c.

We could branch that change out of the patch series and handle the
topics independently

P.

>
>      Arnd
>

2023-11-21 10:46:23

[permalink] [raw]

Subject: Re: [PATCH 1/4] lib: move pci_iomap.c to drivers/pci/

On Tue, 2023-11-21 at 12:20 +0800, kernel test robot wrote:
> Hi Philipp,
>
> kernel test robot noticed the following build errors:
>
> [auto build test ERROR on pci/next]
> [also build test ERROR on pci/for-linus linus/master v6.7-rc2 next-
> 20231120]
> [If your patch is applied to the wrong git tree, kindly drop us a
> note.
> And when submitting patch, we suggest to use '--base' as documented
> in
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
>
> url:
> https://github.com/intel-lab-lkp/linux/commits/Philipp-Stanner/lib-move-pci_iomap-c-to-drivers-pci/20231121-060258
> base:
> https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git next
> patch link:
> https://lore.kernel.org/r/20231120215945.52027-3-pstanner%40redhat.com
> patch subject: [PATCH 1/4] lib: move pci_iomap.c to drivers/pci/
> config: openrisc-allnoconfig
> (https://download.01.org/0day-ci/archive/20231121/202311211216.KqPYvO
> [email protected]/config)
> compiler: or1k-linux-gcc (GCC) 13.2.0
> reproduce (this is a W=1 build):
> (https://download.01.org/0day-ci/archive/20231121/202311211216.KqPYvO
> [email protected]/reproduce)
>
> If you fix the issue in a separate patch/commit (i.e. not just a new
> version of
> the same patch/commit), kindly add following tags
> > Reported-by: kernel test robot <[email protected]>
> > Closes:
> > https://lore.kernel.org/oe-kbuild-all/[email protected]/
>
> All error/warnings (new ones prefixed by >>):
>
> > > drivers/pci/iomap.c:27:15: error: redefinition of
> > > 'pci_iomap_range'
>       27 | void __iomem *pci_iomap_range(struct pci_dev *dev,
>          |               ^~~~~~~~~~~~~~~
>    In file included from include/asm-generic/io.h:20,
>                     from arch/openrisc/include/asm/io.h:37,
>                     from include/linux/io.h:13,
>                     from include/linux/irq.h:20,
>                     from include/asm-generic/hardirq.h:17,
>                     from
> ./arch/openrisc/include/generated/asm/hardirq.h:1,
>                     from include/linux/hardirq.h:11,
>                     from include/linux/interrupt.h:11,
>                     from include/linux/pci.h:38,
>                     from drivers/pci/iomap.c:7:
>    include/asm-generic/pci_iomap.h:44:29: note: previous definition
> of 'pci_iomap_range' with type 'void *(struct pci_dev *, int, long
> unsigned int, long unsigned int)'
>       44 | static inline void __iomem *pci_iomap_range(struct pci_dev
> *dev, int bar,
>          |                             ^~~~~~~~~~~~~~~
>    drivers/pci/iomap.c: In function 'pci_iomap_range':
> > > drivers/pci/iomap.c:43:24: error: implicit declaration of
> > > function '__pci_ioport_map'; did you mean 'devm_ioport_map'? [-
> > > Werror=implicit-function-declaration]
>       43 |                 return __pci_ioport_map(dev, start, len);
>          |                        ^~~~~~~~~~~~~~~~
>          |                        devm_ioport_map
> > > drivers/pci/iomap.c:43:24: warning: returning 'int' from a
> > > function with return type 'void *' makes pointer from integer
> > > without a cast [-Wint-conversion]
>       43 |                 return __pci_ioport_map(dev, start, len);
>          |                        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>    drivers/pci/iomap.c: At top level:
> > > drivers/pci/iomap.c:67:15: error: redefinition of
> > > 'pci_iomap_wc_range'
>       67 | void __iomem *pci_iomap_wc_range(struct pci_dev *dev,
>          |               ^~~~~~~~~~~~~~~~~~
>    include/asm-generic/pci_iomap.h:50:29: note: previous definition
> of 'pci_iomap_wc_range' with type 'void *(struct pci_dev *, int,
> long unsigned int, long unsigned int)'
>       50 | static inline void __iomem *pci_iomap_wc_range(struct
> pci_dev *dev, int bar,
>          |                             ^~~~~~~~~~~~~~~~~~
> > > drivers/pci/iomap.c:110:15: error: redefinition of 'pci_iomap'
>      110 | void __iomem *pci_iomap(struct pci_dev *dev, int bar,
> unsigned long maxlen)
>          |               ^~~~~~~~~
>    include/asm-generic/pci_iomap.h:35:29: note: previous definition
> of 'pci_iomap' with type 'void *(struct pci_dev *, int, long
> unsigned int)'
>       35 | static inline void __iomem *pci_iomap(struct pci_dev *dev,
> int bar, unsigned long max)
>          |                             ^~~~~~~~~
> > > drivers/pci/iomap.c:131:15: error: redefinition of 'pci_iomap_wc'
>      131 | void __iomem *pci_iomap_wc(struct pci_dev *dev, int bar,
> unsigned long maxlen)
>          |               ^~~~~~~~~~~~
>    include/asm-generic/pci_iomap.h:40:29: note: previous definition
> of 'pci_iomap_wc' with type 'void *(struct pci_dev *, int, long
> unsigned int)'
>       40 | static inline void __iomem *pci_iomap_wc(struct pci_dev
> *dev, int bar, unsigned long max)
>          |                             ^~~~~~~~~~~~
> > > drivers/pci/iomap.c:164:6: error: redefinition of 'pci_iounmap'
>      164 | void pci_iounmap(struct pci_dev *dev, void __iomem *p)
>          |      ^~~~~~~~~~~
>    include/asm-generic/pci_iomap.h:56:20: note: previous definition
> of 'pci_iounmap' with type 'void(struct pci_dev *, void *)'
>       56 | static inline void pci_iounmap(struct pci_dev *dev, void
> __iomem *addr)
>          |                    ^~~~~~~~~~~
>    cc1: some warnings being treated as errors
>
> Kconfig warnings: (for reference only)
>    WARNING: unmet direct dependencies detected for GENERIC_PCI_IOMAP
>    Depends on [n]: PCI [=n]
>    Selected by [y]:
>    - OPENRISC [=y]

OK, so the issue here seems to be that you can not have
GENERIC_PCI_IOMAP depend on PCI.

Previously, #ifdef CONFIG_PCI made the (in this case) redundant
function definitions disappear, which is not the case anymore for
configs that want GENERIC_PCI_IOMAP but not PCI.

My bad.
I'll address that in the next version.

P.

>
>
> vim +/pci_iomap_range +27 drivers/pci/iomap.c
>
> 66eab4df288aae lib/pci_iomap.c Michael S. Tsirkin 2011-11-24   11
> 66eab4df288aae lib/pci_iomap.c Michael S. Tsirkin 2011-11-24   12
> /**
> eb29d8d2aad706 lib/pci_iomap.c Michael S. Tsirkin 2013-05-29   13   *
> pci_iomap_range - create a virtual mapping cookie for a PCI BAR
> 66eab4df288aae lib/pci_iomap.c Michael S. Tsirkin 2011-11-24   14   *
> @dev: PCI device that owns the BAR
> 66eab4df288aae lib/pci_iomap.c Michael S. Tsirkin 2011-11-24   15   *
> @bar: BAR number
> eb29d8d2aad706 lib/pci_iomap.c Michael S. Tsirkin 2013-05-29   16   *
> @offset: map memory at the given offset in BAR
> eb29d8d2aad706 lib/pci_iomap.c Michael S. Tsirkin 2013-05-29   17   *
> @maxlen: max length of the memory to map
> 66eab4df288aae lib/pci_iomap.c Michael S. Tsirkin 2011-11-24   18   *
> 66eab4df288aae lib/pci_iomap.c Michael S. Tsirkin 2011-11-24   19   *
> Using this function you will get a __iomem address to your device
> BAR.
> 66eab4df288aae lib/pci_iomap.c Michael S. Tsirkin 2011-11-24   20   *
> You can access it using ioread*() and iowrite*(). These functions
> hide
> 66eab4df288aae lib/pci_iomap.c Michael S. Tsirkin 2011-11-24   21   *
> the details if this is a MMIO or PIO address space and will just do
> what
> 66eab4df288aae lib/pci_iomap.c Michael S. Tsirkin 2011-11-24   22   *
> you expect from them in the correct way.
> 66eab4df288aae lib/pci_iomap.c Michael S. Tsirkin 2011-11-24   23   *
> 66eab4df288aae lib/pci_iomap.c Michael S. Tsirkin 2011-11-24   24   *
> @maxlen specifies the maximum length to map. If you want to get
> access to
> eb29d8d2aad706 lib/pci_iomap.c Michael S. Tsirkin 2013-05-29   25   *
> the complete BAR from offset to the end, pass %0 here.
> 66eab4df288aae lib/pci_iomap.c Michael S. Tsirkin 2011-11-24   26   *
> */
> eb29d8d2aad706 lib/pci_iomap.c Michael S. Tsirkin 2013-05-29 @27
> void __iomem *pci_iomap_range(struct pci_dev *dev,
> eb29d8d2aad706 lib/pci_iomap.c Michael S. Tsirkin 2013-05-29
> 28                             int bar,
> eb29d8d2aad706 lib/pci_iomap.c Michael S. Tsirkin 2013-05-29
> 29                             unsigned long offset,
> eb29d8d2aad706 lib/pci_iomap.c Michael S. Tsirkin 2013-05-29
> 30                             unsigned long maxlen)
> 66eab4df288aae lib/pci_iomap.c Michael S. Tsirkin 2011-11-24   31 {
> 66eab4df288aae lib/pci_iomap.c Michael S. Tsirkin 2011-11-24
> 32       resource_size_t start = pci_resource_start(dev, bar);
> 66eab4df288aae lib/pci_iomap.c Michael S. Tsirkin 2011-11-24
> 33       resource_size_t len = pci_resource_len(dev, bar);
> 66eab4df288aae lib/pci_iomap.c Michael S. Tsirkin 2011-11-24
> 34       unsigned long flags = pci_resource_flags(dev, bar);
> 66eab4df288aae lib/pci_iomap.c Michael S. Tsirkin 2011-11-24   35
> eb29d8d2aad706 lib/pci_iomap.c Michael S. Tsirkin 2013-05-29
> 36       if (len <= offset || !start)
> 66eab4df288aae lib/pci_iomap.c Michael S. Tsirkin 2011-11-24
> 37               return NULL;
> eb29d8d2aad706 lib/pci_iomap.c Michael S. Tsirkin 2013-05-29
> 38       len -= offset;
> eb29d8d2aad706 lib/pci_iomap.c Michael S. Tsirkin 2013-05-29
> 39       start += offset;
> 66eab4df288aae lib/pci_iomap.c Michael S. Tsirkin 2011-11-24
> 40       if (maxlen && len > maxlen)
> 66eab4df288aae lib/pci_iomap.c Michael S. Tsirkin 2011-11-24
> 41               len = maxlen;
> 66eab4df288aae lib/pci_iomap.c Michael S. Tsirkin 2011-11-24
> 42       if (flags & IORESOURCE_IO)
> b923650b84068b lib/pci_iomap.c Michael S. Tsirkin 2012-01-30
> @43               return __pci_ioport_map(dev, start, len);
> 92b19ff50e8f24 lib/pci_iomap.c Dan Williams       2015-08-10
> 44       if (flags & IORESOURCE_MEM)
> 66eab4df288aae lib/pci_iomap.c Michael S. Tsirkin 2011-11-24
> 45               return ioremap(start, len);
> 66eab4df288aae lib/pci_iomap.c Michael S. Tsirkin 2011-11-24
> 46       /* What? */
> 66eab4df288aae lib/pci_iomap.c Michael S. Tsirkin 2011-11-24
> 47       return NULL;
> 66eab4df288aae lib/pci_iomap.c Michael S. Tsirkin 2011-11-24   48 }
> eb29d8d2aad706 lib/pci_iomap.c Michael S. Tsirkin 2013-05-29   49
> EXPORT_SYMBOL(pci_iomap_range);
> 66eab4df288aae lib/pci_iomap.c Michael S. Tsirkin 2011-11-24   50
> 1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24   51
> /**
> 1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24   52   *
> pci_iomap_wc_range - create a virtual WC mapping cookie for a PCI BAR
> 1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24   53   *
> @dev: PCI device that owns the BAR
> 1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24   54   *
> @bar: BAR number
> 1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24   55   *
> @offset: map memory at the given offset in BAR
> 1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24   56   *
> @maxlen: max length of the memory to map
> 1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24   57   *
> 1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24   58   *
> Using this function you will get a __iomem address to your device
> BAR.
> 1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24   59   *
> You can access it using ioread*() and iowrite*(). These functions
> hide
> 1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24   60   *
> the details if this is a MMIO or PIO address space and will just do
> what
> 1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24   61   *
> you expect from them in the correct way. When possible write
> combining
> 1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24   62   *
> is used.
> 1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24   63   *
> 1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24   64   *
> @maxlen specifies the maximum length to map. If you want to get
> access to
> 1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24   65   *
> the complete BAR from offset to the end, pass %0 here.
> 1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24   66   *
> */
> 1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24 @67
> void __iomem *pci_iomap_wc_range(struct pci_dev *dev,
> 1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24
> 68                                int bar,
> 1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24
> 69                                unsigned long offset,
> 1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24
> 70                                unsigned long maxlen)
> 1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24   71 {
> 1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24
> 72       resource_size_t start = pci_resource_start(dev, bar);
> 1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24
> 73       resource_size_t len = pci_resource_len(dev, bar);
> 1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24
> 74       unsigned long flags = pci_resource_flags(dev, bar);
> 1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24   75
> 1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24   76
> 1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24
> 77       if (flags & IORESOURCE_IO)
> 1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24
> 78               return NULL;
> 1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24   79
> 1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24
> 80       if (len <= offset || !start)
> 1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24
> 81               return NULL;
> 1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24   82
> 1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24
> 83       len -= offset;
> 1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24
> 84       start += offset;
> 1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24
> 85       if (maxlen && len > maxlen)
> 1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24
> 86               len = maxlen;
> 1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24   87
> 1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24
> 88       if (flags & IORESOURCE_MEM)
> 1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24
> 89               return ioremap_wc(start, len);
> 1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24   90
> 1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24
> 91       /* What? */
> 1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24
> 92       return NULL;
> 1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24   93 }
> 1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24   94
> EXPORT_SYMBOL_GPL(pci_iomap_wc_range);
> 1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24   95
> eb29d8d2aad706 lib/pci_iomap.c Michael S. Tsirkin 2013-05-29   96
> /**
> eb29d8d2aad706 lib/pci_iomap.c Michael S. Tsirkin 2013-05-29   97   *
> pci_iomap - create a virtual mapping cookie for a PCI BAR
> eb29d8d2aad706 lib/pci_iomap.c Michael S. Tsirkin 2013-05-29   98   *
> @dev: PCI device that owns the BAR
> eb29d8d2aad706 lib/pci_iomap.c Michael S. Tsirkin 2013-05-29   99   *
> @bar: BAR number
> eb29d8d2aad706 lib/pci_iomap.c Michael S. Tsirkin 2013-05-29 100   *
> @maxlen: length of the memory to map
> eb29d8d2aad706 lib/pci_iomap.c Michael S. Tsirkin 2013-05-29 101   *
> eb29d8d2aad706 lib/pci_iomap.c Michael S. Tsirkin 2013-05-29 102   *
> Using this function you will get a __iomem address to your device
> BAR.
> eb29d8d2aad706 lib/pci_iomap.c Michael S. Tsirkin 2013-05-29 103   *
> You can access it using ioread*() and iowrite*(). These functions
> hide
> eb29d8d2aad706 lib/pci_iomap.c Michael S. Tsirkin 2013-05-29 104   *
> the details if this is a MMIO or PIO address space and will just do
> what
> eb29d8d2aad706 lib/pci_iomap.c Michael S. Tsirkin 2013-05-29 105   *
> you expect from them in the correct way.
> eb29d8d2aad706 lib/pci_iomap.c Michael S. Tsirkin 2013-05-29 106   *
> eb29d8d2aad706 lib/pci_iomap.c Michael S. Tsirkin 2013-05-29 107   *
> @maxlen specifies the maximum length to map. If you want to get
> access to
> eb29d8d2aad706 lib/pci_iomap.c Michael S. Tsirkin 2013-05-29 108   *
> the complete BAR without checking for its length first, pass %0 here.
> eb29d8d2aad706 lib/pci_iomap.c Michael S. Tsirkin 2013-05-29 109   *
> */
> eb29d8d2aad706 lib/pci_iomap.c Michael S. Tsirkin 2013-05-29 @110
> void __iomem *pci_iomap(struct pci_dev *dev, int bar, unsigned long
> maxlen)
> eb29d8d2aad706 lib/pci_iomap.c Michael S. Tsirkin 2013-05-29 111 {
> eb29d8d2aad706 lib/pci_iomap.c Michael S. Tsirkin 2013-05-29
> 112       return pci_iomap_range(dev, bar, 0, maxlen);
> eb29d8d2aad706 lib/pci_iomap.c Michael S. Tsirkin 2013-05-29 113 }
> 66eab4df288aae lib/pci_iomap.c Michael S. Tsirkin 2011-11-24 114
> EXPORT_SYMBOL(pci_iomap);
> 1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24 115
> 1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24 116
> /**
> 1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24 117   *
> pci_iomap_wc - create a virtual WC mapping cookie for a PCI BAR
> 1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24 118   *
> @dev: PCI device that owns the BAR
> 1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24 119   *
> @bar: BAR number
> 1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24 120   *
> @maxlen: length of the memory to map
> 1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24 121   *
> 1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24 122   *
> Using this function you will get a __iomem address to your device
> BAR.
> 1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24 123   *
> You can access it using ioread*() and iowrite*(). These functions
> hide
> 1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24 124   *
> the details if this is a MMIO or PIO address space and will just do
> what
> 1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24 125   *
> you expect from them in the correct way. When possible write
> combining
> 1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24 126   *
> is used.
> 1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24 127   *
> 1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24 128   *
> @maxlen specifies the maximum length to map. If you want to get
> access to
> 1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24 129   *
> the complete BAR without checking for its length first, pass %0 here.
> 1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24 130   *
> */
> 1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24 @131
> void __iomem *pci_iomap_wc(struct pci_dev *dev, int bar, unsigned
> long maxlen)
> 1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24 132 {
> 1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24
> 133       return pci_iomap_wc_range(dev, bar, 0, maxlen);
> 1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24 134 }
> 1b3d4200c1e00a lib/pci_iomap.c Luis R. Rodriguez 2015-08-24 135
> EXPORT_SYMBOL_GPL(pci_iomap_wc);
> 316e8d79a0959c lib/pci_iomap.c Linus Torvalds     2021-09-19 136
> 316e8d79a0959c lib/pci_iomap.c Linus Torvalds     2021-09-19 137 /*
> 316e8d79a0959c lib/pci_iomap.c Linus Torvalds     2021-09-19 138   *
> pci_iounmap() somewhat illogically comes from lib/iomap.c for the
> 316e8d79a0959c lib/pci_iomap.c Linus Torvalds     2021-09-19 139   *
> CONFIG_GENERIC_IOMAP case, because that's the code that knows about
> 316e8d79a0959c lib/pci_iomap.c Linus Torvalds     2021-09-19 140   *
> the different IOMAP ranges.
> 316e8d79a0959c lib/pci_iomap.c Linus Torvalds     2021-09-19 141   *
> 316e8d79a0959c lib/pci_iomap.c Linus Torvalds     2021-09-19 142   *
> But if the architecture does not use the generic iomap code, and if
> 316e8d79a0959c lib/pci_iomap.c Linus Torvalds     2021-09-19 143   *
> it has _not_ defined it's own private pci_iounmap function, we define
> 316e8d79a0959c lib/pci_iomap.c Linus Torvalds     2021-09-19 144   *
> it here.
> 316e8d79a0959c lib/pci_iomap.c Linus Torvalds     2021-09-19 145   *
> 316e8d79a0959c lib/pci_iomap.c Linus Torvalds     2021-09-19 146   *
> NOTE! This default implementation assumes that if the architecture
> 316e8d79a0959c lib/pci_iomap.c Linus Torvalds     2021-09-19 147   *
> support ioport mapping (HAS_IOPORT_MAP), the ioport mapping will
> 316e8d79a0959c lib/pci_iomap.c Linus Torvalds     2021-09-19 148   *
> be fixed to the range [ PCI_IOBASE, PCI_IOBASE+IO_SPACE_LIMIT [,
> 316e8d79a0959c lib/pci_iomap.c Linus Torvalds     2021-09-19 149   *
> and does not need unmapping with 'ioport_unmap()'.
> 316e8d79a0959c lib/pci_iomap.c Linus Torvalds     2021-09-19 150   *
> 316e8d79a0959c lib/pci_iomap.c Linus Torvalds     2021-09-19 151   *
> If you have different rules for your architecture, you need to
> 316e8d79a0959c lib/pci_iomap.c Linus Torvalds     2021-09-19 152   *
> implement your own pci_iounmap() that knows the rules for where
> 316e8d79a0959c lib/pci_iomap.c Linus Torvalds     2021-09-19 153   *
> and how IO vs MEM get mapped.
> 316e8d79a0959c lib/pci_iomap.c Linus Torvalds     2021-09-19 154   *
> 316e8d79a0959c lib/pci_iomap.c Linus Torvalds     2021-09-19 155   *
> This code is odd, and the ARCH_HAS/ARCH_WANTS #define logic comes
> 316e8d79a0959c lib/pci_iomap.c Linus Torvalds     2021-09-19 156   *
> from legacy <asm-generic/io.h> header file behavior. In particular,
> 316e8d79a0959c lib/pci_iomap.c Linus Torvalds     2021-09-19 157   *
> it would seem to make sense to do the iounmap(p) for the non-IO-space
> 316e8d79a0959c lib/pci_iomap.c Linus Torvalds     2021-09-19 158   *
> case here regardless, but that's not what the old header file code
> 316e8d79a0959c lib/pci_iomap.c Linus Torvalds     2021-09-19 159   *
> did. Probably incorrectly, but this is meant to be bug-for-bug
> 316e8d79a0959c lib/pci_iomap.c Linus Torvalds     2021-09-19 160   *
> compatible.
> 316e8d79a0959c lib/pci_iomap.c Linus Torvalds     2021-09-19 161
> */
> 316e8d79a0959c lib/pci_iomap.c Linus Torvalds     2021-09-19 162
> #if defined(ARCH_WANTS_GENERIC_PCI_IOUNMAP)
> 316e8d79a0959c lib/pci_iomap.c Linus Torvalds     2021-09-19 163
> 316e8d79a0959c lib/pci_iomap.c Linus Torvalds     2021-09-19 @164
> void pci_iounmap(struct pci_dev *dev, void __iomem *p)
> 316e8d79a0959c lib/pci_iomap.c Linus Torvalds     2021-09-19 165 {
> 316e8d79a0959c lib/pci_iomap.c Linus Torvalds     2021-09-19 166
> #ifdef ARCH_HAS_GENERIC_IOPORT_MAP
> 316e8d79a0959c lib/pci_iomap.c Linus Torvalds     2021-09-19
> 167       uintptr_t start = (uintptr_t) PCI_IOBASE;
> 316e8d79a0959c lib/pci_iomap.c Linus Torvalds     2021-09-19
> 168       uintptr_t addr = (uintptr_t) p;
> 316e8d79a0959c lib/pci_iomap.c Linus Torvalds     2021-09-19 169
> 316e8d79a0959c lib/pci_iomap.c Linus Torvalds     2021-09-19
> 170       if (addr >= start && addr < start + IO_SPACE_LIMIT)
> 316e8d79a0959c lib/pci_iomap.c Linus Torvalds     2021-09-19
> 171               return;
> 316e8d79a0959c lib/pci_iomap.c Linus Torvalds     2021-09-19
> 172       iounmap(p);
> 316e8d79a0959c lib/pci_iomap.c Linus Torvalds     2021-09-19 173
> #endif
> 316e8d79a0959c lib/pci_iomap.c Linus Torvalds     2021-09-19 174 }
> 316e8d79a0959c lib/pci_iomap.c Linus Torvalds     2021-09-19 175
> EXPORT_SYMBOL(pci_iounmap);
> 316e8d79a0959c lib/pci_iomap.c Linus Torvalds     2021-09-19 176
>

2023-11-21 10:46:37

[permalink] [raw]

Subject: Re: [PATCH 1/4] lib: move pci_iomap.c to drivers/pci/

2023-11-21 10:49:09

[permalink] [raw]

Subject: Re: [PATCH 1/4] lib: move pci_iomap.c to drivers/pci/

2023-11-21 13:15:52

[permalink] [raw]

Subject: Re: [PATCH 1/4] lib: move pci_iomap.c to drivers/pci/

2023-11-21 14:39:28

[permalink] [raw]

Subject: Re: [PATCH 4/4] lib/iomap.c: improve comment about pci anomaly

On Tue, 2023-11-21 at 11:03 +0100, Arnd Bergmann wrote:
> On Mon, Nov 20, 2023, at 22:59, Philipp Stanner wrote:
> > lib/iomap.c contains one of the definitions of pci_iounmap(). The
> > current comment above this out-of-place function does not clarify
> > WHY
> > the function is defined here.
> >
> > Linus's detailed comment above pci_iounmap() in drivers/pci/iomap.c
> > clarifies that in a far better way.
> >
> > Extend the existing comment with an excerpt from Linus's and hint
> > at the
> > other implementation in drivers/pci/iomap.c
> >
> > Signed-off-by: Philipp Stanner <[email protected]>
>
> I think instead of explaining why the code is so complicated
> here, I'd prefer to make it more logical and not have to
> explain it.
>
> We should be able to define a generic version like
>
> void pci_iounmap(struct pci_dev *dev, void __iomem * addr)
> {
> #ifdef CONFIG_HAS_IOPORT
>        if (iomem_is_ioport(addr)) {
>               ioport_unmap(addr);
>               return;
>        }
> #endif
>       iounmap(addr)
> }

And where would you like such a function to reside?
drivers/pci/iomap.c?

P.

>
> and then define iomem_is_ioport() in lib/iomap.c for x86,
> while defining it in asm-generic/io.h for the rest,
> with an override in asm/io.h for those architectures
> that need a custom inb().
>
> Note that with ia64 gone, GENERIC_IOMAP is not at all
> generic any more and could just move it to x86 or name
> it something else. This is what currently uses it:
>
> arch/hexagon/Kconfig:   select GENERIC_IOMAP
> arch/um/Kconfig:        select GENERIC_IOMAP
>
> These have no port I/O at all, so it doesn't do anything.
>
> arch/m68k/Kconfig:      select GENERIC_IOMAP
>
> on m68knommu, the default implementation from asm-generic/io.h
> as the same effect as GENERIC_IOMAP but is more efficient.
> On classic m68k, GENERIC_IOMAP does not do what it is
> meant to because I/O ports on ISA devices have port
> numbers above PIO_OFFSET. Also they don't have PCI.
>
> arch/mips/Kconfig:      select GENERIC_IOMAP
>
> This looks completely bogus because it sets PIO_RESERVED
> to 0 and always uses the mmio part of lib/iomap.c.
>
> arch/powerpc/platforms/Kconfig: select GENERIC_IOMAP
>
> This is only used for two platforms: cell and powernv,
> though on Cell it no longer does anything after the
> commit f4981a00636 ("powerpc: Remove the celleb support");
> I think the entire io_workarounds code now be folded
> back into spider_pci.c if we wanted to.
>
> The PowerNV LPC support does seem to still rely on it.
> This tries to do the exact same thing as lib/logic_pio.c
> for Huawei arm64 servers. I suspect that neither of them
> does it entirely correctly since the powerpc side appears
> to just override any non-LPC PIO support while the arm64
> side is missing the ioread/iowrite support.
>
>      Arnd
>

2023-11-21 14:39:50

[permalink] [raw]

Subject: Re: [PATCH 1/4] lib: move pci_iomap.c to drivers/pci/

2023-11-21 14:42:07

[permalink] [raw]

Subject: Re: [PATCH 4/4] lib/iomap.c: improve comment about pci anomaly

On Tue, Nov 21, 2023, at 15:38, Philipp Stanner wrote:
> On Tue, 2023-11-21 at 11:03 +0100, Arnd Bergmann wrote:
>> On Mon, Nov 20, 2023, at 22:59, Philipp Stanner wrote:

>> We should be able to define a generic version like
>>
>> void pci_iounmap(struct pci_dev *dev, void __iomem * addr)
>> {
>> #ifdef CONFIG_HAS_IOPORT
>>        if (iomem_is_ioport(addr)) {
>>               ioport_unmap(addr);
>>               return;
>>        }
>> #endif
>>       iounmap(addr)
>> }
>
> And where would you like such a function to reside?
> drivers/pci/iomap.c?

Yes, I think that would be the logical place. It could also
be an inline function but that's not great on architectures
that don't also have iomem_is_ioport() inline.

Arnd

2023-11-21 15:05:51

[permalink] [raw]

Subject: Re: [PATCH 1/4] lib: move pci_iomap.c to drivers/pci/

2023-11-21 15:41:56

[permalink] [raw]

Subject: Re: [PATCH 1/4] lib: move pci_iomap.c to drivers/pci/

2023-11-21 15:58:37

[permalink] [raw]

Subject: Re: [PATCH 1/4] lib: move pci_iomap.c to drivers/pci/

2023-11-22 01:53:58

by Yujie Liu

[permalink] [raw]

Subject: Re: [PATCH 1/4] lib: move pci_iomap.c to drivers/pci/

Please kindly ignore these duplicate reports. There seems to be a bug
in the robot and we will fix this ASAP. Sorry for the noise.

On Tue, 2023-11-21 at 23:56 +0800, kernel test robot wrote:
> Hi Philipp,
>
> kernel test robot noticed the following build warnings:
>
> [auto build test WARNING on pci/next]
> [also build test WARNING on pci/for-linus linus/master v6.7-rc2 next-
> 20231121]
> [If your patch is applied to the wrong git tree, kindly drop us a
> note.
> And when submitting patch, we suggest to use '--base' as documented
> in
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
>
> url:
> https://github.com/intel-lab-lkp/linux/commits/Philipp-Stanner/lib-move-pci_iomap-c-to-drivers-pci/20231121-060258
> base:
> https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git next
> patch link:
> https://lore.kernel.org/r/20231120215945.52027-3-pstanner%40redhat.com
> patch subject: [PATCH 1/4] lib: move pci_iomap.c to drivers/pci/
> config: sparc64-kismet-CONFIG_GENERIC_PCI_IOMAP-CONFIG_SPARC-0-0
> (https://download.01.org/0day-ci/archive/20231121/202311212316.a0awwk
> [email protected]/config)
> reproduce:
> (https://download.01.org/0day-ci/archive/20231121/202311212316.a0awwk
> [email protected]/reproduce)
>
> If you fix the issue in a separate patch/commit (i.e. not just a new
> version of
> the same patch/commit), kindly add following tags
> > Reported-by: kernel test robot <[email protected]>
> > Closes:
> > https://lore.kernel.org/oe-kbuild-all/[email protected]/
>
> kismet warnings: (new ones prefixed by >>)
> > > kismet: WARNING: unmet direct dependencies detected for
> > > GENERIC_PCI_IOMAP when selected by SPARC
>    /usr/bin/grep: /db/releases/20231121182703/kernel-
> tests/etc/kcflags: No such file or directory
>    {"timestamp":"2023-11-21 22:16:15 +0800", "level":"WARN",
> "event":"kbuild.sh:3942:in `add_etc_kcflags': grep exit 2
> (ShellError)", "detail":"cmd: '/usr/bin/grep' '-v' '-e' '^#' '-e'
> '^$' '/db/releases/20231121182703/kernel-tests/etc/kcflags' \nstderr:
> /usr/bin/grep: /db/releases/20231121182703/kernel-tests/etc/kcflags:
> No such file or directory\n\n", "hostname":"community-kbuild-
> consumer-123", "host_hostname":"lkp-worker50",
> "call_stack":"/zday/kernel-tests/lib/kbuild.sh:3942:in
> `add_etc_kcflags': /usr/bin/grep: /db/releases/20231121182703/kernel-
> tests/etc/kcflags: No such file or directory (ShellError 2)\n from
> /zday/kernel-tests/lib/kbuild.sh:3971: setup_kcflags\n from
> /zday/kernel-tests/lib/kbuild.sh:4016: invoke_make\n from
> /zday/kernel-tests/lib/kbuild.sh:4122: make\n from /zday/kernel-
> tests/lib/kbuild.sh:5623: make_config_allyes\n from /zday/kernel-
> tests/common.sh:209: redirect_error_to_screen\n from /zday/kernel-
> tests/common.sh:217: redirect_command_errors\n from /zday/kernel-
> tests/lib/kbuild.sh:5630: make_config\n from /zday/kernel-
> tests/lib/builder/kismet.sh:156:
> generate_make_olddefconfig_warnings\n from /zday/kernel-
> tests/lib/builder/kismet.sh:297: builder_compile\n from
> /zday/kernel-tests/bisect-test-build-error.sh:94: main\n"}
>
>    WARNING: unmet direct dependencies detected for GENERIC_PCI_IOMAP
>      Depends on [n]: PCI [=n]
>      Selected by [y]:
>      - SPARC [=y]
>

2023-11-22 08:16:32

[permalink] [raw]

Subject: Re: [PATCH 1/4] lib: move pci_iomap.c to drivers/pci/

On Wed, 2023-11-22 at 01:51 +0000, Liu, Yujie wrote:
> Please kindly ignore these duplicate reports. There seems to be a bug
> in the robot and we will fix this ASAP. Sorry for the noise.

They are not exactly duplicates, I think. You notice that by the mails'
bottoms:

Mail N:
WARNING: unmet direct dependencies detected for GENERIC_PCI_IOMAP
Depends on [n]: PCI [=n]
Selected by [y]:
- SPARC [=y]

Mail N-1:
WARNING: unmet direct dependencies detected for GENERIC_PCI_IOMAP
Depends on [n]: PCI [=n]
Selected by [y]:
- PARISC [=y]

etc...

So it seems to me that it's testing all the architectures and then
sends an email for each one where the build fails.

P.

>
> On Tue, 2023-11-21 at 23:56 +0800, kernel test robot wrote:
> > Hi Philipp,
> >
> > kernel test robot noticed the following build warnings:
> >
> > [auto build test WARNING on pci/next]
> > [also build test WARNING on pci/for-linus linus/master v6.7-rc2
> > next-
> > 20231121]
> > [If your patch is applied to the wrong git tree, kindly drop us a
> > note.
> > And when submitting patch, we suggest to use '--base' as documented
> > in
> > https://git-scm.com/docs/git-format-patch#_base_tree_information]
> >
> > url:
> > https://github.com/intel-lab-lkp/linux/commits/Philipp-Stanner/lib-move-pci_iomap-c-to-drivers-pci/20231121-060258
> > base:
> > https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git next
> > patch link:
> > https://lore.kernel.org/r/20231120215945.52027-3-pstanner%40redhat.com
> > patch subject: [PATCH 1/4] lib: move pci_iomap.c to drivers/pci/
> > config: sparc64-kismet-CONFIG_GENERIC_PCI_IOMAP-CONFIG_SPARC-0-0
> > (
> > https://download.01.org/0day-ci/archive/20231121/202311212316.a0awwk
> > [email protected]/config)
> > reproduce:
> > (
> > https://download.01.org/0day-ci/archive/20231121/202311212316.a0awwk
> > [email protected]/reproduce)
> >
> > If you fix the issue in a separate patch/commit (i.e. not just a
> > new
> > version of
> > the same patch/commit), kindly add following tags
> > > Reported-by: kernel test robot <[email protected]>
> > > Closes:
> > > https://lore.kernel.org/oe-kbuild-all/[email protected]/
> >
> > kismet warnings: (new ones prefixed by >>)
> > > > kismet: WARNING: unmet direct dependencies detected for
> > > > GENERIC_PCI_IOMAP when selected by SPARC
> >    /usr/bin/grep: /db/releases/20231121182703/kernel-
> > tests/etc/kcflags: No such file or directory
> >    {"timestamp":"2023-11-21 22:16:15 +0800", "level":"WARN",
> > "event":"kbuild.sh:3942:in `add_etc_kcflags': grep exit 2
> > (ShellError)", "detail":"cmd: '/usr/bin/grep' '-v' '-e' '^#' '-e'
> > '^$' '/db/releases/20231121182703/kernel-tests/etc/kcflags'
> > \nstderr:
> > /usr/bin/grep: /db/releases/20231121182703/kernel-
> > tests/etc/kcflags:
> > No such file or directory\n\n", "hostname":"community-kbuild-
> > consumer-123", "host_hostname":"lkp-worker50",
> > "call_stack":"/zday/kernel-tests/lib/kbuild.sh:3942:in
> > `add_etc_kcflags': /usr/bin/grep:
> > /db/releases/20231121182703/kernel-
> > tests/etc/kcflags: No such file or directory (ShellError 2)\n from
> > /zday/kernel-tests/lib/kbuild.sh:3971: setup_kcflags\n from
> > /zday/kernel-tests/lib/kbuild.sh:4016: invoke_make\n from
> > /zday/kernel-tests/lib/kbuild.sh:4122: make\n from /zday/kernel-
> > tests/lib/kbuild.sh:5623: make_config_allyes\n from /zday/kernel-
> > tests/common.sh:209: redirect_error_to_screen\n from /zday/kernel-
> > tests/common.sh:217: redirect_command_errors\n from /zday/kernel-
> > tests/lib/kbuild.sh:5630: make_config\n from /zday/kernel-
> > tests/lib/builder/kismet.sh:156:
> > generate_make_olddefconfig_warnings\n from /zday/kernel-
> > tests/lib/builder/kismet.sh:297: builder_compile\n from
> > /zday/kernel-tests/bisect-test-build-error.sh:94: main\n"}
> >
> >    WARNING: unmet direct dependencies detected for
> > GENERIC_PCI_IOMAP
> >      Depends on [n]: PCI [=n]
> >      Selected by [y]:
> >      - SPARC [=y]
> >
>

2023-11-22 16:29:18

[permalink] [raw]

Subject: Re: [PATCH 1/4] lib: move pci_iomap.c to drivers/pci/

2023-11-23 06:47:18

by Yujie Liu

[permalink] [raw]

Subject: Re: [PATCH 1/4] lib: move pci_iomap.c to drivers/pci/

Hi Philipp,

On Wed, Nov 22, 2023 at 09:15:52AM +0100, Philipp Stanner wrote:
> On Wed, 2023-11-22 at 01:51 +0000, Liu, Yujie wrote:
> > Please kindly ignore these duplicate reports. There seems to be a bug
> > in the robot and we will fix this ASAP. Sorry for the noise.
>
> They are not exactly duplicates, I think. You notice that by the mails'
> bottoms:
>
> Mail N:
> WARNING: unmet direct dependencies detected for GENERIC_PCI_IOMAP
> Depends on [n]: PCI [=n]
> Selected by [y]:
> - SPARC [=y]
>
> Mail N-1:
> WARNING: unmet direct dependencies detected for GENERIC_PCI_IOMAP
> Depends on [n]: PCI [=n]
> Selected by [y]:
> - PARISC [=y]
>
> etc...
>
> So it seems to me that it's testing all the architectures and then
> sends an email for each one where the build fails.

Thanks for the feedback. Yes, it was testing on various architectures so
they were considered as different unique issues, and a separate report
was sent out for each of them.

However, there is some noise info in the report as shown below, which
indicates a bug in the bot. We will fix this ASAP.

/usr/bin/grep: /db/releases/20231121182703/kernel-tests/etc/kcflags: No such file or directory
{"timestamp":"2023-11-21 22:16:15 +0800", "level":"WARN", "event":"kbuild.sh:3942:in `add_etc_kcflags': grep exit 2 (ShellError)",

Best Regards,
Yujie

>
> P.
>
> >
> > On Tue, 2023-11-21 at 23:56 +0800, kernel test robot wrote:
> > > Hi Philipp,
> > >
> > > kernel test robot noticed the following build warnings:
> > >
> > > [auto build test WARNING on pci/next]
> > > [also build test WARNING on pci/for-linus linus/master v6.7-rc2
> > > next-
> > > 20231121]
> > > [If your patch is applied to the wrong git tree, kindly drop us a
> > > note.
> > > And when submitting patch, we suggest to use '--base' as documented
> > > in
> > > https://git-scm.com/docs/git-format-patch#_base_tree_information]
> > >
> > > url:???
> > > https://github.com/intel-lab-lkp/linux/commits/Philipp-Stanner/lib-move-pci_iomap-c-to-drivers-pci/20231121-060258
> > > base:??
> > > https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git?next
> > > patch link:???
> > > https://lore.kernel.org/r/20231120215945.52027-3-pstanner%40redhat.com
> > > patch subject: [PATCH 1/4] lib: move pci_iomap.c to drivers/pci/
> > > config: sparc64-kismet-CONFIG_GENERIC_PCI_IOMAP-CONFIG_SPARC-0-0
> > > (
> > > https://download.01.org/0day-ci/archive/20231121/202311212316.a0awwk
> > > [email protected]/config)
> > > reproduce:
> > > (
> > > https://download.01.org/0day-ci/archive/20231121/202311212316.a0awwk
> > > [email protected]/reproduce)
> > >
> > > If you fix the issue in a separate patch/commit (i.e. not just a
> > > new
> > > version of
> > > the same patch/commit), kindly add following tags
> > > > Reported-by: kernel test robot <[email protected]>
> > > > Closes:
> > > > https://lore.kernel.org/oe-kbuild-all/[email protected]/
> > >
> > > kismet warnings: (new ones prefixed by >>)
> > > > > kismet: WARNING: unmet direct dependencies detected for
> > > > > GENERIC_PCI_IOMAP when selected by SPARC
> > > ?? /usr/bin/grep: /db/releases/20231121182703/kernel-
> > > tests/etc/kcflags: No such file or directory
> > > ?? {"timestamp":"2023-11-21 22:16:15 +0800", "level":"WARN",
> > > "event":"kbuild.sh:3942:in `add_etc_kcflags': grep exit 2
> > > (ShellError)", "detail":"cmd: '/usr/bin/grep' '-v' '-e' '^#' '-e'
> > > '^$' '/db/releases/20231121182703/kernel-tests/etc/kcflags'
> > > \nstderr:
> > > /usr/bin/grep: /db/releases/20231121182703/kernel-
> > > tests/etc/kcflags:
> > > No such file or directory\n\n", "hostname":"community-kbuild-
> > > consumer-123", "host_hostname":"lkp-worker50",
> > > "call_stack":"/zday/kernel-tests/lib/kbuild.sh:3942:in
> > > `add_etc_kcflags': /usr/bin/grep:
> > > /db/releases/20231121182703/kernel-
> > > tests/etc/kcflags: No such file or directory (ShellError 2)\n? from
> > > /zday/kernel-tests/lib/kbuild.sh:3971: setup_kcflags\n? from
> > > /zday/kernel-tests/lib/kbuild.sh:4016: invoke_make\n? from
> > > /zday/kernel-tests/lib/kbuild.sh:4122: make\n? from /zday/kernel-
> > > tests/lib/kbuild.sh:5623: make_config_allyes\n? from /zday/kernel-
> > > tests/common.sh:209: redirect_error_to_screen\n? from /zday/kernel-
> > > tests/common.sh:217: redirect_command_errors\n? from /zday/kernel-
> > > tests/lib/kbuild.sh:5630: make_config\n? from /zday/kernel-
> > > tests/lib/builder/kismet.sh:156:
> > > generate_make_olddefconfig_warnings\n? from /zday/kernel-
> > > tests/lib/builder/kismet.sh:297: builder_compile\n? from
> > > /zday/kernel-tests/bisect-test-build-error.sh:94: main\n"}
> > > ??
> > > ?? WARNING: unmet direct dependencies detected for
> > > GENERIC_PCI_IOMAP
> > > ???? Depends on [n]: PCI [=n]
> > > ???? Selected by [y]:
> > > ???? - SPARC [=y]
> > >
> >
>
>

2023-11-24 19:08:52

by Danilo Krummrich

[permalink] [raw]

Subject: Re: [PATCH 4/4] lib/iomap.c: improve comment about pci anomaly

Hi Arnd,

On 11/21/23 11:03, Arnd Bergmann wrote:
> On Mon, Nov 20, 2023, at 22:59, Philipp Stanner wrote:
>> lib/iomap.c contains one of the definitions of pci_iounmap(). The
>> current comment above this out-of-place function does not clarify WHY
>> the function is defined here.
>>
>> Linus's detailed comment above pci_iounmap() in drivers/pci/iomap.c
>> clarifies that in a far better way.
>>
>> Extend the existing comment with an excerpt from Linus's and hint at the
>> other implementation in drivers/pci/iomap.c
>>
>> Signed-off-by: Philipp Stanner <[email protected]>
>
> I think instead of explaining why the code is so complicated
> here, I'd prefer to make it more logical and not have to
> explain it.
>
> We should be able to define a generic version like
>
> void pci_iounmap(struct pci_dev *dev, void __iomem * addr)

Let's shed some light on the different config options related to this.

To me it looks like GENERIC_IOMAP always implies GENERIC_PCI_IOMAP.

lib/iomap.c contains a definition of pci_iounmap() since it uses the
common IO_COND() macro. This definitions wins if GENERIC_IOMAP was
selected.

lib/pci_iomap.c contains another definition of pci_iounmap() which is
guarded by ARCH_WANTS_GENERIC_PCI_IOUNMAP to prevent multiple definitions
in case either GENERIC_IOMAP is set or the architecture already defined
pci_iounmap().

What's the purpose of not having set ARCH_HAS_GENERIC_IOPORT_MAP producing
an empty definition of pci_iounmap() though [1]?

And more generally, is there any other (subtle) logic behind this?

[1] https://elixir.bootlin.com/linux/latest/source/lib/pci_iomap.c#L167

> {
> #ifdef CONFIG_HAS_IOPORT
> if (iomem_is_ioport(addr)) {
> ioport_unmap(addr);
> return;
> }
> #endif
> iounmap(addr)
> }
>
> and then define iomem_is_ioport() in lib/iomap.c for x86,
> while defining it in asm-generic/io.h for the rest,
> with an override in asm/io.h for those architectures
> that need a custom inb().

So, that would be similar to IO_COND(), right? What would we need inb() for
in this context?

- Danilo

>
> Note that with ia64 gone, GENERIC_IOMAP is not at all
> generic any more and could just move it to x86 or name
> it something else. This is what currently uses it:
>
> arch/hexagon/Kconfig: select GENERIC_IOMAP
> arch/um/Kconfig: select GENERIC_IOMAP
>
> These have no port I/O at all, so it doesn't do anything.
>
> arch/m68k/Kconfig: select GENERIC_IOMAP
>
> on m68knommu, the default implementation from asm-generic/io.h
> as the same effect as GENERIC_IOMAP but is more efficient.
> On classic m68k, GENERIC_IOMAP does not do what it is
> meant to because I/O ports on ISA devices have port
> numbers above PIO_OFFSET. Also they don't have PCI.
>
> arch/mips/Kconfig: select GENERIC_IOMAP
>
> This looks completely bogus because it sets PIO_RESERVED
> to 0 and always uses the mmio part of lib/iomap.c.
>
> arch/powerpc/platforms/Kconfig: select GENERIC_IOMAP
>
> This is only used for two platforms: cell and powernv,
> though on Cell it no longer does anything after the
> commit f4981a00636 ("powerpc: Remove the celleb support");
> I think the entire io_workarounds code now be folded
> back into spider_pci.c if we wanted to.
>
> The PowerNV LPC support does seem to still rely on it.
> This tries to do the exact same thing as lib/logic_pio.c
> for Huawei arm64 servers. I suspect that neither of them
> does it entirely correctly since the powerpc side appears
> to just override any non-LPC PIO support while the arm64
> side is missing the ioread/iowrite support.
>
> Arnd
>

2023-11-29 10:17:07

[permalink] [raw]

Subject: Re: [PATCH 4/4] lib/iomap.c: improve comment about pci anomaly

Hi,

On Fri, 2023-11-24 at 20:08 +0100, Danilo Krummrich wrote:
> Hi Arnd,
>
> On 11/21/23 11:03, Arnd Bergmann wrote:
> > On Mon, Nov 20, 2023, at 22:59, Philipp Stanner wrote:
> > > lib/iomap.c contains one of the definitions of pci_iounmap(). The
> > > current comment above this out-of-place function does not clarify
> > > WHY
> > > the function is defined here.
> > >
> > > Linus's detailed comment above pci_iounmap() in
> > > drivers/pci/iomap.c
> > > clarifies that in a far better way.
> > >
> > > Extend the existing comment with an excerpt from Linus's and hint
> > > at the
> > > other implementation in drivers/pci/iomap.c
> > >
> > > Signed-off-by: Philipp Stanner <[email protected]>
> >
> > I think instead of explaining why the code is so complicated
> > here, I'd prefer to make it more logical and not have to
> > explain it.
> >
> > We should be able to define a generic version like
> >
> > void pci_iounmap(struct pci_dev *dev, void __iomem * addr)
>
> Let's shed some light on the different config options related to
> this.
>
> To me it looks like GENERIC_IOMAP always implies GENERIC_PCI_IOMAP.
>
> lib/iomap.c contains a definition of pci_iounmap() since it uses the
> common IO_COND() macro. This definitions wins if GENERIC_IOMAP was
> selected.

Yes. So it seems the only way the implementation in lib/pci_iomap.c can
ever win is when someone selects GENERIC_PCI_IOMAP *without* selecting
GENERIC_IOMAP.

>
> lib/pci_iomap.c contains another definition of pci_iounmap() which is
> guarded by ARCH_WANTS_GENERIC_PCI_IOUNMAP to prevent multiple
> definitions
> in case either GENERIC_IOMAP is set or the architecture already
> defined
> pci_iounmap().

To clarify that, here's the relevant excerpt from include/asm-
generic/io.h:

#ifndef CONFIG_GENERIC_IOMAP
#ifndef pci_iounmap
#define ARCH_WANTS_GENERIC_PCI_IOUNMAP
#endif
#endif

>
> What's the purpose of not having set ARCH_HAS_GENERIC_IOPORT_MAP
> producing
> an empty definition of pci_iounmap() though [1]?
>
> And more generally, is there any other (subtle) logic behind this?

That's indeed also very hard to understand for me, because you'd expect
that if pci_iomap() exists (and does something), pci_iounmap() should
also exist and, of course, unmapp the memory again.

From include/asm-generic/io.h:

#ifdef CONFIG_HAS_IOPORT_MAP
#ifndef CONFIG_GENERIC_IOMAP
#ifndef ioport_map
#define ioport_map ioport_map
static inline void __iomem *ioport_map(unsigned long port, unsigned int nr)
{
port &= IO_SPACE_LIMIT;
return (port > MMIO_UPPER_LIMIT) ? NULL : PCI_IOBASE + port;
}
#define ARCH_HAS_GENERIC_IOPORT_MAP
#endif

As far as I understand the logic, an empty pci_iounmap() is generated
(and used?) in lib/pci_iounmap.c if:
* CONFIG_HAS_IOPORT_MAP has not been defined
* CONFIG_GENERIC_IOMAP has been defined (makes sense, then we use the
one from lib/iomap.c anyways)
* ioport_map has been defined by someone other than asm-generic/io.h

Regarding the last point, a number of architectures define their own
ioport_map():

arch/alpha/kernel/io.c, line 684 (as a function)
arch/arc/include/asm/io.h, line 27 (as a function)
arch/arm/mm/iomap.c, line 19 (as a function)
arch/m68k/include/asm/kmap.h, line 60 (as a function)
arch/parisc/lib/iomap.c, line 504 (as a function)
arch/powerpc/kernel/iomap.c, line 14 (as a function)
arch/s390/include/asm/io.h, line 38 (as a function)
arch/sh/kernel/ioport.c, line 24 (as a function)
arch/sparc/lib/iomap.c, line 10 (as a function)

I grepped through those archs and as I see it, none of those specify an
empty pci_iomap() that could be a counterpart to the potentially empty
pci_iounmap() in lib/pci_iomap.c

All of them use the generic pci_iomap.c, which can _never_ be empty.
Perhaps when the functions returning always NULL in include/asm-
generic/pci_iomap.h were to be used...?
But I think they should never be used, because then pci_iomap.c wins.
Arnds seems to agree with that, because he pointed out that these
functions are now surplus relicts in his reply to my Patch Nr.1:

> From what I can tell looking at the header, I think we can
> just remove the "#elif defined(CONFIG_GENERIC_PCI_IOMAP)"
> bit entirely, as it no longer serves the purpose it originally
> had.

So it seems that the empty unmap-function in pci_iomap.c is the left-
over counterpart of those mapping functions always returning NULL.

@Arnd:
Your code draft

void pci_iounmap(struct pci_dev *dev, void __iomem * addr)
{
#ifdef CONFIG_HAS_IOPORT
if (iomem_is_ioport(addr)) {
ioport_unmap(addr);
return;
}
#endif
iounmap(addr)
}

seems to agree with that: There will never be the need for an empty
function that does nothing. Correct?

P.

>
> [1]
> https://elixir.bootlin.com/linux/latest/source/lib/pci_iomap.c#L167
>
> > {
> > #ifdef CONFIG_HAS_IOPORT
> >         if (iomem_is_ioport(addr)) {
> >                ioport_unmap(addr);
> >                return;
> >         }
> > #endif
> >        iounmap(addr)
> > }
> >
> > and then define iomem_is_ioport() in lib/iomap.c for x86,
> > while defining it in asm-generic/io.h for the rest,
> > with an override in asm/io.h for those architectures
> > that need a custom inb().
>
> So, that would be similar to IO_COND(), right? What would we need
> inb() for
> in this context?
>
> - Danilo
>
> >
> > Note that with ia64 gone, GENERIC_IOMAP is not at all
> > generic any more and could just move it to x86 or name
> > it something else. This is what currently uses it:
> >
> > arch/hexagon/Kconfig:   select GENERIC_IOMAP
> > arch/um/Kconfig:        select GENERIC_IOMAP
> >
> > These have no port I/O at all, so it doesn't do anything.
> >
> > arch/m68k/Kconfig:      select GENERIC_IOMAP
> >
> > on m68knommu, the default implementation from asm-generic/io.h
> > as the same effect as GENERIC_IOMAP but is more efficient.
> > On classic m68k, GENERIC_IOMAP does not do what it is
> > meant to because I/O ports on ISA devices have port
> > numbers above PIO_OFFSET. Also they don't have PCI.
> >
> > arch/mips/Kconfig:      select GENERIC_IOMAP
> >
> > This looks completely bogus because it sets PIO_RESERVED
> > to 0 and always uses the mmio part of lib/iomap.c.
> >
> > arch/powerpc/platforms/Kconfig: select GENERIC_IOMAP
> >
> > This is only used for two platforms: cell and powernv,
> > though on Cell it no longer does anything after the
> > commit f4981a00636 ("powerpc: Remove the celleb support");
> > I think the entire io_workarounds code now be folded
> > back into spider_pci.c if we wanted to.
> >
> > The PowerNV LPC support does seem to still rely on it.
> > This tries to do the exact same thing as lib/logic_pio.c
> > for Huawei arm64 servers. I suspect that neither of them
> > does it entirely correctly since the powerpc side appears
> > to just override any non-LPC PIO support while the arm64
> > side is missing the ioread/iowrite support.
> >
> >       Arnd
> >
>

2023-11-29 12:41:16

[permalink] [raw]

Subject: Re: [PATCH 4/4] lib/iomap.c: improve comment about pci anomaly

Hi again,

so I thought about this for a while and want to ask some follow up
questions in addition to those by Danilo in the other mail.

(btw, -CC Herbert Xu, since the mailserver is bouncing)

On Tue, 2023-11-21 at 11:03 +0100, Arnd Bergmann wrote:
> On Mon, Nov 20, 2023, at 22:59, Philipp Stanner wrote:
> > lib/iomap.c contains one of the definitions of pci_iounmap(). The
> > current comment above this out-of-place function does not clarify
> > WHY
> > the function is defined here.
> >
> > Linus's detailed comment above pci_iounmap() in drivers/pci/iomap.c
> > clarifies that in a far better way.
> >
> > Extend the existing comment with an excerpt from Linus's and hint
> > at the
> > other implementation in drivers/pci/iomap.c
> >
> > Signed-off-by: Philipp Stanner <[email protected]>
>
> I think instead of explaining why the code is so complicated
> here, I'd prefer to make it more logical and not have to
> explain it.
>
> We should be able to define a generic version like
>
> void pci_iounmap(struct pci_dev *dev, void __iomem * addr)
> {
> #ifdef CONFIG_HAS_IOPORT
>        if (iomem_is_ioport(addr)) {
>               ioport_unmap(addr);
>               return;
>        }
> #endif
>       iounmap(addr)
> }

ACK, I think this makes sense – if we agree (as in the other thread)
that we never need an empty pci_iounmap().

>
> and then define iomem_is_ioport() in lib/iomap.c for x86,

Wait a minute.
lib/ should never contain architecture-specific code, should it? I
assume your argument is that we write a default version of
iomem_is_ioport(), that could, in theory, be used by many
architectures, but ultimately only x86 will use that default.

> while defining it in asm-generic/io.h for the rest,

So we're not talking about the function prototypes here, but about the
actual implementation that should reside in asm-generic/io.h, aye?
Because the prototype obviously always has to be identical.

> with an override in asm/io.h for those architectures
> that need a custom inb().

So like this in ARCH/include/asm/io.h:

#define iomem_is_ioport iomem_is_ioport
bool iomem_is_ioport(...);

and in include/asm-generic/io.h:

#ifndef iomem_is_ioport
bool iomem_is_ioport(...);

correct?

Still, as Danilo has asked in his email, the question about how inb()
is related to it still stands

>
> Note that with ia64 gone, GENERIC_IOMAP is not at all
> generic any more and could just move it to x86 or name
> it something else. This is what currently uses it:
>
> arch/hexagon/Kconfig:   select GENERIC_IOMAP
> arch/um/Kconfig:        select GENERIC_IOMAP
>
> These have no port I/O at all, so it doesn't do anything.
>
> arch/m68k/Kconfig:      select GENERIC_IOMAP
>
> on m68knommu, the default implementation from asm-generic/io.h
> as the same effect as GENERIC_IOMAP but is more efficient.
> On classic m68k, GENERIC_IOMAP does not do what it is
> meant to because I/O ports on ISA devices have port
> numbers above PIO_OFFSET. Also they don't have PCI.
>
> arch/mips/Kconfig:      select GENERIC_IOMAP
>
> This looks completely bogus because it sets PIO_RESERVED
> to 0 and always uses the mmio part of lib/iomap.c.
>
> arch/powerpc/platforms/Kconfig: select GENERIC_IOMAP
>
> This is only used for two platforms: cell and powernv,
> though on Cell it no longer does anything after the
> commit f4981a00636 ("powerpc: Remove the celleb support");
> I think the entire io_workarounds code now be folded
> back into spider_pci.c if we wanted to.
>
> The PowerNV LPC support does seem to still rely on it.
> This tries to do the exact same thing as lib/logic_pio.c
> for Huawei arm64 servers. I suspect that neither of them
> does it entirely correctly since the powerpc side appears
> to just override any non-LPC PIO support while the arm64
> side is missing the ioread/iowrite support.

I think by now I get what the issue with GENERIC_IOMAP is. But do you
want me to do something about GENERIC_IOMAP (besides the things
directly related to the PCI functionality I'm touching) for you to
approve of a modified version of this patch series?

P.

>
>      Arnd
>

2023-11-29 16:53:17

[permalink] [raw]

Subject: Re: [PATCH 4/4] lib/iomap.c: improve comment about pci anomaly

On Wed, Nov 29, 2023, at 13:40, Philipp Stanner wrote:
> On Tue, 2023-11-21 at 11:03 +0100, Arnd Bergmann wrote:
>> On Mon, Nov 20, 2023, at 22:59, Philipp Stanner wrote:
>> We should be able to define a generic version like
>>
>> void pci_iounmap(struct pci_dev *dev, void __iomem * addr)
>> {
>> #ifdef CONFIG_HAS_IOPORT
>>        if (iomem_is_ioport(addr)) {
>>               ioport_unmap(addr);
>>               return;
>>        }
>> #endif
>>       iounmap(addr)
>> }
>
> ACK, I think this makes sense – if we agree (as in the other thread)
> that we never need an empty pci_iounmap().
>
>>
>> and then define iomem_is_ioport() in lib/iomap.c for x86,
>
> Wait a minute.
> lib/ should never contain architecture-specific code, should it? I
> assume your argument is that we write a default version of
> iomem_is_ioport(), that could, in theory, be used by many
> architectures, but ultimately only x86 will use that default.

I would hope that eventually we can build lib/iomap.c
only on x86, as everything else can live without it.

>> while defining it in asm-generic/io.h for the rest,
>
> So we're not talking about the function prototypes here, but about the
> actual implementation that should reside in asm-generic/io.h, aye?
> Because the prototype obviously always has to be identical.

It could live in lib/pci_iomap.c or in
include/asm-generic/pci_iomap.h, it doesn't really matter
since the definition is trivial. asm-generic/io.h is probably
not the right place, unless we want to merge all of
asm-generic/pci_iomap.h into asm-generic/io.h. We could
do that now that all architectures include asm-generic/io.h
and that includes asm-generic/pci_iomap.h unconditionally.

>> with an override in asm/io.h for those architectures
>> that need a custom inb().
>
> So like this in ARCH/include/asm/io.h:
>
> #define iomem_is_ioport iomem_is_ioport
> bool iomem_is_ioport(...);
>
> and in include/asm-generic/io.h:
>
> #ifndef iomem_is_ioport
> bool iomem_is_ioport(...);
>
> correct?

Yes.

>> arch/powerpc/platforms/Kconfig: select GENERIC_IOMAP
>>
>> This is only used for two platforms: cell and powernv,
>> though on Cell it no longer does anything after the
>> commit f4981a00636 ("powerpc: Remove the celleb support");
>> I think the entire io_workarounds code now be folded
>> back into spider_pci.c if we wanted to.
>>
>> The PowerNV LPC support does seem to still rely on it.
>> This tries to do the exact same thing as lib/logic_pio.c
>> for Huawei arm64 servers. I suspect that neither of them
>> does it entirely correctly since the powerpc side appears
>> to just override any non-LPC PIO support while the arm64
>> side is missing the ioread/iowrite support.
>
> I think by now I get what the issue with GENERIC_IOMAP is. But do you
> want me to do something about GENERIC_IOMAP (besides the things
> directly related to the PCI functionality I'm touching) for you to
> approve of a modified version of this patch series?

It would be nice to clean up some of the architectures
that incorrectly select it at the moment, but that
can be a separate series if you want to get this one
done first, or I can take a look myself.

Arnd

2023-11-29 17:38:11