Jonathan Cameron wrote:
> On Thu, 17 Jul 2025 11:33:51 -0700
> Dan Williams <dan.j.williams@xxxxxxxxx> 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@xxxxxxxxx>
>
> 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@xxxxxxxxxx>
>
> > 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.
