[PATCH v9 00/13] Make PCI's devres API more consistent

[PATCH v9 00/13] Make PCI's devres API more consistent

[Philipp Stanner]

Changes in v9:
  - Remove forgotten dead code ('enabled' bit in struct pci_dev) in
    patch No.8 ("Move pinned status bit...")
  - Rework patch No.3:
      - Change title from "Reimplement plural devres functions"
        to "Add partial-BAR devres support".
      - Drop excessive details about the general cleanup from the commit
	message. Only motivate why this patch's new infrastructure is
	necessary.
  - Fix some minor spelling issues (s/pci/PCI ...)

Changes in v8:
  - Rebase the series on the already merged patches which were slightly
    modified by Bjorn Helgaas.
  - Reword the pci_intx() commit message so it clearly states it's about
    reworking pci_intx().
  - Move the removal of find_pci_dr() from patch "Remove legacy
    pcim_release()" to patch "Give pci_intx() its own devres callback"
    since this later patch already removed all calls to that function.
  - In patch "Give pci_intx() its own devres callback": use
    pci_is_enabled() (and, thus, the enabled_cnt in struct pci_dev)
    instead of a separate enabled field. (Bjorn)

Changes in v7:
  - Split the entire series in smaller, more atomic chunks / patches
    (Bjorn)
  - Remove functions (such as pcim_iomap_region_range()) that do not yet
    have a user (Bjorn)
  - Don't export interfaces publicly anymore, except for
    pcim_iomap_range(), needed by vboxvideo (Bjorn)
  - Mention the actual (vboxvideo) bug in "PCI: Warn users..." commit
    (Bjorn)
  - Drop docstring warnings on PCI-internal functions (Bjorn)
  - Rework docstring warnings
  - Fix spelling in a few places. Rewrapp paragraphs (Bjorn)

Changes in v6:
  - Restructure the cleanup in pcim_iomap_regions_request_all() so that
    it doesn't trigger a (false positive) test robot warning. No
    behavior change intended. (Dan Carpenter)

Changes in v5:
  - Add Hans's Reviewed-by to vboxvideo patch (Hans de Goede)
  - Remove stable-kernel from CC in vboxvideo patch (Hans de Goede)

Changes in v4:
  - Rebase against linux-next

Changes in v3:
  - Use the term "PCI devres API" at some forgotten places.
  - Fix more grammar errors in patch #3.
  - Remove the comment advising to call (the outdated) pcim_intx() in pci.c
  - Rename __pcim_request_region_range() flags-field "exclusive" to
    "req_flags", since this is what the int actually represents.
  - Remove the call to pcim_region_request() from patch #10. (Hans)

Changes in v2:
  - Make commit head lines congruent with PCI's style (Bjorn)
  - Add missing error checks for devm_add_action(). (Andy)
  - Repair the "Returns: " marks for docu generation (Andy)
  - Initialize the addr_devres struct with memset(). (Andy)
  - Make pcim_intx() a PCI-internal function so that new drivers won't
    be encouraged to use the outdated pci_intx() mechanism.
    (Andy / Philipp)
  - Fix grammar and spelling (Bjorn)
  - Be more precise on why pcim_iomap_table() is problematic (Bjorn)
  - Provide the actual structs' and functions' names in the commit
    messages (Bjorn)
  - Remove redundant variable initializers (Andy)
  - Regroup PM bitfield members in struct pci_dev (Andy)
  - Make pcim_intx() visible only for the PCI subsystem so that new    
    drivers won't use this outdated API (Andy, Myself)
  - Add a NOTE to pcim_iomap() to warn about this function being the one
    exception that does just return NULL.
  - Consistently use the term "PCI devres API"; also in Patch #10 (Bjorn)


¡Hola!

PCI's devres API suffers several weaknesses:

1. There are functions prefixed with pcim_. Those are always managed
   counterparts to never-managed functions prefixed with pci_ – or so one
   would like to think. There are some apparently unmanaged functions
   (all region-request / release functions, and pci_intx()) which
   suddenly become managed once the user has initialized the device with
   pcim_enable_device() instead of pci_enable_device(). This "sometimes
   yes, sometimes no" nature of those functions is confusing and
   therefore bug-provoking. In fact, it has already caused a bug in DRM.
   The last patch in this series fixes that bug.
2. iomappings: Instead of giving each mapping its own callback, the
   existing API uses a statically allocated struct tracking one mapping
   per bar. This is not extensible. Especially, you can't create
   _ranged_ managed mappings that way, which many drivers want.
3. Managed request functions only exist as "plural versions" with a
   bit-mask as a parameter. That's quite over-engineered considering
   that each user only ever mapps one, maybe two bars.

This series:
- add a set of new "singular" devres functions that use devres the way
  its intended, with one callback per resource.
- deprecates the existing iomap-table mechanism.
- deprecates the hybrid nature of pci_ functions.
- preserves backwards compatibility so that drivers using the existing
  API won't notice any changes.
- adds documentation, especially some warning users about the
  complicated nature of PCI's devres.


Note that this series is based on my "unify pci_iounmap"-series from a
few weeks ago. [1]

I tested this on a x86 VM with a simple pci test-device with two
regions. Operates and reserves resources as intended on my system.
Kasan and kmemleak didn't find any problems.

I believe this series cleans the API up as much as possible without
having to port all existing drivers to the new API. Especially, I think
that this implementation is easy to extend if the need for new managed
functions arises :)

Greetings,
P.

Philipp Stanner (13):
  PCI: Add and use devres helper for bit masks
  PCI: Add devres helpers for iomap table
  PCI: Add partial-BAR devres support
  PCI: Deprecate two surplus devres functions
  PCI: Make devres region requests consistent
  PCI: Warn users about complicated devres nature
  PCI: Remove enabled status bit from pci_devres
  PCI: Move pinned status bit to struct pci_dev
  PCI: Give pcim_set_mwi() its own devres callback
  PCI: Give pci_intx() its own devres callback
  PCI: Remove legacy pcim_release()
  PCI: Add pcim_iomap_range()
  drm/vboxvideo: fix mapping leaks

 drivers/gpu/drm/vboxvideo/vbox_main.c |  20 +-
 drivers/pci/devres.c                  | 903 +++++++++++++++++++++-----
 drivers/pci/iomap.c                   |  16 +
 drivers/pci/pci.c                     |  94 ++-
 drivers/pci/pci.h                     |  23 +-
 include/linux/pci.h                   |   5 +-
 6 files changed, 858 insertions(+), 203 deletions(-)

-- 
2.45.0

[PATCH v9 01/13] PCI: Add and use devres helper for bit masks

[Philipp Stanner]

The current derves implementation uses manual shift operations to check
whether a bit in a mask is set. The code can be made more readable by
writing a small helper function for that.

Implement mask_contains_bar() and use it where applicable.

Link: https://lore.kernel.org/r/20240605081605.18769-3-pstanner@redhat.com
Signed-off-by: Philipp Stanner <pstanner@redhat.com>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/devres.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/devres.c b/drivers/pci/devres.c
index 2c562b9eaf80..f13edd4a3873 100644
--- a/drivers/pci/devres.c
+++ b/drivers/pci/devres.c
@@ -161,6 +161,10 @@ int pcim_set_mwi(struct pci_dev *dev)
 }
 EXPORT_SYMBOL(pcim_set_mwi);
 
+static inline bool mask_contains_bar(int mask, int bar)
+{
+	return mask & BIT(bar);
+}
 
 static void pcim_release(struct device *gendev, void *res)
 {
@@ -169,7 +173,7 @@ static void pcim_release(struct device *gendev, void *res)
 	int i;
 
 	for (i = 0; i < DEVICE_COUNT_RESOURCE; i++)
-		if (this->region_mask & (1 << i))
+		if (mask_contains_bar(this->region_mask, i))
 			pci_release_region(dev, i);
 
 	if (this->mwi)
@@ -363,7 +367,7 @@ int pcim_iomap_regions(struct pci_dev *pdev, int mask, const char *name)
 	for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
 		unsigned long len;
 
-		if (!(mask & (1 << i)))
+		if (!mask_contains_bar(mask, i))
 			continue;
 
 		rc = -EINVAL;
@@ -386,7 +390,7 @@ int pcim_iomap_regions(struct pci_dev *pdev, int mask, const char *name)
 	pci_release_region(pdev, i);
  err_inval:
 	while (--i >= 0) {
-		if (!(mask & (1 << i)))
+		if (!mask_contains_bar(mask, i))
 			continue;
 		pcim_iounmap(pdev, iomap[i]);
 		pci_release_region(pdev, i);
@@ -438,7 +442,7 @@ void pcim_iounmap_regions(struct pci_dev *pdev, int mask)
 		return;
 
 	for (i = 0; i < PCIM_IOMAP_MAX; i++) {
-		if (!(mask & (1 << i)))
+		if (!mask_contains_bar(mask, i))
 			continue;
 
 		pcim_iounmap(pdev, iomap[i]);
-- 
2.45.0

[PATCH v9 02/13] PCI: Add devres helpers for iomap table

[Philipp Stanner]

The pcim_iomap_devres.table administrated by pcim_iomap_table() has its
entries set and unset at several places throughout devres.c using manual
iterations which are effectively code duplications.

Add pcim_add_mapping_to_legacy_table() and
pcim_remove_mapping_from_legacy_table() helper functions and use them where
possible.

Link: https://lore.kernel.org/r/20240605081605.18769-4-pstanner@redhat.com
Signed-off-by: Philipp Stanner <pstanner@redhat.com>
[bhelgaas: s/short bar/int bar/ for consistency]
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/devres.c | 77 +++++++++++++++++++++++++++++++++-----------
 1 file changed, 58 insertions(+), 19 deletions(-)

diff --git a/drivers/pci/devres.c b/drivers/pci/devres.c
index f13edd4a3873..845d6fab0ce7 100644
--- a/drivers/pci/devres.c
+++ b/drivers/pci/devres.c
@@ -297,6 +297,52 @@ void __iomem * const *pcim_iomap_table(struct pci_dev *pdev)
 }
 EXPORT_SYMBOL(pcim_iomap_table);
 
+/*
+ * Fill the legacy mapping-table, so that drivers using the old API can
+ * still get a BAR's mapping address through pcim_iomap_table().
+ */
+static int pcim_add_mapping_to_legacy_table(struct pci_dev *pdev,
+					    void __iomem *mapping, int bar)
+{
+	void __iomem **legacy_iomap_table;
+
+	if (bar >= PCI_STD_NUM_BARS)
+		return -EINVAL;
+
+	legacy_iomap_table = (void __iomem **)pcim_iomap_table(pdev);
+	if (!legacy_iomap_table)
+		return -ENOMEM;
+
+	/* The legacy mechanism doesn't allow for duplicate mappings. */
+	WARN_ON(legacy_iomap_table[bar]);
+
+	legacy_iomap_table[bar] = mapping;
+
+	return 0;
+}
+
+/*
+ * Remove a mapping. The table only contains whole-BAR mappings, so this will
+ * never interfere with ranged mappings.
+ */
+static void pcim_remove_mapping_from_legacy_table(struct pci_dev *pdev,
+						  void __iomem *addr)
+{
+	int bar;
+	void __iomem **legacy_iomap_table;
+
+	legacy_iomap_table = (void __iomem **)pcim_iomap_table(pdev);
+	if (!legacy_iomap_table)
+		return;
+
+	for (bar = 0; bar < PCI_STD_NUM_BARS; bar++) {
+		if (legacy_iomap_table[bar] == addr) {
+			legacy_iomap_table[bar] = NULL;
+			return;
+		}
+	}
+}
+
 /**
  * pcim_iomap - Managed pcim_iomap()
  * @pdev: PCI device to iomap for
@@ -308,16 +354,20 @@ EXPORT_SYMBOL(pcim_iomap_table);
  */
 void __iomem *pcim_iomap(struct pci_dev *pdev, int bar, unsigned long maxlen)
 {
-	void __iomem **tbl;
+	void __iomem *mapping;
 
-	BUG_ON(bar >= PCIM_IOMAP_MAX);
-
-	tbl = (void __iomem **)pcim_iomap_table(pdev);
-	if (!tbl || tbl[bar])	/* duplicate mappings not allowed */
+	mapping = pci_iomap(pdev, bar, maxlen);
+	if (!mapping)
 		return NULL;
 
-	tbl[bar] = pci_iomap(pdev, bar, maxlen);
-	return tbl[bar];
+	if (pcim_add_mapping_to_legacy_table(pdev, mapping, bar) != 0)
+		goto err_table;
+
+	return mapping;
+
+err_table:
+	pci_iounmap(pdev, mapping);
+	return NULL;
 }
 EXPORT_SYMBOL(pcim_iomap);
 
@@ -330,20 +380,9 @@ EXPORT_SYMBOL(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);
+	pcim_remove_mapping_from_legacy_table(pdev, addr);
 }
 EXPORT_SYMBOL(pcim_iounmap);
 
-- 
2.45.0

[PATCH v9 03/13] PCI: Add partial-BAR devres support

[Philipp Stanner]

With the current PCI devres API implementing a managed version of
pci_iomap_range() is impossible.

Furthermore, the PCI devres API currently is inconsistent and
complicated. This is in large part due to the fact that there are hybrid
functions which are only sometimes managed via devres, and functions
IO-mapping and requesting several BARs at once and returning mappings
through a separately administrated table.

This table's indexing mechanism does not support partial-BAR mappings.

Another notable problem is that there are no separate managed
counterparts for region-request functions such as pci_request_region(),
as they exist for other PCI functions (e.g., pci_iomap() <->
pcim_iomap()). Instead, functions based on __pci_request_region() change
their internal behavior and suddenly become managed functions when
pcim_enable_device() instead of pci_enable_device() is used.

This API is hard to understand and potentially bug-provoking. Hence, it
should be made more consistent.

This patch adds the necessary infrastructure for partial-BAR mappings
managed with devres. That infrastructure also serves as a ground layer
for significantly simplifying the PCI devres API in subsequent patches
which can then cleanly separate managed and unmanaged API.

When having the long term goal of providing always managed functions
prefixed with "pcim_" and never managed functions prefixed with "pci_"
and, thus, separating managed and unmanaged APIs cleanly, new PCI devres
infrastructure cannot use __pci_request_region() and its wrappers since
those would then again interact with PCI devres and, consequently,
prevent the managed nature from being removed from the pci_* functions
in the first place. Thus, it's necessary to provide an alternative to
__pci_request_region().

This patch addresses the following problems of the PCI devres API:

  a) There is no PCI devres infrastructure on which a managed counter
     part to pci_iomap_range() could be based on.

  b) The vast majority of the users of plural functions such as
     pcim_iomap_regions() only ever sets a single bit in the bit mask,
     consequently making them singular functions anyways.

  c) region-request functions being sometimes managed and sometimes not
     is bug-provoking. pcim_* functions should always be managed, pci_*
     functions never.

Add a new PCI device resource, pcim_addr_devres, that serves to
encapsulate all device resource types related to region requests and
IO-mappings since those are very frequently created together.

Add a set of alternatives cleanly separated from the hybrid mechanism in
__pci_request_region() and its respective wrappers:
  - __pcim_request_region_range()
  - __pcim_release_region_range()
  - __pcim_request_region()
  - __pcim_release_region()

Add the following PCI-internal devres functions based on the above:
  - pcim_iomap_region()
  - pcim_iounmap_region()
  - _pcim_request_region()
  - pcim_request_region()
  - pcim_release_region()
  - pcim_request_all_regions()
  - pcim_release_all_regions()

Add new needed helper pcim_remove_bar_from_legacy_table().

Rework the following public interfaces using the new infrastructure
listed above:
  - pcim_iomap_release()
  - pcim_iomap()
  - pcim_iounmap()
  - pcim_iomap_regions()
  - pcim_iomap_regions_request_all()
  - pcim_iounmap_regions()

Update API documentation.

Link: https://lore.kernel.org/r/20240605081605.18769-5-pstanner@redhat.com
Signed-off-by: Philipp Stanner <pstanner@redhat.com>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/devres.c | 608 ++++++++++++++++++++++++++++++++++++++-----
 drivers/pci/pci.c    |  22 ++
 drivers/pci/pci.h    |   5 +
 3 files changed, 568 insertions(+), 67 deletions(-)

diff --git a/drivers/pci/devres.c b/drivers/pci/devres.c
index 845d6fab0ce7..cf2c11b54ca6 100644
--- a/drivers/pci/devres.c
+++ b/drivers/pci/devres.c
@@ -4,14 +4,243 @@
 #include "pci.h"
 
 /*
- * PCI iomap devres
+ * On the state of PCI's devres implementation:
+ *
+ * The older devres API for PCI has two significant problems:
+ *
+ * 1. It is very strongly tied to the statically allocated mapping table in
+ *    struct pcim_iomap_devres below. This is mostly solved in the sense of the
+ *    pcim_ functions in this file providing things like ranged mapping by
+ *    bypassing this table, wheras the functions that were present in the old
+ *    API still enter the mapping addresses into the table for users of the old
+ *    API.
+ *
+ * 2. The region-request-functions in pci.c do become managed IF the device has
+ *    been enabled with pcim_enable_device() instead of pci_enable_device().
+ *    This resulted in the API becoming inconsistent: Some functions have an
+ *    obviously managed counter-part (e.g., pci_iomap() <-> pcim_iomap()),
+ *    whereas some don't and are never managed, while others don't and are
+ *    _sometimes_ managed (e.g. pci_request_region()).
+ *
+ *    Consequently, in the new API, region requests performed by the pcim_
+ *    functions are automatically cleaned up through the devres callback
+ *    pcim_addr_resource_release(), while requests performed by
+ *    pcim_enable_device() + pci_*region*() are automatically cleaned up
+ *    through the for-loop in pcim_release().
+ *
+ * TODO 1:
+ * Remove the legacy table entirely once all calls to pcim_iomap_table() in
+ * the kernel have been removed.
+ *
+ * TODO 2:
+ * Port everyone calling pcim_enable_device() + pci_*region*() to using the
+ * pcim_ functions. Then, remove all devres functionality from pci_*region*()
+ * functions and remove the associated cleanups described above in point #2.
  */
-#define PCIM_IOMAP_MAX	PCI_STD_NUM_BARS
 
+/*
+ * Legacy struct storing addresses to whole mapped BARs.
+ */
 struct pcim_iomap_devres {
-	void __iomem *table[PCIM_IOMAP_MAX];
+	void __iomem *table[PCI_STD_NUM_BARS];
+};
+
+enum pcim_addr_devres_type {
+	/* Default initializer. */
+	PCIM_ADDR_DEVRES_TYPE_INVALID,
+
+	/* A requested region spanning an entire BAR. */
+	PCIM_ADDR_DEVRES_TYPE_REGION,
+
+	/*
+	 * A requested region spanning an entire BAR, and a mapping for
+	 * the entire BAR.
+	 */
+	PCIM_ADDR_DEVRES_TYPE_REGION_MAPPING,
+
+	/*
+	 * A mapping within a BAR, either spanning the whole BAR or just a
+	 * range.  Without a requested region.
+	 */
+	PCIM_ADDR_DEVRES_TYPE_MAPPING,
 };
 
+/*
+ * This struct envelops IO or MEM addresses, i.e., mappings and region
+ * requests, because those are very frequently requested and released
+ * together.
+ */
+struct pcim_addr_devres {
+	enum pcim_addr_devres_type type;
+	void __iomem *baseaddr;
+	unsigned long offset;
+	unsigned long len;
+	short bar;
+};
+
+static inline void pcim_addr_devres_clear(struct pcim_addr_devres *res)
+{
+	memset(res, 0, sizeof(*res));
+	res->bar = -1;
+}
+
+/*
+ * The following functions, __pcim_*_region*, exist as counterparts to the
+ * versions from pci.c - which, unfortunately, can be in "hybrid mode", i.e.,
+ * sometimes managed, sometimes not.
+ *
+ * To separate the APIs cleanly, we define our own, simplified versions here.
+ */
+
+/**
+ * __pcim_request_region_range - Request a ranged region
+ * @pdev: PCI device the region belongs to
+ * @bar: BAR the range is within
+ * @offset: offset from the BAR's start address
+ * @maxlen: length in bytes, beginning at @offset
+ * @name: name associated with the request
+ * @req_flags: flags for the request, e.g., for kernel-exclusive requests
+ *
+ * Returns: 0 on success, a negative error code on failure.
+ *
+ * Request a range within a device's PCI BAR.  Sanity check the input.
+ */
+static int __pcim_request_region_range(struct pci_dev *pdev, int bar,
+		unsigned long offset, unsigned long maxlen,
+		const char *name, int req_flags)
+{
+	resource_size_t start = pci_resource_start(pdev, bar);
+	resource_size_t len = pci_resource_len(pdev, bar);
+	unsigned long dev_flags = pci_resource_flags(pdev, bar);
+
+	if (start == 0 || len == 0) /* Unused BAR. */
+		return 0;
+	if (len <= offset)
+		return  -EINVAL;
+
+	start += offset;
+	len -= offset;
+
+	if (len > maxlen && maxlen != 0)
+		len = maxlen;
+
+	if (dev_flags & IORESOURCE_IO) {
+		if (!request_region(start, len, name))
+			return -EBUSY;
+	} else if (dev_flags & IORESOURCE_MEM) {
+		if (!__request_mem_region(start, len, name, req_flags))
+			return -EBUSY;
+	} else {
+		/* That's not a device we can request anything on. */
+		return -ENODEV;
+	}
+
+	return 0;
+}
+
+static void __pcim_release_region_range(struct pci_dev *pdev, int bar,
+		unsigned long offset, unsigned long maxlen)
+{
+	resource_size_t start = pci_resource_start(pdev, bar);
+	resource_size_t len = pci_resource_len(pdev, bar);
+	unsigned long flags = pci_resource_flags(pdev, bar);
+
+	if (len <= offset || start == 0)
+		return;
+
+	if (len == 0 || maxlen == 0) /* This an unused BAR. Do nothing. */
+		return;
+
+	start += offset;
+	len -= offset;
+
+	if (len > maxlen)
+		len = maxlen;
+
+	if (flags & IORESOURCE_IO)
+		release_region(start, len);
+	else if (flags & IORESOURCE_MEM)
+		release_mem_region(start, len);
+}
+
+static int __pcim_request_region(struct pci_dev *pdev, int bar,
+		const char *name, int flags)
+{
+	unsigned long offset = 0;
+	unsigned long len = pci_resource_len(pdev, bar);
+
+	return __pcim_request_region_range(pdev, bar, offset, len, name, flags);
+}
+
+static void __pcim_release_region(struct pci_dev *pdev, int bar)
+{
+	unsigned long offset = 0;
+	unsigned long len = pci_resource_len(pdev, bar);
+
+	__pcim_release_region_range(pdev, bar, offset, len);
+}
+
+static void pcim_addr_resource_release(struct device *dev, void *resource_raw)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+	struct pcim_addr_devres *res = resource_raw;
+
+	switch (res->type) {
+	case PCIM_ADDR_DEVRES_TYPE_REGION:
+		__pcim_release_region(pdev, res->bar);
+		break;
+	case PCIM_ADDR_DEVRES_TYPE_REGION_MAPPING:
+		pci_iounmap(pdev, res->baseaddr);
+		__pcim_release_region(pdev, res->bar);
+		break;
+	case PCIM_ADDR_DEVRES_TYPE_MAPPING:
+		pci_iounmap(pdev, res->baseaddr);
+		break;
+	default:
+		break;
+	}
+}
+
+static struct pcim_addr_devres *pcim_addr_devres_alloc(struct pci_dev *pdev)
+{
+	struct pcim_addr_devres *res;
+
+	res = devres_alloc_node(pcim_addr_resource_release, sizeof(*res),
+			GFP_KERNEL, dev_to_node(&pdev->dev));
+	if (res)
+		pcim_addr_devres_clear(res);
+	return res;
+}
+
+/* Just for consistency and readability. */
+static inline void pcim_addr_devres_free(struct pcim_addr_devres *res)
+{
+	devres_free(res);
+}
+
+/*
+ * Used by devres to identify a pcim_addr_devres.
+ */
+static int pcim_addr_resources_match(struct device *dev, void *a_raw, void *b_raw)
+{
+	struct pcim_addr_devres *a, *b;
+
+	a = a_raw;
+	b = b_raw;
+
+	if (a->type != b->type)
+		return 0;
+
+	switch (a->type) {
+	case PCIM_ADDR_DEVRES_TYPE_REGION:
+	case PCIM_ADDR_DEVRES_TYPE_REGION_MAPPING:
+		return a->bar == b->bar;
+	case PCIM_ADDR_DEVRES_TYPE_MAPPING:
+		return a->baseaddr == b->baseaddr;
+	default:
+		return 0;
+	}
+}
 
 static void devm_pci_unmap_iospace(struct device *dev, void *ptr)
 {
@@ -92,8 +321,8 @@ EXPORT_SYMBOL(devm_pci_remap_cfgspace);
  *
  * 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::
+ * Returns a pointer to the remapped memory or an IOMEM_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);
@@ -172,6 +401,17 @@ static void pcim_release(struct device *gendev, void *res)
 	struct pci_devres *this = res;
 	int i;
 
+	/*
+	 * This is legacy code.
+	 *
+	 * All regions requested by a pcim_ function do get released through
+	 * pcim_addr_resource_release(). Thanks to the hybrid nature of the pci_
+	 * region-request functions, this for-loop has to release the regions
+	 * if they have been requested by such a function.
+	 *
+	 * TODO: Remove this once all users of pcim_enable_device() PLUS
+	 * pci-region-request-functions have been ported to pcim_ functions.
+	 */
 	for (i = 0; i < DEVICE_COUNT_RESOURCE; i++)
 		if (mask_contains_bar(this->region_mask, i))
 			pci_release_region(dev, i);
@@ -258,19 +498,21 @@ EXPORT_SYMBOL(pcim_pin_device);
 
 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]);
+	/*
+	 * Do nothing. This is legacy code.
+	 *
+	 * Cleanup of the mappings is now done directly through the callbacks
+	 * registered when creating them.
+	 */
 }
 
 /**
  * pcim_iomap_table - access iomap allocation table
  * @pdev: PCI device to access iomap table for
  *
+ * Returns:
+ * Const pointer to array of __iomem pointers on success, NULL on failure.
+ *
  * 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
@@ -343,30 +585,67 @@ static void pcim_remove_mapping_from_legacy_table(struct pci_dev *pdev,
 	}
 }
 
+/*
+ * The same as pcim_remove_mapping_from_legacy_table(), but identifies the
+ * mapping by its BAR index.
+ */
+static void pcim_remove_bar_from_legacy_table(struct pci_dev *pdev, short bar)
+{
+	void __iomem **legacy_iomap_table;
+
+	if (bar >= PCI_STD_NUM_BARS)
+		return;
+
+	legacy_iomap_table = (void __iomem **)pcim_iomap_table(pdev);
+	if (!legacy_iomap_table)
+		return;
+
+	legacy_iomap_table[bar] = NULL;
+}
+
 /**
  * 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.
+ * Returns: __iomem pointer on success, NULL on failure.
+ *
+ * Managed pci_iomap(). Map is automatically unmapped on driver detach. If
+ * desired, unmap manually only with pcim_iounmap().
+ *
+ * This SHOULD only be used once per BAR.
+ *
+ * NOTE:
+ * Contrary to the other pcim_* functions, this function does not return an
+ * IOMEM_ERR_PTR() on failure, but a simple NULL. This is done for backwards
+ * compatibility.
  */
 void __iomem *pcim_iomap(struct pci_dev *pdev, int bar, unsigned long maxlen)
 {
 	void __iomem *mapping;
+	struct pcim_addr_devres *res;
+
+	res = pcim_addr_devres_alloc(pdev);
+	if (!res)
+		return NULL;
+	res->type = PCIM_ADDR_DEVRES_TYPE_MAPPING;
 
 	mapping = pci_iomap(pdev, bar, maxlen);
 	if (!mapping)
-		return NULL;
+		goto err_iomap;
+	res->baseaddr = mapping;
 
 	if (pcim_add_mapping_to_legacy_table(pdev, mapping, bar) != 0)
 		goto err_table;
 
+	devres_add(&pdev->dev, res);
 	return mapping;
 
 err_table:
 	pci_iounmap(pdev, mapping);
+err_iomap:
+	pcim_addr_devres_free(res);
 	return NULL;
 }
 EXPORT_SYMBOL(pcim_iomap);
@@ -376,91 +655,291 @@ EXPORT_SYMBOL(pcim_iomap);
  * @pdev: PCI device to iounmap for
  * @addr: Address to unmap
  *
- * Managed pci_iounmap().  @addr must have been mapped using pcim_iomap().
+ * Managed pci_iounmap(). @addr must have been mapped using a pcim_* mapping
+ * function.
  */
 void pcim_iounmap(struct pci_dev *pdev, void __iomem *addr)
 {
-	pci_iounmap(pdev, addr);
+	struct pcim_addr_devres res_searched;
+
+	pcim_addr_devres_clear(&res_searched);
+	res_searched.type = PCIM_ADDR_DEVRES_TYPE_MAPPING;
+	res_searched.baseaddr = addr;
+
+	if (devres_release(&pdev->dev, pcim_addr_resource_release,
+			pcim_addr_resources_match, &res_searched) != 0) {
+		/* Doesn't exist. User passed nonsense. */
+		return;
+	}
 
 	pcim_remove_mapping_from_legacy_table(pdev, addr);
 }
 EXPORT_SYMBOL(pcim_iounmap);
 
+/**
+ * pcim_iomap_region - Request and iomap a PCI BAR
+ * @pdev: PCI device to map IO resources for
+ * @bar: Index of a BAR to map
+ * @name: Name associated with the request
+ *
+ * Returns: __iomem pointer on success, an IOMEM_ERR_PTR on failure.
+ *
+ * Mapping and region will get automatically released on driver detach. If
+ * desired, release manually only with pcim_iounmap_region().
+ */
+static void __iomem *pcim_iomap_region(struct pci_dev *pdev, int bar,
+				       const char *name)
+{
+	int ret;
+	struct pcim_addr_devres *res;
+
+	res = pcim_addr_devres_alloc(pdev);
+	if (!res)
+		return IOMEM_ERR_PTR(-ENOMEM);
+
+	res->type = PCIM_ADDR_DEVRES_TYPE_REGION_MAPPING;
+	res->bar = bar;
+
+	ret = __pcim_request_region(pdev, bar, name, 0);
+	if (ret != 0)
+		goto err_region;
+
+	res->baseaddr = pci_iomap(pdev, bar, 0);
+	if (!res->baseaddr) {
+		ret = -EINVAL;
+		goto err_iomap;
+	}
+
+	devres_add(&pdev->dev, res);
+	return res->baseaddr;
+
+err_iomap:
+	__pcim_release_region(pdev, bar);
+err_region:
+	pcim_addr_devres_free(res);
+
+	return IOMEM_ERR_PTR(ret);
+}
+
+/**
+ * pcim_iounmap_region - Unmap and release a PCI BAR
+ * @pdev: PCI device to operate on
+ * @bar: Index of BAR to unmap and release
+ *
+ * Unmap a BAR and release its region manually. Only pass BARs that were
+ * previously mapped by pcim_iomap_region().
+ */
+static void pcim_iounmap_region(struct pci_dev *pdev, int bar)
+{
+	struct pcim_addr_devres res_searched;
+
+	pcim_addr_devres_clear(&res_searched);
+	res_searched.type = PCIM_ADDR_DEVRES_TYPE_REGION_MAPPING;
+	res_searched.bar = bar;
+
+	devres_release(&pdev->dev, pcim_addr_resource_release,
+			pcim_addr_resources_match, &res_searched);
+}
+
 /**
  * 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
+ * @name: Name associated with the requests
+ *
+ * Returns: 0 on success, negative error code on failure.
  *
  * 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;
+	int ret;
+	short bar;
+	void __iomem *mapping;
 
-	iomap = pcim_iomap_table(pdev);
-	if (!iomap)
-		return -ENOMEM;
+	for (bar = 0; bar < DEVICE_COUNT_RESOURCE; bar++) {
+		if (!mask_contains_bar(mask, bar))
+			continue;
 
-	for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
-		unsigned long len;
+		mapping = pcim_iomap_region(pdev, bar, name);
+		if (IS_ERR(mapping)) {
+			ret = PTR_ERR(mapping);
+			goto err;
+		}
+		ret = pcim_add_mapping_to_legacy_table(pdev, mapping, bar);
+		if (ret != 0)
+			goto err;
+	}
 
-		if (!mask_contains_bar(mask, i))
-			continue;
+	return 0;
 
-		rc = -EINVAL;
-		len = pci_resource_len(pdev, i);
-		if (!len)
-			goto err_inval;
+err:
+	while (--bar >= 0) {
+		pcim_iounmap_region(pdev, bar);
+		pcim_remove_bar_from_legacy_table(pdev, bar);
+	}
 
-		rc = pci_request_region(pdev, i, name);
-		if (rc)
-			goto err_inval;
+	return ret;
+}
+EXPORT_SYMBOL(pcim_iomap_regions);
 
-		rc = -ENOMEM;
-		if (!pcim_iomap(pdev, i, 0))
-			goto err_region;
+static int _pcim_request_region(struct pci_dev *pdev, int bar, const char *name,
+		int request_flags)
+{
+	int ret;
+	struct pcim_addr_devres *res;
+
+	res = pcim_addr_devres_alloc(pdev);
+	if (!res)
+		return -ENOMEM;
+	res->type = PCIM_ADDR_DEVRES_TYPE_REGION;
+	res->bar = bar;
+
+	ret = __pcim_request_region(pdev, bar, name, request_flags);
+	if (ret != 0) {
+		pcim_addr_devres_free(res);
+		return ret;
 	}
 
+	devres_add(&pdev->dev, res);
 	return 0;
+}
 
- err_region:
-	pci_release_region(pdev, i);
- err_inval:
-	while (--i >= 0) {
-		if (!mask_contains_bar(mask, i))
-			continue;
-		pcim_iounmap(pdev, iomap[i]);
-		pci_release_region(pdev, i);
+/**
+ * pcim_request_region - Request a PCI BAR
+ * @pdev: PCI device to requestion region for
+ * @bar: Index of BAR to request
+ * @name: Name associated with the request
+ *
+ * Returns: 0 on success, a negative error code on failure.
+ *
+ * Request region specified by @bar.
+ *
+ * The region will automatically be released on driver detach. If desired,
+ * release manually only with pcim_release_region().
+ */
+static int pcim_request_region(struct pci_dev *pdev, int bar, const char *name)
+{
+	return _pcim_request_region(pdev, bar, name, 0);
+}
+
+/**
+ * pcim_release_region - Release a PCI BAR
+ * @pdev: PCI device to operate on
+ * @bar: Index of BAR to release
+ *
+ * Release a region manually that was previously requested by
+ * pcim_request_region().
+ */
+static void pcim_release_region(struct pci_dev *pdev, int bar)
+{
+	struct pcim_addr_devres res_searched;
+
+	pcim_addr_devres_clear(&res_searched);
+	res_searched.type = PCIM_ADDR_DEVRES_TYPE_REGION;
+	res_searched.bar = bar;
+
+	devres_release(&pdev->dev, pcim_addr_resource_release,
+			pcim_addr_resources_match, &res_searched);
+}
+
+
+/**
+ * pcim_release_all_regions - Release all regions of a PCI-device
+ * @pdev: the PCI device
+ *
+ * Release all regions previously requested through pcim_request_region()
+ * or pcim_request_all_regions().
+ *
+ * Can be called from any context, i.e., not necessarily as a counterpart to
+ * pcim_request_all_regions().
+ */
+static void pcim_release_all_regions(struct pci_dev *pdev)
+{
+	short bar;
+
+	for (bar = 0; bar < PCI_STD_NUM_BARS; bar++)
+		pcim_release_region(pdev, bar);
+}
+
+/**
+ * pcim_request_all_regions - Request all regions
+ * @pdev: PCI device to map IO resources for
+ * @name: name associated with the request
+ *
+ * Returns: 0 on success, negative error code on failure.
+ *
+ * Requested regions will automatically be released at driver detach. If
+ * desired, release individual regions with pcim_release_region() or all of
+ * them at once with pcim_release_all_regions().
+ */
+static int pcim_request_all_regions(struct pci_dev *pdev, const char *name)
+{
+	int ret;
+	short bar;
+
+	for (bar = 0; bar < PCI_STD_NUM_BARS; bar++) {
+		ret = pcim_request_region(pdev, bar, name);
+		if (ret != 0)
+			goto err;
 	}
 
-	return rc;
+	return 0;
+
+err:
+	pcim_release_all_regions(pdev);
+
+	return ret;
 }
-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
+ * @name: Name associated with the requests
+ *
+ * Returns: 0 on success, negative error code on failure.
  *
  * Request all PCI BARs and iomap regions specified by @mask.
+ *
+ * To release these resources manually, call pcim_release_region() for the
+ * regions and pcim_iounmap() for the mappings.
  */
 int pcim_iomap_regions_request_all(struct pci_dev *pdev, int mask,
 				   const char *name)
 {
-	int request_mask = ((1 << 6) - 1) & ~mask;
-	int rc;
+	short bar;
+	int ret;
+	void __iomem **legacy_iomap_table;
 
-	rc = pci_request_selected_regions(pdev, request_mask, name);
-	if (rc)
-		return rc;
+	ret = pcim_request_all_regions(pdev, name);
+	if (ret != 0)
+		return ret;
 
-	rc = pcim_iomap_regions(pdev, mask, name);
-	if (rc)
-		pci_release_selected_regions(pdev, request_mask);
-	return rc;
+	for (bar = 0; bar < PCI_STD_NUM_BARS; bar++) {
+		if (!mask_contains_bar(mask, bar))
+			continue;
+		if (!pcim_iomap(pdev, bar, 0))
+			goto err;
+	}
+
+	return 0;
+
+err:
+	/*
+	 * If bar is larger than 0, then pcim_iomap() above has most likely
+	 * failed because of -EINVAL. If it is equal 0, most likely the table
+	 * couldn't be created, indicating -ENOMEM.
+	 */
+	ret = bar > 0 ? -EINVAL : -ENOMEM;
+	legacy_iomap_table = (void __iomem **)pcim_iomap_table(pdev);
+
+	while (--bar >= 0)
+		pcim_iounmap(pdev, legacy_iomap_table[bar]);
+
+	pcim_release_all_regions(pdev);
+
+	return ret;
 }
 EXPORT_SYMBOL(pcim_iomap_regions_request_all);
 
@@ -473,19 +952,14 @@ EXPORT_SYMBOL(pcim_iomap_regions_request_all);
  */
 void pcim_iounmap_regions(struct pci_dev *pdev, int mask)
 {
-	void __iomem * const *iomap;
-	int i;
+	short bar;
 
-	iomap = pcim_iomap_table(pdev);
-	if (!iomap)
-		return;
-
-	for (i = 0; i < PCIM_IOMAP_MAX; i++) {
-		if (!mask_contains_bar(mask, i))
+	for (bar = 0; bar < PCI_STD_NUM_BARS; bar++) {
+		if (!mask_contains_bar(mask, bar))
 			continue;
 
-		pcim_iounmap(pdev, iomap[i]);
-		pci_release_region(pdev, i);
+		pcim_iounmap_region(pdev, bar);
+		pcim_remove_bar_from_legacy_table(pdev, bar);
 	}
 }
 EXPORT_SYMBOL(pcim_iounmap_regions);
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 59e0949fb079..d94445f5f882 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -3883,6 +3883,17 @@ void pci_release_region(struct pci_dev *pdev, int bar)
 		release_mem_region(pci_resource_start(pdev, bar),
 				pci_resource_len(pdev, bar));
 
+	/*
+	 * This devres utility makes this function sometimes managed
+	 * (when pcim_enable_device() has been called before).
+	 *
+	 * This is bad because it conflicts with the pcim_ functions being
+	 * exclusively responsible for managed PCI. Its "sometimes yes,
+	 * sometimes no" nature can cause bugs.
+	 *
+	 * TODO: Remove this once all users that use pcim_enable_device() PLUS
+	 * a region request function have been ported to using pcim_ functions.
+	 */
 	dr = find_pci_dr(pdev);
 	if (dr)
 		dr->region_mask &= ~(1 << bar);
@@ -3927,6 +3938,17 @@ static int __pci_request_region(struct pci_dev *pdev, int bar,
 			goto err_out;
 	}
 
+	/*
+	 * This devres utility makes this function sometimes managed
+	 * (when pcim_enable_device() has been called before).
+	 *
+	 * This is bad because it conflicts with the pcim_ functions being
+	 * exclusively responsible for managed pci. Its "sometimes yes,
+	 * sometimes no" nature can cause bugs.
+	 *
+	 * TODO: Remove this once all users that use pcim_enable_device() PLUS
+	 * a region request function have been ported to using pcim_ functions.
+	 */
 	dr = find_pci_dr(pdev);
 	if (dr)
 		dr->region_mask |= 1 << bar;
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index fd44565c4756..c09487f5550c 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -826,6 +826,11 @@ struct pci_devres {
 	unsigned int orig_intx:1;
 	unsigned int restore_intx:1;
 	unsigned int mwi:1;
+
+	/*
+	 * TODO: remove the region_mask once everyone calling
+	 * pcim_enable_device() + pci_*region*() is ported to pcim_ functions.
+	 */
 	u32 region_mask;
 };
 
-- 
2.45.0

[PATCH v9 04/13] PCI: Deprecate two surplus devres functions

[Philipp Stanner]

pcim_iomap_table() should not be used anymore because it contributed to the
PCI devres API being designed contrary to devres's design goals.

pcim_iomap_regions_request_all() is a surplus, complicated function that
can easily be replaced by using a pcim_* request function in combination
with a pcim_* mapping function.

Mark pcim_iomap_table() and pcim_iomap_regions_request_all() as deprecated
in the function documentation.

Link: https://lore.kernel.org/r/20240605081605.18769-6-pstanner@redhat.com
Signed-off-by: Philipp Stanner <pstanner@redhat.com>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/devres.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/devres.c b/drivers/pci/devres.c
index cf2c11b54ca6..5ecffc7424ed 100644
--- a/drivers/pci/devres.c
+++ b/drivers/pci/devres.c
@@ -507,7 +507,7 @@ static void pcim_iomap_release(struct device *gendev, void *res)
 }
 
 /**
- * pcim_iomap_table - access iomap allocation table
+ * pcim_iomap_table - access iomap allocation table (DEPRECATED)
  * @pdev: PCI device to access iomap table for
  *
  * Returns:
@@ -521,6 +521,11 @@ static void pcim_iomap_release(struct device *gendev, void *res)
  * This function might sleep when the table is first allocated but can
  * be safely called without context and guaranteed to succeed once
  * allocated.
+ *
+ * This function is DEPRECATED. Do not use it in new code. Instead, obtain a
+ * mapping's address directly from one of the pcim_* mapping functions. For
+ * example:
+ * void __iomem *mappy = pcim_iomap(pdev, bar, length);
  */
 void __iomem * const *pcim_iomap_table(struct pci_dev *pdev)
 {
@@ -894,6 +899,7 @@ static int pcim_request_all_regions(struct pci_dev *pdev, const char *name)
 
 /**
  * pcim_iomap_regions_request_all - Request all BARs and iomap specified ones
+ *			(DEPRECATED)
  * @pdev: PCI device to map IO resources for
  * @mask: Mask of BARs to iomap
  * @name: Name associated with the requests
@@ -904,6 +910,10 @@ static int pcim_request_all_regions(struct pci_dev *pdev, const char *name)
  *
  * To release these resources manually, call pcim_release_region() for the
  * regions and pcim_iounmap() for the mappings.
+ *
+ * This function is DEPRECATED. Don't use it in new code. Instead, use one
+ * of the pcim_* region request functions in combination with a pcim_*
+ * mapping function.
  */
 int pcim_iomap_regions_request_all(struct pci_dev *pdev, int mask,
 				   const char *name)
-- 
2.45.0

[PATCH v9 05/13] PCI: Make devres region requests consistent

[Philipp Stanner]

Now that pure managed region request functions are available, the
implementation of the hybrid-functions which are only sometimes managed can
be made more consistent and readable by wrapping those always-managed
functions.

Implement pcim_request_region_exclusive() as a PCI-internal helper.  Have
the PCI request / release functions call their pcim_ counterparts.  Remove
the now surplus region_mask from struct pci_devres.

Link: https://lore.kernel.org/r/20240605081605.18769-7-pstanner@redhat.com
Signed-off-by: Philipp Stanner <pstanner@redhat.com>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/devres.c | 53 ++++++++++++++++++++++----------------------
 drivers/pci/pci.c    | 47 +++++++++++++--------------------------
 drivers/pci/pci.h    | 10 ++++-----
 3 files changed, 45 insertions(+), 65 deletions(-)

diff --git a/drivers/pci/devres.c b/drivers/pci/devres.c
index 5ecffc7424ed..d90bed785c3f 100644
--- a/drivers/pci/devres.c
+++ b/drivers/pci/devres.c
@@ -24,18 +24,15 @@
  *
  *    Consequently, in the new API, region requests performed by the pcim_
  *    functions are automatically cleaned up through the devres callback
- *    pcim_addr_resource_release(), while requests performed by
- *    pcim_enable_device() + pci_*region*() are automatically cleaned up
- *    through the for-loop in pcim_release().
+ *    pcim_addr_resource_release().
+ *    Users utilizing pcim_enable_device() + pci_*region*() are redirected in
+ *    pci.c to the managed functions here in this file. This isn't exactly
+ *    perfect, but the only alternative way would be to port ALL drivers using
+ *    said combination to pcim_ functions.
  *
- * TODO 1:
+ * TODO:
  * Remove the legacy table entirely once all calls to pcim_iomap_table() in
  * the kernel have been removed.
- *
- * TODO 2:
- * Port everyone calling pcim_enable_device() + pci_*region*() to using the
- * pcim_ functions. Then, remove all devres functionality from pci_*region*()
- * functions and remove the associated cleanups described above in point #2.
  */
 
 /*
@@ -399,22 +396,6 @@ static void pcim_release(struct device *gendev, void *res)
 {
 	struct pci_dev *dev = to_pci_dev(gendev);
 	struct pci_devres *this = res;
-	int i;
-
-	/*
-	 * This is legacy code.
-	 *
-	 * All regions requested by a pcim_ function do get released through
-	 * pcim_addr_resource_release(). Thanks to the hybrid nature of the pci_
-	 * region-request functions, this for-loop has to release the regions
-	 * if they have been requested by such a function.
-	 *
-	 * TODO: Remove this once all users of pcim_enable_device() PLUS
-	 * pci-region-request-functions have been ported to pcim_ functions.
-	 */
-	for (i = 0; i < DEVICE_COUNT_RESOURCE; i++)
-		if (mask_contains_bar(this->region_mask, i))
-			pci_release_region(dev, i);
 
 	if (this->mwi)
 		pci_clear_mwi(dev);
@@ -823,11 +804,29 @@ static int _pcim_request_region(struct pci_dev *pdev, int bar, const char *name,
  * The region will automatically be released on driver detach. If desired,
  * release manually only with pcim_release_region().
  */
-static int pcim_request_region(struct pci_dev *pdev, int bar, const char *name)
+int pcim_request_region(struct pci_dev *pdev, int bar, const char *name)
 {
 	return _pcim_request_region(pdev, bar, name, 0);
 }
 
+/**
+ * pcim_request_region_exclusive - Request a PCI BAR exclusively
+ * @pdev: PCI device to requestion region for
+ * @bar: Index of BAR to request
+ * @name: Name associated with the request
+ *
+ * Returns: 0 on success, a negative error code on failure.
+ *
+ * Request region specified by @bar exclusively.
+ *
+ * The region will automatically be released on driver detach. If desired,
+ * release manually only with pcim_release_region().
+ */
+int pcim_request_region_exclusive(struct pci_dev *pdev, int bar, const char *name)
+{
+	return _pcim_request_region(pdev, bar, name, IORESOURCE_EXCLUSIVE);
+}
+
 /**
  * pcim_release_region - Release a PCI BAR
  * @pdev: PCI device to operate on
@@ -836,7 +835,7 @@ static int pcim_request_region(struct pci_dev *pdev, int bar, const char *name)
  * Release a region manually that was previously requested by
  * pcim_request_region().
  */
-static void pcim_release_region(struct pci_dev *pdev, int bar)
+void pcim_release_region(struct pci_dev *pdev, int bar)
 {
 	struct pcim_addr_devres res_searched;
 
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index d94445f5f882..7013699db242 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -3872,7 +3872,15 @@ EXPORT_SYMBOL(pci_enable_atomic_ops_to_root);
  */
 void pci_release_region(struct pci_dev *pdev, int bar)
 {
-	struct pci_devres *dr;
+	/*
+	 * This is done for backwards compatibility, because the old PCI devres
+	 * API had a mode in which the function became managed if it had been
+	 * enabled with pcim_enable_device() instead of pci_enable_device().
+	 */
+	if (pci_is_managed(pdev)) {
+		pcim_release_region(pdev, bar);
+		return;
+	}
 
 	if (pci_resource_len(pdev, bar) == 0)
 		return;
@@ -3882,21 +3890,6 @@ void pci_release_region(struct pci_dev *pdev, int bar)
 	else if (pci_resource_flags(pdev, bar) & IORESOURCE_MEM)
 		release_mem_region(pci_resource_start(pdev, bar),
 				pci_resource_len(pdev, bar));
-
-	/*
-	 * This devres utility makes this function sometimes managed
-	 * (when pcim_enable_device() has been called before).
-	 *
-	 * This is bad because it conflicts with the pcim_ functions being
-	 * exclusively responsible for managed PCI. Its "sometimes yes,
-	 * sometimes no" nature can cause bugs.
-	 *
-	 * TODO: Remove this once all users that use pcim_enable_device() PLUS
-	 * a region request function have been ported to using pcim_ functions.
-	 */
-	dr = find_pci_dr(pdev);
-	if (dr)
-		dr->region_mask &= ~(1 << bar);
 }
 EXPORT_SYMBOL(pci_release_region);
 
@@ -3922,7 +3915,12 @@ EXPORT_SYMBOL(pci_release_region);
 static int __pci_request_region(struct pci_dev *pdev, int bar,
 				const char *res_name, int exclusive)
 {
-	struct pci_devres *dr;
+	if (pci_is_managed(pdev)) {
+		if (exclusive == IORESOURCE_EXCLUSIVE)
+			return pcim_request_region_exclusive(pdev, bar, res_name);
+
+		return pcim_request_region(pdev, bar, res_name);
+	}
 
 	if (pci_resource_len(pdev, bar) == 0)
 		return 0;
@@ -3938,21 +3936,6 @@ static int __pci_request_region(struct pci_dev *pdev, int bar,
 			goto err_out;
 	}
 
-	/*
-	 * This devres utility makes this function sometimes managed
-	 * (when pcim_enable_device() has been called before).
-	 *
-	 * This is bad because it conflicts with the pcim_ functions being
-	 * exclusively responsible for managed pci. Its "sometimes yes,
-	 * sometimes no" nature can cause bugs.
-	 *
-	 * TODO: Remove this once all users that use pcim_enable_device() PLUS
-	 * a region request function have been ported to using pcim_ functions.
-	 */
-	dr = find_pci_dr(pdev);
-	if (dr)
-		dr->region_mask |= 1 << bar;
-
 	return 0;
 
 err_out:
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index c09487f5550c..2403c5a0ff7a 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -826,16 +826,14 @@ struct pci_devres {
 	unsigned int orig_intx:1;
 	unsigned int restore_intx:1;
 	unsigned int mwi:1;
-
-	/*
-	 * TODO: remove the region_mask once everyone calling
-	 * pcim_enable_device() + pci_*region*() is ported to pcim_ functions.
-	 */
-	u32 region_mask;
 };
 
 struct pci_devres *find_pci_dr(struct pci_dev *pdev);
 
+int pcim_request_region(struct pci_dev *pdev, int bar, const char *name);
+int pcim_request_region_exclusive(struct pci_dev *pdev, int bar, const char *name);
+void pcim_release_region(struct pci_dev *pdev, int bar);
+
 /*
  * Config Address for PCI Configuration Mechanism #1
  *
-- 
2.45.0

[PATCH v9 06/13] PCI: Warn users about complicated devres nature

[Philipp Stanner]

The PCI region-request functions become managed functions when
pcim_enable_device() has been called previously instead of
pci_enable_device().

This has already caused a bug (in 8558de401b5f) by confusing users, who
came to believe that all PCI functions, such as pci_iomap_range(), suddenly
are managed that way, which is not the case.

Add comments to the relevant functions' docstrings that warn users about
this behavior.

Link: https://lore.kernel.org/r/20240605081605.18769-8-pstanner@redhat.com
Signed-off-by: Philipp Stanner <pstanner@redhat.com>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/iomap.c | 16 ++++++++++++++++
 drivers/pci/pci.c   | 42 +++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 57 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/iomap.c b/drivers/pci/iomap.c
index c9725428e387..a715a4803c95 100644
--- a/drivers/pci/iomap.c
+++ b/drivers/pci/iomap.c
@@ -23,6 +23,10 @@
  *
  * @maxlen specifies the maximum length to map. If you want to get access to
  * the complete BAR from offset to the end, pass %0 here.
+ *
+ * NOTE:
+ * This function is never managed, even if you initialized with
+ * pcim_enable_device().
  * */
 void __iomem *pci_iomap_range(struct pci_dev *dev,
 			      int bar,
@@ -63,6 +67,10 @@ EXPORT_SYMBOL(pci_iomap_range);
  *
  * @maxlen specifies the maximum length to map. If you want to get access to
  * the complete BAR from offset to the end, pass %0 here.
+ *
+ * NOTE:
+ * This function is never managed, even if you initialized with
+ * pcim_enable_device().
  * */
 void __iomem *pci_iomap_wc_range(struct pci_dev *dev,
 				 int bar,
@@ -106,6 +114,10 @@ EXPORT_SYMBOL_GPL(pci_iomap_wc_range);
  *
  * @maxlen specifies the maximum length to map. If you want to get access to
  * the complete BAR without checking for its length first, pass %0 here.
+ *
+ * NOTE:
+ * This function is never managed, even if you initialized with
+ * pcim_enable_device(). If you need automatic cleanup, use pcim_iomap().
  * */
 void __iomem *pci_iomap(struct pci_dev *dev, int bar, unsigned long maxlen)
 {
@@ -127,6 +139,10 @@ EXPORT_SYMBOL(pci_iomap);
  *
  * @maxlen specifies the maximum length to map. If you want to get access to
  * the complete BAR without checking for its length first, pass %0 here.
+ *
+ * NOTE:
+ * This function is never managed, even if you initialized with
+ * pcim_enable_device().
  * */
 void __iomem *pci_iomap_wc(struct pci_dev *dev, int bar, unsigned long maxlen)
 {
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 7013699db242..5e4f377411ec 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -3900,6 +3900,8 @@ EXPORT_SYMBOL(pci_release_region);
  * @res_name: Name to be associated with resource.
  * @exclusive: whether the region access is exclusive or not
  *
+ * Returns: 0 on success, negative error code on failure.
+ *
  * Mark the PCI region associated with PCI device @pdev BAR @bar as
  * being reserved by owner @res_name.  Do not access any
  * address inside the PCI regions unless this call returns
@@ -3950,6 +3952,8 @@ static int __pci_request_region(struct pci_dev *pdev, int bar,
  * @bar: BAR to be reserved
  * @res_name: Name to be associated with resource
  *
+ * Returns: 0 on success, negative error code on failure.
+ *
  * Mark the PCI region associated with PCI device @pdev BAR @bar as
  * being reserved by owner @res_name.  Do not access any
  * address inside the PCI regions unless this call returns
@@ -3957,6 +3961,11 @@ static int __pci_request_region(struct pci_dev *pdev, int bar,
  *
  * Returns 0 on success, or %EBUSY on error.  A warning
  * message is also printed on failure.
+ *
+ * NOTE:
+ * This is a "hybrid" function: It's normally unmanaged, but becomes managed
+ * when pcim_enable_device() has been called in advance. This hybrid feature is
+ * DEPRECATED! If you want managed cleanup, use the pcim_* functions instead.
  */
 int pci_request_region(struct pci_dev *pdev, int bar, const char *res_name)
 {
@@ -4007,6 +4016,13 @@ static int __pci_request_selected_regions(struct pci_dev *pdev, int bars,
  * @pdev: PCI device whose resources are to be reserved
  * @bars: Bitmask of BARs to be requested
  * @res_name: Name to be associated with resource
+ *
+ * Returns: 0 on success, negative error code on failure.
+ *
+ * NOTE:
+ * This is a "hybrid" function: It's normally unmanaged, but becomes managed
+ * when pcim_enable_device() has been called in advance. This hybrid feature is
+ * DEPRECATED! If you want managed cleanup, use the pcim_* functions instead.
  */
 int pci_request_selected_regions(struct pci_dev *pdev, int bars,
 				 const char *res_name)
@@ -4015,6 +4031,19 @@ int pci_request_selected_regions(struct pci_dev *pdev, int bars,
 }
 EXPORT_SYMBOL(pci_request_selected_regions);
 
+/**
+ * pci_request_selected_regions_exclusive - Request regions exclusively
+ * @pdev: PCI device to request regions from
+ * @bars: bit mask of BARs to request
+ * @res_name: name to be associated with the requests
+ *
+ * Returns: 0 on success, negative error code on failure.
+ *
+ * NOTE:
+ * This is a "hybrid" function: It's normally unmanaged, but becomes managed
+ * when pcim_enable_device() has been called in advance. This hybrid feature is
+ * DEPRECATED! If you want managed cleanup, use the pcim_* functions instead.
+ */
 int pci_request_selected_regions_exclusive(struct pci_dev *pdev, int bars,
 					   const char *res_name)
 {
@@ -4032,7 +4061,6 @@ EXPORT_SYMBOL(pci_request_selected_regions_exclusive);
  * successful call to pci_request_regions().  Call this function only
  * after all use of the PCI regions has ceased.
  */
-
 void pci_release_regions(struct pci_dev *pdev)
 {
 	pci_release_selected_regions(pdev, (1 << PCI_STD_NUM_BARS) - 1);
@@ -4064,6 +4092,8 @@ EXPORT_SYMBOL(pci_request_regions);
  * @pdev: PCI device whose resources are to be reserved
  * @res_name: Name to be associated with resource.
  *
+ * Returns: 0 on success, negative error code on failure.
+ *
  * Mark all PCI regions associated with PCI device @pdev as being reserved
  * by owner @res_name.  Do not access any address inside the PCI regions
  * unless this call returns successfully.
@@ -4073,6 +4103,11 @@ EXPORT_SYMBOL(pci_request_regions);
  *
  * Returns 0 on success, or %EBUSY on error.  A warning message is also
  * printed on failure.
+ *
+ * NOTE:
+ * This is a "hybrid" function: It's normally unmanaged, but becomes managed
+ * when pcim_enable_device() has been called in advance. This hybrid feature is
+ * DEPRECATED! If you want managed cleanup, use the pcim_* functions instead.
  */
 int pci_request_regions_exclusive(struct pci_dev *pdev, const char *res_name)
 {
@@ -4404,6 +4439,11 @@ void pci_disable_parity(struct pci_dev *dev)
  * @enable: boolean: whether to enable or disable PCI INTx
  *
  * Enables/disables PCI INTx for device @pdev
+ *
+ * NOTE:
+ * This is a "hybrid" function: It's normally unmanaged, but becomes managed
+ * when pcim_enable_device() has been called in advance. This hybrid feature is
+ * DEPRECATED!
  */
 void pci_intx(struct pci_dev *pdev, int enable)
 {
-- 
2.45.0

[PATCH v9 07/13] PCI: Remove enabled status bit from pci_devres

[Philipp Stanner]

The PCI devres implementation has a separate boolean to track whether a
device is enabled. That, however, can easily be tracked in an agnostic
manner through the function pci_is_enabled().

Using it allows for simplifying the PCI devres implementation.

Replace the separate 'enabled' status bit from struct pci_devres with
calls to pci_is_enabled() at the appropriate places.

Signed-off-by: Philipp Stanner <pstanner@redhat.com>
---
 drivers/pci/devres.c | 11 ++++-------
 drivers/pci/pci.c    |  6 ------
 drivers/pci/pci.h    |  1 -
 3 files changed, 4 insertions(+), 14 deletions(-)

diff --git a/drivers/pci/devres.c b/drivers/pci/devres.c
index d90bed785c3f..643e3a94a1d6 100644
--- a/drivers/pci/devres.c
+++ b/drivers/pci/devres.c
@@ -403,7 +403,7 @@ static void pcim_release(struct device *gendev, void *res)
 	if (this->restore_intx)
 		pci_intx(dev, this->orig_intx);
 
-	if (this->enabled && !this->pinned)
+	if (pci_is_enabled(dev) && !this->pinned)
 		pci_disable_device(dev);
 }
 
@@ -446,14 +446,11 @@ int pcim_enable_device(struct pci_dev *pdev)
 	dr = get_pci_dr(pdev);
 	if (unlikely(!dr))
 		return -ENOMEM;
-	if (dr->enabled)
-		return 0;
 
 	rc = pci_enable_device(pdev);
-	if (!rc) {
+	if (!rc)
 		pdev->is_managed = 1;
-		dr->enabled = 1;
-	}
+
 	return rc;
 }
 EXPORT_SYMBOL(pcim_enable_device);
@@ -471,7 +468,7 @@ void pcim_pin_device(struct pci_dev *pdev)
 	struct pci_devres *dr;
 
 	dr = find_pci_dr(pdev);
-	WARN_ON(!dr || !dr->enabled);
+	WARN_ON(!dr || !pci_is_enabled(pdev));
 	if (dr)
 		dr->pinned = 1;
 }
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 5e4f377411ec..db2cc48f3d63 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -2218,12 +2218,6 @@ void pci_disable_enabled_device(struct pci_dev *dev)
  */
 void pci_disable_device(struct pci_dev *dev)
 {
-	struct pci_devres *dr;
-
-	dr = find_pci_dr(dev);
-	if (dr)
-		dr->enabled = 0;
-
 	dev_WARN_ONCE(&dev->dev, atomic_read(&dev->enable_cnt) <= 0,
 		      "disabling already-disabled device");
 
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 2403c5a0ff7a..d7f00b43b098 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -821,7 +821,6 @@ static inline pci_power_t mid_pci_get_power_state(struct pci_dev *pdev)
  * then remove them from here.
  */
 struct pci_devres {
-	unsigned int enabled:1;
 	unsigned int pinned:1;
 	unsigned int orig_intx:1;
 	unsigned int restore_intx:1;
-- 
2.45.0

[PATCH v9 08/13] PCI: Move pinned status bit to struct pci_dev

[Philipp Stanner]

The bit describing whether the PCI device is currently pinned is stored
in struct pci_devres. To clean up and simplify the PCI devres API, it's
better if this information is stored in struct pci_dev.

This will later permit simplifying pcim_enable_device().

Move the 'pinned' boolean bit to struct pci_dev.

Restructure bits in struct pci_dev so the pm / pme fields are next to
each other.

Signed-off-by: Philipp Stanner <pstanner@redhat.com>
---
 drivers/pci/devres.c | 14 ++++----------
 drivers/pci/pci.h    |  1 -
 include/linux/pci.h  |  3 ++-
 3 files changed, 6 insertions(+), 12 deletions(-)

diff --git a/drivers/pci/devres.c b/drivers/pci/devres.c
index 643e3a94a1d6..84caa0034813 100644
--- a/drivers/pci/devres.c
+++ b/drivers/pci/devres.c
@@ -403,7 +403,7 @@ static void pcim_release(struct device *gendev, void *res)
 	if (this->restore_intx)
 		pci_intx(dev, this->orig_intx);
 
-	if (pci_is_enabled(dev) && !this->pinned)
+	if (pci_is_enabled(dev) && !dev->pinned)
 		pci_disable_device(dev);
 }
 
@@ -459,18 +459,12 @@ 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().
+ * 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 || !pci_is_enabled(pdev));
-	if (dr)
-		dr->pinned = 1;
+	pdev->pinned = true;
 }
 EXPORT_SYMBOL(pcim_pin_device);
 
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index d7f00b43b098..6e02ba1b5947 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -821,7 +821,6 @@ static inline pci_power_t mid_pci_get_power_state(struct pci_dev *pdev)
  * then remove them from here.
  */
 struct pci_devres {
-	unsigned int pinned:1;
 	unsigned int orig_intx:1;
 	unsigned int restore_intx:1;
 	unsigned int mwi:1;
diff --git a/include/linux/pci.h b/include/linux/pci.h
index fb004fd4e889..0c19f0717899 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -367,10 +367,11 @@ struct pci_dev {
 					   this is D0-D3, D0 being fully
 					   functional, and D3 being off. */
 	u8		pm_cap;		/* PM capability offset */
-	unsigned int	imm_ready:1;	/* Supports Immediate Readiness */
 	unsigned int	pme_support:5;	/* Bitmask of states from which PME#
 					   can be generated */
 	unsigned int	pme_poll:1;	/* Poll device's PME status bit */
+	unsigned int	pinned:1;	/* Whether this dev is pinned */
+	unsigned int	imm_ready:1;	/* Supports Immediate Readiness */
 	unsigned int	d1_support:1;	/* Low power state D1 is supported */
 	unsigned int	d2_support:1;	/* Low power state D2 is supported */
 	unsigned int	no_d1d2:1;	/* D1 and D2 are forbidden */
-- 
2.45.0

[PATCH v9 09/13] PCI: Give pcim_set_mwi() its own devres callback

[Philipp Stanner]

Managing pci_set_mwi() with devres can easily be done with its own
callback, without the necessity to store any state about it in a
device-related struct.

Remove the MWI state from struct pci_devres.
Give pcim_set_mwi() a separate devres-callback.

Signed-off-by: Philipp Stanner <pstanner@redhat.com>
---
 drivers/pci/devres.c | 29 ++++++++++++++++++-----------
 drivers/pci/pci.h    |  1 -
 2 files changed, 18 insertions(+), 12 deletions(-)

diff --git a/drivers/pci/devres.c b/drivers/pci/devres.c
index 84caa0034813..e8de93e95eb6 100644
--- a/drivers/pci/devres.c
+++ b/drivers/pci/devres.c
@@ -366,24 +366,34 @@ void __iomem *devm_pci_remap_cfg_resource(struct device *dev,
 }
 EXPORT_SYMBOL(devm_pci_remap_cfg_resource);
 
+static void __pcim_clear_mwi(void *pdev_raw)
+{
+	struct pci_dev *pdev = pdev_raw;
+
+	pci_clear_mwi(pdev);
+}
+
 /**
  * pcim_set_mwi - a device-managed pci_set_mwi()
- * @dev: the PCI device for which MWI is enabled
+ * @pdev: 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)
+int pcim_set_mwi(struct pci_dev *pdev)
 {
-	struct pci_devres *dr;
+	int ret;
 
-	dr = find_pci_dr(dev);
-	if (!dr)
-		return -ENOMEM;
+	ret = devm_add_action(&pdev->dev, __pcim_clear_mwi, pdev);
+	if (ret != 0)
+		return ret;
+
+	ret = pci_set_mwi(pdev);
+	if (ret != 0)
+		devm_remove_action(&pdev->dev, __pcim_clear_mwi, pdev);
 
-	dr->mwi = 1;
-	return pci_set_mwi(dev);
+	return ret;
 }
 EXPORT_SYMBOL(pcim_set_mwi);
 
@@ -397,9 +407,6 @@ static void pcim_release(struct device *gendev, void *res)
 	struct pci_dev *dev = to_pci_dev(gendev);
 	struct pci_devres *this = res;
 
-	if (this->mwi)
-		pci_clear_mwi(dev);
-
 	if (this->restore_intx)
 		pci_intx(dev, this->orig_intx);
 
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 6e02ba1b5947..c355bb6a698d 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -823,7 +823,6 @@ static inline pci_power_t mid_pci_get_power_state(struct pci_dev *pdev)
 struct pci_devres {
 	unsigned int orig_intx:1;
 	unsigned int restore_intx:1;
-	unsigned int mwi:1;
 };
 
 struct pci_devres *find_pci_dr(struct pci_dev *pdev);
-- 
2.45.0

[PATCH v9 10/13] PCI: Give pci_intx() its own devres callback

[Philipp Stanner]

pci_intx() is one of the functions that have "hybrid mode" (i.e.,
sometimes managed, sometimes not). Providing a separate pcim_intx()
function with its own device resource and cleanup callback allows for
removing further large parts of the legacy PCI devres implementation.

As in the region-request-functions, pci_intx() has to call into its
managed counterpart for backwards compatibility.

As pci_intx() is an outdated function, pcim_intx() shall not be made
visible to drivers via a public API.

Implement pcim_intx() with its own device resource.
Make pci_intx() call pcim_intx() in the managed case.
Remove the now surplus function find_pci_dr().

Signed-off-by: Philipp Stanner <pstanner@redhat.com>
---
 drivers/pci/devres.c | 76 ++++++++++++++++++++++++++++++++++++--------
 drivers/pci/pci.c    | 21 ++++++------
 drivers/pci/pci.h    | 13 ++++----
 3 files changed, 80 insertions(+), 30 deletions(-)

diff --git a/drivers/pci/devres.c b/drivers/pci/devres.c
index e8de93e95eb6..7b72c952a9e5 100644
--- a/drivers/pci/devres.c
+++ b/drivers/pci/devres.c
@@ -42,6 +42,11 @@ struct pcim_iomap_devres {
 	void __iomem *table[PCI_STD_NUM_BARS];
 };
 
+/* Used to restore the old intx state on driver detach. */
+struct pcim_intx_devres {
+	int orig_intx;
+};
+
 enum pcim_addr_devres_type {
 	/* Default initializer. */
 	PCIM_ADDR_DEVRES_TYPE_INVALID,
@@ -397,32 +402,75 @@ int pcim_set_mwi(struct pci_dev *pdev)
 }
 EXPORT_SYMBOL(pcim_set_mwi);
 
+
 static inline bool mask_contains_bar(int mask, int bar)
 {
 	return mask & BIT(bar);
 }
 
-static void pcim_release(struct device *gendev, void *res)
+static void pcim_intx_restore(struct device *dev, void *data)
 {
-	struct pci_dev *dev = to_pci_dev(gendev);
-	struct pci_devres *this = res;
+	struct pci_dev *pdev = to_pci_dev(dev);
+	struct pcim_intx_devres *res = data;
 
-	if (this->restore_intx)
-		pci_intx(dev, this->orig_intx);
+	pci_intx(pdev, res->orig_intx);
+}
 
-	if (pci_is_enabled(dev) && !dev->pinned)
-		pci_disable_device(dev);
+static struct pcim_intx_devres *get_or_create_intx_devres(struct device *dev)
+{
+	struct pcim_intx_devres *res;
+
+	res = devres_find(dev, pcim_intx_restore, NULL, NULL);
+	if (res)
+		return res;
+
+	res = devres_alloc(pcim_intx_restore, sizeof(*res), GFP_KERNEL);
+	if (res)
+		devres_add(dev, res);
+
+	return res;
 }
 
-/*
- * TODO: After the last four callers in pci.c are ported, find_pci_dr()
- * needs to be made static again.
+/**
+ * pcim_intx - managed pci_intx()
+ * @pdev: the PCI device to operate on
+ * @enable: boolean: whether to enable or disable PCI INTx
+ *
+ * Returns: 0 on success, -ENOMEM on error.
+ *
+ * Enables/disables PCI INTx for device @pdev.
+ * Restores the original state on driver detach.
  */
-struct pci_devres *find_pci_dr(struct pci_dev *pdev)
+int pcim_intx(struct pci_dev *pdev, int enable)
 {
-	if (pci_is_managed(pdev))
-		return devres_find(&pdev->dev, pcim_release, NULL, NULL);
-	return NULL;
+	u16 pci_command, new;
+	struct pcim_intx_devres *res;
+
+	res = get_or_create_intx_devres(&pdev->dev);
+	if (!res)
+		return -ENOMEM;
+
+	res->orig_intx = !enable;
+
+	pci_read_config_word(pdev, PCI_COMMAND, &pci_command);
+
+	if (enable)
+		new = pci_command & ~PCI_COMMAND_INTX_DISABLE;
+	else
+		new = pci_command | PCI_COMMAND_INTX_DISABLE;
+
+	if (new != pci_command)
+		pci_write_config_word(pdev, PCI_COMMAND, new);
+
+	return 0;
+}
+
+static void pcim_release(struct device *gendev, void *res)
+{
+	struct pci_dev *dev = to_pci_dev(gendev);
+
+	if (pci_is_enabled(dev) && !dev->pinned)
+		pci_disable_device(dev);
 }
 
 static struct pci_devres *get_pci_dr(struct pci_dev *pdev)
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index db2cc48f3d63..1b4832a60047 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -4443,6 +4443,16 @@ void pci_intx(struct pci_dev *pdev, int enable)
 {
 	u16 pci_command, new;
 
+	/*
+	 * This is done for backwards compatibility, because the old PCI devres
+	 * API had a mode in which this function became managed if the dev had
+	 * been enabled with pcim_enable_device() instead of pci_enable_device().
+	 */
+	if (pci_is_managed(pdev)) {
+		WARN_ON_ONCE(pcim_intx(pdev, enable) != 0);
+		return;
+	}
+
 	pci_read_config_word(pdev, PCI_COMMAND, &pci_command);
 
 	if (enable)
@@ -4450,17 +4460,8 @@ void pci_intx(struct pci_dev *pdev, int enable)
 	else
 		new = pci_command | PCI_COMMAND_INTX_DISABLE;
 
-	if (new != pci_command) {
-		struct pci_devres *dr;
-
+	if (new != pci_command)
 		pci_write_config_word(pdev, PCI_COMMAND, new);
-
-		dr = find_pci_dr(pdev);
-		if (dr && !dr->restore_intx) {
-			dr->restore_intx = 1;
-			dr->orig_intx = !enable;
-		}
-	}
 }
 EXPORT_SYMBOL_GPL(pci_intx);
 
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index c355bb6a698d..9e87528f1157 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -816,16 +816,17 @@ static inline pci_power_t mid_pci_get_power_state(struct pci_dev *pdev)
  * there's no need to track it separately.  pci_devres is initialized
  * when a device is enabled using managed PCI device enable interface.
  *
- * TODO: Struct pci_devres and find_pci_dr() only need to be here because
- * they're used in pci.c.  Port or move these functions to devres.c and
- * then remove them from here.
+ * TODO: Struct pci_devres only needs to be here because they're used in pci.c.
+ * Port or move these functions to devres.c and then remove them from here.
  */
 struct pci_devres {
-	unsigned int orig_intx:1;
-	unsigned int restore_intx:1;
+	/*
+	 * TODO:
+	 * This struct is now surplus. Remove it by refactoring pci/devres.c
+	 */
 };
 
-struct pci_devres *find_pci_dr(struct pci_dev *pdev);
+int pcim_intx(struct pci_dev *dev, int enable);
 
 int pcim_request_region(struct pci_dev *pdev, int bar, const char *name);
 int pcim_request_region_exclusive(struct pci_dev *pdev, int bar, const char *name);
-- 
2.45.0

[PATCH v9 11/13] PCI: Remove legacy pcim_release()

[Philipp Stanner]

Thanks to preceding cleanup steps, pcim_release() is now not needed
anymore and can be replaced by pcim_disable_device(), which is the exact
counterpart to pcim_enable_device().

This permits removing further parts of the old PCI devres implementation.

Replace pcim_release() with pcim_disable_device().
Remove the now surplus function get_pci_dr().
Remove the struct pci_devres from pci.h.

Signed-off-by: Philipp Stanner <pstanner@redhat.com>
---
 drivers/pci/devres.c | 53 +++++++++++++++++++++-----------------------
 drivers/pci/pci.h    | 16 -------------
 2 files changed, 25 insertions(+), 44 deletions(-)

diff --git a/drivers/pci/devres.c b/drivers/pci/devres.c
index 7b72c952a9e5..37ac8fd37291 100644
--- a/drivers/pci/devres.c
+++ b/drivers/pci/devres.c
@@ -465,48 +465,45 @@ int pcim_intx(struct pci_dev *pdev, int enable)
 	return 0;
 }
 
-static void pcim_release(struct device *gendev, void *res)
+static void pcim_disable_device(void *pdev_raw)
 {
-	struct pci_dev *dev = to_pci_dev(gendev);
-
-	if (pci_is_enabled(dev) && !dev->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;
+	struct pci_dev *pdev = pdev_raw;
 
-	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);
+	if (!pdev->pinned)
+		pci_disable_device(pdev);
 }
 
 /**
  * pcim_enable_device - Managed pci_enable_device()
  * @pdev: PCI device to be initialized
  *
- * Managed pci_enable_device().
+ * Returns: 0 on success, negative error code on failure.
+ *
+ * Managed pci_enable_device(). Device will automatically be disabled on
+ * driver detach.
  */
 int pcim_enable_device(struct pci_dev *pdev)
 {
-	struct pci_devres *dr;
-	int rc;
+	int ret;
 
-	dr = get_pci_dr(pdev);
-	if (unlikely(!dr))
-		return -ENOMEM;
+	ret = devm_add_action(&pdev->dev, pcim_disable_device, pdev);
+	if (ret != 0)
+		return ret;
 
-	rc = pci_enable_device(pdev);
-	if (!rc)
-		pdev->is_managed = 1;
+	/*
+	 * We prefer removing the action in case of an error over
+	 * devm_add_action_or_reset() because the later could theoretically be
+	 * disturbed by users having pinned the device too soon.
+	 */
+	ret = pci_enable_device(pdev);
+	if (ret != 0) {
+		devm_remove_action(&pdev->dev, pcim_disable_device, pdev);
+		return ret;
+	}
 
-	return rc;
+	pdev->is_managed = true;
+
+	return ret;
 }
 EXPORT_SYMBOL(pcim_enable_device);
 
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 9e87528f1157..e51e6fa79fcc 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -810,22 +810,6 @@ static inline pci_power_t mid_pci_get_power_state(struct pci_dev *pdev)
 }
 #endif
 
-/*
- * 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.
- *
- * TODO: Struct pci_devres only needs to be here because they're used in pci.c.
- * Port or move these functions to devres.c and then remove them from here.
- */
-struct pci_devres {
-	/*
-	 * TODO:
-	 * This struct is now surplus. Remove it by refactoring pci/devres.c
-	 */
-};
-
 int pcim_intx(struct pci_dev *dev, int enable);
 
 int pcim_request_region(struct pci_dev *pdev, int bar, const char *name);
-- 
2.45.0

[PATCH v9 12/13] PCI: Add pcim_iomap_range()

[Philipp Stanner]

The only managed mapping function currently is pcim_iomap() which
doesn't allow for mapping an area starting at a certain offset, which
many drivers want.

Add pcim_iomap_range() as an exported function.

Signed-off-by: Philipp Stanner <pstanner@redhat.com>
---
 drivers/pci/devres.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/pci.h  |  2 ++
 2 files changed, 46 insertions(+)

diff --git a/drivers/pci/devres.c b/drivers/pci/devres.c
index 37ac8fd37291..2f0379a4e58f 100644
--- a/drivers/pci/devres.c
+++ b/drivers/pci/devres.c
@@ -1015,3 +1015,47 @@ void pcim_iounmap_regions(struct pci_dev *pdev, int mask)
 	}
 }
 EXPORT_SYMBOL(pcim_iounmap_regions);
+
+/**
+ * pcim_iomap_range - Create a ranged __iomap mapping within a PCI BAR
+ * @pdev: PCI device to map IO resources for
+ * @bar: Index of the BAR
+ * @offset: Offset from the begin of the BAR
+ * @len: Length in bytes for the mapping
+ *
+ * Returns: __iomem pointer on success, an IOMEM_ERR_PTR on failure.
+ *
+ * Creates a new IO-Mapping within the specified @bar, ranging from @offset to
+ * @offset + @len.
+ *
+ * The mapping will automatically get unmapped on driver detach. If desired,
+ * release manually only with pcim_iounmap().
+ */
+void __iomem *pcim_iomap_range(struct pci_dev *pdev, int bar,
+		unsigned long offset, unsigned long len)
+{
+	void __iomem *mapping;
+	struct pcim_addr_devres *res;
+
+	res = pcim_addr_devres_alloc(pdev);
+	if (!res)
+		return IOMEM_ERR_PTR(-ENOMEM);
+
+	mapping = pci_iomap_range(pdev, bar, offset, len);
+	if (!mapping) {
+		pcim_addr_devres_free(res);
+		return IOMEM_ERR_PTR(-EINVAL);
+	}
+
+	res->type = PCIM_ADDR_DEVRES_TYPE_MAPPING;
+	res->baseaddr = mapping;
+
+	/*
+	 * Ranged mappings don't get added to the legacy-table, since the table
+	 * only ever keeps track of whole BARs.
+	 */
+
+	devres_add(&pdev->dev, res);
+	return mapping;
+}
+EXPORT_SYMBOL(pcim_iomap_range);
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 0c19f0717899..98893a89bb5b 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -2303,6 +2303,8 @@ int pcim_iomap_regions(struct pci_dev *pdev, int mask, const char *name);
 int pcim_iomap_regions_request_all(struct pci_dev *pdev, int mask,
 				   const char *name);
 void pcim_iounmap_regions(struct pci_dev *pdev, int mask);
+void __iomem *pcim_iomap_range(struct pci_dev *pdev, int bar,
+				unsigned long offset, unsigned long len);
 
 extern int pci_pci_problems;
 #define PCIPCI_FAIL		1	/* No PCI PCI DMA */
-- 
2.45.0

[PATCH v9 13/13] drm/vboxvideo: fix mapping leaks

[Philipp Stanner]

When the PCI devres API was introduced to this driver, it was wrongly
assumed that initializing the device with pcim_enable_device() instead
of pci_enable_device() will make all PCI functions managed.

This is wrong and was caused by the quite confusing PCI devres API in
which some, but not all, functions become managed that way.

The function pci_iomap_range() is never managed.

Replace pci_iomap_range() with the actually managed function
pcim_iomap_range().

Fixes: 8558de401b5f ("drm/vboxvideo: use managed pci functions")
Signed-off-by: Philipp Stanner <pstanner@redhat.com>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/gpu/drm/vboxvideo/vbox_main.c | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/vboxvideo/vbox_main.c b/drivers/gpu/drm/vboxvideo/vbox_main.c
index 42c2d8a99509..d4ade9325401 100644
--- a/drivers/gpu/drm/vboxvideo/vbox_main.c
+++ b/drivers/gpu/drm/vboxvideo/vbox_main.c
@@ -42,12 +42,11 @@ static int vbox_accel_init(struct vbox_private *vbox)
 	/* Take a command buffer for each screen from the end of usable VRAM. */
 	vbox->available_vram_size -= vbox->num_crtcs * VBVA_MIN_BUFFER_SIZE;
 
-	vbox->vbva_buffers = pci_iomap_range(pdev, 0,
-					     vbox->available_vram_size,
-					     vbox->num_crtcs *
-					     VBVA_MIN_BUFFER_SIZE);
-	if (!vbox->vbva_buffers)
-		return -ENOMEM;
+	vbox->vbva_buffers = pcim_iomap_range(
+			pdev, 0, vbox->available_vram_size,
+			vbox->num_crtcs * VBVA_MIN_BUFFER_SIZE);
+	if (IS_ERR(vbox->vbva_buffers))
+		return PTR_ERR(vbox->vbva_buffers);
 
 	for (i = 0; i < vbox->num_crtcs; ++i) {
 		vbva_setup_buffer_context(&vbox->vbva_info[i],
@@ -116,11 +115,10 @@ int vbox_hw_init(struct vbox_private *vbox)
 	DRM_INFO("VRAM %08x\n", vbox->full_vram_size);
 
 	/* Map guest-heap at end of vram */
-	vbox->guest_heap =
-	    pci_iomap_range(pdev, 0, GUEST_HEAP_OFFSET(vbox),
-			    GUEST_HEAP_SIZE);
-	if (!vbox->guest_heap)
-		return -ENOMEM;
+	vbox->guest_heap = pcim_iomap_range(pdev, 0,
+			GUEST_HEAP_OFFSET(vbox), GUEST_HEAP_SIZE);
+	if (IS_ERR(vbox->guest_heap))
+		return PTR_ERR(vbox->guest_heap);
 
 	/* Create guest-heap mem-pool use 2^4 = 16 byte chunks */
 	vbox->guest_pool = devm_gen_pool_create(vbox->ddev.dev, 4, -1,
-- 
2.45.0

Re: [PATCH v9 10/13] PCI: Give pci_intx() its own devres callback

[Bjorn Helgaas]

On Thu, Jun 13, 2024 at 01:50:23PM +0200, Philipp Stanner wrote:
> pci_intx() is one of the functions that have "hybrid mode" (i.e.,
> sometimes managed, sometimes not). Providing a separate pcim_intx()
> function with its own device resource and cleanup callback allows for
> removing further large parts of the legacy PCI devres implementation.
> 
> As in the region-request-functions, pci_intx() has to call into its
> managed counterpart for backwards compatibility.
> 
> As pci_intx() is an outdated function, pcim_intx() shall not be made
> visible to drivers via a public API.

What makes pci_intx() outdated?  If it's outdated, we should mention
why and what the 30+ callers (including a couple in drivers/pci/)
should use instead.

Bjorn

Re: [PATCH v9 03/13] PCI: Add partial-BAR devres support

[Bjorn Helgaas]

On Thu, Jun 13, 2024 at 01:50:16PM +0200, Philipp Stanner wrote:
> With the current PCI devres API implementing a managed version of
> pci_iomap_range() is impossible.
> 
> Furthermore, the PCI devres API currently is inconsistent and
> complicated. This is in large part due to the fact that there are hybrid
> functions which are only sometimes managed via devres, and functions
> IO-mapping and requesting several BARs at once and returning mappings
> through a separately administrated table.
> 
> This table's indexing mechanism does not support partial-BAR mappings.
> 
> Another notable problem is that there are no separate managed
> counterparts for region-request functions such as pci_request_region(),
> as they exist for other PCI functions (e.g., pci_iomap() <->
> pcim_iomap()). Instead, functions based on __pci_request_region() change
> their internal behavior and suddenly become managed functions when
> pcim_enable_device() instead of pci_enable_device() is used.

The hybrid thing is certainly a problem, but does this patch address
it?  I don't see that it does (other than adding comments in
__pci_request_region() and pci_release_region()), but maybe I missed
it.

Correct me if I'm wrong, but I don't think this patch makes any
user-visible changes.

I'm proposing this:

  PCI: Add managed partial-BAR request and map infrastructure

  The pcim_iomap_devres table tracks entire-BAR mappings, so we can't use it
  to build a managed version of pci_iomap_range(), which maps partial BARs.

  Add struct pcim_addr_devres, which can track request and mapping of both
  entire BARs and partial BARs.

  Add the following internal devres functions based on struct
  pcim_addr_devres:

    pcim_iomap_region()               # request & map entire BAR
    pcim_iounmap_region()             # unmap & release entire BAR
    pcim_request_region()             # request entire BAR
    pcim_release_region()             # release entire BAR
    pcim_request_all_regions()        # request all entire BARs
    pcim_release_all_regions()        # release all entire BARs

  Rework the following public interfaces using the new infrastructure
  listed above:

    pcim_iomap()                      # map partial BAR
    pcim_iounmap()                    # unmap partial BAR
    pcim_iomap_regions()              # request & map specified BARs
    pcim_iomap_regions_request_all()  # request all BARs, map specified BARs
    pcim_iounmap_regions()            # unmap & release specified BARs


> This API is hard to understand and potentially bug-provoking. Hence, it
> should be made more consistent.
> 
> This patch adds the necessary infrastructure for partial-BAR mappings
> managed with devres. That infrastructure also serves as a ground layer
> for significantly simplifying the PCI devres API in subsequent patches
> which can then cleanly separate managed and unmanaged API.
> 
> When having the long term goal of providing always managed functions
> prefixed with "pcim_" and never managed functions prefixed with "pci_"
> and, thus, separating managed and unmanaged APIs cleanly, new PCI devres
> infrastructure cannot use __pci_request_region() and its wrappers since
> those would then again interact with PCI devres and, consequently,
> prevent the managed nature from being removed from the pci_* functions
> in the first place. Thus, it's necessary to provide an alternative to
> __pci_request_region().
> 
> This patch addresses the following problems of the PCI devres API:
> 
>   a) There is no PCI devres infrastructure on which a managed counter
>      part to pci_iomap_range() could be based on.
> 
>   b) The vast majority of the users of plural functions such as
>      pcim_iomap_regions() only ever sets a single bit in the bit mask,
>      consequently making them singular functions anyways.
> 
>   c) region-request functions being sometimes managed and sometimes not
>      is bug-provoking. pcim_* functions should always be managed, pci_*
>      functions never.
> 
> Add a new PCI device resource, pcim_addr_devres, that serves to
> encapsulate all device resource types related to region requests and
> IO-mappings since those are very frequently created together.
> 
> Add a set of alternatives cleanly separated from the hybrid mechanism in
> __pci_request_region() and its respective wrappers:
>   - __pcim_request_region_range()
>   - __pcim_release_region_range()
>   - __pcim_request_region()
>   - __pcim_release_region()
> 
> Add the following PCI-internal devres functions based on the above:
>   - pcim_iomap_region()
>   - pcim_iounmap_region()
>   - _pcim_request_region()
>   - pcim_request_region()
>   - pcim_release_region()
>   - pcim_request_all_regions()
>   - pcim_release_all_regions()
> 
> Add new needed helper pcim_remove_bar_from_legacy_table().
> 
> Rework the following public interfaces using the new infrastructure
> listed above:
>   - pcim_iomap_release()
>   - pcim_iomap()
>   - pcim_iounmap()
>   - pcim_iomap_regions()
>   - pcim_iomap_regions_request_all()
>   - pcim_iounmap_regions()
> 
> Update API documentation.
> 
> Link: https://lore.kernel.org/r/20240605081605.18769-5-pstanner@redhat.com
> Signed-off-by: Philipp Stanner <pstanner@redhat.com>
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
>  drivers/pci/devres.c | 608 ++++++++++++++++++++++++++++++++++++++-----
>  drivers/pci/pci.c    |  22 ++
>  drivers/pci/pci.h    |   5 +
>  3 files changed, 568 insertions(+), 67 deletions(-)
> 
> diff --git a/drivers/pci/devres.c b/drivers/pci/devres.c
> index 845d6fab0ce7..cf2c11b54ca6 100644
> --- a/drivers/pci/devres.c
> +++ b/drivers/pci/devres.c
> @@ -4,14 +4,243 @@
>  #include "pci.h"
>  
>  /*
> - * PCI iomap devres
> + * On the state of PCI's devres implementation:
> + *
> + * The older devres API for PCI has two significant problems:
> + *
> + * 1. It is very strongly tied to the statically allocated mapping table in
> + *    struct pcim_iomap_devres below. This is mostly solved in the sense of the
> + *    pcim_ functions in this file providing things like ranged mapping by
> + *    bypassing this table, wheras the functions that were present in the old
> + *    API still enter the mapping addresses into the table for users of the old
> + *    API.
> + *
> + * 2. The region-request-functions in pci.c do become managed IF the device has
> + *    been enabled with pcim_enable_device() instead of pci_enable_device().
> + *    This resulted in the API becoming inconsistent: Some functions have an
> + *    obviously managed counter-part (e.g., pci_iomap() <-> pcim_iomap()),
> + *    whereas some don't and are never managed, while others don't and are
> + *    _sometimes_ managed (e.g. pci_request_region()).
> + *
> + *    Consequently, in the new API, region requests performed by the pcim_
> + *    functions are automatically cleaned up through the devres callback
> + *    pcim_addr_resource_release(), while requests performed by
> + *    pcim_enable_device() + pci_*region*() are automatically cleaned up
> + *    through the for-loop in pcim_release().
> + *
> + * TODO 1:
> + * Remove the legacy table entirely once all calls to pcim_iomap_table() in
> + * the kernel have been removed.
> + *
> + * TODO 2:
> + * Port everyone calling pcim_enable_device() + pci_*region*() to using the
> + * pcim_ functions. Then, remove all devres functionality from pci_*region*()
> + * functions and remove the associated cleanups described above in point #2.
>   */
> -#define PCIM_IOMAP_MAX	PCI_STD_NUM_BARS
>  
> +/*
> + * Legacy struct storing addresses to whole mapped BARs.
> + */
>  struct pcim_iomap_devres {
> -	void __iomem *table[PCIM_IOMAP_MAX];
> +	void __iomem *table[PCI_STD_NUM_BARS];
> +};
> +
> +enum pcim_addr_devres_type {
> +	/* Default initializer. */
> +	PCIM_ADDR_DEVRES_TYPE_INVALID,
> +
> +	/* A requested region spanning an entire BAR. */
> +	PCIM_ADDR_DEVRES_TYPE_REGION,
> +
> +	/*
> +	 * A requested region spanning an entire BAR, and a mapping for
> +	 * the entire BAR.
> +	 */
> +	PCIM_ADDR_DEVRES_TYPE_REGION_MAPPING,
> +
> +	/*
> +	 * A mapping within a BAR, either spanning the whole BAR or just a
> +	 * range.  Without a requested region.
> +	 */
> +	PCIM_ADDR_DEVRES_TYPE_MAPPING,
>  };
>  
> +/*
> + * This struct envelops IO or MEM addresses, i.e., mappings and region
> + * requests, because those are very frequently requested and released
> + * together.
> + */
> +struct pcim_addr_devres {
> +	enum pcim_addr_devres_type type;
> +	void __iomem *baseaddr;
> +	unsigned long offset;
> +	unsigned long len;
> +	short bar;
> +};
> +
> +static inline void pcim_addr_devres_clear(struct pcim_addr_devres *res)
> +{
> +	memset(res, 0, sizeof(*res));
> +	res->bar = -1;
> +}
> +
> +/*
> + * The following functions, __pcim_*_region*, exist as counterparts to the
> + * versions from pci.c - which, unfortunately, can be in "hybrid mode", i.e.,
> + * sometimes managed, sometimes not.
> + *
> + * To separate the APIs cleanly, we define our own, simplified versions here.
> + */
> +
> +/**
> + * __pcim_request_region_range - Request a ranged region
> + * @pdev: PCI device the region belongs to
> + * @bar: BAR the range is within
> + * @offset: offset from the BAR's start address
> + * @maxlen: length in bytes, beginning at @offset
> + * @name: name associated with the request
> + * @req_flags: flags for the request, e.g., for kernel-exclusive requests
> + *
> + * Returns: 0 on success, a negative error code on failure.
> + *
> + * Request a range within a device's PCI BAR.  Sanity check the input.
> + */
> +static int __pcim_request_region_range(struct pci_dev *pdev, int bar,
> +		unsigned long offset, unsigned long maxlen,
> +		const char *name, int req_flags)
> +{
> +	resource_size_t start = pci_resource_start(pdev, bar);
> +	resource_size_t len = pci_resource_len(pdev, bar);
> +	unsigned long dev_flags = pci_resource_flags(pdev, bar);
> +
> +	if (start == 0 || len == 0) /* Unused BAR. */
> +		return 0;
> +	if (len <= offset)
> +		return  -EINVAL;
> +
> +	start += offset;
> +	len -= offset;
> +
> +	if (len > maxlen && maxlen != 0)
> +		len = maxlen;
> +
> +	if (dev_flags & IORESOURCE_IO) {
> +		if (!request_region(start, len, name))
> +			return -EBUSY;
> +	} else if (dev_flags & IORESOURCE_MEM) {
> +		if (!__request_mem_region(start, len, name, req_flags))
> +			return -EBUSY;
> +	} else {
> +		/* That's not a device we can request anything on. */
> +		return -ENODEV;
> +	}
> +
> +	return 0;
> +}
> +
> +static void __pcim_release_region_range(struct pci_dev *pdev, int bar,
> +		unsigned long offset, unsigned long maxlen)
> +{
> +	resource_size_t start = pci_resource_start(pdev, bar);
> +	resource_size_t len = pci_resource_len(pdev, bar);
> +	unsigned long flags = pci_resource_flags(pdev, bar);
> +
> +	if (len <= offset || start == 0)
> +		return;
> +
> +	if (len == 0 || maxlen == 0) /* This an unused BAR. Do nothing. */
> +		return;
> +
> +	start += offset;
> +	len -= offset;
> +
> +	if (len > maxlen)
> +		len = maxlen;
> +
> +	if (flags & IORESOURCE_IO)
> +		release_region(start, len);
> +	else if (flags & IORESOURCE_MEM)
> +		release_mem_region(start, len);
> +}
> +
> +static int __pcim_request_region(struct pci_dev *pdev, int bar,
> +		const char *name, int flags)
> +{
> +	unsigned long offset = 0;
> +	unsigned long len = pci_resource_len(pdev, bar);
> +
> +	return __pcim_request_region_range(pdev, bar, offset, len, name, flags);
> +}
> +
> +static void __pcim_release_region(struct pci_dev *pdev, int bar)
> +{
> +	unsigned long offset = 0;
> +	unsigned long len = pci_resource_len(pdev, bar);
> +
> +	__pcim_release_region_range(pdev, bar, offset, len);
> +}
> +
> +static void pcim_addr_resource_release(struct device *dev, void *resource_raw)
> +{
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +	struct pcim_addr_devres *res = resource_raw;
> +
> +	switch (res->type) {
> +	case PCIM_ADDR_DEVRES_TYPE_REGION:
> +		__pcim_release_region(pdev, res->bar);
> +		break;
> +	case PCIM_ADDR_DEVRES_TYPE_REGION_MAPPING:
> +		pci_iounmap(pdev, res->baseaddr);
> +		__pcim_release_region(pdev, res->bar);
> +		break;
> +	case PCIM_ADDR_DEVRES_TYPE_MAPPING:
> +		pci_iounmap(pdev, res->baseaddr);
> +		break;
> +	default:
> +		break;
> +	}
> +}
> +
> +static struct pcim_addr_devres *pcim_addr_devres_alloc(struct pci_dev *pdev)
> +{
> +	struct pcim_addr_devres *res;
> +
> +	res = devres_alloc_node(pcim_addr_resource_release, sizeof(*res),
> +			GFP_KERNEL, dev_to_node(&pdev->dev));
> +	if (res)
> +		pcim_addr_devres_clear(res);
> +	return res;
> +}
> +
> +/* Just for consistency and readability. */
> +static inline void pcim_addr_devres_free(struct pcim_addr_devres *res)
> +{
> +	devres_free(res);
> +}
> +
> +/*
> + * Used by devres to identify a pcim_addr_devres.
> + */
> +static int pcim_addr_resources_match(struct device *dev, void *a_raw, void *b_raw)
> +{
> +	struct pcim_addr_devres *a, *b;
> +
> +	a = a_raw;
> +	b = b_raw;
> +
> +	if (a->type != b->type)
> +		return 0;
> +
> +	switch (a->type) {
> +	case PCIM_ADDR_DEVRES_TYPE_REGION:
> +	case PCIM_ADDR_DEVRES_TYPE_REGION_MAPPING:
> +		return a->bar == b->bar;
> +	case PCIM_ADDR_DEVRES_TYPE_MAPPING:
> +		return a->baseaddr == b->baseaddr;
> +	default:
> +		return 0;
> +	}
> +}
>  
>  static void devm_pci_unmap_iospace(struct device *dev, void *ptr)
>  {
> @@ -92,8 +321,8 @@ EXPORT_SYMBOL(devm_pci_remap_cfgspace);
>   *
>   * 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::
> + * Returns a pointer to the remapped memory or an IOMEM_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);
> @@ -172,6 +401,17 @@ static void pcim_release(struct device *gendev, void *res)
>  	struct pci_devres *this = res;
>  	int i;
>  
> +	/*
> +	 * This is legacy code.
> +	 *
> +	 * All regions requested by a pcim_ function do get released through
> +	 * pcim_addr_resource_release(). Thanks to the hybrid nature of the pci_
> +	 * region-request functions, this for-loop has to release the regions
> +	 * if they have been requested by such a function.
> +	 *
> +	 * TODO: Remove this once all users of pcim_enable_device() PLUS
> +	 * pci-region-request-functions have been ported to pcim_ functions.
> +	 */
>  	for (i = 0; i < DEVICE_COUNT_RESOURCE; i++)
>  		if (mask_contains_bar(this->region_mask, i))
>  			pci_release_region(dev, i);
> @@ -258,19 +498,21 @@ EXPORT_SYMBOL(pcim_pin_device);
>  
>  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]);
> +	/*
> +	 * Do nothing. This is legacy code.
> +	 *
> +	 * Cleanup of the mappings is now done directly through the callbacks
> +	 * registered when creating them.
> +	 */
>  }
>  
>  /**
>   * pcim_iomap_table - access iomap allocation table
>   * @pdev: PCI device to access iomap table for
>   *
> + * Returns:
> + * Const pointer to array of __iomem pointers on success, NULL on failure.
> + *
>   * 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
> @@ -343,30 +585,67 @@ static void pcim_remove_mapping_from_legacy_table(struct pci_dev *pdev,
>  	}
>  }
>  
> +/*
> + * The same as pcim_remove_mapping_from_legacy_table(), but identifies the
> + * mapping by its BAR index.
> + */
> +static void pcim_remove_bar_from_legacy_table(struct pci_dev *pdev, short bar)
> +{
> +	void __iomem **legacy_iomap_table;
> +
> +	if (bar >= PCI_STD_NUM_BARS)
> +		return;
> +
> +	legacy_iomap_table = (void __iomem **)pcim_iomap_table(pdev);
> +	if (!legacy_iomap_table)
> +		return;
> +
> +	legacy_iomap_table[bar] = NULL;
> +}
> +
>  /**
>   * 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.
> + * Returns: __iomem pointer on success, NULL on failure.
> + *
> + * Managed pci_iomap(). Map is automatically unmapped on driver detach. If
> + * desired, unmap manually only with pcim_iounmap().
> + *
> + * This SHOULD only be used once per BAR.
> + *
> + * NOTE:
> + * Contrary to the other pcim_* functions, this function does not return an
> + * IOMEM_ERR_PTR() on failure, but a simple NULL. This is done for backwards
> + * compatibility.
>   */
>  void __iomem *pcim_iomap(struct pci_dev *pdev, int bar, unsigned long maxlen)
>  {
>  	void __iomem *mapping;
> +	struct pcim_addr_devres *res;
> +
> +	res = pcim_addr_devres_alloc(pdev);
> +	if (!res)
> +		return NULL;
> +	res->type = PCIM_ADDR_DEVRES_TYPE_MAPPING;
>  
>  	mapping = pci_iomap(pdev, bar, maxlen);
>  	if (!mapping)
> -		return NULL;
> +		goto err_iomap;
> +	res->baseaddr = mapping;
>  
>  	if (pcim_add_mapping_to_legacy_table(pdev, mapping, bar) != 0)
>  		goto err_table;
>  
> +	devres_add(&pdev->dev, res);
>  	return mapping;
>  
>  err_table:
>  	pci_iounmap(pdev, mapping);
> +err_iomap:
> +	pcim_addr_devres_free(res);
>  	return NULL;
>  }
>  EXPORT_SYMBOL(pcim_iomap);
> @@ -376,91 +655,291 @@ EXPORT_SYMBOL(pcim_iomap);
>   * @pdev: PCI device to iounmap for
>   * @addr: Address to unmap
>   *
> - * Managed pci_iounmap().  @addr must have been mapped using pcim_iomap().
> + * Managed pci_iounmap(). @addr must have been mapped using a pcim_* mapping
> + * function.
>   */
>  void pcim_iounmap(struct pci_dev *pdev, void __iomem *addr)
>  {
> -	pci_iounmap(pdev, addr);
> +	struct pcim_addr_devres res_searched;
> +
> +	pcim_addr_devres_clear(&res_searched);
> +	res_searched.type = PCIM_ADDR_DEVRES_TYPE_MAPPING;
> +	res_searched.baseaddr = addr;
> +
> +	if (devres_release(&pdev->dev, pcim_addr_resource_release,
> +			pcim_addr_resources_match, &res_searched) != 0) {
> +		/* Doesn't exist. User passed nonsense. */
> +		return;
> +	}
>  
>  	pcim_remove_mapping_from_legacy_table(pdev, addr);
>  }
>  EXPORT_SYMBOL(pcim_iounmap);
>  
> +/**
> + * pcim_iomap_region - Request and iomap a PCI BAR
> + * @pdev: PCI device to map IO resources for
> + * @bar: Index of a BAR to map
> + * @name: Name associated with the request
> + *
> + * Returns: __iomem pointer on success, an IOMEM_ERR_PTR on failure.
> + *
> + * Mapping and region will get automatically released on driver detach. If
> + * desired, release manually only with pcim_iounmap_region().
> + */
> +static void __iomem *pcim_iomap_region(struct pci_dev *pdev, int bar,
> +				       const char *name)
> +{
> +	int ret;
> +	struct pcim_addr_devres *res;
> +
> +	res = pcim_addr_devres_alloc(pdev);
> +	if (!res)
> +		return IOMEM_ERR_PTR(-ENOMEM);
> +
> +	res->type = PCIM_ADDR_DEVRES_TYPE_REGION_MAPPING;
> +	res->bar = bar;
> +
> +	ret = __pcim_request_region(pdev, bar, name, 0);
> +	if (ret != 0)
> +		goto err_region;
> +
> +	res->baseaddr = pci_iomap(pdev, bar, 0);
> +	if (!res->baseaddr) {
> +		ret = -EINVAL;
> +		goto err_iomap;
> +	}
> +
> +	devres_add(&pdev->dev, res);
> +	return res->baseaddr;
> +
> +err_iomap:
> +	__pcim_release_region(pdev, bar);
> +err_region:
> +	pcim_addr_devres_free(res);
> +
> +	return IOMEM_ERR_PTR(ret);
> +}
> +
> +/**
> + * pcim_iounmap_region - Unmap and release a PCI BAR
> + * @pdev: PCI device to operate on
> + * @bar: Index of BAR to unmap and release
> + *
> + * Unmap a BAR and release its region manually. Only pass BARs that were
> + * previously mapped by pcim_iomap_region().
> + */
> +static void pcim_iounmap_region(struct pci_dev *pdev, int bar)
> +{
> +	struct pcim_addr_devres res_searched;
> +
> +	pcim_addr_devres_clear(&res_searched);
> +	res_searched.type = PCIM_ADDR_DEVRES_TYPE_REGION_MAPPING;
> +	res_searched.bar = bar;
> +
> +	devres_release(&pdev->dev, pcim_addr_resource_release,
> +			pcim_addr_resources_match, &res_searched);
> +}
> +
>  /**
>   * 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
> + * @name: Name associated with the requests
> + *
> + * Returns: 0 on success, negative error code on failure.
>   *
>   * 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;
> +	int ret;
> +	short bar;
> +	void __iomem *mapping;
>  
> -	iomap = pcim_iomap_table(pdev);
> -	if (!iomap)
> -		return -ENOMEM;
> +	for (bar = 0; bar < DEVICE_COUNT_RESOURCE; bar++) {
> +		if (!mask_contains_bar(mask, bar))
> +			continue;
>  
> -	for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
> -		unsigned long len;
> +		mapping = pcim_iomap_region(pdev, bar, name);
> +		if (IS_ERR(mapping)) {
> +			ret = PTR_ERR(mapping);
> +			goto err;
> +		}
> +		ret = pcim_add_mapping_to_legacy_table(pdev, mapping, bar);
> +		if (ret != 0)
> +			goto err;
> +	}
>  
> -		if (!mask_contains_bar(mask, i))
> -			continue;
> +	return 0;
>  
> -		rc = -EINVAL;
> -		len = pci_resource_len(pdev, i);
> -		if (!len)
> -			goto err_inval;
> +err:
> +	while (--bar >= 0) {
> +		pcim_iounmap_region(pdev, bar);
> +		pcim_remove_bar_from_legacy_table(pdev, bar);
> +	}
>  
> -		rc = pci_request_region(pdev, i, name);
> -		if (rc)
> -			goto err_inval;
> +	return ret;
> +}
> +EXPORT_SYMBOL(pcim_iomap_regions);
>  
> -		rc = -ENOMEM;
> -		if (!pcim_iomap(pdev, i, 0))
> -			goto err_region;
> +static int _pcim_request_region(struct pci_dev *pdev, int bar, const char *name,
> +		int request_flags)
> +{
> +	int ret;
> +	struct pcim_addr_devres *res;
> +
> +	res = pcim_addr_devres_alloc(pdev);
> +	if (!res)
> +		return -ENOMEM;
> +	res->type = PCIM_ADDR_DEVRES_TYPE_REGION;
> +	res->bar = bar;
> +
> +	ret = __pcim_request_region(pdev, bar, name, request_flags);
> +	if (ret != 0) {
> +		pcim_addr_devres_free(res);
> +		return ret;
>  	}
>  
> +	devres_add(&pdev->dev, res);
>  	return 0;
> +}
>  
> - err_region:
> -	pci_release_region(pdev, i);
> - err_inval:
> -	while (--i >= 0) {
> -		if (!mask_contains_bar(mask, i))
> -			continue;
> -		pcim_iounmap(pdev, iomap[i]);
> -		pci_release_region(pdev, i);
> +/**
> + * pcim_request_region - Request a PCI BAR
> + * @pdev: PCI device to requestion region for
> + * @bar: Index of BAR to request
> + * @name: Name associated with the request
> + *
> + * Returns: 0 on success, a negative error code on failure.
> + *
> + * Request region specified by @bar.
> + *
> + * The region will automatically be released on driver detach. If desired,
> + * release manually only with pcim_release_region().
> + */
> +static int pcim_request_region(struct pci_dev *pdev, int bar, const char *name)
> +{
> +	return _pcim_request_region(pdev, bar, name, 0);
> +}
> +
> +/**
> + * pcim_release_region - Release a PCI BAR
> + * @pdev: PCI device to operate on
> + * @bar: Index of BAR to release
> + *
> + * Release a region manually that was previously requested by
> + * pcim_request_region().
> + */
> +static void pcim_release_region(struct pci_dev *pdev, int bar)
> +{
> +	struct pcim_addr_devres res_searched;
> +
> +	pcim_addr_devres_clear(&res_searched);
> +	res_searched.type = PCIM_ADDR_DEVRES_TYPE_REGION;
> +	res_searched.bar = bar;
> +
> +	devres_release(&pdev->dev, pcim_addr_resource_release,
> +			pcim_addr_resources_match, &res_searched);
> +}
> +
> +
> +/**
> + * pcim_release_all_regions - Release all regions of a PCI-device
> + * @pdev: the PCI device
> + *
> + * Release all regions previously requested through pcim_request_region()
> + * or pcim_request_all_regions().
> + *
> + * Can be called from any context, i.e., not necessarily as a counterpart to
> + * pcim_request_all_regions().
> + */
> +static void pcim_release_all_regions(struct pci_dev *pdev)
> +{
> +	short bar;
> +
> +	for (bar = 0; bar < PCI_STD_NUM_BARS; bar++)
> +		pcim_release_region(pdev, bar);
> +}
> +
> +/**
> + * pcim_request_all_regions - Request all regions
> + * @pdev: PCI device to map IO resources for
> + * @name: name associated with the request
> + *
> + * Returns: 0 on success, negative error code on failure.
> + *
> + * Requested regions will automatically be released at driver detach. If
> + * desired, release individual regions with pcim_release_region() or all of
> + * them at once with pcim_release_all_regions().
> + */
> +static int pcim_request_all_regions(struct pci_dev *pdev, const char *name)
> +{
> +	int ret;
> +	short bar;
> +
> +	for (bar = 0; bar < PCI_STD_NUM_BARS; bar++) {
> +		ret = pcim_request_region(pdev, bar, name);
> +		if (ret != 0)
> +			goto err;
>  	}
>  
> -	return rc;
> +	return 0;
> +
> +err:
> +	pcim_release_all_regions(pdev);
> +
> +	return ret;
>  }
> -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
> + * @name: Name associated with the requests
> + *
> + * Returns: 0 on success, negative error code on failure.
>   *
>   * Request all PCI BARs and iomap regions specified by @mask.
> + *
> + * To release these resources manually, call pcim_release_region() for the
> + * regions and pcim_iounmap() for the mappings.
>   */
>  int pcim_iomap_regions_request_all(struct pci_dev *pdev, int mask,
>  				   const char *name)
>  {
> -	int request_mask = ((1 << 6) - 1) & ~mask;
> -	int rc;
> +	short bar;
> +	int ret;
> +	void __iomem **legacy_iomap_table;
>  
> -	rc = pci_request_selected_regions(pdev, request_mask, name);
> -	if (rc)
> -		return rc;
> +	ret = pcim_request_all_regions(pdev, name);
> +	if (ret != 0)
> +		return ret;
>  
> -	rc = pcim_iomap_regions(pdev, mask, name);
> -	if (rc)
> -		pci_release_selected_regions(pdev, request_mask);
> -	return rc;
> +	for (bar = 0; bar < PCI_STD_NUM_BARS; bar++) {
> +		if (!mask_contains_bar(mask, bar))
> +			continue;
> +		if (!pcim_iomap(pdev, bar, 0))
> +			goto err;
> +	}
> +
> +	return 0;
> +
> +err:
> +	/*
> +	 * If bar is larger than 0, then pcim_iomap() above has most likely
> +	 * failed because of -EINVAL. If it is equal 0, most likely the table
> +	 * couldn't be created, indicating -ENOMEM.
> +	 */
> +	ret = bar > 0 ? -EINVAL : -ENOMEM;
> +	legacy_iomap_table = (void __iomem **)pcim_iomap_table(pdev);
> +
> +	while (--bar >= 0)
> +		pcim_iounmap(pdev, legacy_iomap_table[bar]);
> +
> +	pcim_release_all_regions(pdev);
> +
> +	return ret;
>  }
>  EXPORT_SYMBOL(pcim_iomap_regions_request_all);
>  
> @@ -473,19 +952,14 @@ EXPORT_SYMBOL(pcim_iomap_regions_request_all);
>   */
>  void pcim_iounmap_regions(struct pci_dev *pdev, int mask)
>  {
> -	void __iomem * const *iomap;
> -	int i;
> +	short bar;
>  
> -	iomap = pcim_iomap_table(pdev);
> -	if (!iomap)
> -		return;
> -
> -	for (i = 0; i < PCIM_IOMAP_MAX; i++) {
> -		if (!mask_contains_bar(mask, i))
> +	for (bar = 0; bar < PCI_STD_NUM_BARS; bar++) {
> +		if (!mask_contains_bar(mask, bar))
>  			continue;
>  
> -		pcim_iounmap(pdev, iomap[i]);
> -		pci_release_region(pdev, i);
> +		pcim_iounmap_region(pdev, bar);
> +		pcim_remove_bar_from_legacy_table(pdev, bar);
>  	}
>  }
>  EXPORT_SYMBOL(pcim_iounmap_regions);
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 59e0949fb079..d94445f5f882 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -3883,6 +3883,17 @@ void pci_release_region(struct pci_dev *pdev, int bar)
>  		release_mem_region(pci_resource_start(pdev, bar),
>  				pci_resource_len(pdev, bar));
>  
> +	/*
> +	 * This devres utility makes this function sometimes managed
> +	 * (when pcim_enable_device() has been called before).
> +	 *
> +	 * This is bad because it conflicts with the pcim_ functions being
> +	 * exclusively responsible for managed PCI. Its "sometimes yes,
> +	 * sometimes no" nature can cause bugs.
> +	 *
> +	 * TODO: Remove this once all users that use pcim_enable_device() PLUS
> +	 * a region request function have been ported to using pcim_ functions.
> +	 */
>  	dr = find_pci_dr(pdev);
>  	if (dr)
>  		dr->region_mask &= ~(1 << bar);
> @@ -3927,6 +3938,17 @@ static int __pci_request_region(struct pci_dev *pdev, int bar,
>  			goto err_out;
>  	}
>  
> +	/*
> +	 * This devres utility makes this function sometimes managed
> +	 * (when pcim_enable_device() has been called before).
> +	 *
> +	 * This is bad because it conflicts with the pcim_ functions being
> +	 * exclusively responsible for managed pci. Its "sometimes yes,
> +	 * sometimes no" nature can cause bugs.
> +	 *
> +	 * TODO: Remove this once all users that use pcim_enable_device() PLUS
> +	 * a region request function have been ported to using pcim_ functions.
> +	 */
>  	dr = find_pci_dr(pdev);
>  	if (dr)
>  		dr->region_mask |= 1 << bar;
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index fd44565c4756..c09487f5550c 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -826,6 +826,11 @@ struct pci_devres {
>  	unsigned int orig_intx:1;
>  	unsigned int restore_intx:1;
>  	unsigned int mwi:1;
> +
> +	/*
> +	 * TODO: remove the region_mask once everyone calling
> +	 * pcim_enable_device() + pci_*region*() is ported to pcim_ functions.
> +	 */
>  	u32 region_mask;
>  };
>  
> -- 
> 2.45.0
> 

Re: [PATCH v9 00/13] Make PCI's devres API more consistent

[Bjorn Helgaas]

On Thu, Jun 13, 2024 at 01:50:13PM +0200, Philipp Stanner wrote:
> Changes in v9:
>   - Remove forgotten dead code ('enabled' bit in struct pci_dev) in
>     patch No.8 ("Move pinned status bit...")
>   - Rework patch No.3:
>       - Change title from "Reimplement plural devres functions"
>         to "Add partial-BAR devres support".
>       - Drop excessive details about the general cleanup from the commit
> 	message. Only motivate why this patch's new infrastructure is
> 	necessary.
>   - Fix some minor spelling issues (s/pci/PCI ...)
> 
> Changes in v8:
>   - Rebase the series on the already merged patches which were slightly
>     modified by Bjorn Helgaas.
>   - Reword the pci_intx() commit message so it clearly states it's about
>     reworking pci_intx().
>   - Move the removal of find_pci_dr() from patch "Remove legacy
>     pcim_release()" to patch "Give pci_intx() its own devres callback"
>     since this later patch already removed all calls to that function.
>   - In patch "Give pci_intx() its own devres callback": use
>     pci_is_enabled() (and, thus, the enabled_cnt in struct pci_dev)
>     instead of a separate enabled field. (Bjorn)
> 
> Changes in v7:
>   - Split the entire series in smaller, more atomic chunks / patches
>     (Bjorn)
>   - Remove functions (such as pcim_iomap_region_range()) that do not yet
>     have a user (Bjorn)
>   - Don't export interfaces publicly anymore, except for
>     pcim_iomap_range(), needed by vboxvideo (Bjorn)
>   - Mention the actual (vboxvideo) bug in "PCI: Warn users..." commit
>     (Bjorn)
>   - Drop docstring warnings on PCI-internal functions (Bjorn)
>   - Rework docstring warnings
>   - Fix spelling in a few places. Rewrapp paragraphs (Bjorn)
> 
> Changes in v6:
>   - Restructure the cleanup in pcim_iomap_regions_request_all() so that
>     it doesn't trigger a (false positive) test robot warning. No
>     behavior change intended. (Dan Carpenter)
> 
> Changes in v5:
>   - Add Hans's Reviewed-by to vboxvideo patch (Hans de Goede)
>   - Remove stable-kernel from CC in vboxvideo patch (Hans de Goede)
> 
> Changes in v4:
>   - Rebase against linux-next
> 
> Changes in v3:
>   - Use the term "PCI devres API" at some forgotten places.
>   - Fix more grammar errors in patch #3.
>   - Remove the comment advising to call (the outdated) pcim_intx() in pci.c
>   - Rename __pcim_request_region_range() flags-field "exclusive" to
>     "req_flags", since this is what the int actually represents.
>   - Remove the call to pcim_region_request() from patch #10. (Hans)
> 
> Changes in v2:
>   - Make commit head lines congruent with PCI's style (Bjorn)
>   - Add missing error checks for devm_add_action(). (Andy)
>   - Repair the "Returns: " marks for docu generation (Andy)
>   - Initialize the addr_devres struct with memset(). (Andy)
>   - Make pcim_intx() a PCI-internal function so that new drivers won't
>     be encouraged to use the outdated pci_intx() mechanism.
>     (Andy / Philipp)
>   - Fix grammar and spelling (Bjorn)
>   - Be more precise on why pcim_iomap_table() is problematic (Bjorn)
>   - Provide the actual structs' and functions' names in the commit
>     messages (Bjorn)
>   - Remove redundant variable initializers (Andy)
>   - Regroup PM bitfield members in struct pci_dev (Andy)
>   - Make pcim_intx() visible only for the PCI subsystem so that new    
>     drivers won't use this outdated API (Andy, Myself)
>   - Add a NOTE to pcim_iomap() to warn about this function being the one
>     exception that does just return NULL.
>   - Consistently use the term "PCI devres API"; also in Patch #10 (Bjorn)
> 
> 
> ¡Hola!
> 
> PCI's devres API suffers several weaknesses:
> 
> 1. There are functions prefixed with pcim_. Those are always managed
>    counterparts to never-managed functions prefixed with pci_ – or so one
>    would like to think. There are some apparently unmanaged functions
>    (all region-request / release functions, and pci_intx()) which
>    suddenly become managed once the user has initialized the device with
>    pcim_enable_device() instead of pci_enable_device(). This "sometimes
>    yes, sometimes no" nature of those functions is confusing and
>    therefore bug-provoking. In fact, it has already caused a bug in DRM.
>    The last patch in this series fixes that bug.
> 2. iomappings: Instead of giving each mapping its own callback, the
>    existing API uses a statically allocated struct tracking one mapping
>    per bar. This is not extensible. Especially, you can't create
>    _ranged_ managed mappings that way, which many drivers want.
> 3. Managed request functions only exist as "plural versions" with a
>    bit-mask as a parameter. That's quite over-engineered considering
>    that each user only ever mapps one, maybe two bars.
> 
> This series:
> - add a set of new "singular" devres functions that use devres the way
>   its intended, with one callback per resource.
> - deprecates the existing iomap-table mechanism.
> - deprecates the hybrid nature of pci_ functions.
> - preserves backwards compatibility so that drivers using the existing
>   API won't notice any changes.
> - adds documentation, especially some warning users about the
>   complicated nature of PCI's devres.
> 
> 
> Note that this series is based on my "unify pci_iounmap"-series from a
> few weeks ago. [1]
> 
> I tested this on a x86 VM with a simple pci test-device with two
> regions. Operates and reserves resources as intended on my system.
> Kasan and kmemleak didn't find any problems.
> 
> I believe this series cleans the API up as much as possible without
> having to port all existing drivers to the new API. Especially, I think
> that this implementation is easy to extend if the need for new managed
> functions arises :)
> 
> Greetings,
> P.
> 
> Philipp Stanner (13):
>   PCI: Add and use devres helper for bit masks
>   PCI: Add devres helpers for iomap table
>   PCI: Add partial-BAR devres support
>   PCI: Deprecate two surplus devres functions
>   PCI: Make devres region requests consistent
>   PCI: Warn users about complicated devres nature
>   PCI: Remove enabled status bit from pci_devres
>   PCI: Move pinned status bit to struct pci_dev
>   PCI: Give pcim_set_mwi() its own devres callback
>   PCI: Give pci_intx() its own devres callback
>   PCI: Remove legacy pcim_release()
>   PCI: Add pcim_iomap_range()
>   drm/vboxvideo: fix mapping leaks
> 
>  drivers/gpu/drm/vboxvideo/vbox_main.c |  20 +-
>  drivers/pci/devres.c                  | 903 +++++++++++++++++++++-----
>  drivers/pci/iomap.c                   |  16 +
>  drivers/pci/pci.c                     |  94 ++-
>  drivers/pci/pci.h                     |  23 +-
>  include/linux/pci.h                   |   5 +-
>  6 files changed, 858 insertions(+), 203 deletions(-)

This is on pci/devres with some commit log rework and the following
diffs.  I think the bar short/int thing is the only actual code
change.  Happy to squash in any other updates or things I botched.

Planned for v6.11.

diff --git a/drivers/pci/devres.c b/drivers/pci/devres.c
index 2f0379a4e58f..d9b78a0d903a 100644
--- a/drivers/pci/devres.c
+++ b/drivers/pci/devres.c
@@ -11,7 +11,7 @@
  * 1. It is very strongly tied to the statically allocated mapping table in
  *    struct pcim_iomap_devres below. This is mostly solved in the sense of the
  *    pcim_ functions in this file providing things like ranged mapping by
- *    bypassing this table, wheras the functions that were present in the old
+ *    bypassing this table, whereas the functions that were present in the old
  *    API still enter the mapping addresses into the table for users of the old
  *    API.
  *
@@ -25,10 +25,11 @@
  *    Consequently, in the new API, region requests performed by the pcim_
  *    functions are automatically cleaned up through the devres callback
  *    pcim_addr_resource_release().
- *    Users utilizing pcim_enable_device() + pci_*region*() are redirected in
+ *
+ *    Users of pcim_enable_device() + pci_*region*() are redirected in
  *    pci.c to the managed functions here in this file. This isn't exactly
- *    perfect, but the only alternative way would be to port ALL drivers using
- *    said combination to pcim_ functions.
+ *    perfect, but the only alternative way would be to port ALL drivers
+ *    using said combination to pcim_ functions.
  *
  * TODO:
  * Remove the legacy table entirely once all calls to pcim_iomap_table() in
@@ -42,7 +43,7 @@ struct pcim_iomap_devres {
 	void __iomem *table[PCI_STD_NUM_BARS];
 };
 
-/* Used to restore the old intx state on driver detach. */
+/* Used to restore the old INTx state on driver detach. */
 struct pcim_intx_devres {
 	int orig_intx;
 };
@@ -77,7 +78,7 @@ struct pcim_addr_devres {
 	void __iomem *baseaddr;
 	unsigned long offset;
 	unsigned long len;
-	short bar;
+	int bar;
 };
 
 static inline void pcim_addr_devres_clear(struct pcim_addr_devres *res)
@@ -108,8 +109,9 @@ static inline void pcim_addr_devres_clear(struct pcim_addr_devres *res)
  * Request a range within a device's PCI BAR.  Sanity check the input.
  */
 static int __pcim_request_region_range(struct pci_dev *pdev, int bar,
-		unsigned long offset, unsigned long maxlen,
-		const char *name, int req_flags)
+				       unsigned long offset,
+				       unsigned long maxlen,
+				       const char *name, int req_flags)
 {
 	resource_size_t start = pci_resource_start(pdev, bar);
 	resource_size_t len = pci_resource_len(pdev, bar);
@@ -118,7 +120,7 @@ static int __pcim_request_region_range(struct pci_dev *pdev, int bar,
 	if (start == 0 || len == 0) /* Unused BAR. */
 		return 0;
 	if (len <= offset)
-		return  -EINVAL;
+		return -EINVAL;
 
 	start += offset;
 	len -= offset;
@@ -141,7 +143,8 @@ static int __pcim_request_region_range(struct pci_dev *pdev, int bar,
 }
 
 static void __pcim_release_region_range(struct pci_dev *pdev, int bar,
-		unsigned long offset, unsigned long maxlen)
+					unsigned long offset,
+					unsigned long maxlen)
 {
 	resource_size_t start = pci_resource_start(pdev, bar);
 	resource_size_t len = pci_resource_len(pdev, bar);
@@ -166,7 +169,7 @@ static void __pcim_release_region_range(struct pci_dev *pdev, int bar,
 }
 
 static int __pcim_request_region(struct pci_dev *pdev, int bar,
-		const char *name, int flags)
+				 const char *name, int flags)
 {
 	unsigned long offset = 0;
 	unsigned long len = pci_resource_len(pdev, bar);
@@ -208,7 +211,7 @@ static struct pcim_addr_devres *pcim_addr_devres_alloc(struct pci_dev *pdev)
 	struct pcim_addr_devres *res;
 
 	res = devres_alloc_node(pcim_addr_resource_release, sizeof(*res),
-			GFP_KERNEL, dev_to_node(&pdev->dev));
+				GFP_KERNEL, dev_to_node(&pdev->dev));
 	if (res)
 		pcim_addr_devres_clear(res);
 	return res;
@@ -223,7 +226,8 @@ static inline void pcim_addr_devres_free(struct pcim_addr_devres *res)
 /*
  * Used by devres to identify a pcim_addr_devres.
  */
-static int pcim_addr_resources_match(struct device *dev, void *a_raw, void *b_raw)
+static int pcim_addr_resources_match(struct device *dev,
+				     void *a_raw, void *b_raw)
 {
 	struct pcim_addr_devres *a, *b;
 
@@ -402,7 +406,6 @@ int pcim_set_mwi(struct pci_dev *pdev)
 }
 EXPORT_SYMBOL(pcim_set_mwi);
 
-
 static inline bool mask_contains_bar(int mask, int bar)
 {
 	return mask & BIT(bar);
@@ -438,8 +441,8 @@ static struct pcim_intx_devres *get_or_create_intx_devres(struct device *dev)
  *
  * Returns: 0 on success, -ENOMEM on error.
  *
- * Enables/disables PCI INTx for device @pdev.
- * Restores the original state on driver detach.
+ * Enable/disable PCI INTx for device @pdev.
+ * Restore the original state on driver detach.
  */
 int pcim_intx(struct pci_dev *pdev, int enable)
 {
@@ -492,7 +495,7 @@ int pcim_enable_device(struct pci_dev *pdev)
 
 	/*
 	 * We prefer removing the action in case of an error over
-	 * devm_add_action_or_reset() because the later could theoretically be
+	 * devm_add_action_or_reset() because the latter could theoretically be
 	 * disturbed by users having pinned the device too soon.
 	 */
 	ret = pci_enable_device(pdev);
@@ -618,7 +621,7 @@ static void pcim_remove_mapping_from_legacy_table(struct pci_dev *pdev,
  * The same as pcim_remove_mapping_from_legacy_table(), but identifies the
  * mapping by its BAR index.
  */
-static void pcim_remove_bar_from_legacy_table(struct pci_dev *pdev, short bar)
+static void pcim_remove_bar_from_legacy_table(struct pci_dev *pdev, int bar)
 {
 	void __iomem **legacy_iomap_table;
 
@@ -783,7 +786,7 @@ static void pcim_iounmap_region(struct pci_dev *pdev, int bar)
 int pcim_iomap_regions(struct pci_dev *pdev, int mask, const char *name)
 {
 	int ret;
-	short bar;
+	int bar;
 	void __iomem *mapping;
 
 	for (bar = 0; bar < DEVICE_COUNT_RESOURCE; bar++) {
@@ -813,7 +816,7 @@ int pcim_iomap_regions(struct pci_dev *pdev, int mask, const char *name)
 EXPORT_SYMBOL(pcim_iomap_regions);
 
 static int _pcim_request_region(struct pci_dev *pdev, int bar, const char *name,
-		int request_flags)
+				int request_flags)
 {
 	int ret;
 	struct pcim_addr_devres *res;
@@ -903,7 +906,7 @@ void pcim_release_region(struct pci_dev *pdev, int bar)
  */
 static void pcim_release_all_regions(struct pci_dev *pdev)
 {
-	short bar;
+	int bar;
 
 	for (bar = 0; bar < PCI_STD_NUM_BARS; bar++)
 		pcim_release_region(pdev, bar);
@@ -923,7 +926,7 @@ static void pcim_release_all_regions(struct pci_dev *pdev)
 static int pcim_request_all_regions(struct pci_dev *pdev, const char *name)
 {
 	int ret;
-	short bar;
+	int bar;
 
 	for (bar = 0; bar < PCI_STD_NUM_BARS; bar++) {
 		ret = pcim_request_region(pdev, bar, name);
@@ -960,7 +963,7 @@ static int pcim_request_all_regions(struct pci_dev *pdev, const char *name)
 int pcim_iomap_regions_request_all(struct pci_dev *pdev, int mask,
 				   const char *name)
 {
-	short bar;
+	int bar;
 	int ret;
 	void __iomem **legacy_iomap_table;
 
@@ -1004,14 +1007,14 @@ EXPORT_SYMBOL(pcim_iomap_regions_request_all);
  */
 void pcim_iounmap_regions(struct pci_dev *pdev, int mask)
 {
-	short bar;
+	int i;
 
-	for (bar = 0; bar < PCI_STD_NUM_BARS; bar++) {
-		if (!mask_contains_bar(mask, bar))
+	for (i = 0; i < PCI_STD_NUM_BARS; i++) {
+		if (!mask_contains_bar(mask, i))
 			continue;
 
-		pcim_iounmap_region(pdev, bar);
-		pcim_remove_bar_from_legacy_table(pdev, bar);
+		pcim_iounmap_region(pdev, i);
+		pcim_remove_bar_from_legacy_table(pdev, i);
 	}
 }
 EXPORT_SYMBOL(pcim_iounmap_regions);
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 1b4832a60047..807f8be043cd 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -4073,6 +4073,11 @@ EXPORT_SYMBOL(pci_release_regions);
  *
  * Returns 0 on success, or %EBUSY on error.  A warning
  * message is also printed on failure.
+ *
+ * NOTE:
+ * This is a "hybrid" function: It's normally unmanaged, but becomes managed
+ * when pcim_enable_device() has been called in advance. This hybrid feature is
+ * DEPRECATED! If you want managed cleanup, use the pcim_* functions instead.
  */
 int pci_request_regions(struct pci_dev *pdev, const char *res_name)
 {
@@ -4437,17 +4442,13 @@ void pci_disable_parity(struct pci_dev *dev)
  * NOTE:
  * This is a "hybrid" function: It's normally unmanaged, but becomes managed
  * when pcim_enable_device() has been called in advance. This hybrid feature is
- * DEPRECATED!
+ * DEPRECATED! If you want managed cleanup, use pcim_intx() instead.
  */
 void pci_intx(struct pci_dev *pdev, int enable)
 {
 	u16 pci_command, new;
 
-	/*
-	 * This is done for backwards compatibility, because the old PCI devres
-	 * API had a mode in which this function became managed if the dev had
-	 * been enabled with pcim_enable_device() instead of pci_enable_device().
-	 */
+	/* Preserve the "hybrid" behavior for backwards compatibility */
 	if (pci_is_managed(pdev)) {
 		WARN_ON_ONCE(pcim_intx(pdev, enable) != 0);
 		return;
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index e51e6fa79fcc..e6d299b93c21 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -813,7 +813,8 @@ static inline pci_power_t mid_pci_get_power_state(struct pci_dev *pdev)
 int pcim_intx(struct pci_dev *dev, int enable);
 
 int pcim_request_region(struct pci_dev *pdev, int bar, const char *name);
-int pcim_request_region_exclusive(struct pci_dev *pdev, int bar, const char *name);
+int pcim_request_region_exclusive(struct pci_dev *pdev, int bar,
+				  const char *name);
 void pcim_release_region(struct pci_dev *pdev, int bar);
 
 /*

Re: [PATCH v9 03/13] PCI: Add partial-BAR devres support

[Philipp Stanner]

On Thu, 2024-06-13 at 16:28 -0500, Bjorn Helgaas wrote:
> On Thu, Jun 13, 2024 at 01:50:16PM +0200, Philipp Stanner wrote:
> > With the current PCI devres API implementing a managed version of
> > pci_iomap_range() is impossible.
> > 
> > Furthermore, the PCI devres API currently is inconsistent and
> > complicated. This is in large part due to the fact that there are
> > hybrid
> > functions which are only sometimes managed via devres, and
> > functions
> > IO-mapping and requesting several BARs at once and returning
> > mappings
> > through a separately administrated table.
> > 
> > This table's indexing mechanism does not support partial-BAR
> > mappings.
> > 
> > Another notable problem is that there are no separate managed
> > counterparts for region-request functions such as
> > pci_request_region(),
> > as they exist for other PCI functions (e.g., pci_iomap() <->
> > pcim_iomap()). Instead, functions based on __pci_request_region()
> > change
> > their internal behavior and suddenly become managed functions when
> > pcim_enable_device() instead of pci_enable_device() is used.
> 
> The hybrid thing is certainly a problem, but does this patch address
> it?  I don't see that it does (other than adding comments in
> __pci_request_region() and pci_release_region()), but maybe I missed
> it.

This is just justification for why __pcim_request_region() etc. are
implemented. They bypass the hybrid nature  of __pci_request_region().
If the latter wouldn't have that behavior

> 
> Correct me if I'm wrong, but I don't think this patch makes any
> user-visible changes.

Except for deprecating that two functions and adding a new public one,
the entire series shouldn't make user-visible changes. That's the
point.

P.

> 
> I'm proposing this:
> 
>   PCI: Add managed partial-BAR request and map infrastructure
> 
>   The pcim_iomap_devres table tracks entire-BAR mappings, so we can't
> use it
>   to build a managed version of pci_iomap_range(), which maps partial
> BARs.
> 
>   Add struct pcim_addr_devres, which can track request and mapping of
> both
>   entire BARs and partial BARs.
> 
>   Add the following internal devres functions based on struct
>   pcim_addr_devres:
> 
>     pcim_iomap_region()               # request & map entire BAR
>     pcim_iounmap_region()             # unmap & release entire BAR
>     pcim_request_region()             # request entire BAR
>     pcim_release_region()             # release entire BAR
>     pcim_request_all_regions()        # request all entire BARs
>     pcim_release_all_regions()        # release all entire BARs
> 
>   Rework the following public interfaces using the new infrastructure
>   listed above:
> 
>     pcim_iomap()                      # map partial BAR
>     pcim_iounmap()                    # unmap partial BAR
>     pcim_iomap_regions()              # request & map specified BARs
>     pcim_iomap_regions_request_all()  # request all BARs, map
> specified BARs
>     pcim_iounmap_regions()            # unmap & release specified
> BARs
> 
> 
> > This API is hard to understand and potentially bug-provoking.
> > Hence, it
> > should be made more consistent.
> > 
> > This patch adds the necessary infrastructure for partial-BAR
> > mappings
> > managed with devres. That infrastructure also serves as a ground
> > layer
> > for significantly simplifying the PCI devres API in subsequent
> > patches
> > which can then cleanly separate managed and unmanaged API.
> > 
> > When having the long term goal of providing always managed
> > functions
> > prefixed with "pcim_" and never managed functions prefixed with
> > "pci_"
> > and, thus, separating managed and unmanaged APIs cleanly, new PCI
> > devres
> > infrastructure cannot use __pci_request_region() and its wrappers
> > since
> > those would then again interact with PCI devres and, consequently,
> > prevent the managed nature from being removed from the pci_*
> > functions
> > in the first place. Thus, it's necessary to provide an alternative
> > to
> > __pci_request_region().
> > 
> > This patch addresses the following problems of the PCI devres API:
> > 
> >   a) There is no PCI devres infrastructure on which a managed
> > counter
> >      part to pci_iomap_range() could be based on.
> > 
> >   b) The vast majority of the users of plural functions such as
> >      pcim_iomap_regions() only ever sets a single bit in the bit
> > mask,
> >      consequently making them singular functions anyways.
> > 
> >   c) region-request functions being sometimes managed and sometimes
> > not
> >      is bug-provoking. pcim_* functions should always be managed,
> > pci_*
> >      functions never.
> > 
> > Add a new PCI device resource, pcim_addr_devres, that serves to
> > encapsulate all device resource types related to region requests
> > and
> > IO-mappings since those are very frequently created together.
> > 
> > Add a set of alternatives cleanly separated from the hybrid
> > mechanism in
> > __pci_request_region() and its respective wrappers:
> >   - __pcim_request_region_range()
> >   - __pcim_release_region_range()
> >   - __pcim_request_region()
> >   - __pcim_release_region()
> > 
> > Add the following PCI-internal devres functions based on the above:
> >   - pcim_iomap_region()
> >   - pcim_iounmap_region()
> >   - _pcim_request_region()
> >   - pcim_request_region()
> >   - pcim_release_region()
> >   - pcim_request_all_regions()
> >   - pcim_release_all_regions()
> > 
> > Add new needed helper pcim_remove_bar_from_legacy_table().
> > 
> > Rework the following public interfaces using the new infrastructure
> > listed above:
> >   - pcim_iomap_release()
> >   - pcim_iomap()
> >   - pcim_iounmap()
> >   - pcim_iomap_regions()
> >   - pcim_iomap_regions_request_all()
> >   - pcim_iounmap_regions()
> > 
> > Update API documentation.
> > 
> > Link:
> > https://lore.kernel.org/r/20240605081605.18769-5-pstanner@redhat.com
> > Signed-off-by: Philipp Stanner <pstanner@redhat.com>
> > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> > ---
> >  drivers/pci/devres.c | 608 ++++++++++++++++++++++++++++++++++++++-
> > ----
> >  drivers/pci/pci.c    |  22 ++
> >  drivers/pci/pci.h    |   5 +
> >  3 files changed, 568 insertions(+), 67 deletions(-)
> > 
> > diff --git a/drivers/pci/devres.c b/drivers/pci/devres.c
> > index 845d6fab0ce7..cf2c11b54ca6 100644
> > --- a/drivers/pci/devres.c
> > +++ b/drivers/pci/devres.c
> > @@ -4,14 +4,243 @@
> >  #include "pci.h"
> >  
> >  /*
> > - * PCI iomap devres
> > + * On the state of PCI's devres implementation:
> > + *
> > + * The older devres API for PCI has two significant problems:
> > + *
> > + * 1. It is very strongly tied to the statically allocated mapping
> > table in
> > + *    struct pcim_iomap_devres below. This is mostly solved in the
> > sense of the
> > + *    pcim_ functions in this file providing things like ranged
> > mapping by
> > + *    bypassing this table, wheras the functions that were present
> > in the old
> > + *    API still enter the mapping addresses into the table for
> > users of the old
> > + *    API.
> > + *
> > + * 2. The region-request-functions in pci.c do become managed IF
> > the device has
> > + *    been enabled with pcim_enable_device() instead of
> > pci_enable_device().
> > + *    This resulted in the API becoming inconsistent: Some
> > functions have an
> > + *    obviously managed counter-part (e.g., pci_iomap() <->
> > pcim_iomap()),
> > + *    whereas some don't and are never managed, while others don't
> > and are
> > + *    _sometimes_ managed (e.g. pci_request_region()).
> > + *
> > + *    Consequently, in the new API, region requests performed by
> > the pcim_
> > + *    functions are automatically cleaned up through the devres
> > callback
> > + *    pcim_addr_resource_release(), while requests performed by
> > + *    pcim_enable_device() + pci_*region*() are automatically
> > cleaned up
> > + *    through the for-loop in pcim_release().
> > + *
> > + * TODO 1:
> > + * Remove the legacy table entirely once all calls to
> > pcim_iomap_table() in
> > + * the kernel have been removed.
> > + *
> > + * TODO 2:
> > + * Port everyone calling pcim_enable_device() + pci_*region*() to
> > using the
> > + * pcim_ functions. Then, remove all devres functionality from
> > pci_*region*()
> > + * functions and remove the associated cleanups described above in
> > point #2.
> >   */
> > -#define PCIM_IOMAP_MAX PCI_STD_NUM_BARS
> >  
> > +/*
> > + * Legacy struct storing addresses to whole mapped BARs.
> > + */
> >  struct pcim_iomap_devres {
> > -       void __iomem *table[PCIM_IOMAP_MAX];
> > +       void __iomem *table[PCI_STD_NUM_BARS];
> > +};
> > +
> > +enum pcim_addr_devres_type {
> > +       /* Default initializer. */
> > +       PCIM_ADDR_DEVRES_TYPE_INVALID,
> > +
> > +       /* A requested region spanning an entire BAR. */
> > +       PCIM_ADDR_DEVRES_TYPE_REGION,
> > +
> > +       /*
> > +        * A requested region spanning an entire BAR, and a mapping
> > for
> > +        * the entire BAR.
> > +        */
> > +       PCIM_ADDR_DEVRES_TYPE_REGION_MAPPING,
> > +
> > +       /*
> > +        * A mapping within a BAR, either spanning the whole BAR or
> > just a
> > +        * range.  Without a requested region.
> > +        */
> > +       PCIM_ADDR_DEVRES_TYPE_MAPPING,
> >  };
> >  
> > +/*
> > + * This struct envelops IO or MEM addresses, i.e., mappings and
> > region
> > + * requests, because those are very frequently requested and
> > released
> > + * together.
> > + */
> > +struct pcim_addr_devres {
> > +       enum pcim_addr_devres_type type;
> > +       void __iomem *baseaddr;
> > +       unsigned long offset;
> > +       unsigned long len;
> > +       short bar;
> > +};
> > +
> > +static inline void pcim_addr_devres_clear(struct pcim_addr_devres
> > *res)
> > +{
> > +       memset(res, 0, sizeof(*res));
> > +       res->bar = -1;
> > +}
> > +
> > +/*
> > + * The following functions, __pcim_*_region*, exist as
> > counterparts to the
> > + * versions from pci.c - which, unfortunately, can be in "hybrid
> > mode", i.e.,
> > + * sometimes managed, sometimes not.
> > + *
> > + * To separate the APIs cleanly, we define our own, simplified
> > versions here.
> > + */
> > +
> > +/**
> > + * __pcim_request_region_range - Request a ranged region
> > + * @pdev: PCI device the region belongs to
> > + * @bar: BAR the range is within
> > + * @offset: offset from the BAR's start address
> > + * @maxlen: length in bytes, beginning at @offset
> > + * @name: name associated with the request
> > + * @req_flags: flags for the request, e.g., for kernel-exclusive
> > requests
> > + *
> > + * Returns: 0 on success, a negative error code on failure.
> > + *
> > + * Request a range within a device's PCI BAR.  Sanity check the
> > input.
> > + */
> > +static int __pcim_request_region_range(struct pci_dev *pdev, int
> > bar,
> > +               unsigned long offset, unsigned long maxlen,
> > +               const char *name, int req_flags)
> > +{
> > +       resource_size_t start = pci_resource_start(pdev, bar);
> > +       resource_size_t len = pci_resource_len(pdev, bar);
> > +       unsigned long dev_flags = pci_resource_flags(pdev, bar);
> > +
> > +       if (start == 0 || len == 0) /* Unused BAR. */
> > +               return 0;
> > +       if (len <= offset)
> > +               return  -EINVAL;
> > +
> > +       start += offset;
> > +       len -= offset;
> > +
> > +       if (len > maxlen && maxlen != 0)
> > +               len = maxlen;
> > +
> > +       if (dev_flags & IORESOURCE_IO) {
> > +               if (!request_region(start, len, name))
> > +                       return -EBUSY;
> > +       } else if (dev_flags & IORESOURCE_MEM) {
> > +               if (!__request_mem_region(start, len, name,
> > req_flags))
> > +                       return -EBUSY;
> > +       } else {
> > +               /* That's not a device we can request anything on.
> > */
> > +               return -ENODEV;
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> > +static void __pcim_release_region_range(struct pci_dev *pdev, int
> > bar,
> > +               unsigned long offset, unsigned long maxlen)
> > +{
> > +       resource_size_t start = pci_resource_start(pdev, bar);
> > +       resource_size_t len = pci_resource_len(pdev, bar);
> > +       unsigned long flags = pci_resource_flags(pdev, bar);
> > +
> > +       if (len <= offset || start == 0)
> > +               return;
> > +
> > +       if (len == 0 || maxlen == 0) /* This an unused BAR. Do
> > nothing. */
> > +               return;
> > +
> > +       start += offset;
> > +       len -= offset;
> > +
> > +       if (len > maxlen)
> > +               len = maxlen;
> > +
> > +       if (flags & IORESOURCE_IO)
> > +               release_region(start, len);
> > +       else if (flags & IORESOURCE_MEM)
> > +               release_mem_region(start, len);
> > +}
> > +
> > +static int __pcim_request_region(struct pci_dev *pdev, int bar,
> > +               const char *name, int flags)
> > +{
> > +       unsigned long offset = 0;
> > +       unsigned long len = pci_resource_len(pdev, bar);
> > +
> > +       return __pcim_request_region_range(pdev, bar, offset, len,
> > name, flags);
> > +}
> > +
> > +static void __pcim_release_region(struct pci_dev *pdev, int bar)
> > +{
> > +       unsigned long offset = 0;
> > +       unsigned long len = pci_resource_len(pdev, bar);
> > +
> > +       __pcim_release_region_range(pdev, bar, offset, len);
> > +}
> > +
> > +static void pcim_addr_resource_release(struct device *dev, void
> > *resource_raw)
> > +{
> > +       struct pci_dev *pdev = to_pci_dev(dev);
> > +       struct pcim_addr_devres *res = resource_raw;
> > +
> > +       switch (res->type) {
> > +       case PCIM_ADDR_DEVRES_TYPE_REGION:
> > +               __pcim_release_region(pdev, res->bar);
> > +               break;
> > +       case PCIM_ADDR_DEVRES_TYPE_REGION_MAPPING:
> > +               pci_iounmap(pdev, res->baseaddr);
> > +               __pcim_release_region(pdev, res->bar);
> > +               break;
> > +       case PCIM_ADDR_DEVRES_TYPE_MAPPING:
> > +               pci_iounmap(pdev, res->baseaddr);
> > +               break;
> > +       default:
> > +               break;
> > +       }
> > +}
> > +
> > +static struct pcim_addr_devres *pcim_addr_devres_alloc(struct
> > pci_dev *pdev)
> > +{
> > +       struct pcim_addr_devres *res;
> > +
> > +       res = devres_alloc_node(pcim_addr_resource_release,
> > sizeof(*res),
> > +                       GFP_KERNEL, dev_to_node(&pdev->dev));
> > +       if (res)
> > +               pcim_addr_devres_clear(res);
> > +       return res;
> > +}
> > +
> > +/* Just for consistency and readability. */
> > +static inline void pcim_addr_devres_free(struct pcim_addr_devres
> > *res)
> > +{
> > +       devres_free(res);
> > +}
> > +
> > +/*
> > + * Used by devres to identify a pcim_addr_devres.
> > + */
> > +static int pcim_addr_resources_match(struct device *dev, void
> > *a_raw, void *b_raw)
> > +{
> > +       struct pcim_addr_devres *a, *b;
> > +
> > +       a = a_raw;
> > +       b = b_raw;
> > +
> > +       if (a->type != b->type)
> > +               return 0;
> > +
> > +       switch (a->type) {
> > +       case PCIM_ADDR_DEVRES_TYPE_REGION:
> > +       case PCIM_ADDR_DEVRES_TYPE_REGION_MAPPING:
> > +               return a->bar == b->bar;
> > +       case PCIM_ADDR_DEVRES_TYPE_MAPPING:
> > +               return a->baseaddr == b->baseaddr;
> > +       default:
> > +               return 0;
> > +       }
> > +}
> >  
> >  static void devm_pci_unmap_iospace(struct device *dev, void *ptr)
> >  {
> > @@ -92,8 +321,8 @@ EXPORT_SYMBOL(devm_pci_remap_cfgspace);
> >   *
> >   * 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::
> > + * Returns a pointer to the remapped memory or an IOMEM_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);
> > @@ -172,6 +401,17 @@ static void pcim_release(struct device
> > *gendev, void *res)
> >         struct pci_devres *this = res;
> >         int i;
> >  
> > +       /*
> > +        * This is legacy code.
> > +        *
> > +        * All regions requested by a pcim_ function do get
> > released through
> > +        * pcim_addr_resource_release(). Thanks to the hybrid
> > nature of the pci_
> > +        * region-request functions, this for-loop has to release
> > the regions
> > +        * if they have been requested by such a function.
> > +        *
> > +        * TODO: Remove this once all users of pcim_enable_device()
> > PLUS
> > +        * pci-region-request-functions have been ported to pcim_
> > functions.
> > +        */
> >         for (i = 0; i < DEVICE_COUNT_RESOURCE; i++)
> >                 if (mask_contains_bar(this->region_mask, i))
> >                         pci_release_region(dev, i);
> > @@ -258,19 +498,21 @@ EXPORT_SYMBOL(pcim_pin_device);
> >  
> >  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]);
> > +       /*
> > +        * Do nothing. This is legacy code.
> > +        *
> > +        * Cleanup of the mappings is now done directly through the
> > callbacks
> > +        * registered when creating them.
> > +        */
> >  }
> >  
> >  /**
> >   * pcim_iomap_table - access iomap allocation table
> >   * @pdev: PCI device to access iomap table for
> >   *
> > + * Returns:
> > + * Const pointer to array of __iomem pointers on success, NULL on
> > failure.
> > + *
> >   * 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
> > @@ -343,30 +585,67 @@ static void
> > pcim_remove_mapping_from_legacy_table(struct pci_dev *pdev,
> >         }
> >  }
> >  
> > +/*
> > + * The same as pcim_remove_mapping_from_legacy_table(), but
> > identifies the
> > + * mapping by its BAR index.
> > + */
> > +static void pcim_remove_bar_from_legacy_table(struct pci_dev
> > *pdev, short bar)
> > +{
> > +       void __iomem **legacy_iomap_table;
> > +
> > +       if (bar >= PCI_STD_NUM_BARS)
> > +               return;
> > +
> > +       legacy_iomap_table = (void __iomem
> > **)pcim_iomap_table(pdev);
> > +       if (!legacy_iomap_table)
> > +               return;
> > +
> > +       legacy_iomap_table[bar] = NULL;
> > +}
> > +
> >  /**
> >   * 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.
> > + * Returns: __iomem pointer on success, NULL on failure.
> > + *
> > + * Managed pci_iomap(). Map is automatically unmapped on driver
> > detach. If
> > + * desired, unmap manually only with pcim_iounmap().
> > + *
> > + * This SHOULD only be used once per BAR.
> > + *
> > + * NOTE:
> > + * Contrary to the other pcim_* functions, this function does not
> > return an
> > + * IOMEM_ERR_PTR() on failure, but a simple NULL. This is done for
> > backwards
> > + * compatibility.
> >   */
> >  void __iomem *pcim_iomap(struct pci_dev *pdev, int bar, unsigned
> > long maxlen)
> >  {
> >         void __iomem *mapping;
> > +       struct pcim_addr_devres *res;
> > +
> > +       res = pcim_addr_devres_alloc(pdev);
> > +       if (!res)
> > +               return NULL;
> > +       res->type = PCIM_ADDR_DEVRES_TYPE_MAPPING;
> >  
> >         mapping = pci_iomap(pdev, bar, maxlen);
> >         if (!mapping)
> > -               return NULL;
> > +               goto err_iomap;
> > +       res->baseaddr = mapping;
> >  
> >         if (pcim_add_mapping_to_legacy_table(pdev, mapping, bar) !=
> > 0)
> >                 goto err_table;
> >  
> > +       devres_add(&pdev->dev, res);
> >         return mapping;
> >  
> >  err_table:
> >         pci_iounmap(pdev, mapping);
> > +err_iomap:
> > +       pcim_addr_devres_free(res);
> >         return NULL;
> >  }
> >  EXPORT_SYMBOL(pcim_iomap);
> > @@ -376,91 +655,291 @@ EXPORT_SYMBOL(pcim_iomap);
> >   * @pdev: PCI device to iounmap for
> >   * @addr: Address to unmap
> >   *
> > - * Managed pci_iounmap().  @addr must have been mapped using
> > pcim_iomap().
> > + * Managed pci_iounmap(). @addr must have been mapped using a
> > pcim_* mapping
> > + * function.
> >   */
> >  void pcim_iounmap(struct pci_dev *pdev, void __iomem *addr)
> >  {
> > -       pci_iounmap(pdev, addr);
> > +       struct pcim_addr_devres res_searched;
> > +
> > +       pcim_addr_devres_clear(&res_searched);
> > +       res_searched.type = PCIM_ADDR_DEVRES_TYPE_MAPPING;
> > +       res_searched.baseaddr = addr;
> > +
> > +       if (devres_release(&pdev->dev, pcim_addr_resource_release,
> > +                       pcim_addr_resources_match, &res_searched)
> > != 0) {
> > +               /* Doesn't exist. User passed nonsense. */
> > +               return;
> > +       }
> >  
> >         pcim_remove_mapping_from_legacy_table(pdev, addr);
> >  }
> >  EXPORT_SYMBOL(pcim_iounmap);
> >  
> > +/**
> > + * pcim_iomap_region - Request and iomap a PCI BAR
> > + * @pdev: PCI device to map IO resources for
> > + * @bar: Index of a BAR to map
> > + * @name: Name associated with the request
> > + *
> > + * Returns: __iomem pointer on success, an IOMEM_ERR_PTR on
> > failure.
> > + *
> > + * Mapping and region will get automatically released on driver
> > detach. If
> > + * desired, release manually only with pcim_iounmap_region().
> > + */
> > +static void __iomem *pcim_iomap_region(struct pci_dev *pdev, int
> > bar,
> > +                                      const char *name)
> > +{
> > +       int ret;
> > +       struct pcim_addr_devres *res;
> > +
> > +       res = pcim_addr_devres_alloc(pdev);
> > +       if (!res)
> > +               return IOMEM_ERR_PTR(-ENOMEM);
> > +
> > +       res->type = PCIM_ADDR_DEVRES_TYPE_REGION_MAPPING;
> > +       res->bar = bar;
> > +
> > +       ret = __pcim_request_region(pdev, bar, name, 0);
> > +       if (ret != 0)
> > +               goto err_region;
> > +
> > +       res->baseaddr = pci_iomap(pdev, bar, 0);
> > +       if (!res->baseaddr) {
> > +               ret = -EINVAL;
> > +               goto err_iomap;
> > +       }
> > +
> > +       devres_add(&pdev->dev, res);
> > +       return res->baseaddr;
> > +
> > +err_iomap:
> > +       __pcim_release_region(pdev, bar);
> > +err_region:
> > +       pcim_addr_devres_free(res);
> > +
> > +       return IOMEM_ERR_PTR(ret);
> > +}
> > +
> > +/**
> > + * pcim_iounmap_region - Unmap and release a PCI BAR
> > + * @pdev: PCI device to operate on
> > + * @bar: Index of BAR to unmap and release
> > + *
> > + * Unmap a BAR and release its region manually. Only pass BARs
> > that were
> > + * previously mapped by pcim_iomap_region().
> > + */
> > +static void pcim_iounmap_region(struct pci_dev *pdev, int bar)
> > +{
> > +       struct pcim_addr_devres res_searched;
> > +
> > +       pcim_addr_devres_clear(&res_searched);
> > +       res_searched.type = PCIM_ADDR_DEVRES_TYPE_REGION_MAPPING;
> > +       res_searched.bar = bar;
> > +
> > +       devres_release(&pdev->dev, pcim_addr_resource_release,
> > +                       pcim_addr_resources_match, &res_searched);
> > +}
> > +
> >  /**
> >   * 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
> > + * @name: Name associated with the requests
> > + *
> > + * Returns: 0 on success, negative error code on failure.
> >   *
> >   * 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;
> > +       int ret;
> > +       short bar;
> > +       void __iomem *mapping;
> >  
> > -       iomap = pcim_iomap_table(pdev);
> > -       if (!iomap)
> > -               return -ENOMEM;
> > +       for (bar = 0; bar < DEVICE_COUNT_RESOURCE; bar++) {
> > +               if (!mask_contains_bar(mask, bar))
> > +                       continue;
> >  
> > -       for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
> > -               unsigned long len;
> > +               mapping = pcim_iomap_region(pdev, bar, name);
> > +               if (IS_ERR(mapping)) {
> > +                       ret = PTR_ERR(mapping);
> > +                       goto err;
> > +               }
> > +               ret = pcim_add_mapping_to_legacy_table(pdev,
> > mapping, bar);
> > +               if (ret != 0)
> > +                       goto err;
> > +       }
> >  
> > -               if (!mask_contains_bar(mask, i))
> > -                       continue;
> > +       return 0;
> >  
> > -               rc = -EINVAL;
> > -               len = pci_resource_len(pdev, i);
> > -               if (!len)
> > -                       goto err_inval;
> > +err:
> > +       while (--bar >= 0) {
> > +               pcim_iounmap_region(pdev, bar);
> > +               pcim_remove_bar_from_legacy_table(pdev, bar);
> > +       }
> >  
> > -               rc = pci_request_region(pdev, i, name);
> > -               if (rc)
> > -                       goto err_inval;
> > +       return ret;
> > +}
> > +EXPORT_SYMBOL(pcim_iomap_regions);
> >  
> > -               rc = -ENOMEM;
> > -               if (!pcim_iomap(pdev, i, 0))
> > -                       goto err_region;
> > +static int _pcim_request_region(struct pci_dev *pdev, int bar,
> > const char *name,
> > +               int request_flags)
> > +{
> > +       int ret;
> > +       struct pcim_addr_devres *res;
> > +
> > +       res = pcim_addr_devres_alloc(pdev);
> > +       if (!res)
> > +               return -ENOMEM;
> > +       res->type = PCIM_ADDR_DEVRES_TYPE_REGION;
> > +       res->bar = bar;
> > +
> > +       ret = __pcim_request_region(pdev, bar, name,
> > request_flags);
> > +       if (ret != 0) {
> > +               pcim_addr_devres_free(res);
> > +               return ret;
> >         }
> >  
> > +       devres_add(&pdev->dev, res);
> >         return 0;
> > +}
> >  
> > - err_region:
> > -       pci_release_region(pdev, i);
> > - err_inval:
> > -       while (--i >= 0) {
> > -               if (!mask_contains_bar(mask, i))
> > -                       continue;
> > -               pcim_iounmap(pdev, iomap[i]);
> > -               pci_release_region(pdev, i);
> > +/**
> > + * pcim_request_region - Request a PCI BAR
> > + * @pdev: PCI device to requestion region for
> > + * @bar: Index of BAR to request
> > + * @name: Name associated with the request
> > + *
> > + * Returns: 0 on success, a negative error code on failure.
> > + *
> > + * Request region specified by @bar.
> > + *
> > + * The region will automatically be released on driver detach. If
> > desired,
> > + * release manually only with pcim_release_region().
> > + */
> > +static int pcim_request_region(struct pci_dev *pdev, int bar,
> > const char *name)
> > +{
> > +       return _pcim_request_region(pdev, bar, name, 0);
> > +}
> > +
> > +/**
> > + * pcim_release_region - Release a PCI BAR
> > + * @pdev: PCI device to operate on
> > + * @bar: Index of BAR to release
> > + *
> > + * Release a region manually that was previously requested by
> > + * pcim_request_region().
> > + */
> > +static void pcim_release_region(struct pci_dev *pdev, int bar)
> > +{
> > +       struct pcim_addr_devres res_searched;
> > +
> > +       pcim_addr_devres_clear(&res_searched);
> > +       res_searched.type = PCIM_ADDR_DEVRES_TYPE_REGION;
> > +       res_searched.bar = bar;
> > +
> > +       devres_release(&pdev->dev, pcim_addr_resource_release,
> > +                       pcim_addr_resources_match, &res_searched);
> > +}
> > +
> > +
> > +/**
> > + * pcim_release_all_regions - Release all regions of a PCI-device
> > + * @pdev: the PCI device
> > + *
> > + * Release all regions previously requested through
> > pcim_request_region()
> > + * or pcim_request_all_regions().
> > + *
> > + * Can be called from any context, i.e., not necessarily as a
> > counterpart to
> > + * pcim_request_all_regions().
> > + */
> > +static void pcim_release_all_regions(struct pci_dev *pdev)
> > +{
> > +       short bar;
> > +
> > +       for (bar = 0; bar < PCI_STD_NUM_BARS; bar++)
> > +               pcim_release_region(pdev, bar);
> > +}
> > +
> > +/**
> > + * pcim_request_all_regions - Request all regions
> > + * @pdev: PCI device to map IO resources for
> > + * @name: name associated with the request
> > + *
> > + * Returns: 0 on success, negative error code on failure.
> > + *
> > + * Requested regions will automatically be released at driver
> > detach. If
> > + * desired, release individual regions with pcim_release_region()
> > or all of
> > + * them at once with pcim_release_all_regions().
> > + */
> > +static int pcim_request_all_regions(struct pci_dev *pdev, const
> > char *name)
> > +{
> > +       int ret;
> > +       short bar;
> > +
> > +       for (bar = 0; bar < PCI_STD_NUM_BARS; bar++) {
> > +               ret = pcim_request_region(pdev, bar, name);
> > +               if (ret != 0)
> > +                       goto err;
> >         }
> >  
> > -       return rc;
> > +       return 0;
> > +
> > +err:
> > +       pcim_release_all_regions(pdev);
> > +
> > +       return ret;
> >  }
> > -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
> > + * @name: Name associated with the requests
> > + *
> > + * Returns: 0 on success, negative error code on failure.
> >   *
> >   * Request all PCI BARs and iomap regions specified by @mask.
> > + *
> > + * To release these resources manually, call pcim_release_region()
> > for the
> > + * regions and pcim_iounmap() for the mappings.
> >   */
> >  int pcim_iomap_regions_request_all(struct pci_dev *pdev, int mask,
> >                                    const char *name)
> >  {
> > -       int request_mask = ((1 << 6) - 1) & ~mask;
> > -       int rc;
> > +       short bar;
> > +       int ret;
> > +       void __iomem **legacy_iomap_table;
> >  
> > -       rc = pci_request_selected_regions(pdev, request_mask,
> > name);
> > -       if (rc)
> > -               return rc;
> > +       ret = pcim_request_all_regions(pdev, name);
> > +       if (ret != 0)
> > +               return ret;
> >  
> > -       rc = pcim_iomap_regions(pdev, mask, name);
> > -       if (rc)
> > -               pci_release_selected_regions(pdev, request_mask);
> > -       return rc;
> > +       for (bar = 0; bar < PCI_STD_NUM_BARS; bar++) {
> > +               if (!mask_contains_bar(mask, bar))
> > +                       continue;
> > +               if (!pcim_iomap(pdev, bar, 0))
> > +                       goto err;
> > +       }
> > +
> > +       return 0;
> > +
> > +err:
> > +       /*
> > +        * If bar is larger than 0, then pcim_iomap() above has
> > most likely
> > +        * failed because of -EINVAL. If it is equal 0, most likely
> > the table
> > +        * couldn't be created, indicating -ENOMEM.
> > +        */
> > +       ret = bar > 0 ? -EINVAL : -ENOMEM;
> > +       legacy_iomap_table = (void __iomem
> > **)pcim_iomap_table(pdev);
> > +
> > +       while (--bar >= 0)
> > +               pcim_iounmap(pdev, legacy_iomap_table[bar]);
> > +
> > +       pcim_release_all_regions(pdev);
> > +
> > +       return ret;
> >  }
> >  EXPORT_SYMBOL(pcim_iomap_regions_request_all);
> >  
> > @@ -473,19 +952,14 @@
> > EXPORT_SYMBOL(pcim_iomap_regions_request_all);
> >   */
> >  void pcim_iounmap_regions(struct pci_dev *pdev, int mask)
> >  {
> > -       void __iomem * const *iomap;
> > -       int i;
> > +       short bar;
> >  
> > -       iomap = pcim_iomap_table(pdev);
> > -       if (!iomap)
> > -               return;
> > -
> > -       for (i = 0; i < PCIM_IOMAP_MAX; i++) {
> > -               if (!mask_contains_bar(mask, i))
> > +       for (bar = 0; bar < PCI_STD_NUM_BARS; bar++) {
> > +               if (!mask_contains_bar(mask, bar))
> >                         continue;
> >  
> > -               pcim_iounmap(pdev, iomap[i]);
> > -               pci_release_region(pdev, i);
> > +               pcim_iounmap_region(pdev, bar);
> > +               pcim_remove_bar_from_legacy_table(pdev, bar);
> >         }
> >  }
> >  EXPORT_SYMBOL(pcim_iounmap_regions);
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > index 59e0949fb079..d94445f5f882 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -3883,6 +3883,17 @@ void pci_release_region(struct pci_dev
> > *pdev, int bar)
> >                 release_mem_region(pci_resource_start(pdev, bar),
> >                                 pci_resource_len(pdev, bar));
> >  
> > +       /*
> > +        * This devres utility makes this function sometimes
> > managed
> > +        * (when pcim_enable_device() has been called before).
> > +        *
> > +        * This is bad because it conflicts with the pcim_
> > functions being
> > +        * exclusively responsible for managed PCI. Its "sometimes
> > yes,
> > +        * sometimes no" nature can cause bugs.
> > +        *
> > +        * TODO: Remove this once all users that use
> > pcim_enable_device() PLUS
> > +        * a region request function have been ported to using
> > pcim_ functions.
> > +        */
> >         dr = find_pci_dr(pdev);
> >         if (dr)
> >                 dr->region_mask &= ~(1 << bar);
> > @@ -3927,6 +3938,17 @@ static int __pci_request_region(struct
> > pci_dev *pdev, int bar,
> >                         goto err_out;
> >         }
> >  
> > +       /*
> > +        * This devres utility makes this function sometimes
> > managed
> > +        * (when pcim_enable_device() has been called before).
> > +        *
> > +        * This is bad because it conflicts with the pcim_
> > functions being
> > +        * exclusively responsible for managed pci. Its "sometimes
> > yes,
> > +        * sometimes no" nature can cause bugs.
> > +        *
> > +        * TODO: Remove this once all users that use
> > pcim_enable_device() PLUS
> > +        * a region request function have been ported to using
> > pcim_ functions.
> > +        */
> >         dr = find_pci_dr(pdev);
> >         if (dr)
> >                 dr->region_mask |= 1 << bar;
> > diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> > index fd44565c4756..c09487f5550c 100644
> > --- a/drivers/pci/pci.h
> > +++ b/drivers/pci/pci.h
> > @@ -826,6 +826,11 @@ struct pci_devres {
> >         unsigned int orig_intx:1;
> >         unsigned int restore_intx:1;
> >         unsigned int mwi:1;
> > +
> > +       /*
> > +        * TODO: remove the region_mask once everyone calling
> > +        * pcim_enable_device() + pci_*region*() is ported to pcim_
> > functions.
> > +        */
> >         u32 region_mask;
> >  };
> >  
> > -- 
> > 2.45.0
> > 
> 

Re: [PATCH v9 10/13] PCI: Give pci_intx() its own devres callback

[Philipp Stanner]

On Thu, 2024-06-13 at 16:06 -0500, Bjorn Helgaas wrote:
> On Thu, Jun 13, 2024 at 01:50:23PM +0200, Philipp Stanner wrote:
> > pci_intx() is one of the functions that have "hybrid mode" (i.e.,
> > sometimes managed, sometimes not). Providing a separate pcim_intx()
> > function with its own device resource and cleanup callback allows
> > for
> > removing further large parts of the legacy PCI devres
> > implementation.
> > 
> > As in the region-request-functions, pci_intx() has to call into its
> > managed counterpart for backwards compatibility.
> > 
> > As pci_intx() is an outdated function, pcim_intx() shall not be
> > made
> > visible to drivers via a public API.
> 
> What makes pci_intx() outdated?  If it's outdated, we should mention
> why and what the 30+ callers (including a couple in drivers/pci/)
> should use instead.

That is 100% based on Andy Shevchenko's (+CC) statement back from
January 2024 a.D. [1]

Apparently INTx is "old IRQ management" and should be done through
pci_alloc_irq_vectors() nowadays.


[1] https://lore.kernel.org/all/ZabyY3csP0y-p7lb@surfacebook.localdomain/


P.


> 
> Bjorn
> 

Re: [PATCH v9 00/13] Make PCI's devres API more consistent

[Philipp Stanner]

On Thu, 2024-06-13 at 16:57 -0500, Bjorn Helgaas wrote:
> On Thu, Jun 13, 2024 at 01:50:13PM +0200, Philipp Stanner wrote:
> > Changes in v9:
> >   - Remove forgotten dead code ('enabled' bit in struct pci_dev) in
> >     patch No.8 ("Move pinned status bit...")
> >   - Rework patch No.3:
> >       - Change title from "Reimplement plural devres functions"
> >         to "Add partial-BAR devres support".
> >       - Drop excessive details about the general cleanup from the
> > commit
> >         message. Only motivate why this patch's new infrastructure
> > is
> >         necessary.
> >   - Fix some minor spelling issues (s/pci/PCI ...)
> > 
> > Changes in v8:
> >   - Rebase the series on the already merged patches which were
> > slightly
> >     modified by Bjorn Helgaas.
> >   - Reword the pci_intx() commit message so it clearly states it's
> > about
> >     reworking pci_intx().
> >   - Move the removal of find_pci_dr() from patch "Remove legacy
> >     pcim_release()" to patch "Give pci_intx() its own devres
> > callback"
> >     since this later patch already removed all calls to that
> > function.
> >   - In patch "Give pci_intx() its own devres callback": use
> >     pci_is_enabled() (and, thus, the enabled_cnt in struct pci_dev)
> >     instead of a separate enabled field. (Bjorn)
> > 
> > Changes in v7:
> >   - Split the entire series in smaller, more atomic chunks /
> > patches
> >     (Bjorn)
> >   - Remove functions (such as pcim_iomap_region_range()) that do
> > not yet
> >     have a user (Bjorn)
> >   - Don't export interfaces publicly anymore, except for
> >     pcim_iomap_range(), needed by vboxvideo (Bjorn)
> >   - Mention the actual (vboxvideo) bug in "PCI: Warn users..."
> > commit
> >     (Bjorn)
> >   - Drop docstring warnings on PCI-internal functions (Bjorn)
> >   - Rework docstring warnings
> >   - Fix spelling in a few places. Rewrapp paragraphs (Bjorn)
> > 
> > Changes in v6:
> >   - Restructure the cleanup in pcim_iomap_regions_request_all() so
> > that
> >     it doesn't trigger a (false positive) test robot warning. No
> >     behavior change intended. (Dan Carpenter)
> > 
> > Changes in v5:
> >   - Add Hans's Reviewed-by to vboxvideo patch (Hans de Goede)
> >   - Remove stable-kernel from CC in vboxvideo patch (Hans de Goede)
> > 
> > Changes in v4:
> >   - Rebase against linux-next
> > 
> > Changes in v3:
> >   - Use the term "PCI devres API" at some forgotten places.
> >   - Fix more grammar errors in patch #3.
> >   - Remove the comment advising to call (the outdated) pcim_intx()
> > in pci.c
> >   - Rename __pcim_request_region_range() flags-field "exclusive" to
> >     "req_flags", since this is what the int actually represents.
> >   - Remove the call to pcim_region_request() from patch #10. (Hans)
> > 
> > Changes in v2:
> >   - Make commit head lines congruent with PCI's style (Bjorn)
> >   - Add missing error checks for devm_add_action(). (Andy)
> >   - Repair the "Returns: " marks for docu generation (Andy)
> >   - Initialize the addr_devres struct with memset(). (Andy)
> >   - Make pcim_intx() a PCI-internal function so that new drivers
> > won't
> >     be encouraged to use the outdated pci_intx() mechanism.
> >     (Andy / Philipp)
> >   - Fix grammar and spelling (Bjorn)
> >   - Be more precise on why pcim_iomap_table() is problematic
> > (Bjorn)
> >   - Provide the actual structs' and functions' names in the commit
> >     messages (Bjorn)
> >   - Remove redundant variable initializers (Andy)
> >   - Regroup PM bitfield members in struct pci_dev (Andy)
> >   - Make pcim_intx() visible only for the PCI subsystem so that
> > new    
> >     drivers won't use this outdated API (Andy, Myself)
> >   - Add a NOTE to pcim_iomap() to warn about this function being
> > the one
> >     exception that does just return NULL.
> >   - Consistently use the term "PCI devres API"; also in Patch #10
> > (Bjorn)
> > 
> > 
> > ¡Hola!
> > 
> > PCI's devres API suffers several weaknesses:
> > 
> > 1. There are functions prefixed with pcim_. Those are always
> > managed
> >    counterparts to never-managed functions prefixed with pci_ – or
> > so one
> >    would like to think. There are some apparently unmanaged
> > functions
> >    (all region-request / release functions, and pci_intx()) which
> >    suddenly become managed once the user has initialized the device
> > with
> >    pcim_enable_device() instead of pci_enable_device(). This
> > "sometimes
> >    yes, sometimes no" nature of those functions is confusing and
> >    therefore bug-provoking. In fact, it has already caused a bug in
> > DRM.
> >    The last patch in this series fixes that bug.
> > 2. iomappings: Instead of giving each mapping its own callback, the
> >    existing API uses a statically allocated struct tracking one
> > mapping
> >    per bar. This is not extensible. Especially, you can't create
> >    _ranged_ managed mappings that way, which many drivers want.
> > 3. Managed request functions only exist as "plural versions" with a
> >    bit-mask as a parameter. That's quite over-engineered
> > considering
> >    that each user only ever mapps one, maybe two bars.
> > 
> > This series:
> > - add a set of new "singular" devres functions that use devres the
> > way
> >   its intended, with one callback per resource.
> > - deprecates the existing iomap-table mechanism.
> > - deprecates the hybrid nature of pci_ functions.
> > - preserves backwards compatibility so that drivers using the
> > existing
> >   API won't notice any changes.
> > - adds documentation, especially some warning users about the
> >   complicated nature of PCI's devres.
> > 
> > 
> > Note that this series is based on my "unify pci_iounmap"-series
> > from a
> > few weeks ago. [1]
> > 
> > I tested this on a x86 VM with a simple pci test-device with two
> > regions. Operates and reserves resources as intended on my system.
> > Kasan and kmemleak didn't find any problems.
> > 
> > I believe this series cleans the API up as much as possible without
> > having to port all existing drivers to the new API. Especially, I
> > think
> > that this implementation is easy to extend if the need for new
> > managed
> > functions arises :)
> > 
> > Greetings,
> > P.
> > 
> > Philipp Stanner (13):
> >   PCI: Add and use devres helper for bit masks
> >   PCI: Add devres helpers for iomap table
> >   PCI: Add partial-BAR devres support
> >   PCI: Deprecate two surplus devres functions
> >   PCI: Make devres region requests consistent
> >   PCI: Warn users about complicated devres nature
> >   PCI: Remove enabled status bit from pci_devres
> >   PCI: Move pinned status bit to struct pci_dev
> >   PCI: Give pcim_set_mwi() its own devres callback
> >   PCI: Give pci_intx() its own devres callback
> >   PCI: Remove legacy pcim_release()
> >   PCI: Add pcim_iomap_range()
> >   drm/vboxvideo: fix mapping leaks
> > 
> >  drivers/gpu/drm/vboxvideo/vbox_main.c |  20 +-
> >  drivers/pci/devres.c                  | 903 +++++++++++++++++++++-
> > ----
> >  drivers/pci/iomap.c                   |  16 +
> >  drivers/pci/pci.c                     |  94 ++-
> >  drivers/pci/pci.h                     |  23 +-
> >  include/linux/pci.h                   |   5 +-
> >  6 files changed, 858 insertions(+), 203 deletions(-)
> 
> This is on pci/devres with some commit log rework and the following
> diffs.  I think the bar short/int thing is the only actual code
> change.  Happy to squash in any other updates or things I botched.

I looked through your tree and only found the following nit:

In commit "PCI: Remove struct pci_devres.enabled status bit" you
changed the line

"The PCI devres implementation has a separate boolean to track whether
a"

to:

"The pci_devres struct has a separate boolean to track whether a device
is"

In past reviews that has been criticized and I was told to always call
it "struct pci_devres", not the other way around. That's also how it's
put in the following paragraph.

> 
> Planned for v6.11.

\o/

P.

> 
> diff --git a/drivers/pci/devres.c b/drivers/pci/devres.c
> index 2f0379a4e58f..d9b78a0d903a 100644
> --- a/drivers/pci/devres.c
> +++ b/drivers/pci/devres.c
> @@ -11,7 +11,7 @@
>   * 1. It is very strongly tied to the statically allocated mapping
> table in
>   *    struct pcim_iomap_devres below. This is mostly solved in the
> sense of the
>   *    pcim_ functions in this file providing things like ranged
> mapping by
> - *    bypassing this table, wheras the functions that were present
> in the old
> + *    bypassing this table, whereas the functions that were present
> in the old
>   *    API still enter the mapping addresses into the table for users
> of the old
>   *    API.
>   *
> @@ -25,10 +25,11 @@
>   *    Consequently, in the new API, region requests performed by the
> pcim_
>   *    functions are automatically cleaned up through the devres
> callback
>   *    pcim_addr_resource_release().
> - *    Users utilizing pcim_enable_device() + pci_*region*() are
> redirected in
> + *
> + *    Users of pcim_enable_device() + pci_*region*() are redirected
> in
>   *    pci.c to the managed functions here in this file. This isn't
> exactly
> - *    perfect, but the only alternative way would be to port ALL
> drivers using
> - *    said combination to pcim_ functions.
> + *    perfect, but the only alternative way would be to port ALL
> drivers
> + *    using said combination to pcim_ functions.
>   *
>   * TODO:
>   * Remove the legacy table entirely once all calls to
> pcim_iomap_table() in
> @@ -42,7 +43,7 @@ struct pcim_iomap_devres {
>         void __iomem *table[PCI_STD_NUM_BARS];
>  };
>  
> -/* Used to restore the old intx state on driver detach. */
> +/* Used to restore the old INTx state on driver detach. */
>  struct pcim_intx_devres {
>         int orig_intx;
>  };
> @@ -77,7 +78,7 @@ struct pcim_addr_devres {
>         void __iomem *baseaddr;
>         unsigned long offset;
>         unsigned long len;
> -       short bar;
> +       int bar;
>  };
>  
>  static inline void pcim_addr_devres_clear(struct pcim_addr_devres
> *res)
> @@ -108,8 +109,9 @@ static inline void pcim_addr_devres_clear(struct
> pcim_addr_devres *res)
>   * Request a range within a device's PCI BAR.  Sanity check the
> input.
>   */
>  static int __pcim_request_region_range(struct pci_dev *pdev, int
> bar,
> -               unsigned long offset, unsigned long maxlen,
> -               const char *name, int req_flags)
> +                                      unsigned long offset,
> +                                      unsigned long maxlen,
> +                                      const char *name, int
> req_flags)
>  {
>         resource_size_t start = pci_resource_start(pdev, bar);
>         resource_size_t len = pci_resource_len(pdev, bar);
> @@ -118,7 +120,7 @@ static int __pcim_request_region_range(struct
> pci_dev *pdev, int bar,
>         if (start == 0 || len == 0) /* Unused BAR. */
>                 return 0;
>         if (len <= offset)
> -               return  -EINVAL;
> +               return -EINVAL;
>  
>         start += offset;
>         len -= offset;
> @@ -141,7 +143,8 @@ static int __pcim_request_region_range(struct
> pci_dev *pdev, int bar,
>  }
>  
>  static void __pcim_release_region_range(struct pci_dev *pdev, int
> bar,
> -               unsigned long offset, unsigned long maxlen)
> +                                       unsigned long offset,
> +                                       unsigned long maxlen)
>  {
>         resource_size_t start = pci_resource_start(pdev, bar);
>         resource_size_t len = pci_resource_len(pdev, bar);
> @@ -166,7 +169,7 @@ static void __pcim_release_region_range(struct
> pci_dev *pdev, int bar,
>  }
>  
>  static int __pcim_request_region(struct pci_dev *pdev, int bar,
> -               const char *name, int flags)
> +                                const char *name, int flags)
>  {
>         unsigned long offset = 0;
>         unsigned long len = pci_resource_len(pdev, bar);
> @@ -208,7 +211,7 @@ static struct pcim_addr_devres
> *pcim_addr_devres_alloc(struct pci_dev *pdev)
>         struct pcim_addr_devres *res;
>  
>         res = devres_alloc_node(pcim_addr_resource_release,
> sizeof(*res),
> -                       GFP_KERNEL, dev_to_node(&pdev->dev));
> +                               GFP_KERNEL, dev_to_node(&pdev->dev));
>         if (res)
>                 pcim_addr_devres_clear(res);
>         return res;
> @@ -223,7 +226,8 @@ static inline void pcim_addr_devres_free(struct
> pcim_addr_devres *res)
>  /*
>   * Used by devres to identify a pcim_addr_devres.
>   */
> -static int pcim_addr_resources_match(struct device *dev, void
> *a_raw, void *b_raw)
> +static int pcim_addr_resources_match(struct device *dev,
> +                                    void *a_raw, void *b_raw)
>  {
>         struct pcim_addr_devres *a, *b;
>  
> @@ -402,7 +406,6 @@ int pcim_set_mwi(struct pci_dev *pdev)
>  }
>  EXPORT_SYMBOL(pcim_set_mwi);
>  
> -
>  static inline bool mask_contains_bar(int mask, int bar)
>  {
>         return mask & BIT(bar);
> @@ -438,8 +441,8 @@ static struct pcim_intx_devres
> *get_or_create_intx_devres(struct device *dev)
>   *
>   * Returns: 0 on success, -ENOMEM on error.
>   *
> - * Enables/disables PCI INTx for device @pdev.
> - * Restores the original state on driver detach.
> + * Enable/disable PCI INTx for device @pdev.
> + * Restore the original state on driver detach.
>   */
>  int pcim_intx(struct pci_dev *pdev, int enable)
>  {
> @@ -492,7 +495,7 @@ int pcim_enable_device(struct pci_dev *pdev)
>  
>         /*
>          * We prefer removing the action in case of an error over
> -        * devm_add_action_or_reset() because the later could
> theoretically be
> +        * devm_add_action_or_reset() because the latter could
> theoretically be
>          * disturbed by users having pinned the device too soon.
>          */
>         ret = pci_enable_device(pdev);
> @@ -618,7 +621,7 @@ static void
> pcim_remove_mapping_from_legacy_table(struct pci_dev *pdev,
>   * The same as pcim_remove_mapping_from_legacy_table(), but
> identifies the
>   * mapping by its BAR index.
>   */
> -static void pcim_remove_bar_from_legacy_table(struct pci_dev *pdev,
> short bar)
> +static void pcim_remove_bar_from_legacy_table(struct pci_dev *pdev,
> int bar)
>  {
>         void __iomem **legacy_iomap_table;
>  
> @@ -783,7 +786,7 @@ static void pcim_iounmap_region(struct pci_dev
> *pdev, int bar)
>  int pcim_iomap_regions(struct pci_dev *pdev, int mask, const char
> *name)
>  {
>         int ret;
> -       short bar;
> +       int bar;
>         void __iomem *mapping;
>  
>         for (bar = 0; bar < DEVICE_COUNT_RESOURCE; bar++) {
> @@ -813,7 +816,7 @@ int pcim_iomap_regions(struct pci_dev *pdev, int
> mask, const char *name)
>  EXPORT_SYMBOL(pcim_iomap_regions);
>  
>  static int _pcim_request_region(struct pci_dev *pdev, int bar, const
> char *name,
> -               int request_flags)
> +                               int request_flags)
>  {
>         int ret;
>         struct pcim_addr_devres *res;
> @@ -903,7 +906,7 @@ void pcim_release_region(struct pci_dev *pdev,
> int bar)
>   */
>  static void pcim_release_all_regions(struct pci_dev *pdev)
>  {
> -       short bar;
> +       int bar;
>  
>         for (bar = 0; bar < PCI_STD_NUM_BARS; bar++)
>                 pcim_release_region(pdev, bar);
> @@ -923,7 +926,7 @@ static void pcim_release_all_regions(struct
> pci_dev *pdev)
>  static int pcim_request_all_regions(struct pci_dev *pdev, const char
> *name)
>  {
>         int ret;
> -       short bar;
> +       int bar;
>  
>         for (bar = 0; bar < PCI_STD_NUM_BARS; bar++) {
>                 ret = pcim_request_region(pdev, bar, name);
> @@ -960,7 +963,7 @@ static int pcim_request_all_regions(struct
> pci_dev *pdev, const char *name)
>  int pcim_iomap_regions_request_all(struct pci_dev *pdev, int mask,
>                                    const char *name)
>  {
> -       short bar;
> +       int bar;
>         int ret;
>         void __iomem **legacy_iomap_table;
>  
> @@ -1004,14 +1007,14 @@
> EXPORT_SYMBOL(pcim_iomap_regions_request_all);
>   */
>  void pcim_iounmap_regions(struct pci_dev *pdev, int mask)
>  {
> -       short bar;
> +       int i;
>  
> -       for (bar = 0; bar < PCI_STD_NUM_BARS; bar++) {
> -               if (!mask_contains_bar(mask, bar))
> +       for (i = 0; i < PCI_STD_NUM_BARS; i++) {
> +               if (!mask_contains_bar(mask, i))
>                         continue;
>  
> -               pcim_iounmap_region(pdev, bar);
> -               pcim_remove_bar_from_legacy_table(pdev, bar);
> +               pcim_iounmap_region(pdev, i);
> +               pcim_remove_bar_from_legacy_table(pdev, i);
>         }
>  }
>  EXPORT_SYMBOL(pcim_iounmap_regions);
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 1b4832a60047..807f8be043cd 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -4073,6 +4073,11 @@ EXPORT_SYMBOL(pci_release_regions);
>   *
>   * Returns 0 on success, or %EBUSY on error.  A warning
>   * message is also printed on failure.
> + *
> + * NOTE:
> + * This is a "hybrid" function: It's normally unmanaged, but becomes
> managed
> + * when pcim_enable_device() has been called in advance. This hybrid
> feature is
> + * DEPRECATED! If you want managed cleanup, use the pcim_* functions
> instead.
>   */
>  int pci_request_regions(struct pci_dev *pdev, const char *res_name)
>  {
> @@ -4437,17 +4442,13 @@ void pci_disable_parity(struct pci_dev *dev)
>   * NOTE:
>   * This is a "hybrid" function: It's normally unmanaged, but becomes
> managed
>   * when pcim_enable_device() has been called in advance. This hybrid
> feature is
> - * DEPRECATED!
> + * DEPRECATED! If you want managed cleanup, use pcim_intx() instead.
>   */
>  void pci_intx(struct pci_dev *pdev, int enable)
>  {
>         u16 pci_command, new;
>  
> -       /*
> -        * This is done for backwards compatibility, because the old
> PCI devres
> -        * API had a mode in which this function became managed if
> the dev had
> -        * been enabled with pcim_enable_device() instead of
> pci_enable_device().
> -        */
> +       /* Preserve the "hybrid" behavior for backwards compatibility
> */
>         if (pci_is_managed(pdev)) {
>                 WARN_ON_ONCE(pcim_intx(pdev, enable) != 0);
>                 return;
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index e51e6fa79fcc..e6d299b93c21 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -813,7 +813,8 @@ static inline pci_power_t
> mid_pci_get_power_state(struct pci_dev *pdev)
>  int pcim_intx(struct pci_dev *dev, int enable);
>  
>  int pcim_request_region(struct pci_dev *pdev, int bar, const char
> *name);
> -int pcim_request_region_exclusive(struct pci_dev *pdev, int bar,
> const char *name);
> +int pcim_request_region_exclusive(struct pci_dev *pdev, int bar,
> +                                 const char *name);
>  void pcim_release_region(struct pci_dev *pdev, int bar);
>  
>  /*
> 

Re: [PATCH v9 10/13] PCI: Give pci_intx() its own devres callback

[Bjorn Helgaas]

On Fri, Jun 14, 2024 at 10:09:46AM +0200, Philipp Stanner wrote:
> On Thu, 2024-06-13 at 16:06 -0500, Bjorn Helgaas wrote:
> > On Thu, Jun 13, 2024 at 01:50:23PM +0200, Philipp Stanner wrote:
> > > pci_intx() is one of the functions that have "hybrid mode" (i.e.,
> > > sometimes managed, sometimes not). Providing a separate pcim_intx()
> > > function with its own device resource and cleanup callback allows
> > > for
> > > removing further large parts of the legacy PCI devres
> > > implementation.
> > > 
> > > As in the region-request-functions, pci_intx() has to call into its
> > > managed counterpart for backwards compatibility.
> > > 
> > > As pci_intx() is an outdated function, pcim_intx() shall not be
> > > made
> > > visible to drivers via a public API.
> > 
> > What makes pci_intx() outdated?  If it's outdated, we should mention
> > why and what the 30+ callers (including a couple in drivers/pci/)
> > should use instead.
> 
> That is 100% based on Andy Shevchenko's (+CC) statement back from
> January 2024 a.D. [1]
> 
> Apparently INTx is "old IRQ management" and should be done through
> pci_alloc_irq_vectors() nowadays.

Do we have pcim_ support for pci_alloc_irq_vectors()?

> [1] https://lore.kernel.org/all/ZabyY3csP0y-p7lb@surfacebook.localdomain/

Re: [PATCH v9 00/13] Make PCI's devres API more consistent

[Bjorn Helgaas]

On Fri, Jun 14, 2024 at 01:38:41PM +0200, Philipp Stanner wrote:
> On Thu, 2024-06-13 at 16:57 -0500, Bjorn Helgaas wrote:

> > This is on pci/devres with some commit log rework and the following
> > diffs.  I think the bar short/int thing is the only actual code
> > change.  Happy to squash in any other updates or things I botched.
> 
> I looked through your tree and only found the following nit:
> 
> In commit "PCI: Remove struct pci_devres.enabled status bit" you
> changed the line
> 
> "The PCI devres implementation has a separate boolean to track whether
> a"
> 
> to:
> 
> "The pci_devres struct has a separate boolean to track whether a device
> is"
> 
> In past reviews that has been criticized and I was told to always call
> it "struct pci_devres", not the other way around. That's also how it's
> put in the following paragraph.

Amended to that, thanks.

Re: [PATCH v9 10/13] PCI: Give pci_intx() its own devres callback

[Philipp Stanner]

On Fri, 2024-06-14 at 11:14 -0500, Bjorn Helgaas wrote:
> On Fri, Jun 14, 2024 at 10:09:46AM +0200, Philipp Stanner wrote:
> > On Thu, 2024-06-13 at 16:06 -0500, Bjorn Helgaas wrote:
> > > On Thu, Jun 13, 2024 at 01:50:23PM +0200, Philipp Stanner wrote:
> > > > pci_intx() is one of the functions that have "hybrid mode"
> > > > (i.e.,
> > > > sometimes managed, sometimes not). Providing a separate
> > > > pcim_intx()
> > > > function with its own device resource and cleanup callback
> > > > allows
> > > > for
> > > > removing further large parts of the legacy PCI devres
> > > > implementation.
> > > > 
> > > > As in the region-request-functions, pci_intx() has to call into
> > > > its
> > > > managed counterpart for backwards compatibility.
> > > > 
> > > > As pci_intx() is an outdated function, pcim_intx() shall not be
> > > > made
> > > > visible to drivers via a public API.
> > > 
> > > What makes pci_intx() outdated?  If it's outdated, we should
> > > mention
> > > why and what the 30+ callers (including a couple in drivers/pci/)
> > > should use instead.
> > 
> > That is 100% based on Andy Shevchenko's (+CC) statement back from
> > January 2024 a.D. [1]
> > 
> > Apparently INTx is "old IRQ management" and should be done through
> > pci_alloc_irq_vectors() nowadays.
> 
> Do we have pcim_ support for pci_alloc_irq_vectors()?

Nope.

All PCI devres functions that exist are now in pci/devres.c, except for
the hybrid functions (pci_intx() and everything calling
__pci_request_region()) in pci.c


P.

> 
> > [1]
> > https://lore.kernel.org/all/ZabyY3csP0y-p7lb@surfacebook.localdomain/
> 

Re: [PATCH v9 10/13] PCI: Give pci_intx() its own devres callback

[Bjorn Helgaas]

On Mon, Jun 17, 2024 at 10:21:10AM +0200, Philipp Stanner wrote:
> On Fri, 2024-06-14 at 11:14 -0500, Bjorn Helgaas wrote:
> > On Fri, Jun 14, 2024 at 10:09:46AM +0200, Philipp Stanner wrote:
> ...

> > > Apparently INTx is "old IRQ management" and should be done through
> > > pci_alloc_irq_vectors() nowadays.
> > 
> > Do we have pcim_ support for pci_alloc_irq_vectors()?
> 
> Nope.

Should we?  Or is IRQ support not amenable to devm?

Happened to see this new driver:
https://lore.kernel.org/all/20240617100359.2550541-3-Basavaraj.Natikar@amd.com/
that uses devm and the only PCI-related part of .remove() is cleaning
up the IRQs.

Re: [PATCH v9 10/13] PCI: Give pci_intx() its own devres callback

[Philipp Stanner]

On Mon, 2024-06-17 at 11:46 -0500, Bjorn Helgaas wrote:
> On Mon, Jun 17, 2024 at 10:21:10AM +0200, Philipp Stanner wrote:
> > On Fri, 2024-06-14 at 11:14 -0500, Bjorn Helgaas wrote:
> > > On Fri, Jun 14, 2024 at 10:09:46AM +0200, Philipp Stanner wrote:
> > ...
> 
> > > > Apparently INTx is "old IRQ management" and should be done
> > > > through
> > > > pci_alloc_irq_vectors() nowadays.
> > > 
> > > Do we have pcim_ support for pci_alloc_irq_vectors()?
> > 
> > Nope.
> 
> Should we?  Or is IRQ support not amenable to devm?

I don't see why it wouldn't work, AFAIU you just register a callback
that deregisters the interrupts again.

This series here, though, stems from me trying to clean up drivers in
DRM. That's when I discovered that regions / IO-mappings (which I need)
are broken.

Adding further stuff to pci/devres.c is no problem at all and
independent from this series; one just has to add the code and call the
appropriate devm_ functions.

> 
> Happened to see this new driver:
> https://lore.kernel.org/all/20240617100359.2550541-3-Basavaraj.Natikar@amd.com/
> that uses devm and the only PCI-related part of .remove() is cleaning
> up the IRQs.
> 

OK. They also use pcim_iomap_table() and stuff. I think we should
inform about the deprecation.

I don't have a user for IRQ at hand for my DRM work right now. I'd try
to upstream new infrastructure we need there as I did for vboxvideo.


Grüße,
P.

Re: [PATCH v9 10/13] PCI: Give pci_intx() its own devres callback

[Ashish Kalra]

With this patch applied, we are observing unloading and then reloading issues with the AMD Crypto (CCP) driver:

with DEVRES logging enabled, we observe the following logs:

[  218.093588] ccp 0000:a2:00.1: DEVRES REL 00000000c18c52fb 0xffff8d09dc1972c0 devm_kzalloc_release (152 bytes)
[  218.105527] ccp 0000:a2:00.1: DEVRES REL 000000003091fb95 0xffff8d09d3aad000 devm_kzalloc_release (3072 bytes)
[  218.117500] ccp 0000:a2:00.1: DEVRES REL 0000000049e4adfe 0xffff8d09d588f000 pcim_intx_restore (4 bytes)
[  218.129519] ccp 0000:a2:00.1: DEVRES ADD 000000001a2ac6ad 0xffff8cfa867b7cc0 pcim_intx_restore (4 bytes)
[  218.140434] ccp 0000:a2:00.1: DEVRES REL 00000000627ecaf7 0xffff8d09d588f680 pcim_msi_release (16 bytes)
[  218.151665] ccp 0000:a2:00.1: DEVRES REL 0000000058b2252a 0xffff8d09dc199680 msi_device_data_release (80 bytes)
[  218.163625] ccp 0000:a2:00.1: DEVRES REL 00000000435cc85e 0xffff8d09d588ff80 devm_attr_group_remove (8 bytes)
[  218.175224] ccp 0000:a2:00.1: DEVRES REL 00000000cb6fcd9b 0xffff8d09eb583660 pcim_addr_resource_release (40 bytes)
[  218.187319] ccp 0000:a2:00.1: DEVRES REL 00000000d64a8b84 0xffff8d09eb583180 pcim_iomap_release (48 bytes)
[  218.198615] ccp 0000:a2:00.1: DEVRES REL 0000000099ac6b28 0xffff8d09eb5830c0 pcim_addr_resource_release (40 bytes)
[  218.210730] ccp 0000:a2:00.1: DEVRES REL 00000000bdd27f88 0xffff8d09d3ac2700 pcim_release (0 bytes)
[  218.221489] ccp 0000:a2:00.1: DEVRES REL 00000000e763315c 0xffff8d09d3ac2240 devm_kzalloc_release (20 bytes)
[  218.233008] ccp 0000:a2:00.1: DEVRES REL 00000000ae90f983 0xffff8d09dc25a800 devm_kzalloc_release (184 bytes)
[  218.245251] ccp 0000:23:00.1: DEVRES REL 00000000a2ec0085 0xffff8cfa86bee700 fw_name_devm_release (16 bytes)
[  218.256748] ccp 0000:23:00.1: DEVRES REL 0000000021bccd98 0xffff8cfaa528d5c0 devm_pages_release (16 bytes)
[  218.268044] ccp 0000:23:00.1: DEVRES REL 000000003ef7cbc7 0xffff8cfaa1b5ec00 devm_kzalloc_release (104 bytes)
[  218.279631] ccp 0000:23:00.1: DEVRES REL 00000000619322e1 0xffff8cfaa1b5e480 devm_kzalloc_release (152 bytes)
[  218.300438] ccp 0000:23:00.1: DEVRES REL 00000000c261523b 0xffff8cfaad88b000 devm_kzalloc_release (3072 bytes)
[  218.331000] ccp 0000:23:00.1: DEVRES REL 00000000fbd19618 0xffff8cfaa528d140 pcim_intx_restore (4 bytes)
[  218.361330] ccp 0000:23:00.1: DEVRES ADD 0000000057f8e767 0xffff8cfa867b7740 pcim_intx_restore (4 bytes)
[  218.391226] ccp 0000:23:00.1: DEVRES REL 0000000058c9dce1 0xffff8cfaa528d880 pcim_msi_release (16 bytes)
[  218.421340] ccp 0000:23:00.1: DEVRES REL 00000000c8ab08a7 0xffff8cfa9e617300 msi_device_data_release (80 bytes)
[  218.452357] ccp 0000:23:00.1: DEVRES REL 00000000cf5baccb 0xffff8cfaa528d8c0 devm_attr_group_remove (8 bytes)
[  218.483011] ccp 0000:23:00.1: DEVRES REL 00000000b8cbbadd 0xffff8cfa9c596060 pcim_addr_resource_release (40 bytes)
[  218.514343] ccp 0000:23:00.1: DEVRES REL 00000000920f9607 0xffff8cfa9c596c60 pcim_iomap_release (48 bytes)
[  218.544659] ccp 0000:23:00.1: DEVRES REL 00000000d401a708 0xffff8cfa9c596840 pcim_addr_resource_release (40 bytes)
[  218.575774] ccp 0000:23:00.1: DEVRES REL 00000000865d2fa2 0xffff8cfaa528d940 pcim_release (0 bytes)
[  218.605758] ccp 0000:23:00.1: DEVRES REL 00000000f5b79222 0xffff8cfaa528d080 devm_kzalloc_release (20 bytes)
[  218.636260] ccp 0000:23:00.1: DEVRES REL 0000000037ef240a 0xffff8cfa9eeb3f00 devm_kzalloc_release (184 bytes)

and the CCP driver reload issue during driver probe:

[  226.552684] pci 0000:23:00.1: Resources present before probing
[  226.568846] pci 0000:a2:00.1: Resources present before probing

From the above DEVRES logging, it looks like pcim_intx_restore associated resource is being released but then
being re-added during detach/unload, which causes really_probe() to fail at probe time, as dev->devres_head is
not empty due to this added resource:
...
[  218.331000] ccp 0000:23:00.1: DEVRES REL 00000000fbd19618 0xffff8cfaa528d140 pcim_intx_restore (4 bytes)
[  218.361330] ccp 0000:23:00.1: DEVRES ADD 0000000057f8e767 0xffff8cfa867b7740 pcim_intx_restore (4 bytes)
...

Going more deep into this: 

This is the initial pcim_intx_resoure associated resource being added during first (CCP) driver load:

[   40.418933]  pcim_intx+0x3a/0x120
[   40.418936]  pci_intx+0x8b/0xa0
[   40.418939]  __pci_enable_msix_range+0x369/0x530
[   40.418943]  pci_enable_msix_range+0x18/0x20
[   40.418946]  sp_pci_probe+0x106/0x310 [ccp]
[   40.418965] ipmi device interface
[   40.418960]  ? srso_alias_return_thunk+0x5/0xfbef5
[   40.418969]  local_pci_probe+0x4f/0xb0
[   40.418973]  work_for_cpu_fn+0x1e/0x30
[   40.418976]  process_one_work+0x183/0x350
[   40.418980]  worker_thread+0x2df/0x3f0
[   40.418982]  ? __pfx_worker_thread+0x10/0x10
[   40.418985]  kthread+0xd0/0x100
[   40.418987]  ? __pfx_kthread+0x10/0x10
[   40.418990]  ret_from_fork+0x40/0x60
[   40.418993]  ? __pfx_kthread+0x10/0x10
[   40.418996]  ret_from_fork_asm+0x1a/0x30
[   40.419001]  </TASK>
..
..
[   40.419012] ccp 0000:23:00.1: DEVRES ADD 00000000fbd19618 0xffff8cfaa528d140 pcim_intx_restore (4 bytes)

Now, at driver unload: 
devres_release_all() -> remove_nodes() -> release_nodes() ...

remove_nodes() moves normal devres entries to the todo list, as can be seen with the following log:
...
[  218.245241] moving node 00000000fbd19618 0xffff8cfaa528d140 from devres to todo list
...

So, now this pcim_intx_resource associated resource is no longer part of dev->devres_head list and has been
moved to the todo list.

Later, when release_nodes() is invoked, it calls the associated release() callback associated with this devres:
...
[  218.331000] ccp 0000:23:00.1: DEVRES REL 00000000fbd19618 0xffff8cfaa528d140 pcim_intx_restore (4 bytes)
...

The call flow for that is:
pcim_intx_restore() -> pci_intx() -> pcim_intx() ...

Now, pcim_intx() calls get_or_create_intx_devres() which tries to find it's associated devres using devres_find(), but 
that fails to find the devres, as the devres is no longer on dev->devres_head and has been moved to todo list.

Therefore, get_or_create_intx_devres() adds a new devres at driver unload/detach time:
...
[  218.361330] ccp 0000:23:00.1: DEVRES ADD 0000000057f8e767 0xffff8cfa867b7740 pcim_intx_restore (4 bytes)
...

But, then this is an issue as pcim_intx() is supposed to restore the original PCI INTx state on driver detach, but it now
operating on a newly added devres and not the original devres (added at driver probe) which contains the original PCI INTx
state, so it will be restoring an incorrect PCI INTx state ?

Additionally, this newly added devres causes driver reload/probe failure as really_probe() now finds resources present
before probing.

Not sure, if this issue has been observed with other PCI device drivers.

Thanks,
Ashish

Re: [PATCH v9 10/13] PCI: Give pci_intx() its own devres callback

[Philipp Stanner]

@Bjorn, @Krzysztof

On Mon, 2024-07-08 at 21:46 +0000, Ashish Kalra wrote:
> With this patch applied, we are observing unloading and then
> reloading issues with the AMD Crypto (CCP) driver:

Thank you very much for digging into this, Ashish

Could you give me some pointers how one could test CCP by himself?

> 
> with DEVRES logging enabled, we observe the following logs:
> 
> [  218.093588] ccp 0000:a2:00.1: DEVRES REL 00000000c18c52fb
> 0xffff8d09dc1972c0 devm_kzalloc_release (152 bytes)
> [  218.105527] ccp 0000:a2:00.1: DEVRES REL 000000003091fb95
> 0xffff8d09d3aad000 devm_kzalloc_release (3072 bytes)
> [  218.117500] ccp 0000:a2:00.1: DEVRES REL 0000000049e4adfe
> 0xffff8d09d588f000 pcim_intx_restore (4 bytes)
> [  218.129519] ccp 0000:a2:00.1: DEVRES ADD 000000001a2ac6ad
> 0xffff8cfa867b7cc0 pcim_intx_restore (4 bytes)
> [  218.140434] ccp 0000:a2:00.1: DEVRES REL 00000000627ecaf7
> 0xffff8d09d588f680 pcim_msi_release (16 bytes)
> [  218.151665] ccp 0000:a2:00.1: DEVRES REL 0000000058b2252a
> 0xffff8d09dc199680 msi_device_data_release (80 bytes)
> [  218.163625] ccp 0000:a2:00.1: DEVRES REL 00000000435cc85e
> 0xffff8d09d588ff80 devm_attr_group_remove (8 bytes)
> [  218.175224] ccp 0000:a2:00.1: DEVRES REL 00000000cb6fcd9b
> 0xffff8d09eb583660 pcim_addr_resource_release (40 bytes)
> [  218.187319] ccp 0000:a2:00.1: DEVRES REL 00000000d64a8b84
> 0xffff8d09eb583180 pcim_iomap_release (48 bytes)
> [  218.198615] ccp 0000:a2:00.1: DEVRES REL 0000000099ac6b28
> 0xffff8d09eb5830c0 pcim_addr_resource_release (40 bytes)
> [  218.210730] ccp 0000:a2:00.1: DEVRES REL 00000000bdd27f88
> 0xffff8d09d3ac2700 pcim_release (0 bytes)
> [  218.221489] ccp 0000:a2:00.1: DEVRES REL 00000000e763315c
> 0xffff8d09d3ac2240 devm_kzalloc_release (20 bytes)
> [  218.233008] ccp 0000:a2:00.1: DEVRES REL 00000000ae90f983
> 0xffff8d09dc25a800 devm_kzalloc_release (184 bytes)
> [  218.245251] ccp 0000:23:00.1: DEVRES REL 00000000a2ec0085
> 0xffff8cfa86bee700 fw_name_devm_release (16 bytes)
> [  218.256748] ccp 0000:23:00.1: DEVRES REL 0000000021bccd98
> 0xffff8cfaa528d5c0 devm_pages_release (16 bytes)
> [  218.268044] ccp 0000:23:00.1: DEVRES REL 000000003ef7cbc7
> 0xffff8cfaa1b5ec00 devm_kzalloc_release (104 bytes)
> [  218.279631] ccp 0000:23:00.1: DEVRES REL 00000000619322e1
> 0xffff8cfaa1b5e480 devm_kzalloc_release (152 bytes)
> [  218.300438] ccp 0000:23:00.1: DEVRES REL 00000000c261523b
> 0xffff8cfaad88b000 devm_kzalloc_release (3072 bytes)
> [  218.331000] ccp 0000:23:00.1: DEVRES REL 00000000fbd19618
> 0xffff8cfaa528d140 pcim_intx_restore (4 bytes)
> [  218.361330] ccp 0000:23:00.1: DEVRES ADD 0000000057f8e767
> 0xffff8cfa867b7740 pcim_intx_restore (4 bytes)
> [  218.391226] ccp 0000:23:00.1: DEVRES REL 0000000058c9dce1
> 0xffff8cfaa528d880 pcim_msi_release (16 bytes)
> [  218.421340] ccp 0000:23:00.1: DEVRES REL 00000000c8ab08a7
> 0xffff8cfa9e617300 msi_device_data_release (80 bytes)
> [  218.452357] ccp 0000:23:00.1: DEVRES REL 00000000cf5baccb
> 0xffff8cfaa528d8c0 devm_attr_group_remove (8 bytes)
> [  218.483011] ccp 0000:23:00.1: DEVRES REL 00000000b8cbbadd
> 0xffff8cfa9c596060 pcim_addr_resource_release (40 bytes)
> [  218.514343] ccp 0000:23:00.1: DEVRES REL 00000000920f9607
> 0xffff8cfa9c596c60 pcim_iomap_release (48 bytes)
> [  218.544659] ccp 0000:23:00.1: DEVRES REL 00000000d401a708
> 0xffff8cfa9c596840 pcim_addr_resource_release (40 bytes)
> [  218.575774] ccp 0000:23:00.1: DEVRES REL 00000000865d2fa2
> 0xffff8cfaa528d940 pcim_release (0 bytes)
> [  218.605758] ccp 0000:23:00.1: DEVRES REL 00000000f5b79222
> 0xffff8cfaa528d080 devm_kzalloc_release (20 bytes)
> [  218.636260] ccp 0000:23:00.1: DEVRES REL 0000000037ef240a
> 0xffff8cfa9eeb3f00 devm_kzalloc_release (184 bytes)
> 
> and the CCP driver reload issue during driver probe:
> 
> [  226.552684] pci 0000:23:00.1: Resources present before probing
> [  226.568846] pci 0000:a2:00.1: Resources present before probing
> 
> From the above DEVRES logging, it looks like pcim_intx_restore
> associated resource is being released but then
> being re-added during detach/unload, which causes really_probe() to
> fail at probe time, as dev->devres_head is
> not empty due to this added resource:
> ...
> [  218.331000] ccp 0000:23:00.1: DEVRES REL 00000000fbd19618
> 0xffff8cfaa528d140 pcim_intx_restore (4 bytes)
> [  218.361330] ccp 0000:23:00.1: DEVRES ADD 0000000057f8e767
> 0xffff8cfa867b7740 pcim_intx_restore (4 bytes)
> ...
> 
> Going more deep into this: 
> 
> This is the initial pcim_intx_resoure associated resource being added
> during first (CCP) driver load:
> 
> [   40.418933]  pcim_intx+0x3a/0x120
> [   40.418936]  pci_intx+0x8b/0xa0
> [   40.418939]  __pci_enable_msix_range+0x369/0x530
> [   40.418943]  pci_enable_msix_range+0x18/0x20
> [   40.418946]  sp_pci_probe+0x106/0x310 [ccp]
> [   40.418965] ipmi device interface
> [   40.418960]  ? srso_alias_return_thunk+0x5/0xfbef5
> [   40.418969]  local_pci_probe+0x4f/0xb0
> [   40.418973]  work_for_cpu_fn+0x1e/0x30
> [   40.418976]  process_one_work+0x183/0x350
> [   40.418980]  worker_thread+0x2df/0x3f0
> [   40.418982]  ? __pfx_worker_thread+0x10/0x10
> [   40.418985]  kthread+0xd0/0x100
> [   40.418987]  ? __pfx_kthread+0x10/0x10
> [   40.418990]  ret_from_fork+0x40/0x60
> [   40.418993]  ? __pfx_kthread+0x10/0x10
> [   40.418996]  ret_from_fork_asm+0x1a/0x30
> [   40.419001]  </TASK>
> ..
> ..
> [   40.419012] ccp 0000:23:00.1: DEVRES ADD 00000000fbd19618
> 0xffff8cfaa528d140 pcim_intx_restore (4 bytes)
> 
> Now, at driver unload: 
> devres_release_all() -> remove_nodes() -> release_nodes() ...
> 
> remove_nodes() moves normal devres entries to the todo list, as can
> be seen with the following log:
> ...
> [  218.245241] moving node 00000000fbd19618 0xffff8cfaa528d140 from
> devres to todo list
> ...
> 
> So, now this pcim_intx_resource associated resource is no longer part
> of dev->devres_head list and has been
> moved to the todo list.
> 
> Later, when release_nodes() is invoked, it calls the associated
> release() callback associated with this devres:
> ...
> [  218.331000] ccp 0000:23:00.1: DEVRES REL 00000000fbd19618
> 0xffff8cfaa528d140 pcim_intx_restore (4 bytes)
> ...
> 
> The call flow for that is:
> pcim_intx_restore() -> pci_intx() -> pcim_intx() ...
> 
> Now, pcim_intx() calls get_or_create_intx_devres() which tries to
> find it's associated devres using devres_find(), but 
> that fails to find the devres, as the devres is no longer on dev-
> >devres_head and has been moved to todo list.
> 
> Therefore, get_or_create_intx_devres() adds a new devres at driver
> unload/detach time:
> ...
> [  218.361330] ccp 0000:23:00.1: DEVRES ADD 0000000057f8e767
> 0xffff8cfa867b7740 pcim_intx_restore (4 bytes)
> ...

You're absolutely right, that seems to be the issue precisely. In fact,
this problem of PCI hybrid functions calling themselves again even
forced me to implement a "pure unmanaged" version of
__pci_request_region(). So it's a pity that I didn't think of that
problem for pci_intx().

> 
> But, then this is an issue as pcim_intx() is supposed to restore the
> original PCI INTx state on driver detach, but it now
> operating on a newly added devres and not the original devres (added
> at driver probe) which contains the original PCI INTx
> state, so it will be restoring an incorrect PCI INTx state ?

I think this is just UB and we don't have to think about whether it's
the correct state or not – it must only be restored once, so it's
broken in any case.

> 
> Additionally, this newly added devres causes driver reload/probe
> failure as really_probe() now finds resources present
> before probing.

Yes, that has to be separated.

@Bjorn:
So I think the solution will be not to call into pci_intx() from
pcim_intx_restore() at all anymore.

Similar as we do with __pci_request_region() <-> __pcim_request_region().

Let me dig into that..

I guess you'll prefer me to send a fixup commit to squash into the
pcim_intx() commit?

I'm quite busy today, but will definitely deliver that quite soon.

> 
> Not sure, if this issue has been observed with other PCI device
> drivers.

Everyone using pci_intx() AND pcim_enable_device() will have this
issue.

The only thing I'm wondering about is where your code in
drivers/crypto/ccp/ calls into pci_intx()?


Regards,
P.

> 
> Thanks,
> Ashish
> 

Re: [PATCH v9 10/13] PCI: Give pci_intx() its own devres callback

[Kalra, Ashish]

Hello Philipp,

On 7/9/2024 2:21 AM, Philipp Stanner wrote:
> @Bjorn, @Krzysztof
>
> On Mon, 2024-07-08 at 21:46 +0000, Ashish Kalra wrote:
>> With this patch applied, we are observing unloading and then
>> reloading issues with the AMD Crypto (CCP) driver:
> Thank you very much for digging into this, Ashish
>
> Could you give me some pointers how one could test CCP by himself?
>
>> with DEVRES logging enabled, we observe the following logs:
>>
>> [  218.093588] ccp 0000:a2:00.1: DEVRES REL 00000000c18c52fb
>> 0xffff8d09dc1972c0 devm_kzalloc_release (152 bytes)
>> [  218.105527] ccp 0000:a2:00.1: DEVRES REL 000000003091fb95
>> 0xffff8d09d3aad000 devm_kzalloc_release (3072 bytes)
>> [  218.117500] ccp 0000:a2:00.1: DEVRES REL 0000000049e4adfe
>> 0xffff8d09d588f000 pcim_intx_restore (4 bytes)
>> [  218.129519] ccp 0000:a2:00.1: DEVRES ADD 000000001a2ac6ad
>> 0xffff8cfa867b7cc0 pcim_intx_restore (4 bytes)
>> [  218.140434] ccp 0000:a2:00.1: DEVRES REL 00000000627ecaf7
>> 0xffff8d09d588f680 pcim_msi_release (16 bytes)
>> [  218.151665] ccp 0000:a2:00.1: DEVRES REL 0000000058b2252a
>> 0xffff8d09dc199680 msi_device_data_release (80 bytes)
>> [  218.163625] ccp 0000:a2:00.1: DEVRES REL 00000000435cc85e
>> 0xffff8d09d588ff80 devm_attr_group_remove (8 bytes)
>> [  218.175224] ccp 0000:a2:00.1: DEVRES REL 00000000cb6fcd9b
>> 0xffff8d09eb583660 pcim_addr_resource_release (40 bytes)
>> [  218.187319] ccp 0000:a2:00.1: DEVRES REL 00000000d64a8b84
>> 0xffff8d09eb583180 pcim_iomap_release (48 bytes)
>> [  218.198615] ccp 0000:a2:00.1: DEVRES REL 0000000099ac6b28
>> 0xffff8d09eb5830c0 pcim_addr_resource_release (40 bytes)
>> [  218.210730] ccp 0000:a2:00.1: DEVRES REL 00000000bdd27f88
>> 0xffff8d09d3ac2700 pcim_release (0 bytes)
>> [  218.221489] ccp 0000:a2:00.1: DEVRES REL 00000000e763315c
>> 0xffff8d09d3ac2240 devm_kzalloc_release (20 bytes)
>> [  218.233008] ccp 0000:a2:00.1: DEVRES REL 00000000ae90f983
>> 0xffff8d09dc25a800 devm_kzalloc_release (184 bytes)
>> [  218.245251] ccp 0000:23:00.1: DEVRES REL 00000000a2ec0085
>> 0xffff8cfa86bee700 fw_name_devm_release (16 bytes)
>> [  218.256748] ccp 0000:23:00.1: DEVRES REL 0000000021bccd98
>> 0xffff8cfaa528d5c0 devm_pages_release (16 bytes)
>> [  218.268044] ccp 0000:23:00.1: DEVRES REL 000000003ef7cbc7
>> 0xffff8cfaa1b5ec00 devm_kzalloc_release (104 bytes)
>> [  218.279631] ccp 0000:23:00.1: DEVRES REL 00000000619322e1
>> 0xffff8cfaa1b5e480 devm_kzalloc_release (152 bytes)
>> [  218.300438] ccp 0000:23:00.1: DEVRES REL 00000000c261523b
>> 0xffff8cfaad88b000 devm_kzalloc_release (3072 bytes)
>> [  218.331000] ccp 0000:23:00.1: DEVRES REL 00000000fbd19618
>> 0xffff8cfaa528d140 pcim_intx_restore (4 bytes)
>> [  218.361330] ccp 0000:23:00.1: DEVRES ADD 0000000057f8e767
>> 0xffff8cfa867b7740 pcim_intx_restore (4 bytes)
>> [  218.391226] ccp 0000:23:00.1: DEVRES REL 0000000058c9dce1
>> 0xffff8cfaa528d880 pcim_msi_release (16 bytes)
>> [  218.421340] ccp 0000:23:00.1: DEVRES REL 00000000c8ab08a7
>> 0xffff8cfa9e617300 msi_device_data_release (80 bytes)
>> [  218.452357] ccp 0000:23:00.1: DEVRES REL 00000000cf5baccb
>> 0xffff8cfaa528d8c0 devm_attr_group_remove (8 bytes)
>> [  218.483011] ccp 0000:23:00.1: DEVRES REL 00000000b8cbbadd
>> 0xffff8cfa9c596060 pcim_addr_resource_release (40 bytes)
>> [  218.514343] ccp 0000:23:00.1: DEVRES REL 00000000920f9607
>> 0xffff8cfa9c596c60 pcim_iomap_release (48 bytes)
>> [  218.544659] ccp 0000:23:00.1: DEVRES REL 00000000d401a708
>> 0xffff8cfa9c596840 pcim_addr_resource_release (40 bytes)
>> [  218.575774] ccp 0000:23:00.1: DEVRES REL 00000000865d2fa2
>> 0xffff8cfaa528d940 pcim_release (0 bytes)
>> [  218.605758] ccp 0000:23:00.1: DEVRES REL 00000000f5b79222
>> 0xffff8cfaa528d080 devm_kzalloc_release (20 bytes)
>> [  218.636260] ccp 0000:23:00.1: DEVRES REL 0000000037ef240a
>> 0xffff8cfa9eeb3f00 devm_kzalloc_release (184 bytes)
>>
>> and the CCP driver reload issue during driver probe:
>>
>> [  226.552684] pci 0000:23:00.1: Resources present before probing
>> [  226.568846] pci 0000:a2:00.1: Resources present before probing
>>
>> From the above DEVRES logging, it looks like pcim_intx_restore
>> associated resource is being released but then
>> being re-added during detach/unload, which causes really_probe() to
>> fail at probe time, as dev->devres_head is
>> not empty due to this added resource:
>> ...
>> [  218.331000] ccp 0000:23:00.1: DEVRES REL 00000000fbd19618
>> 0xffff8cfaa528d140 pcim_intx_restore (4 bytes)
>> [  218.361330] ccp 0000:23:00.1: DEVRES ADD 0000000057f8e767
>> 0xffff8cfa867b7740 pcim_intx_restore (4 bytes)
>> ...
>>
>> Going more deep into this: 
>>
>> This is the initial pcim_intx_resoure associated resource being added
>> during first (CCP) driver load:
>>
>> [   40.418933]  pcim_intx+0x3a/0x120
>> [   40.418936]  pci_intx+0x8b/0xa0
>> [   40.418939]  __pci_enable_msix_range+0x369/0x530
>> [   40.418943]  pci_enable_msix_range+0x18/0x20
>> [   40.418946]  sp_pci_probe+0x106/0x310 [ccp]
>> [   40.418965] ipmi device interface
>> [   40.418960]  ? srso_alias_return_thunk+0x5/0xfbef5
>> [   40.418969]  local_pci_probe+0x4f/0xb0
>> [   40.418973]  work_for_cpu_fn+0x1e/0x30
>> [   40.418976]  process_one_work+0x183/0x350
>> [   40.418980]  worker_thread+0x2df/0x3f0
>> [   40.418982]  ? __pfx_worker_thread+0x10/0x10
>> [   40.418985]  kthread+0xd0/0x100
>> [   40.418987]  ? __pfx_kthread+0x10/0x10
>> [   40.418990]  ret_from_fork+0x40/0x60
>> [   40.418993]  ? __pfx_kthread+0x10/0x10
>> [   40.418996]  ret_from_fork_asm+0x1a/0x30
>> [   40.419001]  </TASK>
>> ..
>> ..
>> [   40.419012] ccp 0000:23:00.1: DEVRES ADD 00000000fbd19618
>> 0xffff8cfaa528d140 pcim_intx_restore (4 bytes)
>>
>> Now, at driver unload: 
>> devres_release_all() -> remove_nodes() -> release_nodes() ...
>>
>> remove_nodes() moves normal devres entries to the todo list, as can
>> be seen with the following log:
>> ...
>> [  218.245241] moving node 00000000fbd19618 0xffff8cfaa528d140 from
>> devres to todo list
>> ...
>>
>> So, now this pcim_intx_resource associated resource is no longer part
>> of dev->devres_head list and has been
>> moved to the todo list.
>>
>> Later, when release_nodes() is invoked, it calls the associated
>> release() callback associated with this devres:
>> ...
>> [  218.331000] ccp 0000:23:00.1: DEVRES REL 00000000fbd19618
>> 0xffff8cfaa528d140 pcim_intx_restore (4 bytes)
>> ...
>>
>> The call flow for that is:
>> pcim_intx_restore() -> pci_intx() -> pcim_intx() ...
>>
>> Now, pcim_intx() calls get_or_create_intx_devres() which tries to
>> find it's associated devres using devres_find(), but 
>> that fails to find the devres, as the devres is no longer on dev-
>>> devres_head and has been moved to todo list.
>> Therefore, get_or_create_intx_devres() adds a new devres at driver
>> unload/detach time:
>> ...
>> [  218.361330] ccp 0000:23:00.1: DEVRES ADD 0000000057f8e767
>> 0xffff8cfa867b7740 pcim_intx_restore (4 bytes)
>> ...
> You're absolutely right, that seems to be the issue precisely. In fact,
> this problem of PCI hybrid functions calling themselves again even
> forced me to implement a "pure unmanaged" version of
> __pci_request_region(). So it's a pity that I didn't think of that
> problem for pci_intx().
>
>> But, then this is an issue as pcim_intx() is supposed to restore the
>> original PCI INTx state on driver detach, but it now
>> operating on a newly added devres and not the original devres (added
>> at driver probe) which contains the original PCI INTx
>> state, so it will be restoring an incorrect PCI INTx state ?
> I think this is just UB and we don't have to think about whether it's
> the correct state or not – it must only be restored once, so it's
> broken in any case.
>
>> Additionally, this newly added devres causes driver reload/probe
>> failure as really_probe() now finds resources present
>> before probing.
> Yes, that has to be separated.
>
> @Bjorn:
> So I think the solution will be not to call into pci_intx() from
> pcim_intx_restore() at all anymore.
>
> Similar as we do with __pci_request_region() <-> __pcim_request_region().
>
> Let me dig into that..
>
> I guess you'll prefer me to send a fixup commit to squash into the
> pcim_intx() commit?
>
> I'm quite busy today, but will definitely deliver that quite soon.
>
>> Not sure, if this issue has been observed with other PCI device
>> drivers.
> Everyone using pci_intx() AND pcim_enable_device() will have this
> issue.
>
> The only thing I'm wondering about is where your code in
> drivers/crypto/ccp/ calls into pci_intx()?
>
Actually, the CCP driver does not explicitly call into pci_intx(), the flow to pci_intx() from CCP driver probe is as follows:

[   40.418933]  pcim_intx+0x3a/0x120
[   40.418936]  pci_intx+0x8b/0xa0
[   40.418939]  __pci_enable_msix_range+0x369/0x530
[   40.418943]  pci_enable_msix_range+0x18/0x20
[   40.418946]  sp_pci_probe+0x106/0x310 [ccp]
[   40.418965] ipmi device interface
[   40.418960]  ? srso_alias_return_thunk+0x5/0xfbef5
[   40.418969]  local_pci_probe+0x4f/0xb0

And obviously, pci_intx()->pcim_intx() get invoked due to pcim_intx_restore() callback being invoked during sp_pci_exit() code path, as below:

[  218.128978]  pcim_intx+0x3a/0x120
[  218.128986]  ? srso_alias_return_thunk+0x5/0xfbef5
[  218.128999]  pci_intx+0x8b/0xa0
[  218.129010]  pcim_intx_restore+0x1b/0x30
[  218.129019]  release_nodes+0x65/0x90
[  218.129031]  devres_release_all+0x9f/0xe0
[  218.129043]  device_unbind_cleanup+0x12/0x80
[  218.129055]  device_release_driver_internal+0x20c/0x250
[  218.129065]  ? srso_alias_return_thunk+0x5/0xfbef5
[  218.129078]  driver_detach+0x4f/0xa0
[  218.129091]  bus_remove_driver+0x87/0x100
[  218.129101]  driver_unregister+0x35/0x60
[  218.129113]  pci_unregister_driver+0x44/0xa0
[  218.129123]  sp_pci_exit+0x19/0x20 [ccp]
[  218.129137]  sp_mod_exit+0x12/0x18 [ccp]

...

Basically, CCP driver calls pci_enable_msix_range() and looking at this function:

pci_enable_msix_range() -> __pci_enable_msix_range() -> msix_capability_init().

And, msix_capability_init() -> pci_intx_for_msi() -> pci_intx().

So it looks like drivers using MSI-X/MSI (using API such as pci_enable_msix_range()) and pcim_enable_device() will get into this issue without explicitly calling into pci_intx().

Thanks, Ashish

> Regards,
> P.
>
>> Thanks,
>> Ashish
>>

Re: [PATCH v9 10/13] PCI: Give pci_intx() its own devres callback

[Philipp Stanner]

From c24bd5b66e798a341caf183fb7cdbdf235502d90 Mon Sep 17 00:00:00 2001
From: Philipp Stanner <pstanner@redhat.com>
Date: Tue, 9 Jul 2024 09:45:48 +0200
Subject: [PATCH] PCI: Fix pcim_intx() recursive calls

pci_intx() calls into pcim_intx() in managed mode, i.e., when
pcim_enable_device() had been called. This recursive call causes a bug
by re-registering the device resource in the release callback.

This is the same phenomenon that made it necessary to implement some
functionality a second time, see __pcim_request_region().

Implement __pcim_intx() to bypass the hybrid nature of pci_intx() on
driver detach.

Fixes: https://lore.kernel.org/all/20240708214656.4721-1-Ashish.Kalra@amd.com/
Reported-by: Ashish Kalra <Ashish.Kalra@amd.com>
Signed-off-by: Philipp Stanner <pstanner@redhat.com>
---
Hi Ashish,
I hacked down this fix that should be applyable on top.
Could you maybe have a first quick look whether this fixes the issue?
---
 drivers/pci/devres.c | 33 +++++++++++++++++++++------------
 1 file changed, 21 insertions(+), 12 deletions(-)

diff --git a/drivers/pci/devres.c b/drivers/pci/devres.c
index 2f0379a4e58f..dcef049b72fe 100644
--- a/drivers/pci/devres.c
+++ b/drivers/pci/devres.c
@@ -408,12 +408,31 @@ static inline bool mask_contains_bar(int mask, int bar)
 	return mask & BIT(bar);
 }
 
+/*
+ * This is a copy of pci_intx() used to bypass the problem of occuring
+ * recursive function calls due to the hybrid nature of pci_intx().
+ */
+static void __pcim_intx(struct pci_dev *pdev, int enable)
+{
+	u16 pci_command, new;
+
+	pci_read_config_word(pdev, PCI_COMMAND, &pci_command);
+
+	if (enable)
+		new = pci_command & ~PCI_COMMAND_INTX_DISABLE;
+	else
+		new = pci_command | PCI_COMMAND_INTX_DISABLE;
+
+	if (new != pci_command)
+		pci_write_config_word(pdev, PCI_COMMAND, new);
+}
+
 static void pcim_intx_restore(struct device *dev, void *data)
 {
 	struct pci_dev *pdev = to_pci_dev(dev);
 	struct pcim_intx_devres *res = data;
 
-	pci_intx(pdev, res->orig_intx);
+	__pcim_intx(pdev, res->orig_intx);
 }
 
 static struct pcim_intx_devres *get_or_create_intx_devres(struct device *dev)
@@ -443,7 +462,6 @@ static struct pcim_intx_devres *get_or_create_intx_devres(struct device *dev)
  */
 int pcim_intx(struct pci_dev *pdev, int enable)
 {
-	u16 pci_command, new;
 	struct pcim_intx_devres *res;
 
 	res = get_or_create_intx_devres(&pdev->dev);
@@ -451,16 +469,7 @@ int pcim_intx(struct pci_dev *pdev, int enable)
 		return -ENOMEM;
 
 	res->orig_intx = !enable;
-
-	pci_read_config_word(pdev, PCI_COMMAND, &pci_command);
-
-	if (enable)
-		new = pci_command & ~PCI_COMMAND_INTX_DISABLE;
-	else
-		new = pci_command | PCI_COMMAND_INTX_DISABLE;
-
-	if (new != pci_command)
-		pci_write_config_word(pdev, PCI_COMMAND, new);
+	__pcim_intx(pdev, enable);
 
 	return 0;
 }
-- 
2.45.0

Re: [PATCH v9 10/13] PCI: Give pci_intx() its own devres callback

[Kalra, Ashish]

Hello Philipp,

I have reviewed and tested this patch, looks to be working fine and fixes the issue.

Thanks, Ashish

On 7/9/2024 3:56 AM, Philipp Stanner wrote:
> From c24bd5b66e798a341caf183fb7cdbdf235502d90 Mon Sep 17 00:00:00 2001
> From: Philipp Stanner <pstanner@redhat.com>
> Date: Tue, 9 Jul 2024 09:45:48 +0200
> Subject: [PATCH] PCI: Fix pcim_intx() recursive calls
>
> pci_intx() calls into pcim_intx() in managed mode, i.e., when
> pcim_enable_device() had been called. This recursive call causes a bug
> by re-registering the device resource in the release callback.
>
> This is the same phenomenon that made it necessary to implement some
> functionality a second time, see __pcim_request_region().
>
> Implement __pcim_intx() to bypass the hybrid nature of pci_intx() on
> driver detach.
>
> Fixes: https://lore.kernel.org/all/20240708214656.4721-1-Ashish.Kalra@amd.com/
> Reported-by: Ashish Kalra <Ashish.Kalra@amd.com>
> Signed-off-by: Philipp Stanner <pstanner@redhat.com>
> ---
> Hi Ashish,
> I hacked down this fix that should be applyable on top.
> Could you maybe have a first quick look whether this fixes the issue?
> ---
>  drivers/pci/devres.c | 33 +++++++++++++++++++++------------
>  1 file changed, 21 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/pci/devres.c b/drivers/pci/devres.c
> index 2f0379a4e58f..dcef049b72fe 100644
> --- a/drivers/pci/devres.c
> +++ b/drivers/pci/devres.c
> @@ -408,12 +408,31 @@ static inline bool mask_contains_bar(int mask, int bar)
>  	return mask & BIT(bar);
>  }
>  
> +/*
> + * This is a copy of pci_intx() used to bypass the problem of occuring
> + * recursive function calls due to the hybrid nature of pci_intx().
> + */
> +static void __pcim_intx(struct pci_dev *pdev, int enable)
> +{
> +	u16 pci_command, new;
> +
> +	pci_read_config_word(pdev, PCI_COMMAND, &pci_command);
> +
> +	if (enable)
> +		new = pci_command & ~PCI_COMMAND_INTX_DISABLE;
> +	else
> +		new = pci_command | PCI_COMMAND_INTX_DISABLE;
> +
> +	if (new != pci_command)
> +		pci_write_config_word(pdev, PCI_COMMAND, new);
> +}
> +
>  static void pcim_intx_restore(struct device *dev, void *data)
>  {
>  	struct pci_dev *pdev = to_pci_dev(dev);
>  	struct pcim_intx_devres *res = data;
>  
> -	pci_intx(pdev, res->orig_intx);
> +	__pcim_intx(pdev, res->orig_intx);
>  }
>  
>  static struct pcim_intx_devres *get_or_create_intx_devres(struct device *dev)
> @@ -443,7 +462,6 @@ static struct pcim_intx_devres *get_or_create_intx_devres(struct device *dev)
>   */
>  int pcim_intx(struct pci_dev *pdev, int enable)
>  {
> -	u16 pci_command, new;
>  	struct pcim_intx_devres *res;
>  
>  	res = get_or_create_intx_devres(&pdev->dev);
> @@ -451,16 +469,7 @@ int pcim_intx(struct pci_dev *pdev, int enable)
>  		return -ENOMEM;
>  
>  	res->orig_intx = !enable;
> -
> -	pci_read_config_word(pdev, PCI_COMMAND, &pci_command);
> -
> -	if (enable)
> -		new = pci_command & ~PCI_COMMAND_INTX_DISABLE;
> -	else
> -		new = pci_command | PCI_COMMAND_INTX_DISABLE;
> -
> -	if (new != pci_command)
> -		pci_write_config_word(pdev, PCI_COMMAND, new);
> +	__pcim_intx(pdev, enable);
>  
>  	return 0;
>  }
Tested-by: Ashish Kalra <Ashish.Kalra@amd.com>

Re: [PATCH v9 10/13] PCI: Give pci_intx() its own devres callback

[Krzysztof Wilczyński]

Hello Ashish and Philipp,

> I have reviewed and tested this patch, looks to be working fine and fixes the issue.

Great news!

Ashish, thank you for taking the time to report the problem and then also
testing the fix.  Much appreciated.

Philipp, I will take this patch and squash into the series you have sent
earlier, since the changes are not yet part of the mainline.

	Krzysztof

Re: [PATCH v9 10/13] PCI: Give pci_intx() its own devres callback

[Krzysztof Wilczyński]

[...]
> pci_intx() calls into pcim_intx() in managed mode, i.e., when
> pcim_enable_device() had been called. This recursive call causes a bug
> by re-registering the device resource in the release callback.
> 
> This is the same phenomenon that made it necessary to implement some
> functionality a second time, see __pcim_request_region().
> 
> Implement __pcim_intx() to bypass the hybrid nature of pci_intx() on
> driver detach.

Squashed against devres branch, thank you!  See:

  https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/commit/?h=devres&id=37c9f6c55cfd63a9e38a98b5aa1d7da75845c2b2

To credit Ashish, I kept the Fixes: and Tested-by: tags.

	Krzysztof

Re: [PATCH v9 02/13] PCI: Add devres helpers for iomap table [resulting in backtraces on HPPA]

[Guenter Roeck]

Hi,

On Thu, Jun 13, 2024 at 01:50:15PM +0200, Philipp Stanner wrote:
> The pcim_iomap_devres.table administrated by pcim_iomap_table() has its
> entries set and unset at several places throughout devres.c using manual
> iterations which are effectively code duplications.
> 
> Add pcim_add_mapping_to_legacy_table() and
> pcim_remove_mapping_from_legacy_table() helper functions and use them where
> possible.
> 
> Link: https://lore.kernel.org/r/20240605081605.18769-4-pstanner@redhat.com
> Signed-off-by: Philipp Stanner <pstanner@redhat.com>
> [bhelgaas: s/short bar/int bar/ for consistency]
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
>  drivers/pci/devres.c | 77 +++++++++++++++++++++++++++++++++-----------
>  1 file changed, 58 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/pci/devres.c b/drivers/pci/devres.c
> index f13edd4a3873..845d6fab0ce7 100644
> --- a/drivers/pci/devres.c
> +++ b/drivers/pci/devres.c
> @@ -297,6 +297,52 @@ void __iomem * const *pcim_iomap_table(struct pci_dev *pdev)
>  }
>  EXPORT_SYMBOL(pcim_iomap_table);
>  
> +/*
> + * Fill the legacy mapping-table, so that drivers using the old API can
> + * still get a BAR's mapping address through pcim_iomap_table().
> + */
> +static int pcim_add_mapping_to_legacy_table(struct pci_dev *pdev,
> +					    void __iomem *mapping, int bar)
> +{
> +	void __iomem **legacy_iomap_table;
> +
> +	if (bar >= PCI_STD_NUM_BARS)
> +		return -EINVAL;
> +
> +	legacy_iomap_table = (void __iomem **)pcim_iomap_table(pdev);
> +	if (!legacy_iomap_table)
> +		return -ENOMEM;
> +
> +	/* The legacy mechanism doesn't allow for duplicate mappings. */
> +	WARN_ON(legacy_iomap_table[bar]);
> +

Ever since this patch has been applied, I see this warning on all hppa
(parisc) systems.

[    0.978177] WARNING: CPU: 0 PID: 1 at drivers/pci/devres.c:473 pcim_add_mapping_to_legacy_table.part.0+0x54/0x80
[    0.978850] Modules linked in:
[    0.979277] CPU: 0 UID: 0 PID: 1 Comm: swapper/0 Not tainted 6.18.0-rc6-64bit+ #1 NONE
[    0.979519] Hardware name: 9000/785/C3700
[    0.979715]
[    0.979768]      YZrvWESTHLNXBCVMcbcbcbcbOGFRQPDI
[    0.979886] PSW: 00001000000001000000000000001111 Not tainted
[    0.980006] r00-03  000000000804000f 00000000414e10a0 0000000040acb300 00000000434b1440
[    0.980167] r04-07  00000000414a78a0 0000000000029000 0000000000000000 0000000043522000
[    0.980314] r08-11  0000000000000000 0000000000000008 0000000000000000 00000000434b0de8
[    0.980461] r12-15  00000000434b11b0 000000004156a8a0 0000000043c655a0 0000000000000000
[    0.980608] r16-19  000000004016e080 000000004019e7d8 0000000000000030 0000000043549780
[    0.981106] r20-23  0000000020000000 0000000000000000 000000000800000e 0000000000000000
[    0.981317] r24-27  0000000000000000 000000000800000f 0000000043522260 00000000414a78a0
[    0.981480] r28-31  00000000436af480 00000000434b1680 00000000434b14d0 0000000000027000
[    0.981641] sr00-03  0000000000000000 0000000000000000 0000000000000000 0000000000000000
[    0.981805] sr04-07  0000000000000000 0000000000000000 0000000000000000 0000000000000000
[    0.981972]
[    0.982024] IASQ: 0000000000000000 0000000000000000 IAOQ: 0000000040acb31c 0000000040acb320
[    0.982185]  IIR: 03ffe01f    ISR: 0000000000000000  IOR: 00000000436af410
[    0.982322]  CPU:        0   CR30: 0000000043549780 CR31: 0000000000000000
[    0.982458]  ORIG_R28: 00000000434b16b0
[    0.982548]  IAOQ[0]: pcim_add_mapping_to_legacy_table.part.0+0x54/0x80
[    0.982733]  IAOQ[1]: pcim_add_mapping_to_legacy_table.part.0+0x58/0x80
[    0.982871]  RP(r2): pcim_add_mapping_to_legacy_table.part.0+0x38/0x80
[    0.983100] Backtrace:
[    0.983439]  [<0000000040acba1c>] pcim_iomap+0xc4/0x170
[    0.983577]  [<0000000040ba3e4c>] serial8250_pci_setup_port+0x8c/0x168
[    0.983725]  [<0000000040ba7588>] setup_port+0x38/0x50
[    0.983837]  [<0000000040ba7d94>] pci_hp_diva_setup+0x8c/0xd8
[    0.983957]  [<0000000040baa47c>] pciserial_init_ports+0x2c4/0x358
[    0.984088]  [<0000000040baa8bc>] pciserial_init_one+0x31c/0x330
[    0.984214]  [<0000000040abfab4>] pci_device_probe+0x194/0x270

Looking into serial8250_pci_setup_port():

        if (pci_resource_flags(dev, bar) & IORESOURCE_MEM) {
                if (!pcim_iomap(dev, bar, 0) && !pcim_iomap_table(dev))
                        return -ENOMEM;

This suggests that the failure is expected. I can see that pcim_iomap_table()
is deprecated, and that one is supposed to use pcim_iomap() instead. However,
pcim_iomap() _is_ alrady used, and I don't see a function which lets the
caller replicate what is done above (attach multiple serial ports to the
same PCI bar).

How would you suggest to fix the problem ?

Thanks,
Guenter

Re: [PATCH v9 02/13] PCI: Add devres helpers for iomap table [resulting in backtraces on HPPA]

[Philipp Stanner]

On Sun, 2025-11-23 at 08:42 -0800, Guenter Roeck wrote:
> Hi,
> 
> On Thu, Jun 13, 2024 at 01:50:15PM +0200, Philipp Stanner wrote:
> > The pcim_iomap_devres.table administrated by pcim_iomap_table() has its
> > entries set and unset at several places throughout devres.c using manual
> > iterations which are effectively code duplications.
> > 
> > Add pcim_add_mapping_to_legacy_table() and
> > pcim_remove_mapping_from_legacy_table() helper functions and use them where
> > possible.
> > 
> > Link: https://lore.kernel.org/r/20240605081605.18769-4-pstanner@redhat.com
> > Signed-off-by: Philipp Stanner <pstanner@redhat.com>
> > [bhelgaas: s/short bar/int bar/ for consistency]
> > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> > ---
> >  drivers/pci/devres.c | 77 +++++++++++++++++++++++++++++++++-----------
> >  1 file changed, 58 insertions(+), 19 deletions(-)
> > 
> > diff --git a/drivers/pci/devres.c b/drivers/pci/devres.c
> > index f13edd4a3873..845d6fab0ce7 100644
> > --- a/drivers/pci/devres.c
> > +++ b/drivers/pci/devres.c
> > @@ -297,6 +297,52 @@ void __iomem * const *pcim_iomap_table(struct pci_dev *pdev)
> >  }
> >  EXPORT_SYMBOL(pcim_iomap_table);
> >  
> > +/*
> > + * Fill the legacy mapping-table, so that drivers using the old API can
> > + * still get a BAR's mapping address through pcim_iomap_table().
> > + */
> > +static int pcim_add_mapping_to_legacy_table(struct pci_dev *pdev,
> > +					    void __iomem *mapping, int bar)
> > +{
> > +	void __iomem **legacy_iomap_table;
> > +
> > +	if (bar >= PCI_STD_NUM_BARS)
> > +		return -EINVAL;
> > +
> > +	legacy_iomap_table = (void __iomem **)pcim_iomap_table(pdev);
> > +	if (!legacy_iomap_table)
> > +		return -ENOMEM;
> > +
> > +	/* The legacy mechanism doesn't allow for duplicate mappings. */
> > +	WARN_ON(legacy_iomap_table[bar]);
> > +
> 
> Ever since this patch has been applied, I see this warning on all hppa
> (parisc) systems.
> 
> [    0.978177] WARNING: CPU: 0 PID: 1 at drivers/pci/devres.c:473 pcim_add_mapping_to_legacy_table.part.0+0x54/0x80
> [    0.978850] Modules linked in:
> [    0.979277] CPU: 0 UID: 0 PID: 1 Comm: swapper/0 Not tainted 6.18.0-rc6-64bit+ #1 NONE
> [    0.979519] Hardware name: 9000/785/C3700
> [    0.979715]
> [    0.979768]      YZrvWESTHLNXBCVMcbcbcbcbOGFRQPDI
> [    0.979886] PSW: 00001000000001000000000000001111 Not tainted
> [    0.980006] r00-03  000000000804000f 00000000414e10a0 0000000040acb300 00000000434b1440
> [    0.980167] r04-07  00000000414a78a0 0000000000029000 0000000000000000 0000000043522000
> [    0.980314] r08-11  0000000000000000 0000000000000008 0000000000000000 00000000434b0de8
> [    0.980461] r12-15  00000000434b11b0 000000004156a8a0 0000000043c655a0 0000000000000000
> [    0.980608] r16-19  000000004016e080 000000004019e7d8 0000000000000030 0000000043549780
> [    0.981106] r20-23  0000000020000000 0000000000000000 000000000800000e 0000000000000000
> [    0.981317] r24-27  0000000000000000 000000000800000f 0000000043522260 00000000414a78a0
> [    0.981480] r28-31  00000000436af480 00000000434b1680 00000000434b14d0 0000000000027000
> [    0.981641] sr00-03  0000000000000000 0000000000000000 0000000000000000 0000000000000000
> [    0.981805] sr04-07  0000000000000000 0000000000000000 0000000000000000 0000000000000000
> [    0.981972]
> [    0.982024] IASQ: 0000000000000000 0000000000000000 IAOQ: 0000000040acb31c 0000000040acb320
> [    0.982185]  IIR: 03ffe01f    ISR: 0000000000000000  IOR: 00000000436af410
> [    0.982322]  CPU:        0   CR30: 0000000043549780 CR31: 0000000000000000
> [    0.982458]  ORIG_R28: 00000000434b16b0
> [    0.982548]  IAOQ[0]: pcim_add_mapping_to_legacy_table.part.0+0x54/0x80
> [    0.982733]  IAOQ[1]: pcim_add_mapping_to_legacy_table.part.0+0x58/0x80
> [    0.982871]  RP(r2): pcim_add_mapping_to_legacy_table.part.0+0x38/0x80
> [    0.983100] Backtrace:
> [    0.983439]  [<0000000040acba1c>] pcim_iomap+0xc4/0x170
> [    0.983577]  [<0000000040ba3e4c>] serial8250_pci_setup_port+0x8c/0x168
> [    0.983725]  [<0000000040ba7588>] setup_port+0x38/0x50
> [    0.983837]  [<0000000040ba7d94>] pci_hp_diva_setup+0x8c/0xd8
> [    0.983957]  [<0000000040baa47c>] pciserial_init_ports+0x2c4/0x358
> [    0.984088]  [<0000000040baa8bc>] pciserial_init_one+0x31c/0x330
> [    0.984214]  [<0000000040abfab4>] pci_device_probe+0x194/0x270
> 
> Looking into serial8250_pci_setup_port():
> 
>         if (pci_resource_flags(dev, bar) & IORESOURCE_MEM) {
>                 if (!pcim_iomap(dev, bar, 0) && !pcim_iomap_table(dev))
>                         return -ENOMEM;

Strange. From the code I see here the WARN_ON in
pcim_add_mapping_to_legacy_table() should not fire. I suspect that it's
actually triggered somewhere else.

> 
> This suggests that the failure is expected. I can see that pcim_iomap_table()
> is deprecated, and that one is supposed to use pcim_iomap() instead. However,
> pcim_iomap() _is_ alrady used, and I don't see a function which lets the
> caller replicate what is done above (attach multiple serial ports to the
> same PCI bar).

Is serial8250_pci_setup_port() invoked in a loop somewhere? Where does
the "attach multiple" happen?

> 
> How would you suggest to fix the problem ?

I suggest you try to remove the `&& pcim_iomap_table(dev)` from above
to see if that's really the cause. pcim_iomap() already creates the
table, and if it succeeds the table has been created with absolute
certainty. The entries will also be present. So the table-check is
surplus.

Report back please if that helps, I'd want to understand what's
happening here.


Regards
P.

Re: [PATCH v9 02/13] PCI: Add devres helpers for iomap table [resulting in backtraces on HPPA]

[Guenter Roeck]

On 11/25/25 07:48, Philipp Stanner wrote:
> On Sun, 2025-11-23 at 08:42 -0800, Guenter Roeck wrote:
>> Hi,
>>
>> On Thu, Jun 13, 2024 at 01:50:15PM +0200, Philipp Stanner wrote:
>>> The pcim_iomap_devres.table administrated by pcim_iomap_table() has its
>>> entries set and unset at several places throughout devres.c using manual
>>> iterations which are effectively code duplications.
>>>
>>> Add pcim_add_mapping_to_legacy_table() and
>>> pcim_remove_mapping_from_legacy_table() helper functions and use them where
>>> possible.
>>>
>>> Link: https://lore.kernel.org/r/20240605081605.18769-4-pstanner@redhat.com
>>> Signed-off-by: Philipp Stanner <pstanner@redhat.com>
>>> [bhelgaas: s/short bar/int bar/ for consistency]
>>> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>>> ---
>>>   drivers/pci/devres.c | 77 +++++++++++++++++++++++++++++++++-----------
>>>   1 file changed, 58 insertions(+), 19 deletions(-)
>>>
>>> diff --git a/drivers/pci/devres.c b/drivers/pci/devres.c
>>> index f13edd4a3873..845d6fab0ce7 100644
>>> --- a/drivers/pci/devres.c
>>> +++ b/drivers/pci/devres.c
>>> @@ -297,6 +297,52 @@ void __iomem * const *pcim_iomap_table(struct pci_dev *pdev)
>>>   }
>>>   EXPORT_SYMBOL(pcim_iomap_table);
>>>   
>>> +/*
>>> + * Fill the legacy mapping-table, so that drivers using the old API can
>>> + * still get a BAR's mapping address through pcim_iomap_table().
>>> + */
>>> +static int pcim_add_mapping_to_legacy_table(struct pci_dev *pdev,
>>> +					    void __iomem *mapping, int bar)
>>> +{
>>> +	void __iomem **legacy_iomap_table;
>>> +
>>> +	if (bar >= PCI_STD_NUM_BARS)
>>> +		return -EINVAL;
>>> +
>>> +	legacy_iomap_table = (void __iomem **)pcim_iomap_table(pdev);
>>> +	if (!legacy_iomap_table)
>>> +		return -ENOMEM;
>>> +
>>> +	/* The legacy mechanism doesn't allow for duplicate mappings. */
>>> +	WARN_ON(legacy_iomap_table[bar]);
>>> +
>>
>> Ever since this patch has been applied, I see this warning on all hppa
>> (parisc) systems.
>>
>> [    0.978177] WARNING: CPU: 0 PID: 1 at drivers/pci/devres.c:473 pcim_add_mapping_to_legacy_table.part.0+0x54/0x80
>> [    0.978850] Modules linked in:
>> [    0.979277] CPU: 0 UID: 0 PID: 1 Comm: swapper/0 Not tainted 6.18.0-rc6-64bit+ #1 NONE
>> [    0.979519] Hardware name: 9000/785/C3700
>> [    0.979715]
>> [    0.979768]      YZrvWESTHLNXBCVMcbcbcbcbOGFRQPDI
>> [    0.979886] PSW: 00001000000001000000000000001111 Not tainted
>> [    0.980006] r00-03  000000000804000f 00000000414e10a0 0000000040acb300 00000000434b1440
>> [    0.980167] r04-07  00000000414a78a0 0000000000029000 0000000000000000 0000000043522000
>> [    0.980314] r08-11  0000000000000000 0000000000000008 0000000000000000 00000000434b0de8
>> [    0.980461] r12-15  00000000434b11b0 000000004156a8a0 0000000043c655a0 0000000000000000
>> [    0.980608] r16-19  000000004016e080 000000004019e7d8 0000000000000030 0000000043549780
>> [    0.981106] r20-23  0000000020000000 0000000000000000 000000000800000e 0000000000000000
>> [    0.981317] r24-27  0000000000000000 000000000800000f 0000000043522260 00000000414a78a0
>> [    0.981480] r28-31  00000000436af480 00000000434b1680 00000000434b14d0 0000000000027000
>> [    0.981641] sr00-03  0000000000000000 0000000000000000 0000000000000000 0000000000000000
>> [    0.981805] sr04-07  0000000000000000 0000000000000000 0000000000000000 0000000000000000
>> [    0.981972]
>> [    0.982024] IASQ: 0000000000000000 0000000000000000 IAOQ: 0000000040acb31c 0000000040acb320
>> [    0.982185]  IIR: 03ffe01f    ISR: 0000000000000000  IOR: 00000000436af410
>> [    0.982322]  CPU:        0   CR30: 0000000043549780 CR31: 0000000000000000
>> [    0.982458]  ORIG_R28: 00000000434b16b0
>> [    0.982548]  IAOQ[0]: pcim_add_mapping_to_legacy_table.part.0+0x54/0x80
>> [    0.982733]  IAOQ[1]: pcim_add_mapping_to_legacy_table.part.0+0x58/0x80
>> [    0.982871]  RP(r2): pcim_add_mapping_to_legacy_table.part.0+0x38/0x80
>> [    0.983100] Backtrace:
>> [    0.983439]  [<0000000040acba1c>] pcim_iomap+0xc4/0x170
>> [    0.983577]  [<0000000040ba3e4c>] serial8250_pci_setup_port+0x8c/0x168
>> [    0.983725]  [<0000000040ba7588>] setup_port+0x38/0x50
>> [    0.983837]  [<0000000040ba7d94>] pci_hp_diva_setup+0x8c/0xd8
>> [    0.983957]  [<0000000040baa47c>] pciserial_init_ports+0x2c4/0x358
>> [    0.984088]  [<0000000040baa8bc>] pciserial_init_one+0x31c/0x330
>> [    0.984214]  [<0000000040abfab4>] pci_device_probe+0x194/0x270
>>
>> Looking into serial8250_pci_setup_port():
>>
>>          if (pci_resource_flags(dev, bar) & IORESOURCE_MEM) {
>>                  if (!pcim_iomap(dev, bar, 0) && !pcim_iomap_table(dev))
>>                          return -ENOMEM;
> 
> Strange. From the code I see here the WARN_ON in
> pcim_add_mapping_to_legacy_table() should not fire. I suspect that it's
> actually triggered somewhere else.
> 

pcim_iomap() calls pcim_add_mapping_to_legacy_table() which triggers the traceback.
The caller uses the returned error to determine that it needs to call pcim_iomap_table()
instead. As you point out below, that may not be necessary, but then it is already
too late and the warning was triggered.

>>
>> This suggests that the failure is expected. I can see that pcim_iomap_table()
>> is deprecated, and that one is supposed to use pcim_iomap() instead. However,
>> pcim_iomap() _is_ alrady used, and I don't see a function which lets the
>> caller replicate what is done above (attach multiple serial ports to the
>> same PCI bar).
> 
> Is serial8250_pci_setup_port() invoked in a loop somewhere? Where does
> the "attach multiple" happen?
> 

It is called for multiple serial ports, each of which are in the same bar but
with different offset into the bar.

>>
>> How would you suggest to fix the problem ?
> 
> I suggest you try to remove the `&& pcim_iomap_table(dev)` from above
> to see if that's really the cause. pcim_iomap() already creates the
> table, and if it succeeds the table has been created with absolute
> certainty. The entries will also be present. So the table-check is
> surplus.
> 

How would that fix anything ? The warning would still be triggered from the
failed call to pcim_iomap() for the 2nd and subsequent serial port on the
same bar.

Guenter

Re: [PATCH v9 02/13] PCI: Add devres helpers for iomap table [resulting in backtraces on HPPA]

[Philipp Stanner]

On Tue, 2025-11-25 at 08:12 -0800, Guenter Roeck wrote:
> On 11/25/25 07:48, Philipp Stanner wrote:
> > On Sun, 2025-11-23 at 08:42 -0800, Guenter Roeck wrote:
> > > Hi,
> > > 
> > > On Thu, Jun 13, 2024 at 01:50:15PM +0200, Philipp Stanner wrote:
> > > > The pcim_iomap_devres.table administrated by pcim_iomap_table() has its
> > > > entries set and unset at several places throughout devres.c using manual
> > > > iterations which are effectively code duplications.
> > > > 
> > > > Add pcim_add_mapping_to_legacy_table() and
> > > > pcim_remove_mapping_from_legacy_table() helper functions and use them where
> > > > possible.
> > > > 
> > > > Link: https://lore.kernel.org/r/20240605081605.18769-4-pstanner@redhat.com
> > > > Signed-off-by: Philipp Stanner <pstanner@redhat.com>
> > > > [bhelgaas: s/short bar/int bar/ for consistency]
> > > > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> > > > ---
> > > >   drivers/pci/devres.c | 77 +++++++++++++++++++++++++++++++++-----------
> > > >   1 file changed, 58 insertions(+), 19 deletions(-)
> > > > 
> > > > diff --git a/drivers/pci/devres.c b/drivers/pci/devres.c
> > > > index f13edd4a3873..845d6fab0ce7 100644
> > > > --- a/drivers/pci/devres.c
> > > > +++ b/drivers/pci/devres.c
> > > > @@ -297,6 +297,52 @@ void __iomem * const *pcim_iomap_table(struct pci_dev *pdev)
> > > >   }
> > > >   EXPORT_SYMBOL(pcim_iomap_table);
> > > >   
> > > > +/*
> > > > + * Fill the legacy mapping-table, so that drivers using the old API can
> > > > + * still get a BAR's mapping address through pcim_iomap_table().
> > > > + */
> > > > +static int pcim_add_mapping_to_legacy_table(struct pci_dev *pdev,
> > > > +					    void __iomem *mapping, int bar)
> > > > +{
> > > > +	void __iomem **legacy_iomap_table;
> > > > +
> > > > +	if (bar >= PCI_STD_NUM_BARS)
> > > > +		return -EINVAL;
> > > > +
> > > > +	legacy_iomap_table = (void __iomem **)pcim_iomap_table(pdev);
> > > > +	if (!legacy_iomap_table)
> > > > +		return -ENOMEM;
> > > > +
> > > > +	/* The legacy mechanism doesn't allow for duplicate mappings. */
> > > > +	WARN_ON(legacy_iomap_table[bar]);
> > > > +
> > > 
> > > Ever since this patch has been applied, I see this warning on all hppa
> > > (parisc) systems.
> > > 
> > > [    0.978177] WARNING: CPU: 0 PID: 1 at drivers/pci/devres.c:473 pcim_add_mapping_to_legacy_table.part.0+0x54/0x80
> > > [    0.978850] Modules linked in:
> > > [    0.979277] CPU: 0 UID: 0 PID: 1 Comm: swapper/0 Not tainted 6.18.0-rc6-64bit+ #1 NONE
> > > [    0.979519] Hardware name: 9000/785/C3700
> > > [    0.979715]
> > > [    0.979768]      YZrvWESTHLNXBCVMcbcbcbcbOGFRQPDI
> > > [    0.979886] PSW: 00001000000001000000000000001111 Not tainted
> > > [    0.980006] r00-03  000000000804000f 00000000414e10a0 0000000040acb300 00000000434b1440
> > > [    0.980167] r04-07  00000000414a78a0 0000000000029000 0000000000000000 0000000043522000
> > > [    0.980314] r08-11  0000000000000000 0000000000000008 0000000000000000 00000000434b0de8
> > > [    0.980461] r12-15  00000000434b11b0 000000004156a8a0 0000000043c655a0 0000000000000000
> > > [    0.980608] r16-19  000000004016e080 000000004019e7d8 0000000000000030 0000000043549780
> > > [    0.981106] r20-23  0000000020000000 0000000000000000 000000000800000e 0000000000000000
> > > [    0.981317] r24-27  0000000000000000 000000000800000f 0000000043522260 00000000414a78a0
> > > [    0.981480] r28-31  00000000436af480 00000000434b1680 00000000434b14d0 0000000000027000
> > > [    0.981641] sr00-03  0000000000000000 0000000000000000 0000000000000000 0000000000000000
> > > [    0.981805] sr04-07  0000000000000000 0000000000000000 0000000000000000 0000000000000000
> > > [    0.981972]
> > > [    0.982024] IASQ: 0000000000000000 0000000000000000 IAOQ: 0000000040acb31c 0000000040acb320
> > > [    0.982185]  IIR: 03ffe01f    ISR: 0000000000000000  IOR: 00000000436af410
> > > [    0.982322]  CPU:        0   CR30: 0000000043549780 CR31: 0000000000000000
> > > [    0.982458]  ORIG_R28: 00000000434b16b0
> > > [    0.982548]  IAOQ[0]: pcim_add_mapping_to_legacy_table.part.0+0x54/0x80
> > > [    0.982733]  IAOQ[1]: pcim_add_mapping_to_legacy_table.part.0+0x58/0x80
> > > [    0.982871]  RP(r2): pcim_add_mapping_to_legacy_table.part.0+0x38/0x80
> > > [    0.983100] Backtrace:
> > > [    0.983439]  [<0000000040acba1c>] pcim_iomap+0xc4/0x170
> > > [    0.983577]  [<0000000040ba3e4c>] serial8250_pci_setup_port+0x8c/0x168
> > > [    0.983725]  [<0000000040ba7588>] setup_port+0x38/0x50
> > > [    0.983837]  [<0000000040ba7d94>] pci_hp_diva_setup+0x8c/0xd8
> > > [    0.983957]  [<0000000040baa47c>] pciserial_init_ports+0x2c4/0x358
> > > [    0.984088]  [<0000000040baa8bc>] pciserial_init_one+0x31c/0x330
> > > [    0.984214]  [<0000000040abfab4>] pci_device_probe+0x194/0x270
> > > 
> > > Looking into serial8250_pci_setup_port():
> > > 
> > >          if (pci_resource_flags(dev, bar) & IORESOURCE_MEM) {
> > >                  if (!pcim_iomap(dev, bar, 0) && !pcim_iomap_table(dev))
> > >                          return -ENOMEM;
> > 
> > Strange. From the code I see here the WARN_ON in
> > pcim_add_mapping_to_legacy_table() should not fire. I suspect that it's
> > actually triggered somewhere else.
> > 
> 
> pcim_iomap() calls pcim_add_mapping_to_legacy_table() which triggers the traceback.
> The caller uses the returned error to determine that it needs to call pcim_iomap_table()
> instead. As you point out below, that may not be necessary, but then it is already
> too late and the warning was triggered.
> 
> > > 
> > > This suggests that the failure is expected. I can see that pcim_iomap_table()
> > > is deprecated, and that one is supposed to use pcim_iomap() instead. However,
> > > pcim_iomap() _is_ alrady used, and I don't see a function which lets the
> > > caller replicate what is done above (attach multiple serial ports to the
> > > same PCI bar).
> > 
> > Is serial8250_pci_setup_port() invoked in a loop somewhere? Where does
> > the "attach multiple" happen?
> > 
> 
> It is called for multiple serial ports, each of which are in the same bar but
> with different offset into the bar.
> 
> > > 
> > > How would you suggest to fix the problem ?
> > 
> > I suggest you try to remove the `&& pcim_iomap_table(dev)` from above
> > to see if that's really the cause. pcim_iomap() already creates the
> > table, and if it succeeds the table has been created with absolute
> > certainty. The entries will also be present. So the table-check is
> > surplus.
> > 
> 
> How would that fix anything ? The warning would still be triggered from the
> failed call to pcim_iomap() for the 2nd and subsequent serial port on the
> same bar.

OK, I failed to see that it's really pcim_iomap() which is called
multiple times for the same bar.

The warning itself is harmless, so it's not urgent.
Cleanup is always done through devres callbacks, one per resource. The
table is not used for that, just for accessors of existing mappings. So
at first glance I think that removing the WARN_ON would be OK. I'd like
to hear Bjorn's opinion on that, though.

Maybe you could investigate removing pcim_iomap_table() from this
driver, obtaining the mappings directly from calls to pcim_iomap().
Calling it multiple times for the same BAR is valid, it's just the
table which complains. Since you are the first party I ever hear from
about that WARN_ON. So with this driver ported one could argue that
removing it is justified..

Another possiblity could be to switch to unmanaged PCI. Use pci_iomap()
and pci_enable_device() etc.

In case you have lots of spare cycles, the cleanest way would be to
remove the legacy table altogether. To do so, one would have to port
all users of pcim_iomap_table(). I have worked on that for a while and
have removed many of them. The most difficult remaining users are AFAIR
in drivers/ata/


P.

Re: [PATCH v9 02/13] PCI: Add devres helpers for iomap table [resulting in backtraces on HPPA]

[Guenter Roeck]

On 11/25/25 08:28, Philipp Stanner wrote:
> On Tue, 2025-11-25 at 08:12 -0800, Guenter Roeck wrote:
>> On 11/25/25 07:48, Philipp Stanner wrote:
>>> On Sun, 2025-11-23 at 08:42 -0800, Guenter Roeck wrote:
>>>> Hi,
>>>>
>>>> On Thu, Jun 13, 2024 at 01:50:15PM +0200, Philipp Stanner wrote:
>>>>> The pcim_iomap_devres.table administrated by pcim_iomap_table() has its
>>>>> entries set and unset at several places throughout devres.c using manual
>>>>> iterations which are effectively code duplications.
>>>>>
>>>>> Add pcim_add_mapping_to_legacy_table() and
>>>>> pcim_remove_mapping_from_legacy_table() helper functions and use them where
>>>>> possible.
>>>>>
>>>>> Link: https://lore.kernel.org/r/20240605081605.18769-4-pstanner@redhat.com
>>>>> Signed-off-by: Philipp Stanner <pstanner@redhat.com>
>>>>> [bhelgaas: s/short bar/int bar/ for consistency]
>>>>> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>>>>> ---
>>>>>    drivers/pci/devres.c | 77 +++++++++++++++++++++++++++++++++-----------
>>>>>    1 file changed, 58 insertions(+), 19 deletions(-)
>>>>>
>>>>> diff --git a/drivers/pci/devres.c b/drivers/pci/devres.c
>>>>> index f13edd4a3873..845d6fab0ce7 100644
>>>>> --- a/drivers/pci/devres.c
>>>>> +++ b/drivers/pci/devres.c
>>>>> @@ -297,6 +297,52 @@ void __iomem * const *pcim_iomap_table(struct pci_dev *pdev)
>>>>>    }
>>>>>    EXPORT_SYMBOL(pcim_iomap_table);
>>>>>    
>>>>> +/*
>>>>> + * Fill the legacy mapping-table, so that drivers using the old API can
>>>>> + * still get a BAR's mapping address through pcim_iomap_table().
>>>>> + */
>>>>> +static int pcim_add_mapping_to_legacy_table(struct pci_dev *pdev,
>>>>> +					    void __iomem *mapping, int bar)
>>>>> +{
>>>>> +	void __iomem **legacy_iomap_table;
>>>>> +
>>>>> +	if (bar >= PCI_STD_NUM_BARS)
>>>>> +		return -EINVAL;
>>>>> +
>>>>> +	legacy_iomap_table = (void __iomem **)pcim_iomap_table(pdev);
>>>>> +	if (!legacy_iomap_table)
>>>>> +		return -ENOMEM;
>>>>> +
>>>>> +	/* The legacy mechanism doesn't allow for duplicate mappings. */
>>>>> +	WARN_ON(legacy_iomap_table[bar]);
>>>>> +
>>>>
>>>> Ever since this patch has been applied, I see this warning on all hppa
>>>> (parisc) systems.
>>>>
>>>> [    0.978177] WARNING: CPU: 0 PID: 1 at drivers/pci/devres.c:473 pcim_add_mapping_to_legacy_table.part.0+0x54/0x80
>>>> [    0.978850] Modules linked in:
>>>> [    0.979277] CPU: 0 UID: 0 PID: 1 Comm: swapper/0 Not tainted 6.18.0-rc6-64bit+ #1 NONE
>>>> [    0.979519] Hardware name: 9000/785/C3700
>>>> [    0.979715]
>>>> [    0.979768]      YZrvWESTHLNXBCVMcbcbcbcbOGFRQPDI
>>>> [    0.979886] PSW: 00001000000001000000000000001111 Not tainted
>>>> [    0.980006] r00-03  000000000804000f 00000000414e10a0 0000000040acb300 00000000434b1440
>>>> [    0.980167] r04-07  00000000414a78a0 0000000000029000 0000000000000000 0000000043522000
>>>> [    0.980314] r08-11  0000000000000000 0000000000000008 0000000000000000 00000000434b0de8
>>>> [    0.980461] r12-15  00000000434b11b0 000000004156a8a0 0000000043c655a0 0000000000000000
>>>> [    0.980608] r16-19  000000004016e080 000000004019e7d8 0000000000000030 0000000043549780
>>>> [    0.981106] r20-23  0000000020000000 0000000000000000 000000000800000e 0000000000000000
>>>> [    0.981317] r24-27  0000000000000000 000000000800000f 0000000043522260 00000000414a78a0
>>>> [    0.981480] r28-31  00000000436af480 00000000434b1680 00000000434b14d0 0000000000027000
>>>> [    0.981641] sr00-03  0000000000000000 0000000000000000 0000000000000000 0000000000000000
>>>> [    0.981805] sr04-07  0000000000000000 0000000000000000 0000000000000000 0000000000000000
>>>> [    0.981972]
>>>> [    0.982024] IASQ: 0000000000000000 0000000000000000 IAOQ: 0000000040acb31c 0000000040acb320
>>>> [    0.982185]  IIR: 03ffe01f    ISR: 0000000000000000  IOR: 00000000436af410
>>>> [    0.982322]  CPU:        0   CR30: 0000000043549780 CR31: 0000000000000000
>>>> [    0.982458]  ORIG_R28: 00000000434b16b0
>>>> [    0.982548]  IAOQ[0]: pcim_add_mapping_to_legacy_table.part.0+0x54/0x80
>>>> [    0.982733]  IAOQ[1]: pcim_add_mapping_to_legacy_table.part.0+0x58/0x80
>>>> [    0.982871]  RP(r2): pcim_add_mapping_to_legacy_table.part.0+0x38/0x80
>>>> [    0.983100] Backtrace:
>>>> [    0.983439]  [<0000000040acba1c>] pcim_iomap+0xc4/0x170
>>>> [    0.983577]  [<0000000040ba3e4c>] serial8250_pci_setup_port+0x8c/0x168
>>>> [    0.983725]  [<0000000040ba7588>] setup_port+0x38/0x50
>>>> [    0.983837]  [<0000000040ba7d94>] pci_hp_diva_setup+0x8c/0xd8
>>>> [    0.983957]  [<0000000040baa47c>] pciserial_init_ports+0x2c4/0x358
>>>> [    0.984088]  [<0000000040baa8bc>] pciserial_init_one+0x31c/0x330
>>>> [    0.984214]  [<0000000040abfab4>] pci_device_probe+0x194/0x270
>>>>
>>>> Looking into serial8250_pci_setup_port():
>>>>
>>>>           if (pci_resource_flags(dev, bar) & IORESOURCE_MEM) {
>>>>                   if (!pcim_iomap(dev, bar, 0) && !pcim_iomap_table(dev))
>>>>                           return -ENOMEM;
>>>
>>> Strange. From the code I see here the WARN_ON in
>>> pcim_add_mapping_to_legacy_table() should not fire. I suspect that it's
>>> actually triggered somewhere else.
>>>
>>
>> pcim_iomap() calls pcim_add_mapping_to_legacy_table() which triggers the traceback.
>> The caller uses the returned error to determine that it needs to call pcim_iomap_table()
>> instead. As you point out below, that may not be necessary, but then it is already
>> too late and the warning was triggered.
>>
>>>>
>>>> This suggests that the failure is expected. I can see that pcim_iomap_table()
>>>> is deprecated, and that one is supposed to use pcim_iomap() instead. However,
>>>> pcim_iomap() _is_ alrady used, and I don't see a function which lets the
>>>> caller replicate what is done above (attach multiple serial ports to the
>>>> same PCI bar).
>>>
>>> Is serial8250_pci_setup_port() invoked in a loop somewhere? Where does
>>> the "attach multiple" happen?
>>>
>>
>> It is called for multiple serial ports, each of which are in the same bar but
>> with different offset into the bar.
>>
>>>>
>>>> How would you suggest to fix the problem ?
>>>
>>> I suggest you try to remove the `&& pcim_iomap_table(dev)` from above
>>> to see if that's really the cause. pcim_iomap() already creates the
>>> table, and if it succeeds the table has been created with absolute
>>> certainty. The entries will also be present. So the table-check is
>>> surplus.
>>>
>>
>> How would that fix anything ? The warning would still be triggered from the
>> failed call to pcim_iomap() for the 2nd and subsequent serial port on the
>> same bar.
> 
> OK, I failed to see that it's really pcim_iomap() which is called
> multiple times for the same bar.
> 
> The warning itself is harmless, so it's not urgent.
> Cleanup is always done through devres callbacks, one per resource. The
> table is not used for that, just for accessors of existing mappings. So
> at first glance I think that removing the WARN_ON would be OK. I'd like
> to hear Bjorn's opinion on that, though.
> 
> Maybe you could investigate removing pcim_iomap_table() from this
> driver, obtaining the mappings directly from calls to pcim_iomap().

serial8250_pci_setup_port() is an API function. It is called from several
drivers. Its callers have no means to pass "This is the first request for
this bar", and at least for some of the callers this would not be easy
to accomplish unless one changes and adds complexity to all that calling
code.

> Calling it multiple times for the same BAR is valid, it's just the
> table which complains. Since you are the first party I ever hear from
> about that WARN_ON. So with this driver ported one could argue that
> removing it is justified..
> 

Maybe the reason for that is that there are more and more pointless
backtraces in the kernel. That only results in people ignoring them.
I do see some more, but then I notice that such warnings proliferate,
and feedback such as "The warning itself is harmless" doesn't really
encourage me doing that.

> Another possiblity could be to switch to unmanaged PCI. Use pci_iomap()
> and pci_enable_device() etc.
> 
> In case you have lots of spare cycles, the cleanest way would be to
> remove the legacy table altogether. To do so, one would have to port
> all users of pcim_iomap_table(). I have worked on that for a while and
> have removed many of them. The most difficult remaining users are AFAIR
> in drivers/ata/
> 

Sorry, "spare cycles" is not exactly something that happens in my life.

Guenter