diff mbox series

[RFC,4/8] pci: walk bus recursively

Message ID 20240722151936.1452299-5-kbusch@meta.com (mailing list archive)
State RFC
Delegated to: Bjorn Helgaas
Headers show
Series pci: rescan/remove locking rework | expand

Commit Message

Keith Busch July 22, 2024, 3:19 p.m. UTC
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.

Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 drivers/pci/bus.c | 34 ++++++++++------------------------
 1 file changed, 10 insertions(+), 24 deletions(-)

Comments

Jonathan Cameron Aug. 15, 2024, 2:33 p.m. UTC | #1
On Mon, 22 Jul 2024 08:19:32 -0700
Keith Busch <kbusch@meta.com> wrote:

> 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.
> 
> Signed-off-by: Keith Busch <kbusch@kernel.org>
Trivial naming question inline. Otherwise LGTM
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/pci/bus.c | 34 ++++++++++------------------------
>  1 file changed, 10 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
> index 7c07a141e8772..b7208e644c79f 100644
> --- a/drivers/pci/bus.c
> +++ b/drivers/pci/bus.c
> @@ -389,37 +389,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 *),
> +static int __pci_walk_bus(struct pci_bus *top, int (*cb)(struct pci_dev *, void *),
Keeping the parameter name of 'top' seems less than intuitive
now this is recursive as on the recursions it is no longer the top.
Maybe just call it bus?  That will make this diff really confusing however.

>  			   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;
>  }
>  
>  /**
diff mbox series

Patch

diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
index 7c07a141e8772..b7208e644c79f 100644
--- a/drivers/pci/bus.c
+++ b/drivers/pci/bus.c
@@ -389,37 +389,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 *),
+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;
 }
 
 /**