Re: [PATCH v16 01/22] cxl: Add type2 device basic support

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

On 5/20/25 08:17, dan.j.williams@intel.com 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.
>>
>> Use same new initialization with the type3 pci driver.
>>
>> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
>> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>> Reviewed-by: Dave Jiang <dave.jiang@intel.com>
>> ---
>>   drivers/cxl/core/mbox.c      |  11 +-
>>   drivers/cxl/core/memdev.c    |  32 +++++
>>   drivers/cxl/core/pci.c       |   1 +
>>   drivers/cxl/core/regs.c      |   1 +
>>   drivers/cxl/cxl.h            |  97 +--------------
>>   drivers/cxl/cxlmem.h         |  88 +-------------
>>   drivers/cxl/cxlpci.h         |  21 ----
>>   drivers/cxl/pci.c            |  17 +--
>>   include/cxl/cxl.h            | 226 +++++++++++++++++++++++++++++++++++
>>   include/cxl/pci.h            |  23 ++++
>>   tools/testing/cxl/test/mem.c |   3 +-
>>   11 files changed, 305 insertions(+), 215 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 d72764056ce6..ab994d459f46 100644
>> --- a/drivers/cxl/core/mbox.c
>> +++ b/drivers/cxl/core/mbox.c
>> @@ -1484,23 +1484,20 @@ 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;
>>   	int rc;
>>   
>> -	mds = devm_kzalloc(dev, sizeof(*mds), GFP_KERNEL);
>> +	mds = cxl_dev_state_create(dev, CXL_DEVTYPE_CLASSMEM, serial, dvsec,
>> +				   struct cxl_memdev_state, cxlds, true);
> Existing cxl_memdev_state_create() callers expect that any state
> allocation is managed by devres.
>
> It is ok to make cxl_memdev_state_create() manually allocate, but then
> you still need to take care of existing caller expectations.


I'm surprised to see this. I think the expectation was the 
cxl_dev_state_create to be managed by devres, and somehow it ended up 
without it.


I think the best option here is to add it. I do not think it needs any 
special action but just the freeing of that memory.


>>   	if (!mds) {
>>   		dev_err(dev, "No memory available\n");
>>   		return ERR_PTR(-ENOMEM);
>>   	}
>>   
>>   	mutex_init(&mds->event.log_lock);
>> -	mds->cxlds.dev = dev;
>> -	mds->cxlds.reg_map.host = dev;
>> -	mds->cxlds.cxl_mbox.host = dev;
>> -	mds->cxlds.reg_map.resource = CXL_RESOURCE_NONE;
>> -	mds->cxlds.type = CXL_DEVTYPE_CLASSMEM;
>>   
>>   	rc = devm_cxl_register_mce_notifier(dev, &mds->mce_notifier);
> Ugh, but this function is now confused as some resources are devm, and
> some are manual alloc. this is a bit of a mess. Like why does this need
> to dev_warn() every boot on MCE-less archs like ARM64?
>
> I was trying to keep the incremental fixup small, but this makes it
> bigger, and likely means we need to clean this up before this patch.
>
> Ugh2, looks like this current arrangment will cause a NULL pointer
> de-reference if an MCE fires between cxl_memdev_state_create() and
> devm_cxl_add_memdev().
>
> Ugh3, looks like the MCE is registered once per memdev, but triggers
> memory_failure() once per spa match. That really wants to be registered
> once per-region.
>
> That whole situation needs a rethink, but for now make the other
> cleanups a TODO.


I can do those TODOs but I do not think that is relevant for the patch. 
I mean, you have discovered a problem there but this patch is not 
introducing the problem, AFAIK.


>
>>   	if (rc == -EOPNOTSUPP)
>> diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
>> index a16a5886d40a..6cc732aeb9de 100644
>> --- a/drivers/cxl/core/memdev.c
>> +++ b/drivers/cxl/core/memdev.c
>> @@ -633,6 +633,38 @@ 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,
>> +			bool has_mbox)
> As far as I can see this can be static as _cxl_dev_state_create() is the
> only caller in this whole series. Fixup included below:


Sure.


>> +{
>> +	*cxlds = (struct cxl_dev_state) {
>> +		.dev = dev,
>> +		.type = type,
>> +		.serial = serial,
>> +		.cxl_dvsec = dvsec,
>> +		.reg_map.host = dev,
>> +		.reg_map.resource = CXL_RESOURCE_NONE,
>> +	};
>> +
>> +	if (has_mbox)
>> +		cxlds->cxl_mbox.host = dev;
>> +}
>> +
>> +struct cxl_dev_state *_cxl_dev_state_create(struct device *dev,
>> +					    enum cxl_devtype type, u64 serial,
>> +					    u16 dvsec, size_t size,
>> +					    bool has_mbox)
>> +{
>> +	struct cxl_dev_state *cxlds __free(kfree) = kzalloc(size, GFP_KERNEL);
>> +
>> +	if (!cxlds)
>> +		return NULL;
>> +
>> +	cxl_dev_state_init(cxlds, dev, type, serial, dvsec, has_mbox);
>> +	return_ptr(cxlds);
> This function is so simple, there is no need to use scope-based cleanup.
>

OK. I'll do so.

Thanks!