linux-kernel - Re: [PATCH v4 03/10] PCI: Introduce pci_walk_bus_reverse(), for_each_pci_dev

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <6892993f3ed0e_55f0910079@dwillia2-xfh.jf.intel.com.notmuch>
Date: Tue, 5 Aug 2025 16:52:31 -0700
From: <dan.j.williams@...el.com>
To: Jonathan Cameron <Jonathan.Cameron@...wei.com>, Dan Williams
	<dan.j.williams@...el.com>
CC: <linux-coco@...ts.linux.dev>, <linux-pci@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>, <bhelgaas@...gle.com>, <aik@....com>,
	<lukas@...ner.de>
Subject: Re: [PATCH v4 03/10] PCI: Introduce pci_walk_bus_reverse(),
 for_each_pci_dev_reverse()

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