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: <Z0S7+U5W2DOmzdJL@lizhi-Precision-Tower-5810>
Date: Mon, 25 Nov 2024 13:03:37 -0500
From: Frank Li <Frank.li@....com>
To: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
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 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.

>
> > +	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
>
> --
> மணிவண்ணன் சதாசிவம்