Re: [PATCH v6 2/2] PCI: endpoint: pci-epf-vntb: Manage virtual NTB and PCI bus lifetime

[Koichiro Den]

On Fri, Sep 11, 2026 at 04:45:07PM -0500, Bjorn Helgaas wrote:
> On Tue, Sep 01, 2026 at 03:32:38PM +0900, Koichiro Den wrote:
> > The virtual PCI driver registers an ntb_dev but has no remove callback.
> > Unbinding the endpoint function can therefore free BARs while the NTB
> > device and its client still use them. It also leaves the virtual PCI
> > devices and root bus allocated.
> > 
> > Allocate an ntb_dev for each virtual PCI probe and unregister it from the
> > matching remove callback. Start command processing only after registration.
> > Publish the device for doorbell IRQs at the same point. During remove, stop
> > the command work and drain IRQ handlers before unregistering the device.
> > 
> > Retain the root bus returned by pci_scan_bus() so it can be removed on
> > unbind. Unregister the virtual PCI driver before releasing endpoint
> > resources. Stop and remove the root bus under the PCI rescan/remove lock,
> > then release its host bridge.
> > 
> > Fixes: e35f56bb0330 ("PCI: endpoint: Support NTB transfer between RC and EP")
> > Cc: stable@xxxxxxxxxxxxxxx # 6.0+
> > Signed-off-by: Koichiro Den <den@xxxxxxxxxxxxx>
> > ---
> > Changes in v6:
> >   - Rework v5 patch 5 on v7.3-rc1.
> >   - Publish ntb_dev only after registration, and tie command work and
> >     doorbell delivery to its lifetime.
> >   - Retain and remove the virtual root bus, then release its host bridge.
> >   - Drop v5 patch 4; f7245901de89 ("PCI: Check parent for NULL in
> >     of_pci_bus_release_domain_nr()") fixed the bug it worked around.
> >     v5: https://lore.kernel.org/r/20260226084142.2226875-6-den@xxxxxxxxxxxxx/
> > 
> > @Frank, the code changed substantially since v5, so I did not carry your
> > R-b tag. I would appreciate another look.
> > 
> >  drivers/pci/endpoint/functions/pci-epf-vntb.c | 85 +++++++++++++++----
> >  1 file changed, 70 insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/pci/endpoint/functions/pci-epf-vntb.c b/drivers/pci/endpoint/functions/pci-epf-vntb.c
> > index 412e8cc6fb1d..992f5e7f8d4a 100644
> > --- a/drivers/pci/endpoint/functions/pci-epf-vntb.c
> > +++ b/drivers/pci/endpoint/functions/pci-epf-vntb.c
> > @@ -132,7 +132,7 @@ struct epf_ntb_ctrl {
> >  } __packed;
> >  
> >  struct epf_ntb {
> > -	struct ntb_dev ntb;
> > +	struct ntb_dev *ntb;
> >  	struct pci_epf *epf;
> >  	struct config_group group;
> >  
> > @@ -166,10 +166,15 @@ struct epf_ntb {
> >  	void __iomem *vpci_mw_addr[MAX_MW];
> >  
> >  	struct delayed_work cmd_handler;
> > +	struct pci_bus *vpci_bus;
> >  };
> >  
> >  #define to_epf_ntb(epf_group) container_of((epf_group), struct epf_ntb, group)
> > -#define ntb_ndev(__ntb) container_of(__ntb, struct epf_ntb, ntb)
> > +
> > +static struct epf_ntb *ntb_ndev(struct ntb_dev *ntb)
> > +{
> > +	return ntb->pdev->sysdata;
> > +}
> >  
> >  static struct pci_epf_header epf_ntb_header = {
> >  	.vendorid	= PCI_ANY_ID,
> > @@ -195,7 +200,7 @@ static int epf_ntb_link_up(struct epf_ntb *ntb, bool link_up)
> >  	else
> >  		ntb->reg->link_status &= ~LINK_STATUS_UP;
> >  
> > -	ntb_link_event(&ntb->ntb);
> > +	ntb_link_event(ntb->ntb);
> >  	return 0;
> >  }
> >  
> > @@ -284,7 +289,7 @@ static void epf_ntb_cmd_handler(struct work_struct *work)
> >  	     i++) {
> >  		if (ntb->epf_db[i]) {
> >  			atomic64_or(1 << (i - EPF_IRQ_DB_START), &ntb->db);
> > -			ntb_db_event(&ntb->ntb, i - EPF_IRQ_DB_START);
> > +			ntb_db_event(ntb->ntb, i - EPF_IRQ_DB_START);
> >  			ntb->epf_db[i] = 0;
> >  		}
> >  	}
> > @@ -348,12 +353,18 @@ static void epf_ntb_cmd_handler(struct work_struct *work)
> >  static irqreturn_t epf_ntb_doorbell_handler(int irq, void *data)
> >  {
> >  	struct epf_ntb *ntb = data;
> > +	struct ntb_dev *ndev;
> >  	int i;
> >  
> > +	/* Pair with smp_store_release() in pci_vntb_probe(). */
> > +	ndev = smp_load_acquire(&ntb->ntb);
> > +	if (!ndev)
> > +		return IRQ_HANDLED;
> > +
> >  	for (i = EPF_IRQ_DB_START; i < ntb->db_count; i++)
> >  		if (irq == ntb->epf->db_msg[i].virq) {
> >  			atomic64_or(1 << (i - EPF_IRQ_DB_START), &ntb->db);
> > -			ntb_db_event(&ntb->ntb, i - EPF_IRQ_DB_START);
> > +			ntb_db_event(ndev, i - EPF_IRQ_DB_START);
> >  		}
> >  
> >  	return IRQ_HANDLED;
> > @@ -985,7 +996,6 @@ static int epf_ntb_epc_init(struct epf_ntb *ntb)
> >  	}
> >  
> >  	INIT_DELAYED_WORK(&ntb->cmd_handler, epf_ntb_cmd_handler);
> > -	queue_work(kpcintb_workqueue, &ntb->cmd_handler.work);
> >  
> >  	atomic64_set(&ntb->peer_db_pending, 0);
> >  	enable_work(&ntb->peer_db_work);
> > @@ -1340,6 +1350,7 @@ static int vpci_scan_bus(void *sysdata)
> >  		pci_unlock_rescan_remove();
> >  		return -EINVAL;
> >  	}
> > +	ndev->vpci_bus = vpci_bus;
> >  
> >  	pci_bus_add_devices(vpci_bus);
> >  
> > @@ -1425,7 +1436,7 @@ static int vntb_epf_mw_set_trans(struct ntb_dev *ndev, int pidx, int idx,
> >  	int ret;
> >  	struct device *dev;
> >  
> > -	dev = &ntb->ntb.dev;
> > +	dev = &ndev->dev;
> >  	barno = ntb->epf_ntb_bar[BAR_MW1 + idx];
> >  	epf_bar = &ntb->epf->bar[barno];
> >  	epf_bar->phys_addr = addr;
> > @@ -1563,7 +1574,7 @@ static void vntb_epf_peer_db_work(struct work_struct *work)
> >  			ret = pci_epc_raise_irq(epf->epc, func_no, vfunc_no,
> >  						PCI_IRQ_MSI, interrupt_num);
> >  			if (ret)
> > -				dev_err(&ntb->ntb.dev,
> > +				dev_err(&epf->dev,
> >  					"Failed to raise IRQ for interrupt_num %u: %d\n",
> >  					interrupt_num, ret);
> >  		}
> > @@ -1681,13 +1692,18 @@ static const struct ntb_dev_ops vntb_epf_ops = {
> >  
> >  static int pci_vntb_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> >  {
> > -	int ret;
> >  	struct epf_ntb *ndev = (struct epf_ntb *)pdev->sysdata;
> >  	struct device *dev = &pdev->dev;
> > +	struct ntb_dev *ntb;
> > +	int ret;
> > +
> > +	ntb = devm_kzalloc(dev, sizeof(*ntb), GFP_KERNEL);
> > +	if (!ntb)
> > +		return -ENOMEM;
> >  
> > -	ndev->ntb.pdev = pdev;
> > -	ndev->ntb.topo = NTB_TOPO_NONE;
> > -	ndev->ntb.ops =  &vntb_epf_ops;
> > +	ntb->pdev = pdev;
> > +	ntb->topo = NTB_TOPO_NONE;
> > +	ntb->ops = &vntb_epf_ops;
> >  
> >  	ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32));
> >  	if (ret) {
> > @@ -1695,16 +1711,41 @@ static int pci_vntb_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> >  		return ret;
> >  	}
> >  
> > -	ret = ntb_register_device(&ndev->ntb);
> > +	ret = ntb_register_device(ntb);
> >  	if (ret) {
> >  		dev_err(dev, "Failed to register NTB device\n");
> >  		return ret;
> >  	}
> >  
> > +	/* Publish after ntb_register_device() succeeds. */
> > +	smp_store_release(&ndev->ntb, ntb);
> > +	queue_delayed_work(kpcintb_workqueue, &ndev->cmd_handler, 0);
> > +
> >  	dev_dbg(dev, "PCI Virtual NTB driver loaded\n");
> >  	return 0;
> >  }
> >  
> > +static void pci_vntb_remove(struct pci_dev *pdev)
> > +{
> > +	struct epf_ntb *ndev = pdev->sysdata;
> > +	struct ntb_dev *ntb;
> > +	unsigned int i;
> > +
> > +	/* Stop the work reader, then close and drain the IRQ gate. */
> > +	cancel_delayed_work_sync(&ndev->cmd_handler);
> > +
> > +	ntb = xchg(&ndev->ntb, NULL);
> > +
> > +	if (ndev->msi_doorbell)
> > +		for (i = 0; i < ndev->db_count; i++) {
> > +			if (epf_ntb_db_irq_is_duplicated(ndev->epf, i))
> > +				continue;
> > +			synchronize_irq(ndev->epf->db_msg[i].virq);
> > +		}
> > +
> > +	ntb_unregister_device(ntb);
> > +}
> > +
> >  static struct pci_device_id pci_vntb_table[] = {
> >  	{
> >  		PCI_DEVICE(0xffff, 0xffff),
> > @@ -1716,6 +1757,7 @@ static struct pci_driver vntb_pci_driver = {
> >  	.name           = "pci-vntb",
> >  	.id_table       = pci_vntb_table,
> >  	.probe          = pci_vntb_probe,
> > +	.remove         = pci_vntb_remove,
> >  };
> >  
> >  /* ============ PCIe EPF Driver Bind ====================*/
> > @@ -1796,12 +1838,25 @@ static int epf_ntb_bind(struct pci_epf *epf)
> >   */
> >  static void epf_ntb_unbind(struct pci_epf *epf)
> >  {
> > +	struct pci_host_bridge *bridge;
> >  	struct epf_ntb *ntb = epf_get_drvdata(epf);
> >  
> > +	pci_unregister_driver(&vntb_pci_driver);
> > +
> > +	if (ntb->vpci_bus) {
> > +		bridge = to_pci_host_bridge(ntb->vpci_bus->bridge);
> 
> Where was this host bridge allocated?

The existing pci_scan_bus() call allocates it during bind:

-> epf_ntb_bind()
  -> vpci_scan_bus()
    -> pci_scan_bus()
      -> pci_create_root_bus()
        -> pci_alloc_host_bridge()

Best regards,
Koichiro

> 
> > +		pci_lock_rescan_remove();
> > +		pci_stop_root_bus(ntb->vpci_bus);
> > +		pci_remove_root_bus(ntb->vpci_bus);
> > +		ntb->vpci_bus = NULL;
> > +		pci_unlock_rescan_remove();
> > +
> > +		pci_free_host_bridge(bridge);
> > +	}
> > +
> >  	epf_ntb_epc_cleanup(ntb);
> >  	epf_ntb_config_spad_bar_free(ntb);
> > -
> > -	pci_unregister_driver(&vntb_pci_driver);
> >  }
> >  
> >  // EPF driver probe
> > -- 
> > 2.51.0
> >