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

dan.j.williams
2025-10-29T16:05:53Z

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.