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

[Bjorn Helgaas <helgaas@kernel.org>]

On Thu, Jul 17, 2025 at 11:33:51AM -0700, Dan Williams 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.

s/SRIOV/SR-IOV/

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

I really don't like these search and iterator interfaces.  I wish we
didn't need them like this because code that uses them becomes a
one-time thing that doesn't handle hotplug and has potential locking
and race issues.  But I assume you really do need these.

> +++ b/drivers/base/bus.c
> +static struct device *prev_device(struct klist_iter *i)
> +{
> +	struct klist_node *n = klist_prev(i);
> +	struct device *dev = NULL;
> +	struct device_private *dev_prv;
> +
> +	if (n) {
> +		dev_prv = to_device_private_bus(n);
> +		dev = dev_prv->device;
> +	}
> +	return dev;

I think this would be simpler as:

  if (!n)
    return NULL;

  dev_prv = to_device_private_bus(n);
  return dev_prv->device;

> +++ b/drivers/pci/bus.c
> +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;

Why not:

  list_for_each_entry_reverse(...) {
    ...
    if (ret)
      return ret;
  }
  return 0;