[PATCH v2 0/6] PCI: endpoint/NTB: Harden vNTB resource management

[PATCH v2 0/6] PCI: endpoint/NTB: Harden vNTB resource management

[Koichiro Den]

The vNTB endpoint function (pci-epf-vntb) can be configured and reconfigured
through configfs (link/unlink functions, start/stop the controller, update
parameters). In practice, several pitfalls present: double-unmapping when two
windows share a BAR, wrong parameter order in .drop_link leading to wrong
object lookups, duplicate EPC teardown that leads to oopses, a work item
running after resources were torn down, and inability to re-link/restart
fundamentally because ntb_dev was embedded and the vPCI bus teardown was
incomplete.

This series addresses those issues and hardens resource management across NTB
EPF and PCI EP core:

- Avoid double iounmap when PEER_SPAD and CONFIG share the same BAR.
- Fix configfs .drop_link parameter order so the correct groups are used during
  unlink.
- Remove duplicate EPC resource teardown in both pci-epf-vntb and pci-epf-ntb,
  avoiding crashes on .allow_link failures and during .drop_link.
- Stop the delayed cmd_handler work before clearing BARs/doorbells.
- Manage ntb_dev as a devm-managed allocation and implement .remove() in the
  vNTB PCI driver. Switch to pci_scan_root_bus().

With these changes, the controller can now be stopped, a function unlinked,
configfs settings updated, and the controller re-linked and restarted
without rebooting the endpoint, as long as the underlying pci_epc_ops
.stop() is non-destructive and .start() restores normal operation.

Patches 1-5 carry Fixes tags and are candidates for stable. Patch 6 is a
behavioral improvement that completes lifetime management for relink/restart
scenarios.


v1->v2 changes:
  - Incorporated feedback from Frank.
  - Added Reviewed-by tags (except for patches #4 and #6).
  - Fixed a typo in patch #5 title.
  (No code changes overall.)

v1: https://lore.kernel.org/all/20251023071757.901181-1-den@valinux.co.jp/


Koichiro Den (6):
  NTB: epf: Avoid pci_iounmap() with offset when PEER_SPAD and CONFIG
    share BAR
  PCI: endpoint: Fix parameter order for .drop_link
  PCI: endpoint: pci-epf-vntb: Remove duplicate resource teardown
  PCI: endpoint: pci-epf-ntb: Remove duplicate resource teardown
  NTB: epf: vntb: Stop cmd_handler work in epf_ntb_epc_cleanup
  PCI: endpoint: pci-epf-vntb: manage ntb_dev lifetime and fix vpci bus
    teardown

 drivers/ntb/hw/epf/ntb_hw_epf.c               |  3 +-
 drivers/pci/endpoint/functions/pci-epf-ntb.c  | 56 +-----------
 drivers/pci/endpoint/functions/pci-epf-vntb.c | 86 ++++++++++++-------
 drivers/pci/endpoint/pci-ep-cfs.c             |  8 +-
 4 files changed, 62 insertions(+), 91 deletions(-)

-- 
2.48.1

[PATCH v2 1/6] NTB: epf: Avoid pci_iounmap() with offset when PEER_SPAD and CONFIG share BAR

[Koichiro Den]

When BAR_PEER_SPAD and BAR_CONFIG share one PCI BAR, the module teardown
path ends up calling pci_iounmap() on the same iomem with some offset,
which is unnecessary and triggers a kernel warning like the following:

  Trying to vunmap() nonexistent vm area (0000000069a5ffe8)
  WARNING: mm/vmalloc.c:3470 at vunmap+0x58/0x68, CPU#5: modprobe/2937
  [...]
  Call trace:
   vunmap+0x58/0x68 (P)
   iounmap+0x34/0x48
   pci_iounmap+0x2c/0x40
   ntb_epf_pci_remove+0x44/0x80 [ntb_hw_epf]
   pci_device_remove+0x48/0xf8
   device_remove+0x50/0x88
   device_release_driver_internal+0x1c8/0x228
   driver_detach+0x50/0xb0
   bus_remove_driver+0x74/0x100
   driver_unregister+0x34/0x68
   pci_unregister_driver+0x34/0xa0
   ntb_epf_pci_driver_exit+0x14/0xfe0 [ntb_hw_epf]
  [...]

Fix it by unmapping only when PEER_SPAD and CONFIG use difference bars.

Cc: <stable@vger.kernel.org>
Fixes: e75d5ae8ab88 ("NTB: epf: Allow more flexibility in the memory BAR map method")
Reviewed-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Koichiro Den <den@valinux.co.jp>
---
 drivers/ntb/hw/epf/ntb_hw_epf.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/ntb/hw/epf/ntb_hw_epf.c b/drivers/ntb/hw/epf/ntb_hw_epf.c
index d3ecf25a5162..9935da48a52e 100644
--- a/drivers/ntb/hw/epf/ntb_hw_epf.c
+++ b/drivers/ntb/hw/epf/ntb_hw_epf.c
@@ -646,7 +646,8 @@ static void ntb_epf_deinit_pci(struct ntb_epf_dev *ndev)
 	struct pci_dev *pdev = ndev->ntb.pdev;
 
 	pci_iounmap(pdev, ndev->ctrl_reg);
-	pci_iounmap(pdev, ndev->peer_spad_reg);
+	if (ndev->barno_map[BAR_PEER_SPAD] != ndev->barno_map[BAR_CONFIG])
+		pci_iounmap(pdev, ndev->peer_spad_reg);
 	pci_iounmap(pdev, ndev->db_reg);
 
 	pci_release_regions(pdev);
-- 
2.48.1

[PATCH v2 2/6] PCI: endpoint: Fix parameter order for .drop_link

[Koichiro Den]

The unlink callbacks passed the parameters in the wrong order that led
to looking up the wrong group objects. Swap the arguments so that the
first parameter is the epf item and the second is the epc item.

Cc: <stable@vger.kernel.org>
Fixes: e85a2d783762 ("PCI: endpoint: Add support in configfs to associate two EPCs with EPF")
Reviewed-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Koichiro Den <den@valinux.co.jp>
---
 drivers/pci/endpoint/pci-ep-cfs.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/endpoint/pci-ep-cfs.c b/drivers/pci/endpoint/pci-ep-cfs.c
index ef50c82e647f..c7cf6c76d116 100644
--- a/drivers/pci/endpoint/pci-ep-cfs.c
+++ b/drivers/pci/endpoint/pci-ep-cfs.c
@@ -69,8 +69,8 @@ static int pci_secondary_epc_epf_link(struct config_item *epf_item,
 	return 0;
 }
 
-static void pci_secondary_epc_epf_unlink(struct config_item *epc_item,
-					 struct config_item *epf_item)
+static void pci_secondary_epc_epf_unlink(struct config_item *epf_item,
+					 struct config_item *epc_item)
 {
 	struct pci_epf_group *epf_group = to_pci_epf_group(epf_item->ci_parent);
 	struct pci_epc_group *epc_group = to_pci_epc_group(epc_item);
@@ -133,8 +133,8 @@ static int pci_primary_epc_epf_link(struct config_item *epf_item,
 	return 0;
 }
 
-static void pci_primary_epc_epf_unlink(struct config_item *epc_item,
-				       struct config_item *epf_item)
+static void pci_primary_epc_epf_unlink(struct config_item *epf_item,
+				       struct config_item *epc_item)
 {
 	struct pci_epf_group *epf_group = to_pci_epf_group(epf_item->ci_parent);
 	struct pci_epc_group *epc_group = to_pci_epc_group(epc_item);
-- 
2.48.1

[PATCH v2 3/6] PCI: endpoint: pci-epf-vntb: Remove duplicate resource teardown

[Koichiro Den]

epf_ntb_epc_destroy() duplicates the teardown that the caller is
supposed to perform later. This leads to an oops when .allow_link fails
or when .drop_link is performed. The following is an example oops of the
former case:

  Unable to handle kernel paging request at virtual address dead000000000108
  [...]
  [dead000000000108] address between user and kernel address ranges
  Internal error: Oops: 0000000096000044 [#1]  SMP
  [...]
  Call trace:
   pci_epc_remove_epf+0x78/0xe0 (P)
   pci_primary_epc_epf_link+0x88/0xa8
   configfs_symlink+0x1f4/0x5a0
   vfs_symlink+0x134/0x1d8
   do_symlinkat+0x88/0x138
   __arm64_sys_symlinkat+0x74/0xe0
  [...]

Remove the helper, and drop pci_epc_put(). EPC device refcounting is
tied to the configfs EPC group lifetime, and pci_epc_put() in the
.drop_link path is sufficient.

Cc: <stable@vger.kernel.org>
Fixes: e35f56bb0330 ("PCI: endpoint: Support NTB transfer between RC and EP")
Reviewed-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Koichiro Den <den@valinux.co.jp>
---
 drivers/pci/endpoint/functions/pci-epf-vntb.c | 19 +------------------
 1 file changed, 1 insertion(+), 18 deletions(-)

diff --git a/drivers/pci/endpoint/functions/pci-epf-vntb.c b/drivers/pci/endpoint/functions/pci-epf-vntb.c
index 83e9ab10f9c4..49ce5d4b0ee5 100644
--- a/drivers/pci/endpoint/functions/pci-epf-vntb.c
+++ b/drivers/pci/endpoint/functions/pci-epf-vntb.c
@@ -644,19 +644,6 @@ static void epf_ntb_mw_bar_clear(struct epf_ntb *ntb, int num_mws)
 	}
 }
 
-/**
- * epf_ntb_epc_destroy() - Cleanup NTB EPC interface
- * @ntb: NTB device that facilitates communication between HOST and VHOST
- *
- * Wrapper for epf_ntb_epc_destroy_interface() to cleanup all the NTB interfaces
- */
-static void epf_ntb_epc_destroy(struct epf_ntb *ntb)
-{
-	pci_epc_remove_epf(ntb->epf->epc, ntb->epf, 0);
-	pci_epc_put(ntb->epf->epc);
-}
-
-
 /**
  * epf_ntb_is_bar_used() - Check if a bar is used in the ntb configuration
  * @ntb: NTB device that facilitates communication between HOST and VHOST
@@ -1406,7 +1393,7 @@ static int epf_ntb_bind(struct pci_epf *epf)
 	ret = epf_ntb_init_epc_bar(ntb);
 	if (ret) {
 		dev_err(dev, "Failed to create NTB EPC\n");
-		goto err_bar_init;
+		return ret;
 	}
 
 	ret = epf_ntb_config_spad_bar_alloc(ntb);
@@ -1446,9 +1433,6 @@ static int epf_ntb_bind(struct pci_epf *epf)
 err_bar_alloc:
 	epf_ntb_config_spad_bar_free(ntb);
 
-err_bar_init:
-	epf_ntb_epc_destroy(ntb);
-
 	return ret;
 }
 
@@ -1464,7 +1448,6 @@ static void epf_ntb_unbind(struct pci_epf *epf)
 
 	epf_ntb_epc_cleanup(ntb);
 	epf_ntb_config_spad_bar_free(ntb);
-	epf_ntb_epc_destroy(ntb);
 
 	pci_unregister_driver(&vntb_pci_driver);
 }
-- 
2.48.1

[PATCH v2 4/6] PCI: endpoint: pci-epf-ntb: Remove duplicate resource teardown

[Koichiro Den]

epf_ntb_epc_destroy() duplicates the teardown that the caller is
supposed to do later. This leads to an oops when .allow_link fails or
when .drop_link is performed. Remove the helper.

Also drop pci_epc_put(). EPC device refcounting is tied to configfs EPC
group lifetime, and pci_epc_put() in the .drop_link path is sufficient.

Cc: <stable@vger.kernel.org>
Fixes: 8b821cf76150 ("PCI: endpoint: Add EP function driver to provide NTB functionality")
Signed-off-by: Koichiro Den <den@valinux.co.jp>
---
 drivers/pci/endpoint/functions/pci-epf-ntb.c | 56 +-------------------
 1 file changed, 2 insertions(+), 54 deletions(-)

diff --git a/drivers/pci/endpoint/functions/pci-epf-ntb.c b/drivers/pci/endpoint/functions/pci-epf-ntb.c
index e01a98e74d21..7702ebb81d99 100644
--- a/drivers/pci/endpoint/functions/pci-epf-ntb.c
+++ b/drivers/pci/endpoint/functions/pci-epf-ntb.c
@@ -1494,47 +1494,6 @@ static int epf_ntb_db_mw_bar_init(struct epf_ntb *ntb,
 	return ret;
 }
 
-/**
- * epf_ntb_epc_destroy_interface() - Cleanup NTB EPC interface
- * @ntb: NTB device that facilitates communication between HOST1 and HOST2
- * @type: PRIMARY interface or SECONDARY interface
- *
- * Unbind NTB function device from EPC and relinquish reference to pci_epc
- * for each of the interface.
- */
-static void epf_ntb_epc_destroy_interface(struct epf_ntb *ntb,
-					  enum pci_epc_interface_type type)
-{
-	struct epf_ntb_epc *ntb_epc;
-	struct pci_epc *epc;
-	struct pci_epf *epf;
-
-	if (type < 0)
-		return;
-
-	epf = ntb->epf;
-	ntb_epc = ntb->epc[type];
-	if (!ntb_epc)
-		return;
-	epc = ntb_epc->epc;
-	pci_epc_remove_epf(epc, epf, type);
-	pci_epc_put(epc);
-}
-
-/**
- * epf_ntb_epc_destroy() - Cleanup NTB EPC interface
- * @ntb: NTB device that facilitates communication between HOST1 and HOST2
- *
- * Wrapper for epf_ntb_epc_destroy_interface() to cleanup all the NTB interfaces
- */
-static void epf_ntb_epc_destroy(struct epf_ntb *ntb)
-{
-	enum pci_epc_interface_type type;
-
-	for (type = PRIMARY_INTERFACE; type <= SECONDARY_INTERFACE; type++)
-		epf_ntb_epc_destroy_interface(ntb, type);
-}
-
 /**
  * epf_ntb_epc_create_interface() - Create and initialize NTB EPC interface
  * @ntb: NTB device that facilitates communication between HOST1 and HOST2
@@ -1614,15 +1573,8 @@ static int epf_ntb_epc_create(struct epf_ntb *ntb)
 
 	ret = epf_ntb_epc_create_interface(ntb, epf->sec_epc,
 					   SECONDARY_INTERFACE);
-	if (ret) {
+	if (ret)
 		dev_err(dev, "SECONDARY intf: Fail to create NTB EPC\n");
-		goto err_epc_create;
-	}
-
-	return 0;
-
-err_epc_create:
-	epf_ntb_epc_destroy_interface(ntb, PRIMARY_INTERFACE);
 
 	return ret;
 }
@@ -1887,7 +1839,7 @@ static int epf_ntb_bind(struct pci_epf *epf)
 	ret = epf_ntb_init_epc_bar(ntb);
 	if (ret) {
 		dev_err(dev, "Failed to create NTB EPC\n");
-		goto err_bar_init;
+		return ret;
 	}
 
 	ret = epf_ntb_config_spad_bar_alloc_interface(ntb);
@@ -1909,9 +1861,6 @@ static int epf_ntb_bind(struct pci_epf *epf)
 err_bar_alloc:
 	epf_ntb_config_spad_bar_free(ntb);
 
-err_bar_init:
-	epf_ntb_epc_destroy(ntb);
-
 	return ret;
 }
 
@@ -1927,7 +1876,6 @@ static void epf_ntb_unbind(struct pci_epf *epf)
 
 	epf_ntb_epc_cleanup(ntb);
 	epf_ntb_config_spad_bar_free(ntb);
-	epf_ntb_epc_destroy(ntb);
 }
 
 #define EPF_NTB_R(_name)						\
-- 
2.48.1

[PATCH v2 5/6] NTB: epf: vntb: Stop cmd_handler work in epf_ntb_epc_cleanup

[Koichiro Den]

Disable the delayed work before clearing BAR mappings and doorbells to
avoid running the handler after resources have been torn down.

  Unable to handle kernel paging request at virtual address ffff800083f46004
  [...]
  Internal error: Oops: 0000000096000007 [#1]  SMP
  [...]
  Call trace:
   epf_ntb_cmd_handler+0x54/0x200 [pci_epf_vntb] (P)
   process_one_work+0x154/0x3b0
   worker_thread+0x2c8/0x400
   kthread+0x148/0x210
   ret_from_fork+0x10/0x20

Cc: <stable@vger.kernel.org>
Fixes: e35f56bb0330 ("PCI: endpoint: Support NTB transfer between RC and EP")
Reviewed-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Koichiro Den <den@valinux.co.jp>
---
 drivers/pci/endpoint/functions/pci-epf-vntb.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/pci/endpoint/functions/pci-epf-vntb.c b/drivers/pci/endpoint/functions/pci-epf-vntb.c
index 49ce5d4b0ee5..750a246f79c9 100644
--- a/drivers/pci/endpoint/functions/pci-epf-vntb.c
+++ b/drivers/pci/endpoint/functions/pci-epf-vntb.c
@@ -823,6 +823,7 @@ static int epf_ntb_epc_init(struct epf_ntb *ntb)
  */
 static void epf_ntb_epc_cleanup(struct epf_ntb *ntb)
 {
+	disable_delayed_work_sync(&ntb->cmd_handler);
 	epf_ntb_mw_bar_clear(ntb, ntb->num_mws);
 	epf_ntb_db_bar_clear(ntb);
 	epf_ntb_config_sspad_bar_clear(ntb);
-- 
2.48.1

[PATCH v2 6/6] PCI: endpoint: pci-epf-vntb: manage ntb_dev lifetime and fix vpci bus teardown

[Koichiro Den]

Currently ntb_dev is embedded in epf_ntb, while configfs allows starting
or stopping controller and linking or unlinking functions as you want.
In fact, re-linking and re-starting is not possible with the embedded
design and leads to oopses.

Allocate ntb_dev with devm and add a .remove callback to the pci driver
that calls ntb_unregister_device(). This allows a fresh device to be
created on the next .bind call.

With these changes, the controller can now be stopped, a function
unlinked, configfs settings updated, and the controller re-linked and
restarted without rebooting the endpoint, as long as the underlying
pci_epc_ops .stop() operation is non-destructive, and .start() can
restore normal operations.

Signed-off-by: Koichiro Den <den@valinux.co.jp>
---
 drivers/pci/endpoint/functions/pci-epf-vntb.c | 66 +++++++++++++++----
 1 file changed, 52 insertions(+), 14 deletions(-)

diff --git a/drivers/pci/endpoint/functions/pci-epf-vntb.c b/drivers/pci/endpoint/functions/pci-epf-vntb.c
index 750a246f79c9..3059ed85a955 100644
--- a/drivers/pci/endpoint/functions/pci-epf-vntb.c
+++ b/drivers/pci/endpoint/functions/pci-epf-vntb.c
@@ -118,7 +118,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;
 
@@ -144,10 +144,16 @@ 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 inline struct epf_ntb *ntb_ndev(struct ntb_dev *ntb)
+{
+	return (struct epf_ntb *)ntb->pdev->sysdata;
+}
 
 static struct pci_epf_header epf_ntb_header = {
 	.vendorid	= PCI_ANY_ID,
@@ -173,7 +179,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;
 }
 
@@ -261,7 +267,7 @@ static void epf_ntb_cmd_handler(struct work_struct *work)
 	for (i = 1; i < ntb->db_count; i++) {
 		if (ntb->epf_db[i]) {
 			ntb->db |= 1 << (i - 1);
-			ntb_db_event(&ntb->ntb, i);
+			ntb_db_event(ntb->ntb, i);
 			ntb->epf_db[i] = 0;
 		}
 	}
@@ -1097,12 +1103,24 @@ static int vpci_scan_bus(void *sysdata)
 {
 	struct pci_bus *vpci_bus;
 	struct epf_ntb *ndev = sysdata;
-
-	vpci_bus = pci_scan_bus(ndev->vbus_number, &vpci_ops, sysdata);
+	LIST_HEAD(resources);
+	static struct resource busn_res = {
+		.start = 0,
+		.end = 255,
+		.flags = IORESOURCE_BUS,
+	};
+
+	pci_add_resource(&resources, &ioport_resource);
+	pci_add_resource(&resources, &iomem_resource);
+	pci_add_resource(&resources, &busn_res);
+
+	vpci_bus = pci_scan_root_bus(&ndev->epf->epc->dev, ndev->vbus_number,
+				     &vpci_ops, sysdata, &resources);
 	if (!vpci_bus) {
 		pr_err("create pci bus failed\n");
 		return -EINVAL;
 	}
+	ndev->vpci_bus = vpci_bus;
 
 	pci_bus_add_devices(vpci_bus);
 
@@ -1147,7 +1165,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 = &ntb->ntb->dev;
 	barno = ntb->epf_ntb_bar[BAR_MW1 + idx];
 	epf_bar = &ntb->epf->bar[barno];
 	epf_bar->phys_addr = addr;
@@ -1247,7 +1265,7 @@ static int vntb_epf_peer_db_set(struct ntb_dev *ndev, u64 db_bits)
 	ret = pci_epc_raise_irq(ntb->epf->epc, func_no, vfunc_no,
 				PCI_IRQ_MSI, interrupt_num + 1);
 	if (ret)
-		dev_err(&ntb->ntb.dev, "Failed to raise IRQ\n");
+		dev_err(&ntb->ntb->dev, "Failed to raise IRQ\n");
 
 	return ret;
 }
@@ -1334,9 +1352,12 @@ static int pci_vntb_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	struct epf_ntb *ndev = (struct epf_ntb *)pdev->sysdata;
 	struct device *dev = &pdev->dev;
 
-	ndev->ntb.pdev = pdev;
-	ndev->ntb.topo = NTB_TOPO_NONE;
-	ndev->ntb.ops =  &vntb_epf_ops;
+	ndev->ntb = devm_kzalloc(dev, sizeof(*ndev->ntb), GFP_KERNEL);
+	if (!ndev->ntb)
+		return -ENOMEM;
+	ndev->ntb->pdev = pdev;
+	ndev->ntb->topo = NTB_TOPO_NONE;
+	ndev->ntb->ops = &vntb_epf_ops;
 
 	ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32));
 	if (ret) {
@@ -1344,7 +1365,7 @@ 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(ndev->ntb);
 	if (ret) {
 		dev_err(dev, "Failed to register NTB device\n");
 		return ret;
@@ -1354,6 +1375,17 @@ static int pci_vntb_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	return 0;
 }
 
+static void pci_vntb_remove(struct pci_dev *pdev)
+{
+	struct epf_ntb *ndev = (struct epf_ntb *)pdev->sysdata;
+
+	if (!ndev || !ndev->ntb)
+		return;
+
+	ntb_unregister_device(ndev->ntb);
+	ndev->ntb = NULL;
+}
+
 static struct pci_device_id pci_vntb_table[] = {
 	{
 		PCI_DEVICE(0xffff, 0xffff),
@@ -1365,6 +1397,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 ====================*/
@@ -1447,10 +1480,15 @@ static void epf_ntb_unbind(struct pci_epf *epf)
 {
 	struct epf_ntb *ntb = epf_get_drvdata(epf);
 
+	pci_unregister_driver(&vntb_pci_driver);
+
+	pci_lock_rescan_remove();
+	pci_stop_root_bus(ntb->vpci_bus);
+	pci_remove_root_bus(ntb->vpci_bus);
+	pci_unlock_rescan_remove();
+
 	epf_ntb_epc_cleanup(ntb);
 	epf_ntb_config_spad_bar_free(ntb);
-
-	pci_unregister_driver(&vntb_pci_driver);
 }
 
 // EPF driver probe
-- 
2.48.1

Re: [PATCH v2 4/6] PCI: endpoint: pci-epf-ntb: Remove duplicate resource teardown

[Frank Li]

On Wed, Oct 29, 2025 at 05:03:19PM +0900, Koichiro Den wrote:
> epf_ntb_epc_destroy() duplicates the teardown that the caller is
> supposed to do later. This leads to an oops when .allow_link fails or
> when .drop_link is performed. Remove the helper.
>
> Also drop pci_epc_put(). EPC device refcounting is tied to configfs EPC
> group lifetime, and pci_epc_put() in the .drop_link path is sufficient.
>
> Cc: <stable@vger.kernel.org>
> Fixes: 8b821cf76150 ("PCI: endpoint: Add EP function driver to provide NTB functionality")
> Signed-off-by: Koichiro Den <den@valinux.co.jp>

Reviewed-by: Frank Li <Frank.Li@nxp.com>

> ---
>  drivers/pci/endpoint/functions/pci-epf-ntb.c | 56 +-------------------
>  1 file changed, 2 insertions(+), 54 deletions(-)
>
> diff --git a/drivers/pci/endpoint/functions/pci-epf-ntb.c b/drivers/pci/endpoint/functions/pci-epf-ntb.c
> index e01a98e74d21..7702ebb81d99 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-ntb.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-ntb.c
> @@ -1494,47 +1494,6 @@ static int epf_ntb_db_mw_bar_init(struct epf_ntb *ntb,
>  	return ret;
>  }
>
> -/**
> - * epf_ntb_epc_destroy_interface() - Cleanup NTB EPC interface
> - * @ntb: NTB device that facilitates communication between HOST1 and HOST2
> - * @type: PRIMARY interface or SECONDARY interface
> - *
> - * Unbind NTB function device from EPC and relinquish reference to pci_epc
> - * for each of the interface.
> - */
> -static void epf_ntb_epc_destroy_interface(struct epf_ntb *ntb,
> -					  enum pci_epc_interface_type type)
> -{
> -	struct epf_ntb_epc *ntb_epc;
> -	struct pci_epc *epc;
> -	struct pci_epf *epf;
> -
> -	if (type < 0)
> -		return;
> -
> -	epf = ntb->epf;
> -	ntb_epc = ntb->epc[type];
> -	if (!ntb_epc)
> -		return;
> -	epc = ntb_epc->epc;
> -	pci_epc_remove_epf(epc, epf, type);
> -	pci_epc_put(epc);
> -}
> -
> -/**
> - * epf_ntb_epc_destroy() - Cleanup NTB EPC interface
> - * @ntb: NTB device that facilitates communication between HOST1 and HOST2
> - *
> - * Wrapper for epf_ntb_epc_destroy_interface() to cleanup all the NTB interfaces
> - */
> -static void epf_ntb_epc_destroy(struct epf_ntb *ntb)
> -{
> -	enum pci_epc_interface_type type;
> -
> -	for (type = PRIMARY_INTERFACE; type <= SECONDARY_INTERFACE; type++)
> -		epf_ntb_epc_destroy_interface(ntb, type);
> -}
> -
>  /**
>   * epf_ntb_epc_create_interface() - Create and initialize NTB EPC interface
>   * @ntb: NTB device that facilitates communication between HOST1 and HOST2
> @@ -1614,15 +1573,8 @@ static int epf_ntb_epc_create(struct epf_ntb *ntb)
>
>  	ret = epf_ntb_epc_create_interface(ntb, epf->sec_epc,
>  					   SECONDARY_INTERFACE);
> -	if (ret) {
> +	if (ret)
>  		dev_err(dev, "SECONDARY intf: Fail to create NTB EPC\n");
> -		goto err_epc_create;
> -	}
> -
> -	return 0;
> -
> -err_epc_create:
> -	epf_ntb_epc_destroy_interface(ntb, PRIMARY_INTERFACE);
>
>  	return ret;
>  }
> @@ -1887,7 +1839,7 @@ static int epf_ntb_bind(struct pci_epf *epf)
>  	ret = epf_ntb_init_epc_bar(ntb);
>  	if (ret) {
>  		dev_err(dev, "Failed to create NTB EPC\n");
> -		goto err_bar_init;
> +		return ret;
>  	}
>
>  	ret = epf_ntb_config_spad_bar_alloc_interface(ntb);
> @@ -1909,9 +1861,6 @@ static int epf_ntb_bind(struct pci_epf *epf)
>  err_bar_alloc:
>  	epf_ntb_config_spad_bar_free(ntb);
>
> -err_bar_init:
> -	epf_ntb_epc_destroy(ntb);
> -
>  	return ret;
>  }
>
> @@ -1927,7 +1876,6 @@ static void epf_ntb_unbind(struct pci_epf *epf)
>
>  	epf_ntb_epc_cleanup(ntb);
>  	epf_ntb_config_spad_bar_free(ntb);
> -	epf_ntb_epc_destroy(ntb);
>  }
>
>  #define EPF_NTB_R(_name)						\
> --
> 2.48.1
>

Re: [PATCH v2 6/6] PCI: endpoint: pci-epf-vntb: manage ntb_dev lifetime and fix vpci bus teardown

[Frank Li]

On Wed, Oct 29, 2025 at 05:03:21PM +0900, Koichiro Den wrote:
> Currently ntb_dev is embedded in epf_ntb, while configfs allows starting
> or stopping controller and linking or unlinking functions as you want.
> In fact, re-linking and re-starting is not possible with the embedded
> design and leads to oopses.
>
> Allocate ntb_dev with devm and add a .remove callback to the pci driver
> that calls ntb_unregister_device(). This allows a fresh device to be
> created on the next .bind call.
>
> With these changes, the controller can now be stopped, a function
> unlinked, configfs settings updated, and the controller re-linked and
> restarted without rebooting the endpoint, as long as the underlying
> pci_epc_ops .stop() operation is non-destructive, and .start() can
> restore normal operations.
>
> Signed-off-by: Koichiro Den <den@valinux.co.jp>
> ---
>  drivers/pci/endpoint/functions/pci-epf-vntb.c | 66 +++++++++++++++----
>  1 file changed, 52 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/pci/endpoint/functions/pci-epf-vntb.c b/drivers/pci/endpoint/functions/pci-epf-vntb.c
> index 750a246f79c9..3059ed85a955 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-vntb.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-vntb.c
> @@ -118,7 +118,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;
>
> @@ -144,10 +144,16 @@ 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 inline struct epf_ntb *ntb_ndev(struct ntb_dev *ntb)
> +{
> +	return (struct epf_ntb *)ntb->pdev->sysdata;
> +}
>
>  static struct pci_epf_header epf_ntb_header = {
>  	.vendorid	= PCI_ANY_ID,
> @@ -173,7 +179,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;
>  }
>
> @@ -261,7 +267,7 @@ static void epf_ntb_cmd_handler(struct work_struct *work)
>  	for (i = 1; i < ntb->db_count; i++) {
>  		if (ntb->epf_db[i]) {
>  			ntb->db |= 1 << (i - 1);
> -			ntb_db_event(&ntb->ntb, i);
> +			ntb_db_event(ntb->ntb, i);
>  			ntb->epf_db[i] = 0;
>  		}
>  	}
> @@ -1097,12 +1103,24 @@ static int vpci_scan_bus(void *sysdata)
>  {
>  	struct pci_bus *vpci_bus;
>  	struct epf_ntb *ndev = sysdata;
> -
> -	vpci_bus = pci_scan_bus(ndev->vbus_number, &vpci_ops, sysdata);
> +	LIST_HEAD(resources);
> +	static struct resource busn_res = {
> +		.start = 0,
> +		.end = 255,
> +		.flags = IORESOURCE_BUS,
> +	};
> +
> +	pci_add_resource(&resources, &ioport_resource);
> +	pci_add_resource(&resources, &iomem_resource);
> +	pci_add_resource(&resources, &busn_res);
> +
> +	vpci_bus = pci_scan_root_bus(&ndev->epf->epc->dev, ndev->vbus_number,
> +				     &vpci_ops, sysdata, &resources);

look this part is not belong to this patch. just change API
pci_scan_bus() to pci_scan_root_bus()?

Frank

>  	if (!vpci_bus) {
>  		pr_err("create pci bus failed\n");
>  		return -EINVAL;
>  	}
> +	ndev->vpci_bus = vpci_bus;
>
>  	pci_bus_add_devices(vpci_bus);
>
> @@ -1147,7 +1165,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 = &ntb->ntb->dev;
>  	barno = ntb->epf_ntb_bar[BAR_MW1 + idx];
>  	epf_bar = &ntb->epf->bar[barno];
>  	epf_bar->phys_addr = addr;
> @@ -1247,7 +1265,7 @@ static int vntb_epf_peer_db_set(struct ntb_dev *ndev, u64 db_bits)
>  	ret = pci_epc_raise_irq(ntb->epf->epc, func_no, vfunc_no,
>  				PCI_IRQ_MSI, interrupt_num + 1);
>  	if (ret)
> -		dev_err(&ntb->ntb.dev, "Failed to raise IRQ\n");
> +		dev_err(&ntb->ntb->dev, "Failed to raise IRQ\n");
>
>  	return ret;
>  }
> @@ -1334,9 +1352,12 @@ static int pci_vntb_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  	struct epf_ntb *ndev = (struct epf_ntb *)pdev->sysdata;
>  	struct device *dev = &pdev->dev;
>
> -	ndev->ntb.pdev = pdev;
> -	ndev->ntb.topo = NTB_TOPO_NONE;
> -	ndev->ntb.ops =  &vntb_epf_ops;
> +	ndev->ntb = devm_kzalloc(dev, sizeof(*ndev->ntb), GFP_KERNEL);
> +	if (!ndev->ntb)
> +		return -ENOMEM;
> +	ndev->ntb->pdev = pdev;
> +	ndev->ntb->topo = NTB_TOPO_NONE;
> +	ndev->ntb->ops = &vntb_epf_ops;
>
>  	ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32));
>  	if (ret) {
> @@ -1344,7 +1365,7 @@ 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(ndev->ntb);
>  	if (ret) {
>  		dev_err(dev, "Failed to register NTB device\n");
>  		return ret;
> @@ -1354,6 +1375,17 @@ static int pci_vntb_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  	return 0;
>  }
>
> +static void pci_vntb_remove(struct pci_dev *pdev)
> +{
> +	struct epf_ntb *ndev = (struct epf_ntb *)pdev->sysdata;
> +
> +	if (!ndev || !ndev->ntb)
> +		return;
> +
> +	ntb_unregister_device(ndev->ntb);
> +	ndev->ntb = NULL;
> +}
> +
>  static struct pci_device_id pci_vntb_table[] = {
>  	{
>  		PCI_DEVICE(0xffff, 0xffff),
> @@ -1365,6 +1397,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 ====================*/
> @@ -1447,10 +1480,15 @@ static void epf_ntb_unbind(struct pci_epf *epf)
>  {
>  	struct epf_ntb *ntb = epf_get_drvdata(epf);
>
> +	pci_unregister_driver(&vntb_pci_driver);
> +
> +	pci_lock_rescan_remove();
> +	pci_stop_root_bus(ntb->vpci_bus);
> +	pci_remove_root_bus(ntb->vpci_bus);
> +	pci_unlock_rescan_remove();
> +
>  	epf_ntb_epc_cleanup(ntb);
>  	epf_ntb_config_spad_bar_free(ntb);
> -
> -	pci_unregister_driver(&vntb_pci_driver);
>  }
>
>  // EPF driver probe
> --
> 2.48.1
>

Re: [PATCH v2 6/6] PCI: endpoint: pci-epf-vntb: manage ntb_dev lifetime and fix vpci bus teardown

[Koichiro Den]

On Wed, Oct 29, 2025 at 12:53:40PM -0400, Frank Li wrote:
> On Wed, Oct 29, 2025 at 05:03:21PM +0900, Koichiro Den wrote:
> > Currently ntb_dev is embedded in epf_ntb, while configfs allows starting
> > or stopping controller and linking or unlinking functions as you want.
> > In fact, re-linking and re-starting is not possible with the embedded
> > design and leads to oopses.
> >
> > Allocate ntb_dev with devm and add a .remove callback to the pci driver
> > that calls ntb_unregister_device(). This allows a fresh device to be
> > created on the next .bind call.
> >
> > With these changes, the controller can now be stopped, a function
> > unlinked, configfs settings updated, and the controller re-linked and
> > restarted without rebooting the endpoint, as long as the underlying
> > pci_epc_ops .stop() operation is non-destructive, and .start() can
> > restore normal operations.
> >
> > Signed-off-by: Koichiro Den <den@valinux.co.jp>
> > ---
> >  drivers/pci/endpoint/functions/pci-epf-vntb.c | 66 +++++++++++++++----
> >  1 file changed, 52 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/pci/endpoint/functions/pci-epf-vntb.c b/drivers/pci/endpoint/functions/pci-epf-vntb.c
> > index 750a246f79c9..3059ed85a955 100644
> > --- a/drivers/pci/endpoint/functions/pci-epf-vntb.c
> > +++ b/drivers/pci/endpoint/functions/pci-epf-vntb.c
> > @@ -118,7 +118,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;
> >
> > @@ -144,10 +144,16 @@ 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 inline struct epf_ntb *ntb_ndev(struct ntb_dev *ntb)
> > +{
> > +	return (struct epf_ntb *)ntb->pdev->sysdata;
> > +}
> >
> >  static struct pci_epf_header epf_ntb_header = {
> >  	.vendorid	= PCI_ANY_ID,
> > @@ -173,7 +179,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;
> >  }
> >
> > @@ -261,7 +267,7 @@ static void epf_ntb_cmd_handler(struct work_struct *work)
> >  	for (i = 1; i < ntb->db_count; i++) {
> >  		if (ntb->epf_db[i]) {
> >  			ntb->db |= 1 << (i - 1);
> > -			ntb_db_event(&ntb->ntb, i);
> > +			ntb_db_event(ntb->ntb, i);
> >  			ntb->epf_db[i] = 0;
> >  		}
> >  	}
> > @@ -1097,12 +1103,24 @@ static int vpci_scan_bus(void *sysdata)
> >  {
> >  	struct pci_bus *vpci_bus;
> >  	struct epf_ntb *ndev = sysdata;
> > -
> > -	vpci_bus = pci_scan_bus(ndev->vbus_number, &vpci_ops, sysdata);
> > +	LIST_HEAD(resources);
> > +	static struct resource busn_res = {
> > +		.start = 0,
> > +		.end = 255,
> > +		.flags = IORESOURCE_BUS,
> > +	};
> > +
> > +	pci_add_resource(&resources, &ioport_resource);
> > +	pci_add_resource(&resources, &iomem_resource);
> > +	pci_add_resource(&resources, &busn_res);
> > +
> > +	vpci_bus = pci_scan_root_bus(&ndev->epf->epc->dev, ndev->vbus_number,
> > +				     &vpci_ops, sysdata, &resources);
> 
> look this part is not belong to this patch. just change API
> pci_scan_bus() to pci_scan_root_bus()?

To make things work symmetrically and avoid crashes (when unlinking),
pci_scan_bus() needed to be switched to pci_scan_root_bus() to set the
parent device, like no longer existing pci_scan_bus_parented(). Otherwise,
pci_epf_unbind()->epf_ntb_unbind()->pci_remove_root_bus()->pci_bus_release_domain_nr()
would crash.

Perhaps I should've added an explanation for this in the git commit message?

Thanks for reviewing.

-Koichiro

> 
> Frank
> 
> >  	if (!vpci_bus) {
> >  		pr_err("create pci bus failed\n");
> >  		return -EINVAL;
> >  	}
> > +	ndev->vpci_bus = vpci_bus;
> >
> >  	pci_bus_add_devices(vpci_bus);
> >
> > @@ -1147,7 +1165,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 = &ntb->ntb->dev;
> >  	barno = ntb->epf_ntb_bar[BAR_MW1 + idx];
> >  	epf_bar = &ntb->epf->bar[barno];
> >  	epf_bar->phys_addr = addr;
> > @@ -1247,7 +1265,7 @@ static int vntb_epf_peer_db_set(struct ntb_dev *ndev, u64 db_bits)
> >  	ret = pci_epc_raise_irq(ntb->epf->epc, func_no, vfunc_no,
> >  				PCI_IRQ_MSI, interrupt_num + 1);
> >  	if (ret)
> > -		dev_err(&ntb->ntb.dev, "Failed to raise IRQ\n");
> > +		dev_err(&ntb->ntb->dev, "Failed to raise IRQ\n");
> >
> >  	return ret;
> >  }
> > @@ -1334,9 +1352,12 @@ static int pci_vntb_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> >  	struct epf_ntb *ndev = (struct epf_ntb *)pdev->sysdata;
> >  	struct device *dev = &pdev->dev;
> >
> > -	ndev->ntb.pdev = pdev;
> > -	ndev->ntb.topo = NTB_TOPO_NONE;
> > -	ndev->ntb.ops =  &vntb_epf_ops;
> > +	ndev->ntb = devm_kzalloc(dev, sizeof(*ndev->ntb), GFP_KERNEL);
> > +	if (!ndev->ntb)
> > +		return -ENOMEM;
> > +	ndev->ntb->pdev = pdev;
> > +	ndev->ntb->topo = NTB_TOPO_NONE;
> > +	ndev->ntb->ops = &vntb_epf_ops;
> >
> >  	ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32));
> >  	if (ret) {
> > @@ -1344,7 +1365,7 @@ 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(ndev->ntb);
> >  	if (ret) {
> >  		dev_err(dev, "Failed to register NTB device\n");
> >  		return ret;
> > @@ -1354,6 +1375,17 @@ static int pci_vntb_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> >  	return 0;
> >  }
> >
> > +static void pci_vntb_remove(struct pci_dev *pdev)
> > +{
> > +	struct epf_ntb *ndev = (struct epf_ntb *)pdev->sysdata;
> > +
> > +	if (!ndev || !ndev->ntb)
> > +		return;
> > +
> > +	ntb_unregister_device(ndev->ntb);
> > +	ndev->ntb = NULL;
> > +}
> > +
> >  static struct pci_device_id pci_vntb_table[] = {
> >  	{
> >  		PCI_DEVICE(0xffff, 0xffff),
> > @@ -1365,6 +1397,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 ====================*/
> > @@ -1447,10 +1480,15 @@ static void epf_ntb_unbind(struct pci_epf *epf)
> >  {
> >  	struct epf_ntb *ntb = epf_get_drvdata(epf);
> >
> > +	pci_unregister_driver(&vntb_pci_driver);
> > +
> > +	pci_lock_rescan_remove();
> > +	pci_stop_root_bus(ntb->vpci_bus);
> > +	pci_remove_root_bus(ntb->vpci_bus);
> > +	pci_unlock_rescan_remove();
> > +
> >  	epf_ntb_epc_cleanup(ntb);
> >  	epf_ntb_config_spad_bar_free(ntb);
> > -
> > -	pci_unregister_driver(&vntb_pci_driver);
> >  }
> >
> >  // EPF driver probe
> > --
> > 2.48.1
> >

Re: [PATCH v2 6/6] PCI: endpoint: pci-epf-vntb: manage ntb_dev lifetime and fix vpci bus teardown

[Frank Li]

On Thu, Oct 30, 2025 at 11:20:22AM +0900, Koichiro Den wrote:
> On Wed, Oct 29, 2025 at 12:53:40PM -0400, Frank Li wrote:
> > On Wed, Oct 29, 2025 at 05:03:21PM +0900, Koichiro Den wrote:
> > > Currently ntb_dev is embedded in epf_ntb, while configfs allows starting
> > > or stopping controller and linking or unlinking functions as you want.
> > > In fact, re-linking and re-starting is not possible with the embedded
> > > design and leads to oopses.
> > >
> > > Allocate ntb_dev with devm and add a .remove callback to the pci driver
> > > that calls ntb_unregister_device(). This allows a fresh device to be
> > > created on the next .bind call.
> > >
> > > With these changes, the controller can now be stopped, a function
> > > unlinked, configfs settings updated, and the controller re-linked and
> > > restarted without rebooting the endpoint, as long as the underlying
> > > pci_epc_ops .stop() operation is non-destructive, and .start() can
> > > restore normal operations.
> > >
> > > Signed-off-by: Koichiro Den <den@valinux.co.jp>
> > > ---
> > >  drivers/pci/endpoint/functions/pci-epf-vntb.c | 66 +++++++++++++++----
> > >  1 file changed, 52 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/drivers/pci/endpoint/functions/pci-epf-vntb.c b/drivers/pci/endpoint/functions/pci-epf-vntb.c
> > > index 750a246f79c9..3059ed85a955 100644
> > > --- a/drivers/pci/endpoint/functions/pci-epf-vntb.c
> > > +++ b/drivers/pci/endpoint/functions/pci-epf-vntb.c
> > > @@ -118,7 +118,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;
> > >
> > > @@ -144,10 +144,16 @@ 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 inline struct epf_ntb *ntb_ndev(struct ntb_dev *ntb)
> > > +{
> > > +	return (struct epf_ntb *)ntb->pdev->sysdata;
> > > +}
> > >
> > >  static struct pci_epf_header epf_ntb_header = {
> > >  	.vendorid	= PCI_ANY_ID,
> > > @@ -173,7 +179,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;
> > >  }
> > >
> > > @@ -261,7 +267,7 @@ static void epf_ntb_cmd_handler(struct work_struct *work)
> > >  	for (i = 1; i < ntb->db_count; i++) {
> > >  		if (ntb->epf_db[i]) {
> > >  			ntb->db |= 1 << (i - 1);
> > > -			ntb_db_event(&ntb->ntb, i);
> > > +			ntb_db_event(ntb->ntb, i);
> > >  			ntb->epf_db[i] = 0;
> > >  		}
> > >  	}
> > > @@ -1097,12 +1103,24 @@ static int vpci_scan_bus(void *sysdata)
> > >  {
> > >  	struct pci_bus *vpci_bus;
> > >  	struct epf_ntb *ndev = sysdata;
> > > -
> > > -	vpci_bus = pci_scan_bus(ndev->vbus_number, &vpci_ops, sysdata);
> > > +	LIST_HEAD(resources);
> > > +	static struct resource busn_res = {
> > > +		.start = 0,
> > > +		.end = 255,
> > > +		.flags = IORESOURCE_BUS,
> > > +	};
> > > +
> > > +	pci_add_resource(&resources, &ioport_resource);
> > > +	pci_add_resource(&resources, &iomem_resource);
> > > +	pci_add_resource(&resources, &busn_res);
> > > +
> > > +	vpci_bus = pci_scan_root_bus(&ndev->epf->epc->dev, ndev->vbus_number,
> > > +				     &vpci_ops, sysdata, &resources);
> >
> > look this part is not belong to this patch. just change API
> > pci_scan_bus() to pci_scan_root_bus()?
>
> To make things work symmetrically and avoid crashes (when unlinking),
> pci_scan_bus() needed to be switched to pci_scan_root_bus() to set the
> parent device, like no longer existing pci_scan_bus_parented(). Otherwise,
> pci_epf_unbind()->epf_ntb_unbind()->pci_remove_root_bus()->pci_bus_release_domain_nr()
> would crash.
>
> Perhaps I should've added an explanation for this in the git commit message?

Can you create new patch just convert pci_scan_bus() to pci_scan_root_bus()
and explanation at this new patch's commit message.

Frank

>
> Thanks for reviewing.
>
> -Koichiro
>
> >
> > Frank
> >
> > >  	if (!vpci_bus) {
> > >  		pr_err("create pci bus failed\n");
> > >  		return -EINVAL;
> > >  	}
> > > +	ndev->vpci_bus = vpci_bus;
> > >
> > >  	pci_bus_add_devices(vpci_bus);
> > >
> > > @@ -1147,7 +1165,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 = &ntb->ntb->dev;
> > >  	barno = ntb->epf_ntb_bar[BAR_MW1 + idx];
> > >  	epf_bar = &ntb->epf->bar[barno];
> > >  	epf_bar->phys_addr = addr;
> > > @@ -1247,7 +1265,7 @@ static int vntb_epf_peer_db_set(struct ntb_dev *ndev, u64 db_bits)
> > >  	ret = pci_epc_raise_irq(ntb->epf->epc, func_no, vfunc_no,
> > >  				PCI_IRQ_MSI, interrupt_num + 1);
> > >  	if (ret)
> > > -		dev_err(&ntb->ntb.dev, "Failed to raise IRQ\n");
> > > +		dev_err(&ntb->ntb->dev, "Failed to raise IRQ\n");
> > >
> > >  	return ret;
> > >  }
> > > @@ -1334,9 +1352,12 @@ static int pci_vntb_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> > >  	struct epf_ntb *ndev = (struct epf_ntb *)pdev->sysdata;
> > >  	struct device *dev = &pdev->dev;
> > >
> > > -	ndev->ntb.pdev = pdev;
> > > -	ndev->ntb.topo = NTB_TOPO_NONE;
> > > -	ndev->ntb.ops =  &vntb_epf_ops;
> > > +	ndev->ntb = devm_kzalloc(dev, sizeof(*ndev->ntb), GFP_KERNEL);
> > > +	if (!ndev->ntb)
> > > +		return -ENOMEM;
> > > +	ndev->ntb->pdev = pdev;
> > > +	ndev->ntb->topo = NTB_TOPO_NONE;
> > > +	ndev->ntb->ops = &vntb_epf_ops;
> > >
> > >  	ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32));
> > >  	if (ret) {
> > > @@ -1344,7 +1365,7 @@ 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(ndev->ntb);
> > >  	if (ret) {
> > >  		dev_err(dev, "Failed to register NTB device\n");
> > >  		return ret;
> > > @@ -1354,6 +1375,17 @@ static int pci_vntb_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> > >  	return 0;
> > >  }
> > >
> > > +static void pci_vntb_remove(struct pci_dev *pdev)
> > > +{
> > > +	struct epf_ntb *ndev = (struct epf_ntb *)pdev->sysdata;
> > > +
> > > +	if (!ndev || !ndev->ntb)
> > > +		return;
> > > +
> > > +	ntb_unregister_device(ndev->ntb);
> > > +	ndev->ntb = NULL;
> > > +}
> > > +
> > >  static struct pci_device_id pci_vntb_table[] = {
> > >  	{
> > >  		PCI_DEVICE(0xffff, 0xffff),
> > > @@ -1365,6 +1397,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 ====================*/
> > > @@ -1447,10 +1480,15 @@ static void epf_ntb_unbind(struct pci_epf *epf)
> > >  {
> > >  	struct epf_ntb *ntb = epf_get_drvdata(epf);
> > >
> > > +	pci_unregister_driver(&vntb_pci_driver);
> > > +
> > > +	pci_lock_rescan_remove();
> > > +	pci_stop_root_bus(ntb->vpci_bus);
> > > +	pci_remove_root_bus(ntb->vpci_bus);
> > > +	pci_unlock_rescan_remove();
> > > +
> > >  	epf_ntb_epc_cleanup(ntb);
> > >  	epf_ntb_config_spad_bar_free(ntb);
> > > -
> > > -	pci_unregister_driver(&vntb_pci_driver);
> > >  }
> > >
> > >  // EPF driver probe
> > > --
> > > 2.48.1
> > >

Re: [PATCH v2 6/6] PCI: endpoint: pci-epf-vntb: manage ntb_dev lifetime and fix vpci bus teardown

[Koichiro Den]

On Fri, Oct 31, 2025 at 03:52:22PM -0400, Frank Li wrote:
> On Thu, Oct 30, 2025 at 11:20:22AM +0900, Koichiro Den wrote:
> > On Wed, Oct 29, 2025 at 12:53:40PM -0400, Frank Li wrote:
> > > On Wed, Oct 29, 2025 at 05:03:21PM +0900, Koichiro Den wrote:
> > > > Currently ntb_dev is embedded in epf_ntb, while configfs allows starting
> > > > or stopping controller and linking or unlinking functions as you want.
> > > > In fact, re-linking and re-starting is not possible with the embedded
> > > > design and leads to oopses.
> > > >
> > > > Allocate ntb_dev with devm and add a .remove callback to the pci driver
> > > > that calls ntb_unregister_device(). This allows a fresh device to be
> > > > created on the next .bind call.
> > > >
> > > > With these changes, the controller can now be stopped, a function
> > > > unlinked, configfs settings updated, and the controller re-linked and
> > > > restarted without rebooting the endpoint, as long as the underlying
> > > > pci_epc_ops .stop() operation is non-destructive, and .start() can
> > > > restore normal operations.
> > > >
> > > > Signed-off-by: Koichiro Den <den@valinux.co.jp>
> > > > ---
> > > >  drivers/pci/endpoint/functions/pci-epf-vntb.c | 66 +++++++++++++++----
> > > >  1 file changed, 52 insertions(+), 14 deletions(-)
> > > >
> > > > diff --git a/drivers/pci/endpoint/functions/pci-epf-vntb.c b/drivers/pci/endpoint/functions/pci-epf-vntb.c
> > > > index 750a246f79c9..3059ed85a955 100644
> > > > --- a/drivers/pci/endpoint/functions/pci-epf-vntb.c
> > > > +++ b/drivers/pci/endpoint/functions/pci-epf-vntb.c
> > > > @@ -118,7 +118,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;
> > > >
> > > > @@ -144,10 +144,16 @@ 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 inline struct epf_ntb *ntb_ndev(struct ntb_dev *ntb)
> > > > +{
> > > > +	return (struct epf_ntb *)ntb->pdev->sysdata;
> > > > +}
> > > >
> > > >  static struct pci_epf_header epf_ntb_header = {
> > > >  	.vendorid	= PCI_ANY_ID,
> > > > @@ -173,7 +179,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;
> > > >  }
> > > >
> > > > @@ -261,7 +267,7 @@ static void epf_ntb_cmd_handler(struct work_struct *work)
> > > >  	for (i = 1; i < ntb->db_count; i++) {
> > > >  		if (ntb->epf_db[i]) {
> > > >  			ntb->db |= 1 << (i - 1);
> > > > -			ntb_db_event(&ntb->ntb, i);
> > > > +			ntb_db_event(ntb->ntb, i);
> > > >  			ntb->epf_db[i] = 0;
> > > >  		}
> > > >  	}
> > > > @@ -1097,12 +1103,24 @@ static int vpci_scan_bus(void *sysdata)
> > > >  {
> > > >  	struct pci_bus *vpci_bus;
> > > >  	struct epf_ntb *ndev = sysdata;
> > > > -
> > > > -	vpci_bus = pci_scan_bus(ndev->vbus_number, &vpci_ops, sysdata);
> > > > +	LIST_HEAD(resources);
> > > > +	static struct resource busn_res = {
> > > > +		.start = 0,
> > > > +		.end = 255,
> > > > +		.flags = IORESOURCE_BUS,
> > > > +	};
> > > > +
> > > > +	pci_add_resource(&resources, &ioport_resource);
> > > > +	pci_add_resource(&resources, &iomem_resource);
> > > > +	pci_add_resource(&resources, &busn_res);
> > > > +
> > > > +	vpci_bus = pci_scan_root_bus(&ndev->epf->epc->dev, ndev->vbus_number,
> > > > +				     &vpci_ops, sysdata, &resources);
> > >
> > > look this part is not belong to this patch. just change API
> > > pci_scan_bus() to pci_scan_root_bus()?
> >
> > To make things work symmetrically and avoid crashes (when unlinking),
> > pci_scan_bus() needed to be switched to pci_scan_root_bus() to set the
> > parent device, like no longer existing pci_scan_bus_parented(). Otherwise,
> > pci_epf_unbind()->epf_ntb_unbind()->pci_remove_root_bus()->pci_bus_release_domain_nr()
> > would crash.
> >
> > Perhaps I should've added an explanation for this in the git commit message?
> 
> Can you create new patch just convert pci_scan_bus() to pci_scan_root_bus()
> and explanation at this new patch's commit message.

Ok, I'll do so. Let me respin the series later (v3).
Thanks for the review.

-Koichiro

> 
> Frank
> 
> >
> > Thanks for reviewing.
> >
> > -Koichiro
> >
> > >
> > > Frank
> > >
> > > >  	if (!vpci_bus) {
> > > >  		pr_err("create pci bus failed\n");
> > > >  		return -EINVAL;
> > > >  	}
> > > > +	ndev->vpci_bus = vpci_bus;
> > > >
> > > >  	pci_bus_add_devices(vpci_bus);
> > > >
> > > > @@ -1147,7 +1165,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 = &ntb->ntb->dev;
> > > >  	barno = ntb->epf_ntb_bar[BAR_MW1 + idx];
> > > >  	epf_bar = &ntb->epf->bar[barno];
> > > >  	epf_bar->phys_addr = addr;
> > > > @@ -1247,7 +1265,7 @@ static int vntb_epf_peer_db_set(struct ntb_dev *ndev, u64 db_bits)
> > > >  	ret = pci_epc_raise_irq(ntb->epf->epc, func_no, vfunc_no,
> > > >  				PCI_IRQ_MSI, interrupt_num + 1);
> > > >  	if (ret)
> > > > -		dev_err(&ntb->ntb.dev, "Failed to raise IRQ\n");
> > > > +		dev_err(&ntb->ntb->dev, "Failed to raise IRQ\n");
> > > >
> > > >  	return ret;
> > > >  }
> > > > @@ -1334,9 +1352,12 @@ static int pci_vntb_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> > > >  	struct epf_ntb *ndev = (struct epf_ntb *)pdev->sysdata;
> > > >  	struct device *dev = &pdev->dev;
> > > >
> > > > -	ndev->ntb.pdev = pdev;
> > > > -	ndev->ntb.topo = NTB_TOPO_NONE;
> > > > -	ndev->ntb.ops =  &vntb_epf_ops;
> > > > +	ndev->ntb = devm_kzalloc(dev, sizeof(*ndev->ntb), GFP_KERNEL);
> > > > +	if (!ndev->ntb)
> > > > +		return -ENOMEM;
> > > > +	ndev->ntb->pdev = pdev;
> > > > +	ndev->ntb->topo = NTB_TOPO_NONE;
> > > > +	ndev->ntb->ops = &vntb_epf_ops;
> > > >
> > > >  	ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32));
> > > >  	if (ret) {
> > > > @@ -1344,7 +1365,7 @@ 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(ndev->ntb);
> > > >  	if (ret) {
> > > >  		dev_err(dev, "Failed to register NTB device\n");
> > > >  		return ret;
> > > > @@ -1354,6 +1375,17 @@ static int pci_vntb_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> > > >  	return 0;
> > > >  }
> > > >
> > > > +static void pci_vntb_remove(struct pci_dev *pdev)
> > > > +{
> > > > +	struct epf_ntb *ndev = (struct epf_ntb *)pdev->sysdata;
> > > > +
> > > > +	if (!ndev || !ndev->ntb)
> > > > +		return;
> > > > +
> > > > +	ntb_unregister_device(ndev->ntb);
> > > > +	ndev->ntb = NULL;
> > > > +}
> > > > +
> > > >  static struct pci_device_id pci_vntb_table[] = {
> > > >  	{
> > > >  		PCI_DEVICE(0xffff, 0xffff),
> > > > @@ -1365,6 +1397,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 ====================*/
> > > > @@ -1447,10 +1480,15 @@ static void epf_ntb_unbind(struct pci_epf *epf)
> > > >  {
> > > >  	struct epf_ntb *ntb = epf_get_drvdata(epf);
> > > >
> > > > +	pci_unregister_driver(&vntb_pci_driver);
> > > > +
> > > > +	pci_lock_rescan_remove();
> > > > +	pci_stop_root_bus(ntb->vpci_bus);
> > > > +	pci_remove_root_bus(ntb->vpci_bus);
> > > > +	pci_unlock_rescan_remove();
> > > > +
> > > >  	epf_ntb_epc_cleanup(ntb);
> > > >  	epf_ntb_config_spad_bar_free(ntb);
> > > > -
> > > > -	pci_unregister_driver(&vntb_pci_driver);
> > > >  }
> > > >
> > > >  // EPF driver probe
> > > > --
> > > > 2.48.1
> > > >