Re: [PATCH v7 3/9] PCI: Introduce pci_walk_bus_reverse(), for_each_pci_dev_reverse()

[Jonathan Cameron <jonathan.cameron@huawei.com>]

On Thu, 23 Oct 2025 19:04:12 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> PCI/TSM, the PCI core functionality for the PCIe TEE Device Interface
> Security Protocol (TDISP), has a need to walk all subordinate functions of
> a Device Security Manager (DSM) to setup a device security context. A DSM
> is physical function 0 of multi-function or SR-IOV device endpoint, or it
> is an upstream switch port.
> 
> In error scenarios or when a TEE Security Manager (TSM) device is removed
> it needs to unwind all established DSM contexts.
> 
> Introduce reverse versions of PCI device iteration helpers to mirror the
> setup path and ensure that dependent children are handled before parents.
> 
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

Bit of archaeology was needed as there are some existing oddities in the
functions this is based on.

My suggestions for this are don't use guard() and drop the void * cast that
we should cleanup in the existing code.

> diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
> index f26aec6ff588..1c981ca72b03 100644
> --- a/drivers/pci/bus.c
> +++ b/drivers/pci/bus.c
> @@ -8,6 +8,7 @@
>   */
>  #include <linux/module.h>
>  #include <linux/kernel.h>
> +#include <linux/cleanup.h>
>  #include <linux/pci.h>
>  #include <linux/errno.h>
>  #include <linux/ioport.h>
> @@ -432,6 +433,27 @@ static int __pci_walk_bus(struct pci_bus *top, int (*cb)(struct pci_dev *, void
>  	return ret;
>  }
>  
> +static int __pci_walk_bus_reverse(struct pci_bus *top,
> +				  int (*cb)(struct pci_dev *, void *),
> +				  void *userdata)
> +{
> +	struct pci_dev *dev;
> +	int ret = 0;
> +
> +	list_for_each_entry_reverse(dev, &top->devices, bus_list) {
> +		if (dev->subordinate) {
> +			ret = __pci_walk_bus_reverse(dev->subordinate, cb,
> +						     userdata);
> +			if (ret)
> +				break;
> +		}
> +		ret = cb(dev, userdata);
> +		if (ret)
> +			break;
> +	}
> +	return ret;

I see this is local style so fair enough, but I'd have gone with early
returns as it's a simple case of return ret if it is ever set.

> +}

> +/**
> + * pci_walk_bus_reverse - walk devices on/under bus, calling callback.
> + * @top: bus whose devices should be walked
> + * @cb: callback to be called for each device found
> + * @userdata: arbitrary pointer to be passed to callback
> + *
> + * Same semantics as pci_walk_bus(), but walks the bus in reverse order.
> + */
> +void pci_walk_bus_reverse(struct pci_bus *top,
> +			  int (*cb)(struct pci_dev *, void *), void *userdata)
> +{
> +	guard(rwsem_read)(&pci_bus_sem);

So this ends up different to pci_walk_bus.  I'd be tempted to just
not bother bringing a single guard() usage here. Gain is trivial and
mixing and matching style in a file isn't particularly nice.

I'd not mind changing pci_walk_bus() as well but that would need
to be a trivial precursor patch I think.

> +	__pci_walk_bus_reverse(top, cb, userdata);
> +}
> +EXPORT_SYMBOL_GPL(pci_walk_bus_reverse);
> +

> diff --git a/drivers/pci/search.c b/drivers/pci/search.c
> index 53840634fbfc..e6e84dc62e82 100644
> --- a/drivers/pci/search.c
> +++ b/drivers/pci/search.c
> @@ -282,6 +282,45 @@ static struct pci_dev *pci_get_dev_by_id(const struct pci_device_id *id,
>  	return pdev;
>  }
>  
> +static struct pci_dev *pci_get_dev_by_id_reverse(const struct pci_device_id *id,
> +						 struct pci_dev *from)
> +{
> +	struct device *dev;
> +	struct device *dev_start = NULL;
> +	struct pci_dev *pdev = NULL;
> +
> +	if (from)
> +		dev_start = &from->dev;
> +	dev = bus_find_device_reverse(&pci_bus_type, dev_start, (void *)id,

That (void *) is casting away a const but bus_find_device_reverse takes
a const void *.
I think you are fine just relying on implicit cast for that parameter.

Not that important and pci_get_device_by_id() does have same odd casting.
Looks like way back bus_find_device() didn't take a const pointer 

Seems to be true in 3.19 (random choice jumping back through time on elixir)
but not sure when it changed.

Anyhow, would be nice to clean that up in existing code if anyone is bored
enough.

> +				      match_pci_dev_by_id);
> +	if (dev)
> +		pdev = to_pci_dev(dev);
> +	pci_dev_put(from);
> +	return pdev;
> +}