[PATCH v3 0/6] PCI: EP: Add RC-to-EP doorbell with platform MSI controller

[PATCH v3 0/6] PCI: EP: Add RC-to-EP doorbell with platform MSI controller

[Frank Li]

┌────────────┐   ┌───────────────────────────────────┐   ┌────────────────┐
│            │   │                                   │   │                │
│            │   │ PCI Endpoint                      │   │ PCI Host       │
│            │   │                                   │   │                │
│            │◄──┤ 1.platform_msi_domain_alloc_irqs()│   │                │
│            │   │                                   │   │                │
│ MSI        ├──►│ 2.write_msi_msg()                 ├──►├─BAR<n>         │
│ Controller │   │   update doorbell register address│   │                │
│            │   │   for BAR                         │   │                │
│            │   │                                   │   │ 3. Write BAR<n>│
│            │◄──┼───────────────────────────────────┼───┤                │
│            │   │                                   │   │                │
│            ├──►│ 4.Irq Handle                      │   │                │
│            │   │                                   │   │                │
│            │   │                                   │   │                │
└────────────┘   └───────────────────────────────────┘   └────────────────┘

This patches based on old https://lore.kernel.org/imx/20221124055036.1630573-1-Frank.Li@nxp.com/

Original patch only target to vntb driver. But actually it is common
method.

This patches add new API to pci-epf-core, so any EP driver can use it.

Previous v2 discussion here.
https://lore.kernel.org/imx/20230911220920.1817033-1-Frank.Li@nxp.com/

Change from v2 to v3
- Fixed manivannan's comments
- Move common part to pci-ep-msi.c and pci-ep-msi.h
- rebase to 6.12-rc1
- use RevID to distingiush old version

mkdir /sys/kernel/config/pci_ep/functions/pci_epf_test/func1
echo 16 > /sys/kernel/config/pci_ep/functions/pci_epf_test/func1/msi_interrupts
echo 0x080c > /sys/kernel/config/pci_ep/functions/pci_epf_test/func1/deviceid
echo 0x1957 > /sys/kernel/config/pci_ep/functions/pci_epf_test/func1/vendorid
echo 1 > /sys/kernel/config/pci_ep/functions/pci_epf_test/func1/revid
^^^^^^ to enable platform msi support.
ln -s /sys/kernel/config/pci_ep/functions/pci_epf_test/func1 /sys/kernel/config/pci_ep/controllers/4c380000.pcie-ep

- use new device ID, which identify support doorbell to avoid broken
compatility.

    Enable doorbell support only for PCI_DEVICE_ID_IMX8_DB, while other devices
    keep the same behavior as before.

           EP side             RC with old driver      RC with new driver
    PCI_DEVICE_ID_IMX8_DB          no probe              doorbell enabled
    Other device ID             doorbell disabled*       doorbell disabled*

    * Behavior remains unchanged.

Change from v1 to v2
- Add missed patch for endpont/pci-epf-test.c
- Move alloc and free to epc driver from epf.
- Provide general help function for EPC driver to alloc platform msi irq.
- Fixed manivannan's comments.

To: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
To: Krzysztof Wilczyński <kw@linux.com>
To: Kishon Vijay Abraham I <kishon@kernel.org>
To: Bjorn Helgaas <bhelgaas@google.com>
To: Arnd Bergmann <arnd@arndb.de>
To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: linux-kernel@vger.kernel.org
Cc: linux-pci@vger.kernel.org
Cc: imx@lists.linux.dev
Cc: Niklas Cassel <cassel@kernel.org>
Cc: cassel@kernel.org
Cc: dlemoal@kernel.org
Cc: maz@kernel.org
Cc: tglx@linutronix.de
Cc: jdmason@kudzu.us

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
Frank Li (6):
      genirq/msi: Add cleanup guard define for msi_lock_descs()/msi_unlock_descs()
      PCI: endpoint: Add pci_epc_get_fn() API for customizable filtering
      PCI: endpoint: Add RC-to-EP doorbell support using platform MSI controller
      PCI: endpoint: pci-epf-test: Add doorbell test support
      misc: pci_endpoint_test: Add doorbell test case
      tools: PCI: Add 'B' option for test doorbell

 drivers/misc/pci_endpoint_test.c              |  60 ++++++++++
 drivers/pci/endpoint/Makefile                 |   2 +-
 drivers/pci/endpoint/functions/pci-epf-test.c |  58 ++++++++-
 drivers/pci/endpoint/pci-ep-msi.c             | 162 ++++++++++++++++++++++++++
 drivers/pci/endpoint/pci-epc-core.c           |  23 +++-
 include/linux/msi.h                           |   2 +
 include/linux/pci-ep-msi.h                    |  15 +++
 include/linux/pci-epc.h                       |   2 +
 include/linux/pci-epf.h                       |   6 +
 include/uapi/linux/pcitest.h                  |   1 +
 tools/pci/pcitest.c                           |  16 ++-
 11 files changed, 341 insertions(+), 6 deletions(-)
---
base-commit: f231847d7f5a171be4566099f654521606b3ec37
change-id: 20241010-ep-msi-8b4cab33b1be

Best regards,
---
Frank Li <Frank.Li@nxp.com>

[PATCH v3 1/6] genirq/msi: Add cleanup guard define for msi_lock_descs()/msi_unlock_descs()

[Frank Li]

Add a cleanup DEFINE_GUARD macro for msi_lock_descs() and
msi_unlock_descs() to simplify lock and unlock operations in error path.

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
 include/linux/msi.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/linux/msi.h b/include/linux/msi.h
index b10093c4d00ea..0b6cb7f303887 100644
--- a/include/linux/msi.h
+++ b/include/linux/msi.h
@@ -228,6 +228,8 @@ int msi_setup_device_data(struct device *dev);
 void msi_lock_descs(struct device *dev);
 void msi_unlock_descs(struct device *dev);
 
+DEFINE_GUARD(msi_descs, struct device *, msi_lock_descs(_T), msi_unlock_descs(_T))
+
 struct msi_desc *msi_domain_first_desc(struct device *dev, unsigned int domid,
 				       enum msi_desc_filter filter);
 

-- 
2.34.1

[PATCH v3 2/6] PCI: endpoint: Add pci_epc_get_fn() API for customizable filtering

[Frank Li]

Introduce the pci_epc_get_fn() API, allowing a custom filter callback
function to be passed. Reimplement pci_epc_get() using pci_epc_get_fn()
with a match name callback. Prepare the codebase for adding RC-to-EP
doorbell support.

Add DEFINE_FREE(pci_epc_put) to implement scope based auto cleanup.

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
 drivers/pci/endpoint/pci-epc-core.c | 23 +++++++++++++++++++++--
 include/linux/pci-epc.h             |  2 ++
 2 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/endpoint/pci-epc-core.c b/drivers/pci/endpoint/pci-epc-core.c
index 17f0071092550..14e6011df4b2c 100644
--- a/drivers/pci/endpoint/pci-epc-core.c
+++ b/drivers/pci/endpoint/pci-epc-core.c
@@ -48,6 +48,11 @@ void pci_epc_put(struct pci_epc *epc)
 }
 EXPORT_SYMBOL_GPL(pci_epc_put);
 
+static bool pci_epc_match_name(struct device *dev, void *name)
+{
+	return strcmp(dev_name(dev), name) == 0;
+}
+
 /**
  * pci_epc_get() - get the PCI endpoint controller
  * @epc_name: device name of the endpoint controller
@@ -56,6 +61,20 @@ EXPORT_SYMBOL_GPL(pci_epc_put);
  * endpoint controller
  */
 struct pci_epc *pci_epc_get(const char *epc_name)
+{
+	return pci_epc_get_fn(pci_epc_match_name, (void *)epc_name);
+}
+EXPORT_SYMBOL_GPL(pci_epc_get);
+
+/**
+ * pci_epc_get_fn() - get the PCI endpoint controller
+ * @fn: callback match filter function, return true when matched
+ * @param: parameter for callback function fn
+ *
+ * Invoke to get struct pci_epc * corresponding to the device name of the
+ * endpoint controller
+ */
+struct pci_epc *pci_epc_get_fn(bool (*fn)(struct device *dev, void *param), void *param)
 {
 	int ret = -EINVAL;
 	struct pci_epc *epc;
@@ -64,7 +83,7 @@ struct pci_epc *pci_epc_get(const char *epc_name)
 
 	class_dev_iter_init(&iter, &pci_epc_class, NULL, NULL);
 	while ((dev = class_dev_iter_next(&iter))) {
-		if (strcmp(epc_name, dev_name(dev)))
+		if (!fn(dev, param))
 			continue;
 
 		epc = to_pci_epc(dev);
@@ -82,7 +101,7 @@ struct pci_epc *pci_epc_get(const char *epc_name)
 	class_dev_iter_exit(&iter);
 	return ERR_PTR(ret);
 }
-EXPORT_SYMBOL_GPL(pci_epc_get);
+EXPORT_SYMBOL_GPL(pci_epc_get_fn);
 
 /**
  * pci_epc_get_first_free_bar() - helper to get first unreserved BAR
diff --git a/include/linux/pci-epc.h b/include/linux/pci-epc.h
index 42ef06136bd1a..682808f4510be 100644
--- a/include/linux/pci-epc.h
+++ b/include/linux/pci-epc.h
@@ -266,7 +266,9 @@ pci_epc_get_first_free_bar(const struct pci_epc_features *epc_features);
 enum pci_barno pci_epc_get_next_free_bar(const struct pci_epc_features
 					 *epc_features, enum pci_barno bar);
 struct pci_epc *pci_epc_get(const char *epc_name);
+struct pci_epc *pci_epc_get_fn(bool (*fn)(struct device *dev, void *param), void *param);
 void pci_epc_put(struct pci_epc *epc);
+DEFINE_FREE(pci_epc_put, void *, if (!(_T)) pci_epc_put(_T))
 
 int pci_epc_mem_init(struct pci_epc *epc, phys_addr_t base,
 		     size_t size, size_t page_size);

-- 
2.34.1

[PATCH v3 3/6] PCI: endpoint: Add RC-to-EP doorbell support using platform MSI controller

[Frank Li]

Doorbell feature is implemented by mapping the EP's MSI interrupt
controller message address to a dedicated BAR in the EPC core. It is the
responsibility of the EPF driver to pass the actual message data to be
written by the host to the doorbell BAR region through its own logic.

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
 drivers/pci/endpoint/Makefile     |   2 +-
 drivers/pci/endpoint/pci-ep-msi.c | 162 ++++++++++++++++++++++++++++++++++++++
 include/linux/pci-ep-msi.h        |  15 ++++
 include/linux/pci-epf.h           |   6 ++
 4 files changed, 184 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/endpoint/Makefile b/drivers/pci/endpoint/Makefile
index 95b2fe47e3b06..a1ccce440c2c5 100644
--- a/drivers/pci/endpoint/Makefile
+++ b/drivers/pci/endpoint/Makefile
@@ -5,4 +5,4 @@
 
 obj-$(CONFIG_PCI_ENDPOINT_CONFIGFS)	+= pci-ep-cfs.o
 obj-$(CONFIG_PCI_ENDPOINT)		+= pci-epc-core.o pci-epf-core.o\
-					   pci-epc-mem.o functions/
+					   pci-epc-mem.o pci-ep-msi.o functions/
diff --git a/drivers/pci/endpoint/pci-ep-msi.c b/drivers/pci/endpoint/pci-ep-msi.c
new file mode 100644
index 0000000000000..534dcd05c435c
--- /dev/null
+++ b/drivers/pci/endpoint/pci-ep-msi.c
@@ -0,0 +1,162 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * PCI Endpoint *Controller* (EPC) MSI library
+ *
+ * Copyright (C) 2024 NXP
+ * Author: Frank Li <Frank.Li@nxp.com>
+ */
+
+#include <linux/cleanup.h>
+#include <linux/device.h>
+#include <linux/slab.h>
+#include <linux/module.h>
+
+#include <linux/msi.h>
+#include <linux/pci-epc.h>
+#include <linux/pci-epf.h>
+#include <linux/pci-ep-cfs.h>
+#include <linux/pci-ep-msi.h>
+
+static irqreturn_t pci_epf_doorbell_handler(int irq, void *data)
+{
+	struct pci_epf *epf = data;
+	struct msi_desc *desc = irq_get_msi_desc(irq);
+
+	if (desc && epf->event_ops && epf->event_ops->doorbell)
+		epf->event_ops->doorbell(epf, desc->msi_index);
+
+	return IRQ_HANDLED;
+}
+
+static bool pci_epc_match_parent(struct device *dev, void *param)
+{
+	return dev->parent == param;
+}
+
+static void pci_epc_write_msi_msg(struct msi_desc *desc, struct msi_msg *msg)
+{
+	struct pci_epc *epc __free(pci_epc_put) = NULL;
+	struct pci_epf *epf;
+
+	epc = pci_epc_get_fn(pci_epc_match_parent, desc->dev);
+
+	if (!epc)
+		return;
+
+	/* Only support one EPF for doorbell */
+	epf = list_first_entry_or_null(&epc->pci_epf, struct pci_epf, list);
+	if (!epf)
+		return;
+
+	if (epf->msg && desc->msi_index < epf->num_db)
+		memcpy(epf->msg, msg, sizeof(*msg));
+}
+
+static int pci_epc_alloc_doorbell(struct pci_epc *epc, u8 func_no, u8 vfunc_no, int num_db)
+{
+	struct msi_desc *desc, *failed_desc;
+	struct pci_epf *epf;
+	struct device *dev;
+	int i = 0;
+	int ret;
+
+	if (IS_ERR_OR_NULL(epc))
+		return -EINVAL;
+
+	/* Currently only support one func and one vfunc for doorbell */
+	if (func_no || vfunc_no)
+		return -EINVAL;
+
+	epf = list_first_entry_or_null(&epc->pci_epf, struct pci_epf, list);
+	if (!epf)
+		return -EINVAL;
+
+	dev = epc->dev.parent;
+	ret = platform_device_msi_init_and_alloc_irqs(dev, num_db, pci_epc_write_msi_msg);
+	if (ret) {
+		dev_err(dev, "Failed to allocate MSI\n");
+		return -ENOMEM;
+	}
+
+	scoped_guard(msi_descs, dev)
+		msi_for_each_desc(desc, dev, MSI_DESC_ALL) {
+			ret = request_irq(desc->irq, pci_epf_doorbell_handler, 0,
+					  kasprintf(GFP_KERNEL, "pci-epc-doorbell%d", i++), epf);
+			if (ret) {
+				dev_err(dev, "Failed to request doorbell\n");
+				failed_desc = desc;
+				goto err_request_irq;
+			}
+		}
+
+	return 0;
+
+err_request_irq:
+	scoped_guard(msi_descs, dev)
+		msi_for_each_desc(desc, dev, MSI_DESC_ALL) {
+			if (desc == failed_desc)
+				break;
+			kfree(free_irq(desc->irq, epf));
+		}
+
+	platform_device_msi_free_irqs_all(epc->dev.parent);
+
+	return ret;
+}
+
+static void pci_epc_free_doorbell(struct pci_epc *epc, u8 func_no, u8 vfunc_no)
+{
+	struct msi_desc *desc;
+	struct pci_epf *epf;
+	struct device *dev;
+
+	dev = epc->dev.parent;
+
+	scoped_guard(msi_descs, dev)
+		msi_for_each_desc(desc, dev, MSI_DESC_ALL)
+			kfree(free_irq(desc->irq, epf));
+
+	platform_device_msi_free_irqs_all(epc->dev.parent);
+}
+
+int pci_epf_alloc_doorbell(struct pci_epf *epf, u16 num_db)
+{
+	struct pci_epc *epc;
+	struct device *dev;
+	void *msg;
+	int ret;
+
+	epc = epf->epc;
+	dev = &epc->dev;
+
+	guard(mutex)(&epc->lock);
+
+	msg = kcalloc(num_db, sizeof(struct msi_msg), GFP_KERNEL);
+	if (!msg)
+		return -ENOMEM;
+
+	epf->num_db = num_db;
+	epf->msg = msg;
+
+	ret = pci_epc_alloc_doorbell(epc, epf->func_no, epf->vfunc_no, num_db);
+	if (ret)
+		kfree(msg);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(pci_epf_alloc_doorbell);
+
+void pci_epf_free_doorbell(struct pci_epf *epf)
+{
+	struct pci_epc *epc = epf->epc;
+
+	guard(mutex)(&epc->lock);
+
+	epc = epf->epc;
+	pci_epc_free_doorbell(epc, epf->func_no, epf->vfunc_no);
+
+	kfree(epf->msg);
+	epf->msg = NULL;
+	epf->num_db = 0;
+}
+EXPORT_SYMBOL_GPL(pci_epf_free_doorbell);
diff --git a/include/linux/pci-ep-msi.h b/include/linux/pci-ep-msi.h
new file mode 100644
index 0000000000000..f0cfecf491199
--- /dev/null
+++ b/include/linux/pci-ep-msi.h
@@ -0,0 +1,15 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * PCI Endpoint *Function* side MSI header file
+ *
+ * Copyright (C) 2024 NXP
+ * Author: Frank Li <Frank.Li@nxp.com>
+ */
+
+#ifndef __PCI_EP_MSI__
+#define __PCI_EP_MSI__
+
+int pci_epf_alloc_doorbell(struct pci_epf *epf, u16 nums);
+void pci_epf_free_doorbell(struct pci_epf *epf);
+
+#endif /* __PCI_EP_MSI__ */
diff --git a/include/linux/pci-epf.h b/include/linux/pci-epf.h
index 18a3aeb62ae4e..1e7e5eb4067d7 100644
--- a/include/linux/pci-epf.h
+++ b/include/linux/pci-epf.h
@@ -75,6 +75,7 @@ struct pci_epf_ops {
  * @link_up: Callback for the EPC link up event
  * @link_down: Callback for the EPC link down event
  * @bus_master_enable: Callback for the EPC Bus Master Enable event
+ * @doorbell: Callback for EPC receive MSI from RC side
  */
 struct pci_epc_event_ops {
 	int (*epc_init)(struct pci_epf *epf);
@@ -82,6 +83,7 @@ struct pci_epc_event_ops {
 	int (*link_up)(struct pci_epf *epf);
 	int (*link_down)(struct pci_epf *epf);
 	int (*bus_master_enable)(struct pci_epf *epf);
+	int (*doorbell)(struct pci_epf *epf, int index);
 };
 
 /**
@@ -152,6 +154,8 @@ struct pci_epf_bar {
  * @vfunction_num_map: bitmap to manage virtual function number
  * @pci_vepf: list of virtual endpoint functions associated with this function
  * @event_ops: Callbacks for capturing the EPC events
+ * @msg: data for MSI from RC side
+ * @num_db: number of doorbells
  */
 struct pci_epf {
 	struct device		dev;
@@ -182,6 +186,8 @@ struct pci_epf {
 	unsigned long		vfunction_num_map;
 	struct list_head	pci_vepf;
 	const struct pci_epc_event_ops *event_ops;
+	struct msi_msg *msg;
+	u16 num_db;
 };
 
 /**

-- 
2.34.1

[PATCH v3 4/6] PCI: endpoint: pci-epf-test: Add doorbell test support

[Frank Li]

Add three registers: doorbell_bar, doorbell_addr, and doorbell_data,
along with doorbell_done. Use pci_epf_alloc_doorbell() to allocate a
doorbell address space.

Enable the Root Complex (RC) side driver to trigger pci-epc-test's doorbell
callback handler by writing doorbell_data to the mapped doorbell_bar's
address space.

Set doorbell_done in the doorbell callback to indicate completion.

To avoid broken compatibility, use new PID/VID and set RevID bigger than 0.
So only new pcitest program can distinguish with/without doorbell support
and avoid wrongly write test data to doorbell bar.

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
 drivers/pci/endpoint/functions/pci-epf-test.c | 58 ++++++++++++++++++++++++++-
 1 file changed, 56 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
index 7c2ed6eae53ad..c054d621353a6 100644
--- a/drivers/pci/endpoint/functions/pci-epf-test.c
+++ b/drivers/pci/endpoint/functions/pci-epf-test.c
@@ -11,12 +11,14 @@
 #include <linux/dmaengine.h>
 #include <linux/io.h>
 #include <linux/module.h>
+#include <linux/msi.h>
 #include <linux/slab.h>
 #include <linux/pci_ids.h>
 #include <linux/random.h>
 
 #include <linux/pci-epc.h>
 #include <linux/pci-epf.h>
+#include <linux/pci-ep-msi.h>
 #include <linux/pci_regs.h>
 
 #define IRQ_TYPE_INTX			0
@@ -39,6 +41,7 @@
 #define STATUS_IRQ_RAISED		BIT(6)
 #define STATUS_SRC_ADDR_INVALID		BIT(7)
 #define STATUS_DST_ADDR_INVALID		BIT(8)
+#define STATUS_DOORBELL_SUCCESS		BIT(9)
 
 #define FLAG_USE_DMA			BIT(0)
 
@@ -50,6 +53,7 @@ struct pci_epf_test {
 	void			*reg[PCI_STD_NUM_BARS];
 	struct pci_epf		*epf;
 	enum pci_barno		test_reg_bar;
+	enum pci_barno		doorbell_bar;
 	size_t			msix_table_offset;
 	struct delayed_work	cmd_handler;
 	struct dma_chan		*dma_chan_tx;
@@ -74,6 +78,9 @@ struct pci_epf_test_reg {
 	u32	irq_type;
 	u32	irq_number;
 	u32	flags;
+	u32	doorbell_bar;
+	u32	doorbell_addr;
+	u32	doorbell_data;
 } __packed;
 
 static struct pci_epf_header test_header = {
@@ -695,7 +702,7 @@ static int pci_epf_test_set_bar(struct pci_epf *epf)
 	enum pci_barno test_reg_bar = epf_test->test_reg_bar;
 
 	for (bar = 0; bar < PCI_STD_NUM_BARS; bar++) {
-		if (!epf_test->reg[bar])
+		if (!epf_test->reg[bar] && bar != epf_test->doorbell_bar)
 			continue;
 
 		ret = pci_epc_set_bar(epc, epf->func_no, epf->vfunc_no,
@@ -810,11 +817,24 @@ static int pci_epf_test_link_down(struct pci_epf *epf)
 	return 0;
 }
 
+static int pci_epf_test_doorbell(struct pci_epf *epf, int index)
+{
+	struct pci_epf_test *epf_test = epf_get_drvdata(epf);
+	enum pci_barno test_reg_bar = epf_test->test_reg_bar;
+	struct pci_epf_test_reg *reg = epf_test->reg[test_reg_bar];
+
+	reg->status |= STATUS_DOORBELL_SUCCESS;
+	pci_epf_test_raise_irq(epf_test, reg);
+
+	return 0;
+}
+
 static const struct pci_epc_event_ops pci_epf_test_event_ops = {
 	.epc_init = pci_epf_test_epc_init,
 	.epc_deinit = pci_epf_test_epc_deinit,
 	.link_up = pci_epf_test_link_up,
 	.link_down = pci_epf_test_link_down,
+	.doorbell = pci_epf_test_doorbell,
 };
 
 static int pci_epf_test_alloc_space(struct pci_epf *epf)
@@ -853,7 +873,7 @@ static int pci_epf_test_alloc_space(struct pci_epf *epf)
 		if (bar == NO_BAR)
 			break;
 
-		if (bar == test_reg_bar)
+		if (bar == test_reg_bar || bar == epf_test->doorbell_bar)
 			continue;
 
 		base = pci_epf_alloc_space(epf, bar_size[bar], bar,
@@ -887,7 +907,11 @@ static int pci_epf_test_bind(struct pci_epf *epf)
 	struct pci_epf_test *epf_test = epf_get_drvdata(epf);
 	const struct pci_epc_features *epc_features;
 	enum pci_barno test_reg_bar = BAR_0;
+	enum pci_barno doorbell_bar = NO_BAR;
 	struct pci_epc *epc = epf->epc;
+	struct msi_msg *msg;
+	u64 doorbell_addr;
+	u32 align;
 
 	if (WARN_ON_ONCE(!epc))
 		return -EINVAL;
@@ -905,10 +929,40 @@ static int pci_epf_test_bind(struct pci_epf *epf)
 	epf_test->test_reg_bar = test_reg_bar;
 	epf_test->epc_features = epc_features;
 
+	align = epc_features->align;
+	align = align ? align : 128;
+
+	/* Only revid >=1 support RC-to-EP Door bell */
+	ret = epf->header->revid > 0 ?  pci_epf_alloc_doorbell(epf, 1) : -EINVAL;
+	if (!ret) {
+		msg = epf->msg;
+		doorbell_bar = pci_epc_get_next_free_bar(epc_features, test_reg_bar + 1);
+
+		if (doorbell_bar > 0) {
+			epf_test->doorbell_bar = doorbell_bar;
+			doorbell_addr = msg->address_hi;
+			doorbell_addr <<= 32;
+			doorbell_addr |= msg->address_lo;
+			epf->bar[doorbell_bar].phys_addr = round_down(doorbell_addr, align);
+			epf->bar[doorbell_bar].barno = doorbell_bar;
+			epf->bar[doorbell_bar].size = align;
+		} else {
+			pci_epf_free_doorbell(epf);
+		}
+	}
+
 	ret = pci_epf_test_alloc_space(epf);
 	if (ret)
 		return ret;
 
+	if (doorbell_bar > 0) {
+		struct pci_epf_test_reg *reg = epf_test->reg[test_reg_bar];
+
+		reg->doorbell_addr = doorbell_addr & (align - 1);
+		reg->doorbell_data = msg->data;
+		reg->doorbell_bar = doorbell_bar;
+	}
+
 	return 0;
 }
 

-- 
2.34.1

[PATCH v3 5/6] misc: pci_endpoint_test: Add doorbell test case

[Frank Li]

Add three registers: PCIE_ENDPOINT_TEST_DB_BAR, PCIE_ENDPOINT_TEST_DB_ADDR,
and PCIE_ENDPOINT_TEST_DB_DATA.

Trigger the doorbell by writing data from PCI_ENDPOINT_TEST_DB_DATA to the
address provided by PCI_ENDPOINT_TEST_DB_ADDR and wait for endpoint
feedback.

Enable doorbell support only for PCI_DEVICE_ID_IMX8_DB, while other devices
keep the same behavior as before.

       EP side             RC with old driver      RC with new driver
PCI_DEVICE_ID_IMX8_DB          no probe              doorbell enabled
Other device ID             doorbell disabled*       doorbell disabled*

* Behavior remains unchanged.

Return failure if pcitest try to test doorbell bar.

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
 drivers/misc/pci_endpoint_test.c | 60 ++++++++++++++++++++++++++++++++++++++++
 include/uapi/linux/pcitest.h     |  1 +
 2 files changed, 61 insertions(+)

diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
index 3aaaf47fa4ee2..a2b68c613c782 100644
--- a/drivers/misc/pci_endpoint_test.c
+++ b/drivers/misc/pci_endpoint_test.c
@@ -53,6 +53,7 @@
 #define STATUS_IRQ_RAISED			BIT(6)
 #define STATUS_SRC_ADDR_INVALID			BIT(7)
 #define STATUS_DST_ADDR_INVALID			BIT(8)
+#define STATUS_DOORBELL_SUCCESS			BIT(9)
 
 #define PCI_ENDPOINT_TEST_LOWER_SRC_ADDR	0x0c
 #define PCI_ENDPOINT_TEST_UPPER_SRC_ADDR	0x10
@@ -67,7 +68,12 @@
 #define PCI_ENDPOINT_TEST_IRQ_NUMBER		0x28
 
 #define PCI_ENDPOINT_TEST_FLAGS			0x2c
+#define PCI_ENDPOINT_TEST_DB_BAR		0x30
+#define PCI_ENDPOINT_TEST_DB_ADDR		0x34
+#define PCI_ENDPOINT_TEST_DB_DATA		0x38
+
 #define FLAG_USE_DMA				BIT(0)
+#define FLAG_SUPPORT_DOORBELL			BIT(1)
 
 #define PCI_DEVICE_ID_TI_AM654			0xb00c
 #define PCI_DEVICE_ID_TI_J7200			0xb00f
@@ -75,6 +81,7 @@
 #define PCI_DEVICE_ID_TI_J721S2		0xb013
 #define PCI_DEVICE_ID_LS1088A			0x80c0
 #define PCI_DEVICE_ID_IMX8			0x0808
+#define PCI_DEVICE_ID_IMX8_DB			0x080c
 
 #define is_am654_pci_dev(pdev)		\
 		((pdev)->device == PCI_DEVICE_ID_TI_AM654)
@@ -108,6 +115,7 @@ enum pci_barno {
 	BAR_3,
 	BAR_4,
 	BAR_5,
+	NO_BAR = -1,
 };
 
 struct pci_endpoint_test {
@@ -124,12 +132,14 @@ struct pci_endpoint_test {
 	enum pci_barno test_reg_bar;
 	size_t alignment;
 	const char *name;
+	bool support_db;
 };
 
 struct pci_endpoint_test_data {
 	enum pci_barno test_reg_bar;
 	size_t alignment;
 	int irq_type;
+	bool support_db;
 };
 
 static inline u32 pci_endpoint_test_readl(struct pci_endpoint_test *test,
@@ -746,6 +756,39 @@ static bool pci_endpoint_test_set_irq(struct pci_endpoint_test *test,
 	return false;
 }
 
+static bool pci_endpoint_test_doorbell(struct pci_endpoint_test *test)
+{
+	enum pci_barno bar;
+	u32 data, status;
+	u32 addr;
+
+	if (!test->support_db)
+		return false;
+
+	bar = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_DB_BAR);
+	if (bar == NO_BAR)
+		return false;
+
+	data = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_DB_DATA);
+	addr = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_DB_ADDR);
+	bar = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_DB_BAR);
+
+	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_IRQ_TYPE, irq_type);
+	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_IRQ_NUMBER, 1);
+
+	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_STATUS, 0);
+
+	writel(data, test->bar[bar] + addr);
+
+	wait_for_completion_timeout(&test->irq_raised, msecs_to_jiffies(1000));
+
+	status = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_STATUS);
+	if (status & STATUS_DOORBELL_SUCCESS)
+		return true;
+
+	return false;
+}
+
 static long pci_endpoint_test_ioctl(struct file *file, unsigned int cmd,
 				    unsigned long arg)
 {
@@ -762,6 +805,9 @@ static long pci_endpoint_test_ioctl(struct file *file, unsigned int cmd,
 	switch (cmd) {
 	case PCITEST_BAR:
 		bar = arg;
+		if (test->support_db &&
+		    bar == pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_DB_BAR))
+			goto ret;
 		if (bar > BAR_5)
 			goto ret;
 		if (is_am654_pci_dev(pdev) && bar == BAR_0)
@@ -793,6 +839,9 @@ static long pci_endpoint_test_ioctl(struct file *file, unsigned int cmd,
 	case PCITEST_CLEAR_IRQ:
 		ret = pci_endpoint_test_clear_irq(test);
 		break;
+	case PCITEST_DOORBELL:
+		ret = pci_endpoint_test_doorbell(test);
+		break;
 	}
 
 ret:
@@ -839,6 +888,7 @@ static int pci_endpoint_test_probe(struct pci_dev *pdev,
 		test_reg_bar = data->test_reg_bar;
 		test->test_reg_bar = test_reg_bar;
 		test->alignment = data->alignment;
+		test->support_db = data->support_db;
 		irq_type = data->irq_type;
 	}
 
@@ -986,6 +1036,13 @@ static const struct pci_endpoint_test_data default_data = {
 	.irq_type = IRQ_TYPE_MSI,
 };
 
+static const struct pci_endpoint_test_data default_data_db = {
+	.test_reg_bar = BAR_0,
+	.alignment = SZ_4K,
+	.irq_type = IRQ_TYPE_MSI,
+	.support_db = true,
+};
+
 static const struct pci_endpoint_test_data am654_data = {
 	.test_reg_bar = BAR_2,
 	.alignment = SZ_64K,
@@ -1017,6 +1074,9 @@ static const struct pci_device_id pci_endpoint_test_tbl[] = {
 	  .driver_data = (kernel_ulong_t)&default_data,
 	},
 	{ PCI_DEVICE(PCI_VENDOR_ID_FREESCALE, PCI_DEVICE_ID_IMX8),},
+	{ PCI_DEVICE(PCI_VENDOR_ID_FREESCALE, PCI_DEVICE_ID_IMX8_DB),
+	  .driver_data = (kernel_ulong_t)&default_data_db,
+	},
 	{ PCI_DEVICE(PCI_VENDOR_ID_FREESCALE, PCI_DEVICE_ID_LS1088A),
 	  .driver_data = (kernel_ulong_t)&default_data,
 	},
diff --git a/include/uapi/linux/pcitest.h b/include/uapi/linux/pcitest.h
index 94b46b043b536..06d9f548b510e 100644
--- a/include/uapi/linux/pcitest.h
+++ b/include/uapi/linux/pcitest.h
@@ -21,6 +21,7 @@
 #define PCITEST_SET_IRQTYPE	_IOW('P', 0x8, int)
 #define PCITEST_GET_IRQTYPE	_IO('P', 0x9)
 #define PCITEST_CLEAR_IRQ	_IO('P', 0x10)
+#define PCITEST_DOORBELL	_IO('P', 0x11)
 
 #define PCITEST_FLAGS_USE_DMA	0x00000001
 

-- 
2.34.1

[PATCH v3 6/6] tools: PCI: Add 'B' option for test doorbell

[Frank Li]

Add doorbell test support.

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
 tools/pci/pcitest.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/tools/pci/pcitest.c b/tools/pci/pcitest.c
index 470258009ddc2..bbe26ebbfd945 100644
--- a/tools/pci/pcitest.c
+++ b/tools/pci/pcitest.c
@@ -34,6 +34,7 @@ struct pci_test {
 	bool		copy;
 	unsigned long	size;
 	bool		use_dma;
+	bool		doorbell;
 };
 
 static int run_test(struct pci_test *test)
@@ -147,6 +148,15 @@ static int run_test(struct pci_test *test)
 			fprintf(stdout, "%s\n", result[ret]);
 	}
 
+	if (test->doorbell) {
+		ret = ioctl(fd, PCITEST_DOORBELL, 0);
+		fprintf(stdout, "Ringing doorbell on the EP\t\t");
+		if (ret < 0)
+			fprintf(stdout, "TEST FAILED\n");
+		else
+			fprintf(stdout, "%s\n", result[ret]);
+	}
+
 	fflush(stdout);
 	close(fd);
 	return (ret < 0) ? ret : 1 - ret; /* return 0 if test succeeded */
@@ -172,7 +182,7 @@ int main(int argc, char **argv)
 	/* set default endpoint device */
 	test->device = "/dev/pci-endpoint-test.0";
 
-	while ((c = getopt(argc, argv, "D:b:m:x:i:deIlhrwcs:")) != EOF)
+	while ((c = getopt(argc, argv, "D:b:m:x:i:BdeIlhrwcs:")) != EOF)
 	switch (c) {
 	case 'D':
 		test->device = optarg;
@@ -222,6 +232,9 @@ int main(int argc, char **argv)
 	case 'd':
 		test->use_dma = true;
 		continue;
+	case 'B':
+		test->doorbell = true;
+		continue;
 	case 'h':
 	default:
 usage:
@@ -241,6 +254,7 @@ int main(int argc, char **argv)
 			"\t-w			Write buffer test\n"
 			"\t-c			Copy buffer test\n"
 			"\t-s <size>		Size of buffer {default: 100KB}\n"
+			"\t-B			Doorbell test\n"
 			"\t-h			Print this help message\n",
 			argv[0]);
 		return -EINVAL;

-- 
2.34.1

Re: [PATCH v3 1/6] genirq/msi: Add cleanup guard define for msi_lock_descs()/msi_unlock_descs()

[Damien Le Moal]

On 10/16/24 7:07 AM, Frank Li wrote:
> Add a cleanup DEFINE_GUARD macro for msi_lock_descs() and
> msi_unlock_descs() to simplify lock and unlock operations in error path.
> 
> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---
>  include/linux/msi.h | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/include/linux/msi.h b/include/linux/msi.h
> index b10093c4d00ea..0b6cb7f303887 100644
> --- a/include/linux/msi.h
> +++ b/include/linux/msi.h
> @@ -228,6 +228,8 @@ int msi_setup_device_data(struct device *dev);
>  void msi_lock_descs(struct device *dev);
>  void msi_unlock_descs(struct device *dev);
>  
> +DEFINE_GUARD(msi_descs, struct device *, msi_lock_descs(_T), msi_unlock_descs(_T))
> +

This belongs with patch 3 since it is first used there.

>  struct msi_desc *msi_domain_first_desc(struct device *dev, unsigned int domid,
>  				       enum msi_desc_filter filter);

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v3 3/6] PCI: endpoint: Add RC-to-EP doorbell support using platform MSI controller

[Damien Le Moal]

On 10/16/24 7:07 AM, Frank Li wrote:
> Doorbell feature is implemented by mapping the EP's MSI interrupt
> controller message address to a dedicated BAR in the EPC core. It is the
> responsibility of the EPF driver to pass the actual message data to be
> written by the host to the doorbell BAR region through its own logic.
> 
> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---
>  drivers/pci/endpoint/Makefile     |   2 +-
>  drivers/pci/endpoint/pci-ep-msi.c | 162 ++++++++++++++++++++++++++++++++++++++
>  include/linux/pci-ep-msi.h        |  15 ++++
>  include/linux/pci-epf.h           |   6 ++
>  4 files changed, 184 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/endpoint/Makefile b/drivers/pci/endpoint/Makefile
> index 95b2fe47e3b06..a1ccce440c2c5 100644
> --- a/drivers/pci/endpoint/Makefile
> +++ b/drivers/pci/endpoint/Makefile
> @@ -5,4 +5,4 @@
>  
>  obj-$(CONFIG_PCI_ENDPOINT_CONFIGFS)	+= pci-ep-cfs.o
>  obj-$(CONFIG_PCI_ENDPOINT)		+= pci-epc-core.o pci-epf-core.o\
> -					   pci-epc-mem.o functions/
> +					   pci-epc-mem.o pci-ep-msi.o functions/
> diff --git a/drivers/pci/endpoint/pci-ep-msi.c b/drivers/pci/endpoint/pci-ep-msi.c
> new file mode 100644
> index 0000000000000..534dcd05c435c
> --- /dev/null
> +++ b/drivers/pci/endpoint/pci-ep-msi.c
> @@ -0,0 +1,162 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * PCI Endpoint *Controller* (EPC) MSI library
> + *
> + * Copyright (C) 2024 NXP
> + * Author: Frank Li <Frank.Li@nxp.com>
> + */
> +
> +#include <linux/cleanup.h>
> +#include <linux/device.h>
> +#include <linux/slab.h>
> +#include <linux/module.h>
> +

Stray blank line here.

> +#include <linux/msi.h>
> +#include <linux/pci-epc.h>
> +#include <linux/pci-epf.h>
> +#include <linux/pci-ep-cfs.h>
> +#include <linux/pci-ep-msi.h>
> +
> +static irqreturn_t pci_epf_doorbell_handler(int irq, void *data)
> +{
> +	struct pci_epf *epf = data;
> +	struct msi_desc *desc = irq_get_msi_desc(irq);
> +
> +	if (desc && epf->event_ops && epf->event_ops->doorbell)
> +		epf->event_ops->doorbell(epf, desc->msi_index);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static bool pci_epc_match_parent(struct device *dev, void *param)
> +{
> +	return dev->parent == param;
> +}
> +
> +static void pci_epc_write_msi_msg(struct msi_desc *desc, struct msi_msg *msg)
> +{
> +	struct pci_epc *epc __free(pci_epc_put) = NULL;
> +	struct pci_epf *epf;
> +
> +	epc = pci_epc_get_fn(pci_epc_match_parent, desc->dev);
> +

No need for the blank line here.

> +	if (!epc)
> +		return;
> +
> +	/* Only support one EPF for doorbell */
> +	epf = list_first_entry_or_null(&epc->pci_epf, struct pci_epf, list);
> +	if (!epf)
> +		return;
> +
> +	if (epf->msg && desc->msi_index < epf->num_db)

Change this to:

	if (epf && epf->msg && desc->msi_index < epf->num_db)

and then remove the "if(!epf) return;" above. Shorter code :)

> +		memcpy(epf->msg, msg, sizeof(*msg));
> +}
> +
> +static int pci_epc_alloc_doorbell(struct pci_epc *epc, u8 func_no, u8 vfunc_no, int num_db)
> +{
> +	struct msi_desc *desc, *failed_desc;
> +	struct pci_epf *epf;
> +	struct device *dev;
> +	int i = 0;
> +	int ret;
> +
> +	if (IS_ERR_OR_NULL(epc))
> +		return -EINVAL;

Is this really needed ?

> +
> +	/* Currently only support one func and one vfunc for doorbell */
> +	if (func_no || vfunc_no)
> +		return -EINVAL;
> +
> +	epf = list_first_entry_or_null(&epc->pci_epf, struct pci_epf, list);
> +	if (!epf)
> +		return -EINVAL;

Why do you need this ? epf is unused in this function...

> +
> +	dev = epc->dev.parent;
> +	ret = platform_device_msi_init_and_alloc_irqs(dev, num_db, pci_epc_write_msi_msg);
> +	if (ret) {
> +		dev_err(dev, "Failed to allocate MSI\n");
> +		return -ENOMEM;

		return ret;

> +	}
> +
> +	scoped_guard(msi_descs, dev)

I personnally really dislike this... This adds one level of identation and
hides code. Really ugly in my opinion. But that is only that, my opinion. I
will let the maintainer decide on this.

> +		msi_for_each_desc(desc, dev, MSI_DESC_ALL) {
> +			ret = request_irq(desc->irq, pci_epf_doorbell_handler, 0,
> +					  kasprintf(GFP_KERNEL, "pci-epc-doorbell%d", i++), epf);
> +			if (ret) {
> +				dev_err(dev, "Failed to request doorbell\n");
> +				failed_desc = desc;
> +				goto err_request_irq;
> +			}
> +		}
> +
> +	return 0;
> +
> +err_request_irq:
> +	scoped_guard(msi_descs, dev)
> +		msi_for_each_desc(desc, dev, MSI_DESC_ALL) {
> +			if (desc == failed_desc)
> +				break;
> +			kfree(free_irq(desc->irq, epf));

If request_irq() failed and you want to cleanup everything, I do not think you
need to track the failed_desc since free_irq() will return NULL if the irq was
not requested. That would simplify this code.

> +		}
> +
> +	platform_device_msi_free_irqs_all(epc->dev.parent);
> +
> +	return ret;
> +}
> +
> +static void pci_epc_free_doorbell(struct pci_epc *epc, u8 func_no, u8 vfunc_no)
> +{
> +	struct msi_desc *desc;
> +	struct pci_epf *epf;
> +	struct device *dev;
> +
> +	dev = epc->dev.parent;

Nit: move the affectation to the declaration line:

	struct device *dev = epc->dev.parent;

> +
> +	scoped_guard(msi_descs, dev)
> +		msi_for_each_desc(desc, dev, MSI_DESC_ALL)
> +			kfree(free_irq(desc->irq, epf));
> +
> +	platform_device_msi_free_irqs_all(epc->dev.parent);
> +}
> +
> +int pci_epf_alloc_doorbell(struct pci_epf *epf, u16 num_db)
> +{
> +	struct pci_epc *epc;
> +	struct device *dev;
> +	void *msg;
> +	int ret;
> +
> +	epc = epf->epc;
> +	dev = &epc->dev;

Same here: move this to the declaration lines.

> +
> +	guard(mutex)(&epc->lock);

Another code hiding trick... Having the calls to mutex_lock/unlock(&epc->lock)
would not complicate this code and makes things a lot more clear in my opinion...

But more importantly: you are taking the epc lock in a pci_epf_ function, which
is a little odd... Shouldn't this lock be taken in pci_epc_alloc_doorbell()
instead ?

> +
> +	msg = kcalloc(num_db, sizeof(struct msi_msg), GFP_KERNEL);
> +	if (!msg)
> +		return -ENOMEM;

You can do this allocation before taking the epc mutex lock.

> +
> +	epf->num_db = num_db;
> +	epf->msg = msg;
> +
> +	ret = pci_epc_alloc_doorbell(epc, epf->func_no, epf->vfunc_no, num_db);
> +	if (ret)
> +		kfree(msg);

Looking at this, it seems that pci_epc_alloc_doorbell() should allocate msg and
return a pointer to it (or ERR_PTR). This entire function would become:

	msg = pci_epc_alloc_doorbell(epc, epf->func_no, epf->vfunc_no, num_db);
	if (IS_ERR(msg))
		return PTR_ERR(msg);

	epf->num_db = num_db;
	epf->msg = msg;

	return 0;

> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(pci_epf_alloc_doorbell);
> +
> +void pci_epf_free_doorbell(struct pci_epf *epf)
> +{
> +	struct pci_epc *epc = epf->epc;
> +
> +	guard(mutex)(&epc->lock);

Same comment about the location of this lock. That should be in
pci_epc_free_doorbell() I think.

> +
> +	epc = epf->epc;

You did that at delaration time already. But this epc variable is used only
below so it is not really needed at all.

> +	pci_epc_free_doorbell(epc, epf->func_no, epf->vfunc_no);
> +
> +	kfree(epf->msg);

This also should be in pci_epc_free_doorbell. So something like:

	pci_epc_free_doorbell(epf->epc, epf->func_no, epf->vfunc_no, epf->msg);

to be consistent with the changed proposed above for the allloc function.

> +	epf->msg = NULL;
> +	epf->num_db = 0;
> +}
> +EXPORT_SYMBOL_GPL(pci_epf_free_doorbell);
> diff --git a/include/linux/pci-ep-msi.h b/include/linux/pci-ep-msi.h
> new file mode 100644
> index 0000000000000..f0cfecf491199
> --- /dev/null
> +++ b/include/linux/pci-ep-msi.h
> @@ -0,0 +1,15 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * PCI Endpoint *Function* side MSI header file
> + *
> + * Copyright (C) 2024 NXP
> + * Author: Frank Li <Frank.Li@nxp.com>
> + */
> +
> +#ifndef __PCI_EP_MSI__
> +#define __PCI_EP_MSI__
> +
> +int pci_epf_alloc_doorbell(struct pci_epf *epf, u16 nums);
> +void pci_epf_free_doorbell(struct pci_epf *epf);
> +
> +#endif /* __PCI_EP_MSI__ */

I do not see the point of this file. Why not add these function declarations to
include/linux/pci-epf.h to keep all EPF API together ?

> diff --git a/include/linux/pci-epf.h b/include/linux/pci-epf.h
> index 18a3aeb62ae4e..1e7e5eb4067d7 100644
> --- a/include/linux/pci-epf.h
> +++ b/include/linux/pci-epf.h
> @@ -75,6 +75,7 @@ struct pci_epf_ops {
>   * @link_up: Callback for the EPC link up event
>   * @link_down: Callback for the EPC link down event
>   * @bus_master_enable: Callback for the EPC Bus Master Enable event
> + * @doorbell: Callback for EPC receive MSI from RC side
>   */
>  struct pci_epc_event_ops {
>  	int (*epc_init)(struct pci_epf *epf);
> @@ -82,6 +83,7 @@ struct pci_epc_event_ops {
>  	int (*link_up)(struct pci_epf *epf);
>  	int (*link_down)(struct pci_epf *epf);
>  	int (*bus_master_enable)(struct pci_epf *epf);
> +	int (*doorbell)(struct pci_epf *epf, int index);
>  };
>  
>  /**
> @@ -152,6 +154,8 @@ struct pci_epf_bar {
>   * @vfunction_num_map: bitmap to manage virtual function number
>   * @pci_vepf: list of virtual endpoint functions associated with this function
>   * @event_ops: Callbacks for capturing the EPC events
> + * @msg: data for MSI from RC side
> + * @num_db: number of doorbells
>   */
>  struct pci_epf {
>  	struct device		dev;
> @@ -182,6 +186,8 @@ struct pci_epf {
>  	unsigned long		vfunction_num_map;
>  	struct list_head	pci_vepf;
>  	const struct pci_epc_event_ops *event_ops;
> +	struct msi_msg *msg;
> +	u16 num_db;
>  };
>  
>  /**
> 


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v3 3/6] PCI: endpoint: Add RC-to-EP doorbell support using platform MSI controller

[Frank Li]

On Wed, Oct 16, 2024 at 10:36:20AM +0900, Damien Le Moal wrote:
> On 10/16/24 7:07 AM, Frank Li wrote:
> > Doorbell feature is implemented by mapping the EP's MSI interrupt
> > controller message address to a dedicated BAR in the EPC core. It is the
> > responsibility of the EPF driver to pass the actual message data to be
> > written by the host to the doorbell BAR region through its own logic.
> >
> > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > ---
> >  drivers/pci/endpoint/Makefile     |   2 +-
> >  drivers/pci/endpoint/pci-ep-msi.c | 162 ++++++++++++++++++++++++++++++++++++++
> >  include/linux/pci-ep-msi.h        |  15 ++++
> >  include/linux/pci-epf.h           |   6 ++
> >  4 files changed, 184 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/pci/endpoint/Makefile b/drivers/pci/endpoint/Makefile
> > index 95b2fe47e3b06..a1ccce440c2c5 100644
> > --- a/drivers/pci/endpoint/Makefile
> > +++ b/drivers/pci/endpoint/Makefile
> > @@ -5,4 +5,4 @@
> >
> >  obj-$(CONFIG_PCI_ENDPOINT_CONFIGFS)	+= pci-ep-cfs.o
> >  obj-$(CONFIG_PCI_ENDPOINT)		+= pci-epc-core.o pci-epf-core.o\
> > -					   pci-epc-mem.o functions/
> > +					   pci-epc-mem.o pci-ep-msi.o functions/
> > diff --git a/drivers/pci/endpoint/pci-ep-msi.c b/drivers/pci/endpoint/pci-ep-msi.c
> > new file mode 100644
> > index 0000000000000..534dcd05c435c
> > --- /dev/null
> > +++ b/drivers/pci/endpoint/pci-ep-msi.c
> > @@ -0,0 +1,162 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * PCI Endpoint *Controller* (EPC) MSI library
> > + *
> > + * Copyright (C) 2024 NXP
> > + * Author: Frank Li <Frank.Li@nxp.com>
> > + */
> > +
> > +#include <linux/cleanup.h>
> > +#include <linux/device.h>
> > +#include <linux/slab.h>
> > +#include <linux/module.h>
> > +
>
> Stray blank line here.
>
> > +#include <linux/msi.h>
> > +#include <linux/pci-epc.h>
> > +#include <linux/pci-epf.h>
> > +#include <linux/pci-ep-cfs.h>
> > +#include <linux/pci-ep-msi.h>
> > +
> > +static irqreturn_t pci_epf_doorbell_handler(int irq, void *data)
> > +{
> > +	struct pci_epf *epf = data;
> > +	struct msi_desc *desc = irq_get_msi_desc(irq);
> > +
> > +	if (desc && epf->event_ops && epf->event_ops->doorbell)
> > +		epf->event_ops->doorbell(epf, desc->msi_index);
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +static bool pci_epc_match_parent(struct device *dev, void *param)
> > +{
> > +	return dev->parent == param;
> > +}
> > +
> > +static void pci_epc_write_msi_msg(struct msi_desc *desc, struct msi_msg *msg)
> > +{
> > +	struct pci_epc *epc __free(pci_epc_put) = NULL;
> > +	struct pci_epf *epf;
> > +
> > +	epc = pci_epc_get_fn(pci_epc_match_parent, desc->dev);
> > +
>
> No need for the blank line here.
>
> > +	if (!epc)
> > +		return;
> > +
> > +	/* Only support one EPF for doorbell */
> > +	epf = list_first_entry_or_null(&epc->pci_epf, struct pci_epf, list);
> > +	if (!epf)
> > +		return;
> > +
> > +	if (epf->msg && desc->msi_index < epf->num_db)
>
> Change this to:
>
> 	if (epf && epf->msg && desc->msi_index < epf->num_db)
>
> and then remove the "if(!epf) return;" above. Shorter code :)
>
> > +		memcpy(epf->msg, msg, sizeof(*msg));
> > +}
> > +
> > +static int pci_epc_alloc_doorbell(struct pci_epc *epc, u8 func_no, u8 vfunc_no, int num_db)
> > +{
> > +	struct msi_desc *desc, *failed_desc;
> > +	struct pci_epf *epf;
> > +	struct device *dev;
> > +	int i = 0;
> > +	int ret;
> > +
> > +	if (IS_ERR_OR_NULL(epc))
> > +		return -EINVAL;
>
> Is this really needed ?
>
> > +
> > +	/* Currently only support one func and one vfunc for doorbell */
> > +	if (func_no || vfunc_no)
> > +		return -EINVAL;
> > +
> > +	epf = list_first_entry_or_null(&epc->pci_epf, struct pci_epf, list);
> > +	if (!epf)
> > +		return -EINVAL;
>
> Why do you need this ? epf is unused in this function...

epf need at request_irq.

>
> > +
> > +	dev = epc->dev.parent;
> > +	ret = platform_device_msi_init_and_alloc_irqs(dev, num_db, pci_epc_write_msi_msg);
> > +	if (ret) {
> > +		dev_err(dev, "Failed to allocate MSI\n");
> > +		return -ENOMEM;
>
> 		return ret;
>
> > +	}
> > +
> > +	scoped_guard(msi_descs, dev)
>
> I personnally really dislike this... This adds one level of identation and
> hides code. Really ugly in my opinion. But that is only that, my opinion. I
> will let the maintainer decide on this.

I think it is better if treat 'scoped_guard' as key words 'if'. There are
'goto' at error branch, if no scope_guard, need unlock at two place, which
is quite easy to forget one at error path.

>
> > +		msi_for_each_desc(desc, dev, MSI_DESC_ALL) {
> > +			ret = request_irq(desc->irq, pci_epf_doorbell_handler, 0,
> > +					  kasprintf(GFP_KERNEL, "pci-epc-doorbell%d", i++), epf);
> > +			if (ret) {
> > +				dev_err(dev, "Failed to request doorbell\n");
> > +				failed_desc = desc;
> > +				goto err_request_irq;
> > +			}
> > +		}
> > +
> > +	return 0;
> > +
> > +err_request_irq:
> > +	scoped_guard(msi_descs, dev)
> > +		msi_for_each_desc(desc, dev, MSI_DESC_ALL) {
> > +			if (desc == failed_desc)
> > +				break;
> > +			kfree(free_irq(desc->irq, epf));
>
> If request_irq() failed and you want to cleanup everything, I do not think you
> need to track the failed_desc since free_irq() will return NULL if the irq was
> not requested. That would simplify this code.
>
> > +		}
> > +
> > +	platform_device_msi_free_irqs_all(epc->dev.parent);
> > +
> > +	return ret;
> > +}
> > +
> > +static void pci_epc_free_doorbell(struct pci_epc *epc, u8 func_no, u8 vfunc_no)
> > +{
> > +	struct msi_desc *desc;
> > +	struct pci_epf *epf;
> > +	struct device *dev;
> > +
> > +	dev = epc->dev.parent;
>
> Nit: move the affectation to the declaration line:
>
> 	struct device *dev = epc->dev.parent;
>
> > +
> > +	scoped_guard(msi_descs, dev)
> > +		msi_for_each_desc(desc, dev, MSI_DESC_ALL)
> > +			kfree(free_irq(desc->irq, epf));
> > +
> > +	platform_device_msi_free_irqs_all(epc->dev.parent);
> > +}
> > +
> > +int pci_epf_alloc_doorbell(struct pci_epf *epf, u16 num_db)
> > +{
> > +	struct pci_epc *epc;
> > +	struct device *dev;
> > +	void *msg;
> > +	int ret;
> > +
> > +	epc = epf->epc;
> > +	dev = &epc->dev;
>
> Same here: move this to the declaration lines.
>
> > +
> > +	guard(mutex)(&epc->lock);
>
> Another code hiding trick... Having the calls to mutex_lock/unlock(&epc->lock)
> would not complicate this code and makes things a lot more clear in my opinion...
>
> But more importantly: you are taking the epc lock in a pci_epf_ function, which
> is a little odd... Shouldn't this lock be taken in pci_epc_alloc_doorbell()
> instead ?

Yes, lock should be epc part.

>
> > +
> > +	msg = kcalloc(num_db, sizeof(struct msi_msg), GFP_KERNEL);
> > +	if (!msg)
> > +		return -ENOMEM;
>
> You can do this allocation before taking the epc mutex lock.
>
> > +
> > +	epf->num_db = num_db;
> > +	epf->msg = msg;
> > +
> > +	ret = pci_epc_alloc_doorbell(epc, epf->func_no, epf->vfunc_no, num_db);
> > +	if (ret)
> > +		kfree(msg);
>
> Looking at this, it seems that pci_epc_alloc_doorbell() should allocate msg and
> return a pointer to it (or ERR_PTR). This entire function would become:

There are callback "pci_epc_write_msi_msg()" need epf->msg. So need
allocate and init epf->msg before pci_epc_alloc_doorbell().

>
> 	msg = pci_epc_alloc_doorbell(epc, epf->func_no, epf->vfunc_no, num_db);
> 	if (IS_ERR(msg))
> 		return PTR_ERR(msg);
>
> 	epf->num_db = num_db;
> 	epf->msg = msg;
>
> 	return 0;
>
> > +
> > +	return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(pci_epf_alloc_doorbell);
> > +
> > +void pci_epf_free_doorbell(struct pci_epf *epf)
> > +{
> > +	struct pci_epc *epc = epf->epc;
> > +
> > +	guard(mutex)(&epc->lock);
>
> Same comment about the location of this lock. That should be in
> pci_epc_free_doorbell() I think.

Yes

>
> > +
> > +	epc = epf->epc;
>
> You did that at delaration time already. But this epc variable is used only
> below so it is not really needed at all.
>
> > +	pci_epc_free_doorbell(epc, epf->func_no, epf->vfunc_no);
> > +
> > +	kfree(epf->msg);
>
> This also should be in pci_epc_free_doorbell. So something like:

See above alloc problem.

Frank

>
> 	pci_epc_free_doorbell(epf->epc, epf->func_no, epf->vfunc_no, epf->msg);
>
> to be consistent with the changed proposed above for the allloc function.
>
> > +	epf->msg = NULL;
> > +	epf->num_db = 0;
> > +}
> > +EXPORT_SYMBOL_GPL(pci_epf_free_doorbell);
> > diff --git a/include/linux/pci-ep-msi.h b/include/linux/pci-ep-msi.h
> > new file mode 100644
> > index 0000000000000..f0cfecf491199
> > --- /dev/null
> > +++ b/include/linux/pci-ep-msi.h
> > @@ -0,0 +1,15 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * PCI Endpoint *Function* side MSI header file
> > + *
> > + * Copyright (C) 2024 NXP
> > + * Author: Frank Li <Frank.Li@nxp.com>
> > + */
> > +
> > +#ifndef __PCI_EP_MSI__
> > +#define __PCI_EP_MSI__
> > +
> > +int pci_epf_alloc_doorbell(struct pci_epf *epf, u16 nums);
> > +void pci_epf_free_doorbell(struct pci_epf *epf);
> > +
> > +#endif /* __PCI_EP_MSI__ */
>
> I do not see the point of this file. Why not add these function declarations to
> include/linux/pci-epf.h to keep all EPF API together ?

Mani prefer a seperate header file at v2 see
https://lore.kernel.org/imx/20231020181008.GF46191@thinkpad/

>
> > diff --git a/include/linux/pci-epf.h b/include/linux/pci-epf.h
> > index 18a3aeb62ae4e..1e7e5eb4067d7 100644
> > --- a/include/linux/pci-epf.h
> > +++ b/include/linux/pci-epf.h
> > @@ -75,6 +75,7 @@ struct pci_epf_ops {
> >   * @link_up: Callback for the EPC link up event
> >   * @link_down: Callback for the EPC link down event
> >   * @bus_master_enable: Callback for the EPC Bus Master Enable event
> > + * @doorbell: Callback for EPC receive MSI from RC side
> >   */
> >  struct pci_epc_event_ops {
> >  	int (*epc_init)(struct pci_epf *epf);
> > @@ -82,6 +83,7 @@ struct pci_epc_event_ops {
> >  	int (*link_up)(struct pci_epf *epf);
> >  	int (*link_down)(struct pci_epf *epf);
> >  	int (*bus_master_enable)(struct pci_epf *epf);
> > +	int (*doorbell)(struct pci_epf *epf, int index);
> >  };
> >
> >  /**
> > @@ -152,6 +154,8 @@ struct pci_epf_bar {
> >   * @vfunction_num_map: bitmap to manage virtual function number
> >   * @pci_vepf: list of virtual endpoint functions associated with this function
> >   * @event_ops: Callbacks for capturing the EPC events
> > + * @msg: data for MSI from RC side
> > + * @num_db: number of doorbells
> >   */
> >  struct pci_epf {
> >  	struct device		dev;
> > @@ -182,6 +186,8 @@ struct pci_epf {
> >  	unsigned long		vfunction_num_map;
> >  	struct list_head	pci_vepf;
> >  	const struct pci_epc_event_ops *event_ops;
> > +	struct msi_msg *msg;
> > +	u16 num_db;
> >  };
> >
> >  /**
> >
>
>
> --
> Damien Le Moal
> Western Digital Research

Re: [PATCH v3 1/6] genirq/msi: Add cleanup guard define for msi_lock_descs()/msi_unlock_descs()

[Thomas Gleixner]

On Tue, Oct 15 2024 at 18:07, Frank Li wrote:

> Add a cleanup DEFINE_GUARD macro for msi_lock_descs() and
> msi_unlock_descs() to simplify lock and unlock operations in error
> path.

What for?

Thanks,

        tglx

Re: [PATCH v3 3/6] PCI: endpoint: Add RC-to-EP doorbell support using platform MSI controller

[Thomas Gleixner]

On Tue, Oct 15 2024 at 18:07, Frank Li wrote:
> +static int pci_epc_alloc_doorbell(struct pci_epc *epc, u8 func_no, u8 vfunc_no, int num_db)
> +{
> +	struct msi_desc *desc, *failed_desc;
> +	struct pci_epf *epf;
> +	struct device *dev;
> +	int i = 0;
> +	int ret;
> +
> +	if (IS_ERR_OR_NULL(epc))
> +		return -EINVAL;
> +
> +	/* Currently only support one func and one vfunc for doorbell */
> +	if (func_no || vfunc_no)
> +		return -EINVAL;
> +
> +	epf = list_first_entry_or_null(&epc->pci_epf, struct pci_epf, list);
> +	if (!epf)
> +		return -EINVAL;
> +
> +	dev = epc->dev.parent;
> +	ret = platform_device_msi_init_and_alloc_irqs(dev, num_db, pci_epc_write_msi_msg);
> +	if (ret) {
> +		dev_err(dev, "Failed to allocate MSI\n");
> +		return -ENOMEM;
> +	}
> +
> +	scoped_guard(msi_descs, dev)
> +		msi_for_each_desc(desc, dev, MSI_DESC_ALL) {

That's just wrong. Nothing in this code has to fiddle with MSI
descriptors or the descriptor lock.

        for (i = 0; i < num_db; i++) {
            virq = msi_get_virq(dev, i);

> +			ret = request_irq(desc->irq, pci_epf_doorbell_handler, 0,
> +					  kasprintf(GFP_KERNEL, "pci-epc-doorbell%d", i++), epf);
> +			if (ret) {
> +				dev_err(dev, "Failed to request doorbell\n");
> +				failed_desc = desc;
> +				goto err_request_irq;
> +			}
> +		}
> +
> +	return 0;
> +
> +err_request_irq:
> +	scoped_guard(msi_descs, dev)
> +		msi_for_each_desc(desc, dev, MSI_DESC_ALL) {
> +			if (desc == failed_desc)
> +				break;
> +			kfree(free_irq(desc->irq, epf));

All instances of interrupts get 'epf' as device id. So if the third
instance failed to be requested, you free 'epf' when freeing the first
interrupt and then again when freeing the second one.

Thanks,

        tglx

Re: [PATCH v3 1/6] genirq/msi: Add cleanup guard define for msi_lock_descs()/msi_unlock_descs()

[Frank Li]

On Wed, Oct 16, 2024 at 06:21:50PM +0200, Thomas Gleixner wrote:
> On Tue, Oct 15 2024 at 18:07, Frank Li wrote:
>
> > Add a cleanup DEFINE_GUARD macro for msi_lock_descs() and
> > msi_unlock_descs() to simplify lock and unlock operations in error
> > path.
>
> What for?

See [PATCH v3 3/6] PCI: endpoint: Add RC-to-EP doorbell support using platform MSI controller

scoped_guard(msi_descs, dev)
	msi_for_each_desc(desc, dev, MSI_DESC_ALL) {
		...
		if (...)
			goto ...
	}

So cleanup can simplify unlock at error branch.

Frank

>
> Thanks,
>
>         tglx

Re: [PATCH v3 3/6] PCI: endpoint: Add RC-to-EP doorbell support using platform MSI controller

[Frank Li]

On Wed, Oct 16, 2024 at 06:30:40PM +0200, Thomas Gleixner wrote:
> On Tue, Oct 15 2024 at 18:07, Frank Li wrote:
> > +static int pci_epc_alloc_doorbell(struct pci_epc *epc, u8 func_no, u8 vfunc_no, int num_db)
> > +{
> > +	struct msi_desc *desc, *failed_desc;
> > +	struct pci_epf *epf;
> > +	struct device *dev;
> > +	int i = 0;
> > +	int ret;
> > +
> > +	if (IS_ERR_OR_NULL(epc))
> > +		return -EINVAL;
> > +
> > +	/* Currently only support one func and one vfunc for doorbell */
> > +	if (func_no || vfunc_no)
> > +		return -EINVAL;
> > +
> > +	epf = list_first_entry_or_null(&epc->pci_epf, struct pci_epf, list);
> > +	if (!epf)
> > +		return -EINVAL;
> > +
> > +	dev = epc->dev.parent;
> > +	ret = platform_device_msi_init_and_alloc_irqs(dev, num_db, pci_epc_write_msi_msg);
> > +	if (ret) {
> > +		dev_err(dev, "Failed to allocate MSI\n");
> > +		return -ENOMEM;
> > +	}
> > +
> > +	scoped_guard(msi_descs, dev)
> > +		msi_for_each_desc(desc, dev, MSI_DESC_ALL) {
>
> That's just wrong. Nothing in this code has to fiddle with MSI
> descriptors or the descriptor lock.
>
>         for (i = 0; i < num_db; i++) {
>             virq = msi_get_virq(dev, i);

Thanks, Change to msi_for_each_desc() is based on comments on
https://lore.kernel.org/imx/20231017183722.GB137137@thinkpad/

So my original implement is correct.

>
> > +			ret = request_irq(desc->irq, pci_epf_doorbell_handler, 0,
> > +					  kasprintf(GFP_KERNEL, "pci-epc-doorbell%d", i++), epf);
> > +			if (ret) {
> > +				dev_err(dev, "Failed to request doorbell\n");
> > +				failed_desc = desc;
> > +				goto err_request_irq;
> > +			}
> > +		}
> > +
> > +	return 0;
> > +
> > +err_request_irq:
> > +	scoped_guard(msi_descs, dev)
> > +		msi_for_each_desc(desc, dev, MSI_DESC_ALL) {
> > +			if (desc == failed_desc)
> > +				break;
> > +			kfree(free_irq(desc->irq, epf));
>
> All instances of interrupts get 'epf' as device id. So if the third
> instance failed to be requested, you free 'epf' when freeing the first
> interrupt and then again when freeing the second one.

Thanks, I actually want to free kasprintf() at request_irq(). I miss
understand the return value of free_irq().

Frank
>
> Thanks,
>
>         tglx

Re: [PATCH v3 3/6] PCI: endpoint: Add RC-to-EP doorbell support using platform MSI controller

[Thomas Gleixner]

On Wed, Oct 16 2024 at 12:54, Frank Li wrote:
> On Wed, Oct 16, 2024 at 06:30:40PM +0200, Thomas Gleixner wrote:
>> > +	scoped_guard(msi_descs, dev)
>> > +		msi_for_each_desc(desc, dev, MSI_DESC_ALL) {
>>
>> That's just wrong. Nothing in this code has to fiddle with MSI
>> descriptors or the descriptor lock.
>>
>>         for (i = 0; i < num_db; i++) {
>>             virq = msi_get_virq(dev, i);
>
> Thanks, Change to msi_for_each_desc() is based on comments on
> https://lore.kernel.org/imx/20231017183722.GB137137@thinkpad/
>
> So my original implement is correct.

Yes, very much so.

Thanks,

        tglx

Re: [PATCH v3 4/6] PCI: endpoint: pci-epf-test: Add doorbell test support

[Niklas Cassel]

Hello Frank,

On Tue, Oct 15, 2024 at 06:07:17PM -0400, Frank Li wrote:
> Add three registers: doorbell_bar, doorbell_addr, and doorbell_data,
> along with doorbell_done. Use pci_epf_alloc_doorbell() to allocate a
> doorbell address space.
> 
> Enable the Root Complex (RC) side driver to trigger pci-epc-test's doorbell
> callback handler by writing doorbell_data to the mapped doorbell_bar's
> address space.
> 
> Set doorbell_done in the doorbell callback to indicate completion.
> 
> To avoid broken compatibility, use new PID/VID and set RevID bigger than 0.
> So only new pcitest program can distinguish with/without doorbell support
> and avoid wrongly write test data to doorbell bar.
> 
> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---
>  drivers/pci/endpoint/functions/pci-epf-test.c | 58 ++++++++++++++++++++++++++-
>  1 file changed, 56 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> index 7c2ed6eae53ad..c054d621353a6 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> @@ -11,12 +11,14 @@
>  #include <linux/dmaengine.h>
>  #include <linux/io.h>
>  #include <linux/module.h>
> +#include <linux/msi.h>
>  #include <linux/slab.h>
>  #include <linux/pci_ids.h>
>  #include <linux/random.h>
>  
>  #include <linux/pci-epc.h>
>  #include <linux/pci-epf.h>
> +#include <linux/pci-ep-msi.h>
>  #include <linux/pci_regs.h>
>  
>  #define IRQ_TYPE_INTX			0
> @@ -39,6 +41,7 @@
>  #define STATUS_IRQ_RAISED		BIT(6)
>  #define STATUS_SRC_ADDR_INVALID		BIT(7)
>  #define STATUS_DST_ADDR_INVALID		BIT(8)
> +#define STATUS_DOORBELL_SUCCESS		BIT(9)
>  
>  #define FLAG_USE_DMA			BIT(0)
>  
> @@ -50,6 +53,7 @@ struct pci_epf_test {
>  	void			*reg[PCI_STD_NUM_BARS];
>  	struct pci_epf		*epf;
>  	enum pci_barno		test_reg_bar;
> +	enum pci_barno		doorbell_bar;
>  	size_t			msix_table_offset;
>  	struct delayed_work	cmd_handler;
>  	struct dma_chan		*dma_chan_tx;
> @@ -74,6 +78,9 @@ struct pci_epf_test_reg {
>  	u32	irq_type;
>  	u32	irq_number;
>  	u32	flags;
> +	u32	doorbell_bar;
> +	u32	doorbell_addr;
> +	u32	doorbell_data;
>  } __packed;
>  
>  static struct pci_epf_header test_header = {
> @@ -695,7 +702,7 @@ static int pci_epf_test_set_bar(struct pci_epf *epf)
>  	enum pci_barno test_reg_bar = epf_test->test_reg_bar;
>  
>  	for (bar = 0; bar < PCI_STD_NUM_BARS; bar++) {
> -		if (!epf_test->reg[bar])
> +		if (!epf_test->reg[bar] && bar != epf_test->doorbell_bar)
>  			continue;
>  
>  		ret = pci_epc_set_bar(epc, epf->func_no, epf->vfunc_no,
> @@ -810,11 +817,24 @@ static int pci_epf_test_link_down(struct pci_epf *epf)
>  	return 0;
>  }
>  
> +static int pci_epf_test_doorbell(struct pci_epf *epf, int index)
> +{
> +	struct pci_epf_test *epf_test = epf_get_drvdata(epf);
> +	enum pci_barno test_reg_bar = epf_test->test_reg_bar;
> +	struct pci_epf_test_reg *reg = epf_test->reg[test_reg_bar];
> +
> +	reg->status |= STATUS_DOORBELL_SUCCESS;
> +	pci_epf_test_raise_irq(epf_test, reg);
> +
> +	return 0;
> +}
> +
>  static const struct pci_epc_event_ops pci_epf_test_event_ops = {
>  	.epc_init = pci_epf_test_epc_init,
>  	.epc_deinit = pci_epf_test_epc_deinit,
>  	.link_up = pci_epf_test_link_up,
>  	.link_down = pci_epf_test_link_down,
> +	.doorbell = pci_epf_test_doorbell,
>  };
>  
>  static int pci_epf_test_alloc_space(struct pci_epf *epf)
> @@ -853,7 +873,7 @@ static int pci_epf_test_alloc_space(struct pci_epf *epf)
>  		if (bar == NO_BAR)
>  			break;
>  
> -		if (bar == test_reg_bar)
> +		if (bar == test_reg_bar || bar == epf_test->doorbell_bar)
>  			continue;
>  
>  		base = pci_epf_alloc_space(epf, bar_size[bar], bar,
> @@ -887,7 +907,11 @@ static int pci_epf_test_bind(struct pci_epf *epf)
>  	struct pci_epf_test *epf_test = epf_get_drvdata(epf);
>  	const struct pci_epc_features *epc_features;
>  	enum pci_barno test_reg_bar = BAR_0;
> +	enum pci_barno doorbell_bar = NO_BAR;
>  	struct pci_epc *epc = epf->epc;
> +	struct msi_msg *msg;
> +	u64 doorbell_addr;
> +	u32 align;
>  
>  	if (WARN_ON_ONCE(!epc))
>  		return -EINVAL;
> @@ -905,10 +929,40 @@ static int pci_epf_test_bind(struct pci_epf *epf)
>  	epf_test->test_reg_bar = test_reg_bar;
>  	epf_test->epc_features = epc_features;
>  
> +	align = epc_features->align;
> +	align = align ? align : 128;
> +
> +	/* Only revid >=1 support RC-to-EP Door bell */
> +	ret = epf->header->revid > 0 ?  pci_epf_alloc_doorbell(epf, 1) : -EINVAL;

I really, really don't like this idea.

This means that you would need to write a revid > 1 in configfs to test this.
I also don't think that it is right that pci-epf-test takes ownership of "rev".

How about something like this instead:

My thinking is that you add a doorbell_capable struct member to epc_features,
and then populate CAPS_DOORBELL_SUPPORT based on epc_features in
pci_epf_test_init_caps() (similar to how my proposal sets CAPS_MSI_SUPPORT).


From 0f6bb535c6d56e03e9b3550194deec04a1c1d370 Mon Sep 17 00:00:00 2001
From: Niklas Cassel <cassel@kernel.org>
Date: Fri, 18 Oct 2024 10:32:39 +0200
Subject: [PATCH] PCI: endpoint: pci-epf-test: Add support for exposing EPC
 capabilities

Currently, there is no way for the pci-endpoint-test driver (RC side),
to know which features the EPC supports.

Expose some of the EPC:s capabilities in the test_reg_bar, such that
the pci-endpoint-test driver can know if a feature (e.g. MSI-X or DMA)
is supported before attempting to test it.

Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/misc/pci_endpoint_test.c              | 34 +++++++++++++++
 drivers/pci/endpoint/functions/pci-epf-test.c | 43 +++++++++++++++++++
 2 files changed, 77 insertions(+)

diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
index 3aaaf47fa4ee..7eb045dc81b6 100644
--- a/drivers/misc/pci_endpoint_test.c
+++ b/drivers/misc/pci_endpoint_test.c
@@ -69,6 +69,20 @@
 #define PCI_ENDPOINT_TEST_FLAGS			0x2c
 #define FLAG_USE_DMA				BIT(0)
 
+#define CAPS_MAGIC				0x25ccf687
+#define PCI_ENDPOINT_TEST_CAPS_MAGIC		0x30
+#define PCI_ENDPOINT_TEST_CAPS_VERSION		0x34
+#define PCI_ENDPOINT_TEST_CAPS			0x38
+
+#define CAPS_MSI_SUPPORT		BIT(0)
+#define CAPS_MSIX_SUPPORT		BIT(1)
+#define CAPS_DMA_SUPPORT		BIT(2)
+#define CAPS_DMA_IS_PRIVATE		BIT(3) /* only valid if DMA_SUPPORT */
+#define CAPS_DOORBELL_SUPPORT		BIT(4)
+#define CAPS_DOORBELL_BAR_MASK		GENMASK(7, 5) /* only valid if DOORBELL_SUPPORT */
+#define CAPS_DOORBELL_BAR_SHIFT		5
+#define CAPS_DOORBELL_BAR(x)		(((x) & CAPS_DOORBELL_BAR_MASK) >> CAPS_DOORBELL_BAR_SHIFT)
+
 #define PCI_DEVICE_ID_TI_AM654			0xb00c
 #define PCI_DEVICE_ID_TI_J7200			0xb00f
 #define PCI_DEVICE_ID_TI_AM64			0xb010
@@ -805,6 +819,24 @@ static const struct file_operations pci_endpoint_test_fops = {
 	.unlocked_ioctl = pci_endpoint_test_ioctl,
 };
 
+static void pci_endpoint_get_caps(struct pci_endpoint_test *test)
+{
+	u32 caps_magic, caps;
+
+	/* check if endpoint has CAPS support */
+	caps_magic = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_CAPS_MAGIC);
+	if (caps_magic != CAPS_MAGIC)
+		return;
+
+	caps = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_CAPS);
+	pr_info("CAPS: MSI support: %u\n", (caps & CAPS_MSI_SUPPORT) ? 1 : 0);
+	pr_info("CAPS: MSI-X support: %u\n", (caps & CAPS_MSIX_SUPPORT) ? 1 : 0);
+	pr_info("CAPS: DMA support: %u\n", (caps & CAPS_DMA_SUPPORT) ? 1 : 0);
+	pr_info("CAPS: DMA is private: %u\n", (caps & CAPS_DMA_IS_PRIVATE) ? 1 : 0);
+	pr_info("CAPS: DOORBELL support: %u\n", (caps & CAPS_DOORBELL_SUPPORT) ? 1 : 0);
+	pr_info("CAPS: DOORBELL BAR: %lu\n", CAPS_DOORBELL_BAR(caps));
+}
+
 static int pci_endpoint_test_probe(struct pci_dev *pdev,
 				   const struct pci_device_id *ent)
 {
@@ -906,6 +938,8 @@ static int pci_endpoint_test_probe(struct pci_dev *pdev,
 		goto err_kfree_test_name;
 	}
 
+	pci_endpoint_get_caps(test);
+
 	misc_device = &test->miscdev;
 	misc_device->minor = MISC_DYNAMIC_MINOR;
 	misc_device->name = kstrdup(name, GFP_KERNEL);
diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
index a73bc0771d35..2dd90e2e8565 100644
--- a/drivers/pci/endpoint/functions/pci-epf-test.c
+++ b/drivers/pci/endpoint/functions/pci-epf-test.c
@@ -44,6 +44,18 @@
 
 #define TIMER_RESOLUTION		1
 
+#define CAPS_MAGIC			0x25ccf687
+#define CAPS_VERSION			0x1
+
+#define CAPS_MSI_SUPPORT		BIT(0)
+#define CAPS_MSIX_SUPPORT		BIT(1)
+#define CAPS_DMA_SUPPORT		BIT(2)
+#define CAPS_DMA_IS_PRIVATE		BIT(3) /* only valid if DMA_SUPPORT */
+#define CAPS_DOORBELL_SUPPORT		BIT(4)
+#define CAPS_DOORBELL_BAR_MASK		GENMASK(7, 5) /* only valid if DOORBELL_SUPPORT */
+#define CAPS_DOORBELL_BAR_SHIFT		5
+#define CAPS_DOORBELL_BAR(x)		(((x) & CAPS_DOORBELL_BAR_MASK) >> CAPS_DOORBELL_BAR_SHIFT)
+
 static struct workqueue_struct *kpcitest_workqueue;
 
 struct pci_epf_test {
@@ -74,6 +86,9 @@ struct pci_epf_test_reg {
 	u32	irq_type;
 	u32	irq_number;
 	u32	flags;
+	u32	caps_magic;
+	u32	caps_version;
+	u32	caps;
 } __packed;
 
 static struct pci_epf_header test_header = {
@@ -741,6 +756,32 @@ static void pci_epf_test_clear_bar(struct pci_epf *epf)
 	}
 }
 
+static void pci_epf_test_init_caps(struct pci_epf *epf)
+{
+	struct pci_epf_test *epf_test = epf_get_drvdata(epf);
+	const struct pci_epc_features *epc_features = epf_test->epc_features;
+	enum pci_barno test_reg_bar = epf_test->test_reg_bar;
+	struct pci_epf_test_reg *reg = epf_test->reg[test_reg_bar];
+	u32 caps = 0;
+
+	reg->caps_magic = cpu_to_le32(CAPS_MAGIC);
+	reg->caps_version = cpu_to_le32(CAPS_VERSION);
+
+	if (epc_features->msi_capable)
+		caps |= CAPS_MSI_SUPPORT;
+
+	if (epc_features->msix_capable)
+		caps |= CAPS_MSIX_SUPPORT;
+
+	if (epf_test->dma_supported)
+		caps |= CAPS_DMA_SUPPORT;
+
+	if (epf_test->dma_private)
+		caps |= CAPS_DMA_IS_PRIVATE;
+
+	reg->caps = cpu_to_le64(caps);
+}
+
 static int pci_epf_test_epc_init(struct pci_epf *epf)
 {
 	struct pci_epf_test *epf_test = epf_get_drvdata(epf);
@@ -765,6 +806,8 @@ static int pci_epf_test_epc_init(struct pci_epf *epf)
 		}
 	}
 
+	pci_epf_test_init_caps(epf);
+
 	ret = pci_epf_test_set_bar(epf);
 	if (ret)
 		return ret;
-- 
2.47.0

Re: [PATCH v3 4/6] PCI: endpoint: pci-epf-test: Add doorbell test support

[Niklas Cassel]

On Fri, Oct 18, 2024 at 04:01:05PM +0200, Niklas Cassel wrote:
> @@ -741,6 +756,32 @@ static void pci_epf_test_clear_bar(struct pci_epf *epf)
>  	}
>  }
>  
> +static void pci_epf_test_init_caps(struct pci_epf *epf)
> +{
> +	struct pci_epf_test *epf_test = epf_get_drvdata(epf);
> +	const struct pci_epc_features *epc_features = epf_test->epc_features;
> +	enum pci_barno test_reg_bar = epf_test->test_reg_bar;
> +	struct pci_epf_test_reg *reg = epf_test->reg[test_reg_bar];
> +	u32 caps = 0;
> +
> +	reg->caps_magic = cpu_to_le32(CAPS_MAGIC);
> +	reg->caps_version = cpu_to_le32(CAPS_VERSION);
> +
> +	if (epc_features->msi_capable)
> +		caps |= CAPS_MSI_SUPPORT;
> +
> +	if (epc_features->msix_capable)
> +		caps |= CAPS_MSIX_SUPPORT;
> +
> +	if (epf_test->dma_supported)
> +		caps |= CAPS_DMA_SUPPORT;
> +
> +	if (epf_test->dma_private)
> +		caps |= CAPS_DMA_IS_PRIVATE;
> +
> +	reg->caps = cpu_to_le64(caps);

opps, this should have been cpu_to_le32(caps);


Kind regards,
Niklas

Re: [PATCH v3 4/6] PCI: endpoint: pci-epf-test: Add doorbell test support

[Frank Li]

On Fri, Oct 18, 2024 at 04:01:05PM +0200, Niklas Cassel wrote:
> Hello Frank,
>
> On Tue, Oct 15, 2024 at 06:07:17PM -0400, Frank Li wrote:
> > Add three registers: doorbell_bar, doorbell_addr, and doorbell_data,
> > along with doorbell_done. Use pci_epf_alloc_doorbell() to allocate a
> > doorbell address space.
> >
> > Enable the Root Complex (RC) side driver to trigger pci-epc-test's doorbell
> > callback handler by writing doorbell_data to the mapped doorbell_bar's
> > address space.
> >
> > Set doorbell_done in the doorbell callback to indicate completion.
> >
> > To avoid broken compatibility, use new PID/VID and set RevID bigger than 0.
> > So only new pcitest program can distinguish with/without doorbell support
> > and avoid wrongly write test data to doorbell bar.
> >
> > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > ---
> >  drivers/pci/endpoint/functions/pci-epf-test.c | 58 ++++++++++++++++++++++++++-
> >  1 file changed, 56 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> > index 7c2ed6eae53ad..c054d621353a6 100644
> > --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> > +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> > @@ -11,12 +11,14 @@
> >  #include <linux/dmaengine.h>
> >  #include <linux/io.h>
> >  #include <linux/module.h>
> > +#include <linux/msi.h>
> >  #include <linux/slab.h>
> >  #include <linux/pci_ids.h>
> >  #include <linux/random.h>
> >
> >  #include <linux/pci-epc.h>
> >  #include <linux/pci-epf.h>
> > +#include <linux/pci-ep-msi.h>
> >  #include <linux/pci_regs.h>
> >
> >  #define IRQ_TYPE_INTX			0
> > @@ -39,6 +41,7 @@
> >  #define STATUS_IRQ_RAISED		BIT(6)
> >  #define STATUS_SRC_ADDR_INVALID		BIT(7)
> >  #define STATUS_DST_ADDR_INVALID		BIT(8)
> > +#define STATUS_DOORBELL_SUCCESS		BIT(9)
> >
> >  #define FLAG_USE_DMA			BIT(0)
> >
> > @@ -50,6 +53,7 @@ struct pci_epf_test {
> >  	void			*reg[PCI_STD_NUM_BARS];
> >  	struct pci_epf		*epf;
> >  	enum pci_barno		test_reg_bar;
> > +	enum pci_barno		doorbell_bar;
> >  	size_t			msix_table_offset;
> >  	struct delayed_work	cmd_handler;
> >  	struct dma_chan		*dma_chan_tx;
> > @@ -74,6 +78,9 @@ struct pci_epf_test_reg {
> >  	u32	irq_type;
> >  	u32	irq_number;
> >  	u32	flags;
> > +	u32	doorbell_bar;
> > +	u32	doorbell_addr;
> > +	u32	doorbell_data;
> >  } __packed;
> >
> >  static struct pci_epf_header test_header = {
> > @@ -695,7 +702,7 @@ static int pci_epf_test_set_bar(struct pci_epf *epf)
> >  	enum pci_barno test_reg_bar = epf_test->test_reg_bar;
> >
> >  	for (bar = 0; bar < PCI_STD_NUM_BARS; bar++) {
> > -		if (!epf_test->reg[bar])
> > +		if (!epf_test->reg[bar] && bar != epf_test->doorbell_bar)
> >  			continue;
> >
> >  		ret = pci_epc_set_bar(epc, epf->func_no, epf->vfunc_no,
> > @@ -810,11 +817,24 @@ static int pci_epf_test_link_down(struct pci_epf *epf)
> >  	return 0;
> >  }
> >
> > +static int pci_epf_test_doorbell(struct pci_epf *epf, int index)
> > +{
> > +	struct pci_epf_test *epf_test = epf_get_drvdata(epf);
> > +	enum pci_barno test_reg_bar = epf_test->test_reg_bar;
> > +	struct pci_epf_test_reg *reg = epf_test->reg[test_reg_bar];
> > +
> > +	reg->status |= STATUS_DOORBELL_SUCCESS;
> > +	pci_epf_test_raise_irq(epf_test, reg);
> > +
> > +	return 0;
> > +}
> > +
> >  static const struct pci_epc_event_ops pci_epf_test_event_ops = {
> >  	.epc_init = pci_epf_test_epc_init,
> >  	.epc_deinit = pci_epf_test_epc_deinit,
> >  	.link_up = pci_epf_test_link_up,
> >  	.link_down = pci_epf_test_link_down,
> > +	.doorbell = pci_epf_test_doorbell,
> >  };
> >
> >  static int pci_epf_test_alloc_space(struct pci_epf *epf)
> > @@ -853,7 +873,7 @@ static int pci_epf_test_alloc_space(struct pci_epf *epf)
> >  		if (bar == NO_BAR)
> >  			break;
> >
> > -		if (bar == test_reg_bar)
> > +		if (bar == test_reg_bar || bar == epf_test->doorbell_bar)
> >  			continue;
> >
> >  		base = pci_epf_alloc_space(epf, bar_size[bar], bar,
> > @@ -887,7 +907,11 @@ static int pci_epf_test_bind(struct pci_epf *epf)
> >  	struct pci_epf_test *epf_test = epf_get_drvdata(epf);
> >  	const struct pci_epc_features *epc_features;
> >  	enum pci_barno test_reg_bar = BAR_0;
> > +	enum pci_barno doorbell_bar = NO_BAR;
> >  	struct pci_epc *epc = epf->epc;
> > +	struct msi_msg *msg;
> > +	u64 doorbell_addr;
> > +	u32 align;
> >
> >  	if (WARN_ON_ONCE(!epc))
> >  		return -EINVAL;
> > @@ -905,10 +929,40 @@ static int pci_epf_test_bind(struct pci_epf *epf)
> >  	epf_test->test_reg_bar = test_reg_bar;
> >  	epf_test->epc_features = epc_features;
> >
> > +	align = epc_features->align;
> > +	align = align ? align : 128;
> > +
> > +	/* Only revid >=1 support RC-to-EP Door bell */
> > +	ret = epf->header->revid > 0 ?  pci_epf_alloc_doorbell(epf, 1) : -EINVAL;
>
> I really, really don't like this idea.
>
> This means that you would need to write a revid > 1 in configfs to test this.
> I also don't think that it is right that pci-epf-test takes ownership of "rev".
>
> How about something like this instead:
>
> My thinking is that you add a doorbell_capable struct member to epc_features,
> and then populate CAPS_DOORBELL_SUPPORT based on epc_features in
> pci_epf_test_init_caps() (similar to how my proposal sets CAPS_MSI_SUPPORT).

The primary issue is that the doorbell is not a capability of the EPC
itself; rather, it's a capability of the entire system that requires an
external MSI/ITS controller. The CAPS_DOORBELL_SUPPORT should handle this
feature. Even we needn't CAPS_DOORBELL_SUPPORT, just call
pci_epf_alloc_doorbell(), if error return, means not support DOORBELL.

One potential problem is that if the EPC supports CAPS_DOORBELL_SUPPORT,
but the user continues to use older PID/VID values to enable EPF testing,
the pcitest tool may treat the doorbell BAR as a normal BAR. This could
lead to confusion for users as to why their system breaks after a kernel
update.

To use the doorbell functionality, the revid can clearly inform users that
this feature breaks previous compatibility. Users will need to update the
host-side driver, PID/VID values, and the pcitest tools accordingly.

Frank

>
>
> From 0f6bb535c6d56e03e9b3550194deec04a1c1d370 Mon Sep 17 00:00:00 2001
> From: Niklas Cassel <cassel@kernel.org>
> Date: Fri, 18 Oct 2024 10:32:39 +0200
> Subject: [PATCH] PCI: endpoint: pci-epf-test: Add support for exposing EPC
>  capabilities
>
> Currently, there is no way for the pci-endpoint-test driver (RC side),
> to know which features the EPC supports.
>
> Expose some of the EPC:s capabilities in the test_reg_bar, such that
> the pci-endpoint-test driver can know if a feature (e.g. MSI-X or DMA)
> is supported before attempting to test it.
>
> Signed-off-by: Niklas Cassel <cassel@kernel.org>
> ---
>  drivers/misc/pci_endpoint_test.c              | 34 +++++++++++++++
>  drivers/pci/endpoint/functions/pci-epf-test.c | 43 +++++++++++++++++++
>  2 files changed, 77 insertions(+)
>
> diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
> index 3aaaf47fa4ee..7eb045dc81b6 100644
> --- a/drivers/misc/pci_endpoint_test.c
> +++ b/drivers/misc/pci_endpoint_test.c
> @@ -69,6 +69,20 @@
>  #define PCI_ENDPOINT_TEST_FLAGS			0x2c
>  #define FLAG_USE_DMA				BIT(0)
>
> +#define CAPS_MAGIC				0x25ccf687
> +#define PCI_ENDPOINT_TEST_CAPS_MAGIC		0x30
> +#define PCI_ENDPOINT_TEST_CAPS_VERSION		0x34
> +#define PCI_ENDPOINT_TEST_CAPS			0x38
> +
> +#define CAPS_MSI_SUPPORT		BIT(0)
> +#define CAPS_MSIX_SUPPORT		BIT(1)
> +#define CAPS_DMA_SUPPORT		BIT(2)
> +#define CAPS_DMA_IS_PRIVATE		BIT(3) /* only valid if DMA_SUPPORT */
> +#define CAPS_DOORBELL_SUPPORT		BIT(4)
> +#define CAPS_DOORBELL_BAR_MASK		GENMASK(7, 5) /* only valid if DOORBELL_SUPPORT */
> +#define CAPS_DOORBELL_BAR_SHIFT		5
> +#define CAPS_DOORBELL_BAR(x)		(((x) & CAPS_DOORBELL_BAR_MASK) >> CAPS_DOORBELL_BAR_SHIFT)
> +
>  #define PCI_DEVICE_ID_TI_AM654			0xb00c
>  #define PCI_DEVICE_ID_TI_J7200			0xb00f
>  #define PCI_DEVICE_ID_TI_AM64			0xb010
> @@ -805,6 +819,24 @@ static const struct file_operations pci_endpoint_test_fops = {
>  	.unlocked_ioctl = pci_endpoint_test_ioctl,
>  };
>
> +static void pci_endpoint_get_caps(struct pci_endpoint_test *test)
> +{
> +	u32 caps_magic, caps;
> +
> +	/* check if endpoint has CAPS support */
> +	caps_magic = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_CAPS_MAGIC);
> +	if (caps_magic != CAPS_MAGIC)
> +		return;
> +
> +	caps = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_CAPS);
> +	pr_info("CAPS: MSI support: %u\n", (caps & CAPS_MSI_SUPPORT) ? 1 : 0);
> +	pr_info("CAPS: MSI-X support: %u\n", (caps & CAPS_MSIX_SUPPORT) ? 1 : 0);
> +	pr_info("CAPS: DMA support: %u\n", (caps & CAPS_DMA_SUPPORT) ? 1 : 0);
> +	pr_info("CAPS: DMA is private: %u\n", (caps & CAPS_DMA_IS_PRIVATE) ? 1 : 0);
> +	pr_info("CAPS: DOORBELL support: %u\n", (caps & CAPS_DOORBELL_SUPPORT) ? 1 : 0);
> +	pr_info("CAPS: DOORBELL BAR: %lu\n", CAPS_DOORBELL_BAR(caps));
> +}
> +
>  static int pci_endpoint_test_probe(struct pci_dev *pdev,
>  				   const struct pci_device_id *ent)
>  {
> @@ -906,6 +938,8 @@ static int pci_endpoint_test_probe(struct pci_dev *pdev,
>  		goto err_kfree_test_name;
>  	}
>
> +	pci_endpoint_get_caps(test);
> +
>  	misc_device = &test->miscdev;
>  	misc_device->minor = MISC_DYNAMIC_MINOR;
>  	misc_device->name = kstrdup(name, GFP_KERNEL);
> diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> index a73bc0771d35..2dd90e2e8565 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> @@ -44,6 +44,18 @@
>
>  #define TIMER_RESOLUTION		1
>
> +#define CAPS_MAGIC			0x25ccf687
> +#define CAPS_VERSION			0x1
> +
> +#define CAPS_MSI_SUPPORT		BIT(0)
> +#define CAPS_MSIX_SUPPORT		BIT(1)
> +#define CAPS_DMA_SUPPORT		BIT(2)
> +#define CAPS_DMA_IS_PRIVATE		BIT(3) /* only valid if DMA_SUPPORT */
> +#define CAPS_DOORBELL_SUPPORT		BIT(4)
> +#define CAPS_DOORBELL_BAR_MASK		GENMASK(7, 5) /* only valid if DOORBELL_SUPPORT */
> +#define CAPS_DOORBELL_BAR_SHIFT		5
> +#define CAPS_DOORBELL_BAR(x)		(((x) & CAPS_DOORBELL_BAR_MASK) >> CAPS_DOORBELL_BAR_SHIFT)
> +
>  static struct workqueue_struct *kpcitest_workqueue;
>
>  struct pci_epf_test {
> @@ -74,6 +86,9 @@ struct pci_epf_test_reg {
>  	u32	irq_type;
>  	u32	irq_number;
>  	u32	flags;
> +	u32	caps_magic;
> +	u32	caps_version;
> +	u32	caps;
>  } __packed;
>
>  static struct pci_epf_header test_header = {
> @@ -741,6 +756,32 @@ static void pci_epf_test_clear_bar(struct pci_epf *epf)
>  	}
>  }
>
> +static void pci_epf_test_init_caps(struct pci_epf *epf)
> +{
> +	struct pci_epf_test *epf_test = epf_get_drvdata(epf);
> +	const struct pci_epc_features *epc_features = epf_test->epc_features;
> +	enum pci_barno test_reg_bar = epf_test->test_reg_bar;
> +	struct pci_epf_test_reg *reg = epf_test->reg[test_reg_bar];
> +	u32 caps = 0;
> +
> +	reg->caps_magic = cpu_to_le32(CAPS_MAGIC);
> +	reg->caps_version = cpu_to_le32(CAPS_VERSION);
> +
> +	if (epc_features->msi_capable)
> +		caps |= CAPS_MSI_SUPPORT;
> +
> +	if (epc_features->msix_capable)
> +		caps |= CAPS_MSIX_SUPPORT;
> +
> +	if (epf_test->dma_supported)
> +		caps |= CAPS_DMA_SUPPORT;
> +
> +	if (epf_test->dma_private)
> +		caps |= CAPS_DMA_IS_PRIVATE;
> +
> +	reg->caps = cpu_to_le64(caps);
> +}
> +
>  static int pci_epf_test_epc_init(struct pci_epf *epf)
>  {
>  	struct pci_epf_test *epf_test = epf_get_drvdata(epf);
> @@ -765,6 +806,8 @@ static int pci_epf_test_epc_init(struct pci_epf *epf)
>  		}
>  	}
>
> +	pci_epf_test_init_caps(epf);
> +
>  	ret = pci_epf_test_set_bar(epf);
>  	if (ret)
>  		return ret;
> --
> 2.47.0
>

Re: [PATCH v3 4/6] PCI: endpoint: pci-epf-test: Add doorbell test support

[Niklas Cassel]

On Fri, Oct 18, 2024 at 11:13:34AM -0400, Frank Li wrote:
> On Fri, Oct 18, 2024 at 04:01:05PM +0200, Niklas Cassel wrote:
> > > +	/* Only revid >=1 support RC-to-EP Door bell */
> > > +	ret = epf->header->revid > 0 ?  pci_epf_alloc_doorbell(epf, 1) : -EINVAL;
> >
> > I really, really don't like this idea.
> >
> > This means that you would need to write a revid > 1 in configfs to test this.
> > I also don't think that it is right that pci-epf-test takes ownership of "rev".
> >
> > How about something like this instead:
> >
> > My thinking is that you add a doorbell_capable struct member to epc_features,
> > and then populate CAPS_DOORBELL_SUPPORT based on epc_features in
> > pci_epf_test_init_caps() (similar to how my proposal sets CAPS_MSI_SUPPORT).
> 
> The primary issue is that the doorbell is not a capability of the EPC
> itself; rather, it's a capability of the entire system that requires an
> external MSI/ITS controller. The CAPS_DOORBELL_SUPPORT should handle this
> feature. Even we needn't CAPS_DOORBELL_SUPPORT, just call
> pci_epf_alloc_doorbell(), if error return, means not support DOORBELL.

Well, the idea is that CAPS_DOORBELL_SUPPORT bit is to tell the host side
driver (pci-endpoint-test.c) that the EPF supports doorbell.

In other words, if pcitest -B is executed, but the EP (pci-epf-test) does
not set the CAPS_DOORBELL_SUPPORT bit to one in the CAPS register,
the host side driver (pci-endpoint-test.c) can error out immediately,
no need to even trying to send any command to the EP.

(We can do the same with MSI and MSI-X, no need to send a command to the EP
if the EP has CAPS (CAPS_MAGIC is set), but does not indicate support for
MSI/MSI-X.)


> To use the doorbell functionality, the revid can clearly inform users that
> this feature breaks previous compatibility. Users will need to update the
> host-side driver, PID/VID values, and the pcitest tools accordingly.

I still really don't like the revid idea.


> One potential problem is that if the EPC supports CAPS_DOORBELL_SUPPORT,
> but the user continues to use older PID/VID values to enable EPF testing,
> the pcitest tool may treat the doorbell BAR as a normal BAR. This could
> lead to confusion for users as to why their system breaks after a kernel
> update.

How about we add a new pcitest --set-doorbell-mode option
(that is similar to pcitest -i which sets the interrupt mode to use).

That way, we can do:
./pcitest --set-doorbell-mode 1
(This will enable doorbell for e.g. BAR0, pci-epf-test will call
pci_epf_alloc_doorbell() when receiving this command from the RC side.
The command will return failure if pci_epf_alloc_doorbell() returned failure.)

./pcitest -B
(This will perform the doorbell test)

./pcitest --set-doorbell-mode 0
(This will disable the doorbell for BAR0,
so it will again not trigger IRQs when BAR0 is written,
and pcitest's tests to read/write the BARs will again behave as expected.)

(We probably also need another option pcitest --get-doorbell-mode.)

I think this should solve all your concerns. Thoughts?


Kind regards,
Niklas

Re: [PATCH v3 4/6] PCI: endpoint: pci-epf-test: Add doorbell test support

[Niklas Cassel]

On Sun, Oct 20, 2024 at 04:41:25PM +0200, Niklas Cassel wrote:
> On Fri, Oct 18, 2024 at 11:13:34AM -0400, Frank Li wrote:
> > On Fri, Oct 18, 2024 at 04:01:05PM +0200, Niklas Cassel wrote:
> 
> How about we add a new pcitest --set-doorbell-mode option
> (that is similar to pcitest -i which sets the interrupt mode to use).
> 
> That way, we can do:
> ./pcitest --set-doorbell-mode 1
> (This will enable doorbell for e.g. BAR0, pci-epf-test will call
> pci_epf_alloc_doorbell() when receiving this command from the RC side.
> The command will return failure if pci_epf_alloc_doorbell() returned failure.)
> 
> ./pcitest -B
> (This will perform the doorbell test)
> 
> ./pcitest --set-doorbell-mode 0
> (This will disable the doorbell for BAR0,
> so it will again not trigger IRQs when BAR0 is written,
> and pcitest's tests to read/write the BARs will again behave as expected.)
> 
> (We probably also need another option pcitest --get-doorbell-mode.)
> 
> I think this should solve all your concerns. Thoughts?

And just to clarify, if we go with the --set-doorbell-mode approach,
then my previous idea (introducing capabilities in pci-epf-test and
pci-endpoint-test) is no longer a necessity.


Kind regards,
Niklas

Re: [PATCH v3 4/6] PCI: endpoint: pci-epf-test: Add doorbell test support

[Frank Li]

On Mon, Oct 21, 2024 at 08:55:38AM +0200, Niklas Cassel wrote:
> On Sun, Oct 20, 2024 at 04:41:25PM +0200, Niklas Cassel wrote:
> > On Fri, Oct 18, 2024 at 11:13:34AM -0400, Frank Li wrote:
> > > On Fri, Oct 18, 2024 at 04:01:05PM +0200, Niklas Cassel wrote:
> >
> > How about we add a new pcitest --set-doorbell-mode option
> > (that is similar to pcitest -i which sets the interrupt mode to use).
> >
> > That way, we can do:
> > ./pcitest --set-doorbell-mode 1
> > (This will enable doorbell for e.g. BAR0, pci-epf-test will call
> > pci_epf_alloc_doorbell() when receiving this command from the RC side.
> > The command will return failure if pci_epf_alloc_doorbell() returned failure.)
> >
> > ./pcitest -B
> > (This will perform the doorbell test)
> >
> > ./pcitest --set-doorbell-mode 0
> > (This will disable the doorbell for BAR0,
> > so it will again not trigger IRQs when BAR0 is written,
> > and pcitest's tests to read/write the BARs will again behave as expected.)
> >
> > (We probably also need another option pcitest --get-doorbell-mode.)
> >
> > I think this should solve all your concerns. Thoughts?
>
> And just to clarify, if we go with the --set-doorbell-mode approach,
> then my previous idea (introducing capabilities in pci-epf-test and
> pci-endpoint-test) is no longer a necessity.

Yes, the problem is that it needs dynamatic switch bar mapping address.
I am not sure all EPC support it. DWC should support it because I did it
for vntb driver. But bar's size should be issue. PCI don't support change
bar's size after pci bus scan devices. ITS's offset is 0x40. Anyway,
ITS + DWC should work.

Frank

>
>
> Kind regards,
> Niklas