[v2,1/5] PCI: endpoint: Add RC-to-EP doorbell support using platform MSI controller

This commit introduces a common method for sending messages from the Root
Complex (RC) to the Endpoint (EP) by utilizing the platform MSI interrupt
controller, such as ARM GIC, as an EP doorbell. Maps the memory assigned
for the BAR region by the PCI host to the message address of the platform
MSI interrupt controller in the PCI EP. As a result, when the PCI RC writes
to the BAR region, it triggers an IRQ at the EP. This implementation serves
as a common method for all endpoint function drivers.

However, it currently supports only one EP physical function due to
limitations in ARM MSI/IMS readiness.

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
 drivers/pci/endpoint/pci-epc-core.c | 192 ++++++++++++++++++++++++++++
 drivers/pci/endpoint/pci-epf-core.c |  44 +++++++
 include/linux/pci-epc.h             |   6 +
 include/linux/pci-epf.h             |   7 +
 4 files changed, 249 insertions(+)

Hi Frank,

On 9/12/2023 3:39 AM, Frank Li wrote:
> This commit introduces a common method for sending messages from the Root
> Complex (RC) to the Endpoint (EP) by utilizing the platform MSI interrupt
> controller, such as ARM GIC, as an EP doorbell. Maps the memory assigned
> for the BAR region by the PCI host to the message address of the platform
> MSI interrupt controller in the PCI EP. As a result, when the PCI RC writes
> to the BAR region, it triggers an IRQ at the EP. This implementation serves
> as a common method for all endpoint function drivers.

This would help avoid the polling used in current EPF drivers. Thanks!
> 
> However, it currently supports only one EP physical function due to
> limitations in ARM MSI/IMS readiness.

Any such platform or architecture restrictions should not be handled in 
the endpoint core layer.
> 
> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---
>   drivers/pci/endpoint/pci-epc-core.c | 192 ++++++++++++++++++++++++++++
>   drivers/pci/endpoint/pci-epf-core.c |  44 +++++++
>   include/linux/pci-epc.h             |   6 +
>   include/linux/pci-epf.h             |   7 +
>   4 files changed, 249 insertions(+)
> 
> diff --git a/drivers/pci/endpoint/pci-epc-core.c b/drivers/pci/endpoint/pci-epc-core.c
> index 5a4a8b0be6262..d336a99c6a94f 100644
> --- a/drivers/pci/endpoint/pci-epc-core.c
> +++ b/drivers/pci/endpoint/pci-epc-core.c
> @@ -10,6 +10,7 @@
>   #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>
> @@ -783,6 +784,197 @@ void pci_epc_bme_notify(struct pci_epc *epc)
>   }
>   EXPORT_SYMBOL_GPL(pci_epc_bme_notify);
>   
> +/**
> + * pci_epc_alloc_doorbell() - alloc an address space to let RC trigger EP side IRQ by write data to
> + *			      the space.
> + *
> + * @epc: the EPC device that need doorbell address and data from RC.
> + * @func_no: the physical endpoint function number in the EPC device.
> + * @vfunc_no: the virtual endpoint function number in the physical function.
> + * @num_msgs: the total number of doorbell messages
> + *
> + * Return: 0 success, other is failure
> + */
> +int pci_epc_alloc_doorbell(struct pci_epc *epc, u8 func_no, u8 vfunc_no, int num_msgs)
> +{
> +	int ret;
> +
> +	if (IS_ERR_OR_NULL(epc) || func_no >= epc->max_functions)
> +		return -EINVAL;
> +
> +	if (vfunc_no > 0 && (!epc->max_vfs || vfunc_no > epc->max_vfs[func_no]))
> +		return -EINVAL;
> +
> +	if (!epc->ops->alloc_doorbell)
> +		return 0;
> +
> +	mutex_lock(&epc->lock);
> +	ret = epc->ops->alloc_doorbell(epc, func_no, vfunc_no, num_msgs);
> +	mutex_unlock(&epc->lock);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(pci_epc_alloc_doorbell);
> +
> +/**
> + * pci_epc_free_doorbell() - free resource allocated by pci_epc_alloc_doorbell()
> + *
> + * @epc: the EPC device that need doorbell address and data from RC.
> + * @func_no: the physical endpoint function number in the EPC device.
> + * @vfunc_no: the virtual endpoint function number in the physical function.
> + *
> + * Return: 0 success, other is failure
> + */
> +void pci_epc_free_doorbell(struct pci_epc *epc, u8 func_no, u8 vfunc_no)
> +{
> +	if (IS_ERR_OR_NULL(epc) || func_no >= epc->max_functions)
> +		return;
> +
> +	if (vfunc_no > 0 && (!epc->max_vfs || vfunc_no > epc->max_vfs[func_no]))
> +		return;
> +
> +	if (!epc->ops->free_doorbell)
> +		return;
> +
> +	mutex_lock(&epc->lock);
> +	epc->ops->free_doorbell(epc, func_no, vfunc_no);
> +	mutex_unlock(&epc->lock);
> +}
> +EXPORT_SYMBOL_GPL(pci_epc_free_doorbell);
> +
> +static irqreturn_t pci_epf_generic_doorbell_handler(int irq, void *data)
> +{
> +	struct pci_epf *epf = data;
> +
> +	if (epf->event_ops && epf->event_ops->doorbell)
> +		epf->event_ops->doorbell(epf, irq - epf->virq_base);
> +
> +	return IRQ_HANDLED;
> +}

IMO the handler should be directly implemented in the EPF drivers. There 
should be one API which returns the virq and the msi_msg to the EPF 
driver and the EPF driver should do request_irq.
> +
> +static void pci_epc_generic_write_msi_msg(struct msi_desc *desc, struct msi_msg *msg)
> +{
> +	struct pci_epc *epc = NULL;
> +	struct class_dev_iter iter;
> +	struct pci_epf *epf;
> +	struct device *dev;
> +
> +	class_dev_iter_init(&iter, pci_epc_class, NULL, NULL);
> +	while ((dev = class_dev_iter_next(&iter))) {
> +		if (dev->parent != desc->dev)
> +			continue;

Ideally the msi_desc should be associated directly with the EPF device.
> +
> +		epc = to_pci_epc(dev);
> +
> +		class_dev_iter_exit(&iter);
> +		break;
> +	}
> +
> +	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 this is a platform restriction, this should be moved elsewhere.
> +
> +	if (epf->msg && desc->msi_index < epf->num_msgs)
> +		epf->msg[desc->msi_index] = *msg;
> +}
> +
> +
> +/**
> + * pci_epc_generic_alloc_doorbell() - Common help function. Allocate address space from MSI
> + *                                    controller
> + *
> + * @epc: the EPC device that need doorbell address and data from RC.
> + * @func_no: the physical endpoint function number in the EPC device.
> + * @vfunc_no: the virtual endpoint function number in the physical function.
> + * @num_msgs: the total number of doorbell messages
> + *
> + * Remark: use this function only if EPC driver just register one EPC device.
> + *
> + * Return: 0 success, other is failure
> + */
> +int pci_epc_generic_alloc_doorbell(struct pci_epc *epc, u8 func_no, u8 vfunc_no, int num_msgs)
> +{
> +	struct pci_epf *epf;
> +	struct device *dev;
> +	int virq, last;
> +	int ret;
> +	int i;
> +
> +	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_msi_domain_alloc_irqs(dev, num_msgs, pci_epc_generic_write_msi_msg);
> +	if (ret) {
> +		dev_err(dev, "Failed to allocate MSI\n");
> +		return -ENOMEM;
> +	}

The alloc_irqs should be for a EPF device IMO.
> +
> +	last = -1;
> +	for (i = 0; i < num_msgs; i++) {
> +		virq = msi_get_virq(dev, i);
> +		if (i == 0)
> +			epf->virq_base = virq;
> +
> +		ret = request_irq(virq, pci_epf_generic_doorbell_handler, 0,
> +				  kasprintf(GFP_KERNEL, "pci-epc-doorbell%d", i), epf);
> +
> +		if (ret) {
> +			dev_err(dev, "Failed to request doorbell\n");
> +			goto err_free_irq;
> +		}
> +		last = i;
> +	}
> +
> +	return 0;
> +
> +err_free_irq:
> +	for (i = 0; i < last; i++)
> +		kfree(free_irq(epf->virq_base + i, epf));
> +	platform_msi_domain_free_irqs(dev);
> +
> +	return -EINVAL;
> +}

Thanks,
Kishon

On Fri, Sep 29, 2023 at 03:00:16PM +0530, Kishon Vijay Abraham I wrote:
> Hi Frank,
> 
> On 9/12/2023 3:39 AM, Frank Li wrote:
> > This commit introduces a common method for sending messages from the Root
> > Complex (RC) to the Endpoint (EP) by utilizing the platform MSI interrupt
> > controller, such as ARM GIC, as an EP doorbell. Maps the memory assigned
> > for the BAR region by the PCI host to the message address of the platform
> > MSI interrupt controller in the PCI EP. As a result, when the PCI RC writes
> > to the BAR region, it triggers an IRQ at the EP. This implementation serves
> > as a common method for all endpoint function drivers.
> 
> This would help avoid the polling used in current EPF drivers. Thanks!
> > 
> > However, it currently supports only one EP physical function due to
> > limitations in ARM MSI/IMS readiness.
> 
> Any such platform or architecture restrictions should not be handled in the
> endpoint core layer.
> > 
> > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > ---
> >   drivers/pci/endpoint/pci-epc-core.c | 192 ++++++++++++++++++++++++++++
> >   drivers/pci/endpoint/pci-epf-core.c |  44 +++++++
> >   include/linux/pci-epc.h             |   6 +
> >   include/linux/pci-epf.h             |   7 +
> >   4 files changed, 249 insertions(+)
> > 
> > diff --git a/drivers/pci/endpoint/pci-epc-core.c b/drivers/pci/endpoint/pci-epc-core.c
> > index 5a4a8b0be6262..d336a99c6a94f 100644
> > --- a/drivers/pci/endpoint/pci-epc-core.c
> > +++ b/drivers/pci/endpoint/pci-epc-core.c
> > @@ -10,6 +10,7 @@
> >   #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>
> > @@ -783,6 +784,197 @@ void pci_epc_bme_notify(struct pci_epc *epc)
> >   }
> >   EXPORT_SYMBOL_GPL(pci_epc_bme_notify);
> > +/**
> > + * pci_epc_alloc_doorbell() - alloc an address space to let RC trigger EP side IRQ by write data to
> > + *			      the space.
> > + *
> > + * @epc: the EPC device that need doorbell address and data from RC.
> > + * @func_no: the physical endpoint function number in the EPC device.
> > + * @vfunc_no: the virtual endpoint function number in the physical function.
> > + * @num_msgs: the total number of doorbell messages
> > + *
> > + * Return: 0 success, other is failure
> > + */
> > +int pci_epc_alloc_doorbell(struct pci_epc *epc, u8 func_no, u8 vfunc_no, int num_msgs)
> > +{
> > +	int ret;
> > +
> > +	if (IS_ERR_OR_NULL(epc) || func_no >= epc->max_functions)
> > +		return -EINVAL;
> > +
> > +	if (vfunc_no > 0 && (!epc->max_vfs || vfunc_no > epc->max_vfs[func_no]))
> > +		return -EINVAL;
> > +
> > +	if (!epc->ops->alloc_doorbell)
> > +		return 0;
> > +
> > +	mutex_lock(&epc->lock);
> > +	ret = epc->ops->alloc_doorbell(epc, func_no, vfunc_no, num_msgs);
> > +	mutex_unlock(&epc->lock);
> > +
> > +	return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(pci_epc_alloc_doorbell);
> > +
> > +/**
> > + * pci_epc_free_doorbell() - free resource allocated by pci_epc_alloc_doorbell()
> > + *
> > + * @epc: the EPC device that need doorbell address and data from RC.
> > + * @func_no: the physical endpoint function number in the EPC device.
> > + * @vfunc_no: the virtual endpoint function number in the physical function.
> > + *
> > + * Return: 0 success, other is failure
> > + */
> > +void pci_epc_free_doorbell(struct pci_epc *epc, u8 func_no, u8 vfunc_no)
> > +{
> > +	if (IS_ERR_OR_NULL(epc) || func_no >= epc->max_functions)
> > +		return;
> > +
> > +	if (vfunc_no > 0 && (!epc->max_vfs || vfunc_no > epc->max_vfs[func_no]))
> > +		return;
> > +
> > +	if (!epc->ops->free_doorbell)
> > +		return;
> > +
> > +	mutex_lock(&epc->lock);
> > +	epc->ops->free_doorbell(epc, func_no, vfunc_no);
> > +	mutex_unlock(&epc->lock);
> > +}
> > +EXPORT_SYMBOL_GPL(pci_epc_free_doorbell);
> > +
> > +static irqreturn_t pci_epf_generic_doorbell_handler(int irq, void *data)
> > +{
> > +	struct pci_epf *epf = data;
> > +
> > +	if (epf->event_ops && epf->event_ops->doorbell)
> > +		epf->event_ops->doorbell(epf, irq - epf->virq_base);
> > +
> > +	return IRQ_HANDLED;
> > +}
> 
> IMO the handler should be directly implemented in the EPF drivers. There
> should be one API which returns the virq and the msi_msg to the EPF driver
> and the EPF driver should do request_irq.
> > +
> > +static void pci_epc_generic_write_msi_msg(struct msi_desc *desc, struct msi_msg *msg)
> > +{
> > +	struct pci_epc *epc = NULL;
> > +	struct class_dev_iter iter;
> > +	struct pci_epf *epf;
> > +	struct device *dev;
> > +
> > +	class_dev_iter_init(&iter, pci_epc_class, NULL, NULL);
> > +	while ((dev = class_dev_iter_next(&iter))) {
> > +		if (dev->parent != desc->dev)
> > +			continue;
> 
> Ideally the msi_desc should be associated directly with the EPF device.

The key problem is platform_msi_domain_alloc_irqs only support put epc
driver's dev into desc.

IMS may resolve this problem, but ARM IMS progress is quite slow.

> > +
> > +		epc = to_pci_epc(dev);
> > +
> > +		class_dev_iter_exit(&iter);
> > +		break;
> > +	}
> > +
> > +	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 this is a platform restriction, this should be moved elsewhere.

where is good place? 

> > +
> > +	if (epf->msg && desc->msi_index < epf->num_msgs)
> > +		epf->msg[desc->msi_index] = *msg;
> > +}
> > +
> > +
> > +/**
> > + * pci_epc_generic_alloc_doorbell() - Common help function. Allocate address space from MSI
> > + *                                    controller
> > + *
> > + * @epc: the EPC device that need doorbell address and data from RC.
> > + * @func_no: the physical endpoint function number in the EPC device.
> > + * @vfunc_no: the virtual endpoint function number in the physical function.
> > + * @num_msgs: the total number of doorbell messages
> > + *
> > + * Remark: use this function only if EPC driver just register one EPC device.
> > + *
> > + * Return: 0 success, other is failure
> > + */
> > +int pci_epc_generic_alloc_doorbell(struct pci_epc *epc, u8 func_no, u8 vfunc_no, int num_msgs)
> > +{
> > +	struct pci_epf *epf;
> > +	struct device *dev;
> > +	int virq, last;
> > +	int ret;
> > +	int i;
> > +
> > +	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_msi_domain_alloc_irqs(dev, num_msgs, pci_epc_generic_write_msi_msg);
> > +	if (ret) {
> > +		dev_err(dev, "Failed to allocate MSI\n");
> > +		return -ENOMEM;
> > +	}
> 
> The alloc_irqs should be for a EPF device IMO.

The GIC ITS driver need of_node information to alloc msi for such devices.
But EPF device have not of_node information.

> > +
> > +	last = -1;
> > +	for (i = 0; i < num_msgs; i++) {
> > +		virq = msi_get_virq(dev, i);
> > +		if (i == 0)
> > +			epf->virq_base = virq;
> > +
> > +		ret = request_irq(virq, pci_epf_generic_doorbell_handler, 0,
> > +				  kasprintf(GFP_KERNEL, "pci-epc-doorbell%d", i), epf);
> > +
> > +		if (ret) {
> > +			dev_err(dev, "Failed to request doorbell\n");
> > +			goto err_free_irq;
> > +		}
> > +		last = i;
> > +	}
> > +
> > +	return 0;
> > +
> > +err_free_irq:
> > +	for (i = 0; i < last; i++)
> > +		kfree(free_irq(epf->virq_base + i, epf));
> > +	platform_msi_domain_free_irqs(dev);
> > +
> > +	return -EINVAL;
> > +}
> 
> Thanks,
> Kishon

On Mon, Sep 11, 2023 at 06:09:16PM -0400, Frank Li wrote:
> This commit introduces a common method for sending messages from the Root
> Complex (RC) to the Endpoint (EP) by utilizing the platform MSI interrupt
> controller, such as ARM GIC, as an EP doorbell. Maps the memory assigned
> for the BAR region by the PCI host to the message address of the platform
> MSI interrupt controller in the PCI EP. As a result, when the PCI RC writes

"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."

> to the BAR region, it triggers an IRQ at the EP. This implementation serves
> as a common method for all endpoint function drivers.
> 
> However, it currently supports only one EP physical function due to
> limitations in ARM MSI/IMS readiness.
> 
> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---
>  drivers/pci/endpoint/pci-epc-core.c | 192 ++++++++++++++++++++++++++++
>  drivers/pci/endpoint/pci-epf-core.c |  44 +++++++
>  include/linux/pci-epc.h             |   6 +
>  include/linux/pci-epf.h             |   7 +
>  4 files changed, 249 insertions(+)
> 
> diff --git a/drivers/pci/endpoint/pci-epc-core.c b/drivers/pci/endpoint/pci-epc-core.c
> index 5a4a8b0be6262..d336a99c6a94f 100644
> --- a/drivers/pci/endpoint/pci-epc-core.c
> +++ b/drivers/pci/endpoint/pci-epc-core.c
> @@ -10,6 +10,7 @@
>  #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>
> @@ -783,6 +784,197 @@ void pci_epc_bme_notify(struct pci_epc *epc)
>  }
>  EXPORT_SYMBOL_GPL(pci_epc_bme_notify);
>  
> +/**
> + * pci_epc_alloc_doorbell() - alloc an address space to let RC trigger EP side IRQ by write data to
> + *			      the space.

"Allocate platform specific doorbell IRQs to be used by the host to trigger
doorbells on EP."

> + *
> + * @epc: the EPC device that need doorbell address and data from RC.

EPC device for which the doorbell needs to be allocated

> + * @func_no: the physical endpoint function number in the EPC device.
> + * @vfunc_no: the virtual endpoint function number in the physical function.
> + * @num_msgs: the total number of doorbell messages

s/num_msgs/num_db

> + *
> + * Return: 0 success, other is failure
> + */
> +int pci_epc_alloc_doorbell(struct pci_epc *epc, u8 func_no, u8 vfunc_no, int num_msgs)
> +{
> +	int ret;
> +
> +	if (IS_ERR_OR_NULL(epc) || func_no >= epc->max_functions)
> +		return -EINVAL;
> +
> +	if (vfunc_no > 0 && (!epc->max_vfs || vfunc_no > epc->max_vfs[func_no]))
> +		return -EINVAL;
> +
> +	if (!epc->ops->alloc_doorbell)
> +		return 0;

You mentioned 0 is a success. So if there is no callback, you want to return
success?

> +
> +	mutex_lock(&epc->lock);
> +	ret = epc->ops->alloc_doorbell(epc, func_no, vfunc_no, num_msgs);

Why can't you just call the generic function here and in other places instead of
implementing callbacks? I do not see a necessity for EPC specific callbacks. If
there is one, please specify.

> +	mutex_unlock(&epc->lock);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(pci_epc_alloc_doorbell);
> +
> +/**
> + * pci_epc_free_doorbell() - free resource allocated by pci_epc_alloc_doorbell()
> + *
> + * @epc: the EPC device that need doorbell address and data from RC.

Same as above.

> + * @func_no: the physical endpoint function number in the EPC device.
> + * @vfunc_no: the virtual endpoint function number in the physical function.
> + *
> + * Return: 0 success, other is failure
> + */
> +void pci_epc_free_doorbell(struct pci_epc *epc, u8 func_no, u8 vfunc_no)
> +{
> +	if (IS_ERR_OR_NULL(epc) || func_no >= epc->max_functions)
> +		return;
> +
> +	if (vfunc_no > 0 && (!epc->max_vfs || vfunc_no > epc->max_vfs[func_no]))
> +		return;
> +
> +	if (!epc->ops->free_doorbell)
> +		return;
> +
> +	mutex_lock(&epc->lock);
> +	epc->ops->free_doorbell(epc, func_no, vfunc_no);

Same as suggested above.

> +	mutex_unlock(&epc->lock);
> +}
> +EXPORT_SYMBOL_GPL(pci_epc_free_doorbell);
> +
> +static irqreturn_t pci_epf_generic_doorbell_handler(int irq, void *data)
> +{
> +	struct pci_epf *epf = data;
> +
> +	if (epf->event_ops && epf->event_ops->doorbell)
> +		epf->event_ops->doorbell(epf, irq - epf->virq_base);

Same as suggested above.

> +
> +	return IRQ_HANDLED;
> +}
> +
> +static void pci_epc_generic_write_msi_msg(struct msi_desc *desc, struct msi_msg *msg)
> +{
> +	struct pci_epc *epc = NULL;
> +	struct class_dev_iter iter;
> +	struct pci_epf *epf;
> +	struct device *dev;
> +
> +	class_dev_iter_init(&iter, pci_epc_class, NULL, NULL);
> +	while ((dev = class_dev_iter_next(&iter))) {
> +		if (dev->parent != desc->dev)
> +			continue;
> +
> +		epc = to_pci_epc(dev);
> +
> +		class_dev_iter_exit(&iter);
> +		break;
> +	}
> +
> +	if (!epc)
> +		return;
> +
> +	/* Only support one EPF for doorbell */
> +	epf = list_first_entry_or_null(&epc->pci_epf, struct pci_epf, list);
> +

No need of this newline

> +	if (!epf)
> +		return;
> +
> +	if (epf->msg && desc->msi_index < epf->num_msgs)
> +		epf->msg[desc->msi_index] = *msg;
> +}
> +
> +

Remove extra newline

> +/**
> + * pci_epc_generic_alloc_doorbell() - Common help function. Allocate address space from MSI
> + *                                    controller
> + *
> + * @epc: the EPC device that need doorbell address and data from RC.
> + * @func_no: the physical endpoint function number in the EPC device.
> + * @vfunc_no: the virtual endpoint function number in the physical function.
> + * @num_msgs: the total number of doorbell messages
> + *

Same comment as for pci_epc_alloc_doorbell()

> + * Remark: use this function only if EPC driver just register one EPC device.
> + *
> + * Return: 0 success, other is failure
> + */
> +int pci_epc_generic_alloc_doorbell(struct pci_epc *epc, u8 func_no, u8 vfunc_no, int num_msgs)
> +{
> +	struct pci_epf *epf;
> +	struct device *dev;
> +	int virq, last;
> +	int ret;
> +	int i;
> +
> +	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_msi_domain_alloc_irqs(dev, num_msgs, pci_epc_generic_write_msi_msg);
> +	if (ret) {
> +		dev_err(dev, "Failed to allocate MSI\n");
> +		return -ENOMEM;
> +	}
> +
> +	last = -1;
> +	for (i = 0; i < num_msgs; i++) {

You should iterate over msi_desc as below:

        msi_lock_descs(dev);
        msi_for_each_desc(desc, dev, MSI_DESC_ALL) {
		...
	}
	msi_unlock_descs(dev);

> +		virq = msi_get_virq(dev, i);
> +		if (i == 0)
> +			epf->virq_base = virq;
> +
> +		ret = request_irq(virq, pci_epf_generic_doorbell_handler, 0,

	request_irq(desc->irq, ...)

> +				  kasprintf(GFP_KERNEL, "pci-epc-doorbell%d", i), epf);
> +
> +		if (ret) {
> +			dev_err(dev, "Failed to request doorbell\n");
> +			goto err_free_irq;
> +		}
> +		last = i;
> +	}
> +
> +	return 0;
> +
> +err_free_irq:
> +	for (i = 0; i < last; i++)
> +		kfree(free_irq(epf->virq_base + i, epf));
> +	platform_msi_domain_free_irqs(dev);
> +
> +	return -EINVAL;

	return ret;

> +}
> +EXPORT_SYMBOL_GPL(pci_epc_generic_alloc_doorbell);
> +

[...]

> diff --git a/include/linux/pci-epf.h b/include/linux/pci-epf.h
> index 3f44b6aec4770..485c146a5efe2 100644
> --- a/include/linux/pci-epf.h
> +++ b/include/linux/pci-epf.h
> @@ -79,6 +79,7 @@ struct pci_epc_event_ops {
>  	int (*link_up)(struct pci_epf *epf);
>  	int (*link_down)(struct pci_epf *epf);
>  	int (*bme)(struct pci_epf *epf);
> +	int (*doorbell)(struct pci_epf *epf, int index);

kdoc missing.

>  };
>  
>  /**
> @@ -180,6 +181,9 @@ 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_msgs;

num_db

You also need to add kdoc for each new member.

- Mani

On Wed, Oct 18, 2023 at 12:07:22AM +0530, Manivannan Sadhasivam wrote:
> On Mon, Sep 11, 2023 at 06:09:16PM -0400, Frank Li wrote:
> > This commit introduces a common method for sending messages from the Root
> > Complex (RC) to the Endpoint (EP) by utilizing the platform MSI interrupt
> > controller, such as ARM GIC, as an EP doorbell. Maps the memory assigned
> > for the BAR region by the PCI host to the message address of the platform
> > MSI interrupt controller in the PCI EP. As a result, when the PCI RC writes
> 
> "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."
> 
> > to the BAR region, it triggers an IRQ at the EP. This implementation serves
> > as a common method for all endpoint function drivers.
> > 
> > However, it currently supports only one EP physical function due to
> > limitations in ARM MSI/IMS readiness.
> > 
> > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > ---
> >  drivers/pci/endpoint/pci-epc-core.c | 192 ++++++++++++++++++++++++++++
> >  drivers/pci/endpoint/pci-epf-core.c |  44 +++++++
> >  include/linux/pci-epc.h             |   6 +
> >  include/linux/pci-epf.h             |   7 +
> >  4 files changed, 249 insertions(+)
> > 
> > diff --git a/drivers/pci/endpoint/pci-epc-core.c b/drivers/pci/endpoint/pci-epc-core.c
> > index 5a4a8b0be6262..d336a99c6a94f 100644
> > --- a/drivers/pci/endpoint/pci-epc-core.c
> > +++ b/drivers/pci/endpoint/pci-epc-core.c
> > @@ -10,6 +10,7 @@
> >  #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>
> > @@ -783,6 +784,197 @@ void pci_epc_bme_notify(struct pci_epc *epc)
> >  }
> >  EXPORT_SYMBOL_GPL(pci_epc_bme_notify);
> >  
> > +/**
> > + * pci_epc_alloc_doorbell() - alloc an address space to let RC trigger EP side IRQ by write data to
> > + *			      the space.
> 
> "Allocate platform specific doorbell IRQs to be used by the host to trigger
> doorbells on EP."
> 
> > + *
> > + * @epc: the EPC device that need doorbell address and data from RC.
> 
> EPC device for which the doorbell needs to be allocated
> 
> > + * @func_no: the physical endpoint function number in the EPC device.
> > + * @vfunc_no: the virtual endpoint function number in the physical function.
> > + * @num_msgs: the total number of doorbell messages
> 
> s/num_msgs/num_db
> 
> > + *
> > + * Return: 0 success, other is failure
> > + */
> > +int pci_epc_alloc_doorbell(struct pci_epc *epc, u8 func_no, u8 vfunc_no, int num_msgs)
> > +{
> > +	int ret;
> > +
> > +	if (IS_ERR_OR_NULL(epc) || func_no >= epc->max_functions)
> > +		return -EINVAL;
> > +
> > +	if (vfunc_no > 0 && (!epc->max_vfs || vfunc_no > epc->max_vfs[func_no]))
> > +		return -EINVAL;
> > +
> > +	if (!epc->ops->alloc_doorbell)
> > +		return 0;
> 
> You mentioned 0 is a success. So if there is no callback, you want to return
> success?
> 
> > +
> > +	mutex_lock(&epc->lock);
> > +	ret = epc->ops->alloc_doorbell(epc, func_no, vfunc_no, num_msgs);
> 
> Why can't you just call the generic function here and in other places instead of
> implementing callbacks? I do not see a necessity for EPC specific callbacks. If
> there is one, please specify.

1. Refer v1 your comments.
https://lore.kernel.org/imx/20230906145227.GC5930@thinkpad/
2. Maybe some ep controller have built-in doorbell support. Write to some
address to trigger doorbell irq.

Frank

> 
> > +	mutex_unlock(&epc->lock);
> > +
> > +	return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(pci_epc_alloc_doorbell);
> > +
> > +/**
> > + * pci_epc_free_doorbell() - free resource allocated by pci_epc_alloc_doorbell()
> > + *
> > + * @epc: the EPC device that need doorbell address and data from RC.
> 
> Same as above.
> 
> > + * @func_no: the physical endpoint function number in the EPC device.
> > + * @vfunc_no: the virtual endpoint function number in the physical function.
> > + *
> > + * Return: 0 success, other is failure
> > + */
> > +void pci_epc_free_doorbell(struct pci_epc *epc, u8 func_no, u8 vfunc_no)
> > +{
> > +	if (IS_ERR_OR_NULL(epc) || func_no >= epc->max_functions)
> > +		return;
> > +
> > +	if (vfunc_no > 0 && (!epc->max_vfs || vfunc_no > epc->max_vfs[func_no]))
> > +		return;
> > +
> > +	if (!epc->ops->free_doorbell)
> > +		return;
> > +
> > +	mutex_lock(&epc->lock);
> > +	epc->ops->free_doorbell(epc, func_no, vfunc_no);
> 
> Same as suggested above.
> 
> > +	mutex_unlock(&epc->lock);
> > +}
> > +EXPORT_SYMBOL_GPL(pci_epc_free_doorbell);
> > +
> > +static irqreturn_t pci_epf_generic_doorbell_handler(int irq, void *data)
> > +{
> > +	struct pci_epf *epf = data;
> > +
> > +	if (epf->event_ops && epf->event_ops->doorbell)
> > +		epf->event_ops->doorbell(epf, irq - epf->virq_base);
> 
> Same as suggested above.
> 
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +static void pci_epc_generic_write_msi_msg(struct msi_desc *desc, struct msi_msg *msg)
> > +{
> > +	struct pci_epc *epc = NULL;
> > +	struct class_dev_iter iter;
> > +	struct pci_epf *epf;
> > +	struct device *dev;
> > +
> > +	class_dev_iter_init(&iter, pci_epc_class, NULL, NULL);
> > +	while ((dev = class_dev_iter_next(&iter))) {
> > +		if (dev->parent != desc->dev)
> > +			continue;
> > +
> > +		epc = to_pci_epc(dev);
> > +
> > +		class_dev_iter_exit(&iter);
> > +		break;
> > +	}
> > +
> > +	if (!epc)
> > +		return;
> > +
> > +	/* Only support one EPF for doorbell */
> > +	epf = list_first_entry_or_null(&epc->pci_epf, struct pci_epf, list);
> > +
> 
> No need of this newline
> 
> > +	if (!epf)
> > +		return;
> > +
> > +	if (epf->msg && desc->msi_index < epf->num_msgs)
> > +		epf->msg[desc->msi_index] = *msg;
> > +}
> > +
> > +
> 
> Remove extra newline
> 
> > +/**
> > + * pci_epc_generic_alloc_doorbell() - Common help function. Allocate address space from MSI
> > + *                                    controller
> > + *
> > + * @epc: the EPC device that need doorbell address and data from RC.
> > + * @func_no: the physical endpoint function number in the EPC device.
> > + * @vfunc_no: the virtual endpoint function number in the physical function.
> > + * @num_msgs: the total number of doorbell messages
> > + *
> 
> Same comment as for pci_epc_alloc_doorbell()
> 
> > + * Remark: use this function only if EPC driver just register one EPC device.
> > + *
> > + * Return: 0 success, other is failure
> > + */
> > +int pci_epc_generic_alloc_doorbell(struct pci_epc *epc, u8 func_no, u8 vfunc_no, int num_msgs)
> > +{
> > +	struct pci_epf *epf;
> > +	struct device *dev;
> > +	int virq, last;
> > +	int ret;
> > +	int i;
> > +
> > +	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_msi_domain_alloc_irqs(dev, num_msgs, pci_epc_generic_write_msi_msg);
> > +	if (ret) {
> > +		dev_err(dev, "Failed to allocate MSI\n");
> > +		return -ENOMEM;
> > +	}
> > +
> > +	last = -1;
> > +	for (i = 0; i < num_msgs; i++) {
> 
> You should iterate over msi_desc as below:
> 
>         msi_lock_descs(dev);
>         msi_for_each_desc(desc, dev, MSI_DESC_ALL) {
> 		...
> 	}
> 	msi_unlock_descs(dev);
> 
> > +		virq = msi_get_virq(dev, i);
> > +		if (i == 0)
> > +			epf->virq_base = virq;
> > +
> > +		ret = request_irq(virq, pci_epf_generic_doorbell_handler, 0,
> 
> 	request_irq(desc->irq, ...)
> 
> > +				  kasprintf(GFP_KERNEL, "pci-epc-doorbell%d", i), epf);
> > +
> > +		if (ret) {
> > +			dev_err(dev, "Failed to request doorbell\n");
> > +			goto err_free_irq;
> > +		}
> > +		last = i;
> > +	}
> > +
> > +	return 0;
> > +
> > +err_free_irq:
> > +	for (i = 0; i < last; i++)
> > +		kfree(free_irq(epf->virq_base + i, epf));
> > +	platform_msi_domain_free_irqs(dev);
> > +
> > +	return -EINVAL;
> 
> 	return ret;
> 
> > +}
> > +EXPORT_SYMBOL_GPL(pci_epc_generic_alloc_doorbell);
> > +
> 
> [...]
> 
> > diff --git a/include/linux/pci-epf.h b/include/linux/pci-epf.h
> > index 3f44b6aec4770..485c146a5efe2 100644
> > --- a/include/linux/pci-epf.h
> > +++ b/include/linux/pci-epf.h
> > @@ -79,6 +79,7 @@ struct pci_epc_event_ops {
> >  	int (*link_up)(struct pci_epf *epf);
> >  	int (*link_down)(struct pci_epf *epf);
> >  	int (*bme)(struct pci_epf *epf);
> > +	int (*doorbell)(struct pci_epf *epf, int index);
> 
> kdoc missing.
> 
> >  };
> >  
> >  /**
> > @@ -180,6 +181,9 @@ 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_msgs;
> 
> num_db
> 
> You also need to add kdoc for each new member.
> 
> - Mani
> 
> -- 
> மணிவண்ணன் சதாசிவம்

On Tue, Oct 17, 2023 at 02:55:57PM -0400, Frank Li wrote:
> On Wed, Oct 18, 2023 at 12:07:22AM +0530, Manivannan Sadhasivam wrote:
> > On Mon, Sep 11, 2023 at 06:09:16PM -0400, Frank Li wrote:
> > > This commit introduces a common method for sending messages from the Root
> > > Complex (RC) to the Endpoint (EP) by utilizing the platform MSI interrupt
> > > controller, such as ARM GIC, as an EP doorbell. Maps the memory assigned
> > > for the BAR region by the PCI host to the message address of the platform
> > > MSI interrupt controller in the PCI EP. As a result, when the PCI RC writes
> > 
> > "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."
> > 
> > > to the BAR region, it triggers an IRQ at the EP. This implementation serves
> > > as a common method for all endpoint function drivers.
> > > 
> > > However, it currently supports only one EP physical function due to
> > > limitations in ARM MSI/IMS readiness.
> > > 
> > > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > > ---
> > >  drivers/pci/endpoint/pci-epc-core.c | 192 ++++++++++++++++++++++++++++
> > >  drivers/pci/endpoint/pci-epf-core.c |  44 +++++++
> > >  include/linux/pci-epc.h             |   6 +
> > >  include/linux/pci-epf.h             |   7 +
> > >  4 files changed, 249 insertions(+)
> > > 
> > > diff --git a/drivers/pci/endpoint/pci-epc-core.c b/drivers/pci/endpoint/pci-epc-core.c
> > > index 5a4a8b0be6262..d336a99c6a94f 100644
> > > --- a/drivers/pci/endpoint/pci-epc-core.c
> > > +++ b/drivers/pci/endpoint/pci-epc-core.c
> > > @@ -10,6 +10,7 @@
> > >  #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>
> > > @@ -783,6 +784,197 @@ void pci_epc_bme_notify(struct pci_epc *epc)
> > >  }
> > >  EXPORT_SYMBOL_GPL(pci_epc_bme_notify);
> > >  
> > > +/**
> > > + * pci_epc_alloc_doorbell() - alloc an address space to let RC trigger EP side IRQ by write data to
> > > + *			      the space.
> > 
> > "Allocate platform specific doorbell IRQs to be used by the host to trigger
> > doorbells on EP."
> > 
> > > + *
> > > + * @epc: the EPC device that need doorbell address and data from RC.
> > 
> > EPC device for which the doorbell needs to be allocated
> > 
> > > + * @func_no: the physical endpoint function number in the EPC device.
> > > + * @vfunc_no: the virtual endpoint function number in the physical function.
> > > + * @num_msgs: the total number of doorbell messages
> > 
> > s/num_msgs/num_db
> > 
> > > + *
> > > + * Return: 0 success, other is failure
> > > + */
> > > +int pci_epc_alloc_doorbell(struct pci_epc *epc, u8 func_no, u8 vfunc_no, int num_msgs)
> > > +{
> > > +	int ret;
> > > +
> > > +	if (IS_ERR_OR_NULL(epc) || func_no >= epc->max_functions)
> > > +		return -EINVAL;
> > > +
> > > +	if (vfunc_no > 0 && (!epc->max_vfs || vfunc_no > epc->max_vfs[func_no]))
> > > +		return -EINVAL;
> > > +
> > > +	if (!epc->ops->alloc_doorbell)
> > > +		return 0;
> > 
> > You mentioned 0 is a success. So if there is no callback, you want to return
> > success?
> > 
> > > +
> > > +	mutex_lock(&epc->lock);
> > > +	ret = epc->ops->alloc_doorbell(epc, func_no, vfunc_no, num_msgs);
> > 
> > Why can't you just call the generic function here and in other places instead of
> > implementing callbacks? I do not see a necessity for EPC specific callbacks. If
> > there is one, please specify.
> 
> 1. Refer v1 your comments.
> https://lore.kernel.org/imx/20230906145227.GC5930@thinkpad/

I do not find where I suggested the callback approach.

> 2. Maybe some ep controller have built-in doorbell support. Write to some
> address to trigger doorbell irq.
> 

We will handle it whenever such EP controllers arrive. Until then, let's keep it
simple.

- Mani

> Frank
> 
> > 
> > > +	mutex_unlock(&epc->lock);
> > > +
> > > +	return ret;
> > > +}
> > > +EXPORT_SYMBOL_GPL(pci_epc_alloc_doorbell);
> > > +
> > > +/**
> > > + * pci_epc_free_doorbell() - free resource allocated by pci_epc_alloc_doorbell()
> > > + *
> > > + * @epc: the EPC device that need doorbell address and data from RC.
> > 
> > Same as above.
> > 
> > > + * @func_no: the physical endpoint function number in the EPC device.
> > > + * @vfunc_no: the virtual endpoint function number in the physical function.
> > > + *
> > > + * Return: 0 success, other is failure
> > > + */
> > > +void pci_epc_free_doorbell(struct pci_epc *epc, u8 func_no, u8 vfunc_no)
> > > +{
> > > +	if (IS_ERR_OR_NULL(epc) || func_no >= epc->max_functions)
> > > +		return;
> > > +
> > > +	if (vfunc_no > 0 && (!epc->max_vfs || vfunc_no > epc->max_vfs[func_no]))
> > > +		return;
> > > +
> > > +	if (!epc->ops->free_doorbell)
> > > +		return;
> > > +
> > > +	mutex_lock(&epc->lock);
> > > +	epc->ops->free_doorbell(epc, func_no, vfunc_no);
> > 
> > Same as suggested above.
> > 
> > > +	mutex_unlock(&epc->lock);
> > > +}
> > > +EXPORT_SYMBOL_GPL(pci_epc_free_doorbell);
> > > +
> > > +static irqreturn_t pci_epf_generic_doorbell_handler(int irq, void *data)
> > > +{
> > > +	struct pci_epf *epf = data;
> > > +
> > > +	if (epf->event_ops && epf->event_ops->doorbell)
> > > +		epf->event_ops->doorbell(epf, irq - epf->virq_base);
> > 
> > Same as suggested above.
> > 
> > > +
> > > +	return IRQ_HANDLED;
> > > +}
> > > +
> > > +static void pci_epc_generic_write_msi_msg(struct msi_desc *desc, struct msi_msg *msg)
> > > +{
> > > +	struct pci_epc *epc = NULL;
> > > +	struct class_dev_iter iter;
> > > +	struct pci_epf *epf;
> > > +	struct device *dev;
> > > +
> > > +	class_dev_iter_init(&iter, pci_epc_class, NULL, NULL);
> > > +	while ((dev = class_dev_iter_next(&iter))) {
> > > +		if (dev->parent != desc->dev)
> > > +			continue;
> > > +
> > > +		epc = to_pci_epc(dev);
> > > +
> > > +		class_dev_iter_exit(&iter);
> > > +		break;
> > > +	}
> > > +
> > > +	if (!epc)
> > > +		return;
> > > +
> > > +	/* Only support one EPF for doorbell */
> > > +	epf = list_first_entry_or_null(&epc->pci_epf, struct pci_epf, list);
> > > +
> > 
> > No need of this newline
> > 
> > > +	if (!epf)
> > > +		return;
> > > +
> > > +	if (epf->msg && desc->msi_index < epf->num_msgs)
> > > +		epf->msg[desc->msi_index] = *msg;
> > > +}
> > > +
> > > +
> > 
> > Remove extra newline
> > 
> > > +/**
> > > + * pci_epc_generic_alloc_doorbell() - Common help function. Allocate address space from MSI
> > > + *                                    controller
> > > + *
> > > + * @epc: the EPC device that need doorbell address and data from RC.
> > > + * @func_no: the physical endpoint function number in the EPC device.
> > > + * @vfunc_no: the virtual endpoint function number in the physical function.
> > > + * @num_msgs: the total number of doorbell messages
> > > + *
> > 
> > Same comment as for pci_epc_alloc_doorbell()
> > 
> > > + * Remark: use this function only if EPC driver just register one EPC device.
> > > + *
> > > + * Return: 0 success, other is failure
> > > + */
> > > +int pci_epc_generic_alloc_doorbell(struct pci_epc *epc, u8 func_no, u8 vfunc_no, int num_msgs)
> > > +{
> > > +	struct pci_epf *epf;
> > > +	struct device *dev;
> > > +	int virq, last;
> > > +	int ret;
> > > +	int i;
> > > +
> > > +	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_msi_domain_alloc_irqs(dev, num_msgs, pci_epc_generic_write_msi_msg);
> > > +	if (ret) {
> > > +		dev_err(dev, "Failed to allocate MSI\n");
> > > +		return -ENOMEM;
> > > +	}
> > > +
> > > +	last = -1;
> > > +	for (i = 0; i < num_msgs; i++) {
> > 
> > You should iterate over msi_desc as below:
> > 
> >         msi_lock_descs(dev);
> >         msi_for_each_desc(desc, dev, MSI_DESC_ALL) {
> > 		...
> > 	}
> > 	msi_unlock_descs(dev);
> > 
> > > +		virq = msi_get_virq(dev, i);
> > > +		if (i == 0)
> > > +			epf->virq_base = virq;
> > > +
> > > +		ret = request_irq(virq, pci_epf_generic_doorbell_handler, 0,
> > 
> > 	request_irq(desc->irq, ...)
> > 
> > > +				  kasprintf(GFP_KERNEL, "pci-epc-doorbell%d", i), epf);
> > > +
> > > +		if (ret) {
> > > +			dev_err(dev, "Failed to request doorbell\n");
> > > +			goto err_free_irq;
> > > +		}
> > > +		last = i;
> > > +	}
> > > +
> > > +	return 0;
> > > +
> > > +err_free_irq:
> > > +	for (i = 0; i < last; i++)
> > > +		kfree(free_irq(epf->virq_base + i, epf));
> > > +	platform_msi_domain_free_irqs(dev);
> > > +
> > > +	return -EINVAL;
> > 
> > 	return ret;
> > 
> > > +}
> > > +EXPORT_SYMBOL_GPL(pci_epc_generic_alloc_doorbell);
> > > +
> > 
> > [...]
> > 
> > > diff --git a/include/linux/pci-epf.h b/include/linux/pci-epf.h
> > > index 3f44b6aec4770..485c146a5efe2 100644
> > > --- a/include/linux/pci-epf.h
> > > +++ b/include/linux/pci-epf.h
> > > @@ -79,6 +79,7 @@ struct pci_epc_event_ops {
> > >  	int (*link_up)(struct pci_epf *epf);
> > >  	int (*link_down)(struct pci_epf *epf);
> > >  	int (*bme)(struct pci_epf *epf);
> > > +	int (*doorbell)(struct pci_epf *epf, int index);
> > 
> > kdoc missing.
> > 
> > >  };
> > >  
> > >  /**
> > > @@ -180,6 +181,9 @@ 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_msgs;
> > 
> > num_db
> > 
> > You also need to add kdoc for each new member.
> > 
> > - Mani
> > 
> > -- 
> > மணிவண்ணன் சதாசிவம்

On Thu, Oct 19, 2023 at 08:34:41PM +0530, Manivannan Sadhasivam wrote:
> On Tue, Oct 17, 2023 at 02:55:57PM -0400, Frank Li wrote:
> > On Wed, Oct 18, 2023 at 12:07:22AM +0530, Manivannan Sadhasivam wrote:
> > > On Mon, Sep 11, 2023 at 06:09:16PM -0400, Frank Li wrote:
> > > > This commit introduces a common method for sending messages from the Root
> > > > Complex (RC) to the Endpoint (EP) by utilizing the platform MSI interrupt
> > > > controller, such as ARM GIC, as an EP doorbell. Maps the memory assigned
> > > > for the BAR region by the PCI host to the message address of the platform
> > > > MSI interrupt controller in the PCI EP. As a result, when the PCI RC writes
> > > 
> > > "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."
> > > 
> > > > to the BAR region, it triggers an IRQ at the EP. This implementation serves
> > > > as a common method for all endpoint function drivers.
> > > > 
> > > > However, it currently supports only one EP physical function due to
> > > > limitations in ARM MSI/IMS readiness.
> > > > 
> > > > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > > > ---
> > > >  drivers/pci/endpoint/pci-epc-core.c | 192 ++++++++++++++++++++++++++++
> > > >  drivers/pci/endpoint/pci-epf-core.c |  44 +++++++
> > > >  include/linux/pci-epc.h             |   6 +
> > > >  include/linux/pci-epf.h             |   7 +
> > > >  4 files changed, 249 insertions(+)
> > > > 
> > > > diff --git a/drivers/pci/endpoint/pci-epc-core.c b/drivers/pci/endpoint/pci-epc-core.c
> > > > index 5a4a8b0be6262..d336a99c6a94f 100644
> > > > --- a/drivers/pci/endpoint/pci-epc-core.c
> > > > +++ b/drivers/pci/endpoint/pci-epc-core.c
> > > > @@ -10,6 +10,7 @@
> > > >  #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>
> > > > @@ -783,6 +784,197 @@ void pci_epc_bme_notify(struct pci_epc *epc)
> > > >  }
> > > >  EXPORT_SYMBOL_GPL(pci_epc_bme_notify);
> > > >  
> > > > +/**
> > > > + * pci_epc_alloc_doorbell() - alloc an address space to let RC trigger EP side IRQ by write data to
> > > > + *			      the space.
> > > 
> > > "Allocate platform specific doorbell IRQs to be used by the host to trigger
> > > doorbells on EP."
> > > 
> > > > + *
> > > > + * @epc: the EPC device that need doorbell address and data from RC.
> > > 
> > > EPC device for which the doorbell needs to be allocated
> > > 
> > > > + * @func_no: the physical endpoint function number in the EPC device.
> > > > + * @vfunc_no: the virtual endpoint function number in the physical function.
> > > > + * @num_msgs: the total number of doorbell messages
> > > 
> > > s/num_msgs/num_db
> > > 
> > > > + *
> > > > + * Return: 0 success, other is failure
> > > > + */
> > > > +int pci_epc_alloc_doorbell(struct pci_epc *epc, u8 func_no, u8 vfunc_no, int num_msgs)
> > > > +{
> > > > +	int ret;
> > > > +
> > > > +	if (IS_ERR_OR_NULL(epc) || func_no >= epc->max_functions)
> > > > +		return -EINVAL;
> > > > +
> > > > +	if (vfunc_no > 0 && (!epc->max_vfs || vfunc_no > epc->max_vfs[func_no]))
> > > > +		return -EINVAL;
> > > > +
> > > > +	if (!epc->ops->alloc_doorbell)
> > > > +		return 0;
> > > 
> > > You mentioned 0 is a success. So if there is no callback, you want to return
> > > success?
> > > 
> > > > +
> > > > +	mutex_lock(&epc->lock);
> > > > +	ret = epc->ops->alloc_doorbell(epc, func_no, vfunc_no, num_msgs);
> > > 
> > > Why can't you just call the generic function here and in other places instead of
> > > implementing callbacks? I do not see a necessity for EPC specific callbacks. If
> > > there is one, please specify.
> > 
> > 1. Refer v1 your comments.
> > https://lore.kernel.org/imx/20230906145227.GC5930@thinkpad/
> 
> I do not find where I suggested the callback approach.

	> > > If that, Each EPF driver need do duplicate work. 
	> > > 
	> > 
	> > Yes, and that's how it should be. EPF core has no job in supplying the of_node.
	> > It is the responsibility of the EPF drivers as they depend on OF for platform
	> > support.
	> 
	> EPF driver still not depend on OF. such pci-epf-test, which was probed by
	> configfs.
	> 

	Hmm, yeah. Then it should be part of the EPC driver.

	Sorry for the confusion.

Here, all "EPF" should be "EPC". The key problem is of_node. EPC core have
not of_node, EPC core's parent driver (like dwc-ep driver) have of_node. 

pci_epc_generic_alloc_doorbell(dev), dev is probed by platform driver, such
as dwc-ep, which have of_node,  EPC core will create child device.

dwc-ep device
 |- epc core device

we can direct call pci_epc_generic_alloc_doorbell(epc->parent) here.

I may miss understand what your means. I think you want to dwc-ep
(with of_node) handle these alloc functions. 

> 
> > 2. Maybe some ep controller have built-in doorbell support. Write to some
> > address to trigger doorbell irq.
> > 
> 
> We will handle it whenever such EP controllers arrive. Until then, let's keep it
> simple.
> 
> - Mani
> 
> > Frank
> > 
> > > 
> > > > +	mutex_unlock(&epc->lock);
> > > > +
> > > > +	return ret;
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(pci_epc_alloc_doorbell);
> > > > +
> > > > +/**
> > > > + * pci_epc_free_doorbell() - free resource allocated by pci_epc_alloc_doorbell()
> > > > + *
> > > > + * @epc: the EPC device that need doorbell address and data from RC.
> > > 
> > > Same as above.
> > > 
> > > > + * @func_no: the physical endpoint function number in the EPC device.
> > > > + * @vfunc_no: the virtual endpoint function number in the physical function.
> > > > + *
> > > > + * Return: 0 success, other is failure
> > > > + */
> > > > +void pci_epc_free_doorbell(struct pci_epc *epc, u8 func_no, u8 vfunc_no)
> > > > +{
> > > > +	if (IS_ERR_OR_NULL(epc) || func_no >= epc->max_functions)
> > > > +		return;
> > > > +
> > > > +	if (vfunc_no > 0 && (!epc->max_vfs || vfunc_no > epc->max_vfs[func_no]))
> > > > +		return;
> > > > +
> > > > +	if (!epc->ops->free_doorbell)
> > > > +		return;
> > > > +
> > > > +	mutex_lock(&epc->lock);
> > > > +	epc->ops->free_doorbell(epc, func_no, vfunc_no);
> > > 
> > > Same as suggested above.
> > > 
> > > > +	mutex_unlock(&epc->lock);
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(pci_epc_free_doorbell);
> > > > +
> > > > +static irqreturn_t pci_epf_generic_doorbell_handler(int irq, void *data)
> > > > +{
> > > > +	struct pci_epf *epf = data;
> > > > +
> > > > +	if (epf->event_ops && epf->event_ops->doorbell)
> > > > +		epf->event_ops->doorbell(epf, irq - epf->virq_base);
> > > 
> > > Same as suggested above.
> > > 
> > > > +
> > > > +	return IRQ_HANDLED;
> > > > +}
> > > > +
> > > > +static void pci_epc_generic_write_msi_msg(struct msi_desc *desc, struct msi_msg *msg)
> > > > +{
> > > > +	struct pci_epc *epc = NULL;
> > > > +	struct class_dev_iter iter;
> > > > +	struct pci_epf *epf;
> > > > +	struct device *dev;
> > > > +
> > > > +	class_dev_iter_init(&iter, pci_epc_class, NULL, NULL);
> > > > +	while ((dev = class_dev_iter_next(&iter))) {
> > > > +		if (dev->parent != desc->dev)
> > > > +			continue;
> > > > +
> > > > +		epc = to_pci_epc(dev);
> > > > +
> > > > +		class_dev_iter_exit(&iter);
> > > > +		break;
> > > > +	}
> > > > +
> > > > +	if (!epc)
> > > > +		return;
> > > > +
> > > > +	/* Only support one EPF for doorbell */
> > > > +	epf = list_first_entry_or_null(&epc->pci_epf, struct pci_epf, list);
> > > > +
> > > 
> > > No need of this newline
> > > 
> > > > +	if (!epf)
> > > > +		return;
> > > > +
> > > > +	if (epf->msg && desc->msi_index < epf->num_msgs)
> > > > +		epf->msg[desc->msi_index] = *msg;
> > > > +}
> > > > +
> > > > +
> > > 
> > > Remove extra newline
> > > 
> > > > +/**
> > > > + * pci_epc_generic_alloc_doorbell() - Common help function. Allocate address space from MSI
> > > > + *                                    controller
> > > > + *
> > > > + * @epc: the EPC device that need doorbell address and data from RC.
> > > > + * @func_no: the physical endpoint function number in the EPC device.
> > > > + * @vfunc_no: the virtual endpoint function number in the physical function.
> > > > + * @num_msgs: the total number of doorbell messages
> > > > + *
> > > 
> > > Same comment as for pci_epc_alloc_doorbell()
> > > 
> > > > + * Remark: use this function only if EPC driver just register one EPC device.
> > > > + *
> > > > + * Return: 0 success, other is failure
> > > > + */
> > > > +int pci_epc_generic_alloc_doorbell(struct pci_epc *epc, u8 func_no, u8 vfunc_no, int num_msgs)
> > > > +{
> > > > +	struct pci_epf *epf;
> > > > +	struct device *dev;
> > > > +	int virq, last;
> > > > +	int ret;
> > > > +	int i;
> > > > +
> > > > +	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_msi_domain_alloc_irqs(dev, num_msgs, pci_epc_generic_write_msi_msg);
> > > > +	if (ret) {
> > > > +		dev_err(dev, "Failed to allocate MSI\n");
> > > > +		return -ENOMEM;
> > > > +	}
> > > > +
> > > > +	last = -1;
> > > > +	for (i = 0; i < num_msgs; i++) {
> > > 
> > > You should iterate over msi_desc as below:
> > > 
> > >         msi_lock_descs(dev);
> > >         msi_for_each_desc(desc, dev, MSI_DESC_ALL) {
> > > 		...
> > > 	}
> > > 	msi_unlock_descs(dev);
> > > 
> > > > +		virq = msi_get_virq(dev, i);
> > > > +		if (i == 0)
> > > > +			epf->virq_base = virq;
> > > > +
> > > > +		ret = request_irq(virq, pci_epf_generic_doorbell_handler, 0,
> > > 
> > > 	request_irq(desc->irq, ...)
> > > 
> > > > +				  kasprintf(GFP_KERNEL, "pci-epc-doorbell%d", i), epf);
> > > > +
> > > > +		if (ret) {
> > > > +			dev_err(dev, "Failed to request doorbell\n");
> > > > +			goto err_free_irq;
> > > > +		}
> > > > +		last = i;
> > > > +	}
> > > > +
> > > > +	return 0;
> > > > +
> > > > +err_free_irq:
> > > > +	for (i = 0; i < last; i++)
> > > > +		kfree(free_irq(epf->virq_base + i, epf));
> > > > +	platform_msi_domain_free_irqs(dev);
> > > > +
> > > > +	return -EINVAL;
> > > 
> > > 	return ret;
> > > 
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(pci_epc_generic_alloc_doorbell);
> > > > +
> > > 
> > > [...]
> > > 
> > > > diff --git a/include/linux/pci-epf.h b/include/linux/pci-epf.h
> > > > index 3f44b6aec4770..485c146a5efe2 100644
> > > > --- a/include/linux/pci-epf.h
> > > > +++ b/include/linux/pci-epf.h
> > > > @@ -79,6 +79,7 @@ struct pci_epc_event_ops {
> > > >  	int (*link_up)(struct pci_epf *epf);
> > > >  	int (*link_down)(struct pci_epf *epf);
> > > >  	int (*bme)(struct pci_epf *epf);
> > > > +	int (*doorbell)(struct pci_epf *epf, int index);
> > > 
> > > kdoc missing.
> > > 
> > > >  };
> > > >  
> > > >  /**
> > > > @@ -180,6 +181,9 @@ 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_msgs;
> > > 
> > > num_db
> > > 
> > > You also need to add kdoc for each new member.
> > > 
> > > - Mani
> > > 
> > > -- 
> > > மணிவண்ணன் சதாசிவம்
> 
> -- 
> மணிவண்ணன் சதாசிவம்

On Thu, Oct 19, 2023 at 12:00:22PM -0400, Frank Li wrote:
> On Thu, Oct 19, 2023 at 08:34:41PM +0530, Manivannan Sadhasivam wrote:
> > On Tue, Oct 17, 2023 at 02:55:57PM -0400, Frank Li wrote:
> > > On Wed, Oct 18, 2023 at 12:07:22AM +0530, Manivannan Sadhasivam wrote:
> > > > On Mon, Sep 11, 2023 at 06:09:16PM -0400, Frank Li wrote:
> > > > > This commit introduces a common method for sending messages from the Root
> > > > > Complex (RC) to the Endpoint (EP) by utilizing the platform MSI interrupt
> > > > > controller, such as ARM GIC, as an EP doorbell. Maps the memory assigned
> > > > > for the BAR region by the PCI host to the message address of the platform
> > > > > MSI interrupt controller in the PCI EP. As a result, when the PCI RC writes
> > > > 
> > > > "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."
> > > > 
> > > > > to the BAR region, it triggers an IRQ at the EP. This implementation serves
> > > > > as a common method for all endpoint function drivers.
> > > > > 
> > > > > However, it currently supports only one EP physical function due to
> > > > > limitations in ARM MSI/IMS readiness.
> > > > > 
> > > > > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > > > > ---
> > > > >  drivers/pci/endpoint/pci-epc-core.c | 192 ++++++++++++++++++++++++++++
> > > > >  drivers/pci/endpoint/pci-epf-core.c |  44 +++++++
> > > > >  include/linux/pci-epc.h             |   6 +
> > > > >  include/linux/pci-epf.h             |   7 +
> > > > >  4 files changed, 249 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/pci/endpoint/pci-epc-core.c b/drivers/pci/endpoint/pci-epc-core.c
> > > > > index 5a4a8b0be6262..d336a99c6a94f 100644
> > > > > --- a/drivers/pci/endpoint/pci-epc-core.c
> > > > > +++ b/drivers/pci/endpoint/pci-epc-core.c
> > > > > @@ -10,6 +10,7 @@
> > > > >  #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>
> > > > > @@ -783,6 +784,197 @@ void pci_epc_bme_notify(struct pci_epc *epc)
> > > > >  }
> > > > >  EXPORT_SYMBOL_GPL(pci_epc_bme_notify);
> > > > >  
> > > > > +/**
> > > > > + * pci_epc_alloc_doorbell() - alloc an address space to let RC trigger EP side IRQ by write data to
> > > > > + *			      the space.
> > > > 
> > > > "Allocate platform specific doorbell IRQs to be used by the host to trigger
> > > > doorbells on EP."
> > > > 
> > > > > + *
> > > > > + * @epc: the EPC device that need doorbell address and data from RC.
> > > > 
> > > > EPC device for which the doorbell needs to be allocated
> > > > 
> > > > > + * @func_no: the physical endpoint function number in the EPC device.
> > > > > + * @vfunc_no: the virtual endpoint function number in the physical function.
> > > > > + * @num_msgs: the total number of doorbell messages
> > > > 
> > > > s/num_msgs/num_db
> > > > 
> > > > > + *
> > > > > + * Return: 0 success, other is failure
> > > > > + */
> > > > > +int pci_epc_alloc_doorbell(struct pci_epc *epc, u8 func_no, u8 vfunc_no, int num_msgs)
> > > > > +{
> > > > > +	int ret;
> > > > > +
> > > > > +	if (IS_ERR_OR_NULL(epc) || func_no >= epc->max_functions)
> > > > > +		return -EINVAL;
> > > > > +
> > > > > +	if (vfunc_no > 0 && (!epc->max_vfs || vfunc_no > epc->max_vfs[func_no]))
> > > > > +		return -EINVAL;
> > > > > +
> > > > > +	if (!epc->ops->alloc_doorbell)
> > > > > +		return 0;
> > > > 
> > > > You mentioned 0 is a success. So if there is no callback, you want to return
> > > > success?
> > > > 
> > > > > +
> > > > > +	mutex_lock(&epc->lock);
> > > > > +	ret = epc->ops->alloc_doorbell(epc, func_no, vfunc_no, num_msgs);
> > > > 
> > > > Why can't you just call the generic function here and in other places instead of
> > > > implementing callbacks? I do not see a necessity for EPC specific callbacks. If
> > > > there is one, please specify.
> > > 
> > > 1. Refer v1 your comments.
> > > https://lore.kernel.org/imx/20230906145227.GC5930@thinkpad/
> > 
> > I do not find where I suggested the callback approach.
> 
> 	> > > If that, Each EPF driver need do duplicate work. 
> 	> > > 
> 	> > 
> 	> > Yes, and that's how it should be. EPF core has no job in supplying the of_node.
> 	> > It is the responsibility of the EPF drivers as they depend on OF for platform
> 	> > support.
> 	> 
> 	> EPF driver still not depend on OF. such pci-epf-test, which was probed by
> 	> configfs.
> 	> 
> 
> 	Hmm, yeah. Then it should be part of the EPC driver.
> 
> 	Sorry for the confusion.
> 
> Here, all "EPF" should be "EPC". The key problem is of_node. EPC core have
> not of_node, EPC core's parent driver (like dwc-ep driver) have of_node. 
> 
> pci_epc_generic_alloc_doorbell(dev), dev is probed by platform driver, such
> as dwc-ep, which have of_node,  EPC core will create child device.
> 
> dwc-ep device
>  |- epc core device
> 
> we can direct call pci_epc_generic_alloc_doorbell(epc->parent) here.
> 
> I may miss understand what your means. I think you want to dwc-ep
> (with of_node) handle these alloc functions. 
> 

My comment was to have just one function definition. But looking at it again, I
think it is better to move all the (alloc, free, write_msg) definitions to
dwc-ep, since the contents of those functions are not EPC core specific.

In the EPC core, you can still have the callbacks specific to each EPC. This
also solves your of_node problem.

- Mani

> > 
> > > 2. Maybe some ep controller have built-in doorbell support. Write to some
> > > address to trigger doorbell irq.
> > > 
> > 
> > We will handle it whenever such EP controllers arrive. Until then, let's keep it
> > simple.
> > 
> > - Mani
> > 
> > > Frank
> > > 
> > > > 
> > > > > +	mutex_unlock(&epc->lock);
> > > > > +
> > > > > +	return ret;
> > > > > +}
> > > > > +EXPORT_SYMBOL_GPL(pci_epc_alloc_doorbell);
> > > > > +
> > > > > +/**
> > > > > + * pci_epc_free_doorbell() - free resource allocated by pci_epc_alloc_doorbell()
> > > > > + *
> > > > > + * @epc: the EPC device that need doorbell address and data from RC.
> > > > 
> > > > Same as above.
> > > > 
> > > > > + * @func_no: the physical endpoint function number in the EPC device.
> > > > > + * @vfunc_no: the virtual endpoint function number in the physical function.
> > > > > + *
> > > > > + * Return: 0 success, other is failure
> > > > > + */
> > > > > +void pci_epc_free_doorbell(struct pci_epc *epc, u8 func_no, u8 vfunc_no)
> > > > > +{
> > > > > +	if (IS_ERR_OR_NULL(epc) || func_no >= epc->max_functions)
> > > > > +		return;
> > > > > +
> > > > > +	if (vfunc_no > 0 && (!epc->max_vfs || vfunc_no > epc->max_vfs[func_no]))
> > > > > +		return;
> > > > > +
> > > > > +	if (!epc->ops->free_doorbell)
> > > > > +		return;
> > > > > +
> > > > > +	mutex_lock(&epc->lock);
> > > > > +	epc->ops->free_doorbell(epc, func_no, vfunc_no);
> > > > 
> > > > Same as suggested above.
> > > > 
> > > > > +	mutex_unlock(&epc->lock);
> > > > > +}
> > > > > +EXPORT_SYMBOL_GPL(pci_epc_free_doorbell);
> > > > > +
> > > > > +static irqreturn_t pci_epf_generic_doorbell_handler(int irq, void *data)
> > > > > +{
> > > > > +	struct pci_epf *epf = data;
> > > > > +
> > > > > +	if (epf->event_ops && epf->event_ops->doorbell)
> > > > > +		epf->event_ops->doorbell(epf, irq - epf->virq_base);
> > > > 
> > > > Same as suggested above.
> > > > 
> > > > > +
> > > > > +	return IRQ_HANDLED;
> > > > > +}
> > > > > +
> > > > > +static void pci_epc_generic_write_msi_msg(struct msi_desc *desc, struct msi_msg *msg)
> > > > > +{
> > > > > +	struct pci_epc *epc = NULL;
> > > > > +	struct class_dev_iter iter;
> > > > > +	struct pci_epf *epf;
> > > > > +	struct device *dev;
> > > > > +
> > > > > +	class_dev_iter_init(&iter, pci_epc_class, NULL, NULL);
> > > > > +	while ((dev = class_dev_iter_next(&iter))) {
> > > > > +		if (dev->parent != desc->dev)
> > > > > +			continue;
> > > > > +
> > > > > +		epc = to_pci_epc(dev);
> > > > > +
> > > > > +		class_dev_iter_exit(&iter);
> > > > > +		break;
> > > > > +	}
> > > > > +
> > > > > +	if (!epc)
> > > > > +		return;
> > > > > +
> > > > > +	/* Only support one EPF for doorbell */
> > > > > +	epf = list_first_entry_or_null(&epc->pci_epf, struct pci_epf, list);
> > > > > +
> > > > 
> > > > No need of this newline
> > > > 
> > > > > +	if (!epf)
> > > > > +		return;
> > > > > +
> > > > > +	if (epf->msg && desc->msi_index < epf->num_msgs)
> > > > > +		epf->msg[desc->msi_index] = *msg;
> > > > > +}
> > > > > +
> > > > > +
> > > > 
> > > > Remove extra newline
> > > > 
> > > > > +/**
> > > > > + * pci_epc_generic_alloc_doorbell() - Common help function. Allocate address space from MSI
> > > > > + *                                    controller
> > > > > + *
> > > > > + * @epc: the EPC device that need doorbell address and data from RC.
> > > > > + * @func_no: the physical endpoint function number in the EPC device.
> > > > > + * @vfunc_no: the virtual endpoint function number in the physical function.
> > > > > + * @num_msgs: the total number of doorbell messages
> > > > > + *
> > > > 
> > > > Same comment as for pci_epc_alloc_doorbell()
> > > > 
> > > > > + * Remark: use this function only if EPC driver just register one EPC device.
> > > > > + *
> > > > > + * Return: 0 success, other is failure
> > > > > + */
> > > > > +int pci_epc_generic_alloc_doorbell(struct pci_epc *epc, u8 func_no, u8 vfunc_no, int num_msgs)
> > > > > +{
> > > > > +	struct pci_epf *epf;
> > > > > +	struct device *dev;
> > > > > +	int virq, last;
> > > > > +	int ret;
> > > > > +	int i;
> > > > > +
> > > > > +	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_msi_domain_alloc_irqs(dev, num_msgs, pci_epc_generic_write_msi_msg);
> > > > > +	if (ret) {
> > > > > +		dev_err(dev, "Failed to allocate MSI\n");
> > > > > +		return -ENOMEM;
> > > > > +	}
> > > > > +
> > > > > +	last = -1;
> > > > > +	for (i = 0; i < num_msgs; i++) {
> > > > 
> > > > You should iterate over msi_desc as below:
> > > > 
> > > >         msi_lock_descs(dev);
> > > >         msi_for_each_desc(desc, dev, MSI_DESC_ALL) {
> > > > 		...
> > > > 	}
> > > > 	msi_unlock_descs(dev);
> > > > 
> > > > > +		virq = msi_get_virq(dev, i);
> > > > > +		if (i == 0)
> > > > > +			epf->virq_base = virq;
> > > > > +
> > > > > +		ret = request_irq(virq, pci_epf_generic_doorbell_handler, 0,
> > > > 
> > > > 	request_irq(desc->irq, ...)
> > > > 
> > > > > +				  kasprintf(GFP_KERNEL, "pci-epc-doorbell%d", i), epf);
> > > > > +
> > > > > +		if (ret) {
> > > > > +			dev_err(dev, "Failed to request doorbell\n");
> > > > > +			goto err_free_irq;
> > > > > +		}
> > > > > +		last = i;
> > > > > +	}
> > > > > +
> > > > > +	return 0;
> > > > > +
> > > > > +err_free_irq:
> > > > > +	for (i = 0; i < last; i++)
> > > > > +		kfree(free_irq(epf->virq_base + i, epf));
> > > > > +	platform_msi_domain_free_irqs(dev);
> > > > > +
> > > > > +	return -EINVAL;
> > > > 
> > > > 	return ret;
> > > > 
> > > > > +}
> > > > > +EXPORT_SYMBOL_GPL(pci_epc_generic_alloc_doorbell);
> > > > > +
> > > > 
> > > > [...]
> > > > 
> > > > > diff --git a/include/linux/pci-epf.h b/include/linux/pci-epf.h
> > > > > index 3f44b6aec4770..485c146a5efe2 100644
> > > > > --- a/include/linux/pci-epf.h
> > > > > +++ b/include/linux/pci-epf.h
> > > > > @@ -79,6 +79,7 @@ struct pci_epc_event_ops {
> > > > >  	int (*link_up)(struct pci_epf *epf);
> > > > >  	int (*link_down)(struct pci_epf *epf);
> > > > >  	int (*bme)(struct pci_epf *epf);
> > > > > +	int (*doorbell)(struct pci_epf *epf, int index);
> > > > 
> > > > kdoc missing.
> > > > 
> > > > >  };
> > > > >  
> > > > >  /**
> > > > > @@ -180,6 +181,9 @@ 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_msgs;
> > > > 
> > > > num_db
> > > > 
> > > > You also need to add kdoc for each new member.
> > > > 
> > > > - Mani
> > > > 
> > > > -- 
> > > > மணிவண்ணன் சதாசிவம்
> > 
> > -- 
> > மணிவண்ணன் சதாசிவம்

On Thu, Oct 19, 2023 at 10:53:47PM +0530, Manivannan Sadhasivam wrote:
> On Thu, Oct 19, 2023 at 12:00:22PM -0400, Frank Li wrote:
> > On Thu, Oct 19, 2023 at 08:34:41PM +0530, Manivannan Sadhasivam wrote:
> > > On Tue, Oct 17, 2023 at 02:55:57PM -0400, Frank Li wrote:
> > > > On Wed, Oct 18, 2023 at 12:07:22AM +0530, Manivannan Sadhasivam wrote:
> > > > > On Mon, Sep 11, 2023 at 06:09:16PM -0400, Frank Li wrote:
> > > > > > This commit introduces a common method for sending messages from the Root
> > > > > > Complex (RC) to the Endpoint (EP) by utilizing the platform MSI interrupt
> > > > > > controller, such as ARM GIC, as an EP doorbell. Maps the memory assigned
> > > > > > for the BAR region by the PCI host to the message address of the platform
> > > > > > MSI interrupt controller in the PCI EP. As a result, when the PCI RC writes
> > > > > 
> > > > > "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."
> > > > > 
> > > > > > to the BAR region, it triggers an IRQ at the EP. This implementation serves
> > > > > > as a common method for all endpoint function drivers.
> > > > > > 
> > > > > > However, it currently supports only one EP physical function due to
> > > > > > limitations in ARM MSI/IMS readiness.
> > > > > > 
> > > > > > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > > > > > ---
> > > > > >  drivers/pci/endpoint/pci-epc-core.c | 192 ++++++++++++++++++++++++++++
> > > > > >  drivers/pci/endpoint/pci-epf-core.c |  44 +++++++
> > > > > >  include/linux/pci-epc.h             |   6 +
> > > > > >  include/linux/pci-epf.h             |   7 +
> > > > > >  4 files changed, 249 insertions(+)
> > > > > > 
> > > > > > diff --git a/drivers/pci/endpoint/pci-epc-core.c b/drivers/pci/endpoint/pci-epc-core.c
> > > > > > index 5a4a8b0be6262..d336a99c6a94f 100644
> > > > > > --- a/drivers/pci/endpoint/pci-epc-core.c
> > > > > > +++ b/drivers/pci/endpoint/pci-epc-core.c
> > > > > > @@ -10,6 +10,7 @@
> > > > > >  #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>
> > > > > > @@ -783,6 +784,197 @@ void pci_epc_bme_notify(struct pci_epc *epc)
> > > > > >  }
> > > > > >  EXPORT_SYMBOL_GPL(pci_epc_bme_notify);
> > > > > >  
> > > > > > +/**
> > > > > > + * pci_epc_alloc_doorbell() - alloc an address space to let RC trigger EP side IRQ by write data to
> > > > > > + *			      the space.
> > > > > 
> > > > > "Allocate platform specific doorbell IRQs to be used by the host to trigger
> > > > > doorbells on EP."
> > > > > 
> > > > > > + *
> > > > > > + * @epc: the EPC device that need doorbell address and data from RC.
> > > > > 
> > > > > EPC device for which the doorbell needs to be allocated
> > > > > 
> > > > > > + * @func_no: the physical endpoint function number in the EPC device.
> > > > > > + * @vfunc_no: the virtual endpoint function number in the physical function.
> > > > > > + * @num_msgs: the total number of doorbell messages
> > > > > 
> > > > > s/num_msgs/num_db
> > > > > 
> > > > > > + *
> > > > > > + * Return: 0 success, other is failure
> > > > > > + */
> > > > > > +int pci_epc_alloc_doorbell(struct pci_epc *epc, u8 func_no, u8 vfunc_no, int num_msgs)
> > > > > > +{
> > > > > > +	int ret;
> > > > > > +
> > > > > > +	if (IS_ERR_OR_NULL(epc) || func_no >= epc->max_functions)
> > > > > > +		return -EINVAL;
> > > > > > +
> > > > > > +	if (vfunc_no > 0 && (!epc->max_vfs || vfunc_no > epc->max_vfs[func_no]))
> > > > > > +		return -EINVAL;
> > > > > > +
> > > > > > +	if (!epc->ops->alloc_doorbell)
> > > > > > +		return 0;
> > > > > 
> > > > > You mentioned 0 is a success. So if there is no callback, you want to return
> > > > > success?
> > > > > 
> > > > > > +
> > > > > > +	mutex_lock(&epc->lock);
> > > > > > +	ret = epc->ops->alloc_doorbell(epc, func_no, vfunc_no, num_msgs);
> > > > > 
> > > > > Why can't you just call the generic function here and in other places instead of
> > > > > implementing callbacks? I do not see a necessity for EPC specific callbacks. If
> > > > > there is one, please specify.
> > > > 
> > > > 1. Refer v1 your comments.
> > > > https://lore.kernel.org/imx/20230906145227.GC5930@thinkpad/
> > > 
> > > I do not find where I suggested the callback approach.
> > 
> > 	> > > If that, Each EPF driver need do duplicate work. 
> > 	> > > 
> > 	> > 
> > 	> > Yes, and that's how it should be. EPF core has no job in supplying the of_node.
> > 	> > It is the responsibility of the EPF drivers as they depend on OF for platform
> > 	> > support.
> > 	> 
> > 	> EPF driver still not depend on OF. such pci-epf-test, which was probed by
> > 	> configfs.
> > 	> 
> > 
> > 	Hmm, yeah. Then it should be part of the EPC driver.
> > 
> > 	Sorry for the confusion.
> > 
> > Here, all "EPF" should be "EPC". The key problem is of_node. EPC core have
> > not of_node, EPC core's parent driver (like dwc-ep driver) have of_node. 
> > 
> > pci_epc_generic_alloc_doorbell(dev), dev is probed by platform driver, such
> > as dwc-ep, which have of_node,  EPC core will create child device.
> > 
> > dwc-ep device
> >  |- epc core device
> > 
> > we can direct call pci_epc_generic_alloc_doorbell(epc->parent) here.
> > 
> > I may miss understand what your means. I think you want to dwc-ep
> > (with of_node) handle these alloc functions. 
> > 
> 
> My comment was to have just one function definition. But looking at it again, I
> think it is better to move all the (alloc, free, write_msg) definitions to
> dwc-ep, since the contents of those functions are not EPC core specific.

There are still problem. (alloc, free, write_msg) is quite common for all
controller and the system with MSI.

If move these into dwc-ep,  cdns or other controller have to duplicate 
these codes.

If you think it is not EPC core specific, how about create new help files?

Frank

> 
> In the EPC core, you can still have the callbacks specific to each EPC. This
> also solves your of_node problem.
> 
> - Mani
> 
> > > 
> > > > 2. Maybe some ep controller have built-in doorbell support. Write to some
> > > > address to trigger doorbell irq.
> > > > 
> > > 
> > > We will handle it whenever such EP controllers arrive. Until then, let's keep it
> > > simple.
> > > 
> > > - Mani
> > > 
> > > > Frank
> > > > 
> > > > > 
> > > > > > +	mutex_unlock(&epc->lock);
> > > > > > +
> > > > > > +	return ret;
> > > > > > +}
> > > > > > +EXPORT_SYMBOL_GPL(pci_epc_alloc_doorbell);
> > > > > > +
> > > > > > +/**
> > > > > > + * pci_epc_free_doorbell() - free resource allocated by pci_epc_alloc_doorbell()
> > > > > > + *
> > > > > > + * @epc: the EPC device that need doorbell address and data from RC.
> > > > > 
> > > > > Same as above.
> > > > > 
> > > > > > + * @func_no: the physical endpoint function number in the EPC device.
> > > > > > + * @vfunc_no: the virtual endpoint function number in the physical function.
> > > > > > + *
> > > > > > + * Return: 0 success, other is failure
> > > > > > + */
> > > > > > +void pci_epc_free_doorbell(struct pci_epc *epc, u8 func_no, u8 vfunc_no)
> > > > > > +{
> > > > > > +	if (IS_ERR_OR_NULL(epc) || func_no >= epc->max_functions)
> > > > > > +		return;
> > > > > > +
> > > > > > +	if (vfunc_no > 0 && (!epc->max_vfs || vfunc_no > epc->max_vfs[func_no]))
> > > > > > +		return;
> > > > > > +
> > > > > > +	if (!epc->ops->free_doorbell)
> > > > > > +		return;
> > > > > > +
> > > > > > +	mutex_lock(&epc->lock);
> > > > > > +	epc->ops->free_doorbell(epc, func_no, vfunc_no);
> > > > > 
> > > > > Same as suggested above.
> > > > > 
> > > > > > +	mutex_unlock(&epc->lock);
> > > > > > +}
> > > > > > +EXPORT_SYMBOL_GPL(pci_epc_free_doorbell);
> > > > > > +
> > > > > > +static irqreturn_t pci_epf_generic_doorbell_handler(int irq, void *data)
> > > > > > +{
> > > > > > +	struct pci_epf *epf = data;
> > > > > > +
> > > > > > +	if (epf->event_ops && epf->event_ops->doorbell)
> > > > > > +		epf->event_ops->doorbell(epf, irq - epf->virq_base);
> > > > > 
> > > > > Same as suggested above.
> > > > > 
> > > > > > +
> > > > > > +	return IRQ_HANDLED;
> > > > > > +}
> > > > > > +
> > > > > > +static void pci_epc_generic_write_msi_msg(struct msi_desc *desc, struct msi_msg *msg)
> > > > > > +{
> > > > > > +	struct pci_epc *epc = NULL;
> > > > > > +	struct class_dev_iter iter;
> > > > > > +	struct pci_epf *epf;
> > > > > > +	struct device *dev;
> > > > > > +
> > > > > > +	class_dev_iter_init(&iter, pci_epc_class, NULL, NULL);
> > > > > > +	while ((dev = class_dev_iter_next(&iter))) {
> > > > > > +		if (dev->parent != desc->dev)
> > > > > > +			continue;
> > > > > > +
> > > > > > +		epc = to_pci_epc(dev);
> > > > > > +
> > > > > > +		class_dev_iter_exit(&iter);
> > > > > > +		break;
> > > > > > +	}
> > > > > > +
> > > > > > +	if (!epc)
> > > > > > +		return;
> > > > > > +
> > > > > > +	/* Only support one EPF for doorbell */
> > > > > > +	epf = list_first_entry_or_null(&epc->pci_epf, struct pci_epf, list);
> > > > > > +
> > > > > 
> > > > > No need of this newline
> > > > > 
> > > > > > +	if (!epf)
> > > > > > +		return;
> > > > > > +
> > > > > > +	if (epf->msg && desc->msi_index < epf->num_msgs)
> > > > > > +		epf->msg[desc->msi_index] = *msg;
> > > > > > +}
> > > > > > +
> > > > > > +
> > > > > 
> > > > > Remove extra newline
> > > > > 
> > > > > > +/**
> > > > > > + * pci_epc_generic_alloc_doorbell() - Common help function. Allocate address space from MSI
> > > > > > + *                                    controller
> > > > > > + *
> > > > > > + * @epc: the EPC device that need doorbell address and data from RC.
> > > > > > + * @func_no: the physical endpoint function number in the EPC device.
> > > > > > + * @vfunc_no: the virtual endpoint function number in the physical function.
> > > > > > + * @num_msgs: the total number of doorbell messages
> > > > > > + *
> > > > > 
> > > > > Same comment as for pci_epc_alloc_doorbell()
> > > > > 
> > > > > > + * Remark: use this function only if EPC driver just register one EPC device.
> > > > > > + *
> > > > > > + * Return: 0 success, other is failure
> > > > > > + */
> > > > > > +int pci_epc_generic_alloc_doorbell(struct pci_epc *epc, u8 func_no, u8 vfunc_no, int num_msgs)
> > > > > > +{
> > > > > > +	struct pci_epf *epf;
> > > > > > +	struct device *dev;
> > > > > > +	int virq, last;
> > > > > > +	int ret;
> > > > > > +	int i;
> > > > > > +
> > > > > > +	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_msi_domain_alloc_irqs(dev, num_msgs, pci_epc_generic_write_msi_msg);
> > > > > > +	if (ret) {
> > > > > > +		dev_err(dev, "Failed to allocate MSI\n");
> > > > > > +		return -ENOMEM;
> > > > > > +	}
> > > > > > +
> > > > > > +	last = -1;
> > > > > > +	for (i = 0; i < num_msgs; i++) {
> > > > > 
> > > > > You should iterate over msi_desc as below:
> > > > > 
> > > > >         msi_lock_descs(dev);
> > > > >         msi_for_each_desc(desc, dev, MSI_DESC_ALL) {
> > > > > 		...
> > > > > 	}
> > > > > 	msi_unlock_descs(dev);
> > > > > 
> > > > > > +		virq = msi_get_virq(dev, i);
> > > > > > +		if (i == 0)
> > > > > > +			epf->virq_base = virq;
> > > > > > +
> > > > > > +		ret = request_irq(virq, pci_epf_generic_doorbell_handler, 0,
> > > > > 
> > > > > 	request_irq(desc->irq, ...)
> > > > > 
> > > > > > +				  kasprintf(GFP_KERNEL, "pci-epc-doorbell%d", i), epf);
> > > > > > +
> > > > > > +		if (ret) {
> > > > > > +			dev_err(dev, "Failed to request doorbell\n");
> > > > > > +			goto err_free_irq;
> > > > > > +		}
> > > > > > +		last = i;
> > > > > > +	}
> > > > > > +
> > > > > > +	return 0;
> > > > > > +
> > > > > > +err_free_irq:
> > > > > > +	for (i = 0; i < last; i++)
> > > > > > +		kfree(free_irq(epf->virq_base + i, epf));
> > > > > > +	platform_msi_domain_free_irqs(dev);
> > > > > > +
> > > > > > +	return -EINVAL;
> > > > > 
> > > > > 	return ret;
> > > > > 
> > > > > > +}
> > > > > > +EXPORT_SYMBOL_GPL(pci_epc_generic_alloc_doorbell);
> > > > > > +
> > > > > 
> > > > > [...]
> > > > > 
> > > > > > diff --git a/include/linux/pci-epf.h b/include/linux/pci-epf.h
> > > > > > index 3f44b6aec4770..485c146a5efe2 100644
> > > > > > --- a/include/linux/pci-epf.h
> > > > > > +++ b/include/linux/pci-epf.h
> > > > > > @@ -79,6 +79,7 @@ struct pci_epc_event_ops {
> > > > > >  	int (*link_up)(struct pci_epf *epf);
> > > > > >  	int (*link_down)(struct pci_epf *epf);
> > > > > >  	int (*bme)(struct pci_epf *epf);
> > > > > > +	int (*doorbell)(struct pci_epf *epf, int index);
> > > > > 
> > > > > kdoc missing.
> > > > > 
> > > > > >  };
> > > > > >  
> > > > > >  /**
> > > > > > @@ -180,6 +181,9 @@ 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_msgs;
> > > > > 
> > > > > num_db
> > > > > 
> > > > > You also need to add kdoc for each new member.
> > > > > 
> > > > > - Mani
> > > > > 
> > > > > -- 
> > > > > மணிவண்ணன் சதாசிவம்
> > > 
> > > -- 
> > > மணிவண்ணன் சதாசிவம்
> 
> -- 
> மணிவண்ணன் சதாசிவம்

On Thu, Oct 19, 2023 at 02:11:22PM -0400, Frank Li wrote:
> On Thu, Oct 19, 2023 at 10:53:47PM +0530, Manivannan Sadhasivam wrote:
> > On Thu, Oct 19, 2023 at 12:00:22PM -0400, Frank Li wrote:
> > > On Thu, Oct 19, 2023 at 08:34:41PM +0530, Manivannan Sadhasivam wrote:
> > > > On Tue, Oct 17, 2023 at 02:55:57PM -0400, Frank Li wrote:
> > > > > On Wed, Oct 18, 2023 at 12:07:22AM +0530, Manivannan Sadhasivam wrote:
> > > > > > On Mon, Sep 11, 2023 at 06:09:16PM -0400, Frank Li wrote:
> > > > > > > This commit introduces a common method for sending messages from the Root
> > > > > > > Complex (RC) to the Endpoint (EP) by utilizing the platform MSI interrupt
> > > > > > > controller, such as ARM GIC, as an EP doorbell. Maps the memory assigned
> > > > > > > for the BAR region by the PCI host to the message address of the platform
> > > > > > > MSI interrupt controller in the PCI EP. As a result, when the PCI RC writes
> > > > > > 
> > > > > > "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."
> > > > > > 
> > > > > > > to the BAR region, it triggers an IRQ at the EP. This implementation serves
> > > > > > > as a common method for all endpoint function drivers.
> > > > > > > 
> > > > > > > However, it currently supports only one EP physical function due to
> > > > > > > limitations in ARM MSI/IMS readiness.
> > > > > > > 
> > > > > > > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > > > > > > ---
> > > > > > >  drivers/pci/endpoint/pci-epc-core.c | 192 ++++++++++++++++++++++++++++
> > > > > > >  drivers/pci/endpoint/pci-epf-core.c |  44 +++++++
> > > > > > >  include/linux/pci-epc.h             |   6 +
> > > > > > >  include/linux/pci-epf.h             |   7 +
> > > > > > >  4 files changed, 249 insertions(+)
> > > > > > > 
> > > > > > > diff --git a/drivers/pci/endpoint/pci-epc-core.c b/drivers/pci/endpoint/pci-epc-core.c
> > > > > > > index 5a4a8b0be6262..d336a99c6a94f 100644
> > > > > > > --- a/drivers/pci/endpoint/pci-epc-core.c
> > > > > > > +++ b/drivers/pci/endpoint/pci-epc-core.c
> > > > > > > @@ -10,6 +10,7 @@
> > > > > > >  #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>
> > > > > > > @@ -783,6 +784,197 @@ void pci_epc_bme_notify(struct pci_epc *epc)
> > > > > > >  }
> > > > > > >  EXPORT_SYMBOL_GPL(pci_epc_bme_notify);
> > > > > > >  
> > > > > > > +/**
> > > > > > > + * pci_epc_alloc_doorbell() - alloc an address space to let RC trigger EP side IRQ by write data to
> > > > > > > + *			      the space.
> > > > > > 
> > > > > > "Allocate platform specific doorbell IRQs to be used by the host to trigger
> > > > > > doorbells on EP."
> > > > > > 
> > > > > > > + *
> > > > > > > + * @epc: the EPC device that need doorbell address and data from RC.
> > > > > > 
> > > > > > EPC device for which the doorbell needs to be allocated
> > > > > > 
> > > > > > > + * @func_no: the physical endpoint function number in the EPC device.
> > > > > > > + * @vfunc_no: the virtual endpoint function number in the physical function.
> > > > > > > + * @num_msgs: the total number of doorbell messages
> > > > > > 
> > > > > > s/num_msgs/num_db
> > > > > > 
> > > > > > > + *
> > > > > > > + * Return: 0 success, other is failure
> > > > > > > + */
> > > > > > > +int pci_epc_alloc_doorbell(struct pci_epc *epc, u8 func_no, u8 vfunc_no, int num_msgs)
> > > > > > > +{
> > > > > > > +	int ret;
> > > > > > > +
> > > > > > > +	if (IS_ERR_OR_NULL(epc) || func_no >= epc->max_functions)
> > > > > > > +		return -EINVAL;
> > > > > > > +
> > > > > > > +	if (vfunc_no > 0 && (!epc->max_vfs || vfunc_no > epc->max_vfs[func_no]))
> > > > > > > +		return -EINVAL;
> > > > > > > +
> > > > > > > +	if (!epc->ops->alloc_doorbell)
> > > > > > > +		return 0;
> > > > > > 
> > > > > > You mentioned 0 is a success. So if there is no callback, you want to return
> > > > > > success?
> > > > > > 
> > > > > > > +
> > > > > > > +	mutex_lock(&epc->lock);
> > > > > > > +	ret = epc->ops->alloc_doorbell(epc, func_no, vfunc_no, num_msgs);
> > > > > > 
> > > > > > Why can't you just call the generic function here and in other places instead of
> > > > > > implementing callbacks? I do not see a necessity for EPC specific callbacks. If
> > > > > > there is one, please specify.
> > > > > 
> > > > > 1. Refer v1 your comments.
> > > > > https://lore.kernel.org/imx/20230906145227.GC5930@thinkpad/
> > > > 
> > > > I do not find where I suggested the callback approach.
> > > 
> > > 	> > > If that, Each EPF driver need do duplicate work. 
> > > 	> > > 
> > > 	> > 
> > > 	> > Yes, and that's how it should be. EPF core has no job in supplying the of_node.
> > > 	> > It is the responsibility of the EPF drivers as they depend on OF for platform
> > > 	> > support.
> > > 	> 
> > > 	> EPF driver still not depend on OF. such pci-epf-test, which was probed by
> > > 	> configfs.
> > > 	> 
> > > 
> > > 	Hmm, yeah. Then it should be part of the EPC driver.
> > > 
> > > 	Sorry for the confusion.
> > > 
> > > Here, all "EPF" should be "EPC". The key problem is of_node. EPC core have
> > > not of_node, EPC core's parent driver (like dwc-ep driver) have of_node. 
> > > 
> > > pci_epc_generic_alloc_doorbell(dev), dev is probed by platform driver, such
> > > as dwc-ep, which have of_node,  EPC core will create child device.
> > > 
> > > dwc-ep device
> > >  |- epc core device
> > > 
> > > we can direct call pci_epc_generic_alloc_doorbell(epc->parent) here.
> > > 
> > > I may miss understand what your means. I think you want to dwc-ep
> > > (with of_node) handle these alloc functions. 
> > > 
> > 
> > My comment was to have just one function definition. But looking at it again, I
> > think it is better to move all the (alloc, free, write_msg) definitions to
> > dwc-ep, since the contents of those functions are not EPC core specific.
> 
> There are still problem. (alloc, free, write_msg) is quite common for all
> controller and the system with MSI.
> 
> If move these into dwc-ep,  cdns or other controller have to duplicate 
> these codes.
> 
> If you think it is not EPC core specific, how about create new help files?
> 

Hmm, that sounds good to me. I think the best place would be:
drivers/pci/endpoint/pci-ep-msi.c

Reason is, we cannot have this generic code under drivers/pci/controller/ as it
is not a standalone PCI controller but a platform MSI controller. So having it
under pci/endpoint/ makes much sense to me.

And this is not specific to EPF drivers as well, so we cannot have it under
pci/endpoint/functions/.

- Mani

> Frank
> 
> > 
> > In the EPC core, you can still have the callbacks specific to each EPC. This
> > also solves your of_node problem.
> > 
> > - Mani
> > 
> > > > 
> > > > > 2. Maybe some ep controller have built-in doorbell support. Write to some
> > > > > address to trigger doorbell irq.
> > > > > 
> > > > 
> > > > We will handle it whenever such EP controllers arrive. Until then, let's keep it
> > > > simple.
> > > > 
> > > > - Mani
> > > > 
> > > > > Frank
> > > > > 
> > > > > > 
> > > > > > > +	mutex_unlock(&epc->lock);
> > > > > > > +
> > > > > > > +	return ret;
> > > > > > > +}
> > > > > > > +EXPORT_SYMBOL_GPL(pci_epc_alloc_doorbell);
> > > > > > > +
> > > > > > > +/**
> > > > > > > + * pci_epc_free_doorbell() - free resource allocated by pci_epc_alloc_doorbell()
> > > > > > > + *
> > > > > > > + * @epc: the EPC device that need doorbell address and data from RC.
> > > > > > 
> > > > > > Same as above.
> > > > > > 
> > > > > > > + * @func_no: the physical endpoint function number in the EPC device.
> > > > > > > + * @vfunc_no: the virtual endpoint function number in the physical function.
> > > > > > > + *
> > > > > > > + * Return: 0 success, other is failure
> > > > > > > + */
> > > > > > > +void pci_epc_free_doorbell(struct pci_epc *epc, u8 func_no, u8 vfunc_no)
> > > > > > > +{
> > > > > > > +	if (IS_ERR_OR_NULL(epc) || func_no >= epc->max_functions)
> > > > > > > +		return;
> > > > > > > +
> > > > > > > +	if (vfunc_no > 0 && (!epc->max_vfs || vfunc_no > epc->max_vfs[func_no]))
> > > > > > > +		return;
> > > > > > > +
> > > > > > > +	if (!epc->ops->free_doorbell)
> > > > > > > +		return;
> > > > > > > +
> > > > > > > +	mutex_lock(&epc->lock);
> > > > > > > +	epc->ops->free_doorbell(epc, func_no, vfunc_no);
> > > > > > 
> > > > > > Same as suggested above.
> > > > > > 
> > > > > > > +	mutex_unlock(&epc->lock);
> > > > > > > +}
> > > > > > > +EXPORT_SYMBOL_GPL(pci_epc_free_doorbell);
> > > > > > > +
> > > > > > > +static irqreturn_t pci_epf_generic_doorbell_handler(int irq, void *data)
> > > > > > > +{
> > > > > > > +	struct pci_epf *epf = data;
> > > > > > > +
> > > > > > > +	if (epf->event_ops && epf->event_ops->doorbell)
> > > > > > > +		epf->event_ops->doorbell(epf, irq - epf->virq_base);
> > > > > > 
> > > > > > Same as suggested above.
> > > > > > 
> > > > > > > +
> > > > > > > +	return IRQ_HANDLED;
> > > > > > > +}
> > > > > > > +
> > > > > > > +static void pci_epc_generic_write_msi_msg(struct msi_desc *desc, struct msi_msg *msg)
> > > > > > > +{
> > > > > > > +	struct pci_epc *epc = NULL;
> > > > > > > +	struct class_dev_iter iter;
> > > > > > > +	struct pci_epf *epf;
> > > > > > > +	struct device *dev;
> > > > > > > +
> > > > > > > +	class_dev_iter_init(&iter, pci_epc_class, NULL, NULL);
> > > > > > > +	while ((dev = class_dev_iter_next(&iter))) {
> > > > > > > +		if (dev->parent != desc->dev)
> > > > > > > +			continue;
> > > > > > > +
> > > > > > > +		epc = to_pci_epc(dev);
> > > > > > > +
> > > > > > > +		class_dev_iter_exit(&iter);
> > > > > > > +		break;
> > > > > > > +	}
> > > > > > > +
> > > > > > > +	if (!epc)
> > > > > > > +		return;
> > > > > > > +
> > > > > > > +	/* Only support one EPF for doorbell */
> > > > > > > +	epf = list_first_entry_or_null(&epc->pci_epf, struct pci_epf, list);
> > > > > > > +
> > > > > > 
> > > > > > No need of this newline
> > > > > > 
> > > > > > > +	if (!epf)
> > > > > > > +		return;
> > > > > > > +
> > > > > > > +	if (epf->msg && desc->msi_index < epf->num_msgs)
> > > > > > > +		epf->msg[desc->msi_index] = *msg;
> > > > > > > +}
> > > > > > > +
> > > > > > > +
> > > > > > 
> > > > > > Remove extra newline
> > > > > > 
> > > > > > > +/**
> > > > > > > + * pci_epc_generic_alloc_doorbell() - Common help function. Allocate address space from MSI
> > > > > > > + *                                    controller
> > > > > > > + *
> > > > > > > + * @epc: the EPC device that need doorbell address and data from RC.
> > > > > > > + * @func_no: the physical endpoint function number in the EPC device.
> > > > > > > + * @vfunc_no: the virtual endpoint function number in the physical function.
> > > > > > > + * @num_msgs: the total number of doorbell messages
> > > > > > > + *
> > > > > > 
> > > > > > Same comment as for pci_epc_alloc_doorbell()
> > > > > > 
> > > > > > > + * Remark: use this function only if EPC driver just register one EPC device.
> > > > > > > + *
> > > > > > > + * Return: 0 success, other is failure
> > > > > > > + */
> > > > > > > +int pci_epc_generic_alloc_doorbell(struct pci_epc *epc, u8 func_no, u8 vfunc_no, int num_msgs)
> > > > > > > +{
> > > > > > > +	struct pci_epf *epf;
> > > > > > > +	struct device *dev;
> > > > > > > +	int virq, last;
> > > > > > > +	int ret;
> > > > > > > +	int i;
> > > > > > > +
> > > > > > > +	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_msi_domain_alloc_irqs(dev, num_msgs, pci_epc_generic_write_msi_msg);
> > > > > > > +	if (ret) {
> > > > > > > +		dev_err(dev, "Failed to allocate MSI\n");
> > > > > > > +		return -ENOMEM;
> > > > > > > +	}
> > > > > > > +
> > > > > > > +	last = -1;
> > > > > > > +	for (i = 0; i < num_msgs; i++) {
> > > > > > 
> > > > > > You should iterate over msi_desc as below:
> > > > > > 
> > > > > >         msi_lock_descs(dev);
> > > > > >         msi_for_each_desc(desc, dev, MSI_DESC_ALL) {
> > > > > > 		...
> > > > > > 	}
> > > > > > 	msi_unlock_descs(dev);
> > > > > > 
> > > > > > > +		virq = msi_get_virq(dev, i);
> > > > > > > +		if (i == 0)
> > > > > > > +			epf->virq_base = virq;
> > > > > > > +
> > > > > > > +		ret = request_irq(virq, pci_epf_generic_doorbell_handler, 0,
> > > > > > 
> > > > > > 	request_irq(desc->irq, ...)
> > > > > > 
> > > > > > > +				  kasprintf(GFP_KERNEL, "pci-epc-doorbell%d", i), epf);
> > > > > > > +
> > > > > > > +		if (ret) {
> > > > > > > +			dev_err(dev, "Failed to request doorbell\n");
> > > > > > > +			goto err_free_irq;
> > > > > > > +		}
> > > > > > > +		last = i;
> > > > > > > +	}
> > > > > > > +
> > > > > > > +	return 0;
> > > > > > > +
> > > > > > > +err_free_irq:
> > > > > > > +	for (i = 0; i < last; i++)
> > > > > > > +		kfree(free_irq(epf->virq_base + i, epf));
> > > > > > > +	platform_msi_domain_free_irqs(dev);
> > > > > > > +
> > > > > > > +	return -EINVAL;
> > > > > > 
> > > > > > 	return ret;
> > > > > > 
> > > > > > > +}
> > > > > > > +EXPORT_SYMBOL_GPL(pci_epc_generic_alloc_doorbell);
> > > > > > > +
> > > > > > 
> > > > > > [...]
> > > > > > 
> > > > > > > diff --git a/include/linux/pci-epf.h b/include/linux/pci-epf.h
> > > > > > > index 3f44b6aec4770..485c146a5efe2 100644
> > > > > > > --- a/include/linux/pci-epf.h
> > > > > > > +++ b/include/linux/pci-epf.h
> > > > > > > @@ -79,6 +79,7 @@ struct pci_epc_event_ops {
> > > > > > >  	int (*link_up)(struct pci_epf *epf);
> > > > > > >  	int (*link_down)(struct pci_epf *epf);
> > > > > > >  	int (*bme)(struct pci_epf *epf);
> > > > > > > +	int (*doorbell)(struct pci_epf *epf, int index);
> > > > > > 
> > > > > > kdoc missing.
> > > > > > 
> > > > > > >  };
> > > > > > >  
> > > > > > >  /**
> > > > > > > @@ -180,6 +181,9 @@ 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_msgs;
> > > > > > 
> > > > > > num_db
> > > > > > 
> > > > > > You also need to add kdoc for each new member.
> > > > > > 
> > > > > > - Mani
> > > > > > 
> > > > > > -- 
> > > > > > மணிவண்ணன் சதாசிவம்
> > > > 
> > > > -- 
> > > > மணிவண்ணன் சதாசிவம்
> > 
> > -- 
> > மணிவண்ணன் சதாசிவம்

On Fri, Oct 20, 2023 at 10:42:15PM +0530, Manivannan Sadhasivam wrote:
> On Thu, Oct 19, 2023 at 02:11:22PM -0400, Frank Li wrote:
> > On Thu, Oct 19, 2023 at 10:53:47PM +0530, Manivannan Sadhasivam wrote:
> > > On Thu, Oct 19, 2023 at 12:00:22PM -0400, Frank Li wrote:
> > > > On Thu, Oct 19, 2023 at 08:34:41PM +0530, Manivannan Sadhasivam wrote:
> > > > > On Tue, Oct 17, 2023 at 02:55:57PM -0400, Frank Li wrote:
> > > > > > On Wed, Oct 18, 2023 at 12:07:22AM +0530, Manivannan Sadhasivam wrote:
> > > > > > > On Mon, Sep 11, 2023 at 06:09:16PM -0400, Frank Li wrote:
> > > > > > > > This commit introduces a common method for sending messages from the Root
> > > > > > > > Complex (RC) to the Endpoint (EP) by utilizing the platform MSI interrupt
> > > > > > > > controller, such as ARM GIC, as an EP doorbell. Maps the memory assigned
> > > > > > > > for the BAR region by the PCI host to the message address of the platform
> > > > > > > > MSI interrupt controller in the PCI EP. As a result, when the PCI RC writes
> > > > > > > 
> > > > > > > "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."
> > > > > > > 
> > > > > > > > to the BAR region, it triggers an IRQ at the EP. This implementation serves
> > > > > > > > as a common method for all endpoint function drivers.
> > > > > > > > 
> > > > > > > > However, it currently supports only one EP physical function due to
> > > > > > > > limitations in ARM MSI/IMS readiness.
> > > > > > > > 
> > > > > > > > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > > > > > > > ---
> > > > > > > >  drivers/pci/endpoint/pci-epc-core.c | 192 ++++++++++++++++++++++++++++
> > > > > > > >  drivers/pci/endpoint/pci-epf-core.c |  44 +++++++
> > > > > > > >  include/linux/pci-epc.h             |   6 +
> > > > > > > >  include/linux/pci-epf.h             |   7 +
> > > > > > > >  4 files changed, 249 insertions(+)
> > > > > > > > 
> > > > > > > > diff --git a/drivers/pci/endpoint/pci-epc-core.c b/drivers/pci/endpoint/pci-epc-core.c
> > > > > > > > index 5a4a8b0be6262..d336a99c6a94f 100644
> > > > > > > > --- a/drivers/pci/endpoint/pci-epc-core.c
> > > > > > > > +++ b/drivers/pci/endpoint/pci-epc-core.c
> > > > > > > > @@ -10,6 +10,7 @@
> > > > > > > >  #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>
> > > > > > > > @@ -783,6 +784,197 @@ void pci_epc_bme_notify(struct pci_epc *epc)
> > > > > > > >  }
> > > > > > > >  EXPORT_SYMBOL_GPL(pci_epc_bme_notify);
> > > > > > > >  
> > > > > > > > +/**
> > > > > > > > + * pci_epc_alloc_doorbell() - alloc an address space to let RC trigger EP side IRQ by write data to
> > > > > > > > + *			      the space.
> > > > > > > 
> > > > > > > "Allocate platform specific doorbell IRQs to be used by the host to trigger
> > > > > > > doorbells on EP."
> > > > > > > 
> > > > > > > > + *
> > > > > > > > + * @epc: the EPC device that need doorbell address and data from RC.
> > > > > > > 
> > > > > > > EPC device for which the doorbell needs to be allocated
> > > > > > > 
> > > > > > > > + * @func_no: the physical endpoint function number in the EPC device.
> > > > > > > > + * @vfunc_no: the virtual endpoint function number in the physical function.
> > > > > > > > + * @num_msgs: the total number of doorbell messages
> > > > > > > 
> > > > > > > s/num_msgs/num_db
> > > > > > > 
> > > > > > > > + *
> > > > > > > > + * Return: 0 success, other is failure
> > > > > > > > + */
> > > > > > > > +int pci_epc_alloc_doorbell(struct pci_epc *epc, u8 func_no, u8 vfunc_no, int num_msgs)
> > > > > > > > +{
> > > > > > > > +	int ret;
> > > > > > > > +
> > > > > > > > +	if (IS_ERR_OR_NULL(epc) || func_no >= epc->max_functions)
> > > > > > > > +		return -EINVAL;
> > > > > > > > +
> > > > > > > > +	if (vfunc_no > 0 && (!epc->max_vfs || vfunc_no > epc->max_vfs[func_no]))
> > > > > > > > +		return -EINVAL;
> > > > > > > > +
> > > > > > > > +	if (!epc->ops->alloc_doorbell)
> > > > > > > > +		return 0;
> > > > > > > 
> > > > > > > You mentioned 0 is a success. So if there is no callback, you want to return
> > > > > > > success?
> > > > > > > 
> > > > > > > > +
> > > > > > > > +	mutex_lock(&epc->lock);
> > > > > > > > +	ret = epc->ops->alloc_doorbell(epc, func_no, vfunc_no, num_msgs);
> > > > > > > 
> > > > > > > Why can't you just call the generic function here and in other places instead of
> > > > > > > implementing callbacks? I do not see a necessity for EPC specific callbacks. If
> > > > > > > there is one, please specify.
> > > > > > 
> > > > > > 1. Refer v1 your comments.
> > > > > > https://lore.kernel.org/imx/20230906145227.GC5930@thinkpad/
> > > > > 
> > > > > I do not find where I suggested the callback approach.
> > > > 
> > > > 	> > > If that, Each EPF driver need do duplicate work. 
> > > > 	> > > 
> > > > 	> > 
> > > > 	> > Yes, and that's how it should be. EPF core has no job in supplying the of_node.
> > > > 	> > It is the responsibility of the EPF drivers as they depend on OF for platform
> > > > 	> > support.
> > > > 	> 
> > > > 	> EPF driver still not depend on OF. such pci-epf-test, which was probed by
> > > > 	> configfs.
> > > > 	> 
> > > > 
> > > > 	Hmm, yeah. Then it should be part of the EPC driver.
> > > > 
> > > > 	Sorry for the confusion.
> > > > 
> > > > Here, all "EPF" should be "EPC". The key problem is of_node. EPC core have
> > > > not of_node, EPC core's parent driver (like dwc-ep driver) have of_node. 
> > > > 
> > > > pci_epc_generic_alloc_doorbell(dev), dev is probed by platform driver, such
> > > > as dwc-ep, which have of_node,  EPC core will create child device.
> > > > 
> > > > dwc-ep device
> > > >  |- epc core device
> > > > 
> > > > we can direct call pci_epc_generic_alloc_doorbell(epc->parent) here.
> > > > 
> > > > I may miss understand what your means. I think you want to dwc-ep
> > > > (with of_node) handle these alloc functions. 
> > > > 
> > > 
> > > My comment was to have just one function definition. But looking at it again, I
> > > think it is better to move all the (alloc, free, write_msg) definitions to
> > > dwc-ep, since the contents of those functions are not EPC core specific.
> > 
> > There are still problem. (alloc, free, write_msg) is quite common for all
> > controller and the system with MSI.
> > 
> > If move these into dwc-ep,  cdns or other controller have to duplicate 
> > these codes.
> > 
> > If you think it is not EPC core specific, how about create new help files?
> > 
> 
> Hmm, that sounds good to me. I think the best place would be:
> drivers/pci/endpoint/pci-ep-msi.c

How about header file?

int pci_epc_generic_alloc_doorbell(struct pci_epc *epc, u8 func_no, u8 vfunc_no, int num_msgs);     
void pci_epc_generic_free_doorbell(struct pci_epc *epc, u8 func_no, u8 vfunc_no);

Is it in include/linux/pci-epc.h, just 2 lines.

Frank
> 
> Reason is, we cannot have this generic code under drivers/pci/controller/ as it
> is not a standalone PCI controller but a platform MSI controller. So having it
> under pci/endpoint/ makes much sense to me.
> 
> And this is not specific to EPF drivers as well, so we cannot have it under
> pci/endpoint/functions/.
> 
> - Mani
> 
> > Frank
> > 
> > > 
> > > In the EPC core, you can still have the callbacks specific to each EPC. This
> > > also solves your of_node problem.
> > > 
> > > - Mani
> > > 
> > > > > 
> > > > > > 2. Maybe some ep controller have built-in doorbell support. Write to some
> > > > > > address to trigger doorbell irq.
> > > > > > 
> > > > > 
> > > > > We will handle it whenever such EP controllers arrive. Until then, let's keep it
> > > > > simple.
> > > > > 
> > > > > - Mani
> > > > > 
> > > > > > Frank
> > > > > > 
> > > > > > > 
> > > > > > > > +	mutex_unlock(&epc->lock);
> > > > > > > > +
> > > > > > > > +	return ret;
> > > > > > > > +}
> > > > > > > > +EXPORT_SYMBOL_GPL(pci_epc_alloc_doorbell);
> > > > > > > > +
> > > > > > > > +/**
> > > > > > > > + * pci_epc_free_doorbell() - free resource allocated by pci_epc_alloc_doorbell()
> > > > > > > > + *
> > > > > > > > + * @epc: the EPC device that need doorbell address and data from RC.
> > > > > > > 
> > > > > > > Same as above.
> > > > > > > 
> > > > > > > > + * @func_no: the physical endpoint function number in the EPC device.
> > > > > > > > + * @vfunc_no: the virtual endpoint function number in the physical function.
> > > > > > > > + *
> > > > > > > > + * Return: 0 success, other is failure
> > > > > > > > + */
> > > > > > > > +void pci_epc_free_doorbell(struct pci_epc *epc, u8 func_no, u8 vfunc_no)
> > > > > > > > +{
> > > > > > > > +	if (IS_ERR_OR_NULL(epc) || func_no >= epc->max_functions)
> > > > > > > > +		return;
> > > > > > > > +
> > > > > > > > +	if (vfunc_no > 0 && (!epc->max_vfs || vfunc_no > epc->max_vfs[func_no]))
> > > > > > > > +		return;
> > > > > > > > +
> > > > > > > > +	if (!epc->ops->free_doorbell)
> > > > > > > > +		return;
> > > > > > > > +
> > > > > > > > +	mutex_lock(&epc->lock);
> > > > > > > > +	epc->ops->free_doorbell(epc, func_no, vfunc_no);
> > > > > > > 
> > > > > > > Same as suggested above.
> > > > > > > 
> > > > > > > > +	mutex_unlock(&epc->lock);
> > > > > > > > +}
> > > > > > > > +EXPORT_SYMBOL_GPL(pci_epc_free_doorbell);
> > > > > > > > +
> > > > > > > > +static irqreturn_t pci_epf_generic_doorbell_handler(int irq, void *data)
> > > > > > > > +{
> > > > > > > > +	struct pci_epf *epf = data;
> > > > > > > > +
> > > > > > > > +	if (epf->event_ops && epf->event_ops->doorbell)
> > > > > > > > +		epf->event_ops->doorbell(epf, irq - epf->virq_base);
> > > > > > > 
> > > > > > > Same as suggested above.
> > > > > > > 
> > > > > > > > +
> > > > > > > > +	return IRQ_HANDLED;
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +static void pci_epc_generic_write_msi_msg(struct msi_desc *desc, struct msi_msg *msg)
> > > > > > > > +{
> > > > > > > > +	struct pci_epc *epc = NULL;
> > > > > > > > +	struct class_dev_iter iter;
> > > > > > > > +	struct pci_epf *epf;
> > > > > > > > +	struct device *dev;
> > > > > > > > +
> > > > > > > > +	class_dev_iter_init(&iter, pci_epc_class, NULL, NULL);
> > > > > > > > +	while ((dev = class_dev_iter_next(&iter))) {
> > > > > > > > +		if (dev->parent != desc->dev)
> > > > > > > > +			continue;
> > > > > > > > +
> > > > > > > > +		epc = to_pci_epc(dev);
> > > > > > > > +
> > > > > > > > +		class_dev_iter_exit(&iter);
> > > > > > > > +		break;
> > > > > > > > +	}
> > > > > > > > +
> > > > > > > > +	if (!epc)
> > > > > > > > +		return;
> > > > > > > > +
> > > > > > > > +	/* Only support one EPF for doorbell */
> > > > > > > > +	epf = list_first_entry_or_null(&epc->pci_epf, struct pci_epf, list);
> > > > > > > > +
> > > > > > > 
> > > > > > > No need of this newline
> > > > > > > 
> > > > > > > > +	if (!epf)
> > > > > > > > +		return;
> > > > > > > > +
> > > > > > > > +	if (epf->msg && desc->msi_index < epf->num_msgs)
> > > > > > > > +		epf->msg[desc->msi_index] = *msg;
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +
> > > > > > > 
> > > > > > > Remove extra newline
> > > > > > > 
> > > > > > > > +/**
> > > > > > > > + * pci_epc_generic_alloc_doorbell() - Common help function. Allocate address space from MSI
> > > > > > > > + *                                    controller
> > > > > > > > + *
> > > > > > > > + * @epc: the EPC device that need doorbell address and data from RC.
> > > > > > > > + * @func_no: the physical endpoint function number in the EPC device.
> > > > > > > > + * @vfunc_no: the virtual endpoint function number in the physical function.
> > > > > > > > + * @num_msgs: the total number of doorbell messages
> > > > > > > > + *
> > > > > > > 
> > > > > > > Same comment as for pci_epc_alloc_doorbell()
> > > > > > > 
> > > > > > > > + * Remark: use this function only if EPC driver just register one EPC device.
> > > > > > > > + *
> > > > > > > > + * Return: 0 success, other is failure
> > > > > > > > + */
> > > > > > > > +int pci_epc_generic_alloc_doorbell(struct pci_epc *epc, u8 func_no, u8 vfunc_no, int num_msgs)
> > > > > > > > +{
> > > > > > > > +	struct pci_epf *epf;
> > > > > > > > +	struct device *dev;
> > > > > > > > +	int virq, last;
> > > > > > > > +	int ret;
> > > > > > > > +	int i;
> > > > > > > > +
> > > > > > > > +	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_msi_domain_alloc_irqs(dev, num_msgs, pci_epc_generic_write_msi_msg);
> > > > > > > > +	if (ret) {
> > > > > > > > +		dev_err(dev, "Failed to allocate MSI\n");
> > > > > > > > +		return -ENOMEM;
> > > > > > > > +	}
> > > > > > > > +
> > > > > > > > +	last = -1;
> > > > > > > > +	for (i = 0; i < num_msgs; i++) {
> > > > > > > 
> > > > > > > You should iterate over msi_desc as below:
> > > > > > > 
> > > > > > >         msi_lock_descs(dev);
> > > > > > >         msi_for_each_desc(desc, dev, MSI_DESC_ALL) {
> > > > > > > 		...
> > > > > > > 	}
> > > > > > > 	msi_unlock_descs(dev);
> > > > > > > 
> > > > > > > > +		virq = msi_get_virq(dev, i);
> > > > > > > > +		if (i == 0)
> > > > > > > > +			epf->virq_base = virq;
> > > > > > > > +
> > > > > > > > +		ret = request_irq(virq, pci_epf_generic_doorbell_handler, 0,
> > > > > > > 
> > > > > > > 	request_irq(desc->irq, ...)
> > > > > > > 
> > > > > > > > +				  kasprintf(GFP_KERNEL, "pci-epc-doorbell%d", i), epf);
> > > > > > > > +
> > > > > > > > +		if (ret) {
> > > > > > > > +			dev_err(dev, "Failed to request doorbell\n");
> > > > > > > > +			goto err_free_irq;
> > > > > > > > +		}
> > > > > > > > +		last = i;
> > > > > > > > +	}
> > > > > > > > +
> > > > > > > > +	return 0;
> > > > > > > > +
> > > > > > > > +err_free_irq:
> > > > > > > > +	for (i = 0; i < last; i++)
> > > > > > > > +		kfree(free_irq(epf->virq_base + i, epf));
> > > > > > > > +	platform_msi_domain_free_irqs(dev);
> > > > > > > > +
> > > > > > > > +	return -EINVAL;
> > > > > > > 
> > > > > > > 	return ret;
> > > > > > > 
> > > > > > > > +}
> > > > > > > > +EXPORT_SYMBOL_GPL(pci_epc_generic_alloc_doorbell);
> > > > > > > > +
> > > > > > > 
> > > > > > > [...]
> > > > > > > 
> > > > > > > > diff --git a/include/linux/pci-epf.h b/include/linux/pci-epf.h
> > > > > > > > index 3f44b6aec4770..485c146a5efe2 100644
> > > > > > > > --- a/include/linux/pci-epf.h
> > > > > > > > +++ b/include/linux/pci-epf.h
> > > > > > > > @@ -79,6 +79,7 @@ struct pci_epc_event_ops {
> > > > > > > >  	int (*link_up)(struct pci_epf *epf);
> > > > > > > >  	int (*link_down)(struct pci_epf *epf);
> > > > > > > >  	int (*bme)(struct pci_epf *epf);
> > > > > > > > +	int (*doorbell)(struct pci_epf *epf, int index);
> > > > > > > 
> > > > > > > kdoc missing.
> > > > > > > 
> > > > > > > >  };
> > > > > > > >  
> > > > > > > >  /**
> > > > > > > > @@ -180,6 +181,9 @@ 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_msgs;
> > > > > > > 
> > > > > > > num_db
> > > > > > > 
> > > > > > > You also need to add kdoc for each new member.
> > > > > > > 
> > > > > > > - Mani
> > > > > > > 
> > > > > > > -- 
> > > > > > > மணிவண்ணன் சதாசிவம்
> > > > > 
> > > > > -- 
> > > > > மணிவண்ணன் சதாசிவம்
> > > 
> > > -- 
> > > மணிவண்ணன் சதாசிவம்
> 
> -- 
> மணிவண்ணன் சதாசிவம்

On Fri, Oct 20, 2023 at 02:00:06PM -0400, Frank Li wrote:
> On Fri, Oct 20, 2023 at 10:42:15PM +0530, Manivannan Sadhasivam wrote:
> > On Thu, Oct 19, 2023 at 02:11:22PM -0400, Frank Li wrote:
> > > On Thu, Oct 19, 2023 at 10:53:47PM +0530, Manivannan Sadhasivam wrote:
> > > > On Thu, Oct 19, 2023 at 12:00:22PM -0400, Frank Li wrote:
> > > > > On Thu, Oct 19, 2023 at 08:34:41PM +0530, Manivannan Sadhasivam wrote:
> > > > > > On Tue, Oct 17, 2023 at 02:55:57PM -0400, Frank Li wrote:
> > > > > > > On Wed, Oct 18, 2023 at 12:07:22AM +0530, Manivannan Sadhasivam wrote:
> > > > > > > > On Mon, Sep 11, 2023 at 06:09:16PM -0400, Frank Li wrote:
> > > > > > > > > This commit introduces a common method for sending messages from the Root
> > > > > > > > > Complex (RC) to the Endpoint (EP) by utilizing the platform MSI interrupt
> > > > > > > > > controller, such as ARM GIC, as an EP doorbell. Maps the memory assigned
> > > > > > > > > for the BAR region by the PCI host to the message address of the platform
> > > > > > > > > MSI interrupt controller in the PCI EP. As a result, when the PCI RC writes
> > > > > > > > 
> > > > > > > > "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."
> > > > > > > > 
> > > > > > > > > to the BAR region, it triggers an IRQ at the EP. This implementation serves
> > > > > > > > > as a common method for all endpoint function drivers.
> > > > > > > > > 
> > > > > > > > > However, it currently supports only one EP physical function due to
> > > > > > > > > limitations in ARM MSI/IMS readiness.
> > > > > > > > > 
> > > > > > > > > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > > > > > > > > ---
> > > > > > > > >  drivers/pci/endpoint/pci-epc-core.c | 192 ++++++++++++++++++++++++++++
> > > > > > > > >  drivers/pci/endpoint/pci-epf-core.c |  44 +++++++
> > > > > > > > >  include/linux/pci-epc.h             |   6 +
> > > > > > > > >  include/linux/pci-epf.h             |   7 +
> > > > > > > > >  4 files changed, 249 insertions(+)
> > > > > > > > > 
> > > > > > > > > diff --git a/drivers/pci/endpoint/pci-epc-core.c b/drivers/pci/endpoint/pci-epc-core.c
> > > > > > > > > index 5a4a8b0be6262..d336a99c6a94f 100644
> > > > > > > > > --- a/drivers/pci/endpoint/pci-epc-core.c
> > > > > > > > > +++ b/drivers/pci/endpoint/pci-epc-core.c
> > > > > > > > > @@ -10,6 +10,7 @@
> > > > > > > > >  #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>
> > > > > > > > > @@ -783,6 +784,197 @@ void pci_epc_bme_notify(struct pci_epc *epc)
> > > > > > > > >  }
> > > > > > > > >  EXPORT_SYMBOL_GPL(pci_epc_bme_notify);
> > > > > > > > >  
> > > > > > > > > +/**
> > > > > > > > > + * pci_epc_alloc_doorbell() - alloc an address space to let RC trigger EP side IRQ by write data to
> > > > > > > > > + *			      the space.
> > > > > > > > 
> > > > > > > > "Allocate platform specific doorbell IRQs to be used by the host to trigger
> > > > > > > > doorbells on EP."
> > > > > > > > 
> > > > > > > > > + *
> > > > > > > > > + * @epc: the EPC device that need doorbell address and data from RC.
> > > > > > > > 
> > > > > > > > EPC device for which the doorbell needs to be allocated
> > > > > > > > 
> > > > > > > > > + * @func_no: the physical endpoint function number in the EPC device.
> > > > > > > > > + * @vfunc_no: the virtual endpoint function number in the physical function.
> > > > > > > > > + * @num_msgs: the total number of doorbell messages
> > > > > > > > 
> > > > > > > > s/num_msgs/num_db
> > > > > > > > 
> > > > > > > > > + *
> > > > > > > > > + * Return: 0 success, other is failure
> > > > > > > > > + */
> > > > > > > > > +int pci_epc_alloc_doorbell(struct pci_epc *epc, u8 func_no, u8 vfunc_no, int num_msgs)
> > > > > > > > > +{
> > > > > > > > > +	int ret;
> > > > > > > > > +
> > > > > > > > > +	if (IS_ERR_OR_NULL(epc) || func_no >= epc->max_functions)
> > > > > > > > > +		return -EINVAL;
> > > > > > > > > +
> > > > > > > > > +	if (vfunc_no > 0 && (!epc->max_vfs || vfunc_no > epc->max_vfs[func_no]))
> > > > > > > > > +		return -EINVAL;
> > > > > > > > > +
> > > > > > > > > +	if (!epc->ops->alloc_doorbell)
> > > > > > > > > +		return 0;
> > > > > > > > 
> > > > > > > > You mentioned 0 is a success. So if there is no callback, you want to return
> > > > > > > > success?
> > > > > > > > 
> > > > > > > > > +
> > > > > > > > > +	mutex_lock(&epc->lock);
> > > > > > > > > +	ret = epc->ops->alloc_doorbell(epc, func_no, vfunc_no, num_msgs);
> > > > > > > > 
> > > > > > > > Why can't you just call the generic function here and in other places instead of
> > > > > > > > implementing callbacks? I do not see a necessity for EPC specific callbacks. If
> > > > > > > > there is one, please specify.
> > > > > > > 
> > > > > > > 1. Refer v1 your comments.
> > > > > > > https://lore.kernel.org/imx/20230906145227.GC5930@thinkpad/
> > > > > > 
> > > > > > I do not find where I suggested the callback approach.
> > > > > 
> > > > > 	> > > If that, Each EPF driver need do duplicate work. 
> > > > > 	> > > 
> > > > > 	> > 
> > > > > 	> > Yes, and that's how it should be. EPF core has no job in supplying the of_node.
> > > > > 	> > It is the responsibility of the EPF drivers as they depend on OF for platform
> > > > > 	> > support.
> > > > > 	> 
> > > > > 	> EPF driver still not depend on OF. such pci-epf-test, which was probed by
> > > > > 	> configfs.
> > > > > 	> 
> > > > > 
> > > > > 	Hmm, yeah. Then it should be part of the EPC driver.
> > > > > 
> > > > > 	Sorry for the confusion.
> > > > > 
> > > > > Here, all "EPF" should be "EPC". The key problem is of_node. EPC core have
> > > > > not of_node, EPC core's parent driver (like dwc-ep driver) have of_node. 
> > > > > 
> > > > > pci_epc_generic_alloc_doorbell(dev), dev is probed by platform driver, such
> > > > > as dwc-ep, which have of_node,  EPC core will create child device.
> > > > > 
> > > > > dwc-ep device
> > > > >  |- epc core device
> > > > > 
> > > > > we can direct call pci_epc_generic_alloc_doorbell(epc->parent) here.
> > > > > 
> > > > > I may miss understand what your means. I think you want to dwc-ep
> > > > > (with of_node) handle these alloc functions. 
> > > > > 
> > > > 
> > > > My comment was to have just one function definition. But looking at it again, I
> > > > think it is better to move all the (alloc, free, write_msg) definitions to
> > > > dwc-ep, since the contents of those functions are not EPC core specific.
> > > 
> > > There are still problem. (alloc, free, write_msg) is quite common for all
> > > controller and the system with MSI.
> > > 
> > > If move these into dwc-ep,  cdns or other controller have to duplicate 
> > > these codes.
> > > 
> > > If you think it is not EPC core specific, how about create new help files?
> > > 
> > 
> > Hmm, that sounds good to me. I think the best place would be:
> > drivers/pci/endpoint/pci-ep-msi.c
> 
> How about header file?
> 
> int pci_epc_generic_alloc_doorbell(struct pci_epc *epc, u8 func_no, u8 vfunc_no, int num_msgs);     
> void pci_epc_generic_free_doorbell(struct pci_epc *epc, u8 func_no, u8 vfunc_no);
> 
> Is it in include/linux/pci-epc.h, just 2 lines.
> 

Sorry, I don't understand what you are suggesting. Can you elaborate?

My suggestion was to place the function definitions in
drivers/pci/endpoint/pci-ep-msi.c, create a separate header
include/linux/pci-ep-msi.h and place the declarations there. The EPC drivers
want to use generic MSI support should include this header.

- Mani

> Frank
> > 
> > Reason is, we cannot have this generic code under drivers/pci/controller/ as it
> > is not a standalone PCI controller but a platform MSI controller. So having it
> > under pci/endpoint/ makes much sense to me.
> > 
> > And this is not specific to EPF drivers as well, so we cannot have it under
> > pci/endpoint/functions/.
> > 
> > - Mani
> > 
> > > Frank
> > > 
> > > > 
> > > > In the EPC core, you can still have the callbacks specific to each EPC. This
> > > > also solves your of_node problem.
> > > > 
> > > > - Mani
> > > > 
> > > > > > 
> > > > > > > 2. Maybe some ep controller have built-in doorbell support. Write to some
> > > > > > > address to trigger doorbell irq.
> > > > > > > 
> > > > > > 
> > > > > > We will handle it whenever such EP controllers arrive. Until then, let's keep it
> > > > > > simple.
> > > > > > 
> > > > > > - Mani
> > > > > > 
> > > > > > > Frank
> > > > > > > 
> > > > > > > > 
> > > > > > > > > +	mutex_unlock(&epc->lock);
> > > > > > > > > +
> > > > > > > > > +	return ret;
> > > > > > > > > +}
> > > > > > > > > +EXPORT_SYMBOL_GPL(pci_epc_alloc_doorbell);
> > > > > > > > > +
> > > > > > > > > +/**
> > > > > > > > > + * pci_epc_free_doorbell() - free resource allocated by pci_epc_alloc_doorbell()
> > > > > > > > > + *
> > > > > > > > > + * @epc: the EPC device that need doorbell address and data from RC.
> > > > > > > > 
> > > > > > > > Same as above.
> > > > > > > > 
> > > > > > > > > + * @func_no: the physical endpoint function number in the EPC device.
> > > > > > > > > + * @vfunc_no: the virtual endpoint function number in the physical function.
> > > > > > > > > + *
> > > > > > > > > + * Return: 0 success, other is failure
> > > > > > > > > + */
> > > > > > > > > +void pci_epc_free_doorbell(struct pci_epc *epc, u8 func_no, u8 vfunc_no)
> > > > > > > > > +{
> > > > > > > > > +	if (IS_ERR_OR_NULL(epc) || func_no >= epc->max_functions)
> > > > > > > > > +		return;
> > > > > > > > > +
> > > > > > > > > +	if (vfunc_no > 0 && (!epc->max_vfs || vfunc_no > epc->max_vfs[func_no]))
> > > > > > > > > +		return;
> > > > > > > > > +
> > > > > > > > > +	if (!epc->ops->free_doorbell)
> > > > > > > > > +		return;
> > > > > > > > > +
> > > > > > > > > +	mutex_lock(&epc->lock);
> > > > > > > > > +	epc->ops->free_doorbell(epc, func_no, vfunc_no);
> > > > > > > > 
> > > > > > > > Same as suggested above.
> > > > > > > > 
> > > > > > > > > +	mutex_unlock(&epc->lock);
> > > > > > > > > +}
> > > > > > > > > +EXPORT_SYMBOL_GPL(pci_epc_free_doorbell);
> > > > > > > > > +
> > > > > > > > > +static irqreturn_t pci_epf_generic_doorbell_handler(int irq, void *data)
> > > > > > > > > +{
> > > > > > > > > +	struct pci_epf *epf = data;
> > > > > > > > > +
> > > > > > > > > +	if (epf->event_ops && epf->event_ops->doorbell)
> > > > > > > > > +		epf->event_ops->doorbell(epf, irq - epf->virq_base);
> > > > > > > > 
> > > > > > > > Same as suggested above.
> > > > > > > > 
> > > > > > > > > +
> > > > > > > > > +	return IRQ_HANDLED;
> > > > > > > > > +}
> > > > > > > > > +
> > > > > > > > > +static void pci_epc_generic_write_msi_msg(struct msi_desc *desc, struct msi_msg *msg)
> > > > > > > > > +{
> > > > > > > > > +	struct pci_epc *epc = NULL;
> > > > > > > > > +	struct class_dev_iter iter;
> > > > > > > > > +	struct pci_epf *epf;
> > > > > > > > > +	struct device *dev;
> > > > > > > > > +
> > > > > > > > > +	class_dev_iter_init(&iter, pci_epc_class, NULL, NULL);
> > > > > > > > > +	while ((dev = class_dev_iter_next(&iter))) {
> > > > > > > > > +		if (dev->parent != desc->dev)
> > > > > > > > > +			continue;
> > > > > > > > > +
> > > > > > > > > +		epc = to_pci_epc(dev);
> > > > > > > > > +
> > > > > > > > > +		class_dev_iter_exit(&iter);
> > > > > > > > > +		break;
> > > > > > > > > +	}
> > > > > > > > > +
> > > > > > > > > +	if (!epc)
> > > > > > > > > +		return;
> > > > > > > > > +
> > > > > > > > > +	/* Only support one EPF for doorbell */
> > > > > > > > > +	epf = list_first_entry_or_null(&epc->pci_epf, struct pci_epf, list);
> > > > > > > > > +
> > > > > > > > 
> > > > > > > > No need of this newline
> > > > > > > > 
> > > > > > > > > +	if (!epf)
> > > > > > > > > +		return;
> > > > > > > > > +
> > > > > > > > > +	if (epf->msg && desc->msi_index < epf->num_msgs)
> > > > > > > > > +		epf->msg[desc->msi_index] = *msg;
> > > > > > > > > +}
> > > > > > > > > +
> > > > > > > > > +
> > > > > > > > 
> > > > > > > > Remove extra newline
> > > > > > > > 
> > > > > > > > > +/**
> > > > > > > > > + * pci_epc_generic_alloc_doorbell() - Common help function. Allocate address space from MSI
> > > > > > > > > + *                                    controller
> > > > > > > > > + *
> > > > > > > > > + * @epc: the EPC device that need doorbell address and data from RC.
> > > > > > > > > + * @func_no: the physical endpoint function number in the EPC device.
> > > > > > > > > + * @vfunc_no: the virtual endpoint function number in the physical function.
> > > > > > > > > + * @num_msgs: the total number of doorbell messages
> > > > > > > > > + *
> > > > > > > > 
> > > > > > > > Same comment as for pci_epc_alloc_doorbell()
> > > > > > > > 
> > > > > > > > > + * Remark: use this function only if EPC driver just register one EPC device.
> > > > > > > > > + *
> > > > > > > > > + * Return: 0 success, other is failure
> > > > > > > > > + */
> > > > > > > > > +int pci_epc_generic_alloc_doorbell(struct pci_epc *epc, u8 func_no, u8 vfunc_no, int num_msgs)
> > > > > > > > > +{
> > > > > > > > > +	struct pci_epf *epf;
> > > > > > > > > +	struct device *dev;
> > > > > > > > > +	int virq, last;
> > > > > > > > > +	int ret;
> > > > > > > > > +	int i;
> > > > > > > > > +
> > > > > > > > > +	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_msi_domain_alloc_irqs(dev, num_msgs, pci_epc_generic_write_msi_msg);
> > > > > > > > > +	if (ret) {
> > > > > > > > > +		dev_err(dev, "Failed to allocate MSI\n");
> > > > > > > > > +		return -ENOMEM;
> > > > > > > > > +	}
> > > > > > > > > +
> > > > > > > > > +	last = -1;
> > > > > > > > > +	for (i = 0; i < num_msgs; i++) {
> > > > > > > > 
> > > > > > > > You should iterate over msi_desc as below:
> > > > > > > > 
> > > > > > > >         msi_lock_descs(dev);
> > > > > > > >         msi_for_each_desc(desc, dev, MSI_DESC_ALL) {
> > > > > > > > 		...
> > > > > > > > 	}
> > > > > > > > 	msi_unlock_descs(dev);
> > > > > > > > 
> > > > > > > > > +		virq = msi_get_virq(dev, i);
> > > > > > > > > +		if (i == 0)
> > > > > > > > > +			epf->virq_base = virq;
> > > > > > > > > +
> > > > > > > > > +		ret = request_irq(virq, pci_epf_generic_doorbell_handler, 0,
> > > > > > > > 
> > > > > > > > 	request_irq(desc->irq, ...)
> > > > > > > > 
> > > > > > > > > +				  kasprintf(GFP_KERNEL, "pci-epc-doorbell%d", i), epf);
> > > > > > > > > +
> > > > > > > > > +		if (ret) {
> > > > > > > > > +			dev_err(dev, "Failed to request doorbell\n");
> > > > > > > > > +			goto err_free_irq;
> > > > > > > > > +		}
> > > > > > > > > +		last = i;
> > > > > > > > > +	}
> > > > > > > > > +
> > > > > > > > > +	return 0;
> > > > > > > > > +
> > > > > > > > > +err_free_irq:
> > > > > > > > > +	for (i = 0; i < last; i++)
> > > > > > > > > +		kfree(free_irq(epf->virq_base + i, epf));
> > > > > > > > > +	platform_msi_domain_free_irqs(dev);
> > > > > > > > > +
> > > > > > > > > +	return -EINVAL;
> > > > > > > > 
> > > > > > > > 	return ret;
> > > > > > > > 
> > > > > > > > > +}
> > > > > > > > > +EXPORT_SYMBOL_GPL(pci_epc_generic_alloc_doorbell);
> > > > > > > > > +
> > > > > > > > 
> > > > > > > > [...]
> > > > > > > > 
> > > > > > > > > diff --git a/include/linux/pci-epf.h b/include/linux/pci-epf.h
> > > > > > > > > index 3f44b6aec4770..485c146a5efe2 100644
> > > > > > > > > --- a/include/linux/pci-epf.h
> > > > > > > > > +++ b/include/linux/pci-epf.h
> > > > > > > > > @@ -79,6 +79,7 @@ struct pci_epc_event_ops {
> > > > > > > > >  	int (*link_up)(struct pci_epf *epf);
> > > > > > > > >  	int (*link_down)(struct pci_epf *epf);
> > > > > > > > >  	int (*bme)(struct pci_epf *epf);
> > > > > > > > > +	int (*doorbell)(struct pci_epf *epf, int index);
> > > > > > > > 
> > > > > > > > kdoc missing.
> > > > > > > > 
> > > > > > > > >  };
> > > > > > > > >  
> > > > > > > > >  /**
> > > > > > > > > @@ -180,6 +181,9 @@ 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_msgs;
> > > > > > > > 
> > > > > > > > num_db
> > > > > > > > 
> > > > > > > > You also need to add kdoc for each new member.
> > > > > > > > 
> > > > > > > > - Mani
> > > > > > > > 
> > > > > > > > -- 
> > > > > > > > மணிவண்ணன் சதாசிவம்
> > > > > > 
> > > > > > -- 
> > > > > > மணிவண்ணன் சதாசிவம்
> > > > 
> > > > -- 
> > > > மணிவண்ணன் சதாசிவம்
> > 
> > -- 
> > மணிவண்ணன் சதாசிவம்