Re: [PATCH v10 05/13] PCI: liveupdate: Auto-preserve upstream bridges across Live Update

[Bjorn Helgaas <helgaas@kernel.org>]

On Fri, Nov 06, 2026 at 06:20:36PM +0000, David Matlack wrote:
> On Fri Nov 6, 2026 at 5:34 PM UTC, Bjorn Helgaas wrote:
> > On Tue, Oct 13, 2026 at 04:25:21PM +0000, David Matlack wrote:
> >>  static int pci_liveupdate_preserve_device(struct pci_flb_outgoing *outgoing,
> >>  					  struct pci_dev *dev)
> >>  {
> >> -	struct pci_dev_ser *dev_ser;
> >> -
> >>  	if (dev->is_virtfn) {
> >>  		pci_warn(dev, "Cannot preserve Virtual Functions\n");
> >>  		return -EINVAL;
> >>  	}
> >>  
> >> -	if (dev->liveupdate.outgoing) {
> >> +	/*
> >> +	 * Endpoint devices should not be preserved more than once.
> >> +	 * Bridges are preserved once for every downstream device that
> >> +	 * is preserved.
> >> +	 */
> >> +	if (dev->liveupdate.outgoing && !dev->subordinate) {
> >>  		pci_warn(dev, "Device is already preserved\n");
> >>  		return -EBUSY;
> >>  	}
> >>  
> >> -	if (!pci_is_root_bus(dev->bus)) {
> >> -		pci_warn(dev, "Cannot preserve devices behind bridges\n");
> >> +	if (dev->liveupdate.outgoing && !dev->liveupdate.outgoing->refcount) {
> >> +		pci_WARN(dev, 1, "Preserved device with 0 refcount!\n");
> >>  		return -EINVAL;
> >>  	}
> >>  
> >> -	dev_ser = pci_flb_alloc_dev_ser(outgoing);
> >> -	if (IS_ERR(dev_ser))
> >> -		return PTR_ERR(dev_ser);
> >> +	if (!dev->liveupdate.outgoing) {
> >> +		struct pci_dev_ser *dev_ser;
> >>  
> >> -	pci_info(dev, "Device will be preserved across next Live Update\n");
> >> -	outgoing->ser->nr_devices++;
> >> -	outgoing->ser->devices = kho_block_set_head_pa(&outgoing->block_set);
> >> +		dev_ser = pci_flb_alloc_dev_ser(outgoing);
> >> +		if (IS_ERR(dev_ser))
> >> +			return PTR_ERR(dev_ser);
> >>  
> >> -	dev_ser->domain = pci_domain_nr(dev->bus);
> >> -	dev_ser->bdf = pci_dev_id(dev);
> >> -	dev_ser->refcount++;
> >> +		pci_info(dev, "Device will be preserved across next Live Update\n");
> >> +		outgoing->ser->nr_devices++;
> >
> > This looks a little funny because prior to this patch, nr_devices was
> > always incremented in pci_liveupdate_preserve_device() and always
> > decremented in pci_liveupdate_unpreserve_device().  Now it's only
> > incremented once (per device) in pci_liveupdate_preserve_device(), so
> > it looks unbalanced.
> >
> > My guess is this is because previously we didn't expect to preserve
> > bridges at all, and now pci_liveupdate_preserve_device() might be
> > called several times for a bridge, and it's all just fine, but it's a
> > little subtle.
> 
> Right. nr_devices counts struct pci_dev_ser entries that are in use, so
> it goes up when a device's refcount goes from 0 to 1 (the entry is
> allocated) and down when it goes from 1 to 0 (the entry is released).
> For endpoints that is every preserve/unpreserve call, but a bridge is
> preserved once per preserved device below it, so most calls for a
> bridge only change its refcount.
> 
> > I suppose the "(dev->liveupdate.outgoing && !dev->subordinate)" test
> > above is what allows us to just increment refcount each time a bridge
> > is preserved.  And the test around pci_flb_alloc_dev_ser() is doing
> > basically the same thing.  I wonder if there's a way to connect them
> > more explicitly.
> 
> Agreed, both points come from the same thing being spread out. I think
> both can be fixed by splitting pci_liveupdate_preserve_device() into
> "already preserved" and "first preservation" cases, and moving the
> nr_devices accounting into helpers that allocate and release the
> struct pci_dev_ser, so it is visibly paired:
> 
> 	if (dev_ser) {
> 		/*
> 		 * Endpoints can only be preserved once. Bridges are
> 		 * preserved once for every preserved device below them.
> 		 */
> 		if (!dev->subordinate) {
> 			pci_warn(dev, "Device is already preserved\n");
> 			return -EBUSY;
> 		}
> 		...
> 	} else {
> 		dev_ser = pci_liveupdate_alloc_dev_ser(outgoing, dev);
> 		if (IS_ERR(dev_ser))
> 			return PTR_ERR(dev_ser);
> 	}
> 
> 	dev_ser->refcount++;
> 
> with pci_liveupdate_unpreserve_device() calling a matching
> pci_liveupdate_free_dev_ser() when the refcount drops to 0.

I think that would be good.