On Tue, Nov 26, 2024 at 09:44:49AM +0530, Manivannan Sadhasivam wrote:
> On Mon, Nov 25, 2024 at 01:03:37PM -0500, Frank Li wrote:
> > 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.
> >
>
> No one should change the EPC name just like that. The name is exposed to
> configfs interface and the existing userspace scripts rely on that. So changing
> the name will break them.
>
> I'd strongly suggest you to use the existing API instead of adding a new one for
> the same purpose.
>
> > >
> > > > + 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().
> >
>
> I don't know how to parse this last sentence. But my question was why don't you
> impose this one EPF restriction in pci_epf_alloc_doorbell() before allocating
> the MSI domain using platform_device_msi_init_and_alloc_irqs()? This way if
> someone calls pci_epf_alloc_doorbell() multi EPF, it will fail.
Yes, it is limitation for current platfrom msi framework. It is totally
equal.
call stack is
pci_epf_alloc_doorbell()
platform_device_msi_init_and_alloc_irqs()
...
pci_epc_write_msi_msg()
It is totally equal return at pci_epc_write_msi_msg() or return before
platform_device_msi_init_and_alloc_irqs().
why check epf in pci_epc_write_msi_msg() is because it use epf in here.
pci_epf_alloc_doorbell() never use epf variable. If check unused variable
in pci_epf_alloc_doorbell(), user don't know why and what's exactly
restrict it.
Frank
>
> - Mani
>
> --
> மணிவண்ணன் சதாசிவம்
