[PATCHv3 0/5] pci cleanup/prep patches

[PATCHv3 0/5] pci cleanup/prep patches

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

This is a subset of a previous RFC bus lock patches that are simply good
cleanups that should help make it easier to introduce different locking
later.

Changes from v2:

  Rebased to pci/next

  Added memory barriers around bit ops for patch 1.

  Added reviews.

Keith Busch (5):
  pci: make pci_stop_dev concurrent safe
  pci: make pci_destroy_dev concurrent safe
  pci: move the walk bus lock to where its needed
  pci: walk bus recursively
  pci: unexport pci_walk_bus_locked

 drivers/pci/bus.c    | 49 +++++++++++++++-----------------------------
 drivers/pci/pci.h    | 17 +++++++++++++--
 drivers/pci/remove.c | 22 +++++++++-----------
 3 files changed, 41 insertions(+), 47 deletions(-)

-- 
2.43.5

[PATCHv3 1/5] pci: make pci_stop_dev concurrent safe

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

Use the atomic ADDED flag to safely ensure concurrent callers can't
attempt to stop the device multiple times. Callers should currently all
be holding the pci_rescan_remove_lock, so there shouldn't be an existing
race. But that global lock can cause lock dependency issues, so this is
preparing to reduce reliance on that lock by using the existing existing
atomic bit ops.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 drivers/pci/bus.c    |  2 +-
 drivers/pci/pci.h    | 11 +++++++++--
 drivers/pci/remove.c | 20 +++++++++-----------
 3 files changed, 19 insertions(+), 14 deletions(-)

diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
index e0a2441be6d32..aec08e81abff7 100644
--- a/drivers/pci/bus.c
+++ b/drivers/pci/bus.c
@@ -358,7 +358,7 @@ void pci_bus_add_device(struct pci_dev *dev)
 	if (retval < 0 && retval != -EPROBE_DEFER)
 		pci_warn(dev, "device attach failed (%d)\n", retval);
 
-	pci_dev_assign_added(dev, true);
+	pci_dev_assign_added(dev);
 
 	if (dev_of_node(&dev->dev) && pci_is_bridge(dev)) {
 		retval = of_platform_populate(dev_of_node(&dev->dev), NULL, NULL,
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index d89fdbf04f363..c4cceec006d0d 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -470,9 +470,16 @@ static inline int pci_dev_set_disconnected(struct pci_dev *dev, void *unused)
 #define PCI_DPC_RECOVERED 1
 #define PCI_DPC_RECOVERING 2
 
-static inline void pci_dev_assign_added(struct pci_dev *dev, bool added)
+static inline void pci_dev_assign_added(struct pci_dev *dev)
 {
-	assign_bit(PCI_DEV_ADDED, &dev->priv_flags, added);
+	smp_mb__before_atomic();
+	set_bit(PCI_DEV_ADDED, &dev->priv_flags);
+	smp_mb__after_atomic();
+}
+
+static inline bool pci_dev_test_and_clear_added(struct pci_dev *dev)
+{
+	return test_and_clear_bit(PCI_DEV_ADDED, &dev->priv_flags);
 }
 
 static inline bool pci_dev_is_added(const struct pci_dev *dev)
diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c
index e4ce1145aa3ee..e0d4402ec3391 100644
--- a/drivers/pci/remove.c
+++ b/drivers/pci/remove.c
@@ -31,18 +31,16 @@ static int pci_pwrctl_unregister(struct device *dev, void *data)
 
 static void pci_stop_dev(struct pci_dev *dev)
 {
-	pci_pme_active(dev, false);
-
-	if (pci_dev_is_added(dev)) {
-		device_for_each_child(dev->dev.parent, dev_of_node(&dev->dev),
-				      pci_pwrctl_unregister);
-		device_release_driver(&dev->dev);
-		pci_proc_detach_device(dev);
-		pci_remove_sysfs_dev_files(dev);
-		of_pci_remove_node(dev);
+	if (!pci_dev_test_and_clear_added(dev))
+		return;
 
-		pci_dev_assign_added(dev, false);
-	}
+	pci_pme_active(dev, false);
+	device_for_each_child(dev->dev.parent, dev_of_node(&dev->dev),
+			      pci_pwrctl_unregister);
+	device_release_driver(&dev->dev);
+	pci_proc_detach_device(dev);
+	pci_remove_sysfs_dev_files(dev);
+	of_pci_remove_node(dev);
 }
 
 static void pci_destroy_dev(struct pci_dev *dev)
-- 
2.43.5

[PATCHv3 2/5] pci: make pci_destroy_dev concurrent safe

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

Use an atomic flag instead of the racey check against the device's kobj
parent. We shouldn't be poking into device implementation details at
this level anyway.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 drivers/pci/pci.h    | 6 ++++++
 drivers/pci/remove.c | 2 +-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index c4cceec006d0d..cae4f55d5a4be 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -469,6 +469,7 @@ static inline int pci_dev_set_disconnected(struct pci_dev *dev, void *unused)
 #define PCI_DEV_ADDED 0
 #define PCI_DPC_RECOVERED 1
 #define PCI_DPC_RECOVERING 2
+#define PCI_DEV_REMOVED 3
 
 static inline void pci_dev_assign_added(struct pci_dev *dev)
 {
@@ -487,6 +488,11 @@ static inline bool pci_dev_is_added(const struct pci_dev *dev)
 	return test_bit(PCI_DEV_ADDED, &dev->priv_flags);
 }
 
+static inline bool pci_dev_test_and_set_removed(struct pci_dev *dev)
+{
+	return test_and_set_bit(PCI_DEV_REMOVED, &dev->priv_flags);
+}
+
 #ifdef CONFIG_PCIEAER
 #include <linux/aer.h>
 
diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c
index e0d4402ec3391..2e940101ce1bf 100644
--- a/drivers/pci/remove.c
+++ b/drivers/pci/remove.c
@@ -45,7 +45,7 @@ static void pci_stop_dev(struct pci_dev *dev)
 
 static void pci_destroy_dev(struct pci_dev *dev)
 {
-	if (!dev->dev.kobj.parent)
+	if (pci_dev_test_and_set_removed(dev))
 		return;
 
 	pci_npem_remove(dev);
-- 
2.43.5

[PATCHv3 3/5] pci: move the walk bus lock to where its needed

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

Simplify the common function by removing an unnecessary parameter that
can be more easily handled in the only caller that wants it.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Reviewed-by: Davidlohr Bueso <dave@stgolabs.net>
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 drivers/pci/bus.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
index aec08e81abff7..b863b8bdd6ee6 100644
--- a/drivers/pci/bus.c
+++ b/drivers/pci/bus.c
@@ -400,7 +400,7 @@ void pci_bus_add_devices(const struct pci_bus *bus)
 EXPORT_SYMBOL(pci_bus_add_devices);
 
 static void __pci_walk_bus(struct pci_bus *top, int (*cb)(struct pci_dev *, void *),
-			   void *userdata, bool locked)
+			   void *userdata)
 {
 	struct pci_dev *dev;
 	struct pci_bus *bus;
@@ -408,8 +408,6 @@ static void __pci_walk_bus(struct pci_bus *top, int (*cb)(struct pci_dev *, void
 	int retval;
 
 	bus = top;
-	if (!locked)
-		down_read(&pci_bus_sem);
 	next = top->devices.next;
 	for (;;) {
 		if (next == &bus->devices) {
@@ -432,8 +430,6 @@ static void __pci_walk_bus(struct pci_bus *top, int (*cb)(struct pci_dev *, void
 		if (retval)
 			break;
 	}
-	if (!locked)
-		up_read(&pci_bus_sem);
 }
 
 /**
@@ -451,7 +447,9 @@ static void __pci_walk_bus(struct pci_bus *top, int (*cb)(struct pci_dev *, void
  */
 void pci_walk_bus(struct pci_bus *top, int (*cb)(struct pci_dev *, void *), void *userdata)
 {
-	__pci_walk_bus(top, cb, userdata, false);
+	down_read(&pci_bus_sem);
+	__pci_walk_bus(top, cb, userdata);
+	up_read(&pci_bus_sem);
 }
 EXPORT_SYMBOL_GPL(pci_walk_bus);
 
@@ -459,7 +457,7 @@ void pci_walk_bus_locked(struct pci_bus *top, int (*cb)(struct pci_dev *, void *
 {
 	lockdep_assert_held(&pci_bus_sem);
 
-	__pci_walk_bus(top, cb, userdata, true);
+	__pci_walk_bus(top, cb, userdata);
 }
 EXPORT_SYMBOL_GPL(pci_walk_bus_locked);
 
-- 
2.43.5

[PATCHv3 4/5] pci: walk bus recursively

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

The original implementation purposefully chose a non-recursive walk,
presumably as a precaution on stack use. We do recursive bus walking in
other places though. For example:

  pci_bus_resettable
  pci_stop_bus_device
  pci_remove_bus_device
  pci_bus_allocate_dev_resources

So, recursive pci bus walking is well tested and safe, and the
implementation is easier to follow. The motivation for changing it now
is to make it easier to introduce finer grain locking in the future.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 drivers/pci/bus.c | 36 +++++++++++-------------------------
 1 file changed, 11 insertions(+), 25 deletions(-)

diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
index b863b8bdd6ee6..9c552396241e0 100644
--- a/drivers/pci/bus.c
+++ b/drivers/pci/bus.c
@@ -399,37 +399,23 @@ void pci_bus_add_devices(const struct pci_bus *bus)
 }
 EXPORT_SYMBOL(pci_bus_add_devices);
 
-static void __pci_walk_bus(struct pci_bus *top, int (*cb)(struct pci_dev *, void *),
-			   void *userdata)
+static int __pci_walk_bus(struct pci_bus *top, int (*cb)(struct pci_dev *, void *),
+			  void *userdata)
 {
 	struct pci_dev *dev;
-	struct pci_bus *bus;
-	struct list_head *next;
-	int retval;
+	int ret = 0;
 
-	bus = top;
-	next = top->devices.next;
-	for (;;) {
-		if (next == &bus->devices) {
-			/* end of this bus, go up or finish */
-			if (bus == top)
+	list_for_each_entry(dev, &top->devices, bus_list) {
+		ret = cb(dev, userdata);
+		if (ret)
+			break;
+		if (dev->subordinate) {
+			ret = __pci_walk_bus(dev->subordinate, cb, userdata);
+			if (ret)
 				break;
-			next = bus->self->bus_list.next;
-			bus = bus->self->bus;
-			continue;
 		}
-		dev = list_entry(next, struct pci_dev, bus_list);
-		if (dev->subordinate) {
-			/* this is a pci-pci bridge, do its devices next */
-			next = dev->subordinate->devices.next;
-			bus = dev->subordinate;
-		} else
-			next = dev->bus_list.next;
-
-		retval = cb(dev, userdata);
-		if (retval)
-			break;
 	}
+	return ret;
 }
 
 /**
-- 
2.43.5

[PATCHv3 5/5] pci: unexport pci_walk_bus_locked

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

There's only one user of this, and it's internal, so no need to export
it.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Reviewed-by: Davidlohr Bueso <dave@stgolabs.net>
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 drivers/pci/bus.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
index 9c552396241e0..d015d5821cefc 100644
--- a/drivers/pci/bus.c
+++ b/drivers/pci/bus.c
@@ -445,7 +445,6 @@ void pci_walk_bus_locked(struct pci_bus *top, int (*cb)(struct pci_dev *, void *
 
 	__pci_walk_bus(top, cb, userdata);
 }
-EXPORT_SYMBOL_GPL(pci_walk_bus_locked);
 
 struct pci_bus *pci_bus_get(struct pci_bus *bus)
 {
-- 
2.43.5

Re: [PATCHv3 5/5] pci: unexport pci_walk_bus_locked

[Bjorn Helgaas]

On Tue, Oct 22, 2024 at 03:48:51PM -0700, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
> 
> There's only one user of this, and it's internal, so no need to export
> it.
> 
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Reviewed-by: Davidlohr Bueso <dave@stgolabs.net>
> Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> Signed-off-by: Keith Busch <kbusch@kernel.org>
> ---
>  drivers/pci/bus.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
> index 9c552396241e0..d015d5821cefc 100644
> --- a/drivers/pci/bus.c
> +++ b/drivers/pci/bus.c
> @@ -445,7 +445,6 @@ void pci_walk_bus_locked(struct pci_bus *top, int (*cb)(struct pci_dev *, void *
>  
>  	__pci_walk_bus(top, cb, userdata);
>  }
> -EXPORT_SYMBOL_GPL(pci_walk_bus_locked);

I think we could also move the declaration from include/linux/pci.h to
drivers/pci/pci.h, right?

I guess there's some argument for keeping it in include/linux/pci.h
next to the pci_walk_bus() declaration, but I certainly don't want to
encourage more use of either one, especially outside the PCI core.

Re: [PATCHv3 5/5] pci: unexport pci_walk_bus_locked

[Keith Busch]

On Wed, Oct 23, 2024 at 04:43:20PM -0500, Bjorn Helgaas wrote:
> On Tue, Oct 22, 2024 at 03:48:51PM -0700, Keith Busch wrote:
> > @@ -445,7 +445,6 @@ void pci_walk_bus_locked(struct pci_bus *top, int (*cb)(struct pci_dev *, void *
> >  
> >  	__pci_walk_bus(top, cb, userdata);
> >  }
> > -EXPORT_SYMBOL_GPL(pci_walk_bus_locked);
> 
> I think we could also move the declaration from include/linux/pci.h to
> drivers/pci/pci.h, right?
> 
> I guess there's some argument for keeping it in include/linux/pci.h
> next to the pci_walk_bus() declaration, but I certainly don't want to
> encourage more use of either one, especially outside the PCI core.

Thanks, that's a good point! Not only are modules not using this,
neither does any kernel code outside drivers/pci/, so no need to
declare this in include/linux/.

Re: [PATCHv3 5/5] pci: unexport pci_walk_bus_locked

[Bjorn Helgaas]

On Wed, Oct 23, 2024 at 04:00:42PM -0600, Keith Busch wrote:
> On Wed, Oct 23, 2024 at 04:43:20PM -0500, Bjorn Helgaas wrote:
> > On Tue, Oct 22, 2024 at 03:48:51PM -0700, Keith Busch wrote:
> > > @@ -445,7 +445,6 @@ void pci_walk_bus_locked(struct pci_bus *top, int (*cb)(struct pci_dev *, void *
> > >  
> > >  	__pci_walk_bus(top, cb, userdata);
> > >  }
> > > -EXPORT_SYMBOL_GPL(pci_walk_bus_locked);
> > 
> > I think we could also move the declaration from include/linux/pci.h to
> > drivers/pci/pci.h, right?
> > 
> > I guess there's some argument for keeping it in include/linux/pci.h
> > next to the pci_walk_bus() declaration, but I certainly don't want to
> > encourage more use of either one, especially outside the PCI core.
> 
> Thanks, that's a good point! Not only are modules not using this,
> neither does any kernel code outside drivers/pci/, so no need to
> declare this in include/linux/.

I'll make this change locally so you don't need to repost for it.

Re: [PATCHv3 0/5] pci cleanup/prep patches

[Bjorn Helgaas]

On Tue, Oct 22, 2024 at 03:48:46PM -0700, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
> 
> This is a subset of a previous RFC bus lock patches that are simply good
> cleanups that should help make it easier to introduce different locking
> later.
> 
> Changes from v2:
> 
>   Rebased to pci/next
> 
>   Added memory barriers around bit ops for patch 1.
> 
>   Added reviews.
> 
> Keith Busch (5):
>   pci: make pci_stop_dev concurrent safe
>   pci: make pci_destroy_dev concurrent safe
>   pci: move the walk bus lock to where its needed
>   pci: walk bus recursively
>   pci: unexport pci_walk_bus_locked
> 
>  drivers/pci/bus.c    | 49 +++++++++++++++-----------------------------
>  drivers/pci/pci.h    | 17 +++++++++++++--
>  drivers/pci/remove.c | 22 +++++++++-----------
>  3 files changed, 41 insertions(+), 47 deletions(-)

Applied to pci/locking for v6.13, thanks Keith!

Re: [PATCHv3 1/5] pci: make pci_stop_dev concurrent safe

[Lukas Wunner]

On Tue, Oct 22, 2024 at 03:48:47PM -0700, Keith Busch wrote:
> --- a/drivers/pci/remove.c
> +++ b/drivers/pci/remove.c
> @@ -31,18 +31,16 @@ static int pci_pwrctl_unregister(struct device *dev, void *data)
>  
>  static void pci_stop_dev(struct pci_dev *dev)
>  {
> -	pci_pme_active(dev, false);
> -
> -	if (pci_dev_is_added(dev)) {
> -		device_for_each_child(dev->dev.parent, dev_of_node(&dev->dev),
> -				      pci_pwrctl_unregister);
> -		device_release_driver(&dev->dev);
> -		pci_proc_detach_device(dev);
> -		pci_remove_sysfs_dev_files(dev);
> -		of_pci_remove_node(dev);
> +	if (!pci_dev_test_and_clear_added(dev))
> +		return;
>  
> -		pci_dev_assign_added(dev, false);
> -	}
> +	pci_pme_active(dev, false);
> +	device_for_each_child(dev->dev.parent, dev_of_node(&dev->dev),
> +			      pci_pwrctl_unregister);
> +	device_release_driver(&dev->dev);
> +	pci_proc_detach_device(dev);
> +	pci_remove_sysfs_dev_files(dev);
> +	of_pci_remove_node(dev);
>  }

The above is now queued for v6.13 as commit 6d6d962a8dc2 on pci/locking.

I note there's a behavioral change here:

Previously "pci_pme_active(dev, false)" was called unconditionally,
now only if the "added" flag has been set.  The commit message
doesn't explain why this change is fine, so perhaps it's inadvertent?

Thanks,

Lukas

Re: [PATCHv3 1/5] pci: make pci_stop_dev concurrent safe

[Keith Busch]

On Thu, Nov 07, 2024 at 03:06:57PM +0100, Lukas Wunner wrote:
> On Tue, Oct 22, 2024 at 03:48:47PM -0700, Keith Busch wrote:
> > --- a/drivers/pci/remove.c
> > +++ b/drivers/pci/remove.c
> > @@ -31,18 +31,16 @@ static int pci_pwrctl_unregister(struct device *dev, void *data)
> >  
> >  static void pci_stop_dev(struct pci_dev *dev)
> >  {
> > -	pci_pme_active(dev, false);
> > -
> > -	if (pci_dev_is_added(dev)) {
> > -		device_for_each_child(dev->dev.parent, dev_of_node(&dev->dev),
> > -				      pci_pwrctl_unregister);
> > -		device_release_driver(&dev->dev);
> > -		pci_proc_detach_device(dev);
> > -		pci_remove_sysfs_dev_files(dev);
> > -		of_pci_remove_node(dev);
> > +	if (!pci_dev_test_and_clear_added(dev))
> > +		return;
> >  
> > -		pci_dev_assign_added(dev, false);
> > -	}
> > +	pci_pme_active(dev, false);
> > +	device_for_each_child(dev->dev.parent, dev_of_node(&dev->dev),
> > +			      pci_pwrctl_unregister);
> > +	device_release_driver(&dev->dev);
> > +	pci_proc_detach_device(dev);
> > +	pci_remove_sysfs_dev_files(dev);
> > +	of_pci_remove_node(dev);
> >  }
> 
> The above is now queued for v6.13 as commit 6d6d962a8dc2 on pci/locking.
> 
> I note there's a behavioral change here:
> 
> Previously "pci_pme_active(dev, false)" was called unconditionally,
> now only if the "added" flag has been set.  The commit message
> doesn't explain why this change is fine, so perhaps it's inadvertent?

Hm, not exactly intentional. It doesn't appear to accomplish anything to
call it multiple times, but it also looks hamrless to do so.  Looking at
the history of this, it looks like it was purposefully done
unconditionally with the understanding it's "safe" to do that. With that
in mind, I'm happy to move it back where it was.

Re: [PATCHv3 1/5] pci: make pci_stop_dev concurrent safe

[Lukas Wunner]

On Thu, Nov 07, 2024 at 08:56:26AM -0700, Keith Busch wrote:
> On Thu, Nov 07, 2024 at 03:06:57PM +0100, Lukas Wunner wrote:
> > On Tue, Oct 22, 2024 at 03:48:47PM -0700, Keith Busch wrote:
> > > --- a/drivers/pci/remove.c
> > > +++ b/drivers/pci/remove.c
> > > @@ -31,18 +31,16 @@ static int pci_pwrctl_unregister(struct device *dev, void *data)
> > >  
> > >  static void pci_stop_dev(struct pci_dev *dev)
> > >  {
> > > -	pci_pme_active(dev, false);
> > > -
> > > -	if (pci_dev_is_added(dev)) {
> > > -		device_for_each_child(dev->dev.parent, dev_of_node(&dev->dev),
> > > -				      pci_pwrctl_unregister);
> > > -		device_release_driver(&dev->dev);
> > > -		pci_proc_detach_device(dev);
> > > -		pci_remove_sysfs_dev_files(dev);
> > > -		of_pci_remove_node(dev);
> > > +	if (!pci_dev_test_and_clear_added(dev))
> > > +		return;
> > >  
> > > -		pci_dev_assign_added(dev, false);
> > > -	}
> > > +	pci_pme_active(dev, false);
> > > +	device_for_each_child(dev->dev.parent, dev_of_node(&dev->dev),
> > > +			      pci_pwrctl_unregister);
> > > +	device_release_driver(&dev->dev);
> > > +	pci_proc_detach_device(dev);
> > > +	pci_remove_sysfs_dev_files(dev);
> > > +	of_pci_remove_node(dev);
> > >  }
> > 
> > The above is now queued for v6.13 as commit 6d6d962a8dc2 on pci/locking.
> > 
> > I note there's a behavioral change here:
> > 
> > Previously "pci_pme_active(dev, false)" was called unconditionally,
> > now only if the "added" flag has been set.  The commit message
> > doesn't explain why this change is fine, so perhaps it's inadvertent?
> 
> Hm, not exactly intentional. It doesn't appear to accomplish anything to
> call it multiple times, but it also looks hamrless to do so.  Looking at
> the history of this, it looks like it was purposefully done
> unconditionally with the understanding it's "safe" to do that. With that
> in mind, I'm happy to move it back where it was.

Yes I think it would be good if you could submit a fixup that Bjorn
could fold into 6d6d962a8dc2, just to minimize regression potential.

Thanks,

Lukas

Re: [PATCHv3 4/5] pci: walk bus recursively

[Lukas Wunner]

On Tue, Oct 22, 2024 at 03:48:50PM -0700, Keith Busch wrote:
> The original implementation purposefully chose a non-recursive walk,
> presumably as a precaution on stack use. We do recursive bus walking in
> other places though. For example:
> 
>   pci_bus_resettable
>   pci_stop_bus_device
>   pci_remove_bus_device
>   pci_bus_allocate_dev_resources
> 
> So, recursive pci bus walking is well tested and safe, and the
> implementation is easier to follow. The motivation for changing it now
> is to make it easier to introduce finer grain locking in the future.

Hm, I find the loss of non-recursive bus walking regrettable.

If the solution proposed earlier today...

https://lore.kernel.org/all/ZzG5koPOn16KQ8uM@wunner.de/

... is workable and we find it doesn't need recursive bus walking,
perhaps we should consider reverting this change (which is now
e24eafdda271 on pci/locking).

Thanks,

Lukas