Message-ID: <677dd5ea832e6_f58f29444@dwillia2-xfh.jf.intel.com.notmuch>
Date: Tue, 7 Jan 2025 17:33:30 -0800
From: Dan Williams <dan.j.williams@...el.com>
To: Dan Williams <dan.j.williams@...el.com>, <alejandro.lucero-palau@....com>,
	<linux-cxl@...r.kernel.org>, <netdev@...r.kernel.org>,
	<martin.habets@...inx.com>, <edward.cree@....com>, <davem@...emloft.net>,
	<kuba@...nel.org>, <pabeni@...hat.com>, <edumazet@...gle.com>,
	<dave.jiang@...el.com>
CC: Alejandro Lucero <alucerop@....com>
Subject: Re: [PATCH v8 01/27] cxl: add type2 device basic support

Dan Williams wrote:
> alejandro.lucero-palau@ wrote:
> > From: Alejandro Lucero <alucerop@....com>
> > 
> > Differentiate CXL memory expanders (type 3) from CXL device accelerators
> > (type 2) with a new function for initializing cxl_dev_state.
> > 
> > Create accessors to cxl_dev_state to be used by accel drivers.
> > 
> > Based on previous work by Dan Williams [1]
> > 
> > Link: [1] https://lore.kernel.org/linux-cxl/168592160379.1948938.12863272903570476312.stgit@dwillia2-xfh.jf.intel.com/
> > Signed-off-by: Alejandro Lucero <alucerop@....com>
> > Co-developed-by: Dan Williams <dan.j.williams@...el.com>
> > Reviewed-by: Dave Jiang <dave.jiang@...el.com>
> > Reviewed-by: Fan Ni <fan.ni@...sung.com>
> 
> This patch causes 

Whoops, forgot to complete this thought. Someting in this series causes:

depmod: ERROR: Cycle detected: ecdh_generic
depmod: ERROR: Cycle detected: tpm
depmod: ERROR: Cycle detected: cxl_mock -> cxl_core -> cxl_mock
depmod: ERROR: Cycle detected: encrypted_keys
depmod: ERROR: Found 2 modules in dependency cycles!

I think the non CXL ones are false likely triggered by the CXL causing
depmod to exit early.

Given cxl-test is unfamiliar territory to many submitters I always offer
to fix up the breakage. I came up with the below incremental patch to
fold in that also addresses my other feedback.

Now the depmod error is something Alison saw too, and while I can also
see it on patch1 if I do:

- apply whole series
- build => see the error
- rollback patch1
- build => see the error

...a subsequent build the error goes away, so I think that transient
behavior is a quirk of how cxl-test is built, but some later patch in
that series makes the failure permanent.

In any event I figured that out after creating the below fixup and
realizing that it does not fix the cxl-test build issue:

-- 8< --
diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
index 548564c770c0..584766d34b05 100644
--- a/drivers/cxl/core/mbox.c
+++ b/drivers/cxl/core/mbox.c
@@ -1435,7 +1435,7 @@ int cxl_mailbox_init(struct cxl_mailbox *cxl_mbox, struct device *host)
 }
 EXPORT_SYMBOL_NS_GPL(cxl_mailbox_init, "CXL");
 
-struct cxl_memdev_state *cxl_memdev_state_create(struct device *dev)
+struct cxl_memdev_state *cxl_memdev_state_create(struct device *dev, u64 serial, u16 dvsec)
 {
 	struct cxl_memdev_state *mds;
 
@@ -1445,11 +1445,9 @@ struct cxl_memdev_state *cxl_memdev_state_create(struct device *dev)
 		return ERR_PTR(-ENOMEM);
 	}
 
+	cxl_dev_state_init(&mds->cxlds, dev, CXL_DEVTYPE_CLASSMEM, serial,
+			   dvsec);
 	mutex_init(&mds->event.log_lock);
-	mds->cxlds.dev = dev;
-	mds->cxlds.reg_map.host = dev;
-	mds->cxlds.reg_map.resource = CXL_RESOURCE_NONE;
-	mds->cxlds.type = CXL_DEVTYPE_CLASSMEM;
 	mds->ram_perf.qos_class = CXL_QOS_CLASS_INVALID;
 	mds->pmem_perf.qos_class = CXL_QOS_CLASS_INVALID;
 
diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
index 99f533caae1e..9b8b9b4d1392 100644
--- a/drivers/cxl/core/memdev.c
+++ b/drivers/cxl/core/memdev.c
@@ -617,24 +617,18 @@ static void detach_memdev(struct work_struct *work)
 
 static struct lock_class_key cxl_memdev_key;
 
-struct cxl_dev_state *cxl_accel_state_create(struct device *dev)
+void cxl_dev_state_init(struct cxl_dev_state *cxlds, struct device *dev,
+			enum cxl_devtype type, u64 serial, u16 dvsec)
 {
-	struct cxl_dev_state *cxlds;
-
-	cxlds = kzalloc(sizeof(*cxlds), GFP_KERNEL);
-	if (!cxlds)
-		return ERR_PTR(-ENOMEM);
-
 	cxlds->dev = dev;
-	cxlds->type = CXL_DEVTYPE_DEVMEM;
+	cxlds->type = type;
+	cxlds->reg_map.host = dev;
+	cxlds->reg_map.resource = CXL_RESOURCE_NONE;
 
 	cxlds->dpa_res = DEFINE_RES_MEM_NAMED(0, 0, "dpa");
 	cxlds->ram_res = DEFINE_RES_MEM_NAMED(0, 0, "ram");
 	cxlds->pmem_res = DEFINE_RES_MEM_NAMED(0, 0, "pmem");
-
-	return cxlds;
 }
-EXPORT_SYMBOL_NS_GPL(cxl_accel_state_create, "CXL");
 
 static struct cxl_memdev *cxl_memdev_alloc(struct cxl_dev_state *cxlds,
 					   const struct file_operations *fops)
@@ -713,37 +707,6 @@ static int cxl_memdev_open(struct inode *inode, struct file *file)
 	return 0;
 }
 
-void cxl_set_dvsec(struct cxl_dev_state *cxlds, u16 dvsec)
-{
-	cxlds->cxl_dvsec = dvsec;
-}
-EXPORT_SYMBOL_NS_GPL(cxl_set_dvsec, "CXL");
-
-void cxl_set_serial(struct cxl_dev_state *cxlds, u64 serial)
-{
-	cxlds->serial = serial;
-}
-EXPORT_SYMBOL_NS_GPL(cxl_set_serial, "CXL");
-
-int cxl_set_resource(struct cxl_dev_state *cxlds, struct resource res,
-		     enum cxl_resource type)
-{
-	switch (type) {
-	case CXL_RES_DPA:
-		cxlds->dpa_res = res;
-		return 0;
-	case CXL_RES_RAM:
-		cxlds->ram_res = res;
-		return 0;
-	case CXL_RES_PMEM:
-		cxlds->pmem_res = res;
-		return 0;
-	}
-
-	return -EINVAL;
-}
-EXPORT_SYMBOL_NS_GPL(cxl_set_resource, "CXL");
-
 static int cxl_memdev_release_file(struct inode *inode, struct file *file)
 {
 	struct cxl_memdev *cxlmd =
diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
index 2a25d1957ddb..1e4b64b8f35a 100644
--- a/drivers/cxl/cxlmem.h
+++ b/drivers/cxl/cxlmem.h
@@ -4,6 +4,7 @@
 #define __CXL_MEM_H__
 #include <uapi/linux/cxl_mem.h>
 #include <linux/pci.h>
+#include <cxl/cxl.h>
 #include <linux/cdev.h>
 #include <linux/uuid.h>
 #include <linux/node.h>
@@ -380,20 +381,6 @@ struct cxl_security_state {
 	struct kernfs_node *sanitize_node;
 };
 
-/*
- * enum cxl_devtype - delineate type-2 from a generic type-3 device
- * @CXL_DEVTYPE_DEVMEM - Vendor specific CXL Type-2 device implementing HDM-D or
- *			 HDM-DB, no requirement that this device implements a
- *			 mailbox, or other memory-device-standard manageability
- *			 flows.
- * @CXL_DEVTYPE_CLASSMEM - Common class definition of a CXL Type-3 device with
- *			   HDM-H and class-mandatory memory device registers
- */
-enum cxl_devtype {
-	CXL_DEVTYPE_DEVMEM,
-	CXL_DEVTYPE_CLASSMEM,
-};
-
 /**
  * struct cxl_dpa_perf - DPA performance property entry
  * @dpa_range: range for DPA address
@@ -411,9 +398,9 @@ struct cxl_dpa_perf {
 /**
  * struct cxl_dev_state - The driver device state
  *
- * cxl_dev_state represents the CXL driver/device state.  It provides an
- * interface to mailbox commands as well as some cached data about the device.
- * Currently only memory devices are represented.
+ * cxl_dev_state represents the minimal data about a CXL device to allow
+ * the CXL core to manage common initialization of generic CXL and HDM capabilities of
+ * memory expanders and accelerators with device-memory
  *
  * @dev: The device associated with this CXL state
  * @cxlmd: The device representing the CXL.mem capabilities of @dev
@@ -426,7 +413,7 @@ struct cxl_dpa_perf {
  * @pmem_res: Active Persistent memory capacity configuration
  * @ram_res: Active Volatile memory capacity configuration
  * @serial: PCIe Device Serial Number
- * @type: Generic Memory Class device or Vendor Specific Memory device
+ * @type: Generic Memory Class device or an accelerator with CXL.mem
  * @cxl_mbox: CXL mailbox context
  */
 struct cxl_dev_state {
@@ -819,7 +806,8 @@ int cxl_dev_state_identify(struct cxl_memdev_state *mds);
 int cxl_await_media_ready(struct cxl_dev_state *cxlds);
 int cxl_enumerate_cmds(struct cxl_memdev_state *mds);
 int cxl_mem_create_range_info(struct cxl_memdev_state *mds);
-struct cxl_memdev_state *cxl_memdev_state_create(struct device *dev);
+struct cxl_memdev_state *cxl_memdev_state_create(struct device *dev, u64 serial,
+						 u16 dvsec);
 void set_exclusive_cxl_commands(struct cxl_memdev_state *mds,
 				unsigned long *cmds);
 void clear_exclusive_cxl_commands(struct cxl_memdev_state *mds,
diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index 36098e2b4235..b51e47fd28b3 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -922,21 +922,19 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 		return rc;
 	pci_set_master(pdev);
 
-	mds = cxl_memdev_state_create(&pdev->dev);
-	if (IS_ERR(mds))
-		return PTR_ERR(mds);
-	cxlds = &mds->cxlds;
-	pci_set_drvdata(pdev, cxlds);
-
-	cxlds->rcd = is_cxl_restricted(pdev);
-	cxl_set_serial(cxlds, pci_get_dsn(pdev));
 	dvsec = pci_find_dvsec_capability(pdev, PCI_VENDOR_ID_CXL,
 					  CXL_DVSEC_PCIE_DEVICE);
 	if (!dvsec)
 		dev_warn(&pdev->dev,
 			 "Device DVSEC not present, skip CXL.mem init\n");
 
-	cxl_set_dvsec(cxlds, dvsec);
+	mds = cxl_memdev_state_create(&pdev->dev, pci_get_dsn(pdev), dvsec);
+	if (IS_ERR(mds))
+		return PTR_ERR(mds);
+	cxlds = &mds->cxlds;
+	pci_set_drvdata(pdev, cxlds);
+
+	cxlds->rcd = is_cxl_restricted(pdev);
 
 	rc = cxl_pci_setup_regs(pdev, CXL_REGLOC_RBI_MEMDEV, &map);
 	if (rc)
diff --git a/include/cxl/cxl.h b/include/cxl/cxl.h
index aa4480d49e48..9db4fb6d2c74 100644
--- a/include/cxl/cxl.h
+++ b/include/cxl/cxl.h
@@ -4,21 +4,25 @@
 #ifndef __CXL_H
 #define __CXL_H
 
-#include <linux/ioport.h>
+#include <linux/types.h>
 
-enum cxl_resource {
-	CXL_RES_DPA,
-	CXL_RES_RAM,
-	CXL_RES_PMEM,
+/*
+ * enum cxl_devtype - delineate type-2 from a generic type-3 device
+ * @CXL_DEVTYPE_DEVMEM - Vendor specific CXL Type-2 device implementing HDM-D or
+ *			 HDM-DB, no requirement that this device implements a
+ *			 mailbox, or other memory-device-standard manageability
+ *			 flows.
+ * @CXL_DEVTYPE_CLASSMEM - Common class definition of a CXL Type-3 device with
+ *			   HDM-H and class-mandatory memory device registers
+ */
+enum cxl_devtype {
+	CXL_DEVTYPE_DEVMEM,
+	CXL_DEVTYPE_CLASSMEM,
 };
 
 struct cxl_dev_state;
 struct device;
 
-struct cxl_dev_state *cxl_accel_state_create(struct device *dev);
-
-void cxl_set_dvsec(struct cxl_dev_state *cxlds, u16 dvsec);
-void cxl_set_serial(struct cxl_dev_state *cxlds, u64 serial);
-int cxl_set_resource(struct cxl_dev_state *cxlds, struct resource res,
-		     enum cxl_resource);
+void cxl_dev_state_init(struct cxl_dev_state *cxlds, struct device *dev,
+			enum cxl_devtype type, u64 serial, u16 dvsec);
 #endif
diff --git a/tools/testing/cxl/test/mem.c b/tools/testing/cxl/test/mem.c
index 347c1e7b37bd..24cac1cc30f9 100644
--- a/tools/testing/cxl/test/mem.c
+++ b/tools/testing/cxl/test/mem.c
@@ -1500,7 +1500,7 @@ static int cxl_mock_mem_probe(struct platform_device *pdev)
 	if (rc)
 		return rc;
 
-	mds = cxl_memdev_state_create(dev);
+	mds = cxl_memdev_state_create(dev, pdev->id, 0);
 	if (IS_ERR(mds))
 		return PTR_ERR(mds);
 
@@ -1516,7 +1516,6 @@ static int cxl_mock_mem_probe(struct platform_device *pdev)
 	mds->event.buf = (struct cxl_get_event_payload *) mdata->event_buf;
 	INIT_DELAYED_WORK(&mds->security.poll_dwork, cxl_mockmem_sanitize_work);
 
-	cxlds->serial = pdev->id;
 	if (is_rcd(pdev))
 		cxlds->rcd = true;
 

Powered by blists - more mailing lists