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

[<dan.j.williams@intel.com>]

Jonathan Cameron wrote:
> 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

Oh, true. On last revision I copied him on whole series. Missed that this
time.

> 
> 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()?

Sure.

> > 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? 

I dislike reading:

   return __pci_get_subsys(vendor, device, ss_vendor, ss_device, from, false);

...in isolation where I must walk the symbol to the function to figure
out what that parameter means vs:

   return __pci_get_subsys(vendor, device, ss_vendor, ss_device, from,
                           PCI_SEARCH_FORWARD);

...which is immediately clear.

> 
> > +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.

ok.