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@xxxxxxx>
> > ---
> > 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
>
> --
> மணிவண்ணன் சதாசிவம்
