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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20241126041449.qouyatajd5rie5o2@thinkpad>
Date: Tue, 26 Nov 2024 09:44:49 +0530
From: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
To: Frank Li <Frank.li@....com>
Cc: Krzysztof Wilczyński <kw@...ux.com>,
	Kishon Vijay Abraham I <kishon@...nel.org>,
	Bjorn Helgaas <bhelgaas@...gle.com>, Arnd Bergmann <arnd@...db.de>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	linux-kernel@...r.kernel.org, linux-pci@...r.kernel.org,
	imx@...ts.linux.dev, Niklas Cassel <cassel@...nel.org>,
	dlemoal@...nel.org, maz@...nel.org, tglx@...utronix.de,
	jdmason@...zu.us
Subject: Re: [PATCH v8 2/6] PCI: endpoint: Add RC-to-EP doorbell support
 using platform MSI controller

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@...nel.org>
> > > Signed-off-by: Frank Li <Frank.Li@....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@....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.

- Mani

-- 
மணிவண்ணன் சதாசிவம்