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

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

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

Right, happy to go cleanup old code if there is appetite for it, but I
am not sure it was worth destroying the blame history for that. In the
meantime, keep the new helpers "style-bug" compatible.