On Wed, Oct 16, 2024 at 10:36:20AM +0900, Damien Le Moal wrote:
> On 10/16/24 7:07 AM, Frank Li wrote:
> > Doorbell feature is implemented by mapping the EP's MSI interrupt
> > controller message address to a dedicated BAR in the EPC core. It is the
> > responsibility of the EPF driver to pass the actual message data to be
> > written by the host to the doorbell BAR region through its own logic.
> >
> > Signed-off-by: Frank Li <Frank.Li@xxxxxxx>
> > ---
> > drivers/pci/endpoint/Makefile | 2 +-
> > drivers/pci/endpoint/pci-ep-msi.c | 162 ++++++++++++++++++++++++++++++++++++++
> > include/linux/pci-ep-msi.h | 15 ++++
> > include/linux/pci-epf.h | 6 ++
> > 4 files changed, 184 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/pci/endpoint/Makefile b/drivers/pci/endpoint/Makefile
> > index 95b2fe47e3b06..a1ccce440c2c5 100644
> > --- a/drivers/pci/endpoint/Makefile
> > +++ b/drivers/pci/endpoint/Makefile
> > @@ -5,4 +5,4 @@
> >
> > obj-$(CONFIG_PCI_ENDPOINT_CONFIGFS) += pci-ep-cfs.o
> > obj-$(CONFIG_PCI_ENDPOINT) += pci-epc-core.o pci-epf-core.o\
> > - pci-epc-mem.o functions/
> > + pci-epc-mem.o pci-ep-msi.o functions/
> > diff --git a/drivers/pci/endpoint/pci-ep-msi.c b/drivers/pci/endpoint/pci-ep-msi.c
> > new file mode 100644
> > index 0000000000000..534dcd05c435c
> > --- /dev/null
> > +++ b/drivers/pci/endpoint/pci-ep-msi.c
> > @@ -0,0 +1,162 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * PCI Endpoint *Controller* (EPC) MSI library
> > + *
> > + * Copyright (C) 2024 NXP
> > + * Author: Frank Li <Frank.Li@xxxxxxx>
> > + */
> > +
> > +#include <linux/cleanup.h>
> > +#include <linux/device.h>
> > +#include <linux/slab.h>
> > +#include <linux/module.h>
> > +
>
> Stray blank line here.
>
> > +#include <linux/msi.h>
> > +#include <linux/pci-epc.h>
> > +#include <linux/pci-epf.h>
> > +#include <linux/pci-ep-cfs.h>
> > +#include <linux/pci-ep-msi.h>
> > +
> > +static irqreturn_t pci_epf_doorbell_handler(int irq, void *data)
> > +{
> > + struct pci_epf *epf = data;
> > + struct msi_desc *desc = irq_get_msi_desc(irq);
> > +
> > + if (desc && epf->event_ops && epf->event_ops->doorbell)
> > + epf->event_ops->doorbell(epf, desc->msi_index);
> > +
> > + return IRQ_HANDLED;
> > +}
> > +
> > +static bool pci_epc_match_parent(struct device *dev, void *param)
> > +{
> > + return dev->parent == param;
> > +}
> > +
> > +static void pci_epc_write_msi_msg(struct msi_desc *desc, struct msi_msg *msg)
> > +{
> > + struct pci_epc *epc __free(pci_epc_put) = NULL;
> > + struct pci_epf *epf;
> > +
> > + epc = pci_epc_get_fn(pci_epc_match_parent, desc->dev);
> > +
>
> No need for the blank line here.
>
> > + if (!epc)
> > + return;
> > +
> > + /* Only support one EPF for doorbell */
> > + epf = list_first_entry_or_null(&epc->pci_epf, struct pci_epf, list);
> > + if (!epf)
> > + return;
> > +
> > + if (epf->msg && desc->msi_index < epf->num_db)
>
> Change this to:
>
> if (epf && epf->msg && desc->msi_index < epf->num_db)
>
> and then remove the "if(!epf) return;" above. Shorter code :)
>
> > + memcpy(epf->msg, msg, sizeof(*msg));
> > +}
> > +
> > +static int pci_epc_alloc_doorbell(struct pci_epc *epc, u8 func_no, u8 vfunc_no, int num_db)
> > +{
> > + struct msi_desc *desc, *failed_desc;
> > + struct pci_epf *epf;
> > + struct device *dev;
> > + int i = 0;
> > + int ret;
> > +
> > + if (IS_ERR_OR_NULL(epc))
> > + return -EINVAL;
>
> Is this really needed ?
>
> > +
> > + /* Currently only support one func and one vfunc for doorbell */
> > + if (func_no || vfunc_no)
> > + return -EINVAL;
> > +
> > + epf = list_first_entry_or_null(&epc->pci_epf, struct pci_epf, list);
> > + if (!epf)
> > + return -EINVAL;
>
> Why do you need this ? epf is unused in this function...
epf need at request_irq.
>
> > +
> > + dev = epc->dev.parent;
> > + ret = platform_device_msi_init_and_alloc_irqs(dev, num_db, pci_epc_write_msi_msg);
> > + if (ret) {
> > + dev_err(dev, "Failed to allocate MSI\n");
> > + return -ENOMEM;
>
> return ret;
>
> > + }
> > +
> > + scoped_guard(msi_descs, dev)
>
> I personnally really dislike this... This adds one level of identation and
> hides code. Really ugly in my opinion. But that is only that, my opinion. I
> will let the maintainer decide on this.
I think it is better if treat 'scoped_guard' as key words 'if'. There are
'goto' at error branch, if no scope_guard, need unlock at two place, which
is quite easy to forget one at error path.
>
> > + msi_for_each_desc(desc, dev, MSI_DESC_ALL) {
> > + ret = request_irq(desc->irq, pci_epf_doorbell_handler, 0,
> > + kasprintf(GFP_KERNEL, "pci-epc-doorbell%d", i++), epf);
> > + if (ret) {
> > + dev_err(dev, "Failed to request doorbell\n");
> > + failed_desc = desc;
> > + goto err_request_irq;
> > + }
> > + }
> > +
> > + return 0;
> > +
> > +err_request_irq:
> > + scoped_guard(msi_descs, dev)
> > + msi_for_each_desc(desc, dev, MSI_DESC_ALL) {
> > + if (desc == failed_desc)
> > + break;
> > + kfree(free_irq(desc->irq, epf));
>
> If request_irq() failed and you want to cleanup everything, I do not think you
> need to track the failed_desc since free_irq() will return NULL if the irq was
> not requested. That would simplify this code.
>
> > + }
> > +
> > + platform_device_msi_free_irqs_all(epc->dev.parent);
> > +
> > + return ret;
> > +}
> > +
> > +static void pci_epc_free_doorbell(struct pci_epc *epc, u8 func_no, u8 vfunc_no)
> > +{
> > + struct msi_desc *desc;
> > + struct pci_epf *epf;
> > + struct device *dev;
> > +
> > + dev = epc->dev.parent;
>
> Nit: move the affectation to the declaration line:
>
> struct device *dev = epc->dev.parent;
>
> > +
> > + scoped_guard(msi_descs, dev)
> > + msi_for_each_desc(desc, dev, MSI_DESC_ALL)
> > + kfree(free_irq(desc->irq, epf));
> > +
> > + platform_device_msi_free_irqs_all(epc->dev.parent);
> > +}
> > +
> > +int pci_epf_alloc_doorbell(struct pci_epf *epf, u16 num_db)
> > +{
> > + struct pci_epc *epc;
> > + struct device *dev;
> > + void *msg;
> > + int ret;
> > +
> > + epc = epf->epc;
> > + dev = &epc->dev;
>
> Same here: move this to the declaration lines.
>
> > +
> > + guard(mutex)(&epc->lock);
>
> Another code hiding trick... Having the calls to mutex_lock/unlock(&epc->lock)
> would not complicate this code and makes things a lot more clear in my opinion...
>
> But more importantly: you are taking the epc lock in a pci_epf_ function, which
> is a little odd... Shouldn't this lock be taken in pci_epc_alloc_doorbell()
> instead ?
Yes, lock should be epc part.
>
> > +
> > + msg = kcalloc(num_db, sizeof(struct msi_msg), GFP_KERNEL);
> > + if (!msg)
> > + return -ENOMEM;
>
> You can do this allocation before taking the epc mutex lock.
>
> > +
> > + epf->num_db = num_db;
> > + epf->msg = msg;
> > +
> > + ret = pci_epc_alloc_doorbell(epc, epf->func_no, epf->vfunc_no, num_db);
> > + if (ret)
> > + kfree(msg);
>
> Looking at this, it seems that pci_epc_alloc_doorbell() should allocate msg and
> return a pointer to it (or ERR_PTR). This entire function would become:
There are callback "pci_epc_write_msi_msg()" need epf->msg. So need
allocate and init epf->msg before pci_epc_alloc_doorbell().
>
> msg = pci_epc_alloc_doorbell(epc, epf->func_no, epf->vfunc_no, num_db);
> if (IS_ERR(msg))
> return PTR_ERR(msg);
>
> epf->num_db = num_db;
> epf->msg = msg;
>
> return 0;
>
> > +
> > + return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(pci_epf_alloc_doorbell);
> > +
> > +void pci_epf_free_doorbell(struct pci_epf *epf)
> > +{
> > + struct pci_epc *epc = epf->epc;
> > +
> > + guard(mutex)(&epc->lock);
>
> Same comment about the location of this lock. That should be in
> pci_epc_free_doorbell() I think.
Yes
>
> > +
> > + epc = epf->epc;
>
> You did that at delaration time already. But this epc variable is used only
> below so it is not really needed at all.
>
> > + pci_epc_free_doorbell(epc, epf->func_no, epf->vfunc_no);
> > +
> > + kfree(epf->msg);
>
> This also should be in pci_epc_free_doorbell. So something like:
See above alloc problem.
Frank
>
> pci_epc_free_doorbell(epf->epc, epf->func_no, epf->vfunc_no, epf->msg);
>
> to be consistent with the changed proposed above for the allloc function.
>
> > + epf->msg = NULL;
> > + epf->num_db = 0;
> > +}
> > +EXPORT_SYMBOL_GPL(pci_epf_free_doorbell);
> > diff --git a/include/linux/pci-ep-msi.h b/include/linux/pci-ep-msi.h
> > new file mode 100644
> > index 0000000000000..f0cfecf491199
> > --- /dev/null
> > +++ b/include/linux/pci-ep-msi.h
> > @@ -0,0 +1,15 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * PCI Endpoint *Function* side MSI header file
> > + *
> > + * Copyright (C) 2024 NXP
> > + * Author: Frank Li <Frank.Li@xxxxxxx>
> > + */
> > +
> > +#ifndef __PCI_EP_MSI__
> > +#define __PCI_EP_MSI__
> > +
> > +int pci_epf_alloc_doorbell(struct pci_epf *epf, u16 nums);
> > +void pci_epf_free_doorbell(struct pci_epf *epf);
> > +
> > +#endif /* __PCI_EP_MSI__ */
>
> I do not see the point of this file. Why not add these function declarations to
> include/linux/pci-epf.h to keep all EPF API together ?
Mani prefer a seperate header file at v2 see
https://lore.kernel.org/imx/20231020181008.GF46191@thinkpad/
>
> > diff --git a/include/linux/pci-epf.h b/include/linux/pci-epf.h
> > index 18a3aeb62ae4e..1e7e5eb4067d7 100644
> > --- a/include/linux/pci-epf.h
> > +++ b/include/linux/pci-epf.h
> > @@ -75,6 +75,7 @@ struct pci_epf_ops {
> > * @link_up: Callback for the EPC link up event
> > * @link_down: Callback for the EPC link down event
> > * @bus_master_enable: Callback for the EPC Bus Master Enable event
> > + * @doorbell: Callback for EPC receive MSI from RC side
> > */
> > struct pci_epc_event_ops {
> > int (*epc_init)(struct pci_epf *epf);
> > @@ -82,6 +83,7 @@ struct pci_epc_event_ops {
> > int (*link_up)(struct pci_epf *epf);
> > int (*link_down)(struct pci_epf *epf);
> > int (*bus_master_enable)(struct pci_epf *epf);
> > + int (*doorbell)(struct pci_epf *epf, int index);
> > };
> >
> > /**
> > @@ -152,6 +154,8 @@ struct pci_epf_bar {
> > * @vfunction_num_map: bitmap to manage virtual function number
> > * @pci_vepf: list of virtual endpoint functions associated with this function
> > * @event_ops: Callbacks for capturing the EPC events
> > + * @msg: data for MSI from RC side
> > + * @num_db: number of doorbells
> > */
> > struct pci_epf {
> > struct device dev;
> > @@ -182,6 +186,8 @@ struct pci_epf {
> > unsigned long vfunction_num_map;
> > struct list_head pci_vepf;
> > const struct pci_epc_event_ops *event_ops;
> > + struct msi_msg *msg;
> > + u16 num_db;
> > };
> >
> > /**
> >
>
>
> --
> Damien Le Moal
> Western Digital Research
