Re: [PATCH 1/2] cxl/pci: Add generic MSI-X/MSI irq support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Nov 03, 2022 at 06:09:16PM +0000, Jonathan Cameron wrote:
> On Wed, 2 Nov 2022 17:18:33 -0700
> Davidlohr Bueso <dave@xxxxxxxxxxxx> wrote:
> 
> > On Wed, 02 Nov 2022, Ira Weiny wrote:
> > 
> > >On Wed, Nov 02, 2022 at 10:15:24AM -0700, Davidlohr Bueso wrote:  
> > >> Most CXL features that can have irqs will normally use only the first 16,
> > >> with the exception of isolation (cxl 3.0), which per the spec is up to 32.  
> > >
> > >Dan, Dave, and I were discussing this and we agree.  For now the only things
> > >people are working on are within the first 16 so why not just request 16 as the
> > >max for now?  
> > 
> > It is a fair compromise, yes.
> 
> works for me.

I made what I thought would be a simple change to your patch and built this
into my series.

Unfortunately the following does not work with the current Qemu.

/*
 * NOTE: Currently all the functions which are enabled for CXL require their
 * vectors to be in the first 16.  Allocate this number as the min/max.
 */
#define CXL_PCI_REQUIRED_VECTORS 16

...

        rc = pci_alloc_irq_vectors(pdev, CXL_PCI_REQUIRED_VECTORS,            
                                   CXL_PCI_REQUIRED_VECTORS,
                                   PCI_IRQ_MSIX | PCI_IRQ_MSI);

This is because Qemu CXL devices only support (with the event changes I have
made) 8 msg numbers.  So the code fails to allocate any vectors.

I guess I should have known better.  But allocating something less than 16 I
guess needs to be allowed.

But that also means that beyond knowing _if_ irq's have been enabled I think
each CXL feature needs to know the number of vectors allocated so they can
ensure their msg numbers are going to work.

So how about the following as a diff to this patch?

In the event code I have then used the nr_irq_vecs field to determine if I
should enable the irq for each log.

If you are ok with it I'm going to squash it into your patch and send out a new
version of the event log series.

Thanks,
Ira


>From 105561243c800442a2b7ff39b931e73b0a89bc34 Mon Sep 17 00:00:00 2001
From: Ira Weiny <ira.weiny@xxxxxxxxx>
Date: Wed, 9 Nov 2022 12:35:07 -0800
Subject: [PATCH] squash: Allocate up to a static 16 vectors.

This covers the current desired features which CXL needs now.
---
 drivers/cxl/cxlmem.h |  4 +--
 drivers/cxl/cxlpci.h |  6 ++++
 drivers/cxl/pci.c    | 68 +++++++++-----------------------------------
 3 files changed, 22 insertions(+), 56 deletions(-)

diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
index 78ff6dca3c4b..03da4f8f74d3 100644
--- a/drivers/cxl/cxlmem.h
+++ b/drivers/cxl/cxlmem.h
@@ -212,7 +212,7 @@ struct cxl_endpoint_dvsec_info {
  * @info: Cached DVSEC information about the device.
  * @serial: PCIe Device Serial Number
  * @doe_mbs: PCI DOE mailbox array
- * @has_irq: PCIe MSI-X/MSI support
+ * @nr_irq_vecs: Number of MSI-X/MSI vectors available
  * @mbox_send: @dev specific transport for transmitting mailbox commands
  *
  * See section 8.2.9.5.2 Capacity Configuration and Label Storage for
@@ -249,7 +249,7 @@ struct cxl_dev_state {
 
 	struct xarray doe_mbs;
 
-	bool has_irq;
+	int nr_irq_vecs;
 
 	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 9dc32b802594..e0d511575b45 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -428,71 +428,34 @@ static void devm_cxl_pci_create_doe(struct cxl_dev_state *cxlds)
 	}
 }
 
-/**
- * struct cxl_irq_cap - CXL feature that is capable of receiving MSI-X/MSI irqs.
- *
- * @name: Name of the device/component generating this interrupt.
- * @get_max_msgnum: Get the feature's largest interrupt message number.  If the
- *		    feature does not have the Interrupt Supported bit set, then
- *		    return -1.
- */
-struct cxl_irq_cap {
-	const char *name;
-	int (*get_max_msgnum)(struct cxl_dev_state *cxlds);
-};
-
-static const struct cxl_irq_cap cxl_irq_cap_table[] = {
-	NULL
-};
-
 static void cxl_pci_free_irq_vectors(void *data)
 {
 	pci_free_irq_vectors(data);
 }
 
-/*
- * Attempt to allocate the largest amount of necessary vectors.
- *
- * Returns 0 upon a successful allocation of *all* vectors, or a
- * negative value otherwise.
- */
-static int cxl_pci_alloc_irq_vectors(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 rc, i, vectors = -1;
-
-	for (i = 0; i < ARRAY_SIZE(cxl_irq_cap_table); i++) {
-		int irq;
-
-		if (!cxl_irq_cap_table[i].get_max_msgnum)
-			continue;
-
-		irq = cxl_irq_cap_table[i].get_max_msgnum(cxlds);
-		vectors = max_t(int, irq, vectors);
-	}
-
-	/*
-	 * Semantically lack of irq support is not an error, but we
-	 * still fail to allocate, so return negative.
-	 */
-	if (vectors == -1)
-		return -1;
+	int nvecs;
+	int rc;
 
-	vectors++;
-	rc = pci_alloc_irq_vectors(pdev, vectors, vectors,
+	nvecs = pci_alloc_irq_vectors(pdev, 1, CXL_PCI_REQUIRED_VECTORS,
 				   PCI_IRQ_MSIX | PCI_IRQ_MSI);
-	if (rc < 0)
-		return rc;
-
-	if (rc != vectors) {
+	if (nvecs < 0) {
 		dev_dbg(dev, "Not enough interrupts; use polling instead.\n");
+		return;
+	}
+
+	rc = devm_add_action_or_reset(dev, cxl_pci_free_irq_vectors, pdev);
+	if (rc) {
+		dev_dbg(dev, "Device managed call failed; interrupts disabled.\n");
 		/* some got allocated, clean them up */
 		cxl_pci_free_irq_vectors(pdev);
-		return -ENOSPC;
+		return;
 	}
 
-	return devm_add_action_or_reset(dev, cxl_pci_free_irq_vectors, pdev);
+	cxlds->nr_irq_vecs = nvecs;
 }
 
 static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
@@ -561,10 +524,7 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	if (rc)
 		return rc;
 
-	if (!cxl_pci_alloc_irq_vectors(cxlds)) {
-		cxlds->has_irq = true;
-	} else
-		cxlds->has_irq = false;
+	cxl_pci_alloc_irq_vectors(cxlds);
 
 	cxlmd = devm_cxl_add_memdev(cxlds);
 	if (IS_ERR(cxlmd))

base-commit: aae703b02f92bde9264366c545e87cec451de471
prerequisite-patch-id: e3c882f3fce0872c2259538d00ad798236ec251f
prerequisite-patch-id: a8faa71e6d79cb30eae9b863349f0cb5ffa55b05
prerequisite-patch-id: f8e6edeb4a1d8bc4b34a509cb3d4a625becdf1b3
prerequisite-patch-id: 665a2b5af761a3f50e20da2fa8e5fdc9df13969d
prerequisite-patch-id: 5cd9f56597f9c8637201193849f68811a94d2309
prerequisite-patch-id: 89be9f2bd84118682b9b37e4f3a6e057fd4ad0d6
prerequisite-patch-id: 3de731a18a28c32572bf65f38e8eb2fb927e4f56
-- 
2.37.2




[Index of Archives]     [DMA Engine]     [Linux Coverity]     [Linux USB]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Greybus]

  Powered by Linux