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

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

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

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

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

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