Re: [PATCH RFC 6/8] pci: add helpers for stop and remove bus

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Mon, 22 Jul 2024 08:19:34 -0700
Keith Busch <kbusch@meta.com> wrote:

> From: Keith Busch <kbusch@kernel.org>
> 
> There are repeated patterns of tearing down pci buses, so combine to
> helper functions and use these.
There are some subtle changes in ordering in here. I'm not
immediately convinced by all of them.

Perhaps this should be broken down further so we get the direct
code replacements that are easy to review, the movement of calls
to different functions (e.g. addition of pci_clear_bus() in 
pci_remove_bus() and dropping that call, or the code that it matches
in two other places).


> 
> Signed-off-by: Keith Busch <kbusch@kernel.org>
> ---
>  drivers/pci/remove.c | 46 +++++++++++++++++++++++---------------------
>  1 file changed, 24 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c
> index 8284ab20949c9..288162a11ab19 100644
> --- a/drivers/pci/remove.c
> +++ b/drivers/pci/remove.c
> @@ -4,6 +4,9 @@
>  #include <linux/of_platform.h>
>  #include "pci.h"
>  
> +static void pci_stop_bus(struct pci_bus *bus);
> +static void pci_remove_bus_device(struct pci_dev *dev);
> +
>  static void pci_free_resources(struct pci_dev *dev)
>  {
>  	struct resource *res;
> @@ -45,8 +48,17 @@ static void pci_destroy_dev(struct pci_dev *dev)
>  	put_device(&dev->dev);
>  }
>  
> +static void pci_clear_bus(struct pci_bus *bus)
> +{
> +	struct pci_dev *dev, *next;
> +
> +	list_for_each_entry_safe(dev, next, &bus->devices, bus_list)
> +		pci_remove_bus_device(dev);
> +}
> +
>  void pci_remove_bus(struct pci_bus *bus)
>  {
> +	pci_clear_bus(bus);
So this is replacing the list_for_each_entry_safe block that
was previously in pci_remove_root_bus / pci_remove_bus_device but there
are other callers of this function such as in xen-pcifront.c which
are going to see this change.

>  	pci_proc_detach_bus(bus);
>  
>  	down_write(&pci_bus_sem);
> @@ -66,7 +78,15 @@ EXPORT_SYMBOL(pci_remove_bus);

>  
>  static void pci_remove_bus_device(struct pci_dev *dev)
>  {
>  	struct pci_bus *bus = dev->subordinate;
> -	struct pci_dev *child, *tmp;
>  
>  	if (bus) {
> -		list_for_each_entry_safe(child, tmp,
> -					 &bus->devices, bus_list)
> -			pci_remove_bus_device(child);
> -
>  		pci_remove_bus(bus);
>  		dev->subordinate = NULL;
>  	}
> -
Grumpy reviewer hat.  Unrelated change.

>  	pci_destroy_dev(dev);
>  }
>  
> @@ -129,16 +138,13 @@ EXPORT_SYMBOL_GPL(pci_stop_and_remove_bus_device_locked);
>  
>  void pci_stop_root_bus(struct pci_bus *bus)
>  {
> -	struct pci_dev *child, *tmp;
>  	struct pci_host_bridge *host_bridge;
>  
>  	if (!pci_is_root_bus(bus))
>  		return;
>  
>  	host_bridge = to_pci_host_bridge(bus->bridge);
> -	list_for_each_entry_safe_reverse(child, tmp,
> -					 &bus->devices, bus_list)
> -		pci_stop_bus_device(child);
> +	pci_stop_bus(bus);
>  
>  	/* stop the host bridge */
>  	device_release_driver(&host_bridge->dev);
> @@ -147,16 +153,12 @@ EXPORT_SYMBOL_GPL(pci_stop_root_bus);
>  
>  void pci_remove_root_bus(struct pci_bus *bus)
>  {
> -	struct pci_dev *child, *tmp;
>  	struct pci_host_bridge *host_bridge;
>  
>  	if (!pci_is_root_bus(bus))
>  		return;
>  
>  	host_bridge = to_pci_host_bridge(bus->bridge);
> -	list_for_each_entry_safe(child, tmp,
> -				 &bus->devices, bus_list)
> -		pci_remove_bus_device(child);
>  
>  #ifdef CONFIG_PCI_DOMAINS_GENERIC
>  	/* Release domain_nr if it was dynamically allocated */