Re: [PATCH v8 2/6] PCI: endpoint: Add RC-to-EP doorbell support using platform MSI controller

[Frank Li <Frank.li@nxp.com>]

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@kernel.org>
> > > > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > > > ---
> > > > 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@nxp.com>
> > > > + */
> > > > +
> > > > +#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
>
> --
> மணிவண்ணன் சதாசிவம்