Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 17 Jun 2022 13:40:46 -0700
From:   Davidlohr Bueso <dave@stgolabs.net>
To:     "ira.weiny@intel.com" <ira.weiny@intel.com>
Cc:     Dan Williams <dan.j.williams@intel.com>,
        Bjorn Helgaas <bhelgaas@google.com>,
        Jonathan Cameron <Jonathan.Cameron@huawei.com>,
        Ben Widawsky <bwidawsk@kernel.org>,
        Alison Schofield <alison.schofield@intel.com>,
        Vishal Verma <vishal.l.verma@intel.com>,
        Dave Jiang <dave.jiang@intel.com>,
        linux-kernel@vger.kernel.org, linux-cxl@vger.kernel.org,
        linux-pci@vger.kernel.org, a.manzanares@samsung.com
Subject: Re: [PATCH v11 4/8] cxl/pci: Create PCI DOE mailbox's for memory
 devices
Message-ID: <20220617204046.qdkza6iemkfv2aze@offworld>
References: <20220610202259.3544623-1-ira.weiny@intel.com>
 <20220610202259.3544623-5-ira.weiny@intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii; format=flowed
Content-Disposition: inline
In-Reply-To: <20220610202259.3544623-5-ira.weiny@intel.com>
User-Agent: NeoMutt/20220429
Precedence: bulk

On Fri, 10 Jun 2022, ira.weiny@intel.com wrote:
>+++ b/drivers/cxl/cxlmem.h
>@@ -191,6 +191,8 @@ struct cxl_endpoint_dvsec_info {
>  * @component_reg_phys: register base of component registers
>  * @info: Cached DVSEC information about the device.
>  * @serial: PCIe Device Serial Number

Missing doc:

@doe_use_irq: Use interrupt vectors for DOEs over polling.

However introducing such flags is not pretty, and this is only used by
devm_cxl_pci_create_doe(). Do we really need it? See below.

>+ * @doe_mbs: PCI DOE mailbox array
>+ * @num_mbs: Number of DOE mailboxes
>  * @mbox_send: @dev specific transport for transmitting mailbox commands
>  *
>  * See section 8.2.9.5.2 Capacity Configuration and Label Storage for
>@@ -224,6 +226,10 @@ struct cxl_dev_state {
>	resource_size_t component_reg_phys;
>	u64 serial;
>
>+	bool doe_use_irq;
>+	struct pci_doe_mb **doe_mbs;
>+	int num_mbs;
>+
>	int (*mbox_send)(struct cxl_dev_state *cxlds, struct cxl_mbox_cmd *cmd);
> };
>
>diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
>index 5a0ae46d4989..72c7b535f5df 100644
>--- a/drivers/cxl/pci.c
>+++ b/drivers/cxl/pci.c
>@@ -8,6 +8,7 @@
> #include <linux/mutex.h>
> #include <linux/list.h>
> #include <linux/pci.h>
>+#include <linux/pci-doe.h>
> #include <linux/io.h>
> #include "cxlmem.h"
> #include "cxlpci.h"
>@@ -386,6 +387,116 @@ static int cxl_setup_regs(struct pci_dev *pdev, enum cxl_regloc_type type,
>	return rc;
> }
>
>+static void cxl_pci_free_irq_vectors(void *data)
>+{
>+	pci_free_irq_vectors(data);
>+}
>+
>+static void cxl_doe_destroy_mb(void *ds)
>+{
>+	struct cxl_dev_state *cxlds = ds;
>+	int i;
>+
>+	for (i = 0; i < cxlds->num_mbs; i++) {
>+		if (cxlds->doe_mbs[i])
>+			pci_doe_destroy_mb(cxlds->doe_mbs[i]);
>+	}
>+}
>+
>+static void cxl_alloc_irq_vectors(struct cxl_dev_state *cxlds)
>+{
>+	struct device *dev = cxlds->dev;
>+	struct pci_dev *pdev = to_pci_dev(dev);
>+	int max_irqs = 0;
>+	int off = 0;
>+	int rc;
>+
>+	/* Account for all the DOE vectors needed */
>+	pci_doe_for_each_off(pdev, off) {
>+		int irq = pci_doe_get_irq_num(pdev, off);
>+
>+		if (irq < 0)
>+			continue;
>+		max_irqs = max(max_irqs, irq + 1);
>+	}
>+
>+	if (!max_irqs)
>+		return;
>+
>+	cxlds->doe_use_irq = false;

Is this unnecessary, it should already be 0 per the devm_kzalloc().

>+
>+	/*
>+	 * Allocate enough vectors for the DOE's
>+	 */
>+	rc = pci_alloc_irq_vectors(pdev, max_irqs, max_irqs, PCI_IRQ_MSI |
>+							     PCI_IRQ_MSIX);
>+	if (rc != max_irqs) {
>+		pci_err(pdev, "Not enough interrupts; use polling\n");
>+		/* Some got allocated; clean them up */
>+		if (rc > 0)
>+			cxl_pci_free_irq_vectors(pdev);
>+		return;
>+	}
>+
>+	rc = devm_add_action_or_reset(dev, cxl_pci_free_irq_vectors, pdev);
>+	if (rc)
>+		return;
>+
>+	cxlds->doe_use_irq = true;
>+}
>+
>+/**
>+ * devm_cxl_pci_create_doe - Scan and set up DOE mailboxes
>+ *
>+ * @cxlds: The CXL device state
>+ */
>+static void devm_cxl_pci_create_doe(struct cxl_dev_state *cxlds)
>+{
>+	struct device *dev = cxlds->dev;
>+	struct pci_dev *pdev = to_pci_dev(dev);
>+	u16 off = 0;
>+	int num_mbs = 0;
>+	int rc;
>+
>+	pci_doe_for_each_off(pdev, off)
>+		num_mbs++;
>+
>+	if (!num_mbs) {
>+		pci_dbg(pdev, "0 DOE mailbox's found\n");
>+		return;
>+	}
>+
>+	cxlds->doe_mbs = devm_kcalloc(dev, num_mbs, sizeof(*cxlds->doe_mbs),
>+				      GFP_KERNEL);
>+	if (!cxlds->doe_mbs)
>+		return;
>+
>+	pci_doe_for_each_off(pdev, off) {
>+		struct pci_doe_mb *doe_mb;
>+		int irq = -1;
>+
>+		if (cxlds->doe_use_irq)
>+			irq = pci_doe_get_irq_num(pdev, off);
>+
>+		doe_mb = pci_doe_create_mb(pdev, off, irq);
>+		if (IS_ERR(doe_mb)) {
>+			pci_err(pdev,
>+				"Failed to create MB object for MB @ %x\n",
>+				off);
>+			doe_mb = NULL;
>+		}
>+
>+		cxlds->doe_mbs[cxlds->num_mbs] = doe_mb;
>+		cxlds->num_mbs++;
>+	}
>+
>+	rc = devm_add_action_or_reset(dev, cxl_doe_destroy_mb, cxlds);
>+	if (rc)
>+		return;
>+
>+	pci_info(pdev, "Configured %d DOE mailbox's\n", cxlds->num_mbs);
>+}
>+
> static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> {
>	struct cxl_register_map map;
>@@ -434,6 +545,9 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>
>	cxlds->component_reg_phys = cxl_regmap_to_base(pdev, &map);
>
>+	cxl_alloc_irq_vectors(cxlds);
>+	devm_cxl_pci_create_doe(cxlds);

Should cxl_alloc_irq_vectors() just be called directly from devm_cxl_pci_create_doe()
instead? Also if devm_cxl_pci_create_doe() fails (say ENOMEM), why do we
bother continuing the cxl_pci probing?

Thanks,
Davidlohr

------
diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
index ce5b00f3ebcb..44098c785a8b 100644
--- a/drivers/cxl/cxlmem.h
+++ b/drivers/cxl/cxlmem.h
@@ -230,7 +230,6 @@ struct cxl_dev_state {
	resource_size_t component_reg_phys;
	u64 serial;

-	bool doe_use_irq;
	struct pci_doe_mb **doe_mbs;
	int num_mbs;

diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index 72c7b535f5df..47c3741f7768 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -403,7 +403,7 @@ static void cxl_doe_destroy_mb(void *ds)
	}
  }

-static void cxl_alloc_irq_vectors(struct cxl_dev_state *cxlds)
+static int cxl_alloc_irq_vectors(struct cxl_dev_state *cxlds)
  {
	struct device *dev = cxlds->dev;
	struct pci_dev *pdev = to_pci_dev(dev);
@@ -421,9 +421,7 @@ static void cxl_alloc_irq_vectors(struct cxl_dev_state *cxlds)
	}

	if (!max_irqs)
-		return;
-
-	cxlds->doe_use_irq = false;
+		return -ENOMEM;

	/*
	 * Allocate enough vectors for the DOE's
@@ -435,14 +433,10 @@ static void cxl_alloc_irq_vectors(struct cxl_dev_state *cxlds)
		/* Some got allocated; clean them up */
		if (rc > 0)
			cxl_pci_free_irq_vectors(pdev);
-		return;
+		return -ENOMEM;
	}

-	rc = devm_add_action_or_reset(dev, cxl_pci_free_irq_vectors, pdev);
-	if (rc)
-		return;
-
-	cxlds->doe_use_irq = true;
+	return devm_add_action_or_reset(dev, cxl_pci_free_irq_vectors, pdev);
  }

  /**
@@ -457,6 +451,10 @@ static void devm_cxl_pci_create_doe(struct cxl_dev_state *cxlds)
	u16 off = 0;
	int num_mbs = 0;
	int rc;
+	bool doe_use_irq = false;
+
+	if (cxl_alloc_irq_vectors(cxlds))
+		doe_use_irq = true;

	pci_doe_for_each_off(pdev, off)
		num_mbs++;
@@ -475,7 +473,7 @@ static void devm_cxl_pci_create_doe(struct cxl_dev_state *cxlds)
		struct pci_doe_mb *doe_mb;
		int irq = -1;

-		if (cxlds->doe_use_irq)
+		if (doe_use_irq)
			irq = pci_doe_get_irq_num(pdev, off);

		doe_mb = pci_doe_create_mb(pdev, off, irq);
@@ -545,7 +543,6 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)

	cxlds->component_reg_phys = cxl_regmap_to_base(pdev, &map);

-	cxl_alloc_irq_vectors(cxlds);
	devm_cxl_pci_create_doe(cxlds);

	rc = cxl_pci_setup_mailbox(cxlds);