| Message ID | 20241116-ep-msi-v8-2-6f1f68ffd1bb@nxp.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | PCI: EP: Add RC-to-EP doorbell with platform MSI controller | expand |
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. > + 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? > + > + 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
On 24 November 2024 08:11:00 CET, Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> wrote: >On Sat, Nov 16, 2024 at 09:40:42AM -0500, Frank Li wrote: >> +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. If you look at the existing pcie_ep device tree nodes for all SoCs, you will see that it is very rare to see an EP node which specifies msi-parent. I guess that makes sense, since before this series, AFAICT, msi-parent was completely unused, so there was no need for an EP node to specify it. I'm okay to change the error print as you suggested, but in that case I really think that we should add a comment above the check, something suggestive like: /* * The pcie_ep DT node has to specify * 'msi-parent' for EP doorbell support to work. * Right now only GIC ITS is supported. * If you have GIC ITS and reached this print, * perhaps you are missing 'msi-parent' in DT? */ if (ret) { dev_err(dev, "Failed to allocate MSI\n"); return -ENODEV; } Kind regards, Niklas
On Sun, Nov 24, 2024 at 10:56:38AM +0100, Niklas Cassel wrote: > > > On 24 November 2024 08:11:00 CET, Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> wrote: > >On Sat, Nov 16, 2024 at 09:40:42AM -0500, Frank Li wrote: > >> +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. > > If you look at the existing pcie_ep device tree nodes for all SoCs, you will see that it is very rare to see an EP node which specifies msi-parent. > > I guess that makes sense, since before this series, AFAICT, msi-parent was completely unused, so there was no need for an EP node to specify it. > > I'm okay to change the error print as you suggested, but in that case I really think that we should add a comment above the check, something suggestive like: > > /* > * The pcie_ep DT node has to specify > * 'msi-parent' for EP doorbell support to work. > * Right now only GIC ITS is supported. > * If you have GIC ITS and reached this print, > * perhaps you are missing 'msi-parent' in DT? > */ Looks good to me (except that the comment needs to fit 80 columns) :) - Mani > if (ret) { > dev_err(dev, "Failed to allocate MSI\n"); > return -ENODEV; > } > > > Kind regards, > Niklas
On 24 November 2024 14:17:01 CET, Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> wrote: >On Sun, Nov 24, 2024 at 10:56:38AM +0100, Niklas Cassel wrote: >> >> I'm okay to change the error print as you suggested, but in that case I really think that we should add a comment above the check, something suggestive like: >> >> /* >> * The pcie_ep DT node has to specify >> * 'msi-parent' for EP doorbell support to work. >> * Right now only GIC ITS is supported. >> * If you have GIC ITS and reached this print, >> * perhaps you are missing 'msi-parent' in DT? >> */ > >Looks good to me (except that the comment needs to fit 80 columns) :) Sorry about that, I wrote the reply from my phone :D Kind regards, Niklas
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. > > > + 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 > > -- > மணிவண்ணன் சதாசிவம்
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. - Mani
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 > > -- > மணிவண்ணன் சதாசிவம்
On Tue, Nov 26, 2024 at 12:19:01PM -0500, Frank Li wrote: [...] > > > > > + /* 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(). > No, these two are not the same. pci_epc_write_msi_msg() will only be called when enabling the MSI, which is too late IMO. Why are you insisting in calling platform_device_msi_init_and_alloc_irqs() for multi EPF? I don't quite understand. If the platform cannot support it, then it should not be called in first place. > 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. > This is not a good argument and doesn't make sense, sorry. You should not call platform_device_msi_init_and_alloc_irqs() for multi EPF. - Mani
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> +#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); + if (!epc) + return; + + /* Only support one EPF for doorbell */ + epf = list_first_entry_or_null(&epc->pci_epf, struct pci_epf, list); + + 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"); + return -ENOMEM; + } + + for (i = 0; i < num_db; i++) + epf->db_msg[i].virq = msi_get_virq(dev, i); + + return 0; +}; + +int pci_epf_alloc_doorbell(struct pci_epf *epf, u16 num_db) +{ + struct pci_epc *epc = epf->epc; + void *msg; + int ret; + + msg = kcalloc(num_db, sizeof(struct pci_epf_doorbell_msg), GFP_KERNEL); + if (!msg) + return -ENOMEM; + + epf->num_db = num_db; + epf->db_msg = msg; + + ret = pci_epc_alloc_doorbell(epc, epf); + if (ret) + kfree(msg); + + return ret; +} +EXPORT_SYMBOL_GPL(pci_epf_alloc_doorbell); + +void pci_epf_free_doorbell(struct pci_epf *epf) +{ + struct pci_epc *epc = epf->epc; + + pci_epc_free_doorbell(epc, epf); + + kfree(epf->db_msg); + epf->db_msg = NULL; + epf->num_db = 0; +} +EXPORT_SYMBOL_GPL(pci_epf_free_doorbell); diff --git a/include/linux/pci-ep-msi.h b/include/linux/pci-ep-msi.h new file mode 100644 index 0000000000000..f0cfecf491199 --- /dev/null +++ b/include/linux/pci-ep-msi.h @@ -0,0 +1,15 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* + * PCI Endpoint *Function* side MSI header file + * + * Copyright (C) 2024 NXP + * Author: Frank Li <Frank.Li@nxp.com> + */ + +#ifndef __PCI_EP_MSI__ +#define __PCI_EP_MSI__ + +int pci_epf_alloc_doorbell(struct pci_epf *epf, u16 nums); +void pci_epf_free_doorbell(struct pci_epf *epf); + +#endif /* __PCI_EP_MSI__ */ diff --git a/include/linux/pci-epf.h b/include/linux/pci-epf.h index 18a3aeb62ae4e..5374e6515ffa0 100644 --- a/include/linux/pci-epf.h +++ b/include/linux/pci-epf.h @@ -12,6 +12,7 @@ #include <linux/configfs.h> #include <linux/device.h> #include <linux/mod_devicetable.h> +#include <linux/msi.h> #include <linux/pci.h> struct pci_epf; @@ -125,6 +126,17 @@ struct pci_epf_bar { int flags; }; +/** + * struct pci_epf_doorbell_msg - represents doorbell message + * @msi_msg: MSI message + * @virq: irq number of this doorbell MSI message + * @name: irq name for doorbell interrupt + */ +struct pci_epf_doorbell_msg { + struct msi_msg msg; + int virq; +}; + /** * struct pci_epf - represents the PCI EPF device * @dev: the PCI EPF device @@ -152,6 +164,8 @@ struct pci_epf_bar { * @vfunction_num_map: bitmap to manage virtual function number * @pci_vepf: list of virtual endpoint functions associated with this function * @event_ops: Callbacks for capturing the EPC events + * @db_msg: data for MSI from RC side + * @num_db: number of doorbells */ struct pci_epf { struct device dev; @@ -182,6 +196,8 @@ struct pci_epf { unsigned long vfunction_num_map; struct list_head pci_vepf; const struct pci_epc_event_ops *event_ops; + struct pci_epf_doorbell_msg *db_msg; + u16 num_db; }; /**