On Tue, 27 Aug 2024, Keith Busch wrote:
> From: Keith Busch <kbusch@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> Signed-off-by: Keith Busch <kbusch@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxx>
--
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;
> }
>
> /**
>
