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

[Pranjal Shrivastava <praan@google.com>]

On Mon, Jun 08, 2026 at 09:34:57PM +0000, David Matlack wrote:
> On 2026-06-06 10:15 PM, Pranjal Shrivastava wrote:
> > On Fri, May 22, 2026 at 08:24:04PM +0000, David Matlack wrote:
> > > When a PCI device is preserved across a Live Update, all of its upstream
> > > bridges up to the root port must also be preserved. This enables the PCI
> > > core and any drivers bound to the bridges to manage bridges correctly
> > > across a Live Update.
> > > 
> > > Notably, this will be used in subsequent commits to ensure that
> > > preserved devices can continue performing memory transactions without a
> > > disruption or change in routing.
> > > 
> > > To preserve bridges, the PCI core tracks the number of downstream
> > > devices preserved under each bridge using a reference count in struct
> > > pci_dev_ser. This allows a bridge to remain preserved until all its
> > > downstream preserved devices are unpreserved or finish their
> > > participation in the Live Update.
> > > 
> > > Signed-off-by: David Matlack <dmatlack@google.com>
> > > ---
> > >  drivers/pci/liveupdate.c    | 136 +++++++++++++++++++++++++++++++-----
> > >  include/linux/kho/abi/pci.h |   5 +-
> > >  2 files changed, 122 insertions(+), 19 deletions(-)
> > > 
> > 
> > [...]
> > 
> > > +
> > > +#define for_each_pci_dev_in_path(_d, _start, _end) \
> > > +	for ((_d) = (_start); (_d) != (_end); (_d) = (_d)->bus->self)
> > > +
> > > +static void __pci_liveupdate_unpreserve_path(struct pci_ser *ser,
> > > +					     struct pci_dev *start,
> > > +					     struct pci_dev *end)
> > > +{
> > > +	struct pci_dev *dev;
> > > +
> > > +	for_each_pci_dev_in_path(dev, start, end) {
> > > +		if (pci_liveupdate_unpreserve_device(ser, dev))
> > 
> > I might be reading this wrong but are we leaking some upstream devs if 
> > an intermediate node fails?
> > 
> > 			  EP0
> > 			/
> > Assume we have: RC -> B1 -> B2 
> > 				\
> > 				 EP1
> > 
> > and EP0 & EP1 were preserved successfully.
> > 
> > And then we try unpreserving EP1, we follow:
> > 
> > unpreserve EP1 -> unpreserve B2 failed due to a corruption.
> > 
> > This aborts the loop, skipping B1 and RC completely?
> > Their refcounts remain elevated, effectively leaking them as preserved 
> > state permanently? (i.e. if we unpreserve EP0 after this, B1 & RC will
> > still get preserved).
> 
> Yes, but that would only happen if there is some sort of kernel bug or
> silent data corruption. I guess we could proceed with trying to
> unpreserve the bridges upstream. But I opted to log a big warning and
> bail immediately.
> 
> pci_liveupdate_finish_path() has the same behavior BTW.

Fair point. I agree we are in a broken state if we hit this. 

I was originally thinking of a situation where we'd want to keep the
failure localized. For example: unpreserve EP1 fails -> user sees the
warning -> resets EP1 -> retries preserving it later. 

But given the recent discussion/decision that retrieve operations
will no longer be retried, I guess there isn't really a use-case for
retrying anything. It makes sense to just bail here.

> 
> > 
> > > +			return;
> > > +	}
> > > +}
> > > +
> > > +static void pci_liveupdate_unpreserve_path(struct pci_ser *ser,
> > > +					   struct pci_dev *start)
> > > +{
> > > +	__pci_liveupdate_unpreserve_path(ser, start, /*end=*/NULL);
> > > +}
> > > +
> > > +static int pci_liveupdate_preserve_path(struct pci_ser *ser,
> > > +					struct pci_dev *start)
> > > +{
> > > +	struct pci_dev *dev;
> > > +	int ret;
> > > +
> > > +	for_each_pci_dev_in_path(dev, start, NULL) {
> > > +		ret = pci_liveupdate_preserve_device(ser, dev);
> > > +		if (ret) {
> > > +			__pci_liveupdate_unpreserve_path(ser, start, dev);
> > > +			return ret;
> > > +		}
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >  /**
> > >   * pci_liveupdate_preserve() - Preserve a PCI device across Live Update
> > >   * @dev: The PCI device to preserve.
> > > @@ -321,6 +403,9 @@ static int pci_liveupdate_preserve_device(struct pci_ser *ser, struct pci_dev *d
> > >   * pci_liveupdate_preserve() from their struct liveupdate_file_handler
> > >   * preserve() callback to ensure the outgoing struct pci_ser is already set up.
> > >   *
> > > + * pci_liveupdate_preserve() automatically preserves all bridges upstream of
> > > + * @dev.
> > > + *
> > >   * Returns: 0 on success, <0 on failure.
> > >   */
> > >  int pci_liveupdate_preserve(struct pci_dev *dev)
> > > @@ -336,7 +421,7 @@ int pci_liveupdate_preserve(struct pci_dev *dev)
> > >  	if (IS_ERR(ser))
> > >  		return PTR_ERR(ser);
> > >  
> > > -	return pci_liveupdate_preserve_device(ser, dev);
> > > +	return pci_liveupdate_preserve_path(ser, dev);
> > 
> > Minor nit: I might be too nitpicky here (and it's NOT a strong opinion)
> > but naming it pci_liveupdate_preserve_path_for_dev() reads better to me.
> 
> Noted :). I'll keep the current name for now since that is pretty long,
> but if anyone else votes for it I'm happy to be overridden.

Sounds good.

Thanks,
Praan