mbox series

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

Message ID 20240613115032.29098-1-pstanner@redhat.com
Headers show
Series Make PCI's devres API more consistent | expand

Message

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

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

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

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

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

Changes in v4:
  - Rebase against linux-next

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

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


¡Hola!

PCI's devres API suffers several weaknesses:

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

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


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

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

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

Greetings,
P.

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

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

Comments

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

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

Planned for v6.11.

diff --git a/drivers/pci/devres.c b/drivers/pci/devres.c
index 2f0379a4e58f..d9b78a0d903a 100644
--- a/drivers/pci/devres.c
+++ b/drivers/pci/devres.c
@@ -11,7 +11,7 @@
  * 1. It is very strongly tied to the statically allocated mapping table in
  *    struct pcim_iomap_devres below. This is mostly solved in the sense of the
  *    pcim_ functions in this file providing things like ranged mapping by
- *    bypassing this table, wheras the functions that were present in the old
+ *    bypassing this table, whereas the functions that were present in the old
  *    API still enter the mapping addresses into the table for users of the old
  *    API.
  *
@@ -25,10 +25,11 @@
  *    Consequently, in the new API, region requests performed by the pcim_
  *    functions are automatically cleaned up through the devres callback
  *    pcim_addr_resource_release().
- *    Users utilizing pcim_enable_device() + pci_*region*() are redirected in
+ *
+ *    Users of pcim_enable_device() + pci_*region*() are redirected in
  *    pci.c to the managed functions here in this file. This isn't exactly
- *    perfect, but the only alternative way would be to port ALL drivers using
- *    said combination to pcim_ functions.
+ *    perfect, but the only alternative way would be to port ALL drivers
+ *    using said combination to pcim_ functions.
  *
  * TODO:
  * Remove the legacy table entirely once all calls to pcim_iomap_table() in
@@ -42,7 +43,7 @@ struct pcim_iomap_devres {
 	void __iomem *table[PCI_STD_NUM_BARS];
 };
 
-/* Used to restore the old intx state on driver detach. */
+/* Used to restore the old INTx state on driver detach. */
 struct pcim_intx_devres {
 	int orig_intx;
 };
@@ -77,7 +78,7 @@ struct pcim_addr_devres {
 	void __iomem *baseaddr;
 	unsigned long offset;
 	unsigned long len;
-	short bar;
+	int bar;
 };
 
 static inline void pcim_addr_devres_clear(struct pcim_addr_devres *res)
@@ -108,8 +109,9 @@ static inline void pcim_addr_devres_clear(struct pcim_addr_devres *res)
  * Request a range within a device's PCI BAR.  Sanity check the input.
  */
 static int __pcim_request_region_range(struct pci_dev *pdev, int bar,
-		unsigned long offset, unsigned long maxlen,
-		const char *name, int req_flags)
+				       unsigned long offset,
+				       unsigned long maxlen,
+				       const char *name, int req_flags)
 {
 	resource_size_t start = pci_resource_start(pdev, bar);
 	resource_size_t len = pci_resource_len(pdev, bar);
@@ -118,7 +120,7 @@ static int __pcim_request_region_range(struct pci_dev *pdev, int bar,
 	if (start == 0 || len == 0) /* Unused BAR. */
 		return 0;
 	if (len <= offset)
-		return  -EINVAL;
+		return -EINVAL;
 
 	start += offset;
 	len -= offset;
@@ -141,7 +143,8 @@ static int __pcim_request_region_range(struct pci_dev *pdev, int bar,
 }
 
 static void __pcim_release_region_range(struct pci_dev *pdev, int bar,
-		unsigned long offset, unsigned long maxlen)
+					unsigned long offset,
+					unsigned long maxlen)
 {
 	resource_size_t start = pci_resource_start(pdev, bar);
 	resource_size_t len = pci_resource_len(pdev, bar);
@@ -166,7 +169,7 @@ static void __pcim_release_region_range(struct pci_dev *pdev, int bar,
 }
 
 static int __pcim_request_region(struct pci_dev *pdev, int bar,
-		const char *name, int flags)
+				 const char *name, int flags)
 {
 	unsigned long offset = 0;
 	unsigned long len = pci_resource_len(pdev, bar);
@@ -208,7 +211,7 @@ static struct pcim_addr_devres *pcim_addr_devres_alloc(struct pci_dev *pdev)
 	struct pcim_addr_devres *res;
 
 	res = devres_alloc_node(pcim_addr_resource_release, sizeof(*res),
-			GFP_KERNEL, dev_to_node(&pdev->dev));
+				GFP_KERNEL, dev_to_node(&pdev->dev));
 	if (res)
 		pcim_addr_devres_clear(res);
 	return res;
@@ -223,7 +226,8 @@ static inline void pcim_addr_devres_free(struct pcim_addr_devres *res)
 /*
  * Used by devres to identify a pcim_addr_devres.
  */
-static int pcim_addr_resources_match(struct device *dev, void *a_raw, void *b_raw)
+static int pcim_addr_resources_match(struct device *dev,
+				     void *a_raw, void *b_raw)
 {
 	struct pcim_addr_devres *a, *b;
 
@@ -402,7 +406,6 @@ int pcim_set_mwi(struct pci_dev *pdev)
 }
 EXPORT_SYMBOL(pcim_set_mwi);
 
-
 static inline bool mask_contains_bar(int mask, int bar)
 {
 	return mask & BIT(bar);
@@ -438,8 +441,8 @@ static struct pcim_intx_devres *get_or_create_intx_devres(struct device *dev)
  *
  * Returns: 0 on success, -ENOMEM on error.
  *
- * Enables/disables PCI INTx for device @pdev.
- * Restores the original state on driver detach.
+ * Enable/disable PCI INTx for device @pdev.
+ * Restore the original state on driver detach.
  */
 int pcim_intx(struct pci_dev *pdev, int enable)
 {
@@ -492,7 +495,7 @@ int pcim_enable_device(struct pci_dev *pdev)
 
 	/*
 	 * We prefer removing the action in case of an error over
-	 * devm_add_action_or_reset() because the later could theoretically be
+	 * devm_add_action_or_reset() because the latter could theoretically be
 	 * disturbed by users having pinned the device too soon.
 	 */
 	ret = pci_enable_device(pdev);
@@ -618,7 +621,7 @@ static void pcim_remove_mapping_from_legacy_table(struct pci_dev *pdev,
  * The same as pcim_remove_mapping_from_legacy_table(), but identifies the
  * mapping by its BAR index.
  */
-static void pcim_remove_bar_from_legacy_table(struct pci_dev *pdev, short bar)
+static void pcim_remove_bar_from_legacy_table(struct pci_dev *pdev, int bar)
 {
 	void __iomem **legacy_iomap_table;
 
@@ -783,7 +786,7 @@ static void pcim_iounmap_region(struct pci_dev *pdev, int bar)
 int pcim_iomap_regions(struct pci_dev *pdev, int mask, const char *name)
 {
 	int ret;
-	short bar;
+	int bar;
 	void __iomem *mapping;
 
 	for (bar = 0; bar < DEVICE_COUNT_RESOURCE; bar++) {
@@ -813,7 +816,7 @@ int pcim_iomap_regions(struct pci_dev *pdev, int mask, const char *name)
 EXPORT_SYMBOL(pcim_iomap_regions);
 
 static int _pcim_request_region(struct pci_dev *pdev, int bar, const char *name,
-		int request_flags)
+				int request_flags)
 {
 	int ret;
 	struct pcim_addr_devres *res;
@@ -903,7 +906,7 @@ void pcim_release_region(struct pci_dev *pdev, int bar)
  */
 static void pcim_release_all_regions(struct pci_dev *pdev)
 {
-	short bar;
+	int bar;
 
 	for (bar = 0; bar < PCI_STD_NUM_BARS; bar++)
 		pcim_release_region(pdev, bar);
@@ -923,7 +926,7 @@ static void pcim_release_all_regions(struct pci_dev *pdev)
 static int pcim_request_all_regions(struct pci_dev *pdev, const char *name)
 {
 	int ret;
-	short bar;
+	int bar;
 
 	for (bar = 0; bar < PCI_STD_NUM_BARS; bar++) {
 		ret = pcim_request_region(pdev, bar, name);
@@ -960,7 +963,7 @@ static int pcim_request_all_regions(struct pci_dev *pdev, const char *name)
 int pcim_iomap_regions_request_all(struct pci_dev *pdev, int mask,
 				   const char *name)
 {
-	short bar;
+	int bar;
 	int ret;
 	void __iomem **legacy_iomap_table;
 
@@ -1004,14 +1007,14 @@ EXPORT_SYMBOL(pcim_iomap_regions_request_all);
  */
 void pcim_iounmap_regions(struct pci_dev *pdev, int mask)
 {
-	short bar;
+	int i;
 
-	for (bar = 0; bar < PCI_STD_NUM_BARS; bar++) {
-		if (!mask_contains_bar(mask, bar))
+	for (i = 0; i < PCI_STD_NUM_BARS; i++) {
+		if (!mask_contains_bar(mask, i))
 			continue;
 
-		pcim_iounmap_region(pdev, bar);
-		pcim_remove_bar_from_legacy_table(pdev, bar);
+		pcim_iounmap_region(pdev, i);
+		pcim_remove_bar_from_legacy_table(pdev, i);
 	}
 }
 EXPORT_SYMBOL(pcim_iounmap_regions);
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 1b4832a60047..807f8be043cd 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -4073,6 +4073,11 @@ EXPORT_SYMBOL(pci_release_regions);
  *
  * Returns 0 on success, or %EBUSY on error.  A warning
  * message is also printed on failure.
+ *
+ * NOTE:
+ * This is a "hybrid" function: It's normally unmanaged, but becomes managed
+ * when pcim_enable_device() has been called in advance. This hybrid feature is
+ * DEPRECATED! If you want managed cleanup, use the pcim_* functions instead.
  */
 int pci_request_regions(struct pci_dev *pdev, const char *res_name)
 {
@@ -4437,17 +4442,13 @@ void pci_disable_parity(struct pci_dev *dev)
  * NOTE:
  * This is a "hybrid" function: It's normally unmanaged, but becomes managed
  * when pcim_enable_device() has been called in advance. This hybrid feature is
- * DEPRECATED!
+ * DEPRECATED! If you want managed cleanup, use pcim_intx() instead.
  */
 void pci_intx(struct pci_dev *pdev, int enable)
 {
 	u16 pci_command, new;
 
-	/*
-	 * This is done for backwards compatibility, because the old PCI devres
-	 * API had a mode in which this function became managed if the dev had
-	 * been enabled with pcim_enable_device() instead of pci_enable_device().
-	 */
+	/* Preserve the "hybrid" behavior for backwards compatibility */
 	if (pci_is_managed(pdev)) {
 		WARN_ON_ONCE(pcim_intx(pdev, enable) != 0);
 		return;
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index e51e6fa79fcc..e6d299b93c21 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -813,7 +813,8 @@ static inline pci_power_t mid_pci_get_power_state(struct pci_dev *pdev)
 int pcim_intx(struct pci_dev *dev, int enable);
 
 int pcim_request_region(struct pci_dev *pdev, int bar, const char *name);
-int pcim_request_region_exclusive(struct pci_dev *pdev, int bar, const char *name);
+int pcim_request_region_exclusive(struct pci_dev *pdev, int bar,
+				  const char *name);
 void pcim_release_region(struct pci_dev *pdev, int bar);
 
 /*
Philipp Stanner June 14, 2024, 11:38 a.m. UTC | #2
On Thu, 2024-06-13 at 16:57 -0500, Bjorn Helgaas wrote:
> On Thu, Jun 13, 2024 at 01:50:13PM +0200, Philipp Stanner wrote:
> > Changes in v9:
> >   - Remove forgotten dead code ('enabled' bit in struct pci_dev) in
> >     patch No.8 ("Move pinned status bit...")
> >   - Rework patch No.3:
> >       - Change title from "Reimplement plural devres functions"
> >         to "Add partial-BAR devres support".
> >       - Drop excessive details about the general cleanup from the
> > commit
> >         message. Only motivate why this patch's new infrastructure
> > is
> >         necessary.
> >   - Fix some minor spelling issues (s/pci/PCI ...)
> > 
> > Changes in v8:
> >   - Rebase the series on the already merged patches which were
> > slightly
> >     modified by Bjorn Helgaas.
> >   - Reword the pci_intx() commit message so it clearly states it's
> > about
> >     reworking pci_intx().
> >   - Move the removal of find_pci_dr() from patch "Remove legacy
> >     pcim_release()" to patch "Give pci_intx() its own devres
> > callback"
> >     since this later patch already removed all calls to that
> > function.
> >   - In patch "Give pci_intx() its own devres callback": use
> >     pci_is_enabled() (and, thus, the enabled_cnt in struct pci_dev)
> >     instead of a separate enabled field. (Bjorn)
> > 
> > Changes in v7:
> >   - Split the entire series in smaller, more atomic chunks /
> > patches
> >     (Bjorn)
> >   - Remove functions (such as pcim_iomap_region_range()) that do
> > not yet
> >     have a user (Bjorn)
> >   - Don't export interfaces publicly anymore, except for
> >     pcim_iomap_range(), needed by vboxvideo (Bjorn)
> >   - Mention the actual (vboxvideo) bug in "PCI: Warn users..."
> > commit
> >     (Bjorn)
> >   - Drop docstring warnings on PCI-internal functions (Bjorn)
> >   - Rework docstring warnings
> >   - Fix spelling in a few places. Rewrapp paragraphs (Bjorn)
> > 
> > Changes in v6:
> >   - Restructure the cleanup in pcim_iomap_regions_request_all() so
> > that
> >     it doesn't trigger a (false positive) test robot warning. No
> >     behavior change intended. (Dan Carpenter)
> > 
> > Changes in v5:
> >   - Add Hans's Reviewed-by to vboxvideo patch (Hans de Goede)
> >   - Remove stable-kernel from CC in vboxvideo patch (Hans de Goede)
> > 
> > Changes in v4:
> >   - Rebase against linux-next
> > 
> > Changes in v3:
> >   - Use the term "PCI devres API" at some forgotten places.
> >   - Fix more grammar errors in patch #3.
> >   - Remove the comment advising to call (the outdated) pcim_intx()
> > in pci.c
> >   - Rename __pcim_request_region_range() flags-field "exclusive" to
> >     "req_flags", since this is what the int actually represents.
> >   - Remove the call to pcim_region_request() from patch #10. (Hans)
> > 
> > Changes in v2:
> >   - Make commit head lines congruent with PCI's style (Bjorn)
> >   - Add missing error checks for devm_add_action(). (Andy)
> >   - Repair the "Returns: " marks for docu generation (Andy)
> >   - Initialize the addr_devres struct with memset(). (Andy)
> >   - Make pcim_intx() a PCI-internal function so that new drivers
> > won't
> >     be encouraged to use the outdated pci_intx() mechanism.
> >     (Andy / Philipp)
> >   - Fix grammar and spelling (Bjorn)
> >   - Be more precise on why pcim_iomap_table() is problematic
> > (Bjorn)
> >   - Provide the actual structs' and functions' names in the commit
> >     messages (Bjorn)
> >   - Remove redundant variable initializers (Andy)
> >   - Regroup PM bitfield members in struct pci_dev (Andy)
> >   - Make pcim_intx() visible only for the PCI subsystem so that
> > new    
> >     drivers won't use this outdated API (Andy, Myself)
> >   - Add a NOTE to pcim_iomap() to warn about this function being
> > the one
> >     exception that does just return NULL.
> >   - Consistently use the term "PCI devres API"; also in Patch #10
> > (Bjorn)
> > 
> > 
> > ¡Hola!
> > 
> > PCI's devres API suffers several weaknesses:
> > 
> > 1. There are functions prefixed with pcim_. Those are always
> > managed
> >    counterparts to never-managed functions prefixed with pci_ – or
> > so one
> >    would like to think. There are some apparently unmanaged
> > functions
> >    (all region-request / release functions, and pci_intx()) which
> >    suddenly become managed once the user has initialized the device
> > with
> >    pcim_enable_device() instead of pci_enable_device(). This
> > "sometimes
> >    yes, sometimes no" nature of those functions is confusing and
> >    therefore bug-provoking. In fact, it has already caused a bug in
> > DRM.
> >    The last patch in this series fixes that bug.
> > 2. iomappings: Instead of giving each mapping its own callback, the
> >    existing API uses a statically allocated struct tracking one
> > mapping
> >    per bar. This is not extensible. Especially, you can't create
> >    _ranged_ managed mappings that way, which many drivers want.
> > 3. Managed request functions only exist as "plural versions" with a
> >    bit-mask as a parameter. That's quite over-engineered
> > considering
> >    that each user only ever mapps one, maybe two bars.
> > 
> > This series:
> > - add a set of new "singular" devres functions that use devres the
> > way
> >   its intended, with one callback per resource.
> > - deprecates the existing iomap-table mechanism.
> > - deprecates the hybrid nature of pci_ functions.
> > - preserves backwards compatibility so that drivers using the
> > existing
> >   API won't notice any changes.
> > - adds documentation, especially some warning users about the
> >   complicated nature of PCI's devres.
> > 
> > 
> > Note that this series is based on my "unify pci_iounmap"-series
> > from a
> > few weeks ago. [1]
> > 
> > I tested this on a x86 VM with a simple pci test-device with two
> > regions. Operates and reserves resources as intended on my system.
> > Kasan and kmemleak didn't find any problems.
> > 
> > I believe this series cleans the API up as much as possible without
> > having to port all existing drivers to the new API. Especially, I
> > think
> > that this implementation is easy to extend if the need for new
> > managed
> > functions arises :)
> > 
> > Greetings,
> > P.
> > 
> > Philipp Stanner (13):
> >   PCI: Add and use devres helper for bit masks
> >   PCI: Add devres helpers for iomap table
> >   PCI: Add partial-BAR devres support
> >   PCI: Deprecate two surplus devres functions
> >   PCI: Make devres region requests consistent
> >   PCI: Warn users about complicated devres nature
> >   PCI: Remove enabled status bit from pci_devres
> >   PCI: Move pinned status bit to struct pci_dev
> >   PCI: Give pcim_set_mwi() its own devres callback
> >   PCI: Give pci_intx() its own devres callback
> >   PCI: Remove legacy pcim_release()
> >   PCI: Add pcim_iomap_range()
> >   drm/vboxvideo: fix mapping leaks
> > 
> >  drivers/gpu/drm/vboxvideo/vbox_main.c |  20 +-
> >  drivers/pci/devres.c                  | 903 +++++++++++++++++++++-
> > ----
> >  drivers/pci/iomap.c                   |  16 +
> >  drivers/pci/pci.c                     |  94 ++-
> >  drivers/pci/pci.h                     |  23 +-
> >  include/linux/pci.h                   |   5 +-
> >  6 files changed, 858 insertions(+), 203 deletions(-)
> 
> This is on pci/devres with some commit log rework and the following
> diffs.  I think the bar short/int thing is the only actual code
> change.  Happy to squash in any other updates or things I botched.

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

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

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

to:

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

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

> 
> Planned for v6.11.

\o/

P.

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

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

Amended to that, thanks.