Re: [PATCH v4 03/10] PCI: Introduce pci_walk_bus_reverse(), for_each_pci_dev_reverse()

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

On Thu, 17 Jul 2025 11:33:51 -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 SRIOV 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.
> 
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

A couple of trivial comments.

Probably want to +CC Greg KH on next version given bits in drivers/base

Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>

> diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
> index 69048869ef1c..d894c87ce1fd 100644
> --- a/drivers/pci/bus.c
> +++ b/drivers/pci/bus.c

include cleanup.h perhaps for access to guard()?


> diff --git a/drivers/pci/search.c b/drivers/pci/search.c
> index 53840634fbfc..7a4623f65256 100644
> --- a/drivers/pci/search.c
> +++ b/drivers/pci/search.c
> @@ -282,6 +282,46 @@ 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,
> +				      match_pci_dev_by_id);
> +	if (dev)
> +		pdev = to_pci_dev(dev);
> +	pci_dev_put(from);
> +	return pdev;
> +}
> +
> +enum pci_search_direction {
> +	PCI_SEARCH_FORWARD,
> +	PCI_SEARCH_REVERSE,
> +};
> +

I don't really care, but given there are only two sane directions maybe
a bool reverse as a parameter to __pci_get_subsys() would be sufficient? 

> +static struct pci_dev *__pci_get_subsys(unsigned int vendor, unsigned int device,
> +				 unsigned int ss_vendor, unsigned int ss_device,
> +				 struct pci_dev *from, enum pci_search_direction dir)
> +{
> +	struct pci_device_id id = {
> +		.vendor = vendor,
> +		.device = device,
> +		.subvendor = ss_vendor,
> +		.subdevice = ss_device,
> +	};
> +
> +	if (dir == PCI_SEARCH_FORWARD)
> +		return pci_get_dev_by_id(&id, from);
> +	else
> +		return pci_get_dev_by_id_reverse(&id, from);
> +}
> +
This file seems to use 1 blank line only between functions.
> +
>  /**
>   * pci_get_subsys - begin or continue searching for a PCI device by vendor/subvendor/device/subdevice id
>   * @vendor: PCI vendor id to match, or %PCI_ANY_ID to match all vendor ids
> @@ -302,14 +342,8 @@ struct pci_dev *pci_get_subsys(unsigned int vendor, unsigned int device,
>  			       unsigned int ss_vendor, unsigned int ss_device,
>  			       struct pci_dev *from)
>  {
> -	struct pci_device_id id = {
> -		.vendor = vendor,
> -		.device = device,
> -		.subvendor = ss_vendor,
> -		.subdevice = ss_device,
> -	};
> -
> -	return pci_get_dev_by_id(&id, from);
> +	return __pci_get_subsys(vendor, device, ss_vendor, ss_device, from,
> +				PCI_SEARCH_FORWARD);
>  }
>  EXPORT_SYMBOL(pci_get_subsys);
>  
> @@ -334,6 +368,19 @@ struct pci_dev *pci_get_device(unsigned int vendor, unsigned int device,
>  }
>  EXPORT_SYMBOL(pci_get_device);
>  
> +/*
> + * Same semantics as pci_get_device(), except walks the PCI device list
> + * in reverse discovery order.
> + */
> +struct pci_dev *pci_get_device_reverse(unsigned int vendor,
> +				       unsigned int device,
> +				       struct pci_dev *from)
> +{
> +	return __pci_get_subsys(vendor, device, PCI_ANY_ID, PCI_ANY_ID, from,
> +				PCI_SEARCH_REVERSE);
> +}
> +EXPORT_SYMBOL(pci_get_device_reverse);
> +
>  /**
>   * pci_get_class - begin or continue searching for a PCI device by class
>   * @class: search for a PCI device with this class designation
> diff --git a/include/linux/device/bus.h b/include/linux/device/bus.h
> index f5a56efd2bd6..99b1002b3e31 100644
> --- a/include/linux/device/bus.h
> +++ b/include/linux/device/bus.h
> @@ -150,6 +150,9 @@ int bus_for_each_dev(const struct bus_type *bus, struct device *start,
>  		     void *data, device_iter_t fn);
>  struct device *bus_find_device(const struct bus_type *bus, struct device *start,
>  			       const void *data, device_match_t match);
> +struct device *bus_find_device_reverse(const struct bus_type *bus,
> +				       struct device *start, const void *data,
> +				       device_match_t match);
>  /**
>   * bus_find_device_by_name - device iterator for locating a particular device
>   * of a specific name.
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 3fac811376b5..b8bca0711967 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -575,6 +575,8 @@ struct pci_dev *pci_alloc_dev(struct pci_bus *bus);
>  
>  #define	to_pci_dev(n) container_of(n, struct pci_dev, dev)
>  #define for_each_pci_dev(d) while ((d = pci_get_device(PCI_ANY_ID, PCI_ANY_ID, d)) != NULL)
> +#define for_each_pci_dev_reverse(d) \
> +	while ((d = pci_get_device_reverse(PCI_ANY_ID, PCI_ANY_ID, d)) != NULL)
>  
>  static inline int pci_channel_offline(struct pci_dev *pdev)
>  {
> @@ -1220,6 +1222,8 @@ u64 pci_get_dsn(struct pci_dev *dev);
>  
>  struct pci_dev *pci_get_device(unsigned int vendor, unsigned int device,
>  			       struct pci_dev *from);
> +struct pci_dev *pci_get_device_reverse(unsigned int vendor, unsigned int device,
> +				       struct pci_dev *from);
>  struct pci_dev *pci_get_subsys(unsigned int vendor, unsigned int device,
>  			       unsigned int ss_vendor, unsigned int ss_device,
>  			       struct pci_dev *from);
> @@ -1639,6 +1643,8 @@ int pci_scan_bridge(struct pci_bus *bus, struct pci_dev *dev, int max,
>  
>  void pci_walk_bus(struct pci_bus *top, int (*cb)(struct pci_dev *, void *),
>  		  void *userdata);
> +void pci_walk_bus_reverse(struct pci_bus *top,
> +			  int (*cb)(struct pci_dev *, void *), void *userdata);
>  int pci_cfg_space_size(struct pci_dev *dev);
>  unsigned char pci_bus_max_busnr(struct pci_bus *bus);
>  resource_size_t pcibios_window_alignment(struct pci_bus *bus,
> @@ -2031,6 +2037,11 @@ static inline struct pci_dev *pci_get_device(unsigned int vendor,
>  					     struct pci_dev *from)
>  { return NULL; }
>  
> +static inline struct pci_dev *pci_get_device_reverse(unsigned int vendor,
> +						     unsigned int device,
> +						     struct pci_dev *from)
> +{ return NULL; }
> +
>  static inline struct pci_dev *pci_get_subsys(unsigned int vendor,
>  					     unsigned int device,
>  					     unsigned int ss_vendor,