Message-ID: <92e3b256-b660-5074-f3aa-c8ab158fcb8b@amd.com>
Date: Tue, 14 Jan 2025 14:35:21 +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 14:32, Alejandro Lucero Palau wrote:
>
> 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
>

Hi Dan,


There's a problem with this approach and it is the need of the driver 
having access to internal cxl structs like cxl_dev_state.

Your patch does not cover it but for an accel driver that struct needs 
to be allocated before using the new cxl_dev_state_init.

I think we reached an agreement in initial discussions about avoiding 
this need through an API for accel drivers indirectly doing whatever is 
needed regarding internal CXL structs. Initially it was stated this 
being necessary for avoiding drivers doing wrong things but Jonathan 
pointed out the main concern being changing those internal structs in 
the future could benefit from this approach. Whatever the reason, that 
was the assumption.


I could add a function for accel drivers doing the allocation as with 
current v9 code, and then using your changes for having common code.


Also, I completely agree with merging the serial and dvsec 
initializations through arguments to cxl_dev_state_init, but we need the 
cxl_set_resource function for accel drivers. The current code for adding 
resources with memdev is relying on mbox commands, and although we could 
change that code for supporting accel drivers without an mbox, I would 
say the function/code added is simple enough for not requiring that 
effort. Note my goal is for an accel device without an mbox, but we will 
see devices with one in the future, so I bet for leaving any change 
there to that moment.

Let me know what you think about these two things. I would like to send 
v10 this week.


Thank you


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