Re: [RFC type cxl initialization 1/2] cxl: add type2 device basic support

[Alejandro Lucero Palau <alucerop@amd.com>]

Hi Dan,


Thanks for the review. My comments below.


On 3/3/25 20:15, Dan Williams wrote:
> alejandro.lucero-palau@ wrote:
>> From: Alejandro Lucero <alucerop@amd.com>
>>
>> Differentiate CXL memory expanders (type 3) from CXL device accelerators
>> (type 2) with a new function for initializing cxl_dev_state and a macro
>> for helping accel drivers to embed cxl_dev_state inside a private
>> struct.
>>
>> Move structs to include/cxl as the size of the accel driver private
>> struct embedding cxl_dev_state needs to know the size of this struct.
>>
>> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
>> ---
>>   drivers/cxl/core/mbox.c   |   3 +-
>>   drivers/cxl/core/memdev.c |  25 +++++
>>   drivers/cxl/core/pci.c    |   1 +
>>   drivers/cxl/core/regs.c   |   1 +
>>   drivers/cxl/cxl.h         |  98 +----------------
>>   drivers/cxl/cxlmem.h      | 108 +-----------------
>>   drivers/cxl/cxlpci.h      |  21 ----
>>   drivers/cxl/pci.c         |  17 +--
>>   include/cxl/cxl.h         | 225 ++++++++++++++++++++++++++++++++++++++
>>   include/cxl/pci.h         |  23 ++++
>>   10 files changed, 293 insertions(+), 229 deletions(-)
>>   create mode 100644 include/cxl/cxl.h
>>   create mode 100644 include/cxl/pci.h
>>
>> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
>> index c5eedcae3b02..ae92db38a921 100644
>> --- a/drivers/cxl/core/mbox.c
>> +++ b/drivers/cxl/core/mbox.c
>> @@ -1426,7 +1426,8 @@ 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;
> I would ultimately expect that this starts to use the
> cxl_dev_state_create() internally as well to keep those internals
> common.
>

OK. I think I can do this for the impending v11.


>>   
>> diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
>> index 2914e3ff104b..14bd74823467 100644
>> --- a/drivers/cxl/core/memdev.c
>> +++ b/drivers/cxl/core/memdev.c
>> @@ -636,6 +636,31 @@ static void detach_memdev(struct work_struct *work)
>>   
>>   static struct lock_class_key cxl_memdev_key;
>>   
>> +void cxl_dev_state_init(struct cxl_dev_state *cxlds, struct device *dev,
>> +			enum cxl_devtype type, u64 serial, u16 dvsec)
>> +{
>> +	cxlds->dev = dev;
>> +	cxlds->type = type;
>> +	cxlds->serial = serial;
>> +	cxlds->cxl_dvsec = dvsec;
>> +	cxlds->reg_map.host = dev;
>> +	cxlds->reg_map.resource = CXL_RESOURCE_NONE;
> Let's take the opportunity to now use compound literal syntax which
> immediately answers the question of whether any new or not included
> fields are initialized. I.e.
>
>      *cxlds = (struct cxl_dev_state) {
> 	.dev = dev,
> 	...
>      };
>

OK.


>> +}
>> +
>> +struct cxl_dev_state *_cxl_dev_state_create(struct device *dev,
>> +					   u64 serial, u16 dvsec,
>> +					   size_t size)
>> +{
>> +	struct cxl_dev_state *cxlds __free(kfree) = kzalloc(size, GFP_KERNEL);
>> +
>> +	if (!cxlds)
>> +		return NULL;
>> +
>> +	cxl_dev_state_init(cxlds, dev, CXL_DEVTYPE_DEVMEM, serial, dvsec);
> I expect the devtype will need to be passed in as _cxl_dev_state_create
> can not assume memory expander vs an accelerator.


Yes, this is needed for the work to do related to the first comment. 
I'll do that.


<snip>


> Ok, all of the above make sense to move because they are needed to be
> able to export the size. It would be nice for the follow on patches to
> delineate which fields of 'struct cxl_dev_state' are expected to be
> private to the CXL core.
>

I'll do so.


<snip>

>>   
>> -/**
>> - * struct cxl_memdev - CXL bus object representing a Type-3 Memory Device
>> - * @dev: driver core device object
>> - * @cdev: char dev core object for ioctl operations
>> - * @cxlds: The device state backing this device
>> - * @detach_work: active memdev lost a port in its ancestry
>> - * @cxl_nvb: coordinate removal of @cxl_nvd if present
>> - * @cxl_nvd: optional bridge to an nvdimm if the device supports pmem
>> - * @endpoint: connection to the CXL port topology for this memory device
>> - * @id: id number of this memdev instance.
>> - * @depth: endpoint port depth
>> - */
>> -struct cxl_memdev {
>> -	struct device dev;
>> -	struct cdev cdev;
>> -	struct cxl_dev_state *cxlds;
>> -	struct work_struct detach_work;
>> -	struct cxl_nvdimm_bridge *cxl_nvb;
>> -	struct cxl_nvdimm *cxl_nvd;
>> -	struct cxl_port *endpoint;
>> -	int id;
>> -	int depth;
>> -};
> Why does this need to be public?


Good catch. This comes from the previous try for using this in v10. It 
is not needed for what the RFC tries to do.

<snip>

> Looks good.


I think I got what I need now for v11.

Thanks!