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

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 2597 bytes --]

On Tue, 27 Aug 2024, Keith Busch 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.
> 
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.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 7c07a141e8772..8491e9c7f0586 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 *),
> -			   void *userdata)
> +static int __pci_walk_bus(struct pci_bus *top, int (*cb)(struct pci_dev *, void *),

I guess the reason why this parameter was called "top" is now gone so you 
could just name it "bus"?

Other than that, the change looks okay,

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

-- 
 i.

> +			  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;
>  }
>  
>  /**
>