On Sun, Nov 24, 2024 at 12:41:00PM +0530, Manivannan Sadhasivam wrote:
> On Sat, Nov 16, 2024 at 09:40:42AM -0500, 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.
> >
> > Tested-by: Niklas Cassel <cassel@xxxxxxxxxx>
> > Signed-off-by: Frank Li <Frank.Li@xxxxxxx>
> > ---
> > change from v5 to v8
> > -none
> >
> > Change from v4 to v5
> > - Remove request_irq() in pci_epc_alloc_doorbell() and leave to EP function
> > driver, so ep function driver can register differece call back function for
> > difference doorbell events and set irq affinity to differece CPU core.
> > - Improve error message when MSI allocate failure.
> >
> > Change from v3 to v4
> > - msi change to use msi_get_virq() avoid use msi_for_each_desc().
> > - add new struct for pci_epf_doorbell_msg to msi msg,virq and irq name.
> > - move mutex lock to epc function
> > - initialize variable at declear place.
> > - passdown epf to epc*() function to simplify code.
> > ---
> > drivers/pci/endpoint/Makefile | 2 +-
> > drivers/pci/endpoint/pci-ep-msi.c | 99 +++++++++++++++++++++++++++++++++++++++
> > include/linux/pci-ep-msi.h | 15 ++++++
> > include/linux/pci-epf.h | 16 +++++++
> > 4 files changed, 131 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..7868a529dce37
> > --- /dev/null
> > +++ b/drivers/pci/endpoint/pci-ep-msi.c
> > @@ -0,0 +1,99 @@
> > +// 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>
>
> Please sort alphabetically.
>
> > +#include <linux/module.h>
> > +#include <linux/msi.h>
> > +#include <linux/pci-epc.h>
> > +#include <linux/pci-epf.h>
> > +#include <linux/pci-ep-cfs.h>
> > +#include <linux/pci-ep-msi.h>
> > +
> > +static 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);
>
> You were passing 'epc->dev.parent' to platform_device_msi_init_and_alloc_irqs().
> So 'desc->dev' should be the EPC parent, right? If so, you can do:
>
> epc = pci_epc_get(dev_name(msi_desc_to_dev(desc)));
>
> since we are reusing the parent dev name for EPC.
I think it is not good to depend on hidden situation, "name is the same."
May it change in future because no one will realize here depend on the same
name and just think it is trivial update for device name.
>
> > + if (!epc)
> > + return;
> > +
> > + /* Only support one EPF for doorbell */
> > + epf = list_first_entry_or_null(&epc->pci_epf, struct pci_epf, list);
>
> Why don't you impose this restriction in pci_epf_alloc_doorbell() itself?
This is callback from platform_device_msi_init_and_alloc_irqs(). So it is
actually restriction at pci_epf_alloc_doorbell().
>
> > +
> > + if (epf && epf->db_msg && desc->msi_index < epf->num_db)
> > + memcpy(&epf->db_msg[desc->msi_index].msg, msg, sizeof(*msg));
> > +}
> > +
> > +static void pci_epc_free_doorbell(struct pci_epc *epc, struct pci_epf *epf)
> > +{
> > + guard(mutex)(&epc->lock);
> > +
> > + platform_device_msi_free_irqs_all(epc->dev.parent);
> > +}
> > +
> > +static int pci_epc_alloc_doorbell(struct pci_epc *epc, struct pci_epf *epf)
> > +{
> > + struct device *dev = epc->dev.parent;
> > + u16 num_db = epf->num_db;
> > + int i = 0;
> > + int ret;
> > +
> > + guard(mutex)(&epc->lock);
> > +
> > + 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, may miss 'msi-parent' at your DTS\n");
>
> No need to mention 'msi-parent'. Just 'Failed to allocate MSI' is enough.
>
> > + return -ENOMEM;
>
> -ENODEV?
>
> - Mani
>
> --
> மணிவண்ணன் சதாசிவம்
