Message-ID: <c3812ef0-dd17-a6f5-432a-93d98aff70b7@amd.com>
Date: Wed, 8 Jan 2025 14:32:16 +0000
From: Alejandro Lucero Palau <alucerop@....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
Subject: Re: [PATCH v8 01/27] cxl: add type2 device basic support


On 1/8/25 01:33, Dan Williams wrote:
> 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:


Ok. but it is a good way of showing what you had in your mind about the 
suggested changes.

I'll use it for v10.

Thanks


>
> -- 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