Re: [PATCH v7 06/12] PCI: liveupdate: Auto-preserve upstream bridges across Live Update

[David Matlack]

On Fri, Jul 24, 2026 at 1:55 PM Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote:
>
> On Fri, Jul 10, 2026 at 09:26:09PM +0000, David Matlack wrote:

> > -static int pci_liveupdate_preserve_device(struct pci_flb_outgoing *outgoing, struct pci_dev *dev)
> > +static int pci_liveupdate_preserve_device_again(struct pci_dev *dev)
>
> Could pci_liveupdate_preserve_device_again() be folded into
> pci_liveupdate_preserve_device()?  That's the only caller, and having
> so many *liveupdate_preserve*() functions gets to be a bit of a maze
> of small functions:
>
>   __pci_liveupdate_preserve_device
>   pci_liveupdate_preserve_device_again
>   pci_liveupdate_preserve_device
>   pci_liveupdate_preserve_path
>   pci_liveupdate_preserve
>
> Maybe even more could be squashed; pci_liveupdate_preserve() is the
> only caller of pci_liveupdate_preserve_path(), which is itself the
> only caller of pci_liveupdate_preserve_device().
>
> As a reader, I think there's some advantage in collecting the whole
> "preserve" flow in one place, but maybe nesting depth would be an
> issue, especially for the unpreserve cleanup in the failure path.
> Maybe a "goto" error path could mitigate part of that.
>
> If there's any squashing, I guess the place to start would be in
> earlier patches, e.g., "PCI: liveupdate: Track outgoing preserved PCI
> devices" and "PCI: liveupdate: Track incoming preserved PCI devices".

I'll see what I can do in v8 to avoid splitting the
preserve/unpreserve logic across so many functions. I suspect it can
go from 5 to 2-3 while still having a reasonable amount of nesting.

> > +#define for_each_pci_dev_in_path(_d, _start, _end) \
> > +     for ((_d) = (_start); (_d) != (_end); (_d) = (_d)->bus->self)
>
> I think you might want pci_upstream_bridge() instead of (_d)->bus->self;
> see https://git.kernel.org/linus/c6bde215acfd ("PCI: Add
> pci_upstream_bridge()").  I think VFs are currently out of scope, but
> no harm in making this future-proof.

Yes in fact I noticed pci_upstream_bridge() while working on other
changes in v8 and already swapped to it :). It also makes the code
more readable.

> Other than "_end", this looks fairly generic and might be a candidate
> for putting in include/linux/pci.h.  I think there might be other
> potential users, e.g., calc_map_type_and_dist(),
> pci_enable_atomic_ops_to_root(), pcie_bandwidth_available(),
> get_upstream_port(), pcie_find_root_port(),
> pci_is_thunderbolt_attached().
>
> The "_end" termination case could perhaps be done in the body instead
> of the macro since it depends on the use case and several of the other
> loops that iterate over the path already include terminations in the
> body.

Sounds good. I'll move it to include/linux/pci.h and and handle _end
in the loop bodies. Then once this series lands I can send a follow up
to convert those users to use the new iterator.