Messages in this thread | | | Date | Thu, 1 Dec 2022 16:34:27 -0800 | From | Ira Weiny <> | Subject | Re: [PATCH V2 01/11] cxl/pci: Add generic MSI-X/MSI irq support |
| |
On Thu, Dec 01, 2022 at 04:23:21PM -0800, Dan Williams wrote: > ira.weiny@ wrote: > > From: Davidlohr Bueso <dave@stgolabs.net> > > > > Currently the only CXL features targeted for irq support require their > > message numbers to be within the first 16 entries. The device may > > however support less than 16 entries depending on the support it > > provides. > > > > Attempt to allocate these 16 irq vectors. If the device supports less > > then the PCI infrastructure will allocate that number. > > What happens if the device supports 16, but irq-core allocates less? I > believe the answer is with the first user, but this patch does not > include a user. > > > Store the number of vectors actually allocated in the device state for > > later use by individual functions. > > The patch does not do that.
Sorry missed updating this message.
> > I know this patch has gone through a lot of discussion, but this > mismatch shows it should really be squashed with the first user because > it does not stand on its own anymore.
It is separate because it was Davidlohr's to begin with.
I'll squash it back.
> > > Upon successful allocation, users can plug in their respective isr at > > any point thereafter, for example, if the irq setup is not done in the > > PCI driver, such as the case of the CXL-PMU. > > > > Cc: Bjorn Helgaas <helgaas@kernel.org> > > Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > Co-developed-by: Ira Weiny <ira.weiny@intel.com> > > Signed-off-by: Ira Weiny <ira.weiny@intel.com> > > Signed-off-by: Davidlohr Bueso <dave@stgolabs.net> > > > > --- > > Changes from V1: > > Jonathan > > pci_alloc_irq_vectors() cleans up the vectors automatically > > use msi_enabled rather than nr_irq_vecs > > > > Changes from Ira > > Remove reviews > > Allocate up to a static 16 vectors. > > Change cover letter > > --- > > drivers/cxl/cxlmem.h | 3 +++ > > drivers/cxl/cxlpci.h | 6 ++++++ > > drivers/cxl/pci.c | 23 +++++++++++++++++++++++ > > 3 files changed, 32 insertions(+) > > > > diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h > > index 88e3a8e54b6a..cd35f43fedd4 100644 > > --- a/drivers/cxl/cxlmem.h > > +++ b/drivers/cxl/cxlmem.h > > @@ -211,6 +211,7 @@ struct cxl_endpoint_dvsec_info { > > * @info: Cached DVSEC information about the device. > > * @serial: PCIe Device Serial Number > > * @doe_mbs: PCI DOE mailbox array > > + * @msi_enabled: MSI-X/MSI has been enabled > > * @mbox_send: @dev specific transport for transmitting mailbox commands > > * > > * See section 8.2.9.5.2 Capacity Configuration and Label Storage for > > @@ -247,6 +248,8 @@ struct cxl_dev_state { > > > > struct xarray doe_mbs; > > > > + bool msi_enabled; > > + > > This goes unused in this patch and it also duplicates what the core > offers with pdev->{msi,msix}_enabled.
I tried to argue that with Jonathan and lost. What I had in V1 was to store the number actually allocated. Then if a function reports something higher later it can't be used.
I admit that at this point I really don't understand PCI interrupts at all. Every time this patch is discussed I get (what is to me) confusing information. And I've been unable to discern from the spec how exactly this is all supposed to work.
> > > int (*mbox_send)(struct cxl_dev_state *cxlds, struct cxl_mbox_cmd *cmd); > > }; > > > > diff --git a/drivers/cxl/cxlpci.h b/drivers/cxl/cxlpci.h > > index eec597dbe763..b7f4e2f417d3 100644 > > --- a/drivers/cxl/cxlpci.h > > +++ b/drivers/cxl/cxlpci.h > > @@ -53,6 +53,12 @@ > > #define CXL_DVSEC_REG_LOCATOR_BLOCK_ID_MASK GENMASK(15, 8) > > #define CXL_DVSEC_REG_LOCATOR_BLOCK_OFF_LOW_MASK GENMASK(31, 16) > > > > +/* > > + * NOTE: Currently all the functions which are enabled for CXL require their > > + * vectors to be in the first 16. Use this as the max. > > + */ > > +#define CXL_PCI_REQUIRED_VECTORS 16 > > + > > /* Register Block Identifier (RBI) */ > > enum cxl_regloc_type { > > CXL_REGLOC_RBI_EMPTY = 0, > > diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c > > index faeb5d9d7a7a..8f86f85d89c7 100644 > > --- a/drivers/cxl/pci.c > > +++ b/drivers/cxl/pci.c > > @@ -428,6 +428,27 @@ static void devm_cxl_pci_create_doe(struct cxl_dev_state *cxlds) > > } > > } > > > > +static void cxl_pci_alloc_irq_vectors(struct cxl_dev_state *cxlds) > > +{ > > + struct device *dev = cxlds->dev; > > + struct pci_dev *pdev = to_pci_dev(dev); > > + int nvecs; > > + > > + /* > > + * NOTE: pci_alloc_irq_vectors() handles calling pci_free_irq_vectors() > > + * automatically despite not being called pcim_*. See > > + * pci_setup_msi_context(). > > + */ > > + nvecs = pci_alloc_irq_vectors(pdev, 1, CXL_PCI_REQUIRED_VECTORS, > > + PCI_IRQ_MSIX | PCI_IRQ_MSI); > > clang-format would scooch that second line in for you. > > Might also be worth a comment for the next person that goes looking for > why this isn't PCI_IRQ_ALL_TYPES. > > From CXL 3.0 3.1.1 CXL.io Endpoint: > A Function on a CXL device must not generate INTx messages if that > Function participates in CXL.cache protocol or CXL.mem protocols.
Seems reasonable.
Ira
> > > > + if (nvecs < 0) { > > + dev_dbg(dev, "Failed to alloc irq vectors; use polling instead.\n"); > > + return; > > + } > > + > > + cxlds->msi_enabled = true; > > +} > > + > > static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) > > { > > struct cxl_register_map map; > > @@ -494,6 +515,8 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) > > if (rc) > > return rc; > > > > + cxl_pci_alloc_irq_vectors(cxlds); > > + > > cxlmd = devm_cxl_add_memdev(cxlds); > > if (IS_ERR(cxlmd)) > > return PTR_ERR(cxlmd); > > -- > > 2.37.2 > > > >
| |