This v8 is based on [1]. It contains the already applied patches in
unchanged form; just for readability and consistency.
Thx for taking the first set of patches! This series provides the
enabled_cnt rework and some other minor fixes. See below.
[1] https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/log/?h=devres
P.
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 onee
xception 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: Reimplement plural devres functions
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 | 6 +-
6 files changed, 859 insertions(+), 203 deletions(-)
--
2.45.0
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/[email protected]
Signed-off-by: Philipp Stanner <[email protected]>
Signed-off-by: Bjorn Helgaas <[email protected]>
---
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
When the original PCI devres API was implemented, priority was given to the
creation of a set of "plural functions" such as pcim_request_regions().
These functions have bit masks as parameters to specify which BARs shall
get mapped. Most users, however, only use those to map 1-3 BARs.
A complete set of "singular functions" does not exist.
As functions mapping / requesting multiple BARs at once have (almost) no
mechanism in C to return the resources to the caller of the plural
function, the PCI devres API utilizes the iomap-table administrated by the
function pcim_iomap_table().
The entire PCI devres API was strongly tied to that table which only allows
for mapping whole, complete BARs, as the BAR's index is used as table
index. Consequently, it's not possible to, e.g., have a pcim_iomap_range()
function with that mechanism.
An additional problem is that the PCI devres API has been implemented in a
sort of "hybrid-mode": Some unmanaged functions have managed counterparts
(e.g.: pci_iomap() <-> pcim_iomap()), making their managed nature obvious
to the programmer. However, the region-request functions in pci.c, prefixed
with pci_, behave either managed or unmanaged, depending on whether
pci_enable_device() or pcim_enable_device() has been called in advance.
This hybrid API is confusing and should be more cleanly separated by
providing always-managed functions prefixed with pcim_.
Thus, the existing PCI devres API is not desirable because:
a) The vast majority of the users of the plural functions only ever sets
a single bit in the bit mask, consequently making them singular
functions anyways.
b) There is no mechanism to request / iomap only part of a BAR.
c) The iomap-table mechanism is over-engineered and complicated. Even
worse, some users index over the table administration function
directly, e.g.:
void __iomem *mapping = pcim_iomap_table(pdev)[my_index];
This can not perform bounds checks; an invalid index won't cause
return of -EINVAL or even NULL, resulting in undefined behavior.
d) region-request functions being sometimes managed and sometimes not
is bug-provoking.
Implement a set of internal helper functions that don't have the problem of
a hybrid nature that their counter parts in pci.c have. Write those helpers
in a generic manner so that they can easily be extended to, e.g., ranged
mappings and requests.
Implement a set of singular functions that use devres as it's intended and
use those singular functions to reimplement the plural functions.
Link: https://lore.kernel.org/r/[email protected]
Signed-off-by: Philipp Stanner <[email protected]>
Signed-off-by: Bjorn Helgaas <[email protected]>
---
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..82f71f5e164a 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
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 <[email protected]>
---
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 2696baef5c2c..a0a59338cd92 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
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/[email protected]
Signed-off-by: Philipp Stanner <[email protected]>
Signed-off-by: Bjorn Helgaas <[email protected]>
---
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 82f71f5e164a..54b10f5433ab 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
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 <[email protected]>
---
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 a0a59338cd92..0bb144fdb69b 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
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 <[email protected]>
---
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 e92a8802832f..96f18243742b 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 cc9247f78158..bee1b2754219 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -2304,6 +2304,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
The PCI devres implementation has a separate boolean to track whether a
device is enabled. However, that can easily be tracked through the
function pci_is_enabled() which is agnostic.
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 <[email protected]>
---
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 f2a1250c0679..9d25940ce260 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
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 <[email protected]>
---
drivers/pci/devres.c | 14 ++++----------
drivers/pci/pci.h | 1 -
include/linux/pci.h | 4 +++-
3 files changed, 7 insertions(+), 12 deletions(-)
diff --git a/drivers/pci/devres.c b/drivers/pci/devres.c
index 9d25940ce260..2696baef5c2c 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..cc9247f78158 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -367,10 +367,12 @@ 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 enabled:1; /* Whether this dev is enabled */
+ 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
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/[email protected]
Signed-off-by: Philipp Stanner <[email protected]>
Signed-off-by: Bjorn Helgaas <[email protected]>
---
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 54b10f5433ab..f2a1250c0679 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
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 <[email protected]>
---
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 0bb144fdb69b..e92a8802832f 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
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 <[email protected]>
Reviewed-by: Hans de Goede <[email protected]>
---
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
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/[email protected]
Signed-off-by: Philipp Stanner <[email protected]>
[bhelgaas: s/short bar/int bar/ for consistency]
Signed-off-by: Bjorn Helgaas <[email protected]>
---
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
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/[email protected]
Signed-off-by: Philipp Stanner <[email protected]>
Signed-off-by: Bjorn Helgaas <[email protected]>
---
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
On Mon, 2024-06-10 at 11:31 +0200, Philipp Stanner wrote:
> 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 <[email protected]>
> ---
> drivers/pci/devres.c | 14 ++++----------
> drivers/pci/pci.h | 1 -
> include/linux/pci.h | 4 +++-
> 3 files changed, 7 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/pci/devres.c b/drivers/pci/devres.c
> index 9d25940ce260..2696baef5c2c 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..cc9247f78158 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -367,10 +367,12 @@ 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 enabled:1; /* Whether this dev is enabled */
Ah crap, here it survived for some reason...
Should just be dead code and not have any effect. In any case, we
should remove it.
@Bjorn: Feel free to remove it yourself. Otherwise I could provide a v9
together with potential further feedback taken into account in a few
days
Thx,
P.
> + 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 */
I'm trying to merge these into pci/next, but I'm having a hard time
writing the merge commit log. I want a one-sentence description of
each patch that tells me what the benefit of the patch is. Usually
the subject line is a good start.
"Reimplement plural devres functions" is kind of vague and doesn't
quite motivate this patch, and I'm having a hard time extracting the
relevant details from the commit log below.
On Mon, Jun 10, 2024 at 11:31:25AM +0200, Philipp Stanner wrote:
> When the original PCI devres API was implemented, priority was given to the
> creation of a set of "plural functions" such as pcim_request_regions().
> These functions have bit masks as parameters to specify which BARs shall
> get mapped. Most users, however, only use those to map 1-3 BARs.
>
> A complete set of "singular functions" does not exist.
>
> As functions mapping / requesting multiple BARs at once have (almost) no
> mechanism in C to return the resources to the caller of the plural
> function, the PCI devres API utilizes the iomap-table administrated by the
> function pcim_iomap_table().
>
> The entire PCI devres API was strongly tied to that table which only allows
> for mapping whole, complete BARs, as the BAR's index is used as table
> index. Consequently, it's not possible to, e.g., have a pcim_iomap_range()
> function with that mechanism.
I'm getting the hint that part of the point of this patch is to add
infrastructure so we can request and map either an entire BAR or just
part of a BAR?
> An additional problem is that the PCI devres API has been implemented in a
> sort of "hybrid-mode": Some unmanaged functions have managed counterparts
> (e.g.: pci_iomap() <-> pcim_iomap()), making their managed nature obvious
> to the programmer. However, the region-request functions in pci.c, prefixed
> with pci_, behave either managed or unmanaged, depending on whether
> pci_enable_device() or pcim_enable_device() has been called in advance.
>
> This hybrid API is confusing and should be more cleanly separated by
> providing always-managed functions prefixed with pcim_.
I'm not sure these two paragraphs apply to this patch. If they do, be
specific about which functions are affected and how this patch fixes
them.
> Thus, the existing PCI devres API is not desirable because:
>
> a) The vast majority of the users of the plural functions only ever sets
> a single bit in the bit mask, consequently making them singular
> functions anyways.
>
> b) There is no mechanism to request / iomap only part of a BAR.
> c) The iomap-table mechanism is over-engineered and complicated. Even
> worse, some users index over the table administration function
> directly, e.g.:
>
> void __iomem *mapping = pcim_iomap_table(pdev)[my_index];
>
> This can not perform bounds checks; an invalid index won't cause
> return of -EINVAL or even NULL, resulting in undefined behavior.
True, but *this* patch doesn't remove or deprecate pcim_iomap_table(),
so needs to be in the patch that does that, not here.
> d) region-request functions being sometimes managed and sometimes not
> is bug-provoking.
I'm not sure all the deficiencies of the past are necessary. I'm more
interested in specifics about what's being added or fixed.
> Implement a set of internal helper functions that don't have the problem of
> a hybrid nature that their counter parts in pci.c have. Write those helpers
> in a generic manner so that they can easily be extended to, e.g., ranged
> mappings and requests.
>
> Implement a set of singular functions
What is this set of functions? My guess is below.
> that use devres as it's intended and
> use those singular functions to reimplement the plural functions.
What does "as it's intended" mean? Too nebulous to fit here.
IIUC, this patch adds struct pcim_addr_devres, which contains:
- a BAR number
- an offset into the BAR
- a length
- the type of operation (request, map, or both)
- the ioremapped address (if applicable)
and it looks like we're adding these (which are currently only for use
inside drivers/pci/):
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_release_all_regions() # release all entire BARs
pcim_request_all_regions() # request all entire BARs
and reimplements these interfaces (which are mostly exported to
drivers) so they build on top of struct pcim_addr_devres and use some
of the internal functions added above:
pcim_iomap_release() # internal only
pcim_iomap() # map partial BAR
pcim_iounmap()
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 is the kind of specific detail I'm looking for.
> Link: https://lore.kernel.org/r/[email protected]
> Signed-off-by: Philipp Stanner <[email protected]>
> Signed-off-by: Bjorn Helgaas <[email protected]>
> ---
> 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..82f71f5e164a 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
>
On Tue, 2024-06-11 at 16:44 -0500, Bjorn Helgaas wrote:
> I'm trying to merge these into pci/next, but I'm having a hard time
> writing the merge commit log. I want a one-sentence description of
> each patch that tells me what the benefit of the patch is. Usually
> the subject line is a good start.
>
> "Reimplement plural devres functions" is kind of vague and doesn't
> quite motivate this patch, and I'm having a hard time extracting the
> relevant details from the commit log below.
I would say that the summary would be something along the lines:
"Set ground layer for devres simplification and extension"
because this patch simplifies the existing functions and adds
infrastructure that can later be used to deprecate the bloated existing
functions, remove the hybrid mechanism and add pcim_iomap_range().
>
> On Mon, Jun 10, 2024 at 11:31:25AM +0200, Philipp Stanner wrote:
> > When the original PCI devres API was implemented, priority was
> > given to the
> > creation of a set of "plural functions" such as
> > pcim_request_regions().
> > These functions have bit masks as parameters to specify which BARs
> > shall
> > get mapped. Most users, however, only use those to map 1-3 BARs.
> >
> > A complete set of "singular functions" does not exist.
> >
> > As functions mapping / requesting multiple BARs at once have
> > (almost) no
> > mechanism in C to return the resources to the caller of the plural
> > function, the PCI devres API utilizes the iomap-table administrated
> > by the
> > function pcim_iomap_table().
> >
> > The entire PCI devres API was strongly tied to that table which
> > only allows
> > for mapping whole, complete BARs, as the BAR's index is used as
> > table
> > index. Consequently, it's not possible to, e.g., have a
> > pcim_iomap_range()
> > function with that mechanism.
>
> I'm getting the hint that part of the point of this patch is to add
> infrastructure so we can request and map either an entire BAR or just
> part of a BAR?
Yes, that and in the long term the simplification of the PCI devres API
is the goal.
>
> > An additional problem is that the PCI devres API has been
> > implemented in a
> > sort of "hybrid-mode": Some unmanaged functions have managed
> > counterparts
> > (e.g.: pci_iomap() <-> pcim_iomap()), making their managed nature
> > obvious
> > to the programmer. However, the region-request functions in pci.c,
> > prefixed
> > with pci_, behave either managed or unmanaged, depending on whether
> > pci_enable_device() or pcim_enable_device() has been called in
> > advance.
> >
> > This hybrid API is confusing and should be more cleanly separated
> > by
> > providing always-managed functions prefixed with pcim_.
>
> I'm not sure these two paragraphs apply to this patch. If they do,
> be
> specific about which functions are affected and how this patch fixes
> them.
That's a relict from the days when this series consisted of fewer
commits. Back then this commit's ancestor served preparing the entire
series; therefore, it contained a lot of motivational information.
I can just cut that out. I guess a link to the mail thread and its
cover letter in the commit message would explain the wider motivation
behind all of this.
>
> > Thus, the existing PCI devres API is not desirable because:
> >
> > a) The vast majority of the users of the plural functions only
> > ever sets
> > a single bit in the bit mask, consequently making them
> > singular
> > functions anyways.
> >
> > b) There is no mechanism to request / iomap only part of a BAR.
>
> > c) The iomap-table mechanism is over-engineered and complicated.
> > Even
> > worse, some users index over the table administration function
> > directly, e.g.:
> >
> > void __iomem *mapping = pcim_iomap_table(pdev)[my_index];
> >
> > This can not perform bounds checks; an invalid index won't
> > cause
> > return of -EINVAL or even NULL, resulting in undefined
> > behavior.
>
> True, but *this* patch doesn't remove or deprecate
> pcim_iomap_table(),
> so needs to be in the patch that does that, not here.
>
>
> ACK, I will remove it.
>
>
> > d) region-request functions being sometimes managed and sometimes
> > not
> > is bug-provoking.
>
> I'm not sure all the deficiencies of the past are necessary. I'm
> more
> interested in specifics about what's being added or fixed.
ACK
>
> > Implement a set of internal helper functions that don't have the
> > problem of
> > a hybrid nature that their counter parts in pci.c have. Write those
> > helpers
> > in a generic manner so that they can easily be extended to, e.g.,
> > ranged
> > mappings and requests.
> >
> > Implement a set of singular functions
>
> What is this set of functions? My guess is below.
>
> > that use devres as it's intended and
> > use those singular functions to reimplement the plural functions.
>
> What does "as it's intended" mean? Too nebulous to fit here.
Well, the idea behind devres is that you allocate a device resource
_for each_ object you want to be freed / deinitialized automatically.
One devres object per driver / subsystem object, one devres callback
per cleanup job for the driver / subsystem.
What PCI devres did instead was to use just ONE devres object _for
everything_ and then it had to implement all sorts of checks to check
which sub-resource this master resource is actually about:
(from devres.c)
static void pcim_release(struct device *gendev, void *res)
{
struct pci_dev *dev = to_pci_dev(gendev);
struct pci_devres *this = res;
int i;
for (i = 0; i < DEVICE_COUNT_RESOURCE; i++)
if (this->region_mask & (1 << i))
pci_release_region(dev, i);
if (this->mwi)
pci_clear_mwi(dev);
if (this->restore_intx)
pci_intx(dev, this->orig_intx);
if (this->enabled && !this->pinned)
pci_disable_device(dev);
}
So one could dare to say that devres was partially re-implemented on
top of devres.
The for-loop and the if-conditions constitute that "re-implementation".
No one has any clue why it has been done that way, because it provides
0 upsides and would have been far easier to implement by just letting
devres do its job.
Would you like to see the above details in the commit message?
>
> IIUC, this patch adds struct pcim_addr_devres, which contains:
>
> - a BAR number
> - an offset into the BAR
> - a length
> - the type of operation (request, map, or both)
> - the ioremapped address (if applicable)
Yup. pcim_addr_devres couples region requests and io-mappings, since
those are very frequently created together
>
> and it looks like we're adding these (which are currently only for
> use
> inside drivers/pci/):
>
> 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_release_all_regions() # release all entire BARs
> pcim_request_all_regions() # request all entire BARs
>
> and reimplements these interfaces (which are mostly exported to
> drivers) so they build on top of struct pcim_addr_devres and use some
> of the internal functions added above:
>
> pcim_iomap_release() # internal only
>
> pcim_iomap() # map partial BAR
> pcim_iounmap()
> 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 is the kind of specific detail I'm looking for.
Correct :)
So let me rework this commit message so it only states the basic
motivation (simplification, groundlayer later extension of PCI devres)
and then lists precisely what is being done.
I can provide that in v9 together with the forgotten "enabled" bit in
the other patch.
Cheers,
P.
>
> > Link:
> > https://lore.kernel.org/r/[email protected]
> > Signed-off-by: Philipp Stanner <[email protected]>
> > Signed-off-by: Bjorn Helgaas <[email protected]>
> > ---
> > 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..82f71f5e164a 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
> >
>
On Wed, Jun 12, 2024 at 10:51:40AM +0200, Philipp Stanner wrote:
> On Tue, 2024-06-11 at 16:44 -0500, Bjorn Helgaas wrote:
> > I'm trying to merge these into pci/next, but I'm having a hard time
> > writing the merge commit log. I want a one-sentence description of
> > each patch that tells me what the benefit of the patch is. Usually
> > the subject line is a good start.
> >
> > "Reimplement plural devres functions" is kind of vague and doesn't
> > quite motivate this patch, and I'm having a hard time extracting the
> > relevant details from the commit log below.
>
> I would say that the summary would be something along the lines:
> "Set ground layer for devres simplification and extension"
>
> because this patch simplifies the existing functions and adds
> infrastructure that can later be used to deprecate the bloated existing
> functions, remove the hybrid mechanism and add pcim_iomap_range().
I think something concrete like "Add partial-BAR devres support" would
give people a hint about what to look for.
This patch contains quite a bit more than that, and if it were
possible, it might be nice to split the rest to a different patch, but
I'm not sure it's even possible and I just want to get this series out
the door.
If the commit log includes the partial-BAR idea and the specific
functions added, I think that will hold together. And then it makes
sense for why the "plural" functions would be implemented on top of
the "singular" ones.
> > > Implement a set of singular functions
> >
> > What is this set of functions? My guess is below.
> >
> > > that use devres as it's intended and
> > > use those singular functions to reimplement the plural functions.
> >
> > What does "as it's intended" mean? Too nebulous to fit here.
>
> Well, the idea behind devres is that you allocate a device resource
> _for each_ object you want to be freed / deinitialized automatically.
> One devres object per driver / subsystem object, one devres callback
> per cleanup job for the driver / subsystem.
>
> What PCI devres did instead was to use just ONE devres object _for
> everything_ and then it had to implement all sorts of checks to check
> which sub-resource this master resource is actually about:
>
> (from devres.c)
> static void pcim_release(struct device *gendev, void *res)
> {
> struct pci_dev *dev = to_pci_dev(gendev);
> struct pci_devres *this = res;
> int i;
>
> for (i = 0; i < DEVICE_COUNT_RESOURCE; i++)
> if (this->region_mask & (1 << i))
> pci_release_region(dev, i);
>
> if (this->mwi)
> pci_clear_mwi(dev);
>
> if (this->restore_intx)
> pci_intx(dev, this->orig_intx);
>
> if (this->enabled && !this->pinned)
> pci_disable_device(dev);
> }
>
>
> So one could dare to say that devres was partially re-implemented on
> top of devres.
>
> The for-loop and the if-conditions constitute that "re-implementation".
> No one has any clue why it has been done that way, because it provides
> 0 upsides and would have been far easier to implement by just letting
> devres do its job.
>
> Would you like to see the above details in the commit message?
No. Just remove the "use devres as it's intended" since that's not
needed to motivate this patch. I think we need fewer and
more-specific words.
Bjorn
On Wed, 2024-06-12 at 15:42 -0500, Bjorn Helgaas wrote:
> On Wed, Jun 12, 2024 at 10:51:40AM +0200, Philipp Stanner wrote:
> > On Tue, 2024-06-11 at 16:44 -0500, Bjorn Helgaas wrote:
> > > I'm trying to merge these into pci/next, but I'm having a hard
> > > time
> > > writing the merge commit log. I want a one-sentence description
> > > of
> > > each patch that tells me what the benefit of the patch is.
> > > Usually
> > > the subject line is a good start.
> > >
> > > "Reimplement plural devres functions" is kind of vague and
> > > doesn't
> > > quite motivate this patch, and I'm having a hard time extracting
> > > the
> > > relevant details from the commit log below.
> >
> > I would say that the summary would be something along the lines:
> > "Set ground layer for devres simplification and extension"
> >
> > because this patch simplifies the existing functions and adds
> > infrastructure that can later be used to deprecate the bloated
> > existing
> > functions, remove the hybrid mechanism and add pcim_iomap_range().
>
> I think something concrete like "Add partial-BAR devres support"
> would
> give people a hint about what to look for.
Okay, will do.
>
> This patch contains quite a bit more than that, and if it were
> possible, it might be nice to split the rest to a different patch,
> but
> I'm not sure it's even possible
I tried and got screamed at by the build chain because of dead code. So
I don't really think they can be split more, unfortunately.
In possibly following series's to PCI I'll pay attention to design
things as atomically as possible from the start.
> and I just want to get this series out
> the door.
That's actually something you and I have in common. I have been working
on the preparations for this since November last year ^^'
>
> If the commit log includes the partial-BAR idea and the specific
> functions added, I think that will hold together. And then it makes
> sense for why the "plural" functions would be implemented on top of
> the "singular" ones.
>
> > > > Implement a set of singular functions
> > >
> > > What is this set of functions? My guess is below.
> > >
> > > > that use devres as it's intended and
> > > > use those singular functions to reimplement the plural
> > > > functions.
> > >
> > > What does "as it's intended" mean? Too nebulous to fit here.
> >
> > Well, the idea behind devres is that you allocate a device resource
> > _for each_ object you want to be freed / deinitialized
> > automatically.
> > One devres object per driver / subsystem object, one devres
> > callback
> > per cleanup job for the driver / subsystem.
> >
> > What PCI devres did instead was to use just ONE devres object _for
> > everything_ and then it had to implement all sorts of checks to
> > check
> > which sub-resource this master resource is actually about:
> >
> > (from devres.c)
> > static void pcim_release(struct device *gendev, void *res)
> > {
> > struct pci_dev *dev = to_pci_dev(gendev);
> > struct pci_devres *this = res;
> > int i;
> >
> > for (i = 0; i < DEVICE_COUNT_RESOURCE; i++)
> > if (this->region_mask & (1 << i))
> > pci_release_region(dev, i);
> >
> > if (this->mwi)
> > pci_clear_mwi(dev);
> >
> > if (this->restore_intx)
> > pci_intx(dev, this->orig_intx);
> >
> > if (this->enabled && !this->pinned)
> > pci_disable_device(dev);
> > }
> >
> >
> > So one could dare to say that devres was partially re-implemented
> > on
> > top of devres.
> >
> > The for-loop and the if-conditions constitute that "re-
> > implementation".
> > No one has any clue why it has been done that way, because it
> > provides
> > 0 upsides and would have been far easier to implement by just
> > letting
> > devres do its job.
> >
> > Would you like to see the above details in the commit message?
>
> No. Just remove the "use devres as it's intended" since that's not
> needed to motivate this patch. I think we need fewer and
> more-specific words.
ACK. I will rework it
Thank you Bjorn for your time and effort,
P.
>
> Bjorn
>