* Re: [PATCH v6 2/2] PCI: endpoint: pci-epf-vntb: Manage virtual NTB and PCI bus lifetime
2026-09-01 6:32 ` [PATCH v6 2/2] PCI: endpoint: pci-epf-vntb: Manage virtual NTB and PCI bus lifetime Koichiro Den
@ 2026-09-01 19:05 ` Frank Li
2026-09-11 21:45 ` Bjorn Helgaas
2026-09-14 15:44 ` Frank Li
2 siblings, 0 replies; 10+ messages in thread
From: Frank Li @ 2026-09-01 19:05 UTC (permalink / raw)
To: Koichiro Den
Cc: Manivannan Sadhasivam, Frank Li, Jon Mason, Dave Jiang,
Allen Hubbe, Krzysztof Wilczyński, Kishon Vijay Abraham I,
Bjorn Helgaas, ntb, linux-pci, linux-kernel
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@vger.kernel.org # 6.0+
> Signed-off-by: Koichiro Den <den@valinux.co.jp>
> ---
Reviewed-by: Frank Li <Frank.Li@nxp.com>
> 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@valinux.co.jp/
>
> @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);
> +
> + 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
>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v6 2/2] PCI: endpoint: pci-epf-vntb: Manage virtual NTB and PCI bus lifetime
2026-09-01 6:32 ` [PATCH v6 2/2] PCI: endpoint: pci-epf-vntb: Manage virtual NTB and PCI bus lifetime Koichiro Den
2026-09-01 19:05 ` Frank Li
@ 2026-09-11 21:45 ` Bjorn Helgaas
2026-09-12 14:57 ` Koichiro Den
2026-09-14 15:44 ` Frank Li
2 siblings, 1 reply; 10+ messages in thread
From: Bjorn Helgaas @ 2026-09-11 21:45 UTC (permalink / raw)
To: Koichiro Den
Cc: Manivannan Sadhasivam, Frank Li, Jon Mason, Dave Jiang,
Allen Hubbe, Krzysztof Wilczyński, Kishon Vijay Abraham I,
Bjorn Helgaas, ntb, linux-pci, linux-kernel
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@vger.kernel.org # 6.0+
> Signed-off-by: Koichiro Den <den@valinux.co.jp>
> ---
> 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@valinux.co.jp/
>
> @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?
> + 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
>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v6 2/2] PCI: endpoint: pci-epf-vntb: Manage virtual NTB and PCI bus lifetime
2026-09-11 21:45 ` Bjorn Helgaas
@ 2026-09-12 14:57 ` Koichiro Den
0 siblings, 0 replies; 10+ messages in thread
From: Koichiro Den @ 2026-09-12 14:57 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Manivannan Sadhasivam, Frank Li, Jon Mason, Dave Jiang,
Allen Hubbe, Krzysztof Wilczyński, Kishon Vijay Abraham I,
Bjorn Helgaas, ntb, linux-pci, linux-kernel
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@vger.kernel.org # 6.0+
> > Signed-off-by: Koichiro Den <den@valinux.co.jp>
> > ---
> > 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@valinux.co.jp/
> >
> > @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
> >
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v6 2/2] PCI: endpoint: pci-epf-vntb: Manage virtual NTB and PCI bus lifetime
2026-09-01 6:32 ` [PATCH v6 2/2] PCI: endpoint: pci-epf-vntb: Manage virtual NTB and PCI bus lifetime Koichiro Den
2026-09-01 19:05 ` Frank Li
2026-09-11 21:45 ` Bjorn Helgaas
@ 2026-09-14 15:44 ` Frank Li
2 siblings, 0 replies; 10+ messages in thread
From: Frank Li @ 2026-09-14 15:44 UTC (permalink / raw)
To: Koichiro Den
Cc: Manivannan Sadhasivam, Frank Li, Jon Mason, Dave Jiang,
Allen Hubbe, Krzysztof Wilczyński, Kishon Vijay Abraham I,
Bjorn Helgaas, ntb, linux-pci, linux-kernel
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@vger.kernel.org # 6.0+
> Signed-off-by: Koichiro Den <den@valinux.co.jp>
> ---
Reviewed-by: Frank Li <Frank.Li@nxp.com>
> 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@valinux.co.jp/
>
> @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);
> +
> + 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
>
^ permalink raw reply [flat|nested] 10+ messages in thread